Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]

> >On Mon, 29 Jun 2020 at 09:17, Wentong Wu <wentong.wu@intel.com> wrote:
> >
> > wrctl instruction on nios2 target will cause checking cpu
> > interrupt but tcg_handle_interrupt() will call cpu_abort()
> > if the CPU gets an interrupt while it's not in 'can do IO'
> > state, so add gen_io_start around wrctl instruction. Also
> > at the same time, end the onging TB with DISAS_UPDATE.
> >
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > ---
> >  target/nios2/translate.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> > index 83c10eb2..51347ada 100644
> > --- a/target/nios2/translate.c
> > +++ b/target/nios2/translate.c
> > @@ -32,6 +32,7 @@
> >  #include "exec/cpu_ldst.h"
> >  #include "exec/translator.h"
> >  #include "qemu/qemu-print.h"
> > +#include "exec/gen-icount.h"
> >
> >  /* is_jmp field values */
> >  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> > @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code,
> > uint32_t flags)
> >      /* If interrupts were enabled using WRCTL, trigger them. */
> >  #if !defined(CONFIG_USER_ONLY)
> >      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> > +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> > +            gen_io_start();
> > +        }
> >          gen_helper_check_interrupts(dc->cpu_env);
> > +        dc->is_jmp = DISAS_UPDATE;
> >      }
> >  #endif
> >  }
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Hi Peter,

Will this be merged? If not, I would like to follow any suggestions, thanks

Thanks

>
> though as Richard notes ideally the interrupt handling code here should
> be rewritten because the check_interrupts helper is a very weird way
> to implement things.
>
> thanks
> -- PMM


[-- Attachment #2: Type: text/html, Size: 11574 bytes --]

RE: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

Hi,
I think we can get this patch series merged first in order to get qemu_nios2 working with icount, actually we are blocked by it for some time. 
BTW if maintainers(Chris Wulff and Marek Vasut) don't have time for the re-work, I'd like to take it.

Thanks
> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org> 
> Sent: Monday, July 6, 2020 1:10 AM
> To: Wu, Wentong <wentong.wu@intel.com>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial <qemu-trivial@nongnu.org>; Chris Wulff <crwulff@gmail.com>; Marek Vasut <marex@denx.de>
> Subject: Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction
>
> On Mon, 29 Jun 2020 at 09:17, Wentong Wu <wentong.wu@intel.com> wrote:
> >
> > wrctl instruction on nios2 target will cause checking cpu interrupt 
> > but tcg_handle_interrupt() will call cpu_abort() if the CPU gets an 
> > interrupt while it's not in 'can do IO'
> > state, so add gen_io_start around wrctl instruction. Also at the same 
> > time, end the onging TB with DISAS_UPDATE.
> >
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > ---
> >  target/nios2/translate.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 
> > 83c10eb2..51347ada 100644
> > --- a/target/nios2/translate.c
> > +++ b/target/nios2/translate.c
> > @@ -32,6 +32,7 @@
> >  #include "exec/cpu_ldst.h"
> >  #include "exec/translator.h"
> >  #include "qemu/qemu-print.h"
> > +#include "exec/gen-icount.h"
> >
> >  /* is_jmp field values */
> >  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> > @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
> >      /* If interrupts were enabled using WRCTL, trigger them. */  #if 
> > !defined(CONFIG_USER_ONLY)
> >      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> > +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> > +            gen_io_start();
> > +        }
> >          gen_helper_check_interrupts(dc->cpu_env);
> > +        dc->is_jmp = DISAS_UPDATE;
> >      }
> >  #endif
> >  }
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> though as Richard notes ideally the interrupt handling code here should be rewritten because the check_interrupts helper is a very weird way to implement things.
>
> thanks
> -- PMM

Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Peter Maydell]

