Re: [PATCH 09/22] target/arm: Adjust the order of Phys and Stage2 ARMMMUIdx

[Peter Maydell]

On Tue, 24 Jan 2023 at 00:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> It will be helpful to have ARMMMUIdx_Phys_* to be in the same
> relative order as ARMSecuritySpace enumerators. This requires
> the adjustment to the nstable check. While there, check for being
> in secure state rather than rely on clearing the low bit making
> no change to non-secure state.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h | 12 ++++++------
>  target/arm/ptw.c | 10 ++++------
>  2 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index cfc62d60b0..0114e1ed87 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3057,18 +3057,18 @@ typedef enum ARMMMUIdx {
>      ARMMMUIdx_E2        = 6 | ARM_MMU_IDX_A,
>      ARMMMUIdx_E3        = 7 | ARM_MMU_IDX_A,
>
> -    /* TLBs with 1-1 mapping to the physical address spaces. */
> -    ARMMMUIdx_Phys_NS   = 8 | ARM_MMU_IDX_A,
> -    ARMMMUIdx_Phys_S    = 9 | ARM_MMU_IDX_A,
> -
>      /*
>       * Used for second stage of an S12 page table walk, or for descriptor
>       * loads during first stage of an S1 page table walk.  Note that both
>       * are in use simultaneously for SecureEL2: the security state for
>       * the S2 ptw is selected by the NS bit from the S1 ptw.
>       */
> -    ARMMMUIdx_Stage2    = 10 | ARM_MMU_IDX_A,
> -    ARMMMUIdx_Stage2_S  = 11 | ARM_MMU_IDX_A,
> +    ARMMMUIdx_Stage2_S  = 8 | ARM_MMU_IDX_A,
> +    ARMMMUIdx_Stage2    = 9 | ARM_MMU_IDX_A,
> +
> +    /* TLBs with 1-1 mapping to the physical address spaces. */
> +    ARMMMUIdx_Phys_S    = 10 | ARM_MMU_IDX_A,
> +    ARMMMUIdx_Phys_NS   = 11 | ARM_MMU_IDX_A,
>
>      /*
>       * These are not allocated TLBs and are used only for AT system
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 437f6fefa9..59cf64d0a6 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1410,16 +1410,14 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>      descaddr |= (address >> (stride * (4 - level))) & indexmask;
>      descaddr &= ~7ULL;
>      nstable = extract32(tableattrs, 4, 1);
> -    if (nstable) {
> +    if (nstable && ptw->in_secure) {
>          /*
>           * Stage2_S -> Stage2 or Phys_S -> Phys_NS
>           * Assert that the non-secure idx are even, and relative order.
>           */

Comment needs updating to match the code change (we're no
longer asserting that the NS indexes are even).

> -        QEMU_BUILD_BUG_ON((ARMMMUIdx_Phys_NS & 1) != 0);
> -        QEMU_BUILD_BUG_ON((ARMMMUIdx_Stage2 & 1) != 0);
> -        QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_NS + 1 != ARMMMUIdx_Phys_S);
> -        QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2 + 1 != ARMMMUIdx_Stage2_S);
> -        ptw->in_ptw_idx &= ~1;
> +        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;
>      }
>      if (!S1_ptw_translate(env, ptw, descaddr, fi)) {

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM