linux-kernel - Re: [PATCH v4 17/31] x86/resctrl: Add second part of telemetry event enumeration

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: <74c45403-ae57-440e-8a49-d058bae9c21a@intel.com>
Date: Thu, 8 May 2025 08:54:07 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, 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>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>,
	Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v4 17/31] x86/resctrl: Add second part of telemetry event
 enumeration

Hi Tony,

On 4/28/25 5:33 PM, Tony Luck wrote:
> There may be multiple telemetry aggregators per package, each enumerated
> by a telemetry region structure in the feature group.
> 
> Scan the array of telemetry region structures and count how many are
> in each package in preparation to allocate structures to save the MMIO
> addresses for each in a convenient format for use when reading event
> counters.

Note that reader does not know at this point that the subsequent processing
will be done by further expanding configure_events() or via a new function
called by get_pmt_feature() after configure_events() completes. Without knowing this
this patch looks buggy since it seems to forget to save a pointer to
initialized data.

> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index dda44baf75ae..a0365c3ce982 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -52,6 +52,27 @@ static struct event_group *known_event_groups[] = {
>  
>  static bool configure_events(struct event_group *e, struct pmt_feature_group *p)
>  {
> +	int *pkgcounts __free(kfree) = NULL;
> +	struct telemetry_region *tr;
> +	int num_pkgs;
> +
> +	num_pkgs = topology_max_packages();
> +	pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
> +	if (!pkgcounts)
> +		return false;

-ENOMEM

> +
> +	/* Get per-package counts of telemetry_region for this guid */

How should "telemetry_region" be interpreted? If this is intended to refer
to the individual structs then it should be "counts of telemetry_region structs",
if it is intended to refer to what the structs represent, it can be "telemetry
regions"

Also, it is not obvious what is mean with "this guid" ... there are two guids in
snippet below.

> +	for (int i = 0; i < p->count; i++) {
> +		tr = &p->regions[i];
> +		if (tr->guid != e->guid)
> +			continue;
> +		if (tr->plat_info.package_id >= num_pkgs) {
> +			pr_warn_once("Bad package %d\n", tr->plat_info.package_id);
> +			continue;
> +		}
> +		pkgcounts[tr->plat_info.package_id]++;
> +	}
> +
>  	return false;

configure_events() returns false on success and failure? Perhaps this is temporary
until all parsing has been implemented but that is another thing that reader needs
to guess now or look at later patches to understand.

>  }
>  

Reinette