Re: [PATCH v14 09/32] x86/resctrl: Detect Assignable Bandwidth Monitoring feature details

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

Hi Babu,

On 6/13/25 2:04 PM, Babu Moger wrote:
> ABMC feature details are reported via CPUID Fn8000_0020_EBX_x5.
> Bits Description
> 15:0 MAX_ABMC Maximum Supported Assignable Bandwidth
>      Monitoring Counter ID + 1
> 
> The feature details are documented in APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
> Monitoring (ABMC).
> 
> Detect the feature and number of assignable counters supported. Also,
> enable QOS_L3_MBM_TOTAL_EVENT_ID and QOS_L3_MBM_LOCAL_EVENT_ID upon
> detecting the ABMC feature. The current expectation is to support
> these two events by default when ABMC is enabled.

"The current expectation ..." this need not be vague since this is what
this series does. Perhaps previous sentence can be:
"For backward compatibility, upon detecting the assignable counter feature,
enable the mbm_total_bytes and mbm_local_bytes events that users are
familiar with as part of original L3 MBM support." Although, when it comes to
this patch this may not be appropriate in that this is something that
resctrl fs should do, not the architecture. 


> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  arch/x86/kernel/cpu/resctrl/core.c    |  4 ++--
>  arch/x86/kernel/cpu/resctrl/monitor.c | 11 ++++++++---
>  include/linux/resctrl.h               |  4 ++++
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 22a414802cbb..01b210febc7d 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -873,11 +873,11 @@ static __init bool get_rdt_mon_resources(void)
>  		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID);
>  		ret = true;
>  	}
> -	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
> +	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL) || rdt_cpu_has(X86_FEATURE_ABMC)) {
>  		resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID);
>  		ret = true;
>  	}
> -	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL)) {
> +	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL) || rdt_cpu_has(X86_FEATURE_ABMC)) {
>  		resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID);
>  		ret = true;

This backward compatibility needs to be managed by resctrl fs, no? What do you think of
instead doing:

int resctrl_mon_resource_init(void) {

	...
	if (r->mon.mbm_cntr_assignable) {
		resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID);
		resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID);
	}
	...
}
		
There is another dependency that does not seem to be handled ... ABMC requires
properties enumerated in resctrl_cpu_detect(), but that enumeration is only
done if legacy monitoring features are supported, not ABMC. Does AMD support
enumeration CPUID.(EAX=0xF, ECX=1) if ABMC is supported but not the legacy MBM
total and local?


Reinette