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

["Luck, Tony" <tony.luck@intel.com>]

On Mon, Oct 06, 2025 at 02:33:00PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 10/6/25 11:19 AM, Luck, Tony wrote:
> > On Fri, Oct 03, 2025 at 04:35:11PM -0700, Reinette Chatre wrote:
> >> Hi Tony,
> >>
> >> On 9/25/25 1:03 PM, Tony Luck wrote:
> >>> +
> >>> +/*
> >>> + * 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;
> >>> +}
> > 
> > Perhaps David wants to cope with a future system that supports multiple
> > guids?
> > 
> > You are right that my code will not handle this. It will just enable
> > the first recognised guid and ignore any others.
> > 
> > How about this. Take an extra reference on any pmt_feature_group
> > structures that include a known guid (to keep the accounting right
> > when intel_aet_exit() is called). This simplifies the function so
> > I don't need the __free() handler that confuses checkpatch.pl :-)
> > 
> > 
> > /*
> >  * 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 element of
> >  * the array describes one telemetry aggregator.
> >  * A single pmt_feature_group may include multiple different guids.
> >  * Try to use every telemetry aggregator with a known guid.
> >  */
> > static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> > 			    unsigned int num_evg)
> > {
> > 	struct pmt_feature_group *p = intel_pmt_get_regions_by_feature(feature);
> > 	struct event_group **peg;
> > 	bool ret = false;
> > 
> > 	if (IS_ERR_OR_NULL(p))
> > 		return false;
> > 
> > 	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> > 		if (enable_events(*peg, p)) {
> > 			kref_get(&p->kref);
> 
> This is not clear to me ... would enable_events() still mark all telemetry_regions
> that do not match the event_group's guid as unusable? It seems to me that if more
> than one even_group refers to the same pmt_feature_group then the first one to match
> will "win" and make the other event_group's telemetry regions unusable.

Extra context needed. Sorry.

I'm changing enable_events() to only mark telemetry_regions regions as
unusable if they have a bad package id, or the MMIO size doesn't match.
I.e. they truly are bad.

Mis-match on guid will skip then while associating with a specific
event_gruoup, but leave them as usable.

This means that intel_aet_read_event() now has to check the guid as
well as !addr.

An alternative approach would be to ask the PMT code for separate
copies of the pmt_feature_group to attach to each event_group. I
didn't like this, do you think it would be better?

> 
> > 			(*peg)->pfg = no_free_ptr(p);
> > 			ret = true;
> > 		}
> > 	}
> > 	intel_pmt_put_feature_group(p);
> > 
> > 	return ret;
> > }
> > 
> 
> Reinette
> 

-Tony