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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a011002c-a4af-4ae5-8cd1-a1180a0f62c2@intel.com>
Date: Wed, 8 Oct 2025 10:12:28 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...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

Hi Tony,

On 10/7/25 1:47 PM, Luck, Tony wrote:
> On Mon, Oct 06, 2025 at 02:47:15PM -0700, Luck, Tony wrote:
>> On Mon, Oct 06, 2025 at 02:33:00PM -0700, Reinette Chatre wrote:
>>> Hi Tony,
>>>> 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?
> 
> Working through more patches in the series, I've come to the one
> that adjusts the number of RMIDs.  The alternative approach of

I see, with the number of RMIDs a property of the event group self this
seems reasonable. While there is duplication of pmt_feature_group I am not
able to tell if this is a big issue since I am not clear on how/if systems
will be built this way.

> having a separate copy of the pmt_feature_group is suddently looking
> more attractive.
> 
> So the code would become:
> 
> 
> static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> 			    unsigned int num_evg)
> {
> 	struct pmt_feature_group *p;
> 	struct event_group **peg;
> 	bool ret = false;
> 
> 	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> 		p = intel_pmt_get_regions_by_feature(feature);
> 		if (IS_ERR_OR_NULL(p))
> 			return false;
> 
> 		if (enable_events(*peg, p)) {
> 			(*peg)->pfg = p;
> 			ret = true;
> 		} else {
> 			intel_pmt_put_feature_group(p);
> 		}
> 	}
> 	intel_pmt_put_feature_group(p);

I am not able to tell why this "put" is needed? I assume the "put" of a
pmt_feature_group assigned to an event_group will still be done in
intel_aet_exit()?

Reinette