Re: [PATCH v4 3/5] arm_mpam: resctrl: Add resctrl_arch_config_cntr() for ABMC use

[Fenghua Yu <fenghuay@nvidia.com>]

Hi, Ben,

On 5/20/26 14:24, Ben Horgan wrote:
> From: James Morse <james.morse@arm.com>
> 
> ABMC, mbm_event mode, has a helper resctrl_arch_config_cntr() for changing
> the mapping between 'cntr_id' and a CLOSID/RMID pair.
> 
> Add the helper.
> 
> For MPAM this is done by updating the mon->mbwu_idx_to_mon[] array, and as
> usual CDP means it needs doing in three different ways.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>

Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>

Please check following nits.

> ---
> Changes since new rfc:
> Mention mbm_event mode in commit message
> 
> Changes since v3:
> Warning bound (Sashiko)
> ---
>   drivers/resctrl/mpam_resctrl.c | 44 +++++++++++++++++++++++++++++-----
>   1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index a13eb232a19d..1f9a8ae157ca 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -127,12 +127,6 @@ void resctrl_arch_reset_cntr(struct rdt_resource *r, struct rdt_l3_mon_domain *d
>   {
>   }
>   
> -void resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_l3_mon_domain *d,
> -			      enum resctrl_event_id evtid, u32 rmid, u32 closid,
> -			      u32 cntr_id, bool assign)
> -{
> -}
> -
>   int resctrl_arch_cntr_read(struct rdt_resource *r, struct rdt_l3_mon_domain *d,
>   			   u32 unused, u32 rmid, int cntr_id,
>   			   enum resctrl_event_id eventid, u64 *val)
> @@ -1076,6 +1070,44 @@ static void mpam_resctrl_pick_counters(void)
>   	}
>   }
>   
> +static void __config_cntr(struct mpam_resctrl_mon *mon, u32 cntr_id,
> +			  enum resctrl_conf_type cdp_type, u32 closid, u32 rmid,
> +			  bool assign)
> +{
> +	u32 mbwu_idx, mon_idx = resctrl_get_config_index(cntr_id, cdp_type);

Nit:

Reuse resctrl_get_config_index() to get cntr_id->mon_idx which 
resctrl_get_config_index() handles closid:
/* For use by arch code to remap resctrl's smaller CDP CLOSID range */
static inline u32 resctrl_get_config_index(u32 closid,
                                            enum resctrl_conf_type type)

Logically it's correct. It's just a little confusing when reading it.

Maybe adding a comment to make reading easier?
+       /* Same CDP index remap as closid; maps cntr_id to 
assigned_counters[]. */

But it's up to you.

> +
> +	WARN_ON_ONCE(mon_idx >= l3_num_allocated_mbwu);> +
> +	closid = resctrl_get_config_index(closid, cdp_type);
> +	mbwu_idx = resctrl_arch_rmid_idx_encode(closid, rmid);
> +
> +	if (assign)
> +		mon->mbwu_idx_to_mon[mbwu_idx] = mon->assigned_counters[mon_idx];
> +	else
> +		mon->mbwu_idx_to_mon[mbwu_idx] = -1;
> +}
> +
> +void resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_l3_mon_domain *d,
> +			      enum resctrl_event_id evtid, u32 rmid, u32 closid,
> +			      u32 cntr_id, bool assign)
> +{
> +	struct mpam_resctrl_mon *mon = &mpam_resctrl_counters[evtid];

Add a sanity checking on evtid == QOS_L3_MBM_TOTAL_EVENT_ID here? To 
make sure this helper is not for llc occupancy event. And the sanity 
checking matches other functions in mpam_resctrl.c.

> +
> +	if (!mon->mbwu_idx_to_mon || !mon->assigned_counters) {
> +		pr_debug("monitor arrays not allocated\n");
> +		return;
> +	}
> +
> +	if (cdp_enabled) {
> +		__config_cntr(mon, cntr_id, CDP_CODE, closid, rmid, assign);
> +		__config_cntr(mon, cntr_id, CDP_DATA, closid, rmid, assign);
> +	} else {
> +		__config_cntr(mon, cntr_id, CDP_NONE, closid, rmid, assign);
> +	}
> +
> +	resctrl_arch_reset_rmid(r, d, closid, rmid, evtid);
> +}
> +
>   static int mpam_resctrl_control_init(struct mpam_resctrl_res *res)
>   {
>   	struct mpam_class *class = res->class;

Thanks.

-Fenghua