linux-kernel - Re: [PATCH v5 06/20] x86/resctrl: Add support to enable/disable AMD ABMC feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5e9d6516-abe7-405b-8db9-52c381559c58@intel.com>
Date: Tue, 16 Jul 2024 10:51:38 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: <babu.moger@....com>, <corbet@....net>, <fenghua.yu@...el.com>,
	<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
	<rdunlap@...radead.org>, <tj@...nel.org>, <peterz@...radead.org>,
	<yanjiewtw@...il.com>, <kim.phillips@....com>, <lukas.bulwahn@...il.com>,
	<seanjc@...gle.com>, <jmattson@...gle.com>, <leitao@...ian.org>,
	<jpoimboe@...nel.org>, <rick.p.edgecombe@...el.com>,
	<kirill.shutemov@...ux.intel.com>, <jithu.joseph@...el.com>,
	<kai.huang@...el.com>, <kan.liang@...ux.intel.com>,
	<daniel.sneddon@...ux.intel.com>, <pbonzini@...hat.com>,
	<sandipan.das@....com>, <ilpo.jarvinen@...ux.intel.com>,
	<peternewman@...gle.com>, <maciej.wieczor-retman@...el.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<eranian@...gle.com>, <james.morse@....com>
Subject: Re: [PATCH v5 06/20] x86/resctrl: Add support to enable/disable AMD
 ABMC feature

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@....com>
>>> 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?

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.

Reinette