Re: [PATCH v5 18/29] x86/resctrl: Count valid telemetry aggregators per package

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

Hi Tony,

On 5/21/25 3:50 PM, Tony Luck wrote:
> There may be multiple telemetry aggregators per package, each enumerated
> by a telemetry region structure in the feature group.
> 
> Scan the array of telemetry region structures and count how many are
> in each package in preparation to allocate structures to save the MMIO
> addresses for each in a convenient format for use when reading event
> counters.
> 
> Sanity check that the telemetry region structures have a valid
> package_id and that the size they report for the MMIO space is as
> large as expected from the XML description of the registers in
> the region.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 45 ++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index df73b9476c4d..ffcb54be54ea 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -14,6 +14,7 @@
>  #include <linux/cleanup.h>
>  #include <linux/cpu.h>
>  #include <linux/resctrl.h>
> +#include <linux/slab.h>
>  
>  /* Temporary - delete from final version */
>  #include "fake_intel_aet_features.h"
> @@ -26,6 +27,7 @@
>   *			within the OOBMSM driver that contains data for all
>   *			telemetry regions.
>   * @guid:		Unique number per XML description file.
> + * @mmio_size:		Number of bytes of MMIO registers for this group.
>   */
>  struct event_group {
>  	/* Data fields used by this code. */
> @@ -33,6 +35,7 @@ struct event_group {
>  
>  	/* Remaining fields initialized from XML file. */
>  	u32				guid;
> +	size_t				mmio_size;
>  };
>  
>  /*
> @@ -41,6 +44,7 @@ struct event_group {
>   */
>  static struct event_group energy_0x26696143 = {
>  	.guid		= 0x26696143,
> +	.mmio_size	= (576 * 2 + 3) * 8,

Could you please add a snippet to the struct description that
documents what these constants mean?

>  };
>  
>  /*
> @@ -49,6 +53,7 @@ static struct event_group energy_0x26696143 = {
>   */
>  static struct event_group perf_0x26557651 = {
>  	.guid		= 0x26557651,
> +	.mmio_size	= (576 * 7 + 3) * 8,

Same here.

>  };
>  
>  static struct event_group *known_event_groups[] = {
> @@ -58,9 +63,47 @@ static struct event_group *known_event_groups[] = {
>  
>  #define NUM_KNOWN_GROUPS ARRAY_SIZE(known_event_groups)
>  
> -/* Stub for now */
> +static bool skip_this_region(struct telemetry_region *tr, struct event_group *e)
> +{
> +	if (tr->guid != e->guid)
> +		return true;
> +	if (tr->plat_info.package_id >= topology_max_packages()) {
> +		pr_warn_once("Bad package %d in guid 0x%x\n", tr->plat_info.package_id,
> +			     tr->guid);
> +		return true;
> +	}
> +	if (tr->size < e->mmio_size) {
> +		pr_warn_once("MMIO space too small for guid 0x%x\n", e->guid);

With e->mmio_size hardcoded it may be useful to print the size claimed
by the telemetry region.

> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * Configure events from one pmt_feature_group.
> + * 1) Count how many per package.
> + * 2...) To be continued.
> + */
>  static int configure_events(struct event_group *e, struct pmt_feature_group *p)
>  {
> +	int *pkgcounts __free(kfree) = NULL;
> +	struct telemetry_region *tr;
> +	int num_pkgs;
> +
> +	num_pkgs = topology_max_packages();
> +	pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
> +	if (!pkgcounts)
> +		return -ENOMEM;
> +
> +	/* Get per-package counts of telemetry_regions for this event group */
> +	for (int i = 0; i < p->count; i++) {
> +		tr = &p->regions[i];
> +		if (skip_this_region(tr, e))
> +			continue;
> +		pkgcounts[tr->plat_info.package_id]++;
> +	}
> +
>  	return -EINVAL;
>  }
>  


Reinette