2023-03-02 11:13:08

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 00/11] Parallel CPU bringup for x86_64

The main code change over v12 is to fix the build error when
CONFIG_FORCE_NR_CPUS is present.

The commit message for removing initial stack has also been improved, typos
have been fixed and extra comments have been added to make code clearer.

Thanks,
Usama

Changes across versions:
v2: Cut it back to just INIT/SIPI/SIPI in parallel for now, nothing more
v3: Clean up x2apic patch, add MTRR optimisation, lock topology update
in preparation for more parallelisation.
v4: Fixes to the real mode parallelisation patch spotted by SeanC, to
avoid scribbling on initial_gs in common_cpu_up(), and to allow all
24 bits of the physical X2APIC ID to be used. That patch still needs
a Signed-off-by from its original author, who once claimed not to
remember writing it at all. But now we've fixed it, hopefully he'll
admit it now :)
v5: rebase to v6.1 and remeasure performance, disable parallel bringup
for AMD CPUs.
v6: rebase to v6.2-rc6, disabled parallel boot on amd as a cpu bug and
reused timer calibration for secondary CPUs.
v7: [David Woodhouse] iterate over all possible CPUs to find any existing
cluster mask in alloc_clustermask. (patch 1/9)
Keep parallel AMD support enabled in AMD, using APIC ID in CPUID leaf
0x0B (for x2APIC mode) or CPUID leaf 0x01 where 8 bits are sufficient.
Included sanity checks for APIC id from 0x0B. (patch 6/9)
Removed patch for reusing timer calibration for secondary CPUs.
commit message and code improvements.
v8: Fix CPU0 hotplug by setting up the initial_gs, initial_stack and
early_gdt_descr.
Drop trampoline lock and bail if APIC ID not found in find_cpunr.
Code comments improved and debug prints added.
v9: Drop patch to avoid repeated saves of MTRR at boot time.
rebased and retested at v6.2-rc8.
added kernel doc for no_parallel_bringup and made do_parallel_bringup
__ro_after_init.
v10: Fixed suspend/resume not working with parallel smpboot.
rebased and retested to 6.2.
fixed checkpatch errors.
v11: Added patches from Brian Gerst to remove the global variables initial_gs,
initial_stack, and early_gdt_descr from the 64-bit boot code
(https://lore.kernel.org/all/[email protected]/).
v12: Fixed compilation errors, acquire tr_lock for every stack setup in
trampoline_64.S.
Rearranged commits for a cleaner git history.
v13: Fix build error with CONFIG_FORCE_NR_CPUS.
Commit message improved, typos fixed and extra comments added.

Brian Gerst (3):
x86/smpboot: Remove initial_stack on 64-bit
x86/smpboot: Remove early_gdt_descr on 64-bit
x86/smpboot: Remove initial_gs

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

.../admin-guide/kernel-parameters.txt | 3 +
arch/x86/include/asm/processor.h | 6 +-
arch/x86/include/asm/realmode.h | 4 +-
arch/x86/include/asm/smp.h | 15 +-
arch/x86/include/asm/topology.h | 2 -
arch/x86/kernel/acpi/sleep.c | 30 +-
arch/x86/kernel/apic/apic.c | 2 +-
arch/x86/kernel/apic/x2apic_cluster.c | 126 ++++---
arch/x86/kernel/asm-offsets.c | 1 +
arch/x86/kernel/cpu/common.c | 6 +-
arch/x86/kernel/head_64.S | 132 +++++--
arch/x86/kernel/smpboot.c | 350 +++++++++++++-----
arch/x86/realmode/init.c | 3 +
arch/x86/realmode/rm/trampoline_64.S | 27 +-
arch/x86/xen/smp_pv.c | 4 +-
arch/x86/xen/xen-head.S | 2 +-
include/linux/cpuhotplug.h | 2 +
include/linux/smpboot.h | 7 +
kernel/cpu.c | 31 +-
kernel/smpboot.h | 2 -
20 files changed, 556 insertions(+), 199 deletions(-)

--
2.25.1



2023-03-02 11:13:12

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 10/11] x86/smpboot: Send INIT/SIPI/SIPI to secondary CPUs in parallel

From: David Woodhouse <[email protected]>

When the APs can find their own APIC ID without assistance, perform the
AP bringup in parallel.

Register a CPUHP_BP_PARALLEL_DYN stage "x86/cpu:kick" which just calls
do_boot_cpu() to deliver INIT/SIPI/SIPI to each AP in turn before the
normal native_cpu_up() does the rest of the hand-holding.

The APs will then take turns through the real mode code (which has its
own bitlock for exclusion) until they make it to their own stack, then
proceed through the first few lines of start_secondary() and execute
these parts in parallel:

start_secondary()
-> cr4_init()
-> (some 32-bit only stuff so not in the parallel cases)
-> cpu_init_secondary()
-> cpu_init_exception_handling()
-> cpu_init()
-> wait_for_master_cpu()

At this point they wait for the BSP to set their bit in cpu_callout_mask
(from do_wait_cpu_initialized()), and release them to continue through
the rest of cpu_init() and beyond.

This reduces the time taken for bringup on my 28-thread Haswell system
from about 120ms to 80ms. On a socket 96-thread Skylake it takes the
bringup time from 500ms to 100ms.

There is more speedup to be had by doing the remaining parts in parallel
too — especially notify_cpu_starting() in which the AP takes itself
through all the stages from CPUHP_BRINGUP_CPU to CPUHP_ONLINE. But those
require careful auditing to ensure they are reentrant, before we can go
that far.

Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Tested-by: Oleksandr Natalenko <[email protected]>
Tested-by: Guilherme G. Piccoli <[email protected]>
---
arch/x86/kernel/smpboot.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 19b9b89b7458..711573cd9b87 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -57,6 +57,7 @@
#include <linux/pgtable.h>
#include <linux/overflow.h>
#include <linux/stackprotector.h>
+#include <linux/smpboot.h>

#include <asm/acpi.h>
#include <asm/cacheinfo.h>
@@ -1325,9 +1326,12 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
{
int ret;

- ret = do_cpu_up(cpu, tidle);
- if (ret)
- return ret;
+ /* If parallel AP bringup isn't enabled, perform the first steps now. */
+ if (!do_parallel_bringup) {
+ ret = do_cpu_up(cpu, tidle);
+ if (ret)
+ return ret;
+ }

ret = do_wait_cpu_initialized(cpu);
if (ret)
@@ -1349,6 +1353,12 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
return ret;
}

+/* Bringup step one: Send INIT/SIPI to the target AP */
+static int native_cpu_kick(unsigned int cpu)
+{
+ return do_cpu_up(cpu, idle_thread_get(cpu));
+}
+
/**
* arch_disable_smp_support() - disables SMP support for x86 at runtime
*/
@@ -1566,6 +1576,11 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
smpboot_control = STARTUP_APICID_CPUID_01;
}

+ if (do_parallel_bringup) {
+ cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
+ native_cpu_kick, NULL);
+ }
+
snp_set_wakeup_secondary_cpu();
}

--
2.25.1


2023-03-02 11:13:16

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 02/11] cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>

From: David Woodhouse <[email protected]>

Instead of relying purely on the special-case wrapper in bringup_cpu()
to pass the idle thread to __cpu_up(), expose idle_thread_get() so that
the architecture code can obtain it directly when necessary.

This will be useful when the existing __cpu_up() is split into multiple
phases, only *one* of which will actually need the idle thread.

If the architecture code is to register its new pre-bringup states with
the cpuhp core, having a special-case wrapper to pass extra arguments is
non-trivial and it's easier just to let the arch register its function
pointer to be invoked with the standard API.

Signed-off-by: David Woodhouse <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Tested-by: Oleksandr Natalenko <[email protected]>
Tested-by: Guilherme G. Piccoli <[email protected]>
---
include/linux/smpboot.h | 7 +++++++
kernel/smpboot.h | 2 --
2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
index 9d1bc65d226c..3862addcaa34 100644
--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -5,6 +5,13 @@
#include <linux/types.h>

struct task_struct;
+
+#ifdef CONFIG_GENERIC_SMP_IDLE_THREAD
+struct task_struct *idle_thread_get(unsigned int cpu);
+#else
+static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; }
+#endif
+
/* Cookie handed to the thread_fn*/
struct smpboot_thread_data;

diff --git a/kernel/smpboot.h b/kernel/smpboot.h
index 34dd3d7ba40b..60c609318ad6 100644
--- a/kernel/smpboot.h
+++ b/kernel/smpboot.h
@@ -5,11 +5,9 @@
struct task_struct;

#ifdef CONFIG_GENERIC_SMP_IDLE_THREAD
-struct task_struct *idle_thread_get(unsigned int cpu);
void idle_thread_set_boot_cpu(void);
void idle_threads_init(void);
#else
-static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; }
static inline void idle_thread_set_boot_cpu(void) { }
static inline void idle_threads_init(void) { }
#endif
--
2.25.1


2023-03-02 11:13:20

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 03/11] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU

From: David Woodhouse <[email protected]>

There is often significant latency in the early stages of CPU bringup,
and time is wasted by waking each CPU (e.g. with SIPI/INIT/INIT on x86)
and then waiting for it to respond before moving on to the next.

Allow a platform to register a set of pre-bringup CPUHP states to which
each CPU can be stepped in parallel, thus absorbing some of that latency.

There is a subtlety here: even with an empty CPUHP_BP_PARALLEL_DYN step,
this means that *all* CPUs are brought through the prepare states and to
CPUHP_BP_PREPARE_DYN before any of them are taken to CPUHP_BRINGUP_CPU
and then are allowed to run for themselves to CPUHP_ONLINE.

So any combination of prepare/start calls which depend on A-B ordering
for each CPU in turn, such as the X2APIC code which used to allocate a
cluster mask 'just in case' and store it in a global variable in the
prep stage, then potentially consume that preallocated structure from
the AP and set the global pointer to NULL to be reallocated in
CPUHP_X2APIC_PREPARE for the next CPU... would explode horribly.

Any platform enabling the CPUHP_BP_PARALLEL_DYN steps must be reviewed
and tested to ensure that such issues do not exist, and the existing
behaviour of bringing CPUs to CPUHP_BP_PREPARE_DYN and then immediately
to CPUHP_BRINGUP_CPU and CPUHP_ONLINE only one at a time does not change
unless such a state is registered.

Note that the new parallel stages do *not* yet bring each AP to the
CPUHP_BRINGUP_CPU state at the same time, only to the new states which
exist before it. The final loop in bringup_nonboot_cpus() is untouched,
bringing each AP in turn from the final PARALLEL_DYN state (or all the
way from CPUHP_OFFLINE) to CPUHP_BRINGUP_CPU and then waiting for that
AP to do its own processing and reach CPUHP_ONLINE before releasing the
next.

