Each CPU collects data for telemetry events that it sends to the nearest
telemetry event aggregator either when the value of IA32_PQR_ASSOC.RMID
changes, or when a two millisecond timer expires.

The telemetry event aggregators maintain per-RMID per-event counts of the
total seen for all the CPUs. There may be more than one set of telemetry
event aggregators per package.

There are separate sets of aggregators for each type of event, but all
aggregators for a given type are symmetric keeping counts for the same
set of events for the CPUs that provide data to them.

Each telemetry event aggregator is responsible for a specific group of
events. E.g. on the Intel Clearwater Forest CPU there are two types of
aggregators. One type tracks a pair of energy related events. The other
type tracks a subset of "perf" type events.

The event counts are made available to Linux in a region of MMIO space
for each aggregator. All details about the layout of counters in each
aggregator MMIO region are described in XML files published by Intel and
made available in a GitHub repository [1].

The key to matching a specific telemetry aggregator to the XML file that
describes the MMIO layout is a 32-bit value. The Linux telemetry subsystem
refers to this as a "guid" while the XML files call it a "uniqueid".

Each XML file provides the following information:
1) Which telemetry events are included in the group.
2) The order in which the event counters appear for each RMID.
3) The value type of each event counter (integer or fixed-point).
4) The number of RMIDs supported.
5) Which additional aggregator status registers are included.
6) The total size of the MMIO region for an aggregator.

The INTEL_PMT_TELEMETRY driver enumerates support for telemetry events.
This driver provides intel_pmt_get_regions_by_feature() to list all
available telemetry event aggregators. The list includes the "guid",
the base address in MMIO space for the region where the event counters
are exposed, and the package id where the all the CPUs that report to this
aggregator are located.

Add a new Kconfig option CONFIG_X86_CPU_RESCTRL_INTEL_AET for the Intel
specific parts of telemetry code. This depends on the INTEL_PMT_TELEMETRY
and INTEL_TPMI drivers being built-in to the kernel for enumeration of
telemetry features.

Use INTEL_PMT_TELEMETRY's intel_pmt_get_regions_by_feature() with
each per-RMID telemetry feature id to obtain a private copy of
struct pmt_feature_group that contains all discovered/enumerated
telemetry aggregator data for all event groups (known and unknown
to resctrl) of that feature id. Further processing on this structure
will enable all supported events in resctrl. Return the structure to
INTEL_PMT_TELEMETRY at resctrl exit time.

Signed-off-by: Tony Luck <tony.luck@intel.com>
Link: https://github.com/intel/Intel-PMT # [1]
---
Note that checkpatch complains about this:

DEFINE_FREE(intel_pmt_put_feature_group, struct pmt_feature_group *,
		if (!IS_ERR_OR_NULL(_T))
			intel_pmt_put_feature_group(_T))
with:
CHECK: Alignment should match open parenthesis

But if the alignment is fixed, it then complains:
WARNING: Statements should start on a tabstop
---
 arch/x86/kernel/cpu/resctrl/internal.h  |   8 ++
 arch/x86/kernel/cpu/resctrl/core.c      |   5 +
 arch/x86/kernel/cpu/resctrl/intel_aet.c | 144 ++++++++++++++++++++++++
 arch/x86/Kconfig                        |  13 +++
 arch/x86/kernel/cpu/resctrl/Makefile    |   1 +
 5 files changed, 171 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 14fadcff0d2b..886261a82b81 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -217,4 +217,12 @@ void __init intel_rdt_mbm_apply_quirk(void);
 void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
 void resctrl_arch_mbm_cntr_assign_set_one(struct rdt_resource *r);
 
+#ifdef CONFIG_X86_CPU_RESCTRL_INTEL_AET
+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 __exit 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 64c6f507b7bc..9003a6344410 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -734,6 +734,9 @@ void resctrl_arch_pre_mount(void)
 
 	if (!atomic_try_cmpxchg(&only_once, &old, 1))
 		return;
