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

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

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.

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.


> 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,


> -		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