Re: [PATCH 12/22] target/arm: NSTable is RES0 for the RME EL3 regime

[Peter Maydell]

On Tue, 24 Jan 2023 at 00:01, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Test in_space instead of in_secure so that we don't switch
> out of Root space.  Handle the output space change immediately,
> rather than try and combine the NSTable and NS bits later.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/ptw.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index c1b0b8e610..ddafb1f329 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1240,7 +1240,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>  {
>      ARMCPU *cpu = env_archcpu(env);
>      ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
> -    bool is_secure = ptw->in_secure;
>      int32_t level;
>      ARMVAParameters param;
>      uint64_t ttbr;
> @@ -1256,7 +1255,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>      uint64_t descaddrmask;
>      bool aarch64 = arm_el_is_aa64(env, el);
>      uint64_t descriptor, new_descriptor;
> -    bool nstable;
>
>      /* TODO: This code does not support shareability levels. */
>      if (aarch64) {
> @@ -1417,29 +1415,29 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>          descaddrmask = MAKE_64BIT_MASK(0, 40);
>      }
>      descaddrmask &= ~indexmask_grainsize;
> -
> -    /*
> -     * Secure accesses start with the page table in secure memory and
> -     * can be downgraded to non-secure at any step. Non-secure accesses
> -     * remain non-secure. We implement this by just ORing in the NSTable/NS
> -     * bits at each step.
> -     */
> -    tableattrs = is_secure ? 0 : (1 << 4);
> +    tableattrs = 0;
>
>   next_level:
>      descaddr |= (address >> (stride * (4 - level))) & indexmask;
>      descaddr &= ~7ULL;
> -    nstable = extract32(tableattrs, 4, 1);
> -    if (nstable && ptw->in_secure) {
> -        /*
> -         * Stage2_S -> Stage2 or Phys_S -> Phys_NS
> -         * Assert that the non-secure idx are even, and relative order.
> -         */
> +
> +    /*
> +     * Process the NSTable bit from the previous level.  This changes
> +     * the table address space and the output space from Secure to
> +     * NonSecure.  With RME, the EL3 translation regime does not change
> +     * from Root to NonSecure.
> +     */

To check my understanding, this means that the bit that the spec
describes as FEAT_RME changing the behaviour of NSTable in the EL3
stage 1 translation regime is implemented by us by having the
in_space for EL3 be different for FEAT_RME and not-FEAT_RME ?

> +    if (extract32(tableattrs, 4, 1) && ptw->in_space == ARMSS_Secure) {
> +        /* Stage2_S -> Stage2 or Phys_S -> Phys_NS */
>          QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_S + 1 != ARMMMUIdx_Phys_NS);
>          QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2_S + 1 != ARMMMUIdx_Stage2);
>          ptw->in_ptw_idx += 1;
>          ptw->in_secure = false;
> +        ptw->in_space = ARMSS_NonSecure;
> +        result->f.attrs.secure = false;
> +        result->f.attrs.space = ARMSS_NonSecure;
>      }
> +
>      if (!S1_ptw_translate(env, ptw, descaddr, fi)) {
>          goto do_fault;
>      }
> @@ -1542,7 +1540,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>       */
>      attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 
> 14));
>      if (!regime_is_stage2(mmu_idx)) {
> -        attrs |= nstable << 5; /* NS */

This removes the code where we copy the NSTable bit across to attrs,
but there's still code below here that assumes it can get the combined
NS bit from bit 5 of attrs, isn't there? (It passes it to get_S1prot().)

thanks
-- PMM