RE: [PATCH] hw/nios2: Update interrupt request when CR_STATUS_PIE disabled

[Wu, Wentong]

> >
> > Update interrupt request when external interupt pends for STATUS_PIE 
> > disabled. Otherwise on icount enabled nios2 target there will be cpu 
> > abort when guest code changes state register with wrctl instruction.
> >
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > ---
> > hw/nios2/cpu_pic.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/nios2/cpu_pic.c b/hw/nios2/cpu_pic.c index 
> > 1c1989d5..2abc8fa8 100644
> > --- a/hw/nios2/cpu_pic.c
> > +++ b/hw/nios2/cpu_pic.c
> > @@ -42,6 +42,8 @@ static void nios2_pic_cpu_handler(void *opaque, int irq, 
> > int level)
> >        } else if (!level) {
> >             env->irq_pending = 0;
> >             cpu_reset_interrupt(cs, type);
> > +        } else {
> > +            cs->interrupt_request |= type;
> >        }

> Thanks for the clarification in your other email about the issue you're 
> trying to address:

Thanks for the review!

> > I’m running icount mode on qemu_nios2 with customized platform but cpu 
> > abort happened(qemu: fatal: Raised interrupt while not in I/O 
> > function) when guest code changes state register with wrctl 
> > instruction add some debug code finding that it’s caused by the 
> > interrupt_request mismatch.

> I don't think the change you've made is the correct fix.
> Setting cs->interrupt_request like this is pretty much the same thing that 
> calling cpu_interrupt() does, so what your patch is doing is essentially 
> "ignore the status.PIE bit and always deliver interrupts", which isn't how 
> the hardware behaves.

> The assertion you've run into is saying "some instruction caused us to take 
> an interrupt, but it wasn't marked up to indicate that it might cause a side 
> effect". (This only matters in icount mode, where we insist that we never get 
> unexpected sideeffects like this.) If the guest writes to status.PIE to 
> unmask interrupts that's the kind of thing that will cause an interrupt to be 
> taken, so the problem really here is that the nios2 translate.c code hasn't 
> indicated that this insn can do this.

> The right fix here will be that target/nios2/translate.c needs to do this:
>   if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
>        gen_io_start();
>   }
> before generating code for an insn like this one, and then
>   if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
>        gen_io_end();
>   }
> after it. (Compare the xtensa target which does a similar kind of thing for 
> its interrupt handling.) For wrctl to STATUS it should I think also end the 
> TB, because we want to actually take any pending interrupt now, not in a few 
> instructions time when the next branch comes along.

> The fact that the current nios2 code has no calls to
> gen_io_start() in it at all (apart from one in boilerplate
> code) suggests to me that this target is simply broken for use with -icount 
> at the moment. There may well be other bugs of a similar kind where 
> particular insns that cause interrupts or touch devices (any equivalent to the
> x86 in/out insns, for instance) also need to be marked up as IO.


Thanks for the detail, you are right, understand this more. Thanks

> (Beyond that, the way that nios2_check_interrupts() works looks weird; in an 
> ideal world it would be rewritten to work in a way that's more in line with 
> how we'd write that kind of code today. It should be possible to get it to 
> work with icount without getting into that kind of refactoring/rework,
> though.)

> thanks
> -- PMM