RE: [PATCH v2 11/40] xen/mpu: build up start-of-day Xen MPU memory region map

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Thursday, January 19, 2023 11:04 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v2 11/40] xen/mpu: build up start-of-day Xen MPU
> memory region map
> 
> Hi Penny,
>

Hi Julien

Sorry for the late response, just come back from Chinese Spring Festival 
Holiday~
 
> On 13/01/2023 05:28, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@xxxxxxx>
> >
> > The start-of-day Xen MPU memory region layout shall be like as follows:
> >
> > xen_mpumap[0] : Xen text
> > xen_mpumap[1] : Xen read-only data
> > xen_mpumap[2] : Xen read-only after init data xen_mpumap[3] : Xen
> > read-write data xen_mpumap[4] : Xen BSS ......
> > xen_mpumap[max_xen_mpumap - 2]: Xen init data
> > xen_mpumap[max_xen_mpumap - 1]: Xen init text
> 
> Can you explain why the init region should be at the end of the MPU?
> 

As discussed in the v1 Serie, I'd like to put all transient MPU regions, like 
boot-only region,
at the end of the MPU.
Since they will get removed at the end of the boot, I am trying not to leave 
holes in the MPU
map by putting all transient MPU regions at rear. 

> >
> > max_xen_mpumap refers to the number of regions supported by the EL2
> MPU.
> > The layout shall be compliant with what we describe in xen.lds.S, or
> > the codes need adjustment.
> >
> > As MMU system and MPU system have different functions to create the
> > boot MMU/MPU memory management data, instead of introducing extra
> > #ifdef in main code flow, we introduce a neutral name
> > prepare_early_mappings for both, and also to replace create_page_tables
> for MMU.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/arm64/Makefile              |   2 +
> >   xen/arch/arm/arm64/head.S                |  17 +-
> >   xen/arch/arm/arm64/head_mmu.S            |   4 +-
> >   xen/arch/arm/arm64/head_mpu.S            | 323
> +++++++++++++++++++++++
> >   xen/arch/arm/include/asm/arm64/mpu.h     |  63 +++++
> >   xen/arch/arm/include/asm/arm64/sysregs.h |  49 ++++
> >   xen/arch/arm/mm_mpu.c                    |  48 ++++
> >   xen/arch/arm/xen.lds.S                   |   4 +
> >   8 files changed, 502 insertions(+), 8 deletions(-)
> >   create mode 100644 xen/arch/arm/arm64/head_mpu.S
> >   create mode 100644 xen/arch/arm/include/asm/arm64/mpu.h
> >   create mode 100644 xen/arch/arm/mm_mpu.c
> >
> > +/*
> > + * Macro to create a new MPU memory region entry, which is a
> > +structure
> > + * of pr_t,  in \prmap.
> > + *
> > + * Inputs:
> > + * prmap:   mpu memory region map table symbol
> > + * sel:     region selector
> > + * prbar:   preserve value for PRBAR_EL2
> > + * prlar    preserve value for PRLAR_EL2
> > + *
> > + * Clobbers \tmp1, \tmp2
> > + *
> > + */
> > +.macro create_mpu_entry prmap, sel, prbar, prlar, tmp1, tmp2
> > +    mov   \tmp2, \sel
> > +    lsl   \tmp2, \tmp2, #MPU_ENTRY_SHIFT
> > +    adr_l \tmp1, \prmap
> > +    /* Write the first 8 bytes(prbar_t) of pr_t */
> > +    str   \prbar, [\tmp1, \tmp2]
> > +
> > +    add   \tmp2, \tmp2, #8
> > +    /* Write the last 8 bytes(prlar_t) of pr_t */
> > +    str   \prlar, [\tmp1, \tmp2]
> 
> Any particular reason to not use 'stp'?
> 
> Also, AFAICT, with data cache disabled. But at least on ARMv8-A, the cache is
> never really off. So don't need some cache maintainance?
> 
> FAOD, I know the existing MMU code has the same issue. But I would rather
> prefer if the new code introduced is compliant to the Arm Arm.
> 

True, `stp` is better and I will clean data cache to be compliant to the Arm 
Arm.
I write the following example to see if I catch what you suggested:
```
add \tmp1, \tmp1, \tmp2
stp \prbar, \prlar, [\tmp1]
dc cvau, \tmp1
isb
dsb sy
```

