Re: [PATCH-for-8.0 2/4] gdbstub: Use vaddr type for generic insert/remove_breakpoint() API

[Philippe Mathieu-Daudé]

On 7/12/22 19:23, Peter Maydell wrote:
> On Wed, 7 Dec 2022 at 17:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > Both insert/remove_breakpoint() handlers are used in system and
> > user emulation. We can not use the 'hwaddr' type on user emulation,
> > we have to use 'vaddr' which is defined as "wide enough to contain
> > any #target_ulong virtual address".
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   gdbstub/internals.h        | 6 ++++--
> >   include/sysemu/accel-ops.h | 6 +++---
> >   2 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> > index eabb0341d1..b23999f951 100644
> > --- a/gdbstub/internals.h
> > +++ b/gdbstub/internals.h
> > @@ -9,9 +9,11 @@
> >   #ifndef _INTERNALS_H_
> >   #define _INTERNALS_H_
> >
> > +#include "exec/cpu-common.h"
> > +
> >   bool gdb_supports_guest_debug(void);
> > -int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len);
> > -int gdb_breakpoint_remove(CPUState *cs, int type, hwaddr addr, hwaddr len);
> > +int gdb_breakpoint_insert(CPUState *cs, int type, vaddr addr, vaddr len);
> > +int gdb_breakpoint_remove(CPUState *cs, int type, vaddr addr, vaddr len);
> >   void gdb_breakpoint_remove_all(CPUState *cs);
> >
> >   #endif /* _INTERNALS_H_ */
> > diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
> > index 8cc7996def..30690c71bd 100644
> > --- a/include/sysemu/accel-ops.h
> > +++ b/include/sysemu/accel-ops.h
> > @@ -10,7 +10,7 @@
> >   #ifndef ACCEL_OPS_H
> >   #define ACCEL_OPS_H
> >
> > -#include "exec/hwaddr.h"
> > +#include "exec/cpu-common.h"
> >   #include "qom/object.h"
> >
> >   #define ACCEL_OPS_SUFFIX "-ops"
> > @@ -48,8 +48,8 @@ struct AccelOpsClass {
> >
> >       /* gdbstub hooks */
> >       bool (*supports_guest_debug)(void);
> > -    int (*insert_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len);
> > -    int (*remove_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len);
> > +    int (*insert_breakpoint)(CPUState *cpu, int type, vaddr addr, vaddr len);
> > +    int (*remove_breakpoint)(CPUState *cpu, int type, vaddr addr, vaddr len);
> >       void (*remove_all_breakpoints)(CPUState *cpu);
> >   };
>
> If you're changing the prototype of these methods on AccelOpsClass
> don't you also want to change the implementations, eg tcg_breakpoint_insert()?
> Interestingly that function calls cpu_breakpoint_insert() which
> already takes a 'vaddr' rather than a 'hwaddr'.

Yes I neglected to include these changes here:

-- >8 --
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index be88ca0d71..c3fbc31123 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -48,7 +48,6 @@
 #include "sysemu/runstate.h"
 #include "semihosting/semihost.h"
 #include "exec/exec-all.h"
-#include "exec/hwaddr.h"
 #include "sysemu/replay.h"

 #include "internals.h"
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index f208c6cf15..129575e510 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -11,7 +11,6 @@

 #include "qemu/osdep.h"
 #include "exec/gdbstub.h"
-#include "exec/hwaddr.h"
 #include "sysemu/cpus.h"
 #include "internals.h"

@@ -24,7 +23,7 @@ bool gdb_supports_guest_debug(void)
     return false;
 }

-int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len)
+int gdb_breakpoint_insert(CPUState *cs, int type, vaddr addr, vaddr len)
 {
     const AccelOpsClass *ops = cpus_get_accel();
     if (ops->insert_breakpoint) {
@@ -33,7 +32,7 @@ int gdb_breakpoint_insert(CPUState *cs, int type, 
hwaddr addr, hwaddr len)
     return -ENOSYS;
 }

-int gdb_breakpoint_remove(CPUState *cs, int type, hwaddr addr, hwaddr len)
+int gdb_breakpoint_remove(CPUState *cs, int type, vaddr addr, vaddr len)
 {
     const AccelOpsClass *ops = cpus_get_accel();
     if (ops->remove_breakpoint) {
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 033e5fdd71..484bd8f461 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -9,7 +9,6 @@
  */

 #include "qemu/osdep.h"
-#include "exec/hwaddr.h"
 #include "exec/gdbstub.h"
 #include "hw/core/cpu.h"
 #include "internals.h"
@@ -20,7 +19,7 @@ bool gdb_supports_guest_debug(void)
     return true;
 }

-int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len)
+int gdb_breakpoint_insert(CPUState *cs, int type, vaddr addr, vaddr len)
 {
     CPUState *cpu;
     int err = 0;
@@ -41,7 +40,7 @@ int gdb_breakpoint_insert(CPUState *cs, int type, 
hwaddr addr, hwaddr len)
     }
 }

-int gdb_breakpoint_remove(CPUState *cs, int type, hwaddr addr, hwaddr len)
+int gdb_breakpoint_remove(CPUState *cs, int type, vaddr addr, vaddr len)
 {
     CPUState *cpu;
     int err = 0;
---

> In looking at this I discovered some rather confusing gdbstub behaviour:
> if you use the qemu.PhyMemMode custom gdb flag to put the stub into
> "physical memory mode", data reads and writes are done on physical
> addresses, but breakpoints and watchpoints continue to take virtual
> addresses.
>
> But at any rate given that currently breakpoints are always on virtual
> addresses, vaddr is definitely the right type here and probably all
> the way down through the callstack.
>
> thanks
> -- PMM