Re: [PATCH] accel/tcg/cpu-exec: fix precise single-stepping after interrupt

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

On 2/24/22 14:23, Richard Henderson wrote:
> On 2/14/22 03:26, Luc Michel wrote:
>> In some cases, cpu->exit_request can be false after handling the
>> interrupt, leading to another TB being executed instead of returning
>> to the main loop.
>>
>> Fix this by returning true unconditionally when in single-step mode.
>>
>> Fixes: ba3c35d9c4026361fd380b269dc6def9510b7166
>>
>> Signed-off-by: Luc Michel <lmichel@kalray.eu>
>> ---
>> Coming back on this issue I worked on with Richard in 2020. The issue is
>> that when debugging the guest with GDB, the first instruction of the IRQ
>> handler is missed by GDB (it's still executed though).
>>
>> It happened to me again in TCG RR mode (but not in MTTCG). It seems that
>> cpu->exit_request can be false in RR mode when returning from
>> cc->tcg_ops->cpu_exec_interrupt, leading to cpu_handle_interrupt
>> returning false and the next TB being executed, instead of the EXCP_DEBUG
>> being handled.
>> ---
>>   accel/tcg/cpu-exec.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index 8b4cd6c59d..74d7f83f34 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -796,13 +796,17 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>>                   /*
>>                    * After processing the interrupt, ensure an EXCP_DEBUG is
>>                    * raised when single-stepping so that GDB doesn't miss the
>>                    * next instruction.
>>                    */
>> -                cpu->exception_index =
>> -                    (cpu->singlestep_enabled ? EXCP_DEBUG : -1);
>> -                *last_tb = NULL;
>> +                if (unlikely(cpu->singlestep_enabled)) {
>> +                    cpu->exception_index = EXCP_DEBUG;
>> +                    return true;
> 
> By returning here, you also need to qemu_mutex_unlock_iothread().
> 
>> +                } else {
> 
> You can remove the else after the return.
> Otherwise this looks good; sorry for the delay.

I'll just fix this up when applying, it's minor enough.

r~