LinuxLists.cc - [PATCH v16 0/8] Parallel CPU bringup for x86

[permalink] [raw]

Subject: [PATCH v16 1/8] 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-21 19:42:11

[permalink] [raw]

Subject: [PATCH v16 5/8] 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/cpu.h | 1 +
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/cpu/topology.c | 3 +-
arch/x86/kernel/head_64.S | 67 +++++++++++++++++++
arch/x86/kernel/smpboot.c | 50 +++++++++++++-
arch/x86/realmode/init.c | 3 +
arch/x86/realmode/rm/trampoline_64.S | 27 ++++++--
11 files changed, 165 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 6221a1d057dd..424151f296ff 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3822,6 +3822,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/cpu.h b/arch/x86/include/asm/cpu.h
index 78796b98a544..ef8ba318dca1 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -97,5 +97,6 @@ static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1,
extern u64 x86_read_arch_cap_msr(void);
int intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type);
+int check_extended_topology_leaf(int leaf);

#endif /* _ASM_X86_CPU_H */
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/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index 5e868b62a7c4..4373442e500a 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -9,6 +9,7 @@
#include <asm/apic.h>
#include <asm/memtype.h>
#include <asm/processor.h>
+#include <asm/cpu.h>

#include "cpu.h"

@@ -32,7 +33,7 @@ EXPORT_SYMBOL(__max_die_per_package);
/*
* Check if given CPUID extended topology "leaf" is implemented
*/
-static int check_extended_topology_leaf(int leaf)
+int check_extended_topology_leaf(int leaf)
{
unsigned int eax, ebx, ecx, edx;

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 6a8238702eab..ff3a5f008d8a 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
@@ -251,6 +305,17 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
movq pcpu_hot + X86_current_task(%rdx), %rax
movq TASK_threadsp(%rax), %rsp

+ /*
+ * Now that this CPU is running on its own stack, drop the realmode
+ * protection. For the boot CPU the pointer is NULL!
+ */
+ movq trampoline_lock(%rip), %rax
+ testq %rax, %rax
+ jz .Lsetup_gdt
+ lock
+ btrl $0, (%rax)
+
+.Lsetup_gdt:
/*
* 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
@@ -435,6 +500,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 2a8a18e23cc6..8f7f36721ea8 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;
}

@@ -1473,6 +1483,41 @@ 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 prepare_parallel_bringup(void)
+{
+ if (IS_ENABLED(CONFIG_X86_32) || cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+ return false;
+
+ if (x2apic_mode) {
+ 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;
+ }
+
+ 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;
+ }
+
+ return true;
+}
+
/*
* Prepare for SMP bootup.
* @max_cpus: configured maximum number of CPUs, It is a legacy parameter
@@ -1513,6 +1558,9 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)

speculative_store_bypass_ht_init();

+ if (do_parallel_bringup)
+ do_parallel_bringup = prepare_parallel_bringup();
+
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-21 19:42:37

[permalink] [raw]

Subject: [PATCH v16 6/8] 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, 17 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 8f7f36721ea8..bb818e6d8701 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>
@@ -992,7 +993,8 @@ static void announce_cpu(int cpu, int apicid)
node_width = num_digits(num_possible_nodes()) + 1; /* + '#' */

if (cpu == 1)
- printk(KERN_INFO "x86: Booting SMP configuration:\n");
+ printk(KERN_INFO "x86: Booting SMP configuration in %s:\n",
+ do_parallel_bringup ? "parallel" : "series");

if (system_state < SYSTEM_RUNNING) {
if (node != current_node) {
@@ -1325,9 +1327,12 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
{
int ret;

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

ret = do_wait_cpu_initialized(cpu);
if (ret)
@@ -1347,6 +1352,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
*/
@@ -1515,6 +1526,8 @@ static bool prepare_parallel_bringup(void)
smpboot_control = STARTUP_APICID_CPUID_01;
}

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

--
2.25.1

2023-03-21 19:42:37

[permalink] [raw]

Subject: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

From: David Woodhouse <[email protected]>

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.

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

Suggested-by: Sabin Rapan <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Usama Arif <[email protected]>
Reviewed-by: Tom Lendacky <[email protected]>
---
arch/x86/coco/core.c | 5 ++++
arch/x86/include/asm/coco.h | 1 +
arch/x86/include/asm/sev-common.h | 3 +++
arch/x86/include/asm/smp.h | 5 +++-
arch/x86/kernel/head_64.S | 30 ++++++++++++++++++++++++
arch/x86/kernel/smpboot.c | 39 ++++++++++++++++++++++++++-----
6 files changed, 76 insertions(+), 7 deletions(-)

diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index 49b44f881484..0bab38efb15a 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -129,6 +129,11 @@ u64 cc_mkdec(u64 val)
}
EXPORT_SYMBOL_GPL(cc_mkdec);

+enum cc_vendor cc_get_vendor(void)
+{
+ return vendor;
+}
+
__init void cc_set_vendor(enum cc_vendor v)
{
vendor = v;
diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index 3d98c3a60d34..0428d9712c96 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -12,6 +12,7 @@ enum cc_vendor {
};

void cc_set_vendor(enum cc_vendor v);
+enum cc_vendor cc_get_vendor(void);
void cc_set_mask(u64 mask);

#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b63be696b776..0abf8a39cee1 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
*
@@ -161,6 +162,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..1584f04a7007 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -204,7 +204,10 @@ 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 ff3a5f008d8a..9c38849fcac8 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -26,6 +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
@@ -242,6 +243,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 +251,10 @@ 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
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+ testl $STARTUP_APICID_SEV_ES, %ecx
+ jnz .Luse_sev_cpuid_0b
+#endif
andl $0x0FFFFFFF, %ecx
jmp .Lsetup_cpu

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

+#ifdef CONFIG_AMD_MEM_ENCRYPT
+.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 */
+ rep; vmmcall /* 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
+#endif
+
.Luse_cpuid_0b:
mov $0x0B, %eax
xorl %ecx, %ecx
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 0dba5c247be0..ef37356ab695 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -85,6 +85,7 @@
#include <asm/hw_irq.h>
#include <asm/stackprotector.h>
#include <asm/sev.h>
+#include <asm/coco.h>

/* representing HT siblings of each logical CPU */
DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_map);
@@ -1513,15 +1514,36 @@ void __init smp_prepare_cpus_common(void)
* 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.
+ * hard.
*/
static bool prepare_parallel_bringup(void)
{
- if (IS_ENABLED(CONFIG_X86_32) || cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+ bool has_sev_es = false;
+
+ if (IS_ENABLED(CONFIG_X86_32))
return false;

- if (x2apic_mode) {
+ /*
+ * 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)) {
+ switch (cc_get_vendor()) {
+ case CC_VENDOR_AMD:
+ has_sev_es = true;
+ break;
+
+ default:
+ pr_info("Disabling parallel bringup due to guest state encryption\n");
+ return false;
+ }
+ }
+
+ if (x2apic_mode || has_sev_es) {
if (boot_cpu_data.cpuid_level < 0x0b)
return false;

@@ -1530,8 +1552,13 @@ static bool prepare_parallel_bringup(void)
return false;
}

- pr_debug("Using CPUID 0xb for parallel CPU startup\n");
- smpboot_control = STARTUP_APICID_CPUID_0B;
+ 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)
--
2.25.1

2023-03-22 22:49:29

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On Tue, Mar 21, 2023 at 07:40:08PM +0000, Usama Arif wrote:
> From: David Woodhouse <[email protected]>
>
> 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.
>
> Thanks to Sabin for talking me through the way this works.
>
> Suggested-by: Sabin Rapan <[email protected]>
> Signed-off-by: David Woodhouse <[email protected]>
> Signed-off-by: Usama Arif <[email protected]>
> Reviewed-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/coco/core.c | 5 ++++
> arch/x86/include/asm/coco.h | 1 +
> arch/x86/include/asm/sev-common.h | 3 +++
> arch/x86/include/asm/smp.h | 5 +++-
> arch/x86/kernel/head_64.S | 30 ++++++++++++++++++++++++
> arch/x86/kernel/smpboot.c | 39 ++++++++++++++++++++++++++-----
> 6 files changed, 76 insertions(+), 7 deletions(-)

How's the below (totally untested yet but it builds) instead after
applying those two prerequisites:

https://lore.kernel.org/r/[email protected]

---
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b63be696b776..0abf8a39cee1 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
*
@@ -161,6 +162,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/sev.h b/arch/x86/include/asm/sev.h
index 1335781e4976..516bcb4f0719 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -200,6 +200,7 @@ void snp_set_wakeup_secondary_cpu(void);
bool snp_init(struct boot_params *bp);
void __init __noreturn snp_abort(void);
int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err);
+u32 sev_get_cpuid_0b(void);
#else
static inline void sev_es_ist_enter(struct pt_regs *regs) { }
static inline void sev_es_ist_exit(void) { }
@@ -224,6 +225,7 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
{
return -ENOTTY;
}
+static inline u32 sev_get_cpuid_0b(void) { return 0; }
#endif

#endif
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index defe76ee9e64..1584f04a7007 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -204,7 +204,10 @@ 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 ff3a5f008d8a..574bd56288bf 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -26,6 +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
@@ -242,6 +243,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 +251,10 @@ 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
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+ testl $STARTUP_APICID_SEV_ES, %ecx
+ jnz .Luse_sev_cpuid_0b
+#endif
andl $0x0FFFFFFF, %ecx
jmp .Lsetup_cpu

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

+.Luse_sev_cpuid_0b:
+ call sev_get_cpuid_0b
+ test %eax, %eax
+ jz 1f
+ movl %eax, %edx
+ jmp .Lsetup_AP
+
.Luse_cpuid_0b:
mov $0x0B, %eax
xorl %ecx, %ecx
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index ce371f62167b..96ff63cb5622 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1142,6 +1142,24 @@ void snp_set_wakeup_secondary_cpu(void)
apic->wakeup_secondary_cpu = wakeup_cpu_via_vmgexit;
}

+u32 sev_get_cpuid_0b(void)
+{
+ u32 eax, edx;
+
+ /* Request CPUID 0xB_EDX through GHCB protocol */
+ native_wrmsr(MSR_AMD64_SEV_ES_GHCB,
+ (GHCB_CPUID_REQ_EDX << 30) | GHCB_MSR_CPUID_REQ,
+ 0xb);
+ VMGEXIT();
+
+ native_rdmsr(MSR_AMD64_SEV_ES_GHCB, eax, edx);
+
+ if ((eax & GHCB_MSR_INFO_MASK) == GHCB_MSR_CPUID_RESP)
+ return edx;
+
+ return 0;
+}
+
int __init sev_es_setup_ap_jump_table(struct real_mode_header *rmh)
{
u16 startup_cs, startup_ip;
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 0dba5c247be0..6734c26ad848 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -85,6 +85,7 @@
#include <asm/hw_irq.h>
#include <asm/stackprotector.h>
#include <asm/sev.h>
+#include <asm/coco.h>

/* representing HT siblings of each logical CPU */
DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_map);
@@ -1513,15 +1514,36 @@ void __init smp_prepare_cpus_common(void)
* 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.
+ * hard.
*/
static bool prepare_parallel_bringup(void)
{
- if (IS_ENABLED(CONFIG_X86_32) || cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
+ bool has_sev_es = false;
+
+ if (IS_ENABLED(CONFIG_X86_32))
return false;

- if (x2apic_mode) {
+ /*
+ * 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)) {
+ switch (cc_vendor) {
+ case CC_VENDOR_AMD:
+ has_sev_es = true;
+ break;
+
+ default:
+ pr_info("Disabling parallel bringup due to guest state encryption\n");
+ return false;
+ }
+ }
+
+ if (x2apic_mode || has_sev_es) {
if (boot_cpu_data.cpuid_level < 0x0b)
return false;

@@ -1530,8 +1552,13 @@ static bool prepare_parallel_bringup(void)
return false;
}

- pr_debug("Using CPUID 0xb for parallel CPU startup\n");
- smpboot_control = STARTUP_APICID_CPUID_0B;
+ 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)

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2023-03-23 08:48:03

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On Wed, 2023-03-22 at 23:47 +0100, Borislav Petkov wrote:
>
> +.Luse_sev_cpuid_0b:
> +       call sev_get_cpuid_0b
> +       test %eax, %eax
> +       jz 1f
> +       movl %eax, %edx
> +       jmp .Lsetup_AP
> +

Can this code path never be taken for bringing up CPU0 even after a
hotplug? I'd rather have -1 as the error indication.

> .Luse_cpuid_0b:
>         mov     $0x0B, %eax
>         xorl    %ecx, %ecx
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index ce371f62167b..96ff63cb5622 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -1142,6 +1142,24 @@ void snp_set_wakeup_secondary_cpu(void)
>         apic->wakeup_secondary_cpu = wakeup_cpu_via_vmgexit;
> }
>
> +u32 sev_get_cpuid_0b(void)
> +{
> +       u32 eax, edx;
> +
> +       /* Request CPUID 0xB_EDX through GHCB protocol */
> +       native_wrmsr(MSR_AMD64_SEV_ES_GHCB,
> +                    (GHCB_CPUID_REQ_EDX << 30) | GHCB_MSR_CPUID_REQ,
> +                    0xb);
> +       VMGEXIT();
> +
> +       native_rdmsr(MSR_AMD64_SEV_ES_GHCB, eax, edx);
> +
> +       if ((eax & GHCB_MSR_INFO_MASK) == GHCB_MSR_CPUID_RESP)
> +               return edx;
> +
> +       return 0;
> +}
> +
> int __init sev_es_setup_ap_jump_table(struct real_mode_header *rmh)
> {

Perhaps put this in head64.c so it gets built with -pg when needed? And
then you'll spot that the other functions in there are marked __head to
put them in .head.text, and maybe find some other stuff to cargo-cult
to make it safe to run C code that early...

Attachments:

smime.p7s (5.83 kB)

2023-03-23 08:56:19

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On Thu, Mar 23, 2023 at 08:32:49AM +0000, David Woodhouse wrote:
> and maybe find some other stuff to cargo-cult to make it safe to run
> C code that early...

I'm trying to have less shit asm, not more if possible. We've been
aiming to convert stuff to C for years.

WTF are you calling cargo-cult?

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2023-03-23 09:12:47

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On Thu, 2023-03-23 at 09:51 +0100, Borislav Petkov wrote:
> On Thu, Mar 23, 2023 at 08:32:49AM +0000, David Woodhouse wrote:
> > and maybe find some other stuff to cargo-cult to make it safe to run
> > C code that early...
>
> I'm trying to have less shit asm, not more if possible. We've been
> aiming to convert stuff to C for years.

Absolutely, I understand. There are a few hoops we have to jump through
to be able to run C code this early, but it's worth it.

> WTF are you calling cargo-cult?

Off the top of my head, I don't actually *remember* all the hoops we
have to jump through to run C code this early.

But there is head64.c with examples. By 'cargo cult' I refer to the
process of copying what they do even if we don't really understand it.

I spotted that it builds with -pg, and that the functions in there are
tagged with __head to put them in the .head.text section. I didn't look
much further; there might be *other* details to copy… to 'cargo cult'.

Attachments:

smime.p7s (5.83 kB)

2023-03-23 13:33:28

by Tom Lendacky

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On 3/22/23 17:47, Borislav Petkov wrote:
> On Tue, Mar 21, 2023 at 07:40:08PM +0000, Usama Arif wrote:
>> From: David Woodhouse <[email protected]>
>>
>> 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.
>>
>> Thanks to Sabin for talking me through the way this works.
>>
>> Suggested-by: Sabin Rapan <[email protected]>
>> Signed-off-by: David Woodhouse <[email protected]>
>> Signed-off-by: Usama Arif <[email protected]>
>> Reviewed-by: Tom Lendacky <[email protected]>
>> ---
>> arch/x86/coco/core.c | 5 ++++
>> arch/x86/include/asm/coco.h | 1 +
>> arch/x86/include/asm/sev-common.h | 3 +++
>> arch/x86/include/asm/smp.h | 5 +++-
>> arch/x86/kernel/head_64.S | 30 ++++++++++++++++++++++++
>> arch/x86/kernel/smpboot.c | 39 ++++++++++++++++++++++++++-----
>> 6 files changed, 76 insertions(+), 7 deletions(-)
>
> How's the below (totally untested yet but it builds) instead after
> applying those two prerequisites:
>
> https://lore.kernel.org/r/[email protected]
>
> ---

...

> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index ce371f62167b..96ff63cb5622 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -1142,6 +1142,24 @@ void snp_set_wakeup_secondary_cpu(void)
> apic->wakeup_secondary_cpu = wakeup_cpu_via_vmgexit;
> }
>
> +u32 sev_get_cpuid_0b(void)

Maybe call this sev_early_get_cpuid_0b() or such to imply that the MSR
protocol is being used and only retrieving / returning edx.

Also, since it is a function now and can be used at any point, the current
GHCB MSR should be saved on entry and restored on exit.

Thanks,
Tom

> +{
> + u32 eax, edx;
> +
> + /* Request CPUID 0xB_EDX through GHCB protocol */
> + native_wrmsr(MSR_AMD64_SEV_ES_GHCB,
> + (GHCB_CPUID_REQ_EDX << 30) | GHCB_MSR_CPUID_REQ,
> + 0xb);
> + VMGEXIT();
> +
> + native_rdmsr(MSR_AMD64_SEV_ES_GHCB, eax, edx);
> +
> + if ((eax & GHCB_MSR_INFO_MASK) == GHCB_MSR_CPUID_RESP)
> + return edx;
> +
> + return 0;
> +}
> +
> int __init sev_es_setup_ap_jump_table(struct real_mode_header *rmh)
> {
> u16 startup_cs, startup_ip;

2023-03-23 14:24:19

by Brian Gerst

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On Thu, Mar 23, 2023 at 5:04 AM David Woodhouse <[email protected]> wrote:
>
> On Thu, 2023-03-23 at 09:51 +0100, Borislav Petkov wrote:
> > On Thu, Mar 23, 2023 at 08:32:49AM +0000, David Woodhouse wrote:
> > > and maybe find some other stuff to cargo-cult to make it safe to run
> > > C code that early...
> >
> > I'm trying to have less shit asm, not more if possible. We've been
> > aiming to convert stuff to C for years.
>
> Absolutely, I understand. There are a few hoops we have to jump through
> to be able to run C code this early, but it's worth it.
>
> > WTF are you calling cargo-cult?
>
> Off the top of my head, I don't actually *remember* all the hoops we
> have to jump through to run C code this early.

Making sure that the stack protector is either disabled or properly
set up, and disabling any instrumentation/profiling/debug crap that
isn't initialized yet.

--
Brian Gerst

2023-03-23 18:34:22

by Tom Lendacky

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On 3/23/23 10:34, Woodhouse, David wrote:
> On Thu, 2023-03-23 at 08:16 -0500, Tom Lendacky wrote:
>>
>> Maybe call this sev_early_get_cpuid_0b() or such to imply that the MSR
>> protocol is being used and only retrieving / returning edx.
>
> Or indeed sev_early_get_apicid() since that's what it's actually doing,
> and the caller doesn't care *how*.

Sounds good.

>
>> Also, since it is a function now and can be used at any point, the current
>> GHCB MSR should be saved on entry and restored on exit.
>
> Well, I don't know that it should be callable at any point. This is
> only supposed to be called from head_64.S.

I agree. But once it's there, someone somewhere in the future may look and
go, oh, I can call this. So I think it either needs a nice comment above
it about how it is currently used/called and what to do if it needs to be
called from someplace other than head_64.S or the MSR needs to be
saved/restored.

Thanks,
Tom

>
>
>
>
>
> Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
>
>
>

2023-03-23 22:15:01

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On March 23, 2023 7:28:38 PM GMT+01:00, Tom Lendacky <[email protected]> wrote:
>
>I agree. But once it's there, someone somewhere in the future may look and go, oh, I can call this. So I think it either needs a nice comment above it about how it is currently used/called and what to do if it needs to be called from someplace other than head_64.S or the MSR needs to be saved/restored.

Or it could be __init and get discarded when the system is up.

--
Sent from a small device: formatting sucks and brevity is inevitable.

2023-03-23 22:31:54

[permalink] [raw]

Subject: Re: [EXTERNAL][PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On Thu, 2023-03-23 at 23:13 +0100, Borislav Petkov wrote:
> On March 23, 2023 7:28:38 PM GMT+01:00, Tom Lendacky
> <[email protected]> wrote:
> >
> > I agree. But once it's there, someone somewhere in the future may
> > look and go, oh, I can call this. So I think it either needs a nice
> > comment above it about how it is currently used/called and what to
> > do if it needs to be called from someplace other than head_64.S or
> > the MSR needs to be saved/restored.
>
> Or it could be __init and get discarded when the system is up.

Not if we have CPU hotplug. Or system suspend.

Attachments:

smime.p7s (5.83 kB)

2023-03-27 17:50:11

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On Thu, Mar 23, 2023 at 10:23:02AM -0400, Brian Gerst wrote:
> Making sure that the stack protector is either disabled or properly
> set up, and disabling any instrumentation/profiling/debug crap that
> isn't initialized yet.

Lemme dump brain of what Tom and I were talking about today so that it
is documented somewhere.

* re: stack protector: I was thinking to mark this function

__attribute__((no_stack_protector))

but gcc added the function attribute way later:

~/src/gcc/gcc.git> git tag --contains 346b302d09c1e6db56d9fe69048acb32fbb97845
basepoints/gcc-12
basepoints/gcc-13
releases/gcc-11.1.0
releases/gcc-11.2.0
releases/gcc-11.3.0
releases/gcc-12.1.0
releases/gcc-12.2.0

which means, that function would have to live somewhere in a file which
has stack protector disabled. One possible place would be
arch/x86/mm/mem_encrypt_identity.c which is kinda related.

* re: stack: in order to be able to call a C function that early, we'd
have to put the VA of the initial stack back into %rsp as we switch
pagetables a bit earlier in there (thx Tom).

So by then, doing all that cargo-cult just in order to not have a bunch
of lines in asm doesn't sound all that great anymore.

* The __head per-function attribute is easily solved by lifting the
__head define into a common header.

So meh, dunno. I guess we can do the asm thing for now, until a cleaner
solution without too many warts presents itself.

As to exporting cc_vendor:

https://lore.kernel.org/r/[email protected]

I'll redo those and the SEV-ES patch won't have to add cc_get_vendor().

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2023-03-27 18:15:47

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On Mon, 2023-03-27 at 19:47 +0200, Borislav Petkov wrote:
> > Making sure that the stack protector is either disabled or properly
> > set up, and disabling any instrumentation/profiling/debug crap that
> > isn't initialized yet.
>
> Lemme dump brain of what Tom and I were talking about today so that it
> is documented somewhere.
>
> * re: stack protector: I was thinking to mark this function
>
> __attribute__((no_stack_protector))
>
> but gcc added the function attribute way later:
>
> ~/src/gcc/gcc.git> git tag --contains 346b302d09c1e6db56d9fe69048acb32fbb97845
> basepoints/gcc-12
> basepoints/gcc-13
> releases/gcc-11.1.0
> releases/gcc-11.2.0
> releases/gcc-11.3.0
> releases/gcc-12.1.0
> releases/gcc-12.2.0
>
> which means, that function would have to live somewhere in a file which
> has stack protector disabled. One possible place would be
> arch/x86/mm/mem_encrypt_identity.c which is kinda related.

Shouldn't the rest of head64.c have the stack protector disabled, for
similar reasons?

> * re: stack: in order to be able to call a C function that early, we'd
> have to put the VA of the initial stack back into %rsp as we switch
> pagetables a bit earlier in there (thx Tom).

Hm, don't you have a stack at the point you added that call? I thought
you did? It doesn't have to be *the* stack for the AP in question.
Just "a" stack. And you have the lock on the real-mode one that you're
using.

> So by then, doing all that cargo-cult just in order to not have a bunch
> of lines in asm doesn't sound all that great anymore.
>
> * The __head per-function attribute is easily solved by lifting the
> __head define into a common header.
>
> So meh, dunno. I guess we can do the asm thing for now, until a cleaner
> solution without too many warts presents itself.

Hm, doesn't most of that just go away (or at least become "Already
Broken; Someone Else's Problem™") if you just concede to put your new C
function into head64.c along with a whole bunch of other existing
CONFIG_AMD_MEM_ENCRYPT support?

(We still have to fix it if it's Someone Else's Problem, of course.
It's just that you don't have to count that complexity towards your own
part.)

Attachments:

smime.p7s (5.83 kB)

2023-03-27 19:16:19

by Tom Lendacky

[permalink] [raw]

Subject: Re: [PATCH v16 8/8] x86/smpboot: Allow parallel bringup for SEV-ES

On 3/27/23 13:14, David Woodhouse wrote:
> On Mon, 2023-03-27 at 19:47 +0200, Borislav Petkov wrote:
>>> Making sure that the stack protector is either disabled or properly
>>> set up, and disabling any instrumentation/profiling/debug crap that
>>> isn't initialized yet.
>>
>> Lemme dump brain of what Tom and I were talking about today so that it
>> is documented somewhere.
>>
>> * re: stack protector: I was thinking to mark this function
>>
>> __attribute__((no_stack_protector))
>>
>> but gcc added the function attribute way later:
>>
>> ~/src/gcc/gcc.git> git tag --contains 346b302d09c1e6db56d9fe69048acb32fbb97845
>> basepoints/gcc-12
>> basepoints/gcc-13
>> releases/gcc-11.1.0
>> releases/gcc-11.2.0
>> releases/gcc-11.3.0
>> releases/gcc-12.1.0
>> releases/gcc-12.2.0
>>
>> which means, that function would have to live somewhere in a file which
>> has stack protector disabled. One possible place would be
>> arch/x86/mm/mem_encrypt_identity.c which is kinda related.
>
> Shouldn't the rest of head64.c have the stack protector disabled, for
> similar reasons?
>
>> * re: stack: in order to be able to call a C function that early, we'd
>> have to put the VA of the initial stack back into %rsp as we switch
>> pagetables a bit earlier in there (thx Tom).
>
> Hm, don't you have a stack at the point you added that call? I thought
> you did? It doesn't have to be *the* stack for the AP in question.
> Just "a" stack. And you have the lock on the real-mode one that you're
> using.

Unfortunately RSP has the identity mapped stack value and when the
pagetable switch is performed the mapping to that stack is lost. It would
need to be updated to the equivalent of __va(RSP) to get a stack that can
be used without page faulting.

Thanks,
Tom

>
>> So by then, doing all that cargo-cult just in order to not have a bunch
>> of lines in asm doesn't sound all that great anymore.
>>
>> * The __head per-function attribute is easily solved by lifting the
>> __head define into a common header.
>>
>> So meh, dunno. I guess we can do the asm thing for now, until a cleaner
>> solution without too many warts presents itself.
>
> Hm, doesn't most of that just go away (or at least become "Already
> Broken; Someone Else's Problem™") if you just concede to put your new C
> function into head64.c along with a whole bunch of other existing
> CONFIG_AMD_MEM_ENCRYPT support?
>
> (We still have to fix it if it's Someone Else's Problem, of course.
> It's just that you don't have to count that complexity towards your own
> part.)

2023-03-27 19:35:35