Re: [PATCH v9 03/12] x86/resctrl: Parse ACPI ERDT table and save CACD cpumask per RMD

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

Hi Reinette,

On Fri, Oct 23, 2026 at 04:50:24PM -0700, Reinette Chatre wrote:
> Hi Chenyu,
> 
> On 10/13/26 7:58 PM, Chen Yu wrote:
> > Enhanced RDT (ERDT) exposes per-domain MMIO registers that resctrl needs
> > in order to read hardware monitoring counters on the upcoming MMIO-based
> > path. The kernel discovers this hardware through a single per-platform
> > ERDT ACPI table.
> > 
> > Each Resource Management Domain Description (RMDD) sub-table within the
> > ERDT table represents one resource management domain (RMD). When
> > RMDD_FLAG_CPU_L3_DOMAIN is set, it identifies the domain as a CPU-scoped
> > L3 domain. The MMIO base of the domain's CMT counters is carried by the
> > RMDD's Cache Monitoring Registers for CPU Agents Description (CMRC)
> 
> Please drop this sentency about CMRC table and CMT counters, it is unrelated
> to this patch and distracting from what it aims to achieve.
>

OK, will do.

> > Changes since v8:
> > - Rename the subject from "... save CACD cpumask for RMDD domains" to
> >   "... save CACD cpumask per RMD". (Reinette Chatre)
> > - Rewrite the changelog to lead with the motivation, drop the duplicated
> >   first paragraph, and add a diagram of the ERDT/RMDD/CACD hierarchy.
> >   (Reinette Chatre)
> 
> Did I really ask for a diagram?

No, you did not. I added this diagram to help describe the hierarchy
more clearly.

> It is nice but I think it highlights
> how the various domain IDs can be confusing. The diagram shows domain_id at
> two different scope which raises this importance of these values but these
> domain IDs are not actually (apart from some debugging) used in this patch.
>

OK, will remove domain_id from both the RMDD and the CACD tables.

> > diff --git a/arch/x86/kernel/cpu/resctrl/Makefile b/arch/x86/kernel/cpu/resctrl/Makefile
> > index 273ddfa30836..a975c73e273e 100644
> > --- a/arch/x86/kernel/cpu/resctrl/Makefile
> > +++ b/arch/x86/kernel/cpu/resctrl/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_X86_CPU_RESCTRL)		+= core.o rdtgroup.o monitor.o
> >  obj-$(CONFIG_X86_CPU_RESCTRL)		+= ctrlmondata.o
> > +obj-$(CONFIG_X86_CPU_RESCTRL)		+= erdt.o
> 
> nit: erdt.o could just be added to the line with ctrlmondata.o?
>

OK, will change it.

> >  obj-$(CONFIG_X86_CPU_RESCTRL_INTEL_AET)	+= intel_aet.o
> >  obj-$(CONFIG_RESCTRL_FS_PSEUDO_LOCK)	+= pseudo_lock.o
> >  
> 
> ...
> 
> > +/*
> > + * The minimal max_rmid of different domains. Using minimal is to avoid the domain with
> > + * small RMID accessing an invalid RMID.
> 
> Going back and forth between minimum and maximum is confusing.
> Could this comment just be simplified to "Largest RMID that is valid in every ERDT domain."?
>

OK, will change it.

> > +static __init bool cacd_init(struct acpi_subtbl_hdr_16 *subtbl,
> > +			     struct erdt_domain_info *domain_info)
> > +{
> > +	struct acpi_erdt_cacd *cacd = (struct acpi_erdt_cacd *)subtbl;
> > +	unsigned int num_ids;
> > +	int cpu;
> > +
> > +	if (cacd->header.length < struct_size(cacd, X2APICIDS, 1)) {
> > +		pr_warn(FW_BUG "Invalid x2APIC ID CACD table\n");
> > +		return false;
> > +	}
> > +
> > +	num_ids = (cacd->header.length - sizeof(*cacd)) / sizeof(cacd->X2APICIDS[0]);
> > +
> > +	for (unsigned int i = 0; i < num_ids; i++) {
> > +		cpu = topo_lookup_cpuid(cacd->X2APICIDS[i]);
> > +		/* Skip CPUs not in the possible mask, e.g. excluded by nr_cpus= */
> > +		if (cpu < 0)
> > +			continue;
> 
> With this new change it is possible for cacd_init() to return "true" without
> finding any matching CPUs. A "true" return would result in an ERDT domain without
> any CPUs added to the domain_info_list. Should such a domain rather be skipped?
>

