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

[Peter Maydell]

On Thu, 11 Jun 2020 at 01:23, wentongw <wentong.wu@intel.com> wrote:
>
> 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:

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

(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