Re: [Qemu-devel] [PATCH 2/2] tcg-arm: more instruction execution control

[Peter Maydell]

On 13 January 2015 at 15:48, Andrew Jones <address@hidden> wrote:
> Cleanup XN/PXN handling in get_phys_addr_lpae, and implement all but
> EL2 support of the following ARMv8 sections
>
>   D4.5.1 Memory access control: Access permissions for instruction
>          execution
>   G4.7.2 Execute-never restrictions on instruction fetching
>
> G4.7.2 matches the ARMv7 section B3.7.2 when long-descriptors are used.
>
> Signed-off-by: Andrew Jones <address@hidden>
>
> ---
> v3:
>   - correct logic for (is_user && !(ap & 1)) case
>   - base on "user doesn't need R/W access to exec" patch
> v2: correct assert in el2 case
>
> I didn't test this with secure mode (I didn't even check if that's
> currently possible), but I did test all other EL1&0 XN controls with
> both tcg-arm (cortex-a15) and tcg-aarch64 (cortex-a57) by hacking up
> some tests with kvm-unit-tests/arm[64]. I also booted Linux (just
> up to looking for a rootfs) with both.
>
>  target-arm/helper.c | 80 
> +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 66 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 7c30a2669a0f2..715e0f47bec7d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4516,6 +4516,69 @@ static inline int check_ap(CPUARMState *env, int ap, 
> int domain_prot,
>    }
>  }
>
> +/* Check section/page execution permission.
> + *
> + * Returns PAGE_EXEC if execution is permitted, otherwise zero.
> + *
> + * Supports AArch64, AArch32, and v7-LPAE. Doesn't yet support
> + * EL2. Actually, for v7-LPAE, we'd also need to emulate Virt
> + * Extensions to truly have WXN/UWXN. We don't support checking
> + * for that yet, so we just assume we have them.

If you want to know if the Virtualization Extensions are
present, check feature bit ARM_FEATURE_EL2.

In general this code seems to be rather short on feature
bit checks -- notice that the code it is replacing was
doing checks on the V8 feature bit, for instance.

> + *
> + * @env         : CPUARMState
> + * @is_user     : 0 for privileged access, 1 for user
> + * @ap          : Access permissions (AP[2:1]) from descriptor
> + * @ns          : (NSTable || NS) from descriptors
> + * @xn          : ([U]XNTable || [U]XN) from descriptors
> + * @pxn         : (PXNTable || PXN) from descriptors
> + */
> +static int check_xn_lpae(CPUARMState *env, int is_user, int ap,
> +                         int ns, int xn, int pxn)
> +{
> +    int wxn;
> +
> +    switch (arm_current_el(env)) {
> +    case 0:

If arm_current_el() is 0 but EL3 is 32 bit then the
controlling environment here is the Secure PL1, which
is EL3, and falling through to look at sctlr_el[1] is
wrong. (SCTLR(S) is sctlr_el[3].)

(Also get_phys_addr() can be called via the ats_write()
functions, which means that "arm_current_el()" and "the
address translation regime we're trying to determine the
answer for" aren't necessarily the same...)

I think we're very soon going to need to bite the bullet and
make this code have a concept of the current "translation
regime", as the ARM ARM terms it...

If you wish to avoid that (and I wouldn't object, given
this patchset is snowballing in scope already) then do what
get_phys_addr() does to get the relevant SCTLR:
    uint32_t sctlr = A32_BANKED_CURRENT_REG_GET(env, sctlr);
This will work for everything we currently support:
 * AArch64 EL1 (with an EL0 either AArch32 or AArch64)
 * AArch32 EL1
 * TZ for the limited case of EL3 == AArch32 (and so no EL1)

> +    case 1:
> +        wxn = env->cp15.sctlr_el[1] & SCTLR_WXN;
> +        if (arm_el_is_aa64(env, 1)) {
> +            if (is_user && !(ap & 1)) {
> +                wxn = 0;
> +            }

I don't understand what we're doing here: as far as I can see
the ARM ARM simply defines that WXN is the bit from the
relevant SCTLR, and it always applies if the page is writable.
I have a feeling you're re-calculating half of "is this
page writable" here, and then doing the other half below.
It would be cleaner to pass in the 'prot' bits that have
already been calculated.

> +            pxn = pxn || ((ap & 1) && !(ap & 2));

Very weird way to write "ap == 1" ?

> +            xn = is_user ? xn : pxn;
> +        } else {
> +            int uwxn = env->cp15.sctlr_el[1] & SCTLR_UWXN;
> +            pxn = pxn || (uwxn && (ap & 1) && !(ap & 2));
> +            xn = xn || (!is_user && pxn) || (is_user && !(ap & 1));
> +        }
> +        if (arm_is_secure(env)) {
> +            xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
> +        }
> +        break;
> +    case 2:
> +        /* TODO actually support EL2 */
> +        assert(false);
> +
> +        if (arm_el_is_aa64(env, 2)) {
> +            wxn = env->cp15.sctlr_el[2] & SCTLR_WXN;
> +        } else {
> +            /* wxn = HSCTLR.WXN */
> +        }

You can just write this as
    wxn = env->cp15.sctlr_el[2] & SCTLR_WXN;

because SCTLR_EL2 and HSCTLR are architecturally mapped and so
we will store the state in the same env->cp15 field whether
EL2 is 32 or 64 bit.

> +        break;
> +    case 3:
> +        if (arm_el_is_aa64(env, 3)) {
> +            wxn = env->cp15.sctlr_el[3] & SCTLR_WXN;
> +        } else {
> +            wxn = 0;

This should also be checking sctlr_el[3]'s WXN bit.
(That's SCTLR(S) in AArch32 terms.)

You're also missing the handling of SCTLR.UWXN here.
(Remember that if EL3 is AArch32 then all the traditional
"operating system" PL1 privilege level code runs at EL3,
not EL1.)

> +        }
> +        xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
> +        break;
> +    }
> +
> +    return xn || (!(ap & 2) && wxn) ? 0 : PAGE_EXEC;

I think this is the other half of "recalculate whether
the page is writable". Something more like this would be clearer:

  if (xn || (wxn && (prot & PAGE_WRITE))) {
      return 0;
  }
  return PAGE_EXEC;


> +}
> +
>  static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
>                                           uint32_t address)
>  {
> @@ -4941,7 +5004,7 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>          if (extract32(tableattrs, 2, 1)) {
>              attrs &= ~(1 << 4);
>          }
> -        /* Since we're always in the Non-secure state, NSTable is ignored. */
> +        attrs |= extract32(tableattrs, 4, 1) << 3; /* NS */
>          break;
>      }
>      /* Here descaddr is the final physical address, and attributes
> @@ -4962,19 +5025,8 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>          *prot |= !(ap & 2) ? PAGE_WRITE : 0;
>      }
>
> -    *prot |= PAGE_EXEC;
> -    if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) 
> ||
> -        (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
> -        (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) ||
> -        (!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;
> -    }
> +    *prot |= check_xn_lpae(env, is_user, ap, extract32(attrs, 3, 1),
> +                           extract32(attrs, 12, 1), extract32(attrs, 11, 1));
>
>      if ((*prot == 0)
>              || (!(*prot & PAGE_WRITE) && access_type == 1)
> --
> 1.9.3
>

thanks
-- PMM