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

[Rémi Denis-Courmont]

Le tiistaina 3. marraskuuta 2020, 20.32.21 EET Richard Henderson a écrit :
> 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?

Possible. It would save a few hundred bytes from a quick glance.

It could also be argued that E2 and E20_2 should be one and the same. The 
regimes are distinct but they cannot coexist. The mode's TLB mode could be 
flushed when HCR.E2H is flipped, I guess.


-- 
Rémi Denis-Courmont