Re: [RFC PATCH v3 03/17] x86/resctrl: Detect Assignable Bandwidth Monitoring feature details

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

Hi Babu,

On 3/28/2024 6:06 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).
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> ---
> 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/core.c     | 17 +++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/monitor.c  |  3 +++
>  include/linux/resctrl.h                | 12 ++++++++++++
>  4 files changed, 33 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 57a8c6f30dd6..bb82b392cf5d 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -740,6 +740,23 @@ bool __init rdt_cpu_has(int flag)
>  	return ret;
>  }
>  
> +inline bool __init resctrl_arch_has_abmc(struct rdt_resource *r)
> +{
> +	bool ret = rdt_cpu_has(X86_FEATURE_ABMC);
> +	u32 eax, ebx, ecx, edx;
> +
> +	if (ret) {
> +		/*
> +		 * Query CPUID_Fn80000020_EBX_x05 for number of
> +		 * ABMC counters
> +		 */
> +		cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
> +		r->mbm_assign_cntrs = (ebx & 0xFFFF) + 1;
> +	}
> +
> +	return ret;
> +}

It is not clear to me why this function is needed. I went back to
read James' comment and it sounds to me as though he expected it
to be called from non-arch code ... but this is only called
from rdt_get_mon_l3_config() which is very much architecture specific
and will remain in arch/x86 where rdt_cpu_has() will be accessible.

> +
>  static __init bool get_mem_config(void)
>  {
>  	struct rdt_hw_resource *hw_res = &rdt_resources_all[RDT_RESOURCE_MBA];
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c99f26ebe7a6..c4ae6f3993aa 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -584,6 +584,7 @@ void free_rmid(u32 closid, u32 rmid);
>  int rdt_get_mon_l3_config(struct rdt_resource *r);
>  void __exit rdt_put_mon_l3_config(void);
>  bool __init rdt_cpu_has(int flag);
> +bool __init resctrl_arch_has_abmc(struct rdt_resource *r);
>  void mon_event_count(void *info);
>  int rdtgroup_mondata_show(struct seq_file *m, void *arg);
>  void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index c34a35ec0f03..e5938bf53d5a 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1055,6 +1055,9 @@ 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 (resctrl_arch_has_abmc(r))
> +			r->mbm_assign_capable = ABMC_ASSIGN;
>  	}

This is confusing to me in two ways:
(a) why need different layers of abstraction to initialize r->mbm_assign_capable
    and r->mbm_assign_cntrs? Can they not just be assigned at the same time?
(b) r->mbm_assign_capable is a bool ... but it is assigned an enum? Why is
    this enum needed for this?

>  
>  	l3_mon_evt_init(r);
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index a365f67131ec..a1ee9afabff3 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -150,6 +150,14 @@ struct resctrl_membw {
>  struct rdt_parse_data;
>  struct resctrl_schema;
>  
> +/**
> + * enum mbm_assign_type - The type of assignable monitoring.
> + * @ABMC_ASSIGN: Assignable Bandwidth Monitoring Counters.
> + */
> +enum mbm_assign_type {
> +	ABMC_ASSIGN	= 0x01,
> +};
> +

Either the resource is mbm_assign_capable or not ... it is not clear
to me why an enum is needed.

>  /**
>   * struct rdt_resource - attributes of a resctrl resource
>   * @rid:		The index of the resource
> @@ -168,6 +176,8 @@ struct resctrl_schema;
>   * @evt_list:		List of monitoring events
>   * @fflags:		flags to choose base and info files
>   * @cdp_capable:	Is the CDP feature available on this resource
> + * @mbm_assign_capable:	Does system capable of supporting monitor assignment?

"Does system capable" -> "Is system capable"?

> + * @mbm_assign_cntrs:	Maximum number of assignable counters
>   */
>  struct rdt_resource {
>  	int			rid;
> @@ -188,6 +198,8 @@ struct rdt_resource {
>  	struct list_head	evt_list;
>  	unsigned long		fflags;
>  	bool			cdp_capable;
> +	bool			mbm_assign_capable;
> +	u32                     mbm_assign_cntrs;
>  };

Please check tabs vs spaces (in this whole series please).

I'm thinking that a new "MBM specific" struct within
struct rdt_resource will be helpful to clearly separate the MBM related
data. This will be similar to struct resctrl_cache for 
cache allocation and struct resctrl_membw for memory bandwidth
allocation.

>  
>  /**

Reinette