by Yuan Yao

[permalink] [raw]

Subject: Re: [PATCH v7 5/9] x86/smpboot: Split up native_cpu_up into separate phases and document them

On Tue, Feb 07, 2023 at 11:04:32PM +0000, Usama Arif wrote:
> From: David Woodhouse <[email protected]>
>
> There are four logical parts to what native_cpu_up() does on the BSP (or
> on the controlling CPU for a later hotplug):
>
> 1) Wake the AP by sending the INIT/SIPI/SIPI sequence.
>
> 2) Wait for the AP to make it as far as wait_for_master_cpu() which
> sets that CPU's bit in cpu_initialized_mask, then sets the bit in
> cpu_callout_mask to let the AP proceed through cpu_init().
>
> 3) Waits for the AP to finish cpu_init() and get as far as the
> smp_callin() call, which sets that CPU's bit in cpu_callin_mask.
>
> 4) Perform the TSC synchronization and wait for the AP to actually
> mark itself online in cpu_online_mask.
>
> In preparation to allow these phases to operate in parallel on multiple
> APs, split them out into separate functions and document the interactions
> a little more clearly in both the BSP and AP code paths.
>
> No functional change intended.
>
> [Usama Arif: fixed rebase conflict]
> Signed-off-by: David Woodhouse <[email protected]>
> Signed-off-by: Usama Arif <[email protected]>
> ---
> arch/x86/kernel/smpboot.c | 182 +++++++++++++++++++++++++++-----------
> 1 file changed, 128 insertions(+), 54 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 3a793772a2aa..eee717086ab7 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -204,6 +204,10 @@ static void smp_callin(void)
>
> wmb();
>
> + /*
> + * This runs the AP through all the cpuhp states to its target
> + * state (CPUHP_ONLINE in the case of serial bringup).
> + */
> notify_cpu_starting(cpuid);
>
> /*
> @@ -231,17 +235,33 @@ static void notrace start_secondary(void *unused)
> load_cr3(swapper_pg_dir);
> __flush_tlb_all();
> #endif
> + /*
> + * Sync point with do_wait_cpu_initialized(). On boot, all secondary
> + * CPUs reach this stage after receiving INIT/SIPI from do_cpu_up()
> + * in the x86/cpu:kick cpuhp stage. At the start of cpu_init() they
> + * will wait for do_wait_cpu_initialized() to set their bit in
> + * smp_callout_mask to release them.

The last sentence of the comment looks confused. The fact is:

For serial case, The BSP waits AP to set cpu_initialized_mask from
wait_for_master_cpu() after fired INIT/SIPI, then AP starts to wait
cpu_callout_mask set by BSP from do_boot_cpu().

Or the comments below "Bringup step two:..." which also looks clear
enough then above.

> + */
> cpu_init_secondary();
> rcu_cpu_starting(raw_smp_processor_id());
> x86_cpuinit.early_percpu_clock_init();
> +
> + /*
> + * Sync point with do_wait_cpu_callin(). The AP doesn't wait here
> + * but just sets the bit to let the controlling CPU (BSP) know that
> + * it's got this far.
> + */
> smp_callin();
>
> enable_start_cpu0 = 0;
>
> /* otherwise gcc will move up smp_processor_id before the cpu_init */
> barrier();
> +
> /*
> - * Check TSC synchronization with the boot CPU:
> + * Check TSC synchronization with the boot CPU (or whichever CPU
> + * is controlling the bringup). It will do its part of this from
> + * do_wait_cpu_online(), making it an implicit sync point.
> */
> check_tsc_sync_target();
>
> @@ -254,6 +274,7 @@ static void notrace start_secondary(void *unused)
> * half valid vector space.
> */
> lock_vector_lock();
> + /* Sync point with do_wait_cpu_online() */
> set_cpu_online(smp_processor_id(), true);
> lapic_online();
> unlock_vector_lock();
> @@ -1083,7 +1104,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
> unsigned long start_ip = real_mode_header->trampoline_start;
>
> unsigned long boot_error = 0;
> - unsigned long timeout;
>
> #ifdef CONFIG_X86_64
> /* If 64-bit wakeup method exists, use the 64-bit mode trampoline IP */
> @@ -1144,55 +1164,94 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
> boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
> cpu0_nmi_registered);
>
> - if (!boot_error) {
> - /*
> - * Wait 10s total for first sign of life from AP
> - */
> - boot_error = -1;
> - timeout = jiffies + 10*HZ;
> - while (time_before(jiffies, timeout)) {
> - if (cpumask_test_cpu(cpu, cpu_initialized_mask)) {
> - /*
> - * Tell AP to proceed with initialization
> - */
> - cpumask_set_cpu(cpu, cpu_callout_mask);
> - boot_error = 0;
> - break;
> - }
> - schedule();
> - }
> - }
> + return boot_error;
> +}
>
> - if (!boot_error) {
> - /*
> - * Wait till AP completes initial initialization
> - */
> - while (!cpumask_test_cpu(cpu, cpu_callin_mask)) {
> - /*
> - * Allow other tasks to run while we wait for the
> - * AP to come online. This also gives a chance
> - * for the MTRR work(triggered by the AP coming online)
> - * to be completed in the stop machine context.
> - */
> - schedule();
> - }
> +static int do_wait_cpu_cpumask(unsigned int cpu, const struct cpumask *mask)
> +{
> + unsigned long timeout;
> +
> + /*
> + * Wait up to 10s for the CPU to report in.
> + */
> + timeout = jiffies + 10*HZ;
> + while (time_before(jiffies, timeout)) {
> + if (cpumask_test_cpu(cpu, mask))
> + return 0;
> +
> + schedule();
> }
> + return -1;
> +}
>
> - if (x86_platform.legacy.warm_reset) {
> - /*
> - * Cleanup possible dangling ends...
> - */
> - smpboot_restore_warm_reset_vector();
> +/*
> + * Bringup step two: Wait for the target AP to reach cpu_init_secondary()
> + * and thus wait_for_master_cpu(), then set cpu_callout_mask to allow it
> + * to proceed. The AP will then proceed past setting its 'callin' bit
> + * and end up waiting in check_tsc_sync_target() until we reach
> + * do_wait_cpu_online() to tend to it.
> + */
> +static int do_wait_cpu_initialized(unsigned int cpu)
> +{
> + /*
> + * Wait for first sign of life from AP.
> + */
> + if (do_wait_cpu_cpumask(cpu, cpu_initialized_mask))
> + return -1;
> +
> + cpumask_set_cpu(cpu, cpu_callout_mask);
> + return 0;
> +}
> +
> +/*
> + * Bringup step three: Wait for the target AP to reach smp_callin().
> + * The AP is not waiting for us here so we don't need to parallelise
> + * this step. Not entirely clear why we care about this, since we just
> + * proceed directly to TSC synchronization which is the next sync
> + * point with the AP anyway.
> + */
> +static int do_wait_cpu_callin(unsigned int cpu)
> +{
> + /*
> + * Wait till AP completes initial initialization.
> + */
> + return do_wait_cpu_cpumask(cpu, cpu_callin_mask);
> +}
> +
> +/*
> + * Bringup step four: Synchronize the TSC and wait for the target AP
> + * to reach set_cpu_online() in start_secondary().
> + */
> +static int do_wait_cpu_online(unsigned int cpu)
> +{
> + unsigned long flags;
> +
> + /*
> + * Check TSC synchronization with the AP (keep irqs disabled
> + * while doing so):
> + */
> + local_irq_save(flags);
> + check_tsc_sync_source(cpu);
> + local_irq_restore(flags);
> +
> + /*
> + * Wait for the AP to mark itself online. Not entirely
> + * clear why we care, since the generic cpuhp code will
> + * wait for it to each CPUHP_AP_ONLINE_IDLE before going
> + * ahead with the rest of the bringup anyway.
> + */
> + while (!cpu_online(cpu)) {
> + cpu_relax();
> + touch_nmi_watchdog();
> }
>
> - return boot_error;
> + return 0;
> }
>
> -int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
> +static int do_cpu_up(unsigned int cpu, struct task_struct *tidle)
> {
> int apicid = apic->cpu_present_to_apicid(cpu);
> int cpu0_nmi_registered = 0;
> - unsigned long flags;
> int err, ret = 0;
>
> lockdep_assert_irqs_enabled();
> @@ -1239,19 +1298,6 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
> goto unreg_nmi;
> }
>
> - /*
> - * Check TSC synchronization with the AP (keep irqs disabled
> - * while doing so):
> - */
> - local_irq_save(flags);
> - check_tsc_sync_source(cpu);
> - local_irq_restore(flags);
> -
> - while (!cpu_online(cpu)) {
> - cpu_relax();
> - touch_nmi_watchdog();
> - }
> -
> unreg_nmi:
> /*
> * Clean up the nmi handler. Do this after the callin and callout sync
> @@ -1263,6 +1309,34 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
> return ret;
> }
>
> +int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
> +{
> + int ret;
> +
> + ret = do_cpu_up(cpu, tidle);
> + if (ret)
> + return ret;
> +
> + ret = do_wait_cpu_initialized(cpu);
> + if (ret)
> + return ret;
> +
> + ret = do_wait_cpu_callin(cpu);
> + if (ret)
> + return ret;
> +
> + ret = do_wait_cpu_online(cpu);
> +
> + if (x86_platform.legacy.warm_reset) {
> + /*
> + * Cleanup possible dangling ends...
> + */
> + smpboot_restore_warm_reset_vector();
> + }
> +
> + return ret;
> +}
> +
> /**
> * arch_disable_smp_support() - disables SMP support for x86 at runtime
> */
> --
> 2.25.1
>

