| Message ID | 20190722213958.5761-34-julien.grall@arm.com (mailing list archive) |
|---|---|
| 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 using conditional instruction and make the call > 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: > - Patch added > --- > xen/arch/arm/arm32/head.S | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index e0f8c2e0cb..6d55a2119a 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -170,6 +170,11 @@ primary_switched: > /* Use a virtual address to access the UART. */ > mov_w r11, EARLY_UART_VIRTUAL_ADDRESS > #endif > + PRINT("- Ready -\r\n") > + /* Setup the arguments for start_xen and jump to C world */ > + mov r0, r10 /* r0 := Physical offset */ > + mov r1, r8 /* r1 := paddr(FDT) */ > + ldr r2, =start_xen > b launch > ENDPROC(start) > > @@ -234,6 +239,9 @@ secondary_switched: > /* Use a virtual address to access the UART. */ > mov_w r11, EARLY_UART_VIRTUAL_ADDRESS > #endif > + PRINT("- Ready -\r\n") > + /* Jump to C world */ > + ldr r2, =start_secondary > b launch > ENDPROC(init_secondary) > > @@ -578,19 +586,25 @@ setup_fixmap: > mov pc, lr > ENDPROC(setup_fixmap) > > +/* > + * Setup the initial stack and jump to the C world > + * > + * Inputs: > + * r0 : Argument 0 of the C function to call > + * r1 : Argument 1 of the C function to call > + * r2 : C entry point > + * > + * Clobbers r3 > + */ > launch: > - PRINT("- Ready -\r\n") > - > - ldr r0, =init_data > - add r0, #INITINFO_stack /* Find the boot-time stack */ > - ldr sp, [r0] > + ldr r3, =init_data > + add r3, #INITINFO_stack /* Find the boot-time stack */ > + ldr sp, [r3] > add sp, #STACK_SIZE /* (which grows down from the top). */ > sub sp, #CPUINFO_sizeof /* Make room for CPU save record */ > - teq r12, #0 > - moveq r0, r10 /* Marshal args: - phys_offset */ > - moveq r1, r8 /* - DTB address */ > - beq start_xen /* and disappear into the land of C */ > - b start_secondary /* (to the appropriate entry point) */ > + > + /* Jump to C world */ > + bx r2 Why bx? > ENDPROC(launch) > > /* Fail-stop */ > -- > 2.11.0 >
On 30/07/2019 22:21, 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 using conditional instruction and make the call >> 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: >> - Patch added >> --- >> xen/arch/arm/arm32/head.S | 34 ++++++++++++++++++++++++---------- >> 1 file changed, 24 insertions(+), 10 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index e0f8c2e0cb..6d55a2119a 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -170,6 +170,11 @@ primary_switched: >> /* Use a virtual address to access the UART. */ >> mov_w r11, EARLY_UART_VIRTUAL_ADDRESS >> #endif >> + PRINT("- Ready -\r\n") >> + /* Setup the arguments for start_xen and jump to C world */ >> + mov r0, r10 /* r0 := Physical offset */ >> + mov r1, r8 /* r1 := paddr(FDT) */ >> + ldr r2, =start_xen >> b launch >> ENDPROC(start) >> >> @@ -234,6 +239,9 @@ secondary_switched: >> /* Use a virtual address to access the UART. */ >> mov_w r11, EARLY_UART_VIRTUAL_ADDRESS >> #endif >> + PRINT("- Ready -\r\n") >> + /* Jump to C world */ >> + ldr r2, =start_secondary >> b launch >> ENDPROC(init_secondary) >> >> @@ -578,19 +586,25 @@ setup_fixmap: >> mov pc, lr >> ENDPROC(setup_fixmap) >> >> +/* >> + * Setup the initial stack and jump to the C world >> + * >> + * Inputs: >> + * r0 : Argument 0 of the C function to call >> + * r1 : Argument 1 of the C function to call >> + * r2 : C entry point >> + * >> + * Clobbers r3 >> + */ >> launch: >> - PRINT("- Ready -\r\n") >> - >> - ldr r0, =init_data >> - add r0, #INITINFO_stack /* Find the boot-time stack */ >> - ldr sp, [r0] >> + ldr r3, =init_data >> + add r3, #INITINFO_stack /* Find the boot-time stack */ >> + ldr sp, [r3] >> add sp, #STACK_SIZE /* (which grows down from the top). */ >> sub sp, #CPUINFO_sizeof /* Make room for CPU save record */ >> - teq r12, #0 >> - moveq r0, r10 /* Marshal args: - phys_offset */ >> - moveq r1, r8 /* - DTB address */ >> - beq start_xen /* and disappear into the land of C */ >> - b start_secondary /* (to the appropriate entry point) */ >> + >> + /* Jump to C world */ >> + bx r2 > > Why bx? The only two other possible instructions would be: 1) blx r2: we don't need to save the return address 2) mov pc, r2: The Arm Arm recommends to use bx/blx instead of this. So bx seems the best fit. Any other suggestion? Also, I would probably replace all the "mov pc, lr" I added with "bx lr". Cheers,
On Tue, 30 Jul 2019, Julien Grall wrote: > On 30/07/2019 22:21, 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 using conditional instruction and make the call > >> 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: > >> - Patch added > >> --- > >> xen/arch/arm/arm32/head.S | 34 ++++++++++++++++++++++++---------- > >> 1 file changed, 24 insertions(+), 10 deletions(-) > >> > >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > >> index e0f8c2e0cb..6d55a2119a 100644 > >> --- a/xen/arch/arm/arm32/head.S > >> +++ b/xen/arch/arm/arm32/head.S > >> @@ -170,6 +170,11 @@ primary_switched: > >> /* Use a virtual address to access the UART. */ > >> mov_w r11, EARLY_UART_VIRTUAL_ADDRESS > >> #endif > >> + PRINT("- Ready -\r\n") > >> + /* Setup the arguments for start_xen and jump to C world */ > >> + mov r0, r10 /* r0 := Physical offset */ > >> + mov r1, r8 /* r1 := paddr(FDT) */ > >> + ldr r2, =start_xen > >> b launch > >> ENDPROC(start) > >> > >> @@ -234,6 +239,9 @@ secondary_switched: > >> /* Use a virtual address to access the UART. */ > >> mov_w r11, EARLY_UART_VIRTUAL_ADDRESS > >> #endif > >> + PRINT("- Ready -\r\n") > >> + /* Jump to C world */ > >> + ldr r2, =start_secondary > >> b launch > >> ENDPROC(init_secondary) > >> > >> @@ -578,19 +586,25 @@ setup_fixmap: > >> mov pc, lr > >> ENDPROC(setup_fixmap) > >> > >> +/* > >> + * Setup the initial stack and jump to the C world > >> + * > >> + * Inputs: > >> + * r0 : Argument 0 of the C function to call > >> + * r1 : Argument 1 of the C function to call > >> + * r2 : C entry point > >> + * > >> + * Clobbers r3 > >> + */ > >> launch: > >> - PRINT("- Ready -\r\n") > >> - > >> - ldr r0, =init_data > >> - add r0, #INITINFO_stack /* Find the boot-time stack */ > >> - ldr sp, [r0] > >> + ldr r3, =init_data > >> + add r3, #INITINFO_stack /* Find the boot-time stack */ > >> + ldr sp, [r3] > >> add sp, #STACK_SIZE /* (which grows down from the top). */ > >> sub sp, #CPUINFO_sizeof /* Make room for CPU save record */ > >> - teq r12, #0 > >> - moveq r0, r10 /* Marshal args: - phys_offset */ > >> - moveq r1, r8 /* - DTB address */ > >> - beq start_xen /* and disappear into the land of C */ > >> - b start_secondary /* (to the appropriate entry point) */ > >> + > >> + /* Jump to C world */ > >> + bx r2 > > > > Why bx? > The only two other possible instructions would be: > 1) blx r2: we don't need to save the return address > 2) mov pc, r2: The Arm Arm recommends to use bx/blx instead of this. > > So bx seems the best fit. Any other suggestion? > > Also, I would probably replace all the "mov pc, lr" I added with "bx lr". There is really no alternative to bx. I forgot that "b" doesn't support a register as an operand. Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index e0f8c2e0cb..6d55a2119a 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -170,6 +170,11 @@ primary_switched: /* Use a virtual address to access the UART. */ mov_w r11, EARLY_UART_VIRTUAL_ADDRESS #endif + PRINT("- Ready -\r\n") + /* Setup the arguments for start_xen and jump to C world */ + mov r0, r10 /* r0 := Physical offset */ + mov r1, r8 /* r1 := paddr(FDT) */ + ldr r2, =start_xen b launch ENDPROC(start) @@ -234,6 +239,9 @@ secondary_switched: /* Use a virtual address to access the UART. */ mov_w r11, EARLY_UART_VIRTUAL_ADDRESS #endif + PRINT("- Ready -\r\n") + /* Jump to C world */ + ldr r2, =start_secondary b launch ENDPROC(init_secondary) @@ -578,19 +586,25 @@ setup_fixmap: mov pc, lr ENDPROC(setup_fixmap) +/* + * Setup the initial stack and jump to the C world + * + * Inputs: + * r0 : Argument 0 of the C function to call + * r1 : Argument 1 of the C function to call + * r2 : C entry point + * + * Clobbers r3 + */ launch: - PRINT("- Ready -\r\n") - - ldr r0, =init_data - add r0, #INITINFO_stack /* Find the boot-time stack */ - ldr sp, [r0] + ldr r3, =init_data + add r3, #INITINFO_stack /* Find the boot-time stack */ + ldr sp, [r3] add sp, #STACK_SIZE /* (which grows down from the top). */ sub sp, #CPUINFO_sizeof /* Make room for CPU save record */ - teq r12, #0 - moveq r0, r10 /* Marshal args: - phys_offset */ - moveq r1, r8 /* - DTB address */ - beq start_xen /* and disappear into the land of C */ - b start_secondary /* (to the appropriate entry point) */ + + /* Jump to C world */ + bx r2 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 using conditional instruction and make the call 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: - Patch added --- xen/arch/arm/arm32/head.S | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-)