| Message ID | 20190610193215.23704-17-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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: > Boot CPU and secondary CPUs will use different entry point to C code. At > the moment, the decision on which entry to use is taken within launch(). > > In order to avoid a branch for the decision and make the code clearer, > launch() is reworked to take in parameters the entry point and its > arguments. > > Lastly, document the behavior and the main registers usage within the > function. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/arm64/head.S | 41 +++++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 4f7fa6769f..130ab66d8e 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -312,6 +312,11 @@ primary_switched: > /* Use a virtual address to access the UART. */ > ldr x23, =EARLY_UART_VIRTUAL_ADDRESS > #endif > + PRINT("- Ready -\r\n") > + /* Setup the arguments for start_xen and jump to C world */ > + mov x0, x20 /* x0 := phys_offset */ > + mov x1, x21 /* x1 := paddr(FDT) */ > + ldr x2, =start_xen > b launch > ENDPROC(real_start) > > @@ -374,6 +379,9 @@ secondary_switched: > /* Use a virtual address to access the UART. */ > ldr x23, =EARLY_UART_VIRTUAL_ADDRESS > #endif > + PRINT("- Ready -\r\n") > + /* Jump to C world */ > + ldr x2, =start_secondary > b launch > ENDPROC(init_secondary) > > @@ -734,23 +742,24 @@ setup_fixmap: > ret > ENDPROC(setup_fixmap) > > +/* > + * Setup the initial stack and jump to the C world > + * > + * Inputs: > + * x0 : Argument 0 of the C function to call > + * x1 : Argument 1 of the C function to call > + * x2 : C entry point I know it is the last one before C-land, but we might as well add a "Clobbers" section too, just in case. Here it clobbers x4 (or x3, see below). > + */ > launch: > - PRINT("- Ready -\r\n") > - > - ldr x0, =init_data > - add x0, x0, #INITINFO_stack /* Find the boot-time stack */ > - ldr x0, [x0] > - add x0, x0, #STACK_SIZE /* (which grows down from the top). */ > - sub x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */ > - mov sp, x0 > - > - cbnz x22, 1f > - > - mov x0, x20 /* Marshal args: - phys_offset */ > - mov x1, x21 /* - FDT */ > - b start_xen /* and disappear into the land of C */ > -1: > - b start_secondary /* (to the appropriate entry point) */ > + ldr x4, =init_data why not use x3 instead of x4? > + add x4, x4, #INITINFO_stack /* Find the boot-time stack */ > + ldr x4, [x4] > + add x4, x4, #STACK_SIZE /* (which grows down from the top). */ If you are going to respin it, could you please align the comments a bit better (one space to the right)? > + sub x4, x4, #CPUINFO_sizeof /* Make room for CPU save record */ > + mov sp, x4 > + > + /* Jump to C world */ > + br x2 > ENDPROC(launch) > > /* Fail-stop */
Hi Stefano, On 6/26/19 8:12 PM, Stefano Stabellini wrote: > On Mon, 10 Jun 2019, Julien Grall wrote: >> Boot CPU and secondary CPUs will use different entry point to C code. At >> the moment, the decision on which entry to use is taken within launch(). >> >> In order to avoid a branch for the decision and make the code clearer, >> launch() is reworked to take in parameters the entry point and its >> arguments. >> >> Lastly, document the behavior and the main registers usage within the >> function. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/arm64/head.S | 41 +++++++++++++++++++++++++---------------- >> 1 file changed, 25 insertions(+), 16 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index 4f7fa6769f..130ab66d8e 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -312,6 +312,11 @@ primary_switched: >> /* Use a virtual address to access the UART. */ >> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS >> #endif >> + PRINT("- Ready -\r\n") >> + /* Setup the arguments for start_xen and jump to C world */ >> + mov x0, x20 /* x0 := phys_offset */ >> + mov x1, x21 /* x1 := paddr(FDT) */ >> + ldr x2, =start_xen >> b launch >> ENDPROC(real_start) >> >> @@ -374,6 +379,9 @@ secondary_switched: >> /* Use a virtual address to access the UART. */ >> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS >> #endif >> + PRINT("- Ready -\r\n") >> + /* Jump to C world */ >> + ldr x2, =start_secondary >> b launch >> ENDPROC(init_secondary) >> >> @@ -734,23 +742,24 @@ setup_fixmap: >> ret >> ENDPROC(setup_fixmap) >> >> +/* >> + * Setup the initial stack and jump to the C world >> + * >> + * Inputs: >> + * x0 : Argument 0 of the C function to call >> + * x1 : Argument 1 of the C function to call >> + * x2 : C entry point > > I know it is the last one before C-land, but we might as well add a > "Clobbers" section too, just in case. Here it clobbers x4 (or x3, see > below). Sure. > > >> + */ >> launch: >> - PRINT("- Ready -\r\n") >> - >> - ldr x0, =init_data >> - add x0, x0, #INITINFO_stack /* Find the boot-time stack */ >> - ldr x0, [x0] >> - add x0, x0, #STACK_SIZE /* (which grows down from the top). */ >> - sub x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */ >> - mov sp, x0 >> - >> - cbnz x22, 1f >> - >> - mov x0, x20 /* Marshal args: - phys_offset */ >> - mov x1, x21 /* - FDT */ >> - b start_xen /* and disappear into the land of C */ >> -1: >> - b start_secondary /* (to the appropriate entry point) */ >> + ldr x4, =init_data > > why not use x3 instead of x4? I think a remnant of early rework when start_secondary was taking 3 parameters. I will update it. > > >> + add x4, x4, #INITINFO_stack /* Find the boot-time stack */ >> + ldr x4, [x4] >> + add x4, x4, #STACK_SIZE /* (which grows down from the top). */ > > If you are going to respin it, could you please align the comments a bit > better (one space to the right)? Sure. > > >> + sub x4, x4, #CPUINFO_sizeof /* Make room for CPU save record */ >> + mov sp, x4 >> + >> + /* Jump to C world */ >> + br x2 >> ENDPROC(launch) >> >> /* Fail-stop */ Cheers,
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 4f7fa6769f..130ab66d8e 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -312,6 +312,11 @@ primary_switched: /* Use a virtual address to access the UART. */ ldr x23, =EARLY_UART_VIRTUAL_ADDRESS #endif + PRINT("- Ready -\r\n") + /* Setup the arguments for start_xen and jump to C world */ + mov x0, x20 /* x0 := phys_offset */ + mov x1, x21 /* x1 := paddr(FDT) */ + ldr x2, =start_xen b launch ENDPROC(real_start) @@ -374,6 +379,9 @@ secondary_switched: /* Use a virtual address to access the UART. */ ldr x23, =EARLY_UART_VIRTUAL_ADDRESS #endif + PRINT("- Ready -\r\n") + /* Jump to C world */ + ldr x2, =start_secondary b launch ENDPROC(init_secondary) @@ -734,23 +742,24 @@ setup_fixmap: ret ENDPROC(setup_fixmap) +/* + * Setup the initial stack and jump to the C world + * + * Inputs: + * x0 : Argument 0 of the C function to call + * x1 : Argument 1 of the C function to call + * x2 : C entry point + */ launch: - PRINT("- Ready -\r\n") - - ldr x0, =init_data - add x0, x0, #INITINFO_stack /* Find the boot-time stack */ - ldr x0, [x0] - add x0, x0, #STACK_SIZE /* (which grows down from the top). */ - sub x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */ - mov sp, x0 - - cbnz x22, 1f - - mov x0, x20 /* Marshal args: - phys_offset */ - mov x1, x21 /* - FDT */ - b start_xen /* and disappear into the land of C */ -1: - b start_secondary /* (to the appropriate entry point) */ + ldr x4, =init_data + add x4, x4, #INITINFO_stack /* Find the boot-time stack */ + ldr x4, [x4] + add x4, x4, #STACK_SIZE /* (which grows down from the top). */ + sub x4, x4, #CPUINFO_sizeof /* Make room for CPU save record */ + mov sp, x4 + + /* Jump to C world */ + br x2 ENDPROC(launch) /* Fail-stop */
Boot CPU and secondary CPUs will use different entry point to C code. At the moment, the decision on which entry to use is taken within launch(). In order to avoid a branch for the decision and make the code clearer, launch() is reworked to take in parameters the entry point and its arguments. Lastly, document the behavior and the main registers usage within the function. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/arm64/head.S | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)