On Sun, 5 Jul 2020 at 21:54, Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> On Sun, Jul 5, 2020 at 11:16 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
> > On Sun, Jul 5, 2020 at 10:09 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > On Thu, 2 Jul 2020 at 19:53, Richard Henderson
> > > <richard.henderson@linaro.org> wrote:
> > > > This isn't right.  Not so much the gen_io_start portion, but the entire
> > > > existence of helper_check_interrupt.
> > > I agree that it looks bogus (xtensa has a similar helper as well, incidentally),
> > I think there was a reason for it.
>
> ...and the reason is that this helper calls cpu_[re]set_interrupt
> to update CPU_INTERRUPT_HARD, which makes exit to the
> main CPU loop do something to handle IRQ.
> Maybe 'check_interrupt' is not a good name for that, but the
> action taken there seems right to me.

Usually I would expect that CPU_INTERRUPT_HARD would be
set by whatever was setting the interrupt (often a handler
for an inbound qemu_irq line to the CPU). Then the
cpu_exec_interrupt hook only actually does something if
both the INTERRUPT_HARD bit is set and the CPU register
says "and interrupts are unmasked". If you have a design
like that then "unmasking interrupts just means going out
to the main loop" will work.

thanks
-- PMM

RE: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

Maybe below patch will reduce some overhead, because currently it will exit to main loop to handle interrupt but if with (env->regs[CR_STATUS] & CR_STATUS_PIE) = False, it does nothing except set env->irq_pending again.

diff --git a/hw/nios2/cpu_pic.c b/hw/nios2/cpu_pic.c
index 1c1989d5..5ea7e52a 100644
--- a/hw/nios2/cpu_pic.c
+++ b/hw/nios2/cpu_pic.c
@@ -54,7 +54,8 @@ static void nios2_pic_cpu_handler(void *opaque, int irq, int level)

 void nios2_check_interrupts(CPUNios2State *env)
 {
-    if (env->irq_pending) {
+    if (env->irq_pending &&
+        (env->regs[CR_STATUS] & CR_STATUS_PIE)) {
         env->irq_pending = 0;
         cpu_interrupt(env_cpu(env), CPU_INTERRUPT_HARD);
     }

-----Original Message-----
From: Richard Henderson <richard.henderson@linaro.org> 
Sent: Friday, July 3, 2020 2:54 AM
To: Wu, Wentong <wentong.wu@intel.com>; qemu-devel@nongnu.org
Cc: qemu-trivial@nongnu.org; marex@denx.de; crwulff@gmail.com; peter.maydell@linaro.org
Subject: Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

On 6/29/20 9:05 AM, Wentong Wu wrote:
> wrctl instruction on nios2 target will cause checking cpu interrupt 
> but tcg_handle_interrupt() will call cpu_abort() if the CPU gets an 
> interrupt while it's not in 'can do IO'
> state, so add gen_io_start around wrctl instruction. Also at the same 
> time, end the onging TB with DISAS_UPDATE.
> 
> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> ---
>  target/nios2/translate.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 
> 83c10eb2..51347ada 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -32,6 +32,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "exec/translator.h"
>  #include "qemu/qemu-print.h"
> +#include "exec/gen-icount.h"
>  
>  /* is_jmp field values */
>  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
>      /* If interrupts were enabled using WRCTL, trigger them. */  #if 
> !defined(CONFIG_USER_ONLY)
>      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> +            gen_io_start();
> +        }
>          gen_helper_check_interrupts(dc->cpu_env);
> +        dc->is_jmp = DISAS_UPDATE;
>      }
>  #endif

This isn't right.  Not so much the gen_io_start portion, but the entire existence of helper_check_interrupt.

The correct way to acknowledge interrupts after changing an interrupt mask bit is to exit the TB back to the cpu main loop.
Which you are doing here with DISAS_UPDATE, so that part is fine.  (Although you could merge that into the switch statement above.)

Looking at nios_pic_cpu_handler, there are two other bugs:

1) Get rid of env->irq_pending and use cpu_interrupt/cpu_reset_interrupt instead.

2) Do not check env->regs[CR_STATUS] & CR_STATUS_PIE.  That variable does not belong to the pic and should not be checked there.  The check belongs in nios2_cpu_exec_interrupt, and is in fact already there.


r~

RE: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

Thanks, I think we can get this series merged currently.

