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@xxxxxxxxxxxxxxx # Only v6.9 and above >> Link: https://lore.kernel.org/lkml/1529686927-7665-1-git-send-email-suravee.suthikulpanit@xxxxxxx/ [1] >> Link: https://lore.kernel.org/lkml/20080818181435.523309000@xxxxxxxxxxxxxxxxxxxxx/ [2] >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 [3] >> Suggested-by: Naveen N Rao (AMD) <naveen@xxxxxxxxxx> >> Fixes: 3986a0a805e6 ("x86/CPU/AMD: Derive CPU topology from CPUID function 0xB when available") >> Signed-off-by: K Prateek Nayak <kprateek.nayak@xxxxxxx> >> --- >> 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
