Doing the INIT/SIPI/SIPI in parallel for all APs and *then* waiting for
them shaves about 80% off the AP bringup time on a 96-thread 2-socket
Skylake box (EC2 c5.metal) — from about 500ms to 100ms.
There are more wins to be had with further parallelisation, but this is
the simple part.
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 :)
David Woodhouse (8):
x86/apic/x2apic: Fix parallel handling of cluster_mask
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
[dwoodhou@i7 linux-2.6]$ git diff --stat v5.17-rc2..share/parallel-5.17-part1
arch/x86/include/asm/realmode.h | 3 +
arch/x86/include/asm/smp.h | 13 +-
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 | 108 ++++++-----
arch/x86/kernel/cpu/common.c | 6 +-
arch/x86/kernel/cpu/mtrr/mtrr.c | 9 +
arch/x86/kernel/head_64.S | 73 ++++++++
arch/x86/kernel/smpboot.c | 325 ++++++++++++++++++++++++----------
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 | 27 ++-
kernel/smpboot.c | 2 +-
kernel/smpboot.h | 2 -
18 files changed, 442 insertions(+), 161 deletions(-)
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).
First it actually wakes the AP by sending the INIT/SIPI/SIPI sequence.
Second, it waits 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().
Then, it 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.
Finally, it does the TSC synchronization and waits for the AP to actually
mark itself online in cpu_online_mask.
This commit should have no behavioural change, but merely splits those
phases out into separate functions so that future commits can make them
happen in parallel for all APs. And adds some comments around them on
both the BSP and AP code paths.
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/kernel/smpboot.c | 183 ++++++++++++++++++++++++++------------
1 file changed, 128 insertions(+), 55 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 84a6048d7b69..38c5d65a568d 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -214,6 +214,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);
/*
@@ -241,17 +245,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.
+ */
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();
@@ -264,6 +284,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();
@@ -1091,9 +1112,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
{
/* start_ip had better be page-aligned! */
unsigned long start_ip = real_mode_header->trampoline_start;
-
unsigned long boot_error = 0;
- unsigned long timeout;
idle->thread.sp = (unsigned long)task_pt_regs(idle);
early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
@@ -1146,55 +1165,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)
+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();
@@ -1241,19 +1299,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
@@ -1265,6 +1310,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.33.1
From: David Woodhouse <[email protected]>
For each CPU being brought up, the alloc_clustermask() function
allocates a new struct cluster_mask just in case it's needed. Then the
target CPU actually runs, and in init_x2apic_ldr() it either uses a
cluster_mask from a previous CPU in the same cluster, or consumes the
"spare" one and sets the global pointer to NULL.
That isn't going to parallelise stunningly well.
Ditch the global variable, let alloc_clustermask() install the struct
*directly* in the per_cpu data for the CPU being brought up. As an
optimisation, actually make it do so for *all* present CPUs in the same
cluster, which means only one iteration over for_each_present_cpu()
instead of doing so repeatedly, once for each CPU.
Now, in fact, there's no point in the 'node' or 'clusterid' members of
the struct cluster_mask, so just kill it and use struct cpumask instead.
This was a harmless "bug" while CPU bringup wasn't actually happening in
parallel. It's about to become less harmless...
Fixes: 023a611748fd5 ("x86/apic/x2apic: Simplify cluster management")
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/kernel/apic/x2apic_cluster.c | 108 +++++++++++++++-----------
1 file changed, 62 insertions(+), 46 deletions(-)
diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index e696e22d0531..e116dfaf5922 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -9,11 +9,7 @@
#include "local.h"
-struct cluster_mask {
- unsigned int clusterid;
- int node;
- struct cpumask mask;
-};
+#define apic_cluster(apicid) ((apicid) >> 4)
/*
* __x2apic_send_IPI_mask() possibly needs to read
@@ -23,8 +19,7 @@ struct cluster_mask {
static u32 *x86_cpu_to_logical_apicid __read_mostly;
static DEFINE_PER_CPU(cpumask_var_t, ipi_mask);
-static DEFINE_PER_CPU_READ_MOSTLY(struct cluster_mask *, cluster_masks);
-static struct cluster_mask *cluster_hotplug_mask;
+static DEFINE_PER_CPU_READ_MOSTLY(struct cpumask *, cluster_masks);
static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
{
@@ -60,10 +55,10 @@ __x2apic_send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
/* Collapse cpus in a cluster so a single IPI per cluster is sent */
for_each_cpu(cpu, tmpmsk) {
- struct cluster_mask *cmsk = per_cpu(cluster_masks, cpu);
+ struct cpumask *cmsk = per_cpu(cluster_masks, cpu);
dest = 0;
- for_each_cpu_and(clustercpu, tmpmsk, &cmsk->mask)
+ for_each_cpu_and(clustercpu, tmpmsk, cmsk)
dest |= x86_cpu_to_logical_apicid[clustercpu];
if (!dest)
@@ -71,7 +66,7 @@ __x2apic_send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
__x2apic_send_IPI_dest(dest, vector, APIC_DEST_LOGICAL);
/* Remove cluster CPUs from tmpmask */
- cpumask_andnot(tmpmsk, tmpmsk, &cmsk->mask);
+ cpumask_andnot(tmpmsk, tmpmsk, cmsk);
}
local_irq_restore(flags);
@@ -105,55 +100,76 @@ static u32 x2apic_calc_apicid(unsigned int cpu)
static void init_x2apic_ldr(void)
{
- struct cluster_mask *cmsk = this_cpu_read(cluster_masks);
- u32 cluster, apicid = apic_read(APIC_LDR);
- unsigned int cpu;
+ struct cpumask *cmsk = this_cpu_read(cluster_masks);
- x86_cpu_to_logical_apicid[smp_processor_id()] = apicid;
+ BUG_ON(!cmsk);
- if (cmsk)
- goto update;
-
- cluster = apicid >> 16;
- for_each_online_cpu(cpu) {
- cmsk = per_cpu(cluster_masks, cpu);
- /* Matching cluster found. Link and update it. */
- if (cmsk && cmsk->clusterid == cluster)
- goto update;
+ cpumask_set_cpu(smp_processor_id(), cmsk);
+}
+
+/*
+ * As an optimisation during boot, set the cluster_mask for *all*
+ * present CPUs at once, to prevent *each* of them having to iterate
+ * over the others to find the existing cluster_mask.
+ */
+static void prefill_clustermask(struct cpumask *cmsk, u32 cluster)
+{
+ int cpu;
+
+ for_each_present_cpu(cpu) {
+ u32 apicid = apic->cpu_present_to_apicid(cpu);
+ if (apicid != BAD_APICID && apic_cluster(apicid) == cluster) {
+ struct cpumask **cpu_cmsk = &per_cpu(cluster_masks, cpu);
+
+ BUG_ON(*cpu_cmsk && *cpu_cmsk != cmsk);
+ *cpu_cmsk = cmsk;
+ }
}
- cmsk = cluster_hotplug_mask;
- cmsk->clusterid = cluster;
- cluster_hotplug_mask = NULL;
-update:
- this_cpu_write(cluster_masks, cmsk);
- cpumask_set_cpu(smp_processor_id(), &cmsk->mask);
}
-static int alloc_clustermask(unsigned int cpu, int node)
+static int alloc_clustermask(unsigned int cpu, u32 cluster, int node)
{
+ struct cpumask *cmsk = NULL;
+ unsigned int cpu_i;
+ u32 apicid;
+
if (per_cpu(cluster_masks, cpu))
return 0;
- /*
- * If a hotplug spare mask exists, check whether it's on the right
- * node. If not, free it and allocate a new one.
- */
- if (cluster_hotplug_mask) {
- if (cluster_hotplug_mask->node == node)
- return 0;
- kfree(cluster_hotplug_mask);
+
+ /* For the hotplug case, don't always allocate a new one */
+ if (system_state >= SYSTEM_RUNNING) {
+ for_each_present_cpu(cpu_i) {
+ apicid = apic->cpu_present_to_apicid(cpu_i);
+ if (apicid != BAD_APICID && apic_cluster(apicid) == cluster) {
+ cmsk = per_cpu(cluster_masks, cpu_i);
+ if (cmsk)
+ break;
+ }
+ }
+ }
+ if (!cmsk) {
+ cmsk = kzalloc_node(sizeof(*cmsk), GFP_KERNEL, node);
+ if (!cmsk)
+ return -ENOMEM;
}
- cluster_hotplug_mask = kzalloc_node(sizeof(*cluster_hotplug_mask),
- GFP_KERNEL, node);
- if (!cluster_hotplug_mask)
- return -ENOMEM;
- cluster_hotplug_mask->node = node;
+ per_cpu(cluster_masks, cpu) = cmsk;
+
+ if (system_state < SYSTEM_RUNNING)
+ prefill_clustermask(cmsk, cluster);
+
return 0;
}
static int x2apic_prepare_cpu(unsigned int cpu)
{
- if (alloc_clustermask(cpu, cpu_to_node(cpu)) < 0)
+ u32 phys_apicid = apic->cpu_present_to_apicid(cpu);
+ u32 cluster = apic_cluster(phys_apicid);
+ u32 logical_apicid = (cluster << 16) | (1 << (phys_apicid & 0xf));
+
+ x86_cpu_to_logical_apicid[cpu] = logical_apicid;
+
+ if (alloc_clustermask(cpu, cluster, cpu_to_node(cpu)) < 0)
return -ENOMEM;
if (!zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL))
return -ENOMEM;
@@ -162,10 +178,10 @@ static int x2apic_prepare_cpu(unsigned int cpu)
static int x2apic_dead_cpu(unsigned int dead_cpu)
{
- struct cluster_mask *cmsk = per_cpu(cluster_masks, dead_cpu);
+ struct cpumask *cmsk = per_cpu(cluster_masks, dead_cpu);
if (cmsk)
- cpumask_clear_cpu(dead_cpu, &cmsk->mask);
+ cpumask_clear_cpu(dead_cpu, cmsk);
free_cpumask_var(per_cpu(ipi_mask, dead_cpu));
return 0;
}
--
2.33.1
From: David Woodhouse <[email protected]>
Instead of relying purely on the special-case wrapper in bringup_cpu()
to pass the idle thread to __cpu_up(), expose idle_thread_get() so that
the architecture code can obtain it directly when necessary.
This will be useful when the existing __cpu_up() is split into multiple
phases, only *one* of which will actually need the idle thread.
If the architecture code is to register its new pre-bringup states with
the cpuhp core, having a special-case wrapper to pass extra arguments is
non-trivial and it's easier just to let the arch register its function
pointer to be invoked with the standard API.
Signed-off-by: David Woodhouse <[email protected]>
---
include/linux/smpboot.h | 7 +++++++
kernel/smpboot.h | 2 --
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
index 9d1bc65d226c..3862addcaa34 100644
--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -5,6 +5,13 @@
#include <linux/types.h>
struct task_struct;
+
+#ifdef CONFIG_GENERIC_SMP_IDLE_THREAD
+struct task_struct *idle_thread_get(unsigned int cpu);
+#else
+static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; }
+#endif
+
/* Cookie handed to the thread_fn*/
struct smpboot_thread_data;
diff --git a/kernel/smpboot.h b/kernel/smpboot.h
index 34dd3d7ba40b..60c609318ad6 100644
--- a/kernel/smpboot.h
+++ b/kernel/smpboot.h
@@ -5,11 +5,9 @@
struct task_struct;
#ifdef CONFIG_GENERIC_SMP_IDLE_THREAD
-struct task_struct *idle_thread_get(unsigned int cpu);
void idle_thread_set_boot_cpu(void);
void idle_threads_init(void);
#else
-static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; }
static inline void idle_thread_set_boot_cpu(void) { }
static inline void idle_threads_init(void) { }
#endif
--
2.33.1
From: Thomas Gleixner <[email protected]>
To allow for parallel AP bringup, we need to avoid the use of global
variables for passing information to the APs, as well as preventing them
from all trying to use the same real-mode stack simultaneously.
So, introduce a 'lock' field in struct trampoline_header to use as a
simple bit-spinlock for the real-mode stack. That lock also protects
the global variables initial_gs, initial_stack and early_gdt_descr,
which can now be calculated...
So how do we calculate those addresses? Well, they they can all be found
from the per_cpu data for this CPU. Simples! Except... how does it know
what its CPU# is? OK, we export the cpuid_to_apicid[] array and it can
search it to find its APIC ID in there.
But now you whine at me that it doesn't even know its APIC ID? Well, if
it's a relatively modern CPU then the APIC ID is in CPUID leaf 0x0B so
we can use that. Otherwise... erm... OK, otherwise it can't have parallel
CPU bringup for now. We'll still use a global variable for those CPUs and
bring them up one at a time.
So add a global 'smpboot_control' field which either contains the APIC
ID, or a flag indicating that it can be found in CPUID.
This adds the 'do_parallel_bringup' flag in preparation but doesn't
actually enable parallel bringup yet.
[ dwmw2: Minor tweaks, write a commit message ]
[ seanc: Fix stray override of initial_gs in common_cpu_up() ]
Not-signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/include/asm/realmode.h | 3 ++
arch/x86/include/asm/smp.h | 9 +++-
arch/x86/kernel/acpi/sleep.c | 1 +
arch/x86/kernel/apic/apic.c | 2 +-
arch/x86/kernel/head_64.S | 73 ++++++++++++++++++++++++++++
arch/x86/kernel/smpboot.c | 32 ++++++++++--
arch/x86/realmode/init.c | 3 ++
arch/x86/realmode/rm/trampoline_64.S | 14 ++++++
kernel/smpboot.c | 2 +-
9 files changed, 132 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index 331474b150f1..1693bc834163 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -51,6 +51,7 @@ struct trampoline_header {
u64 efer;
u32 cr4;
u32 flags;
+ u32 lock;
#endif
};
@@ -64,6 +65,8 @@ extern unsigned long initial_stack;
extern unsigned long initial_vc_handler;
#endif
+extern u32 *trampoline_lock;
+
extern unsigned char real_mode_blob[];
extern unsigned char real_mode_relocs[];
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 81a0211a372d..4fe1320c2e8d 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -196,5 +196,12 @@ extern void nmi_selftest(void);
#define nmi_selftest() do { } while (0)
#endif
-#endif /* __ASSEMBLY__ */
+extern unsigned int smpboot_control;
+
+#endif /* !__ASSEMBLY__ */
+
+/* Control bits for startup_64 */
+#define STARTUP_PARALLEL 0x80000000
+#define STARTUP_SECONDARY 0x40000000
+
#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 1e97f944b47d..4f26cc9346ac 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -114,6 +114,7 @@ int x86_acpi_suspend_lowlevel(void)
early_gdt_descr.address =
(unsigned long)get_cpu_gdt_rw(smp_processor_id());
initial_gs = per_cpu_offset(smp_processor_id());
+ smpboot_control = 0;
#endif
initial_code = (unsigned long)wakeup_long64;
saved_magic = 0x123456789abcdef0L;
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b70344bf6600..5b20e051d84c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2335,7 +2335,7 @@ static int nr_logical_cpuids = 1;
/*
* Used to store mapping between logical CPU IDs and APIC IDs.
*/
-static int cpuid_to_apicid[] = {
+int cpuid_to_apicid[] = {
[0 ... NR_CPUS - 1] = -1,
};
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 9c63fc5988cd..b0d8c9fffc73 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -25,6 +25,7 @@
#include <asm/export.h>
#include <asm/nospec-branch.h>
#include <asm/fixmap.h>
+#include <asm/smp.h>
/*
* We are not able to switch in one step to the final KERNEL ADDRESS SPACE
@@ -193,6 +194,66 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
1:
UNWIND_HINT_EMPTY
+ /*
+ * Is this the boot CPU coming up? If so everything is available
+ * in initial_gs, initial_stack and early_gdt_descr.
+ */
+ movl smpboot_control(%rip), %eax
+ testl %eax, %eax
+ 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:
+ * Bit 0-29 APIC ID if STARTUP_PARALLEL flag is not set
+ * Bit 30 STARTUP_SECONDARY flag
+ * Bit 31 STARTUP_PARALLEL flag (use CPUID 0x0b for APIC ID)
+ */
+ testl $STARTUP_PARALLEL, %eax
+ jnz .Luse_cpuid_0b
+ andl $0x0FFFFFFF, %eax
+ jmp .Lsetup_AP
+
+.Luse_cpuid_0b:
+ mov $0x0B, %eax
+ xorl %ecx, %ecx
+ cpuid
+ mov %edx, %eax
+
+.Lsetup_AP:
+ /* EAX contains the APICID of the current CPU */
+ xorl %ecx, %ecx
+ leaq cpuid_to_apicid(%rip), %rbx
+
+.Lfind_cpunr:
+ cmpl (%rbx), %eax
+ jz .Linit_cpu_data
+ addq $4, %rbx
+ addq $8, %rcx
+ jmp .Lfind_cpunr
+
+.Linit_cpu_data:
+ /* Get the per cpu offset */
+ leaq __per_cpu_offset(%rip), %rbx
+ addq %rcx, %rbx
+ movq (%rbx), %rbx
+ /* Save it for GS BASE setup */
+ movq %rbx, initial_gs(%rip)
+
+ /* Calculate the GDT address */
+ movq $gdt_page, %rcx
+ addq %rbx, %rcx
+ movq %rcx, early_gdt_descr_base(%rip)
+
+ /* Find the idle task stack */
+ movq $idle_threads, %rcx
+ addq %rbx, %rcx
+ movq (%rcx), %rcx
+ movq TASK_threadsp(%rcx), %rcx
+ movq %rcx, initial_stack(%rip)
+
+.Lsetup_cpu:
/*
* 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
@@ -233,6 +294,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
*/
movq initial_stack(%rip), %rsp
+ /* 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
@@ -364,6 +433,7 @@ SYM_DATA(initial_vc_handler, .quad handle_vc_boot_ghcb)
* reliably detect the end of the stack.
*/
SYM_DATA(initial_stack, .quad init_thread_union + THREAD_SIZE - FRAME_SIZE)
+SYM_DATA(trampoline_lock, .quad 0);
__FINITDATA
__INIT
@@ -589,6 +659,9 @@ SYM_DATA_END(level1_fixmap_pgt)
SYM_DATA(early_gdt_descr, .word GDT_ENTRIES*8-1)
SYM_DATA_LOCAL(early_gdt_descr_base, .quad INIT_PER_CPU_VAR(gdt_page))
+ .align 16
+SYM_DATA(smpboot_control, .long 0)
+
.align 16
/* This must match the first entry in level2_kernel_pgt */
SYM_DATA(phys_base, .quad 0x0)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 38c5d65a568d..e060bbd79cc2 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -808,6 +808,16 @@ static int __init cpu_init_udelay(char *str)
}
early_param("cpu_init_udelay", cpu_init_udelay);
+static bool do_parallel_bringup = true;
+
+static int __init no_parallel_bringup(char *str)
+{
+ do_parallel_bringup = false;
+
+ return 0;
+}
+early_param("no_parallel_bringup", no_parallel_bringup);
+
static void __init smp_quirk_init_udelay(void)
{
/* if cmdline changed it from default, leave it alone */
@@ -1095,8 +1105,6 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle)
#ifdef CONFIG_X86_32
/* Stack for startup_32 can be just as for start_secondary onwards */
per_cpu(cpu_current_top_of_stack, cpu) = task_top_of_stack(idle);
-#else
- initial_gs = per_cpu_offset(cpu);
#endif
return 0;
}
@@ -1115,9 +1123,16 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
unsigned long boot_error = 0;
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)) {
+ 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 | STARTUP_PARALLEL;
+ } else {
+ smpboot_control = STARTUP_SECONDARY | apicid;
+ }
/* Enable the espfix hack for this CPU */
init_espfix_ap(cpu);
@@ -1516,6 +1531,15 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
smp_quirk_init_udelay();
speculative_store_bypass_ht_init();
+
+ /*
+ * We can do 64-bit AP bringup in parallel if the CPU reports its
+ * APIC ID in CPUID leaf 0x0B. Otherwise it's too hard. And not
+ * for SEV-ES guests because they can't use CPUID that early.
+ */
+ if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 0x0B ||
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+ do_parallel_bringup = false;
}
void arch_thaw_secondary_cpus_begin(void)
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index c5e29db02a46..21b9e8b55618 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -154,6 +154,9 @@ static void __init setup_real_mode(void)
trampoline_header->flags = 0;
+ trampoline_lock = &trampoline_header->lock;
+ *trampoline_lock = 0;
+
trampoline_pgd = (u64 *) __va(real_mode_header->trampoline_pgd);
/* Map the real mode stub as virtual == physical */
diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
index cc8391f86cdb..12a540904e80 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -49,6 +49,19 @@ SYM_CODE_START(trampoline_start)
mov %ax, %es
mov %ax, %ss
+ /*
+ * Make sure only one CPU fiddles with the realmode stack
+ */
+.Llock_rm:
+ btl $0, tr_lock
+ jnc 2f
+ pause
+ jmp .Llock_rm
+2:
+ lock
+ btsl $0, tr_lock
+ jc .Llock_rm
+
# Setup stack
movl $rm_stack_end, %esp
@@ -192,6 +205,7 @@ SYM_DATA_START(trampoline_header)
SYM_DATA(tr_efer, .space 8)
SYM_DATA(tr_cr4, .space 4)
SYM_DATA(tr_flags, .space 4)
+ SYM_DATA(tr_lock, .space 4)
SYM_DATA_END(trampoline_header)
#include "trampoline_common.S"
diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index f6bc0bc8a2aa..934e64ff4eed 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -25,7 +25,7 @@
* For the hotplug case we keep the task structs around and reuse
* them.
*/
-static DEFINE_PER_CPU(struct task_struct *, idle_threads);
+DEFINE_PER_CPU(struct task_struct *, idle_threads);
struct task_struct *idle_thread_get(unsigned int cpu)
{
--
2.33.1
On 2/1/22 14:53, David Woodhouse wrote:
> Doing the INIT/SIPI/SIPI in parallel for all APs and *then* waiting for
> them shaves about 80% off the AP bringup time on a 96-thread 2-socket
> Skylake box (EC2 c5.metal) — from about 500ms to 100ms.
>
> There are more wins to be had with further parallelisation, but this is
> the simple part.
>
> 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 :)
I'm no longer seeing crashes launching high vCPU-count guests with this
series.
Thanks,
Tom
>
> David Woodhouse (8):
> x86/apic/x2apic: Fix parallel handling of cluster_mask
> 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
>
> [dwoodhou@i7 linux-2.6]$ git diff --stat v5.17-rc2..share/parallel-5.17-part1
> arch/x86/include/asm/realmode.h | 3 +
> arch/x86/include/asm/smp.h | 13 +-
> 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 | 108 ++++++-----
> arch/x86/kernel/cpu/common.c | 6 +-
> arch/x86/kernel/cpu/mtrr/mtrr.c | 9 +
> arch/x86/kernel/head_64.S | 73 ++++++++
> arch/x86/kernel/smpboot.c | 325 ++++++++++++++++++++++++----------
> 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 | 27 ++-
> kernel/smpboot.c | 2 +-
> kernel/smpboot.h | 2 -
> 18 files changed, 442 insertions(+), 161 deletions(-)
>
>
>
On 01/02/2022 20:53, David Woodhouse wrote:
> Doing the INIT/SIPI/SIPI in parallel for all APs and *then* waiting for
> them shaves about 80% off the AP bringup time on a 96-thread 2-socket
> Skylake box (EC2 c5.metal) — from about 500ms to 100ms.
>
> There are more wins to be had with further parallelisation, but this is
> the simple part.
>
Hi,
We are interested in reducing the boot time of servers (with kexec), and
smpboot takes up a significant amount of time while booting. When
testing the patch series (rebased to v6.1) on a server with 128 CPUs
split across 2 NUMA nodes, it brought down the smpboot time from ~700ms
to 100ms. Adding another cpuhp state for do_wait_cpu_initialized to make
sure cpu_init is reached (as done in v1 of the series + using the
cpu_finishup_mask) brought it down further to ~30ms.
I just wanted to check what was needed to progress the patch series
further for review? There weren't any comments on v4 of the patch so I
couldn't figure out what more is needed. I think its quite useful to
have this working so would be really glad help in anything needed to
restart the review.
Thanks!
Usama
> 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 :)
>
> David Woodhouse (8):
> x86/apic/x2apic: Fix parallel handling of cluster_mask
> 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
>
> [dwoodhou@i7 linux-2.6]$ git diff --stat v5.17-rc2..share/parallel-5.17-part1
> arch/x86/include/asm/realmode.h | 3 +
> arch/x86/include/asm/smp.h | 13 +-
> 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 | 108 ++++++-----
> arch/x86/kernel/cpu/common.c | 6 +-
> arch/x86/kernel/cpu/mtrr/mtrr.c | 9 +
> arch/x86/kernel/head_64.S | 73 ++++++++
> arch/x86/kernel/smpboot.c | 325 ++++++++++++++++++++++++----------
> 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 | 27 ++-
> kernel/smpboot.c | 2 +-
> kernel/smpboot.h | 2 -
> 18 files changed, 442 insertions(+), 161 deletions(-)
>
>
>
>
On Wed, 2023-02-01 at 14:40 +0000, Usama Arif wrote:
> On 01/02/2022 20:53, David Woodhouse wrote:
> > Doing the INIT/SIPI/SIPI in parallel for all APs and *then* waiting for
> > them shaves about 80% off the AP bringup time on a 96-thread 2-socket
> > Skylake box (EC2 c5.metal) — from about 500ms to 100ms.
> >
> > There are more wins to be had with further parallelisation, but this is
> > the simple part.
> >
>
> Hi,
>
> We are interested in reducing the boot time of servers (with kexec), and
> smpboot takes up a significant amount of time while booting. When
> testing the patch series (rebased to v6.1) on a server with 128 CPUs
> split across 2 NUMA nodes, it brought down the smpboot time from ~700ms
> to 100ms. Adding another cpuhp state for do_wait_cpu_initialized to make
> sure cpu_init is reached (as done in v1 of the series + using the
> cpu_finishup_mask) brought it down further to ~30ms.
>
> I just wanted to check what was needed to progress the patch series
> further for review? There weren't any comments on v4 of the patch so I
> couldn't figure out what more is needed. I think its quite useful to
> have this working so would be really glad help in anything needed to
> restart the review.
I believe the only thing holding it back was the fact that it broke on
some AMD CPUs.
We don't *think* there are any remaining software issues; we think it's
hardware. Either an actual hardware race in CPU or chipset, or perhaps
even something as simple as a voltage regulator which can't cope with
an increase in power draw from *all* the CPUs at the same time.
We have prodded AMD a few times to investigate, but so far to no avail.
Last time I actually spoke to Thomas in person, I think he agreed that
we should just merge it and disable the parallel mode for the affected
AMD CPUs.
If you've already rebased to a newer kernel and tested it, perhaps now
is the time to do just that.
On 01/02/2023 15:08, David Woodhouse wrote:
> On Wed, 2023-02-01 at 14:40 +0000, Usama Arif wrote:
>> On 01/02/2022 20:53, David Woodhouse wrote:
>>> Doing the INIT/SIPI/SIPI in parallel for all APs and *then* waiting for
>>> them shaves about 80% off the AP bringup time on a 96-thread 2-socket
>>> Skylake box (EC2 c5.metal) — from about 500ms to 100ms.
>>>
>>> There are more wins to be had with further parallelisation, but this is
>>> the simple part.
>>>
>>
>> Hi,
>>
>> We are interested in reducing the boot time of servers (with kexec), and
>> smpboot takes up a significant amount of time while booting. When
>> testing the patch series (rebased to v6.1) on a server with 128 CPUs
>> split across 2 NUMA nodes, it brought down the smpboot time from ~700ms
>> to 100ms. Adding another cpuhp state for do_wait_cpu_initialized to make
>> sure cpu_init is reached (as done in v1 of the series + using the
>> cpu_finishup_mask) brought it down further to ~30ms.
>>
>> I just wanted to check what was needed to progress the patch series
>> further for review? There weren't any comments on v4 of the patch so I
>> couldn't figure out what more is needed. I think its quite useful to
>> have this working so would be really glad help in anything needed to
>> restart the review.
>
>
> I believe the only thing holding it back was the fact that it broke on
> some AMD CPUs.
>
> We don't *think* there are any remaining software issues; we think it's
> hardware. Either an actual hardware race in CPU or chipset, or perhaps
> even something as simple as a voltage regulator which can't cope with
> an increase in power draw from *all* the CPUs at the same time.
>
> We have prodded AMD a few times to investigate, but so far to no avail.
>
> Last time I actually spoke to Thomas in person, I think he agreed that
> we should just merge it and disable the parallel mode for the affected
> AMD CPUs.
>
From the comments in v3, it seems to affect multiple generations, would
it be worth proceeding with the patches by disabling it on all AMD CPUs
to be on the safe side, until the actual issue is found and what causes
it, and then follow up later if the issue is found by disabling it only
on affected cpus. Maybe simply do something like below?
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 0f144773a7fc..6b8884592341 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1575,7 +1575,8 @@ void __init native_smp_prepare_cpus(unsigned int
max_cpus)
* for SEV-ES guests because they can't use CPUID that early.
*/
if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level <
0x0B ||
- cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) ||
+ boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
do_parallel_bringup = false;
if (do_parallel_bringup) {
> If you've already rebased to a newer kernel and tested it, perhaps now
> is the time to do just that.
If you would like me to repost the rebased patches to restart the
reviews (with do_parallel_bringup disabled for AMD), please let me know!
Thanks,
Usama
On February 1, 2023 8:38:14 AM PST, Usama Arif <[email protected]> wrote:
>
>
>On 01/02/2023 15:08, David Woodhouse wrote:
>> On Wed, 2023-02-01 at 14:40 +0000, Usama Arif wrote:
>>> On 01/02/2022 20:53, David Woodhouse wrote:
>>>> Doing the INIT/SIPI/SIPI in parallel for all APs and *then* waiting for
>>>> them shaves about 80% off the AP bringup time on a 96-thread 2-socket
>>>> Skylake box (EC2 c5.metal) — from about 500ms to 100ms.
>>>>
>>>> There are more wins to be had with further parallelisation, but this is
>>>> the simple part.
>>>>
>>>
>>> Hi,
>>>
>>> We are interested in reducing the boot time of servers (with kexec), and
>>> smpboot takes up a significant amount of time while booting. When
>>> testing the patch series (rebased to v6.1) on a server with 128 CPUs
>>> split across 2 NUMA nodes, it brought down the smpboot time from ~700ms
>>> to 100ms. Adding another cpuhp state for do_wait_cpu_initialized to make
>>> sure cpu_init is reached (as done in v1 of the series + using the
>>> cpu_finishup_mask) brought it down further to ~30ms.
>>>
>>> I just wanted to check what was needed to progress the patch series
>>> further for review? There weren't any comments on v4 of the patch so I
>>> couldn't figure out what more is needed. I think its quite useful to
>>> have this working so would be really glad help in anything needed to
>>> restart the review.
>>
>>
>> I believe the only thing holding it back was the fact that it broke on
>> some AMD CPUs.
>>
>> We don't *think* there are any remaining software issues; we think it's
>> hardware. Either an actual hardware race in CPU or chipset, or perhaps
>> even something as simple as a voltage regulator which can't cope with
>> an increase in power draw from *all* the CPUs at the same time.
>>
>> We have prodded AMD a few times to investigate, but so far to no avail.
>>
>> Last time I actually spoke to Thomas in person, I think he agreed that
>> we should just merge it and disable the parallel mode for the affected
>> AMD CPUs.
>>
>
>From the comments in v3, it seems to affect multiple generations, would it be worth proceeding with the patches by disabling it on all AMD CPUs to be on the safe side, until the actual issue is found and what causes it, and then follow up later if the issue is found by disabling it only on affected cpus. Maybe simply do something like below?
>
>diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>index 0f144773a7fc..6b8884592341 100644
>--- a/arch/x86/kernel/smpboot.c
>+++ b/arch/x86/kernel/smpboot.c
>@@ -1575,7 +1575,8 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
> * for SEV-ES guests because they can't use CPUID that early.
> */
> if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 0x0B ||
>- cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
>+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) ||
>+ boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> do_parallel_bringup = false;
>
> if (do_parallel_bringup) {
>
>
>
>
>> If you've already rebased to a newer kernel and tested it, perhaps now
>> is the time to do just that.
>
>If you would like me to repost the rebased patches to restart the reviews (with do_parallel_bringup disabled for AMD), please let me know!
>
>Thanks,
>Usama
This should be a CPU bug flag in my option.
On Wed, 2023-02-01 at 08:55 -0800, H. Peter Anvin wrote:
> This should be a CPU bug flag in my option.
This is in the tree that I've just rebased to v6.2-rc6 for Usama to
continue testing and repost as appropriate.
(Oh, as I post it in email I realise we should probably retcon the
explicit check for AMD out of the previous patch in the series. You can
see it being *removed* in this patch.)
From 1f7cece1241e5b9c9988f943962155bb7154d4f8 Mon Sep 17 00:00:00 2001
From: David Woodhouse <[email protected]>
Date: Thu, 2 Feb 2023 09:53:26 +0000
Subject: [PATCH 07/15] x86/smpboot: Disable parallel boot for AMD CPUs
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/kernel/cpu/amd.c | 11 +++++++++++
arch/x86/kernel/smpboot.c | 9 +++++++--
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 61012476d66e..ed7f32354edc 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -466,5 +466,6 @@
#define X86_BUG_MMIO_UNKNOWN X86_BUG(26) /* CPU is too old and its MMIO Stale Data status is unknown */
#define X86_BUG_RETBLEED X86_BUG(27) /* CPU is affected by RETBleed */
#define X86_BUG_EIBRS_PBRSB X86_BUG(28) /* EIBRS is vulnerable to Post Barrier RSB Predictions */
+#define X86_BUG_NO_PARALLEL_BRINGUP X86_BUG(29) /* CPU has hardware issues with parallel AP bringup */
#endif /* _ASM_X86_CPUFEATURES_H */
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index f769d6d08b43..19b5c8342d7e 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -941,6 +941,17 @@ static void init_amd(struct cpuinfo_x86 *c)
case 0x19: init_amd_zn(c); break;
}
+ /*
+ * Various AMD CPUs appear to not to cope with APs being brought up
+ * in parallel. In debugging, the AP doesn't even seem to reach an
+ * outb to port 0x3f8 right at the top of the startup trampoline.
+ * We don't *think* there are any remaining software issues which
+ * may contribute to this, although it's possible. So far, attempts
+ * to get AMD to investigate this have been to no avail. So just
+ * disable parallel bring up for all AMD CPUs for now.
+ */
+ set_cpu_bug(c, X86_BUG_NO_PARALLEL_BRINGUP);
+
/*
* Enable workaround for FXSAVE leak on CPUs
* without a XSaveErPtr feature
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 7920823d5a3b..95c182023d09 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1538,9 +1538,14 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
* it for all AMD CPUs to be on the safe side.
*/
if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 0x0B ||
- cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) ||
- boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
do_parallel_bringup = false;
+ }
+ if (do_parallel_bringup &&
+ boot_cpu_has_bug(X86_BUG_NO_PARALLEL_BRINGUP)) {
+ pr_info("Disabling parallel bringup due to CPU bugs\n");
+ do_parallel_bringup = false;
+ }
snp_set_wakeup_secondary_cpu();
}
--
2.39.0