Re: [PATCH v5 03/10] x86/resctrl: Parse ACPI ERDT table and save CACD cpumask for RMDD domains

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

Hi Chenyu,

On 7/1/26 6:45 AM, Chen Yu wrote:
> From: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> 
> Parse the ERDT (Enhanced RDT) ACPI table so enhanced RDT features can
> consume firmware-provided domain information.
> 
> The ERDT may contain these sub-tables:
> 
>   - Resource Management Domain Description Structure (RMDD)
>   - CPU Agent Collection Description Structure (CACD)
>   - Cache Monitoring Registers for CPU Agents Description Structure
>     (CMRC)

How should "The ERDT may contain these sub-tables" be interpreted here?
This sounds like some high level partial description of ERDT that is
not specific to this patch.

> 
> There is one ERDT per platform. Each RMDD describes one resource

"one ERDT" -> "one ERDT table"?

> management domain (RMD), also known as an L3 domain, and carries MMIO
> base information for later monitoring support.
> 
> Add basic ERDT table parsing and retain the relevant sub-table
> information for later processing.
> 
> Handle RMDD specially. For each RMDD, parse the associated CACD, map
> its x2APIC IDs to logical CPUs, and save the resulting CPU mask. This
> mask associates each ERDT domain with the CPUs that belong to it and is
> used later when attaching ERDT data to resctrl monitoring domains.
> 
> Suggested-by: Tony Luck <tony.luck@intel.com>
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Tested-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---

...

> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> new file mode 100644
> index 000000000000..6405df9be817
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Enhanced Resource Director Technology (ERDT)
> + *
> + * Copyright (C) 2026 Intel Corporation
> + *
> + */
> +
> +#define pr_fmt(fmt)     "resctrl: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/cleanup.h>
> +#include <linux/cpu.h>

Which parts of cpu.h are used?

> +#include <linux/err.h>
> +#include <linux/overflow.h>
> +#include <linux/resctrl.h>
> +#include <linux/sizes.h>
> +#include <linux/xarray.h>

xarray is no longer needed?

> +
> +#include <asm/apic.h>
> +
> +#include "internal.h"
> +
> +static LIST_HEAD(domain_info_list);
> +
> +static bool __erdt_enabled;

Is double underscore needed?
Could you please add a comment above the variable to describe what it means when
"erdt is enabled"?

> +
> +#define ERDT_VALID_VERSION		1
> +#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;
> +
> +int erdt_get_max_rmid(int cpu)
> +{
> +	struct erdt_domain_info *d;
> +	struct list_head *pos;
> +
> +	if (!__erdt_enabled)
> +		return 0;
> +
> +	list_for_each(pos, &domain_info_list) {
> +		d = container_of(pos, struct erdt_domain_info, list);

(list_for_each_entry()?)

> +
> +		if (cpumask_test_cpu(cpu, d->cpu_mask))
> +			return d->max_rmid;
> +	}
> +
> +	return -1;
> +}

