Hi Reinette, On 5/3/24 18:26, Reinette Chatre wrote: > 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. Yes. That is correct. I will revert it and move it to rdt_get_mon_l3_config. > >> + >> 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? Yes. we can. > (b) r->mbm_assign_capable is a bool ... but it is assigned an enum? Why is > this enum needed for this? Enum is really not required. Will correct it. > >> >> 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. This is not required. > >> /** >> * 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"? Sure. > >> + * @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). Sure. Will do. > > 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. Did you mean to move all the fields for monitoring to move to new struct? Struct resctrl_mbm { int num_rmid; bool mbm_assign_capable; u32 mbm_assign_cntrs; }: -- Thanks Babu Moger
