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

["Chen, Yu C" <yu.c.chen@intel.com>]

Hi Chenyu,

On 7/11/2026 7:42 AM, Reinette Chatre wrote:
> 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.
> 

I planned to add some background context on ERDT. Let me remove unrelated
content and change the sentence to:
Parse the RMDD subtables within the ERDT ACPI table and their nested
CACD entries to construct per-domain CPU masks.

>>
>> There is one ERDT per platform. Each RMDD describes one resource
> 
> "one ERDT" -> "one ERDT table"?
> 

OK.


>> 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?
> 

It was used in previous version but not needed in this version.
Additionally, cleanup.h and err.h can be removed as well, I will
drop them.

>> +#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?
> 

Not needed, will remove it.

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

The double underscore was used to prevent name collisions with the
existing erdt_enabled(). Let me switch it to a single underscore instead.

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

OK, let me add this:

/*
  * Set when the ERDT ACPI table has been successfully parsed and at least
  * one valid RMDD domain with a CACD cpumask exists. Cleared on teardown.
  */

>> +
>> +#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()?)
> 

OK, will do.

>> +
>> +		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?
> 

Got it, let me switch to one global max_rmid = min(all CPU domains's 
non-zero max_rmid ).
IO-domain RMID checks can be deferred for future support.

>> +
>> +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?
>

No need to stick to the size_t convention - there is no risk of overflow,
I’ll remove 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)
> 

Will remove it.

>> +	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)
> 

If I understand correctly, cpumask_var_t can be used for both stack 
contexts and
dynamic allocation? Depending on whether OFFSTACK is defined, it may 
either be a
dynamically allocated pointer or an inline single-element struct cpumask 
array.
That said, since erdt_domain_info itself is dynamically allocated, 
embedding cpu_mask
inside erdt_domain_info would be clearer. Let me change it.

>> +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".
> 

I previously considered the RMDD table an entry within the parent ERDT 
table.
I’ll change the function name to parse_rmdd_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.
> 

OK, will do.

>> +		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)
> 

Ok, let me switch to struct cpumask instead.

>> +
>> +	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) */
> 

OK, will do.

>> +			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?
> 

OK, let print the ID of the first RMDD and the "broken" RMDD.

>> +		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
> 

I overlooked this, let me remove the INT_MAX check.

>> +		pr_warn(FW_BUG "Unreasonable RMDD max_rmid %u\n", rmdd->max_rmid);
> 
> Is there a limit in the spec?
> 

The spec defines it as a 4-byte value, so a u32 here should not overflow.

>> +		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?
> 

OK, will do.

>> +
>> +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()?
> 

OK.

>> +		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?
> 

Yes, it will print the revision, do you mean print the expected revision?

pr_info("Unsupported ERDT table revision %d (expected %d)\n",
         erdt->header.revision, ERDT_VALID_VERSION);

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

OK, will do.

>> +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*
> 

Yes, let me switch to struct cpumask instead.

>> +	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.
> 

OK, let me rename it to "entry".

thanks,
Chenyu