Hi Reinette, On 7/12/24 17:13, Reinette Chatre wrote: > Hi Babu, > > On 7/3/24 2:48 PM, Babu Moger wrote: >> The ABMC feature provides an option to the user to assign a hardware >> counter to an RMID and monitor the bandwidth as long as the counter >> is assigned. The bandwidth events will be tracked by the hardware until >> the user changes the configuration. Each resctrl group can configure >> maximum two counters, one for total event and one for local event. >> >> The counters are configured by writing to MSR L3_QOS_ABMC_CFG. >> Configuration is done by setting the counter id, bandwidth source (RMID) >> and bandwidth configuration supported by BMEC(Bandwidth Monitoring Event >> Configuration). Reading L3_QOS_ABMC_DSC returns the configuration of the >> counter id specified in L3_QOS_ABMC_CFG. >> >> Attempts to read or write these MSRs when ABMC is not enabled will result >> in a #GP(0) exception. >> >> Introduce data structures and definitions for ABMC assignments. >> >> MSR L3_QOS_ABMC_CFG (0xC000_03FDh) and L3_QOS_ABMC_DSC (0xC000_03FEh) >> details. >> ========================================================================= >> Bits Mnemonic Description Access Reset >> Type Value >> ========================================================================= >> 63 CfgEn Configuration Enable R/W 0 >> >> 62 CtrEn Enable/disable Tracking R/W 0 >> >> 61:53 – Reserved MBZ 0 >> >> 52:48 CtrID Counter Identifier R/W 0 >> >> 47 IsCOS BwSrc field is a CLOSID R/W 0 >> (not an RMID) >> >> 46:44 – Reserved MBZ 0 >> >> 43:32 BwSrc Bandwidth Source R/W 0 >> (RMID or CLOSID) >> >> 31:0 BwType Bandwidth configuration R/W 0 >> to track for this counter >> ========================================================================== >> >> 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). > > The changelog only describes the hardware interface yet the patch contains > part hardware interface part new driver support for hardware interface. > Yes. I may have to separate this. >> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 >> --- >> v5: Moved assignment flags here (path 10/19 of v4). >> Added MON_CNTR_UNSET definition to initialize cntr_id's. >> More details in commit log. >> Renamed few fields in l3_qos_abmc_cfg for readability. >> >> v4: Added more descriptions. >> Changed the name abmc_ctr_id to ctr_id. >> Added L3_QOS_ABMC_DSC. Used for reading the configuration. >> >> v3: No changes. >> >> v2: No changes. >> --- >> arch/x86/include/asm/msr-index.h | 2 ++ >> arch/x86/kernel/cpu/resctrl/internal.h | 40 ++++++++++++++++++++++++++ >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++ >> 3 files changed, 60 insertions(+) >> >> diff --git a/arch/x86/include/asm/msr-index.h >> b/arch/x86/include/asm/msr-index.h >> index 263b2d9d00ed..5e44ff91f459 100644 >> --- a/arch/x86/include/asm/msr-index.h >> +++ b/arch/x86/include/asm/msr-index.h >> @@ -1175,6 +1175,8 @@ >> #define MSR_IA32_SMBA_BW_BASE 0xc0000280 >> #define MSR_IA32_EVT_CFG_BASE 0xc0000400 >> #define MSR_IA32_L3_QOS_EXT_CFG 0xc00003ff >> +#define MSR_IA32_L3_QOS_ABMC_CFG 0xc00003fd >> +#define MSR_IA32_L3_QOS_ABMC_DSC 0xc00003fe >> /* 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 4cb1a5d014a3..6925c947682d 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -100,6 +100,18 @@ cpumask_any_housekeeping(const struct cpumask >> *mask, int exclude_cpu) >> /* Setting bit 0 in L3_QOS_EXT_CFG enables the ABMC feature */ >> #define ABMC_ENABLE BIT(0) >> +/* >> + * Assignment flags for ABMC feature >> + */ >> +#define ASSIGN_NONE 0 >> +#define ASSIGN_TOTAL BIT(QOS_L3_MBM_TOTAL_EVENT_ID) >> +#define ASSIGN_LOCAL BIT(QOS_L3_MBM_LOCAL_EVENT_ID) > > These flags do not appear to be part of hardware interface and there > is no explanation for what they mean or how they will be used. They are > also not used in this patch. It is thus not possible to understand if > they belong in this patch or is appropriate in this work. ok. Will remove it from here. Will introduce later when it is used. > >> + >> +#define MON_CNTR_UNSET U32_MAX >> + >> +/* Maximum assignable counters per resctrl group */ >> +#define MAX_CNTRS 2 >> + >> struct rdt_fs_context { >> struct kernfs_fs_context kfc; >> bool enable_cdpl2; >> @@ -228,12 +240,14 @@ enum rdtgrp_mode { >> * @parent: parent rdtgrp >> * @crdtgrp_list: child rdtgroup node list >> * @rmid: rmid for this rdtgroup >> + * @cntr_id: ABMC counter ids assigned to this group > > struct mongroup is private to resctrl fs so it cannot contain an > architecture specific feature. Having it contain a generic "cntr_id" > may be ok at this point, but it should not be termed "ABMC counter". Ok. Sure. > >> */ >> struct mongroup { >> struct kernfs_node *mon_data_kn; >> struct rdtgroup *parent; >> struct list_head crdtgrp_list; >> u32 rmid; >> + u32 cntr_id[MAX_CNTRS]; > > This is a significant addition yet is silently included as part of a patch > that just introduces hardware interface. This is how resctrl will manage > the hardware counters. It is significant since this is what dictates that it > is resctrl fs that will manage the counters, which makes it important which > interfaces are made available and from where it is called. Through > this series I have also not come across a description of this architecture. > With this introduction counters are maintained per monitor group, yet > the new interface supports assigining counters per domain. There > is no high level explanation of this architecture and the reader is forced > to decipher it from the implementation making this work harder to review > that necessary. > > Would it be possible to present the fs and architecture code > separately? I think doing so will make it easier to understand. Sure. Will separate the two parts. > >> }; >> /** >> @@ -607,6 +621,32 @@ union cpuid_0x10_x_edx { >> unsigned int full; >> }; >> +/* >> + * ABMC counters can be configured by writing to L3_QOS_ABMC_CFG. >> + * @bw_type : Bandwidth configuration(supported by BMEC) >> + * to track this counter id. > > Does "to track this counter id" mean "tracked by @cntr_id"? Yea. Sure. > >> + * @bw_src : Bandwidth Source (RMID or CLOSID). > > Please do not capitalize words mid sentence, like "Source" > above, "Identifier", and "Enable" in two instances below. > >> + * @reserved1 : Reserved. >> + * @is_clos : BwSrc field is a CLOSID (not an RMID). > > Just stick to @bw_src. Sure. > >> + * @cntr_id : Counter Identifier. >> + * @reserved : Reserved. >> + * @cntr_en : Tracking Enable bit. > > Can this be more detailed about what happens when this bit is set/clear? Sure. Will add it. cfn_en = 1, cntr_en= 0; Counter will be be configured and tracking is not enabled. cfn_en = 1, cntr_en= 1; Counter will be be configured and tracking will be enabled. > >> + * @cfg_en : Configuration Enable bit. > > What is difference between "configuration enable" and "tracking enable"? > What is relationship, if any, to @bw_type that is the bandwidth > configuration? > >> + */ >> +union l3_qos_abmc_cfg { >> + struct { >> + unsigned long bw_type :32, >> + bw_src :12, >> + reserved1: 3, >> + is_clos : 1, >> + cntr_id : 5, >> + reserved : 9, >> + cntr_en : 1, >> + cfg_en : 1; >> + } split; > > Please check the spacing in this data structure. Tabs are used inconsistently > and the members are not lining up either. Sure. > >> + unsigned long full; >> +}; >> + >> void rdt_last_cmd_clear(void); >> void rdt_last_cmd_puts(const char *s); >> __printf(1, 2) >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index 91c5d45ac367..d2663f1345b7 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -2505,6 +2505,7 @@ static void resctrl_abmc_set_one_amd(void *arg) >> static int _resctrl_abmc_enable(struct rdt_resource *r, bool enable) >> { >> + struct rdtgroup *prgrp, *crgrp; >> struct rdt_mon_domain *d; >> /* >> @@ -2513,6 +2514,17 @@ static int _resctrl_abmc_enable(struct >> rdt_resource *r, bool enable) >> */ >> mbm_cntrs_init(); >> + /* Reset the cntr_id's for all the monitor groups */ >> + list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) { >> + prgrp->mon.cntr_id[0] = MON_CNTR_UNSET; >> + prgrp->mon.cntr_id[1] = MON_CNTR_UNSET; >> + list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, >> + mon.crdtgrp_list) { >> + crgrp->mon.cntr_id[0] = MON_CNTR_UNSET; >> + crgrp->mon.cntr_id[1] = MON_CNTR_UNSET; >> + } >> + } >> + > > No. The counters are in the monitor group that is a structure that is private > to the fs. The architecture code should not be accessing it. This should > only be > done by fs code. Will move this code to FS part before coming here. > >> /* >> * Hardware counters will reset after switching the monitor mode. >> * Reset the architectural state so that reading of hardware >> @@ -3573,6 +3585,8 @@ static int mkdir_rdt_prepare_rmid_alloc(struct >> rdtgroup *rdtgrp) >> return ret; >> } >> rdtgrp->mon.rmid = ret; >> + rdtgrp->mon.cntr_id[0] = MON_CNTR_UNSET; >> + rdtgrp->mon.cntr_id[1] = MON_CNTR_UNSET; >> ret = mkdir_mondata_all(rdtgrp->kn, rdtgrp, >> &rdtgrp->mon.mon_data_kn); >> if (ret) { >> @@ -4128,6 +4142,10 @@ static void __init rdtgroup_setup_default(void) >> rdtgroup_default.closid = RESCTRL_RESERVED_CLOSID; >> rdtgroup_default.mon.rmid = RESCTRL_RESERVED_RMID; >> rdtgroup_default.type = RDTCTRL_GROUP; >> + >> + rdtgroup_default.mon.cntr_id[0] = MON_CNTR_UNSET; >> + rdtgroup_default.mon.cntr_id[1] = MON_CNTR_UNSET; >> + >> INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list); >> list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups); > > Reinette > -- Thanks Babu Moger