-----Original Message-----
From: Peter Maydell <peter.maydell@linaro.org> 
Sent: Monday, July 6, 2020 1:10 AM
To: Wu, Wentong <wentong.wu@intel.com>
Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial <qemu-trivial@nongnu.org>; Chris Wulff <crwulff@gmail.com>; Marek Vasut <marex@denx.de>
Subject: Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

On Mon, 29 Jun 2020 at 09:17, Wentong Wu <wentong.wu@intel.com> wrote:
>
> wrctl instruction on nios2 target will cause checking cpu interrupt 
> but tcg_handle_interrupt() will call cpu_abort() if the CPU gets an 
> interrupt while it's not in 'can do IO'
> state, so add gen_io_start around wrctl instruction. Also at the same 
> time, end the onging TB with DISAS_UPDATE.
>
> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> ---
>  target/nios2/translate.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 
> 83c10eb2..51347ada 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -32,6 +32,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "exec/translator.h"
>  #include "qemu/qemu-print.h"
> +#include "exec/gen-icount.h"
>
>  /* is_jmp field values */
>  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
>      /* If interrupts were enabled using WRCTL, trigger them. */  #if 
> !defined(CONFIG_USER_ONLY)
>      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> +            gen_io_start();
> +        }
>          gen_helper_check_interrupts(dc->cpu_env);
> +        dc->is_jmp = DISAS_UPDATE;
>      }
>  #endif
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

though as Richard notes ideally the interrupt handling code here should be rewritten because the check_interrupts helper is a very weird way to implement things.

thanks
-- PMM

Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Max Filippov]

On Sun, Jul 5, 2020 at 11:16 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
> On Sun, Jul 5, 2020 at 10:09 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > On Thu, 2 Jul 2020 at 19:53, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> > > This isn't right.  Not so much the gen_io_start portion, but the entire
> > > existence of helper_check_interrupt.
> > I agree that it looks bogus (xtensa has a similar helper as well, incidentally),
> I think there was a reason for it.

...and the reason is that this helper calls cpu_[re]set_interrupt
to update CPU_INTERRUPT_HARD, which makes exit to the
main CPU loop do something to handle IRQ.
Maybe 'check_interrupt' is not a good name for that, but the
action taken there seems right to me.

-- 
Thanks.
-- Max

Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Max Filippov]

On Sun, Jul 5, 2020 at 10:09 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> On Thu, 2 Jul 2020 at 19:53, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > This isn't right.  Not so much the gen_io_start portion, but the entire
> > existence of helper_check_interrupt.
>
> I agree that it looks bogus (xtensa has a similar helper as well, incidentally),

I think there was a reason for it. According to Richard

> The correct way to acknowledge interrupts after changing an interrupt mask bit
> is to exit the TB back to the cpu main loop.

But if I do this change for Xtensa I get a bunch of test_interrupt failures
that indicate that the interrupt that should have been taken wasn't taken.
FTR here's the change that I tested, did I miss something?

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index fdf47642e6f1..85e8d65f169d 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -631,18 +631,10 @@ static int gen_postprocess(DisasContext *dc, int slot)
 {
     uint32_t op_flags = dc->op_flags;

-#ifndef CONFIG_USER_ONLY
-    if (op_flags & XTENSA_OP_CHECK_INTERRUPTS) {
-        if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-            gen_io_start();
-        }
-        gen_helper_check_interrupts(cpu_env);
-    }
-#endif
     if (op_flags & XTENSA_OP_SYNC_REGISTER_WINDOW) {
         gen_helper_sync_windowbase(cpu_env);
     }
