[PATCH-for-8.0 0/4] target/cpu: System/User cleanups around hwaddr/vaddr

[PATCH-for-8.0 0/4] target/cpu: System/User cleanups around hwaddr/vaddr

[Philippe Mathieu-Daudé]

We are not supposed to use hwaddr on user emulation.

This series is a - trivial - preparatory cleanup before
few refactors to isolate further System vs User code.

Philippe Mathieu-Daudé (4):
  cputlb: Restrict SavedIOTLB to system emulation
  gdbstub: Use vaddr type for generic insert/remove_breakpoint() API
  target/cpu: Restrict cpu_get_phys_page_debug() handlers to sysemu
  target/sparc: Cleanup around sparc_cpu_do_unaligned_access()

 accel/tcg/cputlb.c         | 4 ++--
 gdbstub/internals.h        | 6 ++++--
 include/hw/core/cpu.h      | 6 ++++--
 include/sysemu/accel-ops.h | 6 +++---
 target/alpha/cpu.h         | 2 +-
 target/cris/cpu.h          | 3 +--
 target/hppa/cpu.h          | 2 +-
 target/m68k/cpu.h          | 2 +-
 target/nios2/cpu.h         | 2 +-
 target/openrisc/cpu.h      | 3 ++-
 target/ppc/cpu.h           | 2 +-
 target/rx/cpu.h            | 2 +-
 target/rx/helper.c         | 4 ++--
 target/sh4/cpu.h           | 2 +-
 target/sparc/cpu.h         | 3 ++-
 target/sparc/mmu_helper.c  | 2 --
 target/xtensa/cpu.h        | 2 +-
 17 files changed, 28 insertions(+), 25 deletions(-)

-- 
2.38.1

[PATCH-for-8.0 1/4] cputlb: Restrict SavedIOTLB to system emulation

[Philippe Mathieu-Daudé]

Commit 2f3a57ee47 ("cputlb: ensure we save the IOTLB data in
case of reset") added the SavedIOTLB structure -- which is
system emulation specific -- in the generic CPUState structure.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/cputlb.c    | 4 ++--
 include/hw/core/cpu.h | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 6f1c00682b..0ea96fbcdf 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1395,7 +1395,7 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
 static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
                             hwaddr mr_offset)
 {
-#ifdef CONFIG_PLUGIN
+#if defined(CONFIG_PLUGIN) && !defined(CONFIG_USER_ONLY)
     SavedIOTLB *saved = &cs->saved_iotlb;
     saved->section = section;
     saved->mr_offset = mr_offset;
@@ -1699,7 +1699,7 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
     return qemu_ram_addr_from_host_nofail(p);
 }
 
-#ifdef CONFIG_PLUGIN
+#if defined(CONFIG_PLUGIN) && !defined(CONFIG_USER_ONLY)
 /*
  * Perform a TLB lookup and populate the qemu_plugin_hwaddr structure.
  * This should be a hot path as we will have just looked this path up
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8830546121..bc3229ae13 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -222,7 +222,7 @@ struct CPUWatchpoint {
     QTAILQ_ENTRY(CPUWatchpoint) entry;
 };
 
-#ifdef CONFIG_PLUGIN
+#if defined(CONFIG_PLUGIN) && !defined(CONFIG_USER_ONLY)
 /*
  * For plugins we sometime need to save the resolved iotlb data before
  * the memory regions get moved around  by io_writex.
@@ -406,9 +406,11 @@ struct CPUState {
 
 #ifdef CONFIG_PLUGIN
     GArray *plugin_mem_cbs;
+#if !defined(CONFIG_USER_ONLY)
     /* saved iotlb data from io_writex */
     SavedIOTLB saved_iotlb;
-#endif
+#endif /* !CONFIG_USER_ONLY */
+#endif /* CONFIG_PLUGIN */
 
     /* TODO Move common fields from CPUArchState here. */
     int cpu_index;
-- 
2.38.1

Re: [PATCH-for-8.0 1/4] cputlb: Restrict SavedIOTLB to system emulation

[Alex Bennée]

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

