| Message ID | 20190610193215.23704-7-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm64: Rework head.S to make it more compliant with the Arm Arm | expand |
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)
Hi Stefano, On 26/06/2019 02:00, Stefano Stabellini wrote: > 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 I notice a few typo in my commit message: s/distincted/distinct/ >> functions. >> >> Follow-ups will remove unecessary calls and do further improvement s/unecessary/unnecessary/ >> +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? Urgh. this line is dropped in a later patch, so the issue only would happen during bisection. I will update the code to avoid the infinite loop here. > > Everything else looks good, and I like the reorg of the code. Thank you! I will rework the arm32 code the same way then :). > > >> 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) > Cheers,
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: 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) /* Fail-stop */ fail: PRINT("- Boot failed -\r\n")
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(-)