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

[Peter Maydell <peter.maydell@linaro.org>]

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 ?

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

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?

The rest looked OK to me.

-- PMM