-----Original Message----- From: Richard Henderson 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<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_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