| Message ID | 20231026170330.4657-2-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for s3 with parallel bootup | expand |
Mario! On Thu, Oct 26 2023 at 12:03, Mario Limonciello wrote: > If x2apic was enabled during boot with parallel startup > it will be needed during resume from suspend to ram as well. Lacks an explanation why it is needed. > Store whether to enable into the smpboot_control global variable > and during startup re-enable it if necessary. > > This fixes resume from suspend on workstation CPUs with x2apic > enabled. You completely fail to describe the failure mode. > It will also work on systems with one maxcpus=1 but still using > x2apic since x2apic is also re-enabled in lapic_resume(). This sentence makes no sense. What's so special about maxcpus=1? Absolutely nothing. lapic_resume() is a syscore op and is always invoked on the CPU which handles suspend/resume. The point is that __x2apic_enable() which is invoked from there becomes a NOOP because X2APIC is already enabled. So what are you trying to tell me? I really appreciate your enthusiasm of chasing and fixing bugs, but your change logs and explanations are really making it hard to keep that appreciation up. > /* > - * 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. The x2apic comment is misplaced. It should be above the x2apic conditional as it has nothing to do with the initial condition because even if X2APIC is enabled then the parallel startup might be disabled. Go and read this comment 3 month from now and try to make sense of it. > */ > - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) > + if (smpboot_control & STARTUP_PARALLEL_MASK) { > + if (x2apic_enabled()) > + smpboot_control |= STARTUP_ENABLE_X2APIC; This bit is sticky after resume, so any subsequent CPU hotplug operation will see it as well. This lacks an explanation why this is correct and why this flag is not set early during boot before the APs are brought up. > + } else { > smpboot_control = smp_processor_id(); > + } > #endif > initial_code = (unsigned long)wakeup_long64; > saved_magic = 0x123456789abcdef0L; > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S > index ea6995920b7a..300901af9fa3 100644 > --- a/arch/x86/kernel/head_64.S > +++ b/arch/x86/kernel/head_64.S > @@ -237,9 +237,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) > * CPU number is encoded in smpboot_control. > * > * Bit 31 STARTUP_READ_APICID (Read APICID from APIC) > + * Bit 30 STARTUP_ENABLE_X2APIC (Enable X2APIC mode) > * Bit 0-23 CPU# if STARTUP_xx flags are not set > */ > movl smpboot_control(%rip), %ecx > + > + testl $STARTUP_ENABLE_X2APIC, %ecx > + jnz .Lenable_x2apic Why is this tested here? The test clearly belongs into .Lread_apicid > + > testl $STARTUP_READ_APICID, %ecx > jnz .Lread_apicid > /* > @@ -249,6 +254,16 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) > andl $(~STARTUP_PARALLEL_MASK), %ecx > jmp .Lsetup_cpu > > +.Lenable_x2apic: > + /* Enable X2APIC if disabled */ > + mov $MSR_IA32_APICBASE, %ecx > + rdmsr > + testl $X2APIC_ENABLE, %eax > + jnz .Lread_apicid_msr > + orl $X2APIC_ENABLE, %eax > + wrmsr > + jmp .Lread_apicid_msr And this part just moves in front of .Lread_apicid_msr and spares the jump at the end. > .Lread_apicid: > /* Check whether X2APIC mode is already enabled */ > mov $MSR_IA32_APICBASE, %ecx That aside, I'm still failing to see the actual failure scenario due to the utter void in the change log. The kernel has two mechanisms which end up with X2APIC enabled: 1) BIOS has it enabled already, which is required for any machine which has more than 255 CPUs, but BIOS can decide to enable X2APIC even with less than 256 CPUs. 2) BIOS has not enabled it, but the kernel enables it because the CPU supports it. The major difference is: #1 prevents the MMIO fixmap for the APIC to be installed #2 installs the fixmap but does not use it. It's never torn down. Let's look at these two cases in the light of resume: #1 If the BIOS enabled X2APIC mode then the only way how this can fail on resume is that the BIOS did not enable X2APIC mode in the resume path before going back into the kernel and due to the non-existent MMIO mapping there is no way to read the APIC ID. #2 It does not matter whether the BIOS enabled X2APIC mode during resume because the MMIO mapping exists and the APIC ID read via MMIO should be identical to the APIC ID read via the X2APIC MSR. If not, then there is something fundamentally wrong. How is this working during the initial bringup of the APs? #1 If the APs do not have X2APIC enabled by the BIOS then they will crash and burn during bringup due to non-existent MMIO mapping. #2 The APs can read their APIC ID just fine via MMIO and it obviously is the same as the APIC ID after the bringup enabled X2APIC mode. Otherwise the kernel would not work at all. So the only reasonable explanation for the failure is that the BIOS fails to reenable X2APIC mode on resume either on the CPU which handles suspend/resume or on the subsequent AP bringups, which is not clear at all due to the bit being sticky and the changelog being full of void in that aspect. That said the sticky bit is wrong for the following case with older CPUs where X2APIC requires up to date microcode loaded to work correctly: boot maxcpus=4 load microcode // Same sequence applies for AP (CPU1-3) enable x2apic suspend set X2APIC enable bit in smpboot_control resume bringup CPU4 enable X2APIC early --> fail due to lack of microcode fix Whether this affects the APIC ID readout or not, which we don't know and even if we consider this case to be academic, it's still fundamentally wrong from a correctness point of view. So without proper information about the failure scenario this "fix" is simply going nowhere. Please provide the following information: - dmesg of the initial boot up to 'smp: Bringing up secondary CPUs ...' - A proper description of the failure case: - Is the CPU which handles suspend/resume failing? - Is a subsequent AP bringup failing? - Is the failure due to the lack of MMIO mapping ? - Is the failure due to a bogus APIC ID retrieved via MMIO ? Thanks for making me do your homework (again), tglx
On 10/27/2023 16:31, Thomas Gleixner wrote: > Mario! > > On Thu, Oct 26 2023 at 12:03, Mario Limonciello wrote: > >> If x2apic was enabled during boot with parallel startup >> it will be needed during resume from suspend to ram as well. > > Lacks an explanation why it is needed. > >> Store whether to enable into the smpboot_control global variable >> and during startup re-enable it if necessary. >> >> This fixes resume from suspend on workstation CPUs with x2apic >> enabled. > > You completely fail to describe the failure mode. > >> It will also work on systems with one maxcpus=1 but still using >> x2apic since x2apic is also re-enabled in lapic_resume(). > > This sentence makes no sense. What's so special about maxcpus=1? > > Absolutely nothing. > > lapic_resume() is a syscore op and is always invoked on the CPU which > handles suspend/resume. The point is that __x2apic_enable() which is > invoked from there becomes a NOOP because X2APIC is already enabled. > > So what are you trying to tell me? > > I really appreciate your enthusiasm of chasing and fixing bugs, but your > change logs and explanations are really making it hard to keep that > appreciation up. > >> /* >> - * 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. > > The x2apic comment is misplaced. It should be above the x2apic > conditional as it has nothing to do with the initial condition because > even if X2APIC is enabled then the parallel startup might be disabled. > > Go and read this comment 3 month from now and try to make sense of it. > >> */ >> - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) >> + if (smpboot_control & STARTUP_PARALLEL_MASK) { >> + if (x2apic_enabled()) >> + smpboot_control |= STARTUP_ENABLE_X2APIC; > > This bit is sticky after resume, so any subsequent CPU hotplug operation > will see it as well. > > This lacks an explanation why this is correct and why this flag is not > set early during boot before the APs are brought up. > >> + } else { >> smpboot_control = smp_processor_id(); >> + } >> #endif >> initial_code = (unsigned long)wakeup_long64; >> saved_magic = 0x123456789abcdef0L; >> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S >> index ea6995920b7a..300901af9fa3 100644 >> --- a/arch/x86/kernel/head_64.S >> +++ b/arch/x86/kernel/head_64.S >> @@ -237,9 +237,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) >> * CPU number is encoded in smpboot_control. >> * >> * Bit 31 STARTUP_READ_APICID (Read APICID from APIC) >> + * Bit 30 STARTUP_ENABLE_X2APIC (Enable X2APIC mode) >> * Bit 0-23 CPU# if STARTUP_xx flags are not set >> */ >> movl smpboot_control(%rip), %ecx >> + >> + testl $STARTUP_ENABLE_X2APIC, %ecx >> + jnz .Lenable_x2apic > > Why is this tested here? The test clearly belongs into .Lread_apicid > >> + >> testl $STARTUP_READ_APICID, %ecx >> jnz .Lread_apicid >> /* >> @@ -249,6 +254,16 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) >> andl $(~STARTUP_PARALLEL_MASK), %ecx >> jmp .Lsetup_cpu >> >> +.Lenable_x2apic: >> + /* Enable X2APIC if disabled */ >> + mov $MSR_IA32_APICBASE, %ecx >> + rdmsr >> + testl $X2APIC_ENABLE, %eax >> + jnz .Lread_apicid_msr >> + orl $X2APIC_ENABLE, %eax >> + wrmsr >> + jmp .Lread_apicid_msr > > And this part just moves in front of .Lread_apicid_msr and spares > the jump at the end. > >> .Lread_apicid: >> /* Check whether X2APIC mode is already enabled */ >> mov $MSR_IA32_APICBASE, %ecx > > That aside, I'm still failing to see the actual failure scenario due to > the utter void in the change log. > > The kernel has two mechanisms which end up with X2APIC enabled: > > 1) BIOS has it enabled already, which is required for any machine > which has more than 255 CPUs, but BIOS can decide to enable > X2APIC even with less than 256 CPUs. > > 2) BIOS has not enabled it, but the kernel enables it because the > CPU supports it. > > The major difference is: > > #1 prevents the MMIO fixmap for the APIC to be installed > > #2 installs the fixmap but does not use it. It's never torn down. > > Let's look at these two cases in the light of resume: > > #1 If the BIOS enabled X2APIC mode then the only way how this can > fail on resume is that the BIOS did not enable X2APIC mode in the > resume path before going back into the kernel and due to the > non-existent MMIO mapping there is no way to read the APIC ID. > > #2 It does not matter whether the BIOS enabled X2APIC mode during > resume because the MMIO mapping exists and the APIC ID read via > MMIO should be identical to the APIC ID read via the X2APIC MSR. > > If not, then there is something fundamentally wrong. > > How is this working during the initial bringup of the APs? > > #1 If the APs do not have X2APIC enabled by the BIOS then they will > crash and burn during bringup due to non-existent MMIO mapping. > > #2 The APs can read their APIC ID just fine via MMIO and it > obviously is the same as the APIC ID after the bringup enabled > X2APIC mode. Otherwise the kernel would not work at all. > > So the only reasonable explanation for the failure is that the BIOS > fails to reenable X2APIC mode on resume either on the CPU which handles > suspend/resume or on the subsequent AP bringups, which is not clear at > all due to the bit being sticky and the changelog being full of void in > that aspect. > > That said the sticky bit is wrong for the following case with older CPUs > where X2APIC requires up to date microcode loaded to work correctly: > > boot maxcpus=4 > load microcode // Same sequence applies for AP (CPU1-3) > enable x2apic > suspend > set X2APIC enable bit in smpboot_control > resume > bringup CPU4 > enable X2APIC early --> fail due to lack of microcode fix > > Whether this affects the APIC ID readout or not, which we don't know and > even if we consider this case to be academic, it's still fundamentally > wrong from a correctness point of view. > > So without proper information about the failure scenario this "fix" is > simply going nowhere. > > Please provide the following information: > > - dmesg of the initial boot up to 'smp: Bringing up secondary CPUs ...' > > - A proper description of the failure case: > > - Is the CPU which handles suspend/resume failing? > > - Is a subsequent AP bringup failing? > > - Is the failure due to the lack of MMIO mapping ? > > - Is the failure due to a bogus APIC ID retrieved via MMIO ? > > Thanks for making me do your homework (again), > > tglx Hi Thomas, Thank you for looking this over. I've reviewed it with the internal team and confirmed there was a BIOS bug where the MSR wasn't restored after the S3 cycle completed. The BIOS team has fixed it. Thanks, #regzbot invalid: BIOS bug
On Tue, Oct 31 2023 at 13:53, Mario Limonciello wrote: > Thank you for looking this over. I've reviewed it with the internal > team and confirmed there was a BIOS bug where the MSR wasn't restored > after the S3 cycle completed. The BIOS team has fixed it. Why am I not surprised? Thanks for the heads up! tglx
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..10d8921b4bb8 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,12 +130,16 @@ 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(); + } #endif initial_code = (unsigned long)wakeup_long64; saved_magic = 0x123456789abcdef0L; diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S index ea6995920b7a..300901af9fa3 100644 --- a/arch/x86/kernel/head_64.S +++ b/arch/x86/kernel/head_64.S @@ -237,9 +237,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) * CPU number is encoded in smpboot_control. * * Bit 31 STARTUP_READ_APICID (Read APICID from APIC) + * Bit 30 STARTUP_ENABLE_X2APIC (Enable X2APIC mode) * Bit 0-23 CPU# if STARTUP_xx flags are not set */ movl smpboot_control(%rip), %ecx + + testl $STARTUP_ENABLE_X2APIC, %ecx + jnz .Lenable_x2apic + testl $STARTUP_READ_APICID, %ecx jnz .Lread_apicid /* @@ -249,6 +254,16 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) andl $(~STARTUP_PARALLEL_MASK), %ecx jmp .Lsetup_cpu +.Lenable_x2apic: + /* Enable X2APIC if disabled */ + mov $MSR_IA32_APICBASE, %ecx + rdmsr + testl $X2APIC_ENABLE, %eax + jnz .Lread_apicid_msr + orl $X2APIC_ENABLE, %eax + wrmsr + jmp .Lread_apicid_msr + .Lread_apicid: /* Check whether X2APIC mode is already enabled */ mov $MSR_IA32_APICBASE, %ecx
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. This fixes resume from suspend on workstation CPUs with x2apic enabled. It will also work on systems with one maxcpus=1 but still using x2apic since x2apic is also re-enabled in lapic_resume(). 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> --- v1->v2: * Clarify it's in workstations in commit message * Fix style issues in comment and curly braces --- arch/x86/include/asm/smp.h | 1 + arch/x86/kernel/acpi/sleep.c | 13 +++++++++---- arch/x86/kernel/head_64.S | 15 +++++++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-)