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

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

[wentongw]

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;
         }
     } else {
         if (level) {
-- 
2.21.3

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

[Wu, Wentong]

Hi,
Can any body help review this patch ? Thanks in advance!

BR

-----Original Message-----
From: Wu, Wentong <wentong.wu@intel.com> 
Sent: Thursday, June 11, 2020 4:13 PM
To: qemu-devel@nongnu.org
Cc: qemu-trivial@nongnu.org; crwulff@gmail.com; marex@denx.de; thuth@redhat.com; Wu, Wentong <wentong.wu@intel.com>
Subject: [PATCH] hw/nios2: Update interrupt request when CR_STATUS_PIE disabled

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;
         }
     } else {
         if (level) {
--
2.21.3

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

[Philippe Mathieu-Daudé]

Hi,

On 6/12/20 3:43 PM, Wu, Wentong wrote:
> Hi,
> Can any body help review this patch ? Thanks in advance!

You just sent this patch yesterday... Please give reviewers more time.

See:
https://wiki.qemu.org/Contribute/SubmitAPatch#Participating_in_Code_Review
In particular:
https://wiki.qemu.org/Contribute/SubmitAPatch#If_your_patch_seems_to_have_been_ignored

> 
> BR
> 
> -----Original Message-----
> From: Wu, Wentong <wentong.wu@intel.com> 
> Sent: Thursday, June 11, 2020 4:13 PM
> To: qemu-devel@nongnu.org
> Cc: qemu-trivial@nongnu.org; crwulff@gmail.com; marex@denx.de; thuth@redhat.com; Wu, Wentong <wentong.wu@intel.com>
> Subject: [PATCH] hw/nios2: Update interrupt request when CR_STATUS_PIE disabled
> 
> 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.

It'd help if you provide more information, what code where you testing,
how you ran QEMU, enough for reviewers to reproduce the issue you had
and test if your patch indeed resolves the issue you described.

Regards,

Phil.

> 
> 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;
>          }
>      } else {
>          if (level) {
> --
> 2.21.3
> 
> 

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

[Wu, Wentong]

>Hi,

>On 6/12/20 3:43 PM, Wu, Wentong wrote:
> > Hi,
> >Can any body help review this patch ? Thanks in advance!

> You just sent this patch yesterday... Please give reviewers more time.

> See:
> https://wiki.qemu.org/Contribute/SubmitAPatch#Participating_in_Code_Review
> In particular:
> https://wiki.qemu.org/Contribute/SubmitAPatch#If_your_patch_seems_to_have_been_ignored

> > 
> > BR
> > 
> > -----Original Message-----
> > From: Wu, Wentong <wentong.wu@intel.com>
> > Sent: Thursday, June 11, 2020 4:13 PM
> > To: qemu-devel@nongnu.org
> > Cc: qemu-trivial@nongnu.org; crwulff@gmail.com; marex@denx.de; 
> > thuth@redhat.com; Wu, Wentong <wentong.wu@intel.com>
> > Subject: [PATCH] hw/nios2: Update interrupt request when CR_STATUS_PIE 
> > disabled
> >
> > 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.

> It'd help if you provide more information, what code where you testing, how you ran QEMU, enough for reviewers to reproduce the issue you had and test if your patch indeed resolves the issue you described.

Hi,
I’m running icount mode on qemu_nios2 with customized platform(https://github.com/zephyrproject-rtos/sdk-ng/blob/master/meta-zephyr-sdk/recipes-devtools/qemu/files/0001-qemu-nios2-Add-Altera-MAX-10-board-support-for-Zephy.patch, almost same with 10m50_devboard) 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. Hope that will be helpful.


> Regards,

> Phil.

> > 
> > 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;
> >        }
> >     } else {
> >        if (level) {
> > --
> > 2.21.3
> >
> >

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

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