On Thu, Feb 12, 2015 at 04:05:06PM +0100, 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.
Just because I like replying to myself so much... re: feature checking
>
> Signed-off-by: Andrew Jones <drjo...@redhat.com>
> ---
> 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)
> {
> 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 */
> +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);
> + 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);
> + }
> +
> + 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);
I probably should have used ARM_FEATURE_LPAE here instead to be a bit closer
to the truth.
> +
> + if (have_wxn) {
> + wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
> + }
> +
> + if (is_aa64) {
> + assert(arm_feature(env, ARM_FEATURE_V8));
> + 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)) {
> + 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));
I think it's OK to trust the caller to only send a non-zero pxn when they
have arm_feature(env, ARM_FEATURE_PXN). However, it just occurred to me that
checking that feature before using pxn may have been a nice touch...
> + }
> + break;
> + 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));
> +
> 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
>
>
