Messages in this thread | | | Date | Tue, 31 Oct 2023 13:53:34 -0500 | Subject | Re: [PATCH v2 1/2] x86: Enable x2apic during resume from suspend if used previously | From | Mario Limonciello <> |
| |
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
| |