Re: [PATCH v14 16/32] x86/resctrl: Discover hardware telemetry events

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

Hi Tony,

On 11/24/25 10:53 AM, Tony Luck wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> new file mode 100644
> index 000000000000..5d34c7349b02
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Resource Director Technology(RDT)
> + * - Intel Application Energy Telemetry
> + *
> + * Copyright (C) 2025 Intel Corporation
> + *
> + * Author:
> + *    Tony Luck <tony.luck@intel.com>
> + */
> +
> +#define pr_fmt(fmt)   "resctrl: " fmt
> +
> +#include <linux/array_size.h>
> +#include <linux/cleanup.h>
> +#include <linux/cpu.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/intel_pmt_features.h>
> +#include <linux/intel_vsec.h>
> +#include <linux/overflow.h>
> +#include <linux/resctrl.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>

Does this patch need all these header files? For example, cleanup.h, overflow.h, and
cpu.h do not seem necessary?

> +
> +#include "internal.h"
> +
> +/**
> + * struct event_group - Events with the same feature type ("energy" or "perf") and guid.
> + * @feature:		Type of events, for example FEATURE_PER_RMID_PERF_TELEM or
> + *			FEATURE_PER_RMID_ENERGY_TELEM, in this group.
> + * @pfg:		Points to the aggregated telemetry space information
> + *			returned by the intel_pmt_get_regions_by_feature()
> + *			call to the INTEL_PMT_TELEMETRY driver that contains
> + *			data for all telemetry regions type @feature.

"regions type @feature" -> "regions of type @feature" or "regions with type @feature"?

(if this sounds familiar ... suggestion was "of type @feature" in v13)

> + *			Valid if the system supports the event group.
> + *			NULL otherwise.

nit: "Valid if the system supports the event group, NULL otherwise."

> + */
> +struct event_group {
> +	/* Data fields for additional structures to manage this group. */
> +	enum pmt_feature_id		feature;
> +	struct pmt_feature_group	*pfg;
> +};
> +
> +static struct event_group *known_event_groups[] = {
> +};
> +
> +#define for_each_event_group(_peg)						\
> +	for (_peg = known_event_groups;						\
> +	     _peg < &known_event_groups[ARRAY_SIZE(known_event_groups)];	\
> +	     _peg++)
> +
> +/* Stub for now */
> +static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
> +{
> +	return false;
> +}
> +
> +/*
> + * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
> + * pmt_feature_group for each known feature. If there is one, the returned

This has been changed to "for each event group", no? We seem to keep talking
past each other (me with feedback and you with a new patch series ignoring the feedback)
since I still find this to be describing a previous implementation.
Previous suggestions to v12 are just ignored without reason. If feedback is faulty,
please respond with motivation so that I can understand why instead of just
resubmitting without addressing feedback.
Consider, for example (please improve):
   
    Request a copy of struct pmt_feature_group for each event group. If there is
    one, the returned structure has an array of telemetry_region structures, each
    element of the array describes one telemetry aggregator. The telemetry aggregators     
    may have different guids so obtain duplicate struct pmt_feature_group       
    for event groups with same feature type but different guid. Post-processing 
    ensures an event group can only use the telemetry aggregators that match its
    guid. An event group keeps a pointer to its struct pmt_feature_group to indicate
    that its events are successfully enabled.                   

> + * structure has an array of telemetry_region structures, each element of
> + * the array describes one telemetry aggregator.
> + * A single pmt_feature_group may include multiple different guids.
> + * Save the pmt_feature_group for enabled events.
> + */
> +bool intel_aet_get_events(void)
> +{

Reinette