Hi Reinette, On 5/3/24 18:30, Reinette Chatre wrote: > Hi Babu, > > On 3/28/2024 6:06 PM, Babu Moger wrote: >> Add the functionality to enable/disable ABMC feature. >> >> ABMC is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the >> state of ABMC is changed, it must be changed to the updated value on all >> logical processors in the QOS Domain. > > This patch does much more than enable what is mentioned above. There is little > information about what this patch aims to accomplish. Without this it makes > review difficult. Sure. Also I need to add details about why resctrl_arch_reset_rmid_all() is required. Will do. > >> >> 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 >> --- >> 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 | 12 ++++ >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 76 +++++++++++++++++++++++++- >> 3 files changed, 88 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h >> index 05956bd8bacf..f16ee50b1a23 100644 >> --- a/arch/x86/include/asm/msr-index.h >> +++ b/arch/x86/include/asm/msr-index.h >> @@ -1165,6 +1165,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 722388621403..8238ee437369 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu) >> return cpu; >> } >> >> +/* ABMC ENABLE */ > > Can this comment be made more useful? How about? /* Setting bit 0 in L3_QOS_EXT_CFG enables ABMC features */ Or I can remove it totally. > >> +#define ABMC_ENABLE BIT(0) >> + >> struct rdt_fs_context { >> struct kernfs_fs_context kfc; >> bool enable_cdpl2; >> @@ -433,6 +436,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. >> @@ -448,6 +452,7 @@ struct rdt_hw_resource { >> unsigned int mbm_width; >> unsigned int mbm_cfg_mask; >> bool cdp_enabled; >> + bool abmc_enabled; >> }; >> >> static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r) >> @@ -491,6 +496,13 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l) >> >> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable); >> >> +static inline bool resctrl_arch_get_abmc_enabled(enum resctrl_res_level l) >> +{ >> + return rdt_resources_all[l].abmc_enabled; >> +} >> + >> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable); >> + >> /* >> * 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 05f551bc316e..f49073c86884 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -850,9 +850,15 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of, >> struct seq_file *s, void *v) >> { >> struct rdt_resource *r = of->kn->parent->priv; >> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); >> >> - if (r->mbm_assign_capable) >> + if (r->mbm_assign_capable && hw_res->abmc_enabled) { >> + seq_puts(s, "[abmc]\n"); >> + seq_puts(s, "legacy_mbm\n"); >> + } else if (r->mbm_assign_capable) { >> seq_puts(s, "abmc\n"); >> + seq_puts(s, "[legacy_mbm]\n"); >> + } >> >> return 0; >> } >> @@ -2433,6 +2439,74 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable) >> return 0; >> } >> >> +static void resctrl_abmc_msrwrite(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); >> +} >> + >> +static int resctrl_abmc_setup(enum resctrl_res_level l, bool enable) >> +{ >> + struct rdt_resource *r = &rdt_resources_all[l].r_resctrl; >> + struct rdt_domain *d; >> + >> + /* Update QOS_CFG MSR on all the CPUs in cpu_mask */ > > "all the CPUs in cpu_mask" -> "all the CPUs associated with the resource"? Sure. > >> + list_for_each_entry(d, &r->domains, list) { >> + on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1); >> + resctrl_arch_reset_rmid_all(r, d); > > Could the changelog please explain why this is needed and what the impact of > this is? Sure. > >> + } >> + >> + return 0; >> +} > > I think the naming can be changed to make these easier to understand. For example, > resctrl_abmc_msrwrite() -> resctrl_abmc_set_one() > resctrl_abmc_setup() -> resctrl_abmc_set_all() Sure. > >> + >> +static int resctrl_abmc_enable(enum resctrl_res_level l) >> +{ >> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l]; >> + int ret = 0; >> + >> + if (!hw_res->abmc_enabled) { >> + ret = resctrl_abmc_setup(l, true); >> + if (!ret) >> + hw_res->abmc_enabled = true; >> + } >> + >> + return ret; >> +} >> + >> +static void resctrl_abmc_disable(enum resctrl_res_level l) >> +{ >> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l]; >> + >> + if (hw_res->abmc_enabled) { >> + resctrl_abmc_setup(l, false); >> + hw_res->abmc_enabled = false; >> + } >> +} >> + >> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable) >> +{ >> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l]; >> + >> + if (!hw_res->r_resctrl.mbm_assign_capable) >> + return -EINVAL; >> + >> + if (enable) >> + return resctrl_abmc_enable(l); >> + >> + resctrl_abmc_disable(l); >> + >> + return 0; >> +} > > Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary > layer of abstraction. > I feel it is better to keep it that way. It is consistent with definition of resctrl_arch_set_cdp_enabled. It handles both enable and disable. Otherwise we have add those checks from the caller. -- Thanks Babu Moger