+
+	if (!intel_aet_get_events())
+		return;
 }
 
 enum {
@@ -1091,6 +1094,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..966c840f0d6b
--- /dev/null
+++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
@@ -0,0 +1,144 @@
+// 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@intel.com>
+ */
+
+#define pr_fmt(fmt)   "resctrl: " fmt
+
+#include <linux/array_size.h>
+#include <linux/cleanup.h>
+#include <linux/cpu.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/intel_pmt_features.h>
+#include <linux/intel_vsec.h>
+#include <linux/overflow.h>
+#include <linux/resctrl.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+
+#include "internal.h"
+
+/**
+ * struct event_group - All information about a group of telemetry events.
+ * @pfg:		Points to the aggregated telemetry space information
+ *			returned by the intel_pmt_get_regions_by_feature()
+ *			call to the INTEL_PMT_TELEMETRY driver that contains
+ *			data for all telemetry regions of a specific type.
+ *			Valid if the system supports the event group.
+ *			NULL otherwise.
+ * @guid:		Unique number per XML description file.
+ */
+struct event_group {
+	/* Data fields for additional structures to manage this group. */
+	struct pmt_feature_group	*pfg;
+
+	/* Remaining fields initialized from XML file. */
+	u32				guid;
+};
+
+/*
+ * Link: https://github.com/intel/Intel-PMT
+ * File: xml/CWF/OOBMSM/RMID-ENERGY/cwf_aggregator.xml
+ */
+static struct event_group energy_0x26696143 = {
+	.guid		= 0x26696143,
+};
+
+/*
+ * Link: https://github.com/intel/Intel-PMT
+ * File: xml/CWF/OOBMSM/RMID-PERF/cwf_aggregator.xml
+ */
+static struct event_group perf_0x26557651 = {
+	.guid		= 0x26557651,
+};
+
+static struct event_group *known_energy_event_groups[] = {
+	&energy_0x26696143,
+};
+
+static struct event_group *known_perf_event_groups[] = {
+	&perf_0x26557651,
+};
+
+#define for_each_enabled_event_group(_peg, _grp)			\
+	for (_peg = (_grp); _peg < &_grp[ARRAY_SIZE(_grp)]; _peg++)	\
+		if ((*_peg)->pfg)
+
+/* Stub for now */
+static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
+{
+	return false;
+}
+
+DEFINE_FREE(intel_pmt_put_feature_group, struct pmt_feature_group *,
+		if (!IS_ERR_OR_NULL(_T))
+			intel_pmt_put_feature_group(_T))
+
+/*
+ * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
+ * pmt_feature_group for a specific feature. If there is one, the returned
+ * structure has an array of telemetry_region structures. Each describes
+ * one telemetry aggregator.
+ * Try to use every telemetry aggregator with a known guid.
+ */
+static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
+			    unsigned int num_evg)
+{
+	struct pmt_feature_group *p __free(intel_pmt_put_feature_group) = NULL;
+	struct event_group **peg;
+	bool ret;
+
+	p = intel_pmt_get_regions_by_feature(feature);
+
+	if (IS_ERR_OR_NULL(p))
+		return false;
+
+	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
+		ret = enable_events(*peg, p);
+		if (ret) {
+			(*peg)->pfg = no_free_ptr(p);
+			return true;
+		}
+	}
+
+	return false;
+}
+
+/*
+ * Ask INTEL_PMT_TELEMETRY driver for all the RMID based telemetry groups
+ * that it supports.
+ */
+bool intel_aet_get_events(void)
+{
+	bool ret1, ret2;
+
+	ret1 = get_pmt_feature(FEATURE_PER_RMID_ENERGY_TELEM,
+			       known_energy_event_groups,
+			       ARRAY_SIZE(known_energy_event_groups));
+	ret2 = get_pmt_feature(FEATURE_PER_RMID_PERF_TELEM,
+			       known_perf_event_groups,
+			       ARRAY_SIZE(known_perf_event_groups));
+
+	return ret1 || ret2;
+}
+
+void __exit intel_aet_exit(void)
+{
+	struct event_group **peg;
+
+	for_each_enabled_event_group(peg, known_energy_event_groups) {
+		intel_pmt_put_feature_group((*peg)->pfg);
+		(*peg)->pfg = NULL;
+	}
+	for_each_enabled_event_group(peg, known_perf_event_groups) {
+		intel_pmt_put_feature_group((*peg)->pfg);
+		(*peg)->pfg = NULL;
+	}
+}
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 52c8910ba2ef..ce9d086625c1 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -525,6 +525,19 @@ config X86_CPU_RESCTRL
 
 	  Say N if unsure.
 
