Expand the probing support with the control and monitor types
we can use with resctrl.

CC: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: James Morse <james.morse@arm.com>
---
 drivers/platform/arm64/mpam/mpam_devices.c  | 154 +++++++++++++++++++-
 drivers/platform/arm64/mpam/mpam_internal.h |  53 +++++++
 2 files changed, 206 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
index 8646fb85ad09..61911831ab39 100644
--- a/drivers/platform/arm64/mpam/mpam_devices.c
+++ b/drivers/platform/arm64/mpam/mpam_devices.c
@@ -102,7 +102,7 @@ static LLIST_HEAD(mpam_garbage);
 
 static u32 __mpam_read_reg(struct mpam_msc *msc, u16 reg)
 {
-	WARN_ON_ONCE(reg > msc->mapped_hwpage_sz);
+	WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
 	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
 
 	return readl_relaxed(msc->mapped_hwpage + reg);
@@ -131,6 +131,20 @@ static inline void _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 va
 }
 #define mpam_write_partsel_reg(msc, reg, val)  _mpam_write_partsel_reg(msc, MPAMCFG_##reg, val)
 
+static inline u32 _mpam_read_monsel_reg(struct mpam_msc *msc, u16 reg)
+{
+	mpam_mon_sel_lock_held(msc);
+	return __mpam_read_reg(msc, reg);
+}
+#define mpam_read_monsel_reg(msc, reg) _mpam_read_monsel_reg(msc, MSMON_##reg)
+
+static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
+{
+	mpam_mon_sel_lock_held(msc);
+	__mpam_write_reg(msc, reg, val);
+}
+#define mpam_write_monsel_reg(msc, reg, val)   _mpam_write_monsel_reg(msc, MSMON_##reg, val)
+
 static u64 mpam_msc_read_idr(struct mpam_msc *msc)
 {
 	u64 idr_high = 0, idr_low;
@@ -645,6 +659,137 @@ static struct mpam_msc_ris *mpam_get_or_create_ris(struct mpam_msc *msc,
 	return found;
 }
 
+/*
+ * IHI009A.a has this nugget: "If a monitor does not support automatic behaviour
+ * of NRDY, software can use this bit for any purpose" - so hardware might not
+ * implement this - but it isn't RES0.
+ *
+ * Try and see what values stick in this bit. If we can write either value,
+ * its probably not implemented by hardware.
+ */
+#define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result)			\
+do {										\
+	u32 now;								\
+	u64 mon_sel;								\
+	bool can_set, can_clear;						\
+	struct mpam_msc *_msc = _ris->vmsc->msc;				\
+										\
+	if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(_msc))) {			\
+		_result = false;						\
+		break;								\
+	}									\
+	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) |			\
+		  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, _ris->ris_idx);		\
+	mpam_write_monsel_reg(_msc, CFG_MON_SEL, mon_sel);			\
+										\
+	mpam_write_monsel_reg(_msc, _mon_reg, MSMON___NRDY);			\
+	now = mpam_read_monsel_reg(_msc, _mon_reg);				\
+	can_set = now & MSMON___NRDY;						\
+										\
+	mpam_write_monsel_reg(_msc, _mon_reg, 0);				\
+	now = mpam_read_monsel_reg(_msc, _mon_reg);				\
+	can_clear = !(now & MSMON___NRDY);					\
+	mpam_mon_sel_inner_unlock(_msc);					\
+										\
+	_result = (!can_set || !can_clear);					\
+} while (0)
+
+static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
+{
+	int err;
+	struct mpam_msc *msc = ris->vmsc->msc;
+	struct mpam_props *props = &ris->props;
+
+	lockdep_assert_held(&msc->probe_lock);
+	lockdep_assert_held(&msc->part_sel_lock);
+
+	/* Cache Portion partitioning */
+	if (FIELD_GET(MPAMF_IDR_HAS_CPOR_PART, ris->idr)) {
+		u32 cpor_features = mpam_read_partsel_reg(msc, CPOR_IDR);
+
+		props->cpbm_wd = FIELD_GET(MPAMF_CPOR_IDR_CPBM_WD, cpor_features);
+		if (props->cpbm_wd)
+			mpam_set_feature(mpam_feat_cpor_part, props);
+	}
+
+	/* Memory bandwidth partitioning */
+	if (FIELD_GET(MPAMF_IDR_HAS_MBW_PART, ris->idr)) {
+		u32 mbw_features = mpam_read_partsel_reg(msc, MBW_IDR);
+
+		/* portion bitmap resolution */
+		props->mbw_pbm_bits = FIELD_GET(MPAMF_MBW_IDR_BWPBM_WD, mbw_features);
+		if (props->mbw_pbm_bits &&
+		    FIELD_GET(MPAMF_MBW_IDR_HAS_PBM, mbw_features))
+			mpam_set_feature(mpam_feat_mbw_part, props);
+
+		props->bwa_wd = FIELD_GET(MPAMF_MBW_IDR_BWA_WD, mbw_features);
+		if (props->bwa_wd && FIELD_GET(MPAMF_MBW_IDR_HAS_MAX, mbw_features))
+			mpam_set_feature(mpam_feat_mbw_max, props);
+	}
+
+	/* Performance Monitoring */
+	if (FIELD_GET(MPAMF_IDR_HAS_MSMON, ris->idr)) {
+		u32 msmon_features = mpam_read_partsel_reg(msc, MSMON_IDR);
+
+		/*
+		 * If the firmware max-nrdy-us property is missing, the
+		 * CSU counters can't be used. Should we wait forever?
+		 */
+		err = device_property_read_u32(&msc->pdev->dev,
+					       "arm,not-ready-us",
+					       &msc->nrdy_usec);
+
+		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_CSU, msmon_features)) {
+			u32 csumonidr;
+
+			csumonidr = mpam_read_partsel_reg(msc, CSUMON_IDR);
+			props->num_csu_mon = FIELD_GET(MPAMF_CSUMON_IDR_NUM_MON, csumonidr);
+			if (props->num_csu_mon) {
+				bool hw_managed;
+
+				mpam_set_feature(mpam_feat_msmon_csu, props);
+
+				/* Is NRDY hardware managed? */
+				mpam_mon_sel_outer_lock(msc);
+				mpam_ris_hw_probe_hw_nrdy(ris, CSU, hw_managed);
+				mpam_mon_sel_outer_unlock(msc);
+				if (hw_managed)
+					mpam_set_feature(mpam_feat_msmon_csu_hw_nrdy, props);
+			}
+
+			/*
+			 * Accept the missing firmware property if NRDY appears
+			 * un-implemented.
+			 */
+			if (err && mpam_has_feature(mpam_feat_msmon_csu_hw_nrdy, props))
+				pr_err_once("Counters are not usable because not-ready timeout was not provided by firmware.");
+		}
+		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_MBWU, msmon_features)) {
+			bool hw_managed;
+			u32 mbwumonidr = mpam_read_partsel_reg(msc, MBWUMON_IDR);
+
+			props->num_mbwu_mon = FIELD_GET(MPAMF_MBWUMON_IDR_NUM_MON, mbwumonidr);
+			if (props->num_mbwu_mon)
+				mpam_set_feature(mpam_feat_msmon_mbwu, props);
+
+			if (FIELD_GET(MPAMF_MBWUMON_IDR_HAS_RWBW, mbwumonidr))
+				mpam_set_feature(mpam_feat_msmon_mbwu_rwbw, props);
+
+			/* Is NRDY hardware managed? */
+			mpam_mon_sel_outer_lock(msc);
+			mpam_ris_hw_probe_hw_nrdy(ris, MBWU, hw_managed);
+			mpam_mon_sel_outer_unlock(msc);
+			if (hw_managed)
+				mpam_set_feature(mpam_feat_msmon_mbwu_hw_nrdy, props);
+
+			/*
+			 * Don't warn about any missing firmware property for
+			 * MBWU NRDY - it doesn't make any sense!
+			 */
+		}
+	}
+}
+
 static int mpam_msc_hw_probe(struct mpam_msc *msc)
 {
 	u64 idr;
@@ -665,6 +810,7 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 
 	idr = mpam_msc_read_idr(msc);
 	mutex_unlock(&msc->part_sel_lock);
+
 	msc->ris_max = FIELD_GET(MPAMF_IDR_RIS_MAX, idr);
 
 	/* Use these values so partid/pmg always starts with a valid value */
@@ -685,6 +831,12 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 		ris = mpam_get_or_create_ris(msc, ris_idx);
 		if (IS_ERR(ris))
 			return PTR_ERR(ris);
+		ris->idr = idr;
+
+		mutex_lock(&msc->part_sel_lock);
+		__mpam_part_sel(ris_idx, 0, msc);
+		mpam_ris_hw_probe(ris);
+		mutex_unlock(&msc->part_sel_lock);
 	}
 
 	spin_lock(&partid_max_lock);
diff --git a/drivers/platform/arm64/mpam/mpam_internal.h b/drivers/platform/arm64/mpam/mpam_internal.h
index 42a454d5f914..ae6fd1f62cc4 100644
--- a/drivers/platform/arm64/mpam/mpam_internal.h
+++ b/drivers/platform/arm64/mpam/mpam_internal.h
@@ -136,6 +136,55 @@ static inline void mpam_mon_sel_lock_held(struct mpam_msc *msc)
 		lockdep_assert_preemption_enabled();
 }
 
+/*
+ * When we compact the supported features, we don't care what they are.
+ * Storing them as a bitmap makes life easy.
+ */
+typedef u16 mpam_features_t;
+
+/* Bits for mpam_features_t */
+enum mpam_device_features {
+	mpam_feat_ccap_part = 0,
+	mpam_feat_cpor_part,
+	mpam_feat_mbw_part,
+	mpam_feat_mbw_min,
+	mpam_feat_mbw_max,
+	mpam_feat_mbw_prop,
+	mpam_feat_msmon,
+	mpam_feat_msmon_csu,
+	mpam_feat_msmon_csu_capture,
+	mpam_feat_msmon_csu_hw_nrdy,
+	mpam_feat_msmon_mbwu,
+	mpam_feat_msmon_mbwu_capture,
+	mpam_feat_msmon_mbwu_rwbw,
+	mpam_feat_msmon_mbwu_hw_nrdy,
+	mpam_feat_msmon_capt,
+	MPAM_FEATURE_LAST,
+};
+#define MPAM_ALL_FEATURES      ((1 << MPAM_FEATURE_LAST) - 1)
+
+struct mpam_props {
+	mpam_features_t		features;
+
+	u16			cpbm_wd;
+	u16			mbw_pbm_bits;
+	u16			bwa_wd;
+	u16			num_csu_mon;
+	u16			num_mbwu_mon;
+};
+
+static inline bool mpam_has_feature(enum mpam_device_features feat,
+				    struct mpam_props *props)
+{
+	return (1 << feat) & props->features;
+}
+
+static inline void mpam_set_feature(enum mpam_device_features feat,
+				    struct mpam_props *props)
+{
+	props->features |= (1 << feat);
+}
+
 struct mpam_class {
 	/* mpam_components in this class */
 	struct list_head	components;
@@ -175,6 +224,8 @@ struct mpam_vmsc {
 	/* mpam_msc_ris in this vmsc */
 	struct list_head	ris;
 
+	struct mpam_props	props;
+
 	/* All RIS in this vMSC are members of this MSC */
 	struct mpam_msc		*msc;
 
@@ -186,6 +237,8 @@ struct mpam_vmsc {
 
 struct mpam_msc_ris {
 	u8			ris_idx;
+	u64			idr;
+	struct mpam_props	props;
 
 	cpumask_t		affinity;
 
-- 
2.39.5

Hi James,

On 7/11/25 19:36, James Morse wrote:
> Expand the probing support with the control and monitor types
> we can use with resctrl.
> 
> CC: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>   drivers/platform/arm64/mpam/mpam_devices.c  | 154 +++++++++++++++++++-
>   drivers/platform/arm64/mpam/mpam_internal.h |  53 +++++++
>   2 files changed, 206 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
> index 8646fb85ad09..61911831ab39 100644
> --- a/drivers/platform/arm64/mpam/mpam_devices.c
> +++ b/drivers/platform/arm64/mpam/mpam_devices.c
> @@ -102,7 +102,7 @@ static LLIST_HEAD(mpam_garbage);
>   
>   static u32 __mpam_read_reg(struct mpam_msc *msc, u16 reg)
>   {
> -	WARN_ON_ONCE(reg > msc->mapped_hwpage_sz);
> +	WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
>   	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
>   
>   	return readl_relaxed(msc->mapped_hwpage + reg);
> @@ -131,6 +131,20 @@ static inline void _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 va
>   }
>   #define mpam_write_partsel_reg(msc, reg, val)  _mpam_write_partsel_reg(msc, MPAMCFG_##reg, val)
>   
> +static inline u32 _mpam_read_monsel_reg(struct mpam_msc *msc, u16 reg)
> +{
> +	mpam_mon_sel_lock_held(msc);
> +	return __mpam_read_reg(msc, reg);
> +}
> +#define mpam_read_monsel_reg(msc, reg) _mpam_read_monsel_reg(msc, MSMON_##reg)
> +
> +static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
> +{
> +	mpam_mon_sel_lock_held(msc);
> +	__mpam_write_reg(msc, reg, val);
> +}
> +#define mpam_write_monsel_reg(msc, reg, val)   _mpam_write_monsel_reg(msc, MSMON_##reg, val)
> +
>   static u64 mpam_msc_read_idr(struct mpam_msc *msc)
>   {
>   	u64 idr_high = 0, idr_low;
> @@ -645,6 +659,137 @@ static struct mpam_msc_ris *mpam_get_or_create_ris(struct mpam_msc *msc,
>   	return found;
>   }
>   
> +/*
> + * IHI009A.a has this nugget: "If a monitor does not support automatic behaviour
> + * of NRDY, software can use this bit for any purpose" - so hardware might not
> + * implement this - but it isn't RES0.
> + *
> + * Try and see what values stick in this bit. If we can write either value,
> + * its probably not implemented by hardware.
> + */
> +#define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result)			\
> +do {										\
> +	u32 now;								\
> +	u64 mon_sel;								\
> +	bool can_set, can_clear;						\
> +	struct mpam_msc *_msc = _ris->vmsc->msc;				\
> +										\
> +	if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(_msc))) {			\
> +		_result = false;						\
> +		break;								\
> +	}									\
> +	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) |			\
> +		  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, _ris->ris_idx);		\
> +	mpam_write_monsel_reg(_msc, CFG_MON_SEL, mon_sel);			\
> +										\
> +	mpam_write_monsel_reg(_msc, _mon_reg, MSMON___NRDY);			\
> +	now = mpam_read_monsel_reg(_msc, _mon_reg);				\
> +	can_set = now & MSMON___NRDY;						\
> +										\
> +	mpam_write_monsel_reg(_msc, _mon_reg, 0);				\
> +	now = mpam_read_monsel_reg(_msc, _mon_reg);				\
> +	can_clear = !(now & MSMON___NRDY);					\
> +	mpam_mon_sel_inner_unlock(_msc);					\
> +										\
> +	_result = (!can_set || !can_clear);					\
> +} while (0)
> +
> +static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
> +{
> +	int err;
> +	struct mpam_msc *msc = ris->vmsc->msc;
> +	struct mpam_props *props = &ris->props;
> +
> +	lockdep_assert_held(&msc->probe_lock);
> +	lockdep_assert_held(&msc->part_sel_lock);
> +
> +	/* Cache Portion partitioning */
> +	if (FIELD_GET(MPAMF_IDR_HAS_CPOR_PART, ris->idr)) {
> +		u32 cpor_features = mpam_read_partsel_reg(msc, CPOR_IDR);
> +
> +		props->cpbm_wd = FIELD_GET(MPAMF_CPOR_IDR_CPBM_WD, cpor_features);
> +		if (props->cpbm_wd)
> +			mpam_set_feature(mpam_feat_cpor_part, props);
> +	}
> +
> +	/* Memory bandwidth partitioning */
> +	if (FIELD_GET(MPAMF_IDR_HAS_MBW_PART, ris->idr)) {
> +		u32 mbw_features = mpam_read_partsel_reg(msc, MBW_IDR);
> +
> +		/* portion bitmap resolution */
> +		props->mbw_pbm_bits = FIELD_GET(MPAMF_MBW_IDR_BWPBM_WD, mbw_features);
> +		if (props->mbw_pbm_bits &&
> +		    FIELD_GET(MPAMF_MBW_IDR_HAS_PBM, mbw_features))
> +			mpam_set_feature(mpam_feat_mbw_part, props);
> +
> +		props->bwa_wd = FIELD_GET(MPAMF_MBW_IDR_BWA_WD, mbw_features);
> +		if (props->bwa_wd && FIELD_GET(MPAMF_MBW_IDR_HAS_MAX, mbw_features))
> +			mpam_set_feature(mpam_feat_mbw_max, props);
> +	}
> +
> +	/* Performance Monitoring */
> +	if (FIELD_GET(MPAMF_IDR_HAS_MSMON, ris->idr)) {
> +		u32 msmon_features = mpam_read_partsel_reg(msc, MSMON_IDR);
> +
> +		/*
> +		 * If the firmware max-nrdy-us property is missing, the
> +		 * CSU counters can't be used. Should we wait forever?
> +		 */
> +		err = device_property_read_u32(&msc->pdev->dev,
> +					       "arm,not-ready-us",
> +					       &msc->nrdy_usec);
> +
> +		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_CSU, msmon_features)) {
> +			u32 csumonidr;
> +
> +			csumonidr = mpam_read_partsel_reg(msc, CSUMON_IDR);
> +			props->num_csu_mon = FIELD_GET(MPAMF_CSUMON_IDR_NUM_MON, csumonidr);
> +			if (props->num_csu_mon) {
> +				bool hw_managed;
> +
> +				mpam_set_feature(mpam_feat_msmon_csu, props);
> +
> +				/* Is NRDY hardware managed? */
> +				mpam_mon_sel_outer_lock(msc);
> +				mpam_ris_hw_probe_hw_nrdy(ris, CSU, hw_managed);
> +				mpam_mon_sel_outer_unlock(msc);
> +				if (hw_managed)
> +					mpam_set_feature(mpam_feat_msmon_csu_hw_nrdy, props);
> +			}
> +
> +			/*
> +			 * Accept the missing firmware property if NRDY appears
> +			 * un-implemented.
> +			 */
> +			if (err && mpam_has_feature(mpam_feat_msmon_csu_hw_nrdy, props))
> +				pr_err_once("Counters are not usable because not-ready timeout was not provided by firmware.");
> +		}
> +		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_MBWU, msmon_features)) {
> +			bool hw_managed;
> +			u32 mbwumonidr = mpam_read_partsel_reg(msc, MBWUMON_IDR);
> +
> +			props->num_mbwu_mon = FIELD_GET(MPAMF_MBWUMON_IDR_NUM_MON, mbwumonidr);
> +			if (props->num_mbwu_mon)
> +				mpam_set_feature(mpam_feat_msmon_mbwu, props);
> +
> +			if (FIELD_GET(MPAMF_MBWUMON_IDR_HAS_RWBW, mbwumonidr))
> +				mpam_set_feature(mpam_feat_msmon_mbwu_rwbw, props);
> +
> +			/* Is NRDY hardware managed? */
> +			mpam_mon_sel_outer_lock(msc);
> +			mpam_ris_hw_probe_hw_nrdy(ris, MBWU, hw_managed);
> +			mpam_mon_sel_outer_unlock(msc);
> +			if (hw_managed)
> +				mpam_set_feature(mpam_feat_msmon_mbwu_hw_nrdy, props);
> +
> +			/*
> +			 * Don't warn about any missing firmware property for
> +			 * MBWU NRDY - it doesn't make any sense!
> +			 */
> +		}
> +	}
> +}
> +
>   static int mpam_msc_hw_probe(struct mpam_msc *msc)
>   {
>   	u64 idr;
> @@ -665,6 +810,7 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>   
>   	idr = mpam_msc_read_idr(msc);
>   	mutex_unlock(&msc->part_sel_lock);
> +
>   	msc->ris_max = FIELD_GET(MPAMF_IDR_RIS_MAX, idr);
>   
>   	/* Use these values so partid/pmg always starts with a valid value */
> @@ -685,6 +831,12 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>   		ris = mpam_get_or_create_ris(msc, ris_idx);
>   		if (IS_ERR(ris))
>   			return PTR_ERR(ris);
> +		ris->idr = idr;
> +
> +		mutex_lock(&msc->part_sel_lock);
> +		__mpam_part_sel(ris_idx, 0, msc);
> +		mpam_ris_hw_probe(ris);
> +		mutex_unlock(&msc->part_sel_lock);
>   	}
>   
>   	spin_lock(&partid_max_lock);
> diff --git a/drivers/platform/arm64/mpam/mpam_internal.h b/drivers/platform/arm64/mpam/mpam_internal.h
> index 42a454d5f914..ae6fd1f62cc4 100644
> --- a/drivers/platform/arm64/mpam/mpam_internal.h
> +++ b/drivers/platform/arm64/mpam/mpam_internal.h
> @@ -136,6 +136,55 @@ static inline void mpam_mon_sel_lock_held(struct mpam_msc *msc)
>   		lockdep_assert_preemption_enabled();
>   }
>   
> +/*
> + * When we compact the supported features, we don't care what they are.
> + * Storing them as a bitmap makes life easy.
> + */
> +typedef u16 mpam_features_t;
> +
> +/* Bits for mpam_features_t */
> +enum mpam_device_features {
> +	mpam_feat_ccap_part = 0,
> +	mpam_feat_cpor_part,
> +	mpam_feat_mbw_part,
> +	mpam_feat_mbw_min,
> +	mpam_feat_mbw_max,
> +	mpam_feat_mbw_prop,
> +	mpam_feat_msmon,
> +	mpam_feat_msmon_csu,
> +	mpam_feat_msmon_csu_capture,
> +	mpam_feat_msmon_csu_hw_nrdy,
> +	mpam_feat_msmon_mbwu,
> +	mpam_feat_msmon_mbwu_capture,
> +	mpam_feat_msmon_mbwu_rwbw,
> +	mpam_feat_msmon_mbwu_hw_nrdy,
> +	mpam_feat_msmon_capt,
> +	MPAM_FEATURE_LAST,
> +};
> +#define MPAM_ALL_FEATURES      ((1 << MPAM_FEATURE_LAST) - 1)

