Re: [PATCH v2 2/2] x86/AMD: Group cpu_llc_id assignment by topology feature and family

[Borislav Petkov]

On Fri, Oct 28, 2016 at 10:51:58AM -0500, Yazen Ghannam wrote:
> Currently, we assume that a system has multiple last level caches only if
> there are multiple nodes, and that the cpu_llc_id is equal to the node_id.
> This no longer applies since Fam17h can have multiple last level caches
> within a node.
> 
> So group the cpu_llc_id assignment by topology feature and family.

That's a very nice intro, good.

> The NODEID_MSR feature only applies to Fam10h in which case the llc is at

s/llc/LLC (Last Level Cache/

Let's try to have abbreviations written out in their first mention in the text.

> the node level.
> 
> The TOPOEXT feature is used on families 15h, 16h and 17h. So far we only
> see multiple last level caches if L3 caches are available. Otherwise, the
> cpu_llc_id will default to be the phys_proc_id. We have L3 caches only on
> families 15h and 17h. On Fam15h, the llc is at the node level. On Fam17h,

s/llc/LLC/g

> the llc is at the core complex level and can be found by right shifting the
      ^^^

LLC

> apicid. Also, keep the family checks explicit so that new families will
> fall back to the default.
> 
> Single node systems in families 10h and 15h will have a Node ID of 0 which
> will be the same as the phys_proc_id, so we don't need to check for
> multiple nodes before using the node_id.
> 
> Signed-off-by: Yazen Ghannam <Yazen.Ghannam@xxxxxxx>
> ---
>  arch/x86/kernel/cpu/amd.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index c3fc337..be70345 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -314,11 +314,31 @@ 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.

x86 instructions in caps please: CPUID

> +		 */
> +		if (cpuid_edx(0x80000006)) {
> +			if (c->x86 == 0x15) {
> +				/* LLC is at the node level. */
> +				per_cpu(cpu_llc_id, cpu) = node_id;
> +
> +			} else if (c->x86 == 0x17) {

				How about >= ?

> +				/*
> +				 * LLC is at the core complex level.
> +				 * Core complex id is ApicId[3].
> +				 */
> +				per_cpu(cpu_llc_id, cpu) = c->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;

Btw, please add for your next submission:

Tested-by: Borislav Petkov <bp@xxxxxxx>

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix ImendÃrffer, Jane Smithard, Graham Norton, HRB 21284 (AG NÃrnberg)
--