+config X86_CPU_RESCTRL_INTEL_AET
+	bool "Intel Application Energy Telemetry"
+	depends on X86_CPU_RESCTRL && CPU_SUP_INTEL && INTEL_PMT_TELEMETRY=y && INTEL_TPMI=y
+	help
+	  Enable per-RMID telemetry events in resctrl.
+
+	  Intel feature that collects per-RMID execution data
+	  about energy consumption, measure of frequency independent
+	  activity and other performance metrics. Data is aggregated
+	  per package.
+
+	  Say N if unsure.
+
 config X86_FRED
 	bool "Flexible Return and Event Delivery"
 	depends on X86_64
diff --git a/arch/x86/kernel/cpu/resctrl/Makefile b/arch/x86/kernel/cpu/resctrl/Makefile
index d8a04b195da2..273ddfa30836 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_X86_CPU_RESCTRL_INTEL_AET)	+= intel_aet.o
 obj-$(CONFIG_RESCTRL_FS_PSEUDO_LOCK)	+= pseudo_lock.o
 
 # To allow define_trace.h's recursive include:
-- 
2.51.0

Hi Tony,

On 9/25/25 1:03 PM, Tony Luck wrote:
> Each CPU collects data for telemetry events that it sends to the nearest
> telemetry event aggregator either when the value of IA32_PQR_ASSOC.RMID

Please note that one of the "Touchups" done during merge of [1] was to
use full names for registers in descriptions. Considering this,
"IA32_PQR_ASSOC.RMID" -> "MSR_IA32_PQR_ASSOC.RMID

(also please make same change in cover letter)

> changes, or when a two millisecond timer expires.
> 

...

> +
> +/**
> + * struct event_group - All information about a group of telemetry events.
> + * @pfg:		Points to the aggregated telemetry space information
> + *			returned by the intel_pmt_get_regions_by_feature()
> + *			call to the INTEL_PMT_TELEMETRY driver that contains
> + *			data for all telemetry regions of a specific type.
> + *			Valid if the system supports the event group.
> + *			NULL otherwise.
> + * @guid:		Unique number per XML description file.
> + */
> +struct event_group {
> +	/* Data fields for additional structures to manage this group. */
> +	struct pmt_feature_group	*pfg;
> +
> +	/* Remaining fields initialized from XML file. */
> +	u32				guid;
> +};


...

> +
> +/*
> + * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
> + * pmt_feature_group for a specific feature. If there is one, the returned
> + * structure has an array of telemetry_region structures. Each describes
> + * one telemetry aggregator.
> + * Try to use every telemetry aggregator with a known guid.

The guid is associated with struct event_group and every telemetry region has
its own guid. It is not clear to me why the guid is not associated with pmt_feature_group.
To me this implies that a pmt_feature_group my contain telemetry regions that have
different guid.

This is not fully apparent in this patch but as this code evolves I do not think
the scenario where telemetry regions have different supported (by resctrl) guid is handled
by this enumeration.
If I understand correctly, all telemetry regions of a given pmt_feature_group will be
matched against a single supported guid at a time and all telemetry regions with that
guid will be considered usable and any other considered unusable without further processing
of that pmt_feature_group. If there are more than one matching guid supported by resctrl
then only events of the first one will be enumerated?

> + */
> +static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> +			    unsigned int num_evg)
> +{
> +	struct pmt_feature_group *p __free(intel_pmt_put_feature_group) = NULL;
> +	struct event_group **peg;
> +	bool ret;
> +
> +	p = intel_pmt_get_regions_by_feature(feature);
> +
> +	if (IS_ERR_OR_NULL(p))
> +		return false;
> +
> +	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> +		ret = enable_events(*peg, p);
> +		if (ret) {
> +			(*peg)->pfg = no_free_ptr(p);
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}

Reinette


[1] https://lore.kernel.org/all/175793566119.709179.8448328033383658699.tip-bot2@tip-bot2/

On Fri, Oct 03, 2025 at 04:35:11PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 9/25/25 1:03 PM, Tony Luck wrote:
> > Each CPU collects data for telemetry events that it sends to the nearest
> > telemetry event aggregator either when the value of IA32_PQR_ASSOC.RMID
> 
> Please note that one of the "Touchups" done during merge of [1] was to
> use full names for registers in descriptions. Considering this,
> "IA32_PQR_ASSOC.RMID" -> "MSR_IA32_PQR_ASSOC.RMID
> 
> (also please make same change in cover letter)

Will do.

> 
> > changes, or when a two millisecond timer expires.
> > 
> 
> ...
> 
> > +
> > +/**
> > + * struct event_group - All information about a group of telemetry events.
> > + * @pfg:		Points to the aggregated telemetry space information
> > + *			returned by the intel_pmt_get_regions_by_feature()
> > + *			call to the INTEL_PMT_TELEMETRY driver that contains
> > + *			data for all telemetry regions of a specific type.
> > + *			Valid if the system supports the event group.
> > + *			NULL otherwise.
> > + * @guid:		Unique number per XML description file.
> > + */
> > +struct event_group {
> > +	/* Data fields for additional structures to manage this group. */
> > +	struct pmt_feature_group	*pfg;
> > +
> > +	/* Remaining fields initialized from XML file. */
> > +	u32				guid;
> > +};
> 
> 
> ...
> 
> > +
> > +/*
> > + * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
> > + * pmt_feature_group for a specific feature. If there is one, the returned
> > + * structure has an array of telemetry_region structures. Each describes
> > + * one telemetry aggregator.
> > + * Try to use every telemetry aggregator with a known guid.
> 
> The guid is associated with struct event_group and every telemetry region has
> its own guid. It is not clear to me why the guid is not associated with pmt_feature_group.
> To me this implies that a pmt_feature_group my contain telemetry regions that have
> different guid.
> 
> This is not fully apparent in this patch but as this code evolves I do not think
> the scenario where telemetry regions have different supported (by resctrl) guid is handled
> by this enumeration.
> If I understand correctly, all telemetry regions of a given pmt_feature_group will be
> matched against a single supported guid at a time and all telemetry regions with that
> guid will be considered usable and any other considered unusable without further processing
> of that pmt_feature_group. If there are more than one matching guid supported by resctrl
> then only events of the first one will be enumerated?
> 
> > + */
> > +static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> > +			    unsigned int num_evg)
> > +{
> > +	struct pmt_feature_group *p __free(intel_pmt_put_feature_group) = NULL;
> > +	struct event_group **peg;
> > +	bool ret;
> > +
> > +	p = intel_pmt_get_regions_by_feature(feature);
> > +
> > +	if (IS_ERR_OR_NULL(p))
> > +		return false;
> > +
> > +	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> > +		ret = enable_events(*peg, p);
> > +		if (ret) {
> > +			(*peg)->pfg = no_free_ptr(p);
> > +			return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}

Perhaps David wants to cope with a future system that supports multiple
guids?

You are right that my code will not handle this. It will just enable
the first recognised guid and ignore any others.

How about this. Take an extra reference on any pmt_feature_group
structures that include a known guid (to keep the accounting right
when intel_aet_exit() is called). This simplifies the function so
I don't need the __free() handler that confuses checkpatch.pl :-)


/*
 * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
 * pmt_feature_group for a specific feature. If there is one, the returned
 * structure has an array of telemetry_region structures, each element of
 * the array describes one telemetry aggregator.
 * A single pmt_feature_group may include multiple different guids.
 * Try to use every telemetry aggregator with a known guid.
 */
static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
			    unsigned int num_evg)
{
	struct pmt_feature_group *p = intel_pmt_get_regions_by_feature(feature);
	struct event_group **peg;
	bool ret = false;

	if (IS_ERR_OR_NULL(p))
		return false;

	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
		if (enable_events(*peg, p)) {
			kref_get(&p->kref);
			(*peg)->pfg = no_free_ptr(p);
			ret = true;
		}
	}
	intel_pmt_put_feature_group(p);

	return ret;
}

> Reinette
> 
> 
> [1] https://lore.kernel.org/all/175793566119.709179.8448328033383658699.tip-bot2@tip-bot2/

Hi Tony,

On 10/6/25 11:19 AM, Luck, Tony wrote:
> On Fri, Oct 03, 2025 at 04:35:11PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 9/25/25 1:03 PM, Tony Luck wrote:
>>> +
>>> +/*
>>> + * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
>>> + * pmt_feature_group for a specific feature. If there is one, the returned
>>> + * structure has an array of telemetry_region structures. Each describes
>>> + * one telemetry aggregator.
>>> + * Try to use every telemetry aggregator with a known guid.
>>
>> The guid is associated with struct event_group and every telemetry region has
>> its own guid. It is not clear to me why the guid is not associated with pmt_feature_group.
>> To me this implies that a pmt_feature_group my contain telemetry regions that have
>> different guid.
>>
>> This is not fully apparent in this patch but as this code evolves I do not think
>> the scenario where telemetry regions have different supported (by resctrl) guid is handled
>> by this enumeration.
>> If I understand correctly, all telemetry regions of a given pmt_feature_group will be
>> matched against a single supported guid at a time and all telemetry regions with that
>> guid will be considered usable and any other considered unusable without further processing
>> of that pmt_feature_group. If there are more than one matching guid supported by resctrl
>> then only events of the first one will be enumerated?
>>
>>> + */
>>> +static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
>>> +			    unsigned int num_evg)
>>> +{
>>> +	struct pmt_feature_group *p __free(intel_pmt_put_feature_group) = NULL;
>>> +	struct event_group **peg;
>>> +	bool ret;
>>> +
>>> +	p = intel_pmt_get_regions_by_feature(feature);
>>> +
>>> +	if (IS_ERR_OR_NULL(p))
>>> +		return false;
>>> +
>>> +	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
>>> +		ret = enable_events(*peg, p);
>>> +		if (ret) {
>>> +			(*peg)->pfg = no_free_ptr(p);
>>> +			return true;
>>> +		}
>>> +	}
>>> +
>>> +	return false;
>>> +}
> 
> Perhaps David wants to cope with a future system that supports multiple
> guids?
> 
> You are right that my code will not handle this. It will just enable
> the first recognised guid and ignore any others.
> 
> How about this. Take an extra reference on any pmt_feature_group
> structures that include a known guid (to keep the accounting right
> when intel_aet_exit() is called). This simplifies the function so
> I don't need the __free() handler that confuses checkpatch.pl :-)
> 
> 
> /*
>  * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
>  * pmt_feature_group for a specific feature. If there is one, the returned
>  * structure has an array of telemetry_region structures, each element of
>  * the array describes one telemetry aggregator.
>  * A single pmt_feature_group may include multiple different guids.
>  * Try to use every telemetry aggregator with a known guid.
>  */
> static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> 			    unsigned int num_evg)
> {
> 	struct pmt_feature_group *p = intel_pmt_get_regions_by_feature(feature);
> 	struct event_group **peg;
> 	bool ret = false;
> 
> 	if (IS_ERR_OR_NULL(p))
> 		return false;
> 
> 	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> 		if (enable_events(*peg, p)) {
> 			kref_get(&p->kref);

This is not clear to me ... would enable_events() still mark all telemetry_regions
that do not match the event_group's guid as unusable? It seems to me that if more
than one even_group refers to the same pmt_feature_group then the first one to match
will "win" and make the other event_group's telemetry regions unusable.

> 			(*peg)->pfg = no_free_ptr(p);
> 			ret = true;
> 		}
> 	}
> 	intel_pmt_put_feature_group(p);
> 
> 	return ret;
> }
> 

