Re: [PATCH v15 09/12] x86/smpboot: Support parallel startup of secondary CPUs

[Brian Gerst]

On Tue, Mar 21, 2023 at 3:12 PM David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
>
> On Tue, 2023-03-21 at 14:28 -0400, Brian Gerst wrote:
> >
> > > @@ -264,6 +318,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> > >          lgdt    (%rsp)
> > >          addq    $16, %rsp
> > >
> > > +       /* Drop the realmode protection. For the boot CPU the pointer is NULL! */
> > > +       movq    trampoline_lock(%rip), %rax
> > > +       testq   %rax, %rax
> > > +       jz      .Lsetup_data_segments
> > > +       lock
> > > +       btrl    $0, (%rax)
> > > +
> > > +.Lsetup_data_segments:
> > >          /* set up data segments */
> > >          xorl %eax,%eax
> > >          movl %eax,%ds
> >
> > This can still go earlier, right after "movq TASK_threadsp(%rax),
> > %rsp".  The GDT descriptor is placed on the idle thread stack, so it's
> > safe to drop the lock before it.
>
>
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -335,6 +335,17 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
>         movq    pcpu_hot + X86_current_task(%rdx), %rax
>         movq    TASK_threadsp(%rax), %rsp
>
> +       /*
> +        * Now that this CPU is running on its own stack, drop the realmode
> +        * protection. For the boot CPU the pointer is NULL!
> +        */
> +       movq    trampoline_lock(%rip), %rax
> +       testq   %rax, %rax
> +       jz      .Lsetup_gdt
> +       lock
> +       btrl    $0, (%rax)
> +
> +.Lsetup_gdt:
>         /*
>          * We must switch to a new descriptor in kernel space for the GDT
>          * because soon the kernel won't have access anymore to the userspace
> @@ -377,14 +388,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
>         shrq    $32, %rdx
>         wrmsr
>
> -       /* Drop the realmode protection. For the boot CPU the pointer is NULL! */
> -       movq    trampoline_lock(%rip), %rax
> -       testq   %rax, %rax
> -       jz      .Lsetup_idt
> -       lock
> -       btrl    $0, (%rax)
> -
> -.Lsetup_idt:
>         /* Setup and Load IDT */
>         pushq   %rsi
>         call    early_setup_idt
>

Looks good, thanks.