Re: [PATCH v6 04/22] x86/resctrl: Detect Assignable Bandwidth Monitoring feature details

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

Hi Babu,

On 8/6/24 3:00 PM, Babu Moger wrote:
> ABMC feature details are reported via CPUID Fn8000_0020_EBX_x5.
> Bits Description
> 15:0 MAX_ABMC Maximum Supported Assignable Bandwidth
>       Monitoring Counter ID + 1
> 
> The feature details are documented in APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
> Monitoring (ABMC).
> 
> Detect the feature and number of assignable counters supported.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v6: Commit message update.
>      Renamed abmc_capable to mbm_cntr_assignable.
> 
> v5: Name change num_cntrs to num_mbm_cntrs.
>      Moved abmc_capable to resctrl_mon.
> 
> v4: Removed resctrl_arch_has_abmc(). Added all the code inline. We dont
>      need to separate this as arch code.
> 
> v3: Removed changes related to mon_features.
>      Moved rdt_cpu_has to core.c and added new function resctrl_arch_has_abmc.
>      Also moved the fields mbm_assign_capable and mbm_assign_cntrs to
>      rdt_resource. (James)
> 
> v2: Changed the field name to mbm_assign_capable from abmc_capable.
> ---
>   arch/x86/kernel/cpu/resctrl/monitor.c | 12 ++++++++++++
>   include/linux/resctrl.h               |  4 ++++
>   2 files changed, 16 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 795fe91a8feb..88312b5f0069 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1229,6 +1229,18 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>   			mbm_local_event.configurable = true;
>   			mbm_config_rftype_init("mbm_local_bytes_config");
>   		}
> +
> +		if (rdt_cpu_has(X86_FEATURE_ABMC)) {
> +			r->mon.mbm_cntr_assignable = true;
> +			/*
> +			 * Query CPUID_Fn80000020_EBX_x05 for number of
> +			 * ABMC counters.
> +			 */

At this point this comment seems unnecessary. Not an issue, it can stay of you
prefer.

> +			cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
> +			r->mon.num_mbm_cntrs = (ebx & 0xFFFF) + 1;
> +			if (WARN_ON(r->mon.num_mbm_cntrs > 64))

Please document where this "64" limit comes from. This is potentially a problem
since the resctrl fs managed bitmap is hardcoded to be of size 64 but the arch code
sets how many counters are supported. Will comment more later on bitmap portions, but
to handle this I expect resctrl fs should at least sanity check the number of counters
before attempting to initialize its bitmap ... or better, as James suggests, make the
bitmap creation dynamic.

> +				r->mon.num_mbm_cntrs = 64;
> +		}
>   	}
>   
>   	l3_mon_evt_init(r);
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 1097559f4987..72c498deeb5e 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -185,10 +185,14 @@ enum resctrl_scope {
>   /**
>    * struct resctrl_mon - Monitoring related data
>    * @num_rmid:		Number of RMIDs available
> + * @num_mbm_cntrs:	Number of monitoring counters
> + * @mbm_cntr_assignable:Is system capable of supporting monitor assignment?
>    * @evt_list:		List of monitoring events
>    */
>   struct resctrl_mon {
>   	int			num_rmid;
> +	int			num_mbm_cntrs;
> +	bool			mbm_cntr_assignable;
>   	struct list_head	evt_list;
>   };
>   

Reinette