Zero is the safe do-nothing value for callers to use.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h | 3 ++-
 target/arm/helper.c    | 4 ++--
 target/arm/ptw.c       | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 2b16579fa5..a6088d551c 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1461,6 +1461,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address,
  * @env: CPUARMState
  * @address: virtual address to get physical address for
  * @access_type: 0 for read, 1 for write, 2 for execute
+ * @memop: memory operation feeding this access, or 0 for none
  * @mmu_idx: MMU index indicating required translation regime
  * @space: security space for the access
  * @result: set on translation success.
@@ -1470,7 +1471,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address,
  * a Granule Protection Check on the resulting address.
  */
 bool get_phys_addr_with_space_nogpc(CPUARMState *env, vaddr address,
-                                    MMUAccessType access_type,
+                                    MMUAccessType access_type, MemOp memop,
                                     ARMMMUIdx mmu_idx, ARMSecuritySpace space,
                                     GetPhysAddrResult *result,
                                     ARMMMUFaultInfo *fi)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3f77b40734..f2f329e00a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3602,8 +3602,8 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
      * I_MXTJT: Granule protection checks are not performed on the final address
      * of a successful translation.
      */
-    ret = get_phys_addr_with_space_nogpc(env, value, access_type, mmu_idx, ss,
-                                         &res, &fi);
+    ret = get_phys_addr_with_space_nogpc(env, value, access_type, 0,
+                                         mmu_idx, ss, &res, &fi);
 
     /*
      * ATS operations only do S1 or S1+S2 translations, so we never
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 373095a339..9af86da597 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -3559,7 +3559,7 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
 }
 
 bool get_phys_addr_with_space_nogpc(CPUARMState *env, vaddr address,
-                                    MMUAccessType access_type,
+                                    MMUAccessType access_type, MemOp memop,
                                     ARMMMUIdx mmu_idx, ARMSecuritySpace space,
                                     GetPhysAddrResult *result,
                                     ARMMMUFaultInfo *fi)
-- 
2.43.0

On Sat, 5 Oct 2024 at 21:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Zero is the safe do-nothing value for callers to use.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h | 3 ++-
>  target/arm/helper.c    | 4 ++--
>  target/arm/ptw.c       | 2 +-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 2b16579fa5..a6088d551c 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1461,6 +1461,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address,
>   * @env: CPUARMState
>   * @address: virtual address to get physical address for
>   * @access_type: 0 for read, 1 for write, 2 for execute
> + * @memop: memory operation feeding this access, or 0 for none
>   * @mmu_idx: MMU index indicating required translation regime
>   * @space: security space for the access
>   * @result: set on translation success.
> @@ -1470,7 +1471,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address,
>   * a Granule Protection Check on the resulting address.
>   */
>  bool get_phys_addr_with_space_nogpc(CPUARMState *env, vaddr address,
> -                                    MMUAccessType access_type,
> +                                    MMUAccessType access_type, MemOp memop,
>                                      ARMMMUIdx mmu_idx, ARMSecuritySpace space,
>                                      GetPhysAddrResult *result,
>                                      ARMMMUFaultInfo *fi)
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3f77b40734..f2f329e00a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3602,8 +3602,8 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>       * I_MXTJT: Granule protection checks are not performed on the final address
>       * of a successful translation.
>       */
> -    ret = get_phys_addr_with_space_nogpc(env, value, access_type, mmu_idx, ss,
> -                                         &res, &fi);
> +    ret = get_phys_addr_with_space_nogpc(env, value, access_type, 0,
> +                                         mmu_idx, ss, &res, &fi);

0 is the correct thing here, because AT operations are effectively
assuming an aligned address (cf AArch64.AT() setting "aligned = true"
in the Arm ARM pseudocode).

Is there a way to write this as something other than "0" as
an indication of "we've definitely thought about this callsite
and it's not just 0 for back-compat behaviour" ? Otherwise we
could add a brief comment.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

On 10/8/24 07:35, Peter Maydell wrote:
>> -    ret = get_phys_addr_with_space_nogpc(env, value, access_type, mmu_idx, ss,
>> -                                         &res, &fi);
>> +    ret = get_phys_addr_with_space_nogpc(env, value, access_type, 0,
>> +                                         mmu_idx, ss, &res, &fi);
> 
> 0 is the correct thing here, because AT operations are effectively
> assuming an aligned address (cf AArch64.AT() setting "aligned = true"
> in the Arm ARM pseudocode).
> 
> Is there a way to write this as something other than "0" as
> an indication of "we've definitely thought about this callsite
> and it's not just 0 for back-compat behaviour" ? Otherwise we
> could add a brief comment.

Nothing other than 0 is leaping to mind.
The documentation for @memop includes "... or 0 for none".

Perhaps

   /* This is a translation not a memory reference, so "memop = none = 0". */


r~

On 10/5/24 22:05, Richard Henderson wrote:
> Zero is the safe do-nothing value for callers to use.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


Reviewed-by: Helge Deller <deller@gmx.de>

> ---
>   target/arm/internals.h | 3 ++-
>   target/arm/helper.c    | 4 ++--
>   target/arm/ptw.c       | 2 +-
>   3 files changed, 5 insertions(+), 4 deletions(-)