Re: [PATCH 1/5] target/arm/ptw: Load stage-2 tables from realm physical space

[Peter Maydell]

On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> In realm state, stage-2 translation tables are fetched from the realm
> physical address space (R_PGRQD).
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  target/arm/ptw.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index d1de934702..6318e13b98 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -164,7 +164,11 @@ static ARMMMUIdx ptw_idx_for_stage_2(CPUARMState *env, 
> ARMMMUIdx stage2idx)
>       * an NS stage 1+2 lookup while the NS bit is 0.)
>       */
>      if (!arm_is_secure_below_el3(env) || !arm_el_is_aa64(env, 3)) {
> -        return ARMMMUIdx_Phys_NS;
> +        if (arm_security_space_below_el3(env) == ARMSS_Realm) {
> +            return ARMMMUIdx_Phys_Realm;
> +        } else {
> +            return ARMMMUIdx_Phys_NS;
> +        }
>      }
>      if (stage2idx == ARMMMUIdx_Stage2_S) {
>          s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);

This isn't wrong, but arm_is_secure_below_el3()
calls arm_security_space_below_el3(), so we kinda
duplicate work there. I think we should instead have:

    if (!arm_el_is_aa64(env, 3)) {
        return ARMMMUIdx_Phys_NS;
    }

    switch (arm_security_space_below_el3(env)) {
    case ARMSS_NonSecure:
        return ARMMUIdx_Phys_NS;
    case ARMSS_Realm:
        return ARMMMUIdx_Phys_Realm;
    case ARMSS_Secure:
        [existing code to look at the SW/NSW bits]
        return s2walk_secure ? ...;
    default:
        g_assert_not_reached();
    }

The comment above the function also needs tweaking
to say "SCR_EL3.NS or SCR_EL3.NSE bits" (we do already
do the TLB flush in scr_write).

thanks
-- PMM