Hi Babu, On 3/28/2024 6:06 PM, Babu Moger wrote: > ABMC feature details are reported via CPUID Fn8000_0020_EBX_x5. > Bits Description > 15:0 MAX_ABMC Maximum Supported Assignable Bandwidth > Monitoring Counter ID + 1 > > The 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: Removed changes related to mon_features. > Moved rdt_cpu_has to core.c and added new function resctrl_arch_has_abmc. > Also moved the fields mbm_assign_capable and mbm_assign_cntrs to > rdt_resource. (James) > > v2: Changed the field name to mbm_assign_capable from abmc_capable. > --- > arch/x86/kernel/cpu/resctrl/core.c | 17 +++++++++++++++++ > arch/x86/kernel/cpu/resctrl/internal.h | 1 + > arch/x86/kernel/cpu/resctrl/monitor.c | 3 +++ > include/linux/resctrl.h | 12 ++++++++++++ > 4 files changed, 33 insertions(+) > > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c > index 57a8c6f30dd6..bb82b392cf5d 100644 > --- a/arch/x86/kernel/cpu/resctrl/core.c > +++ b/arch/x86/kernel/cpu/resctrl/core.c > @@ -740,6 +740,23 @@ bool __init rdt_cpu_has(int flag) > return ret; > } > > +inline bool __init resctrl_arch_has_abmc(struct rdt_resource *r) > +{ > + bool ret = rdt_cpu_has(X86_FEATURE_ABMC); > + u32 eax, ebx, ecx, edx; > + > + if (ret) { > + /* > + * Query CPUID_Fn80000020_EBX_x05 for number of > + * ABMC counters > + */ > + cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx); > + r->mbm_assign_cntrs = (ebx & 0xFFFF) + 1; > + } > + > + return ret; > +} It is not clear to me why this function is needed. I went back to read James' comment and it sounds to me as though he expected it to be called from non-arch code ... but this is only called from rdt_get_mon_l3_config() which is very much architecture specific and will remain in arch/x86 where rdt_cpu_has() will be accessible. > + > static __init bool get_mem_config(void) > { > struct rdt_hw_resource *hw_res = &rdt_resources_all[RDT_RESOURCE_MBA]; > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index c99f26ebe7a6..c4ae6f3993aa 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -584,6 +584,7 @@ void free_rmid(u32 closid, u32 rmid); > int rdt_get_mon_l3_config(struct rdt_resource *r); > void __exit rdt_put_mon_l3_config(void); > bool __init rdt_cpu_has(int flag); > +bool __init resctrl_arch_has_abmc(struct rdt_resource *r); > void mon_event_count(void *info); > int rdtgroup_mondata_show(struct seq_file *m, void *arg); > void mon_event_read(struct rmid_read *rr, struct rdt_resource *r, > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index c34a35ec0f03..e5938bf53d5a 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -1055,6 +1055,9 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r) > mbm_local_event.configurable = true; > mbm_config_rftype_init("mbm_local_bytes_config"); > } > + > + if (resctrl_arch_has_abmc(r)) > + r->mbm_assign_capable = ABMC_ASSIGN; > } This is confusing to me in two ways: (a) why need different layers of abstraction to initialize r->mbm_assign_capable and r->mbm_assign_cntrs? Can they not just be assigned at the same time? (b) r->mbm_assign_capable is a bool ... but it is assigned an enum? Why is this enum needed for this? > > l3_mon_evt_init(r); > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h > index a365f67131ec..a1ee9afabff3 100644 > --- a/include/linux/resctrl.h > +++ b/include/linux/resctrl.h > @@ -150,6 +150,14 @@ struct resctrl_membw { > struct rdt_parse_data; > struct resctrl_schema; > > +/** > + * enum mbm_assign_type - The type of assignable monitoring. > + * @ABMC_ASSIGN: Assignable Bandwidth Monitoring Counters. > + */ > +enum mbm_assign_type { > + ABMC_ASSIGN = 0x01, > +}; > + Either the resource is mbm_assign_capable or not ... it is not clear to me why an enum is needed. > /** > * struct rdt_resource - attributes of a resctrl resource > * @rid: The index of the resource > @@ -168,6 +176,8 @@ struct resctrl_schema; > * @evt_list: List of monitoring events > * @fflags: flags to choose base and info files > * @cdp_capable: Is the CDP feature available on this resource > + * @mbm_assign_capable: Does system capable of supporting monitor assignment? "Does system capable" -> "Is system capable"? > + * @mbm_assign_cntrs: Maximum number of assignable counters > */ > struct rdt_resource { > int rid; > @@ -188,6 +198,8 @@ struct rdt_resource { > struct list_head evt_list; > unsigned long fflags; > bool cdp_capable; > + bool mbm_assign_capable; > + u32 mbm_assign_cntrs; > }; Please check tabs vs spaces (in this whole series please). I'm thinking that a new "MBM specific" struct within struct rdt_resource will be helpful to clearly separate the MBM related data. This will be similar to struct resctrl_cache for cache allocation and struct resctrl_membw for memory bandwidth allocation. > > /** Reinette
