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

[Ben Horgan <ben.horgan@arm.com>]

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.

Thanks,

Ben

> 
> With that:
> Reviewed-by: James Morse <james.morse@arm.com>
> 
> 
>> +
>>  		mpam_mon_sel_unlock(msc);
>>  	}
>>  
> 
> 
> Thanks,
> 
> James