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

[Aurelien Jarno <aurelien@aurel32.net>]

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.
>

If we choose having the interrupt controller as part of the CPU, which
seems to be what you have chosen, the following patch should fix the
remaining issues (and prepare the work for vector interrupt support):


diff --git a/hw/mips_int.c b/hw/mips_int.c
index 80488ba..477f6ab 100644
--- a/hw/mips_int.c
+++ b/hw/mips_int.c
@@ -54,3 +54,12 @@ void cpu_mips_irq_init_cpu(CPUState *env)
         env->irq[i] = qi[i];
     }
 }
+
+void cpu_mips_soft_irq(CPUState *env, int irq, int level)
+{
+    if (irq < 0 || irq > 2) {
+        return;
+    }
+
+    qemu_set_irq(env->irq[irq], level);
+}
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 1578850..b8e6fee 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -597,6 +597,9 @@ void cpu_mips_store_compare (CPUState *env, uint32_t value);
 void cpu_mips_start_count(CPUState *env);
 void cpu_mips_stop_count(CPUState *env);
 
+/* mips_int.c */
+void cpu_mips_soft_irq(CPUState *env, int irq, int level);
+
 /* helper.c */
 int cpu_mips_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
                                int mmu_idx, int is_softmmu);
diff --git a/target-mips/helper.h b/target-mips/helper.h
index a6ba75d..cb13fb2 100644
--- a/target-mips/helper.h
+++ b/target-mips/helper.h
@@ -2,7 +2,6 @@
 
 DEF_HELPER_2(raise_exception_err, void, i32, int)
 DEF_HELPER_1(raise_exception, void, i32)
-DEF_HELPER_0(interrupt_restart, void)
 
 #ifdef TARGET_MIPS64
 DEF_HELPER_3(ldl, tl, tl, tl, int)
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index c963224..8482e69 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -46,18 +46,6 @@ void helper_raise_exception (uint32_t exception)
     helper_raise_exception_err(exception, 0);
 }
 
-void helper_interrupt_restart (void)
-{
-    if (!(env->CP0_Status & (1 << CP0St_EXL)) &&
-        !(env->CP0_Status & (1 << CP0St_ERL)) &&
-        !(env->hflags & MIPS_HFLAG_DM) &&
-        (env->CP0_Status & (1 << CP0St_IE)) &&
-        (env->CP0_Status & env->CP0_Cause & CP0Ca_IP_mask)) {
-        env->CP0_Cause &= ~(0x1f << CP0Ca_EC);
-        helper_raise_exception(EXCP_EXT_INTERRUPT);
-    }
-}
-
 #if !defined(CONFIG_USER_ONLY)
 static void do_restore_state (void *pc_ptr)
 {
@@ -1346,6 +1334,7 @@ void helper_mtc0_cause (target_ulong arg1)
 {
     uint32_t mask = 0x00C00300;
     uint32_t old = env->CP0_Cause;
+    int i;
 
     if (env->insn_flags & ISA_MIPS32R2)
         mask |= 1 << CP0Ca_DC;
@@ -1358,6 +1347,13 @@ void helper_mtc0_cause (target_ulong arg1)
         else
             cpu_mips_start_count(env);
     }
+
+    /* Set/reset software interrupts */
+    for (i = 0 ; i < 2 ; i++) {
+        if ((old ^ env->CP0_Cause) & (1 << (CP0Ca_IP + i))) {
+            cpu_mips_soft_irq(env, i, env->CP0_Cause & (1 << (CP0Ca_IP + i)));
+        }
+    }
 }
 
 void helper_mtc0_ebase (target_ulong arg1)
diff --git a/target-mips/translate.c b/target-mips/translate.c
index 7168273..6c72dee 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -5190,7 +5190,17 @@ static void gen_dmtc0 (CPUState *env, DisasContext *ctx, TCGv arg, int reg, int
         switch (sel) {
         case 0:
             save_cpu_state(ctx, 1);
+            /* Mark as an IO operation because we may trigger a software
+               interrupt.  */
+            if (use_icount) {
+                gen_io_start();
+            }
             gen_helper_mtc0_cause(arg);
+            if (use_icount) {
+                gen_io_end();
+            }
+            /* Stop translation as we may have triggered an intetrupt */
+            ctx->bstate = BS_STOP;
             rn = "Cause";
             break;
         default:
@@ -12365,7 +12375,6 @@ gen_intermediate_code_internal (CPUState *env, TranslationBlock *tb,
     } else {
         switch (ctx.bstate) {
         case BS_STOP:
-            gen_helper_interrupt_restart();
             gen_goto_tb(&ctx, 0, ctx.pc);
             break;
         case BS_NONE:
@@ -12373,7 +12382,6 @@ gen_intermediate_code_internal (CPUState *env, TranslationBlock *tb,
             gen_goto_tb(&ctx, 0, ctx.pc);
             break;
         case BS_EXCP:
-            gen_helper_interrupt_restart();
             tcg_gen_exit_tb(0);
             break;
         case BS_BRANCH:

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