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

[Reinette Chatre <reinette.chatre@intel.com>]

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

> 
> Fixes: cbc82b172638 ("x86: Add support for Intel Cache QoS Monitoring (CQM) detection")
> Signed-off-by: Tony Luck <tony.luck@intel.com>

...

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

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.

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

Reinette