Parallelising that part by bringing them all to CPUHP_BRINGUP_CPU
and then waiting for them all is an exercise for the future.

Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Tested-by: Oleksandr Natalenko <[email protected]>
Tested-by: Guilherme G. Piccoli <[email protected]>
---
include/linux/cpuhotplug.h | 2 ++
kernel/cpu.c | 31 +++++++++++++++++++++++++++++--
2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 6c6859bfc454..e5a73ae6ccc0 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -133,6 +133,8 @@ enum cpuhp_state {
CPUHP_MIPS_SOC_PREPARE,
CPUHP_BP_PREPARE_DYN,
CPUHP_BP_PREPARE_DYN_END = CPUHP_BP_PREPARE_DYN + 20,
+ CPUHP_BP_PARALLEL_DYN,
+ CPUHP_BP_PARALLEL_DYN_END = CPUHP_BP_PARALLEL_DYN + 4,
CPUHP_BRINGUP_CPU,

/*
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6c0a92ca6bb5..fffb0da61ccc 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1504,8 +1504,30 @@ int bringup_hibernate_cpu(unsigned int sleep_cpu)

void bringup_nonboot_cpus(unsigned int setup_max_cpus)
{
+ unsigned int n = setup_max_cpus - num_online_cpus();
unsigned int cpu;

+ /*
+ * An architecture may have registered parallel pre-bringup states to
+ * which each CPU may be brought in parallel. For each such state,
+ * bring N CPUs to it in turn before the final round of bringing them
+ * online.
+ */
+ if (n > 0) {
+ enum cpuhp_state st = CPUHP_BP_PARALLEL_DYN;
+
+ while (st <= CPUHP_BP_PARALLEL_DYN_END && cpuhp_hp_states[st].name) {
+ int i = n;
+
+ for_each_present_cpu(cpu) {
+ cpu_up(cpu, st);
+ if (!--i)
+ break;
+ }
+ st++;
+ }
+ }
+
for_each_present_cpu(cpu) {
if (num_online_cpus() >= setup_max_cpus)
break;
@@ -1882,6 +1904,10 @@ static int cpuhp_reserve_state(enum cpuhp_state state)
step = cpuhp_hp_states + CPUHP_BP_PREPARE_DYN;
end = CPUHP_BP_PREPARE_DYN_END;
break;
+ case CPUHP_BP_PARALLEL_DYN:
+ step = cpuhp_hp_states + CPUHP_BP_PARALLEL_DYN;
+ end = CPUHP_BP_PARALLEL_DYN_END;
+ break;
default:
return -EINVAL;
}
@@ -1906,14 +1932,15 @@ static int cpuhp_store_callbacks(enum cpuhp_state state, const char *name,
/*
* If name is NULL, then the state gets removed.
*
- * CPUHP_AP_ONLINE_DYN and CPUHP_BP_PREPARE_DYN are handed out on
+ * CPUHP_AP_ONLINE_DYN and CPUHP_BP_P*_DYN are handed out on
* the first allocation from these dynamic ranges, so the removal
* would trigger a new allocation and clear the wrong (already
* empty) state, leaving the callbacks of the to be cleared state
* dangling, which causes wreckage on the next hotplug operation.
*/
if (name && (state == CPUHP_AP_ONLINE_DYN ||
- state == CPUHP_BP_PREPARE_DYN)) {
+ state == CPUHP_BP_PREPARE_DYN ||
+ state == CPUHP_BP_PARALLEL_DYN)) {
ret = cpuhp_reserve_state(state);
if (ret < 0)
return ret;
--
2.25.1


2023-03-02 11:13:24

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 11/11] x86/smpboot: Serialize topology updates for secondary bringup

From: David Woodhouse <[email protected]>

The toplogy update is performed by the AP via smp_callin() after the BSP
has called do_wait_cpu_initialized(), setting the AP's bit in
cpu_callout_mask to allow it to proceed.

In preparation to enable further parallelism of AP bringup, add locking to
serialize the update even if multiple APs are (in future) permitted to
proceed through the next stages of bringup in parallel.

Without such ordering (and with that future extra parallelism), confusion
ensues:

[ 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)

Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Tested-by: Oleksandr Natalenko <[email protected]>
Tested-by: Guilherme G. Piccoli <[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 1cf4f1e57570..defe76ee9e64 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -48,8 +48,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;

@@ -137,7 +135,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 458c891a8273..4bccbd949a99 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -136,8 +136,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 f3cc7699e1e1..06d7f9e55d45 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1771,7 +1771,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();
@@ -1782,8 +1782,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
@@ -1974,7 +1972,7 @@ void identify_secondary_cpu(struct cpuinfo_x86 *c)
#ifdef CONFIG_X86_32
enable_sep_cpu();
#endif
- 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 711573cd9b87..9d956571ecc1 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -180,16 +180,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);

ap_init_aperfmperf();

@@ -243,6 +239,12 @@ static void notrace start_secondary(void *unused)
* its bit in cpu_callout_mask to release it.
*/
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();

@@ -351,7 +353,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;

@@ -374,7 +376,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;

@@ -405,25 +407,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 6175f2c5c822..09f94f940689 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.25.1


2023-03-02 11:13:39

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 04/11] x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()

From: David Woodhouse <[email protected]>

When bringing up a secondary CPU from do_boot_cpu(), the warm reset flag
is set in CMOS and the starting IP for the trampoline written inside the
BDA at 0x467. Once the CPU is running, the CMOS flag is unset and the
value in the BDA cleared.

To allow for parallel bringup of CPUs, add a reference count to track the
number of CPUs currently bring brought up, and clear the state only when
the count reaches zero.

Since the RTC spinlock is required to write to the CMOS, it can be used
for mutual exclusion on the refcount too.

Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Tested-by: Oleksandr Natalenko <[email protected]>
Tested-by: Guilherme G. Piccoli <[email protected]>
---
arch/x86/kernel/smpboot.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 55cad72715d9..3a793772a2aa 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -121,17 +121,20 @@ int arch_update_cpu_topology(void)
return retval;
}

+
+static unsigned int smpboot_warm_reset_vector_count;
+
static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
{
unsigned long flags;

spin_lock_irqsave(&rtc_lock, flags);
- CMOS_WRITE(0xa, 0xf);
+ if (!smpboot_warm_reset_vector_count++) {
+ CMOS_WRITE(0xa, 0xf);
+ *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) = start_eip >> 4;
+ *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = start_eip & 0xf;
+ }
spin_unlock_irqrestore(&rtc_lock, flags);
- *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
- start_eip >> 4;
- *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
- start_eip & 0xf;
}

static inline void smpboot_restore_warm_reset_vector(void)
@@ -143,10 +146,12 @@ static inline void smpboot_restore_warm_reset_vector(void)
* to default values.
*/
spin_lock_irqsave(&rtc_lock, flags);
- CMOS_WRITE(0, 0xf);
+ if (!--smpboot_warm_reset_vector_count) {
+ CMOS_WRITE(0, 0xf);
+ *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
+ }
spin_unlock_irqrestore(&rtc_lock, flags);

- *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
}

/*
--
2.25.1


2023-03-02 11:13:44

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 06/11] x86/smpboot: Remove initial_stack on 64-bit

From: Brian Gerst <[email protected]>

In order to facilitate parallel startup, start to eliminate some of the
global variables passing information to CPUs in the startup path.

However, start by introducing one more: smpboot_control. For now this
merely holds the CPU# of the CPU which is coming up. Each CPU can then
find its own per-cpu data, and everything else it needs can be found
from there, allowing the other global variables to be removed.

First to be removed is initial_stack. Each CPU can load %rsp from its
current_task->thread.sp instead. That is already set up with the correct
idle thread for APs. Set up the .sp field in INIT_THREAD on x86 so that
the BSP also finds a suitable stack pointer in the static per-cpu data
when coming up on first boot.

On resume from S3, the CPU needs a temporary stack because its idle task
is already active. Instead of setting initial_stack, the sleep code can
simply set its own current->thread.sp to point to the temporary stack.
Nobody else cares about ->thread.sp for a thread which is currently on
a CPU, because the true value is actually in the %rsp register. Which
is restored with the rest of the CPU context in do_suspend_lowlevel().

Signed-off-by: Brian Gerst <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
Tested-by: Usama Arif <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Guilherme G. Piccoli <[email protected]>
---
arch/x86/include/asm/processor.h | 6 ++++-
arch/x86/include/asm/smp.h | 5 +++-
arch/x86/kernel/acpi/sleep.c | 20 +++++++++++++--
arch/x86/kernel/asm-offsets.c | 1 +
arch/x86/kernel/head_64.S | 43 +++++++++++++++++++++-----------
arch/x86/kernel/smpboot.c | 7 +++++-
arch/x86/xen/xen-head.S | 2 +-
7 files changed, 63 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4e35c66edeb7..bdde7316e75b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -648,7 +648,11 @@ static inline void spin_lock_prefetch(const void *x)
#define KSTK_ESP(task) (task_pt_regs(task)->sp)

#else
-#define INIT_THREAD { }
+extern unsigned long __end_init_task[];
+
+#define INIT_THREAD { \
+ .sp = (unsigned long)&__end_init_task - sizeof(struct pt_regs), \
+}

extern unsigned long KSTK_ESP(struct task_struct *task);

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index b4dbb20dab1a..bf2c51df9e0b 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -199,5 +199,8 @@ extern void nmi_selftest(void);
#define nmi_selftest() do { } while (0)
#endif

-#endif /* __ASSEMBLY__ */
+extern unsigned int smpboot_control;
+
+#endif /* !__ASSEMBLY__ */
+
#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3b7f4cdbf2e0..1b4c43d0819a 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -111,13 +111,29 @@ int x86_acpi_suspend_lowlevel(void)
saved_magic = 0x12345678;
#else /* CONFIG_64BIT */
#ifdef CONFIG_SMP
- initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
+ /*
+ * As each CPU starts up, it will find its own stack pointer
+ * from its current_task->thread.sp. Typically that will be
+ * the idle thread for a newly-started AP, or even the boot
+ * CPU which will find it set to &init_task in the static
+ * per-cpu data.
+ *
+ * Make the resuming CPU use the temporary stack at startup
+ * by setting current->thread.sp to point to that. The true
+ * %rsp will be restored with the rest of the CPU context,
+ * by do_suspend_lowlevel(). And unwinders don't care about
+ * the abuse of ->thread.sp because it's a dead variable
+ * while the thread is running on the CPU anyway; the true
+ * value is in the actual %rsp register.
+ */
+ current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
early_gdt_descr.address =
(unsigned long)get_cpu_gdt_rw(smp_processor_id());
initial_gs = per_cpu_offset(smp_processor_id());
+ smpboot_control = smp_processor_id();
#endif
initial_code = (unsigned long)wakeup_long64;
- saved_magic = 0x123456789abcdef0L;
+ saved_magic = 0x123456789abcdef0L;
#endif /* CONFIG_64BIT */

/*
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 82c783da16a8..797ae1a15c91 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -108,6 +108,7 @@ static void __used common(void)
OFFSET(TSS_sp1, tss_struct, x86_tss.sp1);
OFFSET(TSS_sp2, tss_struct, x86_tss.sp2);
OFFSET(X86_top_of_stack, pcpu_hot, top_of_stack);
+ OFFSET(X86_current_task, pcpu_hot, current_task);
#ifdef CONFIG_CALL_DEPTH_TRACKING
OFFSET(X86_call_depth, pcpu_hot, call_depth);
#endif
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 222efd4a09bc..cc1b145055ac 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -61,8 +61,8 @@ SYM_CODE_START_NOALIGN(startup_64)
* tables and then reload them.
*/

- /* Set up the stack for verify_cpu(), similar to initial_stack below */
- leaq (__end_init_task - FRAME_SIZE)(%rip), %rsp
+ /* Set up the stack for verify_cpu() */
+ leaq (__end_init_task - PTREGS_SIZE)(%rip), %rsp

leaq _text(%rip), %rdi

@@ -241,6 +241,24 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
UNWIND_HINT_EMPTY
ANNOTATE_NOENDBR // above

+#ifdef CONFIG_SMP
+ movl smpboot_control(%rip), %ecx
+
+ /* Get the per cpu offset for the given CPU# which is in ECX */
+ movq __per_cpu_offset(,%rcx,8), %rdx
+#else
+ xorl %edx, %edx /* zero-extended to clear all of RDX */
+#endif /* CONFIG_SMP */
+
+ /*
+ * Setup a boot time stack - Any secondary CPU will have lost its stack
+ * by now because the cr3-switch above unmaps the real-mode stack.
+ *
+ * RDX contains the per-cpu offset
+ */
+ movq pcpu_hot + X86_current_task(%rdx), %rax
+ movq TASK_threadsp(%rax), %rsp
+
/*
* 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
@@ -275,12 +293,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
movl initial_gs+4(%rip),%edx
wrmsr

- /*
- * Setup a boot time stack - Any secondary CPU will have lost its stack
- * by now because the cr3-switch above unmaps the real-mode stack
- */
- movq initial_stack(%rip), %rsp
-
/* Setup and Load IDT */
pushq %rsi
call early_setup_idt
@@ -372,7 +384,11 @@ SYM_CODE_END(secondary_startup_64)
SYM_CODE_START(start_cpu0)
ANNOTATE_NOENDBR
UNWIND_HINT_EMPTY
- movq initial_stack(%rip), %rsp
+
+ /* Find the idle task stack */
+ movq PER_CPU_VAR(pcpu_hot) + X86_current_task, %rcx
+ movq TASK_threadsp(%rcx), %rsp
+
jmp .Ljump_to_C_code
SYM_CODE_END(start_cpu0)
#endif
@@ -420,12 +436,6 @@ SYM_DATA(initial_gs, .quad INIT_PER_CPU_VAR(fixed_percpu_data))
#ifdef CONFIG_AMD_MEM_ENCRYPT
SYM_DATA(initial_vc_handler, .quad handle_vc_boot_ghcb)
#endif
-
-/*
- * The FRAME_SIZE gap is a convention which helps the in-kernel unwinder
- * reliably detect the end of the stack.
- */
-SYM_DATA(initial_stack, .quad init_thread_union + THREAD_SIZE - FRAME_SIZE)
__FINITDATA

__INIT
@@ -660,6 +670,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 b18c1385e181..62e3bf37f0b8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1112,7 +1112,12 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
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)) {
+ initial_stack = idle->thread.sp;
+ } else {
+ smpboot_control = cpu;
+ }

/* Enable the espfix hack for this CPU */
init_espfix_ap(cpu);
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index ffaa62167f6e..6bd391476656 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -49,7 +49,7 @@ SYM_CODE_START(startup_xen)
ANNOTATE_NOENDBR
cld

- mov initial_stack(%rip), %rsp
+ leaq (__end_init_task - PTREGS_SIZE)(%rip), %rsp

