[RFC PATCH 0/3] target/arm/internals: Replace some target_ulong uses

[RFC PATCH 0/3] target/arm/internals: Replace some target_ulong uses

[Philippe Mathieu-Daudé]

Hi

This series replace some target_ulong types in ARMMMUFaultInfo
and some methods by appropriate target agnostic type.

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  target/arm: Replace target_ulong -> hwaddr in ARMMMUFaultInfo
  target/arm: Replace target_ulong -> uint64_t for HWBreakpoint
  target/arm: Replace target_ulong -> vaddr for CPUWatchpoint

 target/arm/internals.h   | 20 +++++++++++---------
 target/arm/hyp_gdbstub.c | 14 +++++++-------
 2 files changed, 18 insertions(+), 16 deletions(-)

-- 
2.47.1

[RFC PATCH 1/3] target/arm: Replace target_ulong -> hwaddr in ARMMMUFaultInfo

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/internals.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 28585c07555..175fb792375 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -25,6 +25,7 @@
 #ifndef TARGET_ARM_INTERNALS_H
 #define TARGET_ARM_INTERNALS_H
 
+#include "exec/hwaddr.h"
 #include "exec/breakpoint.h"
 #include "hw/registerfields.h"
 #include "tcg/tcg-gvec-desc.h"
@@ -724,8 +725,8 @@ typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
 struct ARMMMUFaultInfo {
     ARMFaultType type;
     ARMGPCF gpcf;
-    target_ulong s2addr;
-    target_ulong paddr;
+    hwaddr s2addr;
+    hwaddr paddr;
     ARMSecuritySpace paddr_space;
     int level;
     int domain;
-- 
2.47.1

[RFC PATCH 2/3] target/arm: Replace target_ulong -> uint64_t for HWBreakpoint

[Philippe Mathieu-Daudé]

CPUARMState::pc is of type uint64_t.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/internals.h   | 6 +++---
 target/arm/hyp_gdbstub.c | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 175fb792375..05ceb9bcf5d 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1942,9 +1942,9 @@ extern GArray *hw_breakpoints, *hw_watchpoints;
 #define get_hw_bp(i)    (&g_array_index(hw_breakpoints, HWBreakpoint, i))
 #define get_hw_wp(i)    (&g_array_index(hw_watchpoints, HWWatchpoint, i))
 
-bool find_hw_breakpoint(CPUState *cpu, target_ulong pc);
-int insert_hw_breakpoint(target_ulong pc);
-int delete_hw_breakpoint(target_ulong pc);
+bool find_hw_breakpoint(CPUState *cpu, uint64_t pc);
+int insert_hw_breakpoint(uint64_t pc);
+int delete_hw_breakpoint(uint64_t pc);
 
 bool check_watchpoint_in_range(int i, target_ulong addr);
 CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr);
diff --git a/target/arm/hyp_gdbstub.c b/target/arm/hyp_gdbstub.c
index 1e861263b3d..4e52d37ed80 100644
--- a/target/arm/hyp_gdbstub.c
+++ b/target/arm/hyp_gdbstub.c
@@ -54,7 +54,7 @@ GArray *hw_breakpoints, *hw_watchpoints;
  * here so future PC comparisons will work properly.
  */
 
