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

[Andrew Jones]

On Fri, Jan 16, 2015 at 04:16:02PM +0000, Peter Maydell wrote:
> 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.

OK
> 
> 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.

The code it's replacing wasn't using the V8 feature bit correctly.
It was using it to determine AArch64 vs. AArch32, not just V8 vs.
V7.

> 
> > + *
> > + * @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)

Thanks. I see from another mail that you've opted to get the
translation regime worked up sooner than later. I'll wait for
that to base a revision of this patch on.

> 
> > +    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.

When EL0 doesn't have read/write, it can still execute (that's
what the first patch of this 2 patch series addressed). It can
still execute even when WXN is set, which is what this condition
addresses. It wasn't clear from the table that that's how it
should work, but it doesn't say it doesn't work that way either,
and real hardware shows WXN gets ignored.

> 
> > +            pxn = pxn || ((ap & 1) && !(ap & 2));
> 
> Very weird way to write "ap == 1" ?

True, I guess I was trying to code too closely to the
table in the ARM ARM wrt to AP[2: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.

OK

> 
> > +        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.)

I didn't see WXN defined for secure state of AArch32/V7.
Maybe I missed it?

> 
> 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.)

I don't completely understand how much EL1 behavior is
present at EL3 (always all?). Anyway, I left out UWXN here,
because I don't recall explicitly seeing it defined for EL3.
Maybe I missed it?

> 
> > +        }
> > +        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;
> 

Agreed. I was getting too close to the spec, and wanting to see
AP[2:1] like in the table. Using PAGE_* would look much nicer in
the code. I'll change it.

> 
> > +}
> > +
> >  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

Thank you!

drew