Re: [PATCH v6 3/9] x86/resctrl: Parse ACPI ERDT table and save CACD cpumask for RMDD domains

[Chen Yu]

Hi Reinette,

On Wed, Aug 19, 2026 at 04:01:33PM -0700, Reinette Chatre wrote:
> 
> Hi Chenyu,
> 
> On 7/25/26 2:22 AM, Chen Yu wrote:
> > From: Anil S Keshavamurthy <anil.s.keshavamurthy@xxxxxxxxx>
> > 
> > Parse the RMDD subtables within the ERDT ACPI table and their nested
> > CACD entries to construct per-domain CPU masks.
> 
> The changelog always needs to start with context. Please see "Changelog"
> in Documentation/process/maintainer-tip.rst for complete expectations.
>

OK, I will first add some background information (including what ERDT and CACD are).

> > 
> > There is one ERDT table per platform. Each RMDD describes one resource
> 
> I do not think what "RMDD" stands for has been introduced at this point yet.
> 
> > management domain (RMD), also known as an L3 domain, and carries MMIO
> > base information for later monitoring support.
> > 
> > For each RMDD, parse the associated CACD, map its x2APIC IDs to logical
> 
> What is "CACD"? Please always expand acronym before its first use.
> 
> > 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.
> 
> Please let each patch description stand on its own without referring to later
> patches in series. If a patch provides capability in preparation for
> future changes then you can use language like, 
> "Associate every ERDT domain with the CPUs that belong to it to prepare
>  for attaching ERDT data to resctrl monitoring domains."
>

Thanks for the guidance, will adjust it.
 
> >  static __init bool get_rdt_resources(void)
> >  {
> > +	erdt_init();
> >  	rdt_alloc_capable = get_rdt_alloc_resources();
> >  	rdt_mon_capable = get_rdt_mon_resources();
> >  
> 
> Functions are not expected to leave dangling state when they return failure. get_rdt_resources()
> returning false is considered a failure and now the caller is left to clean up the dangling
> state which is not a familiar pattern to use and thus something that can/will trip people.
> 
> On top of this this implementation pushes the cleanup very far from even the caller making
> this unfamiliar pattern even harder to recognize.
> 
> Please let get_rdt_resources() clean up after itself on failure to find any resources. 
>

The original intent was to encapsulate the cleanup logic as a helper function (erdt_exit()), 
since it is needed in multiple places across the code. All other call sites invoke this function
when necessary. If we place the cleanup call directly inside resctrl_arch_late_init(), it would
only execute erdt_exit() once within that function. Alternatively, if we let get_rdt_resources()
handle cleanup via erdt_exit(), then we would need to add explicit erdt_exit() calls in 
__resctrl_arch_late_init() for each failure path - for example, when cpuhp_setup_state() fails or
when resctrl_init() fails. Anyway, if code readability is a priority, I'll change the logic
as suggested.

> > @@ -1114,7 +1115,7 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c)
> >  	}
> >  }
> >  
> > -static int __init resctrl_arch_late_init(void)
> > +static int __init __resctrl_arch_late_init(void)
> >  {
> >  	struct rdt_resource *r;
> >  	int state, ret, i;
> > @@ -1157,6 +1158,15 @@ static int __init resctrl_arch_late_init(void)
> >  	return 0;
> >  }
> >  
> > +static int __init resctrl_arch_late_init(void)
> > +{
> > +	int ret = __resctrl_arch_late_init();
> > +
> > +	if (ret)
> > +		erdt_exit();
> > +	return ret;
> > +}
> 
> Related to earlier comment on cleanup I find this cleanup to be asymmentrical
> and inconsistent with how resctrl usually does cleanup. Why not do cleanup in
> (original) resctrl_arch_late_init() to be consistent with other cleanup when
> failures are encountered during initialization, for example, cpuhp_remove_state()?
> I find that having the cleanup handled where error is encountered is easier to understand.
>

Thomas previously suggested this approach to avoid using goto.
https://lore.kernel.org/lkml/871pem5jnh.ffs@fw13/
However, I agree that we can adopt the individual cleanup strategy instead, as it
will improve code readability.
 
> > +
> >  late_initcall(resctrl_arch_late_init);
> >  
> >  static void __exit resctrl_arch_exit(void)
> > @@ -1166,6 +1176,8 @@ static void __exit resctrl_arch_exit(void)
> >  	cpuhp_remove_state(rdt_online);
> >  
> >  	resctrl_exit();
> > +
> > +	erdt_exit();
> 
> 
> Here the cleanup is indeed done in the same place as cleanup of
> other init work (cpuhp_remove_state()).
>

