Re: [PATCH v3 14/26] x86/resctrl: Add first part of telemetry event enumeration

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

Hi Tony,

The following three patches progress as follows:
x86/resctrl: Add first part ...
x86/resctrl: Second stage ...
x86/resctrl: Third phase ...

Could you please make the language consistent?

On 4/7/25 4:40 PM, Tony Luck wrote:
> The OOBMSM driver provides an interface to discover any RMID
> based events for "energy" and "perf" classes.

Please add context about what an RMID based event is.

> 
> Hold onto references to any pmt_feature_groups that resctrl

Please add context about what a 
"pmt_feature_groups" (intended to be pmt_feature_group?) is.

> uses until resctrl exit.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h  |  8 ++++
>  arch/x86/kernel/cpu/resctrl/core.c      |  5 ++
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 62 +++++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/Makefile    |  1 +
>  4 files changed, 76 insertions(+)
>  create mode 100644 arch/x86/kernel/cpu/resctrl/intel_aet.c
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 45eabc7919c6..70b63bbc429d 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -172,4 +172,12 @@ void __init intel_rdt_mbm_apply_quirk(void);
>  
>  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);
> +#else
> +static inline bool intel_aet_get_events(void) { return false; }
> +static inline void intel_aet_exit(void) { };
> +#endif
> +
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index a066a9c54a1f..f0f256a5ac66 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -718,6 +718,9 @@ void resctrl_arch_mount(void)
>  	if (only_once)
>  		return;
>  	only_once = true;
> +
> +	if (!intel_aet_get_events())

hmmm ... keep in mind that this is called without
any locking and thus there may be risk of parallel calls?

Please document how/if this is relying on some fs features to 
ensure there are not two instances running at same time.

> +		return;
>  }
>  
>  enum {
> @@ -1063,6 +1066,8 @@ late_initcall(resctrl_arch_late_init);
>  
>  static void __exit resctrl_arch_exit(void)
>  {
> +	intel_aet_exit();
> +
>  	cpuhp_remove_state(rdt_online);
>  
>  	resctrl_exit();
> 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..8e531ad279b5
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -0,0 +1,62 @@
> +// 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/cpu.h>
> +#include <linux/cleanup.h>
> +#include "fake_intel_aet_features.h"
This include can be marked as "Temporary" to highlight that it
will not stay.

Please separate headers into blocks and sort alphabetically.

> +#include <linux/intel_vsec.h>
> +#include <linux/resctrl.h>
> +#include <linux/slab.h>

Are all these headers used in code below?

> +
> +#include "internal.h"
> +
> +static struct pmt_feature_group *feat_energy;
> +static struct pmt_feature_group *feat_perf;
> +
> +DEFINE_FREE(intel_pmt_put_feature_group, struct pmt_feature_group *,	\
> +	if (!IS_ERR_OR_NULL(_T))					\
> +		intel_pmt_put_feature_group(_T))
> +
> +/*
> + * Ask OOBMSM discovery driver for all the RMID based telemetry groups
> + * that it supports.
> + */
> +bool intel_aet_get_events(void)
> +{
> +	struct pmt_feature_group *p1 __free(intel_pmt_put_feature_group) = NULL;
> +	struct pmt_feature_group *p2 __free(intel_pmt_put_feature_group) = NULL;
> +	bool use_p1, use_p2;
> +
> +	p1 = intel_pmt_get_regions_by_feature(FEATURE_PER_RMID_ENERGY_TELEM);
> +	p2 = intel_pmt_get_regions_by_feature(FEATURE_PER_RMID_PERF_TELEM);
> +	use_p1 = !IS_ERR_OR_NULL(p1);
> +	use_p2 = !IS_ERR_OR_NULL(p2);
> +
> +	if (!use_p1 && !use_p2)
> +		return false;
> +
> +	if (use_p1)
> +		feat_energy = no_free_ptr(p1);
> +	if (use_p2)
> +		feat_perf = no_free_ptr(p2);

This reminds me of something I read recently .... "There's a rule in computer
programming that objects appear zero, once, or many times. So code accordingly."
Not expecting a change .... just finding it amusing.

> +
> +	return true;
> +}
> +
> +void __exit intel_aet_exit(void)
> +{
> +	if (feat_energy)
> +		intel_pmt_put_feature_group(feat_energy);
> +	if (feat_perf)
> +		intel_pmt_put_feature_group(feat_perf);
> +}
> diff --git a/arch/x86/kernel/cpu/resctrl/Makefile b/arch/x86/kernel/cpu/resctrl/Makefile
> index c56d3acf8ac7..74c3b2333dde 100644
> --- a/arch/x86/kernel/cpu/resctrl/Makefile
> +++ b/arch/x86/kernel/cpu/resctrl/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_X86_CPU_RESCTRL)		+= core.o rdtgroup.o monitor.o
>  obj-$(CONFIG_X86_CPU_RESCTRL)		+= ctrlmondata.o
> +obj-$(CONFIG_INTEL_AET_RESCTRL)		+= intel_aet.o
>  obj-$(CONFIG_INTEL_AET_RESCTRL)		+= fake_intel_aet_features.o
>  obj-$(CONFIG_RESCTRL_FS_PSEUDO_LOCK)	+= pseudo_lock.o
>  

Reinette