Re: [PATCH v13 17/32] x86,fs/resctrl: Fill in details of events for guid 0x26696143 and 0x26557651

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

Hi Tony,

On 10/29/25 9:21 AM, Tony Luck wrote:
> The Intel Clearwater Forest CPU supports two RMID-based PMT feature
> groups documented in the xml/CWF/OOBMSM/RMID-ENERGY/cwf_aggregator.xml and

It makes the series easier to read when it uses consistent terms throughout and
avoids needing to keep redefining things. With "feature type" defined in previous patch
I think "PMT feature group" can be replaced with "feature type" and then the second
and third paragraph can be dropped?

To help keep things consistent this could be something like below that also gives
users a link that works since, for example, appending the provided
"xml/CWF/OOBMSM/RMID-ENERGY/cwf_aggregator.xml" to provided
https://github.com/intel/Intel-PMT results in a "Not Found".

	The telemetry event aggregators of the Intel Clearwater Forest CPU
	support two RMID-based feature types: "energy" with guid 0x26696143 [1], and
	"perf" with guid 0x26557651 [2].

	Link: https://github.com/intel/Intel-PMT/blob/main/xml/CWF/OOBMSM/RMID-ENERGY/cwf_aggregator.xml # [1]
	Link: https://github.com/intel/Intel-PMT/blob/main/xml/CWF/OOBMSM/RMID-PERF/cwf_aggregator.xml # [2]

> xml/CWF/OOBMSM/RMID-PERF/cwf_aggregator.xml files in the Intel PMT GIT repository
> [1].

Taking a step back there seems to be an ordering issue since the previous patch
already uses the two guid "introduced" here. 

> 
> The struct pmt_feature_group provided by the INTEL_PMT_TELEMETRY driver lists the
> guid and other information for each aggregator of a given type (energy or perf)
> present on the system.
> 
> resctrl has a condensed form of the XML description in struct
> event_group. An event group is enabled if the pfg field points
> to a struct pmt_feature_group.

Above two paragraphs can be dropped. (They anyway were just pasted in without maintaining style nor
connection with rest of changelog.)

> 
> The counter offsets in MMIO space are arranged in groups for each RMID.

(to remind reader the context)
"The counter offsets in MMIO space" -> "The event counter offsets in an aggregator's MMIO space"

> 
> E.g the "energy" counters for guid 0x26696143 are arranged like this:
> 
>         MMIO offset:0x0000 Counter for RMID 0 PMT_EVENT_ENERGY
>         MMIO offset:0x0008 Counter for RMID 0 PMT_EVENT_ACTIVITY
>         MMIO offset:0x0010 Counter for RMID 1 PMT_EVENT_ENERGY
>         MMIO offset:0x0018 Counter for RMID 1 PMT_EVENT_ACTIVITY
>         ...
>         MMIO offset:0x23F0 Counter for RMID 575 PMT_EVENT_ENERGY
>         MMIO offset:0x23F8 Counter for RMID 575 PMT_EVENT_ACTIVITY
> 
> After all counters there are three status registers that provide
> indications of how many times an aggregator was unable to process
> event counts, the time stamp for the most recent loss of data, and
> the time stamp of the most recent successful update.
> 
> 	MMIO offset:0x2400 AGG_DATA_LOSS_COUNT
> 	MMIO offset:0x2408 AGG_DATA_LOSS_TIMESTAMP
> 	MMIO offset:0x2410 LAST_UPDATE_TIMESTAMP
> 
> Define these events in the file system code and add the events

(to be specific about which events)

"these events" -> "the events tracked by the aggregators"?

> to the event_group structures.
> 
> PMT_EVENT_ENERGY and PMT_EVENT_ACTIVITY are produced in fixed point
> format. File system code must output as floating point values.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> Link: https://github.com/intel/Intel-PMT # [1]
> ---

I think it will be more appropriate if the event groups initialized here,
for example "static struct event_group energy_0x26696143", are defined
as part of this patch also.

Reinette