Re: [PATCH v7 6/9] x86/smpboot: Support parallel startup of secondary CPUs

[David Woodhouse]

On Wed, 2023-02-08 at 00:09 -0500, Brian Gerst wrote:
> On Tue, Feb 7, 2023 at 6:10 PM Usama Arif <usama.arif@xxxxxxxxxxxxx> wrote:
> > 
> > From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > 
> > Rework the real-mode startup code to allow for APs to be brought up in
> > parallel. This is in two parts:
> > 
> > 1. Introduce a bit-spinlock to prevent them from all using the real
> >    mode stack at the same time.
> > 
> > 2. Avoid the use of global variables for passing per-CPU information to
> >    the APs.
> > 
> > To achieve the latter, export the cpuid_to_apicid[] array so that each
> > AP can find its own per_cpu data (and thus initial_gs, initial_stack and
> > early_gdt_descr) by searching therein based on its APIC ID.
> > 
> > Introduce a global variable 'smpboot_control' indicating to the AP how
> > it should find its APIC ID. For a serialized bringup, the APIC ID is
> > explicitly passed in the low bits of smpboot_control, while for parallel
> > mode there are flags directing the AP to find its APIC ID in CPUID leaf
> > 0x0b (for X2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
> 
> For the serialized bringup case, it would be simpler to just put the
> cpu number in the lower bits instead of the APIC id, skipping the
> lookup.
> 

I seem to recall that was one of my first thoughts on seeing this
patch.

I quite liked the fact that the code path in head_64.S remained
basically the same for all cases, and was actually testing the lookup
via cpuid_to_apicid[] even when parallel boot isn't available.

But then again, we've tested that part fairly well now, so perhaps
you're right and we could do something like the below? It doesn't
actually add much more complexity; just define the entry conditions to
.Linit_cpu_data a bit more clearly and remember how to use scaled index
addressing so we don't have to explicitly multiply by 8.

I don't know that I care very much either way. It's Thomas's patch
originally, so I'll let him see if he can muster up an opinion.

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 656e6018b9d4..d878869472a2 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -252,20 +252,23 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	jz	.Lsetup_cpu
 
 	/*
-	 * Secondary CPUs find out the offsets via the APIC ID. For parallel
-	 * boot the APIC ID is retrieved from CPUID, otherwise it's encoded
-	 * in smpboot_control:
+	 * Secondary CPUs find out the offsets via their per_cpu data. For
+	 * parallel bringup they find it via the cpuid_to_apicid[] array
+	 * with their APIC ID obtained from CPUID. Otherwise the CPU number
+	 * is encoded directly in smpboot_control:
+	 *
 	 * Bit 31	STARTUP_SECONDARY flag (checked above)
 	 * Bit 30	STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
 	 * Bit 29	STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
-	 * Bit 0-24	APIC ID if STARTUP_APICID_CPUID_xx flags are not set
+	 * Bit 0-24	CPU# if STARTUP_APICID_CPUID_xx flags are not set
 	 */
 	testl	$STARTUP_APICID_CPUID_0B, %edx
 	jnz	.Luse_cpuid_0b
 	testl	$STARTUP_APICID_CPUID_01, %edx
 	jnz	.Luse_cpuid_01
 	andl	$0x0FFFFFFF, %edx
-	jmp	.Lsetup_AP
+	mov	%edx, %ecx
+	jmp	.Linit_cpu_data
 
 .Luse_cpuid_01:
 	mov	$0x01, %eax
@@ -288,14 +291,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	cmpl	(%rbx), %edx
 	jz	.Linit_cpu_data
 	addq	$4, %rbx
-	addq	$8, %rcx
+	inc	%rcx
 	jmp	.Lfind_cpunr
 
 .Linit_cpu_data:
+	/* ECX contains the CPU# of the current CPU */
 	/* Get the per cpu offset */
 	leaq	__per_cpu_offset(%rip), %rbx
-	addq	%rcx, %rbx
-	movq	(%rbx), %rbx
+	movq	(%rbx,%rcx,8), %rbx
 	/* Save it for GS BASE setup */
 	movq	%rbx, initial_gs(%rip)
 
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index cae916040c55..5149676881e0 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1146,7 +1146,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 		early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
 		initial_stack  = idle->thread.sp;
 	} else if (!do_parallel_bringup) {
-		smpboot_control = STARTUP_SECONDARY | apicid;
+		smpboot_control = STARTUP_SECONDARY | cpu;
 	}
 
 	/* Enable the espfix hack for this CPU */

Attachment: smime.p7s
Description: S/MIME cryptographic signature