Re: [PATCH v2 05/12] arm_mpam: Ensure MBWU counters are reset on restore

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

Hi James,

On 02/10/2026 16:13, James Morse wrote:
> Hi Ben,
> 
> On 17/09/2026 15:56, Ben Horgan wrote:
>> When an MSC becomes inaccessible due to cpu offline CFG_MBWU_CTL is set to
>> zero in mpam_save_mbwu_state(). This is very likely to mean that the config
>> will mismatch when restoring and so the monitor will be reset. However, the
>> state may have been lost and so there are no guarantees.
> 
> Power management and kexec are the reason this is done.
> 
> I have a niggling suspicion that some hardware engineer may allow 'running counters'
> to inhibit power-down - which means the cache could stay on when all its CPUs are off.

Ah, I see.

> 
> We may kexec while these CPUs are off. Leaving the hardware in its reset state is
> the least surprising thing to do, and also means we don't get bitten by the above
> (theoretical) power management thing if the next kernel doesn't know about MPAM.

Make sense.

> 
> 
>> Ensure the reset happens by setting the reset_on_next_read
> 
> Doing this makes it more robust,
> 
> 
>> and remove the unnecessary writes from mpam_save_mbwu_state().
> 
> I think this is still a good thing to do.
> 
> 
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index d39d210574a6..6cba3ef21cc8 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1652,6 +1652,7 @@ static int mpam_restore_mbwu_state(void *_ris)
>>  	u64 val;
>>  	struct mon_read mwbu_arg;
>>  	struct mpam_msc_ris *ris = _ris;
>> +	struct msmon_mbwu_state *mbwu_state;
>>  	struct mpam_msc *msc = ris->vmsc->msc;
>>  	struct mpam_class *class = ris->vmsc->comp->class;
>>  
>> @@ -1659,16 +1660,20 @@ static int mpam_restore_mbwu_state(void *_ris)
>>  		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>  			return -EIO;
>>  
>> -		if (!ris->mbwu_state[i].enabled) {
>> +		mbwu_state = &ris->mbwu_state[i];
>> +
>> +		if (!mbwu_state->enabled) {
>>  			mpam_mon_sel_unlock(msc);
>>  			continue;
>>  		}
>>  
>>  		mwbu_arg.ris = ris;
>> -		mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>> +		mwbu_arg.ctx = &mbwu_state->cfg;
>>  		mwbu_arg.type = mpam_msmon_choose_counter(class);
>>  		mwbu_arg.val = &val;
>>  
>> +		mbwu_state->reset_on_next_read = true;
>> +
>>  		mpam_mon_sel_unlock(msc);
>>  
>>  		__ris_msmon_read(&mwbu_arg);
>> @@ -1701,15 +1706,11 @@ 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);
> 
>> -		mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
> 
> I plan to drop this line,

Ok, I agree.

Thanks,

Ben

> 
> 
>> -		if (mpam_ris_has_mbwu_long_counter(ris)) {
>> +		if (mpam_ris_has_mbwu_long_counter(ris))
>>  			val = mpam_msc_read_mbwu_l(msc);
>> -			mpam_msc_zero_mbwu_l(msc);
>> -		} else {
>> +		else
>>  			val = mpam_read_monsel_reg(msc, MBWU);
>> -			mpam_write_monsel_reg(msc, MBWU, 0);
>> -		}
> 
> But keep this. With reset_on_next_read the driver won't consume the stale value, and
> that approach also covers the hardware resetting into unusual states.
> 
> 
> Reviewed-by: James Morse <james.morse@arm.com>
> 
> 
> Thanks,
> 
> James