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