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

2020-07-09 Thread Wu, Wentong 
> >On Mon, 29 Jun 2020 at 09:17, 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 
> > ---
> >  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_JUMPDISAS_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 

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

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

2020-07-06 Thread 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  
> Sent: Monday, July 6, 2020 1:10 AM
> To: Wu, Wentong 
> Cc: QEMU Developers ; QEMU Trivial 
> ; Chris Wulff ; Marek Vasut 
> 
> Subject: Re: [PATCH 3/3] target/nios2: Use gen_io_start around wrctl 
> instruction
>
> On Mon, 29 Jun 2020 at 09:17, 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 
> > ---
> >  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_JUMPDISAS_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 
>
> 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

2020-07-06 Thread Peter Maydell 
On Sun, 5 Jul 2020 at 21:54, Max Filippov  wrote:
>
> On Sun, Jul 5, 2020 at 11:16 AM Max Filippov  wrote:
> > On Sun, Jul 5, 2020 at 10:09 AM Peter Maydell  
> > wrote:
> > > On Thu, 2 Jul 2020 at 19:53, Richard Henderson
> > >  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

2020-07-05 Thread 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  
Sent: Friday, July 3, 2020 2:54 AM
To: Wu, Wentong ; qemu-devel@nongnu.org
Cc: qemu-triv...@nongnu.org; ma...@denx.de; crwu...@gmail.com; 
peter.mayd...@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 
> ---
>  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_JUMPDISAS_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

2020-07-05 Thread Wu, Wentong 
Thanks, I think we can get this series merged currently.

-Original Message-
From: Peter Maydell  
Sent: Monday, July 6, 2020 1:10 AM
To: Wu, Wentong 
Cc: QEMU Developers ; QEMU Trivial 
; Chris Wulff ; Marek Vasut 

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

On Mon, 29 Jun 2020 at 09:17, 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 
> ---
>  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_JUMPDISAS_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 

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

2020-07-05 Thread Max Filippov 
On Sun, Jul 5, 2020 at 11:16 AM Max Filippov  wrote:
> On Sun, Jul 5, 2020 at 10:09 AM Peter Maydell  
> wrote:
> > On Thu, 2 Jul 2020 at 19:53, Richard Henderson
> >  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

2020-07-05 Thread Max Filippov 
On Sun, Jul 5, 2020 at 10:09 AM Peter Maydell  wrote:
> On Thu, 2 Jul 2020 at 19:53, Richard Henderson
>  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

2020-07-05 Thread Peter Maydell 
On Mon, 29 Jun 2020 at 09:17, 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 
> ---
>  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_JUMPDISAS_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 

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

2020-07-05 Thread Peter Maydell 
On Thu, 2 Jul 2020 at 19:53, Richard Henderson
 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

2020-07-05 Thread Wu, Wentong 
Correct the format

> -Original Message-
> From: Richard Henderson  
> Sent: Friday, July 3, 2020 2:54 AM
> To: Wu, Wentong ; qemu-devel@nongnu.org
> Cc: qemu-triv...@nongnu.org; ma...@denx.de; crwu...@gmail.com; 
> peter.mayd...@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 
> > ---
> >  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_JUMPDISAS_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

2020-07-05 Thread Wu, Wentong 
Correct the format

> -Original Message-
> From: Richard Henderson  
> Sent: Friday, July 3, 2020 2:54 AM
> To: Wu, Wentong ; qemu-devel@nongnu.org
> Cc: qemu-triv...@nongnu.org; ma...@denx.de; crwu...@gmail.com; 
> peter.mayd...@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 
> > ---
> >  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_JUMPDISAS_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

2020-07-03 Thread 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  
Sent: Tuesday, June 30, 2020 12:06 AM
To: qemu-devel@nongnu.org
Cc: qemu-triv...@nongnu.org; crwu...@gmail.com; ma...@denx.de; 
peter.mayd...@linaro.org; Wu, Wentong 
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 
---
 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_JUMPDISAS_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

2020-07-03 Thread Wu, Wentong 


  -Original Message-
  From: Richard Henderson
  Sent: Friday, July 3, 2020 2:54 AM
  To: Wu, Wentong ; qemu-devel@nongnu.org
  Cc: qemu-triv...@nongnu.org; ma...@denx.de; crwu...@gmail.com; 
peter.mayd...@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 
mailto:wentong...@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_JUMPDISAS_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

2020-07-02 Thread 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 
> ---
>  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_JUMPDISAS_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

2020-07-01 Thread 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 
> ---
> 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_JUMPDISAS_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