| 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: <ac22b96d-c4bb-42d9-bac8-22b9e032b59e@intel.com>
Date: Fri, 3 May 2024 16:26:04 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <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: [RFC PATCH v3 03/17] x86/resctrl: Detect Assignable Bandwidth
Monitoring feature details
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.
> +
> 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?
(b) r->mbm_assign_capable is a bool ... but it is assigned an enum? Why is
this enum needed for this?
>
> 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.
> /**
> * 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"?
> + * @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).
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.
>
> /**
Reinette
Powered by blists - more mailing lists