Re: [PATCH v10 01/17] x86/resctrl: Fix enumeration of number of supported RMIDs

On Thu, Aug 13, 2026 at 04:55:53PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 7/29/26 10:27 AM, Tony Luck wrote:
> > There is a multi-step decision tree for enumeration on X86 of Intel Resource
> > Director Technology (RDT) also known as AMD Platform Quality of Service (PQOS)
> > 
> > First check CPUID(0x7,0x0).EBX{12} (Linux feature flag X86_FEATURE_CQM).
> > If this is "0" no monitoring features are supported. If it is "1" then
> > monitoring is supported, the IA32_PQR_ASSOC MSR exists, and the RMID field can
> > be written with values from zero to the value enumerated in CPUID(0xF,0x0).EBX.
> > 
> > Second check CPUID(0xF,0x0).EDX{1} (Linux feature flag X86_FEATURE_CQM_LLC).
> > If this is "1" then one or more L3 cache monitoring features exist and
> > further enumeration from CPUID(0xF,0x1) provides information about L3
> > monitoring features.
> > 
> > Linux skips the check for X86_FEATURE_CQM and begins with step two checking
> > X86_FEATURE_CQM_LLC.
> > 
> > This is a problem for systems that do not support any L3 monitoring features
> > (or have disabled them with the kernel boot parameter "clearcpuid=cqm_llc")
> > but do support other features such as Application Energy Telemetry. On
> > such a system cpuinfo_x86::x86_cache_max_rmid is incorrectly set to "-1".
> > 
> > Correct the checks to follow guidance from the Intel Software
> > Developer's manual and AMD Architecture Programmer's Manual.
> > 
> > Fixes: cbc82b172638 ("x86: Add support for Intel Cache QoS Monitoring (CQM) detection")
> > Signed-off-by: Tony Luck <[email protected]>
> > ---
> 
> ...
> 
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index 9b9495174041..e3544c99cff1 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -1075,20 +1075,17 @@ static enum cpuhp_state rdt_online;
> >  /* Runs once on the BSP during boot. */
> >  void resctrl_cpu_detect(struct cpuinfo_x86 *c)
> >  {
> > -	if (!cpu_has(c, X86_FEATURE_CQM_LLC) && !cpu_has(c, X86_FEATURE_ABMC)) {
> > +	if (!cpu_has(c, X86_FEATURE_CQM)) {
> >  		c->x86_cache_max_rmid  = -1;
> >  		c->x86_cache_occ_scale = -1;
> >  		c->x86_cache_mbm_width_offset = -1;
> >  		return;
> >  	}
> >  
> > -	/* will be overridden if occupancy monitoring exists */
> > +	/* May be overridden if L3 monitoring exists and supports fewer RMIDs. */
> >  	c->x86_cache_max_rmid = cpuid_ebx(0xf);
> 
> There seems to be a lot of history behind this code that I am not familiar with making
> a couple of things unclear:
> - Why is resctrl_cpu_detect() needed? I do not see why this early initialization
>   is required since the only consumer of the data initialized here is called
>   from resctrl_arch_late_init() and it could just obtain this data itself? 
>   At this time the enumeration of RDT/PQoS features appear scattered between this
>   and get_rdt_resources(), the latter already running CPUID itself when needed making
>   this early initialization scattered in all the "BSP init code" seem unnecessary? (*)

Git and mailing list archaeology time!

In 2015 Intel was calling the feature "Cache QoS Monitoring (CQM)". The
only part of what is now RDT was L3 cache monitoring. Initial patches
https://lore.kernel.org/all/1388781285-18067-1-git-send-email-peter.p.waskiewicz.jr@intel.com/
attempted to attach the feature to a combination of "perf" and "cgroup".

That's when the x86_cache_max_rmid and x86_cache_occ_scale fields were
added to struct cpuinfo_x86.

In 2020 x86_cache_mbm_width_offset joined them there, and initialization
moved to only being performed on the bsp.
f3d44f18b066 ("x86/resctrl: Support CPUID enumeration of MBM counter width")
923f3a2b48bd ("x86/resctrl: Query LLC monitoring properties once during boot")

> - Why does cpuinfo_x86::x86_cache_max_rmid, cpuinfo_x86::x86_cache_occ_scale, and
>   cpuinfo_x86::x86_cache_mbm_width_offset exist? Only x86 resctrl uses this data
>   and making it global seem unnecessary?

Agreed. When resctrl was invented, this enumeration should have moved to
get_rdt_mon_resources() and the fields deleted from struct cpuinfo_x86
to be replaced by variables within resctrl.

> (*) The one problem with this seems to be intel_rdt_mbm_apply_quirk() that uses
> the data initialized by resctrl_cpu_detect(), but that just seems to be for convenience
> and it is still just called from resctrl_arch_late_init().

The call to check_quirks() only handles this one quirk for Intel, and
happens before resctrl checks to see it MBM is even supported. Perhaps
that quirk call should be done from get_rdt_mon_resources()? I'm not
sure I see the benefit of a centralized call for all quirks.

> 
> What above inquiry boils down to is that I do not believe that this change addresses the
> issue that the patch aims to solve. 
> step 1 (above). c->x86_cache_max_rmid is initialized to maximum RMID that PQR_ASSOC MSR
>                  supports (sidenote: c->x86_pqr_assoc_max_rmid may be a more appropriate
>                  name considering the expanded usage)

Better to move this out of struct cpuinfo_x86. Also switch from
"max_rmid" to "num_rmids" (which can be a "u32" as everywhere else
with "0" meaning "no monitoring" rather than max_rmid = -1.

> step 2 (below). c->x86_cache_max_rmid is initialized to what L3 monitoring, if present, supports.
> 		Ignoring for a moment that this just blindly assumes that the value will be smaller
>                 or equal to the value set in step 1, it does not take into account whether
>                 L3 monitoring may have been disabled on user request or because of resctrl
>                 quirk. 

You don't need to ignore "smaller or equal". SDM states that CPUID(0xF,0x0).EBX
is "Maximum range (zero-based) of RMID within this physical processor of all types."

Thus CPUID(0xF,0x1).ECX must be less than or equal to this "MAX_RMID" value.

>                 The comment above states:
>                 /* May be overridden if L3 monitoring exists and supports fewer RMIDs. */
>                 If L3 monitoring is disabled on command line or via a quirk then this will result
>                 in fewer RMID available to AET than what the system can support in PQR_ASSOC, no?

You are technically correct. But to end up in this unhappy state would
require that Intel builds a CPU that supports fewer L3 RMIDs than the
MAX_RMID value. That would only happen if some additional monitoring
events were enumerated in CPUID leaf 0xF, subleaf 2 (or greater).
That hasn't happened so far. All the possible enumeration bits for such
subleafs in CPUID(0xF0,0x).EDX are currently listed as "Reserved".

> It looks to me that moving monitoring enumeration done in resctrl_cpu_detect() into resctrl proper
> where the RMID range checks can be centralized on features gated by rdt_cpu_has() would be simpler and
> accurate?

Agreed. I'll cook up a patch to do this. I.e.

1) Remove resctrl_cpu_detect() (functionality -> get_rdt_mon_resources()
2) Remove the x86_cache_* fields from struct x86_cpu (replace with variables within resctrl)
3) Move call to intel_rdt_mbm_apply_quirk() to get_rdt_mon_resources()

> > -	if (cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC) ||
> > -	    cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL) ||
> > -	    cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL) ||
> > -	    cpu_has(c, X86_FEATURE_ABMC)) {
> > +	if (cpu_has(c, X86_FEATURE_CQM_LLC) || cpu_has(c, X86_FEATURE_ABMC)) {
> >  		u32 eax, ebx, ecx, edx;
> >  
> >  		/* QoS sub-leaf, EAX=0Fh, ECX=1 */
> 
> Reinette

-Tony