-int insert_hw_breakpoint(target_ulong addr)
+int insert_hw_breakpoint(uint64_t addr)
 {
     HWBreakpoint brk = {
         .bcr = 0x1,                             /* BCR E=1, enable */
@@ -80,7 +80,7 @@ int insert_hw_breakpoint(target_ulong addr)
  * Delete a breakpoint and shuffle any above down
  */
 
-int delete_hw_breakpoint(target_ulong pc)
+int delete_hw_breakpoint(uint64_t pc)
 {
     int i;
     for (i = 0; i < hw_breakpoints->len; i++) {
@@ -226,7 +226,7 @@ int delete_hw_watchpoint(target_ulong addr, target_ulong len, int type)
     return -ENOENT;
 }
 
-bool find_hw_breakpoint(CPUState *cpu, target_ulong pc)
+bool find_hw_breakpoint(CPUState *cpu, uint64_t pc)
 {
     int i;
 
-- 
2.47.1

[RFC PATCH 3/3] target/arm: Replace target_ulong -> vaddr for CPUWatchpoint

[Philippe Mathieu-Daudé]

CPUWatchpoint::vaddr/len are of type vaddr.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/internals.h   | 9 +++++----
 target/arm/hyp_gdbstub.c | 8 ++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 05ceb9bcf5d..a19c805af45 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -26,6 +26,7 @@
 #define TARGET_ARM_INTERNALS_H
 
 #include "exec/hwaddr.h"
+#include "exec/vaddr.h"
 #include "exec/breakpoint.h"
 #include "hw/registerfields.h"
 #include "tcg/tcg-gvec-desc.h"
@@ -1946,10 +1947,10 @@ bool find_hw_breakpoint(CPUState *cpu, uint64_t pc);
 int insert_hw_breakpoint(uint64_t pc);
 int delete_hw_breakpoint(uint64_t pc);
 
-bool check_watchpoint_in_range(int i, target_ulong addr);
-CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr);
-int insert_hw_watchpoint(target_ulong addr, target_ulong len, int type);
-int delete_hw_watchpoint(target_ulong addr, target_ulong len, int type);
+bool check_watchpoint_in_range(int i, vaddr addr);
+CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, vaddr addr);
+int insert_hw_watchpoint(vaddr addr, vaddr len, int type);
+int delete_hw_watchpoint(vaddr addr, vaddr len, int type);
 
 /* Return the current value of the system counter in ticks */
 uint64_t gt_get_countervalue(CPUARMState *env);
diff --git a/target/arm/hyp_gdbstub.c b/target/arm/hyp_gdbstub.c
index 4e52d37ed80..4d8fd933868 100644
--- a/target/arm/hyp_gdbstub.c
+++ b/target/arm/hyp_gdbstub.c
@@ -125,7 +125,7 @@ int delete_hw_breakpoint(uint64_t pc)
  * need to ensure you mask the address as required and set BAS=0xff
  */
 
-int insert_hw_watchpoint(target_ulong addr, target_ulong len, int type)
+int insert_hw_watchpoint(vaddr addr, vaddr len, int type)
 {
     HWWatchpoint wp = {
         .wcr = R_DBGWCR_E_MASK, /* E=1, enable */
@@ -182,7 +182,7 @@ int insert_hw_watchpoint(target_ulong addr, target_ulong len, int type)
     return 0;
 }
 
-bool check_watchpoint_in_range(int i, target_ulong addr)
+bool check_watchpoint_in_range(int i, vaddr addr)
 {
     HWWatchpoint *wp = get_hw_wp(i);
     uint64_t addr_top, addr_bottom = wp->wvr;
@@ -214,7 +214,7 @@ bool check_watchpoint_in_range(int i, target_ulong addr)
  * Delete a breakpoint and shuffle any above down
  */
 
-int delete_hw_watchpoint(target_ulong addr, target_ulong len, int type)
+int delete_hw_watchpoint(vaddr addr, vaddr len, int type)
 {
     int i;
     for (i = 0; i < cur_hw_wps; i++) {
@@ -239,7 +239,7 @@ bool find_hw_breakpoint(CPUState *cpu, uint64_t pc)
     return false;
 }
 
-CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
+CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, vaddr addr)
 {
     int i;
 
-- 
2.47.1

Re: [RFC PATCH 1/3] target/arm: Replace target_ulong -> hwaddr in ARMMMUFaultInfo

[Pierrick Bouvier]

On 4/15/25 10:22, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/internals.h | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 28585c07555..175fb792375 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -25,6 +25,7 @@
>   #ifndef TARGET_ARM_INTERNALS_H
>   #define TARGET_ARM_INTERNALS_H
>   
> +#include "exec/hwaddr.h"
>   #include "exec/breakpoint.h"
>   #include "hw/registerfields.h"
>   #include "tcg/tcg-gvec-desc.h"
> @@ -724,8 +725,8 @@ typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
>   struct ARMMMUFaultInfo {
>       ARMFaultType type;
>       ARMGPCF gpcf;
> -    target_ulong s2addr;
> -    target_ulong paddr;
> +    hwaddr s2addr;
> +    hwaddr paddr;
>       ARMSecuritySpace paddr_space;
>       int level;
>       int domain;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [RFC PATCH 2/3] target/arm: Replace target_ulong -> uint64_t for HWBreakpoint

[Pierrick Bouvier]

On 4/15/25 10:22, Philippe Mathieu-Daudé wrote:
> CPUARMState::pc is of type uint64_t.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/internals.h   | 6 +++---
>   target/arm/hyp_gdbstub.c | 6 +++---
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 175fb792375..05ceb9bcf5d 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1942,9 +1942,9 @@ extern GArray *hw_breakpoints, *hw_watchpoints;
>   #define get_hw_bp(i)    (&g_array_index(hw_breakpoints, HWBreakpoint, i))
>   #define get_hw_wp(i)    (&g_array_index(hw_watchpoints, HWWatchpoint, i))
>   
> -bool find_hw_breakpoint(CPUState *cpu, target_ulong pc);
> -int insert_hw_breakpoint(target_ulong pc);
> -int delete_hw_breakpoint(target_ulong pc);
> +bool find_hw_breakpoint(CPUState *cpu, uint64_t pc);
> +int insert_hw_breakpoint(uint64_t pc);
> +int delete_hw_breakpoint(uint64_t pc);
>   
>   bool check_watchpoint_in_range(int i, target_ulong addr);
>   CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr);
> diff --git a/target/arm/hyp_gdbstub.c b/target/arm/hyp_gdbstub.c
> index 1e861263b3d..4e52d37ed80 100644
> --- a/target/arm/hyp_gdbstub.c
> +++ b/target/arm/hyp_gdbstub.c
> @@ -54,7 +54,7 @@ GArray *hw_breakpoints, *hw_watchpoints;
>    * here so future PC comparisons will work properly.
>    */
>   
> -int insert_hw_breakpoint(target_ulong addr)
> +int insert_hw_breakpoint(uint64_t addr)
>   {
>       HWBreakpoint brk = {
>           .bcr = 0x1,                             /* BCR E=1, enable */
> @@ -80,7 +80,7 @@ int insert_hw_breakpoint(target_ulong addr)
>    * Delete a breakpoint and shuffle any above down
>    */
>   
> -int delete_hw_breakpoint(target_ulong pc)
> +int delete_hw_breakpoint(uint64_t pc)
>   {
>       int i;
>       for (i = 0; i < hw_breakpoints->len; i++) {
> @@ -226,7 +226,7 @@ int delete_hw_watchpoint(target_ulong addr, target_ulong len, int type)
>       return -ENOENT;
>   }
>   
> -bool find_hw_breakpoint(CPUState *cpu, target_ulong pc)
> +bool find_hw_breakpoint(CPUState *cpu, uint64_t pc)
>   {
>       int i;
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [RFC PATCH 3/3] target/arm: Replace target_ulong -> vaddr for CPUWatchpoint

[Pierrick Bouvier]

On 4/15/25 10:22, Philippe Mathieu-Daudé wrote:
> CPUWatchpoint::vaddr/len are of type vaddr.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/internals.h   | 9 +++++----
>   target/arm/hyp_gdbstub.c | 8 ++++----
>   2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 05ceb9bcf5d..a19c805af45 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -26,6 +26,7 @@
>   #define TARGET_ARM_INTERNALS_H
>   
>   #include "exec/hwaddr.h"
> +#include "exec/vaddr.h"
>   #include "exec/breakpoint.h"
>   #include "hw/registerfields.h"
>   #include "tcg/tcg-gvec-desc.h"
> @@ -1946,10 +1947,10 @@ bool find_hw_breakpoint(CPUState *cpu, uint64_t pc);
>   int insert_hw_breakpoint(uint64_t pc);
>   int delete_hw_breakpoint(uint64_t pc);
>   
> -bool check_watchpoint_in_range(int i, target_ulong addr);
> -CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr);
> -int insert_hw_watchpoint(target_ulong addr, target_ulong len, int type);
> -int delete_hw_watchpoint(target_ulong addr, target_ulong len, int type);
> +bool check_watchpoint_in_range(int i, vaddr addr);
> +CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, vaddr addr);
> +int insert_hw_watchpoint(vaddr addr, vaddr len, int type);
> +int delete_hw_watchpoint(vaddr addr, vaddr len, int type);
>   
>   /* Return the current value of the system counter in ticks */
>   uint64_t gt_get_countervalue(CPUARMState *env);
> diff --git a/target/arm/hyp_gdbstub.c b/target/arm/hyp_gdbstub.c
> index 4e52d37ed80..4d8fd933868 100644
> --- a/target/arm/hyp_gdbstub.c
> +++ b/target/arm/hyp_gdbstub.c
> @@ -125,7 +125,7 @@ int delete_hw_breakpoint(uint64_t pc)
>    * need to ensure you mask the address as required and set BAS=0xff
>    */
>   
> -int insert_hw_watchpoint(target_ulong addr, target_ulong len, int type)
> +int insert_hw_watchpoint(vaddr addr, vaddr len, int type)
>   {
>       HWWatchpoint wp = {
>           .wcr = R_DBGWCR_E_MASK, /* E=1, enable */
> @@ -182,7 +182,7 @@ int insert_hw_watchpoint(target_ulong addr, target_ulong len, int type)
>       return 0;
>   }
>   
> -bool check_watchpoint_in_range(int i, target_ulong addr)
> +bool check_watchpoint_in_range(int i, vaddr addr)
>   {
>       HWWatchpoint *wp = get_hw_wp(i);
>       uint64_t addr_top, addr_bottom = wp->wvr;
> @@ -214,7 +214,7 @@ bool check_watchpoint_in_range(int i, target_ulong addr)
>    * Delete a breakpoint and shuffle any above down
>    */
>   
> -int delete_hw_watchpoint(target_ulong addr, target_ulong len, int type)
> +int delete_hw_watchpoint(vaddr addr, vaddr len, int type)
>   {
>       int i;
>       for (i = 0; i < cur_hw_wps; i++) {
> @@ -239,7 +239,7 @@ bool find_hw_breakpoint(CPUState *cpu, uint64_t pc)
>       return false;
>   }
>   
> -CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
> +CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, vaddr addr)
>   {
>       int i;
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [RFC PATCH 2/3] target/arm: Replace target_ulong -> uint64_t for HWBreakpoint

[Richard Henderson]

On 4/15/25 10:22, Philippe Mathieu-Daudé wrote:
> CPUARMState::pc is of type uint64_t.

That's not a good argument.  It's a guest virtual address, and using vaddr would 
self-document that fact.


r~

Re: [RFC PATCH 1/3] target/arm: Replace target_ulong -> hwaddr in ARMMMUFaultInfo

[Richard Henderson]

On 4/15/25 10:22, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/internals.h | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 28585c07555..175fb792375 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -25,6 +25,7 @@
>   #ifndef TARGET_ARM_INTERNALS_H
>   #define TARGET_ARM_INTERNALS_H
>   
> +#include "exec/hwaddr.h"
>   #include "exec/breakpoint.h"
>   #include "hw/registerfields.h"
>   #include "tcg/tcg-gvec-desc.h"
> @@ -724,8 +725,8 @@ typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
>   struct ARMMMUFaultInfo {
>       ARMFaultType type;
>       ARMGPCF gpcf;
> -    target_ulong s2addr;
> -    target_ulong paddr;
> +    hwaddr s2addr;
> +    hwaddr paddr;
>       ARMSecuritySpace paddr_space;
>       int level;
>       int domain;

Re: [RFC PATCH 3/3] target/arm: Replace target_ulong -> vaddr for CPUWatchpoint

[Richard Henderson]

On 4/15/25 10:22, Philippe Mathieu-Daudé wrote:
> CPUWatchpoint::vaddr/len are of type vaddr.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/internals.h   | 9 +++++----
>   target/arm/hyp_gdbstub.c | 8 ++++----
>   2 files changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH 2/3] target/arm: Replace target_ulong -> uint64_t for HWBreakpoint

[Pierrick Bouvier]

On 4/16/25 10:05, Richard Henderson wrote:
> On 4/15/25 10:22, Philippe Mathieu-Daudé wrote:
>> CPUARMState::pc is of type uint64_t.
> 
> That's not a good argument.  It's a guest virtual address, and using vaddr would
> self-document that fact.
>

Ideally, should we update CPUARMState::pc also?

> 
> r~

Re: [RFC PATCH 2/3] target/arm: Replace target_ulong -> uint64_t for HWBreakpoint

[Richard Henderson]

On 4/16/25 10:13, Pierrick Bouvier wrote:
> On 4/16/25 10:05, Richard Henderson wrote:
>> On 4/15/25 10:22, Philippe Mathieu-Daudé wrote:
>>> CPUARMState::pc is of type uint64_t.
>>
>> That's not a good argument.  It's a guest virtual address, and using vaddr would
>> self-document that fact.
>>
> 
> Ideally, should we update CPUARMState::pc also?

I don't think so, no.

r~