RE: [PATCH v6 09/11] xen/arm64: create boot-time MPU protection regions

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, November 7, 2022 4:47 AM
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: nd <nd@xxxxxxx>; Penny Zheng <Penny.Zheng@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v6 09/11] xen/arm64: create boot-time MPU protection
> regions
> 
> Hi Wei,
> 
> On 04/11/2022 10:07, Wei Chen wrote:
> > From: Penny Zheng <penny.zheng@xxxxxxx>
> >
> > Like boot-time page table in MMU system, we need a boot-time MPU
> > protection region configuration in MPU system so Xen can fetch code
> > and data from normal memory.
> >
> > This operation need to access Armv8-R MPU system registers, but these
> > system registers are not supported in GCC version < 11.
> > So we have to encode these Armv8-R MPU system registers in header file
> > explicitly.
> >
> > As MMU system and MPU system have different functions to create the
> > boot MMU/MPU data, this will introduce extra #ifdef in code flow, so
> > we introduce a neutral name prepare_early_mappings to replace
> > create_page_tables for MMU and MPU.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> 
> If Penny is the original author, then her signed-off-by should be first.
> 
> > ---
> >   xen/arch/arm/arm64/Makefile              |  2 +
> >   xen/arch/arm/arm64/head.S                | 13 ++--
> >   xen/arch/arm/arm64/head_mmu.S            |  4 +-
> >   xen/arch/arm/arm64/head_mpu.S            | 70 +++++++++++++++++++
> >   xen/arch/arm/include/asm/arm64/mpu.h     | 13 ++++
> >   xen/arch/arm/include/asm/arm64/sysregs.h | 89
> ++++++++++++++++++++++++
> >   6 files changed, 185 insertions(+), 6 deletions(-)
> >   create mode 100644 xen/arch/arm/arm64/head_mpu.S
> >   create mode 100644 xen/arch/arm/include/asm/arm64/mpu.h
> >
> > 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
> > d9a8da9120..6c1a5f74a1 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -79,12 +79,12 @@
> >    * ---------------------------
> >    *
> >    * 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.
> >    * It should be linked at XEN_VIRT_START, and loaded at any
> > - * 4K-aligned address.  All of text+data+bss must fit in 2MB,
> > + * 4K-aligned address. All of text+data+bss must fit in 2MB,
> 
> The double space after the final point was valid. This is fairly common to use
> it and this is a spurious change.
> 
> 
> >    * or the initial pagetable code below will need adjustment.
> >    */
> >
> > @@ -249,7 +249,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 protection regions for MPU systems.
> > +         */
> 
> head.S is now meant to be generic. So I would prefer if we keep comment
> as generic as possible. IOW, anything after the first comma should be
> dropped.
> 
> > +        bl    prepare_early_mappings
> >           bl    enable_mmu
> >
> >           /* We are still in the 1:1 mapping. Jump to the runtime Virtual
> Address. */
> > @@ -307,7 +312,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 1a3df81a38..fc64819a98 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)
> >
> >   /*
> >    * 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..f60611b556
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/head_mpu.S
> > @@ -0,0 +1,70 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> 
> Coding style:
> 
> /* SPDX ... */
> 
> > +/*
> > + * Start-of-day code for an Armv8-R MPU system.
> > + */
> > +
> > +#include <asm/arm64/mpu.h>
> > +#include <asm/page.h>
> > +#include <asm/early_printk.h>
> 
> Headers should be included in alphabetical order.
> 
> > +
> > +/*
> > + * From the requirements of head.S we know that Xen image should
> > + * be linked at XEN_START_ADDRESS, and all of text + data + bss
> > + * must fit in 2MB. On MPU systems, XEN_START_ADDRESS is also the
> > + * address that Xen image should be loaded at. So for initial MPU
> > + * regions setup, we use 2MB for Xen data memory to setup boot
> > + * region, or the create boot regions code below will need adjustment.
> > + */
> > +#define XEN_START_MEM_SIZE      0x200000
> 
> It sounds like something that should be defined in the header. Also, I
> think the size should be common between MPU and MMU.
> 
> In [1], I was going to name it XEN_VIRT_SIZE. I would be OK to remove
> "VIRT" in the name.
> 

Thx and please, then I will replace it with XEN_SIZE

> > +
> > +/*
> > + * In boot stage, we will use 1 MPU region:
> > + * Region#0: Normal memory for Xen text + data + bss (2MB)
> > + */
> 
> Are we only going to modify the MPU in head.S? If not, then I would
> define the layout in config_mpu.h so there are a single point where you
> can read how this works.
> 

We will remap Xen in C codes in setup_mm().
The whole strategy is aligned with MMU: a very simple setup(map xen
with the maximum size, 2M) at start-of-the-day, and a fit map in
setup_mm.

All the following variables will be only used at head_mpu.S.
It is not very generic, later, when introducing MPU memory region
management in C codes, we will define different macros for
various memory attributes.

> > +#define BOOT_NORMAL_REGION_IDX  0x0
> > +
> > +/* MPU normal memory attributes. */
> > +#define PRBAR_NORMAL_MEM        0x30    /* SH=11 AP=00 XN=00 */
> 
> IIUC, this means that Xen will be mapped write-executable. Is this going
> to be forever? If not, when can't we already mapped Xen properly?
> 

No, this setup is the start-of-day setup, and will only last for a very short 
time.

To be aligned with MMU system, in which L3 memory attributes is as follows:
#define PT_MEM_L3 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
Xen shall be mapped read-only-executable here, and I will fix it.

> > +#define PRLAR_NORMAL_MEM        0x0f    /* NS=0 ATTR=111 EN=1 */
> 
> To me, it feels like this should be fined outside of head.S because this
> could be re-used by other part of Xen.
> 

It is like PT_MEM_L3 in head_mmu.S. It will be only used in compiler codes.

MMU is defining macro like PAGE_HYPERVISOR_RW, for memory attributes
management in C codes, and we intend to follow the same strategy.

> > +
> > +.macro write_pr, sel, prbar, prlar
> > +    msr   PRSELR_EL2, \sel
> > +    dsb   sy
> 
> Is it really necessary to use "sy"  here? Also, it would be good to
> explain the logic. I.e. why do you need two dsb but only one isb?
> 
> In fact, I was expecting an "isb" here than "dsb" to wait for the
> completion of the instruction.
> 
> > +    msr   PRBAR_EL2, \prbar
> > +    msr   PRLAR_EL2, \prlar
> > +    dsb   sy
> > +    isb
> > +.endm
> > +
> > +.section .text.header, "ax", %progbits
> > +
> > +/*
> > + * Static start-of-day EL2 MPU memory layout.
> > + *
> > + * It has a very simple structure, including:
> > + *  - 2MB normal memory mappings of xen at XEN_START_ADDRESS,
> which
> > + * is the address where Xen was loaded by the bootloader.
> 
> Missing details on the clobberred registers.
> 
> > + */
> > +ENTRY(prepare_early_mappings)
> > +    /* Map Xen start memory to a normal memory region. */
> > +    mov x0, #BOOT_NORMAL_REGION_IDX
> > +    ldr x1, =XEN_START_ADDRESS
> > +    and x1, x1, #MPU_REGION_MASK
> > +    mov x3, #PRBAR_NORMAL_MEM
> > +    orr x1, x1, x3
> 
> It looks like to me there are a potential for a macro to compute the
> register.
> 
> > +
> > +    ldr x2, =XEN_START_ADDRESS
> > +    mov x3, #(XEN_START_MEM_SIZE - 1)
> 
> XEN_START_MEM_SIZE is the maximum size of Xen. IOW, Xen may be
> smaller
> and you will map memory that may not be part of Xen. Therefore, you
> likely want to compute the real size to avoid mapping too much.
> 

