Re: [PATCH v4 18/31] x86/resctrl: Add third part of telemetry event enumeration

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

Hi Tony,

On 4/28/25 5:33 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.
> 
> Scan all the telemetry_region structures again and gather these
> addresses into a more convenient structure with addresses for
> each aggregator indexed by package id. Note that there may be
> multiple aggregators per package.

Could this series please provide a clear definition for "telemetry
region" and "aggregator" and then use the terms consistently?
I find the comments to switch between the two causing confusion.

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 55 +++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index a0365c3ce982..03839d5c369b 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -20,6 +20,16 @@
>  
>  #include "internal.h"
>  
> +/**
> + * struct mmio_info - Array of MMIO addresses for a package

Please add description on how this structure is used. Please use
these docs to help readers create a mental model of how these
data structures fit together. 

I am making an attempt at an example below but I am still trying to
understand how things fit together so would appreciate if you
write this instead. (Please consider this when viewing any of the
samples I provide.)

Example,
	Array of MMIO addresses of one event group for a package.

	Provides convenient access to all MMIO addresses of
	one event group for one package. Used when reading
	event data on a package. (needs improvement)

> + * @count:		Number of addresses on this package

Any information on what this number means? For example,
"Number of telemetry regions of a specific event group."

> + * @addrs:		The MMIO addresses

Can the layout of MMIO space be added to the comments? 

> + */
> +struct mmio_info {
> +	int		count;
> +	void __iomem	*addrs[] __counted_by(count);
> +};
> +
>  /**
>   * struct event_group - All information about a group of telemetry events.
>   *                      Some fields initialized with MMIO layout information
> @@ -27,10 +37,12 @@
>   *                      retrieved from intel_pmt_get_regions_by_feature().
>   * @pfg:		The pmt_feature_group for this event group
>   * @guid:		Unique number per XML description file
> + * @pkginfo:		Per-package MMIO addresses

"Per-package MMIO addresses of telemetry regions belonging to this group."?

>   */
>  struct event_group {
>  	struct pmt_feature_group	*pfg;
>  	int				guid;
> +	struct mmio_info		**pkginfo;
>  };
>  
>  /* Link: https://github.com/intel/Intel-PMT xml/CWF/OOBMSM/RMID-ENERGY *.xml */
> @@ -50,12 +62,33 @@ static struct event_group *known_event_groups[] = {
>  
>  #define NUM_KNOWN_GROUPS ARRAY_SIZE(known_event_groups)
>  
> +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))
> +
>  static bool configure_events(struct event_group *e, struct pmt_feature_group *p)
>  {
> +	struct mmio_info __free(mmio_info) **pkginfo = NULL;
>  	int *pkgcounts __free(kfree) = NULL;
>  	struct telemetry_region *tr;
> +	struct mmio_info *mmi;
>  	int num_pkgs;
>  
> +	if (e->pkginfo) {
> +		pr_warn("Duplicate telemetry information for guid 0x%x\n", e->guid);
> +		return false;
> +	}
> +
>  	num_pkgs = topology_max_packages();
>  	pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
>  	if (!pkgcounts)
> @@ -73,6 +106,27 @@ static bool configure_events(struct event_group *e, struct pmt_feature_group *p)
>  		pkgcounts[tr->plat_info.package_id]++;
>  	}
>  
> +	/* Allocate per-package arrays and save MMIO addresses */

per-package arrays of what?

> +	pkginfo = kcalloc(num_pkgs, sizeof(*pkginfo), GFP_KERNEL);
> +	if (!pkginfo)
> +		return false;

-ENOMEM

> +	for (int i = 0; i < num_pkgs; i++) {
> +		pkginfo[i] = kmalloc(struct_size(pkginfo[i], addrs, pkgcounts[i]), GFP_KERNEL);

kzalloc()

> +		if (!pkginfo[i])
> +			return false;

-ENOMEM

> +		pkginfo[i]->count = pkgcounts[i];
> +	}
> +
> +	/* Save MMIO address(es) for each aggregator in per-package structures */

Should "aggregator" be "telemetry region"? It is becoming confusing what "aggregator"
vs "telemetry region" represents here.

> +	for (int i = 0; i < p->count; i++) {
> +		tr = &p->regions[i];
> +		if (tr->guid != e->guid || tr->plat_info.package_id >= num_pkgs)
> +			continue;
> +		mmi = pkginfo[tr->plat_info.package_id];
> +		mmi->addrs[--pkgcounts[tr->plat_info.package_id]] = tr->addr;

For this code to be safe the "if()" checks that precede it must match *exactly*
with the checks used to initialize the pkgcounts array. To ensure this remains the
case I think those checks need to be placed in a function to be called in both
places.

> +	}
> +	e->pkginfo = no_free_ptr(pkginfo);
> +
>  	return false;
>  }
>  
> @@ -130,5 +184,6 @@ void __exit intel_aet_exit(void)
>  			intel_pmt_put_feature_group((*peg)->pfg);
>  			(*peg)->pfg = NULL;
>  		}
> +		free_mmio_info((*peg)->pkginfo);
>  	}
>  }

Reinette