Hi Tony,
On 4/28/25 5:33 PM, Tony Luck wrote:
> Counters for telemetry events are in MMIO space. Each telemetry_region
> structure returned in the pmt_feature_group returned from OOBMSM
> contains the base MMIO address for the counters.
>
> Scan all the telemetry_region structures again and gather these
> addresses into a more convenient structure with addresses for
> each aggregator indexed by package id. Note that there may be
> multiple aggregators per package.
Could this series please provide a clear definition for "telemetry
region" and "aggregator" and then use the terms consistently?
I find the comments to switch between the two causing confusion.
>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> arch/x86/kernel/cpu/resctrl/intel_aet.c | 55 +++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index a0365c3ce982..03839d5c369b 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -20,6 +20,16 @@
>
> #include "internal.h"
>
> +/**
> + * struct mmio_info - Array of MMIO addresses for a package
Please add description on how this structure is used. Please use
these docs to help readers create a mental model of how these
data structures fit together.
I am making an attempt at an example below but I am still trying to
understand how things fit together so would appreciate if you
write this instead. (Please consider this when viewing any of the
samples I provide.)
Example,
Array of MMIO addresses of one event group for a package.
Provides convenient access to all MMIO addresses of
one event group for one package. Used when reading
event data on a package. (needs improvement)
> + * @count: Number of addresses on this package
Any information on what this number means? For example,
"Number of telemetry regions of a specific event group."
> + * @addrs: The MMIO addresses
Can the layout of MMIO space be added to the comments?
> + */
> +struct mmio_info {
> + int count;
> + void __iomem *addrs[] __counted_by(count);
> +};
> +
> /**
> * struct event_group - All information about a group of telemetry events.
> * Some fields initialized with MMIO layout information
> @@ -27,10 +37,12 @@
> * retrieved from intel_pmt_get_regions_by_feature().
> * @pfg: The pmt_feature_group for this event group
> * @guid: Unique number per XML description file
> + * @pkginfo: Per-package MMIO addresses
"Per-package MMIO addresses of telemetry regions belonging to this group."?
> */
> struct event_group {
> struct pmt_feature_group *pfg;
> int guid;
> + struct mmio_info **pkginfo;
> };
>
> /* Link: https://github.com/intel/Intel-PMT xml/CWF/OOBMSM/RMID-ENERGY *.xml */
> @@ -50,12 +62,33 @@ static struct event_group *known_event_groups[] = {
>
> #define NUM_KNOWN_GROUPS ARRAY_SIZE(known_event_groups)
>
> +static void free_mmio_info(struct mmio_info **mmi)
> +{
> + int num_pkgs = topology_max_packages();
> +
> + if (!mmi)
> + return;
> +
> + for (int i = 0; i < num_pkgs; i++)
> + kfree(mmi[i]);
> + kfree(mmi);
> +}
> +
> +DEFINE_FREE(mmio_info, struct mmio_info **, free_mmio_info(_T))
> +
> static bool configure_events(struct event_group *e, struct pmt_feature_group *p)
> {
> + struct mmio_info __free(mmio_info) **pkginfo = NULL;
> int *pkgcounts __free(kfree) = NULL;
> struct telemetry_region *tr;
> + struct mmio_info *mmi;
> int num_pkgs;
>
> + if (e->pkginfo) {
> + pr_warn("Duplicate telemetry information for guid 0x%x\n", e->guid);
> + return false;
> + }
> +
> num_pkgs = topology_max_packages();
> pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
> if (!pkgcounts)
> @@ -73,6 +106,27 @@ static bool configure_events(struct event_group *e, struct pmt_feature_group *p)
> pkgcounts[tr->plat_info.package_id]++;
> }
>
> + /* Allocate per-package arrays and save MMIO addresses */
per-package arrays of what?
> + pkginfo = kcalloc(num_pkgs, sizeof(*pkginfo), GFP_KERNEL);
> + if (!pkginfo)
> + return false;
-ENOMEM
> + for (int i = 0; i < num_pkgs; i++) {
> + pkginfo[i] = kmalloc(struct_size(pkginfo[i], addrs, pkgcounts[i]), GFP_KERNEL);
kzalloc()
> + if (!pkginfo[i])
> + return false;
-ENOMEM
> + pkginfo[i]->count = pkgcounts[i];
> + }
> +
> + /* Save MMIO address(es) for each aggregator in per-package structures */
Should "aggregator" be "telemetry region"? It is becoming confusing what "aggregator"
vs "telemetry region" represents here.
> + for (int i = 0; i < p->count; i++) {
> + tr = &p->regions[i];
> + if (tr->guid != e->guid || tr->plat_info.package_id >= num_pkgs)
> + continue;
> + mmi = pkginfo[tr->plat_info.package_id];
> + mmi->addrs[--pkgcounts[tr->plat_info.package_id]] = tr->addr;
For this code to be safe the "if()" checks that precede it must match *exactly*
with the checks used to initialize the pkgcounts array. To ensure this remains the
case I think those checks need to be placed in a function to be called in both
places.
> + }
> + e->pkginfo = no_free_ptr(pkginfo);
> +
> return false;
> }
>
> @@ -130,5 +184,6 @@ void __exit intel_aet_exit(void)
> intel_pmt_put_feature_group((*peg)->pfg);
> (*peg)->pfg = NULL;
> }
> + free_mmio_info((*peg)->pkginfo);
> }
> }
Reinette