Re: [Xen-devel] [PATCH v2 33/35] xen/arm32: head: Rework and document launch()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> >>
> >> ---
> >>      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@xxxxxxxxxx>

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