-    if (op_flags & XTENSA_OP_EXIT_TB_M1) {
+    if (op_flags & (XTENSA_OP_EXIT_TB_M1 | XTENSA_OP_CHECK_INTERRUPTS)) {
         slot = -1;
     }
     return slot;
@@ -1175,7 +1167,7 @@ static void disas_xtensa_insn(CPUXtensaState
*env, DisasContext *dc)
     if (dc->base.is_jmp == DISAS_NEXT) {
         gen_postprocess(dc, 0);
         dc->op_flags = 0;
-        if (op_flags & XTENSA_OP_EXIT_TB_M1) {
+        if (op_flags & (XTENSA_OP_EXIT_TB_M1 | XTENSA_OP_CHECK_INTERRUPTS)) {
             /* Change in mmu index, memory mapping or tb->flags; exit tb */
             gen_jumpi_check_loop_end(dc, -1);
         } else if (op_flags & XTENSA_OP_EXIT_TB_0) {


-- 
Thanks.
-- Max

Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Peter Maydell]

On Mon, 29 Jun 2020 at 09:17, Wentong Wu <wentong.wu@intel.com> wrote:
>
> wrctl instruction on nios2 target will cause checking cpu
> interrupt but tcg_handle_interrupt() will call cpu_abort()
> if the CPU gets an interrupt while it's not in 'can do IO'
> state, so add gen_io_start around wrctl instruction. Also
> at the same time, end the onging TB with DISAS_UPDATE.
>
> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> ---
>  target/nios2/translate.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> index 83c10eb2..51347ada 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -32,6 +32,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "exec/translator.h"
>  #include "qemu/qemu-print.h"
> +#include "exec/gen-icount.h"
>
>  /* is_jmp field values */
>  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
>      /* If interrupts were enabled using WRCTL, trigger them. */
>  #if !defined(CONFIG_USER_ONLY)
>      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> +            gen_io_start();
> +        }
>          gen_helper_check_interrupts(dc->cpu_env);
> +        dc->is_jmp = DISAS_UPDATE;
>      }
>  #endif
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

though as Richard notes ideally the interrupt handling code here should
be rewritten because the check_interrupts helper is a very weird way
to implement things.

thanks
-- PMM

Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Peter Maydell]

On Thu, 2 Jul 2020 at 19:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
> This isn't right.  Not so much the gen_io_start portion, but the entire
> existence of helper_check_interrupt.

I agree that it looks bogus (xtensa has a similar helper as well, incidentally),
but fixing all that stuff up is more effort than the relatively small job
of getting the icount handling right for the code we have today, which
is why I suggested to Wentong that we could just do this bit for now.

thanks
-- PMM

RE: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

Correct the format

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org> 
> Sent: Friday, July 3, 2020 2:54 AM
> To: Wu, Wentong <wentong.wu@intel.com>; qemu-devel@nongnu.org
> Cc: qemu-trivial@nongnu.org; marex@denx.de; crwulff@gmail.com; peter.maydell@linaro.org
> Subject: Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction
> 
> On 6/29/20 9:05 AM, Wentong Wu wrote:
> > wrctl instruction on nios2 target will cause checking cpu interrupt 
> > but tcg_handle_interrupt() will call cpu_abort() if the CPU gets an 
> > interrupt while it's not in 'can do IO'
> > state, so add gen_io_start around wrctl instruction. Also at the same 
> > time, end the onging TB with DISAS_UPDATE.
> > 
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > ---
> >  target/nios2/translate.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 
> > 83c10eb2..51347ada 100644
> > --- a/target/nios2/translate.c
> > +++ b/target/nios2/translate.c
> > @@ -32,6 +32,7 @@
> >  #include "exec/cpu_ldst.h"
> >  #include "exec/translator.h"
> >  #include "qemu/qemu-print.h"
> > +#include "exec/gen-icount.h"
> >  
> >  /* is_jmp field values */
> >  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> > @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
> >      /* If interrupts were enabled using WRCTL, trigger them. */  #if 
> > !defined(CONFIG_USER_ONLY)
> >      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> > +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> > +            gen_io_start();
> > +        }
> >          gen_helper_check_interrupts(dc->cpu_env);
> > +        dc->is_jmp = DISAS_UPDATE;
> >      }
> >  #endif
> 
> This isn't right.  Not so much the gen_io_start portion, but the entire existence of helper_check_interrupt.
> 
> The correct way to acknowledge interrupts after changing an interrupt mask bit is to exit the TB back to the cpu main loop.
> Which you are doing here with DISAS_UPDATE, so that part is fine.  (Although you could merge that into the switch statement above.)
> 
> Looking at nios_pic_cpu_handler, there are two other bugs:
> 
> 1) Get rid of env->irq_pending and use cpu_interrupt/cpu_reset_interrupt instead.