2023-02-08 12:04:07

On Wed, 2023-02-08 at 00:09 -0500, Brian Gerst wrote:
> On Tue, Feb 7, 2023 at 6:10 PM Usama Arif <[email protected]> wrote:
> >
> > From: Thomas Gleixner <[email protected]>
> >
> > Rework the real-mode startup code to allow for APs to be brought up in
> > parallel. This is in two parts:
> >
> > 1. Introduce a bit-spinlock to prevent them from all using the real
> > mode stack at the same time.
> >
> > 2. Avoid the use of global variables for passing per-CPU information to
> > the APs.
> >
> > To achieve the latter, export the cpuid_to_apicid[] array so that each
> > AP can find its own per_cpu data (and thus initial_gs, initial_stack and
> > early_gdt_descr) by searching therein based on its APIC ID.
> >
> > Introduce a global variable 'smpboot_control' indicating to the AP how
> > it should find its APIC ID. For a serialized bringup, the APIC ID is
> > explicitly passed in the low bits of smpboot_control, while for parallel
> > mode there are flags directing the AP to find its APIC ID in CPUID leaf
> > 0x0b (for X2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
>
> For the serialized bringup case, it would be simpler to just put the
> cpu number in the lower bits instead of the APIC id, skipping the
> lookup.
>

I seem to recall that was one of my first thoughts on seeing this
patch.

I quite liked the fact that the code path in head_64.S remained
basically the same for all cases, and was actually testing the lookup
via cpuid_to_apicid[] even when parallel boot isn't available.

But then again, we've tested that part fairly well now, so perhaps
you're right and we could do something like the below? It doesn't
actually add much more complexity; just define the entry conditions to
.Linit_cpu_data a bit more clearly and remember how to use scaled index
addressing so we don't have to explicitly multiply by 8.

I don't know that I care very much either way. It's Thomas's patch
originally, so I'll let him see if he can muster up an opinion.

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 656e6018b9d4..d878869472a2 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -252,20 +252,23 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
jz .Lsetup_cpu

/*
- * Secondary CPUs find out the offsets via the APIC ID. For parallel
- * boot the APIC ID is retrieved from CPUID, otherwise it's encoded
- * in smpboot_control:
+ * Secondary CPUs find out the offsets via their per_cpu data. For
+ * parallel bringup they find it via the cpuid_to_apicid[] array
+ * with their APIC ID obtained from CPUID. Otherwise the CPU number
+ * is encoded directly in smpboot_control:
+ *
* Bit 31 STARTUP_SECONDARY flag (checked above)
* Bit 30 STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
* Bit 29 STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
- * Bit 0-24 APIC ID if STARTUP_APICID_CPUID_xx flags are not set
+ * Bit 0-24 CPU# if STARTUP_APICID_CPUID_xx flags are not set
*/
testl $STARTUP_APICID_CPUID_0B, %edx
jnz .Luse_cpuid_0b
testl $STARTUP_APICID_CPUID_01, %edx
jnz .Luse_cpuid_01
andl $0x0FFFFFFF, %edx
- jmp .Lsetup_AP
+ mov %edx, %ecx
+ jmp .Linit_cpu_data

.Luse_cpuid_01:
mov $0x01, %eax
@@ -288,14 +291,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
cmpl (%rbx), %edx
jz .Linit_cpu_data
addq $4, %rbx
- addq $8, %rcx
+ inc %rcx
jmp .Lfind_cpunr

.Linit_cpu_data:
+ /* ECX contains the CPU# of the current CPU */
/* Get the per cpu offset */
leaq __per_cpu_offset(%rip), %rbx
- addq %rcx, %rbx
- movq (%rbx), %rbx
+ movq (%rbx,%rcx,8), %rbx
/* Save it for GS BASE setup */
movq %rbx, initial_gs(%rip)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index cae916040c55..5149676881e0 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1146,7 +1146,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
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;
+ smpboot_control = STARTUP_SECONDARY | cpu;
}