> > +.endm
> > +
> > +/*
> > + * Macro to store the maximum number of regions supported by the EL2
> > +MPU
> > + * in max_xen_mpumap, which is identified by MPUIR_EL2.
> > + *
> > + * Outputs:
> > + * nr_regions: preserve the maximum number of regions supported by
> > +the EL2 MPU
> > + *
> > + * Clobbers \tmp1
> > + * > + */
> 
> Are you going to have multiple users? If not, then I would prefer if this is
> folded in the only caller.
> 

Ok. I will fold since I think it is one-time reading thingy.

> > +.macro read_max_el2_regions, nr_regions, tmp1
> > +    load_paddr \tmp1, max_xen_mpumap
> 
> I would rather prefer if we restrict the use of global while the MMU if off 
> (see
> why above).
> 

If we don't use global here, then after MPU enabled, we need to re-read 
MPUIR_EL2
to get the number of maximum EL2 regions.

Or I put data cache clean after accessing global, is it better?
```
str   \nr_regions, [\tmp1]
dc cvau, \tmp1
isb
dsb sy
```

> > +    mrs   \nr_regions, MPUIR_EL2
> > +    isb
> 
> What's that isb for?
> 
> > +    str   \nr_regions, [\tmp1]
> > +.endm
> > +
> > +/*
> > + * ENTRY to configure a EL2 MPU memory region
> > + * ARMv8-R AArch64 at most supports 255 MPU protection regions.
> > + * See section G1.3.18 of the reference manual for ARMv8-R AArch64,
> > + * PRBAR<n>_EL2 and PRLAR<n>_EL2 provides access to the EL2 MPU
> > +region
> > + * determined by the value of 'n' and PRSELR_EL2.REGION as
> > + * PRSELR_EL2.REGION<7:4>:n.(n = 0, 1, 2, ... , 15)
> > + * For example to access regions from 16 to 31 (0b10000 to 0b11111):
> > + * - Set PRSELR_EL2 to 0b1xxxx
> > + * - Region 16 configuration is accessible through PRBAR0_EL2 and
> > +PRLAR0_EL2
> > + * - Region 17 configuration is accessible through PRBAR1_EL2 and
> > +PRLAR1_EL2
> > + * - Region 18 configuration is accessible through PRBAR2_EL2 and
> > +PRLAR2_EL2
> > + * - ...
> > + * - Region 31 configuration is accessible through PRBAR15_EL2 and
> > +PRLAR15_EL2
> > + *
> > + * Inputs:
> > + * x27: region selector
> > + * x28: preserve value for PRBAR_EL2
> > + * x29: preserve value for PRLAR_EL2
> > + *
> > + */
> > +ENTRY(write_pr)
> 
> AFAICT, this function would not be necessary if the index for the init 
> sections
> were hardcoded.
> 
> So I would like to understand why the index cannot be hardcoded.
> 

The reason is that we are putting init sections at the *end* of the MPU map, and
the length of the whole MPU map is platform-specific. We read it from MPUIR_EL2.
 
> > +    msr   PRSELR_EL2, x27
> > +    dsb   sy
> 
> [...]
> 
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index
> > bc45ea2c65..79965a3c17 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -91,6 +91,8 @@ SECTIONS
> >         __ro_after_init_end = .;
> >     } : text
> >
> > +  . = ALIGN(PAGE_SIZE);
> 
> Why do you need this ALIGN?
> 

I need a symbol as the start of the data section, so I introduce
"__data_begin = .;". 
If I use "__ro_after_init_end = .;" instead, I'm afraid in the future,
if someone introduces a new section after ro-after-init section, this part
also needs modification too.

When we define MPU regions for each section in xen.lds.S, we always treat these 
sections
page-aligned.
I checked each section in xen.lds.S, and ". = ALIGN(PAGE_SIZE);" is either added
at section head, or at the tail of the previous section, to make sure starting 
address symbol
page-aligned.

And if we don't put this ALIGN, if "__ro_after_init_end " symbol itself is not 
page-aligned,
the two adjacent sections will overlap in MPU.
 
> > +  __data_begin = .;
> >     .data.read_mostly : {
> >          /* Exception table */
> >          __start___ex_table = .;
> > @@ -157,7 +159,9 @@ SECTIONS
> >          *(.altinstr_replacement)
> 
> I know you are not using alternative instructions yet. But, you should make
> sure they are included. So I think rather than introduce __init_data_begin,
> you want to use "_einitext" for the start of the "Init data" section.
> 
> >     } :text
> >     . = ALIGN(PAGE_SIZE);
> > +
> 
> Spurious?
> 
> >     .init.data : {
> > +       __init_data_begin = .;            /* Init data */
> >          *(.init.rodata)
> >          *(.init.rodata.*)
> >
> 
> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng