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);
 };
 
-- 
2.38.1

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'.

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

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

On Wed, 7 Dec 2022 at 18:27, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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".

> >> @@ -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
> diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
> diff --git a/gdbstub/user.c b/gdbstub/user.c

Those are the callsites to the methods, not the implementations, I think.

-- PMM

On 7/12/22 21:15, Peter Maydell wrote:
> On Wed, 7 Dec 2022 at 18:27, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> 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".
> 
>>>> @@ -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
>> diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
>> diff --git a/gdbstub/user.c b/gdbstub/user.c
> 
> Those are the callsites to the methods, not the implementations, I think.

Oh correct, I missed that :/ Thanks!

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> 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);

Now we should be able to remove the "exec/hwaddr.h" include from
gdbstub.c

>  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);
>  };

On 7/12/22 19:08, Fabiano Rosas wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> 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);
> 
> Now we should be able to remove the "exec/hwaddr.h" include from
> gdbstub.c
Yes. And you made me notice I didn't commit the changes in 
gdbstub/{system,user}.c...