Re: [PATCH v6 19/30] x86/resctrl: Complete telemetry event enumeration

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

Hi Tony,

On 6/26/25 9:49 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
> the base MMIO address for the counters.
> 
> Scan all the telemetry_region structures again and save the number
> of regions together with a flex array of the mmio addresses for each

mmio -> MMIO

> aggregator indexed by package id. Note that there may be multiple
> aggregators per package.

This final "note" seems redundant considering the paragraph
reads: "Scan all the telemetry_region structures (plural) again and
save the number of regions (plural) ..." Perhaps just rework the
paragraph to read:

	There may be multiple aggregators per package. Scan all the
	telemetry_region structures ...

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

since there are multiple arrays in this depiction it can be made
specific with a:
	       | pkginfo[package ID 0]  | pkginfo[package ID 1] |


>              +---------------------+---------------------+
>                         |                     |
>                         v                     v
>                 +----------------+    +----------------+
>                 |struct mmio_info|    |struct mmio_info|
>                 +----------------+    +----------------+
>                 |num_regions = N |    |num_regions = N |
>                 |  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, 64 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 8d67ed709a74..c770039b2525 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -19,17 +19,32 @@
>  
>  #include "internal.h"
>  
> +/**
> + * struct 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 mmio_info {

This struct name is a bit generic. What do you think of "pkg_mmio_info" to
at least help describe it is per package?

> +	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.
> + * @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 mmio_info		**pkginfo;
>  
>  	/* Remaining fields initialized from XML file. */
>  	u32				guid;
> @@ -81,6 +96,20 @@ static bool skip_this_region(struct telemetry_region *tr, struct event_group *e)
>  	return false;
>  }
>  
> +static void free_mmio_info(struct 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(mmio_info, struct mmio_info **, free_mmio_info(_T))
> +
>  /*
>   * Configure events from one pmt_feature_group.
>   * 1) Count how many per package.

(no update to the function comments)

> @@ -88,8 +117,10 @@ static bool skip_this_region(struct telemetry_region *tr, struct event_group *e)
>   */
>  static int configure_events(struct event_group *e, struct pmt_feature_group *p)
>  {
> +	struct mmio_info **pkginfo __free(mmio_info) = NULL;
>  	int *pkgcounts __free(kfree) = NULL;
>  	struct telemetry_region *tr;
> +	struct mmio_info *mmi;
>  	int num_pkgs;
>  
>  	num_pkgs = topology_max_packages();
> @@ -99,6 +130,12 @@ static int configure_events(struct event_group *e, struct pmt_feature_group *p)
>  		tr = &p->regions[i];
>  		if (skip_this_region(tr, e))
>  			continue;
> +
> +		if (e->pkginfo) {
> +			pr_warn_once("Duplicate telemetry information for guid 0x%x\n", e->guid);
> +			return -EINVAL;
> +		}

It does not seem necessary to repeat this check for every telemetry region. Could this check
be moved to start of function to avoid parsing struct pmt_feature_group entirely?

> +
>  		if (!pkgcounts) {
>  			pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
>  			if (!pkgcounts)
> @@ -110,6 +147,32 @@ static int configure_events(struct event_group *e, struct pmt_feature_group *p)
>  	if (!pkgcounts)
>  		return -ENODEV;
>  
> +	/* Allocate array for per-package struct mmio_info data */
> +	pkginfo = kcalloc(num_pkgs, sizeof(*pkginfo), GFP_KERNEL);
> +	if (!pkginfo)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Allocate per-package 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;
>  }
>  
> @@ -169,5 +232,6 @@ void __exit intel_aet_exit(void)
>  			intel_pmt_put_feature_group((*peg)->pfg);
>  			(*peg)->pfg = NULL;
>  		}
> +		free_mmio_info((*peg)->pkginfo);
>  	}
>  }

Reinette