[prev in list] [next in list] [prev in thread] [next in thread] 

List:       xen-devel
Subject:    Re: [Xen-devel] [PATCH v2 24/35] xen/arm32: head: Introduce distinct paths for the boot CPU and seco
From:       Julien Grall <julien.grall () arm ! com>
Date:       2019-07-31 20:31:51
Message-ID: 53b889de-0952-edd8-9dd5-02d8a5c72ad5 () arm ! com
[Download RAW message or body]

Hi Stefano,

On 7/30/19 9:07 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 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 distinct and for now will call each
>> functions.
>>
>> Follow-ups will remove unnecessary 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.
>>
>> Lastly, it is now required to save lr in cpu_init() becauswe the
>> function will call other functions and therefore clobber lr.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm32/head.S | 64 +++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 53 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index bbcfdcd351..13793e85d8 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -148,7 +148,19 @@ past_zImage:
>>   
>>           mov   r12, #0                /* r12 := 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 */
> 
> The arm64 patch has been switched to use "1:1", it would be good to be
> consistent. In the commit message too.
> 
> 
>> +        ldr   r0, =primary_switched
>> +        mov   pc, r0
>> +primary_switched:
>> +        bl    setup_fixmap
>> +        b     launch
>> +ENDPROC(start)
>>   
>>   GLOBAL(init_secondary)
>>           cpsid aif                    /* Disable all interrupts */
>> @@ -179,8 +191,21 @@ GLOBAL(init_secondary)
>>           print_reg r7
>>           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. */
> 
> Same here.
> 
> 
>> +        ldr   r0, =secondary_switched
>> +        mov   pc, r0
>> +secondary_switched:
>> +        bl    setup_fixmap
>> +        b     launch
>> +ENDPROC(init_secondary)
>> +
>> +check_cpu_mode:
>>           /* Check that this CPU has Hyp mode */
>>           mrc   CP32(r0, ID_PFR1)
>>           and   r0, r0, #0xf000        /* Bits 12-15 define virt extensions */
>> @@ -202,7 +227,10 @@ common_start:
>>           b     fail
>>   
>>   hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
>> +        mov   pc, lr
>> +ENDPROC(check_cpu_mode)
>>   
>> +zero_bss:
>>           /* Zero BSS On the boot CPU to avoid nasty surprises */
>>           teq   r12, #0
>>           bne   skip_bss
>> @@ -219,8 +247,14 @@ hyp:    PRINT("- Xen starting in Hyp mode -\r\n")
>>           blo   1b
>>   
>>   skip_bss:
>> +        mov   pc, lr
>> +ENDPROC(zero_bss)
>> +
>> +cpu_init:
>>           PRINT("- Setting up control registers -\r\n")
>>   
>> +        mov   r5, lr			            /* r5 := return address */
> 
> Please align the comment with the others in this proc.

It looks like the line was containing some hard tab. I have replaced 
them with soft tab.

> 
> Other than these minor comments the patch looks fine. Have you had a
> chance to test it on real hardware?

I actually didn't test the SMP path on the model until I sent it because 
I had some issues getting secondary CPU up. I wanted to get some 
feedback first. Hence the "lightly tested" in the cover letter :).

This series fully boot on the model and I still need to test on real 
hardware. I have a TC2 (Cortex-A15 + Cortex-A7) on my desk, so I will 
give it a try there first.

Anyway, I probably need to resend this patch to replace all the "mov pc, 
lr" with "bx lr"

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[prev in list] [next in list] [prev in thread] [next in thread]