[v7,0/9] Parallel CPU bringup for x86_64

  Tested on v7, doing INIT/SIPI/SIPI in parallel brings down the time for
smpboot from ~700ms to 100ms (85% improvement) on a server with 128 CPUs
split across 2 NUMA nodes.

The major change over v6 is keeping parallel smp support enabled in AMD.
APIC ID for parallel CPU bringup is now obtained from CPUID leaf 0x0B
(for x2APIC mode) otherwise CPUID leaf 0x1 (8 bits).

The patch for reusing timer calibration for secondary CPUs is also removed
from the series as its not part of parallel smp bringup and needs to be
further thought about.

Thanks,
Usama

Changes across versions:
v2: Cut it back to just INIT/SIPI/SIPI in parallel for now, nothing more
v3: Clean up x2apic patch, add MTRR optimisation, lock topology update
    in preparation for more parallelisation.
v4: Fixes to the real mode parallelisation patch spotted by SeanC, to
    avoid scribbling on initial_gs in common_cpu_up(), and to allow all
    24 bits of the physical X2APIC ID to be used. That patch still needs
    a Signed-off-by from its original author, who once claimed not to
    remember writing it at all. But now we've fixed it, hopefully he'll
    admit it now :)
v5: rebase to v6.1 and remeasure performance, disable parallel bringup
    for AMD CPUs.
v6: rebase to v6.2-rc6, disabled parallel boot on amd as a cpu bug and
    reused timer calibration for secondary CPUs.
