Re: [PATCH v6 08/13] xen/arm: Fold mmu_init_secondary_cpu() to head.S

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Sep 16, 2023, at 05:58, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Henry,
> 
> On 28/08/2023 02:32, Henry Wang wrote:
>> Currently mmu_init_secondary_cpu() only enforces the page table
>> should not contain mapping that are both Writable and eXecutables
>> after boot. To ease the arch/arm/mm.c split work, fold this function
>> to head.S.
>> Introduce assembly macro pt_enforce_wxn for both arm32 and arm64.
>> For arm64, the macro is called at the end of enable_secondary_cpu_mm().
>> For arm32, the macro is called before secondary CPUs jumping into
>> the C world.
>> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
>> ---
>> v6:
>> - New patch.
>> ---
>>  xen/arch/arm/arm32/head.S     | 20 ++++++++++++++++++++
>>  xen/arch/arm/arm64/mmu/head.S | 21 +++++++++++++++++++++
>>  xen/arch/arm/include/asm/mm.h |  2 --
>>  xen/arch/arm/mm.c             |  6 ------
>>  xen/arch/arm/smpboot.c        |  2 --
>>  5 files changed, 41 insertions(+), 10 deletions(-)
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 33b038e7e0..39218cf15f 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -83,6 +83,25 @@
>>          isb
>>  .endm
>>  +/*
>> + * Enforce Xen page-tables do not contain mapping that are both
>> + * Writable and eXecutables.
>> + *
>> + * This should be called on each secondary CPU.
>> + */
>> +.macro pt_enforce_wxn tmp
>> +        mrc   CP32(\tmp, HSCTLR)
>> +        orr   \tmp, \tmp, #SCTLR_Axx_ELx_WXN
>> +        dsb
>> +        mcr   CP32(\tmp, HSCTLR)
>> +        /*
>> +         * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
>> +         * before flushing the TLBs.
>> +         */
>> +        isb
>> +        flush_xen_tlb_local \tmp
>> +.endm
>> +
>>  /*
>>   * Common register usage in this file:
>>   *   r0  -
>> @@ -254,6 +273,7 @@ secondary_switched:
>>          /* Use a virtual address to access the UART. */
>>          mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
>>  #endif
>> +        pt_enforce_wxn r0
>>          PRINT("- Ready -\r\n")
>>          /* Jump to C world */
>>          mov_w r2, start_secondary
>> diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
>> index a5271e3880..25028bdf07 100644
>> --- a/xen/arch/arm/arm64/mmu/head.S
>> +++ b/xen/arch/arm/arm64/mmu/head.S
>> @@ -31,6 +31,25 @@
>>          isb
>>  .endm
>>  +/*
>> + * Enforce Xen page-tables do not contain mapping that are both
>> + * Writable and eXecutables.
>> + *
>> + * This should be called on each secondary CPU.
>> + */
>> +.macro pt_enforce_wxn tmp
>> +       mrs   \tmp, SCTLR_EL2
>> +       orr   \tmp, \tmp, #SCTLR_Axx_ELx_WXN
>> +       dsb   sy
>> +       msr   SCTLR_EL2, \tmp
>> +       /*
>> +        * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
>> +        * before flushing the TLBs.
>> +        */
>> +       isb
>> +       flush_xen_tlb_local
>> +.endm
>> +
> 
> It would be preferable if we can set the flag right when the MMU is 
> initialized enabled configured. This would avoid the extra TLB flush and 
> SCTLR dance. How about the following (not compiled/cleaned) code:

Thank you for the detailed information. Sure, I will try below code and keep you
updated about if it works. Will update the patch accordingly.

> 
> diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
> index a5271e388071..6b19d15ff89f 100644
> --- a/xen/arch/arm/arm64/mmu/head.S
> +++ b/xen/arch/arm/arm64/mmu/head.S
> @@ -264,10 +264,11 @@ ENDPROC(create_page_tables)
>  * Inputs:
>  *   x0 : Physical address of the page tables.
>  *
> - * Clobbers x0 - x4
> + * Clobbers x0 - x6
>  */
> enable_mmu:
>         mov   x4, x0
> +        mov   x5, x1
>         PRINT("- Turning on paging -\r\n")
> 
>         /*
> @@ -283,6 +284,7 @@ enable_mmu:
>         mrs   x0, SCTLR_EL2
>         orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
>         orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
> +        orr   x0, x0, x5                /* Enable extra flags */
>         dsb   sy                     /* Flush PTE writes and finish reads */
>         msr   SCTLR_EL2, x0          /* now paging is enabled */
>         isb                          /* Now, flush the icache */
> @@ -297,16 +299,17 @@ ENDPROC(enable_mmu)
>  * Inputs:
>  *   lr : Virtual address to return to.
>  *
> - * Clobbers x0 - x5
> + * Clobbers x0 - x6
>  */
> ENTRY(enable_secondary_cpu_mm)
> -        mov   x5, lr
> +        mov   x6, lr
> 
>         load_paddr x0, init_ttbr
>         ldr   x0, [x0]
> 
> +        mov   x1, #SCTLR_Axx_ELx_WXN        /* Enable WxN from the start */
>         bl    enable_mmu
> -        mov   lr, x5
> +        mov   lr, x6
> 
>         /* Return to the virtual address requested by the caller. */
>         ret
> @@ -320,16 +323,17 @@ ENDPROC(enable_secondary_cpu_mm)
>  * Inputs:
>  *   lr : Virtual address to return to.
>  *
> - * Clobbers x0 - x5
> + * Clobbers x0 - x6
>  */
> ENTRY(enable_boot_cpu_mm)
> -        mov   x5, lr
> +        mov   x6, lr
> 
>         bl    create_page_tables
>         load_paddr x0, boot_pgtable
> 
> +        mov   x1, #0        /* No extra SCTLR flags */
>         bl    enable_mmu
> -        mov   lr, x5
> +        mov   lr, x6
> 
>         /*
>          * The MMU is turned on and we are in the 1:1 mapping. Switch
> 
> The same logic could be used for arm32.

Sure. Will do that together.

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall