Re: [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 6/3/20 2:46 PM, Philippe Mathieu-Daudé wrote:
> On 6/3/20 1:24 PM, Alex Bennée wrote:
>> There is no particular reason why you can't have a watchpoint in TCG
>> that covers a large chunk of the address space. We could be clever
>> about it but these cases are pretty rare and we can assume the user
>> will expect a little performance degradation.
>>
>> NB: In my testing gdb will silently squash a watchpoint like:
>>
>>   watch (char[0x7fffffffff]) *0x0
>>
>> to a 4 byte watchpoint. Practically it will limit the maximum size
>> based on max-value-size. However given enough of a tweak the sky is
>> the limit.
>>
>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>   - use cleaner in_page = -(addr | TARGET_PAGE_MASK) logic per rth
> 
> Can we have a macro for this?
> Maybe QEMU_IN_PAGE_OFFSET(addr, TARGET_PAGE_MASK)?
> or QEMU_OFFSET_IN_PAGE()...

As this is queued, I suppose the implicit answer is "no."

> 
>> ---
>>  exec.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 5162f0d12f9..65a4376df37 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1036,6 +1036,7 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>>                            int flags, CPUWatchpoint **watchpoint)
>>  {
>>      CPUWatchpoint *wp;
>> +    vaddr in_page;
>>  
>>      /* forbid ranges which are empty or run off the end of the address space */
>>      if (len == 0 || (addr + len - 1) < addr) {
>> @@ -1056,7 +1057,12 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>>          QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry);
>>      }
>>  
>> -    tlb_flush_page(cpu, addr);
>> +    in_page = -(addr | TARGET_PAGE_MASK);
>> +    if (len <= in_page) {
>> +        tlb_flush_page(cpu, addr);
>> +    } else {
>> +        tlb_flush(cpu);
>> +    }
>>  
>>      if (watchpoint)
>>          *watchpoint = wp;
>>
>