Re: [RFC PATCH v3 07/17] x86/resctrl: Add support to enable/disable ABMC feature

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 5/7/2024 12:12 PM, Moger, Babu wrote:
> On 5/3/24 18:30, Reinette Chatre wrote:
>> On 3/28/2024 6:06 PM, Babu Moger wrote:

...

>>>  /* MSR_IA32_VMX_MISC bits */
>>>  #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 722388621403..8238ee437369 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>>>  	return cpu;
>>>  }
>>>  
>>> +/* ABMC ENABLE */
>>
>> Can this comment be made more useful?
> 
> How about?
> /* Setting bit 0 in L3_QOS_EXT_CFG enables ABMC features */

Regarding "ABMC features" - are there several features connected to
"ABMC"?

> 
> Or I can remove it totally.
> 
>>

...

>>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
>>> +{
>>> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>>> +
>>> +	if (!hw_res->r_resctrl.mbm_assign_capable)
>>> +		return -EINVAL;
>>> +
>>> +	if (enable)
>>> +		return resctrl_abmc_enable(l);
>>> +
>>> +	resctrl_abmc_disable(l);
>>> +
>>> +	return 0;
>>> +}
>>
>> Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary
>> layer of abstraction.
>>
> 
> I feel it is better to keep it that way. It is consistent with definition
> of resctrl_arch_set_cdp_enabled. It handles both enable and disable.
> Otherwise we have add those checks from the caller.

Caller needs to know anyway whether to provide "true" or "false" to this function ...
caller may as well just call the appropriate _enable()/_disable() variant, no?

Reinette