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/14/26 8:19 AM, Chen, Yu C wrote:
> 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.

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.

...

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

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


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

Reinette