Re: [Xen-devel] [PATCH 06/17] xen/arm64: head: Introduce distinct paths for the boot CPU and secondary CPUs

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xenproject.org, Oleksandr_Tyshchenko@epam.com,
	Stefano Stabellini <sstabellini@kernel.org>,
	andrii_anisov@epam.com, andre.przywara@arm.com
Subject: Re: [Xen-devel] [PATCH 06/17] xen/arm64: head: Introduce distinct paths for the boot CPU and secondary CPUs
Date: Tue, 25 Jun 2019 18:00:47 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.1906251727410.5851@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <20190610193215.23704-7-julien.grall@arm.com>

On Mon, 10 Jun 2019, Julien Grall wrote:
> The boot code is currently quite difficult to go through because of the
> lack of documentation and a number of indirection to avoid executing
> some path in either the boot CPU or secondary CPUs.
> 
> In an attempt to make the boot code easier to follow, each parts of the
> boot are now in separate functions. Furthermore, the paths for the boot
> CPU and secondary CPUs are now distincted and for now will call each
> functions.
> 
> Follow-ups will remove unecessary calls and do further improvement
> (such as adding documentation and reshuffling).
> 
> Note that the switch from using the ID mapping to the runtime mapping
> is duplicated for each path. This is because in the future we will need
> to stay longer in the ID mapping for the boot CPU.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 57 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 9142b4a774..ccd8a1b0a8 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -290,7 +290,19 @@ real_start_efi:
>  
>          mov   x22, #0                /* x22 := is_secondary_cpu */
>  
> -        b     common_start
> +        bl    check_cpu_mode
> +        bl    zero_bss
> +        bl    cpu_init
> +        bl    create_page_tables
> +        bl    enable_mmu
> +
> +        /* We are still in the ID map. Jump to the runtime Virtual Address. */
> +        ldr   x0, =primary_switched
> +        br    x0
> +primary_switched:
> +        bl    setup_fixmap
> +        b     launch
> +ENDPROC(real_start)
>  
>  GLOBAL(init_secondary)
>          msr   DAIFSet, 0xf           /* Disable all interrupts */
> @@ -324,9 +336,21 @@ GLOBAL(init_secondary)
>          print_reg x24
>          PRINT(" booting -\r\n")
>  #endif
> -
> -common_start:
> -
> +        bl    check_cpu_mode
> +        bl    zero_bss
> +        bl    cpu_init
> +        bl    create_page_tables
> +        bl    enable_mmu
> +
> +        /* We are still in the ID map. Jump to the runtime Virtual Address. */
> +        ldr   x0, =secondary_switched
> +        br    x0
> +secondary_switched:
> +        bl    setup_fixmap
> +        b     launch
> +ENDPROC(init_secondary)
> +
> +check_cpu_mode:
>          PRINT("- Current EL ")
>          mrs   x5, CurrentEL
>          print_reg x5
> @@ -343,7 +367,10 @@ common_start:
>          b fail
>  
>  el2:    PRINT("- Xen starting at EL2 -\r\n")
> +        ret
> +ENDPROC(check_cpu_mode)
>  
> +zero_bss:
>          /* Zero BSS only when requested */
>          cbnz  x26, skip_bss
>  
> @@ -356,6 +383,10 @@ el2:    PRINT("- Xen starting at EL2 -\r\n")
>          b.lo  1b
>  
>  skip_bss:
> +        ret
> +ENDPROC(zero_bss)
> +
> +cpu_init:
>          PRINT("- Setting up control registers -\r\n")
>  
>          /* Set up memory attribute type tables */
> @@ -390,7 +421,10 @@ skip_bss:
>           * are handled using the EL2 stack pointer, rather
>           * than SP_EL0. */
>          msr spsel, #1
> +        ret
> +ENDPROC(cpu_init)
>  
> +create_page_tables:
>          /* Rebuild the boot pagetable's first-level entries. The structure
>           * is described in mm.c.
>           *
> @@ -515,6 +549,10 @@ virtphys_clash:
>          b     fail
>  
>  1:
> +        ret
> +ENDPROC(create_page_tables)
> +
> +enable_mmu:
>          PRINT("- Turning on paging -\r\n")
>  
>          /*
> @@ -524,16 +562,16 @@ virtphys_clash:
>          tlbi  alle2                  /* Flush hypervisor TLBs */
>          dsb   nsh
>  
> -        ldr   x1, =paging            /* Explicit vaddr, not RIP-relative */
>          mrs   x0, SCTLR_EL2
>          orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
>          orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
>          dsb   sy                     /* Flush PTE writes and finish reads */
>          msr   SCTLR_EL2, x0          /* now paging is enabled */
>          isb                          /* Now, flush the icache */
> -        br    x1                     /* Get a proper vaddr into PC */
> -paging:
> +        ret
> +ENDPROC(enable_mmu)
>  
> +setup_fixmap:
>          /* Now we can install the fixmap and dtb mappings, since we
>           * don't need the 1:1 map any more */
>          dsb   sy
> @@ -575,7 +613,10 @@ paging:
>          tlbi  alle2
>          dsb   sy                     /* Ensure completion of TLB flush */
>          isb
> +        ret
> +ENDPROC(setup_fixmap)
>  
> +launch:
>          PRINT("- Ready -\r\n")
>  
>          /* The boot CPU should go straight into C now */
> @@ -594,7 +635,6 @@ paging:
>          dsb   sy                     /* Ensure completion of TLB flush */
>          isb
>  
> -launch:

Just below PRINT("- Ready -\r\n"), there is still a:

  cbz   x22, launch

moving the launch label up it looks like it will cause an infinite loop?

Everything else looks good, and I like the reorg of the code.

>          ldr   x0, =init_data
>          add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
>          ldr   x0, [x0]
> @@ -609,6 +649,7 @@ launch:
>          b     start_xen              /* and disappear into the land of C */
>  1:
>          b     start_secondary        /* (to the appropriate entry point) */
> +ENDPROC(launch)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel