| 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: <74e6bc40-7d91-4175-a59f-f16dcb281a7d@intel.com>
Date: Fri, 18 Apr 2025 17:08:18 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
<Dave.Martin@....com>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>
CC: <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v3 14/26] x86/resctrl: Add first part of telemetry event
enumeration
Hi Tony,
The following three patches progress as follows:
x86/resctrl: Add first part ...
x86/resctrl: Second stage ...
x86/resctrl: Third phase ...
Could you please make the language consistent?
On 4/7/25 4:40 PM, Tony Luck wrote:
> The OOBMSM driver provides an interface to discover any RMID
> based events for "energy" and "perf" classes.
Please add context about what an RMID based event is.
>
> Hold onto references to any pmt_feature_groups that resctrl
Please add context about what a
"pmt_feature_groups" (intended to be pmt_feature_group?) is.
> uses until resctrl exit.
>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 8 ++++
> arch/x86/kernel/cpu/resctrl/core.c | 5 ++
> arch/x86/kernel/cpu/resctrl/intel_aet.c | 62 +++++++++++++++++++++++++
> arch/x86/kernel/cpu/resctrl/Makefile | 1 +
> 4 files changed, 76 insertions(+)
> create mode 100644 arch/x86/kernel/cpu/resctrl/intel_aet.c
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 45eabc7919c6..70b63bbc429d 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -172,4 +172,12 @@ void __init intel_rdt_mbm_apply_quirk(void);
>
> void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>
> +#ifdef CONFIG_INTEL_AET_RESCTRL
> +bool intel_aet_get_events(void);
> +void __exit intel_aet_exit(void);
> +#else
> +static inline bool intel_aet_get_events(void) { return false; }
> +static inline void intel_aet_exit(void) { };
> +#endif
> +
> #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index a066a9c54a1f..f0f256a5ac66 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -718,6 +718,9 @@ void resctrl_arch_mount(void)
> if (only_once)
> return;
> only_once = true;
> +
> + if (!intel_aet_get_events())
hmmm ... keep in mind that this is called without
any locking and thus there may be risk of parallel calls?
Please document how/if this is relying on some fs features to
ensure there are not two instances running at same time.
> + return;
> }
>
> enum {
> @@ -1063,6 +1066,8 @@ late_initcall(resctrl_arch_late_init);
>
> static void __exit resctrl_arch_exit(void)
> {
> + intel_aet_exit();
> +
> cpuhp_remove_state(rdt_online);
>
> resctrl_exit();
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> new file mode 100644
> index 000000000000..8e531ad279b5
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Resource Director Technology(RDT)
> + * - Intel Application Energy Telemetry
> + *
> + * Copyright (C) 2025 Intel Corporation
> + *
> + * Author:
> + * Tony Luck <tony.luck@...el.com>
> + */
> +
> +#define pr_fmt(fmt) "resctrl: " fmt
> +
> +#include <linux/cpu.h>
> +#include <linux/cleanup.h>
> +#include "fake_intel_aet_features.h"
This include can be marked as "Temporary" to highlight that it
will not stay.
Please separate headers into blocks and sort alphabetically.
> +#include <linux/intel_vsec.h>
> +#include <linux/resctrl.h>
> +#include <linux/slab.h>
Are all these headers used in code below?
> +
> +#include "internal.h"
> +
> +static struct pmt_feature_group *feat_energy;
> +static struct pmt_feature_group *feat_perf;
> +
> +DEFINE_FREE(intel_pmt_put_feature_group, struct pmt_feature_group *, \
> + if (!IS_ERR_OR_NULL(_T)) \
> + intel_pmt_put_feature_group(_T))
> +
> +/*
> + * Ask OOBMSM discovery driver for all the RMID based telemetry groups
> + * that it supports.
> + */
> +bool intel_aet_get_events(void)
> +{
> + struct pmt_feature_group *p1 __free(intel_pmt_put_feature_group) = NULL;
> + struct pmt_feature_group *p2 __free(intel_pmt_put_feature_group) = NULL;
> + bool use_p1, use_p2;
> +
> + p1 = intel_pmt_get_regions_by_feature(FEATURE_PER_RMID_ENERGY_TELEM);
> + p2 = intel_pmt_get_regions_by_feature(FEATURE_PER_RMID_PERF_TELEM);
> + use_p1 = !IS_ERR_OR_NULL(p1);
> + use_p2 = !IS_ERR_OR_NULL(p2);
> +
> + if (!use_p1 && !use_p2)
> + return false;
> +
> + if (use_p1)
> + feat_energy = no_free_ptr(p1);
> + if (use_p2)
> + feat_perf = no_free_ptr(p2);
This reminds me of something I read recently .... "There's a rule in computer
programming that objects appear zero, once, or many times. So code accordingly."
Not expecting a change .... just finding it amusing.
> +
> + return true;
> +}
> +
> +void __exit intel_aet_exit(void)
> +{
> + if (feat_energy)
> + intel_pmt_put_feature_group(feat_energy);
> + if (feat_perf)
> + intel_pmt_put_feature_group(feat_perf);
> +}
> diff --git a/arch/x86/kernel/cpu/resctrl/Makefile b/arch/x86/kernel/cpu/resctrl/Makefile
> index c56d3acf8ac7..74c3b2333dde 100644
> --- a/arch/x86/kernel/cpu/resctrl/Makefile
> +++ b/arch/x86/kernel/cpu/resctrl/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_X86_CPU_RESCTRL) += core.o rdtgroup.o monitor.o
> obj-$(CONFIG_X86_CPU_RESCTRL) += ctrlmondata.o
> +obj-$(CONFIG_INTEL_AET_RESCTRL) += intel_aet.o
> obj-$(CONFIG_INTEL_AET_RESCTRL) += fake_intel_aet_features.o
> obj-$(CONFIG_RESCTRL_FS_PSEUDO_LOCK) += pseudo_lock.o
>
Reinette
Powered by blists - more mailing lists