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