Re: [PATCH v13 01/27] x86/cpufeatures: Add support for Assignable Bandwidth Monitoring Counters (ABMC)

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

Hi Babu,

On 5/27/25 10:23 AM, Moger, Babu wrote:
> On 5/22/25 15:51, Reinette Chatre wrote:
>> On 5/15/25 3:51 PM, Babu Moger wrote:

>>> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
>>> index a2fbea0be535..2f54831e04e5 100644
>>> --- a/arch/x86/kernel/cpu/cpuid-deps.c
>>> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
>>> @@ -71,6 +71,8 @@ static const struct cpuid_dep cpuid_deps[] = {
>>>  	{ X86_FEATURE_CQM_MBM_LOCAL,		X86_FEATURE_CQM_LLC   },
>>>  	{ X86_FEATURE_BMEC,			X86_FEATURE_CQM_MBM_TOTAL   },
>>>  	{ X86_FEATURE_BMEC,			X86_FEATURE_CQM_MBM_LOCAL   },
>>> +	{ X86_FEATURE_ABMC,			X86_FEATURE_CQM_MBM_TOTAL   },
>>> +	{ X86_FEATURE_ABMC,			X86_FEATURE_CQM_MBM_LOCAL   },
>>
>> Is this dependency still accurate now that the implementation switched to the 
>> "extended event ID" variant of ABMC that no longer uses the event IDs associated
>> with X86_FEATURE_CQM_MBM_TOTAL and X86_FEATURE_CQM_MBM_LOCAL?
> 
> That's a good question. Unfortunately, we may need to retain this
> dependency for now, as a significant portion of the code relies on
> functions like resctrl_is_mbm_event(), resctrl_is_mbm_enabled(),
> resctrl_arch_is_mbm_total_enabled(), and others.
> 

Avoiding needing to change code is not a valid reason. 

I think that without this dependency the code will
still rely on "functions like resctrl_is_mbm_event(),
resctrl_is_mbm_enabled(), resctrl_arch_is_mbm_total_enabled(),
and others." though.

The core shift is to stop thinking about QOS_L3_MBM_TOTAL_EVENT_ID
to mean the same as X86_FEATURE_CQM_MBM_TOTAL, similarly to stop
thinking about QOS_L3_MBM_LOCAL_EVENT_ID to mean the same as
X86_FEATURE_CQM_MBM_LOCAL.

I expected that for backwards compatibility ABMC will start by
enabling QOS_L3_MBM_TOTAL_EVENT_ID and QOS_L3_MBM_LOCAL_EVENT_ID 
as part of its initialization, configuring them with the current
defaults for which memory transactions are expected to be monitored
by each. With these events enabled the existing flows using, for
example, resctrl_is_mbm_event(), will continue to work as expected, no?

This would require more familiarity with L3 monitoring enumeration
on AMD since it will still be required to determine the number of
RMIDs etc. but if ABMC does not actually depend on these CQM features
then the current enumeration would need to be re-worked anyway.

Reinette