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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Ayan

> -----Original Message-----
> From: Ayan Kumar Halder <ayankuma@xxxxxxx>
> Sent: Thursday, January 19, 2023 6:19 PM
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Penny Zheng
> <Penny.Zheng@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien
> Grall <julien@xxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr_Babchuk@xxxxxxxx
> Subject: Re: [PATCH v2 11/40] xen/mpu: build up start-of-day Xen MPU
> memory region map
> 
> 
> On 13/01/2023 05:28, Penny Zheng wrote:
> > CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> >
> >
> > 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
> >
> > 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
> >
> > diff --git a/xen/arch/arm/arm64/Makefile
> b/xen/arch/arm/arm64/Makefile
> > index 22da2f54b5..438c9737ad 100644
> > --- a/xen/arch/arm/arm64/Makefile
> > +++ b/xen/arch/arm/arm64/Makefile
> > @@ -10,6 +10,8 @@ obj-y += entry.o
> >   obj-y += head.o
> >   ifneq ($(CONFIG_HAS_MPU),y)
> >   obj-y += head_mmu.o
> > +else
> > +obj-y += head_mpu.o
> >   endif
> >   obj-y += insn.o
> >   obj-$(CONFIG_LIVEPATCH) += livepatch.o diff --git
> > a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index
> > 782bd1f94c..145e3d53dc 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -68,9 +68,9 @@
> >    *  x24 -
> >    *  x25 -
> >    *  x26 - skip_zero_bss (boot cpu only)
> > - *  x27 -
> > - *  x28 -
> > - *  x29 -
> > + *  x27 - region selector (mpu only)
> > + *  x28 - prbar (mpu only)
> > + *  x29 - prlar (mpu only)
> >    *  x30 - lr
> >    */
> >
> > @@ -82,7 +82,7 @@
> >    * ---------------------------
> >    *
> >    * The requirements are:
> > - *   MMU = off, D-cache = off, I-cache = on or off,
> > + *   MMU/MPU = off, D-cache = off, I-cache = on or off,
> >    *   x0 = physical address to the FDT blob.
> >    *
> >    * This must be the very first address in the loaded image.
> > @@ -252,7 +252,12 @@ real_start_efi:
> >
> >           bl    check_cpu_mode
> >           bl    cpu_init
> > -        bl    create_page_tables
> > +
> > +        /*
> > +         * Create boot memory management data, pagetable for MMU
> systems
> > +         * and memory regions for MPU systems.
> > +         */
> > +        bl    prepare_early_mappings
> >           bl    enable_mmu
> >
> >           /* We are still in the 1:1 mapping. Jump to the runtime
> > Virtual Address. */ @@ -310,7 +315,7 @@ GLOBAL(init_secondary)
> >   #endif
> >           bl    check_cpu_mode
> >           bl    cpu_init
> > -        bl    create_page_tables
> > +        bl    prepare_early_mappings
> >           bl    enable_mmu
> >
> >           /* We are still in the 1:1 mapping. Jump to the runtime
> > Virtual Address. */ diff --git a/xen/arch/arm/arm64/head_mmu.S
> > b/xen/arch/arm/arm64/head_mmu.S index 6ff13c751c..2346f755df
> 100644
> > --- a/xen/arch/arm/arm64/head_mmu.S
> > +++ b/xen/arch/arm/arm64/head_mmu.S
> > @@ -123,7 +123,7 @@
> >    *
> >    * Clobbers x0 - x4
> >    */
> > -ENTRY(create_page_tables)
> > +ENTRY(prepare_early_mappings)
> >           /* Prepare the page-tables for mapping Xen */
> >           ldr   x0, =XEN_VIRT_START
> >           create_table_entry boot_pgtable, boot_first, x0, 0, x1, x2,
> > x3 @@ -208,7 +208,7 @@ virtphys_clash:
> >           /* Identity map clashes with boot_third, which we cannot handle 
> > yet
> */
> >           PRINT("- Unable to build boot page tables - virt and phys 
> > addresses
> clash. -\r\n")
> >           b     fail
> > -ENDPROC(create_page_tables)
> > +ENDPROC(prepare_early_mappings)
> 
> NIT:- Can this renaming be done in a separate patch of its own (before this
> patch).
> 

Yay, you're right. I'll put it in different commit.

> So that this patch can be only about the new functionality introduced.
> 
> >
> >   /*
> >    * Turn on the Data Cache and the MMU. The function will return on
> > the 1:1 diff --git a/xen/arch/arm/arm64/head_mpu.S
> > b/xen/arch/arm/arm64/head_mpu.S new file mode 100644 index
> > 0000000000..0b97ce4646
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/head_mpu.S
> > @@ -0,0 +1,323 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Start-of-day code for an Armv8-R AArch64 MPU system.
> > + */
> > +
> > +#include <asm/arm64/mpu.h>
> > +#include <asm/early_printk.h>
> > +#include <asm/page.h>
> > +
> > +/*
> > + * One entry in Xen MPU memory region mapping table(xen_mpumap) is
> a
> > +structure
> > + * of pr_t, which is 16-bytes size, so the entry offset is the order of 4.
> > + */
> NIT :- It would be good to quote Arm ARM from which can be referred for
> the definitions.
> > +#define MPU_ENTRY_SHIFT         0x4
> > +
> > +#define REGION_SEL_MASK         0xf
> > +
> > +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
> > +#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
> > +#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
> > +
> > +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
> > +
> > +/*
> > + * Macro to round up the section address to be PAGE_SIZE aligned
> > + * Each section(e.g. .text, .data, etc) in xen.lds.S is page-aligned,
> > + * which is usually guarded with ". = ALIGN(PAGE_SIZE)" in the head,
> > + * or in the end
> > + */
> > +.macro roundup_section, xb
> > +        add   \xb, \xb, #(PAGE_SIZE-1)
> > +        and   \xb, \xb, #PAGE_MASK
> > +.endm
> > +
> > +/*
> > + * 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]
> > +.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
> > + *
> > + */
> > +.macro read_max_el2_regions, nr_regions, tmp1
> > +    load_paddr \tmp1, max_xen_mpumap
> > +    mrs   \nr_regions, MPUIR_EL2
> > +    isb
> > +    str   \nr_regions, [\tmp1]
> > +.endm
> > +
> > +/*
> > + * Macro to prepare and set a MPU memory region
> > + *
> > + * Inputs:
> > + * base:        base address symbol (should be page-aligned)
> > + * limit:       limit address symbol
> > + * sel:         region selector
> > + * prbar:       store computed PRBAR_EL2 value
> > + * prlar:       store computed PRLAR_EL2 value
> > + * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified
> > +it will be REGION_DATA_PRBAR
> > + * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified
> > +it will be REGION_NORMAL_PRLAR
> > + *
> > + * Clobber \tmp1
> > + *
> > + */
> > +.macro prepare_xen_region, base, limit, sel, prbar, prlar, tmp1,
> attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
> > +    /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
> > +    load_paddr \prbar, \base
> > +    and   \prbar, \prbar, #MPU_REGION_MASK
> > +    mov   \tmp1, #\attr_prbar
> > +    orr   \prbar, \prbar, \tmp1
> > +
> > +    /* Prepare value for PRLAR_EL2 reg and preserve it in \prlar.*/
> > +    load_paddr \prlar, \limit
> > +    /* Round up limit address to be PAGE_SIZE aligned */
> > +    roundup_section \prlar
> > +    /* Limit address should be inclusive */
> > +    sub   \prlar, \prlar, #1
> > +    and   \prlar, \prlar, #MPU_REGION_MASK
> > +    mov   \tmp1, #\attr_prlar
> > +    orr   \prlar, \prlar, \tmp1
> > +
> > +    mov   x27, \sel
> > +    mov   x28, \prbar
> > +    mov   x29, \prlar
> 
> Any reasons for using x27, x28, x29 to pass function parameters.
> 
> https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst
> states x0..x7 should be used (Table 2, General-purpose registers and
> AAPCS64 usage).
> 

These registers are documented and reserved in xen/arch/arm/arm64/head.S, like
how we reserve x26 to pass function parameter in skip_zero_bss, see
```
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 782bd1f94c..145e3d53dc 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -68,9 +68,9 @@
  *  x24 -
  *  x25 -
  *  x26 - skip_zero_bss (boot cpu only)
- *  x27 -
- *  x28 -
- *  x29 -
+ *  x27 - region selector (mpu only)
+ *  x28 - prbar (mpu only)
+ *  x29 - prlar (mpu only)
  *  x30 - lr
  */
```
x0...x7 are already commonly used in xen/arch/arm/arm64/head.S, it is difficult 
for me
to preserve them only for write_pr.

If we are using x0...x7 as function parameter, I need to stack/pop them to 
mutate
stack operation in write_pr to avoid corruption.

> > +    /*
> > +     * x2skip_zero7, x28, x29 are special registers designed as
> > +     * inputs for function write_pr
> > +     */
> > +    bl    write_pr
> > +.endm
> > +
[...]
> > --
> > 2.25.1
> >
> NIT:- Would you consider splitting this patch, something like this :-
> 
> 1. Renaming of the mmu function
> 
> 2. Define sysregs, prlar_t, prbar_t and other other hardware specific macros.
> 
> 3. Define write_pr
> 
> 4. The rest of the changes (ie prepare_early_mappings(), xen.lds.S, etc)
> 

For 2, 3 and 4, it will break the rule of "Always define and introduce at the
first usage".
However, I know that this commit is very big ;/, so as long as maintainers are 
also
in favor of your splitting suggestion, I'm happy to do the split too~

> - Ayan