[v4,6/7] target/arm: Add PMSAv8r functionality

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 | 130 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 110 insertions(+), 20 deletions(-)

On Sun, 23 Oct 2022 at 16:37, <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 | 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