[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 24/35] xen/arm32: head: Introduce distinct paths for the boot CPU and secondary CPUs



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

---
     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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.