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@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:
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