Message-ID: <ce70e96d-7960-4e00-bf26-8f61424e3171@intel.com>
Date: Tue, 2 Dec 2025 08:28:56 -0800
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>, Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v14 24/32] x86/resctrl: Add energy/perf choices to rdt
 boot option

Hi Tony,

On 11/24/25 10:54 AM, Tony Luck wrote:
> Legacy resctrl features are enumerated by X86_FEATURE_* flags. These may be
> overridden by quirks to disable features in the case of errata.  Users can
> use kernel command line options to either disable a feature, or to force
> enable a feature that was disabled by a quirk.
> 
> Provide similar functionality for hardware features that do not have an
> X86_FEATURE_* flag.  Unlike other features that are tied to X86_FEATURE_*
> flags, these are defined by the feature name.

This needs an update. Above describes previous implementation that was a generic
"for hardware features ...." while the new implementation is specific to telemetry.
(also needs imperative)

> 
> Users may force a feature to be disabled. E.g. "rdt=!perf" will ensure that
> none of the perf telemetry events are enabled.
> 
> Resctrl architecture code may disable a feature that does not provide full

The changelog needs to be clear on what it means by "a feature". Above implies
that "a feature" is all event groups with the same feature type ... but the final
paragraph implies that a feature is a specific event group.

> functionality. Users may override that decision.  E.g. "rdt=energy" will
> enable any available energy telemetry events even if they do not provide
> full functionality.
> 
> An optional guid can be included for more granular control of features sharing

"User can specify an optional guid ..."

> a name. E.g. rdt=energy:0x12345 will only override disabling of the energy
> feature with guid = 0x12345.

New implementation information just slapped onto the end of changelog.

> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  6 ++-
>  arch/x86/kernel/cpu/resctrl/internal.h        |  2 +
>  arch/x86/kernel/cpu/resctrl/core.c            |  2 +
>  arch/x86/kernel/cpu/resctrl/intel_aet.c       | 37 +++++++++++++++++++
>  4 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 8c5636a120ee..e47def4a3dd8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6207,9 +6207,13 @@
>  	rdt=		[HW,X86,RDT]
>  			Turn on/off individual RDT features. List is:
>  			cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
> -			mba, smba, bmec, abmc, sdciae.
> +			mba, smba, bmec, abmc, sdciae, energy[:guid],
> +			perf[:guid].
>  			E.g. to turn on cmt and turn off mba use:
>  				rdt=cmt,!mba
> +			To turn off all energy features and ensure that
> +			the 0x12345 perf feature is enabled use:
> +				rdt=!energy,perf:0x12345

The short "energy" and "perf" is reasonable to use as parameters but for help
text I think it will be better to use "energy telemetry" and "perf telemetry". How should
user interpret "energy features"? While "features" may be ok as a general
term the example is not required to use it since it is specific to telemetry monitoring. 
How about something like "all energy telemetry monitoring" and "perf telemetry monitoring
associated with guid 0x12345"

>  
>  	reboot=		[KNL]
>  			Format (x86 or x86_64):
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 0b7f8317be14..304e6e341905 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -236,6 +236,7 @@ void __exit intel_aet_exit(void);
>  int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64 *val);
>  void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
>  				struct list_head *add_pos);
> +bool intel_aet_option(bool force_off, char *tok);
>  #else
>  static inline bool intel_aet_get_events(void) { return false; }
>  static inline void __exit intel_aet_exit(void) { }
> @@ -246,6 +247,7 @@ static inline int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64
>  
>  static inline void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
>  					      struct list_head *add_pos) { }
> +static inline bool intel_aet_option(bool force_off, char *tok) { return false; }
>  #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 283d653002a2..960974ffa866 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -820,6 +820,8 @@ static int __init set_rdt_options(char *str)
>  		force_off = *tok == '!';
>  		if (force_off)
>  			tok++;
> +		if (intel_aet_option(force_off, tok))
> +			continue;
>  		for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
>  			if (strcmp(tok, o->name) == 0) {
>  				if (force_off)
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 46c64419ec10..50c8b4c50790 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -57,12 +57,16 @@ struct pmt_event {
>   * 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.
> + * @name:		Name for this group (used by boot rdt= option)

This needs a new definition since multiple groups can have the same name now.

>   * @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.
>   *			Valid if the system supports the event group.
>   *			NULL otherwise.
> + * @force_off:		True when "rdt" command line disables this @guid.

Per changelog the architecture can also disable a feature.

> + * @force_on:		True when "rdt" command line overrides disable of
> + *			this @guid due to insufficient @num_rmid.

The idea of "insufficient RMID" does not exist at this point.

>   * @guid:		Unique number per XML description file.
>   * @mmio_size:		Number of bytes of MMIO registers for this group.
>   * @num_events:		Number of events in this group.
> @@ -71,7 +75,9 @@ struct pmt_event {
>  struct event_group {
>  	/* Data fields for additional structures to manage this group. */
>  	enum pmt_feature_id		feature;
> +	char				*name;
>  	struct pmt_feature_group	*pfg;
> +	bool				force_off, force_on;
>  
>  	/* Remaining fields initialized from XML file. */
>  	u32				guid;
> @@ -89,6 +95,7 @@ struct event_group {
>   */
>  static struct event_group energy_0x26696143 = {
>  	.feature	= FEATURE_PER_RMID_ENERGY_TELEM,
> +	.name		= "energy",
>  	.guid		= 0x26696143,
>  	.mmio_size	= XML_MMIO_SIZE(576, 2, 3),
>  	.num_events	= 2,
> @@ -104,6 +111,7 @@ static struct event_group energy_0x26696143 = {
>   */
>  static struct event_group perf_0x26557651 = {
>  	.feature	= FEATURE_PER_RMID_PERF_TELEM,
> +	.name		= "perf",
>  	.guid		= 0x26557651,
>  	.mmio_size	= XML_MMIO_SIZE(576, 7, 3),
>  	.num_events	= 7,
> @@ -128,6 +136,32 @@ static struct event_group *known_event_groups[] = {
>  	     _peg < &known_event_groups[ARRAY_SIZE(known_event_groups)];	\
>  	     _peg++)
>  
> +bool intel_aet_option(bool force_off, char *tok)
> +{
> +	struct event_group **peg;
> +	bool ret = false;
> +	u32 guid = 0;
> +	char *name;
> +
> +	name = strsep(&tok, ":");
> +	if (tok && kstrtou32(tok, 16, &guid))
> +		return false;
> +
> +	for_each_event_group(peg) {
> +		if (strcmp(name, (*peg)->name))
> +			continue;
> +		if (guid && (*peg)->guid != guid)
> +			continue;
> +		if (force_off)
> +			(*peg)->force_off = true;
> +		else
> +			(*peg)->force_on = true;
> +		ret = true;
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * Clear the address field of regions that did not pass the checks in
>   * skip_telem_region() so they will not be used by intel_aet_read_event().
> @@ -178,6 +212,9 @@ static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
>  {
>  	int skipped_events = 0;
>  
> +	if (e->force_off)
> +		return false;
> +
>  	if (!group_has_usable_regions(e, p))
>  		return false;
>  

Reinette

Powered by blists - more mailing lists