[PATCH] x86/AMD: Fix LLC ID for AMD Fam17h systems

[PATCH] x86/AMD: Fix LLC ID for AMD Fam17h systems

[Yazen Ghannam]

Fix an underflow bug with the current Fam17h LLC ID derivation by
simplifying the derivation, and also move it into amd_get_topology().

Signed-off-by: Yazen Ghannam <Yazen.Ghannam@amd.com>
Cc: stable@vger.kernel.org # v4.6..
Fixes: 3849e91f571d ("x86/AMD: Fix last level cache topology for AMD Fam17h systems")
---
 arch/x86/kernel/cpu/amd.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index b81fe2d..babb93a 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -314,11 +314,30 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
 		smp_num_siblings = ((ebx >> 8) & 3) + 1;
 		c->x86_max_cores /= smp_num_siblings;
 		c->cpu_core_id = ebx & 0xff;
+
+		/*
+		 * We may have multiple LLCs if L3 Caches exist, so check if we
+		 * have an L3 cache by looking at the L3 cache cpuid leaf.
+		 */
+		if (cpuid_edx(0x80000006)) {
+			/* LLC is at the Node level. */
+			if (c->x86 == 0x15)
+				per_cpu(cpu_llc_id, cpu) = node_id;
+
+			/*
+			 * LLC is at the Core Complex level.
+			 * Core Complex Id is ApicId[3].
+			 */
+			else if (c->x86 == 0x17)
+				per_cpu(cpu_llc_id, cpu) = c->initial_apicid >> 3;
+		}
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;
 
 		rdmsrl(MSR_FAM10H_NODE_ID, value);
 		node_id = value & 7;
+
+		per_cpu(cpu_llc_id, cpu) = node_id;
 	} else
 		return;
 
@@ -329,9 +348,6 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
 		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
 		cus_per_node = c->x86_max_cores / nodes_per_socket;
 
-		/* store NodeID, use llc_shared_map to store sibling info */
-		per_cpu(cpu_llc_id, cpu) = node_id;
-
 		/* core id has to be in the [0 .. cores_per_node - 1] range */
 		c->cpu_core_id %= cus_per_node;
 	}
@@ -347,7 +363,6 @@ static void amd_detect_cmp(struct cpuinfo_x86 *c)
 #ifdef CONFIG_SMP
 	unsigned bits;
 	int cpu = smp_processor_id();
-	unsigned int socket_id, core_complex_id;
 
 	bits = c->x86_coreid_bits;
 	/* Low order bits define the core id (index of core in socket) */
@@ -357,18 +372,6 @@ static void amd_detect_cmp(struct cpuinfo_x86 *c)
 	/* use socket ID also for last level cache */
 	per_cpu(cpu_llc_id, cpu) = c->phys_proc_id;
 	amd_get_topology(c);
-
-	/*
-	 * Fix percpu cpu_llc_id here as LLC topology is different
-	 * for Fam17h systems.
-	 */
-	 if (c->x86 != 0x17 || !cpuid_edx(0x80000006))
-		return;
-
-	socket_id	= (c->apicid >> bits) - 1;
-	core_complex_id	= (c->apicid & ((1 << bits) - 1)) >> 3;
-
-	per_cpu(cpu_llc_id, cpu) = (socket_id << 3) | core_complex_id;
 #endif
 }
 
-- 
1.9.1

Re: [PATCH] x86/AMD: Fix LLC ID for AMD Fam17h systems

[Thomas Gleixner]

On Wed, 26 Oct 2016, Yazen Ghannam wrote:

> Fix an underflow bug with the current Fam17h LLC ID derivation by
> simplifying the derivation, and also move it into amd_get_topology().

This changelog is useless.

It does not give any information what the bug is and what kind of damage it
does on which machines.

Further you claim a simplification and fail to explain what is made
simpler.

Instead you tell what the patch does: "and also move it into
amd_get_topology()". I can see that when reading the patch. 

If you would tell why it's correct to move it there, then it might have a
value.

