| Message ID | 20220826205518.2339352-4-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/avr: Fix skips vs interrupts | expand |
Reviewed-by: Michael Rolnik <mrolnik@gmail.com> On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson < richard.henderson@linaro.org> wrote: > This bit is not saved across interrupts, so we must > delay delivering the interrupt until the skip has > been processed. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1118 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/avr/helper.c | 9 +++++++++ > target/avr/translate.c | 26 ++++++++++++++++++++++---- > 2 files changed, 31 insertions(+), 4 deletions(-) > > diff --git a/target/avr/helper.c b/target/avr/helper.c > index 34f1cbffb2..156dde4e92 100644 > --- a/target/avr/helper.c > +++ b/target/avr/helper.c > @@ -31,6 +31,15 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int > interrupt_request) > AVRCPU *cpu = AVR_CPU(cs); > CPUAVRState *env = &cpu->env; > > + /* > + * We cannot separate a skip from the next instruction, > + * as the skip would not be preserved across the interrupt. > + * Separating the two insn normally only happens at page boundaries. > + */ > + if (env->skip) { > + return false; > + } > + > if (interrupt_request & CPU_INTERRUPT_RESET) { > if (cpu_interrupts_enabled(env)) { > cs->exception_index = EXCP_RESET; > diff --git a/target/avr/translate.c b/target/avr/translate.c > index dc9c3d6bcc..026753c963 100644 > --- a/target/avr/translate.c > +++ b/target/avr/translate.c > @@ -2971,8 +2971,18 @@ static void avr_tr_translate_insn(DisasContextBase > *dcbase, CPUState *cs) > if (skip_label) { > canonicalize_skip(ctx); > gen_set_label(skip_label); > - if (ctx->base.is_jmp == DISAS_NORETURN) { > + > + switch (ctx->base.is_jmp) { > + case DISAS_NORETURN: > ctx->base.is_jmp = DISAS_CHAIN; > + break; > + case DISAS_NEXT: > + if (ctx->base.tb->flags & TB_FLAGS_SKIP) { > + ctx->base.is_jmp = DISAS_TOO_MANY; > + } > + break; > + default: > + break; > } > } > > @@ -2989,6 +2999,11 @@ static void avr_tr_tb_stop(DisasContextBase > *dcbase, CPUState *cs) > { > DisasContext *ctx = container_of(dcbase, DisasContext, base); > bool nonconst_skip = canonicalize_skip(ctx); > + /* > + * Because we disable interrupts while env->skip is set, > + * we must return to the main loop to re-evaluate afterward. > + */ > + bool force_exit = ctx->base.tb->flags & TB_FLAGS_SKIP; > > switch (ctx->base.is_jmp) { > case DISAS_NORETURN: > @@ -2997,7 +3012,7 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, > CPUState *cs) > case DISAS_NEXT: > case DISAS_TOO_MANY: > case DISAS_CHAIN: > - if (!nonconst_skip) { > + if (!nonconst_skip && !force_exit) { > /* Note gen_goto_tb checks singlestep. */ > gen_goto_tb(ctx, 1, ctx->npc); > break; > @@ -3005,8 +3020,11 @@ static void avr_tr_tb_stop(DisasContextBase > *dcbase, CPUState *cs) > tcg_gen_movi_tl(cpu_pc, ctx->npc); > /* fall through */ > case DISAS_LOOKUP: > - tcg_gen_lookup_and_goto_ptr(); > - break; > + if (!force_exit) { > + tcg_gen_lookup_and_goto_ptr(); > + break; > + } > + /* fall through */ > case DISAS_EXIT: > tcg_gen_exit_tb(NULL, 0); > break; > -- > 2.34.1 > >
diff --git a/target/avr/helper.c b/target/avr/helper.c index 34f1cbffb2..156dde4e92 100644 --- a/target/avr/helper.c +++ b/target/avr/helper.c @@ -31,6 +31,15 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request) AVRCPU *cpu = AVR_CPU(cs); CPUAVRState *env = &cpu->env; + /* + * We cannot separate a skip from the next instruction, + * as the skip would not be preserved across the interrupt. + * Separating the two insn normally only happens at page boundaries. + */ + if (env->skip) { + return false; + } + if (interrupt_request & CPU_INTERRUPT_RESET) { if (cpu_interrupts_enabled(env)) { cs->exception_index = EXCP_RESET; diff --git a/target/avr/translate.c b/target/avr/translate.c index dc9c3d6bcc..026753c963 100644 --- a/target/avr/translate.c +++ b/target/avr/translate.c @@ -2971,8 +2971,18 @@ static void avr_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs) if (skip_label) { canonicalize_skip(ctx); gen_set_label(skip_label); - if (ctx->base.is_jmp == DISAS_NORETURN) { + + switch (ctx->base.is_jmp) { + case DISAS_NORETURN: ctx->base.is_jmp = DISAS_CHAIN; + break; + case DISAS_NEXT: + if (ctx->base.tb->flags & TB_FLAGS_SKIP) { + ctx->base.is_jmp = DISAS_TOO_MANY; + } + break; + default: + break; } } @@ -2989,6 +2999,11 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) { DisasContext *ctx = container_of(dcbase, DisasContext, base); bool nonconst_skip = canonicalize_skip(ctx); + /* + * Because we disable interrupts while env->skip is set, + * we must return to the main loop to re-evaluate afterward. + */ + bool force_exit = ctx->base.tb->flags & TB_FLAGS_SKIP; switch (ctx->base.is_jmp) { case DISAS_NORETURN: @@ -2997,7 +3012,7 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) case DISAS_NEXT: case DISAS_TOO_MANY: case DISAS_CHAIN: - if (!nonconst_skip) { + if (!nonconst_skip && !force_exit) { /* Note gen_goto_tb checks singlestep. */ gen_goto_tb(ctx, 1, ctx->npc); break; @@ -3005,8 +3020,11 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) tcg_gen_movi_tl(cpu_pc, ctx->npc); /* fall through */ case DISAS_LOOKUP: - tcg_gen_lookup_and_goto_ptr(); - break; + if (!force_exit) { + tcg_gen_lookup_and_goto_ptr(); + break; + } + /* fall through */ case DISAS_EXIT: tcg_gen_exit_tb(NULL, 0); break;
This bit is not saved across interrupts, so we must delay delivering the interrupt until the skip has been processed. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1118 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/avr/helper.c | 9 +++++++++ target/avr/translate.c | 26 ++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 4 deletions(-)