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

[Thomas Gleixner <tglx@linutronix.de>]

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