Messages in this thread | | | Date | Tue, 24 Oct 2023 12:30:41 -0500 | Subject | Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously | From | Tom Lendacky <> |
| |
On 10/24/23 10:36, Mario Limonciello wrote: > +Tom > > On 10/24/2023 03:36, Ingo Molnar wrote: >> >> * Mario Limonciello <mario.limonciello@amd.com> 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@vger.kernel.org # 6.5+ >>> Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement >>> arch_cpuhp_init_parallel_bringup() and enable it") >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> --- >>> 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. > >
| |