Re: [PATCH v4 04/19] x86/resctrl: Detect Assignable Bandwidth Monitoring feature details

[Reinette Chatre <reinette.chatre@xxxxxxxxx>]

Hi Babu,

On 5/24/24 5:23 AM, 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).
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
> 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 | 14 ++++++++++++++
>   include/linux/resctrl.h               |  4 ++++
>   2 files changed, 18 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index b35d04fc761b..1602b58ba23d 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1066,6 +1066,20 @@ 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->abmc_capable = true;
> +			/*
> +			 * Query CPUID_Fn80000020_EBX_x05 for number of
> +			 * ABMC counters
> +			 */
> +			cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
> +			r->mon.num_cntrs = (ebx & 0xFFFF) + 1;
> +			if (r->mon.num_cntrs > 64) {
> +				WARN(1, "Cannot support more than 64 ABMC counters\n");

if (WARN_ON(...))

> +				r->mon.num_cntrs = 64;
> +			}
> +		}
>   	}
>   
>   	l3_mon_evt_init(r);
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index bf99eb9c6ce4..24087e6efbb6 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -153,10 +153,12 @@ struct resctrl_schema;
>   /**
>    * struct resctrl_mon - Monitoring related data
>    * @num_rmid:		Number of RMIDs available
> + * @num_cntrs:		Maximum number of abmc counters

Maximum implies some limit and it is not clear what limit this applies to.
Can this just be "Number of monitoring counters".

Thinking ahead ... when switching between different "MBM modes" it
may be that one mode has a different number of counters than the other.
Since "ABMC" is just one mode it does not seem appropriate to
connect the "num_cntrs" property to ABMC.


>    * @evt_list:		List of monitoring events
>    */
>   struct resctrl_mon {
>   	int			num_rmid;
> +	int			num_cntrs;
>   	struct list_head	evt_list;
>   };
>   
> @@ -177,6 +179,7 @@ struct resctrl_mon {
>    * @parse_ctrlval:	Per resource function pointer to parse control values
>    * @fflags:		flags to choose base and info files
>    * @cdp_capable:	Is the CDP feature available on this resource
> + * @abmc_capable:	Is system capable of supporting monitor assignment?
>    */
>   struct rdt_resource {
>   	int			rid;
> @@ -196,6 +199,7 @@ struct rdt_resource {
>   						 struct rdt_domain *d);
>   	unsigned long		fflags;
>   	bool			cdp_capable;
> +	bool			abmc_capable;

Shouldn't abmc_capable be a property of the new struct resctrl_mon?

>   };
>   
>   /**

Reinette