| Message ID | 20170614194821.8754-4-rth@twiddle.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Richard Henderson <rth@twiddle.net> writes: > From: Paolo Bonzini <pbonzini@redhat.com> > > Exit to cpu loop so we reevaluate cpu_mips_hw_interrupts. > > Cc: Aurelien Jarno <aurelien@aurel32.net> > Cc: Yongbok Kim <yongbok.kim@imgtec.com> > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > target/mips/translate.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/target/mips/translate.c b/target/mips/translate.c > index 559f8fe..891f14b 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -13403,9 +13403,11 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs) > save_cpu_state(ctx, 1); > gen_helper_ei(t0, cpu_env); > gen_store_gpr(t0, rs); > - /* Stop translation as we may have switched the execution mode */ > - ctx->bstate = BS_STOP; > tcg_temp_free(t0); > + /* BS_STOP isn't good enough here; > + reevaluate cpu_mips_hw_interrupts_enabled. */ nit: technically we want to ensure mips_cpu_exec_interrupt is run (which calls cpu_mips_hw_interrupts_enabled) > + gen_save_pc(ctx->pc + 4); > + ctx->bstate = BS_EXCP; > } > break; > default: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
On 15/06/2017 10:57, Alex Bennée wrote: >> + /* BS_STOP isn't good enough here; >> + reevaluate cpu_mips_hw_interrupts_enabled. */ > nit: technically we want to ensure mips_cpu_exec_interrupt is run (which > calls cpu_mips_hw_interrupts_enabled) Right, that's why it's "reevaluate" not "reexecute". :) Just a nit of course. Paolo
On 2017-06-14 12:48, Richard Henderson wrote: > From: Paolo Bonzini <pbonzini@redhat.com> > > Exit to cpu loop so we reevaluate cpu_mips_hw_interrupts. > > Cc: Aurelien Jarno <aurelien@aurel32.net> > Cc: Yongbok Kim <yongbok.kim@imgtec.com> > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > target/mips/translate.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/target/mips/translate.c b/target/mips/translate.c > index 559f8fe..891f14b 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -13403,9 +13403,11 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs) > save_cpu_state(ctx, 1); > gen_helper_ei(t0, cpu_env); > gen_store_gpr(t0, rs); > - /* Stop translation as we may have switched the execution mode */ > - ctx->bstate = BS_STOP; > tcg_temp_free(t0); > + /* BS_STOP isn't good enough here; > + reevaluate cpu_mips_hw_interrupts_enabled. */ > + gen_save_pc(ctx->pc + 4); > + ctx->bstate = BS_EXCP; > } > break; > default: While the above looks correct, it's not complete. It only fixes the microMIPS EI instruction. The MIPS one also has to be fixed. For what I understood, anything that can change the result of cpu_mips_hw_interrupts_enabled has to stop the translation. In that case I checked that ERET/ERETNC and MTC0/DMTC0 to the Status register are already correct, that said it might be a good idea to update the comments to mention it. I can work on a better patch, but I doubt I'll have time before the week-end. Aurelien
On 06/15/2017 02:19 PM, Aurelien Jarno wrote: > While the above looks correct, it's not complete. It only fixes the > microMIPS EI instruction. The MIPS one also has to be fixed. > > For what I understood, anything that can change the result of > cpu_mips_hw_interrupts_enabled has to stop the translation. In that case > I checked that ERET/ERETNC and MTC0/DMTC0 to the Status register are > already correct, that said it might be a good idea to update the > comments to mention it. > > I can work on a better patch, but I doubt I'll have time before the > week-end. Ok, I'll drop this one for now. It's not urgent. r~
diff --git a/target/mips/translate.c b/target/mips/translate.c index 559f8fe..891f14b 100644 --- a/target/mips/translate.c +++ b/target/mips/translate.c @@ -13403,9 +13403,11 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs) save_cpu_state(ctx, 1); gen_helper_ei(t0, cpu_env); gen_store_gpr(t0, rs); - /* Stop translation as we may have switched the execution mode */ - ctx->bstate = BS_STOP; tcg_temp_free(t0); + /* BS_STOP isn't good enough here; + reevaluate cpu_mips_hw_interrupts_enabled. */ + gen_save_pc(ctx->pc + 4); + ctx->bstate = BS_EXCP; } break; default: