Re: [PATCH v1 3/4] xen/arm: mpu: Create boot-time MPU protection regions

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 28/08/2024 16:01, Julien Grall wrote:
> Hi,
Hi Julien,

I need some clarifications.

> On 23/08/2024 17:31, Ayan Kumar Halder wrote:
> > Define enable_boot_cpu_mm() for the AArch64-V8R system.
> >
> > 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.
> >
> > START_ADDRESS + 2MB memory is mapped to contain the text and data
> > required for early boot of Xen.
> >
> > One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
> > Registers", to get the definitions of PRBAR_EL2, PRLAR_EL2 and
> > PRSELR_EL2 registers. Also, refer to G1.2 "Accessing MPU memory region
> > registers", the following
> >
> > ```
> > The MPU provides two register interfaces to program the MPU regions:
> >   - Access to any of the MPU regions via PRSELR_ELx, PRBAR<n>_ELx, and
> > PRLAR<n>_ELx.
> > ```
> > We use the above mechanism to configure the MPU memory regions.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> >   xen/arch/arm/Makefile                    |  1 +
> >   xen/arch/arm/arm64/mpu/Makefile          |  1 +
> >   xen/arch/arm/arm64/mpu/head.S            | 70 ++++++++++++++++++++++++
> >   xen/arch/arm/include/asm/arm64/sysregs.h | 50 +++++++++++++++++
> >   xen/arch/arm/include/asm/mpu/arm64/mm.h  | 13 +++++
> >   xen/arch/arm/include/asm/mpu/mm.h        | 18 ++++++
> >   6 files changed, 153 insertions(+)
> >   create mode 100644 xen/arch/arm/arm64/mpu/Makefile
> >   create mode 100644 xen/arch/arm/arm64/mpu/head.S
> >   create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
> >   create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
> >
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 7792bff597..aebccec63a 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -1,6 +1,7 @@
> >   obj-$(CONFIG_ARM_32) += arm32/
> >   obj-$(CONFIG_ARM_64) += arm64/
> >   obj-$(CONFIG_MMU) += mmu/
> > +obj-$(CONFIG_MPU) += mpu/
> >   obj-$(CONFIG_ACPI) += acpi/
> >   obj-$(CONFIG_HAS_PCI) += pci/
> >   ifneq ($(CONFIG_NO_PLAT),y)
> > diff --git a/xen/arch/arm/arm64/mpu/Makefile 
> > b/xen/arch/arm/arm64/mpu/Makefile
> > new file mode 100644
> > index 0000000000..3340058c08
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/mpu/Makefile
> > @@ -0,0 +1 @@
> > +obj-y += head.o
> > diff --git a/xen/arch/arm/arm64/mpu/head.S 
> > b/xen/arch/arm/arm64/mpu/head.S
> > new file mode 100644
> > index 0000000000..2b023c346a
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/mpu/head.S
> > @@ -0,0 +1,70 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> Coding style: /* ... */
Ack
> > +/*
> > + * Start-of-day code for an Armv8-R MPU system.
> > + */
> > +
> > +#include <asm/mm.h>
> > +#include <asm/page.h>
> I can't see any use of the definition of page.h. Is it necessary?
I will remove this and ..
> > +#include <asm/early_printk.h>
> You include early_printk.h but don't use it.
this as well.
> > +
> > +/*
> > + * 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.
> Xen can be bigger than 2MB. But rather than hardcoding the limit, you 
> want to use _end.
I  was wondering if we can XEN_VIRT_SIZE here , but that would lead to 
mapping of a large memory region.
So I guess _end should be used here.

> > 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
> See above.
Ack.
> > +
> > +/*
> > + * In boot stage, we will use 1 MPU region:
> > + * Region#0: Normal memory for Xen text + data + bss (2MB)
> > + */
> > +#define BOOT_NORMAL_REGION_IDX  0x0
> > +
> > +/* MPU normal memory attributes. */
> > +#define PRBAR_NORMAL_MEM        0x30    /* SH=11 AP=00 XN=00 */
> > +#define PRLAR_NORMAL_MEM        0x0f    /* NS=0 ATTR=111 EN=1 */
> > +
> > +.macro write_pr, sel, prbar, prlar
> > +    msr   PRSELR_EL2, \sel
> > +    dsb   sy
> I am not sure I understand why this is a dsb rather than isb. Can you 
> clarify?
ISB is not needed here as the memory protection hasn't been activated 
yet. The above instruction just selects the memory region and the below 
two instructions sets the base address and limit for that memory region. 
After the three instructions, we need an ISB so that the memory 
protection takes into affect for further instruction fetches.
However, a DSB is needed here as the below two instructions depend on 
this. So, we definitely want this instruction to complete.
Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1 
"Protection region attributes"
0.

   ```Writes to MPU registers are only guaranteed to be visible
   following a Context synchronization event and DSB operation.```

