Re: [PATCH 08/11] target/i386: fix INHIBIT_IRQ/TF/RF handling for PAUSE

[Richard Henderson <richard.henderson@linaro.org>]

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~