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

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 21/10/2024 16:30, Ayan Kumar Halder wrote:
> 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.
I don't mind to keep it for this reason so long it is documented.

Cheers,


--
Julien Grall