The major change over v7 is fixing CPU0 hotplug not working as reported by
Paul E. McKenney using rcu torture tests. This is fixed by setting up the
initial_gs, initial_stack and early_gdt_descr properly for this case.
The improvement in boot time is the same as v7.
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.
v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
early_gdt_descr.
Drop trampoline lock and bail if APIC ID not found in find_cpunr.
Code comments improved and debug prints added.
David Woodhouse (9):
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: Support parallel startup of secondary CPUs
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
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 | 99 +++++++-
arch/x86/kernel/smpboot.c | 350 +++++++++++++++++++-------
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, 521 insertions(+), 160 deletions(-)
--
2.25.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]>
Reviewed-by: Thomas Gleixner <[email protected]>
Signed-off-by: Usama Arif <[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.25.1
From: David Woodhouse <[email protected]>
There is often significant latency in the early stages of CPU bringup,
and time is wasted by waking each CPU (e.g. with SIPI/INIT/INIT on x86)
and then waiting for it to respond before moving on to the next.
Allow a platform to register a set of pre-bringup CPUHP states to which
each CPU can be stepped in parallel, thus absorbing some of that latency.
There is a subtlety here: even with an empty CPUHP_BP_PARALLEL_DYN step,
this means that *all* CPUs are brought through the prepare states and to
CPUHP_BP_PREPARE_DYN before any of them are taken to CPUHP_BRINGUP_CPU
and then are allowed to run for themselves to CPUHP_ONLINE.
So any combination of prepare/start calls which depend on A-B ordering
for each CPU in turn, such as the X2APIC code which used to allocate a
cluster mask 'just in case' and store it in a global variable in the
prep stage, then potentially consume that preallocated structure from
the AP and set the global pointer to NULL to be reallocated in
CPUHP_X2APIC_PREPARE for the next CPU... would explode horribly.
Any platform enabling the CPUHP_BP_PARALLEL_DYN steps must be reviewed
and tested to ensure that such issues do not exist, and the existing
behaviour of bringing CPUs to CPUHP_BP_PREPARE_DYN and then immediately
to CPUHP_BRINGUP_CPU and CPUHP_ONLINE only one at a time does not change
unless such a state is registered.
Note that the new parallel stages do *not* yet bring each AP to the
CPUHP_BRINGUP_CPU state at the same time, only to the new states which
exist before it. The final loop in bringup_nonboot_cpus() is untouched,
bringing each AP in turn from the final PARALLEL_DYN state (or all the
way from CPUHP_OFFLINE) to CPUHP_BRINGUP_CPU and then waiting for that
AP to do its own processing and reach CPUHP_ONLINE before releasing the
next.
Parallelising that part by bringing them all to CPUHP_BRINGUP_CPU
and then waiting for them all is an exercise for the future.
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
---
include/linux/cpuhotplug.h | 2 ++
kernel/cpu.c | 31 +++++++++++++++++++++++++++++--
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 6c6859bfc454..e5a73ae6ccc0 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -133,6 +133,8 @@ enum cpuhp_state {
CPUHP_MIPS_SOC_PREPARE,
CPUHP_BP_PREPARE_DYN,
CPUHP_BP_PREPARE_DYN_END = CPUHP_BP_PREPARE_DYN + 20,
+ CPUHP_BP_PARALLEL_DYN,
+ CPUHP_BP_PARALLEL_DYN_END = CPUHP_BP_PARALLEL_DYN + 4,
CPUHP_BRINGUP_CPU,
/*
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6c0a92ca6bb5..fffb0da61ccc 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1504,8 +1504,30 @@ int bringup_hibernate_cpu(unsigned int sleep_cpu)
void bringup_nonboot_cpus(unsigned int setup_max_cpus)
{
+ unsigned int n = setup_max_cpus - num_online_cpus();
unsigned int cpu;
+ /*
+ * An architecture may have registered parallel pre-bringup states to
+ * which each CPU may be brought in parallel. For each such state,
+ * bring N CPUs to it in turn before the final round of bringing them
+ * online.
+ */
+ if (n > 0) {
+ enum cpuhp_state st = CPUHP_BP_PARALLEL_DYN;
+
+ while (st <= CPUHP_BP_PARALLEL_DYN_END && cpuhp_hp_states[st].name) {
+ int i = n;
+
+ for_each_present_cpu(cpu) {
+ cpu_up(cpu, st);
+ if (!--i)
+ break;
+ }
+ st++;
+ }
+ }
+
for_each_present_cpu(cpu) {
if (num_online_cpus() >= setup_max_cpus)
break;
@@ -1882,6 +1904,10 @@ static int cpuhp_reserve_state(enum cpuhp_state state)
step = cpuhp_hp_states + CPUHP_BP_PREPARE_DYN;
end = CPUHP_BP_PREPARE_DYN_END;
break;
+ case CPUHP_BP_PARALLEL_DYN:
+ step = cpuhp_hp_states + CPUHP_BP_PARALLEL_DYN;
+ end = CPUHP_BP_PARALLEL_DYN_END;
+ break;
default:
return -EINVAL;
}
@@ -1906,14 +1932,15 @@ static int cpuhp_store_callbacks(enum cpuhp_state state, const char *name,
/*
* If name is NULL, then the state gets removed.
*
- * CPUHP_AP_ONLINE_DYN and CPUHP_BP_PREPARE_DYN are handed out on
+ * CPUHP_AP_ONLINE_DYN and CPUHP_BP_P*_DYN are handed out on
* the first allocation from these dynamic ranges, so the removal
* would trigger a new allocation and clear the wrong (already
* empty) state, leaving the callbacks of the to be cleared state
* dangling, which causes wreckage on the next hotplug operation.
*/
if (name && (state == CPUHP_AP_ONLINE_DYN ||
- state == CPUHP_BP_PREPARE_DYN)) {
+ state == CPUHP_BP_PREPARE_DYN ||
+ state == CPUHP_BP_PARALLEL_DYN)) {
ret = cpuhp_reserve_state(state);
if (ret < 0)
return ret;
--
2.25.1
From: David Woodhouse <[email protected]>
When bringing up a secondary CPU from do_boot_cpu(), the warm reset flag
is set in CMOS and the starting IP for the trampoline written inside the
BDA at 0x467. Once the CPU is running, the CMOS flag is unset and the
value in the BDA cleared.
To allow for parallel bringup of CPUs, add a reference count to track the
number of CPUs currently bring brought up, and clear the state only when
the count reaches zero.
Since the RTC spinlock is required to write to the CMOS, it can be used
for mutual exclusion on the refcount too.
[Usama Arif: fixed rebase conflict]
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
---
arch/x86/kernel/smpboot.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 55cad72715d9..3a793772a2aa 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -121,17 +121,20 @@ int arch_update_cpu_topology(void)
return retval;
}
+
+static unsigned int smpboot_warm_reset_vector_count;
+
static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
{
unsigned long flags;
spin_lock_irqsave(&rtc_lock, flags);
- CMOS_WRITE(0xa, 0xf);
+ if (!smpboot_warm_reset_vector_count++) {
+ CMOS_WRITE(0xa, 0xf);
+ *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) = start_eip >> 4;
+ *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = start_eip & 0xf;
+ }
spin_unlock_irqrestore(&rtc_lock, flags);
- *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
- start_eip >> 4;
- *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
- start_eip & 0xf;
}
static inline void smpboot_restore_warm_reset_vector(void)
@@ -143,10 +146,12 @@ static inline void smpboot_restore_warm_reset_vector(void)
* to default values.
*/
spin_lock_irqsave(&rtc_lock, flags);
- CMOS_WRITE(0, 0xf);
+ if (!--smpboot_warm_reset_vector_count) {
+ CMOS_WRITE(0, 0xf);
+ *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
+ }
spin_unlock_irqrestore(&rtc_lock, flags);
- *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
}
/*
--
2.25.1
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) Wait 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 | 181 ++++++++++++++++++++++++++------------
1 file changed, 127 insertions(+), 54 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 3a793772a2aa..b9366f86c433 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,32 @@ static void notrace start_secondary(void *unused)
load_cr3(swapper_pg_dir);
__flush_tlb_all();
#endif
+ /*
+ * Sync point with do_wait_cpu_initialized(). Before proceeding through
+ * cpu_init(), the AP will call wait_for_master_cpu() which sets its
+ * own bit in cpu_initialized_mask and then waits for the BSP to set
+ * its bit bit in cpu_callout_mask to release it.
+ */
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 +273,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 +1103,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 +1163,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 +1297,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 +1308,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
From: David Woodhouse <[email protected]>
When the APs can find their own APIC ID without assistance, perform the
AP bringup in parallel.
Register a CPUHP_BP_PARALLEL_DYN stage "x86/cpu:kick" which just calls
do_boot_cpu() to deliver INIT/SIPI/SIPI to each AP in turn before the
normal native_cpu_up() does the rest of the hand-holding.
The APs will then take turns through the real mode code (which has its
own bitlock for exclusion) until they make it to their own stack, then
proceed through the first few lines of start_secondary() and execute
these parts in parallel:
start_secondary()
-> cr4_init()
-> (some 32-bit only stuff so not in the parallel cases)
-> cpu_init_secondary()
-> cpu_init_exception_handling()
-> cpu_init()
-> wait_for_master_cpu()
At this point they wait for the BSP to set their bit in cpu_callout_mask
(from do_wait_cpu_initialized()), and release them to continue through
the rest of cpu_init() and beyond.
This reduces the time taken for bringup on my 28-thread Haswell system
from about 120ms to 80ms. On a socket 96-thread Skylake it takes the
bringup time from 500ms to 100ms.
There is more speedup to be had by doing the remaining parts in parallel
too — especially notify_cpu_starting() in which the AP takes itself
through all the stages from CPUHP_BRINGUP_CPU to CPUHP_ONLINE. But those
require careful auditing to ensure they are reentrant, before we can go
that far.
[Usama Arif: fixed rebase conflict]
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
---
arch/x86/kernel/smpboot.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 50621793671d..df839264266b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -57,6 +57,7 @@
#include <linux/pgtable.h>
#include <linux/overflow.h>
#include <linux/stackprotector.h>
+#include <linux/smpboot.h>
#include <asm/acpi.h>
#include <asm/cacheinfo.h>
@@ -1325,9 +1326,12 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
{
int ret;
- ret = do_cpu_up(cpu, tidle);
- if (ret)
- return ret;
+ /* If parallel AP bringup isn't enabled, perform the first steps now. */
+ if (!do_parallel_bringup) {
+ ret = do_cpu_up(cpu, tidle);
+ if (ret)
+ return ret;
+ }
ret = do_wait_cpu_initialized(cpu);
if (ret)
@@ -1349,6 +1353,12 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
return ret;
}
+/* Bringup step one: Send INIT/SIPI to the target AP */
+static int native_cpu_kick(unsigned int cpu)
+{
+ return do_cpu_up(cpu, idle_thread_get(cpu));
+}
+
/**
* arch_disable_smp_support() - disables SMP support for x86 at runtime
*/
@@ -1566,6 +1576,11 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
smpboot_control = STARTUP_SECONDARY | STARTUP_APICID_CPUID_01;
}
+ if (do_parallel_bringup) {
+ cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
+ native_cpu_kick, NULL);
+ }
+
snp_set_wakeup_secondary_cpu();
}
--
2.25.1
From: David Woodhouse <[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.
Parallel startup may be disabled by a command line option, and also if:
• AMD SEV-ES is in use, since the AP may not use CPUID that early.
• X2APIC is enabled, but CPUID leaf 0xb is not present and correect.
• X2APIC is not enabled but not even CPUID leaf 0x01 exists.
Aside from the fact that APs will now look up their per-cpu data via the
newly-exported cpuid_to_apicid[] table, there is no behavioural change
intended yet, since new parallel CPUHP states have not — yet — been added.
[ tglx: Initial proof of concept patch with bitlock and APIC ID lookup ]
[ dwmw2: Rework and testing, commit message, CPUID 0x1 and CPU0 support ]
[ seanc: Fix stray override of initial_gs in common_cpu_up() ]
Co-developed-by: Thomas Gleixner <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
---
arch/x86/include/asm/realmode.h | 3 +
arch/x86/include/asm/smp.h | 10 ++-
arch/x86/kernel/acpi/sleep.c | 1 +
arch/x86/kernel/apic/apic.c | 2 +-
arch/x86/kernel/head_64.S | 99 +++++++++++++++++++++++++++-
arch/x86/kernel/smpboot.c | 62 +++++++++++++++--
arch/x86/realmode/init.c | 3 +
arch/x86/realmode/rm/trampoline_64.S | 14 ++++
kernel/smpboot.c | 2 +-
9 files changed, 188 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index a336feef0af1..f0357cfe2fb0 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -52,6 +52,7 @@ struct trampoline_header {
u64 efer;
u32 cr4;
u32 flags;
+ u32 lock;
#endif
};
@@ -65,6 +66,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 b4dbb20dab1a..33c0d5fd8af6 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -199,5 +199,13 @@ 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_SECONDARY 0x80000000
+#define STARTUP_APICID_CPUID_0B 0x40000000
+#define STARTUP_APICID_CPUID_01 0x20000000
+
#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3b7f4cdbf2e0..06adf340a0f1 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -115,6 +115,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 20d9a604da7c..ac1d7e5da1f2 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2377,7 +2377,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 222efd4a09bc..0e4e53d231db 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
@@ -241,6 +242,83 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
UNWIND_HINT_EMPTY
ANNOTATE_NOENDBR // above
+#ifdef CONFIG_SMP
+ /*
+ * 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), %edx
+ testl $STARTUP_SECONDARY, %edx
+ 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 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
+ */
+ 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
+
+.Luse_cpuid_01:
+ mov $0x01, %eax
+ cpuid
+ mov %ebx, %edx
+ shr $24, %edx
+ jmp .Lsetup_AP
+
+.Luse_cpuid_0b:
+ mov $0x0B, %eax
+ xorl %ecx, %ecx
+ cpuid
+
+.Lsetup_AP:
+ /* 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 %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 */
+ leaq __per_cpu_offset(%rip), %rbx
+ movq (%rbx,%rcx,8), %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(%rbx), %rcx
+ movq TASK_threadsp(%rcx), %rcx
+ movq %rcx, initial_stack(%rip)
+#endif /* CONFIG_SMP */
+
+.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
@@ -281,6 +359,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
@@ -372,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
@@ -426,6 +519,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
@@ -660,6 +754,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 b9366f86c433..50621793671d 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -797,6 +797,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 */
@@ -1084,8 +1094,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(pcpu_hot.top_of_stack, cpu) = task_top_of_stack(idle);
-#else
- initial_gs = per_cpu_offset(cpu);
#endif
return 0;
}
@@ -1110,9 +1118,14 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
start_ip = real_mode_header->trampoline_start64;
#endif
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 | apicid;
+ }
/* Enable the espfix hack for this CPU */
init_espfix_ap(cpu);
@@ -1512,6 +1525,47 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
speculative_store_bypass_ht_init();
+ /*
+ * We can do 64-bit AP bringup in parallel if the CPU reports
+ * its APIC ID in CPUID (either leaf 0x0B if we need the full
+ * APIC ID in X2APIC mode, or leaf 0x01 if 8 bits are
+ * sufficient). 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 < 1 ||
+ (x2apic_mode && boot_cpu_data.cpuid_level < 0xb) ||
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+ do_parallel_bringup = false;
+
+ if (do_parallel_bringup && x2apic_mode) {
+ unsigned int eax, ebx, ecx, edx;
+
+ /*
+ * To support parallel bringup in x2apic mode, the AP will need
+ * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
+ * only 8 bits. Check that it is present and seems correct.
+ */
+ cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
+
+ /*
+ * AMD says that if executed with an umimplemented level in
+ * ECX, then it will return all zeroes in EAX. Intel says it
+ * will return zeroes in both EAX and EBX. Checking only EAX
+ * should be sufficient.
+ */
+ if (eax) {
+ pr_debug("Using CPUID 0xb for parallel CPU startup\n");
+ smpboot_control = STARTUP_SECONDARY | STARTUP_APICID_CPUID_0B;
+ } else {
+ pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
+ do_parallel_bringup = false;
+ }
+ } else if (do_parallel_bringup) {
+ /* Without X2APIC, what's in CPUID 0x01 should suffice. */
+ pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
+ smpboot_control = STARTUP_SECONDARY | STARTUP_APICID_CPUID_01;
+ }
+
snp_set_wakeup_secondary_cpu();
}
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index af565816d2ba..788e5559549f 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 e38d61d6562e..49ebc1636ffd 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
@@ -241,6 +254,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 2c7396da470c..a18a21dff9bc 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.25.1
From: David Woodhouse <[email protected]>
There's no need to repeatedly save the BSP's MTRRs for each AP being
brought up at boot time. And no need to use smp_call_function_single()
even for the one time they do need to be saved.
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
---
arch/x86/kernel/cpu/mtrr/mtrr.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 783f3210d582..b6eae3ad4414 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -721,11 +721,20 @@ void __init mtrr_bp_init(void)
*/
void mtrr_save_state(void)
{
+ static bool mtrr_saved;
int first_cpu;
if (!mtrr_enabled())
return;
+ if (system_state < SYSTEM_RUNNING) {
+ if (!mtrr_saved) {
+ mtrr_save_fixed_ranges(NULL);
+ mtrr_saved = true;
+ }
+ return;
+ }
+
first_cpu = cpumask_first(cpu_online_mask);
smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
}
--
2.25.1
On Thu, Feb 09 2023 at 15:41, Usama Arif wrote:
>
> +static bool do_parallel_bringup = true;
Wants to be __ro_after_init
> +static int __init no_parallel_bringup(char *str)
> +{
> + do_parallel_bringup = false;
> +
> + return 0;
> +}
> +early_param("no_parallel_bringup", no_parallel_bringup);
Lacks an entry in Documentation/admin/kernel-parameters.txt
Thanks,
tglx
On Thu, Feb 09 2023 at 15:41, Usama Arif wrote:
> void mtrr_save_state(void)
> {
> + static bool mtrr_saved;
> int first_cpu;
>
> if (!mtrr_enabled())
> return;
>
> + if (system_state < SYSTEM_RUNNING) {
> + if (!mtrr_saved) {
> + mtrr_save_fixed_ranges(NULL);
> + mtrr_saved = true;
> + }
> + return;
> + }
> +
> first_cpu = cpumask_first(cpu_online_mask);
> smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
So why is this relevant after the initial bringup? The BP MTRRs have
been saved already above, no?
Thanks,
tglx
On 09/02/2023 18:31, Thomas Gleixner wrote:
> On Thu, Feb 09 2023 at 15:41, Usama Arif wrote:
>> void mtrr_save_state(void)
>> {
>> + static bool mtrr_saved;
>> int first_cpu;
>>
>> if (!mtrr_enabled())
>> return;
>>
>> + if (system_state < SYSTEM_RUNNING) {
>> + if (!mtrr_saved) {
>> + mtrr_save_fixed_ranges(NULL);
>> + mtrr_saved = true;
>> + }
>> + return;
>> + }
>> +
>> first_cpu = cpumask_first(cpu_online_mask);
>> smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
>
> So why is this relevant after the initial bringup? The BP MTRRs have
> been saved already above, no?
>
> Thanks,
>
> tglx
I will let David confirm if this is correct and why he did it, but this
is what I thought while reviewing before posting v4:
- At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state
is called in do_cpu_up at roughly the same time so MTRR is going to be
the same, we can just save it once and then reuse for other secondary
cores as it wouldn't have changed for the rest of the do_cpu_up calls.
- When the system is running and you offline and then online a CPU, you
want to make sure that hotplugged CPU gets the current MTRR (which might
have changed since boot?), incase the MTRR has changed after the system
has been booted, you save the MTRR of the first online CPU. When the
hotplugged CPU runs its initialisation code, its fixed-range MTRRs will
be updated with the newly saved fixed-range MTRRs.
So mainly for hotplug, but will let David confirm.
Thanks,
Usama
On Thu, 2023-02-09 at 20:32 +0000, Usama Arif wrote:
>
> I will let David confirm if this is correct and why he did it, but this
> is what I thought while reviewing before posting v4:
>
> - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state
> is called in do_cpu_up at roughly the same time so MTRR is going to be
> the same, we can just save it once and then reuse for other secondary
> cores as it wouldn't have changed for the rest of the do_cpu_up calls.
>
> - When the system is running and you offline and then online a CPU, you
> want to make sure that hotplugged CPU gets the current MTRR (which might
> have changed since boot?), incase the MTRR has changed after the system
> has been booted, you save the MTRR of the first online CPU. When the
> hotplugged CPU runs its initialisation code, its fixed-range MTRRs will
> be updated with the newly saved fixed-range MTRRs.
>
> So mainly for hotplug, but will let David confirm.
Sounds about right.
On 09/02/2023 18:25, Thomas Gleixner wrote:
> On Thu, Feb 09 2023 at 15:41, Usama Arif wrote:
>>
>> +static bool do_parallel_bringup = true;
>
> Wants to be __ro_after_init
>
>> +static int __init no_parallel_bringup(char *str)
>> +{
>> + do_parallel_bringup = false;
>> +
>> + return 0;
>> +}
>> +early_param("no_parallel_bringup", no_parallel_bringup);
>
> Lacks an entry in Documentation/admin/kernel-parameters.txt
>
> Thanks,
>
> tglx
Thanks, I will add the below to next revision. Its quite minor so will
wait for more comments and MTRR question to be resolved, unless told to
send next revision with this diff only.
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 6cfa6e3996cf..d3696c9316f1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3830,6 +3830,9 @@
noreplace-smp [X86-32,SMP] Don't replace SMP instructions
with UP alternatives
+ no_parallel_bringup
+ [X86,SMP] Disables parallel brinugp of secondary
cores.
+
noresume [SWSUSP] Disables resume and restores original swap
space.
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 3ec5182d9698..fecd934e72fb 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -813,7 +813,7 @@ static int __init cpu_init_udelay(char *str)
}
early_param("cpu_init_udelay", cpu_init_udelay);
-static bool do_parallel_bringup = true;
+static bool do_parallel_bringup __ro_after_init = true;
static int __init no_parallel_bringup(char *str)
{
On Thu, Feb 09 2023 at 20:32, Usama Arif wrote:
> On 09/02/2023 18:31, Thomas Gleixner wrote:
>>> first_cpu = cpumask_first(cpu_online_mask);
>>> smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
>>
>> So why is this relevant after the initial bringup? The BP MTRRs have
>> been saved already above, no?
>>
>
> I will let David confirm if this is correct and why he did it, but this
> is what I thought while reviewing before posting v4:
>
> - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state
> is called in do_cpu_up at roughly the same time so MTRR is going to be
> the same, we can just save it once and then reuse for other secondary
> cores as it wouldn't have changed for the rest of the do_cpu_up calls.
>
> - When the system is running and you offline and then online a CPU, you
> want to make sure that hotplugged CPU gets the current MTRR (which might
> have changed since boot?), incase the MTRR has changed after the system
> has been booted, you save the MTRR of the first online CPU. When the
> hotplugged CPU runs its initialisation code, its fixed-range MTRRs will
> be updated with the newly saved fixed-range MTRRs.
I knew that already :) But seriously:
If the MTRRs are changed post boot then the cached values want to be
updated too.
We are not making these changes just to satisfy some fast boot
challenge. They have to make sense in general.
And this does not amke sense at all.
Thanks,
tglx
On Thu, Feb 09, 2023 at 03:41:47PM +0000, Usama Arif wrote:
> The major change over v7 is fixing CPU0 hotplug not working as reported by
> Paul E. McKenney using rcu torture tests. This is fixed by setting up the
> initial_gs, initial_stack and early_gdt_descr properly for this case.
>
> The improvement in boot time is the same as v7.
This one passes moderate rcutorture testing.
Thanx, Paul
> 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.
> v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
> early_gdt_descr.
> Drop trampoline lock and bail if APIC ID not found in find_cpunr.
> Code comments improved and debug prints added.
>
> David Woodhouse (9):
> 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: Support parallel startup of secondary CPUs
> 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
>
> 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 | 99 +++++++-
> arch/x86/kernel/smpboot.c | 350 +++++++++++++++++++-------
> 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, 521 insertions(+), 160 deletions(-)
>
> --
> 2.25.1
>
On Fri, 2023-02-10 at 00:50 +0100, Thomas Gleixner wrote:
> On Thu, Feb 09 2023 at 20:32, Usama Arif wrote:
> > On 09/02/2023 18:31, Thomas Gleixner wrote:
> > > > first_cpu = cpumask_first(cpu_online_mask);
> > > > smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
> > >
> > > So why is this relevant after the initial bringup? The BP MTRRs have
> > > been saved already above, no?
> > >
> >
> > I will let David confirm if this is correct and why he did it, but this
> > is what I thought while reviewing before posting v4:
> >
> > - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state
> > is called in do_cpu_up at roughly the same time so MTRR is going to be
> > the same, we can just save it once and then reuse for other secondary
> > cores as it wouldn't have changed for the rest of the do_cpu_up calls.
> >
> > - When the system is running and you offline and then online a CPU, you
> > want to make sure that hotplugged CPU gets the current MTRR (which might
> > have changed since boot?), incase the MTRR has changed after the system
> > has been booted, you save the MTRR of the first online CPU. When the
> > hotplugged CPU runs its initialisation code, its fixed-range MTRRs will
> > be updated with the newly saved fixed-range MTRRs.
>
> I knew that already :) But seriously:
>
> If the MTRRs are changed post boot then the cached values want to be
> updated too.
They are, aren't they? The only way we come out of mtrr_save_state()
without calling mtrr_save_fixed_ranges() — either directly or via
smp_call_function_single() — is if they've already been saved once
*and* system_state < SYSTEM_RUNNING.
I suppose we could make that clearer by moving the definition of the
mtrr_saved flags inside the if (system_state < SYSTEM_RUNNING) block?
@@ -721,11 +721,20 @@ void __init mtrr_bp_init(void)
*/
void mtrr_save_state(void)
{
int first_cpu;
if (!mtrr_enabled())
return;
+ if (system_state < SYSTEM_RUNNING) {
+ static bool mtrr_saved;
+ if (!mtrr_saved) {
+ mtrr_save_fixed_ranges(NULL);
+ mtrr_saved = true;
+ }
+ return;
+ }
+
first_cpu = cpumask_first(cpu_online_mask);
smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
}
On Thu, 2023-02-09 at 20:11 -0800, Paul E. McKenney wrote:
> On Thu, Feb 09, 2023 at 03:41:47PM +0000, Usama Arif wrote:
> > The major change over v7 is fixing CPU0 hotplug not working as reported by
> > Paul E. McKenney using rcu torture tests. This is fixed by setting up the
> > initial_gs, initial_stack and early_gdt_descr properly for this case.
> >
> > The improvement in boot time is the same as v7.
>
> This one passes moderate rcutorture testing.
Thanks, Paul!
On 10/02/2023 08:55, David Woodhouse wrote:
> On Fri, 2023-02-10 at 00:50 +0100, Thomas Gleixner wrote:
>> On Thu, Feb 09 2023 at 20:32, Usama Arif wrote:
>>> On 09/02/2023 18:31, Thomas Gleixner wrote:
>>>>> first_cpu = cpumask_first(cpu_online_mask);
>>>>> smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
>>>>
>>>> So why is this relevant after the initial bringup? The BP MTRRs have
>>>> been saved already above, no?
>>>>
>>>
>>> I will let David confirm if this is correct and why he did it, but this
>>> is what I thought while reviewing before posting v4:
>>>
>>> - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state
>>> is called in do_cpu_up at roughly the same time so MTRR is going to be
>>> the same, we can just save it once and then reuse for other secondary
>>> cores as it wouldn't have changed for the rest of the do_cpu_up calls.
>>>
>>> - When the system is running and you offline and then online a CPU, you
>>> want to make sure that hotplugged CPU gets the current MTRR (which might
>>> have changed since boot?), incase the MTRR has changed after the system
>>> has been booted, you save the MTRR of the first online CPU. When the
>>> hotplugged CPU runs its initialisation code, its fixed-range MTRRs will
>>> be updated with the newly saved fixed-range MTRRs.
>>
>> I knew that already :) But seriously:
>>
>> If the MTRRs are changed post boot then the cached values want to be
>> updated too.
>
I had previously only done smpboot time measurements for the whole
patchset, but I tested the patchset without this commit and it doesn't
make a difference to smpboot time as its negligable work to read those
MTRR MSRs into mtrr_state.fixed_ranges.
This commit is also independent of parallel smp bringup, similar to
reusing timer calibration so I think it could be considered as a
separate patchset if needed. I will post the next revision without this
commit, but here is my view on MTRR save/restore (which shouldn't matter
for the next revision...).
If the MTRR changes on a running system, there might be a bug during
hotplug in the original code that handles MTRR? which is also carried
over in this patch.
From what I can see, MTRR is only saved+restored during initial boot,
hotplugging CPU and __save/__restore_processor_state() (used in creating
image for hibernation, suspend, kexec...). So if for e.g. in a running
system (that has not hibernated, suspended, kexeced), if MTRR for CPU0
(first_cpu) changed post-boot and CPU3 is hotplugged, only MTRR for CPU3
is updated and CPU0 and CPU3 will hold the same value, while the rest of
the CPUs will have the older first-boot value? This behavior will happen
with or without this patch. I think this is what Thomas is referring to
above when he says that the cached values want to be updated? But the
issue is present in the original code as well.
Thanks!
Usama
> They are, aren't they? The only way we come out of mtrr_save_state()
> without calling mtrr_save_fixed_ranges() — either directly or via
> smp_call_function_single() — is if they've already been saved once
> *and* system_state < SYSTEM_RUNNING.
>
> I suppose we could make that clearer by moving the definition of the
> mtrr_saved flags inside the if (system_state < SYSTEM_RUNNING) block?
>
> @@ -721,11 +721,20 @@ void __init mtrr_bp_init(void)
> */
> void mtrr_save_state(void)
> {
> int first_cpu;
>
> if (!mtrr_enabled())
> return;
>
> + if (system_state < SYSTEM_RUNNING) {
> + static bool mtrr_saved;
> + if (!mtrr_saved) {
> + mtrr_save_fixed_ranges(NULL);
> + mtrr_saved = true;
> + }
> + return;
> + }
> +
> first_cpu = cpumask_first(cpu_online_mask);
> smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
> }
>
On Mon, Feb 13 2023 at 15:19, Usama Arif wrote:
> On 10/02/2023 08:55, David Woodhouse wrote:
>> On Fri, 2023-02-10 at 00:50 +0100, Thomas Gleixner wrote:
>>> I knew that already :) But seriously:
>>>
>>> If the MTRRs are changed post boot then the cached values want to be
>>> updated too.
>>
> I had previously only done smpboot time measurements for the whole
> patchset, but I tested the patchset without this commit and it doesn't
> make a difference to smpboot time as its negligable work to read those
> MTRR MSRs into mtrr_state.fixed_ranges.
> This commit is also independent of parallel smp bringup, similar to
> reusing timer calibration so I think it could be considered as a
> separate patchset if needed. I will post the next revision without this
> commit, but here is my view on MTRR save/restore (which shouldn't matter
> for the next revision...).
Correct. It's an orthogonal problem and not a prerequisite for the
initial parallel bringup.
> If the MTRR changes on a running system, there might be a bug during
> hotplug in the original code that handles MTRR? which is also carried
> over in this patch.
> From what I can see, MTRR is only saved+restored during initial boot,
> hotplugging CPU and __save/__restore_processor_state() (used in creating
> image for hibernation, suspend, kexec...). So if for e.g. in a running
> system (that has not hibernated, suspended, kexeced), if MTRR for CPU0
> (first_cpu) changed post-boot and CPU3 is hotplugged, only MTRR for CPU3
> is updated and CPU0 and CPU3 will hold the same value, while the rest of
> the CPUs will have the older first-boot value? This behavior will happen
> with or without this patch. I think this is what Thomas is referring to
> above when he says that the cached values want to be updated? But the
> issue is present in the original code as well.
No. Changing a MTTR is done via stop_machine() on all online CPUs in
parallel. So there is no bug.
But its stupid that an update to a MTRR is not updating the cached
value, which results in this sillyness of reading back the real values
into the cache on CPU hotplug in order to set them up correctly on the
upcoming CPU.
So the real improvement would be to:
1) Cache the MTRRs on the boot CPU once during early boot
2) When MTRRs are changed then update the cache too
That ensures that the MTRR cache is always up to date and can be
utilized on CPU hotplug.
The 'if (state < RUNNING)' thing is just a sloppy hack which tries to
save a few cycles at boot.
Thanks,
tglx