Hi Reinette, On 4/11/25 16:02, Reinette Chatre wrote: > Hi Babu, > > On 4/3/25 5:18 PM, Babu Moger wrote: >> The ABMC feature provides an option to the user to assign a hardware >> counter to an RMID, event pair and monitor the bandwidth as long as it >> is assigned. The assigned RMID will be tracked by the hardware until the >> user unassigns it manually. >> >> Implement an architecture-specific handler to assign and unassign the >> counter. Configure counters by writing to the L3_QOS_ABMC_CFG MSR, >> specifying the counter ID, bandwidth source (RMID), and event >> configuration. >> >> The feature details are documented in the 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/kernel/cpu/resctrl/monitor.c | 39 +++++++++++++++++++++++++++ >> include/linux/resctrl.h | 15 +++++++++++ >> 2 files changed, 54 insertions(+) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c >> index 8a88ac29d57d..77f8662dc50b 100644 >> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >> @@ -1430,3 +1430,42 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable) >> >> return 0; >> } >> + >> +static void resctrl_abmc_config_one_amd(void *info) >> +{ >> + union l3_qos_abmc_cfg *abmc_cfg = info; >> + >> + wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg->full); >> +} >> + >> +/* >> + * Send an IPI to the domain to assign the counter to RMID, event pair. >> + */ >> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, >> + enum resctrl_event_id evtid, u32 rmid, u32 closid, >> + u32 cntr_id, u32 evt_cfg, bool assign) >> +{ >> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d); >> + union l3_qos_abmc_cfg abmc_cfg = { 0 }; >> + struct arch_mbm_state *am; >> + >> + abmc_cfg.split.cfg_en = 1; >> + abmc_cfg.split.cntr_en = assign ? 1 : 0; >> + abmc_cfg.split.cntr_id = cntr_id; >> + abmc_cfg.split.bw_src = rmid; >> + abmc_cfg.split.bw_type = evt_cfg; >> + >> + smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &abmc_cfg, 1); >> + >> + /* >> + * Reset the architectural state so that reading of hardware >> + * counter is not considered as an overflow in next update. > > Please add something like: "The hardware counter is reset (because cfg_en == 1) > so there is no need to record initial non-zero counts." Sure. > >> + */ >> + if (assign) { >> + am = get_arch_mbm_state(hw_dom, rmid, evtid); >> + if (am) >> + memset(am, 0, sizeof(*am)); >> + } >> + >> + return 0; >> +} >> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h >> index 294b15de664e..60270606f1b8 100644 >> --- a/include/linux/resctrl.h >> +++ b/include/linux/resctrl.h > > Please keep the declaration internal to the arch code. It can be moved when > other architecture needs it. Sure. > >> @@ -394,6 +394,21 @@ void resctrl_arch_mon_event_config_set(void *config_info); >> u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d, >> enum resctrl_event_id eventid); >> >> +/** >> + * resctrl_arch_config_cntr() - Configure the counter on the domain >> + * @r: resource that the counter should be read from. >> + * @d: domain that the counter should be read from. >> + * @evtid: event type to assign >> + * @rmid: rmid of the counter to read. >> + * @closid: closid that matches the rmid. >> + * @cntr_id: Counter ID to configure >> + * @evt_cfg: event configuration > > "event configuration" is simply an expansion of member name and does not help to > understand what the value represents. How about? "MBM Event configuration value representing reads, writes etc.." > >> + * @assign: assign or unassign > > Please rework the kernel doc: consistent sentence structure (starts with upper case, > ends with period), use proper capitalization for acronyms (rmid -> RMID, etc.), > make descriptions informative. Sure. > >> + */ >> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, >> + enum resctrl_event_id evtid, u32 rmid, u32 closid, >> + u32 cntr_id, u32 evt_cfg, bool assign); >> + >> /* For use by arch code to remap resctrl's smaller CDP CLOSID range */ >> static inline u32 resctrl_get_config_index(u32 closid, >> enum resctrl_conf_type type) > > This patch does not pass checkpatch.pl. > Sure. Will check again. -- Thanks Babu Moger
