Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 18/10/2024 23:25, Julien Grall wrote:
> Hi,
Hi Julien,
> On 10/10/2024 15:03, Ayan Kumar Halder wrote:
> > After the regions have been created, now we enable the MPU. For this 
> > we disable
> > the background region so that the new memory map created for the 
> > regions take
> > effect. Also, we treat all RW regions as non executable and the data 
> > cache is
> > enabled.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > Changes from :-
> >
> > v2 - 1. Extracted from the previous patch into a new one.
> >
> > 2. Disabled background region.
> >
> >   xen/arch/arm/arm64/mpu/head.S                | 29 ++++++++++++++++++--
> >   xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  3 ++
> >   2 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/mpu/head.S 
> > b/xen/arch/arm/arm64/mpu/head.S
> > index 4a21bc815c..e354f4552b 100644
> > --- a/xen/arch/arm/arm64/mpu/head.S
> > +++ b/xen/arch/arm/arm64/mpu/head.S
> > @@ -61,6 +61,30 @@
> >       add \xb, \xb, x20       /* x20 - Phys offset */
> >   .endm
> >   +/*
> > + * Enable EL2 MPU and data cache
> > + * If the Background region is enabled, then the MPU uses the 
> > default memory
> > + * map as the Background region for generating the memory
> > + * attributes when MPU is disabled.
> > + * Since the default memory map of the Armv8-R AArch64 architecture is
> > + * IMPLEMENTATION DEFINED, we intend to turn off the Background 
> > region here.
> > + *
> > + * Clobbers x0
> > + *
> > + */
> > +FUNC_LOCAL(enable_mpu)
> > +    mrs   x0, SCTLR_EL2
> > +    bic   x0, x0, #SCTLR_ELx_BR       /* Disable Background region */
> > +    orr   x0, x0, #SCTLR_Axx_ELx_M    /* Enable MPU */
> > +    orr   x0, x0, #SCTLR_Axx_ELx_C    /* Enable D-cache */
> > +    orr   x0, x0, #SCTLR_Axx_ELx_WXN  /* Enable WXN */
> NIT: Can't we have a single "orr" instruction to set all the flags?
Yes, I will change this.
> > +    dsb   sy
> I still question this use of "dsb sy"...
Actually I kept this to ensure that all outstanding memory access are 
completed before MPU is enabled.
However, prepare_xen_region() is invoked before this and it has a 'dsb 
sy' at the end.
So we can drop this barrier.

- Ayan

> > +    msr   SCTLR_EL2, x0
> > +    isb
> > +
> > +    ret
> > +END(enable_mpu)
> > +
> >   /*
> >    * Maps the various sections of Xen (described in xen.lds.S) as 
> > different MPU
> >    * regions.
> > @@ -68,10 +92,11 @@
> >    * Inputs:
> >    *   lr : Address to return to.
> >    *
> > - * Clobbers x0 - x5
> > + * Clobbers x0 - x6
> >    *
> >    */
> >   FUNC(enable_boot_cpu_mm)
> > +    mov   x6, lr
> >         /* Check if the number of regions exceeded the count 
> > specified in MPUIR_EL2 */
> >       mrs   x5, MPUIR_EL2
> > @@ -113,7 +138,7 @@ FUNC(enable_boot_cpu_mm)
> >       beq 5f
> >       prepare_xen_region x0, x1, x2, x3, x4, x5
> >   -5:
> > +5:  mov   lr, x6
> >       ret
> >     fail:
> > diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h 
> > b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> > index b0c31a58ec..3769d23c80 100644
> > --- a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> > +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> > @@ -15,6 +15,9 @@
> >   /* MPU Protection Region Selection Register encode */
> >   #define PRSELR_EL2  S3_4_C6_C2_1
> >   +/* Backgroud region enable/disable */
> > +#define SCTLR_ELx_BR    BIT(17, UL)
> > +
> >   #endif /* __ASM_ARM_ARM64_MPU_SYSREGS_H */
> >     /*
> Cheers,