Re: [PATCH v6 10/11] xen/arm64: introduce helpers for MPU enable/disable

[Julien Grall <julien@xxxxxxx>]

Hi Wei,

On 04/11/2022 10:07, Wei Chen wrote:
> From: Penny Zheng <penny.zheng@xxxxxxx>
>
> We need some helpers for Xen to enable/disable MPU in boot-time
> and runtime. For MPU enable helper, we know that it's an
> essential requirement of MPU system. But for MPU disable,
> we need to use it for some special situations. For example,
> in the progress of tranferring from boot-time to runtime,
> we need to update the MPU protection regions configuration,
> but we can't modify an MPU protection region if there is some
> data accessed by Xen. But in boot-time all of Xen text, data
> and BSS are in one MPU protection region, if Xen want to update
> this protection region, above restriction will be triggered.
This raises the following question: Why can't we create the split 
regions right now?
In particular, disabling the MMU/Cache is fairly risky because you need 
to ensure that anything in the cache you care about have been written 
back to the RAM).
> So in this situation, we need to disable the whole MPU to update
> the protection regions.
>
> In these helper, enable/disable MPU will also enable/disable
> the D-cache. There are two reasons for it:
> 1. Make the function semantic be consistent with enable_mmu.
>     For MMU systems, enable_mmu will turn MMU and D-Cache at
>     the same time.
> 2. When MPU is disabled, the MPU background attributes will
>     be used. On some platforms, the background will treat all
>     memory as device memory. The access to device memory will
>     bypass the cache, even if the C bit is enabled in SCTLR.
>     To avoid this implicit behavior, we disable cache with MPU
>     explicitly to tell user that when MPU is disabled, the
>     memory access is uncacheable.
>
> In this patch, we also introduce a neutral name enable_mm for
> Xen to enable MMU/MPU. This can help us to keep one code flow
> in head.S
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
>   xen/arch/arm/arm64/head.S     |  5 +++--
>   xen/arch/arm/arm64/head_mmu.S |  4 ++--
>   xen/arch/arm/arm64/head_mpu.S | 35 +++++++++++++++++++++++++++++++++++
>   3 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 6c1a5f74a1..228f01db69 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -255,7 +255,8 @@ real_start_efi:
>            * and protection regions for MPU systems.
>            */
>           bl    prepare_early_mappings
> -        bl    enable_mmu
> +        /* Turn on MMU or MPU */
> +        bl    enable_mm
>   
>           /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
>           ldr   x0, =primary_switched
> @@ -313,7 +314,7 @@ GLOBAL(init_secondary)
>           bl    check_cpu_mode
>           bl    cpu_init
>           bl    prepare_early_mappings
> -        bl    enable_mmu
> +        bl    enable_mm
>   
>           /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
>           ldr   x0, =secondary_switched
> diff --git a/xen/arch/arm/arm64/head_mmu.S b/xen/arch/arm/arm64/head_mmu.S
> index fc64819a98..b542755bd2 100644
> --- a/xen/arch/arm/arm64/head_mmu.S
> +++ b/xen/arch/arm/arm64/head_mmu.S
> @@ -217,7 +217,7 @@ ENDPROC(prepare_early_mappings)
>    *
>    * Clobbers x0 - x3
>    */
> -ENTRY(enable_mmu)
> +ENTRY(enable_mm)
>           PRINT("- Turning on paging -\r\n")
>   
>           /*
> @@ -239,7 +239,7 @@ ENTRY(enable_mmu)
>           msr   SCTLR_EL2, x0          /* now paging is enabled */
>           isb                          /* Now, flush the icache */
>           ret
> -ENDPROC(enable_mmu)
> +ENDPROC(enable_mm)
>   
>   /*
>    * Remove the 1:1 map from the page-tables. It is not easy to keep track
> diff --git a/xen/arch/arm/arm64/head_mpu.S b/xen/arch/arm/arm64/head_mpu.S
> index f60611b556..5a1b03e293 100644
> --- a/xen/arch/arm/arm64/head_mpu.S
> +++ b/xen/arch/arm/arm64/head_mpu.S
> @@ -68,3 +68,38 @@ ENTRY(prepare_early_mappings)
>   
>       ret
>   ENDPROC(prepare_early_mappings)
> +
> +/*
> + * Enable EL2 MPU and data cache. Because we will disable cache
> + * with MPU at the same time, in accordance with that, we have
> + * to enable cache with MPU at the same time in this function.
> + * When MPU is disabled, the MPU background attributes will
> + * be used. On some platform, the background will treat all
> + * memory as IO memory.
I was under the impression that all access would be treated as Device 
Memory when the MMU is off. Isn't it the case for the MPU?
Also, I think the correct wording is "device memory" rather than "IO 
memory".
> The access to IO memory will bypass
Ditto.

> + * the cache, even you have enabled the C bit in SCTLR.
> + * To avoid this implicit behavior, we disable cache with MPU
> + * explicitly to tell user that when MPU is disabled, the memory
> + * access is uncacheable.
> + */
> +ENTRY(enable_mm)
> +    mrs   x0, SCTLR_EL2
> +    mov   x1, #(SCTLR_Axx_ELx_M | SCTLR_Axx_ELx_C)
> +    /* Enable EL2 MPU and D-cache */
> +    orr   x0, x0, x1
> +    dsb   sy
> +    msr   SCTLR_EL2, x0
> +    isb
> +    ret
> +ENDPROC(enable_mm)
> +
> +/* Disable MPU system, including data cache. */
> +ENTRY(disable_mm)
I would rather not introduce this function until there is a caller. This 
is because, I believe, there are some assumptions on the state of the 
cache before we can turn off the MMU. So I would like to see the caller 
in order to assess whether this function makes sense.
Cheers,

--
Julien Grall