Correct the format > -----Original Message----- > From: Richard Henderson <richard.hender...@linaro.org> > Sent: Friday, July 3, 2020 2:54 AM > To: Wu, Wentong <wentong...@intel.com>; 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 <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_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