Hi Reinette,
On 2/5/2025 4:49 PM, Reinette Chatre wrote:
Hi Babu,
On 1/22/25 12:20 PM, Babu Moger wrote:
Add the functionality to enable/disable AMD ABMC feature.
AMD ABMC feature is enabled by setting enabled bit(0) in MSR
L3_QOS_EXT_CFG. When the state of ABMC is changed, the MSR needs
to be updated on all the logical processors in the QOS Domain.
Hardware counters will reset when ABMC state is changed.
I find that the state management in this series is organized better
and easier to understand. I do think that it can be simplified more
and a hint to this is that it is mentioned here but not done in the
code introduced here but instead required from the caller. It seems
simpler to me that the architectural state can just be reset at the
same time as enable/disable of ABMC?
Right now, it is done from mbm_cntr_reset(). It does both arch and
non-arch state reset for all the RMIDs in all the domains. It is called
in two places.
1 rdtgroup.c resctrl_mbm_assign_mode_write -> mbm_cntr_reset();
2 rdtgroup.c rdt_kill_sb()-> mbm_cntr_reset();
I will have to introduce another function to reset RMIDs in all the
domains. Also, make sure it is called from both these places.
list_for_each_entry(dom, &r->mon_domains, hdr.list)
resctrl_arch_reset_rmid_all(r, dom);
I feel current code is much more cleaner. What do you think?
The ABMC 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).
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
...
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index c3d7d4c3009a..a7526306f5e4 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -1261,3 +1261,39 @@ void __init intel_rdt_mbm_apply_quirk(void)
mbm_cf_rmidthreshold = mbm_cf_table[cf_index].rmidthreshold;
mbm_cf = mbm_cf_table[cf_index].cf;
}
+
+static void resctrl_abmc_set_one_amd(void *arg)
+{
+ bool *enable = arg;
+
+ if (*enable)
+ msr_set_bit(MSR_IA32_L3_QOS_EXT_CFG, ABMC_ENABLE_BIT);
+ else
+ msr_clear_bit(MSR_IA32_L3_QOS_EXT_CFG, ABMC_ENABLE_BIT);
+}
+
+/*
+ * Update L3_QOS_EXT_CFG MSR on all the CPUs associated with the monitor
+ * domain.
All monitor domains are impacted and above does not clearly state "why".
How about
* ABMC enable/disable requires update of L3_QOS_EXT_CFG MSR on all the CPUs
* associated with all monitor domains.
Sure.
+ */
+static void _resctrl_abmc_enable(struct rdt_resource *r, bool enable)
+{
+ struct rdt_mon_domain *d;
+
+ list_for_each_entry(d, &r->mon_domains, hdr.list)
+ on_each_cpu_mask(&d->hdr.cpu_mask,
+ resctrl_abmc_set_one_amd, &enable, 1);
+}
+
+int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
+{
+ struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
+
+ if (r->mon.mbm_cntr_assignable &&
+ hw_res->mbm_cntr_assign_enabled != enable) {
+ _resctrl_abmc_enable(r, enable);
+ hw_res->mbm_cntr_assign_enabled = enable;
Added benefit of resetting architectural state within this if statement
(perhaps simpler to be done within _resctrl_abmc_enable()) is that it will
not be done unnecessarily if ABMC is already in requested state.
It will be
list_for_each_entry(dom, &r->mon_domains, hdr.list)
resctrl_arch_reset_rmid_all(r, dom);
+ }
+
+ return 0;
+}
Reinette
Thanks
Babu
