Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones <drjones@redhat.com>]

On Wed, Mar 11, 2015 at 05:02:00PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 21:06, Andrew Jones <drjones@redhat.com> 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. It may even work in EL2,
> > when support for that comes, but, as the function name implies,
> > it only works for stage 1 translations.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> I like the general shape of this patch. Minor comment below:
> 
> > ---
> >  target-arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 100 insertions(+), 29 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index d996659652f8d..c457e9ab8c85a 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4962,15 +4962,11 @@ static inline int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> >  /* Translate section/page access permissions to page
> >   * R/W protection flags.
> >   *
> > - * @env:     CPUARMState
> > - * @mmu_idx: MMU index indicating required translation regime
> >   * @ap:      The 2-bit simple AP (AP[2:1])
> > + * @is_user: TRUE if accessing from PL0
> >   */
> > -static inline int
> > -simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> > +static inline int simple_ap_to_rw_prot_is_user(int ap, bool is_user)
> >  {
> > -    bool is_user = regime_is_user(env, mmu_idx);
> > -
> >      switch (ap) {
> >      case 0:
> >          return is_user ? 0 : PAGE_READ | PAGE_WRITE;
> > @@ -4985,6 +4981,94 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> >      }
> >  }
> >
> > +static inline int
> > +simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> > +{
> > +    return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
> > +}
> > +
> > +/* Translate section/page access permissions to protection flags
> > + *
> > + * @env:     CPUARMState
> > + * @mmu_idx: MMU index indicating required translation regime
> > + * @is_aa64: TRUE if AArch64
> > + * @ap:      The 2-bit simple AP (AP[2:1])
> > + * @ns:      NS (non-secure) bit
> > + * @xn:      XN (execute-never) bit
> > + * @pxn:     PXN (privileged execute-never) bit
> > + */
> > +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> > +                      int ap, int ns, int xn, int pxn)
> > +{
> > +    bool is_user = regime_is_user(env, mmu_idx);
> > +    int prot_rw, user_rw;
> > +    bool have_wxn;
> > +    int wxn = 0;
> > +
> > +    assert(mmu_idx != ARMMMUIdx_S2NS);
> > +
> > +    user_rw = simple_ap_to_rw_prot_is_user(ap, true);
> > +    if (is_user) {
> > +        prot_rw = user_rw;
> > +    } else {
> > +        prot_rw = simple_ap_to_rw_prot_is_user(ap, false);
> > +    }
> > +
> > +    if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> > +        return prot_rw;
> > +    }
> > +
> > +    /* TODO have_wxn should be replaced with
> > +     *   ARM_FEATURE_V8 || (ARM_FEATURE_V7 && ARM_FEATURE_EL2)
> > +     * when ARM_FEATURE_EL2 starts getting set. For now we assume all LPAE
> > +     * compatible processors have EL2, which is required for [U]WXN.
> > +     */
> > +    have_wxn = arm_feature(env, ARM_FEATURE_LPAE);
> > +
> > +    if (have_wxn) {
> > +        wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
> > +    }
> > +
> > +    if (is_aa64) {
> > +        switch (regime_el(env, mmu_idx)) {
> > +        case 1:
> > +            if (is_user && !user_rw) {
> > +                wxn = 0;
> 
> I don't understand this. We ignore the WXN bit if this is
> a user access and the page is not readable ?

Yup. If the page is not readable or writeable, AP[1]=0. I almost
submitted an errata to the ARM ARM when I saw this on the 2nd line
of table D4-32. I thought it must be a typo. However I tested it
on hardware, and it works this way. So at least the weirdness has
been implemented consistently...

> 
> I also find the naming of this variable "user_rw" (and
> to a lesser extent "prot_rw") very confusing. I keep
> misreading "if (user_rw)" as meaning "if this page is
> read-write for the user", when in fact it only means
> "if this page is readable for the user".

Right, user_rw really means ("user prot" & (PAGE_READ | PAGE_WRITE)),
but as you can't have PAGE_WRITE without PAGE_READ, then when user_rw
is non-zero we know it means PAGE_READ is set, but we still have to
check for PAGE_WRITE explicitly when we care about it.

> 
> Maybe it would be less confusing if we always did tests
> against a set of PAGE_* flags rather than doing an
> is/is-not-zero test?

Sounds good. Will respin with this change, and also adding that hunk
I forgot that collects NSTable bits.
> 
> The rest looked OK to me.

Thanks!

drew