Thus, I infer that DSB is necessary here.

> If a "dsb" is necessary, "sy" seems to be quite strict.
I can use "st" here as I am only interested in stores (ie MSR) to complete.

Now the whether we want to restrict it to inner shareable/outer 
shareable/POU/full system is a difficult decision to make.
May be here we can use ishst (stores to complete to inner shareable 
region). However ....
> > +    msr   PRBAR_EL2, \prbar
> > +    msr   PRLAR_EL2, \prlar
> > +    dsb   sy
This should be visible to outer shareable domain atleast. The reason 
being one can use the SH[1:0] bits in PRBAR_EL2 to set the region to 
outer shareable.
Thus, the writes to these registers should be visible to outer shareable 
domain as well.
> > +    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.
> > + */
> Please document a bit more the code and how the registers are used. 
> For an example see the mmu/head.S code.
Ack
> > +ENTRY(enable_boot_cpu_mm)
> > +    /* 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
> > +
> > +    ldr x2, =XEN_START_ADDRESS
> > +    mov x3, #(XEN_START_MEM_SIZE - 1)
> > +    add x2, x2, x3
> > +    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
> > +     */
> > +    write_pr x0, x1, x2
> > +
> > +    ret
> > +ENDPROC(enable_boot_cpu_mm)
> Missing emacs magic.
You mean it should be

END(enable_boot_cpu_mm) .

/*
 * Local variables:
 * mode: ASM
 * indent-tabs-mode: nil
 * End:
 */

> > diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h 
> > b/xen/arch/arm/include/asm/arm64/sysregs.h
> > index b593e4028b..0d122e1fa6 100644
> > --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> > @@ -462,6 +462,56 @@
> >   #define ZCR_ELx_LEN_SIZE             9
> >   #define ZCR_ELx_LEN_MASK             0x1ff
> >   +/* System registers for AArch64 with PMSA */
> > +#ifdef CONFIG_MPU
> Can we define the registers in mpu/sysregs.h? This can then be 
> included only where it is needed.
Ack
> > +
> > +/* 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_EL2 S3_4_C6_C1_1
> > +
> > +/* MPU Protection Region Selection Register encode */
> > +#define PRSELR_EL2 S3_4_C6_C2_1
> > +
> > +/* MPU Type registers encode */
> > +#define MPUIR_EL2 S3_4_C0_C0_4
> > +
> > +#endif
> > +
> >   /* Access to system registers */
> >     #define WRITE_SYSREG64(v, name) do {                    \
> > diff --git a/xen/arch/arm/include/asm/mpu/arm64/mm.h 
> > b/xen/arch/arm/include/asm/mpu/arm64/mm.h
> > new file mode 100644
> > index 0000000000..d209eef6db
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/mpu/arm64/mm.h
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> Coding style: /* ... */
Ack
> > +/*
> > + * mpu.h: Arm Memory Protection Unit definitions.
> The file is name mm.h.
Ack
> > + */
> > +
> > +#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__ */
> Missing emacs magic.
/*
 * Local variables:
 * mode: C
 * c-file-style: "BSD"
 * c-basic-offset: 4
 * tab-width: 4
 * indent-tabs-mode: nil
 * End:
 */

> > diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> > b/xen/arch/arm/include/asm/mpu/mm.h
> > new file mode 100644
> > index 0000000000..f5ebca8261
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/mpu/mm.h
> Do we need this file?
In xen/arch/arm/arm64/mpu/head.S, we have

#include <asm/mm.h>

So, it should pick up this file.

- Ayan

> > @@ -0,0 +1,18 @@
> > +#ifndef __ARCH_ARM_MPU__
> > +#define __ARCH_ARM_MPU__
> > +
> > +#if defined(CONFIG_ARM_64)
> > +# include <asm/mpu/arm64/mm.h>
> > +#else
> > +# error "unknown ARM variant"
> > +#endif
> > +
> > +#endif /*  __ARCH_ARM_MPU__ */
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> Cheers,