Re: [RFC PATCH v3 07/17] x86/resctrl: Add support to enable/disable ABMC feature

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

Hi Babu,

On 3/28/2024 6:06 PM, Babu Moger wrote:
> Add the functionality to enable/disable ABMC feature.
> 
> ABMC is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the
> state of ABMC is changed, it must be changed to the updated value on all
> logical processors in the QOS Domain.

This patch does much more than enable what is mentioned above. There is little
information about what this patch aims to accomplish. Without this it makes
review difficult.

> 
> The ABMC 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: No changes.
> 
> v2: Few text changes in commit message.
> ---
>  arch/x86/include/asm/msr-index.h       |  1 +
>  arch/x86/kernel/cpu/resctrl/internal.h | 12 ++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 76 +++++++++++++++++++++++++-
>  3 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 05956bd8bacf..f16ee50b1a23 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1165,6 +1165,7 @@
>  #define MSR_IA32_MBA_BW_BASE		0xc0000200
>  #define MSR_IA32_SMBA_BW_BASE		0xc0000280
>  #define MSR_IA32_EVT_CFG_BASE		0xc0000400
> +#define MSR_IA32_L3_QOS_EXT_CFG		0xc00003ff
>  
>  /* MSR_IA32_VMX_MISC bits */
>  #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 722388621403..8238ee437369 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>  	return cpu;
>  }
>  
> +/* ABMC ENABLE */

Can this comment be made more useful?

> +#define ABMC_ENABLE			BIT(0)
> +
>  struct rdt_fs_context {
>  	struct kernfs_fs_context	kfc;
>  	bool				enable_cdpl2;
> @@ -433,6 +436,7 @@ struct rdt_parse_data {
>   * @mbm_cfg_mask:	Bandwidth sources that can be tracked when Bandwidth
>   *			Monitoring Event Configuration (BMEC) is supported.
>   * @cdp_enabled:	CDP state of this resource
> + * @abmc_enabled:	ABMC feature is enabled
>   *
>   * Members of this structure are either private to the architecture
>   * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g.
> @@ -448,6 +452,7 @@ struct rdt_hw_resource {
>  	unsigned int		mbm_width;
>  	unsigned int		mbm_cfg_mask;
>  	bool			cdp_enabled;
> +	bool			abmc_enabled;
>  };
>  
>  static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r)
> @@ -491,6 +496,13 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>  
>  int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>  
> +static inline bool resctrl_arch_get_abmc_enabled(enum resctrl_res_level l)
> +{
> +	return rdt_resources_all[l].abmc_enabled;
> +}
> +
> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable);
> +
>  /*
>   * To return the common struct rdt_resource, which is contained in struct
>   * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 05f551bc316e..f49073c86884 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -850,9 +850,15 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
>  				    struct seq_file *s, void *v)
>  {
>  	struct rdt_resource *r = of->kn->parent->priv;
> +	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>  
> -	if (r->mbm_assign_capable)
> +	if (r->mbm_assign_capable && hw_res->abmc_enabled) {
> +		seq_puts(s, "[abmc]\n");
> +		seq_puts(s, "legacy_mbm\n");
> +	} else if (r->mbm_assign_capable) {
>  		seq_puts(s, "abmc\n");
> +		seq_puts(s, "[legacy_mbm]\n");
> +	}
>  
>  	return 0;
>  }
> @@ -2433,6 +2439,74 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
>  	return 0;
>  }
>  
> +static void resctrl_abmc_msrwrite(void *arg)
> +{
> +	bool *enable = arg;
> +	u64 msrval;
> +
> +	rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
> +
> +	if (*enable)
> +		msrval |= ABMC_ENABLE;
> +	else
> +		msrval &= ~ABMC_ENABLE;
> +
> +	wrmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
> +}
> +
> +static int resctrl_abmc_setup(enum resctrl_res_level l, bool enable)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
> +	struct rdt_domain *d;
> +
> +	/* Update QOS_CFG MSR on all the CPUs in cpu_mask */

"all the CPUs in cpu_mask" -> "all the CPUs associated with the resource"?

> +	list_for_each_entry(d, &r->domains, list) {
> +		on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1);
> +		resctrl_arch_reset_rmid_all(r, d);

Could the changelog please explain why this is needed and what the impact of
this is?

> +	}
> +
> +	return 0;
> +}

I think the naming can be changed to make these easier to understand. For example,
resctrl_abmc_msrwrite() -> resctrl_abmc_set_one()
resctrl_abmc_setup() -> resctrl_abmc_set_all()

> +
> +static int resctrl_abmc_enable(enum resctrl_res_level l)
> +{
> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +	int ret = 0;
> +
> +	if (!hw_res->abmc_enabled) {
> +		ret = resctrl_abmc_setup(l, true);
> +		if (!ret)
> +			hw_res->abmc_enabled = true;
> +	}
> +
> +	return ret;
> +}
> +
> +static void resctrl_abmc_disable(enum resctrl_res_level l)
> +{
> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +
> +	if (hw_res->abmc_enabled) {
> +		resctrl_abmc_setup(l, false);
> +		hw_res->abmc_enabled = false;
> +	}
> +}
> +
> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
> +{
> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +
> +	if (!hw_res->r_resctrl.mbm_assign_capable)
> +		return -EINVAL;
> +
> +	if (enable)
> +		return resctrl_abmc_enable(l);
> +
> +	resctrl_abmc_disable(l);
> +
> +	return 0;
> +}

Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary
layer of abstraction.

> +
>  /*
>   * We don't allow rdtgroup directories to be created anywhere
>   * except the root directory. Thus when looking for the rdtgroup

Reinette