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 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.
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
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 | 71 ++++++++
arch/x86/kernel/smpboot.c | 324 ++++++++++++++++++++++++----------
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, 441 insertions(+), 159 deletions(-)
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.
[ dwmw2: Minor tweaks, write a commit message ]
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 | 71 ++++++++++++++++++++++++++++
arch/x86/kernel/smpboot.c | 14 +++++-
arch/x86/realmode/init.c | 3 ++
arch/x86/realmode/rm/trampoline_64.S | 14 ++++++
kernel/smpboot.c | 2 +-
9 files changed, 114 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index 5db5d083c873..e1cc4bc746bc 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..ca807c29dc34 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_USE_APICID 0x10000
+#define STARTUP_USE_CPUID_0B 0x20000
+
#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3f85fcae450c..9598ebf4f9d6 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 d8b3ebd2bb85..0249212e23d2 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
@@ -176,6 +177,64 @@ 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-15 APICID if STARTUP_USE_CPUID_0B is not set
+ * Bit 16 Secondary boot flag
+ * Bit 17 Parallel boot flag
+ */
+ testl $STARTUP_USE_CPUID_0B, %eax
+ jz .Lsetup_AP
+
+ mov $0x0B, %eax
+ xorl %ecx, %ecx
+ cpuid
+ mov %edx, %eax
+
+.Lsetup_AP:
+ /* EAX contains the APICID of the current CPU */
+ andl $0xFFFF, %eax
+ 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
@@ -216,6 +275,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
@@ -347,6 +414,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
@@ -572,6 +640,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 7a763b84b6e5..1e38d44c3603 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1104,9 +1104,19 @@ 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 (boot_cpu_data.cpuid_level < 0x0B) {
+ /* Anything with X2APIC should have CPUID leaf 0x0B */
+ if (WARN_ON_ONCE(x2apic_mode) && apicid > 0xffff)
+ return -EIO;
+ smpboot_control = apicid | STARTUP_USE_APICID;
+ } else {
+ smpboot_control = STARTUP_USE_CPUID_0B;
+ }
/* Enable the espfix hack for this CPU */
init_espfix_ap(cpu);
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index 4a3da7592b99..7dc2e817bd02 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -127,6 +127,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);
trampoline_pgd[0] = trampoline_pgd_entry.pgd;
trampoline_pgd[511] = init_top_pgt[511].pgd;
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.31.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.31.1
From: David Woodhouse <[email protected]>
If the platform registers these states, bring all CPUs to each registered
state in turn, before the final bringup to CPUHP_BRINGUP_CPU. This allows
the architecture to parallelise the slow asynchronous tasks like sending
INIT/SIPI and waiting for the AP to come to life.
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.
We believe that X2APIC was the only such case, for x86. But this is why
it remains an architecture opt-in. For now.
Note that the new parallel stages do *not* yet bring each AP to the
CPUHP_BRINGUP_CPU state. 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]>
---
include/linux/cpuhotplug.h | 2 ++
kernel/cpu.c | 27 +++++++++++++++++++++++++--
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 773c83730906..45c327538321 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -131,6 +131,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 192e43a87407..1a46eb57d8f7 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1462,6 +1462,24 @@ int bringup_hibernate_cpu(unsigned int sleep_cpu)
void bringup_nonboot_cpus(unsigned int setup_max_cpus)
{
unsigned int cpu;
+ int n = setup_max_cpus - num_online_cpus();
+
+ /* ∀ parallel pre-bringup state, bring N CPUs to it */
+ 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)
@@ -1829,6 +1847,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;
}
@@ -1853,14 +1875,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.31.1
From: David Woodhouse <[email protected]>
If we bring up secondaries in parallel they might get confused unless we
impose some ordering here:
[ 1.360149] x86: Booting SMP configuration:
[ 1.360221] .... node #0, CPUs: #1 #2 #3 #4 #5 #6 #7 #8 #9 #10 #11 #12 #13 #14 #15 #16 #17 #18 #19 #20 #21 #22 #23
[ 1.366225] .... node #1, CPUs: #24 #25 #26 #27 #28 #29 #30 #31 #32 #33 #34 #35 #36 #37 #38 #39 #40 #41 #42 #43 #44 #45 #46 #47
[ 1.370219] .... node #0, CPUs: #48 #49 #50 #51 #52 #53 #54 #55 #56 #57 #58 #59 #60 #61 #62 #63 #64 #65 #66 #67 #68 #69 #70 #71
[ 1.378226] .... node #1, CPUs: #72 #73 #74 #75 #76 #77 #78 #79 #80 #81 #82 #83 #84 #85 #86 #87 #88 #89 #90 #91 #92 #93 #94 #95
[ 1.382037] Brought 96 CPUs to x86/cpu:kick in 72232606 cycles
[ 0.104104] smpboot: CPU 26 Converting physical 0 to logical die 1
[ 0.104104] smpboot: CPU 27 Converting physical 1 to logical package 2
[ 0.104104] smpboot: CPU 24 Converting physical 1 to logical package 3
[ 0.104104] smpboot: CPU 27 Converting physical 0 to logical die 2
[ 0.104104] smpboot: CPU 25 Converting physical 1 to logical package 4
[ 1.385609] Brought 96 CPUs to x86/cpu:wait-init in 9269218 cycles
[ 1.395285] Brought CPUs online in 28930764 cycles
[ 1.395469] smp: Brought up 2 nodes, 96 CPUs
[ 1.395689] smpboot: Max logical packages: 2
[ 1.396222] smpboot: Total of 96 processors activated (576000.00 BogoMIPS)
Do the full topology update in smp_store_cpu_info() under a spinlock
to ensure that things remain consistent.
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/include/asm/smp.h | 4 +-
arch/x86/include/asm/topology.h | 2 -
arch/x86/kernel/cpu/common.c | 6 +--
arch/x86/kernel/smpboot.c | 73 ++++++++++++++++++++-------------
arch/x86/xen/smp_pv.c | 4 +-
5 files changed, 48 insertions(+), 41 deletions(-)
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ca807c29dc34..0b6012fd3e55 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -59,8 +59,6 @@ struct smp_ops {
};
/* Globals due to paravirt */
-extern void set_cpu_sibling_map(int cpu);
-
#ifdef CONFIG_SMP
extern struct smp_ops smp_ops;
@@ -148,7 +146,7 @@ void native_send_call_func_single_ipi(int cpu);
void x86_idle_thread_init(unsigned int cpu, struct task_struct *idle);
void smp_store_boot_cpu_info(void);
-void smp_store_cpu_info(int id);
+void smp_store_cpu_info(int id, bool force_single_core);
asmlinkage __visible void smp_reboot_interrupt(void);
__visible void smp_reschedule_interrupt(struct pt_regs *regs);
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index cc164777e661..4fe2591f5d8d 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -135,8 +135,6 @@ static inline int topology_max_smt_threads(void)
return __max_smt_threads;
}
-int topology_update_package_map(unsigned int apicid, unsigned int cpu);
-int topology_update_die_map(unsigned int dieid, unsigned int cpu);
int topology_phys_to_logical_pkg(unsigned int pkg);
int topology_phys_to_logical_die(unsigned int die, unsigned int cpu);
bool topology_is_primary_thread(unsigned int cpu);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0083464de5e3..98b4697b2c9b 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1517,7 +1517,7 @@ static void generic_identify(struct cpuinfo_x86 *c)
* Validate that ACPI/mptables have the same information about the
* effective APIC id and update the package map.
*/
-static void validate_apic_and_package_id(struct cpuinfo_x86 *c)
+static void validate_apic_id(struct cpuinfo_x86 *c)
{
#ifdef CONFIG_SMP
unsigned int apicid, cpu = smp_processor_id();
@@ -1528,8 +1528,6 @@ static void validate_apic_and_package_id(struct cpuinfo_x86 *c)
pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x APIC: %x\n",
cpu, apicid, c->initial_apicid);
}
- BUG_ON(topology_update_package_map(c->phys_proc_id, cpu));
- BUG_ON(topology_update_die_map(c->cpu_die_id, cpu));
#else
c->logical_proc_id = 0;
#endif
@@ -1716,7 +1714,7 @@ void identify_secondary_cpu(struct cpuinfo_x86 *c)
enable_sep_cpu();
#endif
mtrr_ap_init();
- validate_apic_and_package_id(c);
+ validate_apic_id(c);
x86_spec_ctrl_setup_ap();
update_srbds_msr();
}
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index d194116305a7..725fede281ac 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -190,16 +190,12 @@ static void smp_callin(void)
apic_ap_setup();
/*
- * Save our processor parameters. Note: this information
- * is needed for clock calibration.
- */
- smp_store_cpu_info(cpuid);
-
- /*
+ * Save our processor parameters and update topology.
+ * Note: this information is needed for clock calibration.
* The topology information must be up to date before
* calibrate_delay() and notify_cpu_starting().
*/
- set_cpu_sibling_map(raw_smp_processor_id());
+ smp_store_cpu_info(cpuid, false);
init_freq_invariance(true, false);
@@ -254,6 +250,12 @@ static void notrace start_secondary(void *unused)
* smp_callout_mask to release them.
*/
cpu_init_secondary();
+
+ /*
+ * Even though notify_cpu_starting() will do this, it does so too late
+ * as the AP may already have triggered lockdep splats by then. See
+ * commit 29368e093 ("x86/smpboot: Move rcu_cpu_starting() earlier").
+ */
rcu_cpu_starting(raw_smp_processor_id());
x86_cpuinit.early_percpu_clock_init();
@@ -362,7 +364,7 @@ EXPORT_SYMBOL(topology_phys_to_logical_die);
* @pkg: The physical package id as retrieved via CPUID
* @cpu: The cpu for which this is updated
*/
-int topology_update_package_map(unsigned int pkg, unsigned int cpu)
+static int topology_update_package_map(unsigned int pkg, unsigned int cpu)
{
int new;
@@ -385,7 +387,7 @@ int topology_update_package_map(unsigned int pkg, unsigned int cpu)
* @die: The die id as retrieved via CPUID
* @cpu: The cpu for which this is updated
*/
-int topology_update_die_map(unsigned int die, unsigned int cpu)
+static int topology_update_die_map(unsigned int die, unsigned int cpu)
{
int new;
@@ -416,25 +418,7 @@ void __init smp_store_boot_cpu_info(void)
c->initialized = true;
}
-/*
- * The bootstrap kernel entry code has set these up. Save them for
- * a given CPU
- */
-void smp_store_cpu_info(int id)
-{
- struct cpuinfo_x86 *c = &cpu_data(id);
-
- /* Copy boot_cpu_data only on the first bringup */
- if (!c->initialized)
- *c = boot_cpu_data;
- c->cpu_index = id;
- /*
- * During boot time, CPU0 has this setup already. Save the info when
- * bringing up AP or offlined CPU0.
- */
- identify_secondary_cpu(c);
- c->initialized = true;
-}
+static arch_spinlock_t topology_lock = __ARCH_SPIN_LOCK_UNLOCKED;
static bool
topology_same_node(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
@@ -629,7 +613,7 @@ static struct sched_domain_topology_level x86_topology[] = {
*/
static bool x86_has_numa_in_package;
-void set_cpu_sibling_map(int cpu)
+static void set_cpu_sibling_map(int cpu)
{
bool has_smt = smp_num_siblings > 1;
bool has_mp = has_smt || boot_cpu_data.x86_max_cores > 1;
@@ -708,6 +692,37 @@ void set_cpu_sibling_map(int cpu)
}
}
+/*
+ * The bootstrap kernel entry code has set these up. Save them for
+ * a given CPU
+ */
+void smp_store_cpu_info(int id, bool force_single_core)
+{
+ struct cpuinfo_x86 *c = &cpu_data(id);
+
+ /* Copy boot_cpu_data only on the first bringup */
+ if (!c->initialized)
+ *c = boot_cpu_data;
+ c->cpu_index = id;
+ /*
+ * During boot time, CPU0 has this setup already. Save the info when
+ * bringing up AP or offlined CPU0.
+ */
+ identify_secondary_cpu(c);
+
+ arch_spin_lock(&topology_lock);
+ BUG_ON(topology_update_package_map(c->phys_proc_id, id));
+ BUG_ON(topology_update_die_map(c->cpu_die_id, id));
+ c->initialized = true;
+
+ /* For Xen PV */
+ if (force_single_core)
+ c->x86_max_cores = 1;
+
+ set_cpu_sibling_map(id);
+ arch_spin_unlock(&topology_lock);
+}
+
/* maps the cpu to the sched domain representing multi-core */
const struct cpumask *cpu_coregroup_mask(int cpu)
{
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 6a8f3b53ab83..ff37dff20dc0 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -71,9 +71,7 @@ static void cpu_bringup(void)
xen_enable_syscall();
}
cpu = smp_processor_id();
- smp_store_cpu_info(cpu);
- cpu_data(cpu).x86_max_cores = 1;
- set_cpu_sibling_map(cpu);
+ smp_store_cpu_info(cpu, true);
speculative_store_bypass_ht_init();
--
2.31.1
From: David Woodhouse <[email protected]>
If we want to do parallel CPU bringup, we're going to need to set this up
and leave it until all CPUs are done. Might as well use the RTC spinlock
to protect the refcount, as we need to take it anyway.
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/kernel/smpboot.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ac2909f0cab3..99c705935f94 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -127,17 +127,22 @@ 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)
@@ -149,10 +154,12 @@ static inline void smpboot_restore_warm_reset_vector(void)
* to default values.
*/
spin_lock_irqsave(&rtc_lock, flags);
- CMOS_WRITE(0, 0xf);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ if (!--smpboot_warm_reset_vector_count) {
+ CMOS_WRITE(0, 0xf);
- *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
+ *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
+ }
+ spin_unlock_irqrestore(&rtc_lock, flags);
}
static void init_freq_invariance(bool secondary, bool cppc_ready);
--
2.31.1
On 12/15/21 8:56 AM, David Woodhouse wrote:
> From: Thomas Gleixner <[email protected]>
>
...
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index d8b3ebd2bb85..0249212e23d2 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
> @@ -176,6 +177,64 @@ 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-15 APICID if STARTUP_USE_CPUID_0B is not set
> + * Bit 16 Secondary boot flag
> + * Bit 17 Parallel boot flag
> + */
> + testl $STARTUP_USE_CPUID_0B, %eax
> + jz .Lsetup_AP
> +
> + mov $0x0B, %eax
> + xorl %ecx, %ecx
> + cpuid
This will break an SEV-ES guest because CPUID will generate a #VC and a
#VC handler has not been established yet.
I guess for now, you can probably just not enable parallel startup for
SEV-ES guests.
Thanks,
Tom
> + mov %edx, %eax
> +
> +.Lsetup_AP:
> + /* EAX contains the APICID of the current CPU */
> + andl $0xFFFF, %eax
> + 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
> @@ -216,6 +275,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
> @@ -347,6 +414,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
> @@ -572,6 +640,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 7a763b84b6e5..1e38d44c3603 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1104,9 +1104,19 @@ 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 (boot_cpu_data.cpuid_level < 0x0B) {
> + /* Anything with X2APIC should have CPUID leaf 0x0B */
> + if (WARN_ON_ONCE(x2apic_mode) && apicid > 0xffff)
> + return -EIO;
> + smpboot_control = apicid | STARTUP_USE_APICID;
> + } else {
> + smpboot_control = STARTUP_USE_CPUID_0B;
> + }
>
> /* Enable the espfix hack for this CPU */
> init_espfix_ap(cpu);
> diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
> index 4a3da7592b99..7dc2e817bd02 100644
> --- a/arch/x86/realmode/init.c
> +++ b/arch/x86/realmode/init.c
> @@ -127,6 +127,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);
> trampoline_pgd[0] = trampoline_pgd_entry.pgd;
> trampoline_pgd[511] = init_top_pgt[511].pgd;
> 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)
> {
>
On 12/15/21 8:56 AM, 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 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.
I applied this series and began booting a regular non-SEV guest and hit a
failure at 39 vCPUs. No panic or warning, just a reset and OVMF was
executing again. I'll try to debug what's going, but not sure how quickly
I'll arrive at anything.
Thanks,
Tom
On Thu, 2021-12-16 at 08:24 -0600, Tom Lendacky wrote:
> This will break an SEV-ES guest because CPUID will generate a #VC and a
> #VC handler has not been established yet.
>
> I guess for now, you can probably just not enable parallel startup for
> SEV-ES guests.
OK, thanks. I'll expand it to allow 24 bits of (physical) APIC ID then,
since it's no longer limited to CPUs without X2APIC. Then we can
refrain from doing parallel bringup for SEV-ES guests, as you suggest.
What precisely is the check I should be using for that?
On 12/16/21 12:24 PM, David Woodhouse wrote:
> On Thu, 2021-12-16 at 08:24 -0600, Tom Lendacky wrote:
>
>> This will break an SEV-ES guest because CPUID will generate a #VC and a
>> #VC handler has not been established yet.
>>
>> I guess for now, you can probably just not enable parallel startup for
>> SEV-ES guests.
>
> OK, thanks. I'll expand it to allow 24 bits of (physical) APIC ID then,
> since it's no longer limited to CPUs without X2APIC. Then we can
> refrain from doing parallel bringup for SEV-ES guests, as you suggest.
>
> What precisely is the check I should be using for that?
Calling cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) will return true for
an SEV-ES guest.
Thanks,
Tom
>
On Thu, 2021-12-16 at 13:00 -0600, Tom Lendacky wrote:
> On 12/16/21 12:24 PM, David Woodhouse wrote:
> > On Thu, 2021-12-16 at 08:24 -0600, Tom Lendacky wrote:
> >
> > > This will break an SEV-ES guest because CPUID will generate a #VC and a
> > > #VC handler has not been established yet.
> > >
> > > I guess for now, you can probably just not enable parallel startup for
> > > SEV-ES guests.
> >
> > OK, thanks. I'll expand it to allow 24 bits of (physical) APIC ID then,
> > since it's no longer limited to CPUs without X2APIC. Then we can
> > refrain from doing parallel bringup for SEV-ES guests, as you suggest.
> >
> > What precisely is the check I should be using for that?
>
> Calling cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) will return true for
> an SEV-ES guest.
Thanks. Incremental patch (which I'll roll into Thomas's patch) looks a
bit like this. Testing it now...
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 0b6012fd3e55..1ac33ce1d60e 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -199,7 +199,6 @@ extern unsigned int smpboot_control;
#endif /* !__ASSEMBLY__ */
/* Control bits for startup_64 */
-#define STARTUP_USE_APICID 0x10000
-#define STARTUP_USE_CPUID_0B 0x20000
+#define STARTUP_PARALLEL 0x80000000
#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 0249212e23d2..3e4c3c416bce 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -189,11 +189,10 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
* 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-15 APICID if STARTUP_USE_CPUID_0B is not set
- * Bit 16 Secondary boot flag
- * Bit 17 Parallel boot flag
+ * Bit 0-30 APIC ID if STARTUP_PARALLEL is not set
+ * Bit 31 Parallel boot flag (use CPUID leaf 0x0b for APIC ID).
*/
- testl $STARTUP_USE_CPUID_0B, %eax
+ testl $STARTUP_PARALLEL, %eax
jz .Lsetup_AP
mov $0x0B, %eax
@@ -203,7 +202,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
.Lsetup_AP:
/* EAX contains the APICID of the current CPU */
- andl $0xFFFF, %eax
xorl %ecx, %ecx
leaq cpuid_to_apicid(%rip), %rbx
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 725fede281ac..acfb22ce8d4f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1125,13 +1125,10 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
if (IS_ENABLED(CONFIG_X86_32)) {
early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
initial_stack = idle->thread.sp;
- } else if (boot_cpu_data.cpuid_level < 0x0B) {
- /* Anything with X2APIC should have CPUID leaf 0x0B */
- if (WARN_ON_ONCE(x2apic_mode) && apicid > 0xffff)
- return -EIO;
- smpboot_control = apicid | STARTUP_USE_APICID;
+ } else if (do_parallel_bringup) {
+ smpboot_control = STARTUP_PARALLEL;
} else {
- smpboot_control = STARTUP_USE_CPUID_0B;
+ smpboot_control = apicid;
}
/* Enable the espfix hack for this CPU */
@@ -1553,9 +1550,11 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
/*
* 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.
+ * 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)
+ if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 0x0B ||
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
do_parallel_bringup = false;
if (do_parallel_bringup)
On Thu, 2021-12-16 at 10:27 -0600, Tom Lendacky wrote:
> On 12/15/21 8:56 AM, 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 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.
>
> I applied this series and began booting a regular non-SEV guest and hit a
> failure at 39 vCPUs. No panic or warning, just a reset and OVMF was
> executing again. I'll try to debug what's going, but not sure how quickly
> I'll arrive at anything.
Thanks for testing. This is working for me with BIOS and EFI boots in
qemu and real hardware but it's mostly been Intel so far. I'll try
harder on an AMD box.
Anything else special about your setup, kernel config or qemu
invocation that might help me reproduce?
If it can repro without KVM, 'qemu -d in_asm' can be extremely useful
for this kind of thing btw.
On Thu, 2021-12-16 at 10:27 -0600, Tom Lendacky wrote:
> On 12/15/21 8:56 AM, 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 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.
>
> I applied this series and began booting a regular non-SEV guest and hit a
> failure at 39 vCPUs. No panic or warning, just a reset and OVMF was
> executing again. I'll try to debug what's going, but not sure how quickly
> I'll arrive at anything.
I've pushed the SEV-ES fix to
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.16
and in doing so I've moved the 'no_parallel_bringup' command line
argument earlier in the series, to Thomas's "Support parallel startup
of secondary CPUs" commit (now 191f0899757). It would be interesting to
see if you can reproduce with just that much, both with and with
no_parallel_bringup. And then whether the subsequent commit that
actually enables the parallel INIT/SIPI/SIPI actually makes the
difference?
Thanks!
On 12/16/21 1:52 PM, David Woodhouse wrote:
> On Thu, 2021-12-16 at 10:27 -0600, Tom Lendacky wrote:
>> On 12/15/21 8:56 AM, 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 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.
>>
>> I applied this series and began booting a regular non-SEV guest and hit a
>> failure at 39 vCPUs. No panic or warning, just a reset and OVMF was
>> executing again. I'll try to debug what's going, but not sure how quickly
>> I'll arrive at anything.
>
> I've pushed the SEV-ES fix to
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.16
> and in doing so I've moved the 'no_parallel_bringup' command line
> argument earlier in the series, to Thomas's "Support parallel startup
> of secondary CPUs" commit (now 191f0899757). It would be interesting to
> see if you can reproduce with just that much, both with and with
> no_parallel_bringup. And then whether the subsequent commit that
> actually enables the parallel INIT/SIPI/SIPI actually makes the
> difference?
>
I'll pull it down and give it try.
Thanks,
Tom
> Thanks!
>
On 16 December 2021 19:55:36 GMT, Tom Lendacky <[email protected]> wrote:
>On 12/16/21 1:52 PM, David Woodhouse wrote:
>> On Thu, 2021-12-16 at 10:27 -0600, Tom Lendacky wrote:
>>> On 12/15/21 8:56 AM, 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 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.
>>>
>>> I applied this series and began booting a regular non-SEV guest and hit a
>>> failure at 39 vCPUs. No panic or warning, just a reset and OVMF was
>>> executing again. I'll try to debug what's going, but not sure how quickly
>>> I'll arrive at anything.
>>
>> I've pushed the SEV-ES fix to
>> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.16
>> and in doing so I've moved the 'no_parallel_bringup' command line
>> argument earlier in the series, to Thomas's "Support parallel startup
>> of secondary CPUs" commit (now 191f0899757). It would be interesting to
>> see if you can reproduce with just that much, both with and with
>> no_parallel_bringup. And then whether the subsequent commit that
>> actually enables the parallel INIT/SIPI/SIPI actually makes the
>> difference?
>>
>
>I'll pull it down and give it try.
Thanks. Note: don't use the whole thing; that last "parallel part2" patch in particular isn't ready. And probably isn't where the next low-hanging fruit is anyway.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On 12/16/21 1:24 PM, David Woodhouse wrote:
> On Thu, 2021-12-16 at 10:27 -0600, Tom Lendacky wrote:
>> On 12/15/21 8:56 AM, 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 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.
>>
>> I applied this series and began booting a regular non-SEV guest and hit a
>> failure at 39 vCPUs. No panic or warning, just a reset and OVMF was
>> executing again. I'll try to debug what's going, but not sure how quickly
>> I'll arrive at anything.
>
> Thanks for testing. This is working for me with BIOS and EFI boots in
> qemu and real hardware but it's mostly been Intel so far. I'll try
> harder on an AMD box.
On baremetal, I haven't seen an issue. This only seems to have a problem
with Qemu/KVM.
With 191f08997577 I could boot without issues with and without the
no_parallel_bringup. Only after I applied e78fa57dd642 did the failure happen.
With e78fa57dd642 I could boot 64 vCPUs pretty consistently, but when I
jumped to 128 vCPUs it failed again. When I moved the series to
df9726cb7178, then 64 vCPUs also failed pretty consistently.
Strange thing is it is random. Sometimes (rarely) it works on the first
boot and then sometimes it doesn't, at which point it will reset and
reboot 3 or 4 times and then make it past the failure and fully boot.
>
> Anything else special about your setup, kernel config or qemu
> invocation that might help me reproduce?
Shouldn't be anything special that I'm aware of:
- EPYC 3rd Gen (Milan)
- Qemu 6.1.0
- OVMF edk2-stable202111
The qemu command line is:
qemu-system-x86_64 -enable-kvm -cpu EPYC,host-phys-bits=true -smp 128 -m
1G -machine type=q35 -drive
if=pflash,format=raw,unit=0,file=/root/kernels/qemu-install/OVMF_CODE.fd,readonly=on
-drive if=pflash,format=raw,unit=1,file=./diskless.fd -nographic -kernel
/root/kernels/linux-build-x86_64/arch/x86/boot/bzImage -append
"console=ttyS0,115200n8" -monitor pty -monitor unix:monitor,server,nowait
I can send the kernel config to you offlist if you're unable to repro with
yours.
>
> If it can repro without KVM, 'qemu -d in_asm' can be extremely useful
> for this kind of thing btw.
I didn't repro the failure without KVM.
Thanks,
Tom
>
On Thu, 2021-12-16 at 16:52 -0600, Tom Lendacky wrote:
> On baremetal, I haven't seen an issue. This only seems to have a problem
> with Qemu/KVM.
>
> With 191f08997577 I could boot without issues with and without the
> no_parallel_bringup. Only after I applied e78fa57dd642 did the failure happen.
>
> With e78fa57dd642 I could boot 64 vCPUs pretty consistently, but when I
> jumped to 128 vCPUs it failed again. When I moved the series to
> df9726cb7178, then 64 vCPUs also failed pretty consistently.
>
> Strange thing is it is random. Sometimes (rarely) it works on the first
> boot and then sometimes it doesn't, at which point it will reset and
> reboot 3 or 4 times and then make it past the failure and fully boot.
Hm, some of that is just artifacts of timing, I'm sure. But now I'm
staring at the way that early_setup_idt() can run in parallel on all
CPUs, rewriting bringup_idt_descr and loading it.
To start with, let's try unlocking the trampoline_lock much later,
after cpu_init_exception_handling() has loaded the real IDT.
I think we can probably make secondaries load the real IDT early and
never use bringup_idt_descr at all, can't we? But let's see if this
makes it go away, to start with...
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0cd6373bc3f2..2307f7575ab4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -59,7 +59,7 @@
#include <asm/cpu_device_id.h>
#include <asm/uv/uv.h>
#include <asm/sigframe.h>
-
+#include <asm/realmode.h>
#include "cpu.h"
u32 elf_hwcap2 __read_mostly;
@@ -2060,6 +2060,7 @@ void cpu_init_secondary(void)
* on this CPU in cpu_init_exception_handling().
*/
cpu_init_exception_handling();
+ clear_bit(0, (unsigned long *)trampoline_lock);
cpu_init();
}
#endif
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 3e4c3c416bce..db01b56574cd 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -273,14 +273,6 @@ 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
On Fri, 17 Dec 2021 00:13:16 +0000
David Woodhouse <[email protected]> wrote:
> On Thu, 2021-12-16 at 16:52 -0600, Tom Lendacky wrote:
> > On baremetal, I haven't seen an issue. This only seems to have a problem
> > with Qemu/KVM.
> >
> > With 191f08997577 I could boot without issues with and without the
> > no_parallel_bringup. Only after I applied e78fa57dd642 did the failure happen.
> >
> > With e78fa57dd642 I could boot 64 vCPUs pretty consistently, but when I
> > jumped to 128 vCPUs it failed again. When I moved the series to
> > df9726cb7178, then 64 vCPUs also failed pretty consistently.
> >
> > Strange thing is it is random. Sometimes (rarely) it works on the first
> > boot and then sometimes it doesn't, at which point it will reset and
> > reboot 3 or 4 times and then make it past the failure and fully boot.
>
> Hm, some of that is just artifacts of timing, I'm sure. But now I'm
that's most likely the case (there is a race somewhere left).
To trigger CPU bringup (hotplug) races, I used to run QEMU guest with
heavy vCPU overcommit. It helps to induce unexpected delays at CPU bringup
time.
> staring at the way that early_setup_idt() can run in parallel on all
> CPUs, rewriting bringup_idt_descr and loading it.
>
> To start with, let's try unlocking the trampoline_lock much later,
> after cpu_init_exception_handling() has loaded the real IDT.
>
> I think we can probably make secondaries load the real IDT early and
> never use bringup_idt_descr at all, can't we? But let's see if this
> makes it go away, to start with...
>
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 0cd6373bc3f2..2307f7575ab4 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -59,7 +59,7 @@
> #include <asm/cpu_device_id.h>
> #include <asm/uv/uv.h>
> #include <asm/sigframe.h>
> -
> +#include <asm/realmode.h>
> #include "cpu.h"
>
> u32 elf_hwcap2 __read_mostly;
> @@ -2060,6 +2060,7 @@ void cpu_init_secondary(void)
> * on this CPU in cpu_init_exception_handling().
> */
> cpu_init_exception_handling();
> + clear_bit(0, (unsigned long *)trampoline_lock);
> cpu_init();
> }
> #endif
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 3e4c3c416bce..db01b56574cd 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -273,14 +273,6 @@ 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
On Fri, 2021-12-17 at 11:09 +0100, Igor Mammedov wrote:
> that's most likely the case (there is a race somewhere left).
> To trigger CPU bringup (hotplug) races, I used to run QEMU guest with
> heavy vCPU overcommit. It helps to induce unexpected delays at CPU bringup
> time.
Yeah, I've been doing a fair amount of that but even with Tom's config
I can't reproduce a crash. Have seen this one now though. It's hard to
reproduce, and I suspect it was there already and I've only tweaked the
timing to expose it (or not even that, and just done enough tests that
I've seen it when it's extremely sporadic).
[ 0.061937] kvm-clock: cpu 24, msr 31801601, secondary cpu clock
[ 0.668842] kvm-guest: stealtime: cpu 24, msr 37c31080
[ 0.061937] kvm-clock: cpu 25, msr 31801641, secondary cpu clock
[ 0.670557] kvm-guest: stealtime: cpu 25, msr 37c71080
[ 0.670557] ------------[ cut here ]------------
[ 0.670557] cfs_rq->avg.load_avg || cfs_rq->avg.util_avg || cfs_rq->avg.runnable_avg
[ 0.670557] WARNING: CPU: 25 PID: 140 at kernel/sched/fair.c:3299 __update_blocked_fair+0x4b7/0x4d0
[ 0.061937] kvm-clock: cpu 26, msr 31801681, secondary cpu clock
[ 0.670740] Modules linked in:
[ 0.670740] CPU: 25 PID: 140 Comm: kworker/25:0H Not tainted 5.16.0-rc2-sos-testing+ #963
[ 0.670740] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[ 0.670740] RIP: 0010:__update_blocked_fair+0x4b7/0x4d0
[ 0.670740] Code: 4f fd ff ff 49 8b 96 48 01 00 00 48 89 90 60 09 00 00 e9 e3 fc ff ff 48 c7 c7 30 36 34 b6 c6 05 77 78 ae 01 01 e8 d5 58 96 00 <0f> 0b 41 8b 86 38 01 00 00 e9 aa fc ff ff 66 66 2e 0f 1f 84 00 00
[ 0.670740] RSP: 0018:ffffc90000cc7d30 EFLAGS: 00010086
[ 0.670740] RAX: 0000000000000000 RBX: 00000000000000c8 RCX: 0000000000000000
[ 0.670740] RDX: 0000000000000003 RSI: ffff88803bbfffe8 RDI: 00000000ffffffff
[ 0.670740] RBP: ffff888037c6f800 R08: 00000000ffffffea R09: 0000000000000000
[ 0.670740] R10: 0000000000000003 R11: 3fffffffffffffff R12: ffff888037c6ff90
[ 0.670740] R13: ffff888037c6fe50 R14: ffff888037c6f6c0 R15: 0000000000000000
[ 0.670740] FS: 0000000000000000(0000) GS:ffff888037c40000(0000) knlGS:0000000000000000
[ 0.670740] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 0.670740] CR2: 0000000000000000 CR3: 000000003060a000 CR4: 0000000000350ee0
[ 0.670740] Call Trace:
[ 0.670740] <TASK>
[ 0.670740] update_blocked_averages+0x98/0x160
[ 0.670740] newidle_balance+0x117/0x390
[ 0.670740] pick_next_task_fair+0x39/0x3c0
[ 0.670740] __schedule+0x156/0x6f0
[ 0.670740] schedule+0x4e/0xc0
[ 0.670740] worker_thread+0xb1/0x300
[ 0.670740] ? rescuer_thread+0x370/0x370
[ 0.687790] kvm-guest: stealtime: cpu 26, msr 37cb1080
[ 0.061937] kvm-clock: cpu 27, msr 318016c1, secondary cpu clock
[ 0.690740] kvm-guest: stealtime: cpu 27, msr 37cf1080
[ 0.061937] kvm-clock: cpu 28, msr 31801701, secondary cpu clock
[ 0.693781] kvm-guest: stealtime: cpu 28, msr 37d31080
[ 0.670740] kthread+0x158/0x180
[ 0.061937] kvm-clock: cpu 29, msr 31801741, secondary cpu clock
[ 0.670740] ? set_kthread_struct+0x40/0x40
[ 0.670740] ret_from_fork+0x22/0x30
[ 0.670740] </TASK>
[ 0.670740] ---[ end trace ac8562dd64da6bb5 ]---
[ 0.747785] kvm-guest: stealtime: cpu 29, msr 37d71080
[ 0.061937] kvm-clock: cpu 30, msr 31801781, secondary cpu clock
[ 0.756784] kvm-guest: stealtime: cpu 30, msr 37db1080
On 12/16/21 6:13 PM, David Woodhouse wrote:
> On Thu, 2021-12-16 at 16:52 -0600, Tom Lendacky wrote:
>> On baremetal, I haven't seen an issue. This only seems to have a problem
>> with Qemu/KVM.
>>
>> With 191f08997577 I could boot without issues with and without the
>> no_parallel_bringup. Only after I applied e78fa57dd642 did the failure happen.
>>
>> With e78fa57dd642 I could boot 64 vCPUs pretty consistently, but when I
>> jumped to 128 vCPUs it failed again. When I moved the series to
>> df9726cb7178, then 64 vCPUs also failed pretty consistently.
>>
>> Strange thing is it is random. Sometimes (rarely) it works on the first
>> boot and then sometimes it doesn't, at which point it will reset and
>> reboot 3 or 4 times and then make it past the failure and fully boot.
>
> Hm, some of that is just artifacts of timing, I'm sure. But now I'm
> staring at the way that early_setup_idt() can run in parallel on all
> CPUs, rewriting bringup_idt_descr and loading it.
>
> To start with, let's try unlocking the trampoline_lock much later,
> after cpu_init_exception_handling() has loaded the real IDT.
>
> I think we can probably make secondaries load the real IDT early and
> never use bringup_idt_descr at all, can't we? But let's see if this
> makes it go away, to start with...
>
This still fails. I ran with -d cpu_reset on the command line and will
forward the full log to you. I ran "grep "[ER]IP=" stderr.log | uniq -c"
and got:
128 EIP=00000000 EFL=00000000 [-------] CPL=0 II=0 A20=0 SMM=0 HLT=0
128 EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
These are before running any of the vCPUs.
1 RIP=ffffffff810705c6 RFL=00000206 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
This is where vCPU0 is at the time of the reset. This address tends to
be different all the time and so I think it is just where it happens to
be when the reset occurs and isn't contributing to the reset.
5 RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
1 RIP=ffffffff8104af06 RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
15 RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
These are some of the APs and all are in wait_for_master_cpu().
1 EIP=0000101b EFL=00000003 [------C] CPL=0 II=0 A20=1 SMM=0 HLT=0
This seems ok because: CS =9900 00099000 0000ffff 00009b00
So likely in the trampoline code.
1 EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
This one seems odd... could it be the one causing the reset?
CS =f000 ffff0000 0000ffff 00009a00
3 RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
2 EIP=0000101b EFL=00000003 [------C] CPL=0 II=0 A20=1 SMM=0 HLT=0
99 EIP=3f36e11b EFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=1
Thanks,
Tom
On Fri, 2021-12-17 at 11:48 -0600, Tom Lendacky wrote:
> On 12/16/21 6:13 PM, David Woodhouse wrote:
> > On Thu, 2021-12-16 at 16:52 -0600, Tom Lendacky wrote:
> > > On baremetal, I haven't seen an issue. This only seems to have a problem
> > > with Qemu/KVM.
> > >
> > > With 191f08997577 I could boot without issues with and without the
> > > no_parallel_bringup. Only after I applied e78fa57dd642 did the failure happen.
> > >
> > > With e78fa57dd642 I could boot 64 vCPUs pretty consistently, but when I
> > > jumped to 128 vCPUs it failed again. When I moved the series to
> > > df9726cb7178, then 64 vCPUs also failed pretty consistently.
> > >
> > > Strange thing is it is random. Sometimes (rarely) it works on the first
> > > boot and then sometimes it doesn't, at which point it will reset and
> > > reboot 3 or 4 times and then make it past the failure and fully boot.
> >
> > Hm, some of that is just artifacts of timing, I'm sure. But now I'm
> > staring at the way that early_setup_idt() can run in parallel on all
> > CPUs, rewriting bringup_idt_descr and loading it.
> >
> > To start with, let's try unlocking the trampoline_lock much later,
> > after cpu_init_exception_handling() has loaded the real IDT.
> >
> > I think we can probably make secondaries load the real IDT early and
> > never use bringup_idt_descr at all, can't we? But let's see if this
> > makes it go away, to start with...
> >
>
> This still fails. I ran with -d cpu_reset on the command line and will
> forward the full log to you. I ran "grep "[ER]IP=" stderr.log | uniq -c"
> and got:
>
> 128 EIP=00000000 EFL=00000000 [-------] CPL=0 II=0 A20=0 SMM=0 HLT=0
> 128 EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> These are before running any of the vCPUs.
>
> 1 RIP=ffffffff810705c6 RFL=00000206 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> This is where vCPU0 is at the time of the reset. This address tends to
> be different all the time and so I think it is just where it happens to
> be when the reset occurs and isn't contributing to the reset.
I note that one is in native_write_msr() though. I wonder what it's writing?
Do you have console output (perhaps with earlyprintk=ttyS0) to go with this?
> 5 RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> 1 RIP=ffffffff8104af06 RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> 15 RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> These are some of the APs and all are in wait_for_master_cpu().
As is right and proper. They should be coming up to that point and
waiting for the... erm... controlling CPU to tell them to go any
further.
> 1 EIP=0000101b EFL=00000003 [------C] CPL=0 II=0 A20=1 SMM=0 HLT=0
> This seems ok because: CS =9900 00099000 0000ffff 00009b00
> So likely in the trampoline code.
Yeah, that'll be in the bitlock waiting for its turn through the real
mode stack.
1010: 66 0f ba 26 18 btw $0x18,(%esi)
1015: 40 inc %eax
1016: 00 73 04 add %dh,0x4(%ebx)
1019: f3 90 pause
101b: eb f3 jmp 1010 <trampoline_start+0x10>
> 1 EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> This one seems odd... could it be the one causing the reset?
> CS =f000 ffff0000 0000ffff 00009a00
Yeah. I'm finding it slightly easier without the 'uniq'...
> CPU Reset (CPU 0)
> RIP=ffffffff810705c6 RFL=00000206 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 1)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 2)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 3)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 4)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 5)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 6)
> RIP=ffffffff8104af06 RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 7)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 8)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 9)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 10)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 11)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 12)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 13)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 14)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 15)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 16)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 17)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 18)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 19)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 20)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 21)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
All those came up and are waiting in wait_for_master_cpu() as they
should.
> CPU Reset (CPU 22)
> EIP=0000101b EFL=00000003 [------C] CPL=0 II=0 A20=1 SMM=0 HLT=0
That one's in the bitlock, also waiting.
> CPU Reset (CPU 23)
> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
This one we suspect. Is this what a triple-fault would look like? Not
if it's *already* at f000:fff0, surely?
CPU Reset (CPU 23)
EAX=00000000 EBX=00000000 ECX=00000000 EDX=00800f12
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00009300
CS =f000 ffff0000 0000ffff 00009a00
SS =0000 00000000 0000ffff 00009200
DS =0000 00000000 0000ffff 00009300
FS =0000 00000000 0000ffff 00009300
GS =0000 00000000 0000ffff 00009300
LDT=0000 00000000 0000ffff 00008200
TR =0000 00000000 0000ffff 00008300
GDT= 00000000 0000ffff
IDT= 00000000 0000ffff
CR0=00000010 CR2=00000000 CR3=00000000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
DR6=00000000ffff0ff0 DR7=0000000000000400
CCS=00000000 CCD=00000000 CCO=DYNAMIC
EFER=0000000000000000
FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
FPR0=0000000000000000 0000 FPR1=0000000000000000 0000
FPR2=0000000000000000 0000 FPR3=0000000000000000 0000
FPR4=0000000000000000 0000 FPR5=0000000000000000 0000
FPR6=0000000000000000 0000 FPR7=0000000000000000 0000
XMM00=0000000000000000 0000000000000000 XMM01=0000000000000000 0000000000000000
XMM02=0000000000000000 0000000000000000 XMM03=0000000000000000 0000000000000000
XMM04=0000000000000000 0000000000000000 XMM05=0000000000000000 0000000000000000
XMM06=0000000000000000 0000000000000000 XMM07=0000000000000000 0000000000000000
> CPU Reset (CPU 24)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 25)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 26)
> RIP=ffffffff8104aefb RFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
These ones made it through the real mode first and are also waiting.
> CPU Reset (CPU 27)
> EIP=0000101b EFL=00000003 [------C] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 28)
> EIP=0000101b EFL=00000003 [------C] CPL=0 II=0 A20=1 SMM=0 HLT=0
> CPU Reset (CPU 29)
Still in the real mode bitlock. And after this point they are still
halted in presumably 32-bit BIOS code because the BSP hasn't even
touched them yet.
> EIP=3f36e11b EFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=1
> CPU Reset (CPU 30)
> EIP=3f36e11b EFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=1
> CPU Reset (CPU 31)
> EIP=3f36e11b EFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=1
> CPU Reset (CPU 32)
> ...
> CPU Reset (CPU 127)
> EIP=3f36e11b EFL=00000046 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=1
On Fri, 2021-12-17 at 19:11 +0000, David Woodhouse wrote:
> I note that one is in native_write_msr() though. I wonder what it's
> writing?
CPU Reset (CPU 0)
RAX=0000000000000000 RBX=0000000000000202 RCX=0000000000000828 RDX=0000000000000000
RSI=0000000000000000 RDI=0000000000000828 RBP=0000000000000000 RSP=ffffc90000023ce0
R8 =0000000000000000 R9 =ffffc90000023b60 R10=0000000000000001 R11=0000000000000001
R12=000000000000069a R13=0000000000000005 R14=000000000000001c R15=0000000000000001
RIP=ffffffff810705c6 RFL=00000206 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
It's writing zero (%rax/%rsi) to MSR 0x828 (%rcx/%rdi) which is the
X2APIC's APIC_ESR.
Can you reproduce this without the guest being in X2APIC mode? You'll
have to cut it back to only 254 vCPUs for that test.
On 12/17/21 1:11 PM, David Woodhouse wrote:
> On Fri, 2021-12-17 at 11:48 -0600, Tom Lendacky wrote:
>> On 12/16/21 6:13 PM, David Woodhouse wrote:
>>> On Thu, 2021-12-16 at 16:52 -0600, Tom Lendacky wrote:
>>>> On baremetal, I haven't seen an issue. This only seems to have a problem
>>>> with Qemu/KVM.
>>>>
>>>> With 191f08997577 I could boot without issues with and without the
>>>> no_parallel_bringup. Only after I applied e78fa57dd642 did the failure happen.
>>>>
>>>> With e78fa57dd642 I could boot 64 vCPUs pretty consistently, but when I
>>>> jumped to 128 vCPUs it failed again. When I moved the series to
>>>> df9726cb7178, then 64 vCPUs also failed pretty consistently.
>>>>
>>>> Strange thing is it is random. Sometimes (rarely) it works on the first
>>>> boot and then sometimes it doesn't, at which point it will reset and
>>>> reboot 3 or 4 times and then make it past the failure and fully boot.
>>>
>>> Hm, some of that is just artifacts of timing, I'm sure. But now I'm
>>> staring at the way that early_setup_idt() can run in parallel on all
>>> CPUs, rewriting bringup_idt_descr and loading it.
>>>
>>> To start with, let's try unlocking the trampoline_lock much later,
>>> after cpu_init_exception_handling() has loaded the real IDT.
>>>
>>> I think we can probably make secondaries load the real IDT early and
>>> never use bringup_idt_descr at all, can't we? But let's see if this
>>> makes it go away, to start with...
>>>
>>
>> This still fails. I ran with -d cpu_reset on the command line and will
>> forward the full log to you. I ran "grep "[ER]IP=" stderr.log | uniq -c"
>> and got:
>>
>> 128 EIP=00000000 EFL=00000000 [-------] CPL=0 II=0 A20=0 SMM=0 HLT=0
>> 128 EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> These are before running any of the vCPUs.
>>
>> 1 RIP=ffffffff810705c6 RFL=00000206 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> This is where vCPU0 is at the time of the reset. This address tends to
>> be different all the time and so I think it is just where it happens to
>> be when the reset occurs and isn't contributing to the reset.
>
> I note that one is in native_write_msr() though. I wonder what it's writing?
>
> Do you have console output (perhaps with earlyprintk=ttyS0) to go with this?
Yes, but it's not really much help...
[ 0.146318] Freeing SMP alternatives memory: 36K
[ 0.249121] smpboot: CPU0: AMD EPYC Processor (family: 0x17, model: 0x1, stepping: 0x2)
[ 0.249291] Performance Events: AMD PMU driver.
[ 0.249771] ... version: 0
[ 0.250170] ... bit width: 48
[ 0.250258] ... generic registers: 4
[ 0.250662] ... value mask: 0000ffffffffffff
[ 0.251258] ... max period: 00007fffffffffff
[ 0.251790] ... fixed-purpose events: 0
[ 0.252258] ... event mask: 000000000000000f
[ 0.252972] rcu: Hierarchical SRCU implementation.
[ 0.255797] smp: Bringing up secondary CPUs ...
[ 0.256372] x86: Booting SMP configuration:
SecCoreStartupWithStack(0xFFFCC000, 0x820000)
Register PPI Notify: DCD0BE23-9586-40F4-B643-06522CED4EDE
Install PPI: 8C8CE578-8A3D-4F1C-9935-896185C32DD3
Install PPI: 5473C07A-3DCB-4DCA-BD6F-1E9689E7349A
The 0th FV start address is 0x00000820000, size is 0x000E0000, handle is 0x820000
There's no WARN or PANIC, just a reset. I can look to try and capture some
KVM trace data if that would help. If so, let me know what events you'd
like captured.
>
>> CPU Reset (CPU 23)
>> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
>
> This one we suspect. Is this what a triple-fault would look like? Not
> if it's *already* at f000:fff0, surely?
Good question. The APM doesn't really document it. I'll see if I can find
some h/w folks to check with.
Thanks,
Tom
>
> CPU Reset (CPU 23)
> EAX=00000000 EBX=00000000 ECX=00000000 EDX=00800f12
> ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0000 00000000 0000ffff 00009300
> CS =f000 ffff0000 0000ffff 00009a00
> SS =0000 00000000 0000ffff 00009200
> DS =0000 00000000 0000ffff 00009300
> FS =0000 00000000 0000ffff 00009300
> GS =0000 00000000 0000ffff 00009300
> LDT=0000 00000000 0000ffff 00008200
> TR =0000 00000000 0000ffff 00008300
> GDT= 00000000 0000ffff
> IDT= 00000000 0000ffff
> CR0=00000010 CR2=00000000 CR3=00000000 CR4=00000000
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> DR6=00000000ffff0ff0 DR7=0000000000000400
> CCS=00000000 CCD=00000000 CCO=DYNAMIC
> EFER=0000000000000000
> FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
> FPR0=0000000000000000 0000 FPR1=0000000000000000 0000
> FPR2=0000000000000000 0000 FPR3=0000000000000000 0000
> FPR4=0000000000000000 0000 FPR5=0000000000000000 0000
> FPR6=0000000000000000 0000 FPR7=0000000000000000 0000
> XMM00=0000000000000000 0000000000000000 XMM01=0000000000000000 0000000000000000
> XMM02=0000000000000000 0000000000000000 XMM03=0000000000000000 0000000000000000
> XMM04=0000000000000000 0000000000000000 XMM05=0000000000000000 0000000000000000
> XMM06=0000000000000000 0000000000000000 XMM07=0000000000000000 0000000000000000
>
On Fri, 2021-12-17 at 13:46 -0600, Tom Lendacky wrote:
> There's no WARN or PANIC, just a reset. I can look to try and capture some
> KVM trace data if that would help. If so, let me know what events you'd
> like captured.
Could start with just kvm_run_exit?
Reason 8 would be KVM_EXIT_SHUTDOWN and would potentially indicate a
triple fault.
Failing that, I'd want to start littering the real mode code with
outputting 'a' 'b' 'c' etc. to the serial port and see if the offending
CPU is really in the trampoline somewhere when something goes wrong.
I can knock up an example patch to do that (not tonight) but this would
be somewhat easier if I could find a machine I can reproduce on. Sadly
I only seem to have access to Milan *guests* without nested virt, not
bare metal. Got a machine I can log in to?
On 12/17/21 1:26 PM, David Woodhouse wrote:
> On Fri, 2021-12-17 at 19:11 +0000, David Woodhouse wrote:
>> I note that one is in native_write_msr() though. I wonder what it's
>> writing?
>
> CPU Reset (CPU 0)
> RAX=0000000000000000 RBX=0000000000000202 RCX=0000000000000828 RDX=0000000000000000
> RSI=0000000000000000 RDI=0000000000000828 RBP=0000000000000000 RSP=ffffc90000023ce0
> R8 =0000000000000000 R9 =ffffc90000023b60 R10=0000000000000001 R11=0000000000000001
> R12=000000000000069a R13=0000000000000005 R14=000000000000001c R15=0000000000000001
> RIP=ffffffff810705c6 RFL=00000206 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
>
> It's writing zero (%rax/%rsi) to MSR 0x828 (%rcx/%rdi) which is the
> X2APIC's APIC_ESR.
>
> Can you reproduce this without the guest being in X2APIC mode? You'll
> have to cut it back to only 254 vCPUs for that test.
Yes, reproducible with guest in xAPIC mode.
Thanks,
Tom
>
On 12/17/21 2:13 PM, David Woodhouse wrote:
> On Fri, 2021-12-17 at 13:46 -0600, Tom Lendacky wrote:
>> There's no WARN or PANIC, just a reset. I can look to try and capture some
>> KVM trace data if that would help. If so, let me know what events you'd
>> like captured.
>
>
> Could start with just kvm_run_exit?
>
> Reason 8 would be KVM_EXIT_SHUTDOWN and would potentially indicate a
> triple fault.
qemu-system-x86-24093 [005] ..... 1601.759486: kvm_exit: vcpu 112 reason shutdown rip 0xffffffff81070574 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x80000b08 error_code 0x00000000
# addr2line -e woodhouse-build-x86_64/vmlinux 0xffffffff81070574
/root/kernels/woodhouse-build-x86_64/./arch/x86/include/asm/desc.h:272
Which is: asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
Thanks,
Tom
>
> Failing that, I'd want to start littering the real mode code with
> outputting 'a' 'b' 'c' etc. to the serial port and see if the offending
> CPU is really in the trampoline somewhere when something goes wrong.
>
> I can knock up an example patch to do that (not tonight) but this would
> be somewhat easier if I could find a machine I can reproduce on. Sadly
> I only seem to have access to Milan *guests* without nested virt, not
> bare metal. Got a machine I can log in to?
>
>
>
On Fri, 2021-12-17 at 14:55 -0600, Tom Lendacky wrote:
> On 12/17/21 2:13 PM, David Woodhouse wrote:
> > On Fri, 2021-12-17 at 13:46 -0600, Tom Lendacky wrote:
> > > There's no WARN or PANIC, just a reset. I can look to try and capture some
> > > KVM trace data if that would help. If so, let me know what events you'd
> > > like captured.
> >
> >
> > Could start with just kvm_run_exit?
> >
> > Reason 8 would be KVM_EXIT_SHUTDOWN and would potentially indicate a
> > triple fault.
>
> qemu-system-x86-24093 [005] ..... 1601.759486: kvm_exit: vcpu 112 reason shutdown rip 0xffffffff81070574 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x80000b08 error_code 0x00000000
>
> # addr2line -e woodhouse-build-x86_64/vmlinux 0xffffffff81070574
> /root/kernels/woodhouse-build-x86_64/./arch/x86/include/asm/desc.h:272
>
> Which is: asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
OK, that seems like enough of a smoking gun; I'll stare at that harder. Thanks.
/*
* The LTR instruction marks the TSS GDT entry as busy. On 64-bit, the GDT is
* a read-only remapping. To prevent a page fault, the GDT is switched to the
* original writeable version when needed.
*/
#ifdef CONFIG_X86_64
static inline void native_load_tr_desc(void)
{
struct desc_ptr gdt;
int cpu = raw_smp_processor_id();
bool restore = 0;
struct desc_struct *fixmap_gdt;
native_store_gdt(&gdt);
fixmap_gdt = get_cpu_gdt_ro(cpu);
/*
* If the current GDT is the read-only fixmap, swap to the original
* writeable version. Swap back at the end.
*/
if (gdt.address == (unsigned long)fixmap_gdt) {
load_direct_gdt(cpu);
restore = 1;
}
asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
if (restore)
load_fixmap_gdt(cpu);
}
On Fri, 2021-12-17 at 11:09 +0100, Igor Mammedov wrote:
> On Fri, 17 Dec 2021 00:13:16 +0000
> David Woodhouse <[email protected]> wrote:
>
> > On Thu, 2021-12-16 at 16:52 -0600, Tom Lendacky wrote:
> > > On baremetal, I haven't seen an issue. This only seems to have a problem
> > > with Qemu/KVM.
> > >
> > > With 191f08997577 I could boot without issues with and without the
> > > no_parallel_bringup. Only after I applied e78fa57dd642 did the failure happen.
> > >
> > > With e78fa57dd642 I could boot 64 vCPUs pretty consistently, but when I
> > > jumped to 128 vCPUs it failed again. When I moved the series to
> > > df9726cb7178, then 64 vCPUs also failed pretty consistently.
> > >
> > > Strange thing is it is random. Sometimes (rarely) it works on the first
> > > boot and then sometimes it doesn't, at which point it will reset and
> > > reboot 3 or 4 times and then make it past the failure and fully boot.
> >
> > Hm, some of that is just artifacts of timing, I'm sure. But now I'm
>
> that's most likely the case (there is a race somewhere left).
> To trigger CPU bringup (hotplug) races, I used to run QEMU guest with
> heavy vCPU overcommit. It helps to induce unexpected delays at CPU bringup
> time.
That last commit which actually enables parallel bringup does *two*
things. It makes the generic cpuhp code bring all the CPUs through all
the CPUHP_*_PREPARE stages and then actually brings them up. With that
test patch I sent, the bringup basically *wasn't* parallel any more;
they were using the trampoline lock all the way to the point where they
start waiting on cpu_callin_mask.
So maybe it's the 'prepare' ordering, like the x2apic one I already
fixed... but some weirdness that only triggers on some CPUs. Can we
back out the actual pseudo-parallel bringup and do *only* the prepare
part, by doing something like this on top...
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1337,7 +1337,7 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
int ret;
/* If parallel AP bringup isn't enabled, perform the first steps now. */
- if (!do_parallel_bringup) {
+ if (1 || !do_parallel_bringup) {
ret = do_cpu_up(cpu, tidle);
if (ret)
return ret;
@@ -1366,7 +1366,8 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
/* 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));
+ return 0;
+ // return do_cpu_up(cpu, idle_thread_get(cpu));
}
/**
On 12/20/21 11:10 AM, David Woodhouse wrote:
> On Fri, 2021-12-17 at 11:09 +0100, Igor Mammedov wrote:
>> On Fri, 17 Dec 2021 00:13:16 +0000
>> David Woodhouse <[email protected]> wrote:
>>
>>> On Thu, 2021-12-16 at 16:52 -0600, Tom Lendacky wrote:
>>>> On baremetal, I haven't seen an issue. This only seems to have a problem
>>>> with Qemu/KVM.
>>>>
>>>> With 191f08997577 I could boot without issues with and without the
>>>> no_parallel_bringup. Only after I applied e78fa57dd642 did the failure happen.
>>>>
>>>> With e78fa57dd642 I could boot 64 vCPUs pretty consistently, but when I
>>>> jumped to 128 vCPUs it failed again. When I moved the series to
>>>> df9726cb7178, then 64 vCPUs also failed pretty consistently.
>>>>
>>>> Strange thing is it is random. Sometimes (rarely) it works on the first
>>>> boot and then sometimes it doesn't, at which point it will reset and
>>>> reboot 3 or 4 times and then make it past the failure and fully boot.
>>>
>>> Hm, some of that is just artifacts of timing, I'm sure. But now I'm
>>
>> that's most likely the case (there is a race somewhere left).
>> To trigger CPU bringup (hotplug) races, I used to run QEMU guest with
>> heavy vCPU overcommit. It helps to induce unexpected delays at CPU bringup
>> time.
>
> That last commit which actually enables parallel bringup does *two*
> things. It makes the generic cpuhp code bring all the CPUs through all
> the CPUHP_*_PREPARE stages and then actually brings them up. With that
> test patch I sent, the bringup basically *wasn't* parallel any more;
> they were using the trampoline lock all the way to the point where they
> start waiting on cpu_callin_mask.
>
> So maybe it's the 'prepare' ordering, like the x2apic one I already
> fixed... but some weirdness that only triggers on some CPUs. Can we
> back out the actual pseudo-parallel bringup and do *only* the prepare
> part, by doing something like this on top...
>
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1337,7 +1337,7 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
> int ret;
>
> /* If parallel AP bringup isn't enabled, perform the first steps now. */
> - if (!do_parallel_bringup) {
> + if (1 || !do_parallel_bringup) {
> ret = do_cpu_up(cpu, tidle);
> if (ret)
> return ret;
> @@ -1366,7 +1366,8 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
> /* 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));
> + return 0;
> + // return do_cpu_up(cpu, idle_thread_get(cpu));
> }
Took the tree back to commit df9726cb7178 and then applied this change.
I'm unable to trigger any kind of failure with this change.
Thanks,
Tom
>
> /**
>
>
On Mon, 2021-12-20 at 12:54 -0600, Tom Lendacky wrote:
> Took the tree back to commit df9726cb7178 and then applied this change.
> I'm unable to trigger any kind of failure with this change.
Hm... I fired up an EC2 m6a.48xlarge instance (192 CPUs) to play with.
I can reproduce your triple-fault on SMP bringup, but only with kexec.
And I basically can't get *anything* to kexec without that triple-
fault. Not a clean 5.16-rc2, not the Fedora stock 5.14.10 kernel.
If I *boot* instead of kexec, I have not yet seen the problem at all.
This is using Legacy BIOS not UEFI.
On 12/20/21 3:29 PM, David Woodhouse wrote:
> On Mon, 2021-12-20 at 12:54 -0600, Tom Lendacky wrote:
>> Took the tree back to commit df9726cb7178 and then applied this change.
>> I'm unable to trigger any kind of failure with this change.
>
> Hm... I fired up an EC2 m6a.48xlarge instance (192 CPUs) to play with.
>
> I can reproduce your triple-fault on SMP bringup, but only with kexec.
> And I basically can't get *anything* to kexec without that triple-
> fault. Not a clean 5.16-rc2, not the Fedora stock 5.14.10 kernel.
>
> If I *boot* instead of kexec, I have not yet seen the problem at all.
> This is using Legacy BIOS not UEFI.
Let me try with a legacy BIOS and see if I can repro. Might not be until
tomorrow, though, since I had to let someone borrow the machine.
Thanks,
Tom
>
>
On 12/20/21 3:47 PM, Tom Lendacky wrote:
> On 12/20/21 3:29 PM, David Woodhouse wrote:
>> On Mon, 2021-12-20 at 12:54 -0600, Tom Lendacky wrote:
>>> Took the tree back to commit df9726cb7178 and then applied this change.
>>> I'm unable to trigger any kind of failure with this change.
>>
>> Hm... I fired up an EC2 m6a.48xlarge instance (192 CPUs) to play with.
>>
>> I can reproduce your triple-fault on SMP bringup, but only with kexec.
>> And I basically can't get *anything* to kexec without that triple-
>> fault. Not a clean 5.16-rc2, not the Fedora stock 5.14.10 kernel.
>>
>> If I *boot* instead of kexec, I have not yet seen the problem at all.
>> This is using Legacy BIOS not UEFI.
>
> Let me try with a legacy BIOS and see if I can repro. Might not be until
> tomorrow, though, since I had to let someone borrow the machine.
I still encounter the issue using a legacy BIOS (SeaBIOS).
Thanks,
Tom
>
> Thanks,
> Tom
>
>>
>>
On 21 December 2021 22:25:35 GMT, Tom Lendacky <[email protected]> wrote:
>On 12/20/21 3:47 PM, Tom Lendacky wrote:
>> On 12/20/21 3:29 PM, David Woodhouse wrote:
>>> On Mon, 2021-12-20 at 12:54 -0600, Tom Lendacky wrote:
>>>> Took the tree back to commit df9726cb7178 and then applied this change.
>>>> I'm unable to trigger any kind of failure with this change.
>>>
>>> Hm... I fired up an EC2 m6a.48xlarge instance (192 CPUs) to play with.
>>>
>>> I can reproduce your triple-fault on SMP bringup, but only with kexec.
>>> And I basically can't get *anything* to kexec without that triple-
>>> fault. Not a clean 5.16-rc2, not the Fedora stock 5.14.10 kernel.
>>>
>>> If I *boot* instead of kexec, I have not yet seen the problem at all.
>>> This is using Legacy BIOS not UEFI.
>>
>> Let me try with a legacy BIOS and see if I can repro. Might not be until
>> tomorrow, though, since I had to let someone borrow the machine.
>
>I still encounter the issue using a legacy BIOS (SeaBIOS).
I haven't had much time to play but have seen it with a stock kernel at least as far back as v5.0. They all triple-fault on bringing up secondary CPUs, on kexec.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Dear David,
Am 15.12.21 um 15:56 schrieb David Woodhouse:
> 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 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.
>
>
> 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
>
> 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 | 71 ++++++++
> arch/x86/kernel/smpboot.c | 324 ++++++++++++++++++++++++----------
> 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, 441 insertions(+), 159 deletions(-)
Thank you for working on this. I tested this on a MSI MS-7A37/B350M
MORTAR (BIOS 1.MW 11/01/2021) with a Ryzen 3 2200G, but nothing was
printed to the screen after the GRUB loading messages, so it crashed or
hung somewhere. Unfortunately, this device is used by others, and no
serial console is connected and I do not know how to capture the Linux
log with other means.
Kind regards,
Paul
Dear David,
Am 27.12.21 um 17:57 schrieb Paul Menzel:
> Am 15.12.21 um 15:56 schrieb David Woodhouse:
>> 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 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.
>>
>>
>> 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
>>
>> 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 | 71 ++++++++
>> arch/x86/kernel/smpboot.c | 324 ++++++++++++++++++++++++----------
>> 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, 441 insertions(+), 159 deletions(-)
>
> Thank you for working on this. I tested this on a MSI MS-7A37/B350M
> MORTAR (BIOS 1.MW 11/01/2021) with a Ryzen 3 2200G, but nothing was
> printed to the screen after the GRUB loading messages, so it crashed or
> hung somewhere. Unfortunately, this device is used by others, and no
> serial console is connected and I do not know how to capture the Linux
> log with other means.
Same on the ASUS F2A85-M PRO with AMD A6-6400K. Without serial console,
the messages below are printed below to the monitor after nine seconds.
[ 1.078879] smp: Bringing up secondary CPUs ...
[ 1.080950] x86: Booting SMP configuration:
Please find the serial log attached.
Kind regards,
Paul
On Tue, 2021-12-28 at 12:34 +0100, Paul Menzel wrote:
> Same on the ASUS F2A85-M PRO with AMD A6-6400K. Without serial console,
> the messages below are printed below to the monitor after nine seconds.
>
> [ 1.078879] smp: Bringing up secondary CPUs ...
> [ 1.080950] x86: Booting SMP configuration:
>
> Please find the serial log attached.
>
Thanks for testing. That looks like the same triple-fault on bringup
that we have been seeing, and that I reproduced without my patches
using kexec all the way back to a 5.0 kernel.
Out of interest, are you also able to reproduce it with kexec and
without the parallel bringup?
And with that patch I sent Tom in
https://lore.kernel.org/lkml/[email protected]/
to expand the bitlock exclusion and stop the bringup being truly in
parallel at all?
Or tbe one in
https://lore.kernel.org/lkml/[email protected]
which makes it do nothing except prepare all the CPUs before bringing
them up one at a time?
My current theory (not that I've spent that much time thinking about it
in the last week) is that there's something about the existing CPU
bringup, possibly a CPU bug or something special about the AMD CPUs,
which is triggered by just making it a little bit *faster*, which is
why bringing them up from kexec (especially in qemu) can cause it too?
Tom seemed to find that it was in load_TR_desc(), so if you could try
this hack on a machine that doesn't magically wink out of existence on
a triplefault before even flushing its serial output, that would be
much appreciated...
diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index ab97b22ac04a..cc6590712ff4 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -8,7 +8,7 @@
#include <asm/fixmap.h>
#include <asm/irq_vectors.h>
#include <asm/cpu_entry_area.h>
-
+#include <asm/io.h>
#include <linux/debug_locks.h>
#include <linux/smp.h>
#include <linux/percpu.h>
@@ -265,11 +265,16 @@ static inline void native_load_tr_desc(void)
* If the current GDT is the read-only fixmap, swap to the original
* writeable version. Swap back at the end.
*/
+ outb('d', 0x3f8);
if (gdt.address == (unsigned long)fixmap_gdt) {
+ outb('e', 0x3f8);
load_direct_gdt(cpu);
restore = 1;
+ outb('f', 0x3f8);
}
+ outb('g', 0x3f8);
asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
+ outb('h', 0x3f8);
if (restore)
load_fixmap_gdt(cpu);
}
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0083464de5e3..5bc8f30c3283 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1716,7 +1716,9 @@ void identify_secondary_cpu(struct cpuinfo_x86 *c)
enable_sep_cpu();
#endif
mtrr_ap_init();
+outb('A', 0x3f8);
validate_apic_and_package_id(c);
+outb('B', 0x3f8);
x86_spec_ctrl_setup_ap();
update_srbds_msr();
}
@@ -1957,6 +1959,7 @@ static inline void tss_setup_io_bitmap(struct tss_struct *tss)
tss->io_bitmap.mapall[IO_BITMAP_LONGS] = ~0UL;
#endif
}
+#include <asm/realmode.h>
/*
* Setup everything needed to handle exceptions from the IDT, including the IST
@@ -1969,16 +1972,24 @@ void cpu_init_exception_handling(void)
/* paranoid_entry() gets the CPU number from the GDT */
setup_getcpu(cpu);
-
+ outb('\n', 0x3f8);
+ outb('0' + cpu / 100, 0x3f8);
+ outb('0' + (cpu % 100) / 10, 0x3f8);
+ outb('0' + (cpu % 10), 0x3f8);
+
/* IST vectors need TSS to be set up. */
tss_setup_ist(tss);
+ outb('a', 0x3f8);
tss_setup_io_bitmap(tss);
set_tss_desc(cpu, &get_cpu_entry_area(cpu)->tss.x86_tss);
-
+ outb('b', 0x3f8);
load_TR_desc();
+ outb('c', 0x3f8);
/* Finally load the IDT */
load_current_idt();
+ outb('z', 0x3f8);
+
}
/*
Dear David,
Am 28.12.21 um 15:18 schrieb David Woodhouse:
> On Tue, 2021-12-28 at 12:34 +0100, Paul Menzel wrote:
>> Same on the ASUS F2A85-M PRO with AMD A6-6400K. Without serial console,
>> the messages below are printed below to the monitor after nine seconds.
>>
>> [ 1.078879] smp: Bringing up secondary CPUs ...
>> [ 1.080950] x86: Booting SMP configuration:
>>
>> Please find the serial log attached.
>
> Thanks for testing. That looks like the same triple-fault on bringup
> that we have been seeing, and that I reproduced without my patches
> using kexec all the way back to a 5.0 kernel.
>
> Out of interest, are you also able to reproduce it with kexec and
> without the parallel bringup?
No, I am not able to reproduce that with Debian’s
*linux-image-5.15.0-2-686*, and kexec. With this board,
`module_blacklist=radeon` is needed, as the driver *radeon* is not able
to deal with kexec – and amdgpu neither [1].
```
[ 3.349911] [drm] Found VCE firmware/feedback version 50.0.1 / 17!
[ 3.365259] clocksource: Switched to clocksource tsc
[ 3.365284] [drm] GART: num cpu pages 262144, num gpu pages 262144
[ 3.405159] random: fast init done
[ 3.420492] [drm] PCIE GART of 1024M enabled (table at
0x00000000001D6000).
[ 3.427634] radeon 0000:00:01.0: WB enabled
[ 3.431828] radeon 0000:00:01.0: fence driver on ring 0 use gpu addr
0x0000000020000c00
[ 3.440100] radeon 0000:00:01.0: fence driver on ring 5 use gpu addr
0x0000000000075a18
[ 3.458182] radeon 0000:00:01.0: failed VCE resume (-22).
[ 3.463591] radeon 0000:00:01.0: fence driver on ring 1 use gpu addr
0x0000000020000c04
[ 3.471615] radeon 0000:00:01.0: fence driver on ring 2 use gpu addr
0x0000000020000c08
[ 3.479636] radeon 0000:00:01.0: fence driver on ring 3 use gpu addr
0x0000000020000c0c
[ 3.487650] radeon 0000:00:01.0: fence driver on ring 4 use gpu addr
0x0000000020000c10
[ 3.495990] radeon 0000:00:01.0: radeon: MSI limited to 32-bit
[ 3.502008] radeon 0000:00:01.0: radeon: using MSI.
[ 3.506906] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[ 3.506918] [drm] radeon: irq initialized.
[
```
> And with that patch I sent Tom in
> https://lore.kernel.org/lkml/[email protected]/
> to expand the bitlock exclusion and stop the bringup being truly in
> parallel at all?
No, this does not help, and the Linux kernel resets at the same spot.
```
[ 1.036036] smpboot: smpboot: After
x86_init.timers.setup_percpu_clockev()
[ 1.037031] smpboot: smp_get_logical_apicid()
[ 1.038031] smpboot: CPU0: AMD A6-6400K APU with Radeon(tm) HD
Graphics (family: 0x15, model: 0x13, stepping: 0x1)
[ 1.039366] Performance Events: Fam15h core perfctr, AMD PMU driver.
[ 1.040033] ... version: 0
[ 1.041031] ... bit width: 48
[ 1.042031] ... generic registers: 6
[ 1.043033] ... value mask: 0000ffffffffffff
[ 1.044031] ... max period: 00007fffffffffff
[ 1.045031] ... fixed-purpose events: 0
[ 1.046031] ... event mask: 000000000000003f
[ 1.048065] rcu: Hierarchical SRCU implementation.
[ 1.050642] NMI watchdog: Enabled. Permanently consumes one hw-PMU
counter.
[ 1.051133] smp: Bringing up secondary CPUs ...
[ 1.053202] x86: Booting SMP configuration:
```
> Or the one in
> https://lore.kernel.org/lkml/[email protected]
> which makes it do nothing except prepare all the CPUs before bringing
> them up one at a time?
I applied it on top the other one, and it made no difference either.
> My current theory (not that I've spent that much time thinking about it
> in the last week) is that there's something about the existing CPU
> bringup, possibly a CPU bug or something special about the AMD CPUs,
> which is triggered by just making it a little bit *faster*, which is
> why bringing them up from kexec (especially in qemu) can cause it too?
Would having the serial console enabled make a difference?
> Tom seemed to find that it was in load_TR_desc(), so if you could try
> this hack on a machine that doesn't magically wink out of existence on
> a triplefault before even flushing its serial output, that would be
> much appreciated...
>
> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
> index ab97b22ac04a..cc6590712ff4 100644
> --- a/arch/x86/include/asm/desc.h
> +++ b/arch/x86/include/asm/desc.h
> @@ -8,7 +8,7 @@
> #include <asm/fixmap.h>
> #include <asm/irq_vectors.h>
> #include <asm/cpu_entry_area.h>
> -
> +#include <asm/io.h>
> #include <linux/debug_locks.h>
> #include <linux/smp.h>
> #include <linux/percpu.h>
> @@ -265,11 +265,16 @@ static inline void native_load_tr_desc(void)
> * If the current GDT is the read-only fixmap, swap to the original
> * writeable version. Swap back at the end.
> */
> + outb('d', 0x3f8);
> if (gdt.address == (unsigned long)fixmap_gdt) {
> + outb('e', 0x3f8);
> load_direct_gdt(cpu);
> restore = 1;
> + outb('f', 0x3f8);
> }
> + outb('g', 0x3f8);
> asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
> + outb('h', 0x3f8);
> if (restore)
> load_fixmap_gdt(cpu);
> }
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 0083464de5e3..5bc8f30c3283 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1716,7 +1716,9 @@ void identify_secondary_cpu(struct cpuinfo_x86 *c)
> enable_sep_cpu();
> #endif
> mtrr_ap_init();
> +outb('A', 0x3f8);
> validate_apic_and_package_id(c);
> +outb('B', 0x3f8);
> x86_spec_ctrl_setup_ap();
> update_srbds_msr();
> }
> @@ -1957,6 +1959,7 @@ static inline void tss_setup_io_bitmap(struct tss_struct *tss)
> tss->io_bitmap.mapall[IO_BITMAP_LONGS] = ~0UL;
> #endif
> }
> +#include <asm/realmode.h>
>
> /*
> * Setup everything needed to handle exceptions from the IDT, including the IST
> @@ -1969,16 +1972,24 @@ void cpu_init_exception_handling(void)
>
> /* paranoid_entry() gets the CPU number from the GDT */
> setup_getcpu(cpu);
> -
> + outb('\n', 0x3f8);
> + outb('0' + cpu / 100, 0x3f8);
> + outb('0' + (cpu % 100) / 10, 0x3f8);
> + outb('0' + (cpu % 10), 0x3f8);
> +
> /* IST vectors need TSS to be set up. */
> tss_setup_ist(tss);
> + outb('a', 0x3f8);
> tss_setup_io_bitmap(tss);
> set_tss_desc(cpu, &get_cpu_entry_area(cpu)->tss.x86_tss);
> -
> + outb('b', 0x3f8);
> load_TR_desc();
> + outb('c', 0x3f8);
>
> /* Finally load the IDT */
> load_current_idt();
> + outb('z', 0x3f8);
> +
> }
>
> /*
Unfortunately, no more messages were printed on the serial console.
Kind regards,
Paul
[1]: https://gitlab.freedesktop.org/drm/amd/-/issues/1597
On Wed, 2021-12-29 at 14:18 +0100, Paul Menzel wrote:
> > Or the one in
> > https://lore.kernel.org/lkml/[email protected]
> >
> > which makes it do nothing except prepare all the CPUs before bringing
> > them up one at a time?
>
> I applied it on top the other one, and it made no difference either.
It's possible I missed something else in the prepare stage that doesn't
cope with all CPUs being prepared first.
My next attempt might be to change the loop in bringup_nonboot_cpus()
to bring all the CPUs not to the CPUHP_BP_PARALLEL_DYN state(s) but
instead just bring them to somewhere like CPUHP_RCUTREE_PREP, which is
somewhere in the middle between CPUHP_OFFLINE and CPUHP_BRINGUP_CPU.
Then a binary chop search — if that one boots, try maybe
CPUHP_TOPOLOGY_PREPARE. And if not, try CPUHP_PROFILE_PREPARE. Etc.
> > My current theory (not that I've spent that much time thinking about it
> > in the last week) is that there's something about the existing CPU
> > bringup, possibly a CPU bug or something special about the AMD CPUs,
> > which is triggered by just making it a little bit *faster*, which is
> > why bringing them up from kexec (especially in qemu) can cause it too?
>
> Would having the serial console enabled make a difference?
>
Yes. I couldn't make this fail in my EC2 m6a instance (for clean boots;
I have never managed to kexec it) until I turned off the serial console
to make things go faster.
> > Tom seemed to find that it was in load_TR_desc(), so if you could try
> > this hack on a machine that doesn't magically wink out of existence on
> > a triplefault before even flushing its serial output, that would be
> > much appreciated...
> Unfortunately, no more messages were printed on the serial console.
I suppose we need to litter those outputs somewhere earlier in the
trampoline then, perhaps it *isn't* getting to load_TR_desc() in your
case?
Will be back online properly next week and can actually provide some of
the above suggestions in patch form if you're willing to keep testing.
Thanks!
On Fri, 2021-12-17 at 14:55 -0600, Tom Lendacky wrote:
> On 12/17/21 2:13 PM, David Woodhouse wrote:
> > On Fri, 2021-12-17 at 13:46 -0600, Tom Lendacky wrote:
> > > There's no WARN or PANIC, just a reset. I can look to try and capture some
> > > KVM trace data if that would help. If so, let me know what events you'd
> > > like captured.
> >
> >
> > Could start with just kvm_run_exit?
> >
> > Reason 8 would be KVM_EXIT_SHUTDOWN and would potentially indicate a
> > triple fault.
>
> qemu-system-x86-24093 [005] ..... 1601.759486: kvm_exit: vcpu 112 reason shutdown rip 0xffffffff81070574 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x80000b08 error_code 0x00000000
>
> # addr2line -e woodhouse-build-x86_64/vmlinux 0xffffffff81070574
> /root/kernels/woodhouse-build-x86_64/./arch/x86/include/asm/desc.h:272
>
> Which is: asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
So, I remain utterly bemused by this, and the Milan *guests* I have
access to can't even kexec with a stock kernel; that is also "too fast"
and they take a triple fault during the bringup in much the same way —
even without my parallel patches, and even going back to fairly old
kernels.
I wasn't able to follow up with raw serial output during the bringup to
pinpoint precisely where it happens, because the VM would tear itself
down in response to the triple fault without actually flushing the last
virtual serial output :)
It would be really useful to get access to a suitable host where I can
spawn this in qemu and watch it fail. I am suspecting a chip-specific
quirk or bug at this point.
I might suggest in the short term that we could unblock the parallel
bringup work by just not doing it for affected chips... but that won't
make existing kexec work.
On Fri, Jan 28, 2022, David Woodhouse wrote:
> On Fri, 2021-12-17 at 14:55 -0600, Tom Lendacky wrote:
> > On 12/17/21 2:13 PM, David Woodhouse wrote:
> > > On Fri, 2021-12-17 at 13:46 -0600, Tom Lendacky wrote:
> > > > There's no WARN or PANIC, just a reset. I can look to try and capture some
> > > > KVM trace data if that would help. If so, let me know what events you'd
> > > > like captured.
> > >
> > >
> > > Could start with just kvm_run_exit?
> > >
> > > Reason 8 would be KVM_EXIT_SHUTDOWN and would potentially indicate a
> > > triple fault.
> >
> > qemu-system-x86-24093 [005] ..... 1601.759486: kvm_exit: vcpu 112 reason shutdown rip 0xffffffff81070574 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x80000b08 error_code 0x00000000
> >
> > # addr2line -e woodhouse-build-x86_64/vmlinux 0xffffffff81070574
> > /root/kernels/woodhouse-build-x86_64/./arch/x86/include/asm/desc.h:272
> >
> > Which is: asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
>
> So, I remain utterly bemused by this, and the Milan *guests* I have
> access to can't even kexec with a stock kernel; that is also "too fast"
> and they take a triple fault during the bringup in much the same way —
> even without my parallel patches, and even going back to fairly old
> kernels.
>
> I wasn't able to follow up with raw serial output during the bringup to
> pinpoint precisely where it happens, because the VM would tear itself
> down in response to the triple fault without actually flushing the last
> virtual serial output :)
>
> It would be really useful to get access to a suitable host where I can
> spawn this in qemu and watch it fail. I am suspecting a chip-specific
> quirk or bug at this point.
Nope. You missed a spot. This also reproduces on a sufficiently large Intel
system (and Milan). initial_gs gets overwritten by common_cpu_up(), which leads
to a CPU getting the wrong MSR_GS_BASE and then the wrong raw_smp_processor_id(),
resulting in cpu_init_exception_handling() stuffing the wrong GDT and leaving a
NULL TR descriptor for itself.
You also have a lurking bug in the x2APIC ID handling. Stripping the boot flags
from the prescribed APICID needs to happen before retrieving the x2APIC ID from
CPUID, otherwise bits 31:16 of the ID will be lost.
You owe me two beers ;-)
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index dcdf49a137d6..23df88c86a0e 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -208,11 +208,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
* in smpboot_control:
* Bit 0-15 APICID if STARTUP_USE_CPUID_0B is not set
* Bit 16 Secondary boot flag
- * Bit 17 Parallel boot flag
+ * Bit 17 Parallel boot flag (STARTUP_USE_CPUID_0B)
*/
testl $STARTUP_USE_CPUID_0B, %eax
- jz .Lsetup_AP
+ jnz .Luse_cpuid_0b
+ andl $0xFFFF, %eax
+ jmp .Lsetup_AP
+.Luse_cpuid_0b:
mov $0x0B, %eax
xorl %ecx, %ecx
cpuid
@@ -220,7 +223,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
.Lsetup_AP:
/* EAX contains the APICID of the current CPU */
- andl $0xFFFF, %eax
xorl %ecx, %ecx
leaq cpuid_to_apicid(%rip), %rbx
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 04f5c8de5606..e7fda406f39a 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1093,6 +1093,17 @@ wakeup_cpu_via_init_nmi(int cpu, unsigned long start_ip, int apicid,
return boot_error;
}
+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);
+
+
int common_cpu_up(unsigned int cpu, struct task_struct *idle)
{
int ret;
@@ -1112,7 +1123,8 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle)
/* 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);
+ if (!do_parallel_bringup)
+ initial_gs = per_cpu_offset(cpu);
#endif
return 0;
}
@@ -1336,16 +1348,6 @@ int do_cpu_up(unsigned int cpu, struct task_struct *tidle)
return ret;
}
-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);
-
int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
{
int ret;
On 28 January 2022 21:40:42 GMT, Sean Christopherson <[email protected]> wrote:
>On Fri, Jan 28, 2022, David Woodhouse wrote:
>> On Fri, 2021-12-17 at 14:55 -0600, Tom Lendacky wrote:
>> > On 12/17/21 2:13 PM, David Woodhouse wrote:
>> > > On Fri, 2021-12-17 at 13:46 -0600, Tom Lendacky wrote:
>> > > > There's no WARN or PANIC, just a reset. I can look to try and capture some
>> > > > KVM trace data if that would help. If so, let me know what events you'd
>> > > > like captured.
>> > >
>> > >
>> > > Could start with just kvm_run_exit?
>> > >
>> > > Reason 8 would be KVM_EXIT_SHUTDOWN and would potentially indicate a
>> > > triple fault.
>> >
>> > qemu-system-x86-24093 [005] ..... 1601.759486: kvm_exit: vcpu 112 reason shutdown rip 0xffffffff81070574 info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x80000b08 error_code 0x00000000
>> >
>> > # addr2line -e woodhouse-build-x86_64/vmlinux 0xffffffff81070574
>> > /root/kernels/woodhouse-build-x86_64/./arch/x86/include/asm/desc.h:272
>> >
>> > Which is: asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
>>
>> So, I remain utterly bemused by this, and the Milan *guests* I have
>> access to can't even kexec with a stock kernel; that is also "too fast"
>> and they take a triple fault during the bringup in much the same way —
>> even without my parallel patches, and even going back to fairly old
>> kernels.
>>
>> I wasn't able to follow up with raw serial output during the bringup to
>> pinpoint precisely where it happens, because the VM would tear itself
>> down in response to the triple fault without actually flushing the last
>> virtual serial output :)
>>
>> It would be really useful to get access to a suitable host where I can
>> spawn this in qemu and watch it fail. I am suspecting a chip-specific
>> quirk or bug at this point.
>
>Nope. You missed a spot. This also reproduces on a sufficiently large Intel
>system (and Milan). initial_gs gets overwritten by common_cpu_up(), which leads
>to a CPU getting the wrong MSR_GS_BASE and then the wrong raw_smp_processor_id(),
>resulting in cpu_init_exception_handling() stuffing the wrong GDT and leaving a
>NULL TR descriptor for itself.
>
>You also have a lurking bug in the x2APIC ID handling. Stripping the boot flags
>from the prescribed APICID needs to happen before retrieving the x2APIC ID from
>CPUID, otherwise bits 31:16 of the ID will be lost.
>
>You owe me two beers ;-)
Oh Sean, I love you.
Thanks.
Will update and retest and resend.
On Fri, 2022-01-28 at 21:40 +0000, Sean Christopherson wrote:
> Nope. You missed a spot. This also reproduces on a sufficiently large Intel
> system (and Milan). initial_gs gets overwritten by common_cpu_up(), which leads
> to a CPU getting the wrong MSR_GS_BASE and then the wrong raw_smp_processor_id(),
> resulting in cpu_init_exception_handling() stuffing the wrong GDT and leaving a
> NULL TR descriptor for itself.
>
> You also have a lurking bug in the x2APIC ID handling. Stripping the boot flags
> from the prescribed APICID needs to happen before retrieving the x2APIC ID from
> CPUID, otherwise bits 31:16 of the ID will be lost.
>
> You owe me two beers ;-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index dcdf49a137d6..23df88c86a0e 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -208,11 +208,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> * in smpboot_control:
> * Bit 0-15 APICID if STARTUP_USE_CPUID_0B is not set
> * Bit 16 Secondary boot flag
> - * Bit 17 Parallel boot flag
> + * Bit 17 Parallel boot flag (STARTUP_USE_CPUID_0B)
> */
> testl $STARTUP_USE_CPUID_0B, %eax
> - jz .Lsetup_AP
> + jnz .Luse_cpuid_0b
> + andl $0xFFFF, %eax
> + jmp .Lsetup_AP
>
> +.Luse_cpuid_0b:
> mov $0x0B, %eax
> xorl %ecx, %ecx
> cpuid
Looks like I had already fixed that one in a cleanup at
https://git.infradead.org/users/dwmw2/linux.git/commitdiff/191f08997577
I removed the mask entirely. We now use the APIC ID from the low 31
bits if bit 31 isn't set... and there's no need to mask it out because
by definition it isn't set.
+ /*
+ * 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-30 APIC ID if STARTUP_PARALLEL is not set
+ * Bit 31 Parallel boot flag (use CPUID leaf 0x0b for APIC ID).
+ */
+ testl $STARTUP_PARALLEL, %eax
+ jz .Lsetup_AP
+
+ mov $0x0B, %eax
+ xorl %ecx, %ecx
+ cpuid
+ mov %edx, %eax
+
+.Lsetup_AP:
I am, of course, still prepared to buy you as many beers as you desire.
Perhaps in Dublin in September, where we're (hopefully) going to be
doing Linux Plumbers Conference in person again at last!
(I actually think I'm going to rework that cleanup because it's given
us a hard-coded assumption that no AP has APIC ID 0. I'll put back the
explicit STARTUP_SECONDARY flag that Thomas had, and work your fix in
too to avoid re-introducing the bug.)
> int common_cpu_up(unsigned int cpu, struct task_struct *idle)
> {
> int ret;
> @@ -1112,7 +1123,8 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle)
> /* 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);
> + if (!do_parallel_bringup)
> + initial_gs = per_cpu_offset(cpu);
> #endif
> return 0;
> }
Hm, I think that can be removed completely, can't it? We don't need to
make it conditional, because even the non-parallel 64-bit bringup will
still take the same path in head_64.S to *find* the stack and other
per-CPU information; it just gets its APIC ID from the global variable
in order to do so.
On Thu, 2021-12-16 at 19:20 +0000, David Woodhouse wrote:
> On Thu, 2021-12-16 at 13:00 -0600, Tom Lendacky wrote:
> > On 12/16/21 12:24 PM, David Woodhouse wrote:
> > > On Thu, 2021-12-16 at 08:24 -0600, Tom Lendacky wrote:
> > >
> > > > This will break an SEV-ES guest because CPUID will generate a #VC and a
> > > > #VC handler has not been established yet.
> > > >
> > > > I guess for now, you can probably just not enable parallel startup for
> > > > SEV-ES guests.
> > >
> > > OK, thanks. I'll expand it to allow 24 bits of (physical) APIC ID then,
> > > since it's no longer limited to CPUs without X2APIC. Then we can
> > > refrain from doing parallel bringup for SEV-ES guests, as you suggest.
> > >
> > > What precisely is the check I should be using for that?
> >
> > Calling cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) will return true for
> > an SEV-ES guest.
>
> Thanks. Incremental patch (which I'll roll into Thomas's patch) looks a
> bit like this. Testing it now...
Further inspection shows I really did want a bit to indicate that this
is a secondary AP startup, which Thomas had documented as such in the
comments in head_64.S but then actually called STARTUP_USE_APICID.
Otherwise the special case of smpboot_control==zero for startup of the
BSP which uses the pre-existing initial_gs etc., might also get invoked
in the rare case that an AP has APIC ID #0.
So we really do need Sean's fix to do the masking in the right place,
which I had 'fixed' by removing that mask altogether. And we also need
Sean's fix to stop scribbling on initial_fs when each AP will calculate
it for itself anyway.
I've rebased and pushed to
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.17
I'll do some more testing and repost the series during next week. The
win is slightly more modest than the original patch sets because it now
only parallelises x86/cpu:kick. I'm going to do more careful review and
testing before doing the same for x86/cpu:wait-init in a later series.
You can see that coming together in the git tree but I'm only going to
post up to the 'Serialise topology updates' patch again for now.
The only real change is this patch; perhaps now we've fixed it Thomas
will provide a Signed-off-by for it? :)
Now looks like this...
From 888741f787a2e59b1471f15177c1ba981d06ad04 Mon Sep 17 00:00:00 2001
From: Thomas Gleixner <[email protected]>
Date: Fri, 12 Feb 2021 18:30:28 +0100
Subject: [PATCH v3.1 6/9] x86/smpboot: Support parallel startup of secondary CPUs
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 Sat, Jan 29, 2022 at 12:04:19PM +0000, David Woodhouse wrote:
> I've rebased and pushed to
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.17
>
> I'll do some more testing and repost the series during next week. The
> win is slightly more modest than the original patch sets because it now
> only parallelises x86/cpu:kick. I'm going to do more careful review and
> testing before doing the same for x86/cpu:wait-init in a later series.
> You can see that coming together in the git tree but I'm only going to
> post up to the 'Serialise topology updates' patch again for now.
Btw, Mr. Cooper points out a very important aspect and I don't know
whether you've verified this already or whether this is not affected
by your series ... yet. In any case it should be checked: microcode
loading.
See __reload_late() and all that dance we do to keep SMT siblings do
nothing at the same time while updating microcode.
With the current boot order, the APs should all do nothing so they won't
need that sync for early loading - load_ucode_{ap,bsp} - but I don't
know if you're changing that order with the parallel startup.
If you do, you'll probably need such syncing for the early loading
too...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, 2022-01-31 at 14:59 +0100, Borislav Petkov wrote:
> On Sat, Jan 29, 2022 at 12:04:19PM +0000, David Woodhouse wrote:
> > I've rebased and pushed to
> > https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.17
> >
> >
> > I'll do some more testing and repost the series during next week. The
> > win is slightly more modest than the original patch sets because it now
> > only parallelises x86/cpu:kick. I'm going to do more careful review and
> > testing before doing the same for x86/cpu:wait-init in a later series.
> > You can see that coming together in the git tree but I'm only going to
> > post up to the 'Serialise topology updates' patch again for now.
>
> Btw, Mr. Cooper points out a very important aspect and I don't know
> whether you've verified this already or whether this is not affected
> by your series ... yet. In any case it should be checked: microcode
> loading.
>
> See __reload_late() and all that dance we do to keep SMT siblings do
> nothing at the same time while updating microcode.
>
> With the current boot order, the APs should all do nothing so they won't
> need that sync for early loading - load_ucode_{ap,bsp} - but I don't
> know if you're changing that order with the parallel startup.
>
> If you do, you'll probably need such syncing for the early loading
> too...
Thanks. It looks like that is only invoked after boot, with a write to
/sys/devices/system/cpu/microcode/reload.
My series is only parallelising the initial bringup at boot time, so it
shouldn't make any difference.
However... it does look like there's nothing preventing a sibling being
brought online *while* the dance you mention above is occurring.
Shouldn't __reload_late() take the device_hotplug_lock to prevent that?
On Tue, 2022-02-01 at 13:56 +0100, Borislav Petkov wrote:
> On Tue, Feb 01, 2022 at 12:39:17PM +0000, David Woodhouse wrote:
> > In the top of my git tree, you can see a half-baked 'parallel part 2'
> > commit which introduces a new x86/cpu:wait-init cpuhp state that would
> > invoke do_wait_cpu_initialized() for each CPU in turn, which *would*
> > release them all into load_ucode_bsp() at the same time and have
> > precisely the problem you're describing.
>
> The load_ucode_bsp() is the variant that runs on the boot CPU but
> yeah...
Right. Brain not fully online today. Sorry.
> > Hm, not sure I see how that's protecting itself from someone
> > simultaneously echoing 1 > /sys/devices/system/cpu/cpu${SIBLING}/online
>
> So
>
> echo 1 > ../online
>
> means onlining the sibling.
>
> But reload_store() grabs the CPU hotplug lock *first* and *then* runs
> check_online_cpus() to see if all CPUs are online. It doesn't do the
> update if even one CPU is missing. You can't offline any CPU for the
> duration of the update...
>
> So I guess you'd need to explain in more detail what protection hole
> you're seeing because I might be missing something here.
No, I'd just missed cpus_read_lock() because I was looking for
something else. My fault; it looks fine. Thanks.
On Tue, 2022-02-01 at 11:56 +0100, Borislav Petkov wrote:
> On Tue, Feb 01, 2022 at 10:25:01AM +0000, David Woodhouse wrote:
> > Thanks. It looks like that is only invoked after boot, with a write to
> > /sys/devices/system/cpu/microcode/reload.
> >
> > My series is only parallelising the initial bringup at boot time, so it
> > shouldn't make any difference.
>
> No, I don't mean __reload_late() - I pointed you at that function to
> show the dance we must do when updating microcode late.
>
> The load_ucode_{ap,bsp}() routines are what is called when loading ucode
> early.
>
> So the question is, does the parallelizing change the order in which APs
> are brought up and can it happen that a SMT sibling of a two-SMT core
> executes *something* while the other SMT sibling is updating microcode.
>
> If so, that would be bad.
Right. So as you surmise, I haven't broken that... yet. At least not in
the patches I've posted :)
The call to ucode_cpu_init() is in cpu_init(), right after the call to
wait_for_master_cpu(), which this AP's bit in cpu_initialized_mask and
then waits for the BSP to set its bit in cpu_callout_mask.
That's a full synchronization point with do_wait_cpu_initalized() on
the BSP, which waits for the former and then sets the later.
So... with the series I've posted, all APs end up waiting in
wait_for_master_cpu() until the final serialized bringup.
In the top of my git tree, you can see a half-baked 'parallel part 2'
commit which introduces a new x86/cpu:wait-init cpuhp state that would
invoke do_wait_cpu_initialized() for each CPU in turn, which *would*
release them all into load_ucode_bsp() at the same time and have
precisely the problem you're describing.
I'll commit a FIXME comment now so that it doesn't slip my mind.
Thanks.
> > However... it does look like there's nothing preventing a sibling being
> > brought online *while* the dance you mention above is occurring.
>
> Bottom line is: of the two SMT siblings, one needs to be updating
> microcode while the other is idle. I.e., what __reload_late() does.
>
> > Shouldn't __reload_late() take the device_hotplug_lock to prevent that?
>
> See reload_store().
Hm, not sure I see how that's protecting itself from someone
simultaneously echoing 1 > /sys/devices/system/cpu/cpu${SIBLING}/online
On Tue, Feb 01, 2022 at 12:39:17PM +0000, David Woodhouse wrote:
> In the top of my git tree, you can see a half-baked 'parallel part 2'
> commit which introduces a new x86/cpu:wait-init cpuhp state that would
> invoke do_wait_cpu_initialized() for each CPU in turn, which *would*
> release them all into load_ucode_bsp() at the same time and have
> precisely the problem you're describing.
The load_ucode_bsp() is the variant that runs on the boot CPU but
yeah...
> I'll commit a FIXME comment now so that it doesn't slip my mind.
Yap, thank Cooper for pointing out that whole thing about how microcode
loading is special and can't always handle parallelism. :)
> Hm, not sure I see how that's protecting itself from someone
> simultaneously echoing 1 > /sys/devices/system/cpu/cpu${SIBLING}/online
So
echo 1 > ../online
means onlining the sibling.
But reload_store() grabs the CPU hotplug lock *first* and *then* runs
check_online_cpus() to see if all CPUs are online. It doesn't do the
update if even one CPU is missing. You can't offline any CPU for the
duration of the update...
So I guess you'd need to explain in more detail what protection hole
you're seeing because I might be missing something here.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Feb 01, 2022 at 10:25:01AM +0000, David Woodhouse wrote:
> Thanks. It looks like that is only invoked after boot, with a write to
> /sys/devices/system/cpu/microcode/reload.
>
> My series is only parallelising the initial bringup at boot time, so it
> shouldn't make any difference.
No, I don't mean __reload_late() - I pointed you at that function to
show the dance we must do when updating microcode late.
The load_ucode_{ap,bsp}() routines are what is called when loading ucode
early.
So the question is, does the parallelizing change the order in which APs
are brought up and can it happen that a SMT sibling of a two-SMT core
executes *something* while the other SMT sibling is updating microcode.
If so, that would be bad.
> However... it does look like there's nothing preventing a sibling being
> brought online *while* the dance you mention above is occurring.
Bottom line is: of the two SMT siblings, one needs to be updating
microcode while the other is idle. I.e., what __reload_late() does.
> Shouldn't __reload_late() take the device_hotplug_lock to prevent that?
See reload_store().
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Dear David,
Am 29.12.21 um 14:54 schrieb David Woodhouse:
> On Wed, 2021-12-29 at 14:18 +0100, Paul Menzel wrote:
>>> Or the one in
>>> https://lore.kernel.org/lkml/[email protected]
>>>
>>> which makes it do nothing except prepare all the CPUs before bringing
>>> them up one at a time?
>>
>> I applied it on top the other one, and it made no difference either.
>
> It's possible I missed something else in the prepare stage that doesn't
> cope with all CPUs being prepared first.
>
> My next attempt might be to change the loop in bringup_nonboot_cpus()
> to bring all the CPUs not to the CPUHP_BP_PARALLEL_DYN state(s) but
> instead just bring them to somewhere like CPUHP_RCUTREE_PREP, which is
> somewhere in the middle between CPUHP_OFFLINE and CPUHP_BRINGUP_CPU.
>
> Then a binary chop search — if that one boots, try maybe
> CPUHP_TOPOLOGY_PREPARE. And if not, try CPUHP_PROFILE_PREPARE. Etc.
>
>>> My current theory (not that I've spent that much time thinking about it
>>> in the last week) is that there's something about the existing CPU
>>> bringup, possibly a CPU bug or something special about the AMD CPUs,
>>> which is triggered by just making it a little bit *faster*, which is
>>> why bringing them up from kexec (especially in qemu) can cause it too?
>>
>> Would having the serial console enabled make a difference?
>
> Yes. I couldn't make this fail in my EC2 m6a instance (for clean boots;
> I have never managed to kexec it) until I turned off the serial console
> to make things go faster.
>
>>> Tom seemed to find that it was in load_TR_desc(), so if you could try
>>> this hack on a machine that doesn't magically wink out of existence on
>>> a triplefault before even flushing its serial output, that would be
>>> much appreciated...
>
>> Unfortunately, no more messages were printed on the serial console.
>
> I suppose we need to litter those outputs somewhere earlier in the
> trampoline then, perhaps it *isn't* getting to load_TR_desc() in your
> case?
>
> Will be back online properly next week and can actually provide some of
> the above suggestions in patch form if you're willing to keep testing.
Sorry for replying so late. I saw your v4 patches, and tried commit
5e3524d21d2a () from your branch `parallel-5.17-part1`. Unfortunately,
the boot problem still persists on an AMD Ryzen 3 2200 g system, I
tested with. Please tell, where I should report these results too (here
or posted v4 patches).
Also, do you have (physical) access to a system with an AMD CPU? If not,
maybe we can get you one, so it’s more convenient for you to test.
Kind regards,
Paul
Dear Paul,
> Sorry for replying so late. I saw your v4 patches, and tried commit
> 5e3524d21d2a () from your branch `parallel-5.17-part1`. Unfortunately,
> the boot problem still persists on an AMD Ryzen 3 2200 g system, I
> tested with. Please tell, where I should report these results too
> (here or posted v4 patches).
We have confirmed the issue on multiple AMD CPUs from multiple
generations, leading to the guess that only Zen and Zen+ CPU seem
affected with Zen3 and Zen2 (only tested ulv) working fine. Tho we
struggled to get any output as the failing machines just go silent.
Not working:
Ryzen 5 Pro 2500u and 7 2700U
Ryzen 3 2300G
while e.g.
Ryzen 7 Pro 4750U
Ryzen 9 5950X
both work fine. We will continue to investigate the issue but are
currently a bit pulled into other topics.
Thomas, could please maybe help us identify which CPUs and MC-Versions
are worth looking at? David suggested you might have a good overview here.
Best regards
Johanna "Mimoja"
On 4/21/22 05:00, Mimoja wrote:
> Dear Paul,
>
>> Sorry for replying so late. I saw your v4 patches, and tried commit
>> 5e3524d21d2a () from your branch `parallel-5.17-part1`. Unfortunately,
>> the boot problem still persists on an AMD Ryzen 3 2200 g system, I
>> tested with. Please tell, where I should report these results too (here
>> or posted v4 patches).
>
> We have confirmed the issue on multiple AMD CPUs from multiple
> generations, leading to the guess that only Zen and Zen+ CPU seem affected
> with Zen3 and Zen2 (only tested ulv) working fine. Tho we struggled to get
> any output as the failing machines just go silent.
>
> Not working:
>
> Ryzen 5 Pro 2500u and 7 2700U
> Ryzen 3 2300G
>
> while e.g.
>
> Ryzen 7 Pro 4750U
> Ryzen 9 5950X
>
> both work fine. We will continue to investigate the issue but are
> currently a bit pulled into other topics.
>
> Thomas, could please maybe help us identify which CPUs and MC-Versions are
> worth looking at? David suggested you might have a good overview here.
Sorry, but not knowing what the actual reason for the boot problem, I
really couldn't give you an idea as to which CPUs and/or MC versions are
appropriate to look at.
Thanks,
Tom
>
>
> Best regards
>
> Johanna "Mimoja"
>
On Fri, 2022-04-22 at 16:19 -0500, Tom Lendacky wrote:
> On 4/21/22 05:00, Mimoja wrote:
> > Dear Paul,
> >
> > > Sorry for replying so late. I saw your v4 patches, and tried commit
> > > 5e3524d21d2a () from your branch `parallel-5.17-part1`. Unfortunately,
> > > the boot problem still persists on an AMD Ryzen 3 2200 g system, I
> > > tested with. Please tell, where I should report these results too (here
> > > or posted v4 patches).
> >
> > We have confirmed the issue on multiple AMD CPUs from multiple
> > generations, leading to the guess that only Zen and Zen+ CPU seem affected
> > with Zen3 and Zen2 (only tested ulv) working fine. Tho we struggled to get
> > any output as the failing machines just go silent.
> >
> > Not working:
> >
> > Ryzen 5 Pro 2500u and 7 2700U
> > Ryzen 3 2300G
> >
> > while e.g.
> >
> > Ryzen 7 Pro 4750U
> > Ryzen 9 5950X
> >
> > both work fine. We will continue to investigate the issue but are
> > currently a bit pulled into other topics.
> >
> > Thomas, could please maybe help us identify which CPUs and MC-Versions are
> > worth looking at? David suggested you might have a good overview here.
>
> Sorry, but not knowing what the actual reason for the boot problem, I
> really couldn't give you an idea as to which CPUs and/or MC versions are
> appropriate to look at.
Well, that's kind of the point... we don't *know* what the problem is.
We think we've eliminated the software concurrency issues, and it seems
like the hardware just dies if you happen to bring up the CPUs 'too
fast'.
If we could get you to reproduce it in a lab and help work out what's
going on, it would be much appreciated!