Re: [RFC PATCH 6/6] softmmu: Restrict watchpoint handlers to TCG accelerator

[Philippe Mathieu-Daudé]

On 1/18/21 10:10 AM, Claudio Fontana wrote:
> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>> Watchpoint funtions use cpu_restore_state() which is only
>> available when TCG accelerator is built. Restrict them
>> to TCG.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I am doing some of this in my series, and I did not notice that
> cpu_watchpoint_insert was also TCG only.
> 
> Probably we should merge this somehow.
> 
> I thought it was used by gdbstub.c as well, passing flags BP_GDB .

BP_MEM_ACCESS for watchpoint.

> I noticed that gdbstub does something else entirely for kvm_enabled(), ie, 
> kvm_insert_breakpoint,
> but what about the other accels, it seems that the code flows to the 
> cpu_breakpoint_insert and watchpoint_insert..?
> 
> should cpu_breakpoint_insert have the same fate then?
> 
> And is this really all TCG specific?
> 
> From gdbstub.c:1020:
> 
> static int gdb_breakpoint_insert(int type, target_ulong addr, target_ulong 
> len)
> {
>     CPUState *cpu;
>     int err = 0;
> 
>     if (kvm_enabled()) {
>         return kvm_insert_breakpoint(gdbserver_state.c_cpu, addr, len, type);

Doh I missed that. I remember Peter and Richard explained it once
but I forgot and couldn't find on the list, maybe it was on IRC.

See i.e. in target/arm/kvm64.c:

 312 int kvm_arch_insert_hw_breakpoint(target_ulong addr,
 313                                   target_ulong len, int type)
 314 {
 315     switch (type) {
 316     case GDB_BREAKPOINT_HW:
 317         return insert_hw_breakpoint(addr);
 318         break;
 319     case GDB_WATCHPOINT_READ:
 320     case GDB_WATCHPOINT_WRITE:
 321     case GDB_WATCHPOINT_ACCESS:
 322         return insert_hw_watchpoint(addr, len, type);
 323     default:
 324         return -ENOSYS;
 325     }
 326 }

> 
>> ---
>> RFC because we could keep that code by adding an empty
>>     stub for cpu_restore_state(), but it is unclear as
>>     the function is named generically.
>> ---
>>  include/hw/core/cpu.h | 4 ++--
>>  softmmu/physmem.c     | 4 ++++
>>  2 files changed, 6 insertions(+), 2 deletions(-)