[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH] hw/nios2: Update interrupt request when CR_STATUS_PIE disabl
From: |
Wu, Wentong |
Subject: |
RE: [PATCH] hw/nios2: Update interrupt request when CR_STATUS_PIE disabled |
Date: |
Thu, 18 Jun 2020 03:08:38 +0000 |
> >
> > 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