Reinette

On Mon, Oct 06, 2025 at 02:33:00PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 10/6/25 11:19 AM, Luck, Tony wrote:
> > On Fri, Oct 03, 2025 at 04:35:11PM -0700, Reinette Chatre wrote:
> >> Hi Tony,
> >>
> >> On 9/25/25 1:03 PM, Tony Luck wrote:
> >>> +
> >>> +/*
> >>> + * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
> >>> + * pmt_feature_group for a specific feature. If there is one, the returned
> >>> + * structure has an array of telemetry_region structures. Each describes
> >>> + * one telemetry aggregator.
> >>> + * Try to use every telemetry aggregator with a known guid.
> >>
> >> The guid is associated with struct event_group and every telemetry region has
> >> its own guid. It is not clear to me why the guid is not associated with pmt_feature_group.
> >> To me this implies that a pmt_feature_group my contain telemetry regions that have
> >> different guid.
> >>
> >> This is not fully apparent in this patch but as this code evolves I do not think
> >> the scenario where telemetry regions have different supported (by resctrl) guid is handled
> >> by this enumeration.
> >> If I understand correctly, all telemetry regions of a given pmt_feature_group will be
> >> matched against a single supported guid at a time and all telemetry regions with that
> >> guid will be considered usable and any other considered unusable without further processing
> >> of that pmt_feature_group. If there are more than one matching guid supported by resctrl
> >> then only events of the first one will be enumerated?
> >>
> >>> + */
> >>> +static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> >>> +			    unsigned int num_evg)
> >>> +{
> >>> +	struct pmt_feature_group *p __free(intel_pmt_put_feature_group) = NULL;
> >>> +	struct event_group **peg;
> >>> +	bool ret;
> >>> +
> >>> +	p = intel_pmt_get_regions_by_feature(feature);
> >>> +
> >>> +	if (IS_ERR_OR_NULL(p))
> >>> +		return false;
> >>> +
> >>> +	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> >>> +		ret = enable_events(*peg, p);
> >>> +		if (ret) {
> >>> +			(*peg)->pfg = no_free_ptr(p);
> >>> +			return true;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	return false;
> >>> +}
> > 
> > Perhaps David wants to cope with a future system that supports multiple
> > guids?
> > 
> > You are right that my code will not handle this. It will just enable
> > the first recognised guid and ignore any others.
> > 
> > How about this. Take an extra reference on any pmt_feature_group
> > structures that include a known guid (to keep the accounting right
> > when intel_aet_exit() is called). This simplifies the function so
> > I don't need the __free() handler that confuses checkpatch.pl :-)
> > 
> > 
> > /*
> >  * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
> >  * pmt_feature_group for a specific feature. If there is one, the returned
> >  * structure has an array of telemetry_region structures, each element of
> >  * the array describes one telemetry aggregator.
> >  * A single pmt_feature_group may include multiple different guids.
> >  * Try to use every telemetry aggregator with a known guid.
> >  */
> > static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> > 			    unsigned int num_evg)
> > {
> > 	struct pmt_feature_group *p = intel_pmt_get_regions_by_feature(feature);
> > 	struct event_group **peg;
> > 	bool ret = false;
> > 
> > 	if (IS_ERR_OR_NULL(p))
> > 		return false;
> > 
> > 	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> > 		if (enable_events(*peg, p)) {
> > 			kref_get(&p->kref);
> 
> This is not clear to me ... would enable_events() still mark all telemetry_regions
> that do not match the event_group's guid as unusable? It seems to me that if more
> than one even_group refers to the same pmt_feature_group then the first one to match
> will "win" and make the other event_group's telemetry regions unusable.

Extra context needed. Sorry.

I'm changing enable_events() to only mark telemetry_regions regions as
unusable if they have a bad package id, or the MMIO size doesn't match.
I.e. they truly are bad.

Mis-match on guid will skip then while associating with a specific
event_gruoup, but leave them as usable.

This means that intel_aet_read_event() now has to check the guid as
well as !addr.

An alternative approach would be to ask the PMT code for separate
copies of the pmt_feature_group to attach to each event_group. I
didn't like this, do you think it would be better?

> 
> > 			(*peg)->pfg = no_free_ptr(p);
> > 			ret = true;
> > 		}
> > 	}
> > 	intel_pmt_put_feature_group(p);
> > 
> > 	return ret;
> > }
> > 
> 
> Reinette
> 

