Re: [PATCH] softmmu: fix watchpoint processing in icount mode

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

On 9/10/21 1:15 PM, David Hildenbrand wrote:
> On 07.09.21 13:30, Pavel Dovgalyuk wrote:
>> Watchpoint processing code restores vCPU state twice:
>> in tb_check_watchpoint and in cpu_loop_exit_restore/cpu_restore_state.
>> Normally it does not affect anything, but in icount mode instruction
>> counter is incremented twice and becomes incorrect.
>> This patch eliminates unneeded CPU state restore.
>>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>> ---
>>   softmmu/physmem.c |    5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 23e77cb771..4025dfab11 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -941,14 +941,11 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
>>                   if (wp->flags & BP_STOP_BEFORE_ACCESS) {
>>                       cpu->exception_index = EXCP_DEBUG;
>>                       mmap_unlock();
>> -                    cpu_loop_exit_restore(cpu, ra);
>> +                    cpu_loop_exit(cpu);
>>                   } else {
>>                       /* Force execution of one insn next time.  */
>>                       cpu->cflags_next_tb = 1 | curr_cflags(cpu);
>>                       mmap_unlock();
>> -                    if (ra) {
>> -                        cpu_restore_state(cpu, ra, true);
>> -                    }
>>                       cpu_loop_exit_noexc(cpu);
>>                   }
>>               }
>>
>>
> 
> I'm not an expert on that code, but it looks good to me.
> 
> Maybe we could have added a comment above the tb_check_watchpoint() call to highlight that 
> the restore will happen in there.

Hmm.  Curious.

Looking at tb_check_watchpoint, I have trouble seeing how it could be correct. 
Watchpoints can happen at any memory reference within the TB.  We should be rolling back 
to the cpu state at the memory reference (cpu_retore_state) and not the cpu state at the 
start of the TB (cpu_restore_state_from_tb).

I'm also not sure why we're invalidating tb's.  Why does watchpoint hit imply that we 
should want to ditch the TB?  If we want different behaviour from the next execution, we 
should be adjusting cflags.


r~