Re: [PATCH v5 5/5] xen/arm64: smpboot: Directly switch to the runtime page-tables

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 27 Jan 2023, at 20:55, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Switching TTBR while the MMU is on is not safe. Now that the identity
> mapping will not clash with the rest of the memory layout, we can avoid
> creating temporary page-tables every time a CPU is brought up.
> 
> The arm32 code will use a different approach. So this issue is for now
> only resolved on arm64.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Tested-by: Luca Fancellu <luca.fancellu@xxxxxxx>

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx 
<mailto:bertrand.marquis@xxxxxxx>>

Cheers
Bertrand

> 
> ---
>    Changes in v5:
>        - Add Luca's reviewed-by and tested-by tags.
> 
>    Changes in v4:
>        - Somehow I forgot to send it in v3. So re-include it.
> 
>    Changes in v2:
>        - Remove arm32 code
> ---
> xen/arch/arm/arm32/smpboot.c   |  4 ++++
> xen/arch/arm/arm64/head.S      | 29 +++++++++--------------------
> xen/arch/arm/arm64/smpboot.c   | 15 ++++++++++++++-
> xen/arch/arm/include/asm/smp.h |  1 +
> xen/arch/arm/smpboot.c         |  1 +
> 5 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/smpboot.c b/xen/arch/arm/arm32/smpboot.c
> index e7368665d50d..518e9f9c7e70 100644
> --- a/xen/arch/arm/arm32/smpboot.c
> +++ b/xen/arch/arm/arm32/smpboot.c
> @@ -21,6 +21,10 @@ int arch_cpu_up(int cpu)
>     return platform_cpu_up(cpu);
> }
> 
> +void arch_cpu_up_finish(void)
> +{
> +}
> +
> /*
>  * Local variables:
>  * mode: C
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 5efd442b24af..a61b4d3c2738 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -308,6 +308,7 @@ 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. */
> @@ -365,29 +366,14 @@ GLOBAL(init_secondary)
> #endif
>         bl    check_cpu_mode
>         bl    cpu_init
> -        bl    create_page_tables
> +        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
> secondary_switched:
> -        /*
> -         * Non-boot CPUs need to move on to the proper pagetables, which were
> -         * setup in init_secondary_pagetables.
> -         *
> -         * XXX: This is not compliant with the Arm Arm.
> -         */
> -        ldr   x4, =init_ttbr         /* VA of TTBR0_EL2 stashed by CPU 0 */
> -        ldr   x4, [x4]               /* Actual value */
> -        dsb   sy
> -        msr   TTBR0_EL2, x4
> -        dsb   sy
> -        isb
> -        tlbi  alle2
> -        dsb   sy                     /* Ensure completion of TLB flush */
> -        isb
> -
> #ifdef CONFIG_EARLY_PRINTK
>         /* Use a virtual address to access the UART. */
>         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> @@ -672,9 +658,13 @@ ENDPROC(create_page_tables)
>  * mapping. In other word, the caller is responsible to switch to the runtime
>  * mapping.
>  *
> - * Clobbers x0 - x3
> + * Inputs:
> + *   x0 : Physical address of the page tables.
> + *
> + * Clobbers x0 - x4
>  */
> enable_mmu:
> +        mov   x4, x0
>         PRINT("- Turning on paging -\r\n")
> 
>         /*
> @@ -685,8 +675,7 @@ enable_mmu:
>         dsb   nsh
> 
>         /* Write Xen's PT's paddr into TTBR0_EL2 */
> -        load_paddr x0, boot_pgtable
> -        msr   TTBR0_EL2, x0
> +        msr   TTBR0_EL2, x4
>         isb
> 
>         mrs   x0, SCTLR_EL2
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 694fbf67e62a..9637f424699e 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -106,10 +106,23 @@ int __init arch_cpu_init(int cpu, struct dt_device_node 
> *dn)
> 
> int arch_cpu_up(int cpu)
> {
> +    int rc;
> +
>     if ( !smp_enable_ops[cpu].prepare_cpu )
>         return -ENODEV;
> 
> -    return smp_enable_ops[cpu].prepare_cpu(cpu);
> +    update_identity_mapping(true);
> +
> +    rc = smp_enable_ops[cpu].prepare_cpu(cpu);
> +    if ( rc )
> +        update_identity_mapping(false);
> +
> +    return rc;
> +}
> +
> +void arch_cpu_up_finish(void)
> +{
> +    update_identity_mapping(false);
> }
> 
> /*
> diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h
> index 8133d5c29572..a37ca55bff2c 100644
> --- a/xen/arch/arm/include/asm/smp.h
> +++ b/xen/arch/arm/include/asm/smp.h
> @@ -25,6 +25,7 @@ extern void noreturn stop_cpu(void);
> extern int arch_smp_init(void);
> extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
> extern int arch_cpu_up(int cpu);
> +extern void arch_cpu_up_finish(void);
> 
> int cpu_up_send_sgi(int cpu);
> 
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 412ae2286906..4a89b3a8345b 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -500,6 +500,7 @@ int __cpu_up(unsigned int cpu)
>     init_data.cpuid = ~0;
>     smp_up_cpu = MPIDR_INVALID;
>     clean_dcache(smp_up_cpu);
> +    arch_cpu_up_finish();
> 
>     if ( !cpu_online(cpu) )
>     {
> -- 
> 2.38.1
> 
>