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

[Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>]

Boris,

On 7/24/17 18:14, Borislav Petkov wrote:
> 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...?

Actually, this is not totally accurate. My apology. This patch is mainly fix to 
incorrect core ID in /proc/cpuinfo. I'll update the commit message accordingly 
for V5.

Currently, with 6-core downcore configuration (out of normally 8), /proc/cpuinfo 
is currently showing "core id" as.

NODE: 0
cpu 0 core id : 0
cpu 1 core id : 1
cpu 2 core id : 2
cpu 3 core id : 4
cpu 4 core id : 5
cpu 5 core id : 0

NODE: 1
cpu 6 core id : 2
cpu 7 core id : 3
cpu 8 core id : 4
cpu 9 core id : 0
cpu 10 core id : 1
cpu 11 core id : 2

NODE: ....

This is due to the cpu_core_id fixup in amd_get_topology() below:

         /* fixup multi-node processor information */
         if (nodes_per_socket > 1) {
                 u32 cus_per_node;

                 set_cpu_cap(c, X86_FEATURE_AMD_DCM);
                 cus_per_node = c->x86_max_cores / nodes_per_socket;

                 /* core id has to be in the [0 .. cores_per_node - 1] range */
                 c->cpu_core_id %= cus_per_node;
         }

In this case, the cpu_core_id are {0, 1, 2 ,4, 5, 6, 8 , 9, 10, 12, 13, 14, 
...}. Here, the x86_max_cores is 24, the node_per_socket is 4, and cus_per_node 
is 6. When apply the fix up, the cpu_core_id ended up incorrect as shown above. 
This logic assumes that the cpu_core_id are contiguous across the socket.

With this patch, this fixup is not needed since the c->cpu_core_id are already 
between [0, 6].

>> [...]
>>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@amd.com
>
> 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.
>

Let me look into this to re-factoring for reuse here.

>> +		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.

Please see above description.

>> +		 */
>> +		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.
>

Well, for now, it has to be for non-family17h, which seems odd. I am planning to 
clean this up as well.

Thanks,
Suravee