cpu_parse_topology_ext() sets X86_FEATURE_XTOPOLOGY before returning
true if any of the XTOPOLOGY leaf could be parsed successfully.

Instead of storing and passing around this return value using
"has_topoext" in parse_topology_amd(), check for X86_FEATURE_XTOPOLOGY
instead in parse_8000_001e() to simplify the flow.

No functional changes intended.

Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
Changelog v2..v3:

o Use cpu_feature_enabled() when checking for X86_FEATURE_XTOPOLOGY.
---
 arch/x86/kernel/cpu/topology_amd.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/topology_amd.c b/arch/x86/kernel/cpu/topology_amd.c
index 3d01675d94f5..138a09528083 100644
--- a/arch/x86/kernel/cpu/topology_amd.c
+++ b/arch/x86/kernel/cpu/topology_amd.c
@@ -59,7 +59,7 @@ static void store_node(struct topo_scan *tscan, u16 nr_nodes, u16 node_id)
 	tscan->amd_node_id = node_id;
 }
 
-static bool parse_8000_001e(struct topo_scan *tscan, bool has_topoext)
+static bool parse_8000_001e(struct topo_scan *tscan)
 {
 	struct {
 		// eax
@@ -81,7 +81,7 @@ static bool parse_8000_001e(struct topo_scan *tscan, bool has_topoext)
 
 	cpuid_leaf(0x8000001e, &leaf);
 
-	if (!has_topoext) {
+	if (!cpu_feature_enabled(X86_FEATURE_XTOPOLOGY)) {
 		/*
 		 * Prefer initial_apicid parsed from XTOPOLOGY leaf
 		 * 0x8000026 or 0xb if available. Otherwise prefer the
@@ -179,6 +179,9 @@ static void topoext_fixup(struct topo_scan *tscan)
 
 static void parse_topology_amd(struct topo_scan *tscan)
 {
+	if (cpu_feature_enabled(X86_FEATURE_AMD_HTR_CORES))
+		tscan->c->topo.cpu_type = cpuid_ebx(0x80000026);
+
 	/*
 	 * Try to get SMT, CORE, TILE, and DIE shifts from extended
 	 * CPUID leaf 0x8000_0026 on supported processors first. If
@@ -186,16 +189,11 @@ static void parse_topology_amd(struct topo_scan *tscan)
 	 * get SMT and CORE shift from leaf 0xb first, then try to
 	 * get the CORE shift from leaf 0x8000_0008.
 	 */
-	bool has_topoext = cpu_parse_topology_ext(tscan);
-
-	if (cpu_feature_enabled(X86_FEATURE_AMD_HTR_CORES))
-		tscan->c->topo.cpu_type = cpuid_ebx(0x80000026);
-
-	if (!has_topoext && !parse_8000_0008(tscan))
+	if (!cpu_parse_topology_ext(tscan) && !parse_8000_0008(tscan))
 		return;
 
 	/* Prefer leaf 0x8000001e if available */
-	if (parse_8000_001e(tscan, has_topoext))
+	if (parse_8000_001e(tscan))
 		return;
 
 	/* Try the NODEID MSR */
-- 
2.34.1

On Mon, Aug 18 2025 at 06:04, K. Prateek Nayak wrote:

> cpu_parse_topology_ext() sets X86_FEATURE_XTOPOLOGY before returning
> true if any of the XTOPOLOGY leaf could be parsed successfully.
>
> Instead of storing and passing around this return value using
> "has_topoext" in parse_topology_amd(), check for X86_FEATURE_XTOPOLOGY
> instead in parse_8000_001e() to simplify the flow.
>
> No functional changes intended.
>
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
> Changelog v2..v3:
>
> o Use cpu_feature_enabled() when checking for X86_FEATURE_XTOPOLOGY.
> ---
>  arch/x86/kernel/cpu/topology_amd.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/topology_amd.c b/arch/x86/kernel/cpu/topology_amd.c
> index 3d01675d94f5..138a09528083 100644
> --- a/arch/x86/kernel/cpu/topology_amd.c
> +++ b/arch/x86/kernel/cpu/topology_amd.c
> @@ -59,7 +59,7 @@ static void store_node(struct topo_scan *tscan, u16 nr_nodes, u16 node_id)
>  	tscan->amd_node_id = node_id;
>  }
>  
> -static bool parse_8000_001e(struct topo_scan *tscan, bool has_topoext)
> +static bool parse_8000_001e(struct topo_scan *tscan)
>  {
>  	struct {
>  		// eax
> @@ -81,7 +81,7 @@ static bool parse_8000_001e(struct topo_scan *tscan, bool has_topoext)
>  
>  	cpuid_leaf(0x8000001e, &leaf);
>  
> -	if (!has_topoext) {
> +	if (!cpu_feature_enabled(X86_FEATURE_XTOPOLOGY)) {
>  		/*
>  		 * Prefer initial_apicid parsed from XTOPOLOGY leaf
>  		 * 0x8000026 or 0xb if available. Otherwise prefer the

That's patently wrong.

The leaves might be "available", but are not guaranteed to be valid. So
FEATURE_XTOPOLOGY gives you the wrong answer.

The has_topoext logic is there for a reason.

https://github.com/InstLatx64/InstLatx64.git has a gazillion of CPUID
samples from various machines and there are systems which advertise 0xB,
but it's empty and you won't have an APIC ID at all...

So all what needs to be done is preventing 8..1e parsing to overwrite
the APIC ID, when a valid 0xb/0x26 was detected.

Something like the below.

Btw, your fixes tag is only half correct. The problem existed already
_before_ the topology rewrite and I remember debating exactly this
problem with Boris and Tom back then when I sanitized this nightmare.

Thanks,

        tglx
---
 arch/x86/kernel/cpu/topology_amd.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

--- a/arch/x86/kernel/cpu/topology_amd.c
+++ b/arch/x86/kernel/cpu/topology_amd.c
@@ -81,20 +81,25 @@ static bool parse_8000_001e(struct topo_
 
 	cpuid_leaf(0x8000001e, &leaf);
 
-	tscan->c->topo.initial_apicid = leaf.ext_apic_id;
-
 	/*
-	 * If leaf 0xb is available, then the domain shifts are set
-	 * already and nothing to do here. Only valid for family >= 0x17.
+	 * If leaf 0xb/0x26 is available, then the APIC ID and the domain
+	 * shifts are set already.
 	 */
-	if (!has_topoext && tscan->c->x86 >= 0x17) {
+	 if (!has_topoext) {
+		tscan->c->topo.initial_apicid = leaf.ext_apic_id;
+
 		/*
-		 * Leaf 0x80000008 set the CORE domain shift already.
-		 * Update the SMT domain, but do not propagate it.
+		 * Leaf 0x8000008 sets the CORE domain shift but not the
+		 * SMT domain shift. On CPUs with family >= 0x17, there
+		 * might be hyperthreads.
 		 */
-		unsigned int nthreads = leaf.core_nthreads + 1;
+		if (tscan->c->x86 >= 0x17) {
+			/* Update the SMT domain, but do not propagate it. */
+			unsigned int nthreads = leaf.core_nthreads + 1;
 
-		topology_update_dom(tscan, TOPO_SMT_DOMAIN, get_count_order(nthreads), nthreads);
+			topology_update_dom(tscan, TOPO_SMT_DOMAIN,
+					    get_count_order(nthreads), nthreads);
+		}
 	}
 
 	store_node(tscan, leaf.nnodes_per_socket + 1, leaf.node_id);

On Sun, Aug 24 2025 at 15:38, Thomas Gleixner wrote:
>> -	if (!has_topoext) {
>> +	if (!cpu_feature_enabled(X86_FEATURE_XTOPOLOGY)) {
>>  		/*
>>  		 * Prefer initial_apicid parsed from XTOPOLOGY leaf
>>  		 * 0x8000026 or 0xb if available. Otherwise prefer the
>
> That's patently wrong.
>
> The leaves might be "available", but are not guaranteed to be valid. So
> FEATURE_XTOPOLOGY gives you the wrong answer.
>
> The has_topoext logic is there for a reason.

Hrm. I have to correct myself. It's set by the 0xb... parsing when that
finds a valid leaf. My memory tricked me on that.

So yes, it can be used for that, but that's a cleanup. The simple fix
should be applied first as that's trivial to backport.

Hello Thomas,

On 8/24/2025 8:25 PM, Thomas Gleixner wrote:
> On Sun, Aug 24 2025 at 15:38, Thomas Gleixner wrote:
>>> -	if (!has_topoext) {
>>> +	if (!cpu_feature_enabled(X86_FEATURE_XTOPOLOGY)) {
>>>  		/*
>>>  		 * Prefer initial_apicid parsed from XTOPOLOGY leaf
>>>  		 * 0x8000026 or 0xb if available. Otherwise prefer the
>>
>> That's patently wrong.
>>
>> The leaves might be "available", but are not guaranteed to be valid. So
>> FEATURE_XTOPOLOGY gives you the wrong answer.
>>
>> The has_topoext logic is there for a reason.
> 
> Hrm. I have to correct myself. It's set by the 0xb... parsing when that
> finds a valid leaf. My memory tricked me on that.
> 
> So yes, it can be used for that, but that's a cleanup. The simple fix
> should be applied first as that's trivial to backport.

Thank you for taking a look at the series. I'll move the fixes ahead and
refresh the current Patch 2 with your suggested diff in the next
version.

-- 
Thanks and Regards,
Prateek