| Message ID | 20240604071833.962574-9-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: fixes for INHIBIT_IRQ, TF and RF | expand |
On 6/4/24 02:18, Paolo Bonzini wrote: > PAUSE uses DISAS_NORETURN because the corresponding helper > calls cpu_loop_exit(). However, while HLT clear HF_INHIBIT_IRQ_MASK > to correctly handle "STI; HLT", the same is missing from PAUSE. > And also gen_eob() clears HF_RF_MASK and synthesizes a #DB exception > if single-step is active; none of this is done by HLT and PAUSE. > Start fixing PAUSE, HLT will follow. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target/i386/tcg/misc_helper.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/i386/tcg/misc_helper.c b/target/i386/tcg/misc_helper.c > index 8316d42ffcd..ed4cda8001e 100644 > --- a/target/i386/tcg/misc_helper.c > +++ b/target/i386/tcg/misc_helper.c > @@ -92,6 +92,10 @@ G_NORETURN void helper_pause(CPUX86State *env) > { > CPUState *cs = env_cpu(env); > > + /* Do gen_eob() tasks before going back to the main loop. */ > + do_end_instruction(env); > + helper_rechecking_single_step(env); > + > /* Just let another CPU run. */ > cs->exception_index = EXCP_INTERRUPT; > cpu_loop_exit(cs); Perhaps it would be better to do void helper_cpu_exit(CPUX86State *env) { cpu_exit(env_cpu(env)); } static void gen_PAUSE(...) { helper_cpu_exit(tcg_env); s->base.is_jmp = DISAS_EOB_NEXT; } to keep from replicating gen_eob? Anyway, this is correct, so, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 6/4/24 08:44, Richard Henderson wrote: > On 6/4/24 02:18, Paolo Bonzini wrote: >> PAUSE uses DISAS_NORETURN because the corresponding helper >> calls cpu_loop_exit(). However, while HLT clear HF_INHIBIT_IRQ_MASK >> to correctly handle "STI; HLT", the same is missing from PAUSE. >> And also gen_eob() clears HF_RF_MASK and synthesizes a #DB exception >> if single-step is active; none of this is done by HLT and PAUSE. >> Start fixing PAUSE, HLT will follow. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> target/i386/tcg/misc_helper.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/target/i386/tcg/misc_helper.c b/target/i386/tcg/misc_helper.c >> index 8316d42ffcd..ed4cda8001e 100644 >> --- a/target/i386/tcg/misc_helper.c >> +++ b/target/i386/tcg/misc_helper.c >> @@ -92,6 +92,10 @@ G_NORETURN void helper_pause(CPUX86State *env) >> { >> CPUState *cs = env_cpu(env); >> + /* Do gen_eob() tasks before going back to the main loop. */ >> + do_end_instruction(env); >> + helper_rechecking_single_step(env); >> + >> /* Just let another CPU run. */ >> cs->exception_index = EXCP_INTERRUPT; >> cpu_loop_exit(cs); > > Perhaps it would be better to do > > void helper_cpu_exit(CPUX86State *env) > { > cpu_exit(env_cpu(env)); > } > > static void gen_PAUSE(...) > { > helper_cpu_exit(tcg_env); > s->base.is_jmp = DISAS_EOB_NEXT; > } > > to keep from replicating gen_eob? > > Anyway, this is correct, so, > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Oh, based on the next patch, it would appear that PAUSE does not single-step properly because it sets EXCP_INTERRUPT, and end-of-insn single-step depends on exception_index == -1. I'm thinking of the bottom of cpu_tb_exec(). r~
On Tue, Jun 4, 2024 at 3:49 PM Richard Henderson <richard.henderson@linaro.org> wrote: > Oh, based on the next patch, it would appear that PAUSE does not single-step properly > because it sets EXCP_INTERRUPT, and end-of-insn single-step depends on exception_index == > -1. I'm thinking of the bottom of cpu_tb_exec(). I'm not sure we're talking of the same thing, but that's why I'm calling helper_rechecking_single_step() before setting EXCP_INTERRUPT. Paolo
On 6/4/24 09:10, Paolo Bonzini wrote: > On Tue, Jun 4, 2024 at 3:49 PM Richard Henderson > <richard.henderson@linaro.org> wrote: >> Oh, based on the next patch, it would appear that PAUSE does not single-step properly >> because it sets EXCP_INTERRUPT, and end-of-insn single-step depends on exception_index == >> -1. I'm thinking of the bottom of cpu_tb_exec(). > > I'm not sure we're talking of the same thing, but that's why I'm > calling helper_rechecking_single_step() before setting EXCP_INTERRUPT. Oh yes, that does it. r~
diff --git a/target/i386/tcg/misc_helper.c b/target/i386/tcg/misc_helper.c index 8316d42ffcd..ed4cda8001e 100644 --- a/target/i386/tcg/misc_helper.c +++ b/target/i386/tcg/misc_helper.c @@ -92,6 +92,10 @@ G_NORETURN void helper_pause(CPUX86State *env) { CPUState *cs = env_cpu(env); + /* Do gen_eob() tasks before going back to the main loop. */ + do_end_instruction(env); + helper_rechecking_single_step(env); + /* Just let another CPU run. */ cs->exception_index = EXCP_INTERRUPT; cpu_loop_exit(cs);
PAUSE uses DISAS_NORETURN because the corresponding helper calls cpu_loop_exit(). However, while HLT clear HF_INHIBIT_IRQ_MASK to correctly handle "STI; HLT", the same is missing from PAUSE. And also gen_eob() clears HF_RF_MASK and synthesizes a #DB exception if single-step is active; none of this is done by HLT and PAUSE. Start fixing PAUSE, HLT will follow. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target/i386/tcg/misc_helper.c | 4 ++++ 1 file changed, 4 insertions(+)