Re: [PATCH v7 19/31] x86/resctrl: Complete telemetry event enumeration

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 7/11/25 4:53 PM, 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
> 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 aggregator indexed by
> package id.

I do not see why it is needed to switch back and forth between interchangeable
regions and aggregators. Why not just stick with telemetry regions? It is
confusing when, for example, saying "number of regions" followed by "for each
aggregator"? Why not just say "number of regions" followed by "for each
region"?

> 
> Completed structure for each event group looks like this:
> 
>              +---------------------+---------------------+
> pkginfo** -->|pkginfo[package ID 0]|pkginfo[package ID 1]|
>              +---------------------+---------------------+
>                         |                     |
>                         v                     v
>                 +----------------+    +----------------+
>                 |struct mmio_info|    |struct mmio_info|

mmio_info -> pkg_mmio_info

>                 +----------------+    +----------------+
>                 |num_regions = N |    |num_regions = N |

The above "There may be multiple aggregators (telemetry regions?) per
package." could add that the number of telemetry regions per package may
be different and supported by an example where one package has "N"
regions and the other "M".

>                 |  addrs[0]      |    |  addrs[0]      |
>                 |  addrs[1]      |    |  addrs[1]      |
>                 |    ...         |    |    ...         |
>                 |  addrs[N-1]    |    |  addrs[N-1]    |
>                 +----------------+    +----------------+
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 64 ++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 7cd6c06f9205..3f383f0a9d08 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -19,12 +19,26 @@
>  
>  #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:		List 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.
>   */
> @@ -32,6 +46,7 @@ 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;
> @@ -90,15 +105,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 used by each aggregator.

"the telemetry regions used by each aggregator" does not sound right. "telemetry region == aggregator", no?

>   */
>  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();
> @@ -108,6 +140,7 @@ static int discover_events(struct event_group *e, struct pmt_feature_group *p)
>  		tr = &p->regions[i];
>  		if (skip_this_region(tr, e))
>  			continue;
> +
>  		if (!pkgcounts) {
>  			pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
>  			if (!pkgcounts)

squash with previous patch.

> @@ -119,6 +152,32 @@ 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);
> +
>  	return 0;
>  }
>  
> @@ -151,6 +210,7 @@ static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **ev
>  			(*peg)->pfg = no_free_ptr(p);
>  			return true;
>  		}
> +		free_pkg_mmio_info((*peg)->pkginfo);

Is this necessary? pkginfo will only be set on success, no?

>  	}
>  
>  	return false;
> @@ -179,6 +239,8 @@ void __exit intel_aet_exit(void)
>  	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);
>  	}
>  }

Reinette