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

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

Hi Tony,

On 5/21/25 3:50 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        |  4 +++
>  arch/x86/kernel/cpu/resctrl/core.c            | 28 +++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/intel_aet.c       |  6 ++++
>  4 files changed, 39 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 42da0a222c7c..524f3c183900 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -167,6 +167,10 @@ void __init intel_rdt_mbm_apply_quirk(void);
>  
>  void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  
> +bool rdt_is_option_force_enabled(char *option);
> +
> +bool rdt_is_option_force_disabled(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, u64 *val);
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index f07f5b58639a..b23309566500 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -797,6 +797,8 @@ enum {
>  	RDT_FLAG_MBA,
>  	RDT_FLAG_SMBA,
>  	RDT_FLAG_BMEC,
> +	RDT_FLAG_ENERGY,
> +	RDT_FLAG_PERF,
>  };
>  
>  #define RDT_OPT(idx, n, f)	\
> @@ -822,6 +824,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)
>  
> @@ -871,6 +875,30 @@ bool rdt_cpu_has(int flag)
>  	return ret;
>  }
>  
> +bool rdt_is_option_force_enabled(char *name)
> +{
> +	struct rdt_options *o;
> +
> +	for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
> +		if (!strcmp(name, o->name))
> +			return o->force_on;
> +	}
> +
> +	return false;
> +}
> +
> +bool rdt_is_option_force_disabled(char *name)
> +{
> +	struct rdt_options *o;
> +
> +	for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
> +		if (!strcmp(name, o->name))
> +			return 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 be52c9302a80..c1fc85dbf0d8 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -51,6 +51,7 @@ struct pmt_event {
>  
>  /**
>   * struct event_group - All information about a group of telemetry events.
> + * @name:		Name for this group (used by boot rdt= option)
>   * @pfg:		Points to the aggregated telemetry space information
>   *			within the OOBMSM driver that contains data for all
>   *			telemetry regions.
> @@ -62,6 +63,7 @@ struct pmt_event {
>   */
>  struct event_group {
>  	/* Data fields used by this code. */
> +	char				*name;
>  	struct pmt_feature_group	*pfg;
>  	struct mmio_info		**pkginfo;
>  
> @@ -77,6 +79,7 @@ struct event_group {
>   * File: xml/CWF/OOBMSM/RMID-ENERGY/cwf_aggregator.xml
>   */
>  static struct event_group energy_0x26696143 = {
> +	.name		= "energy",
>  	.guid		= 0x26696143,
>  	.mmio_size	= (576 * 2 + 3) * 8,
>  	.num_events	= 2,
> @@ -91,6 +94,7 @@ static struct event_group energy_0x26696143 = {
>   * File: xml/CWF/OOBMSM/RMID-PERF/cwf_aggregator.xml
>   */
>  static struct event_group perf_0x26557651 = {
> +	.name		= "perf",
>  	.guid		= 0x26557651,
>  	.mmio_size	= (576 * 7 + 3) * 8,
>  	.num_events	= 7,
> @@ -247,6 +251,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_is_option_force_disabled((*peg)->name))
> +					return false;

I do not see how this supports the "erratum" use case claimed in the changelog.

resctrl supports the "erratum" use case for hardware features and how it
works is that resctrl forces a feature off and those features can be forced on by
a user via the rdt= parameter. The changelog claims that this supports the same.

Consider the scenario when resctrl forces a feature off because of erratum,
for example, by doing something like __check_quirks_intel():

	if (/* test if quirk applies */)
		set_rdt_options(!perf);

The above will set "force_off" for the "perf" feature and no matter if the
user boots with "rdt=perf, the above rdt_is_option_force_disabled() will
return true since it does not check "force_on" that is set via kernel parameter.
Thus it is not possible for user to enable a feature that is forced off
because of erratum.

Note how rdt_cpu_has() deals with this:
		if (o->force_off)
			ret = false;
		if (o->force_on)
			ret = true;

rdt_cpu_has() deals with the "erratum" use case by first checking for "force_off"
that will capture the disable due to erratum, but then allows that to be changed
by checking "force_on" next and change the setting.

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

Reinette