[PATCH v2 00/12] arm_mpam: minor fixes at v7.2

[PATCH v2 00/12] arm_mpam: minor fixes at v7.2

[Ben Horgan]

Despite the subject this is still relevant at v7.3.  This series
addresses a few minor issues in the driver.  Since v1 I've added an
extra patch at the end to fix a leak and reworked the mbwu save/restore
fixes to avoid races by dropping the mon_sel lock in the middle.

Changelogs in patches

Based on v7.3-rc2

Ben Horgan (12):
  arm_mpam: Move MPAMF_ECR write helpers to allow reuse
  arm_mpam: Restore the error interrupt enable from mpam_cpu_online()
  arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth
    counters
  arm_mpam: Add missing mon_sel locking in MBWU save and restore
  arm_mpam: Ensure MBWU counters are reset on restore
  arm_mpam: Use __ris_msmon_read() for saving MBWU state
  arm_mpam: Initialize all of struct mon_read in
    mpam_restore_mbwu_state()
  arm_mpam: resctrl: Correct check that existing class is L3
  arm_mpam: resctrl: Make read_mon_cdp_safe() self consistent
  arm_mpam: Don't loop forever if there is the maximum possible amount
    of PARTIDs
  arm_mpam: Switch to kvzmalloc_objs() for allocation of component cfg
  arm_mpam: resctrl: Don't stop early when tearing down a class

 drivers/resctrl/mpam_devices.c | 161 ++++++++++++++++++++-------------
 drivers/resctrl/mpam_resctrl.c |  13 +--
 2 files changed, 103 insertions(+), 71 deletions(-)

-- 
2.43.0

[PATCH v2 01/12] arm_mpam: Move MPAMF_ECR write helpers to allow reuse

[Ben Horgan]

In preparation for calling mpam_enable_msc_ecr() from the CPU hotplug
handlers move it higher in the file. Move mpam_disable_msc_ecr() too to
keep them together.

Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
---
 drivers/resctrl/mpam_devices.c | 38 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index dd422c56fbb1..580d7ed38569 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1817,6 +1817,25 @@ static void mpam_reprogram_msc(struct mpam_msc *msc)
 	mutex_unlock(&msc->cfg_lock);
 }
 
+static int mpam_enable_msc_ecr(void *_msc)
+{
+	struct mpam_msc *msc = _msc;
+
+	__mpam_write_reg(msc, MPAMF_ECR, MPAMF_ECR_INTEN);
+
+	return 0;
+}
+
+/* This can run in mpam_disable(), and the interrupt handler on the same CPU */
+static int mpam_disable_msc_ecr(void *_msc)
+{
+	struct mpam_msc *msc = _msc;
+
+	__mpam_write_reg(msc, MPAMF_ECR, 0);
+
+	return 0;
+}
+
 static void _enable_percpu_irq(void *_irq)
 {
 	int *irq = _irq;
@@ -2443,25 +2462,6 @@ static char *mpam_errcode_names[16] = {
 	[12 ... 15] = "Reserved"
 };
 
-static int mpam_enable_msc_ecr(void *_msc)
-{
-	struct mpam_msc *msc = _msc;
-
-	__mpam_write_reg(msc, MPAMF_ECR, MPAMF_ECR_INTEN);
-
-	return 0;
-}
-
-/* This can run in mpam_disable(), and the interrupt handler on the same CPU */
-static int mpam_disable_msc_ecr(void *_msc)
-{
-	struct mpam_msc *msc = _msc;
-
-	__mpam_write_reg(msc, MPAMF_ECR, 0);
-
-	return 0;
-}
-
 static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
 {
 	u64 reg;
-- 
2.43.0

[PATCH v2 02/12] arm_mpam: Restore the error interrupt enable from mpam_cpu_online()

[Ben Horgan]

When all the associated CPUs to an MSC are offline, possibly caused by
PSCI_CPU_OFF, the power supply to the MSC can be cut off and all the
registers that are unknown out of reset, including MPAMF_ECR.INTEN, are
lost. Afterwards, we need to explicitly set MPAMF_ECR.INTEN when the first
associated CPU becomes online and the power supply to the MSC is recovered.

Restore MPAMF_ECR.INTEN in mpam_cpu_online() when the first associated CPU
of the MSC becomes online.

Fixes: 49aa621c4dca ("arm_mpam: Register and enable IRQs")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
---
Changes since v1:
Update the commit message (Gavin)
---
 drivers/resctrl/mpam_devices.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 580d7ed38569..32bde94dfbc3 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1856,8 +1856,14 @@ static int mpam_cpu_online(unsigned int cpu)
 		if (msc->reenable_error_ppi)
 			_enable_percpu_irq(&msc->reenable_error_ppi);
 
-		if (atomic_fetch_inc(&msc->online_refs) == 0)
+		if (atomic_fetch_inc(&msc->online_refs) == 0) {
+			mutex_lock(&msc->error_irq_lock);
+			if (msc->error_irq_hw_enabled)
+				mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
+			mutex_unlock(&msc->error_irq_lock);
+
 			mpam_reprogram_msc(msc);
+		}
 	}
 
 	if (mpam_resctrl_enabled)
-- 
2.43.0

[PATCH v2 03/12] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[Ben Horgan]

When MPAMF_MSMON_IDR.MSMON_MBWU is 1, MSMON_MBWU is present and has a VALUE
field of 31 bits. If additionally, MPAMF_MBWUMON_IDR.HAS_LONG is 1, then
MSMON_MBWU_L is also present and has a VALUE field of 44 or 63 bits as
indicated by MPAMF_MBWUMON_IDR.LWD. Hence, if there are memory bandwidth
counters then there are always 31 bit counters irrespective of whether
there also long counters.

Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
counters.

Fixes: fdc29a141d63 ("arm_mpam: Probe for long/lwd mbwu counters")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
---
 drivers/resctrl/mpam_devices.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 32bde94dfbc3..e349db525882 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -930,9 +930,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 						mpam_set_feature(mpam_feat_msmon_mbwu_63counter, props);
 					else
 						mpam_set_feature(mpam_feat_msmon_mbwu_44counter, props);
-				} else {
-					mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
 				}
+
+				mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
 			}
 		}
 	}
-- 
2.43.0

[PATCH v2 04/12] arm_mpam: Add missing mon_sel locking in MBWU save and restore

[Ben Horgan]

The mon_sel_lock is used to protect the mbwu_state, as well as h/w accesses
that use MPAMCFG_MON_SEL. However, in mpam_restore/save_mbwu_state(),
mbwu_state is accessed without holding the mon_sel_lock.

Add the missing locking.

Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
---
Changes since v1:

'continue' if monitor enabled
move locking earlier in mpam_save_mbwu_state() and adjust commit message
---
 drivers/resctrl/mpam_devices.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index e349db525882..d39d210574a6 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1652,17 +1652,26 @@ static int mpam_restore_mbwu_state(void *_ris)
 	u64 val;
 	struct mon_read mwbu_arg;
 	struct mpam_msc_ris *ris = _ris;
+	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++) {
-		if (ris->mbwu_state[i].enabled) {
-			mwbu_arg.ris = ris;
-			mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
-			mwbu_arg.type = mpam_msmon_choose_counter(class);
-			mwbu_arg.val = &val;
+		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
+			return -EIO;
 
-			__ris_msmon_read(&mwbu_arg);
+		if (!ris->mbwu_state[i].enabled) {
+			mpam_mon_sel_unlock(msc);
+			continue;
 		}
+
+		mwbu_arg.ris = ris;
+		mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
+		mwbu_arg.type = mpam_msmon_choose_counter(class);
+		mwbu_arg.val = &val;
+
+		mpam_mon_sel_unlock(msc);
+
+		__ris_msmon_read(&mwbu_arg);
 	}
 
 	return 0;
@@ -1680,12 +1689,12 @@ static int mpam_save_mbwu_state(void *arg)
 	struct mpam_msc *msc = ris->vmsc->msc;
 
 	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
-		mbwu_state = &ris->mbwu_state[i];
-		cfg = &mbwu_state->cfg;
-
 		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
 			return -EIO;
 
+		mbwu_state = &ris->mbwu_state[i];
+		cfg = &mbwu_state->cfg;
+
 		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
 			  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
 		mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
-- 
2.43.0

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

[Ben Horgan]

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>
Tested-by: Gavin Shan <gshan@redhat.com>
---
Changes since v1:
Use local variable mbwu_state consistently (Shaopeng)
Only access ris->mbwu_state when have the mon_sel_lock
---
 drivers/resctrl/mpam_devices.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

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

[PATCH v2 06/12] arm_mpam: Use __ris_msmon_read() for saving MBWU state

[Ben Horgan]

mbwu_save_mbwu_state() reads the MBWU counters and adds that to a saved
correction value. However, the type of counter to read is determined by the
RIS rather than the class and overflow is not taken into account. Fix this
and mitigate against further divergence by using a locked variant of the
same helper used for user monitor reads, __ris_msmon_read(). Using the
locked variant avoids having to drop and retake the mon_sel lock. If the
lock was dropped, an interleaved monitor read which detects overflow would
cause the overflow not to be accounted for in the saved value of
mbwu_state->correction. The correction is no longer updated for disabled
counters but this has no effect as the saved values are not expected to be
useful for disabled counters.

Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
---
Changes since v1:

'continue' if not monitor not enabled
add __ris_msmon_read() to avoid race on update of mbwu_state->correction (Sashiko)
update the commit message
---
 drivers/resctrl/mpam_devices.c | 63 ++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 6cba3ef21cc8..62562ce2f9aa 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1029,12 +1029,6 @@ struct mon_read {
 	bool				waited_timeout;
 };
 
-static bool mpam_ris_has_mbwu_long_counter(struct mpam_msc_ris *ris)
-{
-	return (mpam_has_feature(mpam_feat_msmon_mbwu_63counter, &ris->props) ||
-		mpam_has_feature(mpam_feat_msmon_mbwu_44counter, &ris->props));
-}
-
 static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
 {
 	int retry = 3;
@@ -1202,13 +1196,12 @@ static u64 mpam_msmon_overflow_val(enum mpam_device_features type,
 	return overflow_val;
 }
 
-static void __ris_msmon_read(void *arg)
+static void __ris_msmon_read_locked(struct mon_read *m)
 {
 	u64 now;
 	bool nrdy = false;
 	bool config_mismatch;
 	bool overflow = false;
-	struct mon_read *m = arg;
 	struct mon_cfg *ctx = m->ctx;
 	bool reset_on_next_read = false;
 	struct mpam_msc_ris *ris = m->ris;
@@ -1216,10 +1209,8 @@ static void __ris_msmon_read(void *arg)
 	struct mpam_msc *msc = m->ris->vmsc->msc;
 	u32 mon_sel, ctl_val, flt_val, cur_ctl, cur_flt;
 
-	if (!mpam_mon_sel_lock(msc)) {
-		m->err = -EIO;
-		return;
-	}
+	mpam_mon_sel_lock_held(msc);
+
 	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, ctx->mon) |
 		  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
 	mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
@@ -1312,7 +1303,6 @@ static void __ris_msmon_read(void *arg)
 	default:
 		m->err = -EINVAL;
 	}
-	mpam_mon_sel_unlock(msc);
 
 	if (nrdy)
 		m->err = -EBUSY;
@@ -1323,6 +1313,21 @@ static void __ris_msmon_read(void *arg)
 	*m->val += now;
 }
 
+static void __ris_msmon_read(void *arg)
+{
+	struct mon_read *m = arg;
+	struct mpam_msc *msc = m->ris->vmsc->msc;
+
+	if (!mpam_mon_sel_lock(msc)) {
+		m->err = -EIO;
+		return;
+	}
+
+	__ris_msmon_read_locked(m);
+
+	mpam_mon_sel_unlock(msc);
+}
+
 static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
 {
 	int err, any_err = 0;
@@ -1674,9 +1679,9 @@ static int mpam_restore_mbwu_state(void *_ris)
 
 		mbwu_state->reset_on_next_read = true;
 
-		mpam_mon_sel_unlock(msc);
+		__ris_msmon_read_locked(&mwbu_arg);
 
-		__ris_msmon_read(&mwbu_arg);
+		mpam_mon_sel_unlock(msc);
 	}
 
 	return 0;
@@ -1688,10 +1693,12 @@ static int mpam_save_mbwu_state(void *arg)
 	int i;
 	u64 val;
 	struct mon_cfg *cfg;
+	struct mon_read mbwu_arg;
 	u32 cur_flt, cur_ctl, mon_sel;
 	struct mpam_msc_ris *ris = arg;
 	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++) {
 		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
@@ -1707,17 +1714,31 @@ 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);
 
-		if (mpam_ris_has_mbwu_long_counter(ris))
-			val = mpam_msc_read_mbwu_l(msc);
-		else
-			val = mpam_read_monsel_reg(msc, MBWU);
-
 		cfg->mon = i;
 		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
 		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
 		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
-		mbwu_state->correction += val;
 		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
+
+		if (!mbwu_state->enabled) {
+			mpam_mon_sel_unlock(msc);
+			continue;
+		}
+
+		val = 0;
+		mbwu_arg = (struct mon_read) {
+			.ris = ris,
+			.ctx = cfg,
+			.type = mpam_msmon_choose_counter(class),
+			.val = &val,
+		};
+
+		__ris_msmon_read_locked(&mbwu_arg);
+
+		mbwu_state->reset_on_next_read = true;
+		if (!mbwu_arg.err)
+			mbwu_state->correction = val;
+
 		mpam_mon_sel_unlock(msc);
 	}
 
-- 
2.43.0

[PATCH v2 07/12] arm_mpam: Initialize all of struct mon_read in mpam_restore_mbwu_state()

[Ben Horgan]

m->err and *m->val may be read before initialization in __ris_msmon_read()
when called from mpam_restore_mbwu_state().

Initialize the whole struct mon_read in mpam_restore_mbwu_state() and fix
the spelling of mbwu in the name.

Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
---
Changes since v1:

Set val to 0 (Lee)
Use existing mbwu_arg variable
---
 drivers/resctrl/mpam_devices.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 62562ce2f9aa..cdc2d9d19e19 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1655,7 +1655,7 @@ static int mpam_restore_mbwu_state(void *_ris)
 {
 	int i;
 	u64 val;
-	struct mon_read mwbu_arg;
+	struct mon_read mbwu_arg;
 	struct mpam_msc_ris *ris = _ris;
 	struct msmon_mbwu_state *mbwu_state;
 	struct mpam_msc *msc = ris->vmsc->msc;
@@ -1672,14 +1672,17 @@ static int mpam_restore_mbwu_state(void *_ris)
 			continue;
 		}
 
-		mwbu_arg.ris = ris;
-		mwbu_arg.ctx = &mbwu_state->cfg;
-		mwbu_arg.type = mpam_msmon_choose_counter(class);
-		mwbu_arg.val = &val;
+		val = 0;
+		mbwu_arg = (struct mon_read) {
+			.ris = ris,
+			.ctx = &mbwu_state->cfg,
+			.type = mpam_msmon_choose_counter(class),
+			.val = &val,
+		};
 
 		mbwu_state->reset_on_next_read = true;
 
-		__ris_msmon_read_locked(&mwbu_arg);
+		__ris_msmon_read_locked(&mbwu_arg);
 
 		mpam_mon_sel_unlock(msc);
 	}
-- 
2.43.0

[PATCH v2 08/12] arm_mpam: resctrl: Correct check that existing class is L3

[Ben Horgan]

The class used to back mbm_total_bytes can be either at the L3 or the
memory. If a platform had candidate classes at both the memory and the L3
then, as the check for whether the existing class is L3 or not in
counter_update_class() is broken, the class that will be chosen depends on
which order the classes are considered and so the probe order.

Fix the check.

Fixes: 1458c4f05335 ("arm_mpam: resctrl: Add support for csu counters")
Tested-by: Gavin Shan <gshan@redhat.com>
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_resctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
index 9d223057953a..b825bd90d4bd 100644
--- a/drivers/resctrl/mpam_resctrl.c
+++ b/drivers/resctrl/mpam_resctrl.c
@@ -1076,7 +1076,7 @@ static void counter_update_class(enum resctrl_event_id evt_id,
 	struct mpam_class *existing_class = mpam_resctrl_counters[evt_id].class;
 
 	if (existing_class) {
-		if (class->level == 3) {
+		if (existing_class->level == 3) {
 			pr_debug("Existing class is L3 - L3 wins\n");
 			return;
 		}
-- 
2.43.0

[PATCH v2 09/12] arm_mpam: resctrl: Make read_mon_cdp_safe() self consistent

[Ben Horgan]

When read_mon_cdp_safe() is called with cdp_enabled equal to false then the
reading returned in val is the sum of the monitor values for the given
component. However, when cdp_enabled equal to true it is again the sum of
the monitor values but with the initial value of val also added. Both
resctrl_arch_cntr_read() and resctrl_arch_rmid_read() call
read_mon_cdp_safe() and are documented to return the val directly without
consideration of the older value.

 The val is always zeroed before read_mon_cdp_safe() is called except in
 __check_limbo() and __l3_mon_event_count_sum() but the latter is not
 relevant on MPAM systems.  The extra accumulation in __check_limbo() means
 when there are multiple RMIDs in limbo some of them may be marked dirty
 when they are already clean.

Change the cdp_enabled equals true case to match the false case and the
documentation of the resctrl arch hooks.

Fixes: fb56b29932ca ("arm_mpam: resctrl: Add resctrl_arch_rmid_read()")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
---
Add fixes tag as actually fixes an issue (Sashiko)
Update commit message
---
 drivers/resctrl/mpam_resctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
index b825bd90d4bd..321741a702fe 100644
--- a/drivers/resctrl/mpam_resctrl.c
+++ b/drivers/resctrl/mpam_resctrl.c
@@ -497,7 +497,7 @@ static int read_mon_cdp_safe(struct mpam_resctrl_mon *mon, struct mpam_component
 		if (err)
 			return err;
 
-		*val += code_val + data_val;
+		*val = code_val + data_val;
 		return 0;
 	}
 
-- 
2.43.0

[PATCH v2 10/12] arm_mpam: Don't loop forever if there is the maximum possible amount of PARTIDs

[Ben Horgan]

On a theoretical platform with the maximum possible number of usable
PARTIDs, 0XFFFF, the loops iterating over PARTID in mpam_reset_ris() and
mpam_reprogram_ris() will never reach their termination condition leading
to a system hang when register the cpu hotplug handlers when MPAM is
enabling.

Fix these loops.

Fixes: f188a36ca241 ("arm_mpam: Reset MSC controls from cpuhp callbacks")
Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
---
 drivers/resctrl/mpam_devices.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index cdc2d9d19e19..10fc5168699b 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -1754,7 +1754,7 @@ static int mpam_save_mbwu_state(void *arg)
  */
 static int mpam_reset_ris(void *arg)
 {
-	u16 partid, partid_max;
+	u16 partid_max;
 	struct mpam_config reset_cfg = {};
 	struct mpam_msc_ris *ris = arg;
 
@@ -1764,7 +1764,7 @@ static int mpam_reset_ris(void *arg)
 	spin_lock(&partid_max_lock);
 	partid_max = mpam_partid_max;
 	spin_unlock(&partid_max_lock);
-	for (partid = 0; partid <= partid_max; partid++)
+	for (u32 partid = 0; partid <= partid_max; partid++)
 		mpam_reprogram_ris_partid(ris, partid, &reset_cfg);
 
 	return 0;
@@ -1811,7 +1811,6 @@ static int __write_config(void *arg)
 
 static void mpam_reprogram_msc(struct mpam_msc *msc)
 {
-	u16 partid;
 	bool reset;
 	struct mpam_config *cfg;
 	struct mpam_msc_ris *ris;
@@ -1835,7 +1834,7 @@ static void mpam_reprogram_msc(struct mpam_msc *msc)
 		arg.comp = ris->vmsc->comp;
 		arg.ris = ris;
 		reset = true;
-		for (partid = 0; partid <= mpam_partid_max; partid++) {
+		for (u32 partid = 0; partid <= mpam_partid_max; partid++) {
 			cfg = &ris->vmsc->comp->cfg[partid];
 			if (!bitmap_empty(cfg->features, MPAM_FEATURE_LAST))
 				reset = false;
-- 
2.43.0

[PATCH v2 11/12] arm_mpam: Switch to kvzmalloc_objs() for allocation of component cfg

[Ben Horgan]

Each component cfg is sized by the size of a per-PARTID multiplied by the
number of PARTIDs. On a platform sized to push the limits of the
specification, the allocation using kzalloc_objs() will consistently
fail. Change to use kvzmalloc_objs() so that large allocations fall back to
vmalloc() based allocations.

Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Tested-by: Gavin Shan <gshan@redhat.com>
---
 drivers/resctrl/mpam_devices.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index 10fc5168699b..d74292f8cdfd 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -157,7 +157,7 @@ static void mpam_free_garbage(void)
 		if (iter->pdev)
 			devm_kfree(&iter->pdev->dev, iter->to_free);
 		else
-			kfree(iter->to_free);
+			kvfree(iter->to_free);
 	}
 }
 
@@ -2692,7 +2692,7 @@ static int __allocate_component_cfg(struct mpam_component *comp)
 	if (comp->cfg)
 		return 0;
 
-	comp->cfg = kzalloc_objs(*comp->cfg, mpam_partid_max + 1);
+	comp->cfg = kvzalloc_objs(*comp->cfg, mpam_partid_max + 1);
 	if (!comp->cfg)
 		return -ENOMEM;
 
-- 
2.43.0

[PATCH v2 12/12] arm_mpam: resctrl: Don't stop early when tearing down a class

[Ben Horgan]

An MPAM class can back more than one control and more than one
monitor. mpam_resctrl_teardown_class() iterates through all the controls
but stops if it finds a match which means classes backing more than one
class aren't torn down. The same for monitors, in which case the mbwu
counters will be leaked.

Stop terminating the loops early.

As we are already changing mpam_resctrl_teardown_class() remove the
misleading comment. It doesn't call resctrl_exit() and isn't required to do
so as the only caller does it first.

Fixes: fb481ec08699 ("arm_mpam: resctrl: Call resctrl_init() on platforms that can support resctrl")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
Changes since v1:

New patch
---
 drivers/resctrl/mpam_resctrl.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
index 321741a702fe..9cec5843e8fa 100644
--- a/drivers/resctrl/mpam_resctrl.c
+++ b/drivers/resctrl/mpam_resctrl.c
@@ -1943,10 +1943,6 @@ static void mpam_resctrl_teardown_mon(struct mpam_resctrl_mon *mon, struct mpam_
 	mon->mbwu_idx_to_mon = NULL;
 }
 
-/*
- * The driver is detaching an MSC from this class, if resctrl was using it,
- * pull on resctrl_exit().
- */
 void mpam_resctrl_teardown_class(struct mpam_class *class)
 {
 	struct mpam_resctrl_res *res;
@@ -1957,17 +1953,14 @@ void mpam_resctrl_teardown_class(struct mpam_class *class)
 	might_sleep();
 
 	for_each_mpam_resctrl_control(res, rid) {
-		if (res->class == class) {
+		if (res->class == class)
 			res->class = NULL;
-			break;
-		}
 	}
 	for_each_mpam_resctrl_mon(mon, eventid) {
 		if (mon->class == class) {
 			mon->class = NULL;
 
 			mpam_resctrl_teardown_mon(mon, class);
-			break;
 		}
 	}
 }
-- 
2.43.0

Re: [PATCH v2 01/12] arm_mpam: Move MPAMF_ECR write helpers to allow reuse

[Gavin Shan]

On 9/18/26 12:56 AM, Ben Horgan wrote:
> In preparation for calling mpam_enable_msc_ecr() from the CPU hotplug
> handlers move it higher in the file. Move mpam_disable_msc_ecr() too to
> keep them together.
> 
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> Tested-by: Gavin Shan <gshan@redhat.com>
> ---
>   drivers/resctrl/mpam_devices.c | 38 +++++++++++++++++-----------------
>   1 file changed, 19 insertions(+), 19 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v2 02/12] arm_mpam: Restore the error interrupt enable from mpam_cpu_online()

[Gavin Shan]

On 9/18/26 12:56 AM, Ben Horgan wrote:
> When all the associated CPUs to an MSC are offline, possibly caused by
> PSCI_CPU_OFF, the power supply to the MSC can be cut off and all the
> registers that are unknown out of reset, including MPAMF_ECR.INTEN, are
> lost. Afterwards, we need to explicitly set MPAMF_ECR.INTEN when the first
> associated CPU becomes online and the power supply to the MSC is recovered.
> 
> Restore MPAMF_ECR.INTEN in mpam_cpu_online() when the first associated CPU
> of the MSC becomes online.
> 
> Fixes: 49aa621c4dca ("arm_mpam: Register and enable IRQs")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> Tested-by: Gavin Shan <gshan@redhat.com>
> ---
> Changes since v1:
> Update the commit message (Gavin)
> ---
>   drivers/resctrl/mpam_devices.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v2 03/12] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[Gavin Shan]

On 9/18/26 12:56 AM, Ben Horgan wrote:
> When MPAMF_MSMON_IDR.MSMON_MBWU is 1, MSMON_MBWU is present and has a VALUE
> field of 31 bits. If additionally, MPAMF_MBWUMON_IDR.HAS_LONG is 1, then
> MSMON_MBWU_L is also present and has a VALUE field of 44 or 63 bits as
> indicated by MPAMF_MBWUMON_IDR.LWD. Hence, if there are memory bandwidth
> counters then there are always 31 bit counters irrespective of whether
> there also long counters.
> 
> Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
> counters.
> 
> Fixes: fdc29a141d63 ("arm_mpam: Probe for long/lwd mbwu counters")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> Tested-by: Gavin Shan <gshan@redhat.com>
> ---
>   drivers/resctrl/mpam_devices.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

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

[Gavin Shan]

On 9/18/26 12:56 AM, 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. 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>
> Tested-by: Gavin Shan <gshan@redhat.com>
> ---
> Changes since v1:
> Use local variable mbwu_state consistently (Shaopeng)
> Only access ris->mbwu_state when have the mon_sel_lock
> ---
>   drivers/resctrl/mpam_devices.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v2 08/12] arm_mpam: resctrl: Correct check that existing class is L3

[Gavin Shan]

On 9/18/26 12:56 AM, Ben Horgan wrote:
> The class used to back mbm_total_bytes can be either at the L3 or the
> memory. If a platform had candidate classes at both the memory and the L3
> then, as the check for whether the existing class is L3 or not in
> counter_update_class() is broken, the class that will be chosen depends on
> which order the classes are considered and so the probe order.
> 
> Fix the check.
> 
> Fixes: 1458c4f05335 ("arm_mpam: resctrl: Add support for csu counters")
> Tested-by: Gavin Shan <gshan@redhat.com>
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
>   drivers/resctrl/mpam_resctrl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v2 09/12] arm_mpam: resctrl: Make read_mon_cdp_safe() self consistent

[Gavin Shan]

On 9/18/26 12:56 AM, Ben Horgan wrote:
> When read_mon_cdp_safe() is called with cdp_enabled equal to false then the
> reading returned in val is the sum of the monitor values for the given
> component. However, when cdp_enabled equal to true it is again the sum of
> the monitor values but with the initial value of val also added. Both
> resctrl_arch_cntr_read() and resctrl_arch_rmid_read() call
> read_mon_cdp_safe() and are documented to return the val directly without
> consideration of the older value.
> 
>   The val is always zeroed before read_mon_cdp_safe() is called except in
>   __check_limbo() and __l3_mon_event_count_sum() but the latter is not
>   relevant on MPAM systems.  The extra accumulation in __check_limbo() means
>   when there are multiple RMIDs in limbo some of them may be marked dirty
>   when they are already clean.
> 
> Change the cdp_enabled equals true case to match the false case and the
> documentation of the resctrl arch hooks.
> 
> Fixes: fb56b29932ca ("arm_mpam: resctrl: Add resctrl_arch_rmid_read()")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> Tested-by: Gavin Shan <gshan@redhat.com>
> ---
> Add fixes tag as actually fixes an issue (Sashiko)
> Update commit message
> ---
>   drivers/resctrl/mpam_resctrl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v2 10/12] arm_mpam: Don't loop forever if there is the maximum possible amount of PARTIDs

[Gavin Shan]

On 9/18/26 12:56 AM, Ben Horgan wrote:
> On a theoretical platform with the maximum possible number of usable
> PARTIDs, 0XFFFF, the loops iterating over PARTID in mpam_reset_ris() and
> mpam_reprogram_ris() will never reach their termination condition leading
> to a system hang when register the cpu hotplug handlers when MPAM is
> enabling.
> 
> Fix these loops.
> 
> Fixes: f188a36ca241 ("arm_mpam: Reset MSC controls from cpuhp callbacks")
> Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> Tested-by: Gavin Shan <gshan@redhat.com>
> ---
>   drivers/resctrl/mpam_devices.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v2 11/12] arm_mpam: Switch to kvzmalloc_objs() for allocation of component cfg

[Gavin Shan]

On 9/18/26 12:56 AM, Ben Horgan wrote:
> Each component cfg is sized by the size of a per-PARTID multiplied by the
> number of PARTIDs. On a platform sized to push the limits of the
> specification, the allocation using kzalloc_objs() will consistently
> fail. Change to use kvzmalloc_objs() so that large allocations fall back to
> vmalloc() based allocations.
> 
> Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> Tested-by: Gavin Shan <gshan@redhat.com>
> ---
>   drivers/resctrl/mpam_devices.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v2 12/12] arm_mpam: resctrl: Don't stop early when tearing down a class

[Gavin Shan]

On 9/18/26 12:56 AM, Ben Horgan wrote:
> An MPAM class can back more than one control and more than one
> monitor. mpam_resctrl_teardown_class() iterates through all the controls
> but stops if it finds a match which means classes backing more than one
> class aren't torn down. The same for monitors, in which case the mbwu
> counters will be leaked.
> 
> Stop terminating the loops early.
> 
> As we are already changing mpam_resctrl_teardown_class() remove the
> misleading comment. It doesn't call resctrl_exit() and isn't required to do
> so as the only caller does it first.
> 
> Fixes: fb481ec08699 ("arm_mpam: resctrl: Call resctrl_init() on platforms that can support resctrl")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
> Changes since v1:
> 
> New patch
> ---
>   drivers/resctrl/mpam_resctrl.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

Re: [PATCH v2 01/12] arm_mpam: Move MPAMF_ECR write helpers to allow reuse

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> In preparation for calling mpam_enable_msc_ecr() from the CPU hotplug
> handlers move it higher in the file. Move mpam_disable_msc_ecr() too to
> keep them together.

Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

Re: [PATCH v2 02/12] arm_mpam: Restore the error interrupt enable from mpam_cpu_online()

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> When all the associated CPUs to an MSC are offline, possibly caused by
> PSCI_CPU_OFF, the power supply to the MSC can be cut off and all the
> registers that are unknown out of reset, including MPAMF_ECR.INTEN, are
> lost.

Surely all the registers lose their value - and what we get back is the
architecture's reset values. MPAMF_ECR doesn't seem to have one, we can assume
its UNKNOWN.

(I've rephrased this)


> Afterwards, we need to explicitly set MPAMF_ECR.INTEN when the first
> associated CPU becomes online and the power supply to the MSC is recovered.
> 
> Restore MPAMF_ECR.INTEN in mpam_cpu_online() when the first associated CPU
> of the MSC becomes online.

You won't be surprised to know I didn't have a platform that generated interrupts
when this was written!


> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 580d7ed38569..32bde94dfbc3 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1856,8 +1856,14 @@ static int mpam_cpu_online(unsigned int cpu)
>  		if (msc->reenable_error_ppi)
>  			_enable_percpu_irq(&msc->reenable_error_ppi);
>  
> -		if (atomic_fetch_inc(&msc->online_refs) == 0)
> +		if (atomic_fetch_inc(&msc->online_refs) == 0) {
> +			mutex_lock(&msc->error_irq_lock);
> +			if (msc->error_irq_hw_enabled)
> +				mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
> +			mutex_unlock(&msc->error_irq_lock);
> +
>  			mpam_reprogram_msc(msc);
> +		}
>  	}
>  
>  	if (mpam_resctrl_enabled)


Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

Re: [PATCH v2 03/12] arm_mpam: Set mpam_feat_msmon_mbwu_31counter when there are bandwidth counters

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> When MPAMF_MSMON_IDR.MSMON_MBWU is 1, MSMON_MBWU is present and has a VALUE
> field of 31 bits. If additionally, MPAMF_MBWUMON_IDR.HAS_LONG is 1, then
> MSMON_MBWU_L is also present and has a VALUE field of 44 or 63 bits as
> indicated by MPAMF_MBWUMON_IDR.LWD. Hence, if there are memory bandwidth
> counters then there are always 31 bit counters irrespective of whether
> there also long counters.
> 
> Set the 31 bit bandwidth counter feature bit whenever there are bandwidth
> counters.

Yes, that bitmap is supposed to be what is present, not what the driver is choosing
to use.


> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 32bde94dfbc3..e349db525882 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -930,9 +930,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>  						mpam_set_feature(mpam_feat_msmon_mbwu_63counter, props);
>  					else
>  						mpam_set_feature(mpam_feat_msmon_mbwu_44counter, props);
> -				} else {
> -					mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>  				}
> +
> +				mpam_set_feature(mpam_feat_msmon_mbwu_31counter, props);
>  			}
>  		}
>  	}


Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

Re: [PATCH v2 04/12] arm_mpam: Add missing mon_sel locking in MBWU save and restore

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> The mon_sel_lock is used to protect the mbwu_state, as well as h/w accesses
> that use MPAMCFG_MON_SEL. However, in mpam_restore/save_mbwu_state(),
> mbwu_state is accessed without holding the mon_sel_lock.

This is me being sloppy - I was only worried about concurrent writers, not
use of the array itself. But this is clearly better!


> Add the missing locking.


> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index e349db525882..d39d210574a6 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1652,17 +1652,26 @@ static int mpam_restore_mbwu_state(void *_ris)
>  	u64 val;
>  	struct mon_read mwbu_arg;
>  	struct mpam_msc_ris *ris = _ris;
> +	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++) {
> -		if (ris->mbwu_state[i].enabled) {
> -			mwbu_arg.ris = ris;
> -			mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
> -			mwbu_arg.type = mpam_msmon_choose_counter(class);
> -			mwbu_arg.val = &val;
> +		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> +			return -EIO;
>  
> -			__ris_msmon_read(&mwbu_arg);
> +		if (!ris->mbwu_state[i].enabled) {
> +			mpam_mon_sel_unlock(msc);
> +			continue;
>  		}
> +
> +		mwbu_arg.ris = ris;
> +		mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
> +		mwbu_arg.type = mpam_msmon_choose_counter(class);
> +		mwbu_arg.val = &val;

> +		mpam_mon_sel_unlock(msc);
> +
> +		__ris_msmon_read(&mwbu_arg);

Dropping and re-taking the lock in __ris_msmon_read() means everything we cached
in mbwu_arg could in principle change. I don't think it does - but if we're trying
to be robust/unsurprising: we probably need a __ris_msmon_read_locked(), or because of
that cfg pointer - always get the caller to take the lock.

This is better - so we shouldn't let perfect be the enemy of good.

~

Turns out you clean this up in a few patches time. It's theoretical, so the order
doesn't matter.


>  	}
>  
>  	return 0;
> @@ -1680,12 +1689,12 @@ static int mpam_save_mbwu_state(void *arg)
>  	struct mpam_msc *msc = ris->vmsc->msc;
>  
>  	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
> -		mbwu_state = &ris->mbwu_state[i];
> -		cfg = &mbwu_state->cfg;

Yeah, I was only worried about concurrent writes. But this is clearly better.


>  		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>  			return -EIO;
>  
> +		mbwu_state = &ris->mbwu_state[i];
> +		cfg = &mbwu_state->cfg;
To check - do you agree we don't need to worry about the array being free()d?
Because the first thing teardown does is disable the static-key and unregister
the cpuhp callbacks, it shouldn't be possible to reach this code before the
array gets freed...


>  		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
>  			  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
>  		mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);


Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

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

[James Morse]

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

Re: [PATCH v2 06/12] arm_mpam: Use __ris_msmon_read() for saving MBWU state

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> mbwu_save_mbwu_state() reads the MBWU counters and adds that to a saved
> correction value. However, the type of counter to read is determined by the
> RIS rather than the class and overflow is not taken into account. Fix this
> and mitigate against further divergence by using a locked variant of the
> same helper used for user monitor reads, __ris_msmon_read(). Using the
> locked variant avoids having to drop and retake the mon_sel lock. If the
> lock was dropped, an interleaved monitor read which detects overflow would
> cause the overflow not to be accounted for in the saved value of
> mbwu_state->correction. The correction is no longer updated for disabled
> counters but this has no effect as the saved values are not expected to be
> useful for disabled counters.

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 6cba3ef21cc8..62562ce2f9aa 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c


> @@ -1688,10 +1693,12 @@ static int mpam_save_mbwu_state(void *arg)
>  	int i;
>  	u64 val;
>  	struct mon_cfg *cfg;
> +	struct mon_read mbwu_arg;
>  	u32 cur_flt, cur_ctl, mon_sel;
>  	struct mpam_msc_ris *ris = arg;
>  	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++) {
>  		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> @@ -1707,17 +1714,31 @@ 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);

>  		cfg->mon = i;
>  		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>  		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
>  		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>  		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
> +
> +		if (!mbwu_state->enabled) {
> +			mpam_mon_sel_unlock(msc);
> +			continue;
> +		}
> +
> +		val = 0;
> +		mbwu_arg = (struct mon_read) {
> +			.ris = ris,
> +			.ctx = cfg,
> +			.type = mpam_msmon_choose_counter(class),
> +			.val = &val,
> +		};
> +
> +		__ris_msmon_read_locked(&mbwu_arg);
> +
> +		mbwu_state->reset_on_next_read = true;
> +		if (!mbwu_arg.err)
> +			mbwu_state->correction = val;

+= val?

If the same CPU is offlined twice, the correction should hold the sum of both values.
The idea is the 'correction' is anything that has been consumed, and isn't in the hardware
register. (e.g. due to overflow or reset)

With that:
Reviewed-by: James Morse <james.morse@arm.com>


> +
>  		mpam_mon_sel_unlock(msc);
>  	}
>  


Thanks,

James

Re: [PATCH v2 07/12] arm_mpam: Initialize all of struct mon_read in mpam_restore_mbwu_state()

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> m->err and *m->val may be read before initialization in __ris_msmon_read()
> when called from mpam_restore_mbwu_state().
> 
> Initialize the whole struct mon_read in mpam_restore_mbwu_state() and fix
> the spelling of mbwu in the name.

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 62562ce2f9aa..cdc2d9d19e19 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c

> @@ -1672,14 +1672,17 @@ static int mpam_restore_mbwu_state(void *_ris)
>  			continue;
>  		}
>  
> -		mwbu_arg.ris = ris;
> -		mwbu_arg.ctx = &mbwu_state->cfg;
> -		mwbu_arg.type = mpam_msmon_choose_counter(class);
> -		mwbu_arg.val = &val;
> +		val = 0;
> +		mbwu_arg = (struct mon_read) {
> +			.ris = ris,
> +			.ctx = &mbwu_state->cfg,
> +			.type = mpam_msmon_choose_counter(class),
> +			.val = &val,
> +		};
>  
>  		mbwu_state->reset_on_next_read = true;
>  
> -		__ris_msmon_read_locked(&mwbu_arg);
> +		__ris_msmon_read_locked(&mbwu_arg);
>  
>  		mpam_mon_sel_unlock(msc);
>  	}


Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

Re: [PATCH v2 08/12] arm_mpam: resctrl: Correct check that existing class is L3

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> The class used to back mbm_total_bytes can be either at the L3 or the
> memory. If a platform had candidate classes at both the memory and the L3
> then, as the check for whether the existing class is L3 or not in
> counter_update_class() is broken, the class that will be chosen depends on
> which order the classes are considered and so the probe order.

> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index 9d223057953a..b825bd90d4bd 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -1076,7 +1076,7 @@ static void counter_update_class(enum resctrl_event_id evt_id,
>  	struct mpam_class *existing_class = mpam_resctrl_counters[evt_id].class;
>  
>  	if (existing_class) {
> -		if (class->level == 3) {
> +		if (existing_class->level == 3) {
>  			pr_debug("Existing class is L3 - L3 wins\n");
>  			return;
>  		}

Heh, the debug message strong hints at how that was supposed to work!

Reviewed-by: James Morse <james.morse@arm.com>



Thanks,

James

Re: [PATCH v2 09/12] arm_mpam: resctrl: Make read_mon_cdp_safe() self consistent

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> When read_mon_cdp_safe() is called with cdp_enabled equal to false then the
> reading returned in val is the sum of the monitor values for the given
> component. However, when cdp_enabled equal to true it is again the sum of
> the monitor values but with the initial value of val also added. Both
> resctrl_arch_cntr_read() and resctrl_arch_rmid_read() call
> read_mon_cdp_safe() and are documented to return the val directly without
> consideration of the older value.
> 
>  The val is always zeroed before read_mon_cdp_safe() is called except in
>  __check_limbo() and __l3_mon_event_count_sum() but the latter is not
>  relevant on MPAM systems.  The extra accumulation in __check_limbo() means
>  when there are multiple RMIDs in limbo some of them may be marked dirty
>  when they are already clean.
Weird indenting - I'll fix that.


> Change the cdp_enabled equals true case to match the false case and the
> documentation of the resctrl arch hooks.

Nothing about the documentation of the resctrl arch hooks here...
I'll drop this part of the commit message.


> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index b825bd90d4bd..321741a702fe 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -497,7 +497,7 @@ static int read_mon_cdp_safe(struct mpam_resctrl_mon *mon, struct mpam_component
>  		if (err)
>  			return err;
>  
> -		*val += code_val + data_val;
> +		*val = code_val + data_val;
>  		return 0;
>  	}

I mistakenly thought mpam_msmon_read() was summing, so did the same here. But its not.

Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

Re: [PATCH v2 10/12] arm_mpam: Don't loop forever if there is the maximum possible amount of PARTIDs

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> On a theoretical platform with the maximum possible number of usable
> PARTIDs, 0XFFFF, the loops iterating over PARTID in mpam_reset_ris() and
> mpam_reprogram_ris() will never reach their termination condition leading
> to a system hang when register the cpu hotplug handlers when MPAM is
> enabling.
> 
> Fix these loops.

Ha! MPAM's mixture of num/max fields is really annoying.

Looks like we never loop over the PMG field.


> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index cdc2d9d19e19..10fc5168699b 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1754,7 +1754,7 @@ static int mpam_save_mbwu_state(void *arg)
>   */
>  static int mpam_reset_ris(void *arg)
>  {
> -	u16 partid, partid_max;
> +	u16 partid_max;
>  	struct mpam_config reset_cfg = {};
>  	struct mpam_msc_ris *ris = arg;
>  
> @@ -1764,7 +1764,7 @@ static int mpam_reset_ris(void *arg)
>  	spin_lock(&partid_max_lock);
>  	partid_max = mpam_partid_max;
>  	spin_unlock(&partid_max_lock);
> -	for (partid = 0; partid <= partid_max; partid++)
> +	for (u32 partid = 0; partid <= partid_max; partid++)
>  		mpam_reprogram_ris_partid(ris, partid, &reset_cfg);
>  
>  	return 0;
> @@ -1811,7 +1811,6 @@ static int __write_config(void *arg)
>  
>  static void mpam_reprogram_msc(struct mpam_msc *msc)
>  {
> -	u16 partid;
>  	bool reset;
>  	struct mpam_config *cfg;
>  	struct mpam_msc_ris *ris;
> @@ -1835,7 +1834,7 @@ static void mpam_reprogram_msc(struct mpam_msc *msc)
>  		arg.comp = ris->vmsc->comp;
>  		arg.ris = ris;
>  		reset = true;
> -		for (partid = 0; partid <= mpam_partid_max; partid++) {
> +		for (u32 partid = 0; partid <= mpam_partid_max; partid++) {
>  			cfg = &ris->vmsc->comp->cfg[partid];
>  			if (!bitmap_empty(cfg->features, MPAM_FEATURE_LAST))
>  				reset = false;

Reviewed-by: James Morse <james.morse@arm.com>



Thanks,

James

Re: [PATCH v2 11/12] arm_mpam: Switch to kvzmalloc_objs() for allocation of component cfg

[James Morse]

Hi Ben,

On 17/09/2026 15:56, Ben Horgan wrote:
> Each component cfg is sized by the size of a per-PARTID multiplied by the
> number of PARTIDs. On a platform sized to push the limits of the
> specification, the allocation using kzalloc_objs() will consistently
> fail. Change to use kvzmalloc_objs() so that large allocations fall back to
> vmalloc() based allocations.

Makes sense. We should do this for any allocation that could be larger than
a page.


> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 10fc5168699b..d74292f8cdfd 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -157,7 +157,7 @@ static void mpam_free_garbage(void)
>  		if (iter->pdev)
>  			devm_kfree(&iter->pdev->dev, iter->to_free);
>  		else
> -			kfree(iter->to_free);
> +			kvfree(iter->to_free);

And kvfree() is safe to use with non-vmalloc'd memory that we use for things
like struct class, which get free'd here.


>  	}
>  }
>  
> @@ -2692,7 +2692,7 @@ static int __allocate_component_cfg(struct mpam_component *comp)
>  	if (comp->cfg)
>  		return 0;
>  
> -	comp->cfg = kzalloc_objs(*comp->cfg, mpam_partid_max + 1);
> +	comp->cfg = kvzalloc_objs(*comp->cfg, mpam_partid_max + 1);
>  	if (!comp->cfg)
>  		return -ENOMEM;
>  


Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

Re: [PATCH v2 06/12] arm_mpam: Use __ris_msmon_read() for saving MBWU state

[Ben Horgan]

Hi James,

On 02/10/2026 16:15, James Morse wrote:
> Hi Ben,
> 
> On 17/09/2026 15:56, Ben Horgan wrote:
>> mbwu_save_mbwu_state() reads the MBWU counters and adds that to a saved
>> correction value. However, the type of counter to read is determined by the
>> RIS rather than the class and overflow is not taken into account. Fix this
>> and mitigate against further divergence by using a locked variant of the
>> same helper used for user monitor reads, __ris_msmon_read(). Using the
>> locked variant avoids having to drop and retake the mon_sel lock. If the
>> lock was dropped, an interleaved monitor read which detects overflow would
>> cause the overflow not to be accounted for in the saved value of
>> mbwu_state->correction. The correction is no longer updated for disabled
>> counters but this has no effect as the saved values are not expected to be
>> useful for disabled counters.
> 
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 6cba3ef21cc8..62562ce2f9aa 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
> 
> 
>> @@ -1688,10 +1693,12 @@ static int mpam_save_mbwu_state(void *arg)
>>  	int i;
>>  	u64 val;
>>  	struct mon_cfg *cfg;
>> +	struct mon_read mbwu_arg;
>>  	u32 cur_flt, cur_ctl, mon_sel;
>>  	struct mpam_msc_ris *ris = arg;
>>  	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++) {
>>  		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>> @@ -1707,17 +1714,31 @@ 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);
> 
>>  		cfg->mon = i;
>>  		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>>  		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
>>  		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>>  		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
>> +
>> +		if (!mbwu_state->enabled) {
>> +			mpam_mon_sel_unlock(msc);
>> +			continue;
>> +		}
>> +
>> +		val = 0;
>> +		mbwu_arg = (struct mon_read) {
>> +			.ris = ris,
>> +			.ctx = cfg,
>> +			.type = mpam_msmon_choose_counter(class),
>> +			.val = &val,
>> +		};
>> +
>> +		__ris_msmon_read_locked(&mbwu_arg);
>> +
>> +		mbwu_state->reset_on_next_read = true;
>> +		if (!mbwu_arg.err)
>> +			mbwu_state->correction = val;
> 
> += val?
> 
> If the same CPU is offlined twice, the correction should hold the sum of both values.
> The idea is the 'correction' is anything that has been consumed, and isn't in the hardware
> register. (e.g. due to overflow or reset)

I agree with your definition of correction but isn't that taken into
account in __ris_msmon_read_locked().

Surely +=val here would lead to double counting.

Thanks,

Ben

> 
> With that:
> Reviewed-by: James Morse <james.morse@arm.com>
> 
> 
>> +
>>  		mpam_mon_sel_unlock(msc);
>>  	}
>>  
> 
> 
> Thanks,
> 
> James

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

[Ben Horgan]

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

Re: [PATCH v2 04/12] arm_mpam: Add missing mon_sel locking in MBWU save and restore

[Ben Horgan]

Hi James,

On 02/10/2026 16:01, James Morse wrote:
> Hi Ben,
> 
> On 17/09/2026 15:56, Ben Horgan wrote:
>> The mon_sel_lock is used to protect the mbwu_state, as well as h/w accesses
>> that use MPAMCFG_MON_SEL. However, in mpam_restore/save_mbwu_state(),
>> mbwu_state is accessed without holding the mon_sel_lock.
> 
> This is me being sloppy - I was only worried about concurrent writers, not
> use of the array itself. But this is clearly better!
> 
> 
>> Add the missing locking.
> 
> 
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index e349db525882..d39d210574a6 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1652,17 +1652,26 @@ static int mpam_restore_mbwu_state(void *_ris)
>>  	u64 val;
>>  	struct mon_read mwbu_arg;
>>  	struct mpam_msc_ris *ris = _ris;
>> +	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++) {
>> -		if (ris->mbwu_state[i].enabled) {
>> -			mwbu_arg.ris = ris;
>> -			mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>> -			mwbu_arg.type = mpam_msmon_choose_counter(class);
>> -			mwbu_arg.val = &val;
>> +		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>> +			return -EIO;
>>  
>> -			__ris_msmon_read(&mwbu_arg);
>> +		if (!ris->mbwu_state[i].enabled) {
>> +			mpam_mon_sel_unlock(msc);
>> +			continue;
>>  		}
>> +
>> +		mwbu_arg.ris = ris;
>> +		mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>> +		mwbu_arg.type = mpam_msmon_choose_counter(class);
>> +		mwbu_arg.val = &val;
> 
>> +		mpam_mon_sel_unlock(msc);
>> +
>> +		__ris_msmon_read(&mwbu_arg);
> 
> Dropping and re-taking the lock in __ris_msmon_read() means everything we cached
> in mbwu_arg could in principle change. I don't think it does - but if we're trying
> to be robust/unsurprising: we probably need a __ris_msmon_read_locked(), or because of
> that cfg pointer - always get the caller to take the lock.
> 
> This is better - so we shouldn't let perfect be the enemy of good.
> 
> ~
> 
> Turns out you clean this up in a few patches time. It's theoretical, so the order
> doesn't matter.
> 
> 
>>  	}
>>  
>>  	return 0;
>> @@ -1680,12 +1689,12 @@ static int mpam_save_mbwu_state(void *arg)
>>  	struct mpam_msc *msc = ris->vmsc->msc;
>>  
>>  	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>> -		mbwu_state = &ris->mbwu_state[i];
>> -		cfg = &mbwu_state->cfg;
> 
> Yeah, I was only worried about concurrent writes. But this is clearly better.
> 
> 
>>  		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>  			return -EIO;
>>  
>> +		mbwu_state = &ris->mbwu_state[i];
>> +		cfg = &mbwu_state->cfg;
> To check - do you agree we don't need to worry about the array being free()d?
> Because the first thing teardown does is disable the static-key and unregister
> the cpuhp callbacks, it shouldn't be possible to reach this code before the
> array gets freed...

Yeah, fine for now. We'll have to think more when we fix the unbinding.

Thanks,

Ben

> 
> 
>>  		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
>>  			  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx);
>>  		mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel);
> 
> 
> Reviewed-by: James Morse <james.morse@arm.com>
> 
> 
> Thanks,
> 
> James

Re: [PATCH v2 06/12] arm_mpam: Use __ris_msmon_read() for saving MBWU state

[James Morse]

On 02/10/2026 16:22, Ben Horgan wrote:
> Hi James,
> 
> On 02/10/2026 16:15, James Morse wrote:
>> Hi Ben,
>>
>> On 17/09/2026 15:56, Ben Horgan wrote:
>>> mbwu_save_mbwu_state() reads the MBWU counters and adds that to a saved
>>> correction value. However, the type of counter to read is determined by the
>>> RIS rather than the class and overflow is not taken into account. Fix this
>>> and mitigate against further divergence by using a locked variant of the
>>> same helper used for user monitor reads, __ris_msmon_read(). Using the
>>> locked variant avoids having to drop and retake the mon_sel lock. If the
>>> lock was dropped, an interleaved monitor read which detects overflow would
>>> cause the overflow not to be accounted for in the saved value of
>>> mbwu_state->correction. The correction is no longer updated for disabled
>>> counters but this has no effect as the saved values are not expected to be
>>> useful for disabled counters.
>>
>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>> index 6cba3ef21cc8..62562ce2f9aa 100644
>>> --- a/drivers/resctrl/mpam_devices.c
>>> +++ b/drivers/resctrl/mpam_devices.c
>>
>>
>>> @@ -1688,10 +1693,12 @@ static int mpam_save_mbwu_state(void *arg)
>>>  	int i;
>>>  	u64 val;
>>>  	struct mon_cfg *cfg;
>>> +	struct mon_read mbwu_arg;
>>>  	u32 cur_flt, cur_ctl, mon_sel;
>>>  	struct mpam_msc_ris *ris = arg;
>>>  	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++) {
>>>  		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>> @@ -1707,17 +1714,31 @@ 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);
>>
>>>  		cfg->mon = i;
>>>  		cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>>>  		cfg->match_pmg = FIELD_GET(MSMON_CFG_x_CTL_MATCH_PMG, cur_ctl);
>>>  		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>>>  		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
>>> +
>>> +		if (!mbwu_state->enabled) {
>>> +			mpam_mon_sel_unlock(msc);
>>> +			continue;
>>> +		}
>>> +
>>> +		val = 0;
>>> +		mbwu_arg = (struct mon_read) {
>>> +			.ris = ris,
>>> +			.ctx = cfg,
>>> +			.type = mpam_msmon_choose_counter(class),
>>> +			.val = &val,
>>> +		};
>>> +
>>> +		__ris_msmon_read_locked(&mbwu_arg);
>>> +
>>> +		mbwu_state->reset_on_next_read = true;
>>> +		if (!mbwu_arg.err)
>>> +			mbwu_state->correction = val;
>>
>> += val?
>>
>> If the same CPU is offlined twice, the correction should hold the sum of both values.
>> The idea is the 'correction' is anything that has been consumed, and isn't in the hardware
>> register. (e.g. due to overflow or reset)
> 
> I agree with your definition of correction but isn't that taken into
> account in __ris_msmon_read_locked().
> 
> Surely +=val here would lead to double counting.

Yes - I missed that.

I'll add a comment to that effect as (to me at least) this looks like a reset of that
correction value, which shouldn't happen while the counter is allocated.



Thanks!

James