Later, in setup_mm we will map XEN components by components, such as,
one MPU memory region for read-only-executable text section, one
MPU memory region for read-only data section, etc, etc.
So in there, XEN will be mapped fitly.

IMHO, the mapping in compiler with maximum size of Xen is also what
MMU does.

>
> > +    add x2, x2, x
> > +    and x2, x2, #MPU_REGION_MASK
> > +    mov x3, #PRLAR_NORMAL_MEM
> > +    orr x2, x2, x3
> > +
> > +    /*
> > +     * Write to MPU protection region:
> > +     * x0 for pr_sel, x1 for prbar x2 for prlar
> 
> This is not a very useful comment because this can be inferred from the
> prototype of write_pr. What would be more interesting is to explain the
> logic within this function in the same way we do in head.S and head_mmu.S.
> 
> > +     */
> > +    write_pr x0, x1, x2
> > +
> > +    ret
> > +ENDPROC(prepare_early_mappings)
> > diff --git a/xen/arch/arm/include/asm/arm64/mpu.h
> b/xen/arch/arm/include/asm/arm64/mpu.h
> > new file mode 100644
> > index 0000000000..d209eef6db
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * mpu.h: Arm Memory Protection Unit definitions.
> > + */
> > +
> > +#ifndef __ARM64_MPU_H__
> > +#define __ARM64_MPU_H__
> > +
> > +#define MPU_REGION_SHIFT  6
> > +#define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
> > +#define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
> > +
> > +#endif /* __ARM64_MPU_H__ */
> > diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
> b/xen/arch/arm/include/asm/arm64/sysregs.h
> > index 54670084c3..a596042d6c 100644
> > --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> 
> In the context of this patch, it would be better to only define the
> registers you need. If you want to define all of them, then please
> define them in a separate patch before this one.
> 
> > @@ -458,6 +458,95 @@
> >   #define ZCR_ELx_LEN_SIZE             9
> >   #define ZCR_ELx_LEN_MASK             0x1ff
> >
> > +/* System registers for AArch64 with PMSA */
> > +#ifdef CONFIG_HAS_MPU
> 
> The #ifdef here seems unnecessary.
> 
> > +
> > +/* EL1 MPU Protection Region Base Address Register encode */
> > +#define PRBAR_EL1   S3_0_C6_C8_0
> > +#define PRBAR1_EL1  S3_0_C6_C8_4
> > +#define PRBAR2_EL1  S3_0_C6_C9_0
> > +#define PRBAR3_EL1  S3_0_C6_C9_4
> > +#define PRBAR4_EL1  S3_0_C6_C10_0
> > +#define PRBAR5_EL1  S3_0_C6_C10_4
> > +#define PRBAR6_EL1  S3_0_C6_C11_0
> > +#define PRBAR7_EL1  S3_0_C6_C11_4
> > +#define PRBAR8_EL1  S3_0_C6_C12_0
> > +#define PRBAR9_EL1  S3_0_C6_C12_4
> > +#define PRBAR10_EL1 S3_0_C6_C13_0
> > +#define PRBAR11_EL1 S3_0_C6_C13_4
> > +#define PRBAR12_EL1 S3_0_C6_C14_0
> > +#define PRBAR13_EL1 S3_0_C6_C14_4
> > +#define PRBAR14_EL1 S3_0_C6_C15_0
> > +#define PRBAR15_EL1 S3_0_C6_C15_4
> > +
> > +/* EL1 MPU Protection Region Limit Address Register encode */
> > +#define PRLAR_EL1   S3_0_C6_C8_1
> > +#define PRLAR1_EL1  S3_0_C6_C8_5
> > +#define PRLAR2_EL1  S3_0_C6_C9_1
> > +#define PRLAR3_EL1  S3_0_C6_C9_5
> > +#define PRLAR4_EL1  S3_0_C6_C10_1
> > +#define PRLAR5_EL1  S3_0_C6_C10_5
> > +#define PRLAR6_EL1  S3_0_C6_C11_1
> > +#define PRLAR7_EL1  S3_0_C6_C11_5
> > +#define PRLAR8_EL1  S3_0_C6_C12_1
> > +#define PRLAR9_EL1  S3_0_C6_C12_5
> > +#define PRLAR10_EL1 S3_0_C6_C13_1
> > +#define PRLAR11_EL1 S3_0_C6_C13_5
> > +#define PRLAR12_EL1 S3_0_C6_C14_1
> > +#define PRLAR13_EL1 S3_0_C6_C14_5
> > +#define PRLAR14_EL1 S3_0_C6_C15_1
> > +#define PRLAR15_EL1 S3_0_C6_C15_5
> > +
> > +/* EL2 MPU Protection Region Base Address Register encode */
> > +#define PRBAR_EL2   S3_4_C6_C8_0
> > +#define PRBAR1_EL2  S3_4_C6_C8_4
> > +#define PRBAR2_EL2  S3_4_C6_C9_0
> > +#define PRBAR3_EL2  S3_4_C6_C9_4
> > +#define PRBAR4_EL2  S3_4_C6_C10_0
> > +#define PRBAR5_EL2  S3_4_C6_C10_4
> > +#define PRBAR6_EL2  S3_4_C6_C11_0
> > +#define PRBAR7_EL2  S3_4_C6_C11_4
> > +#define PRBAR8_EL2  S3_4_C6_C12_0
> > +#define PRBAR9_EL2  S3_4_C6_C12_4
> > +#define PRBAR10_EL2 S3_4_C6_C13_0
> > +#define PRBAR11_EL2 S3_4_C6_C13_4
> > +#define PRBAR12_EL2 S3_4_C6_C14_0
> > +#define PRBAR13_EL2 S3_4_C6_C14_4
> > +#define PRBAR14_EL2 S3_4_C6_C15_0
> > +#define PRBAR15_EL2 S3_4_C6_C15_4
> > +
> > +/* EL2 MPU Protection Region Limit Address Register encode */
> > +#define PRLAR_EL2   S3_4_C6_C8_1
> > +#define PRLAR1_EL2  S3_4_C6_C8_5
> > +#define PRLAR2_EL2  S3_4_C6_C9_1
> > +#define PRLAR3_EL2  S3_4_C6_C9_5
> > +#define PRLAR4_EL2  S3_4_C6_C10_1
> > +#define PRLAR5_EL2  S3_4_C6_C10_5
> > +#define PRLAR6_EL2  S3_4_C6_C11_1
> > +#define PRLAR7_EL2  S3_4_C6_C11_5
> > +#define PRLAR8_EL2  S3_4_C6_C12_1
> > +#define PRLAR9_EL2  S3_4_C6_C12_5
> > +#define PRLAR10_EL2 S3_4_C6_C13_1
> > +#define PRLAR11_EL2 S3_4_C6_C13_5
> > +#define PRLAR12_EL2 S3_4_C6_C14_1
> > +#define PRLAR13_EL2 S3_4_C6_C14_5
> > +#define PRLAR14_EL2 S3_4_C6_C15_1
> > +#define PRLAR15_EL2 S3_4_C6_C15_5
> > +
> > +/* MPU Protection Region Enable Register encode */
> > +#define PRENR_EL1 S3_0_C6_C1_1
> > +#define PRENR_EL2 S3_4_C6_C1_1
> > +
> > +/* MPU Protection Region Selection Register encode */
> > +#define PRSELR_EL1 S3_0_C6_C2_1
> > +#define PRSELR_EL2 S3_4_C6_C2_1
> > +
> > +/* MPU Type registers encode */
> > +#define MPUIR_EL1 S3_0_C0_C0_4
> > +#define MPUIR_EL2 S3_4_C0_C0_4
> > +
> > +#endif
> > +
> >   /* Access to system registers */
> >
> >   #define WRITE_SYSREG64(v, name) do {                    \
> 
> Cheers,
> 
> [1] https://lore.kernel.org/all/20221022150422.17707-2-julien@xxxxxxx/
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng