| 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
| ||
|
Message-ID: <892693a7-1a1b-46e4-a989-052a19566231@amd.com>
Date: Tue, 16 Jul 2024 13:48:02 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.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 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@....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?
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
Powered by blists - more mailing lists