| 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: <80b33bc6-7b70-4853-92a0-718c2d8728d3@amd.com>
Date: Mon, 6 May 2024 14:09:38 -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: [RFC PATCH v3 03/17] x86/resctrl: Detect Assignable Bandwidth
Monitoring feature details
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@....com>
>> 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
Powered by blists - more mailing lists