[Xen-devel,13/17] xen/arm64: head: Don't setup the fixmap on secondary CPUs

setup_fixmap() will setup the fixmap in the boot page tables in order to
use earlyprintk and also update the register x23 holding the address to
the UART.

However, secondary CPUs are switching to the runtime page tables before
using the earlyprintk again. So settting up the fixmap in the boot pages
tables is pointless.

This means most of setup_fixmap() is not necessary for the secondary
CPUs. The update of UART address is now moved out of setup_fixmap() and
duplicated in the CPU boot and secondary CPUs boot. Additionally, the
call to setup_fixmap() is removed from secondary CPUs boot.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

On Mon, 10 Jun 2019, Julien Grall wrote:
> setup_fixmap() will setup the fixmap in the boot page tables in order to
> use earlyprintk and also update the register x23 holding the address to
> the UART.
> 
> However, secondary CPUs are switching to the runtime page tables before
> using the earlyprintk again. So settting up the fixmap in the boot pages
> tables is pointless.

Typo: settting

Also, you could add that it is "impossible" to find ourselves in the
position of needing earlyprintk for secondary CPUs before the runtime
page tables are up, because it is done right after in the boot sequence.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> This means most of setup_fixmap() is not necessary for the secondary
> CPUs. The update of UART address is now moved out of setup_fixmap() and
> duplicated in the CPU boot and secondary CPUs boot. Additionally, the
> call to setup_fixmap() is removed from secondary CPUs boot.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 6be4af7579..192af3e8a2 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -301,6 +301,10 @@ real_start_efi:
>          br    x0
>  primary_switched:
>          bl    setup_fixmap
> +#ifdef CONFIG_EARLY_PRINTK
> +        /* Use a virtual address to access the UART. */
> +        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> +#endif
>          b     launch
>  ENDPROC(real_start)
>  
> @@ -343,8 +347,6 @@ GLOBAL(init_secondary)
>          ldr   x0, =secondary_switched
>          br    x0
>  secondary_switched:
> -        bl    setup_fixmap
> -
>          /*
>           * Non-boot CPUs need to move on to the proper pagetables, which were
>           * setup in init_secondary_pagetables.
> @@ -361,6 +363,10 @@ secondary_switched:
>          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
> +#endif
>          b     launch
>  ENDPROC(init_secondary)
>  
> @@ -631,10 +637,6 @@ setup_fixmap:
>           * don't need the 1:1 map any more */
>          dsb   sy
>  #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
> -        /* Non-boot CPUs don't need to rebuild the fixmap itself, just
> -         * the mapping from boot_second to xen_fixmap */
> -        cbnz  x22, 1f
> -
>          /* Add UART to the fixmap table */
>          ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
>          lsr   x2, x23, #THIRD_SHIFT
> @@ -642,7 +644,6 @@ setup_fixmap:
>          mov   x3, #PT_DEV_L3
>          orr   x2, x2, x3             /* x2 := 4K dev map including UART */
>          str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
> -1:
>  
>          /* Map fixmap into boot_second */
>          ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> @@ -652,9 +653,6 @@ setup_fixmap:
>          ldr   x1, =FIXMAP_ADDR(0)
>          lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
>          str   x2, [x4, x1]           /* Map it in the fixmap's slot */
> -
> -        /* Use a virtual address to access the UART. */
> -        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
>  
>          /*
> -- 
> 2.11.0
>

Hi,

On 6/26/19 7:51 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> setup_fixmap() will setup the fixmap in the boot page tables in order to
>> use earlyprintk and also update the register x23 holding the address to
>> the UART.
>>
>> However, secondary CPUs are switching to the runtime page tables before
>> using the earlyprintk again. So settting up the fixmap in the boot pages
>> tables is pointless.
> 
> Typo: settting

ok.

> 
> Also, you could add that it is "impossible" to find ourselves in the
> position of needing earlyprintk for secondary CPUs before the runtime
> page tables are up, because it is done right after in the boot sequence.

It is kind of implied by the comment and the code below. But I can 
clearly state it.

> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Cheers,