-Tony

On Mon, Oct 06, 2025 at 02:47:15PM -0700, Luck, Tony wrote:
> On Mon, Oct 06, 2025 at 02:33:00PM -0700, Reinette Chatre wrote:
> > Hi Tony,
> > > static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> > > 			    unsigned int num_evg)
> > > {
> > > 	struct pmt_feature_group *p = intel_pmt_get_regions_by_feature(feature);
> > > 	struct event_group **peg;
> > > 	bool ret = false;
> > > 
> > > 	if (IS_ERR_OR_NULL(p))
> > > 		return false;
> > > 
> > > 	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> > > 		if (enable_events(*peg, p)) {
> > > 			kref_get(&p->kref);
> > 
> > This is not clear to me ... would enable_events() still mark all telemetry_regions
> > that do not match the event_group's guid as unusable? It seems to me that if more
> > than one even_group refers to the same pmt_feature_group then the first one to match
> > will "win" and make the other event_group's telemetry regions unusable.
> 
> Extra context needed. Sorry.
> 
> I'm changing enable_events() to only mark telemetry_regions regions as
> unusable if they have a bad package id, or the MMIO size doesn't match.
> I.e. they truly are bad.
> 
> Mis-match on guid will skip then while associating with a specific
> event_gruoup, but leave them as usable.
> 
> This means that intel_aet_read_event() now has to check the guid as
> well as !addr.
> 
> An alternative approach would be to ask the PMT code for separate
> copies of the pmt_feature_group to attach to each event_group. I
> didn't like this, do you think it would be better?

Working through more patches in the series, I've come to the one
that adjusts the number of RMIDs.  The alternative approach of
having a separate copy of the pmt_feature_group is suddently looking
more attractive.

So the code would become:


static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
			    unsigned int num_evg)
{
	struct pmt_feature_group *p;
	struct event_group **peg;
	bool ret = false;

	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
		p = intel_pmt_get_regions_by_feature(feature);
		if (IS_ERR_OR_NULL(p))
			return false;

		if (enable_events(*peg, p)) {
			(*peg)->pfg = p;
			ret = true;
		} else {
			intel_pmt_put_feature_group(p);
		}
	}
	intel_pmt_put_feature_group(p);

	return ret;
}

> 
> > 
> > > 			(*peg)->pfg = no_free_ptr(p);
> > > 			ret = true;
> > > 		}
> > > 	}
> > > 	intel_pmt_put_feature_group(p);
> > > 
> > > 	return ret;
> > > }
> > > 
> > 
> > Reinette
> > 
> 
> -Tony
-Tony

