Re: [PATCH v11 14/31] x86/resctrl: Discover hardware telemetry events

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

Hi Tony,

On 9/25/25 1:03 PM, Tony Luck wrote:
> Each CPU collects data for telemetry events that it sends to the nearest
> telemetry event aggregator either when the value of IA32_PQR_ASSOC.RMID

Please note that one of the "Touchups" done during merge of [1] was to
use full names for registers in descriptions. Considering this,
"IA32_PQR_ASSOC.RMID" -> "MSR_IA32_PQR_ASSOC.RMID

(also please make same change in cover letter)

> changes, or when a two millisecond timer expires.
> 

...

> +
> +/**
> + * struct event_group - All information about a group of telemetry events.
> + * @pfg:		Points to the aggregated telemetry space information
> + *			returned by the intel_pmt_get_regions_by_feature()
> + *			call to the INTEL_PMT_TELEMETRY driver that contains
> + *			data for all telemetry regions of a specific type.
> + *			Valid if the system supports the event group.
> + *			NULL otherwise.
> + * @guid:		Unique number per XML description file.
> + */
> +struct event_group {
> +	/* Data fields for additional structures to manage this group. */
> +	struct pmt_feature_group	*pfg;
> +
> +	/* Remaining fields initialized from XML file. */
> +	u32				guid;
> +};


...

> +
> +/*
> + * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
> + * pmt_feature_group for a specific feature. If there is one, the returned
> + * structure has an array of telemetry_region structures. Each describes
> + * one telemetry aggregator.
> + * Try to use every telemetry aggregator with a known guid.

The guid is associated with struct event_group and every telemetry region has
its own guid. It is not clear to me why the guid is not associated with pmt_feature_group.
To me this implies that a pmt_feature_group my contain telemetry regions that have
different guid.

This is not fully apparent in this patch but as this code evolves I do not think
the scenario where telemetry regions have different supported (by resctrl) guid is handled
by this enumeration.
If I understand correctly, all telemetry regions of a given pmt_feature_group will be
matched against a single supported guid at a time and all telemetry regions with that
guid will be considered usable and any other considered unusable without further processing
of that pmt_feature_group. If there are more than one matching guid supported by resctrl
then only events of the first one will be enumerated?

> + */
> +static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> +			    unsigned int num_evg)
> +{
> +	struct pmt_feature_group *p __free(intel_pmt_put_feature_group) = NULL;
> +	struct event_group **peg;
> +	bool ret;
> +
> +	p = intel_pmt_get_regions_by_feature(feature);
> +
> +	if (IS_ERR_OR_NULL(p))
> +		return false;
> +
> +	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> +		ret = enable_events(*peg, p);
> +		if (ret) {
> +			(*peg)->pfg = no_free_ptr(p);
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}

Reinette


[1] https://lore.kernel.org/all/175793566119.709179.8448328033383658699.tip-bot2@tip-bot2/