linux-kernel - Re: [PATCH v3 4/4] x86/cpu/topology: Check for X86_FEATURE_XTOPOLOGY instead of passing has

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ms7o3kn6.ffs@tglx>
Date: Sun, 24 Aug 2025 15:38:05 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: K Prateek Nayak <kprateek.nayak@....com>, Ingo Molnar
 <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
 <dave.hansen@...ux.intel.com>, Sean Christopherson <seanjc@...gle.com>,
 Paolo Bonzini <pbonzini@...hat.com>, x86@...nel.org
Cc: Naveen rao <naveen.rao@....com>, Sairaj Kodilkar <sarunkod@....com>, "H.
 Peter Anvin" <hpa@...or.com>, "Peter Zijlstra (Intel)"
 <peterz@...radead.org>, "Xin Li (Intel)" <xin@...or.com>, Pawan Gupta
 <pawan.kumar.gupta@...ux.intel.com>, linux-kernel@...r.kernel.org,
 kvm@...r.kernel.org, Mario Limonciello <mario.limonciello@....com>,
 "Gautham R. Shenoy" <gautham.shenoy@....com>, Babu Moger
 <babu.moger@....com>, Suravee Suthikulpanit
 <suravee.suthikulpanit@....com>, K Prateek Nayak <kprateek.nayak@....com>
Subject: Re: [PATCH v3 4/4] x86/cpu/topology: Check for
 X86_FEATURE_XTOPOLOGY instead of passing has_topoext

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@....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);