> @@ -314,11 +314,30 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
>  		smp_num_siblings = ((ebx >> 8) & 3) + 1;
>  		c->x86_max_cores /= smp_num_siblings;
>  		c->cpu_core_id = ebx & 0xff;
> +
> +		/*
> +		 * We may have multiple LLCs if L3 Caches exist, so check if we
> +		 * have an L3 cache by looking at the L3 cache cpuid leaf.
> +		 */
> +		if (cpuid_edx(0x80000006)) {
> +			/* LLC is at the Node level. */
> +			if (c->x86 == 0x15)
> +				per_cpu(cpu_llc_id, cpu) = node_id;
> +

So this is new and of course the changelog does not tell anything about the
rationale of this change, which is obviously unrelated to the fam 0x17
issue.

> +			/*
> +			 * LLC is at the Core Complex level.
> +			 * Core Complex Id is ApicId[3].
> +			 */
> +			else if (c->x86 == 0x17)
> +				per_cpu(cpu_llc_id, cpu) = c->initial_apicid >> 3;

This whole if/else block lacks curly braces. See:

  https://marc.info/?l=linux-kernel&m=147351236615103

> +		}
>  	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
>  		u64 value;
>  
>  		rdmsrl(MSR_FAM10H_NODE_ID, value);
>  		node_id = value & 7;
> +
> +		per_cpu(cpu_llc_id, cpu) = node_id;

This seems to be moved from the hunk below. Again, no relation to the
subject of this patch and no explanation at all.

> -	/*
> -	 * Fix percpu cpu_llc_id here as LLC topology is different
> -	 * for Fam17h systems.
> -	 */
> -	 if (c->x86 != 0x17 || !cpuid_edx(0x80000006))
> -		return;
> -
> -	socket_id	= (c->apicid >> bits) - 1;
> -	core_complex_id	= (c->apicid & ((1 << bits) - 1)) >> 3;
> -
> -	per_cpu(cpu_llc_id, cpu) = (socket_id << 3) | core_complex_id;

So if I've read the patch correctly then the trivial fix for fam17h would
have been:

> +	per_cpu(cpu_llc_id, cpu) = c->initial_apicid >> 3;

Right?

And this one liner wants to be a seperate patch with a proper
explanation. And that simple hunk can be tagged for stable.

The rest of the patch is cleanup and improvement and want's to be seperated
out and explained proper.

Thanks,

	tglx

Re: [PATCH] x86/AMD: Fix LLC ID for AMD Fam17h systems

[Yazen Ghannam]

>> +			/*
>> +			 * LLC is at the Core Complex level.
>> +			 * Core Complex Id is ApicId[3].
>> +			 */
>> +			else if (c->x86 == 0x17)
>> +				per_cpu(cpu_llc_id, cpu) = c->initial_apicid >> 3;
> 
> This whole if/else block lacks curly braces. See:
> 
>   https://marc.info/?l=linux-kernel&m=147351236615103
> 

Okay, understood.

>> -	/*
>> -	 * Fix percpu cpu_llc_id here as LLC topology is different
>> -	 * for Fam17h systems.
>> -	 */
>> -	 if (c->x86 != 0x17 || !cpuid_edx(0x80000006))
>> -		return;
>> -
>> -	socket_id	= (c->apicid >> bits) - 1;
>> -	core_complex_id	= (c->apicid & ((1 << bits) - 1)) >> 3;
>> -
>> -	per_cpu(cpu_llc_id, cpu) = (socket_id << 3) | core_complex_id;
> 
> So if I've read the patch correctly then the trivial fix for fam17h would
> have been:
> 
>> +	per_cpu(cpu_llc_id, cpu) = c->initial_apicid >> 3;
> 
> Right?
> 

Right.

> And this one liner wants to be a seperate patch with a proper
> explanation. And that simple hunk can be tagged for stable.
> 

Okay, I'll make it a separate patch. It still wouldn't be a one-liner
because then socket_id and core_complex_id would be left unused and
should be removed.

> The rest of the patch is cleanup and improvement and want's to be seperated
> out and explained proper.
> 

Okay, will do.

Thanks,
Yazen