/* Set up %gs.
*
--
2.25.1


2023-03-02 11:13:49

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 09/11] x86/smpboot: Support parallel startup of secondary CPUs

From: David Woodhouse <[email protected]>

Rework the real-mode startup code to allow for APs to be brought up in
parallel. This is in two parts:

1. Introduce a bit-spinlock to prevent them from all using the real
mode stack at the same time.

2. Avoid needing to use the global smpboot_control variable to pass
each AP its CPU#.

To achieve the latter, export the cpuid_to_apicid[] array so that each
AP can find its own CPU# by searching therein based on its APIC ID.

Introduce flags in the top bits of smpboot_control which indicate methods
by which an AP should find its CPU#. For a serialized bringup, the CPU#
is explicitly passed in the low bits of smpboot_control as before. For
parallel mode there are flags directing the AP to find its APIC ID in
CPUID leaf 0x0b (for X2APIC mode) or CPUID leaf 0x01 where 8 bits are
sufficient, then perform the cpuid_to_apicid[] lookup with that.

Parallel startup may be disabled by a command line option, and also if:
• AMD SEV-ES is in use, since the AP may not use CPUID that early.
• X2APIC is enabled, but CPUID leaf 0xb is not present and correct.
• X2APIC is not enabled but not even CPUID leaf 0x01 exists.

Aside from the fact that APs will now look up their CPU# via the
newly-exported cpuid_to_apicid[] table, there is no behavioural change
intended yet, since new parallel CPUHP states have not — yet — been
added.

[ tglx: Initial proof of concept patch with bitlock and APIC ID lookup ]
[ dwmw2: Rework and testing, commit message, CPUID 0x1 and CPU0 support ]
[ seanc: Fix stray override of initial_gs in common_cpu_up() ]
[ Oleksandr Natalenko: reported suspend/resume issue fixed in
x86_acpi_suspend_lowlevel ]
Co-developed-by: Thomas Gleixner <[email protected]>
Co-developed-by: Brian Gerst <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Brian Gerst <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Paul E. McKenney <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Tested-by: Oleksandr Natalenko <[email protected]>
Tested-by: Guilherme G. Piccoli <[email protected]>
---
.../admin-guide/kernel-parameters.txt | 3 +
arch/x86/include/asm/realmode.h | 3 +
arch/x86/include/asm/smp.h | 6 ++
arch/x86/kernel/acpi/sleep.c | 9 ++-
arch/x86/kernel/apic/apic.c | 2 +-
arch/x86/kernel/head_64.S | 64 +++++++++++++++++++
arch/x86/kernel/smpboot.c | 53 ++++++++++++++-
arch/x86/realmode/init.c | 3 +
arch/x86/realmode/rm/trampoline_64.S | 27 ++++++--
9 files changed, 162 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 6cfa6e3996cf..7bb7020f97e2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3819,6 +3819,9 @@

nomodule Disable module load

+ no_parallel_bringup
+ [X86,SMP] Disable parallel bring-up of secondary cores.
+
nopat [X86] Disable PAT (page attribute table extension of
pagetables) support.

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index f6a1737c77be..87e5482acd0d 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -52,6 +52,7 @@ struct trampoline_header {
u64 efer;
u32 cr4;
u32 flags;
+ u32 lock;
#endif
};

@@ -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 bf2c51df9e0b..1cf4f1e57570 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -203,4 +203,10 @@ extern unsigned int smpboot_control;

#endif /* !__ASSEMBLY__ */

+/* Control bits for startup_64 */
+#define STARTUP_APICID_CPUID_0B 0x80000000
+#define STARTUP_APICID_CPUID_01 0x40000000
+
+#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | STARTUP_APICID_CPUID_0B)
+
#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 1328c221af30..6dfecb27b846 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -16,6 +16,7 @@
#include <asm/cacheflush.h>
#include <asm/realmode.h>
#include <asm/hypervisor.h>
+#include <asm/smp.h>

#include <linux/ftrace.h>
#include "../../realmode/rm/wakeup.h"
@@ -127,7 +128,13 @@ int x86_acpi_suspend_lowlevel(void)
* value is in the actual %rsp register.
*/
current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
- smpboot_control = smp_processor_id();
+ /*
+ * Ensure the CPU knows which one it is when it comes back, if
+ * it isn't in parallel mode and expected to work that out for
+ * itself.
+ */
+ if (!(smpboot_control & STARTUP_PARALLEL_MASK))
+ smpboot_control = smp_processor_id();
#endif
initial_code = (unsigned long)wakeup_long64;
saved_magic = 0x123456789abcdef0L;
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 20d9a604da7c..ac1d7e5da1f2 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2377,7 +2377,7 @@ static int nr_logical_cpuids = 1;
/*
* Used to store mapping between logical CPU IDs and APIC IDs.
*/
-static int cpuid_to_apicid[] = {
+int cpuid_to_apicid[] = {
[0 ... NR_CPUS - 1] = -1,
};

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 6a8238702eab..c35f7c173832 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
@@ -234,8 +235,61 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
ANNOTATE_NOENDBR // above

#ifdef CONFIG_SMP
+ /*
+ * For parallel boot, the APIC ID is retrieved from CPUID, and then
+ * used to look up the CPU number. For booting a single CPU, the
+ * CPU number is encoded in smpboot_control.
+ *
+ * Bit 31 STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
+ * Bit 30 STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
+ * Bit 0-24 CPU# if STARTUP_APICID_CPUID_xx flags are not set
+ */
movl smpboot_control(%rip), %ecx
+ testl $STARTUP_APICID_CPUID_0B, %ecx
+ jnz .Luse_cpuid_0b
+ testl $STARTUP_APICID_CPUID_01, %ecx
+ jnz .Luse_cpuid_01
+ andl $0x0FFFFFFF, %ecx
+ jmp .Lsetup_cpu
+
+.Luse_cpuid_01:
+ mov $0x01, %eax
+ cpuid
+ mov %ebx, %edx
+ shr $24, %edx
+ jmp .Lsetup_AP

+.Luse_cpuid_0b:
+ mov $0x0B, %eax
+ xorl %ecx, %ecx
+ cpuid
+
+.Lsetup_AP:
+ /* EDX contains the APIC ID of the current CPU */
+ xorq %rcx, %rcx
+ leaq cpuid_to_apicid(%rip), %rbx
+
+.Lfind_cpunr:
+ cmpl (%rbx,%rcx,4), %edx
+ jz .Lsetup_cpu
+ inc %ecx
+#ifdef CONFIG_FORCE_NR_CPUS
+ cmpl $NR_CPUS, %ecx
+#else
+ cmpl nr_cpu_ids(%rip), %ecx
+#endif
+ jb .Lfind_cpunr
+
+ /* APIC ID not found in the table. Drop the trampoline lock and bail. */
+ movq trampoline_lock(%rip), %rax
+ lock
+ btrl $0, (%rax)
+
+1: cli
+ hlt
+ jmp 1b
+
+.Lsetup_cpu:
/* Get the per cpu offset for the given CPU# which is in ECX */
movq __per_cpu_offset(,%rcx,8), %rdx
#else
@@ -293,6 +347,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
shrq $32, %rdx
wrmsr

+ /* 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
@@ -435,6 +497,8 @@ SYM_DATA(initial_code, .quad x86_64_start_kernel)
#ifdef CONFIG_AMD_MEM_ENCRYPT
SYM_DATA(initial_vc_handler, .quad handle_vc_boot_ghcb)
#endif
+
+SYM_DATA(trampoline_lock, .quad 0);
__FINITDATA

__INIT
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index b04520085582..19b9b89b7458 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -797,6 +797,16 @@ static int __init cpu_init_udelay(char *str)
}
early_param("cpu_init_udelay", cpu_init_udelay);

+static bool do_parallel_bringup __ro_after_init = 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 */
@@ -1113,7 +1123,7 @@ 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 {
+ } else if (!do_parallel_bringup) {
smpboot_control = cpu;
}

@@ -1515,6 +1525,47 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)

speculative_store_bypass_ht_init();

+ /*
+ * We can do 64-bit AP bringup in parallel if the CPU reports
+ * its APIC ID in CPUID (either leaf 0x0B if we need the full
+ * APIC ID in X2APIC mode, or leaf 0x01 if 8 bits are
+ * sufficient). Otherwise it's too hard. And not for SEV-ES
+ * guests because they can't use CPUID that early.
+ */
+ if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1 ||
+ (x2apic_mode && boot_cpu_data.cpuid_level < 0xb) ||
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+ do_parallel_bringup = false;
+
+ if (do_parallel_bringup && x2apic_mode) {
+ unsigned int eax, ebx, ecx, edx;
+
+ /*
+ * To support parallel bringup in x2apic mode, the AP will need
+ * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
+ * only 8 bits. Check that it is present and seems correct.
+ */
+ cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
+
+ /*
+ * AMD says that if executed with an umimplemented level in
+ * ECX, then it will return all zeroes in EAX. Intel says it
+ * will return zeroes in both EAX and EBX. Checking only EAX
+ * should be sufficient.
+ */
+ if (eax) {
+ pr_debug("Using CPUID 0xb for parallel CPU startup\n");
+ smpboot_control = STARTUP_APICID_CPUID_0B;
+ } else {
+ pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
+ do_parallel_bringup = false;
+ }
+ } else if (do_parallel_bringup) {
+ /* Without X2APIC, what's in CPUID 0x01 should suffice. */
+ pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
+ smpboot_control = STARTUP_APICID_CPUID_01;
+ }
+
snp_set_wakeup_secondary_cpu();
}

diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index af565816d2ba..788e5559549f 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -154,6 +154,9 @@ static void __init setup_real_mode(void)

trampoline_header->flags = 0;

+ trampoline_lock = &trampoline_header->lock;
+ *trampoline_lock = 0;
+
trampoline_pgd = (u64 *) __va(real_mode_header->trampoline_pgd);

/* Map the real mode stub as virtual == physical */
diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
index e38d61d6562e..2dfb1c400167 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -37,6 +37,24 @@
.text
.code16

+.macro LOAD_REALMODE_ESP
+ /*
+ * 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
+.endm
+
.balign PAGE_SIZE
SYM_CODE_START(trampoline_start)
cli # We should be safe anyway
@@ -49,8 +67,7 @@ SYM_CODE_START(trampoline_start)
mov %ax, %es
mov %ax, %ss

- # Setup stack
- movl $rm_stack_end, %esp
+ LOAD_REALMODE_ESP

call verify_cpu # Verify the cpu supports long mode
testl %eax, %eax # Check for return code
@@ -93,8 +110,7 @@ SYM_CODE_START(sev_es_trampoline_start)
mov %ax, %es
mov %ax, %ss

- # Setup stack
- movl $rm_stack_end, %esp
+ LOAD_REALMODE_ESP

jmp .Lswitch_to_protected
SYM_CODE_END(sev_es_trampoline_start)
@@ -177,7 +193,7 @@ SYM_CODE_START(pa_trampoline_compat)
* In compatibility mode. Prep ESP and DX for startup_32, then disable
* paging and complete the switch to legacy 32-bit mode.
*/
- movl $rm_stack_end, %esp
+ LOAD_REALMODE_ESP
movw $__KERNEL_DS, %dx

movl $(CR0_STATE & ~X86_CR0_PG), %eax
@@ -241,6 +257,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"
--
2.25.1


2023-03-02 11:13:52

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 07/11] x86/smpboot: Remove early_gdt_descr on 64-bit

From: Brian Gerst <[email protected]>

Build the GDT descriptor on the stack instead.

Signed-off-by: Brian Gerst <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
Tested-by: Usama Arif <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Guilherme G. Piccoli <[email protected]>
---
arch/x86/kernel/acpi/sleep.c | 2 --
arch/x86/kernel/head_64.S | 11 ++++++-----
arch/x86/kernel/smpboot.c | 2 +-
3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 1b4c43d0819a..de89bb4719d0 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -127,8 +127,6 @@ int x86_acpi_suspend_lowlevel(void)
* value is in the actual %rsp register.
*/
current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
- early_gdt_descr.address =
- (unsigned long)get_cpu_gdt_rw(smp_processor_id());
initial_gs = per_cpu_offset(smp_processor_id());
smpboot_control = smp_processor_id();
#endif
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index cc1b145055ac..a5b46c2fba05 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -265,7 +265,12 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
* addresses where we're currently running on. We have to do that here
* because in 32bit we couldn't load a 64bit linear address.
*/
- lgdt early_gdt_descr(%rip)
+ subq $16, %rsp
+ movw $(GDT_SIZE-1), (%rsp)
+ leaq gdt_page(%rdx), %rax
+ movq %rax, 2(%rsp)
+ lgdt (%rsp)
+ addq $16, %rsp

/* set up data segments */
xorl %eax,%eax
@@ -667,10 +672,6 @@ SYM_DATA_END(level1_fixmap_pgt)
.data
.align 16

