Re: [RFC v2 02/12] xen/arm32: head: Jump to the runtime mapping in enable_mmu()

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 22/10/2022 17:04, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> At the moment, enable_mmu() will return to an address in the 1:1 mapping
> and each path are responsible to switch to the runtime mapping.
s/are/is/

> 
> In a follow-up patch, the behavior to switch to the runtime mapping
> will become more complex. So to avoid more code/comment duplication,
> move the switch in enable_mmu().
> 
> Lastly, take the opportunity to replace load from literal pool with
> mov_w.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
>  xen/arch/arm/arm32/head.S | 51 ++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index a558c2a6876e..163bd6596dec 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -167,19 +167,12 @@ past_zImage:
>          bl    check_cpu_mode
>          bl    cpu_init
>          bl    create_page_tables
> -        bl    enable_mmu
> 
> -        /* We are still in the 1:1 mapping. Jump to the runtime Virtual 
> Address. */
> -        ldr   r0, =primary_switched
> -        mov   pc, r0
> +        /* Address in the runtime mapping to jump to after the MMU is 
> enabled */
> +        mov_w lr, primary_switched
We seem to still widely use ldr instead of mov_w which is faster.
It looks like a prerequisite patch to convert all occurences or something to 
keep in a backlog.

> +        b     enable_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. */
> @@ -223,12 +216,10 @@ GLOBAL(init_secondary)
>          bl    check_cpu_mode
>          bl    cpu_init
>          bl    create_page_tables
> -        bl    enable_mmu
> -
> 
> -        /* We are still in the 1:1 mapping. Jump to the runtime Virtual 
> Address. */
> -        ldr   r0, =secondary_switched
> -        mov   pc, r0
> +        /* Address in the runtime mapping to jump to after the MMU is 
> enabled */
> +        mov_w lr, secondary_switched
> +        b     enable_mmu
>  secondary_switched:
>          /*
>           * Non-boot CPUs need to move on to the proper pagetables, which were
> @@ -523,9 +514,12 @@ virtphys_clash:
>  ENDPROC(create_page_tables)
> 
>  /*
> - * Turn on the Data Cache and the MMU. The function will return on the 1:1
> - * mapping. In other word, the caller is responsible to switch to the runtime
> - * mapping.
> + * Turn on the Data Cache and the MMU. The function will return
> + * to the virtual address provided in LR (e.g. the runtime mapping).
> + *
> + * Inputs:
> + *   r9 : paddr(start)
> + *   lr : Virtual address to return to
>   *
>   * Clobbers r0 - r3
>   */
> @@ -551,7 +545,24 @@ enable_mmu:
>          dsb                          /* Flush PTE writes and finish reads */
>          mcr   CP32(r0, HSCTLR)       /* now paging is enabled */
>          isb                          /* Now, flush the icache */
> -        mov   pc, lr
> +
> +        /*
> +         * The MMU is turned on and we are in the 1:1 mapping. Switch
> +         * to the runtime mapping.
> +         */
> +        mov_w r0, 1f
> +        mov   pc, r0
Would it be possible to stop using:
        mov pc, reg
in favor of using:
        bx reg

Some time ago I saw this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm/include/asm/assembler.h?id=6ebbf2ce437b33022d30badd49dc94d33ecfa498
which states that bx is faster.

> +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.
> +         *
> +         * On return this will jump to the virtual address requested by
> +         * the caller.
> +         */
> +        b     remove_identity_mapping
>  ENDPROC(enable_mmu)
> 
>  /*
> --
> 2.37.1
> 
> 

Apart from that, the change looks ok.

~Michal