Re: [PATCH v9 07/12] x86/resctrl: Add PMT registration API for AET enumeration callbacks

[Reinette Chatre]

Hi Tony,

On 7/1/26 2:35 PM, Tony Luck wrote:
> resctrl is always built-in; INTEL_PMT_TELEMETRY may be a module.  Add, and
> export, register/unregister functions so the PMT module can supply/clear
> enumeration callback functions when loaded/unloaded.
> 
> Suggested-by: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
>  arch/x86/include/asm/resctrl.h          | 22 ++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 34 +++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index a9f481b7a8ed..48ad55bcda66 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -4,6 +4,8 @@
>  
>  #ifdef CONFIG_X86_CPU_RESCTRL
>  
> +#include <linux/intel_pmt_features.h>
> +#include <linux/intel_vsec.h>
>  #include <linux/jump_label.h>
>  #include <linux/percpu.h>
>  #include <linux/resctrl_types.h>
> @@ -189,11 +191,31 @@ static inline void resctrl_arch_mon_ctx_free(struct rdt_resource *r,
>  
>  void resctrl_cpu_detect(struct cpuinfo_x86 *c);
>  
> +#ifdef CONFIG_X86_CPU_RESCTRL_INTEL_AET
> +void intel_aet_register_enumeration(struct module *module,
> +				    struct pmt_feature_group *(*get)(enum pmt_feature_id id),
> +				    void (*put)(struct pmt_feature_group *p));
> +void intel_aet_unregister_enumeration(void);
>  #else
> +static inline void intel_aet_register_enumeration(struct module *module,
> +						  struct pmt_feature_group *(*get)(enum pmt_feature_id id),
> +						  void (*put)(struct pmt_feature_group *p)) { }
> +static inline void intel_aet_unregister_enumeration(void) { }
> +#endif /* CONFIG_X86_CPU_RESCTRL_INTEL_AET */
> +
> +#else
> +
> +#include <linux/intel_pmt_features.h>
> +#include <linux/intel_vsec.h>

Are we sure we want to include these in arch/x86/kernel/process_32.c and
arch/x86/kernel/process_64.c when resctrl is not built in?

>  
>  static inline void resctrl_arch_sched_in(struct task_struct *tsk) {}
>  static inline void resctrl_cpu_detect(struct cpuinfo_x86 *c) {}
>  
> +static inline void intel_aet_register_enumeration(struct module *module,
> +						  struct pmt_feature_group *(*get)(enum pmt_feature_id id),
> +						  void (*put)(struct pmt_feature_group *p)) { }
> +static inline void intel_aet_unregister_enumeration(void) { }
> +
>  #endif /* CONFIG_X86_CPU_RESCTRL */
>  
>  #endif /* _ASM_X86_RESCTRL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index cd8257c58f84..c26bd813e057 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -12,17 +12,21 @@
>  #define pr_fmt(fmt)   "resctrl: " fmt
>  
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/compiler_types.h>
>  #include <linux/container_of.h>
>  #include <linux/cpumask.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
> +#include <linux/export.h>
>  #include <linux/gfp_types.h>
>  #include <linux/init.h>
>  #include <linux/intel_pmt_features.h>
>  #include <linux/intel_vsec.h>
>  #include <linux/io.h>
>  #include <linux/minmax.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
>  #include <linux/printk.h>
>  #include <linux/rculist.h>
>  #include <linux/rcupdate.h>
> @@ -291,6 +295,10 @@ static enum pmt_feature_id lookup_pfid(const char *pfname)
>  	return FEATURE_INVALID;
>  }
>  
> +static struct module *pmt_module;
> +static struct pmt_feature_group *(*get_feature)(enum pmt_feature_id id);
> +static void (*put_feature)(struct pmt_feature_group *p);
> +
>  /*
>   * 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,
> @@ -325,6 +333,32 @@ bool intel_aet_get_events(void)
>  	return ret;
>  }
>  
> +/*
> + * Defend against races between pmt_telemetry register/unregister and
> + * resctrl file system mount/unmount.
> + */

Please help developers understand what this mutex protects. I am not able to deduce
from above when this mutex needs to be taken.

> +static DEFINE_MUTEX(aet_register_lock);
> +
> +void intel_aet_register_enumeration(struct module *module,
> +				    struct pmt_feature_group *(*get)(enum pmt_feature_id id),
> +				    void (*put)(struct pmt_feature_group *p))
> +{
> +	guard(mutex)(&aet_register_lock);
> +	get_feature = get;
> +	put_feature = put;
> +	pmt_module = module;
> +}
> +EXPORT_SYMBOL_NS_GPL(intel_aet_register_enumeration, "INTEL_PMT");
> +
> +void intel_aet_unregister_enumeration(void)
> +{
> +	guard(mutex)(&aet_register_lock);
> +	pmt_module = NULL;
> +	get_feature = NULL;
> +	put_feature = NULL;
> +}
> +EXPORT_SYMBOL_NS_GPL(intel_aet_unregister_enumeration, "INTEL_PMT");
> +
>  void __exit intel_aet_exit(void)
>  {
>  	struct event_group **peg;

Reinette