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

[Moger, Babu]

Hi Reinette,

On 6/24/25 16:33, Reinette Chatre wrote:
> 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. 

Sure. Added the above line. Removed the "The current expectation" line.

> 
> 
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
> 
> ...
> 
>> ---
>>  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:

Looks good to me.

diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index dcc6c00eb362..7e816341da6a 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -924,6 +924,11 @@ int resctrl_mon_resource_init(void)
        else if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID))
                mba_mbps_default_event = QOS_L3_MBM_TOTAL_EVENT_ID;

+       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);
+       }
+
        return 0;
 }


> 
> 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?

Yes. The CPUID.(EAX=0xF, ECX=1) is kind of building block. I would think
it will always be supported.

Added this check now.

diff --git a/arch/x86/kernel/cpu/resctrl/core.c
b/arch/x86/kernel/cpu/resctrl/core.c
index 22a414802cbb..e9a8c4778d22 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -988,7 +988,8 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c)

        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)) {
+           cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL) ||
+           cpu_has(c, X86_FEATURE_ABMC)) {
                u32 eax, ebx, ecx, edx;

                /* QoS sub-leaf, EAX=0Fh, ECX=1 */


-- 
Thanks
Babu Moger