| Message ID | 20231023160018.164054-1-mario.limonciello@amd.com |
|---|---|
| Headers | show |
| Series | Fixes for s3 with parallel bootup | expand |
* 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? 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. 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
* Mario Limonciello <mario.limonciello@amd.com> 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? > > > > 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. Yes, I suspected as much, but that's irrelevant to the arguments I outlined, that extending upon this quirk that makes SMP parallel bringup HW environment dependent, and then coupling s2ram x2apic re-enablement to that functional feature is inviting trouble in the long run. For example, what guarantees that the x2apic will be turned back on after suspend if a system is booted with maxcpus=1? Obviously something very close to your fix is needed. > I've looped Tom in to comment whether it's possible to improve AMD-SEV as > well. Thanks! Ingo
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. > >
On 10/24/2023 12:01, Ingo Molnar wrote: > > * Mario Limonciello <mario.limonciello@amd.com> 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? >>> >>> 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. > > Yes, I suspected as much, but that's irrelevant to the arguments I > outlined, that extending upon this quirk that makes SMP parallel bringup HW > environment dependent, and then coupling s2ram x2apic re-enablement to that > functional feature is inviting trouble in the long run. > I spent some more time looking at ways to decouple this, and AFAICT thaw_secondary_cpus() doesn't actually bring CPUs back after resume in parallel mode. To be symmetrical with that, another way to solve this that removes the "HW environment" aspect is to disable parallel boot for resume from sleep entirely. Like this: diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c index 6dfecb27b846..9265d97f497b 100644 --- a/arch/x86/kernel/acpi/sleep.c +++ b/arch/x86/kernel/acpi/sleep.c @@ -128,13 +128,12 @@ int x86_acpi_suspend_lowlevel(void) * value is in the actual %rsp register. */ 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. + /* + * Don't use parallel startup for resume from sleep. This avoids + * hangs that may occur if x2apic was in use but platform + * has not enabled x2apic on it's own after resume. */ - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) - smpboot_control = smp_processor_id(); + smpboot_control = smp_processor_id(); #endif initial_code = (unsigned long)wakeup_long64; saved_magic = 0x123456789abcdef0L; > For example, what guarantees that the x2apic will be turned back on after > suspend if a system is booted with maxcpus=1? lapic_resume() will do this after the boot CPU makes it up. > > Obviously something very close to your fix is needed. > Given lapic_resume() handles this, I'd think with the style fixups you suggested my patch is appropriate. >> I've looped Tom in to comment whether it's possible to improve AMD-SEV as >> well. > > Thanks! > > Ingo