Re: [PATCH v6 07/11] x86/smpboot: Disable parallel boot for AMD CPUs

From: Thomas Gleixner <tglx@linutronix.de>
To: David Woodhouse <dwmw2@infradead.org>,
	Kim Phillips <kim.phillips@amd.com>,
	Usama Arif <usama.arif@bytedance.com>,
	arjan@linux.intel.com
Cc: mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
	hpa@zytor.com, x86@kernel.org, pbonzini@redhat.com,
	paulmck@kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, rcu@vger.kernel.org, mimoja@mimoja.de,
	hewenliang4@huawei.com, thomas.lendacky@amd.com,
	seanjc@google.com, pmenzel@molgen.mpg.de,
	fam.zheng@bytedance.com, punit.agrawal@bytedance.com,
	simon.evans@bytedance.com, liangma@liangbit.com,
	Mario Limonciello <Mario.Limonciello@amd.com>
Subject: Re: [PATCH v6 07/11] x86/smpboot: Disable parallel boot for AMD CPUs
Date: Tue, 07 Feb 2023 15:44:11 +0100	[thread overview]
Message-ID: <87pmalv8lg.ffs@tglx> (raw)
In-Reply-To: <9acc229e3d4931fff9106d60b57e0f46941bfb50.camel@infradead.org>

David!

On Tue, Feb 07 2023 at 10:04, David Woodhouse wrote:
> On Tue, 2023-02-07 at 01:23 +0100, Thomas Gleixner wrote:
>> > When we're not in x2apic mode, we can use CPUID 0x1 because the 8 bits
>> > of APIC ID we find there are perfectly sufficient.
>> 
>> Is that worth the trouble?
>
> Well, that's what was being debated. I think the conclusion that was
> bring reached was that it *is* worth the trouble, because there will be
> a number of physical and especially virtual machines which have a high
> CPU count but which don't actually use X2APIC mode. And which might not
> even *support* CPUID 0xb.
>
> So using CPUID 0x1 when there is no APIC ID greater than 254 does seem
> to make sense.

Fair enough.

>> > Even though we *can* support non-X2APIC processors, we *might* want to
>> > play it safe and not go back that far; only enabling parallel bringup
>> > on machines with X2APIC which roughly correlates with "lots of CPUs"
>> > since that's where the benefit is.
>> 
>> The parallel bringup code is complex enough already, so please don't
>> optimize for the non-interesting case in the first place. When this has
>> stabilized then the CPUID 0x1 mechanism can be added if anyone thinks
>> it's interesting. KISS is still the best engineering principle.
>
> Actually it ends up being trivial. It probably makes sense to keep it
> in there even if it can only be exercised by a deliberate opt-in on
> older CPUs. I reworked the register usage from your original anyway,
> which helps a little.
>
> 	testl	$STARTUP_APICID_CPUID_0B, %edx
> 	jnz	.Luse_cpuid_0b
> 	testl	$STARTUP_APICID_CPUID_01, %edx
> 	jnz	.Luse_cpuid_01
> 	andl	$0x0FFFFFFF, %edx
> 	jmp	.Lsetup_AP
>
> .Luse_cpuid_01:
> 	mov	$0x01, %eax
> 	cpuid
> 	mov	%ebx, %edx
> 	shr	$24, %edx
> 	jmp	.Lsetup_AP
>
> .Luse_cpuid_0b:
> 	mov	$0x0B, %eax
> 	xorl	%ecx, %ecx
> 	cpuid
>
> .Lsetup_AP:
> 	/* EDX contains the APICID of the current CPU */

That looks trivial enough. So no objections from my side. Not sure
whether this needs a special opt-in though. We probably want an opt-out
for the parallel bringup mode for diagnosis purposes anyway and that
should be good enough for a start.

Thanks,

        tglx