Re: [PATCH v3 33/42] target/arm: Split out S1TranslateResult type

[Peter Maydell]

On Sat, 1 Oct 2022 at 17:56, Richard Henderson
 wrote:
>
> Consolidate the results of S1_ptw_translate in one struct.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/ptw.c | 70 +---
>  1 file changed, 36 insertions(+), 34 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 7a77bea2c7..99ad894180 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -220,13 +220,18 @@ static bool S2_attrs_are_device(uint64_t hcr, uint8_t 
> attrs)
>  }
>  }
>
> +typedef struct {
> +bool is_secure;
> +void *hphys;
> +hwaddr gphys;
> +} S1TranslateResult;

Ah, I was wondering whether to suggest this for the previous patch
that added the hphys and gphys arguments :-)

Reviewed-by: Peter Maydell 

thanks
-- PMM

[PATCH v3 33/42] target/arm: Split out S1TranslateResult type

[Richard Henderson]

Consolidate the results of S1_ptw_translate in one struct.

Signed-off-by: Richard Henderson 
---
 target/arm/ptw.c | 70 +---
 1 file changed, 36 insertions(+), 34 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 7a77bea2c7..99ad894180 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -220,13 +220,18 @@ static bool S2_attrs_are_device(uint64_t hcr, uint8_t 
attrs)
 }
 }
 
+typedef struct {
+bool is_secure;
+void *hphys;
+hwaddr gphys;
+} S1TranslateResult;
+
 /* Translate a S1 pagetable walk through S2 if needed.  */
 static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
  ARMMMUIdx s2_mmu_idx, hwaddr addr,
- bool *is_secure_ptr, void **hphys, hwaddr *gphys,
- bool debug, ARMMMUFaultInfo *fi)
+ bool is_secure, bool debug,
+ S1TranslateResult *res, ARMMMUFaultInfo *fi)
 {
-bool is_secure = *is_secure_ptr;
 uint8_t pte_attrs;
 bool s2_phys, pte_secure;
 
@@ -238,7 +243,7 @@ static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx 
mmu_idx,
  * state of the cpu at all, including softmmu tlb contents.
  */
 if (s2_phys) {
-*gphys = addr;
+res->gphys = addr;
 pte_attrs = 0;
 pte_secure = is_secure;
 } else {
@@ -251,11 +256,11 @@ static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx 
mmu_idx,
 &s2, fi)) {
 goto fail;
 }
-*gphys = s2.f.phys_addr;
+res->gphys = s2.f.phys_addr;
 pte_attrs = s2.cacheattrs.attrs;
 pte_secure = s2.f.attrs.secure;
 }
-*hphys = NULL;
+res->hphys = NULL;
 } else {
 CPUTLBEntryFull *full;
 int flags;
@@ -263,13 +268,13 @@ static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx 
mmu_idx,
 env->tlb_fi = fi;
 flags = probe_access_full(env, addr, MMU_DATA_LOAD,
   arm_to_core_mmu_idx(s2_mmu_idx),
-  true, hphys, &full, 0);
+  true, &res->hphys, &full, 0);
 env->tlb_fi = NULL;
 
 if (unlikely(flags & TLB_INVALID_MASK)) {
 goto fail;
 }
-*gphys = full->phys_addr;
+res->gphys = full->phys_addr;
 pte_attrs = full->pte_attrs;
 pte_secure = full->attrs.secure;
 }
@@ -291,12 +296,11 @@ static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx 
mmu_idx,
 }
 }
 
-if (is_secure) {
-/* Check if page table walk is to secure or non-secure PA space. */
-*is_secure_ptr = !(pte_secure
-   ? env->cp15.vstcr_el2 & VSTCR_SW
-   : env->cp15.vtcr_el2 & VTCR_NSW);
-}
+/* Check if page table walk is to secure or non-secure PA space. */
+res->is_secure = (is_secure &&
+  !(pte_secure
+? env->cp15.vstcr_el2 & VSTCR_SW
+: env->cp15.vtcr_el2 & VTCR_NSW));
 return true;
 
  fail:
@@ -314,36 +318,35 @@ static uint32_t arm_ldl_ptw(CPUARMState *env, hwaddr 
addr, bool is_secure,
 bool debug, ARMMMUFaultInfo *fi)
 {
 CPUState *cs = env_cpu(env);
-void *hphys;
-hwaddr gphys;
+S1TranslateResult s1;
 uint32_t data;
 bool be;
 
-if (!S1_ptw_translate(env, mmu_idx, ptw_idx, addr, &is_secure,
-  &hphys, &gphys, debug, fi)) {
+if (!S1_ptw_translate(env, mmu_idx, ptw_idx, addr, is_secure,
+  debug, &s1, fi)) {
 /* Failure. */
 assert(fi->s1ptw);
 return 0;
 }
 
 be = regime_translation_big_endian(env, mmu_idx);
-if (likely(hphys)) {
+if (likely(s1.hphys)) {
 /* Page tables are in RAM, and we have the host address. */
 if (be) {
-data = ldl_be_p(hphys);
+data = ldl_be_p(s1.hphys);
 } else {
-data = ldl_le_p(hphys);
+data = ldl_le_p(s1.hphys);
 }
 } else {
 /* Page tables are in MMIO. */
-MemTxAttrs attrs = { .secure = is_secure };
+MemTxAttrs attrs = { .secure = s1.is_secure };
 AddressSpace *as = arm_addressspace(cs, attrs);
 MemTxResult result = MEMTX_OK;
 
 if (be) {
-data = address_space_ldl_be(as, gphys, attrs, &result);
+data = address_space_ldl_be(as, s1.gphys, attrs, &result);
 } else {
-data = address_space_ldl_le(as, gphys, attrs, &result);
+data = address_space_ldl_le(as, s1.gphys, attrs, &result);
 }
 if (unlikely(result != MEMTX_OK)) {
 fi->type = ARMFault_SyncExternalOnWalk;
@@ -359,36 +362,35 @@ static uint64_t arm_ldq_ptw(CPUARMState *env