Re: [PATCH v4 22/31] x86/resctrl: Read core telemetry events

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

Hi Tony,

On 4/28/25 5:33 PM, Tony Luck wrote:
> The resctrl file system passes requests to read event monitor files to
> the architecture resctrl_arch_rmid_read() function to collect values

nit: no need to say "function" when using ().

> from hardware counters.
> 
> Use the resctrl resource to differentiate between calls to read legacy
> L3 events from the new telemetry events (which are attached to
> RDT_RESOURCE_PERF_PKG).
> 
> There may be multiple devices tracking each package, so scan all of them

"devices" seems to be in the mix of similar term as aggregator and
telemetry regions. Having multiple terms for same/similar thing is confusing.

> and add up all counters.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h  |  5 ++++
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 34 +++++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/monitor.c   |  3 +++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 571db665eca6..dd5fe8a98304 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -170,9 +170,14 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  #ifdef CONFIG_INTEL_AET_RESCTRL
>  bool intel_aet_get_events(void);
>  void __exit intel_aet_exit(void);
> +int intel_aet_read_event(int domid, int rmid, enum resctrl_event_id evtid, u64 *val);
>  #else
>  static inline bool intel_aet_get_events(void) { return false; }
>  static inline void intel_aet_exit(void) { };
> +static inline int intel_aet_read_event(int domid, int rmid, enum resctrl_event_id evtid, u64 *val)
> +{
> +	return -EINVAL;
> +}
>  #endif
>  
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index e1cb6bd4788d..0bbf991da981 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -13,6 +13,7 @@
>  
>  #include <linux/cleanup.h>
>  #include <linux/cpu.h>
> +#include <linux/io.h>
>  #include <linux/resctrl.h>
>  
>  /* Temporary - delete from final version */
> @@ -246,3 +247,36 @@ void __exit intel_aet_exit(void)
>  		free_mmio_info((*peg)->pkginfo);
>  	}
>  }
> +
> +#define VALID_BIT	BIT_ULL(63)
> +#define DATA_BITS	GENMASK_ULL(62, 0)
> +
> +/*
> + * Read counter for an event on a domain (summing all aggregators
> + * on the domain).
> + */
> +int intel_aet_read_event(int domid, int rmid, enum resctrl_event_id evtid, u64 *val)
> +{
> +	struct evtinfo *info = &evtinfo[evtid];
> +	struct mmio_info *mmi;
> +	u64 evtcount;
> +	int idx;
> +
> +	idx = rmid * info->event_group->num_events;
> +	idx += info->idx;
> +	mmi = info->event_group->pkginfo[domid];
> +
> +	if (idx * sizeof(u64) > info->event_group->mmio_size) {

Reading offset "idx * sizeof(u64)" when 
"idx * sizeof(u64) == info->event_group->mmio_size" is overflow, no?
How about (please check):
	if (idx * sizeof(u64) - sizeof(u64) >= info->event_group->mmio_size)
	

> +		pr_warn_once("MMIO index %d out of range\n", idx);
> +		return -EINVAL;

The function's return percolates up to rdtgroup_mondata_show() where
the return code is translated into text: -EINVAL becomes "Unavailable"
and -EIO becomes "Error". Seems like this should be -EIO instead?

> +	}
> +
> +	for (int i = 0; i < mmi->count; i++) {
> +		evtcount = readq(mmi->addrs[i] + idx * sizeof(u64));
> +		if (!(evtcount & VALID_BIT))
> +			return -EINVAL;

What does set of "VALID_BIT" mean? That it is a valid counter or
that the data within is valid?

> +		*val += evtcount & DATA_BITS;
> +	}


> +
> +	return 0;
> +}
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 8d8ec86929fa..04214585824b 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -237,6 +237,9 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_l3_mon_domain *d,
>  
>  	resctrl_arch_rmid_read_context_check();
>  
> +	if (r->rid == RDT_RESOURCE_PERF_PKG)
> +		return intel_aet_read_event(d->hdr.id, rmid, eventid, val);
> +

Please add comment or check that code that follows is for L3 resource

>  	prmid = logical_rmid_to_physical_rmid(cpu, rmid);
>  	ret = __rmid_read_phys(prmid, eventid, &msr_val);
>  	if (ret)

Reinette