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

[Shaopeng Tan (Fujitsu)]

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

Inside the for loop, you introduced `mbwu_state = &ris->mbwu_state[i];` a few lines earlier, 
and then used `mbwu_state->reset_on_next_read = true;` shortly after.
Therefore, using mbwu_state->enabled and mbwu_state->cfg keeps 
the whole block consistent and avoids repeating ris->mbwu_state[i] multiple times. 

Best regards,
Shaopeng TAN

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