Re: [PATCH 25/33] arm_mpam: Probe and reset the rest of the features

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

Hi James,

On 8/22/25 16:30, James Morse wrote:
> MPAM supports more features than are going to be exposed to resctrl.
> For partid other than 0, the reset values of these controls isn't
> known.
> 
> Discover the rest of the features so they can be reset to avoid any
> side effects when resctrl is in use.
> 
> PARTID narrowing allows MSC/RIS to support less configuration space than
> is usable. If this feature is found on a class of device we are likely
> to use, then reduce the partid_max to make it usable. This allows us
> to map a PARTID to itself.
> 
> CC: Rohit Mathew <Rohit.Mathew@arm.com>
> CC: Zeng Heng <zengheng4@huawei.com>
> CC: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  drivers/resctrl/mpam_devices.c  | 175 ++++++++++++++++++++++++++++++++
>  drivers/resctrl/mpam_internal.h |  16 ++-
>  2 files changed, 189 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 8f6df2406c22..aedd743d6827 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -213,6 +213,15 @@ static void __mpam_part_sel(u8 ris_idx, u16 partid, struct mpam_msc *msc)
>  	__mpam_part_sel_raw(partsel, msc);
>  }
>  
> +static void __mpam_intpart_sel(u8 ris_idx, u16 intpartid, struct mpam_msc *msc)
> +{
> +	u32 partsel = FIELD_PREP(MPAMCFG_PART_SEL_RIS, ris_idx) |
> +		      FIELD_PREP(MPAMCFG_PART_SEL_PARTID_SEL, intpartid) |
> +		      MPAMCFG_PART_SEL_INTERNAL;
> +
> +	__mpam_part_sel_raw(partsel, msc);
> +}
> +
>  int mpam_register_requestor(u16 partid_max, u8 pmg_max)
>  {
>  	int err = 0;
> @@ -743,10 +752,35 @@ 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;
> +	struct mpam_class *class = ris->vmsc->comp->class;
>  
>  	lockdep_assert_held(&msc->probe_lock);
>  	lockdep_assert_held(&msc->part_sel_lock);
>  
> +	/* Cache Capacity Partitioning */
> +	if (FIELD_GET(MPAMF_IDR_HAS_CCAP_PART, ris->idr)) {
> +		u32 ccap_features = mpam_read_partsel_reg(msc, CCAP_IDR);
> +
> +		props->cmax_wd = FIELD_GET(MPAMF_CCAP_IDR_CMAX_WD, ccap_features);
> +		if (props->cmax_wd &&
> +		    FIELD_GET(MPAMF_CCAP_IDR_HAS_CMAX_SOFTLIM, ccap_features))
> +			mpam_set_feature(mpam_feat_cmax_softlim, props);
> +
> +		if (props->cmax_wd &&
> +		    !FIELD_GET(MPAMF_CCAP_IDR_NO_CMAX, ccap_features))
> +			mpam_set_feature(mpam_feat_cmax_cmax, props);
> +
> +		if (props->cmax_wd &&
> +		    FIELD_GET(MPAMF_CCAP_IDR_HAS_CMIN, ccap_features))
> +			mpam_set_feature(mpam_feat_cmax_cmin, props);
> +
> +		props->cassoc_wd = FIELD_GET(MPAMF_CCAP_IDR_CASSOC_WD, ccap_features);
> +
> +		if (props->cassoc_wd &&
> +		    FIELD_GET(MPAMF_CCAP_IDR_HAS_CASSOC, ccap_features))
> +			mpam_set_feature(mpam_feat_cmax_cassoc, props);
> +	}
> +
>  	/* Cache Portion partitioning */
>  	if (FIELD_GET(MPAMF_IDR_HAS_CPOR_PART, ris->idr)) {
>  		u32 cpor_features = mpam_read_partsel_reg(msc, CPOR_IDR);
> @@ -769,6 +803,31 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>  		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);
> +
> +		if (props->bwa_wd && FIELD_GET(MPAMF_MBW_IDR_HAS_MIN, mbw_features))
> +			mpam_set_feature(mpam_feat_mbw_min, props);
> +
> +		if (props->bwa_wd && FIELD_GET(MPAMF_MBW_IDR_HAS_PROP, mbw_features))
> +			mpam_set_feature(mpam_feat_mbw_prop, props);
> +	}
> +
> +	/* Priority partitioning */
> +	if (FIELD_GET(MPAMF_IDR_HAS_PRI_PART, ris->idr)) {
> +		u32 pri_features = mpam_read_partsel_reg(msc, PRI_IDR);
> +
> +		props->intpri_wd = FIELD_GET(MPAMF_PRI_IDR_INTPRI_WD, pri_features);
> +		if (props->intpri_wd && FIELD_GET(MPAMF_PRI_IDR_HAS_INTPRI, pri_features)) {
> +			mpam_set_feature(mpam_feat_intpri_part, props);
> +			if (FIELD_GET(MPAMF_PRI_IDR_INTPRI_0_IS_LOW, pri_features))
> +				mpam_set_feature(mpam_feat_intpri_part_0_low, props);
> +		}
> +
> +		props->dspri_wd = FIELD_GET(MPAMF_PRI_IDR_DSPRI_WD, pri_features);
> +		if (props->dspri_wd && FIELD_GET(MPAMF_PRI_IDR_HAS_DSPRI, pri_features)) {
> +			mpam_set_feature(mpam_feat_dspri_part, props);
> +			if (FIELD_GET(MPAMF_PRI_IDR_DSPRI_0_IS_LOW, pri_features))
> +				mpam_set_feature(mpam_feat_dspri_part_0_low, props);
> +		}
>  	}
>  
>  	/* Performance Monitoring */
> @@ -832,6 +891,21 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>  			 */
>  		}
>  	}
> +
> +	/*
> +	 * RIS with PARTID narrowing don't have enough storage for one
> +	 * configuration per PARTID. If these are in a class we could use,
> +	 * reduce the supported partid_max to match the number of intpartid.
> +	 * If the class is unknown, just ignore it.
> +	 */
> +	if (FIELD_GET(MPAMF_IDR_HAS_PARTID_NRW, ris->idr) &&
> +	    class->type != MPAM_CLASS_UNKNOWN) {
> +		u32 nrwidr = mpam_read_partsel_reg(msc, PARTID_NRW_IDR);
> +		u16 partid_max = FIELD_GET(MPAMF_PARTID_NRW_IDR_INTPARTID_MAX, nrwidr);
> +
> +		mpam_set_feature(mpam_feat_partid_nrw, props);
> +		msc->partid_max = min(msc->partid_max, partid_max);
> +	}
>  }
>  
>  static int mpam_msc_hw_probe(struct mpam_msc *msc)
> @@ -929,13 +1003,29 @@ static void mpam_reset_msc_bitmap(struct mpam_msc *msc, u16 reg, u16 wd)
>  static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
>  				      struct mpam_config *cfg)
>  {
> +	u32 pri_val = 0;
> +	u16 cmax = MPAMCFG_CMAX_CMAX;
>  	u16 bwa_fract = MPAMCFG_MBW_MAX_MAX;
>  	struct mpam_msc *msc = ris->vmsc->msc;
>  	struct mpam_props *rprops = &ris->props;
> +	u16 dspri = GENMASK(rprops->dspri_wd, 0);
> +	u16 intpri = GENMASK(rprops->intpri_wd, 0);
>  
>  	mutex_lock(&msc->part_sel_lock);
>  	__mpam_part_sel(ris->ris_idx, partid, msc);
>  
> +	if (mpam_has_feature(mpam_feat_partid_nrw, rprops)) {
> +		/* Update the intpartid mapping */
> +		mpam_write_partsel_reg(msc, INTPARTID,
> +				       MPAMCFG_INTPARTID_INTERNAL | partid);
> +
> +		/*
> +		 * Then switch to the 'internal' partid to update the
> +		 * configuration.
> +		 */
> +		__mpam_intpart_sel(ris->ris_idx, partid, msc);
> +	}
> +
>  	if (mpam_has_feature(mpam_feat_cpor_part, rprops)) {
>  		if (mpam_has_feature(mpam_feat_cpor_part, cfg))
>  			mpam_write_partsel_reg(msc, CPBM, cfg->cpbm);
> @@ -964,6 +1054,29 @@ static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
>  
>  	if (mpam_has_feature(mpam_feat_mbw_prop, rprops))
>  		mpam_write_partsel_reg(msc, MBW_PROP, bwa_fract);
> +
> +	if (mpam_has_feature(mpam_feat_cmax_cmax, rprops))
> +		mpam_write_partsel_reg(msc, CMAX, cmax);
> +
> +	if (mpam_has_feature(mpam_feat_cmax_cmin, rprops))
> +		mpam_write_partsel_reg(msc, CMIN, 0);

Missing reset for cmax_cassoc. I wonder if it makes sense to have
separate enums for partitioning features, which require reset, and the rest.
> +
> +	if (mpam_has_feature(mpam_feat_intpri_part, rprops) ||
> +	    mpam_has_feature(mpam_feat_dspri_part, rprops)) {
> +		/* aces high? */
> +		if (!mpam_has_feature(mpam_feat_intpri_part_0_low, rprops))
> +			intpri = 0;
> +		if (!mpam_has_feature(mpam_feat_dspri_part_0_low, rprops))
> +			dspri = 0;
> +
> +		if (mpam_has_feature(mpam_feat_intpri_part, rprops))
> +			pri_val |= FIELD_PREP(MPAMCFG_PRI_INTPRI, intpri);
> +		if (mpam_has_feature(mpam_feat_dspri_part, rprops))
> +			pri_val |= FIELD_PREP(MPAMCFG_PRI_DSPRI, dspri);
> +
> +		mpam_write_partsel_reg(msc, PRI, pri_val);
> +	}
> +
>  	mutex_unlock(&msc->part_sel_lock);
>  }
>  
> @@ -1529,6 +1642,16 @@ static bool mpam_has_bwa_wd_feature(struct mpam_props *props)
>  	return false;
>  }
>  
> +/* Any of these features mean the CMAX_WD field is valid. */
> +static bool mpam_has_cmax_wd_feature(struct mpam_props *props)
> +{
> +	if (mpam_has_feature(mpam_feat_cmax_cmax, props))
> +		return true;
> +	if (mpam_has_feature(mpam_feat_cmax_cmin, props))
> +		return true;
> +	return false;
> +}
> +
>  #define MISMATCHED_HELPER(parent, child, helper, field, alias)		\
>  	helper(parent) &&						\
>  	((helper(child) && (parent)->field != (child)->field) ||	\
> @@ -1583,6 +1706,23 @@ static void __props_mismatch(struct mpam_props *parent,
>  		parent->bwa_wd = min(parent->bwa_wd, child->bwa_wd);
>  	}
>  
> +	if (alias && !mpam_has_cmax_wd_feature(parent) && mpam_has_cmax_wd_feature(child)) {
> +		parent->cmax_wd = child->cmax_wd;
> +	} else if (MISMATCHED_HELPER(parent, child, mpam_has_cmax_wd_feature,
> +				     cmax_wd, alias)) {
> +		pr_debug("%s took the min cmax_wd\n", __func__);
> +		parent->cmax_wd = min(parent->cmax_wd, child->cmax_wd);
> +	}
> +
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_cmax_cassoc, alias)) {
> +		parent->cassoc_wd = child->cassoc_wd;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_cmax_cassoc,
> +				   cassoc_wd, alias)) {
> +		pr_debug("%s cleared cassoc_wd\n", __func__);
> +		mpam_clear_feature(mpam_feat_cmax_cassoc, &parent->features);
> +		parent->cassoc_wd = 0;
> +	}
> +
>  	/* For num properties, take the minimum */
>  	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_csu, alias)) {
>  		parent->num_csu_mon = child->num_csu_mon;
> @@ -1600,6 +1740,41 @@ static void __props_mismatch(struct mpam_props *parent,
>  		parent->num_mbwu_mon = min(parent->num_mbwu_mon, child->num_mbwu_mon);
>  	}
>  
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_intpri_part, alias)) {
> +		parent->intpri_wd = child->intpri_wd;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_intpri_part,
> +				   intpri_wd, alias)) {
> +		pr_debug("%s took the min intpri_wd\n", __func__);
> +		parent->intpri_wd = min(parent->intpri_wd, child->intpri_wd);
> +	}
> +
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_dspri_part, alias)) {
> +		parent->dspri_wd = child->dspri_wd;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_dspri_part,
> +				   dspri_wd, alias)) {
> +		pr_debug("%s took the min dspri_wd\n", __func__);
> +		parent->dspri_wd = min(parent->dspri_wd, child->dspri_wd);
> +	}
> +
> +	/* TODO: alias support for these two */
> +	/* {int,ds}pri may not have differing 0-low behaviour */
> +	if (mpam_has_feature(mpam_feat_intpri_part, parent) &&
> +	    (!mpam_has_feature(mpam_feat_intpri_part, child) ||
> +	     mpam_has_feature(mpam_feat_intpri_part_0_low, parent) !=
> +	     mpam_has_feature(mpam_feat_intpri_part_0_low, child))) {
> +		pr_debug("%s cleared intpri_part\n", __func__);
> +		mpam_clear_feature(mpam_feat_intpri_part, &parent->features);
> +		mpam_clear_feature(mpam_feat_intpri_part_0_low, &parent->features);
> +	}
> +	if (mpam_has_feature(mpam_feat_dspri_part, parent) &&
> +	    (!mpam_has_feature(mpam_feat_dspri_part, child) ||
> +	     mpam_has_feature(mpam_feat_dspri_part_0_low, parent) !=
> +	     mpam_has_feature(mpam_feat_dspri_part_0_low, child))) {
> +		pr_debug("%s cleared dspri_part\n", __func__);
> +		mpam_clear_feature(mpam_feat_dspri_part, &parent->features);
> +		mpam_clear_feature(mpam_feat_dspri_part_0_low, &parent->features);
> +	}
> +
>  	if (alias) {
>  		/* Merge features for aliased resources */
>  		parent->features |= child->features;
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index 70cba9f22746..23445aedbabd 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -157,16 +157,23 @@ static inline void mpam_mon_sel_lock_held(struct mpam_msc *msc)
>   * 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;
> +typedef u32 mpam_features_t;
>  
>  /* Bits for mpam_features_t */
>  enum mpam_device_features {
> -	mpam_feat_ccap_part = 0,
> +	mpam_feat_cmax_softlim,
> +	mpam_feat_cmax_cmax,
> +	mpam_feat_cmax_cmin,
> +	mpam_feat_cmax_cassoc,
>  	mpam_feat_cpor_part,
>  	mpam_feat_mbw_part,
>  	mpam_feat_mbw_min,
>  	mpam_feat_mbw_max,
>  	mpam_feat_mbw_prop,
> +	mpam_feat_intpri_part,
> +	mpam_feat_intpri_part_0_low,
> +	mpam_feat_dspri_part,
> +	mpam_feat_dspri_part_0_low,
>  	mpam_feat_msmon,
>  	mpam_feat_msmon_csu,
>  	mpam_feat_msmon_csu_capture,
> @@ -176,6 +183,7 @@ enum mpam_device_features {
>  	mpam_feat_msmon_mbwu_rwbw,
>  	mpam_feat_msmon_mbwu_hw_nrdy,
>  	mpam_feat_msmon_capt,
> +	mpam_feat_partid_nrw,
>  	MPAM_FEATURE_LAST,
>  };
>  static_assert(BITS_PER_TYPE(mpam_features_t) >= MPAM_FEATURE_LAST);
> @@ -187,6 +195,10 @@ struct mpam_props {
>  	u16			cpbm_wd;
>  	u16			mbw_pbm_bits;
>  	u16			bwa_wd;
> +	u16			cmax_wd;
> +	u16			cassoc_wd;
> +	u16			intpri_wd;
> +	u16			dspri_wd;
>  	u16			num_csu_mon;
>  	u16			num_mbwu_mon;
>  };

Thanks,

Ben