Re: [PATCH v4 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration

[Borislav Petkov]

On Mon, Jul 24, 2017 at 04:22:45AM -0500, Suravee Suthikulpanit wrote:
> For family17h, current cpu_core_id is directly taken from the value
> CPUID_Fn8000001E_EBX[7:0] (CoreId), which is the physical ID of the
> core within a die. However, on system with downcore configuration
> (where not all physical cores within a die are available), this could
> result in the case where cpu_core_id > (cores_per_node - 1).

And that is a problem because...?

> Fix up the cpu_core_id by breaking down the bitfields of CoreId,
> and calculate relative ID using available topology information.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
>  arch/x86/kernel/cpu/amd.c | 74 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 54 insertions(+), 20 deletions(-)

Btw, I have to say, it is much easier to review now.

> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index a2a52b5..b5ea28f 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -305,37 +305,71 @@ static void __get_topoext(struct cpuinfo_x86 *c, int cpu)
>  {
>  	u32 eax, ebx, ecx, edx;
>  	u8 node_id;
> +	u16 l3_nshared = 0;
> +
> +	if (cpuid_edx(0x80000006)) {

Ok, so Janakarajan did some L3 detection:

https://lkml.kernel.org/r/cover.1497452002.git.Janakarajan.Natarajan@xxxxxxx

and now that you need it too, I'd like you to refactor and unify that
L3 detection code and put it in arch/x86/kernel/cpu/intel_cacheinfo.c
(yes, we will rename that file one fine day :-)) along with accessors
for other users to call. init_amd_cacheinfo() looks good to me right
now.

> +		cpuid_count(0x8000001d, 3, &eax, &ebx, &ecx, &edx);
> +		l3_nshared = ((eax >> 14) & 0xfff) + 1;
> +	}
>  
>  	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>  
>  	node_id  = ecx & 0xff;
>  	smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
>  
> -	if (c->x86 == 0x15)
> -		c->cu_id = ebx & 0xff;
> -
> -	if (c->x86 >= 0x17) {
> -		c->cpu_core_id = ebx & 0xff;
> -
> -		if (smp_num_siblings > 1)
> -			c->x86_max_cores /= smp_num_siblings;
> -	}
> +	switch (c->x86) {
> +	case 0x17: {
> +		u32 tmp, ccx_offset, cpu_offset;
>  
> -	/*
> -	 * 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)) {
> -		if (c->x86 == 0x17) {
> +		/*
> +		 * In family 17h, the CPUID_Fn8000001E_EBX[7:0] (CoreId)
> +		 * is non-contiguous in downcore and non-SMT cases.
> +		 * Fixup the cpu_core_id to be contiguous for cores within
> +		 * the die.

Why do we need it to be contiguous? It is not contiguous on Intel too.

> +		 */
> +		tmp = ebx & 0xff;
> +		if (smp_num_siblings == 1) {
>  			/*
> -			 * LLC is at the core complex level.
> -			 * Core complex id is ApicId[3].
> +			 * CoreId bit-encoding for SMT-disabled
> +			 * [7:4] : die
> +			 * [3]   : ccx
> +			 * [2:0] : core
>  			 */
> -			per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
> +			ccx_offset = ((tmp >> 3) & 1) * l3_nshared;
> +			cpu_offset = tmp & 7;
>  		} else {
> -			/* LLC is at the node level. */
> -			per_cpu(cpu_llc_id, cpu) = node_id;
> +			/*
> +			 * CoreId bit-encoding for SMT-enabled
> +			 * [7:3] : die
> +			 * [2]   : ccx
> +			 * [1:0] : core
> +			 */
> +			ccx_offset = ((tmp >> 2) & 1) * l3_nshared /
> +				       smp_num_siblings;
> +			cpu_offset = tmp & 3;
> +			c->x86_max_cores /= smp_num_siblings;
> +
>  		}
> +		c->cpu_core_id = ccx_offset + cpu_offset;
> +
> +		/*
> +		 * Family17h L3 cache (LLC) is at Core Complex (CCX).
> +		 * There could be multiple CCXs in a node.
> +		 * CCX ID is ApicId[3].
> +		 */
> +		per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
> +
> +		pr_debug("Fixup coreid:%#x to cpu_core_id:%#x\n",
> +			 tmp, c->cpu_core_id);
> +		break;
> +	}
> +	case 0x15:
> +		c->cu_id = ebx & 0xff;
> +		/* Follow through */
> +	default:
> +		/* LLC is default to L3, which generally per-node */
> +		if (l3_nshared > 0)
> +			per_cpu(cpu_llc_id, cpu) = node_id;

If this needs to be executed unconditionally, just move it out of the
switch-case.

-- 
Regards/Gruss,
    Boris.

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