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

[Edgar E. Iglesias]

On Tue, Dec 21, 2010 at 11:28:43PM +0100, Aurelien Jarno wrote:
> On Sun, Jul 25, 2010 at 07:46:49AM +0200, Edgar E. Iglesias wrote:
> > On Sun, Jul 25, 2010 at 05:08:07AM +0200, Aurelien Jarno wrote:
> > > On Sun, Jul 25, 2010 at 02:07:54AM +0200, Edgar E. Iglesias wrote:
> > > > On Sun, Jul 25, 2010 at 12:55:45AM +0200, Aurelien Jarno wrote:
> > > > > On Thu, Jul 22, 2010 at 01:32:18PM +0200, Edgar E. Iglesias wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > I'm seeing an error when emulating MIPS guests with -icount.
> > > > > > In cpu_interrupt:
> > > > > >   cpu_abort(env, "Raised interrupt while not in I/O function");
> > > > > 
> > > > > I am not able to reproduce the issue. Do you have more details how to
> > > > > reproduce the problem?
> > > > 
> > > > You need a machine with devices that raise the hw interrupts. I didn't
> > > > see the error on the images on the wiki though. But I've got a machine
> > > > here that trigs it easily. Will check if I can publish it and an image.
> > > > 
> > > 
> > > That would be nice if you can share it.
> > > 
> > > > > > It seems to me like the MIPS interrupt glue logic between interrupt
> > > > > > controllers and the MIPS core is not modeled correctly.
> > > > > 
> > > > > It seems indeed that sometimes interrupt are triggered while not in 
> > > > > I/O functions, your patch addresses part of the problem.
> > > > > 
> > > > > > When hw interrupt pending bits in CP0_Cause are set, the CPU should
> > > > > > see the hw interrupt line as active. The CPU may or may not take the
> > > > > > interrupt based on internal state (global irq mask etc) but the glue
> > > > > > logic shouldn't care about that. Am I missing something here?
> > > > > 
> > > > > I don't think it is correct. On the real hardware, the interrupt line 
> > > > > is
> > > > > actually active only when all conditions are fulfilled.
> > > > > 
> > > > > The thing to remember is that the interrupts are level triggered. So 
> > > > > if
> > > > > an interrupt is masked, it should be rejected by the CPU, but could be
> > > > > triggered again as soon as the interrupt mask is changed.
> > > > 
> > > > Agreed, that behaviour is what I'm trying to acheive. The problem now
> > > > is that the the level triggered line, prior to CPU masking is beeing 
> > > > masked
> > > > by external logic. IMO it shouldnt. See hw/mips_int.c and cpu-exec.c 
> > > > prior
> > > > to the patch.
> > > 
> > > Actually all depends if you consider the MIPS interrupt controller part
> > > of the CPU or not. It could be entirely modeled in the CPU, that is in 
> > > cpu-exec.c or entirely modeled as a separate controller, that is in 
> > > mips_int.c.
> > > 
> > > IMHO it should be in mips_int.c. It is an interrupt controller like
> > > another that combines a few interrupt lines into a single one that feeds
> > > the CPU. It is like for example the i8259, with the exception that the 
> > > configuration is not done by load/store into MMIO area, but directly 
> > > using CPU special registers. We should probably mark these instructions 
> > > as I/O.
> > 
> > 
> > Hi,
> > 
> > I agree that it's not obvious where things should be modeled, I'll try to
> > explain my view.
> > 
> > As a first step I'm trying to model a MIPS configured with Vectored
> > Interrupts. We've got external interrupt logic feeding the hw
> > interrupt lines. These lines are level triggered, held active by
> > the external logic as long as interrupts are pending. Regardless
> > of wether the CPU want's to take the interrupt now or later. In fact,
> > there is no way to access the internal flags from RTL logic located
> > here (AFAIK). In my mind, this layers pretty much ends in hw/mips_int.c.
> > 
> > Internally in the MIPS core, I'm guessing there is logic that simpliy
> > applies the internal CPU masks, outputing a single internal IRQ line
> > that decides wether the CPU should take the IRQ or not. Here, things like
> > IE flags etc matter. I don't have access to RTL on the MIPS side so I'm
> > just guessing here.
> > 
> > In my mind, we should model this latter part by asserting INTERRUPT_HARD
> > from hw/mips_int.c whenever any hw lines are active and letting the
> > CPU in cpu-exec.c decide when to take the interrupt by applying it's
> > internal masking.
> > 
> 
> Sorry to come back so long after this discussion, but I now have another
> argument. This commit causes a regression, the host CPU is now always at
> 100%. QEMU spent all its time looping because the CPU interrupt line is
> asserted.
> 
> Not asserting the CPU interrupt line when interrupts are disabled fixes
> the issue.


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?

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.

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.

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

diff --git a/target-mips/exec.h b/target-mips/exec.h
index af61b54..8cd5178 100644
--- a/target-mips/exec.h
+++ b/target-mips/exec.h
@@ -19,10 +19,19 @@ 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;
 
+    /* MIPS wakes up when hw interrupts pass the internal masking?  */
+    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)
 {