Re: [PATCH 2/2] x86/resctrl: Support CPUID enumeration of MBM counter width

[Reinette Chatre]

Hi Borislav,

On 5/4/2020 9:19 PM, Reinette Chatre wrote:
> On 5/4/2020 12:56 AM, Borislav Petkov wrote:
>> Hi,
>>
>> On Sun, May 03, 2020 at 11:51:00AM -0700, Reinette Chatre wrote:
>>> I am struggling with what should follow ...
>>
>> Since a diff is better than a thousand words :-) see below.
>>
> 
> Thank you so much for providing the details. Your explanation is clear
> to me but I do have one clarification question ...
> 
> 
>> @@ -597,6 +598,8 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
>>  			x86_amd_ls_cfg_ssbd_mask = 1ULL << bit;
>>  		}
>>  	}
>> +
>> +	resctrl_cpu_detect(c);
>>  }
>>  
> 
> ...
> 
>> @@ -322,6 +323,11 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>>  		detect_ht_early(c);
>>  }
>>  
>> +static void bsp_init_intel(struct cpuinfo_x86 *c)
>> +{
>> +	resctrl_cpu_detect(c);
>> +}
>> +
>>  #ifdef CONFIG_X86_32
>>  /*
>>   *	Early probe support logic for ppro memory erratum #50
>> @@ -961,6 +967,7 @@ static const struct cpu_dev intel_cpu_dev = {
>>  #endif
>>  	.c_detect_tlb	= intel_detect_tlb,
>>  	.c_early_init   = early_init_intel,
>> +	.c_bsp_init	= bsp_init_intel,
>>  	.c_init		= init_intel,
>>  	.c_x86_vendor	= X86_VENDOR_INTEL,
>>  };
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index d8cc5223b7ce..5e5955aa6593 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -22,7 +22,7 @@
>>  #include <linux/cpuhotplug.h>
>>  
>>  #include <asm/intel-family.h>
>> -#include <asm/resctrl_sched.h>
>> +#include <asm/resctrl.h>
>>  #include "internal.h"
>>  
>>  /* Mutex to protect rdtgroup access. */
>> @@ -958,6 +958,35 @@ static __init void rdt_init_res_defs(void)
>>  
>>  static enum cpuhp_state rdt_online;
>>  
>> +/* Runs once on the BSP during boot. */
>> +void resctrl_cpu_detect(struct cpuinfo_x86 *c)
>> +{
>> +	if (!cpu_has(c, X86_FEATURE_CQM_LLC)) {
>> +		c->x86_cache_max_rmid  = -1;
>> +		c->x86_cache_occ_scale = -1;
>> +		c->x86_cache_mbm_width_offset = -1;
>> +		return;
>> +	}
>> +
>> +	/* will be overridden if occupancy monitoring exists */
>> +	c->x86_cache_max_rmid = cpuid_ebx(0xf);
>> +
>> +	if (cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC) ||
>> +	    cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL) ||
>> +	    cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)) {
>> +		u32 eax, ebx, ecx, edx;
>> +
>> +		/* QoS sub-leaf, EAX=0Fh, ECX=1 */
>> +		cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
>> +
>> +		c->x86_cache_max_rmid  = ecx;
>> +		c->x86_cache_occ_scale = ebx;
>> +
>> +		if (c->x86_vendor == X86_VENDOR_INTEL)
>> +			c->x86_cache_mbm_width_offset = eax & 0xff;
>> +	}
>> +}
>> +
> 
> resctrl_cpu_detect() is now identical among vendors. Do we still need
> the c_bsp_init helpers? Could we not perhaps call resctrl_cpu_detect()
> directly from early_identify_cpu()?
> 

I should have given this more thought ... even though the function will
be identical between the two vendors it does contain vendor-specific
code, thus needing to be called from the c_bsp_init helpers? I will
resubmit the new series that intends to follow all your suggestions shortly.

Thank you very much for your feedback

Reinette