linux-kernel - Re: [PATCH v12 06/11] x86/smpboot: Remove initial

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2776fc04271e5d3697918ce36e7e2893e2a7bc21.camel@infradead.org>
Date:   Tue, 28 Feb 2023 16:25:22 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Usama Arif <usama.arif@...edance.com>, kim.phillips@....com,
        brgerst@...il.com
Cc:     piotrgorski@...hyos.org, oleksandr@...alenko.name,
        arjan@...ux.intel.com, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, x86@...nel.org,
        pbonzini@...hat.com, paulmck@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        rcu@...r.kernel.org, mimoja@...oja.de, hewenliang4@...wei.com,
        thomas.lendacky@....com, seanjc@...gle.com, pmenzel@...gen.mpg.de,
        fam.zheng@...edance.com, punit.agrawal@...edance.com,
        simon.evans@...edance.com, liangma@...ngbit.com
Subject: Re: [PATCH v12 06/11] x86/smpboot: Remove initial_stack on 64-bit

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@...il.com>
> > 
> > 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.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)