Re: [PATCH v2 3/7] x86/resctrl: Detect Smart Data Cache Injection Allocation Enforcement

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

Sorry.. Was on a vacation.  Addressing the comments now.

On 12/23/24 15:13, Reinette Chatre wrote:
> Hi Babu,
> 
> On 12/18/24 1:37 PM, Babu Moger wrote:
>> Introduce io_alloc_capable in struct resctrl_cache to detect SDCIAE
>> (L3 Smart Data Cache Injection Allocation Enforcement) feature.
> 
> Please distinguish clearly between the resctrl feature ("io_alloc_capable")
> and the architecture specific feature (SDCIAE) that backs it. This is similar
> to what you have done for ABMC and makes the work much easier to understand.
> When the resctrl and arch feature is used interchangeably it becomes confusing.

Sure. Will rewrite the commit text.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v2: Changed sdciae_capable to io_alloc_capable to make it generic feature.
>>     Also moved the io_alloc_capable in struct resctrl_cache.
>> ---
>>  arch/x86/kernel/cpu/resctrl/core.c | 7 +++++++
>>  include/linux/resctrl.h            | 4 ++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index c2450cd52511..39e110033d96 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -306,6 +306,11 @@ static void rdt_get_cdp_config(int level)
>>  	rdt_resources_all[level].r_resctrl.cdp_capable = true;
>>  }
>>  
>> +static void rdt_get_sdciae_alloc_cfg(struct rdt_resource *r)
>> +{
>> +	r->cache.io_alloc_capable = true;
>> +}
> 
> rdt_get_sdciae_alloc_cfg() looks like one that should have "set" in its name, not "get".
> This also does not seem architecture specific so "sdciae" should not be in the name.
> rdt_set_io_alloc_capable()?

Sure.

> 
>> +
>>  static void rdt_get_cdp_l3_config(void)
>>  {
>>  	rdt_get_cdp_config(RDT_RESOURCE_L3);
>> @@ -931,6 +936,8 @@ static __init bool get_rdt_alloc_resources(void)
>>  		rdt_get_cache_alloc_cfg(1, r);
>>  		if (rdt_cpu_has(X86_FEATURE_CDP_L3))
>>  			rdt_get_cdp_l3_config();
>> +		if (rdt_cpu_has(X86_FEATURE_SDCIAE))
>> +			rdt_get_sdciae_alloc_cfg(r);
>>  		ret = true;
>>  	}
>>  	if (rdt_cpu_has(X86_FEATURE_CAT_L2)) {
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index d94abba1c716..5837acff7442 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -129,6 +129,8 @@ struct rdt_mon_domain {
>>   * @arch_has_sparse_bitmasks:	True if a bitmask like f00f is valid.
>>   * @arch_has_per_cpu_cfg:	True if QOS_CFG register for this cache
>>   *				level has CPU scope.
>> + * @io_alloc_capable:	Smart Data Cache Injection Allocation Enforcement
>> + *			capable (SDCIAE).
> 
> Please remove arch specific text here. For example,
> "True if portion of the L3 cache can be allocated for I/O traffic."

Sure.
> 
>>   */
>>  struct resctrl_cache {
>>  	unsigned int	cbm_len;
>> @@ -136,6 +138,7 @@ struct resctrl_cache {
>>  	unsigned int	shareable_bits;
>>  	bool		arch_has_sparse_bitmasks;
>>  	bool		arch_has_per_cpu_cfg;
>> +	bool		io_alloc_capable;
>>  };
>>  
>>  /**
>> @@ -202,6 +205,7 @@ enum resctrl_scope {
>>   * @evt_list:		List of monitoring events
>>   * @fflags:		flags to choose base and info files
>>   * @cdp_capable:	Is the CDP feature available on this resource
>> + * @sdciae_capable:	Is SDCIAE feature available on this resource
>>   */
>>  struct rdt_resource {
>>  	int			rid;
> 
> Leftover from previous version?

Yes. Will remove it.

Thanks
Babu Moger