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

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

On 02/10/2026 16:22, Ben Horgan wrote:
> Hi James,
> 
> On 02/10/2026 16:15, James Morse wrote:
>> 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)
> 
> I agree with your definition of correction but isn't that taken into
> account in __ris_msmon_read_locked().
> 
> Surely +=val here would lead to double counting.

Yes - I missed that.

I'll add a comment to that effect as (to me at least) this looks like a reset of that
correction value, which shouldn't happen while the counter is allocated.



Thanks!

James