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

[Reinette Chatre]

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 <tony.luck@xxxxxxxxx>
> ---

...

> 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? (*)
- 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?

(*) 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().

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)
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. 
                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?

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