From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40423) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkUQ6-0005oh-AM for qemu-devel@nongnu.org; Tue, 21 Apr 2015 05:24:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkUQ3-0000S1-2g for qemu-devel@nongnu.org; Tue, 21 Apr 2015 05:24:42 -0400 Received: from static.88-198-71-155.clients.your-server.de ([88.198.71.155]:45800 helo=socrates.bennee.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkUQ2-0000PA-QM for qemu-devel@nongnu.org; Tue, 21 Apr 2015 05:24:39 -0400 References: <1428931324-4973-1-git-send-email-peter.maydell@linaro.org> <1428931324-4973-11-git-send-email-peter.maydell@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1428931324-4973-11-git-send-email-peter.maydell@linaro.org> Date: Tue, 21 Apr 2015 10:24:58 +0100 Message-ID: <877ft5x28l.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 10/14] target-arm: Honour NS bits in page tables List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Peter Crosthwaite , patches@linaro.org, "Edgar E. Iglesias" , qemu-devel@nongnu.org, Greg Bellows , Paolo Bonzini , Richard Henderson Peter Maydell writes: > Honour the NS bit in ARM page tables: > * when adding entries to the TLB, include the Secure/NonSecure > transaction attribute > * set the NS bit in the PAR when doing ATS operations > > Note that we don't yet correctly use the NSTable bit to > cause the page table walk itself to use the right attributes. > > Signed-off-by: Peter Maydell > --- > include/exec/memattrs.h | 2 ++ > target-arm/helper.c | 79 +++++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 69 insertions(+), 12 deletions(-) > > diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h > index 1cb3fc0..68a9c76 100644 > --- a/include/exec/memattrs.h > +++ b/include/exec/memattrs.h > @@ -29,6 +29,8 @@ typedef struct MemTxAttrs { > * "didn't specify" if necessary. > */ > unsigned int unspecified:1; > + /* ARM/AMBA TrustZone Secure access */ > + unsigned int secure:1; > } MemTxAttrs; > > /* Bus masters which don't specify any attributes will get this, > diff --git a/target-arm/helper.c b/target-arm/helper.c > index d77c6de..a568299 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -14,7 +14,7 @@ > #ifndef CONFIG_USER_ONLY > static inline int get_phys_addr(CPUARMState *env, target_ulong address, > int access_type, ARMMMUIdx mmu_idx, > - hwaddr *phys_ptr, int *prot, > + hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot, > target_ulong *page_size); > > /* Definitions for the PMCCNTR and PMCR registers */ > @@ -1466,9 +1466,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, > int prot; > int ret; > uint64_t par64; > + MemTxAttrs attrs = {}; > > ret = get_phys_addr(env, value, access_type, mmu_idx, > - &phys_addr, &prot, &page_size); > + &phys_addr, &attrs, &prot, &page_size); > if (extended_addresses_enabled(env)) { > /* ret is a DFSR/IFSR value for the long descriptor > * translation table format, but with WnR always clear. > @@ -1477,6 +1478,9 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, > par64 = (1 << 11); /* LPAE bit always set */ > if (ret == 0) { > par64 |= phys_addr & ~0xfffULL; > + if (!attrs.secure) { > + par64 |= (1 << 9); /* NS */ > + } I know this is fitting in with the rest of the code but it does seem some of these magic numbers should be defined somewhere or at least a reference to the bitfield format added to the comments. > /* We don't set the ATTR or SH fields in the PAR. */ > } else { > par64 |= 1; /* F */ > @@ -1499,6 +1503,9 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, > } else { > par64 = phys_addr & 0xfffff000; > } > + if (!attrs.secure) { > + par64 |= (1 << 9); /* NS */ > + } > } else { > par64 = ((ret & (1 << 10)) >> 5) | ((ret & (1 << 12)) >> 6) | > ((ret & 0xf) << 1) | 1; > @@ -4858,6 +4865,26 @@ static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx) > } > } > > +/* Return true if this address translation regime is secure */ > +static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx) > +{ > + switch (mmu_idx) { > + case ARMMMUIdx_S12NSE0: > + case ARMMMUIdx_S12NSE1: > + case ARMMMUIdx_S1NSE0: > + case ARMMMUIdx_S1NSE1: > + case ARMMMUIdx_S1E2: > + case ARMMMUIdx_S2NS: > + return false; > + case ARMMMUIdx_S1E3: > + case ARMMMUIdx_S1SE0: > + case ARMMMUIdx_S1SE1: > + return true; > + default: > + g_assert_not_reached(); > + } > +} > + > /* Return the SCTLR value which controls this address translation regime */ > static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx) > { > @@ -5210,6 +5237,7 @@ do_fault: > > static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type, > ARMMMUIdx mmu_idx, hwaddr *phys_ptr, > + MemTxAttrs *attrs, > int *prot, target_ulong *page_size) > { > CPUState *cs = CPU(arm_env_get_cpu(env)); > @@ -5224,6 +5252,7 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type, > int domain_prot; > hwaddr phys_addr; > uint32_t dacr; > + bool ns; > > /* Pagetable walk. */ > /* Lookup l1 descriptor. */ > @@ -5273,10 +5302,12 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type, > xn = desc & (1 << 4); > pxn = desc & 1; > code = 13; > + ns = extract32(desc, 19, 1); > } else { > if (arm_feature(env, ARM_FEATURE_PXN)) { > pxn = (desc >> 2) & 1; > } > + ns = extract32(desc, 3, 1); > /* Lookup l2 entry. */ > table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc); > desc = ldl_phys(cs->as, table); > @@ -5330,6 +5361,13 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type, > goto do_fault; > } > } > + if (ns) { > + /* The NS bit will (as required by the architecture) have no effect if > + * the CPU doesn't support TZ or this is a non-secure translation > + * regime, because the attribute will already be non-secure. > + */ > + attrs->secure = false; > + } > *phys_ptr = phys_addr; > return 0; > do_fault: > @@ -5347,7 +5385,7 @@ typedef enum { > > static int get_phys_addr_lpae(CPUARMState *env, target_ulong address, > int access_type, ARMMMUIdx mmu_idx, > - hwaddr *phys_ptr, int *prot, > + hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot, > target_ulong *page_size_ptr) > { > CPUState *cs = CPU(arm_env_get_cpu(env)); > @@ -5552,6 +5590,13 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address, > goto do_fault; > } > > + if (ns) { > + /* The NS bit will (as required by the architecture) have no effect if > + * the CPU doesn't support TZ or this is a non-secure translation > + * regime, because the attribute will already be non-secure. > + */ > + txattrs->secure = false; > + } > *phys_ptr = descaddr; > *page_size_ptr = page_size; > return 0; > @@ -5635,8 +5680,8 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address, > * by doing a translation table walk on MMU based systems or using the > * MPU state on MPU based systems. > * > - * Returns 0 if the translation was successful. Otherwise, phys_ptr, > - * prot and page_size are not filled in, and the return value provides > + * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs, > + * prot and page_size may not be filled in, and the return value provides > * information on why the translation aborted, in the format of a > * DFSR/IFSR fault register, with the following caveats: > * * we honour the short vs long DFSR format differences. > @@ -5649,12 +5694,13 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address, > * @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 > */ > static inline int get_phys_addr(CPUARMState *env, target_ulong address, > int access_type, ARMMMUIdx mmu_idx, > - hwaddr *phys_ptr, int *prot, > + hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot, > target_ulong *page_size) > { > if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) { > @@ -5667,6 +5713,12 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address, > mmu_idx += ARMMMUIdx_S1NSE0; > } > > + /* The page table entries may downgrade secure to non-secure, but > + * cannot upgrade an non-secure translation regime's attributes > + * to secure. > + */ > + attrs->secure = regime_is_secure(env, mmu_idx); > + > /* Fast Context Switch Extension. This doesn't exist at all in v8. > * In v7 and earlier it affects all stage 1 translations. > */ > @@ -5695,10 +5747,10 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address, > > if (regime_using_lpae_format(env, mmu_idx)) { > return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr, > - prot, page_size); > + attrs, prot, page_size); > } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) { > return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr, > - prot, page_size); > + attrs, prot, page_size); > } else { > return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr, > prot, page_size); > @@ -5716,14 +5768,16 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address, > int ret; > uint32_t syn; > bool same_el = (arm_current_el(env) != 0); > + MemTxAttrs attrs = {}; > > - ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr, &prot, > - &page_size); > + ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr, > + &attrs, &prot, &page_size); > if (ret == 0) { > /* Map a single [sub]page. */ > phys_addr &= TARGET_PAGE_MASK; > address &= TARGET_PAGE_MASK; > - tlb_set_page(cs, address, phys_addr, prot, mmu_idx, page_size); > + tlb_set_page_with_attrs(cs, address, phys_addr, attrs, > + prot, mmu_idx, page_size); > return 0; > } > > @@ -5758,9 +5812,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) > target_ulong page_size; > int prot; > int ret; > + MemTxAttrs attrs = {}; > > ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env), &phys_addr, > - &prot, &page_size); > + &attrs, &prot, &page_size); > > if (ret != 0) { > return -1; Reviewed-by: Alex Bennée -- Alex Bennée