-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
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 62e3bf37f0b8..a22460a07cf8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1110,10 +1110,10 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
start_ip = real_mode_header->trampoline_start64;
#endif
idle->thread.sp = (unsigned long)task_pt_regs(idle);
- early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
initial_code = (unsigned long)start_secondary;

if (IS_ENABLED(CONFIG_X86_32)) {
+ early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
initial_stack = idle->thread.sp;
} else {
smpboot_control = cpu;
--
2.25.1


2023-03-02 11:13:58

by Usama Arif

[permalink] [raw]
Subject: [PATCH v13 08/11] x86/smpboot: Remove initial_gs

From: Brian Gerst <[email protected]>

Given its CPU#, each CPU can find its own per-cpu offset, and directly set
GSBASE accordingly. The global variable can be eliminated.

Signed-off-by: Brian Gerst <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
Tested-by: Usama Arif <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Tested-by: Guilherme G. Piccoli <[email protected]>
---
arch/x86/include/asm/realmode.h | 1 -
arch/x86/kernel/acpi/sleep.c | 1 -
arch/x86/kernel/head_64.S | 22 ++++++++--------------
arch/x86/kernel/smpboot.c | 2 --
4 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index a336feef0af1..f6a1737c77be 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -59,7 +59,6 @@ extern struct real_mode_header *real_mode_header;
extern unsigned char real_mode_blob_end[];

extern unsigned long initial_code;
-extern unsigned long initial_gs;
extern unsigned long initial_stack;
#ifdef CONFIG_AMD_MEM_ENCRYPT
extern unsigned long initial_vc_handler;
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index de89bb4719d0..1328c221af30 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -127,7 +127,6 @@ int x86_acpi_suspend_lowlevel(void)
* value is in the actual %rsp register.
*/
current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
- initial_gs = per_cpu_offset(smp_processor_id());
smpboot_control = smp_processor_id();
#endif
initial_code = (unsigned long)wakeup_long64;
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index a5b46c2fba05..6a8238702eab 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -66,18 +66,10 @@ SYM_CODE_START_NOALIGN(startup_64)

leaq _text(%rip), %rdi

- /*
- * initial_gs points to initial fixed_percpu_data struct with storage for
- * the stack protector canary. Global pointer fixups are needed at this
- * stage, so apply them as is done in fixup_pointer(), and initialize %gs
- * such that the canary can be accessed at %gs:40 for subsequent C calls.
- */
+ /* Setup GSBASE to allow stack canary access for C code */
movl $MSR_GS_BASE, %ecx
- movq initial_gs(%rip), %rax
- movq $_text, %rdx
- subq %rdx, %rax
- addq %rdi, %rax
- movq %rax, %rdx
+ leaq INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
+ movl %edx, %eax
shrq $32, %rdx
wrmsr

@@ -294,8 +286,11 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
* the per cpu areas are set up.
*/
movl $MSR_GS_BASE,%ecx
- movl initial_gs(%rip),%eax
- movl initial_gs+4(%rip),%edx
+#ifndef CONFIG_SMP
+ leaq INIT_PER_CPU_VAR(fixed_percpu_data)(%rip), %rdx
+#endif
+ movl %edx, %eax
+ shrq $32, %rdx
wrmsr

/* Setup and Load IDT */
@@ -437,7 +432,6 @@ SYM_CODE_END(vc_boot_ghcb)
__REFDATA
.balign 8
SYM_DATA(initial_code, .quad x86_64_start_kernel)
-SYM_DATA(initial_gs, .quad INIT_PER_CPU_VAR(fixed_percpu_data))
#ifdef CONFIG_AMD_MEM_ENCRYPT
SYM_DATA(initial_vc_handler, .quad handle_vc_boot_ghcb)
#endif
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index a22460a07cf8..b04520085582 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1084,8 +1084,6 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle)
#ifdef CONFIG_X86_32
/* Stack for startup_32 can be just as for start_secondary onwards */
per_cpu(pcpu_hot.top_of_stack, cpu) = task_top_of_stack(idle);
-#else
- initial_gs = per_cpu_offset(cpu);
#endif
return 0;
}
--
2.25.1


2023-03-07 15:00:16

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On Thu, 2023-03-02 at 11:12 +0000, Usama Arif wrote:
> The main code change over v12 is to fix the build error when
> CONFIG_FORCE_NR_CPUS is present.
>
> The commit message for removing initial stack has also been improved, typos
> have been fixed and extra comments have been added to make code clearer.

Might something like this make it work in parallel with SEV-SNP? If so,
I can clean it up and adjust the C code to actually invoke it...

diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b8357d6ecd47..f25df4bd318e 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -70,6 +70,7 @@
/* GHCBData[63:12] */ \
(((u64)(v) & GENMASK_ULL(63, 12)) >> 12)

+#ifndef __ASSEMBLY__
/*
* SNP Page State Change Operation
*
@@ -160,6 +161,8 @@ struct snp_psc_desc {

#define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK)

+#endif /* __ASSEMBLY__ */
+
/*
* Error codes related to GHCB input that can be communicated back to the guest
* by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index defe76ee9e64..0c26f80f488c 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -204,6 +204,7 @@ extern unsigned int smpboot_control;
/* Control bits for startup_64 */
#define STARTUP_APICID_CPUID_0B 0x80000000
#define STARTUP_APICID_CPUID_01 0x40000000
+#define STARTUP_APICID_SEV_SNP 0x20000000

#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | STARTUP_APICID_CPUID_0B)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index c35f7c173832..b2571034562c 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -26,7 +26,7 @@
#include <asm/nospec-branch.h>
#include <asm/fixmap.h>
#include <asm/smp.h>
-
+#include <asm/sev-common.h>
/*
* We are not able to switch in one step to the final KERNEL ADDRESS SPACE
* because we need identity-mapped pages.
@@ -242,6 +242,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
*
* Bit 31 STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
* Bit 30 STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
+ * Bit 29 STARTUP_APICID_SEV_SNP flag (CPUID 0x0v via GHCB MSR)
* Bit 0-24 CPU# if STARTUP_APICID_CPUID_xx flags are not set
*/
movl smpboot_control(%rip), %ecx
@@ -249,6 +250,8 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
jnz .Luse_cpuid_0b
testl $STARTUP_APICID_CPUID_01, %ecx
jnz .Luse_cpuid_01
+ testl $STARTUP_APICID_SEV_SNP, %ecx
+ jnz .Luse_sev_cpuid_0b
andl $0x0FFFFFFF, %ecx
jmp .Lsetup_cpu

@@ -259,6 +262,18 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
shr $24, %edx
jmp .Lsetup_AP

+.Luse_sev_cpuid_0b:
+ movl $MSR_AMD64_SEV_ES_GHCB, %ecx
+ # GHCB_CPUID_REQ(0x0b, GHCB_CPUID_REQ_EDX)
+ movq $0xc00000040000000b, %rax
+ xorl %edx, %edx
+ vmgexit
+ rdmsr
+ andl $GHCB_MSR_INFO_MASK, %eax
+ cmpl $GHCB_MSR_CPUID_RESP, %eax
+ jne 1f
+ jmp .Lsetup_AP
+
.Luse_cpuid_0b:
mov $0x0B, %eax
xorl %ecx, %ecx



Attachments:
smime.p7s (5.83 kB)

2023-03-07 16:50:28

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On 3/7/23 08:42, David Woodhouse wrote:
> On Thu, 2023-03-02 at 11:12 +0000, Usama Arif wrote:
>> The main code change over v12 is to fix the build error when
>> CONFIG_FORCE_NR_CPUS is present.
>>
>> The commit message for removing initial stack has also been improved, typos
>> have been fixed and extra comments have been added to make code clearer.
>
> Might something like this make it work in parallel with SEV-SNP? If so,
> I can clean it up and adjust the C code to actually invoke it...

This should be ok for both SEV-ES and SEV-SNP.

>
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index b8357d6ecd47..f25df4bd318e 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -70,6 +70,7 @@
> /* GHCBData[63:12] */ \
> (((u64)(v) & GENMASK_ULL(63, 12)) >> 12)
>
> +#ifndef __ASSEMBLY__
> /*
> * SNP Page State Change Operation
> *
> @@ -160,6 +161,8 @@ struct snp_psc_desc {
>
> #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK)
>
> +#endif /* __ASSEMBLY__ */
> +
> /*
> * Error codes related to GHCB input that can be communicated back to the guest
> * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index defe76ee9e64..0c26f80f488c 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -204,6 +204,7 @@ extern unsigned int smpboot_control;
> /* Control bits for startup_64 */
> #define STARTUP_APICID_CPUID_0B 0x80000000
> #define STARTUP_APICID_CPUID_01 0x40000000
> +#define STARTUP_APICID_SEV_SNP 0x20000000
>
> #define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | STARTUP_APICID_CPUID_0B)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index c35f7c173832..b2571034562c 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -26,7 +26,7 @@
> #include <asm/nospec-branch.h>
> #include <asm/fixmap.h>
> #include <asm/smp.h>
> -
> +#include <asm/sev-common.h>
> /*
> * We are not able to switch in one step to the final KERNEL ADDRESS SPACE
> * because we need identity-mapped pages.
> @@ -242,6 +242,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> *
> * Bit 31 STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
> * Bit 30 STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
> + * Bit 29 STARTUP_APICID_SEV_SNP flag (CPUID 0x0v via GHCB MSR)
> * Bit 0-24 CPU# if STARTUP_APICID_CPUID_xx flags are not set
> */
> movl smpboot_control(%rip), %ecx
> @@ -249,6 +250,8 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> jnz .Luse_cpuid_0b
> testl $STARTUP_APICID_CPUID_01, %ecx
> jnz .Luse_cpuid_01
> + testl $STARTUP_APICID_SEV_SNP, %ecx
> + jnz .Luse_sev_cpuid_0b
> andl $0x0FFFFFFF, %ecx
> jmp .Lsetup_cpu
>
> @@ -259,6 +262,18 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> shr $24, %edx
> jmp .Lsetup_AP
>
> +.Luse_sev_cpuid_0b:
> + movl $MSR_AMD64_SEV_ES_GHCB, %ecx
> + # GHCB_CPUID_REQ(0x0b, GHCB_CPUID_REQ_EDX)
> + movq $0xc00000040000000b, %rax
> + xorl %edx, %edx
> + vmgexit

According to the GHCB spec, the GHCB MSR protocol is triggered when the
GHCB value is non-zero in bits 11:0. For the CPUID function, bits 63:32
hold the CPUID function, bits 31:30 hold the requested register and bits
11:0 == 0x4. So this should be:

/* Set the GHCB MSR to request CPUID 0xB_EDX */
movl $MSR_AMD64_SEV_ES_GHCB, %ecx
movl $0xc0000004, %eax
movl $0xb, %edx
wrmsr

/* Perform GHCB MSR protocol */
vmgexit

/*
* Get the result. After the RDMSR:
* EAX should be 0xc0000005
* EDX should have the CPUID register value and since EDX
* is the target register, no need to move the result.
*/
> + rdmsr
> + andl $GHCB_MSR_INFO_MASK, %eax
> + cmpl $GHCB_MSR_CPUID_RESP, %eax
> + jne 1f
> + jmp .Lsetup_AP
> +
> .Luse_cpuid_0b:
> mov $0x0B, %eax
> xorl %ecx, %ecx
>

Thanks,
Tom

>

2023-03-07 19:33:35

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On Tue, 2023-03-07 at 10:45 -0600, Tom Lendacky wrote:
> On 3/7/23 08:42, David Woodhouse wrote:
> > On Thu, 2023-03-02 at 11:12 +0000, Usama Arif wrote:
> > > The main code change over v12 is to fix the build error when
> > > CONFIG_FORCE_NR_CPUS is present.
> > >
> > > The commit message for removing initial stack has also been improved, typos
> > > have been fixed and extra comments have been added to make code clearer.
> >
> > Might something like this make it work in parallel with SEV-SNP? If so,
> > I can clean it up and adjust the C code to actually invoke it...
>
> This should be ok for both SEV-ES and SEV-SNP.

Thanks. So... something like this then?

Is static_branch_unlikely(&sev_es_enable_key) the right thing to use,
and does that cover SNP too?

Pushed to
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-rc8-v12bis

From d03aed91e5cfe840f4b18820fe6aed1765687321 Mon Sep 17 00:00:00 2001
From: David Woodhouse <[email protected]>
Date: Tue, 7 Mar 2023 19:06:50 +0000
Subject: [PATCH] x86/smpboot: Allow parallel bringup for SEV-ES

Enable parallel bringup for SEV-ES guests. The APs can't actually
execute the CPUID instruction directly during early startup, but they
can make the GHCB call directly instead, just as the VC trap handler
would do.

Factor out a prepare_parallel_bringup() function to help reduce the level
of complexity by allowing a simple 'return false' in the bail-out cases/

Thanks to Sabin for talking me through the way this works.

