Re: [PATCH v3 05/52] xen/arm64: head: Introduce enable_boot_mmu and enable_runtime_mmu

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 26/06/2023 04:33, Penny Zheng wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
>
> At the moment, on MMU system, enable_mmu() will return to an
> address in the 1:1 mapping, then each path is responsible to
> switch to virtual runtime mapping. Then remove_identity_mapping()
> is called to remove all 1:1 mapping.
The identity mapping is only removed for the boot CPU. You mention it 
correctly below but here it lead to think it may be called on the 
secondary CPU. So I would add 'on the boot CPU'.
> Since remove_identity_mapping() is not necessary on Non-MMU system,
> and we also avoid creating empty function for Non-MMU system, trying
> to keep only one codeflow in arm64/head.S, we move path switch and
> remove_identity_mapping() in enable_mmu() on MMU system.
>
> As the remove_identity_mapping should only be called for the boot
> CPU only, so we introduce enable_boot_mmu for boot CPU and
> enable_runtime_mmu for secondary CPUs in this patch.
NIT: Add () after enable_runtime_mmu to be consistent with the rest of 
commit message. Same for the title.
Also, I would prefer if we name the functions properly from the start. 
So we don't have to rename them when they are moved in patch 7.
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> v3:
> - new patch
> ---
>   xen/arch/arm/arm64/head.S | 87 +++++++++++++++++++++++++++++++--------
>   1 file changed, 70 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 10a07db428..4dfbe0bc6f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -314,21 +314,12 @@ real_start_efi:
>   
>           bl    check_cpu_mode
>           bl    cpu_init
> -        bl    create_page_tables
> -        load_paddr x0, boot_pgtable
> -        bl    enable_mmu
>   
>           /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
This comment is not accurate anymore given that the MMU is off.

> -        ldr   x0, =primary_switched
> -        br    x0
> +        ldr   lr, =primary_switched
> +        b     enable_boot_mmu
> +
>   primary_switched:
> -        /*
> -         * The 1:1 map may clash with other parts of the Xen virtual memory
> -         * layout. As it is not used anymore, remove it completely to
> -         * avoid having to worry about replacing existing mapping
> -         * afterwards.
> -         */
> -        bl    remove_identity_mapping
>           bl    setup_fixmap
>   #ifdef CONFIG_EARLY_PRINTK
>           /* Use a virtual address to access the UART. */
> @@ -373,13 +364,11 @@ GLOBAL(init_secondary)
>   #endif
>           bl    check_cpu_mode
>           bl    cpu_init
> -        load_paddr x0, init_ttbr
> -        ldr   x0, [x0]
> -        bl    enable_mmu
>   
>           /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
> -        ldr   x0, =secondary_switched
> -        br    x0
> +        ldr   lr, =secondary_switched
> +        b     enable_runtime_mmu
> +
>   secondary_switched:
>   #ifdef CONFIG_EARLY_PRINTK
>           /* Use a virtual address to access the UART. */
> @@ -694,6 +683,70 @@ enable_mmu:
>           ret
>   ENDPROC(enable_mmu)
>   
> +/*
> + * Turn on the Data Cache and the MMU. The function will return
> + * to the virtual address provided in LR (e.g. the runtime mapping).
The description here is exactly the same as below. However, there is a 
different between the two functions. One is to deal with the secondary 
CPUs whilst the second is for the boot CPUs.
> + *
> + * Inputs:
> + *   lr : Virtual address to return to.
> + *
> + * Clobbers x0 - x5
> + */
> +enable_runtime_mmu:
I find "runtime" confusing in this case. How about 
"enable_secondary_cpu_mm"?
> +        mov   x5, lr
> +
> +        load_paddr x0, init_ttbr
> +        ldr   x0, [x0]
> +
> +        bl    enable_mmu
> +        mov   lr, x5
> +
> +        /* return to secondary_switched */
> +        ret
> +ENDPROC(enable_runtime_mmu)
> +
> +/*
> + * Turn on the Data Cache and the MMU. The function will return
> + * to the virtual address provided in LR (e.g. the runtime mapping).
Similar remark as for the comment above.

> + *
> + * Inputs:
> + *   lr : Virtual address to return to.
> + *
> + * Clobbers x0 - x5
> + */
> +enable_boot_mmu:
Based on my comment above, I would sugesst to call it "enable_boot_cpu_mm"

> +        mov   x5, lr
> +
> +        bl    create_page_tables
> +        load_paddr x0, boot_pgtable
> +
> +        bl    enable_mmu
> +        mov   lr, x5
> +
> +        /*
> +         * The MMU is turned on and we are in the 1:1 mapping. Switch
> +         * to the runtime mapping.
> +         */
> +        ldr   x0, =1f
> +        br    x0
> +1:
> +        /*
> +         * The 1:1 map may clash with other parts of the Xen virtual memory
> +         * layout. As it is not used anymore, remove it completely to
> +         * avoid having to worry about replacing existing mapping
> +         * afterwards. Function will return to primary_switched.
> +         */
> +        b     remove_identity_mapping
> +
> +        /*
> +         * Here might not be reached, as "ret" in remove_identity_mapping
> +         * will use the return address in LR in advance. But keep ret here
> +         * might be more safe if "ret" in remove_identity_mapping is removed
> +         * in future.
> +         */
> +        ret
Given this path is meant to be unreachable, I would prefer if we call 
"fail".
> +ENDPROC(enable_boot_mmu)
> +
>   /*
>    * Remove the 1:1 map from the page-tables. It is not easy to keep track
>    * where the 1:1 map was mapped, so we will look for the top-level entry
Cheers,

--
Julien Grall