Understood. I will ensure consistency whenever cpuhp_remove_state() is called, erdt_exit()
will also be invoked in the original resctrl_arch_late_init().

> >  }
> > +/* True when the ERDT ACPI table describes at least one domain with at least one CPU. */
> > +static bool erdt_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;
> > +
> > +/* Domain ID of the first RMDD that established @valid_subtbl_mask, for diagnostics. */
> > +static u16 first_rmdd_domain_id;
> > +
> > +static int erdt_max_rmid;
> 
> Could this ever be negative? Could this instead be of same type as the value it is
> initialized with? Looks like this will improve type safety with the min_t() usage then
> using the accurate and consistent type of both parameters?
>

It should not be negative, let me convert it into u32 and use min() directly.
 
> I also think a comment describing erdt_max_rmid will be helpful, especially considering
> that it has "max" in its name but then its value is determined using the *minimum* of
> all domains' RMID?
>

OK, let me add the following comment when Tony also raised this question.
resctrl_rmid_realloc_threshold is a single global value,
and resctrl_arch_round_mon_val() takes no domain argument, so a
single scale has to be derived from the per-domain cmrc->up_scale.
max() was chosen because the rounding is a floor: a larger scale
yields a slightly lower threshold, i.e. an RMID has to drop to
a slightly lower occupancy before it is reused.

https://lore.kernel.org/all/3053f498-1eb8-4c64-8d52-847829f48a1b@xxxxxxxxx/
 
> > +
> > +int erdt_get_max_rmid(void)
> > +{
> > +	return erdt_max_rmid;
> > +}
> > +
> > +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(num_pages, SZ_4K, &size))
> > +		return NULL;
> > +
> > +	addr = ioremap(base, size);
> 
> The types seem to target a function with prototype ioremap(phys_addr_t base, size_t size)
> but I find arch/x86/include/asm/io.h to declare:
> void __iomem *ioremap(resource_size_t offset, unsigned long size);
> 
> Since it is this code that determines the type it could just use matching accurate type
> from the beginning?
>

OK, let me switch it to unsigned long to be consistent with x86 declaration.
 
> > +	if (!addr)
> > +		pr_warn(FW_BUG "ERDT: Failed to map %s at phys addr %pa (size: %u pages)\n",
> > +		       desc, &base, num_pages);
> 
> Please align to open parenthesis. 
>

OK.
 
> > +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;
> 
> Can num_ids ever be negative? If not, please use unsigned type.
>

It will not be negative, let me switch it to unsigned int.
 
> > +static __init bool parse_rmdd_table(struct acpi_subtbl_hdr_16 *rmdd_hdr)
> > +{
> > +	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 bytes\n", rmdd_hdr->length);
> > +		return false;
> > +	}
> > +
> > +	rmdd = (struct acpi_erdt_rmdd *)rmdd_hdr;
> 
> Could this initialization be done at time of declaration to be consistent with the other
> functions parsing tables? The length comparison could then use rmdd->header.length to
> match how this check is done in all the other places. Being consistent makes the
> code much easier to understand.
> 
>

OK, will modify the code.
 
> > +
> > +	/* 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;
> > +
> > +	domain_info->dom_id = -1;
> > +
> > +	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)) {
> 
> I find it curious how the implementation varies in how the tables are parsed. For example, here
> it uses a for () loop to cycle through the tables while enumerate_erdt_table() uses a while()
> loop for what appears to be the same flow. Are they actually different? Why are the two different
> patterns needed?
>

Let me switch them both to "for" loop to keep consistence.
 
> > +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;
> 
> Should erdt_max_rmid be reset also?
>

Yes, let me fix this.
 
> > +
> > +	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);
> 
> Please use sizeof(*erdt)
>

OK, will do.
 
> > +enum erdt_mmio_type {
> > +	ERDT_MMIO_RMDD_CREG,
> > +	ERDT_MMIO_LAST = ERDT_MMIO_RMDD_CREG
> > +};
> > +
> > +#define ERDT_MMIO_NUM_TYPES	(ERDT_MMIO_LAST + 1)
> > +
> > +/**
> > + * struct erdt_domain_info - Per-domain ERDT information
> > + * @base:	Array of ioremapped MMIO region base addresses, indexed by ERDT_MMIO_* type
> 
> I think "type" can be dropped? The enum is already implicitly an "MMIO type"?
>

Got it, will remove this "type".

thanks,
Chenyu