Suggested-by: Sabin Rapan <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/include/asm/sev-common.h | 3 +
arch/x86/include/asm/smp.h | 3 +-
arch/x86/kernel/head_64.S | 27 ++++++-
arch/x86/kernel/smpboot.c | 112 ++++++++++++++++++------------
4 files changed, 98 insertions(+), 47 deletions(-)

diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b8357d6ecd47..f25df4bd318e 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -70,6 +70,7 @@
/* GHCBData[63:12] */ \
(((u64)(v) & GENMASK_ULL(63, 12)) >> 12)

+#ifndef __ASSEMBLY__
/*
* SNP Page State Change Operation
*
@@ -160,6 +161,8 @@ struct snp_psc_desc {

#define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK)

+#endif /* __ASSEMBLY__ */
+
/*
* Error codes related to GHCB input that can be communicated back to the guest
* by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index defe76ee9e64..b3f67a764bfa 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -204,7 +204,8 @@ extern unsigned int smpboot_control;
/* Control bits for startup_64 */
#define STARTUP_APICID_CPUID_0B 0x80000000
#define STARTUP_APICID_CPUID_01 0x40000000
+#define STARTUP_APICID_SEV_ES 0x20000000

-#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | STARTUP_APICID_CPUID_0B)
+#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | STARTUP_APICID_CPUID_0B | STARTUP_APICID_SEV_ES)

#endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index c35f7c173832..3f5904eab678 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -26,7 +26,7 @@
#include <asm/nospec-branch.h>
#include <asm/fixmap.h>
#include <asm/smp.h>
-
+#include <asm/sev-common.h>
/*
* We are not able to switch in one step to the final KERNEL ADDRESS SPACE
* because we need identity-mapped pages.
@@ -242,6 +242,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
*
* Bit 31 STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
* Bit 30 STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
+ * Bit 29 STARTUP_APICID_SEV_ES flag (CPUID 0x0b via GHCB MSR)
* Bit 0-24 CPU# if STARTUP_APICID_CPUID_xx flags are not set
*/
movl smpboot_control(%rip), %ecx
@@ -249,6 +250,8 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
jnz .Luse_cpuid_0b
testl $STARTUP_APICID_CPUID_01, %ecx
jnz .Luse_cpuid_01
+ testl $STARTUP_APICID_SEV_ES, %ecx
+ jnz .Luse_sev_cpuid_0b
andl $0x0FFFFFFF, %ecx
jmp .Lsetup_cpu

@@ -259,6 +262,28 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
shr $24, %edx
jmp .Lsetup_AP

+.Luse_sev_cpuid_0b:
+ /* Set the GHCB MSR to request CPUID 0xB_EDX */
+ movl $MSR_AMD64_SEV_ES_GHCB, %ecx
+ movl $(GHCB_CPUID_REQ_EDX << 30) | GHCB_MSR_CPUID_REQ, %eax
+ movl $0x0B, %edx
+ wrmsr
+
+ /* Perform GHCB MSR protocol */
+ vmgexit
+
+ /*
+ * Get the result. After the RDMSR:
+ * EAX should be 0xc0000005
+ * EDX should have the CPUID register value and since EDX
+ * is the target register, no need to move the result.
+ */
+ rdmsr
+ andl $GHCB_MSR_INFO_MASK, %eax
+ cmpl $GHCB_MSR_CPUID_RESP, %eax
+ jne 1f
+ jmp .Lsetup_AP
+
.Luse_cpuid_0b:
mov $0x0B, %eax
xorl %ecx, %ecx
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 9d956571ecc1..d194c4ffeef8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1510,6 +1510,71 @@ void __init smp_prepare_cpus_common(void)
set_cpu_sibling_map(0);
}

+
+/*
+ * We can do 64-bit AP bringup in parallel if the CPU reports its APIC
+ * ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
+ * mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
+ * hard. And not for SEV-ES guests because they can't use CPUID that
+ * early.
+ */
+static bool __init prepare_parallel_bringup(void)
+{
+ if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1)
+ return false;
+
+ if (x2apic_mode) {
+ unsigned int eax, ebx, ecx, edx;
+
+ if (boot_cpu_data.cpuid_level < 0xb)
+ return false;
+
+ /*
+ * To support parallel bringup in x2apic mode, the AP will need
+ * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
+ * only 8 bits. Check that it is present and seems correct.
+ */
+ cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
+
+ /*
+ * AMD says that if executed with an umimplemented level in
+ * ECX, then it will return all zeroes in EAX. Intel says it
+ * will return zeroes in both EAX and EBX. Checking only EAX
+ * should be sufficient.
+ */
+ if (!eax) {
+ pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
+ return false;
+ }
+
+ if (IS_ENABLED(AMD_MEM_ENCRYPT) && static_branch_unlikely(&sev_es_enable_key)) {
+ pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n");
+ smpboot_control = STARTUP_APICID_SEV_ES;
+ } else if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
+ /*
+ * Other forms of memory encryption need to implement a way of
+ * finding the APs' APIC IDs that early.
+ */
+ return false;
+ } else {
+ pr_debug("Using CPUID 0xb for parallel CPU startup\n");
+ smpboot_control = STARTUP_APICID_CPUID_0B;
+ }
+ } else {
+ if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+ return false;
+
+ /* Without X2APIC, what's in CPUID 0x01 should suffice. */
+ pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
+ smpboot_control = STARTUP_APICID_CPUID_01;
+ }
+
+ cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
+ native_cpu_kick, NULL);
+
+ return true;
+}
+
/*
* Prepare for SMP bootup.
* @max_cpus: configured maximum number of CPUs, It is a legacy parameter
@@ -1550,51 +1615,8 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)

speculative_store_bypass_ht_init();

- /*
- * We can do 64-bit AP bringup in parallel if the CPU reports
- * its APIC ID in CPUID (either leaf 0x0B if we need the full
- * APIC ID in X2APIC mode, or leaf 0x01 if 8 bits are
- * sufficient). Otherwise it's too hard. And not for SEV-ES
- * guests because they can't use CPUID that early.
- */
- if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1 ||
- (x2apic_mode && boot_cpu_data.cpuid_level < 0xb) ||
- cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
- do_parallel_bringup = false;
-
- if (do_parallel_bringup && x2apic_mode) {
- unsigned int eax, ebx, ecx, edx;
-
- /*
- * To support parallel bringup in x2apic mode, the AP will need
- * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
- * only 8 bits. Check that it is present and seems correct.
- */
- cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
-
- /*
- * AMD says that if executed with an umimplemented level in
- * ECX, then it will return all zeroes in EAX. Intel says it
- * will return zeroes in both EAX and EBX. Checking only EAX
- * should be sufficient.
- */
- if (eax) {
- pr_debug("Using CPUID 0xb for parallel CPU startup\n");
- smpboot_control = STARTUP_APICID_CPUID_0B;
- } else {
- pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
- do_parallel_bringup = false;
- }
- } else if (do_parallel_bringup) {
- /* Without X2APIC, what's in CPUID 0x01 should suffice. */
- pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
- smpboot_control = STARTUP_APICID_CPUID_01;
- }
-
- if (do_parallel_bringup) {
- cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
- native_cpu_kick, NULL);
- }
+ if (do_parallel_bringup)
+ do_parallel_bringup = prepare_parallel_bringup();

snp_set_wakeup_secondary_cpu();
}
--
2.39.0



Attachments:
smime.p7s (5.83 kB)

2023-03-07 20:06:29

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On 3/7/23 13:18, David Woodhouse wrote:
> On Tue, 2023-03-07 at 10:45 -0600, Tom Lendacky wrote:
>> On 3/7/23 08:42, David Woodhouse wrote:
>>> On Thu, 2023-03-02 at 11:12 +0000, Usama Arif wrote:
>>>> The main code change over v12 is to fix the build error when
>>>> CONFIG_FORCE_NR_CPUS is present.
>>>>
>>>> The commit message for removing initial stack has also been improved, typos
>>>> have been fixed and extra comments have been added to make code clearer.
>>>
>>> Might something like this make it work in parallel with SEV-SNP? If so,
>>> I can clean it up and adjust the C code to actually invoke it...
>>
>> This should be ok for both SEV-ES and SEV-SNP.
>
> Thanks. So... something like this then?
>
> Is static_branch_unlikely(&sev_es_enable_key) the right thing to use,
> and does that cover SNP too?

Yes, that will cover SNP, too.

>
> Pushed to
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-rc8-v12bis
>
> From d03aed91e5cfe840f4b18820fe6aed1765687321 Mon Sep 17 00:00:00 2001
> From: David Woodhouse <[email protected]>
> Date: Tue, 7 Mar 2023 19:06:50 +0000
> Subject: [PATCH] x86/smpboot: Allow parallel bringup for SEV-ES
>
> Enable parallel bringup for SEV-ES guests. The APs can't actually
> execute the CPUID instruction directly during early startup, but they
> can make the GHCB call directly instead, just as the VC trap handler
> would do.
>
> Factor out a prepare_parallel_bringup() function to help reduce the level
> of complexity by allowing a simple 'return false' in the bail-out cases/
>
> Thanks to Sabin for talking me through the way this works.
>
> Suggested-by: Sabin Rapan <[email protected]>
> Signed-off-by: David Woodhouse <[email protected]>
> ---
> arch/x86/include/asm/sev-common.h | 3 +
> arch/x86/include/asm/smp.h | 3 +-
> arch/x86/kernel/head_64.S | 27 ++++++-
> arch/x86/kernel/smpboot.c | 112 ++++++++++++++++++------------
> 4 files changed, 98 insertions(+), 47 deletions(-)
>
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index b8357d6ecd47..f25df4bd318e 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -70,6 +70,7 @@
> /* GHCBData[63:12] */ \
> (((u64)(v) & GENMASK_ULL(63, 12)) >> 12)
>
> +#ifndef __ASSEMBLY__
> /*
> * SNP Page State Change Operation
> *
> @@ -160,6 +161,8 @@ struct snp_psc_desc {
>
> #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK)
>
> +#endif /* __ASSEMBLY__ */
> +
> /*
> * Error codes related to GHCB input that can be communicated back to the guest
> * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index defe76ee9e64..b3f67a764bfa 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -204,7 +204,8 @@ extern unsigned int smpboot_control;
> /* Control bits for startup_64 */
> #define STARTUP_APICID_CPUID_0B 0x80000000
> #define STARTUP_APICID_CPUID_01 0x40000000
> +#define STARTUP_APICID_SEV_ES 0x20000000
>
> -#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | STARTUP_APICID_CPUID_0B)
> +#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | STARTUP_APICID_CPUID_0B | STARTUP_APICID_SEV_ES)
>
> #endif /* _ASM_X86_SMP_H */
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index c35f7c173832..3f5904eab678 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -26,7 +26,7 @@
> #include <asm/nospec-branch.h>
> #include <asm/fixmap.h>
> #include <asm/smp.h>
> -
> +#include <asm/sev-common.h>
> /*
> * We are not able to switch in one step to the final KERNEL ADDRESS SPACE
> * because we need identity-mapped pages.
> @@ -242,6 +242,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> *
> * Bit 31 STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
> * Bit 30 STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
> + * Bit 29 STARTUP_APICID_SEV_ES flag (CPUID 0x0b via GHCB MSR)
> * Bit 0-24 CPU# if STARTUP_APICID_CPUID_xx flags are not set
> */
> movl smpboot_control(%rip), %ecx
> @@ -249,6 +250,8 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> jnz .Luse_cpuid_0b
> testl $STARTUP_APICID_CPUID_01, %ecx
> jnz .Luse_cpuid_01
> + testl $STARTUP_APICID_SEV_ES, %ecx
> + jnz .Luse_sev_cpuid_0b
> andl $0x0FFFFFFF, %ecx
> jmp .Lsetup_cpu
>
> @@ -259,6 +262,28 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
> shr $24, %edx
> jmp .Lsetup_AP
>
> +.Luse_sev_cpuid_0b:
> + /* Set the GHCB MSR to request CPUID 0xB_EDX */
> + movl $MSR_AMD64_SEV_ES_GHCB, %ecx
> + movl $(GHCB_CPUID_REQ_EDX << 30) | GHCB_MSR_CPUID_REQ, %eax
> + movl $0x0B, %edx
> + wrmsr
> +
> + /* Perform GHCB MSR protocol */
> + vmgexit
> +
> + /*
> + * Get the result. After the RDMSR:
> + * EAX should be 0xc0000005
> + * EDX should have the CPUID register value and since EDX
> + * is the target register, no need to move the result.
> + */
> + rdmsr
> + andl $GHCB_MSR_INFO_MASK, %eax
> + cmpl $GHCB_MSR_CPUID_RESP, %eax
> + jne 1f
> + jmp .Lsetup_AP
> +
> .Luse_cpuid_0b:
> mov $0x0B, %eax
> xorl %ecx, %ecx
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 9d956571ecc1..d194c4ffeef8 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1510,6 +1510,71 @@ void __init smp_prepare_cpus_common(void)
> set_cpu_sibling_map(0);
> }
>
> +
> +/*
> + * We can do 64-bit AP bringup in parallel if the CPU reports its APIC
> + * ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
> + * mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
> + * hard. And not for SEV-ES guests because they can't use CPUID that
> + * early.
> + */
> +static bool __init prepare_parallel_bringup(void)
> +{
> + if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1)
> + return false;
> +
> + if (x2apic_mode) {
> + unsigned int eax, ebx, ecx, edx;
> +
> + if (boot_cpu_data.cpuid_level < 0xb)
> + return false;
> +
> + /*
> + * To support parallel bringup in x2apic mode, the AP will need
> + * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
> + * only 8 bits. Check that it is present and seems correct.
> + */
> + cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
> +
> + /*
> + * AMD says that if executed with an umimplemented level in
> + * ECX, then it will return all zeroes in EAX. Intel says it
> + * will return zeroes in both EAX and EBX. Checking only EAX
> + * should be sufficient.
> + */
> + if (!eax) {
> + pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
> + return false;
> + }
> +
> + if (IS_ENABLED(AMD_MEM_ENCRYPT) && static_branch_unlikely(&sev_es_enable_key)) {

This should be IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)

Let me take this patch and run some tests to confirm that everything works
as expected.

Thanks!
Tom

> + pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n");
> + smpboot_control = STARTUP_APICID_SEV_ES;
> + } else if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
> + /*
> + * Other forms of memory encryption need to implement a way of
> + * finding the APs' APIC IDs that early.
> + */
> + return false;
> + } else {
> + pr_debug("Using CPUID 0xb for parallel CPU startup\n");
> + smpboot_control = STARTUP_APICID_CPUID_0B;
> + }
> + } else {
> + if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
> + return false;
> +
> + /* Without X2APIC, what's in CPUID 0x01 should suffice. */
> + pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
> + smpboot_control = STARTUP_APICID_CPUID_01;
> + }
> +
> + cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
> + native_cpu_kick, NULL);
> +
> + return true;
> +}
> +
> /*
> * Prepare for SMP bootup.
> * @max_cpus: configured maximum number of CPUs, It is a legacy parameter
> @@ -1550,51 +1615,8 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
>
> speculative_store_bypass_ht_init();
>
> - /*
> - * We can do 64-bit AP bringup in parallel if the CPU reports
> - * its APIC ID in CPUID (either leaf 0x0B if we need the full
> - * APIC ID in X2APIC mode, or leaf 0x01 if 8 bits are
> - * sufficient). Otherwise it's too hard. And not for SEV-ES
> - * guests because they can't use CPUID that early.
> - */
> - if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1 ||
> - (x2apic_mode && boot_cpu_data.cpuid_level < 0xb) ||
> - cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
> - do_parallel_bringup = false;
> -
> - if (do_parallel_bringup && x2apic_mode) {
> - unsigned int eax, ebx, ecx, edx;
> -
> - /*
> - * To support parallel bringup in x2apic mode, the AP will need
> - * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
> - * only 8 bits. Check that it is present and seems correct.
> - */
> - cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
> -
> - /*
> - * AMD says that if executed with an umimplemented level in
> - * ECX, then it will return all zeroes in EAX. Intel says it
> - * will return zeroes in both EAX and EBX. Checking only EAX
> - * should be sufficient.
> - */
> - if (eax) {
> - pr_debug("Using CPUID 0xb for parallel CPU startup\n");
> - smpboot_control = STARTUP_APICID_CPUID_0B;
> - } else {
> - pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
> - do_parallel_bringup = false;
> - }
> - } else if (do_parallel_bringup) {
> - /* Without X2APIC, what's in CPUID 0x01 should suffice. */
> - pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
> - smpboot_control = STARTUP_APICID_CPUID_01;
> - }
> -
> - if (do_parallel_bringup) {
> - cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
> - native_cpu_kick, NULL);
> - }
> + if (do_parallel_bringup)
> + do_parallel_bringup = prepare_parallel_bringup();
>
> snp_set_wakeup_secondary_cpu();
> }