IMHO, we need env->irq_pending, because if ((env->regs[CR_STATUS] & CR_STATUS_PIE) == False), it can indicate there is interrupt pending, we can exit to main loop to handle the interrupt at once if guest code enable irq later.

> 2) Do not check env->regs[CR_STATUS] & CR_STATUS_PIE.  That variable does not belong to the pic and should not be checked there.  The check belongs in nios2_cpu_exec_interrupt, and is in fact already there.

But that will cause some assert happen if device trigger irq with env->regs[CR_STATUS] & CR_STATUS_PIE = False

BR

RE: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

Correct the format

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org> 
> Sent: Friday, July 3, 2020 2:54 AM
> To: Wu, Wentong <wentong.wu@intel.com>; qemu-devel@nongnu.org
> Cc: qemu-trivial@nongnu.org; marex@denx.de; crwulff@gmail.com; peter.maydell@linaro.org
> Subject: Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction
> 
> On 6/29/20 9:05 AM, Wentong Wu wrote:
> > wrctl instruction on nios2 target will cause checking cpu interrupt 
> > but tcg_handle_interrupt() will call cpu_abort() if the CPU gets an 
> > interrupt while it's not in 'can do IO'
> > state, so add gen_io_start around wrctl instruction. Also at the same 
> > time, end the onging TB with DISAS_UPDATE.
> > 
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > ---
> >  target/nios2/translate.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 
> > 83c10eb2..51347ada 100644
> > --- a/target/nios2/translate.c
> > +++ b/target/nios2/translate.c
> > @@ -32,6 +32,7 @@
> >  #include "exec/cpu_ldst.h"
> >  #include "exec/translator.h"
> >  #include "qemu/qemu-print.h"
> > +#include "exec/gen-icount.h"
> >  
> >  /* is_jmp field values */
> >  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> > @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
> >      /* If interrupts were enabled using WRCTL, trigger them. */  #if 
> > !defined(CONFIG_USER_ONLY)
> >      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> > +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> > +            gen_io_start();
> > +        }
> >          gen_helper_check_interrupts(dc->cpu_env);
> > +        dc->is_jmp = DISAS_UPDATE;
> >      }
> >  #endif
> 
> This isn't right.  Not so much the gen_io_start portion, but the entire existence of helper_check_interrupt.
> 
> The correct way to acknowledge interrupts after changing an interrupt mask bit is to exit the TB back to the cpu main loop.
> Which you are doing here with DISAS_UPDATE, so that part is fine.  (Although you could merge that into the switch statement above.)
> 
> Looking at nios_pic_cpu_handler, there are two other bugs:
> 
> 1) Get rid of env->irq_pending and use cpu_interrupt/cpu_reset_interrupt instead.

IMHO, we need env->irq_pending, because if ((env->regs[CR_STATUS] & CR_STATUS_PIE) == False), it can indicate there is interrupt pending, we can exit to main loop to handle the interrupt at once if guest code enable irq later.

> 2) Do not check env->regs[CR_STATUS] & CR_STATUS_PIE.  That variable does not belong to the pic and should not be checked there.  The check belongs in nios2_cpu_exec_interrupt, and is in fact already there.

But that will cause some assert happen if device trigger irq with env->regs[CR_STATUS] & CR_STATUS_PIE = False

BR

RE: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

HI Peter,
Could you please take a look at this patch which is following your pervious suggestion?

Thanks

-----Original Message-----
From: Wu, Wentong <wentong.wu@intel.com> 
Sent: Tuesday, June 30, 2020 12:06 AM
To: qemu-devel@nongnu.org
Cc: qemu-trivial@nongnu.org; crwulff@gmail.com; marex@denx.de; peter.maydell@linaro.org; Wu, Wentong <wentong.wu@intel.com>
Subject: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

