Re: [PATCH v4 1/6] x86/resctrl: Parse ACPI ERDT table and map RMDD domains by L3 cache ID

[Reinette Chatre]

Hi Chenyu,

On 6/22/26 2:07 AM, Chen, Yu C wrote:
> On 6/19/2026 7:37 AM, Reinette Chatre wrote:
>> On 6/13/26 12:56 AM, Chen Yu wrote:

...

>>>   arch/x86/Kconfig                       |   4 +-
>>>   arch/x86/include/asm/apic.h            |   1 +
>>>   arch/x86/include/asm/resctrl.h         |   2 +
>>>   arch/x86/kernel/cpu/resctrl/Makefile   |   1 +
>>>   arch/x86/kernel/cpu/resctrl/core.c     |  14 +-
>>>   arch/x86/kernel/cpu/resctrl/erdt.c     | 296 +++++++++++++++++++++++++
>>>   arch/x86/kernel/cpu/resctrl/internal.h |   3 +
>>>   arch/x86/kernel/cpu/topology.c         |   2 +-
>>
>> Please split out the non-resctrl changes into separate patch with sybsystem-specific
>> prefix to highlight these changes to not appear to sneak changes into other
>> subsystems via resctrl. They can stil form part of this series as preparatory patches.
>>
> 
> I see, let me split two of them out:
> x86/topology: Export topo_lookup_cpuid for module use
> x86/apic: Add declaration for topo_lookup_cpuid

I think a single patch would be ok. Interestingly the subject prefix to
this area is not consistent ... looks like either "x86/topology" or
"x86/cpu/topology" would be ok.

> 
> 
>>>   config X86_CPU_RESCTRL
>>>       bool "x86 CPU resource control support"
>>> -    depends on X86 && (CPU_SUP_INTEL || CPU_SUP_AMD)
>>> +    depends on X86_64 && (CPU_SUP_INTEL || CPU_SUP_AMD)
>>>       depends on MISC_FILESYSTEMS
>>>       select ARCH_HAS_CPU_RESCTRL
>>>       select RESCTRL_FS
>>> @@ -538,7 +538,7 @@ config X86_CPU_RESCTRL
>>>     config X86_CPU_RESCTRL_INTEL_AET
>>>       bool "Intel Application Energy Telemetry"
>>> -    depends on X86_64 && X86_CPU_RESCTRL && CPU_SUP_INTEL && INTEL_PMT_TELEMETRY=y && INTEL_TPMI=y
>>> +    depends on X86_CPU_RESCTRL && CPU_SUP_INTEL && INTEL_PMT_TELEMETRY=y && INTEL_TPMI=y
>>>       help
>>>         Enable per-RMID telemetry events in resctrl.
>>>   
>>
>> Removing 32bit support is a significant change worth a mention and motivation in the changelog.
>>
> 
> The original idea was that MMIO-based access (including CMT/MBM/MBA) uses readq/writeq,
> which depend on 64-bit. Since MMIO CMT relies on CONFIG_X86_CPU_RESCTRL, making
> CONFIG_X86_CPU_RESCTRL depend on X86_64 would simplify the code.
> If I understand correctly, 32-bit x86 lacks practical hardware support for RDT/QoS.
> The Kconfig entry currently depends on X86, which technically permits 32-bit builds.
> However, I am unsure whether there exists any real-world use case where a user compiles
> a 32-bit kernel on a server and enables resctrl?
> If we wish to retain 32-bit RDT support, I can introduce a new config option to gate ERDT:
> config X86_CPU_RESCTRL_ERDT
>     bool
>     depends on X86_CPU_RESCTRL && X86_64 && ACPI
>     default y
> Could you advise which approach is preferable?

x86 resctrl will soon depend on 64bit, this dependency will arrive via this series
or the AET reworks. Whether x86 resctrl depends on 64bit is not the question here.

My question is more specific though:
	What in *this* patch requires dropping 32bit support?
You highlight above that 64bit support is needed for readq/writeq. That is understood.
Even so, this patch does not have any instances of readq/writeq. This patch thus turns
off 32bit support without any mention and without any reason to do so. This is a very
significant change to just sneak into a patch that does not appear to require such
change.