2023-03-07 21:01:03

by Usama Arif

[permalink] [raw]
Subject: Re: [External] Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64




> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 9d956571ecc1..d194c4ffeef8 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1510,6 +1510,71 @@ void __init smp_prepare_cpus_common(void)
> set_cpu_sibling_map(0);
> }
>
> +
> +/*
> + * We can do 64-bit AP bringup in parallel if the CPU reports its APIC
> + * ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
> + * mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
> + * hard. And not for SEV-ES guests because they can't use CPUID that
> + * early.
> + */
> +static bool __init prepare_parallel_bringup(void)
> +{
> + if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1)
> + return false;
> +
> + if (x2apic_mode) {
> + unsigned int eax, ebx, ecx, edx;
> +
> + if (boot_cpu_data.cpuid_level < 0xb)
> + return false;
> +
> + /*
> + * To support parallel bringup in x2apic mode, the AP will need
> + * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
> + * only 8 bits. Check that it is present and seems correct.
> + */
> + cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
> +
> + /*
> + * AMD says that if executed with an umimplemented level in
> + * ECX, then it will return all zeroes in EAX. Intel says it
> + * will return zeroes in both EAX and EBX. Checking only EAX
> + * should be sufficient.
> + */
> + if (!eax) {
> + pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
> + return false;
> + }
> +
> + if (IS_ENABLED(AMD_MEM_ENCRYPT) && static_branch_unlikely(&sev_es_enable_key)) {
> + pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n");
> + smpboot_control = STARTUP_APICID_SEV_ES;
> + } else if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
> + /*
> + * Other forms of memory encryption need to implement a way of
> + * finding the APs' APIC IDs that early.
> + */
> + return false;
> + } else {
> + pr_debug("Using CPUID 0xb for parallel CPU startup\n");
> + smpboot_control = STARTUP_APICID_CPUID_0B;

I believe TDX guests with x2apic mode will end up here and enable
parallel smp if Sean was correct in this
(https://lore.kernel.org/all/[email protected]/). i.e. "TDX
guest state is also encrypted, but TDX doesn't return true
CC_ATTR_GUEST_STATE_ENCRYPT.".

So I believe the above else if
(cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) is not useful as thats
set for just SEV-ES guests? which is covered in the if part.

Thanks,
Usama


> + }
> + } else {
> + if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
> + return false;
> +
> + /* Without X2APIC, what's in CPUID 0x01 should suffice. */
> + pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
> + smpboot_control = STARTUP_APICID_CPUID_01;
> + }
> +
> + cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
> + native_cpu_kick, NULL);
> +
> + return true;
> +}
> +
> /*
> * Prepare for SMP bootup.


2023-03-07 22:23:29

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On 3/7/23 14:06, Tom Lendacky wrote:
> On 3/7/23 13:18, David Woodhouse wrote:
>> On Tue, 2023-03-07 at 10:45 -0600, Tom Lendacky wrote:
>>> On 3/7/23 08:42, David Woodhouse wrote:
>>>> On Thu, 2023-03-02 at 11:12 +0000, Usama Arif wrote:
>>>>> The main code change over v12 is to fix the build error when
>>>>> CONFIG_FORCE_NR_CPUS is present.
>>>>>
>>>>> The commit message for removing initial stack has also been improved,
>>>>> typos
>>>>> have been fixed and extra comments have been added to make code clearer.
>>>>
>>>> Might something like this make it work in parallel with SEV-SNP? If so,
>>>> I can clean it up and adjust the C code to actually invoke it...
>>>
>>> This should be ok for both SEV-ES and SEV-SNP.
>>
>> Thanks. So... something like this then?
>>
>> Is static_branch_unlikely(&sev_es_enable_key) the right thing to use,
>> and does that cover SNP too?
>
> Yes, that will cover SNP, too.
>
>>
>> Pushed to
>> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-rc8-v12bis
>>
>>  From d03aed91e5cfe840f4b18820fe6aed1765687321 Mon Sep 17 00:00:00 2001
>> From: David Woodhouse <[email protected]>
>> Date: Tue, 7 Mar 2023 19:06:50 +0000
>> Subject: [PATCH] x86/smpboot: Allow parallel bringup for SEV-ES
>>
>> Enable parallel bringup for SEV-ES guests. The APs can't actually
>> execute the CPUID instruction directly during early startup, but they
>> can make the GHCB call directly instead, just as the VC trap handler
>> would do.
>>
>> Factor out a prepare_parallel_bringup() function to help reduce the level
>> of complexity by allowing a simple 'return false' in the bail-out cases/
>>
>> Thanks to Sabin for talking me through the way this works.
>>
>> Suggested-by: Sabin Rapan <[email protected]>
>> Signed-off-by: David Woodhouse <[email protected]>

I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB
EAX will be non-zero only if SMT is enabled. So just booting some guests
without CPU topology never did parallel booting ("smpboot: Disabling
parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine
a bare-metal system that has diabled SMT will not do parallel booting, too
(but I haven't had time to test that).

I did have to change "vmgexit" to a "rep; vmmcall" based on my binutils
and the IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) change I mentioned before. But
with that, I was able to successfully boot 64 vCPU SEV-ES and SEV-SNP
guests using parallel booting.

Thanks,
Tom

>> ---
>>   arch/x86/include/asm/sev-common.h |   3 +
>>   arch/x86/include/asm/smp.h        |   3 +-
>>   arch/x86/kernel/head_64.S         |  27 ++++++-
>>   arch/x86/kernel/smpboot.c         | 112 ++++++++++++++++++------------
>>   4 files changed, 98 insertions(+), 47 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sev-common.h
>> b/arch/x86/include/asm/sev-common.h
>> index b8357d6ecd47..f25df4bd318e 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -70,6 +70,7 @@
>>       /* GHCBData[63:12] */                \
>>       (((u64)(v) & GENMASK_ULL(63, 12)) >> 12)
>> +#ifndef __ASSEMBLY__
>>   /*
>>    * SNP Page State Change Operation
>>    *
>> @@ -160,6 +161,8 @@ struct snp_psc_desc {
>>   #define GHCB_RESP_CODE(v)        ((v) & GHCB_MSR_INFO_MASK)
>> +#endif /* __ASSEMBLY__ */
>> +
>>   /*
>>    * Error codes related to GHCB input that can be communicated back to
>> the guest
>>    * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
>> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
>> index defe76ee9e64..b3f67a764bfa 100644
>> --- a/arch/x86/include/asm/smp.h
>> +++ b/arch/x86/include/asm/smp.h
>> @@ -204,7 +204,8 @@ extern unsigned int smpboot_control;
>>   /* Control bits for startup_64 */
>>   #define STARTUP_APICID_CPUID_0B    0x80000000
>>   #define STARTUP_APICID_CPUID_01    0x40000000
>> +#define STARTUP_APICID_SEV_ES    0x20000000
>> -#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 |
>> STARTUP_APICID_CPUID_0B)
>> +#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 |
>> STARTUP_APICID_CPUID_0B | STARTUP_APICID_SEV_ES)
>>   #endif /* _ASM_X86_SMP_H */
>> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>> index c35f7c173832..3f5904eab678 100644
>> --- a/arch/x86/kernel/head_64.S
>> +++ b/arch/x86/kernel/head_64.S
>> @@ -26,7 +26,7 @@
>>   #include <asm/nospec-branch.h>
>>   #include <asm/fixmap.h>
>>   #include <asm/smp.h>
>> -
>> +#include <asm/sev-common.h>
>>   /*
>>    * We are not able to switch in one step to the final KERNEL ADDRESS
>> SPACE
>>    * because we need identity-mapped pages.
>> @@ -242,6 +242,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify,
>> SYM_L_GLOBAL)
>>        *
>>        * Bit 31    STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
>>        * Bit 30    STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
>> +     * Bit 29    STARTUP_APICID_SEV_ES flag (CPUID 0x0b via GHCB MSR)
>>        * Bit 0-24    CPU# if STARTUP_APICID_CPUID_xx flags are not set
>>        */
>>       movl    smpboot_control(%rip), %ecx
>> @@ -249,6 +250,8 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify,
>> SYM_L_GLOBAL)
>>       jnz    .Luse_cpuid_0b
>>       testl    $STARTUP_APICID_CPUID_01, %ecx
>>       jnz    .Luse_cpuid_01
>> +    testl    $STARTUP_APICID_SEV_ES, %ecx
>> +    jnz    .Luse_sev_cpuid_0b
>>       andl    $0x0FFFFFFF, %ecx
>>       jmp    .Lsetup_cpu
>> @@ -259,6 +262,28 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify,
>> SYM_L_GLOBAL)
>>       shr    $24, %edx
>>       jmp    .Lsetup_AP
>> +.Luse_sev_cpuid_0b:
>> +    /* Set the GHCB MSR to request CPUID 0xB_EDX */
>> +    movl    $MSR_AMD64_SEV_ES_GHCB, %ecx
>> +    movl    $(GHCB_CPUID_REQ_EDX << 30) | GHCB_MSR_CPUID_REQ, %eax
>> +    movl    $0x0B, %edx
>> +    wrmsr
>> +
>> +    /* Perform GHCB MSR protocol */
>> +    vmgexit
>> +
>> +    /*
>> +     * Get the result. After the RDMSR:
>> +     *   EAX should be 0xc0000005
>> +     *   EDX should have the CPUID register value and since EDX
>> +     *   is the target register, no need to move the result.
>> +     */
>> +    rdmsr
>> +    andl    $GHCB_MSR_INFO_MASK, %eax
>> +    cmpl    $GHCB_MSR_CPUID_RESP, %eax
>> +    jne    1f
>> +    jmp    .Lsetup_AP
>> +
>>   .Luse_cpuid_0b:
>>       mov    $0x0B, %eax
>>       xorl    %ecx, %ecx
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 9d956571ecc1..d194c4ffeef8 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1510,6 +1510,71 @@ void __init smp_prepare_cpus_common(void)
>>       set_cpu_sibling_map(0);
>>   }
>> +
>> +/*
>> + * We can do 64-bit AP bringup in parallel if the CPU reports its APIC
>> + * ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
>> + * mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
>> + * hard. And not for SEV-ES guests because they can't use CPUID that
>> + * early.
>> + */
>> +static bool __init prepare_parallel_bringup(void)
>> +{
>> +    if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1)
>> +        return false;
>> +
>> +    if (x2apic_mode) {
>> +        unsigned int eax, ebx, ecx, edx;
>> +
>> +        if (boot_cpu_data.cpuid_level < 0xb)
>> +            return false;
>> +
>> +        /*
>> +         * To support parallel bringup in x2apic mode, the AP will need
>> +         * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
>> +         * only 8 bits. Check that it is present and seems correct.
>> +         */
>> +        cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
>> +
>> +        /*
>> +         * AMD says that if executed with an umimplemented level in
>> +         * ECX, then it will return all zeroes in EAX. Intel says it
>> +         * will return zeroes in both EAX and EBX. Checking only EAX
>> +         * should be sufficient.
>> +         */
>> +        if (!eax) {
>> +            pr_info("Disabling parallel bringup because CPUID 0xb looks
>> untrustworthy\n");
>> +            return false;
>> +        }
>> +
>> +        if (IS_ENABLED(AMD_MEM_ENCRYPT) &&
>> static_branch_unlikely(&sev_es_enable_key)) {
>
> This should be IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)
>
> Let me take this patch and run some tests to confirm that everything works
> as expected.
>
> Thanks!
> Tom
>
>> +            pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n");
>> +            smpboot_control = STARTUP_APICID_SEV_ES;
>> +        } else if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
>> +            /*
>> +             * Other forms of memory encryption need to implement a way of
>> +             * finding the APs' APIC IDs that early.
>> +             */
>> +            return false;
>> +        } else {
>> +            pr_debug("Using CPUID 0xb for parallel CPU startup\n");
>> +            smpboot_control = STARTUP_APICID_CPUID_0B;
>> +        }
>> +    } else {
>> +        if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
>> +            return false;
>> +
>> +        /* Without X2APIC, what's in CPUID 0x01 should suffice. */
>> +        pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
>> +        smpboot_control = STARTUP_APICID_CPUID_01;
>> +    }
>> +
>> +    cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
>> +                  native_cpu_kick, NULL);
>> +
>> +    return true;
>> +}
>> +
>>   /*
>>    * Prepare for SMP bootup.
>>    * @max_cpus: configured maximum number of CPUs, It is a legacy parameter
>> @@ -1550,51 +1615,8 @@ void __init native_smp_prepare_cpus(unsigned int
>> max_cpus)
>>       speculative_store_bypass_ht_init();
>> -    /*
>> -     * We can do 64-bit AP bringup in parallel if the CPU reports
>> -     * its APIC ID in CPUID (either leaf 0x0B if we need the full
>> -     * APIC ID in X2APIC mode, or leaf 0x01 if 8 bits are
>> -     * sufficient). Otherwise it's too hard. And not for SEV-ES
>> -     * guests because they can't use CPUID that early.
>> -     */
>> -    if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1 ||
>> -        (x2apic_mode && boot_cpu_data.cpuid_level < 0xb) ||
>> -        cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
>> -        do_parallel_bringup = false;
>> -
>> -    if (do_parallel_bringup && x2apic_mode) {
>> -        unsigned int eax, ebx, ecx, edx;
>> -
>> -        /*
>> -         * To support parallel bringup in x2apic mode, the AP will need
>> -         * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
>> -         * only 8 bits. Check that it is present and seems correct.
>> -         */
>> -        cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
>> -
>> -        /*
>> -         * AMD says that if executed with an umimplemented level in
>> -         * ECX, then it will return all zeroes in EAX. Intel says it
>> -         * will return zeroes in both EAX and EBX. Checking only EAX
>> -         * should be sufficient.
>> -         */
>> -        if (eax) {
>> -            pr_debug("Using CPUID 0xb for parallel CPU startup\n");
>> -            smpboot_control = STARTUP_APICID_CPUID_0B;
>> -        } else {
>> -            pr_info("Disabling parallel bringup because CPUID 0xb looks
>> untrustworthy\n");
>> -            do_parallel_bringup = false;
>> -        }
>> -    } else if (do_parallel_bringup) {
>> -        /* Without X2APIC, what's in CPUID 0x01 should suffice. */
>> -        pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
>> -        smpboot_control = STARTUP_APICID_CPUID_01;
>> -    }
>> -
>> -    if (do_parallel_bringup) {
>> -        cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
>> -                      native_cpu_kick, NULL);
>> -    }
>> +    if (do_parallel_bringup)
>> +        do_parallel_bringup = prepare_parallel_bringup();
>>       snp_set_wakeup_secondary_cpu();
>>   }

