Hi Reinette, On 5/22/25 15:56, Reinette Chatre wrote: > Hi Babu, > > On 5/15/25 3:51 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. >> >> 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> >> --- > > ... > >> --- >> arch/x86/include/asm/msr-index.h | 1 + >> arch/x86/kernel/cpu/resctrl/internal.h | 5 +++ >> arch/x86/kernel/cpu/resctrl/monitor.c | 43 ++++++++++++++++++++++++++ >> include/linux/resctrl.h | 3 ++ >> 4 files changed, 52 insertions(+) >> >> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h >> index e6134ef2263d..3970e0b16e47 100644 >> --- a/arch/x86/include/asm/msr-index.h >> +++ b/arch/x86/include/asm/msr-index.h >> @@ -1203,6 +1203,7 @@ >> /* - AMD: */ >> #define MSR_IA32_MBA_BW_BASE 0xc0000200 >> #define MSR_IA32_SMBA_BW_BASE 0xc0000280 >> +#define MSR_IA32_L3_QOS_EXT_CFG 0xc00003ff >> #define MSR_IA32_EVT_CFG_BASE 0xc0000400 >> >> /* AMD-V MSRs */ >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h >> index 5e3c41b36437..fcc9d23686a1 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -37,6 +37,9 @@ struct arch_mbm_state { >> u64 prev_msr; >> }; >> >> +/* Setting bit 0 in L3_QOS_EXT_CFG enables the ABMC feature. */ >> +#define ABMC_ENABLE_BIT 0 >> + >> /** >> * struct rdt_hw_ctrl_domain - Arch private attributes of a set of CPUs that share >> * a resource for a control function >> @@ -102,6 +105,7 @@ struct msr_param { >> * @mon_scale: cqm counter * mon_scale = occupancy in bytes >> * @mbm_width: Monitor width, to detect and correct for overflow. >> * @cdp_enabled: CDP state of this resource >> + * @mbm_cntr_assign_enabled: ABMC feature is enabled >> * >> * Members of this structure are either private to the architecture >> * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g. >> @@ -115,6 +119,7 @@ struct rdt_hw_resource { >> unsigned int mon_scale; >> unsigned int mbm_width; >> bool cdp_enabled; >> + bool mbm_cntr_assign_enabled; >> }; >> >> static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r) >> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c >> index fd2761d9f3f7..ff4b2abfa044 100644 >> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >> @@ -405,3 +405,46 @@ 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); >> +} >> + >> +/* >> + * ABMC enable/disable requires update of L3_QOS_EXT_CFG MSR on all the CPUs >> + * associated with all monitor domains. >> + */ >> +static void _resctrl_abmc_enable(struct rdt_resource *r, bool enable) >> +{ >> + struct rdt_mon_domain *d; >> + > > It remains a challenge to consider these building blocks without insight into > how/when they will be used. To help out, please add guardrails to help with review. > For example, this could benefit from a: > > lockdep_assert_cpus_held(); Yes. Sure. > >> + 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); >> + resctrl_arch_reset_rmid_all(r, d); >> + } >> +} >> + >> +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; >> + } >> + >> + return 0; >> +} >> + >> +inline bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r) > > This "inline" in the .c file is unexpected. Why is this needed? Not required. No specific reason. Will remove it. > >> +{ >> + return resctrl_to_arch_res(r)->mbm_cntr_assign_enabled; >> +} >> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h >> index 065fb6e38933..bdb264875ef6 100644 >> --- a/include/linux/resctrl.h >> +++ b/include/linux/resctrl.h >> @@ -428,6 +428,9 @@ static inline u32 resctrl_get_config_index(u32 closid, >> bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l); >> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable); >> >> +bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r); >> +int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable); >> + >> /* >> * Update the ctrl_val and apply this config right now. >> * Must be called on one of the domain's CPUs. > > Reinette > > -- Thanks Babu Moger
