Re: [External] Re: [PATCH v7 0/9] Parallel CPU bringup for x86_64

[David Woodhouse]

On Thu, 2023-02-09 at 12:53 +0100, Thomas Gleixner wrote:
> On Thu, Feb 09 2023 at 11:03, David Woodhouse wrote:
> > This one also fixes it for me. If we're happy with this approach, I'll
> > work it into Thomas's original patch (and hopefully eventually he'll be
> > happy enough with it and the commit message that he'll give us his
> > Signed-off-by for it.)
> 
> I'm happy enough by now, but I'm not sure how much of the original patch
> is still left. Also you did the heavy lifting of making it work and
> writing the nice changelog. So please make this:
> 
> From: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> 
> Co-developed-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: David Woodhouse <dwmw2@xxxxxxxxxxxxx>

Thanks. I'll flip that to the Amazon address, of course. It's useless
for actual email (until I apply that LART some more) but I should still
use it for that.

I think I'm going to make one more change to that as I review it; in
the "should never happen" case of not finding the APIC ID in the
cpuid_to_apicid[] array it would just keep searching for ever. I don't
know if there's a better thing to do other than just dropping the
trampoline lock and 1:cli;hlt;jmp 1b but at least it's *attempting* to
handle the failure.

Patch below, and I'll update the tree shortly. There's a "what if
there's noise in the top 32 bits of %rcx" fix in there too.

> > I could probably add a Co-developed-by: tglx for that first x2apic
> > patch in the series too, but then it would *also* need his SoB and I
> > didn't want to be owed two, so I just pasted his suggested code and
> > didn't credit him.
> 
> That's what Suggested-by: is for. For that I don't owe you anything. :)

Well, I didn't think that was really for suggestions which come in
'diff -up' form, but OK :)

--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -280,15 +280,25 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
        cpuid
 
 .Lsetup_AP:
-       /* EDX contains the APICID of the current CPU */
-       xorl    %ecx, %ecx
+       /* EDX contains the APIC ID of the current CPU */
+       xorq    %rcx, %rcx
        leaq    cpuid_to_apicid(%rip), %rbx
 
 .Lfind_cpunr:
        cmpl    (%rbx,%rcx,4), %edx
        jz      .Linit_cpu_data
-       inc     %rcx
-       jmp     .Lfind_cpunr
+       inc     %ecx
+       cmpl    nr_cpu_ids(%rip), %ecx
+       jb      .Lfind_cpunr
+
+       /*  APIC ID not found in the table. Drop the trampoline lock and bail. */
+       movq    trampoline_lock(%rip), %rax
+       lock
+       btrl    $0, (%rax)
+
+1:     cli
+       hlt
+       jmp     1b
 
 .Linit_cpu_data:
        /* Get the per cpu offset for the given CPU# which is in ECX */
@@ -303,9 +313,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
        movq    %rcx, early_gdt_descr_base(%rip)
 
        /* Find the idle task stack */
-       movq    $idle_threads, %rcx
-       addq    %rbx, %rcx
-       movq    (%rcx), %rcx
+       movq    idle_threads(%rbx), %rcx
        movq    TASK_threadsp(%rcx), %rcx
        movq    %rcx, initial_stack(%rip)
 #endif /* CONFIG_SMP */
@@ -450,7 +458,14 @@ SYM_CODE_END(secondary_startup_64)
 SYM_CODE_START(start_cpu0)
        ANNOTATE_NOENDBR
        UNWIND_HINT_EMPTY
-       movq    initial_stack(%rip), %rsp
+       /* Load the per-cpu base for CPU#0 */
+       leaq    __per_cpu_offset(%rip), %rbx
+       movq    (%rbx), %rbx
+
+       /* Find the idle task stack */
+       movq    idle_threads(%rbx), %rcx
+       movq    TASK_threadsp(%rcx), %rsp
+
        jmp     .Ljump_to_C_code
 SYM_CODE_END(start_cpu0)
 #endif

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