2023-03-07 22:30:12

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On Tue, 2023-03-07 at 16:22 -0600, Tom Lendacky wrote:
>
> I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB
> EAX will be non-zero only if SMT is enabled. So just booting some guests
> without CPU topology never did parallel booting ("smpboot: Disabling
> parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine
> a bare-metal system that has diabled SMT will not do parallel booting, too
> (but I haven't had time to test that).

Interesting, thanks. Should I change to checking for *both* EAX and EBX
being zero? That's what I did first, after reading only the Intel SDM.
But I changed to only EAX because the AMD doc only says that EAX will
be zero for unsupported leaves.

> I did have to change "vmgexit" to a "rep; vmmcall" based on my binutils
> and the IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) change I mentioned before. But
> with that, I was able to successfully boot 64 vCPU SEV-ES and SEV-SNP
> guests using parallel booting.

Thanks. I'll look at retconning that rework of can_parallel_bringup()
out into a separate function, into the earlier parts of the series.


Attachments:
smime.p7s (5.83 kB)

2023-03-07 22:59:22

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On 3/7/23 16:27, David Woodhouse wrote:
> On Tue, 2023-03-07 at 16:22 -0600, Tom Lendacky wrote:
>>
>> I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB
>> EAX will be non-zero only if SMT is enabled. So just booting some guests
>> without CPU topology never did parallel booting ("smpboot: Disabling
>> parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine
>> a bare-metal system that has diabled SMT will not do parallel booting, too
>> (but I haven't had time to test that).
>
> Interesting, thanks. Should I change to checking for *both* EAX and EBX
> being zero? That's what I did first, after reading only the Intel SDM.
> But I changed to only EAX because the AMD doc only says that EAX will
> be zero for unsupported leaves.

From a baremetal perspective, I think that works. Rome was the first
generation to support x2apic, and the PPR for Rome states that 0's are
returned in all 4 registers for undefined function numbers.

For virtualization, at least Qemu/KVM, that also looks to be a safe test.

Thanks,
Tom

>
>> I did have to change "vmgexit" to a "rep; vmmcall" based on my binutils
>> and the IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) change I mentioned before. But
>> with that, I was able to successfully boot 64 vCPU SEV-ES and SEV-SNP
>> guests using parallel booting.
>
> Thanks. I'll look at retconning that rework of can_parallel_bringup()
> out into a separate function, into the earlier parts of the series.

2023-03-07 23:36:49

by Sean Christopherson

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On Tue, Mar 07, 2023, David Woodhouse wrote:
> On Tue, 2023-03-07 at 16:22 -0600, Tom Lendacky wrote:
> >
> > I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB
> > EAX will be non-zero only if SMT is enabled. So just booting some guests
> > without CPU topology never did parallel booting ("smpboot: Disabling
> > parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine
> > a bare-metal system that has diabled SMT will not do parallel booting, too
> > (but I haven't had time to test that).
>
> Interesting, thanks. Should I change to checking for *both* EAX and EBX
> being zero? That's what I did first, after reading only the Intel SDM.
> But I changed to only EAX because the AMD doc only says that EAX will
> be zero for unsupported leaves.

LOL, nice. '0' is a prefectly valid output for the shift if there's exactly one
CPU at the current level, which is why Intel states that both EAX and EBX are
cleared. I assume/hope this is effectively a documentation goof, in that the APM
assumes that all "real" CPUs that support CPUID 0xB also support sub-function 0.

Nit, the AMD says EAX will be zero for unsupported _levels_. The distinction
matters because if the entire leaf is unsupported, AMD behavior is to zero out
all output registers, even if the input leaf is above the max supported leaf.

Anyways, the kernel already sanity checks the outputs on all x86 CPUs, just
piggyback that logic, e.g. expose detect_extended_topology_leaf() or so. If AMD
or a VMM is doing something crazy, the kernel is doomed anyways.

2023-03-08 07:40:22

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On Tue, 2023-03-07 at 15:35 -0800, Sean Christopherson wrote:
> On Tue, Mar 07, 2023, David Woodhouse wrote:
> > On Tue, 2023-03-07 at 16:22 -0600, Tom Lendacky wrote:
> > >
> > > I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB
> > > EAX will be non-zero only if SMT is enabled. So just booting some guests
> > > without CPU topology never did parallel booting ("smpboot: Disabling
> > > parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine
> > > a bare-metal system that has diabled SMT will not do parallel booting, too
> > > (but I haven't had time to test that).
> >
> > Interesting, thanks. Should I change to checking for *both* EAX and EBX
> > being zero? That's what I did first, after reading only the Intel SDM.
> > But I changed to only EAX because the AMD doc only says that EAX will
> > be zero for unsupported leaves.
>
> LOL, nice.  '0' is a prefectly valid output for the shift if there's exactly one
> CPU at the current level, which is why Intel states that both EAX and EBX are
> cleared.  I assume/hope this is effectively a documentation goof, in that the APM
> assumes that all "real" CPUs that support CPUID 0xB also support sub-function 0.
>
> Nit, the AMD says EAX will be zero for unsupported _levels_.  The distinction
> matters because if the entire leaf is unsupported, AMD behavior is to zero out
> all output registers, even if the input leaf is above the max supported leaf.
>
> Anyways, the kernel already sanity checks the outputs on all x86 CPUs, just
> piggyback that logic, e.g. expose detect_extended_topology_leaf() or so.  If AMD
> or a VMM is doing something crazy, the kernel is doomed anyways.

Well, we have to be somewhat careful with that assumption. Turns out
that if CPUID 0xb doesn't have valid data, the kernel *wasn't* doomed
anyway. A bunch of our early "WTF doesn't it work on AMD?" issue with
this series were due to precisely that.

But yeah, check_extended_topology_leaf() does check for EBX being non-
zero, and also for SMT_TYPE being in CH. I'll look at just calling
that; it'll make the mess of if/else in the prepare_parallel_bringup()
function a bit cleaner.


Attachments:
smime.p7s (5.83 kB)

2023-03-08 09:06:39

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On Tue, 2023-03-07 at 16:55 -0600, Tom Lendacky wrote:
> On 3/7/23 16:27, David Woodhouse wrote:
> > On Tue, 2023-03-07 at 16:22 -0600, Tom Lendacky wrote:
> > >
> > > I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB
> > > EAX will be non-zero only if SMT is enabled. So just booting some guests
> > > without CPU topology never did parallel booting ("smpboot: Disabling
> > > parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine
> > > a bare-metal system that has diabled SMT will not do parallel booting, too
> > > (but I haven't had time to test that).
> >
> > Interesting, thanks. Should I change to checking for *both* EAX and EBX
> > being zero? That's what I did first, after reading only the Intel SDM.
> > But I changed to only EAX because the AMD doc only says that EAX will
> > be zero for unsupported leaves.
>
>  From a baremetal perspective, I think that works. Rome was the first
> generation to support x2apic, and the PPR for Rome states that 0's are
> returned in all 4 registers for undefined function numbers.
>
> For virtualization, at least Qemu/KVM, that also looks to be a safe test.

At Sean's suggestion, I've switched it to use the existing
check_extended_topology_leaf() which checks for EBX being non-zero, and
CH being 1 (SMT_TYPE).

I also made it work even if the kernel isn't using x2apic mode (is that
even possible, or does SEV-ES require the MSR-based access anyway?)

It just looked odd handling SEV-ES in the CPUID 0x0B path but not the
CPUID 0x01 case, and I certainly didn't want to implement the asm side
for handling CPUID 0x01 via the GHCB protocol. And this way I can pull
the check for CC_ATTR_GUEST_STATE_ENCRYPT up above. Which I've kept for
now for the reason described in the comment, but I won't die on that
hill.

https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-v14

Looks like this:

/*
* We can do 64-bit AP bringup in parallel if the CPU reports its APIC
* ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
* mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
* hard.
*/
static bool prepare_parallel_bringup(void)
{
bool has_sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) &&
static_branch_unlikely(&sev_es_enable_key);

if (IS_ENABLED(CONFIG_X86_32))
return false;

/*
* Encrypted guests other than SEV-ES (in the future) will need to
* implement an early way of finding the APIC ID, since they will
* presumably block direct CPUID too. Be kind to our future selves
* by warning here instead of just letting them break. Parallel
* startup doesn't have to be in the first round of enabling patches
* for any such technology.
*/
if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) || !has_sev_es) {
pr_info("Disabling parallel bringup due to guest memory encryption\n");
return false;
}

if (x2apic_mode || has_sev_es) {
if (boot_cpu_data.cpuid_level < 0x0b)
return false;

if (check_extended_topology_leaf(0x0b) != 0) {
pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
return false;
}

if (has_sev_es) {
pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n");
smpboot_control = STARTUP_APICID_SEV_ES;
} else {
pr_debug("Using CPUID 0xb for parallel CPU startup\n");
smpboot_control = STARTUP_APICID_CPUID_0B;
}
} else {
/* Without X2APIC, what's in CPUID 0x01 should suffice. */
if (boot_cpu_data.cpuid_level < 0x01)
return false;

pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
smpboot_control = STARTUP_APICID_CPUID_01;
}

cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
native_cpu_kick, NULL);
return true;
}


Attachments:
smime.p7s (5.83 kB)

2023-03-08 11:27:35

by Usama Arif

[permalink] [raw]
Subject: Re: [External] Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64



On 08/03/2023 09:04, David Woodhouse wrote:
> On Tue, 2023-03-07 at 16:55 -0600, Tom Lendacky wrote:
>> On 3/7/23 16:27, David Woodhouse wrote:
>>> On Tue, 2023-03-07 at 16:22 -0600, Tom Lendacky wrote:
>>>>
>>>> I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB
>>>> EAX will be non-zero only if SMT is enabled. So just booting some guests
>>>> without CPU topology never did parallel booting ("smpboot: Disabling
>>>> parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine
>>>> a bare-metal system that has diabled SMT will not do parallel booting, too
>>>> (but I haven't had time to test that).
>>>
>>> Interesting, thanks. Should I change to checking for *both* EAX and EBX
>>> being zero? That's what I did first, after reading only the Intel SDM.
>>> But I changed to only EAX because the AMD doc only says that EAX will
>>> be zero for unsupported leaves.
>>
>>  From a baremetal perspective, I think that works. Rome was the first
>> generation to support x2apic, and the PPR for Rome states that 0's are
>> returned in all 4 registers for undefined function numbers.
>>
>> For virtualization, at least Qemu/KVM, that also looks to be a safe test.
>
> At Sean's suggestion, I've switched it to use the existing
> check_extended_topology_leaf() which checks for EBX being non-zero, and
> CH being 1 (SMT_TYPE).
>
> I also made it work even if the kernel isn't using x2apic mode (is that
> even possible, or does SEV-ES require the MSR-based access anyway?)
>
> It just looked odd handling SEV-ES in the CPUID 0x0B path but not the
> CPUID 0x01 case, and I certainly didn't want to implement the asm side
> for handling CPUID 0x01 via the GHCB protocol. And this way I can pull
> the check for CC_ATTR_GUEST_STATE_ENCRYPT up above. Which I've kept for
> now for the reason described in the comment, but I won't die on that
> hill.
>
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-v14
>
> Looks like this:
>
> /*
> * We can do 64-bit AP bringup in parallel if the CPU reports its APIC
> * ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
> * mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
> * hard.
> */
> static bool prepare_parallel_bringup(void)
> {
> bool has_sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) &&
> static_branch_unlikely(&sev_es_enable_key);
>
> if (IS_ENABLED(CONFIG_X86_32))
> return false;
>
> /*
> * Encrypted guests other than SEV-ES (in the future) will need to
> * implement an early way of finding the APIC ID, since they will
> * presumably block direct CPUID too. Be kind to our future selves
> * by warning here instead of just letting them break. Parallel
> * startup doesn't have to be in the first round of enabling patches
> * for any such technology.
> */
> if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) || !has_sev_es) {
> pr_info("Disabling parallel bringup due to guest memory encryption\n");
> return false;

I believe this is still going to enable parallel bringup for TDX?
Looking at include/linux/cc_platform.h, it looks like
CC_ATTR_GUEST_STATE_ENCRYPT is only set for SEV-ES and TDX guest with
x2apic will go on in this function and enable parallel bringup if leaf
0xB is ok. I guess if the apic ID is OK for the TDX guest, then its
fine, but just wanted to check if anyone has tested this on TDX guest?


> }
>
> if (x2apic_mode || has_sev_es) {
> if (boot_cpu_data.cpuid_level < 0x0b)
> return false;
>
> if (check_extended_topology_leaf(0x0b) != 0) {
> pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
> return false;
> }
>
> if (has_sev_es) {
> pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n");
> smpboot_control = STARTUP_APICID_SEV_ES;
> } else {
> pr_debug("Using CPUID 0xb for parallel CPU startup\n");
> smpboot_control = STARTUP_APICID_CPUID_0B;
> }
> } else {
> /* Without X2APIC, what's in CPUID 0x01 should suffice. */
> if (boot_cpu_data.cpuid_level < 0x01)
> return false;
>
> pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
> smpboot_control = STARTUP_APICID_CPUID_01;
> }
>
> cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
> native_cpu_kick, NULL);
> return true;
> }
>

