lkml.org 
[lkml]   [2023]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/2] x86: Enable x2apic during resume from suspend if used previously
From
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

\
 
 \ /
  Last update: 2023-10-31 19:54    [W:0.084 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site