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

[Claudio Fontana]

On 1/18/21 10:36 AM, Philippe Mathieu-Daudé wrote:
> 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(-)
> 

Hello Philippe,

I have reached this issue again when working on the ARM part of the cleanup,
did we reach a conclusion on whether cpu_watchpoint_insert is TCG-specific or 
not,

and more in general about breakpoints and watchpoints?

The way I encountered this issue now is during the KVM/TCG split in target/arm.

there are calls in target/arm/cpu.c and machine.c of the kind:

hw_breakpoint_update_all()
hw_watchpoint_update_all()

is this all TCG-specific,

including also hw_watchpoint_update(), hw_breakpoint_update() ?

kvm64.c seems to have its own handling of breakpoints, including arrays:

GArray *hw_breakpoints, *hw_watchpoints;

while the TCG stuff relies on cpu->watchpoints in softmmu/physmem.c:

$ gid watchpoints
include/hw/core/cpu.h:139: * address before attempting to match it against 
watchpoints.
include/hw/core/cpu.h:388:    QTAILQ_HEAD(, CPUWatchpoint) watchpoints;
linux-user/main.c:210:    /* Clone all break/watchpoints.
linux-user/main.c:212:       BP_CPU break/watchpoints are handled correctly on 
clone. */
linux-user/main.c:214:    QTAILQ_INIT(&new_cpu->watchpoints);
linux-user/main.c:218:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
softmmu/physmem.c:791:    /* keep all GDB-injected watchpoints in front */
softmmu/physmem.c:793:        QTAILQ_INSERT_HEAD(&cpu->watchpoints, wp, entry);
softmmu/physmem.c:795:        QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry);
softmmu/physmem.c:816:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
softmmu/physmem.c:829:    QTAILQ_REMOVE(&cpu->watchpoints, watchpoint, entry);
softmmu/physmem.c:836:/* Remove all matching watchpoints.  */
softmmu/physmem.c:841:    QTAILQ_FOREACH_SAFE(wp, &cpu->watchpoints, entry, 
next) {
softmmu/physmem.c:868:/* Return flags for watchpoints that match addr + prot.  
*/
softmmu/physmem.c:874:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
softmmu/physmem.c:906:    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
softmmu/physmem.c:911:                 * Don't process the watchpoints when we 
are
accel/tcg/cpu-exec.c:511:        QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
accel/tcg/cpu-exec.c:822:               after-access watchpoints.  Since this 
request should never
hw/core/cpu.c:361:    QTAILQ_INIT(&cpu->watchpoints);


Even in i386 there is a bit of confusion still, and I think sorting out this 
could improve the code on i386 as well..

Thanks for any comment,

Ciao,

CLaudio