On Sun, 23 Oct 2022 at 16:37, <tobias.roeh...@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roeh...@rwth-aachen.de>
>
> Add PMSAv8r translation.
>
> Signed-off-by: Tobias Röhmel <tobias.roeh...@rwth-aachen.de>
> ---
> target/arm/ptw.c | 130 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 110 insertions(+), 20 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 4bd7389fa9..a5d890c09a 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1503,6 +1503,23 @@ 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 if (arm_feature(env, ARM_FEATURE_PMSA)) {
> + if (regime_el(env, mmu_idx) == 2) {
> + if (mmu_idx != ARMMMUIdx_E2) {
> + return false;
> + } else if ((mmu_idx == ARMMMUIdx_E2)
> + &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> + return false;
> + }
> + } else {
> + if (mmu_idx != ARMMMUIdx_Stage1_E1) {
> + return false;
> + } else if ((mmu_idx == ARMMMUIdx_Stage1_E1)
> + &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> + return false;
> + }
> + }
> + return true;
> } else {
> return regime_sctlr(env, mmu_idx) & SCTLR_BR;
> }
This logic seems rather confused:
(1) it's not possible to get to this function unless ARM_FEATURE_PMSA,
so the explicit check for that means the final "else" clause is now
unreachable code.
(2) You check for eg "mmu_idx != ARMMMUIdx_E2" and return early,
but that means that in the following
((mmu_idx == ARMMMUIdx_E2)
&&!(regime_sctlr(env, mmu_idx) & SCTLR_BR))
the first clause of the && is always true and is redundant.
(3) the thing we end up actually checking (SCTLR_BR for the
regime SCTLR) is the same now in all three major branches of
this code, so there's a lot of redundancy.
I think what you actually want here is to identify the set
mmu index values (if any!) that we don't want to do the
SCTLR_BR check for. Then return early for those, and have
a single
return regime_sctlr(env, mmu_idx) & SCTLR_BR;
for all the cases where checking BR is the right thing.
I think it may actually be the case that there is no mmuidx
value where we don't want to do the SCLTR_BR check, in
which case we don't need to change this function at all.
> @@ -1696,6 +1713,26 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
> uint32_t address,
> return !(result->prot & (1 << access_type));
> }
>
> +static uint32_t *regime_rbar(CPUARMState *env, ARMMMUIdx mmu_idx,
> + uint32_t secure)
> +{
> + if (regime_el(env, mmu_idx) == 2) {
> + return env->pmsav8.hprbar[secure];
> + } else {
> + return env->pmsav8.rbar[secure];
> + }
> +}
> +
> +static uint32_t *regime_rlar(CPUARMState *env, ARMMMUIdx mmu_idx,
> + uint32_t secure)
> +{
> + if (regime_el(env, mmu_idx) == 2) {
> + return env->pmsav8.hprlar[secure];
> + } else {
> + return env->pmsav8.rlar[secure];
> + }
> +}
> +
> bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> MMUAccessType access_type, ARMMMUIdx mmu_idx,
> bool secure, GetPhysAddrResult *result,
> @@ -1733,6 +1770,10 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t
> address,
> *mregion = -1;
> }
>
> + if (mmu_idx == ARMMMUIdx_Stage2) {
> + fi->stage2 = true;
> + }
> +
> /*
> * Unlike the ARM ARM pseudocode, we don't need to check whether this
> * was an exception vector read from the vector table (which is always
> @@ -1749,17 +1790,27 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t
> address,
> hit = true;
> }
>
> + uint32_t bitmask;
> + if (arm_feature(env, ARM_FEATURE_M)) {
> + bitmask = 0x1f;
> + fi->level = 1;
> + } else {
> + bitmask = 0x3f;
> + fi->level = 0;
> + }
We could in theory clean this up a bit, because on M-profile the
"FSR" value is not guest-facing; we only use it internally to pass
around some details of the fault cause, so as long as we're consistent
between the code here that sets fi->level and the code in m_helper.c
that looks at the FSC values we can set it to any value that's convenient.
But for this patchset we should definitely leave the existing M-profile
behaviour the way it is. I might come back and tweak the code once
this R-profile series has landed (or I might not think it worth bothering
and leave it indefinitely :-))
> +
> for (n = region_counter - 1; n >= 0; n--) {
> /* region search */
> /*
> - * Note that the base address is bits [31:5] from the register
> - * with bits [4:0] all zeroes, but the limit address is bits
> - * [31:5] from the register with bits [4:0] all ones.
> + * Note that the base address is bits [31:x] from the register
> + * with bits [x-1:0] all zeroes, but the limit address is bits
> + * [31:x] from the register with bits [x:0] all ones. Where x is
> + * 5 for Cortex-M and 6 for Cortex-R
> */
> - uint32_t base = env->pmsav8.rbar[secure][n] & ~0x1f;
> - uint32_t limit = env->pmsav8.rlar[secure][n] | 0x1f;
> + uint32_t base = regime_rbar(env, mmu_idx, secure)[n] & ~bitmask;
> + uint32_t limit = regime_rlar(env, mmu_idx, secure)[n] | bitmask;
>
> - if (!(env->pmsav8.rlar[secure][n] & 0x1)) {
> + if (!(regime_rlar(env, mmu_idx, secure)[n] & 0x1)) {
> /* Region disabled */
> continue;
> }
> @@ -1793,7 +1844,6 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t
> address,
> * PMSAv7 where highest-numbered-region wins)
> */
> fi->type = ARMFault_Permission;
> - fi->level = 1;
> return true;
> }
>
> @@ -1803,8 +1853,11 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t
> address,
> }
>
> if (!hit) {
> - /* background fault */
> - fi->type = ARMFault_Background;
> + if (arm_feature(env, ARM_FEATURE_M)) {
> + fi->type = ARMFault_Background;
> + } else {
> + fi->type = ARMFault_Permission;
> + }
> return true;
> }
>
> @@ -1812,12 +1865,14 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t
> address,
> /* hit using the background region */
> get_phys_addr_pmsav7_default(env, mmu_idx, address, &result->prot);
> } else {
> - uint32_t ap = extract32(env->pmsav8.rbar[secure][matchregion], 1, 2);
> - uint32_t xn = extract32(env->pmsav8.rbar[secure][matchregion], 0, 1);
> + uint32_t matched_rbar = regime_rbar(env, mmu_idx,
> secure)[matchregion];
> + uint32_t matched_rlar = regime_rlar(env, mmu_idx,
> secure)[matchregion];
> + uint32_t ap = extract32(matched_rbar, 1, 2);
> + uint32_t xn = extract32(matched_rbar, 0, 1);
> bool pxn = false;
>
> if (arm_feature(env, ARM_FEATURE_V8_1M)) {
> - pxn = extract32(env->pmsav8.rlar[secure][matchregion], 4, 1);
> + pxn = extract32(matched_rlar, 4, 1);
> }
>
> if (m_is_system_region(env, address)) {
> @@ -1825,21 +1880,49 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t
> address,
> xn = 1;
> }
>
> - result->prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> + if (arm_feature(env, ARM_FEATURE_M)) {
> + /*
> + * We don't need to look the attribute up in the MAIR0/MAIR1
> + * registers because that only tells us about cacheability.
> + */
> + result->prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> + } else {
> + if (regime_el(env, mmu_idx) == 2) {
> + result->prot = simple_ap_to_rw_prot_is_user(ap,
> + mmu_idx != ARMMMUIdx_E2);
> + } else {
> + result->prot = simple_ap_to_rw_prot_is_user(ap,
> + mmu_idx !=
> ARMMMUIdx_Stage1_E1);
This second one should be fine as just
result->prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
I think, because this is the EL1 case.
That in turn means you don't need to have the M-profile case separately,
because M-profile will never have the regime EL be 2.
> + }
> +
> + if (regime_sctlr(env, mmu_idx) & SCTLR_WXN
> + && (result->prot & PAGE_WRITE)) {
> + xn = 0x1;
> + }
I think that this will apply HSCTLR.WXN on the stage 2 check for
EL1/EL0 accesses, which I don't think is correct. In the pseudocode
HSCTLR.WXN is checked only for stage 1 accesses from EL2, not as
part of stage 2 checking (see AArch32.CheckPermission()).
> +
> + if ((regime_el(env, mmu_idx) == 1) && regime_sctlr(env, mmu_idx)
> + & SCTLR_UWXN && (ap == 0x1)) {
Don't break the line like this, it implies that the precedence
of & is less than that of &&, which it isn't.
> + xn = 0x1;
This should be setting pxn = 0x1, because SCTLR.UWXN only means
"unprivileged write permission implies EL1 XN", not "implies XN generally".
> + }
> +
> + 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);
> + result->cacheattrs.is_s2_format = false;
> + result->cacheattrs.attrs = extract64(mair, attrindx * 8, 8);
> + result->cacheattrs.shareability = sh;
> + }
> +
> if (result->prot && !xn && !(pxn && !is_user)) {
> result->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;
> return !(result->prot & (1 << access_type));
> }
>
> @@ -2348,8 +2431,15 @@ bool get_phys_addr(CPUARMState *env, target_ulong
> address,
> cacheattrs1 = result->cacheattrs;
> memset(result, 0, sizeof(*result));
>
> - ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
> - is_el0, result, fi);
> + /* S1 is done. Now do S2 translation. */
> + if (arm_feature(env, ARM_FEATURE_PMSA)) {
> + ret = get_phys_addr_pmsav8(env, ipa, access_type, s2_mmu_idx,
> + is_secure, result, fi);
> + } else {
> + ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
> + is_el0, result, fi);
> + }
> +
This bit of code has unfortunately changed a lot due to a
recent refactoring landing...
> fi->s2addr = ipa;
>
> /* Combine the S1 and S2 perms. */
> --
> 2.34.1
thanks
-- PMM
