[1/2] tcg-aarch64: user doesn't need R/W access to exec

Table D4-32 shows that execute access from EL0 doesn't depend
on AP[1].

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target-arm/helper.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

On 13 January 2015 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
> Table D4-32 shows that execute access from EL0 doesn't depend
> on AP[1].

This commit message is a bit sparse, which confused me
for a bit. It would be worth beefing it up a bit:

target-arm: 64-bit EL0 code can execute from unreadable pages

In AArch64 mode, a page can be executable even if it is not
readable (a difference from AArch32). Instead of bailing out
early if the page is not readable, just add "32 bit and
page not readable" to the list of conditions that make a
page non-executable, and check whether the protections and
the access type are compatible once at the end of the function.

> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target-arm/helper.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3ef0f1f38eda5..7c30a2669a0f2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4787,7 +4787,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;
> @@ -4952,14 +4952,20 @@ 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))) {
> -        /* Unprivileged access not enabled */
> -        goto do_fault;
> +    ap = extract32(attrs, 4, 2); /* AP[2:1] */
> +
> +    *prot = 0;
> +    if (!is_user || (ap & 1)) {
> +        *prot |= PAGE_READ;
> +        *prot |= !(ap & 2) ? PAGE_WRITE : 0;

Personally I would find
     if (!(ap & 2)) {
         *prot |= PAGE_WRITE;
     }

clearer.

>      }
> -    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +
> +    *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.
> @@ -4969,12 +4975,11 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          }

There is a "if access_type == 2 goto do_fault" check just
above this hunk which you can delete, because we're now
doing that check in the code you add below.

>          *prot &= ~PAGE_EXEC;
>      }
> -    if (attrs & (1 << 5)) {
> -        /* Write access forbidden */
> -        if (access_type == 1) {
> -            goto do_fault;
> -        }
> -        *prot &= ~PAGE_WRITE;
> +
> +    if ((*prot == 0)
> +            || (!(*prot & PAGE_WRITE) && access_type == 1)
> +            || (!(*prot & PAGE_EXEC) && access_type == 2)) {
> +        goto do_fault;

Why isn't this just
    if (!(*prot & (1 << access_type))) {

? (Or at least, why doesn't it treat PAGE_READ the same way
as the other two bits?) As it is I think we'll treat a page
that is marked exec-not-readable as if it were readable.

thanks
-- PMM

On Fri, Jan 16, 2015 at 02:52:21PM +0000, Peter Maydell wrote:
> On 13 January 2015 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
> > Table D4-32 shows that execute access from EL0 doesn't depend
> > on AP[1].
> 
> This commit message is a bit sparse, which confused me
> for a bit. It would be worth beefing it up a bit:
> 
> target-arm: 64-bit EL0 code can execute from unreadable pages
> 
> In AArch64 mode, a page can be executable even if it is not
> readable (a difference from AArch32). Instead of bailing out
> early if the page is not readable, just add "32 bit and
> page not readable" to the list of conditions that make a
> page non-executable, and check whether the protections and
> the access type are compatible once at the end of the function.

OK

> 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target-arm/helper.c | 27 ++++++++++++++++-----------
> >  1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 3ef0f1f38eda5..7c30a2669a0f2 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4787,7 +4787,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;
> > @@ -4952,14 +4952,20 @@ 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))) {
> > -        /* Unprivileged access not enabled */
> > -        goto do_fault;
> > +    ap = extract32(attrs, 4, 2); /* AP[2:1] */
> > +
> > +    *prot = 0;
> > +    if (!is_user || (ap & 1)) {
> > +        *prot |= PAGE_READ;
> > +        *prot |= !(ap & 2) ? PAGE_WRITE : 0;
> 
> Personally I would find
>      if (!(ap & 2)) {
>          *prot |= PAGE_WRITE;
>      }
> 
> clearer.

OK

> 
> >      }
> > -    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> > +
> > +    *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.
> > @@ -4969,12 +4975,11 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >          }
> 
> There is a "if access_type == 2 goto do_fault" check just
> above this hunk which you can delete, because we're now
> doing that check in the code you add below.

Right. Will do, alternatively I should have brought the PAGE_EXEC
handling below in with patch 2/2, which was my plan, but forgot
to split it out.

> 
> >          *prot &= ~PAGE_EXEC;
> >      }
> > -    if (attrs & (1 << 5)) {
> > -        /* Write access forbidden */
> > -        if (access_type == 1) {
> > -            goto do_fault;
> > -        }
> > -        *prot &= ~PAGE_WRITE;
> > +
> > +    if ((*prot == 0)
> > +            || (!(*prot & PAGE_WRITE) && access_type == 1)
> > +            || (!(*prot & PAGE_EXEC) && access_type == 2)) {
> > +        goto do_fault;
> 
> Why isn't this just
>     if (!(*prot & (1 << access_type))) {

yeah, that would be better

> 
> ? (Or at least, why doesn't it treat PAGE_READ the same way
> as the other two bits?) As it is I think we'll treat a page
> that is marked exec-not-readable as if it were readable.

Oh yes, we should check PAGE_READ as well

Thanks for the review. I see from another mail that you'll be sending some
patches I should base the next version on. So I'll hold off on sending a
revised patch until I see that.

drew