Re: [PATCH v4 26/31] x86/resctrl: Add energy/perf choices to rdt boot option

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

Hi Tony,

On 4/28/25 5:33 PM, Tony Luck wrote:
> Users may want to force either of the telemetry features on
> (in the case where they are disabled due to erratum) or off
> (in the case that a limited number of RMIDs for a telemetry
> feature reduces the number of monitor groups that can be
> created.)
> 
> Unlike other options that are tied to X86_FEATURE_* flags,
> these must be queried by name. Add a function to do that.
> 
> Add checks for users who forced either feature off.
> 
> 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            | 19 +++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/intel_aet.c       |  6 ++++++
>  4 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d9fd26b95b34..4811bc812f0f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5988,7 +5988,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 dd5fe8a98304..92cbba9d82a8 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -167,6 +167,8 @@ void __init intel_rdt_mbm_apply_quirk(void);
>  
>  void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  
> +bool rdt_check_option(char *name, bool is_on, bool is_off);
> +
>  #ifdef CONFIG_INTEL_AET_RESCTRL
>  bool intel_aet_get_events(void);
>  void __exit intel_aet_exit(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 9fa4cc66faf4..dc312e24ab87 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -795,6 +795,8 @@ enum {
>  	RDT_FLAG_MBA,
>  	RDT_FLAG_SMBA,
>  	RDT_FLAG_BMEC,
> +	RDT_FLAG_ENERGY,
> +	RDT_FLAG_PERF,
>  };
>  
>  #define RDT_OPT(idx, n, f)	\
> @@ -820,6 +822,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)
>  
> @@ -869,6 +873,21 @@ bool rdt_cpu_has(int flag)
>  	return ret;
>  }
>  
> +/* Check if a named option has been forced on, or forced off */
> +bool rdt_check_option(char *name, bool is_on, bool is_off)

Please make it obvious what this function does. What does "is_on/is_off" 
parameters represent?
What does it mean when this function returns "true" vs "false"?

Also please reconsider the name of this function to help make it obvious
to reader how to interpret return value.

> +{
> +	struct rdt_options *o;
> +
> +	WARN_ON(!(is_on ^ is_off));
> +
> +	for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
> +		if (!strcmp(name, o->name))
> +			return (is_on && o->force_on) || (is_off && o->force_off);
> +	}
> +
> +	return false;
> +}



> +
>  bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt)
>  {
>  	if (!rdt_cpu_has(X86_FEATURE_BMEC))
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 0bbf991da981..aacaedcc7b74 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -49,6 +49,7 @@ struct pmt_event {
>   *                      gleaned from the XML files. Others are set from data
>   *                      retrieved from intel_pmt_get_regions_by_feature().
>   * @pfg:		The pmt_feature_group for this event group
> + * @name:		Name for this group
>   * @guid:		Unique number per XML description file
>   * @mmio_size:		Number of bytes of mmio registers for this group
>   * @pkginfo:		Per-package MMIO addresses
> @@ -57,6 +58,7 @@ struct pmt_event {
>   */
>  struct event_group {
>  	struct pmt_feature_group	*pfg;
> +	char				*name;
>  	int				guid;
>  	int				mmio_size;
>  	struct mmio_info		**pkginfo;
> @@ -66,6 +68,7 @@ struct event_group {
>  
>  /* Link: https://github.com/intel/Intel-PMT xml/CWF/OOBMSM/RMID-ENERGY *.xml */
>  static struct event_group energy_0x26696143 = {
> +	.name		= "energy",
>  	.guid		= 0x26696143,
>  	.mmio_size	= (576 * 2 + 3) * 8,
>  	.num_events	= 2,
> @@ -77,6 +80,7 @@ static struct event_group energy_0x26696143 = {
>  
>  /* Link: https://github.com/intel/Intel-PMT xml/CWF/OOBMSM/RMID-PERF *.xml */
>  static struct event_group perf_0x26557651 = {
> +	.name		= "perf",
>  	.guid		= 0x26557651,
>  	.mmio_size	= (576 * 7 + 3) * 8,
>  	.num_events	= 7,
> @@ -208,6 +212,8 @@ static bool get_pmt_feature(enum pmt_feature_id feature)
>  	for (peg = &known_event_groups[0]; peg < &known_event_groups[NUM_KNOWN_GROUPS]; peg++) {
>  		for (int i = 0; i < p->count; i++) {
>  			if ((*peg)->guid == p->regions[i].guid) {
> +				if (rdt_check_option((*peg)->name, false, true))

What does the "false" and "true" represent here?

> +					return false;
>  				ret = configure_events((*peg), p);
>  				if (ret) {
>  					(*peg)->pfg = no_free_ptr(p);

Reinette