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