wrctl instruction on nios2 target will cause checking cpu interrupt but tcg_handle_interrupt() will call cpu_abort() if the CPU gets an interrupt while it's not in 'can do IO'
state, so add gen_io_start around wrctl instruction. Also at the same time, end the onging TB with DISAS_UPDATE.

Signed-off-by: Wentong Wu <wentong.wu@intel.com>
---
 target/nios2/translate.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 83c10eb2..51347ada 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -32,6 +32,7 @@
 #include "exec/cpu_ldst.h"
 #include "exec/translator.h"
 #include "qemu/qemu-print.h"
+#include "exec/gen-icount.h"
 
 /* is_jmp field values */
 #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
@@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
     /* If interrupts were enabled using WRCTL, trigger them. */  #if !defined(CONFIG_USER_ONLY)
     if ((instr.imm5 + CR_BASE) == CR_STATUS) {
+        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
+            gen_io_start();
+        }
         gen_helper_check_interrupts(dc->cpu_env);
+        dc->is_jmp = DISAS_UPDATE;
     }
 #endif
 }
--
2.21.3

RE: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]



      -----Original Message-----
      From: Richard Henderson
      Sent: Friday, July 3, 2020 2:54 AM
      To: Wu, Wentong <wentong.wu@intel.com>; qemu-devel@nongnu.org
      Cc: qemu-trivial@nongnu.org; marex@denx.de; crwulff@gmail.com; peter.maydell@linaro.org
      Subject: Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

      On 6/29/20 9:05 AM, Wentong Wu wrote:
      > wrctl instruction on nios2 target will cause checking cpu interrupt
      > but tcg_handle_interrupt() will call cpu_abort() if the CPU gets an
      > interrupt while it's not in 'can do IO'
      > state, so add gen_io_start around wrctl instruction. Also at the same
      > time, end the onging TB with DISAS_UPDATE.
      >
      > Signed-off-by: Wentong Wu <wentong.wu@intel.com<mailto:wentong.wu@intel.com>>
      > ---
      >  target/nios2/translate.c | 5 +++++
      >  1 file changed, 5 insertions(+)
      >
      > diff --git a/target/nios2/translate.c b/target/nios2/translate.c index
      > 83c10eb2..51347ada 100644
      > --- a/target/nios2/translate.c
      > +++ b/target/nios2/translate.c
      > @@ -32,6 +32,7 @@
      >  #include "exec/cpu_ldst.h"
      >  #include "exec/translator.h"
      >  #include "qemu/qemu-print.h"
      > +#include "exec/gen-icount.h"
      >
      >  /* is_jmp field values */
      >  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
      > @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
      >      /* If interrupts were enabled using WRCTL, trigger them. */  #if
      > !defined(CONFIG_USER_ONLY)
      >      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
      > +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
      > +            gen_io_start();
      > +        }
      >          gen_helper_check_interrupts(dc->cpu_env);
      > +        dc->is_jmp = DISAS_UPDATE;
      >      }
      >  #endif

      This isn't right.  Not so much the gen_io_start portion, but the entire existence of helper_check_interrupt.

      The correct way to acknowledge interrupts after changing an interrupt mask bit is to exit the TB back to the cpu main loop.
      Which you are doing here with DISAS_UPDATE, so that part is fine.  (Although you could merge that into the switch statement above.)

      Looking at nios_pic_cpu_handler, there are two other bugs:

      1) Get rid of env->irq_pending and use cpu_interrupt/cpu_reset_interrupt instead

IMHO, we need env->irq_pending, because if ((env->regs[CR_STATUS] & CR_STATUS_PIE) == False), it can indicate there is interrupt pending, we can exit to main loop to handle the interrupt at once if guest code enable irq later.

      2) Do not check env->regs[CR_STATUS] & CR_STATUS_PIE.  That variable does not belong to the pic and should not be checked there.  The check belongs in nios2_cpu_exec_interrupt, and is in fact already there.
But that will cause some assert happen if device trigger irq with env->regs[CR_STATUS] & CR_STATUS_PIE = False

BR


[-- Attachment #2: Type: text/html, Size: 7362 bytes --]

Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Richard Henderson]

