| Message ID | 20190722213958.5761-17-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Rework head.S to make it more compliant with the Arm Arm | expand |
On Mon, 22 Jul 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> > > --- > Changes in v2: > - Use x3 instead of x4 > - Add a clobbers section > --- > xen/arch/arm/arm64/head.S | 43 +++++++++++++++++++++++++++---------------- > 1 file changed, 27 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index f165dd61ca..7541635102 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 := Physical 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) > > @@ -732,23 +740,26 @@ 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 > + * > + * Clobbers x3 > + */ > 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 x3, =init_data > + add x3, x3, #INITINFO_stack /* Find the boot-time stack */ > + ldr x3, [x3] > + add x3, x3, #STACK_SIZE /* (which grows down from the top). */ ^ please move 1 space to the right Aside from this minor code style thing Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > + sub x3, x3, #CPUINFO_sizeof /* Make room for CPU save record */ > + mov sp, x3 > + > + /* Jump to C world */ > + br x2 > ENDPROC(launch) > > /* Fail-stop */ > -- > 2.11.0 >
Hi Stefano, On 7/30/19 6:45 PM, Stefano Stabellini wrote: > On Mon, 22 Jul 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> >> >> --- >> Changes in v2: >> - Use x3 instead of x4 >> - Add a clobbers section >> --- >> xen/arch/arm/arm64/head.S | 43 +++++++++++++++++++++++++++---------------- >> 1 file changed, 27 insertions(+), 16 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index f165dd61ca..7541635102 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 := Physical 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) >> >> @@ -732,23 +740,26 @@ 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 >> + * >> + * Clobbers x3 >> + */ >> 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 x3, =init_data >> + add x3, x3, #INITINFO_stack /* Find the boot-time stack */ >> + ldr x3, [x3] >> + add x3, x3, #STACK_SIZE /* (which grows down from the top). */ > ^ please move 1 space to the > right > > Aside from this minor code style thing If I wanted to be picky, all the rest of the code in this file is indentation at column 38. So this line has the correct indentation but the two others are not. However, this would means there are not space between the instruction and the comment: foobar/* ... */ So I guess, indenting at column 39 would be the best here, although I already know someone that will be unhappy with in the future ;). > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> Thank you! Cheers,
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index f165dd61ca..7541635102 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 := Physical 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) @@ -732,23 +740,26 @@ 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 + * + * Clobbers x3 + */ 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 x3, =init_data + add x3, x3, #INITINFO_stack /* Find the boot-time stack */ + ldr x3, [x3] + add x3, x3, #STACK_SIZE /* (which grows down from the top). */ + sub x3, x3, #CPUINFO_sizeof /* Make room for CPU save record */ + mov sp, x3 + + /* 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> --- Changes in v2: - Use x3 instead of x4 - Add a clobbers section --- xen/arch/arm/arm64/head.S | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-)