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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aOQ44WJMXweGNlL2@agluck-desk3>
Date: Mon, 6 Oct 2025 14:47:13 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
CC: Fenghua Yu <fenghuay@...dia.com>, Maciej Wieczor-Retman
	<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>, "Drew
 Fustini" <dfustini@...libre.com>, Dave Martin <Dave.Martin@....com>, Chen Yu
	<yu.c.chen@...el.com>, <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v11 14/31] x86/resctrl: Discover hardware telemetry events

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