I see, before adding the ERDT domain into the list, will check if that
domain has any CPUs, I've changed it to something like below:

	/* After iterating the RMDD subtables(including CACD) */

	/* Skip an RMDD without any CPU, for example, all excluded by nr_cpus= */
	if (cpumask_empty(&domain_info->cpu_mask)) {
		cleanup_one_domain(domain_info);
		return true;
	}

	if (!valid_subtbl_mask) {
	...

	list_add(&domain_info->entry, &domain_info_list);

> > +
> > +	domain_info->dom_id = ERDT_DOMAIN_ID_UNSET;
> > +
> > +	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 = (void *)rmdd + sizeof(*rmdd);
> > +	     subtbl_valid((void *)rmdd + rmdd->header.length, subtbl);
> > +	     subtbl = next_subtbl(subtbl)) {
> > +		switch (subtbl->type) {
> > +		/* An RMDD table has one or more CACD sub-table(s) */
> > +		case ACPI_ERDT_TYPE_CACD:
> > +			if (!cacd_init(subtbl, domain_info))
> > +				goto cleanup;
> > +
> > +			subtbl_mask |= BIT(ACPI_ERDT_TYPE_CACD);
> 
> Should this bit be set if no valid CPUs found (in which case cacd_init()
> returns true)?
>

I kept setting the bit, because it records which sub-table types firmware
provides, which is what the "same set of sub-tables" check compares. An RMDD
without any CPU is now dropped right after the sub-table walk, before
valid_subtbl_mask is set or compared, so it cannot affect that check.

> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!subtbl_mask)
> > +		goto cleanup;
> 
> How about a check here to ensure that the domain actually has CPUs before adding it
> to the list? If so, it may be easier to map ERDT_MMIO_RMDD_CREG *after* the RMDD
> has been determined to have some valid CPUs?
>

Yes, I change it a little bit to:

/* A CPU-based L3 RMDD must have at least one CACD sub-table */
if (!(subtbl_mask & BIT(ACPI_ERDT_TYPE_CACD))) {
	pr_warn(FW_BUG "RMDD %u has no CACD sub-table\n", rmdd->domain_id);
	goto cleanup;
}

/* Quietly skip an RMDD without any CPU, for example, all excluded by nr_cpus= */
if (cpumask_empty(&domain_info->cpu_mask)) {
	cleanup_one_domain(domain_info);
	return true;
}

if (!valid_subtbl_mask) {
...

list_add(&domain_info->entry, &domain_info_list);

> > +	return false;
> > +}
> > +
> > +void erdt_exit(void)
> > +{
> > +	struct erdt_domain_info *d, *tmp;
> > +
> > +	list_for_each_entry_safe(d, tmp, &domain_info_list, entry) {
> > +		list_del(&d->entry);
> > +		cleanup_one_domain(d);
> > +	}
> > +	erdt_enabled = false;
> > +	valid_subtbl_mask = 0;
> > +	first_rmdd_domain_id = 0;
> > +	erdt_max_rmid = 0;
> 
> The ERDT API seems to expose these globals via helpers that resctrl can call
> at any time irrespective of whether erdt_enabled is true or not. For example, 
> patch #7 adds a new global erdt_scale, and makes it available via erdt_get_scale()
> without any additional checks. If this is the plan then it is very important to
> ensure all globals are reset here and it looks like erdt_scale is not added
> to this list when it is introduced.

Got it, thanks for pointing out, will add erdt_scale = 0 in patch #7.

> Also please consider checking if erdt is
> enabled to ensure a sane value is return from these helpers. Doing so may be
> an easier pattern for contributions to follow than to subtly require a reset
> here that is already missed in this series.
>

I see, will also add erdt_enabled check in every helper functions in subsequent patches.

thanks,
Chenyu