Re: [PATCH v9 12/12] x86/resctrl: Add MMIO-based LLC occupancy monitoring support

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

Hi Chenyu,

On 10/13/26 8:01 PM, Chen Yu wrote:
> Add erdt_mon_read() to read LLC occupancy via MMIO and use it
> when the platform supports ERDT. Register the L3 occupancy event
> with ERDT enabled when available, falling back to the MSR-based
> path otherwise.
> 
> Use the CMRC (Cache Monitoring Registers for CPU Agents Description)
> ACPI sub-table to read LLC occupancy counters for each RMID via MMIO
> when ERDT is enabled.
> 
> Although the occupancy counters can now be read from any CPU via
> MMIO, keep the per-domain limbo handler. A global handler would
> still have to iterate over every domain and would save nr_domains - 1

Please write what "nr_domains - 1" means in English since as it is written
it is not clear how this "does not justify maintaining two limbo handler
models."

> worker threads, which does not justify maintaining two limbo handler
> models.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> Tested-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> ---

...

> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index 110e7b9806b4..1a9847a11de8 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -27,6 +27,9 @@ static bool erdt_enabled;
>  #define RMDD_FLAG_CPU_L3_DOMAIN		BIT(0)
>  #define ERDT_DOMAIN_ID_UNSET		-1
>  
> +#define CMRC_UNAVAILABLE_COUNTER	BIT_ULL(63)
> +#define CMRC_FLAG_UNAVAILABLE_BIT	BIT(0)
> +
>  /* Bitmask of valid sub-tables found in the first RMDD, used to ensure all RMDDs match. */
>  static u32 valid_subtbl_mask;
>  
> @@ -52,14 +55,84 @@ static unsigned int erdt_scale;
>  
>  bool erdt_support(int flag)
>  {
> -	return false;
> +	if (!erdt_enabled)
> +		return false;
> +
> +	switch (flag) {
> +	case X86_FEATURE_CQM_OCCUP_LLC:
> +		return valid_subtbl_mask & BIT(ACPI_ERDT_TYPE_CMRC);
> +	default:
> +		return false;
> +	}
> +}
> +
> +unsigned int erdt_get_max_rmid(void)
> +{
> +	if (!erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> +		return 0;

This does not look right. Why is this restriction needed? At first glance it seems to
be a quirk used to simplify the get_rdt_mon_resources() changes but not actually
be the right thing to do?

> +
> +	return erdt_max_rmid;
>  }
>  
>  unsigned int erdt_get_scale(void)
>  {
> +	if (!erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> +		return 0;
> +
>  	return erdt_scale;

Constraining a global value to one feature is confusing, could this
instead be, for exaple, erdt_get_cmt_scale() that returns erdt_cmt_scale
that will be more accurate and clear why the erdt_cpu_has() check is there?
This change can be added to the introduction of this function when the patch
doing so is placed correctly in the series.

>  }
>  
> +/*
> + * Compute the CMRC MMIO offset for CMRC_SUPPORTED_INDEX_FN using the
> + * MMIO_offset_for_RMID# = (RMID / ClumpSize) * Stride + (RMID % ClumpSize) * 8
> + * formula from the Intel RDT Architecture Specification.
> + */
> +static u32 cmrc_index_function_1(struct acpi_erdt_cmrc *cmrc, u32 rmid)
> +{
> +	return (rmid / cmrc->clump_size) * cmrc->clump_stride +
> +	       (rmid % cmrc->clump_size) * sizeof(u64);
> +}
> +
> +static int erdt_read_l3_occupancy(const struct erdt_domain_info *d, u32 rmid, u64 *val)
> +{
> +	struct acpi_erdt_cmrc *cmrc;
> +	u64 l3_cmt_count;
> +	u32 offset;
> +
> +	cmrc = d->cmrc;
> +	if (!cmrc)
> +		return -EIO;
> +
> +	offset = cmrc_index_function_1(cmrc, rmid);
> +	if (offset + sizeof(u64) > (u32)cmrc->cmt_reg_size * SZ_4K)

Could you please explain what this is validating here? 
cmrc->cmt_reg_size is already u32 so the cast is unexpected. These comparisons
with different types can be tricky, looks like right side is 32 bit and may be
truncated before the comparison to left 64 bit?

> +		return -EINVAL;
> +
> +	l3_cmt_count = readq(d->base[ERDT_MMIO_CMRC_BASE] + offset);
> +	if ((cmrc->flags & CMRC_FLAG_UNAVAILABLE_BIT) &&
> +	    (l3_cmt_count & CMRC_UNAVAILABLE_COUNTER))
> +		return -EINVAL;
> +
> +	*val = l3_cmt_count * cmrc->up_scale;
> +
> +	return 0;
> +}
> +
> +int erdt_mon_read(struct rdt_domain_hdr *hdr, enum resctrl_event_id evtid, u32 rmid, u64 *val)
> +{
> +	struct rdt_hw_l3_mon_domain *hw_dom;
> +	const struct erdt_domain_info *d;
> +
> +	hw_dom = resctrl_to_arch_mon_dom(container_of(hdr, struct rdt_l3_mon_domain, hdr));
> +	d = hw_dom->d_info;
> +	if (!d)
> +		return -EIO;
> +
> +	if (evtid == QOS_L3_OCCUP_EVENT_ID)
> +		return erdt_read_l3_occupancy(d, rmid, val);
> +
> +	return -EIO;
> +}
> +
>  static void __iomem *erdt_ioremap(resource_size_t base, u32 num_pages, const char *desc)
>  {
>  	void __iomem *addr;
> @@ -161,6 +234,8 @@ static __init bool cmrc_init(struct acpi_subtbl_hdr_16 *subtbl,
>  		return false;
>  	}
>  
> +	erdt_scale = max(erdt_scale, cmrc->up_scale);
> +

This change is out of place.

>  	return true;
>  }
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 88fae7a84fc7..3561a7ae8c7e 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -291,6 +291,8 @@ static inline bool intel_handle_aet_option(bool force_off, char *tok) { return f
>  bool erdt_cpu_has(int flag);
>  bool erdt_support(int flag);
>  unsigned int erdt_get_scale(void);
> +unsigned int erdt_get_max_rmid(void);
> +int erdt_mon_read(struct rdt_domain_hdr *hdr, enum resctrl_event_id evtid, u32 rmid, u64 *val);
>  void erdt_init(void);
>  void erdt_exit(void);
>  bool erdt_try_bind_cpu(int cpu);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index e66e4c845739..93ee638119d3 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -283,6 +283,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(r, hdr, rmid, eventid, val);
>  	case RDT_RESOURCE_PERF_PKG:
>  		return intel_aet_read_event(hdr->id, rmid, arch_priv, val);
> @@ -438,7 +442,7 @@ unsigned int resctrl_arch_round_mon_val(unsigned int val)
>  	if (!scale)
>  		scale = boot_cpu_data.x86_cache_occ_scale;
>  
> -	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
> +	/* h/w works in units of the occupancy scale  */

Please move to patch that introduces the "scale" usage to resctrl_arch_round_mon_val().

>  	val /= scale;
>  	return val * scale;
>  }
> @@ -447,12 +451,21 @@ 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);
> +	unsigned int erdt_max_rmid;

Please do not shadow globals.

>  	unsigned int threshold;
> +	int max_rmid;
>  	u32 eax, ebx, ecx, edx;

reverse fir please

>  
> +	erdt_max_rmid = erdt_get_max_rmid();
> +	if (erdt_max_rmid)
> +		max_rmid = min_t(unsigned int, erdt_max_rmid,
> +				 boot_cpu_data.x86_cache_max_rmid);
> +	else
> +		max_rmid = boot_cpu_data.x86_cache_max_rmid;
> +
>  	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;
> -	r->mon.num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;
> +	r->mon.num_rmid = (max_rmid + 1) / snc_nodes_per_l3_cache;
>  	hw_res->mbm_width = MBM_CNTR_WIDTH_BASE;
>  
>  	if (mbm_offset > 0 && mbm_offset <= MBM_CNTR_WIDTH_OFFSET_MAX)

Reinette