Re: [PATCH v6 8/9] x86/resctrl: Introduce erdt_cpu_has() and erdt_support()

[Reinette Chatre]

Hi Chenyu,

On 7/25/26 2:23 AM, Chen Yu wrote:
> Introduce erdt_cpu_has() to verify if a specific RDT feature is backed
> by an ERDT table. erdt_cpu_has() is derived from rdt_cpu_has(), which
> not only considers firmware (ERDT table and its sub-tables) support for
> an event, but also considers userspace input like "rdt=!cmt".
> Similar to rdt_cpu_has(), erdt_cpu_has() expects an x86 feature flag
> as parameter.
> 
> Introduce erdt_support() as a helper called by erdt_cpu_has() to check
> whether the ERDT firmware supports a given feature.
> 
> erdt_support() deliberately claims no feature yet and returns false. A
> feature may only be reported as ERDT backed once the low level accessors
> that read its monitoring data via MMIO exist, otherwise callers would

Are these helpers limited to monitoring ERDT features? First paragraph mentions
"support for an event" and above mentions "read its monitoring data" that
imply these helpers are limited to monitoring features but I do not think this is
the case?

> take the ERDT path and find no implementation behind it. Features are
> added to erdt_support() as their MMIO implementations are introduced.

nit: "are added" -> "should be added"

> 
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> Tested-by: Hongyu Ning <hongyu.ning@xxxxxxxxxxxxxxx>
> ---
> v5->v6:
>     New patch. Split out of the v5 "x86/resctrl: Introduce helpers to
>     read L3 occupancy via MMIO" patch, which carried two logical
>     changes. (Reinette Chatre)
>     Rename erdt_support_features() to erdt_support(). (Reinette Chatre)
>     Reword "expects x86 feature flag as parameters" to "expects an x86
>     feature flag as parameter". (Reinette Chatre)
>     Explain in the changelog why erdt_support() returns false here: a
>     feature is only reported as ERDT backed once the MMIO accessors that
>     read its monitoring data exist. (Reinette Chatre)
>     Drop the unused asm/cpufeatures.h include. (Reinette Chatre)
> ---
>  arch/x86/include/asm/resctrl.h         |  1 +
>  arch/x86/kernel/cpu/resctrl/core.c     | 23 +++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/erdt.c     |  5 +++++
>  arch/x86/kernel/cpu/resctrl/internal.h |  2 ++
>  4 files changed, 31 insertions(+)
> 
> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index e60c2aea7ebd..5491853113dd 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -49,6 +49,7 @@ DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
>  DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
>  DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
>  
> +bool erdt_cpu_has(int flag);
>  int erdt_get_scale(void);
>  

Can this declaration be dropped? Below there is a duplicate declaration
added to arch/x86/kernel/cpu/resctrl/internal.h that should be sufficient?

>  static inline bool resctrl_arch_alloc_capable(void)
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index c2568b29474e..74087d04f1da 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -887,6 +887,29 @@ bool rdt_cpu_has(int flag)
>  	return ret;
>  }
>  
> +bool erdt_cpu_has(int flag)
> +{
> +	struct rdt_options *o;
> +	bool ret;
> +
> +	ret = erdt_support(flag);
> +
> +	if (!ret)
> +		return ret;
> +
> +	for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
> +		if (flag == o->flag) {
> +			if (o->force_off)
> +				ret = false;
> +			if (o->force_on)
> +				ret = true;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  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/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index 422618991927..167d2dfc368d 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -37,6 +37,11 @@ static int erdt_max_rmid;
>  /* Scale to bytes for the monitoring counters when ERDT is enabled. */
>  static int erdt_scale;
>  
> +bool erdt_support(int flag)
> +{
> +	return false;
> +}
> +
>  int erdt_get_max_rmid(void)
>  {
>  	return erdt_max_rmid;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 27f28b0fead8..d14a36d65b05 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -289,6 +289,8 @@ static inline void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resour
>  static inline bool intel_handle_aet_option(bool force_off, char *tok) { return false; }
>  #endif
>  
> +bool erdt_support(int flag);
> +bool erdt_cpu_has(int flag);
>  int erdt_get_max_rmid(void);
>  int erdt_init(void);
>  void erdt_exit(void);

Reinette