2023-03-08 12:16:22

by David Laight

[permalink] [raw]
Subject: RE: [PATCH v13 00/11] Parallel CPU bringup for x86_64

From: David Woodhouse
> Sent: 08 March 2023 09:05
...
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-v14
>
> Looks like this:
>
> /*
> * We can do 64-bit AP bringup in parallel if the CPU reports its APIC
> * ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
> * mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
> * hard.
> */
> static bool prepare_parallel_bringup(void)
> {
> bool has_sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) &&
> static_branch_unlikely(&sev_es_enable_key);
>
> if (IS_ENABLED(CONFIG_X86_32))
> return false;
>
> /*
> * Encrypted guests other than SEV-ES (in the future) will need to
> * implement an early way of finding the APIC ID, since they will
> * presumably block direct CPUID too. Be kind to our future selves
> * by warning here instead of just letting them break. Parallel
> * startup doesn't have to be in the first round of enabling patches
> * for any such technology.
> */
> if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) || !has_sev_es) {
> pr_info("Disabling parallel bringup due to guest memory encryption\n");
> return false;
> }

That looks wrong, won't has_sev_es almost always be false
so it prints the message and returns?
Maybe s/||/&&/ ?

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2023-03-08 12:21:18

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On Wed, 2023-03-08 at 12:15 +0000, David Laight wrote:
>
> >         /*
> >          * Encrypted guests other than SEV-ES (in the future) will need to
> >          * implement an early way of finding the APIC ID, since they will
> >          * presumably block direct CPUID too. Be kind to our future selves
> >          * by warning here instead of just letting them break. Parallel
> >          * startup doesn't have to be in the first round of enabling patches
> >          * for any such technology.
> >          */
> >         if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) || !has_sev_es) {
> >                 pr_info("Disabling parallel bringup due to guest memory encryption\n");
> >                 return false;
> >         }
>
> That looks wrong, won't has_sev_es almost always be false
> so it prints the message and returns?
> Maybe s/||/&&/ ?

D'oh! Fixed; thanks.


Attachments:
smime.p7s (5.83 kB)

2023-03-08 14:11:59

by Usama Arif

[permalink] [raw]
Subject: Re: [External] Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64



> static bool prepare_parallel_bringup(void)
> {
> bool has_sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) &&
> static_branch_unlikely(&sev_es_enable_key);

sev_es_enable_key is only defined when CONFIG_AMD_MEM_ENCRYPT is
enabled, so it gives a build error when AMD_MEM_ENCRYPT is disabled.
maybe below is better?

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 282cca020777..e7df41436cfe 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1519,8 +1519,12 @@ void __init smp_prepare_cpus_common(void)
*/
static bool prepare_parallel_bringup(void)
{
- bool has_sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) &&
- static_branch_unlikely(&sev_es_enable_key);
+ bool has_sev_es;
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+ has_sev_es = static_branch_unlikely(&sev_es_enable_key);
+#else
+ has_sev_es = 0;
+#endif

if (IS_ENABLED(CONFIG_X86_32))
return false;

2023-03-08 14:42:37

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

On 3/8/23 03:04, David Woodhouse wrote:
> On Tue, 2023-03-07 at 16:55 -0600, Tom Lendacky wrote:
>> On 3/7/23 16:27, David Woodhouse wrote:
>>> On Tue, 2023-03-07 at 16:22 -0600, Tom Lendacky wrote:
>>>>
>>>> I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB
>>>> EAX will be non-zero only if SMT is enabled. So just booting some guests
>>>> without CPU topology never did parallel booting ("smpboot: Disabling
>>>> parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine
>>>> a bare-metal system that has diabled SMT will not do parallel booting, too
>>>> (but I haven't had time to test that).
>>>
>>> Interesting, thanks. Should I change to checking for *both* EAX and EBX
>>> being zero? That's what I did first, after reading only the Intel SDM.
>>> But I changed to only EAX because the AMD doc only says that EAX will
>>> be zero for unsupported leaves.
>>
>>  From a baremetal perspective, I think that works. Rome was the first
>> generation to support x2apic, and the PPR for Rome states that 0's are
>> returned in all 4 registers for undefined function numbers.
>>
>> For virtualization, at least Qemu/KVM, that also looks to be a safe test.
>
> At Sean's suggestion, I've switched it to use the existing
> check_extended_topology_leaf() which checks for EBX being non-zero, and
> CH being 1 (SMT_TYPE).
>
> I also made it work even if the kernel isn't using x2apic mode (is that
> even possible, or does SEV-ES require the MSR-based access anyway?)
>
> It just looked odd handling SEV-ES in the CPUID 0x0B path but not the
> CPUID 0x01 case, and I certainly didn't want to implement the asm side
> for handling CPUID 0x01 via the GHCB protocol. And this way I can pull
> the check for CC_ATTR_GUEST_STATE_ENCRYPT up above. Which I've kept for
> now for the reason described in the comment, but I won't die on that
> hill.

You can boot an SEV-ES guest in apic mode, but that would be unusual, so I
think this approach is fine.

Thanks,
Tom

>
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-v14
>
> Looks like this:
>
> /*
> * We can do 64-bit AP bringup in parallel if the CPU reports its APIC
> * ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
> * mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
> * hard.
> */
> static bool prepare_parallel_bringup(void)
> {
> bool has_sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) &&
> static_branch_unlikely(&sev_es_enable_key);
>
> if (IS_ENABLED(CONFIG_X86_32))
> return false;
>
> /*
> * Encrypted guests other than SEV-ES (in the future) will need to
> * implement an early way of finding the APIC ID, since they will
> * presumably block direct CPUID too. Be kind to our future selves
> * by warning here instead of just letting them break. Parallel
> * startup doesn't have to be in the first round of enabling patches
> * for any such technology.
> */
> if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) || !has_sev_es) {
> pr_info("Disabling parallel bringup due to guest memory encryption\n");
> return false;
> }
>
> if (x2apic_mode || has_sev_es) {
> if (boot_cpu_data.cpuid_level < 0x0b)
> return false;
>
> if (check_extended_topology_leaf(0x0b) != 0) {
> pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n");
> return false;
> }
>
> if (has_sev_es) {
> pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n");
> smpboot_control = STARTUP_APICID_SEV_ES;
> } else {
> pr_debug("Using CPUID 0xb for parallel CPU startup\n");
> smpboot_control = STARTUP_APICID_CPUID_0B;
> }
> } else {
> /* Without X2APIC, what's in CPUID 0x01 should suffice. */
> if (boot_cpu_data.cpuid_level < 0x01)
> return false;
>
> pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
> smpboot_control = STARTUP_APICID_CPUID_01;
> }
>
> cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
> native_cpu_kick, NULL);
> return true;
> }
>