>>> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
>>> index 575f8408a9e7..97c2f6bc7a5f 100644
>>> --- a/arch/x86/include/asm/resctrl.h
>>> +++ b/arch/x86/include/asm/resctrl.h
>>> @@ -40,6 +40,8 @@ struct resctrl_pqr_state {
>>>       u32            default_closid;
>>>   };
>>>   +bool erdt_enabled(void);
>>> +
>>
>> I cannot see why this needs to be in asm header ... why not arch/x86/kernel/cpu/resctrl/internal.h?
>>
> 
> OK, let me move it into internal.h. By the way, I believe fs/resctrl
> also needs to call erdt_enabled() for region-aware MBA/MBM. Would it
> be acceptable to expose erdt_enabled() in include/linux/resctrl.h?

I do not see any calls to erdt_enabled() from resctrl fs in this series.
I also do not think such calls would be appropriate. Whether x86's RDT is
backed with CPUID or ACPI is an architectural detail of no concern to resctrl
fs.


....

>>> +
>>> +struct erdt_domain_info {
>>> +    void __iomem        *base[ERDT_MMIO_MAX];
>>> +    struct acpi_erdt_cmrc    *cmrc;
>>> +};
>>> +
>>> +static bool erdt_enabled_flag;
>>
>> What is significance of "flag"? Can it just be "erdt_enabled"? Since it is a bool this seems
>> more intuitive?
>>
> 
> erdt_enabled() was already used as a function name. Let me change it to static bool is_enabled

This could also be named "__erdt_enabled" to highlight that it is internal.

>>> +
>>> +#define ERDT_VALID_VERSION        1
>>> +#define RMDD_FLAG_CPU_DOMAIN        BIT(0)
>>
>> I think this name can be more accurate, how about, for example "RMDD_FLAG_L3_DOMAIN"?
>>
> 
> According to the spec, this flag indicates: CPU L3 cache and I/O L3 Domain, maybe using
> RMDD_FLAG_CPU_L3_DOMAIN?

Looks good to me.

...

>>> +            return -1;
>>> +        }
>>> +
>>> +        if (!cpu_online(cpu))
>>> +            continue;
>>> +
>>> +        tmp = get_cpu_cacheinfo_id(cpu, RESCTRL_L3_CACHE);
>>
>>  From what I can tell it is tmp that is essentially returned by this function and it
>> is the result of get_cpu_cacheinfo_id() that returns the cache ID initialized from
>> CPUID leaf 0x4 ... the function name ("get_l3_cache_id_from_cacd()") and function comment
>> "Resolve L3 cache ID from CACD subtable" implies differently and the way this is done
>> makes me wonder why CACD needs to be parsed at all.
>>
>> resctrl already has a hotplug handler and it dynamically determines the domain ID
>> based on the cache ID returned from get_cpu_cacheinfo_id(). If the domain ID used by the
>> new ERDT tables can be, as it appears is done here, trusted to what Linux already
>> treating as domain ID then why is it needed to parse CACD at all? Why not just
>> pick the domain ID from RMDD directly and use it directly as domain ID?
>>
>> Later the domain ID from RMDD is just used in error messages ... never actually used
>> as a domain ID to guide the implementation so the correlation appears to be implicit with
>> this CACD parsing not intuitive.
>>
> 
> You are right that the cache ID ultimately comes from get_cpu_cacheinfo_id(),
> not from CACD directly - the function name is misleading and should be improved.
> 
> However, my understanding is that CACD parsing acts as the bridge between ACPI's
> RMDD domain namespace and the kernel's L3 domain ID: CACD enumerates which CPUs
> belong to each RMDD, and from those CPUs we derive the kernel-side L3 cache ID.
> The specification defines an RMDD DomainID as an identifier unique within the
> ERDT table, yet it does not guarantee that this ID matches the kernel’s cache ID
> obtained via CPUID leaf 0x4. For this reason, we omitted the RMDD domain ID in
> the current release.

get_l3_cache_id_from_cacd() implies a straightforward query and the function
comments supports this. At the same time the function includes what appears to be
sanity checks but from what you say turns out to be essential enforcement of the
RMDD to CPUID leaf 0x4 mapping. Is this correct? As sanity checks these additional
checks are ok but as RMDD to CPUID leaf 0x4 mapping enforcement I find them inadequate
because its correctness requires all CPUs of all domains to be online which cannot be
guaranteed when this initialization runs.

Consider for example a scenario where RMDD and CPUID do not agree on which CPUs form
part of a domain. If only one CPU of this RMDD domain is online at the time this
initialization is done then the mismatch will never be noticed and resctrl will just
silently use the CPUID mapping, no?

Reinette