Hi Reinette, On 7/16/24 12:51, Reinette Chatre wrote: > Hi Babu, > > On 7/16/24 8:13 AM, Moger, Babu wrote: >> On 7/12/24 17:05, Reinette Chatre wrote: >>> On 7/3/24 2:48 PM, Babu Moger wrote: >>>> Add the functionality to enable/disable AMD ABMC feature. >>>> >>>> AMD ABMC feature is enabled by setting enabled bit(0) in MSR >>>> L3_QOS_EXT_CFG. When the state of ABMC is changed, the MSR needs >>>> to be updated on all the logical processors in the QOS Domain. >>>> >>>> Hardware counters will reset when ABMC state is changed. Reset the >>>> architectural state so that reading of hardware counter is not considered >>>> as an overflow in next update. >>>> >>>> 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 >>>> --- >>>> v5: Renamed resctrl_abmc_enable to resctrl_arch_abmc_enable. >>>> Renamed resctrl_abmc_disable to resctrl_arch_abmc_disable. >>>> Introduced resctrl_arch_get_abmc_enabled to get abmc state from >>>> non-arch code. >>>> Renamed resctrl_abmc_set_all to _resctrl_abmc_enable(). >>>> Modified commit log to make it clear about AMD ABMC feature. >>>> >>>> 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 | 13 +++++ >>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 66 ++++++++++++++++++++++++++ >>>> 3 files changed, 80 insertions(+) >>>> >>>> diff --git a/arch/x86/include/asm/msr-index.h >>>> b/arch/x86/include/asm/msr-index.h >>>> index 01342963011e..263b2d9d00ed 100644 >>>> --- a/arch/x86/include/asm/msr-index.h >>>> +++ b/arch/x86/include/asm/msr-index.h >>>> @@ -1174,6 +1174,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 2bd207624eec..0ce9797f80fe 100644 >>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h >>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >>>> @@ -97,6 +97,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, >>>> int exclude_cpu) >>>> return cpu; >>>> } >>>> +/* Setting bit 0 in L3_QOS_EXT_CFG enables the ABMC feature */ >>> >>> Please be consistent throughout series to have sentences end with period. >> >> Sure. >> >>> >>>> +#define ABMC_ENABLE BIT(0) >>>> + >>>> struct rdt_fs_context { >>>> struct kernfs_fs_context kfc; >>>> bool enable_cdpl2; >>>> @@ -477,6 +480,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. >>>> @@ -491,6 +495,7 @@ struct rdt_hw_resource { >>>> unsigned int mbm_width; >>>> unsigned int mbm_cfg_mask; >>>> bool cdp_enabled; >>>> + bool abmc_enabled; >>>> }; >>> >>> mbm_cntr_enabled? This is architecture specific code so there is more >>> flexibility >>> here, but it may make implementation easier to understand if consistent >>> naming is used >>> between fs and arch code. >> >> How about "mbm_cntr_assign_enabled" or "cntr_assign_enabled" ? > > My preference is to keep the term "mbm_cntr" to be consistent with the > other variables/struct members to help when reading the code. > "mbm_cntr_assign_enabled" does seem to be getting long though. > Are you planning to use it by assigning it to a local variable with shorter > name? Yes. We can do that. > > As a sidenote, I will be offline for large portions of the next few weeks > and thus unresponsive during this time. I'll be back to a regular > schedule on August 12th. Thanks for the heads up. Thanks Babu Moger
