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

[Julien Grall <julien@xxxxxxx>]

Hi,

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?

> +    dsb   sy
I still question this use of "dsb sy"...

> +    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,


--
Julien Grall