Hi Tony,

On 10/7/25 1:47 PM, Luck, Tony wrote:
> On Mon, Oct 06, 2025 at 02:47:15PM -0700, Luck, Tony wrote:
>> On Mon, Oct 06, 2025 at 02:33:00PM -0700, Reinette Chatre wrote:
>>> Hi Tony,
>>>> static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
>>>> 			    unsigned int num_evg)
>>>> {
>>>> 	struct pmt_feature_group *p = intel_pmt_get_regions_by_feature(feature);
>>>> 	struct event_group **peg;
>>>> 	bool ret = false;
>>>>
>>>> 	if (IS_ERR_OR_NULL(p))
>>>> 		return false;
>>>>
>>>> 	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
>>>> 		if (enable_events(*peg, p)) {
>>>> 			kref_get(&p->kref);
>>>
>>> This is not clear to me ... would enable_events() still mark all telemetry_regions
>>> that do not match the event_group's guid as unusable? It seems to me that if more
>>> than one even_group refers to the same pmt_feature_group then the first one to match
>>> will "win" and make the other event_group's telemetry regions unusable.
>>
>> Extra context needed. Sorry.
>>
>> I'm changing enable_events() to only mark telemetry_regions regions as
>> unusable if they have a bad package id, or the MMIO size doesn't match.
>> I.e. they truly are bad.
>>
>> Mis-match on guid will skip then while associating with a specific
>> event_gruoup, but leave them as usable.
>>
>> This means that intel_aet_read_event() now has to check the guid as
>> well as !addr.
>>
>> An alternative approach would be to ask the PMT code for separate
>> copies of the pmt_feature_group to attach to each event_group. I
>> didn't like this, do you think it would be better?
> 
> Working through more patches in the series, I've come to the one
> that adjusts the number of RMIDs.  The alternative approach of

I see, with the number of RMIDs a property of the event group self this
seems reasonable. While there is duplication of pmt_feature_group I am not
able to tell if this is a big issue since I am not clear on how/if systems
will be built this way.

> having a separate copy of the pmt_feature_group is suddently looking
> more attractive.
> 
> So the code would become:
> 
> 
> static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> 			    unsigned int num_evg)
> {
> 	struct pmt_feature_group *p;
> 	struct event_group **peg;
> 	bool ret = false;
> 
> 	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> 		p = intel_pmt_get_regions_by_feature(feature);
> 		if (IS_ERR_OR_NULL(p))
> 			return false;
> 
> 		if (enable_events(*peg, p)) {
> 			(*peg)->pfg = p;
> 			ret = true;
> 		} else {
> 			intel_pmt_put_feature_group(p);
> 		}
> 	}
> 	intel_pmt_put_feature_group(p);

I am not able to tell why this "put" is needed? I assume the "put" of a
pmt_feature_group assigned to an event_group will still be done in
intel_aet_exit()?

Reinette

> > static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> >                         unsigned int num_evg)
> > {
> >     struct pmt_feature_group *p;
> >     struct event_group **peg;
> >     bool ret = false;
> >
> >     for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> >             p = intel_pmt_get_regions_by_feature(feature);
> >             if (IS_ERR_OR_NULL(p))
> >                     return false;
> >
> >             if (enable_events(*peg, p)) {
> >                     (*peg)->pfg = p;
> >                     ret = true;
> >             } else {
> >                     intel_pmt_put_feature_group(p);
> >             }
> >     }
> >     intel_pmt_put_feature_group(p);
>
> I am not able to tell why this "put" is needed? I assume the "put" of a
> pmt_feature_group assigned to an event_group will still be done in
> intel_aet_exit()?

Reinette

That "put" was left over from the previous version. You are right it
isn't needed. The "put" will be done in intel_aet_exit()

-Tony