Re: [PATCH v4 2/4] x86/cpu/topology: Always try cpu_parse_topology_ext() on AMD/Hygon

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Boris,

On 8/30/2025 10:49 PM, Borislav Petkov wrote:
> On Mon, Aug 25, 2025 at 07:57:30AM +0000, K Prateek Nayak wrote:
>> This has not been a problem on baremetal platforms since support for
>> TOPOEXT (Fam 0x15 and later) predates the support for CPUID leaf 0xb
>> (Fam 0x17[Zen2] and later), however, for AMD guests on QEMU, "x2apic"
>> feature can be enabled independent of the "topoext" feature where QEMU
>> expects topology and the initial APICID to be parsed using the CPUID
>> leaf 0xb (especially when number of cores > 255) which is populated
>> independent of the "topoext" feature flag.
> 
> This sounds like we're fixing the kernel because someone *might* supply
> a funky configuration to qemu. You need to understand that we're not wagging
> the dog and fixing the kernel because of that.
> 
> If someone manages to enable some weird concoction of feature flags in qemu,
> we certainly won't "fix" that in the kernel.
> 
> So let's concentrate that text around fixing the issue of parsing CPUID
> topology leafs which we should parse and looking at CPUID flags only for those
> feature leafs, for which those flags are existing.
> 
> If qemu wants stuff to work, then it better emulate the feature flag
> configuration like the hw does.

Ack. I'll add the relevant details in
Documentation/arch/x86/topology.rst in the next version as discussed but
I believe you discovered the intentions for this fix in the kernel
below.

> 
>> Unconditionally call cpu_parse_topology_ext() on AMD and Hygon
>> processors to first parse the topology using the XTOPOLOGY leaves
>> (0x80000026 / 0xb) before using the TOPOEXT leaf (0x8000001e).
>>
>> Cc: stable@vger.kernel.org # Only v6.9 and above
>> Link: https://lore.kernel.org/lkml/1529686927-7665-1-git-send-email-suravee.suthikulpanit@amd.com/ [1]
>> Link: https://lore.kernel.org/lkml/20080818181435.523309000@linux-os.sc.intel.com/ [2]
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 [3]
>> Suggested-by: Naveen N Rao (AMD) <naveen@kernel.org>
>> Fixes: 3986a0a805e6 ("x86/CPU/AMD: Derive CPU topology from CPUID function 0xB when available")
>> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
>> ---
>> Changelog v3..v4:
>>
>> o Quoted relevant section of the PPR justifying the changes.
>>
>> o Moved this patch up ahead.
>>
>> o Cc'd stable and made a note that backports should target v6.9 and
>>   above since this depends on the x86 topology rewrite.
> 
> Put that explanation in the CC:stable comment.

Ack.

> 
>> ---
>>  arch/x86/kernel/cpu/topology_amd.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/topology_amd.c b/arch/x86/kernel/cpu/topology_amd.c
>> index 827dd0dbb6e9..4e3134a5550c 100644
>> --- a/arch/x86/kernel/cpu/topology_amd.c
>> +++ b/arch/x86/kernel/cpu/topology_amd.c
>> @@ -175,18 +175,14 @@ static void topoext_fixup(struct topo_scan *tscan)
>>  
>>  static void parse_topology_amd(struct topo_scan *tscan)
>>  {
>> -	bool has_topoext = false;
>> -
>>  	/*
>> -	 * If the extended topology leaf 0x8000_001e is available
>> -	 * try to get SMT, CORE, TILE, and DIE shifts from extended
>> +	 * Try to get SMT, CORE, TILE, and DIE shifts from extended
>>  	 * CPUID leaf 0x8000_0026 on supported processors first. If
>>  	 * extended CPUID leaf 0x8000_0026 is not supported, try to
>>  	 * get SMT and CORE shift from leaf 0xb first, then try to
>>  	 * get the CORE shift from leaf 0x8000_0008.
>>  	 */
>> -	if (cpu_feature_enabled(X86_FEATURE_TOPOEXT))
>> -		has_topoext = cpu_parse_topology_ext(tscan);
>> +	bool has_topoext = cpu_parse_topology_ext(tscan);
> 
> Ok, I see what the point here is - you want to parse topology regardless of
> X86_FEATURE_TOPOEXT.
> 
> Which is true - latter "indicates support for Core::X86::Cpuid::CachePropEax0
> and Core::X86::Cpuid::ExtApicId" only and the leafs cpu_parse_topology_ext()
> attempts to parse are different ones.
> 
> So, "has_topoext" doesn't have anything to do with X86_FEATURE_TOPOEXT - it
> simply means that the topology extensions parser found some extensions and
> parsed them. So let's rename that variable differently so that there is no
> confusion.
> 
> You can do the renaming in parse_8000_001e() in a later patch as that is only
> a cosmetic thing and we don't want to send that to stable.

Ack! Does "has_xtopology" sound good or should we go for something more
explicit like "has_xtopology_0x26_0xb"?

Patch 3 will get rid of that "has_topoext" argument in parse_8000_001e()
entirely so I'll rename the local variable here and use the subsequent
cleanup for parse_8000_001e().

-- 
Thanks and Regards,
Prateek