v7: [David Woodhouse] iterate over all possible CPUs to find any existing
    cluster mask in alloc_clustermask. (patch 1/9)
    Keep parallel AMD support enabled in AMD, using APIC ID in CPUID leaf
    0x0B (for x2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
    Included sanity checks for APIC id from 0x0B. (patch 6/9)
    Removed patch for reusing timer calibration for secondary CPUs.
    commit message and code improvements.

David Woodhouse (8):
  x86/apic/x2apic: Allow CPU cluster_mask to be populated in parallel
  cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>
  cpu/hotplug: Add dynamic parallel bringup states before
    CPUHP_BRINGUP_CPU
  x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()
  x86/smpboot: Split up native_cpu_up into separate phases and document
    them
  x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel
  x86/mtrr: Avoid repeated save of MTRRs on boot-time CPU bringup
  x86/smpboot: Serialize topology updates for secondary bringup

Thomas Gleixner (1):
  x86/smpboot: Support parallel startup of secondary CPUs

 arch/x86/include/asm/realmode.h       |   3 +
 arch/x86/include/asm/smp.h            |  14 +-
 arch/x86/include/asm/topology.h       |   2 -
 arch/x86/kernel/acpi/sleep.c          |   1 +
 arch/x86/kernel/apic/apic.c           |   2 +-
 arch/x86/kernel/apic/x2apic_cluster.c | 130 ++++++----
 arch/x86/kernel/cpu/common.c          |   6 +-
 arch/x86/kernel/cpu/mtrr/mtrr.c       |   9 +
 arch/x86/kernel/head_64.S             |  84 +++++++
 arch/x86/kernel/smpboot.c             | 349 +++++++++++++++++++-------
 arch/x86/realmode/init.c              |   3 +
 arch/x86/realmode/rm/trampoline_64.S  |  14 ++
 arch/x86/xen/smp_pv.c                 |   4 +-
 include/linux/cpuhotplug.h            |   2 +
 include/linux/smpboot.h               |   7 +
 kernel/cpu.c                          |  31 ++-
 kernel/smpboot.c                      |   2 +-
 kernel/smpboot.h                      |   2 -
 18 files changed, 506 insertions(+), 159 deletions(-)
  

On Tue, Feb 07, 2023 at 11:04:27PM +0000, Usama Arif wrote:
> Tested on v7, doing INIT/SIPI/SIPI in parallel brings down the time for
> smpboot from ~700ms to 100ms (85% improvement) on a server with 128 CPUs
> split across 2 NUMA nodes.
> 
> The major change over v6 is keeping parallel smp support enabled in AMD.
> APIC ID for parallel CPU bringup is now obtained from CPUID leaf 0x0B
> (for x2APIC mode) otherwise CPUID leaf 0x1 (8 bits).
> 
> The patch for reusing timer calibration for secondary CPUs is also removed
> from the series as its not part of parallel smp bringup and needs to be
> further thought about.

Running rcutorture on this got me the following NULL pointer dereference
on scenario TREE01:

------------------------------------------------------------------------

[   34.662066] smpboot: CPU 0 is now offline
[   34.674075] rcu: NOCB: Cannot CB-offload offline CPU 25
[   35.038003] rcu: De-offloading 5
[   35.112997] rcu: Offloading 12
[   35.716011] smpboot: Booting Node 0 Processor 0 APIC 0x0
[   35.762685] BUG: kernel NULL pointer dereference, address: 0000000000000001
[   35.764278] #PF: supervisor instruction fetch in kernel mode
[   35.765530] #PF: error_code(0x0010) - not-present page
[   35.766700] PGD 0 P4D 0
[   35.767278] Oops: 0010 [#1] PREEMPT SMP PTI
[   35.768223] CPU: 36 PID: 0 Comm: swapper/36 Not tainted 6.2.0-rc1-00206-g18a37610b632-dirty #3563
[   35.770201] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014

------------------------------------------------------------------------

Given an x86 system with KVM and qemu, this can be reproduced by running
the following from the top-level directory in the Linux-kernel source
tree:

	tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --configs "TREE01 TINY01" --trust-make

Out of 15 runs, 14 blew up just after the first attempt to bring CPU
0 back online.  The 15th run blew up just after the second attempt to
bring CPU 0 online, the first attempt having succeeded.

My guess is that the CONFIG_BOOTPARAM_HOTPLUG_CPU0=y Kconfig option is
tickling this bug.  This Kconfig option has been added to the TREE01
scenario in the -rcu tree's "dev" branch, which might mean that this test
would pass on mainline.  But CONFIG_BOOTPARAM_HOTPLUG_CPU0=y is not new,
only rcutorture's testing of it.

Thoughts?

							Thanx, Paul
  

On 09/02/2023 03:53, Paul E. McKenney wrote:
> On Tue, Feb 07, 2023 at 11:04:27PM +0000, Usama Arif wrote:
>> Tested on v7, doing INIT/SIPI/SIPI in parallel brings down the time for
>> smpboot from ~700ms to 100ms (85% improvement) on a server with 128 CPUs
>> split across 2 NUMA nodes.
>>
>> The major change over v6 is keeping parallel smp support enabled in AMD.
>> APIC ID for parallel CPU bringup is now obtained from CPUID leaf 0x0B
>> (for x2APIC mode) otherwise CPUID leaf 0x1 (8 bits).
>>
>> The patch for reusing timer calibration for secondary CPUs is also removed
>> from the series as its not part of parallel smp bringup and needs to be
>> further thought about.
> 
> Running rcutorture on this got me the following NULL pointer dereference
> on scenario TREE01:
> 
> ------------------------------------------------------------------------
> 
> [   34.662066] smpboot: CPU 0 is now offline
> [   34.674075] rcu: NOCB: Cannot CB-offload offline CPU 25
> [   35.038003] rcu: De-offloading 5
> [   35.112997] rcu: Offloading 12
> [   35.716011] smpboot: Booting Node 0 Processor 0 APIC 0x0
> [   35.762685] BUG: kernel NULL pointer dereference, address: 0000000000000001
> [   35.764278] #PF: supervisor instruction fetch in kernel mode
> [   35.765530] #PF: error_code(0x0010) - not-present page
> [   35.766700] PGD 0 P4D 0
> [   35.767278] Oops: 0010 [#1] PREEMPT SMP PTI
> [   35.768223] CPU: 36 PID: 0 Comm: swapper/36 Not tainted 6.2.0-rc1-00206-g18a37610b632-dirty #3563
> [   35.770201] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> 
> ------------------------------------------------------------------------
> 
> Given an x86 system with KVM and qemu, this can be reproduced by running
> the following from the top-level directory in the Linux-kernel source
> tree:
> 
> 	tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --configs "TREE01 TINY01" --trust-make
> 
> Out of 15 runs, 14 blew up just after the first attempt to bring CPU
> 0 back online.  The 15th run blew up just after the second attempt to
> bring CPU 0 online, the first attempt having succeeded.
> 
> My guess is that the CONFIG_BOOTPARAM_HOTPLUG_CPU0=y Kconfig option is
> tickling this bug.  This Kconfig option has been added to the TREE01
> scenario in the -rcu tree's "dev" branch, which might mean that this test
> would pass on mainline.  But CONFIG_BOOTPARAM_HOTPLUG_CPU0=y is not new,
> only rcutorture's testing of it.
> 
> Thoughts?
> 
> 							Thanx, Paul

It looks like its because of the initial_gs, initial_stack and
early_gdt_descr not being setup properly for CPU0 hotplug, i.e. 
init_cpu_data isnt called in cpu0 hotplug case.

Its easy to test, just by doing
echo 0 > /sys/devices/system/cpu/cpu0/online;
echo 1 > /sys/devices/system/cpu/cpu0/online;

As a quick check, if we do something like below (probably there is a 
much better place to set these..), the above hotplug commands will work.

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 3ec5182d9698..184135c47ee5 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1080,6 +1080,7 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long 
start_ip, int apicid,
                                           wakeup_cpu0_nmi, 0, "wake_cpu0");

         if (!boot_error) {
+               initial_gs = per_cpu_offset(cpu);
                 enable_start_cpu0 = 1;
                 *cpu0_nmi_registered = 1;
                 id = apic->dest_mode_logical ? cpu0_logical_apicid : 
apicid;
@@ -1188,10 +1189,14 @@ static int do_boot_cpu(int apicid, int cpu, 
struct task_struct *idle,
                 boot_error = apic->wakeup_secondary_cpu_64(apicid, 
start_ip);
         else if (apic->wakeup_secondary_cpu)
                 boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
-       else
+       else {
+               if(!cpu) {
+                       early_gdt_descr.address = (unsigned 
long)get_cpu_gdt_rw(cpu);
+                       initial_stack  = idle->thread.sp;
+               }
                 boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
                                                      cpu0_nmi_registered);
-
+       }
         return boot_error;
  }
  

On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
> 
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1080,6 +1080,7 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long 
> start_ip, int apicid,
>                                            wakeup_cpu0_nmi, 0, "wake_cpu0");
> 
>          if (!boot_error) {
> +               initial_gs = per_cpu_offset(cpu);

That's for 64-bit.

>                  enable_start_cpu0 = 1;
>                  *cpu0_nmi_registered = 1;
>                  id = apic->dest_mode_logical ? cpu0_logical_apicid :
> apicid;
> @@ -1188,10 +1189,14 @@ static int do_boot_cpu(int apicid, int cpu, 
> struct task_struct *idle,
>                  boot_error = apic->wakeup_secondary_cpu_64(apicid, 
> start_ip);
>          else if (apic->wakeup_secondary_cpu)
>                  boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
> -       else
> +       else {
> +               if(!cpu) {
> +                       early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
> +                       initial_stack  = idle->thread.sp;
> +               }

And that's 32-bit.

These were previously done in common_cpu_up() or do_boot_cpu(), which
means they were done not only for the wakeup_cpu_via_init_nmi() code
path, but also when we call apic->wakeup_secondary_cpu() (or _64()) for
the esoteric UV/NumaConnect machines.

Thanks for diagnosing it so quickly; I'll work up a subtly different
fix.

>                  boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
>                                                       cpu0_nmi_registered);
> -
> +       }
>          return boot_error;
>   }
>
  

On 09/02/2023 10:06, David Woodhouse wrote:
> On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
>>
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1080,6 +1080,7 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long
>> start_ip, int apicid,
>>                                             wakeup_cpu0_nmi, 0, "wake_cpu0");
>>
>>           if (!boot_error) {
>> +               initial_gs = per_cpu_offset(cpu);
> 
> That's for 64-bit.
> 
>>                   enable_start_cpu0 = 1;
>>                   *cpu0_nmi_registered = 1;
>>                   id = apic->dest_mode_logical ? cpu0_logical_apicid :
>> apicid;
>> @@ -1188,10 +1189,14 @@ static int do_boot_cpu(int apicid, int cpu,
>> struct task_struct *idle,
>>                   boot_error = apic->wakeup_secondary_cpu_64(apicid,
>> start_ip);
>>           else if (apic->wakeup_secondary_cpu)
>>                   boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
>> -       else
>> +       else {
>> +               if(!cpu) {
>> +                       early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
>> +                       initial_stack  = idle->thread.sp;
>> +               }
> 
> And that's 32-bit.
> 
> These were previously done in common_cpu_up() or do_boot_cpu(), which
> means they were done not only for the wakeup_cpu_via_init_nmi() code
> path, but also when we call apic->wakeup_secondary_cpu() (or _64()) for
> the esoteric UV/NumaConnect machines.
> 
> Thanks for diagnosing it so quickly; I'll work up a subtly different
> fix.
> 

Yeah, was just a hacky fix while I was trying to figure out the issue.

do_boot_cpu is only called in cpu0 in hotplug case, so I think this a 
much better diff:

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 3ec5182d9698..f7ae82969ee4 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1136,13 +1136,16 @@ static int do_boot_cpu(int apicid, int cpu, 
struct task_struct *idle,
         idle->thread.sp = (unsigned long)task_pt_regs(idle);
         initial_code = (unsigned long)start_secondary;

-       if (IS_ENABLED(CONFIG_X86_32)) {
+       if (IS_ENABLED(CONFIG_X86_32) || !cpu) {
                 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;
         }

+       if (!cpu)
+               initial_gs = per_cpu_offset(cpu);
+
         /* Enable the espfix hack for this CPU */
         init_espfix_ap(cpu);



>>                   boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
>>                                                        cpu0_nmi_registered);
>> -
>> +       }
>>           return boot_error;
>>    }
>>
>
  

On Thu, 2023-02-09 at 10:19 +0000, Usama Arif wrote:
> 
> 
> On 09/02/2023 10:06, David Woodhouse wrote:
> > On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
> > > 
> > > --- a/arch/x86/kernel/smpboot.c
> > > +++ b/arch/x86/kernel/smpboot.c
> > > @@ -1080,6 +1080,7 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long
> > > start_ip, int apicid,
> > >                                             wakeup_cpu0_nmi, 0, "wake_cpu0");
> > > 
> > >           if (!boot_error) {
> > > +               initial_gs = per_cpu_offset(cpu);
> > 
> > That's for 64-bit.
> > 
> > >                   enable_start_cpu0 = 1;
> > >                   *cpu0_nmi_registered = 1;
> > >                   id = apic->dest_mode_logical ? cpu0_logical_apicid :
> > > apicid;
> > > @@ -1188,10 +1189,14 @@ static int do_boot_cpu(int apicid, int cpu,
> > > struct task_struct *idle,
> > >                   boot_error = apic->wakeup_secondary_cpu_64(apicid,
> > > start_ip);
> > >           else if (apic->wakeup_secondary_cpu)
> > >                   boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
> > > -       else
> > > +       else {
> > > +               if(!cpu) {
> > > +                       early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
> > > +                       initial_stack  = idle->thread.sp;
> > > +               }
> > 
> > And that's 32-bit.
> > 
> > These were previously done in common_cpu_up() or do_boot_cpu(), which
> > means they were done not only for the wakeup_cpu_via_init_nmi() code
> > path, but also when we call apic->wakeup_secondary_cpu() (or _64()) for
> > the esoteric UV/NumaConnect machines.
> > 
> > Thanks for diagnosing it so quickly; I'll work up a subtly different
> > fix.
> > 
> 
> Yeah, was just a hacky fix while I was trying to figure out the issue.
> 
> do_boot_cpu is only called in cpu0 in hotplug case, so I think this a
> much better diff:
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 3ec5182d9698..f7ae82969ee4 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1136,13 +1136,16 @@ static int do_boot_cpu(int apicid, int cpu, 
> struct task_struct *idle,
>          idle->thread.sp = (unsigned long)task_pt_regs(idle);
>          initial_code = (unsigned long)start_secondary;
> 
> -       if (IS_ENABLED(CONFIG_X86_32)) {
> +       if (IS_ENABLED(CONFIG_X86_32) || !cpu) {
>                  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;
>          }
> 
> +       if (!cpu)
> +               initial_gs = per_cpu_offset(cpu);
> +
>          /* Enable the espfix hack for this CPU */
>          init_espfix_ap(cpu);
> 
> 
> 
> > >                   boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
> > >                                                        cpu0_nmi_registered);
> > > -
> > > +       }
> > >           return boot_error;
> > >    }
> > > 
> > 


I'm trying to find the actual startup path that CPU0 takes when woken
by an NMI.

Can't we make it take the secondary_startup_64 path that actually
checks smpboot_control, sees the STARTUP_SECONDARY flag set, and then
gets all these things from the per-cpu data as $DEITY (well, Thomas)
intended?
  

On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
> 
> Its easy to test, just by doing
> echo 0 > /sys/devices/system/cpu/cpu0/online;
> echo 1 > /sys/devices/system/cpu/cpu0/online;

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 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.


diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5462464fe3ef..ea6052a97619 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -450,7 +450,16 @@ 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, %rcx
+       addq    %rbx, %rcx
+       movq    (%rcx), %rcx
+       movq    TASK_threadsp(%rcx), %rsp
+
        jmp     .Ljump_to_C_code
 SYM_CODE_END(start_cpu0)
 #endif

I cut and pasted some of that, I'm not entirely sure why we have three
instructions to do the equivalent of 'movq idle_threads(%ebx), %ecx'
and may fix that in the original as I work this in.
  

On 09/02/2023 11:03, David Woodhouse wrote:
> On Thu, 2023-02-09 at 09:49 +0000, Usama Arif wrote:
>>
>> Its easy to test, just by doing
>> echo 0 > /sys/devices/system/cpu/cpu0/online;
>> echo 1 > /sys/devices/system/cpu/cpu0/online;
> 
> 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.)
> 

Yes, I think its better!

> 
> 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.
> 
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 5462464fe3ef..ea6052a97619 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -450,7 +450,16 @@ 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, %rcx
> +       addq    %rbx, %rcx
> +       movq    (%rcx), %rcx
> +       movq    TASK_threadsp(%rcx), %rsp
> +
>          jmp     .Ljump_to_C_code
>   SYM_CODE_END(start_cpu0)
>   #endif
> 
> I cut and pasted some of that, I'm not entirely sure why we have three
> instructions to do the equivalent of 'movq idle_threads(%ebx), %ecx'
> and may fix that in the original as I work this in.
  

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@infradead.org>

Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: David Woodhouse <dwmw2@infradead.org>

> 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. :)

Thanks

        tglx
  

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@infradead.org>
> 
> Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>

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
  

On Thu, 2023-02-09 at 12:10 +0000, David Woodhouse wrote:
> 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@infradead.org>
> > 
> > Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> 
> 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.

All done and pushed out to parallel-6.2-rc7-part1 (and not -part1)
branches. Usama, are you able to redo the testing and take it from
here? Thanks for that; it's saving me a lot of time!


I'm mostly done for the week now as by this time tomorrow, I need to
have the skis on the roof of the car and be ready to pick the family up
from school and start driving south...
  

On 09/02/2023 13:48, David Woodhouse wrote:
> On Thu, 2023-02-09 at 12:10 +0000, David Woodhouse wrote:
>> 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@infradead.org>
>>>
>>> Co-developed-by: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
>>
>> 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.
> 
> All done and pushed out to parallel-6.2-rc7-part1 (and not -part1)
> branches. Usama, are you able to redo the testing and take it from
> here? Thanks for that; it's saving me a lot of time!
> 
Thanks! Will retest and repost now!

Usama

> 
> I'm mostly done for the week now as by this time tomorrow, I need to
> have the skis on the roof of the car and be ready to pick the family up
> from school and start driving south...
  

On Thu, Feb 09 2023 at 12:10, David Woodhouse wrote:
> On Thu, 2023-02-09 at 12:53 +0100, Thomas Gleixner wrote:
>> > 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 :)

I can't even remember, but probably diff -up was way faster than writing
a novel :)

Thanks,

        tglx