On 10 March 2015 at 21:06, Andrew Jones <drjo...@redhat.com> 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. 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 <drjo...@redhat.com>
I like the general shape of this patch. Minor comment below: > --- > target-arm/helper.c | 129 > ++++++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 100 insertions(+), 29 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index d996659652f8d..c457e9ab8c85a 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -4962,15 +4962,11 @@ static inline int ap_to_rw_prot(CPUARMState *env, > ARMMMUIdx mmu_idx, > /* Translate section/page access permissions to page > * R/W protection flags. > * > - * @env: CPUARMState > - * @mmu_idx: MMU index indicating required translation regime > * @ap: The 2-bit simple AP (AP[2:1]) > + * @is_user: TRUE if accessing from PL0 > */ > -static inline int > -simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap) > +static inline int simple_ap_to_rw_prot_is_user(int ap, bool is_user) > { > - bool is_user = regime_is_user(env, mmu_idx); > - > switch (ap) { > case 0: > return is_user ? 0 : PAGE_READ | PAGE_WRITE; > @@ -4985,6 +4981,94 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx > mmu_idx, int ap) > } > } > > +static inline int > +simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap) > +{ > + return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx)); > +} > + > +/* Translate section/page access permissions to protection flags > + * > + * @env: CPUARMState > + * @mmu_idx: MMU index indicating required translation regime > + * @is_aa64: TRUE if AArch64 > + * @ap: The 2-bit simple AP (AP[2:1]) > + * @ns: NS (non-secure) bit > + * @xn: XN (execute-never) bit > + * @pxn: PXN (privileged execute-never) bit > + */ > +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64, > + int ap, int ns, int xn, int pxn) > +{ > + bool is_user = regime_is_user(env, mmu_idx); > + int prot_rw, user_rw; > + bool have_wxn; > + int wxn = 0; > + > + assert(mmu_idx != ARMMMUIdx_S2NS); > + > + user_rw = simple_ap_to_rw_prot_is_user(ap, true); > + if (is_user) { > + prot_rw = user_rw; > + } else { > + prot_rw = simple_ap_to_rw_prot_is_user(ap, false); > + } > + > + if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) { > + return prot_rw; > + } > + > + /* TODO have_wxn should be replaced with > + * ARM_FEATURE_V8 || (ARM_FEATURE_V7 && ARM_FEATURE_EL2) > + * when ARM_FEATURE_EL2 starts getting set. For now we assume all LPAE > + * compatible processors have EL2, which is required for [U]WXN. > + */ > + have_wxn = arm_feature(env, ARM_FEATURE_LPAE); > + > + if (have_wxn) { > + wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN; > + } > + > + if (is_aa64) { > + switch (regime_el(env, mmu_idx)) { > + case 1: > + if (is_user && !user_rw) { > + wxn = 0; I don't understand this. We ignore the WXN bit if this is a user access and the page is not readable ? I also find the naming of this variable "user_rw" (and to a lesser extent "prot_rw") very confusing. I keep misreading "if (user_rw)" as meaning "if this page is read-write for the user", when in fact it only means "if this page is readable for the user". Maybe it would be less confusing if we always did tests against a set of PAGE_* flags rather than doing an is/is-not-zero test? The rest looked OK to me. -- PMM