> Commit 2f3a57ee47 ("cputlb: ensure we save the IOTLB data in
> case of reset") added the SavedIOTLB structure -- which is
> system emulation specific -- in the generic CPUState structure.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  accel/tcg/cputlb.c    | 4 ++--
>  include/hw/core/cpu.h | 6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 6f1c00682b..0ea96fbcdf 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1395,7 +1395,7 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
>  static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
>                              hwaddr mr_offset)
>  {
> -#ifdef CONFIG_PLUGIN
> +#if defined(CONFIG_PLUGIN) && !defined(CONFIG_USER_ONLY)

cputlb is softmmu only so I don't think we need to check CONFIG_USER_ONLY here.

>      SavedIOTLB *saved = &cs->saved_iotlb;
>      saved->section = section;
>      saved->mr_offset = mr_offset;
> @@ -1699,7 +1699,7 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
>      return qemu_ram_addr_from_host_nofail(p);
>  }
>  
> -#ifdef CONFIG_PLUGIN
> +#if defined(CONFIG_PLUGIN) && !defined(CONFIG_USER_ONLY)
>  /*
>   * Perform a TLB lookup and populate the qemu_plugin_hwaddr structure.
>   * This should be a hot path as we will have just looked this path up
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 8830546121..bc3229ae13 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -222,7 +222,7 @@ struct CPUWatchpoint {
>      QTAILQ_ENTRY(CPUWatchpoint) entry;
>  };
>  
> -#ifdef CONFIG_PLUGIN
> +#if defined(CONFIG_PLUGIN) && !defined(CONFIG_USER_ONLY)
>  /*
>   * For plugins we sometime need to save the resolved iotlb data before
>   * the memory regions get moved around  by io_writex.
> @@ -406,9 +406,11 @@ struct CPUState {
>  
>  #ifdef CONFIG_PLUGIN
>      GArray *plugin_mem_cbs;
> +#if !defined(CONFIG_USER_ONLY)
>      /* saved iotlb data from io_writex */
>      SavedIOTLB saved_iotlb;
> -#endif
> +#endif /* !CONFIG_USER_ONLY */
> +#endif /* CONFIG_PLUGIN */
>  
>      /* TODO Move common fields from CPUArchState here. */
>      int cpu_index;


-- 
Alex Bennée

Re: [PATCH-for-8.0 1/4] cputlb: Restrict SavedIOTLB to system emulation

[Philippe Mathieu-Daudé]

On 8/12/22 09:40, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Commit 2f3a57ee47 ("cputlb: ensure we save the IOTLB data in
>> case of reset") added the SavedIOTLB structure -- which is
>> system emulation specific -- in the generic CPUState structure.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   accel/tcg/cputlb.c    | 4 ++--
>>   include/hw/core/cpu.h | 6 ++++--
>>   2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index 6f1c00682b..0ea96fbcdf 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -1395,7 +1395,7 @@ static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
>>   static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
>>                               hwaddr mr_offset)
>>   {
>> -#ifdef CONFIG_PLUGIN
>> +#if defined(CONFIG_PLUGIN) && !defined(CONFIG_USER_ONLY)
> 
> cputlb is softmmu only so I don't think we need to check CONFIG_USER_ONLY here.
Indeed, only "hw/core/cpu.h" requires it, thanks!

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

[Philippe Mathieu-Daudé]

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

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

[Fabiano Rosas]

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

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

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

[Peter Maydell]

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

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

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

[Peter Maydell]

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

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

[Philippe Mathieu-Daudé]

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!

[PATCH-for-8.0 3/4] target/cpu: Restrict cpu_get_phys_page_debug() handlers to sysemu

[Philippe Mathieu-Daudé]

cpu_get_phys_page_debug() is a system-emulation specific handler.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/alpha/cpu.h    | 2 +-
 target/cris/cpu.h     | 3 +--
 target/hppa/cpu.h     | 2 +-
 target/m68k/cpu.h     | 2 +-
 target/nios2/cpu.h    | 2 +-
 target/openrisc/cpu.h | 3 ++-
 target/ppc/cpu.h      | 2 +-
 target/rx/cpu.h       | 2 +-
 target/rx/helper.c    | 4 ++--
 target/sh4/cpu.h      | 2 +-
 target/sparc/cpu.h    | 3 ++-
 target/xtensa/cpu.h   | 2 +-
 12 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
index d0abc949a8..5e67304d81 100644
--- a/target/alpha/cpu.h
+++ b/target/alpha/cpu.h
@@ -276,9 +276,9 @@ extern const VMStateDescription vmstate_alpha_cpu;
 
 void alpha_cpu_do_interrupt(CPUState *cpu);
 bool alpha_cpu_exec_interrupt(CPUState *cpu, int int_req);
+hwaddr alpha_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 #endif /* !CONFIG_USER_ONLY */
 void alpha_cpu_dump_state(CPUState *cs, FILE *f, int flags);
-hwaddr alpha_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int alpha_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int alpha_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 
diff --git a/target/cris/cpu.h b/target/cris/cpu.h
index e6776f25b1..71fa1f96e0 100644
--- a/target/cris/cpu.h
+++ b/target/cris/cpu.h
@@ -193,12 +193,11 @@ bool cris_cpu_exec_interrupt(CPUState *cpu, int int_req);
 bool cris_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                        MMUAccessType access_type, int mmu_idx,
                        bool probe, uintptr_t retaddr);
