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

[Andrew Jones <drjones@redhat.com>]

On Fri, Jan 09, 2015 at 06:06:33PM +0100, Andrew Jones 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 <drjones@redhat.com>
> 
> ---
> 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 | 99 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 79 insertions(+), 20 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3ef0f1f38eda5..48a83cff957fb 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.
> + *
> + * @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) descriptors
> + * @pxn         : (PXNTable || PXN) descriptors

Just realized I'm missing 'from' in the last two lines above.

> + */
> +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:
> +    case 1:
> +        if (is_user && !(ap & 1)) {
> +            return 0;
> +        }
> +        wxn = env->cp15.sctlr_el[1] & SCTLR_WXN;
> +        if (arm_el_is_aa64(env, 1)) {
> +            pxn = pxn || ((ap & 1) && !(ap & 2));
> +            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);
> +        }
> +        if (arm_is_secure(env)) {
> +            xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
> +        }
> +        break;
> +    case 2:
> +        /* TODO actually support EL2 */
> +        assert(true);

And, argh... brain wires got crossed here. Of course this should
be 'assert(false)'. I'll send a v2.

> +
> +        if (arm_el_is_aa64(env, 2)) {
> +            wxn = env->cp15.sctlr_el[2] & SCTLR_WXN;
> +        } else {
> +            /* wxn = HSCTLR.WXN */
> +        }
> +        break;
> +    case 3:
> +        if (arm_el_is_aa64(env, 3)) {
> +            wxn = env->cp15.sctlr_el[3] & SCTLR_WXN;
> +        } else {
> +            wxn = 0;
> +        }
> +        xn = xn || (ns && (env->cp15.scr_el3 & SCR_SIF));
> +        break;
> +    }
> +
> +    return xn || (!(ap & 2) && wxn) ? 0 : PAGE_EXEC;
> +}
> +
>  static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
>                                           uint32_t address)
>  {
> @@ -4787,7 +4850,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      hwaddr descaddr, descmask;
>      uint32_t tableattrs;
>      target_ulong page_size;
> -    uint32_t attrs;
> +    uint32_t attrs, ap;
>      int32_t granule_sz = 9;
>      int32_t va_size = 32;
>      int32_t tbi = 0;
> @@ -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
> @@ -4952,29 +5015,25 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          /* Access flag */
>          goto do_fault;
>      }
> +
>      fault_type = permission_fault;
> -    if (is_user && !(attrs & (1 << 4))) {
> +    ap = extract32(attrs, 4, 2); /* AP[2:1] */
> +
> +    if (is_user && !(ap & 1)) {
>          /* Unprivileged access not enabled */
>          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;
> +
> +    *prot = PAGE_READ;
> +    *prot |= check_xn_lpae(env, is_user, ap, extract32(attrs, 3, 1),
> +                           extract32(attrs, 12, 1), extract32(attrs, 11, 1));
> +    if (!(*prot & PAGE_EXEC) && access_type == 2) {
> +        goto do_fault;
>      }
> -    if (attrs & (1 << 5)) {
> -        /* Write access forbidden */
> -        if (access_type == 1) {
> -            goto do_fault;
> -        }
> -        *prot &= ~PAGE_WRITE;
> +
> +    *prot |= !(ap & 2) ? PAGE_WRITE : 0;
> +    if (!(*prot & PAGE_WRITE) && access_type == 1) {
> +        goto do_fault;
>      }
>  
>      *phys_ptr = descaddr;
> -- 
> 1.9.3
>