Re: [PATCH v6 18/30] x86/resctrl: Count valid telemetry aggregators per package

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

On Wed, Jul 09, 2025 at 03:13:20PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 7/9/25 11:12 AM, Luck, Tony wrote:
> > On Tue, Jul 08, 2025 at 07:20:35PM -0700, Reinette Chatre wrote:
> >> As I understand there is 1:1 relationship between struct event_group and struct pmt_feature_group.
> >> It thus seems unnecessary to loop through all the telemetry regions of a struct pmt_feature_group
> >> if it is known to not be associated with the "event group"?
> >> Could it be helpful to add a new (hardcoded) event_group::id that is of type enum pmt_feature_id
> >> that can be used to ensure that only relevant struct pmt_feature_group is used to discover events
> >> for a particular struct event_group?
> >>
> >> Another consideration is that this implementation seems to require that guids are unique across
> >> all telemetry regions of all RMID telemetry features, is this guaranteed?
> > 
> > The guids are unique. The XML file tags them like this:
> > 
> > 	<TELEM:uniqueid>26557651</TELEM:uniqueid>
> 
> I interpret above that guid is expected to be unique for one
> telemetry feature. It is not clear to me that it implies that the guid
> is unique across all telemetry features. For example, what prevents
> a platform from using the same guid for all the telemetry features it
> supports?

There are several non-RMID based telemetry MMIO regions in addition to
the two used by this patch series.

Think of the uniqueid as a signature for the format of the region.
Which event counters are present, in which order? How many total
counters? What is the binary format of each counter?

Or think of it as a key. Usermode telemetry tools that access these MMIO
regions use the uniqueid to choose which XML file to use to interpret
the data. I'm effectively doing this, but without including an XML
parser in the kernel. Just distilling each XML file to the basic
essence described in the event_group structure.

It would be a catastrophic failure if Intel assigned the same uniqueid
to regions that had different formats.

> 
> > 
> > the "guid" naming of the value comes from the Intel PMT_DISCOVERY driver.
> > 
> > An alternative to adding the new event_group::id field would be to
> > separate the arrays of known event groups. I.e. change from:
> > 
> > static struct event_group *known_event_groups[] = {
> >         &energy_0x26696143,
> >         &perf_0x26557651,
> > };
> > 
> > to
> > 
> > static struct event_group *known_energy_event_groups[] = {
> >         &energy_0x26696143,
> > };
> > 
> > static struct event_group *known_perf_event_groups[] = {
> >         &perf_0x26557651,
> > };
> > 
> > then only scan the appropriate array that matches the
> > enum pmt_feature_id passed to get_pmt_feature().
> > 
> > 
> > With only one option in each array today this looks
> > like extra infrasctruture. But I already have a patch
> > for the next generation system that adds another guid.
> 
> This also sounds good. Thank you.

Ok. Thanks. I'll put this idea into code in the next version.

> 
> Reinette
> 

-Tony