Re: [PATCH 13/22] target/arm: Handle Block and Page bits for security space

[Peter Maydell]

On Tue, 24 Jan 2023 at 00:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> With Realm security state, bit 55 of a block or page descriptor during
> the stage2 walk becomes the NS bit; during the stage1 walk the bit 5
> NS bit is RES0.  With Root security state, bit 11 of the block or page
> descriptor during the stage1 walk becomes the NSE bit.
>
> Rather than collecting an NS bit and applying it later, compute the
> output pa space from the input pa space and unconditionally assign.
> This means that we no longer need to adjust the output space earlier
> for the NSTable bit.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/ptw.c | 74 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 60 insertions(+), 14 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index ddafb1f329..849f5e89ca 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1250,11 +1250,12 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>      int32_t stride;
>      int addrsize, inputsize, outputsize;
>      uint64_t tcr = regime_tcr(env, mmu_idx);
> -    int ap, ns, xn, pxn;
> +    int ap, xn, pxn;
>      uint32_t el = regime_el(env, mmu_idx);
>      uint64_t descaddrmask;
>      bool aarch64 = arm_el_is_aa64(env, el);
>      uint64_t descriptor, new_descriptor;
> +    ARMSecuritySpace out_space;
>
>      /* TODO: This code does not support shareability levels. */
>      if (aarch64) {
> @@ -1434,8 +1435,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>          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)) {
> @@ -1552,12 +1551,66 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>      }
>
>      ap = extract32(attrs, 6, 2);
> +    out_space = ptw->in_space;
>      if (regime_is_stage2(mmu_idx)) {
> -        ns = mmu_idx == ARMMMUIdx_Stage2;
> +        /*
> +         * R_GYNXY: For stage2 in Realm security state, bit 55 is NS.
> +         * The bit remains ignored for other security states.
> +         */
> +        if (out_space == ARMSS_Realm && extract64(attrs, 55, 1)) {
> +            out_space = ARMSS_NonSecure;
> +        }
>          xn = extract64(attrs, 53, 2);
>          result->f.prot = get_S2prot(env, ap, xn, s1_is_el0);
>      } else {
> -        ns = extract32(attrs, 5, 1);
> +        int ns = extract32(attrs, 5, 1);
> +        switch (out_space) {
> +        case ARMSS_Root:
> +            /*
> +             * R_GVZML: Bit 11 becomes the NSE field in the EL3 regime.
> +             * R_XTYPW: NSE and NS together select the output pa space.
> +             */
> +            int nse = extract32(attrs, 11, 1);
> +            out_space = (nse << 1) | ns;
> +            if (out_space == ARMSS_Secure &&
> +                !cpu_isar_feature(aa64_sel2, cpu)) {
> +                out_space = ARMSS_NonSecure;
> +            }
> +            break;
> +        case ARMSS_Secure:
> +            if (ns) {
> +                out_space = ARMSS_NonSecure;
> +            }
> +            break;
> +        case ARMSS_Realm:
> +            switch (mmu_idx) {
> +            case ARMMMUIdx_Stage1_E0:
> +            case ARMMMUIdx_Stage1_E1:
> +            case ARMMMUIdx_Stage1_E1_PAN:
> +                /* I_CZPRF: For Realm EL1&0 stage1, NS bit is RES0. */
> +                break;
> +            case ARMMMUIdx_E2:
> +            case ARMMMUIdx_E20_0:
> +            case ARMMMUIdx_E20_2:
> +            case ARMMMUIdx_E20_2_PAN:
> +                /*
> +                 * R_LYKFZ, R_WGRZN: For Realm EL2 and EL2&1,
> +                 * NS changes the output to non-secure space.
> +                 */
> +                if (ns) {
> +                    out_space = ARMSS_NonSecure;
> +                }
> +                break;
> +            default:
> +                g_assert_not_reached();
> +            }
> +            break;
> +        case ARMSS_NonSecure:
> +            /* R_QRMFF: For NonSecure state, the NS bit is RES0. */
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
>          xn = extract64(attrs, 54, 1);
>          pxn = extract64(attrs, 53, 1);
>          result->f.prot = get_S1prot(env, mmu_idx, aarch64, ap, ns, xn, pxn);

Various of these cases say "NS is RES0", but the code is
still extracting it from the attrs bit 5 and passing it
to get_S1prot(), which relies on being passed 1 for any case
where the translation table indicates that the memory is
Non-secure, whether the NS bit in the final block or page descriptor
was 1 or not (it uses it to implement the SCR_EL3.SIF bit).
This used to happen automatically because we set tableattrs to
1 << 4 if the initial access was non-secure, and then that bit
would always end up in attrs bit 5 regardless of what the various
NSTable and NS bits in the descriptors were, but the refactoring
in the previous patch changes that and so we need to do something
else I think.

thanks
-- PMM