Re: [PATCH 08/14] target/arm: add MMU stage 1 for Secure EL2

[Richard Henderson]

On 11/2/20 2:57 AM, remi.denis.courmont@huawei.com wrote:
> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> 
> This adds the MMU indices for EL2 stage 1 in secure mode.
> 
> To keep code contained, which is largelly identical between secure and
> non-secure modes, this patch introduces a secure bit for all new and
> existing stage 1 translation regimes.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> ---
>  target/arm/cpu-param.h     |   2 +-
>  target/arm/cpu.h           |  22 ++++--
>  target/arm/helper.c        | 143 ++++++++++++++++++++++++-------------
>  target/arm/internals.h     |  12 ++++
>  target/arm/translate-a64.c |   4 ++
>  5 files changed, 127 insertions(+), 56 deletions(-)
> 
> diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
> index 6321385b46..0db5e37c17 100644
> --- a/target/arm/cpu-param.h
> +++ b/target/arm/cpu-param.h
> @@ -29,6 +29,6 @@
>  # define TARGET_PAGE_BITS_MIN  10
>  #endif
>  
> -#define NB_MMU_MODES 11
> +#define NB_MMU_MODES 16
>  
>  #endif
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 724b11ee57..3fbb70e273 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2944,6 +2944,9 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync);
>  #define ARM_MMU_IDX_NOTLB 0x20  /* does not have a TLB */
>  #define ARM_MMU_IDX_M     0x40  /* M profile */
>  
> +/* Meanings of the bits for A profile mmu idx values */
> +#define ARM_MMU_IDX_A_S      0x8
> +
>  /* Meanings of the bits for M profile mmu idx values */
>  #define ARM_MMU_IDX_M_PRIV   0x1
>  #define ARM_MMU_IDX_M_NEGPRI 0x2
> @@ -2967,10 +2970,17 @@ typedef enum ARMMMUIdx {
>      ARMMMUIdx_E20_2      =  5 | ARM_MMU_IDX_A,
>      ARMMMUIdx_E20_2_PAN  =  6 | ARM_MMU_IDX_A,
>  
> -    ARMMMUIdx_SE10_0     = 7 | ARM_MMU_IDX_A,
> -    ARMMMUIdx_SE10_1     = 8 | ARM_MMU_IDX_A,
> -    ARMMMUIdx_SE10_1_PAN = 9 | ARM_MMU_IDX_A,
> -    ARMMMUIdx_SE3        = 10 | ARM_MMU_IDX_A,
> +    ARMMMUIdx_SE10_0     = ARMMMUIdx_E10_0 | ARM_MMU_IDX_A_S,
> +    ARMMMUIdx_SE20_0     = ARMMMUIdx_E20_0 | ARM_MMU_IDX_A_S,
> +
> +    ARMMMUIdx_SE10_1     = ARMMMUIdx_E10_1 | ARM_MMU_IDX_A_S,
> +    ARMMMUIdx_SE10_1_PAN = ARMMMUIdx_E10_1_PAN | ARM_MMU_IDX_A_S,
> +
> +    ARMMMUIdx_SE2        = ARMMMUIdx_E2 | ARM_MMU_IDX_A_S,
> +    ARMMMUIdx_SE20_2     = ARMMMUIdx_E20_2 | ARM_MMU_IDX_A_S,
> +    ARMMMUIdx_SE20_2_PAN = ARMMMUIdx_E20_2_PAN | ARM_MMU_IDX_A_S,
> +
> +    ARMMMUIdx_SE3        = 15 | ARM_MMU_IDX_A,

Hum.  So, we're adding 4 new mmu_idx, and increasing the mmu_idx count by 5.
The unused index would be 7 -- no non-secure el3.

Is it worth reversing the S bit to NS, so that index 15 becomes the one that is
unused, and so we don't actually have to add it to NB_MMU_MODES?

> @@ -3649,7 +3655,7 @@ static void ats1h_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo 
> *ri,
>                                       bool isread)
>  {
> -    if (arm_current_el(env) == 3 && !(env->cp15.scr_el3 & SCR_NS)) {
> +    if (arm_current_el(env) == 3 && !(env->cp15.scr_el3 & 
> (SCR_NS|SCR_EEL2))) {

This seems to belong to a different patch?

> +        if (arm_is_secure_below_el3(env))
> +            mask <<= ARM_MMU_IDX_A_S;

Braces.

>      if (raw_read(env, ri) != value) {
> -        tlb_flush_by_mmuidx(cs,
> -                            ARMMMUIdxBit_E10_1 |
> -                            ARMMMUIdxBit_E10_1_PAN |
> -                            ARMMMUIdxBit_E10_0);
> +        uint16_t mask = ARMMMUIdxBit_E10_1 |
> +                        ARMMMUIdxBit_E10_1_PAN |
> +                        ARMMMUIdxBit_E10_0;
> +
> +        if (arm_is_secure_below_el3(env))
> +            mask <<= ARM_MMU_IDX_A_S;

Again.  This appears to be an existing bug with SEL1?  Perhaps it should be
split to a separate patch.

>      if ((hcr & (HCR_E2H | HCR_TGE)) == (HCR_E2H | HCR_TGE)) {
> -        return ARMMMUIdxBit_E20_2 |
> +        mask = ARMMMUIdxBit_E20_2 |
>                 ARMMMUIdxBit_E20_2_PAN |
>                 ARMMMUIdxBit_E20_0;
> -    } else if (arm_is_secure_below_el3(env)) {
> -        return ARMMMUIdxBit_SE10_1 |
> -               ARMMMUIdxBit_SE10_1_PAN |
> -               ARMMMUIdxBit_SE10_0;
>      } else {
> -        return ARMMMUIdxBit_E10_1 |
> +        mask = ARMMMUIdxBit_E10_1 |
>                 ARMMMUIdxBit_E10_1_PAN |
>                 ARMMMUIdxBit_E10_0;
>      }
> +
> +    if (arm_is_secure_below_el3(env)) {
> +        mask <<= ARM_MMU_IDX_A_S;
> +    }

Likewise.

The rest of the patch looks mechanically correct.


r~