On Tue, 2023-02-28 at 17:13 +0100, Thomas Gleixner wrote:
> On Sun, Feb 26 2023 at 11:07, Usama Arif wrote:
> > From: Brian Gerst <brgerst@xxxxxxxxx>
> >
> > Eliminating global variables from the CPU startup path in order to simplify
> > it and facilitate parallel startup.
>
> As this patch is now part of the parallel boot series and actually
> introduces smpboot_control, the above is neither accurate nor useful.
I neglected to mention adding smpboot_control, but the above *is*
accurate. This patch, and the next two, are eliminating global
variables to simplify the startup path and make it possible to run it
in parallel.
Now it's slightly harder to phrase that without the Verboteneworte
'this patch', I'll grant you. But it's trying to explain *why* we're
eliminating those global variables.
I'll try again.
> Folks, really.
Also, while those two lines *happen* to be my addition to Brian's
commit message, I don't know if you knew that. Speak to me how you
like; you know I'll still love you. But be nicer to Brian and Usama.
> > Remove initial_stack, and load RSP from current_task->thread.sp instead.
>
>
> > #ifdef CONFIG_SMP
> > - initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
> > + current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
>
> This lacks a comment about the temporary (ab)use of current->thread.sp
Ack.
> > early_gdt_descr.address =
> > (unsigned long)get_cpu_gdt_rw(smp_processor_id());
> > initial_gs = per_cpu_offset(smp_processor_id());
> > + smpboot_control = smp_processor_id();
> > #endif
>
> > @@ -241,6 +241,24 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> > UNWIND_HINT_EMPTY
> > ANNOTATE_NOENDBR // above
> >
> > +#ifdef CONFIG_SMP
> > + movl smpboot_control(%rip), %ecx
> > +
> > + /* Get the per cpu offset for the given CPU# which is in ECX */
> > + movq __per_cpu_offset(,%rcx,8), %rdx
> > +#else
> > + xorl %edx, %edx
> > +#endif /* CONFIG_SMP */
>
> Sigh, we should finally make CONFIG_SMP def_bool y ...
Not today :)
> > + /*
> > + * Setup a boot time stack - Any secondary CPU will have lost its stack
> > + * by now because the cr3-switch above unmaps the real-mode stack.
> > + *
> > + * RDX contains the per-cpu offset
> > + */
> > + movq pcpu_hot + X86_current_task(%rdx), %rax
> > + movq TASK_threadsp(%rax), %rsp
> > +
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index b18c1385e181..62e3bf37f0b8 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -1112,7 +1112,12 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
> > idle->thread.sp = (unsigned long)task_pt_regs(idle);
> > early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
> > initial_code = (unsigned long)start_secondary;
> > - initial_stack = idle->thread.sp;
> > +
> > + if (IS_ENABLED(CONFIG_X86_32)) {
> > + initial_stack = idle->thread.sp;
> > + } else {
> > + smpboot_control = cpu;
> > + }
>
> Please remove the pointless brackets.
I pondered that, but they only get added back again in the next patch.
It just seemed like adding pointless churn.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
