Re: [PATCH v3 8/9] target/arm: Add PMSAv8r functionality

[Peter Maydell]

On Sat, 20 Aug 2022 at 15:19, <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 | 171 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 150 insertions(+), 21 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index c4f5721012..c7e37c66d0 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -140,6 +140,9 @@ static bool regime_translation_disabled(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>               */
>              return true;
>          }
> +    } else if (arm_feature(env, ARM_FEATURE_V8_R)) {

In general I think you can replace all these checks on
ARM_FEATURE_V8_R with suitable other tests. I've suggested
various ones in the code below.

> +        return !(regime_sctlr(env, mmu_idx) & SCTLR_M) ||
> +        (!(regime_el(env, mmu_idx) == 2) && arm_hcr_el2_eff(env) & HCR_TGE);
>      }

This doesn't look right. What goes wrong if you just use the v8A code?
In particular v8R still needs to consider HCR.TGE and HCR.DC, same
as v8A, as far as I can see.

>      hcr_el2 = arm_hcr_el2_eff(env);
> @@ -1504,6 +1507,8 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, 
> ARMMMUIdx mmu_idx,
>      if (arm_feature(env, ARM_FEATURE_M)) {
>          return env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)]
>              & R_V7M_MPU_CTRL_PRIVDEFENA_MASK;
> +    } else if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        return false;
>      } else {
>          return regime_sctlr(env, mmu_idx) & SCTLR_BR;
>      }

What's going on here? v8R still has SCTLR.BR to enable the background region.

> @@ -1698,6 +1703,77 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, 
> uint32_t address,
>      return !(*prot & (1 << access_type));
>  }
>
> +static uint32_t *regime_rbar(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                             uint32_t secure)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        if (regime_el(env, mmu_idx) == 2) {
> +            return env->pmsav8.hprbarn;
> +        } else {
> +            return env->pmsav8.prbarn;
> +        }
> +    } else {
> +         return env->pmsav8.rbar[secure];
> +    }

If you set up the CPU state fields correctly (as noted in the
previous patch) then you shouldn't need to test a feature bit
at all here -- M profile and R profile should be able to share
the same pmsav8.rbar[] array for stage 1, and for M-profile
regime_el() will never be 2 so it won't go into those parts
of the function.

> +}
> +
> +static uint32_t *regime_rlar(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                             uint32_t secure)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        if (regime_el(env, mmu_idx) == 2) {
> +            return env->pmsav8.hprlarn;
> +        } else {
> +            return env->pmsav8.prlarn;
> +        }
> +    } else {
> +        return env->pmsav8.rlar[secure];
> +    }
> +}
> +
> +static inline void get_phys_addr_pmsav8_default(CPUARMState *env,
> +                                                ARMMMUIdx mmu_idx,
> +                                                uint32_t address, int *prot)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        *prot = PAGE_READ | PAGE_WRITE;
> +        if (address <= 0x7FFFFFFF) {
> +            *prot |= PAGE_EXEC;
> +        }
> +        if ((regime_el(env, mmu_idx) == 2)
> +            && (regime_sctlr(env, mmu_idx) & SCTLR_WXN)
> +            && (regime_sctlr(env, mmu_idx) & SCTLR_M)) {
> +            *prot &= ~PAGE_EXEC;
> +        }

Put the default handling in get_phys_addr_pmsav7_default(), not
as a wrapper function around it. In particular the stage 1
default is the same as the existing R-profile default map,
so you can just use that code. And PMSAv7 will never have
stage 2, so you can add any code to handle stage 2's default
map without conditionalizing it on "is this v8R?".

Trying to handle SCTLR.WXN and SCTLR.M here looks wrong in
two ways:
(1) the default memory maps are not documented as depending on
either of those two bits
(2) those bits don't affect only stage 2.

> +    } else {
> +        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +    }
> +}
> +
> +static bool pmsav8_fault(bool hit, CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        if (regime_el(env, mmu_idx) == 2) {
> +            if (!hit && (mmu_idx != ARMMMUIdx_E2)) {
> +                return true;
> +            } else if (!hit && (mmu_idx == ARMMMUIdx_E2)
> +                       &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> +                return true;
> +            }
> +        } else {
> +            if (!hit && (mmu_idx != ARMMMUIdx_Stage1_E1)) {
> +                return true;
> +            } else if (!hit && (mmu_idx == ARMMMUIdx_Stage1_E1)
> +                       &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> +                return true;
> +            }
> +        }
> +        return false;
> +    } else {
> +        return !hit;
> +    }
> +}
> +
>  bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
>                         MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                         hwaddr *phys_ptr, MemTxAttrs *txattrs,
> @@ -1730,6 +1806,12 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t 
> address,
>          *mregion = -1;
>      }
>
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        if (mmu_idx == ARMMMUIdx_Stage2) {
> +            fi->stage2 = true;
> +        }

No need to guard this with a V8_R check, because there's no other
way you'll get here with a stage 2 mmu index.