/* Enable the espfix hack for this CPU */

Attachments:

smime.p7s (5.83 kB)

2023-02-08 13:13:25

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [PATCH v7 6/9] x86/smpboot: Support parallel startup of secondary CPUs

On Wed, Feb 08 2023 at 00:09, Brian Gerst wrote:
> On Tue, Feb 7, 2023 at 6:10 PM Usama Arif <[email protected]> wrote:
>> Introduce a global variable 'smpboot_control' indicating to the AP how
>> it should find its APIC ID. For a serialized bringup, the APIC ID is
>> explicitly passed in the low bits of smpboot_control, while for parallel
>> mode there are flags directing the AP to find its APIC ID in CPUID leaf
>> 0x0b (for X2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
>
> For the serialized bringup case, it would be simpler to just put the
> cpu number in the lower bits instead of the APIC id, skipping the
> lookup.

Yes and no. It can be done, but I rather prefer the consistency of the
data and the mechanism. The "overhead" or "win" is not even measurable.

Thanks,

tglx

2023-02-08 14:42:37

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;
> }
>

Attachments:

smime.p7s (5.83 kB)

2023-02-09 10:20:15

by Usama Arif

[permalink] [raw]

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

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;
>> }
>>
>

2023-02-09 10:23:12

by David Woodhouse

[permalink] [raw]

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

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?

Attachments:

smime.p7s (5.83 kB)

2023-02-09 11:04:04

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 <[email protected]>
>
> Co-developed-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: David Woodhouse <[email protected]>

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

Attachments:

smime.p7s (5.83 kB)

2023-02-09 13:49:26

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