Re: [PATCH v2 1/2] x86: Enable x2apic during resume from suspend if used previously

[Mario Limonciello <mario.limonciello@amd.com>]

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