Counters for telemetry events are in MMIO space. Each telemetry_region
structure returned in the pmt_feature_group returned from OOBMSM contains
the base MMIO address for the counters.

There may be multiple aggregators per package. Scan all the
telemetry_region structures again and save the number of regions together
with a flex array of the MMIO addresses for each region indexed by
package id.

Completed structure for each event group looks like this:

             +---------------------+---------------------+
pkginfo** -->|pkginfo[package ID 0]|pkginfo[package ID 1]|
             +---------------------+---------------------+
                        |                     |
                        v                     v
                +--------------------+    +--------------------+
                |struct pkg_mmio_info|    |struct pkg_mmio_info|
                +--------------------+    +--------------------+
                |num_regions = M     |    |num_regions = N     |
                |  addrs[0]          |    |  addrs[0]          |
                |  addrs[1]          |    |  addrs[1]          |
                |    ...             |    |    ...             |
                |  addrs[M-1]        |    |  addrs[N-1]        |
                +--------------------+    +--------------------+

Build a list (active_event_groups) of all the event groups that
were successfully enabled. Use it to clean up in intel_aet_exit().

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/resctrl/intel_aet.c | 76 ++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
index 09043d36e08c..151e4b56ab36 100644
--- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
+++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
@@ -19,23 +19,43 @@
 
 #include "internal.h"
 
+/**
+ * struct pkg_mmio_info - MMIO address information for one event group of a package.
+ * @num_regions:	Number of telemetry regions on this package.
+ * @addrs:		Array of MMIO addresses, one per telemetry region on this package.
+ *
+ * Provides convenient access to all MMIO addresses of one event group
+ * for one package. Used when reading event data on a package.
+ */
+struct pkg_mmio_info {
+	unsigned int	num_regions;
+	void __iomem	*addrs[] __counted_by(num_regions);
+};
+
 /**
  * struct event_group - All information about a group of telemetry events.
  * @pfg:		Points to the aggregated telemetry space information
  *			within the OOBMSM driver that contains data for all
  *			telemetry regions.
+ * @list:		Member of active_event_groups.
+ * @pkginfo:		Per-package MMIO addresses of telemetry regions belonging to this group.
  * @guid:		Unique number per XML description file.
  * @mmio_size:		Number of bytes of MMIO registers for this group.
  */
 struct event_group {
 	/* Data fields for additional structures to manage this group. */
 	struct pmt_feature_group	*pfg;
+	struct list_head		list;
+	struct pkg_mmio_info		**pkginfo;
 
 	/* Remaining fields initialized from XML file. */
 	u32				guid;
 	size_t				mmio_size;
 };
 
+/* All successfully enabled event groups */
+static LIST_HEAD(active_event_groups);
+
 #define XML_MMIO_SIZE(num_rmids, num_events, num_extra_status) \
 		      (((num_rmids) * (num_events) + (num_extra_status)) * sizeof(u64))
 
@@ -82,15 +102,32 @@ static bool skip_this_region(struct telemetry_region *tr, struct event_group *e)
 	return false;
 }
 
+static void free_pkg_mmio_info(struct pkg_mmio_info **mmi)
+{
+	int num_pkgs = topology_max_packages();
+
+	if (!mmi)
+		return;
+
+	for (int i = 0; i < num_pkgs; i++)
+		kfree(mmi[i]);
+	kfree(mmi);
+}
+
+DEFINE_FREE(pkg_mmio_info, struct pkg_mmio_info **, free_pkg_mmio_info(_T))
+
 /*
  * Discover events from one pmt_feature_group.
  * 1) Count how many usable telemetry regions per package.
- * 2...) To be continued.
+ * 2) Allocate per-package structures and populate with MMIO
+ *    addresses of the telemetry regions.
  */
 static int discover_events(struct event_group *e, struct pmt_feature_group *p)
 {
+	struct pkg_mmio_info **pkginfo __free(pkg_mmio_info) = NULL;
 	int *pkgcounts __free(kfree) = NULL;
 	struct telemetry_region *tr;
+	struct pkg_mmio_info *mmi;
 	int num_pkgs;
 
 	num_pkgs = topology_max_packages();
@@ -112,6 +149,34 @@ static int discover_events(struct event_group *e, struct pmt_feature_group *p)
 	if (!pkgcounts)
 		return -ENODEV;
 
+	/* Allocate array for per-package struct pkg_mmio_info data */
+	pkginfo = kcalloc(num_pkgs, sizeof(*pkginfo), GFP_KERNEL);
+	if (!pkginfo)
+		return -ENOMEM;
+
+	/*
+	 * Allocate per-package pkg_mmio_info structures and initialize
+	 * count of telemetry_regions in each one.
+	 */
+	for (int i = 0; i < num_pkgs; i++) {
+		pkginfo[i] = kzalloc(struct_size(pkginfo[i], addrs, pkgcounts[i]), GFP_KERNEL);
+		if (!pkginfo[i])
+			return -ENOMEM;
+		pkginfo[i]->num_regions = pkgcounts[i];
+	}
+
+	/* Save MMIO address(es) for each telemetry region in per-package structures */
+	for (int i = 0; i < p->count; i++) {
+		tr = &p->regions[i];
+		if (skip_this_region(tr, e))
+			continue;
+		mmi = pkginfo[tr->plat_info.package_id];
+		mmi->addrs[--pkgcounts[tr->plat_info.package_id]] = tr->addr;
+	}
+	e->pkginfo = no_free_ptr(pkginfo);
+
+	list_add(&e->list, &active_event_groups);
+
 	return 0;
 }
 
@@ -169,4 +234,13 @@ bool intel_aet_get_events(void)
 
 void __exit intel_aet_exit(void)
 {
+	struct event_group *evg, *tmp;
+
+	list_for_each_entry_safe(evg, tmp, &active_event_groups, list) {
+		intel_pmt_put_feature_group(evg->pfg);
+		evg->pfg = NULL;
+		free_pkg_mmio_info(evg->pkginfo);
+		evg->pkginfo = NULL;
+		list_del(&evg->list);
+	}
 }
-- 
2.50.1

Hi Tony,

On 8/11/25 11:16 AM, Tony Luck wrote:
> Counters for telemetry events are in MMIO space. Each telemetry_region
> structure returned in the pmt_feature_group returned from OOBMSM contains

"OOBMSM" -> "INTEL_PMT_TELEMETRY"?

> the base MMIO address for the counters.
> 
> There may be multiple aggregators per package. Scan all the
> telemetry_region structures again and save the number of regions together
> with a flex array of the MMIO addresses for each region indexed by
> package id.
> 
> Completed structure for each event group looks like this:
> 
>              +---------------------+---------------------+
> pkginfo** -->|pkginfo[package ID 0]|pkginfo[package ID 1]|
>              +---------------------+---------------------+
>                         |                     |
>                         v                     v
>                 +--------------------+    +--------------------+
>                 |struct pkg_mmio_info|    |struct pkg_mmio_info|
>                 +--------------------+    +--------------------+
>                 |num_regions = M     |    |num_regions = N     |
>                 |  addrs[0]          |    |  addrs[0]          |
>                 |  addrs[1]          |    |  addrs[1]          |
>                 |    ...             |    |    ...             |
>                 |  addrs[M-1]        |    |  addrs[N-1]        |
>                 +--------------------+    +--------------------+
> 
> Build a list (active_event_groups) of all the event groups that
> were successfully enabled. Use it to clean up in intel_aet_exit().

While this adds a note about active_event_groups it does not motivate 
*why* this additional data structure is needed. I find this additional
data structure unnecessary and actually makes the code harder to understand.
As I understand event_group::pfg can already be used to determine if the
event group is active or not. Adding a new data structure to track this thus seems
unnecessary. If this is a data structure created for convenience I think it
should rather be replaced by helpers that use event_group::pfg.

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

...
  
> @@ -169,4 +234,13 @@ bool intel_aet_get_events(void)
>  
>  void __exit intel_aet_exit(void)
>  {
> +	struct event_group *evg, *tmp;
> +
> +	list_for_each_entry_safe(evg, tmp, &active_event_groups, list) {
> +		intel_pmt_put_feature_group(evg->pfg);
> +		evg->pfg = NULL;
> +		free_pkg_mmio_info(evg->pkginfo);
> +		evg->pkginfo = NULL;
> +		list_del(&evg->list);
> +	}
>  }

I think above can be simplified by making it symmetrical to
intel_aet_get_events(). 
For example:

static void put_pmt_feature(struct event_group **evgs, unsigned int num_evg)    
{                                                                               
	struct event_group **peg;                                               
                                                                                
	for (peg = evgs; peg < &evgs[num_evg]; peg++) {                         
		if (!(*peg)->pfg)                                               
			return;                                                 
		intel_pmt_put_feature_group((*peg)->pfg);                       
		/* rest of cleanup from intel_aet_exit() above */
	}                                                                       
}                                      

void __ exit intel_aet_exit(void) {                                                          
	put_pmt_feature(known_energy_event_groups,                              
			ARRAY_SIZE(known_energy_event_groups));                 
	put_pmt_feature(known_perf_event_groups,                                
			ARRAY_SIZE(known_perf_event_groups));                   
}                                           

Reinette