On 6/29/20 9:05 AM, Wentong Wu wrote:
> wrctl instruction on nios2 target will cause checking cpu
> interrupt but tcg_handle_interrupt() will call cpu_abort()
> if the CPU gets an interrupt while it's not in 'can do IO'
> state, so add gen_io_start around wrctl instruction. Also
> at the same time, end the onging TB with DISAS_UPDATE.
> 
> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> ---
>  target/nios2/translate.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> index 83c10eb2..51347ada 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -32,6 +32,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "exec/translator.h"
>  #include "qemu/qemu-print.h"
> +#include "exec/gen-icount.h"
>  
>  /* is_jmp field values */
>  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
>      /* If interrupts were enabled using WRCTL, trigger them. */
>  #if !defined(CONFIG_USER_ONLY)
>      if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> +            gen_io_start();
> +        }
>          gen_helper_check_interrupts(dc->cpu_env);
> +        dc->is_jmp = DISAS_UPDATE;
>      }
>  #endif

This isn't right.  Not so much the gen_io_start portion, but the entire
existence of helper_check_interrupt.

The correct way to acknowledge interrupts after changing an interrupt mask bit
is to exit the TB back to the cpu main loop.
Which you are doing here with DISAS_UPDATE, so that part is fine.  (Although
you could merge that into the switch statement above.)

Looking at nios_pic_cpu_handler, there are two other bugs:

1) Get rid of env->irq_pending and use cpu_interrupt/cpu_reset_interrupt instead.

2) Do not check env->regs[CR_STATUS] & CR_STATUS_PIE.  That variable does not
belong to the pic and should not be checked there.  The check belongs in
nios2_cpu_exec_interrupt, and is in fact already there.


r~

RE: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wu, Wentong]

Hi,
Could you please take a look the new patch?

Thanks

> -----Original Message-----
> Subject: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction
> wrctl instruction on nios2 target will cause checking cpu interrupt but tcg_handle_interrupt() will call cpu_abort() if the CPU gets an interrupt while it's not in 'can do IO'
> state, so add gen_io_start around wrctl instruction. Also at the same time, end the onging TB with DISAS_UPDATE.

> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> ---
> target/nios2/translate.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 83c10eb2..51347ada 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -32,6 +32,7 @@
> #include "exec/cpu_ldst.h"
> #include "exec/translator.h"
> #include "qemu/qemu-print.h"
> +#include "exec/gen-icount.h"
 
>  /* is_jmp field values */
> #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> @@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
>     /* If interrupts were enabled using WRCTL, trigger them. */  #if !defined(CONFIG_USER_ONLY)
>    if ((instr.imm5 + CR_BASE) == CR_STATUS) {
> +        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
> +            gen_io_start();
> +        }
>          gen_helper_check_interrupts(dc->cpu_env);
> +        dc->is_jmp = DISAS_UPDATE;
>    }
> #endif
> }
> --
> 2.21.3

[PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wentong Wu]

wrctl instruction on nios2 target will cause checking cpu
interrupt but tcg_handle_interrupt() will call cpu_abort()
if the CPU gets an interrupt while it's not in 'can do IO'
state, so add gen_io_start around wrctl instruction. Also
at the same time, end the onging TB with DISAS_UPDATE.

Signed-off-by: Wentong Wu <wentong.wu@intel.com>
---
 target/nios2/translate.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 83c10eb2..51347ada 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -32,6 +32,7 @@
 #include "exec/cpu_ldst.h"
 #include "exec/translator.h"
 #include "qemu/qemu-print.h"
+#include "exec/gen-icount.h"
 
 /* is_jmp field values */
 #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
@@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
     /* If interrupts were enabled using WRCTL, trigger them. */
 #if !defined(CONFIG_USER_ONLY)
     if ((instr.imm5 + CR_BASE) == CR_STATUS) {
+        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
+            gen_io_start();
+        }
         gen_helper_check_interrupts(dc->cpu_env);
+        dc->is_jmp = DISAS_UPDATE;
     }
 #endif
 }
-- 
2.21.3