Re: [PATCH v5 09/10] x86/resctrl: Introduce helpers to read L3 occupancy via MMIO

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

Hi Chenyu,

On 7/1/26 6:47 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". erdt_cpu_has() expects the same input parameters as

parameters -> parameter 
although this can be more helpful with something like:
"Similar to rdt_cpu_has(), erdt_cpu_has() expects an x86 feature flag as parameter."

> rdt_cpu_has().
> 
> Introduce erdt_mon_read(), a helper that retrieves monitoring data
> for a given RMID and event ID from an ERDT domain. erdt_mon_read()
> leverages erdt_cpu_has() to check whether the system supports the
> corresponding ACPI tables, such as ERDT and CMRC (Cache Monitoring
> Registers for CPU Agents Description). It invokes the low-level MMIO
> read callbacks (introduced later) if supported.
> 

Since this patch contains two logical changes, can it be split?

> Tested-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
...

> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 575f8408a9e7..0fd4bf85f628 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -49,6 +49,8 @@ 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);
> +
>  static inline bool resctrl_arch_alloc_capable(void)
>  {
>  	return rdt_alloc_capable;
> @@ -131,6 +133,10 @@ static inline unsigned int resctrl_arch_round_mon_val(unsigned int val)
>  {
>  	unsigned int scale = boot_cpu_data.x86_cache_occ_scale;
>  
> +	/* ERDT itself factors and rounds the data within erdt.c */
> +	if (erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> +		return val;
> +

This does not seem to take all usages of resctrl_arch_round_mon_val() into account.
This function is also used to determine a valid (user controlled) threshold for the limbo
handler because it needs to be ensured that the threshold is a value that the hardware can
actually measure.


Speaking of the limbo handler ... I did not notice any mention of limbo handler or
any changes to it. From what I can tell this implementation keeps one limbo handler per
domain even though occupancy counts can be read from any CPU. Is this intended?

>  	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
>  	val /= scale;
>  	return val * scale;
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 2e95586ebe45..5932cf813cb4 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -895,6 +895,29 @@ bool rdt_cpu_has(int flag)
>  	return ret;
>  }
>  
> +bool erdt_cpu_has(int flag)
> +{
> +	struct rdt_options *o;
> +	bool ret;
> +
> +	ret = erdt_support_features(flag);

erdt_support_features() -> erdt_support_feature() or maybe just "erdt_supports()" ?
(only one feature can be checked at a time?)

> +
> +	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))
> @@ -982,7 +1005,10 @@ static __init bool get_rdt_mon_resources(void)
>  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>  	bool ret = false;
>  
> -	if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> +	if (erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> +		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, true, 0, NULL);
> +		ret = true;
> +	} else if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
>  		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false, 0, NULL);
>  		ret = true;
>  	}
> @@ -1000,7 +1026,10 @@ static __init bool get_rdt_mon_resources(void)
>  	if (!ret)
>  		return false;
>  
> -	return !rdt_get_l3_mon_config(r);
> +	if (rdt_get_l3_mon_config(r))
> +		return false;
> +
> +	return r->mon_capable;

It is not clear to me why this change is necessary.

>  }
>  
>  static __init void __check_quirks_intel(void)
> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index a5754d64fcc1..1114ad4e3b42 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -18,6 +18,7 @@
>  #include <linux/xarray.h>
>  
>  #include <asm/apic.h>
> +#include <asm/cpufeatures.h>
>  

This include is not used in this change?

>  #include "internal.h"
>  
> @@ -27,11 +28,17 @@ static bool __erdt_enabled;
>  
>  #define ERDT_VALID_VERSION		1
>  #define CMRC_SUPPORTED_INDEX_FN		1
> +#define UNAVAILABLE_COUNTER		BIT_ULL(63)

Not used in this patch.

>  #define RMDD_FLAG_CPU_L3_DOMAIN		BIT(0)
>  
>  /* Bitmask of valid sub-tables found in the first RMDD, used to ensure all RMDDs match. */
>  static u32 valid_subtbl_mask;
>  
> +bool erdt_support_features(int flag)
> +{
> +	return false;
> +}

Why not provide implementation here?

> +
>  int erdt_get_max_rmid(int cpu)
>  {
>  	struct erdt_domain_info *d;
> @@ -50,6 +57,11 @@ int erdt_get_max_rmid(int cpu)
>  	return -1;
>  }
>  
> +int erdt_mon_read(struct rdt_domain_hdr *hdr, int ev_id, int rmid, u64 *val)
> +{

Please use accurate types ... rmid is u32, evt_id is enum resctrl_event_id ...

> +	return -EIO;
> +}
> +
>  static void __iomem *erdt_ioremap(phys_addr_t base, u32 num_pages, const char *desc)
>  {
>  	void __iomem *addr;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 6eb0fdea6b63..ecb44f82581e 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -278,8 +278,11 @@ 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_features(int flag);
> +bool erdt_cpu_has(int flag);
>  int erdt_get_max_rmid(int cpu);
>  int erdt_init(void);
>  void erdt_exit(void);
> +int erdt_mon_read(struct rdt_domain_hdr *hdr, int ev_id, int rmid, u64 *val);
>  
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index f4f4c9015ceb..e6d7037f000b 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -279,6 +279,10 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain_hdr *hdr,
>  
>  	switch (r->rid) {
>  	case RDT_RESOURCE_L3:
> +		if (eventid == QOS_L3_OCCUP_EVENT_ID &&
> +		    erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> +			return erdt_mon_read(hdr, eventid, rmid, val);
> +
>  		return arch_l3_read_event(hdr, rmid, eventid, val, r);
>  	case RDT_RESOURCE_PERF_PKG:
>  		return intel_aet_read_event(hdr->id, rmid, arch_priv, val);
> @@ -423,6 +427,11 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)
>  {
>  	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> +	/*
> +	 * Currently assume all CPU domains share the same maximum RMID
> +	 * value from the RMDD table, use CPU0 domain's value.
> +	 */
> +	int erdt_max_rmid = erdt_get_max_rmid(0);
>  	unsigned int threshold;
>  	u32 eax, ebx, ecx, edx;
>  
> @@ -430,7 +439,8 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)
>  
>  	resctrl_rmid_realloc_limit = boot_cpu_data.x86_cache_size * 1024;
>  	hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale / snc_nodes_per_l3_cache;

This implementation still mixes CPUID and ERDT enumeration. Above, for example, a system
that supports ERDT cace occupancy counts is expected to provide another scale value via
CPUID? Could you please clearly separate ERDT and CPUID enumeration.

> -	r->mon.num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;
> +	r->mon.num_rmid = (erdt_max_rmid > 0) ? erdt_max_rmid + 1 :
> +			   (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;

Here things are also intertwined. Above essentially means: "if cache occupancy counters are
read via ERDT then use max RMID from CMRC and let it *override* the RMID from CPUID that
is appropriate for memory bandwidth monitoring"!

I find the code like above to become increasingly obfuscated. This patch starts with a
promise that "erdt_cpu_has() indicates support for ERDT feature" ... but then above
generic code uses the more subtle "erdt_max_rmid > 0" as a subtle check of ERDT support.
Having the CPUID and ERDT support be so intertwined with subtle checks just added for convenience
makes the code difficult to understand and I am concerned it will be error prone when folks try
to build on top of it.


>  	hw_res->mbm_width = MBM_CNTR_WIDTH_BASE;
>  
>  	if (mbm_offset > 0 && mbm_offset <= MBM_CNTR_WIDTH_OFFSET_MAX)
> @@ -477,7 +487,18 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)
>  		hw_res->mbm_cntr_assign_enabled = true;
>  	}
>  
> -	r->mon_capable = true;
> +	/*
> +	 * If the platform has ERDT but the SNC is enabled,
> +	 * this monitor should not be enabled.

(line length, please check entire series)

> +	 */
> +	if (erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC) &&
> +	    snc_nodes_per_l3_cache > 1) {
> +		WARN_ONCE(1, "ERDT is enabled but SNC%d is enabled, monitors for resource[%s] should be disabled\n",
> +			  snc_nodes_per_l3_cache, r->name);
> +		resctrl_disable_mon_event(QOS_L3_OCCUP_EVENT_ID);

This seems backwards. I think this highlights how ERDT needs to be handled separately and
not try to wedge into CPUID enabling. I am surprised about this behavior. Isn't SNC and RDT supported via
MSR? Would this not prevent users from using cache occupancy on an SNC enabled system when it could be
supported? Is a WARN actually justified here? Could SNC capability not be checked early _before_
enabling ERDT cace occupancy event and if SNC is active then the MSR cache occupancy can be used
instead?


> +	} else {
> +		r->mon_capable = true;
> +	}
>  
>  	return 0;
>  }
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 6a7c86a72c51..2cf03e4cf72a 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -1034,6 +1034,12 @@ bool resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu,
>  	return true;
>  }
>  
> +void resctrl_disable_mon_event(enum resctrl_event_id eventid)
> +{
> +	if (mon_event_all[eventid].enabled)
> +		mon_event_all[eventid].enabled = false;
> +}
> +

Please see https://lore.kernel.org/lkml/20260701213553.15222-5-tony.luck@intel.com/

>  bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid)
>  {
>  	return eventid >= QOS_FIRST_EVENT && eventid < QOS_NUM_EVENTS &&
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 73ff522448a0..dfde025432ab 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -420,6 +420,7 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
>  
>  bool resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu,
>  			      unsigned int binary_bits, void *arch_priv);
> +void resctrl_disable_mon_event(enum resctrl_event_id eventid);
>  
>  bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid);
>  

Reinette