Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount

[Aurelien Jarno]

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 <address@hidden>
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().

Based on a patch from Edgar E. Iglesias <address@hidden>

Signed-off-by: Aurelien Jarno <address@hidden>
---
 cpu-exec.c         |    6 +-----
 target-mips/cpu.h  |    8 ++++++++
 target-mips/exec.h |   18 +++++++++++++++---
 3 files changed, 24 insertions(+), 8 deletions(-)

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)
 {

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net