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

[Chen, Yu C]

Hi Reinette,

On 7/15/2026 12:13 AM, Reinette Chatre wrote:
> Hi Chenyu,

[ ... ]

> > > > 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.
>
> Thank you. Making this patch specific will also help to identify unrelated
> changes, for example the introduction of ERDT_MMIO_CMRC_BASE that would be
> more appropriate for the patch that uses it.

OK, will also move the ERDT_MMIO_CMRC_BASE to the patch where it is used.

> > > > +
> > > > +#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.
>
> You say "existing" but I am not able to find erdt_enabled() in this series. Is it
> something that will be needed later?

Ah, erdt_enabled() was removed in current version. Let me change it to
static bool erdt_enabled;

> > > 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.
>
> To make this easier to understand, please aim to have comments that do not describe
> code verbatim but instead what it means. Consider, for example, something like:
> 	"True when the ERDT ACPI table describes at least one domain with at least one CPU."

Got it.

> > > > +        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);
>
> oh, apologies, I misread what you had originally. Even so, your new proposal looks good
> to me.
> As a sidenote, please use most accurate conversion when printing, for example %u for
> unsigned instead of %d.

OK, will do.

thanks,
Chenyu