Re: [PATCH v5 6/7] target/arm: Add PMSAv8r functionality

[Peter Maydell]

On Sun, 27 Nov 2022 at 13:21, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> Add PMSAv8r translation.
>
> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> ---
>  target/arm/ptw.c | 127 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 105 insertions(+), 22 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 7d19829702..0514a83c1b 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1758,9 +1758,14 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, 
> ARMMMUIdx mmu_idx,
>
>      if (arm_feature(env, ARM_FEATURE_M)) {
>          return env->v7m.mpu_ctrl[is_secure] & R_V7M_MPU_CTRL_PRIVDEFENA_MASK;
> -    } else {
> -        return regime_sctlr(env, mmu_idx) & SCTLR_BR;
>      }
> +
> +    if (arm_feature(env, ARM_FEATURE_V8) &&
> +        ((mmu_idx == ARMMMUIdx_Stage2) || (mmu_idx == ARMMMUIdx_Stage1_E0))) 
> {
> +        return false;
> +    }

You don't need to check mmu_idx == ARMMMUIdx_Stage1_E0 here,
because we've already returned false if is_user earlier.
You don't need to check for ARM_FEATURE_V8, because we won't
ever get here with ARMMMUIdx_Stage2 unless we have v8. So
this simplifies to

if (mmu_idx == ARMMMUIdx_Stage2) {
    return false;
}

> +
> +    return regime_sctlr(env, mmu_idx) & SCTLR_BR;
>  }

> @@ -2074,21 +2126,46 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t 
> address,
>              xn = 1;
>          }
>
> -        result->f.prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> +        if (regime_el(env, mmu_idx) == 2) {
> +            result->f.prot = simple_ap_to_rw_prot_is_user(ap,
> +                                            mmu_idx != ARMMMUIdx_E2);
> +        } else {
> +            result->f.prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> +        }
> +
> +        if (!arm_feature(env, ARM_FEATURE_M)) {
> +
> +            if (regime_sctlr(env, mmu_idx) & SCTLR_WXN &&
> +                result->f.prot & PAGE_WRITE && mmu_idx != ARMMMUIdx_Stage2) {
> +                xn = 0x1;
> +            }
> +
> +            if ((regime_el(env, mmu_idx) == 1) &&
> +                regime_sctlr(env, mmu_idx) & SCTLR_UWXN && ap == 0x1) {
> +                pxn = 0x1;
> +            }
> +
> +            uint8_t attrindx = extract32(matched_rlar, 1, 3);
> +            uint64_t mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
> +            uint8_t sh = extract32(matched_rlar, 3, 2);

Don't declare variables in the middle of a code block, please.

> +            result->cacheattrs.is_s2_format = false;
> +            result->cacheattrs.attrs = extract64(mair, attrindx * 8, 8);
> +            result->cacheattrs.shareability = sh;
> +        }
> +
>          if (result->f.prot && !xn && !(pxn && !is_user)) {
>              result->f.prot |= PAGE_EXEC;
>          }
> -        /*
> -         * We don't need to look the attribute up in the MAIR0/MAIR1
> -         * registers because that only tells us about cacheability.
> -         */
> +
>          if (mregion) {
>              *mregion = matchregion;
>          }
>      }
>
>      fi->type = ARMFault_Permission;
> -    fi->level = 1;
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        fi->level = 1;
> +    }
>      return !(result->f.prot & (1 << access_type));
>  }
>
> @@ -2649,7 +2726,13 @@ static bool get_phys_addr_twostage(CPUARMState *env, 
> S1Translate *ptw,
>      cacheattrs1 = result->cacheattrs;
>      memset(result, 0, sizeof(*result));
>
> -    ret = get_phys_addr_lpae(env, ptw, ipa, access_type, is_el0, result, fi);
> +    if (arm_feature(env, ARM_FEATURE_PMSA)) {
> +        ret = get_phys_addr_pmsav8(env, ipa, access_type,
> +                                   ptw->in_mmu_idx, is_secure, result, fi);
> +    } else {
> +        ret = get_phys_addr_lpae(env, ptw, ipa, access_type,
> +                                 is_el0, result, fi);
> +    }
>      fi->s2addr = ipa;
>
>      /* Combine the S1 and S2 perms.  */

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM