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

["Shaopeng Tan (Fujitsu)" <tan.shaopeng@fujitsu.com>]

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