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~