linux-kernel - Re: [PATCH v9 20/31] x86/resctrl: Find and enable usable 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: <9ac43e78-8955-db5d-61be-e08008e41f0d@linux.intel.com>
Date: Mon, 1 Sep 2025 11:58:48 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Tony Luck <tony.luck@...el.com>
cc: Fenghua Yu <fenghuay@...dia.com>, 
    Reinette Chatre <reinette.chatre@...el.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, LKML <linux-kernel@...r.kernel.org>, 
    patches@...ts.linux.dev
Subject: Re: [PATCH v9 20/31] x86/resctrl: Find and enable usable telemetry
 events

On Fri, 29 Aug 2025, Tony Luck wrote:

> The INTEL_PMT driver provides telemetry region structures of the
> types requested by resctrl.
> 
> Scan these structures to discover which pass sanity checks:
> 
> 1) They have guid known to resctrl.
> 2) They have a valid package ID.
> 3) The enumerated size of the MMIO region matches the expected
>    value from the XML description file.
> 4) At least one region passes the above checks.
> 
> Enable the active events in resctrl filesystem to make them available to
> user space. Pass a pointer to the pmt_event structure of the event within
> the struct event_group that resctrl stores in mon_evt::arch_priv. resctrl
> passes this pointer back when asking to read the event data which enables
> the data to be found in MMIO.
> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 36 +++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 565777841f5c..5c5466dc3189 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -102,12 +102,44 @@ static struct event_group *known_perf_event_groups[] = {
>  	for (_peg = _grp; _peg < &_grp[ARRAY_SIZE(_grp)]; _peg++)	\
>  		if ((*_peg)->pfg)
>  
> -/* Stub for now */
> -static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
> +static bool skip_this_region(struct telemetry_region *tr, struct event_group *e)

skip_telem_region? It would get rid of vague "this" in the name.

>  {
> +	if (tr->guid != e->guid)
> +		return true;
> +	if (tr->plat_info.package_id >= topology_max_packages()) {
> +		pr_warn_once("Bad package %d in guid 0x%x\n", tr->plat_info.package_id,
> +			     tr->guid);
> +		return true;
> +	}
> +	if (tr->size != e->mmio_size) {
> +		pr_warn_once("MMIO space wrong size (%zu bytes) for guid 0x%x. Expected %zu bytes.\n",
> +			     tr->size, e->guid, e->mmio_size);

Are _once warranted in these two cases? Do we call this function for the 
same guid and region combination more than once?

+ include for pr_*().

> +		return true;
> +	}
> +
>  	return false;
>  }
>  
> +static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
> +{
> +	bool usable_events = false;
> +
> +	for (int i = 0; i < p->count; i++) {
> +		if (skip_this_region(&p->regions[i], e))
> +			continue;
> +		usable_events = true;

Do you need to loop beyond asserting this?

> +	}
> +
> +	if (!usable_events)
> +		return false;
> +
> +	for (int j = 0; j < e->num_events; j++)
> +		resctrl_enable_mon_event(e->evts[j].id, true,
> +					 e->evts[j].bin_bits, &e->evts[j]);
> +
> +	return true;
> +}
> +
>  DEFINE_FREE(intel_pmt_put_feature_group, struct pmt_feature_group *,
>  		if (!IS_ERR_OR_NULL(_T))
>  			intel_pmt_put_feature_group(_T))
> 

-- 
 i.