Using a CPU as parameter to determine the maximum RMID supported by ERDT is
unexpected. Looking ahead at how this function is used I find only one usage:
	rdt_get_l3_mon_config() {
		...
		/*
		 * 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);

From this usage I do not see any reason why to do any CPU matching ... erdt_get_max_rmid()
could just return the max_rmid of any domain ... but that does not look right either since
the comment states an assumption that is never enforced in the code.

Could this be made more robust by replacing the "per-erdt-domain max_rmid" with one global
"ERDT max RMID" to which all RMDD's max RMID is compared to *ensure* they are all the same.
If there is no such guarantee then I expect this will be the minimum among all RMDD? All seems
to point to there only being one global value that is determined during ERDT enumeration and
then this helper can just return that value directly?

> +
> +static void __iomem *erdt_ioremap(phys_addr_t base, u32 num_pages, const char *desc)
> +{
> +	void __iomem *addr;
> +	size_t size;
> +
> +	if (check_mul_overflow((size_t)num_pages, (size_t)SZ_4K, &size))

I do not think check_mul_overflow() requires size_t type, does it?

> +		return NULL;
> +
> +	addr = ioremap(base, size);
> +	if (!addr) {
> +		pr_err("ERDT: Failed to map %s at phys addr %pa (size: %u pages)\n",
> +		       desc, &base, num_pages);
> +	}

(unnecessary braces)

> +	return addr;
> +}
> +
> +static void erdt_iounmap_domain(struct erdt_domain_info *domain)
> +{
> +	for (int i = 0; i < ERDT_MMIO_NUM_TYPES; i++) {
> +		if (domain->base[i]) {
> +			iounmap(domain->base[i]);
> +			domain->base[i] = NULL;
> +		}
> +	}
> +}
> +
> +static void cleanup_one_domain(struct erdt_domain_info *d)
> +{
> +	erdt_iounmap_domain(d);
> +	free_cpumask_var(d->cpu_mask);

free_cpumask_var() does not look right ... it is for stack usage, no? (more later)

> +	kfree(d);
> +}
> +
> +/*
> + * Save CACD information for this RMDD:
> + * convert the X2APIC to CPU and save them in a mask.
> + */
> +static __init int cacd_init(struct acpi_subtbl_hdr_16 *subtbl,
> +			    struct erdt_domain_info *domain_info)
> +{
> +	struct acpi_erdt_cacd *cacd = (struct acpi_erdt_cacd *)subtbl;
> +	int num_ids, cpu;
> +
> +	if (cacd->header.length < struct_size(cacd, X2APICIDS, 1)) {
> +		pr_warn(FW_BUG "Invalid x2apicid CACD table\n");
> +		return -EIO;
> +	}
> +
> +	num_ids = (cacd->header.length - sizeof(*cacd)) / sizeof(cacd->X2APICIDS[0]);
> +
> +	for (int i = 0; i < num_ids; i++) {
> +		cpu = topo_lookup_cpuid(cacd->X2APICIDS[i]);
> +		if (cpu < 0) {
> +			pr_warn(FW_BUG "Unknown x2apicid 0x%x\n", cacd->X2APICIDS[i]);
> +			return -EIO;
> +		}
> +
> +		cpumask_set_cpu(cpu, domain_info->cpu_mask);
> +	}
> +
> +	return 0;
> +}
> +
> +static inline struct acpi_subtbl_hdr_16 *rmdd_subtbl(struct acpi_erdt_rmdd *rmdd)
> +{
> +	return (void *)rmdd + sizeof(*rmdd);
> +}
> +
> +static inline struct acpi_subtbl_hdr_16 *next_subtbl(struct acpi_subtbl_hdr_16 *subtbl)
> +{
> +	return (void *)subtbl + subtbl->length;
> +}
> +
> +static inline bool subtbl_valid(void *end, struct acpi_subtbl_hdr_16 *subtbl)
> +{
> +	/* Ensure the header is within bounds before dereferencing it. */
> +	if ((void *)subtbl + sizeof(*subtbl) > end)
> +		return false;
> +
> +	/* A sub-table must be at least as large as its header. */
> +	if (subtbl->length < sizeof(*subtbl))
> +		return false;
> +
> +	/* The entire sub-table (including body) must fit within the parent. */
> +	if ((void *)subtbl + subtbl->length > end)
> +		return false;
> +
> +	return true;
> +}
> +
> +static __init bool parse_rmdd_entry(struct acpi_subtbl_hdr_16 *rmdd_hdr)

nit: what is motivation for the "entry" term? this is only occurance of the
word "entry" in this patch while RMDD is more frequently referred to as "table" or
"sub-table".

> +{
> +	struct erdt_domain_info *domain_info;
> +	struct acpi_subtbl_hdr_16 *subtbl;
> +	struct acpi_erdt_rmdd *rmdd;
> +	u32 subtbl_mask = 0;
> +
> +	if (rmdd_hdr->length < sizeof(*rmdd)) {
> +		pr_warn(FW_BUG "Invalid RMDD length %u\n", rmdd_hdr->length);

Please include unit, similar to equivalent ERDT message.

> +		return false;
> +	}
> +
> +	rmdd = (struct acpi_erdt_rmdd *)rmdd_hdr;
> +
> +	/* Quietly ignore non-CPU-based L3 domains */
> +	if (!(rmdd->flags & RMDD_FLAG_CPU_L3_DOMAIN))
> +		return true;
> +
> +	domain_info = kzalloc_obj(*domain_info, GFP_KERNEL);
> +	if (!domain_info)
> +		return false;
> +
> +	if (!zalloc_cpumask_var(&domain_info->cpu_mask, GFP_KERNEL))
> +		goto cleanup;

Similar to free_cpumask_var() this does not look right since the cpu_mask is not on stack here ...
(more later)

> +
> +	domain_info->base[ERDT_MMIO_RMDD_CREG] =
> +		erdt_ioremap(rmdd->creg_base, rmdd->creg_size, "RMDD ctrl base");
> +	if (!domain_info->base[ERDT_MMIO_RMDD_CREG])
> +		goto cleanup;
> +
> +	for (subtbl = rmdd_subtbl(rmdd);
> +	     subtbl_valid((void *)rmdd + rmdd->header.length, subtbl);
> +	     subtbl = next_subtbl(subtbl)) {
> +		switch (subtbl->type) {
> +		case ACPI_ERDT_TYPE_CACD:

It is quite subtle that there could be multiple CACD tables. Could this be highlighted with
a small comment? Something like:
			/* An RMDD table has one or more CACD sub-table(s) */

> +			if (cacd_init(subtbl, domain_info))
> +				goto cleanup;
> +
> +			subtbl_mask |= BIT(ACPI_ERDT_TYPE_CACD);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (!subtbl_mask)
> +		goto cleanup;
> +
> +	/*
> +	 * Require all RMDDs to support same set of sub-tables
> +	 */
> +	if (!valid_subtbl_mask) {
> +		valid_subtbl_mask = subtbl_mask;
> +	} else if (subtbl_mask != valid_subtbl_mask) {
> +		pr_warn(FW_BUG "RMDD sub-table set does not match the first RMDD\n");

Would it be useful to print the domain ID to help diagnistics?

> +		goto cleanup;
> +	}
> +
> +	if (!rmdd->max_rmid || rmdd->max_rmid > INT_MAX) {

rmdd->max_rmid is a u32 so INT_MAX test is not clear to me here

> +		pr_warn(FW_BUG "Unreasonable RMDD max_rmid %u\n", rmdd->max_rmid);

Is there a limit in the spec?

> +		goto cleanup;
> +	}
> +	domain_info->max_rmid = rmdd->max_rmid;

As mentioned before, instead of a per-domain RMID, could there be a global ERDT
RMID that is initialized by first ERDT domain and updated/compared with every following
ERDT domain?


> +
> +	list_add(&domain_info->list, &domain_info_list);
> +
> +	return true;
> +
> +cleanup:
> +	cleanup_one_domain(domain_info);
> +	return false;
> +}
> +
> +void erdt_exit(void)
> +{
> +	struct erdt_domain_info *d;
> +	struct list_head *pos, *n;
> +
> +	list_for_each_safe(pos, n, &domain_info_list) {

list_for_each_entry_safe()?

> +		d = container_of(pos, struct erdt_domain_info, list);
> +		list_del(pos);
> +		cleanup_one_domain(d);
> +	}
> +	__erdt_enabled = false;
> +	valid_subtbl_mask = 0;
> +}
> +
> +static __init int enumerate_erdt_table(struct acpi_table_header *table_hdr)
> +{
> +	struct acpi_table_erdt *erdt = (struct acpi_table_erdt *)table_hdr;
> +	struct acpi_subtbl_hdr_16 *subtbl;
> +	void *table_end;
> +
> +	if (erdt->header.revision != ERDT_VALID_VERSION) {
> +		pr_info("Unsupported ERDT table revision %d\n", erdt->header.revision);

Would it be helpful to print what the ERDT table revision is to help diagnostics?

> +		return -EINVAL;
> +	}
> +
> +	if (erdt->header.length < sizeof(*erdt)) {
> +		pr_warn(FW_BUG "ERDT: Invalid table length %u bytes\n", erdt->header.length);
> +		return -EINVAL;
> +	}
> +
> +	subtbl = (void *)erdt + sizeof(struct acpi_table_erdt);
> +	table_end = (void *)erdt + erdt->header.length;
> +
> +	while (subtbl_valid(table_end, subtbl)) {
> +		if (subtbl->type == ACPI_ERDT_TYPE_RMDD &&
> +		    !parse_rmdd_entry(subtbl))
> +			goto cleanup;
> +
> +		subtbl = next_subtbl(subtbl);
> +	}
> +
> +	if (list_empty(&domain_info_list))
> +		goto cleanup;
> +
> +	__erdt_enabled = true;
> +
> +	return 0;
> +
> +cleanup:
> +	erdt_exit();
> +	return -EINVAL;
> +}
> +
> +int __init erdt_init(void)
> +{
> +	return acpi_table_parse(ACPI_SIG_ERDT, enumerate_erdt_table);
> +}
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e3cfa0c10e92..299d7222f693 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -21,6 +21,26 @@
>  
>  #define RMID_VAL_UNAVAIL		BIT_ULL(62)
>  
> +/*
> + * Index into erdt_domain_info::base[] for each MMIO region.
> + * @ERDT_MMIO_RMDD_CREG: RMDD control register base address
> + * @ERDT_MMIO_CMRC_BASE: CMRC monitoring register base address
> + */
> +enum erdt_mmio_type {
> +	ERDT_MMIO_RMDD_CREG,
> +	ERDT_MMIO_CMRC_BASE,
> +	ERDT_MMIO_LAST = ERDT_MMIO_CMRC_BASE
> +};
> +
> +#define ERDT_MMIO_NUM_TYPES	(ERDT_MMIO_LAST + 1)
> +

Please add documentation here that describes each member of struct erdt_domain_info.

> +struct erdt_domain_info {
> +	void __iomem		*base[ERDT_MMIO_NUM_TYPES];
> +	cpumask_var_t		cpu_mask;

Should this be struct cpumask instead? Compare with struct rdt_domain_hdr.
When evaluating cpumask_var_t, please read the detailed comments above its
definition in include/linux/cpumask_types.h - specifically note the start:
	*cpumask_var_t: struct cpumask for stack usage*

> +	int			max_rmid;
> +	struct list_head	list;

Could you please rename this to be "node" or "entry" to make it obvious that this
is an entry of a list? This is not consistent in resctrl but really helps 
when reading the code.

> +};
> +
>  /*
>   * With the above fields in use 62 bits remain in MSR_IA32_QM_CTR for
>   * data to be returned. The counter width is discovered from the hardware
> @@ -253,4 +273,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
>  
> +int erdt_get_max_rmid(int cpu);
> +int erdt_init(void);
> +void erdt_exit(void);
> +
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */

Reinette