Re: [PATCH v2 06/12] arm_mpam: Use __ris_msmon_read() for saving MBWU state

[James Morse <james.morse@arm.com>]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> mbwu_save_mbwu_state() reads the MBWU counters and adds that to a saved
> correction value. However, the type of counter to read is determined by the
> RIS rather than the class and overflow is not taken into account. Fix this
> and mitigate against further divergence by using a locked variant of the
> same helper used for user monitor reads, __ris_msmon_read(). Using the
> locked variant avoids having to drop and retake the mon_sel lock. If the
> lock was dropped, an interleaved monitor read which detects overflow would
> cause the overflow not to be accounted for in the saved value of
> mbwu_state->correction. The correction is no longer updated for disabled
> counters but this has no effect as the saved values are not expected to be
> useful for disabled counters.

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 6cba3ef21cc8..62562ce2f9aa 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c


> @@ -1688,10 +1693,12 @@ static int mpam_save_mbwu_state(void *arg)
>  	int i;
>  	u64 val;
>  	struct mon_cfg *cfg;
> +	struct mon_read mbwu_arg;
>  	u32 cur_flt, cur_ctl, mon_sel;
>  	struct mpam_msc_ris *ris = arg;
>  	struct msmon_mbwu_state *mbwu_state;
>  	struct mpam_msc *msc = ris->vmsc->msc;
> +	struct mpam_class *class = ris->vmsc->comp->class;
>  
>  	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>  		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> @@ -1707,17 +1714,31 @@ static int mpam_save_mbwu_state(void *arg)
>  		cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
>  		cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);

>  		cfg->mon = i;
>  		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>  		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
>  		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>  		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
> +
> +		if (!mbwu_state->enabled) {
> +			mpam_mon_sel_unlock(msc);
> +			continue;
> +		}
> +
> +		val = 0;
> +		mbwu_arg = (struct mon_read) {
> +			.ris = ris,
> +			.ctx = cfg,
> +			.type = mpam_msmon_choose_counter(class),
> +			.val = &val,
> +		};
> +
> +		__ris_msmon_read_locked(&mbwu_arg);
> +
> +		mbwu_state->reset_on_next_read = true;
> +		if (!mbwu_arg.err)
> +			mbwu_state->correction = val;

+= val?

If the same CPU is offlined twice, the correction should hold the sum of both values.
The idea is the 'correction' is anything that has been consumed, and isn't in the hardware
register. (e.g. due to overflow or reset)

With that:
Reviewed-by: James Morse <james.morse@arm.com>


> +
>  		mpam_mon_sel_unlock(msc);
>  	}
>  


Thanks,

James