[08/62] target/arm: Create GetPhysAddrResult

Combine 5 output pointer argument from get_phys_addr
into a single struct.  Adjust all callers.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h  |  13 ++++-
 target/arm/helper.c     |  27 ++++-----
 target/arm/m_helper.c   |  52 +++++------------
 target/arm/ptw.c        | 125 +++++++++++++++++++++-------------------
 target/arm/tlb_helper.c |  26 ++++-----
 5 files changed, 113 insertions(+), 130 deletions(-)

Richard Henderson <richard.henderson@linaro.org> writes:

> Combine 5 output pointer argument from get_phys_addr
> into a single struct.  Adjust all callers.

This looks to be an improvement - I guess the real benefit is the
compiler isn't jamming so many closely aligned pointers on the stack
frame for all the return values?

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h  |  13 ++++-
>  target/arm/helper.c     |  27 ++++-----
>  target/arm/m_helper.c   |  52 +++++------------
>  target/arm/ptw.c        | 125 +++++++++++++++++++++-------------------
>  target/arm/tlb_helper.c |  26 ++++-----
>  5 files changed, 113 insertions(+), 130 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 268c3c7380..7d08917f88 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1133,11 +1133,18 @@ typedef struct ARMCacheAttrs {
>      bool guarded:1;              /* guarded bit of the v8-64 PTE */
>  } ARMCacheAttrs;
>  
> +/* Fields that are valid upon success. */
> +typedef struct GetPhysAddrResult {
> +    hwaddr phys;
> +    target_ulong page_size;
> +    int prot;
> +    MemTxAttrs attrs;
> +    ARMCacheAttrs cacheattrs;
> +} GetPhysAddrResult;
> +
>  bool get_phys_addr(CPUARMState *env, target_ulong address,
>                     MMUAccessType access_type, ARMMMUIdx mmu_idx,
> -                   hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> -                   target_ulong *page_size,
> -                   ARMMMUFaultInfo *fi, ARMCacheAttrs *cacheattrs)
> +                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
>      __attribute__((nonnull));
>  
>  void arm_log_exception(CPUState *cs);
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f6dcb1a115..fb13e0f4c0 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3173,24 +3173,19 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
>  static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>                               MMUAccessType access_type, ARMMMUIdx mmu_idx)
>  {
> -    hwaddr phys_addr;
> -    target_ulong page_size;
> -    int prot;
>      bool ret;
>      uint64_t par64;
>      bool format64 = false;
> -    MemTxAttrs attrs = {};
>      ARMMMUFaultInfo fi = {};
> -    ARMCacheAttrs cacheattrs = {};
> +    GetPhysAddrResult res = {};
>  
> -    ret = get_phys_addr(env, value, access_type, mmu_idx, &phys_addr, &attrs,
> -                        &prot, &page_size, &fi, &cacheattrs);
> +    ret = get_phys_addr(env, value, access_type, mmu_idx, &res, &fi);
>  
>      /*
>       * ATS operations only do S1 or S1+S2 translations, so we never
>       * have to deal with the ARMCacheAttrs format for S2 only.
>       */
> -    assert(!cacheattrs.is_s2_format);
> +    assert(!res.cacheattrs.is_s2_format);
>  
>      if (ret) {
>          /*
> @@ -3296,12 +3291,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>          /* Create a 64-bit PAR */
>          par64 = (1 << 11); /* LPAE bit always set */
>          if (!ret) {
> -            par64 |= phys_addr & ~0xfffULL;
> -            if (!attrs.secure) {
> +            par64 |= res.phys & ~0xfffULL;
> +            if (!res.attrs.secure) {
>                  par64 |= (1 << 9); /* NS */
>              }
> -            par64 |= (uint64_t)cacheattrs.attrs << 56; /* ATTR */
> -            par64 |= cacheattrs.shareability << 7; /* SH */
> +            par64 |= (uint64_t)res.cacheattrs.attrs << 56; /* ATTR */
> +            par64 |= res.cacheattrs.shareability << 7; /* SH */
>          } else {
>              uint32_t fsr = arm_fi_to_lfsc(&fi);
>  
> @@ -3321,13 +3316,13 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>           */
>          if (!ret) {
>              /* We do not set any attribute bits in the PAR */
> -            if (page_size == (1 << 24)
> +            if (res.page_size == (1 << 24)
>                  && arm_feature(env, ARM_FEATURE_V7)) {
> -                par64 = (phys_addr & 0xff000000) | (1 << 1);
> +                par64 = (res.phys & 0xff000000) | (1 << 1);
>              } else {
> -                par64 = phys_addr & 0xfffff000;
> +                par64 = res.phys & 0xfffff000;
>              }
> -            if (!attrs.secure) {
> +            if (!res.attrs.secure) {
>                  par64 |= (1 << 9); /* NS */
>              }
>          } else {
> diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
> index 308610f6b4..84c6796b8d 100644
> --- a/target/arm/m_helper.c
> +++ b/target/arm/m_helper.c
> @@ -183,19 +183,14 @@ static bool v7m_stack_write(ARMCPU *cpu, uint32_t addr, uint32_t value,
>  {
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
> -    MemTxAttrs attrs = {};
>      MemTxResult txres;
> -    target_ulong page_size;
> -    hwaddr physaddr;
> -    int prot;
> +    GetPhysAddrResult res = {};
>      ARMMMUFaultInfo fi = {};
> -    ARMCacheAttrs cacheattrs = {};
>      bool secure = mmu_idx & ARM_MMU_IDX_M_S;
>      int exc;
>      bool exc_secure;
>  
> -    if (get_phys_addr(env, addr, MMU_DATA_STORE, mmu_idx, &physaddr,
> -                      &attrs, &prot, &page_size, &fi, &cacheattrs)) {
> +    if (get_phys_addr(env, addr, MMU_DATA_STORE, mmu_idx, &res, &fi)) {
>          /* MPU/SAU lookup failed */
>          if (fi.type == ARMFault_QEMU_SFault) {
>              if (mode == STACK_LAZYFP) {
> @@ -228,8 +223,8 @@ static bool v7m_stack_write(ARMCPU *cpu, uint32_t addr, uint32_t value,
>          }
>          goto pend_fault;
>      }
> -    address_space_stl_le(arm_addressspace(cs, attrs), physaddr, value,
> -                         attrs, &txres);
> +    address_space_stl_le(arm_addressspace(cs, res.attrs), res.phys, value,
> +                         res.attrs, &txres);
>      if (txres != MEMTX_OK) {
>          /* BusFault trying to write the data */
>          if (mode == STACK_LAZYFP) {
> @@ -276,20 +271,15 @@ static bool v7m_stack_read(ARMCPU *cpu, uint32_t *dest, uint32_t addr,
>  {
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
> -    MemTxAttrs attrs = {};
>      MemTxResult txres;
> -    target_ulong page_size;
> -    hwaddr physaddr;
> -    int prot;
> +    GetPhysAddrResult res = {};
>      ARMMMUFaultInfo fi = {};
> -    ARMCacheAttrs cacheattrs = {};
>      bool secure = mmu_idx & ARM_MMU_IDX_M_S;
>      int exc;
>      bool exc_secure;
>      uint32_t value;
>  
> -    if (get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &physaddr,
> -                      &attrs, &prot, &page_size, &fi, &cacheattrs)) {
> +    if (get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &res, &fi)) {
>          /* MPU/SAU lookup failed */
>          if (fi.type == ARMFault_QEMU_SFault) {
>              qemu_log_mask(CPU_LOG_INT,
> @@ -308,8 +298,8 @@ static bool v7m_stack_read(ARMCPU *cpu, uint32_t *dest, uint32_t addr,
>          goto pend_fault;
>      }
>  
> -    value = address_space_ldl(arm_addressspace(cs, attrs), physaddr,
> -                              attrs, &txres);
> +    value = address_space_ldl(arm_addressspace(cs, res.attrs), res.phys,
> +                              res.attrs, &txres);
>      if (txres != MEMTX_OK) {
>          /* BusFault trying to read the data */
>          qemu_log_mask(CPU_LOG_INT, "...BusFault with BFSR.UNSTKERR\n");
> @@ -2008,13 +1998,9 @@ static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx mmu_idx,
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
>      V8M_SAttributes sattrs = {};
> -    MemTxAttrs attrs = {};
> +    GetPhysAddrResult res = {};
>      ARMMMUFaultInfo fi = {};
> -    ARMCacheAttrs cacheattrs = {};
>      MemTxResult txres;
> -    target_ulong page_size;
> -    hwaddr physaddr;
> -    int prot;
>  
>      v8m_security_lookup(env, addr, MMU_INST_FETCH, mmu_idx, &sattrs);
>      if (!sattrs.nsc || sattrs.ns) {
> @@ -2028,16 +2014,15 @@ static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx mmu_idx,
>                        "...really SecureFault with SFSR.INVEP\n");
>          return false;
>      }
> -    if (get_phys_addr(env, addr, MMU_INST_FETCH, mmu_idx, &physaddr,
> -                      &attrs, &prot, &page_size, &fi, &cacheattrs)) {
> +    if (get_phys_addr(env, addr, MMU_INST_FETCH, mmu_idx, &res, &fi)) {
>          /* the MPU lookup failed */
>          env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_IACCVIOL_MASK;
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM, env->v7m.secure);
>          qemu_log_mask(CPU_LOG_INT, "...really MemManage with CFSR.IACCVIOL\n");
>          return false;
>      }
> -    *insn = address_space_lduw_le(arm_addressspace(cs, attrs), physaddr,
> -                                 attrs, &txres);
> +    *insn = address_space_lduw_le(arm_addressspace(cs, res.attrs), res.phys,
> +                                  res.attrs, &txres);
>      if (txres != MEMTX_OK) {
>          env->v7m.cfsr[M_REG_NS] |= R_V7M_CFSR_IBUSERR_MASK;
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_BUS, false);
> @@ -2060,17 +2045,12 @@ static bool v7m_read_sg_stack_word(ARMCPU *cpu, ARMMMUIdx mmu_idx,
>       */
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
> -    MemTxAttrs attrs = {};
>      MemTxResult txres;
> -    target_ulong page_size;
> -    hwaddr physaddr;
> -    int prot;
> +    GetPhysAddrResult res = {};
>      ARMMMUFaultInfo fi = {};
> -    ARMCacheAttrs cacheattrs = {};
>      uint32_t value;
>  
> -    if (get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &physaddr,
> -                      &attrs, &prot, &page_size, &fi, &cacheattrs)) {
> +    if (get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &res, &fi)) {
>          /* MPU/SAU lookup failed */
>          if (fi.type == ARMFault_QEMU_SFault) {
>              qemu_log_mask(CPU_LOG_INT,
> @@ -2088,8 +2068,8 @@ static bool v7m_read_sg_stack_word(ARMCPU *cpu, ARMMMUIdx mmu_idx,
>          }
>          return false;
>      }
> -    value = address_space_ldl(arm_addressspace(cs, attrs), physaddr,
> -                              attrs, &txres);
> +    value = address_space_ldl(arm_addressspace(cs, res.attrs), res.phys,
> +                              res.attrs, &txres);
>      if (txres != MEMTX_OK) {
>          /* BusFault trying to read the data */
>          qemu_log_mask(CPU_LOG_INT,
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 204c820026..1a946f3757 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2296,18 +2296,12 @@ static ARMCacheAttrs combine_cacheattrs(CPUARMState *env,
>   * @address: virtual address to get physical address for
>   * @access_type: 0 for read, 1 for write, 2 for execute
>   * @mmu_idx: MMU index indicating required translation regime
> - * @phys_ptr: set to the physical address corresponding to the virtual address
> - * @attrs: set to the memory transaction attributes to use
> - * @prot: set to the permissions for the page containing phys_ptr
> - * @page_size: set to the size of the page containing phys_ptr
> + * @result: set on translation success.
>   * @fi: set to fault info if the translation fails
> - * @cacheattrs: (if non-NULL) set to the cacheability/shareability attributes
>   */
>  bool get_phys_addr(CPUARMState *env, target_ulong address,
>                     MMUAccessType access_type, ARMMMUIdx mmu_idx,
> -                   hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> -                   target_ulong *page_size,
> -                   ARMMMUFaultInfo *fi, ARMCacheAttrs *cacheattrs)
> +                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
>  {
>      ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
>  
> @@ -2318,43 +2312,53 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>           */
>          if (arm_feature(env, ARM_FEATURE_EL2)) {
>              hwaddr ipa;
> -            int s2_prot;
> +            int s1_prot;
>              int ret;
>              bool ipa_secure;
> -            ARMCacheAttrs cacheattrs2 = {};
> +            ARMCacheAttrs cacheattrs1;
>              ARMMMUIdx s2_mmu_idx;
>              bool is_el0;
>  
> -            ret = get_phys_addr(env, address, access_type, s1_mmu_idx, &ipa,
> -                                attrs, prot, page_size, fi, cacheattrs);
> +            ret = get_phys_addr(env, address, access_type, s1_mmu_idx,
> +                                result, fi);
>  
>              /* If S1 fails or S2 is disabled, return early.  */
>              if (ret || regime_translation_disabled(env, ARMMMUIdx_Stage2)) {
> -                *phys_ptr = ipa;
>                  return ret;
>              }
>  
> -            ipa_secure = attrs->secure;
> +            ipa = result->phys;
> +            ipa_secure = result->attrs.secure;
>              if (arm_is_secure_below_el3(env)) {
> -                if (ipa_secure) {
> -                    attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW);
> -                } else {
> -                    attrs->secure = !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW);
> -                }
> +                result->attrs.secure =
> +                    (ipa_secure
> +                     ? !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW)
> +                     : !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW));
>              } else {
>                  assert(!ipa_secure);
>              }
>  
> -            s2_mmu_idx = attrs->secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
> +            s2_mmu_idx = (result->attrs.secure
> +                          ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2);
>              is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0;
>  
> -            /* S1 is done. Now do S2 translation.  */
> +            /*
> +             * S1 is done, now do S2 translation.
> +             * Save the stage1 results so that we may merge
> +             * prot and cacheattrs later.
> +             */
> +            s1_prot = result->prot;
> +            cacheattrs1 = result->cacheattrs;
> +            memset(result, 0, sizeof(*result));
> +
>              ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx, is_el0,
> -                                     phys_ptr, attrs, &s2_prot,
> -                                     page_size, fi, &cacheattrs2);
> +                                     &result->phys, &result->attrs,
> +                                     &result->prot, &result->page_size,
> +                                     fi, &result->cacheattrs);
>              fi->s2addr = ipa;
> +
>              /* Combine the S1 and S2 perms.  */
> -            *prot &= s2_prot;
> +            result->prot &= s1_prot;
>  
>              /* If S2 fails, return early.  */
>              if (ret) {
> @@ -2370,20 +2374,21 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>                   *  Outer Write-Back Read-Allocate Write-Allocate.
>                   * Do not overwrite Tagged within attrs.
>                   */
> -                if (cacheattrs->attrs != 0xf0) {
> -                    cacheattrs->attrs = 0xff;
> +                if (cacheattrs1.attrs != 0xf0) {
> +                    cacheattrs1.attrs = 0xff;
>                  }
> -                cacheattrs->shareability = 0;
> +                cacheattrs1.shareability = 0;
>              }
> -            *cacheattrs = combine_cacheattrs(env, *cacheattrs, cacheattrs2);
> +            result->cacheattrs = combine_cacheattrs(env, cacheattrs1,
> +                                                    result->cacheattrs);
>  
>              /* Check if IPA translates to secure or non-secure PA space. */
>              if (arm_is_secure_below_el3(env)) {
>                  if (ipa_secure) {
> -                    attrs->secure =
> +                    result->attrs.secure =
>                          !(env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW));
>                  } else {
> -                    attrs->secure =
> +                    result->attrs.secure =
>                          !((env->cp15.vtcr_el2.raw_tcr & (VTCR_NSA | VTCR_NSW))
>                          || (env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW)));
>                  }
> @@ -2402,8 +2407,8 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>       * cannot upgrade an non-secure translation regime's attributes
>       * to secure.
>       */
> -    attrs->secure = regime_is_secure(env, mmu_idx);
> -    attrs->user = regime_is_user(env, mmu_idx);
> +    result->attrs.secure = regime_is_secure(env, mmu_idx);
> +    result->attrs.user = regime_is_user(env, mmu_idx);
>  
>      /*
>       * Fast Context Switch Extension. This doesn't exist at all in v8.
> @@ -2420,20 +2425,22 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>  
>      if (arm_feature(env, ARM_FEATURE_PMSA)) {
>          bool ret;
> -        *page_size = TARGET_PAGE_SIZE;
> +        result->page_size = TARGET_PAGE_SIZE;
>  
>          if (arm_feature(env, ARM_FEATURE_V8)) {
>              /* PMSAv8 */
>              ret = get_phys_addr_pmsav8(env, address, access_type, mmu_idx,
> -                                       phys_ptr, attrs, prot, page_size, fi);
> +                                       &result->phys, &result->attrs,
> +                                       &result->prot, &result->page_size, fi);
>          } else if (arm_feature(env, ARM_FEATURE_V7)) {
>              /* PMSAv7 */
>              ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
> -                                       phys_ptr, prot, page_size, fi);
> +                                       &result->phys, &result->prot,
> +                                       &result->page_size, fi);
>          } else {
>              /* Pre-v7 MPU */
>              ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> -                                       phys_ptr, prot, fi);
> +                                       &result->phys, &result->prot, fi);
>          }
>          qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32
>                        " mmu_idx %u -> %s (prot %c%c%c)\n",
> @@ -2441,9 +2448,9 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>                        (access_type == MMU_DATA_STORE ? "writing" : "execute"),
>                        (uint32_t)address, mmu_idx,
>                        ret ? "Miss" : "Hit",
> -                      *prot & PAGE_READ ? 'r' : '-',
> -                      *prot & PAGE_WRITE ? 'w' : '-',
> -                      *prot & PAGE_EXEC ? 'x' : '-');
> +                      result->prot & PAGE_READ ? 'r' : '-',
> +                      result->prot & PAGE_WRITE ? 'w' : '-',
> +                      result->prot & PAGE_EXEC ? 'x' : '-');
>  
>          return ret;
>      }
> @@ -2488,14 +2495,14 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>                  address = extract64(address, 0, 52);
>              }
>          }
> -        *phys_ptr = address;
> -        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> -        *page_size = TARGET_PAGE_SIZE;
> +        result->phys = address;
> +        result->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +        result->page_size = TARGET_PAGE_SIZE;
>  
>          /* Fill in cacheattr a-la AArch64.TranslateAddressS1Off. */
>          hcr = arm_hcr_el2_eff(env);
> -        cacheattrs->shareability = 0;
> -        cacheattrs->is_s2_format = false;
> +        result->cacheattrs.shareability = 0;
> +        result->cacheattrs.is_s2_format = false;
>          if (hcr & HCR_DC) {
>              if (hcr & HCR_DCT) {
>                  memattr = 0xf0;  /* Tagged, Normal, WB, RWA */
> @@ -2508,24 +2515,27 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>              } else {
>                  memattr = 0x44;  /* Normal, NC, No */
>              }
> -            cacheattrs->shareability = 2; /* outer sharable */
> +            result->cacheattrs.shareability = 2; /* outer sharable */
>          } else {
>              memattr = 0x00;      /* Device, nGnRnE */
>          }
> -        cacheattrs->attrs = memattr;
> +        result->cacheattrs.attrs = memattr;
>          return 0;
>      }
>  
>      if (regime_using_lpae_format(env, mmu_idx)) {
>          return get_phys_addr_lpae(env, address, access_type, mmu_idx, false,
> -                                  phys_ptr, attrs, prot, page_size,
> -                                  fi, cacheattrs);
> +                                  &result->phys, &result->attrs,
> +                                  &result->prot, &result->page_size,
> +                                  fi, &result->cacheattrs);
>      } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
>          return get_phys_addr_v6(env, address, access_type, mmu_idx,
> -                                phys_ptr, attrs, prot, page_size, fi);
> +                                &result->phys, &result->attrs,
> +                                &result->prot, &result->page_size, fi);
>      } else {
>          return get_phys_addr_v5(env, address, access_type, mmu_idx,
> -                                    phys_ptr, prot, page_size, fi);
> +                                &result->phys, &result->prot,
> +                                &result->page_size, fi);
>      }
>  }
>  
> @@ -2534,21 +2544,16 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> -    hwaddr phys_addr;
> -    target_ulong page_size;
> -    int prot;
> -    bool ret;
> +    GetPhysAddrResult res = {};
>      ARMMMUFaultInfo fi = {};
>      ARMMMUIdx mmu_idx = arm_mmu_idx(env);
> -    ARMCacheAttrs cacheattrs = {};
> +    bool ret;
>  
> -    *attrs = (MemTxAttrs) {};
> -
> -    ret = get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &phys_addr,
> -                        attrs, &prot, &page_size, &fi, &cacheattrs);
> +    ret = get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &res, &fi);
> +    *attrs = res.attrs;
>  
>      if (ret) {
>          return -1;
>      }
> -    return phys_addr;
> +    return res.phys;
>  }
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 7476fcafeb..28495ff525 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -209,11 +209,8 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      ARMMMUFaultInfo fi = {};
> -    hwaddr phys_addr;
> -    target_ulong page_size;
> -    int prot, ret;
> -    MemTxAttrs attrs = {};
> -    ARMCacheAttrs cacheattrs = {};
> +    GetPhysAddrResult res = {};
> +    int ret;
>  
>      /*
>       * Walk the page table and (if the mapping exists) add the page
> @@ -223,8 +220,7 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       */
>      ret = get_phys_addr(&cpu->env, address, access_type,
>                          core_to_arm_mmu_idx(&cpu->env, mmu_idx),
> -                        &phys_addr, &attrs, &prot, &page_size,
> -                        &fi, &cacheattrs);
> +                        &res, &fi);
>      if (likely(!ret)) {
>          PageEntryExtra extra = {};
>  
> @@ -234,22 +230,22 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>           * pass in the exact addresses.  This only happens for M-profile,
>           * which does not use or require PageEntryExtra.
>           */
> -        if (page_size >= TARGET_PAGE_SIZE) {
> -            phys_addr &= TARGET_PAGE_MASK;
> +        if (res.page_size >= TARGET_PAGE_SIZE) {
> +            res.phys &= TARGET_PAGE_MASK;
>              address &= TARGET_PAGE_MASK;
>  
>              /* Record some particulars for later lookup. */
> -            extra.x = phys_addr;
> +            extra.x = res.phys;
>              extra.x = FIELD_DP64(extra.x, PAGEENTRYEXTRA, ATTRS,
> -                                 cacheattrs.attrs);
> +                                 res.cacheattrs.attrs);
>              extra.x = FIELD_DP64(extra.x, PAGEENTRYEXTRA, SHAREABILITY,
> -                                 cacheattrs.shareability);
> +                                 res.cacheattrs.shareability);
>              extra.x = FIELD_DP64(extra.x, PAGEENTRYEXTRA, GUARDED,
> -                                 cacheattrs.guarded);
> +                                 res.cacheattrs.guarded);
>          }
>  
> -        tlb_set_page_with_extra(cs, address, phys_addr, attrs, extra,
> -                                prot, mmu_idx, page_size);
> +        tlb_set_page_with_extra(cs, address, res.phys, res.attrs, extra,
> +                                res.prot, mmu_idx, res.page_size);
>          return true;
>      } else if (probe) {
>          return false;

On 8/10/22 06:02, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Combine 5 output pointer argument from get_phys_addr
>> into a single struct.  Adjust all callers.
> 
> This looks to be an improvement - I guess the real benefit is the
> compiler isn't jamming so many closely aligned pointers on the stack
> frame for all the return values?

Correct.  The number of parameters is also down to 6, which fits all in register arguments 
for most hosts, including x86_64.   And in turn, we need to copy fewer arguments down to 
the subroutines.


r~