Re: [PATCH v14 17/32] x86/resctrl: Implement resctrl_arch_config_cntr() to assign a counter with ABMC

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

Hi Babu,

With the new arch API this have "x86,fs/resctrl" prefix.

On 6/13/25 2:05 PM, Babu Moger wrote:
> The ABMC feature allows users to assign a hardware counter ID to an RMID,
> event pair and monitor bandwidth usage as long as it is assigned. The
> hardware continues to track the assigned counter until it is explicitly
> unassigned by the user.
> 
> Implement an architecture-specific handler to assign and unassign the
> counter. Configure counters by writing to the L3_QOS_ABMC_CFG MSR,
> specifying the counter ID, bandwidth source (RMID), and event
> configuration.
> 
> The feature details are documented in the 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@amd.com>
> ---

...

> ---
>  arch/x86/kernel/cpu/resctrl/monitor.c | 38 +++++++++++++++++++++++++++
>  include/linux/resctrl.h               | 19 ++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 0ad9c731c13e..6b0ea4b17c7a 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -444,3 +444,41 @@ bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r)
>  {
>  	return resctrl_to_arch_res(r)->mbm_cntr_assign_enabled;
>  }
> +
> +static void resctrl_abmc_config_one_amd(void *info)
> +{
> +	union l3_qos_abmc_cfg *abmc_cfg = info;
> +
> +	wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg->full);
> +}
> +
> +/*
> + * Send an IPI to the domain to assign the counter to RMID, event pair.
> + */
> +void resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> +			      enum resctrl_event_id evtid, u32 rmid, u32 closid,
> +			      u32 cntr_id, bool assign)
> +{
> +	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
> +	union l3_qos_abmc_cfg abmc_cfg = { 0 };
> +	struct arch_mbm_state *am;
> +
> +	abmc_cfg.split.cfg_en = 1;
> +	abmc_cfg.split.cntr_en = assign ? 1 : 0;
> +	abmc_cfg.split.cntr_id = cntr_id;
> +	abmc_cfg.split.bw_src = rmid;
> +	if (assign)
> +		abmc_cfg.split.bw_type = resctrl_get_mon_evt_cfg(evtid);
> +
> +	smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &abmc_cfg, 1);

An optimization to consider is to direct the IPI to a housekeeping CPU.
If one exist, a further optimization could be to queue it on that CPU
instead of IPI. Your call since I am not familiar with the use cases here.
Looks like all paths leading to this is triggered by a user space action
(mount, mkdir, or write to update event config). This would require exposing
the housekeeping CPU code to arch.

> +
> +	/*
> +	 * The hardware counter is reset (because cfg_en == 1) so there is no
> +	 * need to record initial non-zero counts.
> +	 */
> +	if (assign) {
> +		am = get_arch_mbm_state(hw_dom, rmid, evtid);
> +		if (am)
> +			memset(am, 0, sizeof(*am));
> +	}

I am not able to recognize how the struct rdt_resource parameter is used. What am I missing?

> +}
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index a58dd40b7246..1539d1faa1a1 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -594,6 +594,25 @@ void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *
>   */
>  void resctrl_arch_reset_all_ctrls(struct rdt_resource *r);
>  
> +/**
> + * resctrl_arch_config_cntr() - Configure the counter with its new RMID
> + *				and event details.
> + * @r:			Resource structure.
> + * @d:			The domain in which the counter ID is to be configured.

"The domain in which the counter should be configured." or "The domain in which counter
with ID @cntr_id should be configured."?

> + * @evtid:		Monitoring event type (e.g., QOS_L3_MBM_TOTAL_EVENT_ID
> + *			or QOS_L3_MBM_LOCAL_EVENT_ID).
> + * @rmid:		RMID.
> + * @closid:		CLOSID.
> + * @cntr_id:		Counter ID to configure.
> + * @assign:		True to assign the counter, false to unassign
> + *			the counter.

The changelog and comments only mention counter "assignment" but this same call
is used to update an existing assignment, which on ABMC are done the same.
This may be ok for now but I think it will be helpful to amend the above to say
something like:

 * @assign:		True to assign the counter or update an existing assignment, false to unassign
 *			the counter.

> + *
> + * This can be called from any CPU.
> + */
> +void resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> +			      enum resctrl_event_id evtid, u32 rmid, u32 closid,
> +			      u32 cntr_id, bool assign);
> +
>  extern unsigned int resctrl_rmid_realloc_threshold;
>  extern unsigned int resctrl_rmid_realloc_limit;
>  

Reinette