From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41201) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVNKF-0004Gx-Mg for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:48:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVNKB-00054c-5f for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:48:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33938) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVNKA-00054Y-UI for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:48:07 -0400 Date: Tue, 10 Mar 2015 17:48:00 +0100 From: Andrew Jones Message-ID: <20150310164759.GC6320@hawk.usersys.redhat.com> References: <1423753507-30542-1-git-send-email-drjones@redhat.com> <1423753507-30542-5-git-send-email-drjones@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On Tue, Mar 10, 2015 at 03:56:11PM +0000, Peter Maydell wrote: > On 12 February 2015 at 15:05, Andrew Jones wrote: > > This patch makes the following changes to the determination of > > whether an address is executable, when translating addresses > > using LPAE. > > > > 1. No longer assumes that PL0 can't execute when it can't read. > > It can in AArch64, a difference from AArch32. > > 2. Use va_size == 64 to determine we're in AArch64, rather than > > arm_feature(env, ARM_FEATURE_V8), which is insufficient. > > 3. Add additional XN determinants > > - NS && is_secure && (SCR & SCR_SIF) > > - WXN && (prot & PAGE_WRITE) > > - AArch64: (prot_PL0 & PAGE_WRITE) > > - AArch32: UWXN && (prot_PL0 & PAGE_WRITE) > > - XN determination should also work in secure mode (untested) > > - XN may even work in EL2 (currently impossible to test) > > 4. Cleans up the bloated PAGE_EXEC condition - by removing it. > > > > The helper get_S1prot is introduced, which also handles short- > > descriptors and v6 XN determination. It may even work in EL2, > > when support for that comes, but, as the function name implies, > > it only works for stage 1 translations. > > > > Signed-off-by: Andrew Jones > > --- > > target-arm/helper.c | 111 ++++++++++++++++++++++++++++++++++++++++------------ > > 1 file changed, 86 insertions(+), 25 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index df78f481e92f4..20e5753bd216d 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -4695,8 +4695,8 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx) > > /* Translate section/page access permissions to page > > * R/W protection flags > > */ > > -static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, > > - bool is_user, int ap, int domain_prot) > > +static int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, > > + bool is_user, int ap, int domain_prot) > > ...why does this suddenly lose its 'inline' ? Adding another caller, and thought it was a bit fat for explicit inlining, but have no problem returning it. > > > { > > bool simple_ap = regime_using_lpae_format(env, mmu_idx) > > || (regime_sctlr(env, mmu_idx) & SCTLR_AFE); > > @@ -4762,6 +4762,84 @@ static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, > > } > > } > > > > +/* Translate section/page access permissions to protection flags */ > > This is LPAE-format only so it would be nice to mention that in the comment > and function name. Not after the next patch. I probably should have made it completely LPAE-only first, then added two more patches, one preparing it for non-LPAE, and then the next patch, or just done a better job pointing that out in the commit message. > > > +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64, > > + int ap, int domain_prot, int ns, int xn, int pxn) > > +{ > > + bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx); > > But this is always false! Same response as above - not after next patch. > > > + bool is_user = regime_is_user(env, mmu_idx); > > + bool have_wxn; > > + int prot_rw, user_rw; > > + int wxn = 0; > > + > > + assert(mmu_idx != ARMMMUIdx_S2NS); > > + > > + if (domain_prot_valid && domain_prot == 3) { > > + return PAGE_READ | PAGE_WRITE | PAGE_EXEC; > > + } > > + > > + user_rw = get_rw_prot(env, mmu_idx, true, ap, domain_prot); > > + if (is_user) { > > + prot_rw = user_rw; > > + } else { > > + prot_rw = get_rw_prot(env, mmu_idx, false, ap, domain_prot); > > + } > > I think it would be much better not to try to use the short-descriptor > get_rw_prot function. For one thing, we know for definite that we > won't be using the old-fashioned AP[2:0] access format, and that > we don't have to worry about the domain protection stuff. So it's > much simpler and better not to tangle it up with the legacy stuff. > (Pull the simple-access-permissions check out into its own function > if you like.) legacy stuff is here for next patch too > > For instance, you're missing a shift here on the ap bits, because > get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1]. Don't need the shift because get_rw_prot supports the 2-bit format. > > > + > > + if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) { > > + return prot_rw; > > + } > > + > > + /* TODO have_wxn should be replaced with arm_feature(env, ARM_FEATURE_EL2), > > + * when ARM_FEATURE_EL2 starts getting set. For now we assume all v7 > > + * compatible processors have EL2, which is required for [U]WXN. > > + */ > > + have_wxn = arm_feature(env, ARM_FEATURE_V7); > > ARMv8 CPUs without EL2 still have WXN, I think. I think so too. So V8 || (V7 && EL2) would be the most appropriate. > > > + > > + if (have_wxn) { > > + wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN; > > + } > > + > > + if (is_aa64) { > > + assert(arm_feature(env, ARM_FEATURE_V8)); > > I wouldn't bother with this assert. OK > > > + switch (regime_el(env, mmu_idx)) { > > + case 1: > > + if (is_user && !user_rw) { > > + wxn = 0; > > + } else if (!is_user) { > > + xn = pxn || (user_rw & PAGE_WRITE); > > + } > > + break; > > + case 2: > > + case 3: > > + break; > > + } > > + } else if (arm_feature(env, ARM_FEATURE_V6K)) { > > Always true, since you can't have long format descriptors > unless this is at least v7. Next patch again. > > > + switch (regime_el(env, mmu_idx)) { > > + case 1: > > + case 3: > > + if (is_user) { > > + xn = xn || !user_rw; > > + } else { > > + int uwxn = 0; > > + if (have_wxn) { > > + uwxn = regime_sctlr(env, mmu_idx) & SCTLR_UWXN; > > + } > > + xn = xn || !prot_rw || pxn || (uwxn && (user_rw & PAGE_WRITE)); > > + } > > + break; > > Doesn't this lose us the "you need read permission to execute" > check (for 32-bit)? Something in here should be doing a > PAGE_READ check to see if we can have PAGE_EXEC. It's there. It's the '!user_rw' and the '!prot_rw' > > > + case 2: > > + break; > > + } > > + } else { > > + xn = wxn = 0; > > + } > > + > > + if (xn || (wxn && (prot_rw & PAGE_WRITE))) { > > + return prot_rw; > > + } > > + return prot_rw | PAGE_EXEC; > > > > +} > > + > > static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx, > > uint32_t *table, uint32_t address) > > { > > @@ -5047,7 +5125,6 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > int32_t granule_sz = 9; > > int32_t va_size = 32; > > int32_t tbi = 0; > > - bool is_user; > > TCR *tcr = regime_tcr(env, mmu_idx); > > > > /* TODO: > > @@ -5220,31 +5297,15 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > /* Access flag */ > > goto do_fault; > > } > > + > > + *prot = get_S1prot(env, mmu_idx, va_size == 64, extract32(attrs, 4, 2), 0, > > + extract32(attrs, 3, 1), extract32(attrs, 12, 1), > > + extract32(attrs, 11, 1)); > > Urgh. It would be easier to understand if you just passed attrs > into get_S1prot and had it pick the fields apart, because then > you can match them up with variable names without cross-referencing > against the function definition. I can pick them apart before passing into local well-named variables. I'd prefer to keep the helper function independent of get_phys_addr_lpae's internal bitmap. > > > + > > fault_type = permission_fault; > > - is_user = regime_is_user(env, mmu_idx); > > - if (is_user && !(attrs & (1 << 4))) { > > - /* Unprivileged access not enabled */ > > + if (!(*prot & (1 << access_type))) { > > goto do_fault; > > } > > - *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; > > - if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) || > > - (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) || > > - (!is_user && (attrs & (1 << 11)))) { > > - /* XN/UXN or PXN. Since we only implement EL0/EL1 we unconditionally > > - * treat XN/UXN as UXN for v8. > > - */ > > - if (access_type == 2) { > > - goto do_fault; > > - } > > - *prot &= ~PAGE_EXEC; > > - } > > - if (attrs & (1 << 5)) { > > - /* Write access forbidden */ > > - if (access_type == 1) { > > - goto do_fault; > > - } > > - *prot &= ~PAGE_WRITE; > > - } > > > > *phys_ptr = descaddr; > > *page_size_ptr = page_size; > > -- > > 1.9.3 > > > > > -- PMM