Hi Reinette, On 7/12/24 17:05, Reinette Chatre wrote: > Hi Babu, > > On 7/3/24 2:48 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. Reset the >> architectural state so that reading of hardware counter is not considered >> as an overflow in next update. >> >> 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). >> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 >> --- >> v5: Renamed resctrl_abmc_enable to resctrl_arch_abmc_enable. >> Renamed resctrl_abmc_disable to resctrl_arch_abmc_disable. >> Introduced resctrl_arch_get_abmc_enabled to get abmc state from >> non-arch code. >> Renamed resctrl_abmc_set_all to _resctrl_abmc_enable(). >> Modified commit log to make it clear about AMD ABMC feature. >> >> v3: No changes. >> >> v2: Few text changes in commit message. >> --- >> arch/x86/include/asm/msr-index.h | 1 + >> arch/x86/kernel/cpu/resctrl/internal.h | 13 +++++ >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 66 ++++++++++++++++++++++++++ >> 3 files changed, 80 insertions(+) >> >> diff --git a/arch/x86/include/asm/msr-index.h >> b/arch/x86/include/asm/msr-index.h >> index 01342963011e..263b2d9d00ed 100644 >> --- a/arch/x86/include/asm/msr-index.h >> +++ b/arch/x86/include/asm/msr-index.h >> @@ -1174,6 +1174,7 @@ >> #define MSR_IA32_MBA_BW_BASE 0xc0000200 >> #define MSR_IA32_SMBA_BW_BASE 0xc0000280 >> #define MSR_IA32_EVT_CFG_BASE 0xc0000400 >> +#define MSR_IA32_L3_QOS_EXT_CFG 0xc00003ff >> /* MSR_IA32_VMX_MISC bits */ >> #define MSR_IA32_VMX_MISC_INTEL_PT (1ULL << 14) >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h >> b/arch/x86/kernel/cpu/resctrl/internal.h >> index 2bd207624eec..0ce9797f80fe 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -97,6 +97,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, >> int exclude_cpu) >> return cpu; >> } >> +/* Setting bit 0 in L3_QOS_EXT_CFG enables the ABMC feature */ > > Please be consistent throughout series to have sentences end with period. Sure. > >> +#define ABMC_ENABLE BIT(0) >> + >> struct rdt_fs_context { >> struct kernfs_fs_context kfc; >> bool enable_cdpl2; >> @@ -477,6 +480,7 @@ struct rdt_parse_data { >> * @mbm_cfg_mask: Bandwidth sources that can be tracked when Bandwidth >> * Monitoring Event Configuration (BMEC) is supported. >> * @cdp_enabled: CDP state of this resource >> + * @abmc_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. >> @@ -491,6 +495,7 @@ struct rdt_hw_resource { >> unsigned int mbm_width; >> unsigned int mbm_cfg_mask; >> bool cdp_enabled; >> + bool abmc_enabled; >> }; > > mbm_cntr_enabled? This is architecture specific code so there is more > flexibility > here, but it may make implementation easier to understand if consistent > naming is used > between fs and arch code. How about "mbm_cntr_assign_enabled" or "cntr_assign_enabled" ? > >> static inline struct rdt_hw_resource *resctrl_to_arch_res(struct >> rdt_resource *r) >> @@ -536,6 +541,14 @@ int resctrl_arch_set_cdp_enabled(enum >> resctrl_res_level l, bool enable); >> void arch_mon_domain_online(struct rdt_resource *r, struct >> rdt_mon_domain *d); >> +static inline bool resctrl_arch_get_abmc_enabled(void) >> +{ >> + return rdt_resources_all[RDT_RESOURCE_L3].abmc_enabled; >> +} >> + >> +int resctrl_arch_abmc_enable(void); >> +void resctrl_arch_abmc_disable(void); >> + >> /* >> * To return the common struct rdt_resource, which is contained in struct >> * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource. >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index 7e76f8d839fc..471fc0dbd7c3 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -2402,6 +2402,72 @@ int resctrl_arch_set_cdp_enabled(enum >> resctrl_res_level l, bool enable) >> return 0; >> } >> +/* >> + * Update L3_QOS_EXT_CFG MSR on all the CPUs associated with the resource. >> + */ >> +static void resctrl_abmc_set_one_amd(void *arg) >> +{ >> + bool *enable = arg; >> + u64 msrval; >> + >> + rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval); >> + >> + if (*enable) >> + msrval |= ABMC_ENABLE; >> + else >> + msrval &= ~ABMC_ENABLE; >> + >> + wrmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval); >> +} > > msr_set_bit() and msr_clear_bit() can be used here. Sure. > >> + >> +static int _resctrl_abmc_enable(struct rdt_resource *r, bool enable) >> +{ >> + struct rdt_mon_domain *d; >> + >> + /* >> + * Hardware counters will reset after switching the monitor mode. >> + * Reset the architectural state so that reading of hardware >> + * counter is not considered as an overflow in the next update. >> + */ >> + 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); >> + } >> + >> + return 0; >> +} > > Seems like _resctrl_abmc_enable() can just return void. Sure. > >> + >> +int resctrl_arch_abmc_enable(void) > > resctrl_arch_mbm_cntr_enable()? I'll no longer point all these out. Sure. > >> +{ >> + struct rdt_resource *r = >> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; >> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); >> + int ret = 0; >> + >> + lockdep_assert_held(&rdtgroup_mutex); >> + >> + if (r->mon.abmc_capable && !hw_res->abmc_enabled) { >> + ret = _resctrl_abmc_enable(r, true); >> + if (!ret) >> + hw_res->abmc_enabled = true; > > The above error handling seems unnecessary. Sure. > >> + } >> + >> + return ret; > > resctrl_arch_abmc_enable() should probably keep returning an int even though > this implementation does not need it since other archs may indeed return > error. Yea. Sure. > >> +} >> + >> +void resctrl_arch_abmc_disable(void) >> +{ >> + struct rdt_resource *r = >> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; >> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); >> + >> + lockdep_assert_held(&rdtgroup_mutex); >> + >> + if (hw_res->abmc_enabled) { >> + _resctrl_abmc_enable(r, false); >> + hw_res->abmc_enabled = false; >> + } >> +} >> + >> /* >> * We don't allow rdtgroup directories to be created anywhere >> * except the root directory. Thus when looking for the rdtgroup > > Reinette > -- Thanks Babu Moger