+hwaddr cris_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 #endif
 
 void cris_cpu_dump_state(CPUState *cs, FILE *f, int flags);
 
-hwaddr cris_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
-
 int crisv10_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int cris_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int cris_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
index 6f3b6beecf..b595ef25a9 100644
--- a/target/hppa/cpu.h
+++ b/target/hppa/cpu.h
@@ -322,11 +322,11 @@ static inline void cpu_hppa_change_prot_id(CPUHPPAState *env) { }
 void cpu_hppa_change_prot_id(CPUHPPAState *env);
 #endif
 
-hwaddr hppa_cpu_get_phys_page_debug(CPUState *cs, vaddr addr);
 int hppa_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int hppa_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 void hppa_cpu_dump_state(CPUState *cs, FILE *f, int);
 #ifndef CONFIG_USER_ONLY
+hwaddr hppa_cpu_get_phys_page_debug(CPUState *cs, vaddr addr);
 bool hppa_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                        MMUAccessType access_type, int mmu_idx,
                        bool probe, uintptr_t retaddr);
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 3a9cfe2f33..68ed531fc3 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -176,9 +176,9 @@ struct ArchCPU {
 #ifndef CONFIG_USER_ONLY
 void m68k_cpu_do_interrupt(CPUState *cpu);
 bool m68k_cpu_exec_interrupt(CPUState *cpu, int int_req);
+hwaddr m68k_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 #endif /* !CONFIG_USER_ONLY */
 void m68k_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
-hwaddr m68k_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int m68k_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int m68k_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 
diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h
index f85581ee56..2f43b67a8f 100644
--- a/target/nios2/cpu.h
+++ b/target/nios2/cpu.h
@@ -262,7 +262,6 @@ void nios2_tcg_init(void);
 void nios2_cpu_do_interrupt(CPUState *cs);
 void dump_mmu(CPUNios2State *env);
 void nios2_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
-hwaddr nios2_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 G_NORETURN void nios2_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
                                               MMUAccessType access_type, int mmu_idx,
                                               uintptr_t retaddr);
@@ -288,6 +287,7 @@ static inline int cpu_mmu_index(CPUNios2State *env, bool ifetch)
 }
 
 #ifndef CONFIG_USER_ONLY
+hwaddr nios2_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 bool nios2_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                         MMUAccessType access_type, int mmu_idx,
                         bool probe, uintptr_t retaddr);
diff --git a/target/openrisc/cpu.h b/target/openrisc/cpu.h
index 1d5efa5ca2..31a4ae5ad3 100644
--- a/target/openrisc/cpu.h
+++ b/target/openrisc/cpu.h
@@ -312,7 +312,6 @@ struct ArchCPU {
 
 void cpu_openrisc_list(void);
 void openrisc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
-hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int openrisc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int openrisc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 void openrisc_translate_init(void);
@@ -321,6 +320,8 @@ int print_insn_or1k(bfd_vma addr, disassemble_info *info);
 #define cpu_list cpu_openrisc_list
 
 #ifndef CONFIG_USER_ONLY
+hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
+
 bool openrisc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                            MMUAccessType access_type, int mmu_idx,
                            bool probe, uintptr_t retaddr);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 81d4263a07..6a7a8634da 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1346,12 +1346,12 @@ static inline bool vhyp_cpu_in_nested(PowerPCCPU *cpu)
 #endif /* CONFIG_USER_ONLY */
 
 void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
-hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg);
 int ppc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
 #ifndef CONFIG_USER_ONLY
+hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu);
 const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
 #endif
diff --git a/target/rx/cpu.h b/target/rx/cpu.h
index 5655dffeff..555d230f24 100644
--- a/target/rx/cpu.h
+++ b/target/rx/cpu.h
@@ -123,11 +123,11 @@ const char *rx_crname(uint8_t cr);
 #ifndef CONFIG_USER_ONLY
 void rx_cpu_do_interrupt(CPUState *cpu);
 bool rx_cpu_exec_interrupt(CPUState *cpu, int int_req);
+hwaddr rx_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 #endif /* !CONFIG_USER_ONLY */
 void rx_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 int rx_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int rx_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-hwaddr rx_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 
 void rx_translate_init(void);
 void rx_cpu_list(void);
diff --git a/target/rx/helper.c b/target/rx/helper.c
index f34945e7e2..dad5fb4976 100644
--- a/target/rx/helper.c
+++ b/target/rx/helper.c
@@ -144,9 +144,9 @@ bool rx_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     return false;
 }
 
-#endif /* !CONFIG_USER_ONLY */
-
 hwaddr rx_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
 {
     return addr;
 }
+
+#endif /* !CONFIG_USER_ONLY */
diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h
index 727b829598..02bfd612ea 100644
--- a/target/sh4/cpu.h
+++ b/target/sh4/cpu.h
@@ -214,7 +214,6 @@ struct ArchCPU {
 
 
 void superh_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
-hwaddr superh_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int superh_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int superh_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 G_NORETURN void superh_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
@@ -225,6 +224,7 @@ void sh4_translate_init(void);
 void sh4_cpu_list(void);
 
 #if !defined(CONFIG_USER_ONLY)
+hwaddr superh_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 bool superh_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                          MMUAccessType access_type, int mmu_idx,
                          bool probe, uintptr_t retaddr);
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index e478c5eb16..ed0069d0b1 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -569,10 +569,11 @@ struct ArchCPU {
 
 #ifndef CONFIG_USER_ONLY
 extern const VMStateDescription vmstate_sparc_cpu;
+
+hwaddr sparc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 #endif
 
 void sparc_cpu_do_interrupt(CPUState *cpu);
-hwaddr sparc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int sparc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int sparc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 G_NORETURN void sparc_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 579adcb769..b7a54711a6 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -576,9 +576,9 @@ void xtensa_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
                                       unsigned size, MMUAccessType access_type,
                                       int mmu_idx, MemTxAttrs attrs,
                                       MemTxResult response, uintptr_t retaddr);
+hwaddr xtensa_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 #endif
 void xtensa_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
-hwaddr xtensa_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 void xtensa_count_regs(const XtensaConfig *config,
                        unsigned *n_regs, unsigned *n_core_regs);
 int xtensa_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
-- 
2.38.1

[PATCH-for-8.0 4/4] target/sparc: Cleanup around sparc_cpu_do_unaligned_access()

[Philippe Mathieu-Daudé]

Commit caac44a52a ("target/sparc: Make sparc_cpu_tlb_fill sysemu
only") restricted mmu_helper.c to system emulation. Checking
whether CONFIG_USER_ONLY is defined is now pointless.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/sparc/mmu_helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index 919448a494..a7e51e4b7d 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -924,7 +924,6 @@ hwaddr sparc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
     return phys_addr;
 }
 
-#ifndef CONFIG_USER_ONLY
 G_NORETURN void sparc_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
                                               MMUAccessType access_type,
                                               int mmu_idx,
@@ -942,4 +941,3 @@ G_NORETURN void sparc_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
 
     cpu_raise_exception_ra(env, TT_UNALIGNED, retaddr);
 }
-#endif /* !CONFIG_USER_ONLY */
-- 
2.38.1