Re: [PATCH v1 05/11] arm_mpam: Ensure MBWU counters are reset on restore

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

Hi Shaopeng,

On 7/15/26 08:49, Shaopeng Tan (Fujitsu) wrote:
> Hello Ben,
> 
>> 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. Ensure the reset
>> happens by setting the reset_on_next_read and remove the unnecessary writes
>>from mpam_save_mbwu_state().
>>
>> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>>  drivers/resctrl/mpam_devices.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index b34e2a368516..222fc248067e 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1648,10 +1648,13 @@ 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;
>>  
>>          for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>> +               mbwu_state = &ris->mbwu_state[i];
>> +
>>                  if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>                          return -EIO;
>>  
>> @@ -1661,6 +1664,8 @@ static int mpam_restore_mbwu_state(void *_ris)
>>                          mwbu_arg.type = mpam_msmon_choose_counter(class);
>>                          mwbu_arg.val = &val;
>>  
>> +                       mbwu_state->reset_on_next_read = true;
>> +
>>                          mpam_mon_sel_unlock(msc);
> 
>         for (i = 0; i < ris->props.num_mbwu_mon; i++) {
> +               mbwu_state = &ris->mbwu_state[i];
> +
>                 if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>                         return -EIO;
> 
> -               if (ris->mbwu_state[i].enabled) {
> +               if (mbwu_state->enabled) {  //this line might need refactoring

I'm unclear on what you are trying to point out in this email. Please can you explain.

Thanks,

Ben

>                         mwbu_arg.ris = ris;
> -                       mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
> +                       mwbu_arg.ctx = &mbwu_state->cfg;  //and this line
>                         mwbu_arg.type = mpam_msmon_choose_counter(class);
>                         mwbu_arg.val = &val;
>  
> +                       mbwu_state->reset_on_next_read = true;
> +
>                         mpam_mon_sel_unlock(msc);
> 
> 
> Best regards,
> Shaopeng TAN
> 
>>                          __ris_msmon_read(&mwbu_arg);
>> @@ -1696,15 +1701,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);
>>  
>> -               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);
>> -               }
>>  
>>                  cfg->mon = i;
>>                  cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>> -- 
>> 2.43.0