> +    }
> +
>      /*
>       * 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
> @@ -1746,17 +1828,26 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t 
> address,
>              hit = true;
>          }
>
> +        uint32_t bitmask;
> +        if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +            bitmask = 0x3f;
> +        } else {
> +            bitmask = 0x1f;
> +        }

You can avoid a v8R check here by doing an "is this M profile" feature
test instead.

> +
> +
>          for (n = (int)cpu->pmsav7_dregion - 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;
>              }
> @@ -1799,22 +1890,25 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t 
> address,
>          }
>      }
>
> -    if (!hit) {
> -        /* background fault */
> -        fi->type = ARMFault_Background;
> +    if (pmsav8_fault(hit, env, mmu_idx)) {
> +        fi->type = ARMFault_Permission;
> +        fi->level = 0;
>          return true;
>      }

This code change looks like it's going to break the v8M reporting
of background faults.

>
>      if (matchregion == -1) {
>          /* hit using the background region */
> -        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +        get_phys_addr_pmsav8_default(env, mmu_idx, address, 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 ap = extract32(regime_rbar(env,
> +                                mmu_idx, secure)[matchregion], 1, 2);
> +        uint32_t xn = extract32(regime_rbar(env,
> +                                mmu_idx, secure)[matchregion], 0, 1);
>          bool pxn = false;
>
>          if (arm_feature(env, ARM_FEATURE_V8_1M)) {
> -            pxn = extract32(env->pmsav8.rlar[secure][matchregion], 4, 1);
> +            pxn = extract32(regime_rlar(env,
> +                            mmu_idx, secure)[matchregion], 4, 1);
>          }
>
>          if (m_is_system_region(env, address)) {
> @@ -1822,14 +1916,42 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t 
> address,
>              xn = 1;
>          }
>
> -        *prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> +        if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +            if (regime_el(env, mmu_idx) == 2) {
> +                *prot = simple_ap_to_rw_prot_is_user(ap,
> +                                                mmu_idx != ARMMMUIdx_E2);
> +            } else {
> +                *prot = simple_ap_to_rw_prot_is_user(ap,
> +                                                mmu_idx != 
> ARMMMUIdx_Stage1_E1);
> +            }
> +
> +            if (regime_sctlr(env, mmu_idx) & SCTLR_WXN
> +                && (*prot & PAGE_WRITE)) {
> +                xn = 0x1;
> +            }
> +
> +            if ((regime_el(env, mmu_idx) == 1) && regime_sctlr(env, mmu_idx)
> +                 & SCTLR_UWXN && (ap == 0x1)) {
> +                xn = 0x1;
> +            }
> +
> +            uint8_t attrindx = extract32(regime_rlar(env,
> +                                         mmu_idx, secure)[matchregion], 1, 
> 3);
> +            uint64_t mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
> +            uint8_t sh = extract32(regime_rlar(env,
> +                                   mmu_idx, secure)[matchregion], 3, 2);

The accesses to the rlar/rbar for the matched region have now got cumbersome
enough that we should put them in a local variable at the top of the if().

> +            assert(attrindx <= 4);

What is this assert for? The spec says bits [3:1] can take all of the
8 possible values.

> +            cacheattrs->is_s2_format = false;
> +            cacheattrs->attrs = extract64(mair, attrindx * 8, 8);
> +            cacheattrs->shareability = sh;
> +        } else {
> +            *prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> +        }
> +
>          if (*prot && !xn && !(pxn && !is_user)) {
>              *prot |= PAGE_EXEC;
>          }
> -        /*
> -         * We don't need to look the attribute up in the MAIR0/MAIR1
> -         * registers because that only tells us about cacheability.
> -         */

Why drop the comment ? It's still relevant for M profile, so should move
into the M-profile arm of the above check.

> +
>          if (mregion) {
>              *mregion = matchregion;
>          }
> @@ -2342,9 +2464,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong 
> address,
>              is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == 
> ARMMMUIdx_SE10_0;
>
>              /* S1 is done. Now do S2 translation.  */
> -            ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx, 
> is_el0,
> -                                     phys_ptr, attrs, &s2_prot,
> -                                     page_size, fi, &cacheattrs2);
> +            if (arm_feature(env, ARM_FEATURE_V8_R)) {

"if PMSA" would be a more obvious check to test here.

> +                ret = get_phys_addr_pmsav8(env, ipa, access_type, s2_mmu_idx,
> +                                       phys_ptr, attrs, &s2_prot, page_size,
> +                                       fi, &cacheattrs2);
> +            } else {
> +                ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
> +                                      is_el0, phys_ptr, attrs, &s2_prot,
> +                                      page_size, fi, &cacheattrs2);
> +            }
> +
>              fi->s2addr = ipa;
>              /* Combine the S1 and S2 perms.  */
>              *prot &= s2_prot;
> --
> 2.25.1

thanks
-- PMM