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

[Edgar E. Iglesias]

On Sun, Jul 25, 2010 at 07:52:18AM +0200, Edgar E. Iglesias wrote:
> On Sun, Jul 25, 2010 at 06:44:41AM +0200, Aurelien Jarno 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.
> > >
> > 
> > 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):
> 
> Thanks,
> 
> That looks nice, specially the removing of the helper_interrupt_restart
> parts :)
> 
> I'll test it on my side and send you an ACK.


I've tested this with the same images as before, they still work.
I also created a synthetic testcase for the 2 software interrupt
lines and they seem to behave OK. Our linux port was not so
happy about seeing those raised though, so I had to hack a bit
to see them in action :)

Acked-by: Edgar E. Iglesias <address@hidden>
Tested-by: Edgar E. Iglesias <address@hidden>

Would you mind applying your patch on top of mine?

Thanks alot,
Edgar


> 
> Thanks for your patience Aurelien,
> Edgar
> 
> 
> > 
> > 
> > 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
> > address@hidden                 http://www.aurel32.net