Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously
From: Tom Lendacky
Date: Tue Oct 24 2023 - 13:31:01 EST
On 10/24/23 10:36, Mario Limonciello wrote:
+Tom
On 10/24/2023 03:36, Ingo Molnar wrote:
* Mario Limonciello <mario.limonciello@xxxxxxx> wrote:
If x2apic was enabled during boot with parallel startup
it will be needed during resume from suspend to ram as well.
Store whether to enable into the smpboot_control global variable
and during startup re-enable it if necessary.
Cc: stable@xxxxxxxxxxxxxxx # 6.5+
Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement
arch_cpuhp_init_parallel_bringup() and enable it")
Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
---
arch/x86/include/asm/smp.h | 1 +
arch/x86/kernel/acpi/sleep.c | 12 ++++++++----
arch/x86/kernel/head_64.S | 15 +++++++++++++++
3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index c31c633419fe..86584ffaebc3 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -190,6 +190,7 @@ extern unsigned long apic_mmio_base;
#endif /* !__ASSEMBLY__ */
/* Control bits for startup_64 */
+#define STARTUP_ENABLE_X2APIC 0x40000000
#define STARTUP_READ_APICID 0x80000000
/* Top 8 bits are reserved for control */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 6dfecb27b846..29734a1299f6 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -11,6 +11,7 @@
#include <linux/dmi.h>
#include <linux/cpumask.h>
#include <linux/pgtable.h>
+#include <asm/apic.h>
#include <asm/segment.h>
#include <asm/desc.h>
#include <asm/cacheflush.h>
@@ -129,11 +130,14 @@ int x86_acpi_suspend_lowlevel(void)
*/
current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack);
/*
- * 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.
+ * Ensure x2apic is re-enabled if necessary and 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))
+ if (smpboot_control & STARTUP_PARALLEL_MASK) {
+ if (x2apic_enabled())
+ smpboot_control |= STARTUP_ENABLE_X2APIC;
+ } else
smpboot_control = smp_processor_id();
Yeah, so instead of adding further kludges to the 'parallel bringup is
possible' code path, which is arguably a functional feature that shouldn't
have hardware-management coupled to it, would it be possible to fix
parallel bringup to AMD-SEV systems, so that this code path isn't a
quirk-dependent "parallel boot" codepath, but simply the "x86 SMP boot
codepath", where all SMP x86 systems do a parallel bootup?
The original commit by Thomas says:
0c7ffa32dbd6 ("x86/smpboot/64: Implement
arch_cpuhp_init_parallel_bringup() and enable it")
| Unfortunately there is no RDMSR GHCB protocol at the moment, so
enabling
| AMD-SEV guests for parallel startup needs some more thought.
But that was half a year ago, isn't there RDMSR GHCB access code
available now?
That support requires an update to the GHCB specification to add
RDMSR/WRMSR access to the GHCB MSR protocol, which hasn't been written,
yet. The support would have to be present in both the hypervisor and the
guest.
Thanks,
Tom
This code would all read a lot more natural if it was the regular x86 SMP
bootup path - which it is 'almost' today already, modulo quirk.
Obviously coupling functional features with hardware quirks is fragile, for
example your patch extending x86 SMP parallel bringup doesn't extend the
AMD-SEV case, which may or may not matter in practice.
So, if it's possible, it would be nice to fix AMD-SEV systems as well and
remove this artificial coupling.
It probably isn't clear since I didn't mention it in the commit message,
but this is not a system that supports AMD-SEV. This is a workstation
that supports x2apic. I'll clarify that for V2.
I've looped Tom in to comment whether it's possible to improve AMD-SEV as
well.
Also, side note #1: curly braces should be balanced.
#endif
initial_code = (unsigned long)wakeup_long64;
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index ea6995920b7a..fcfa79105928 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -236,10 +236,15 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify,
SYM_L_GLOBAL)
* used to look up the CPU number. For booting a single CPU, the
* CPU number is encoded in smpboot_control.
*
+ * Bit 30 STARTUP_ENABLE_X2APIC (Enable X2APIC mode)
* Bit 31 STARTUP_READ_APICID (Read APICID from APIC)
* Bit 0-23 CPU# if STARTUP_xx flags are not set
Side note #2: you mixed up the comment ordering here.
Thanks,
Ingo
Sure, thanks for the feedback. I'll adjust the style for v2.