| Message ID | 20101225222214.GA4464@volta.aurel32.net |
|---|---|
| State | New |
| Headers | show |
On Sat, Dec 25, 2010 at 11:22:14PM +0100, Aurelien Jarno wrote: > Hi, > > On Wed, Dec 22, 2010 at 05:12:39PM +0100, Edgar E. Iglesias wrote: > > Hi, I don't see this problem with the qemu.org test images and neither > > with my boards/images. I see QEMU basically not running at all when > > the guest is idle. Do you have more info on how to reproduce it? > > I am seeing the problem with the MIPS malta board and the images from: > > http://people.debian.org/~aurel32/qemu/mips/ > > > If the CPU hw interrupt line is asserted it means some device is > > signaling interrupts. Maybe we are modling the wake up filter > > wrongly in target-mips/exec.h, maybe a real MIPS doesn't wakeup from > > sleep unless the irq passes the CPUs internal masking? The manuals > > are not really clear on this. I'm currently travelling and have > > no access to check with a real MIPS hw. > > According the manual I have checked, it is implementation dependent if > the CPU exits from the WAIT instruction when a non-enabled interrupt is > triggered. However for the few implementations I have checked (4k, 5k, > 34k), the CPU only wakes-up if the interrupt can be taken. > > > If your hw interrupt line is active all the time it sounds to me > > like if something is also wrong with either the guest software or a > > device model. > > The corresponding interrupt line is the timer one. It seems the kernel > sometimes choose to ignore the timer instead of stopping it. I am only > able to reproduce that with a dyntick enabled kernel. > > > I think the following patch should restore the previous wait for > > interrupt wakeup behaviour to let the MIPS sleep until an irq passes > > the internal masking (but I'm not sure this is how real MIPS does it): > > It does, thanks a lot. > > However, according to the manual I think we should also check if > interrupts are enabled (if they are disabled, an interrupt can't be > taken). I therefore propose the following patch: > > From 9c9e5f7ee1e897e408b1cd9f4c42ddf86c30aabe Mon Sep 17 00:00:00 2001 > From: Aurelien Jarno <aurelien@aurel32.net> > Date: Sat, 25 Dec 2010 22:56:32 +0100 > Subject: [PATCH] target-mips: fix host CPU consumption when guest is idle > > When the CPU is in wait state, do not wake-up if an interrupt can't be > taken. This avoid host CPU running at 100% if a device (e.g. timer) has > an interrupt line left enabled. > > Also factorize code to check if interrupts are enabled in > cpu_mips_hw_interrupts_pending(). Thanks Aurelien, It looks good, but one thing that worries me slightly is that streching the wakeup filter to include the IE related flags might break using the wait insn in polling mode. for example: di(); init_hw(); while (1) { wait_for_interrupt(); /* Power Save. */ do_work(); }; I've seem similar constructions in bootcode/firmware for other archs. In this case I guess it would be using undefined behaviour on the mips though, so I'm OK with either patch. At some point I'll see if I can check the IE flags behaviour with a real 34k and we can finetune the models with follow-up patches if needed. Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com> Cheers
On Sun, Dec 26, 2010 at 09:34:20AM +0100, Edgar E. Iglesias wrote: > On Sat, Dec 25, 2010 at 11:22:14PM +0100, Aurelien Jarno wrote: > > Hi, > > > > On Wed, Dec 22, 2010 at 05:12:39PM +0100, Edgar E. Iglesias wrote: > > > Hi, I don't see this problem with the qemu.org test images and neither > > > with my boards/images. I see QEMU basically not running at all when > > > the guest is idle. Do you have more info on how to reproduce it? > > > > I am seeing the problem with the MIPS malta board and the images from: > > > > http://people.debian.org/~aurel32/qemu/mips/ > > > > > If the CPU hw interrupt line is asserted it means some device is > > > signaling interrupts. Maybe we are modling the wake up filter > > > wrongly in target-mips/exec.h, maybe a real MIPS doesn't wakeup from > > > sleep unless the irq passes the CPUs internal masking? The manuals > > > are not really clear on this. I'm currently travelling and have > > > no access to check with a real MIPS hw. > > > > According the manual I have checked, it is implementation dependent if > > the CPU exits from the WAIT instruction when a non-enabled interrupt is > > triggered. However for the few implementations I have checked (4k, 5k, > > 34k), the CPU only wakes-up if the interrupt can be taken. > > > > > If your hw interrupt line is active all the time it sounds to me > > > like if something is also wrong with either the guest software or a > > > device model. > > > > The corresponding interrupt line is the timer one. It seems the kernel > > sometimes choose to ignore the timer instead of stopping it. I am only > > able to reproduce that with a dyntick enabled kernel. > > > > > I think the following patch should restore the previous wait for > > > interrupt wakeup behaviour to let the MIPS sleep until an irq passes > > > the internal masking (but I'm not sure this is how real MIPS does it): > > > > It does, thanks a lot. > > > > However, according to the manual I think we should also check if > > interrupts are enabled (if they are disabled, an interrupt can't be > > taken). I therefore propose the following patch: > > > > From 9c9e5f7ee1e897e408b1cd9f4c42ddf86c30aabe Mon Sep 17 00:00:00 2001 > > From: Aurelien Jarno <aurelien@aurel32.net> > > Date: Sat, 25 Dec 2010 22:56:32 +0100 > > Subject: [PATCH] target-mips: fix host CPU consumption when guest is idle > > > > When the CPU is in wait state, do not wake-up if an interrupt can't be > > taken. This avoid host CPU running at 100% if a device (e.g. timer) has > > an interrupt line left enabled. > > > > Also factorize code to check if interrupts are enabled in > > cpu_mips_hw_interrupts_pending(). > > Thanks Aurelien, > > It looks good, but one thing that worries me slightly is that streching > the wakeup filter to include the IE related flags might break using the > wait insn in polling mode. > > for example: > > di(); > init_hw(); > while (1) { > wait_for_interrupt(); /* Power Save. */ > do_work(); > }; > > I've seem similar constructions in bootcode/firmware for other archs. > In this case I guess it would be using undefined behaviour on the mips > though, so I'm OK with either patch. Yes, the manuals are not fully clear, however it seems to be a possible behaviour for some implementations, also it was the behaviour prior to your patch. Note also that in the case above, the CPU can still be woken-up by an NMI, though it doesn't seem to be implemented in QEMU yet. > At some point I'll see if I can check the IE flags behaviour with a real > 34k and we can finetune the models with follow-up patches if needed. Ok, that would be nice if you can do it. > Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com> > Thanks for your review. Cheers, Aurélien
diff --git a/cpu-exec.c b/cpu-exec.c index 39e5eea..8c9fb8b 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -454,11 +454,7 @@ int cpu_exec(CPUState *env1) } #elif defined(TARGET_MIPS) if ((interrupt_request & CPU_INTERRUPT_HARD) && - cpu_mips_hw_interrupts_pending(env) && - (env->CP0_Status & (1 << CP0St_IE)) && - !(env->CP0_Status & (1 << CP0St_EXL)) && - !(env->CP0_Status & (1 << CP0St_ERL)) && - !(env->hflags & MIPS_HFLAG_DM)) { + cpu_mips_hw_interrupts_pending(env)) { /* Raise it */ env->exception_index = EXCP_EXT_INTERRUPT; env->error_code = 0; diff --git a/target-mips/cpu.h b/target-mips/cpu.h index c1f211f..2419aa9 100644 --- a/target-mips/cpu.h +++ b/target-mips/cpu.h @@ -532,6 +532,14 @@ static inline int cpu_mips_hw_interrupts_pending(CPUState *env) int32_t status; int r; + if (!(env->CP0_Status & (1 << CP0St_IE)) || + (env->CP0_Status & (1 << CP0St_EXL)) || + (env->CP0_Status & (1 << CP0St_ERL)) || + (env->hflags & MIPS_HFLAG_DM)) { + /* Interrupts are disabled */ + return 0; + } + pending = env->CP0_Cause & CP0Ca_IP_mask; status = env->CP0_Status & CP0Ca_IP_mask; diff --git a/target-mips/exec.h b/target-mips/exec.h index af61b54..1273654 100644 --- a/target-mips/exec.h +++ b/target-mips/exec.h @@ -19,10 +19,22 @@ register struct CPUMIPSState *env asm(AREG0); static inline int cpu_has_work(CPUState *env) { - return (env->interrupt_request & - (CPU_INTERRUPT_HARD | CPU_INTERRUPT_TIMER)); -} + int has_work = 0; + + /* It is implementation dependent if non-enabled interrupts + wake-up the CPU, however most of the implementations only + check for interrupts that can be taken. */ + if ((env->interrupt_request & CPU_INTERRUPT_HARD) && + cpu_mips_hw_interrupts_pending(env)) { + has_work = 1; + } + if (env->interrupt_request & CPU_INTERRUPT_TIMER) { + has_work = 1; + } + + return has_work; +} static inline int cpu_halted(CPUState *env) {