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