Consider a static assert to check the type is big enough.

static_assert(BITS_PER_TYPE(mpam_features_t) >= MPAM_FEATURE_LAST);

> +
> +struct mpam_props {
> +	mpam_features_t		features;
> +
> +	u16			cpbm_wd;
> +	u16			mbw_pbm_bits;
> +	u16			bwa_wd;
> +	u16			num_csu_mon;
> +	u16			num_mbwu_mon;
> +};
> +
> +static inline bool mpam_has_feature(enum mpam_device_features feat,
> +				    struct mpam_props *props)
> +{
> +	return (1 << feat) & props->features;
> +}
> +
> +static inline void mpam_set_feature(enum mpam_device_features feat,
> +				    struct mpam_props *props)
> +{
> +	props->features |= (1 << feat);
> +}
> +
>   struct mpam_class {
>   	/* mpam_components in this class */
>   	struct list_head	components;
> @@ -175,6 +224,8 @@ struct mpam_vmsc {
>   	/* mpam_msc_ris in this vmsc */
>   	struct list_head	ris;
>   
> +	struct mpam_props	props;
> +
>   	/* All RIS in this vMSC are members of this MSC */
>   	struct mpam_msc		*msc;
>   
> @@ -186,6 +237,8 @@ struct mpam_vmsc {
>   
>   struct mpam_msc_ris {
>   	u8			ris_idx;
> +	u64			idr;
> +	struct mpam_props	props;
>   
>   	cpumask_t		affinity;
>   

Thanks,

Ben

Hi Ben,

On 28/07/2025 09:56, Ben Horgan wrote:
> On 7/11/25 19:36, James Morse wrote:
>> Expand the probing support with the control and monitor types
>> we can use with resctrl.

>> diff --git a/drivers/platform/arm64/mpam/mpam_internal.h b/drivers/platform/arm64/mpam/
>> mpam_internal.h
>> index 42a454d5f914..ae6fd1f62cc4 100644
>> --- a/drivers/platform/arm64/mpam/mpam_internal.h
>> +++ b/drivers/platform/arm64/mpam/mpam_internal.h
>> @@ -136,6 +136,55 @@ static inline void mpam_mon_sel_lock_held(struct mpam_msc *msc)
>>           lockdep_assert_preemption_enabled();
>>   }
>>   +/*
>> + * When we compact the supported features, we don't care what they are.
>> + * Storing them as a bitmap makes life easy.
>> + */
>> +typedef u16 mpam_features_t;
>> +
>> +/* Bits for mpam_features_t */
>> +enum mpam_device_features {
>> +    mpam_feat_ccap_part = 0,
>> +    mpam_feat_cpor_part,
>> +    mpam_feat_mbw_part,
>> +    mpam_feat_mbw_min,
>> +    mpam_feat_mbw_max,
>> +    mpam_feat_mbw_prop,
>> +    mpam_feat_msmon,
>> +    mpam_feat_msmon_csu,
>> +    mpam_feat_msmon_csu_capture,
>> +    mpam_feat_msmon_csu_hw_nrdy,
>> +    mpam_feat_msmon_mbwu,
>> +    mpam_feat_msmon_mbwu_capture,
>> +    mpam_feat_msmon_mbwu_rwbw,
>> +    mpam_feat_msmon_mbwu_hw_nrdy,
>> +    mpam_feat_msmon_capt,
>> +    MPAM_FEATURE_LAST,
>> +};
>> +#define MPAM_ALL_FEATURES      ((1 << MPAM_FEATURE_LAST) - 1)
> 
> Consider a static assert to check the type is big enough.
> 
> static_assert(BITS_PER_TYPE(mpam_features_t) >= MPAM_FEATURE_LAST);

Fancy.
There used to be an uglier one - not sure what happened to it!



Thanks,

James

Hi James,

On 7/11/25 19:36, James Morse wrote:
> Expand the probing support with the control and monitor types
> we can use with resctrl.
> 
> CC: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>   drivers/platform/arm64/mpam/mpam_devices.c  | 154 +++++++++++++++++++-
>   drivers/platform/arm64/mpam/mpam_internal.h |  53 +++++++
>   2 files changed, 206 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
> index 8646fb85ad09..61911831ab39 100644
> --- a/drivers/platform/arm64/mpam/mpam_devices.c
> +++ b/drivers/platform/arm64/mpam/mpam_devices.c
> @@ -102,7 +102,7 @@ static LLIST_HEAD(mpam_garbage);
>   
>   static u32 __mpam_read_reg(struct mpam_msc *msc, u16 reg)
>   {
> -	WARN_ON_ONCE(reg > msc->mapped_hwpage_sz);
> +	WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
>   	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
>   
>   	return readl_relaxed(msc->mapped_hwpage + reg);
> @@ -131,6 +131,20 @@ static inline void _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 va
>   }
>   #define mpam_write_partsel_reg(msc, reg, val)  _mpam_write_partsel_reg(msc, MPAMCFG_##reg, val)
>   
> +static inline u32 _mpam_read_monsel_reg(struct mpam_msc *msc, u16 reg)
> +{
> +	mpam_mon_sel_lock_held(msc);
> +	return __mpam_read_reg(msc, reg);
> +}
> +#define mpam_read_monsel_reg(msc, reg) _mpam_read_monsel_reg(msc, MSMON_##reg)
> +
> +static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
> +{
> +	mpam_mon_sel_lock_held(msc);
> +	__mpam_write_reg(msc, reg, val);
> +}
> +#define mpam_write_monsel_reg(msc, reg, val)   _mpam_write_monsel_reg(msc, MSMON_##reg, val)
> +
>   static u64 mpam_msc_read_idr(struct mpam_msc *msc)
>   {
>   	u64 idr_high = 0, idr_low;
> @@ -645,6 +659,137 @@ static struct mpam_msc_ris *mpam_get_or_create_ris(struct mpam_msc *msc,
>   	return found;
>   }
>   
> +/*
> + * IHI009A.a has this nugget: "If a monitor does not support automatic behaviour
> + * of NRDY, software can use this bit for any purpose" - so hardware might not
> + * implement this - but it isn't RES0.
> + *
> + * Try and see what values stick in this bit. If we can write either value,
> + * its probably not implemented by hardware.
> + */
> +#define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result)			\
> +do {										\
> +	u32 now;								\
> +	u64 mon_sel;								\
> +	bool can_set, can_clear;						\
> +	struct mpam_msc *_msc = _ris->vmsc->msc;				\
> +										\
> +	if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(_msc))) {			\
> +		_result = false;						\
> +		break;								\
> +	}									\
> +	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) |			\
> +		  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, _ris->ris_idx);		\
> +	mpam_write_monsel_reg(_msc, CFG_MON_SEL, mon_sel);			\
> +										\
> +	mpam_write_monsel_reg(_msc, _mon_reg, MSMON___NRDY);			\
> +	now = mpam_read_monsel_reg(_msc, _mon_reg);				\
> +	can_set = now & MSMON___NRDY;						\
> +										\
> +	mpam_write_monsel_reg(_msc, _mon_reg, 0);				\
> +	now = mpam_read_monsel_reg(_msc, _mon_reg);				\
> +	can_clear = !(now & MSMON___NRDY);					\
> +	mpam_mon_sel_inner_unlock(_msc);					\
> +										\
> +	_result = (!can_set || !can_clear);					\
> +} while (0)
It is a bit surprising that something that looks like a function 
modifies a boolean passed by value. Consider continuing the pattern you 
have above:
#define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result) 
_mpam_ris_hw_probe_hw_nrdy(_ris, MSMON##_mon_reg, _result)

with signature:
void _mpam_ris_hw_probe_hw_nrdy(struct mpam_msc *msc, u16 reg, bool 
*hw_managed);

and using the _mpam functions from the new _mpam_ris_hw_probe_hw_nrdy().

> +
> +static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
> +{
> +	int err;
> +	struct mpam_msc *msc = ris->vmsc->msc;
> +	struct mpam_props *props = &ris->props;
> +
> +	lockdep_assert_held(&msc->probe_lock);
> +	lockdep_assert_held(&msc->part_sel_lock);
> +
> +	/* Cache Portion partitioning */
> +	if (FIELD_GET(MPAMF_IDR_HAS_CPOR_PART, ris->idr)) {
> +		u32 cpor_features = mpam_read_partsel_reg(msc, CPOR_IDR);
> +
> +		props->cpbm_wd = FIELD_GET(MPAMF_CPOR_IDR_CPBM_WD, cpor_features);
> +		if (props->cpbm_wd)
> +			mpam_set_feature(mpam_feat_cpor_part, props);
> +	}
> +
> +	/* Memory bandwidth partitioning */
> +	if (FIELD_GET(MPAMF_IDR_HAS_MBW_PART, ris->idr)) {
> +		u32 mbw_features = mpam_read_partsel_reg(msc, MBW_IDR);
> +
> +		/* portion bitmap resolution */
> +		props->mbw_pbm_bits = FIELD_GET(MPAMF_MBW_IDR_BWPBM_WD, mbw_features);
> +		if (props->mbw_pbm_bits &&
> +		    FIELD_GET(MPAMF_MBW_IDR_HAS_PBM, mbw_features))
> +			mpam_set_feature(mpam_feat_mbw_part, props);
> +
> +		props->bwa_wd = FIELD_GET(MPAMF_MBW_IDR_BWA_WD, mbw_features);
> +		if (props->bwa_wd && FIELD_GET(MPAMF_MBW_IDR_HAS_MAX, mbw_features))
> +			mpam_set_feature(mpam_feat_mbw_max, props);
> +	}
> +
> +	/* Performance Monitoring */
> +	if (FIELD_GET(MPAMF_IDR_HAS_MSMON, ris->idr)) {
> +		u32 msmon_features = mpam_read_partsel_reg(msc, MSMON_IDR);
> +
> +		/*
> +		 * If the firmware max-nrdy-us property is missing, the
> +		 * CSU counters can't be used. Should we wait forever?
> +		 */
> +		err = device_property_read_u32(&msc->pdev->dev,
> +					       "arm,not-ready-us",
> +					       &msc->nrdy_usec);
> +
> +		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_CSU, msmon_features)) {
> +			u32 csumonidr;
> +
> +			csumonidr = mpam_read_partsel_reg(msc, CSUMON_IDR);
> +			props->num_csu_mon = FIELD_GET(MPAMF_CSUMON_IDR_NUM_MON, csumonidr);
> +			if (props->num_csu_mon) {
> +				bool hw_managed;
> +
> +				mpam_set_feature(mpam_feat_msmon_csu, props);
> +
> +				/* Is NRDY hardware managed? */
> +				mpam_mon_sel_outer_lock(msc);
> +				mpam_ris_hw_probe_hw_nrdy(ris, CSU, hw_managed);
> +				mpam_mon_sel_outer_unlock(msc);
> +				if (hw_managed)
> +					mpam_set_feature(mpam_feat_msmon_csu_hw_nrdy, props);
> +			}
> +
> +			/*
> +			 * Accept the missing firmware property if NRDY appears
> +			 * un-implemented.
> +			 */
> +			if (err && mpam_has_feature(mpam_feat_msmon_csu_hw_nrdy, props))
> +				pr_err_once("Counters are not usable because not-ready timeout was not provided by firmware.");
> +		}
> +		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_MBWU, msmon_features)) {
> +			bool hw_managed;
> +			u32 mbwumonidr = mpam_read_partsel_reg(msc, MBWUMON_IDR);
> +
> +			props->num_mbwu_mon = FIELD_GET(MPAMF_MBWUMON_IDR_NUM_MON, mbwumonidr);
> +			if (props->num_mbwu_mon)
> +				mpam_set_feature(mpam_feat_msmon_mbwu, props);
> +
> +			if (FIELD_GET(MPAMF_MBWUMON_IDR_HAS_RWBW, mbwumonidr))
> +				mpam_set_feature(mpam_feat_msmon_mbwu_rwbw, props);
> +
> +			/* Is NRDY hardware managed? */
> +			mpam_mon_sel_outer_lock(msc);
> +			mpam_ris_hw_probe_hw_nrdy(ris, MBWU, hw_managed);
> +			mpam_mon_sel_outer_unlock(msc);
> +			if (hw_managed)
> +				mpam_set_feature(mpam_feat_msmon_mbwu_hw_nrdy, props);
> +
> +			/*
> +			 * Don't warn about any missing firmware property for
> +			 * MBWU NRDY - it doesn't make any sense!
> +			 */
> +		}
> +	}
> +}
> +
>   static int mpam_msc_hw_probe(struct mpam_msc *msc)
>   {
>   	u64 idr;
> @@ -665,6 +810,7 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>   
>   	idr = mpam_msc_read_idr(msc);
>   	mutex_unlock(&msc->part_sel_lock);
> +
>   	msc->ris_max = FIELD_GET(MPAMF_IDR_RIS_MAX, idr);
>   
>   	/* Use these values so partid/pmg always starts with a valid value */
> @@ -685,6 +831,12 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>   		ris = mpam_get_or_create_ris(msc, ris_idx);
>   		if (IS_ERR(ris))
>   			return PTR_ERR(ris);
> +		ris->idr = idr;
> +
> +		mutex_lock(&msc->part_sel_lock);
> +		__mpam_part_sel(ris_idx, 0, msc);
> +		mpam_ris_hw_probe(ris);
> +		mutex_unlock(&msc->part_sel_lock);
>   	}
>   
>   	spin_lock(&partid_max_lock);
> diff --git a/drivers/platform/arm64/mpam/mpam_internal.h b/drivers/platform/arm64/mpam/mpam_internal.h
> index 42a454d5f914..ae6fd1f62cc4 100644
> --- a/drivers/platform/arm64/mpam/mpam_internal.h
> +++ b/drivers/platform/arm64/mpam/mpam_internal.h
> @@ -136,6 +136,55 @@ static inline void mpam_mon_sel_lock_held(struct mpam_msc *msc)
>   		lockdep_assert_preemption_enabled();
>   }
>   
> +/*
> + * When we compact the supported features, we don't care what they are.
> + * Storing them as a bitmap makes life easy.
> + */
> +typedef u16 mpam_features_t;
> +
> +/* Bits for mpam_features_t */
> +enum mpam_device_features {
> +	mpam_feat_ccap_part = 0,
> +	mpam_feat_cpor_part,
> +	mpam_feat_mbw_part,
> +	mpam_feat_mbw_min,
> +	mpam_feat_mbw_max,
> +	mpam_feat_mbw_prop,
> +	mpam_feat_msmon,
> +	mpam_feat_msmon_csu,
> +	mpam_feat_msmon_csu_capture,
> +	mpam_feat_msmon_csu_hw_nrdy,
> +	mpam_feat_msmon_mbwu,
> +	mpam_feat_msmon_mbwu_capture,
> +	mpam_feat_msmon_mbwu_rwbw,
> +	mpam_feat_msmon_mbwu_hw_nrdy,
> +	mpam_feat_msmon_capt,
> +	MPAM_FEATURE_LAST,
> +};
> +#define MPAM_ALL_FEATURES      ((1 << MPAM_FEATURE_LAST) - 1)
> +
> +struct mpam_props {
> +	mpam_features_t		features;
> +
> +	u16			cpbm_wd;
> +	u16			mbw_pbm_bits;
> +	u16			bwa_wd;
> +	u16			num_csu_mon;
> +	u16			num_mbwu_mon;
> +};
> +
> +static inline bool mpam_has_feature(enum mpam_device_features feat,
> +				    struct mpam_props *props)
> +{
> +	return (1 << feat) & props->features;
> +}
> +
> +static inline void mpam_set_feature(enum mpam_device_features feat,
> +				    struct mpam_props *props)
> +{
> +	props->features |= (1 << feat);
> +}
> +
>   struct mpam_class {
>   	/* mpam_components in this class */
>   	struct list_head	components;
> @@ -175,6 +224,8 @@ struct mpam_vmsc {
>   	/* mpam_msc_ris in this vmsc */
>   	struct list_head	ris;
>   
> +	struct mpam_props	props;
> +
>   	/* All RIS in this vMSC are members of this MSC */
>   	struct mpam_msc		*msc;
>   
> @@ -186,6 +237,8 @@ struct mpam_vmsc {
>   
>   struct mpam_msc_ris {
>   	u8			ris_idx;
> +	u64			idr;
> +	struct mpam_props	props;
>   
>   	cpumask_t		affinity;
>   

-- 
Thanks,

Ben

Hi Ben,

On 24/07/2025 16:08, Ben Horgan wrote:
> On 7/11/25 19:36, James Morse wrote:
>> Expand the probing support with the control and monitor types
>> we can use with resctrl.

>> diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/
>> mpam_devices.c
>> index 8646fb85ad09..61911831ab39 100644
>> --- a/drivers/platform/arm64/mpam/mpam_devices.c
>> +++ b/drivers/platform/arm64/mpam/mpam_devices.c

>> @@ -645,6 +659,137 @@ static struct mpam_msc_ris *mpam_get_or_create_ris(struct mpam_msc
>> *msc,
>>       return found;
>>   }
>>   +/*
>> + * IHI009A.a has this nugget: "If a monitor does not support automatic behaviour
>> + * of NRDY, software can use this bit for any purpose" - so hardware might not
>> + * implement this - but it isn't RES0.
>> + *
>> + * Try and see what values stick in this bit. If we can write either value,
>> + * its probably not implemented by hardware.
>> + */
>> +#define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result)            \
>> +do {                                        \
>> +    u32 now;                                \
>> +    u64 mon_sel;                                \
>> +    bool can_set, can_clear;                        \
>> +    struct mpam_msc *_msc = _ris->vmsc->msc;                \
>> +                                        \
>> +    if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(_msc))) {            \
>> +        _result = false;                        \
>> +        break;                                \
>> +    }                                    \
>> +    mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) |            \
>> +          FIELD_PREP(MSMON_CFG_MON_SEL_RIS, _ris->ris_idx);        \
>> +    mpam_write_monsel_reg(_msc, CFG_MON_SEL, mon_sel);            \
>> +                                        \
>> +    mpam_write_monsel_reg(_msc, _mon_reg, MSMON___NRDY);            \
>> +    now = mpam_read_monsel_reg(_msc, _mon_reg);                \
>> +    can_set = now & MSMON___NRDY;                        \
>> +                                        \
>> +    mpam_write_monsel_reg(_msc, _mon_reg, 0);                \
>> +    now = mpam_read_monsel_reg(_msc, _mon_reg);                \
>> +    can_clear = !(now & MSMON___NRDY);                    \
>> +    mpam_mon_sel_inner_unlock(_msc);                    \
>> +                                        \
>> +    _result = (!can_set || !can_clear);                    \
>> +} while (0)

> It is a bit surprising that something that looks like a function modifies a boolean passed
> by value.

> Consider continuing the pattern you have above:
> #define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result)
> _mpam_ris_hw_probe_hw_nrdy(_ris, MSMON##_mon_reg, _result)
> 
> with signature:
> void _mpam_ris_hw_probe_hw_nrdy(struct mpam_msc *msc, u16 reg, bool *hw_managed);
> 
> and using the _mpam functions from the new _mpam_ris_hw_probe_hw_nrdy().

With that, it may as well return the result.
Done.


Thanks,

James

}
> >   
> > +/*
> > + * IHI009A.a has this nugget: "If a monitor does not support automatic behaviour
> > + * of NRDY, software can use this bit for any purpose" - so hardware might not
> > + * implement this - but it isn't RES0.
> > + *
> > + * Try and see what values stick in this bit. If we can write either value,
> > + * its probably not implemented by hardware.
> > + */
> > +#define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result)			\
> > +do {										\
> > +	u32 now;								\
> > +	u64 mon_sel;								\
> > +	bool can_set, can_clear;						\
> > +	struct mpam_msc *_msc = _ris->vmsc->msc;				\
> > +										\
> > +	if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(_msc))) {			\
> > +		_result = false;						\
> > +		break;								\
> > +	}									\
> > +	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) |			\
> > +		  FIELD_PREP(MSMON_CFG_MON_SEL_RIS, _ris->ris_idx);		\
> > +	mpam_write_monsel_reg(_msc, CFG_MON_SEL, mon_sel);			\
> > +										\
> > +	mpam_write_monsel_reg(_msc, _mon_reg, MSMON___NRDY);			\
> > +	now = mpam_read_monsel_reg(_msc, _mon_reg);				\
> > +	can_set = now & MSMON___NRDY;						\
> > +										\
> > +	mpam_write_monsel_reg(_msc, _mon_reg, 0);				\
> > +	now = mpam_read_monsel_reg(_msc, _mon_reg);				\
> > +	can_clear = !(now & MSMON___NRDY);					\
> > +	mpam_mon_sel_inner_unlock(_msc);					\
> > +										\
> > +	_result = (!can_set || !can_clear);					\
> > +} while (0)  
> It is a bit surprising that something that looks like a function 
> modifies a boolean passed by value. Consider continuing the pattern you 
> have above:
> #define mpam_ris_hw_probe_hw_nrdy(_ris, _mon_reg, _result) 
> _mpam_ris_hw_probe_hw_nrdy(_ris, MSMON##_mon_reg, _result)
> 
> with signature:
> void _mpam_ris_hw_probe_hw_nrdy(struct mpam_msc *msc, u16 reg, bool 
> *hw_managed);
> 
> and using the _mpam functions from the new _mpam_ris_hw_probe_hw_nrdy().
> 

Agreed that this is ugly.  Only a tiny bit of macro stuff is actually going on here.
I'd make it function.

If you really want to construct MSMON_CSU etc then wrap that helper with
a macro that builds reg from the name.

I might have missed something though in converting this.
The version I have has some other changes though so not trivial to post here :(