Re: [PATCH v11 02/23] x86/resctrl: Check if monitoring features are enabled

[Luck, Tony]

On Wed, Sep 09, 2026 at 08:52:39PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 8/31/26 10:44 AM, Tony Luck wrote:
> > Both Intel and AMD manuals say that software must first check
> > CPUID(0x7,0x0).EBX[12] to see if any monitoring features are enabled
> > before checking for specific features enabled in subleaves.
> > 
> > Each of the L3-based monitoring features is gated by X86_FEATURE_L3_MON
> 
> (nit: Sentence should end with period. There goes the promise that the available
> AI tools means reviewers only need to focus on what matters :()
> 
> To support this change above could perhaps be expanded with "... which in
> turn is gated by X86_FEATURE_RDT_M."
> 
> > 
> > Add check feature bits.
> 
> This sentence does not sound right. How about something like "Add checks for these feature bits."?

Will update.
> 
> > 
> > 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 2435eddd24eb..fcbecf677914 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -968,20 +968,27 @@ static __init bool get_rdt_mon_resources(void)
> >  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> >  	bool ret = false;
> >  
> > -	if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> > -		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false, 0, NULL);
> > -		ret = true;
> > -	}
> > -	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
> > -		resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID, false, 0, NULL);
> > -		ret = true;
> > -	}
> > -	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL)) {
> > -		resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID, false, 0, NULL);
> > -		ret = true;
> > +	/* Any monitoring at all? */
> > +	if (!cpu_feature_enabled(X86_FEATURE_RDT_M))
> 
> Previous patch aimed for "symmetry with X86_FEATURE_RDT_A" ... but the "symmetrical"
> code in get_rdt_alloc_resources() is:
> 
> 	if (!boot_cpu_has(X86_FEATURE_RDT_A))
> 		return false;

I've dropped the #define name change, so the symmetry goal is now moot.
I still plan to use cpu_feature_enabled() based on the block comment in
arch/x86/include/asm/cpufeature.h saying that cpu_feature_enabled() is:

	"the default CPU features testing macro to use in code"



> 
> Why is enumeration of these feature bits done differently (cpu_feature_enabled() vs
> boot_cpu_has())?
> 
> 
> > +		return false;
> > +
> > +	/* Any of the L3 monitoring features? */
> > +	if (cpu_feature_enabled(X86_FEATURE_L3_MON)) {
> 
> Could this reduce indentation and churn by negating the check? Patch #12 and #13 seem
> to build on the flow introduced here so my comments about this function are split between
> these patches.

Will do.

> > +		if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> > +			resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false, 0, NULL);
> > +			ret = true;
> > +		}
> > +		if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
> > +			resctrl_enable_mon_event(QOS_L3_MBM_TOTAL_EVENT_ID, false, 0, NULL);
> > +			ret = true;
> > +		}
> > +		if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL)) {
> > +			resctrl_enable_mon_event(QOS_L3_MBM_LOCAL_EVENT_ID, false, 0, NULL);
> > +			ret = true;
> > +		}
> > +		if (rdt_cpu_has(X86_FEATURE_ABMC))
> > +			ret = true;
> >  	}
> > -	if (rdt_cpu_has(X86_FEATURE_ABMC))
> > -		ret = true;
> >  
> >  	if (!ret)
> >  		return false;
> 
> In further support of above suggestion the code below calls rdt_get_l3_mon_config(), the
> enumeration depending on X86_FEATURE_L3_MON, outside the block where this feature bit is
> confirmed to be set.

Good point.
> 
> Reinette
> 

-Tony