Re: [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Thu, Nov 05, 2015 at 06:15:51PM +0000, Peter Maydell wrote:
> If we have a secure address space, use it in page table walks:
>  * when doing the physical accesses to read descriptors,
>    make them through the correct address space
>  * when the final result indicates a secure access, pass the
>    correct address space index to tlb_set_page_with_attrs()
> 
> (The descriptor reads are the only direct physical accesses
> made in target-arm/ for CPUs which might have TrustZone.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Nice to see how this falls into place like this :-)

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---
>  target-arm/cpu.h    | 29 +++++++++++++++++++++++++++++
>  target-arm/helper.c | 10 +++++++---
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 815fef8..8dbf4d4 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1720,6 +1720,12 @@ static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
>      return el;
>  }
>  
> +/* Indexes used when registering address spaces with cpu_address_space_init */
> +typedef enum ARMASIdx {
> +    ARMASIdx_NS = 0,
> +    ARMASIdx_S = 1,
> +} ARMASIdx;
> +
>  /* Return the Exception Level targeted by debug exceptions;
>   * currently always EL1 since we don't implement EL2 or EL3.
>   */
> @@ -1991,4 +1997,27 @@ enum {
>      QEMU_PSCI_CONDUIT_HVC = 2,
>  };
>  
> +#ifndef CONFIG_USER_ONLY
> +/* Return the address space index to use for a memory access
> + * (which depends on whether the access is S or NS, and whether
> + * the board gave us a separate AddressSpace for S accesses).
> + */
> +static inline int arm_asidx(CPUState *cs, bool is_secure)
> +{
> +    if (is_secure && cs->num_ases > 1) {
> +        return ARMASIdx_S;
> +    }
> +    return ARMASIdx_NS;
> +}
> +
> +/* Return the AddressSpace to use for a memory access
> + * (which depends on whether the access is S or NS, and whether
> + * the board gave us a separate AddressSpace for S accesses).
> + */
> +static inline AddressSpace *arm_addressspace(CPUState *cs, bool is_secure)
> +{
> +    return cpu_get_address_space(cs, arm_asidx(cs, is_secure));
> +}
> +#endif
> +
>  #endif
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 174371b..242928d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6260,13 +6260,14 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>      MemTxAttrs attrs = {};
> +    AddressSpace *as = arm_addressspace(cs, is_secure);
>  
>      attrs.secure = is_secure;
>      addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
>      if (fi->s1ptw) {
>          return 0;
>      }
> -    return address_space_ldl(cs->as, addr, attrs, NULL);
> +    return address_space_ldl(as, addr, attrs, NULL);
>  }
>  
>  static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
> @@ -6276,13 +6277,14 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>      MemTxAttrs attrs = {};
> +    AddressSpace *as = arm_addressspace(cs, is_secure);
>  
>      attrs.secure = is_secure;
>      addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
>      if (fi->s1ptw) {
>          return 0;
>      }
> -    return address_space_ldq(cs->as, addr, attrs, NULL);
> +    return address_space_ldq(as, addr, attrs, NULL);
>  }
>  
>  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
> @@ -7307,6 +7309,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>      target_ulong page_size;
>      int prot;
>      int ret;
> +    int asidx;
>      MemTxAttrs attrs = {};
>  
>      ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
> @@ -7315,7 +7318,8 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>          /* Map a single [sub]page.  */
>          phys_addr &= TARGET_PAGE_MASK;
>          address &= TARGET_PAGE_MASK;
> -        tlb_set_page_with_attrs(cs, address, 0, phys_addr, attrs,
> +        asidx = arm_asidx(cs, attrs.secure);
> +        tlb_set_page_with_attrs(cs, address, asidx, phys_addr, attrs,
>                                  prot, mmu_idx, page_size);
>          return 0;
>      }
> -- 
> 1.9.1
>