Re: [PATCH v6 24/30] x86/resctrl: Add energy/perf choices to rdt boot option

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

Hi Tony,

On 6/26/25 9:49 AM, Tony Luck wrote:
> Hardware backed 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 software defined features that do not
> have an X86_FEATURE_* flag.
> 
> Unlike other options that are tied to X86_FEATURE_* flags, these must be
> queried by name. Add rdt_is_software_feature_enabled() to check whether
> quirks or kernel command line have disabled a feature. Just like the
> hardware feature options the command line enable overrides quirk disable.

Referring to "Intel Application Energy Telemetry" as a software feature does
not sound right. It is a hardware feature, no? Perhaps just "a hardware feature
that does not have a X86_FEATURE_* flag"? With this rdt_is_software_feature_enabled()
could perhaps just be rdt_is_feature_enabled() (open to other ideas)?

The idea that a "software feature" may have a "quirk" also sounds wrong ...
to me that sounds like a different way of saying a "software bug" and we do not
handle software bugs with quirks. Referring to it as a "software feature" just
does not sound right.

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  2 +-
>  arch/x86/kernel/cpu/resctrl/internal.h        |  2 ++
>  arch/x86/kernel/cpu/resctrl/core.c            | 30 +++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/intel_aet.c       |  7 +++++
>  4 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f1f2c0874da9..4c12159f3ea0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6066,7 +6066,7 @@
>  	rdt=		[HW,X86,RDT]
>  			Turn on/off individual RDT features. List is:
>  			cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
> -			mba, smba, bmec.
> +			mba, smba, bmec, energy, perf.
>  			E.g. to turn on cmt and turn off mba use:
>  				rdt=cmt,!mba
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e8d2a754bc0c..ee1c6204722e 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -169,6 +169,8 @@ void __init intel_rdt_mbm_apply_quirk(void);
>  
>  void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  
> +bool rdt_is_software_feature_enabled(char *option);
> +
>  bool intel_aet_get_events(void);
>  void __exit intel_aet_exit(void);
>  int intel_aet_read_event(int domid, int rmid, enum resctrl_event_id evtid,
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index f857f92e7b8b..f9f3bc58290e 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -791,6 +791,8 @@ enum {
>  	RDT_FLAG_MBA,
>  	RDT_FLAG_SMBA,
>  	RDT_FLAG_BMEC,
> +	RDT_FLAG_ENERGY,
> +	RDT_FLAG_PERF,
>  };
>  
>  #define RDT_OPT(idx, n, f)	\
> @@ -816,6 +818,8 @@ static struct rdt_options rdt_options[]  __ro_after_init = {
>  	RDT_OPT(RDT_FLAG_MBA,	    "mba",	X86_FEATURE_MBA),
>  	RDT_OPT(RDT_FLAG_SMBA,	    "smba",	X86_FEATURE_SMBA),
>  	RDT_OPT(RDT_FLAG_BMEC,	    "bmec",	X86_FEATURE_BMEC),
> +	RDT_OPT(RDT_FLAG_ENERGY,    "energy",	0),
> +	RDT_OPT(RDT_FLAG_PERF,	    "perf",	0),
>  };
>  #define NUM_RDT_OPTIONS ARRAY_SIZE(rdt_options)
>  
> @@ -865,6 +869,32 @@ bool rdt_cpu_has(int flag)
>  	return ret;
>  }
>  
> +/*
> + * Software options that are not based on X86_FEATURE_* bits.

"Hardware features that do not have X86_FEATURE_* bits"?

> + * There is no "h/w does not support this at all" case.
> + * Assume that the caller has already determined that s/w

(please expand h/w to hardware, s/w to software)

> + * support is present and just needs to check if the option has been

Looking at how this function is used I rather interpret
intel_pmt_get_regions_by_feature() as determining that hardware
support is present.

> + * disabled by a quirk that has not been overridden * by a command

(stray *)

> + * line option.
> + */
> +bool rdt_is_software_feature_enabled(char *name)
> +{

Reinette