Re: [PATCH 14/33] arm_mpam: Probe hardware to find the supported partid/pmg values

[Gavin Shan <gshan@redhat.com>]

Hi Ben,

On 11/7/25 10:34 PM, Ben Horgan wrote:
> From: James Morse <james.morse@arm.com>
> 
> CPUs can generate traffic with a range of PARTID and PMG values,
> but each MSC may also have its own maximum size for these fields.
> Before MPAM can be used, the driver needs to probe each RIS on
> each MSC, to find the system-wide smallest value that can be used.
> The limits from requestors (e.g. CPUs) also need taking into account.
> 
> While doing this, RIS entries that firmware didn't describe are created
> under MPAM_CLASS_UNKNOWN.
> 
> This adds the low level MSC write accessors.
> 
> While we're here, implement the mpam_register_requestor() call
> for the arch code to register the CPU limits. Future callers of this
> will tell us about the SMMU and ITS.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Ben Horgan <ben.horgan@arm.com>
> Tested-by: Fenghua Yu <fenghuay@nvidia.com>
> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> Tested-by: Peter Newman <peternewman@google.com>
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
> Changes since v3:
>  From Jonathan:
> Stray comma in printk
> Unnecessary braces
> ---
>   drivers/resctrl/mpam_devices.c  | 148 +++++++++++++++++++++++++++++++-
>   drivers/resctrl/mpam_internal.h |   6 ++
>   include/linux/arm_mpam.h        |  14 +++
>   3 files changed, 167 insertions(+), 1 deletion(-)
> 

One comment related to 'partid_max_init', but this looks good to me in
either way:

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

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 4162a7a57626..e1e26cb350f7 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -6,6 +6,7 @@
>   #include <linux/acpi.h>
>   #include <linux/atomic.h>
>   #include <linux/arm_mpam.h>
> +#include <linux/bitfield.h>
>   #include <linux/cacheinfo.h>
>   #include <linux/cpu.h>
>   #include <linux/cpumask.h>
> @@ -42,6 +43,15 @@ static atomic_t mpam_num_msc;
>   static int mpam_cpuhp_state;
>   static DEFINE_MUTEX(mpam_cpuhp_state_lock);
>   
> +/*
> + * The smallest common values for any CPU or MSC in the system.
> + * Generating traffic outside this range will result in screaming interrupts.
> + */
> +u16 mpam_partid_max;
> +u8 mpam_pmg_max;
> +static bool partid_max_init, partid_max_published;
> +static DEFINE_SPINLOCK(partid_max_lock);
> +

If mpam_partid_max and mpam_pmg_max are initialized to USHRT_MAX and 255 here,
the state related to partid_max_init can be removed...

>   /*
>    * mpam is enabled once all devices have been probed from CPU online callbacks,
>    * scheduled via this work_struct. If access to an MSC depends on a CPU that
> @@ -142,6 +152,70 @@ static inline u32 _mpam_read_partsel_reg(struct mpam_msc *msc, u16 reg)
>   
>   #define mpam_read_partsel_reg(msc, reg) _mpam_read_partsel_reg(msc, MPAMF_##reg)
>   
> +static void __mpam_write_reg(struct mpam_msc *msc, u16 reg, u32 val)
> +{
> +	WARN_ON_ONCE(reg + sizeof(u32) >= msc->mapped_hwpage_sz);
> +	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
> +
> +	writel_relaxed(val, msc->mapped_hwpage + reg);
> +}
> +
> +static inline void _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
> +{
> +	lockdep_assert_held_once(&msc->part_sel_lock);
> +	__mpam_write_reg(msc, reg, val);
> +}
> +
> +#define mpam_write_partsel_reg(msc, reg, val)  _mpam_write_partsel_reg(msc, MPAMCFG_##reg, val)
> +
> +static u64 mpam_msc_read_idr(struct mpam_msc *msc)
> +{
> +	u64 idr_high = 0, idr_low;
> +
> +	lockdep_assert_held(&msc->part_sel_lock);
> +
> +	idr_low = mpam_read_partsel_reg(msc, IDR);
> +	if (FIELD_GET(MPAMF_IDR_EXT, idr_low))
> +		idr_high = mpam_read_partsel_reg(msc, IDR + 4);
> +
> +	return (idr_high << 32) | idr_low;
> +}
> +
> +static void __mpam_part_sel_raw(u32 partsel, struct mpam_msc *msc)
> +{
> +	lockdep_assert_held(&msc->part_sel_lock);
> +
> +	mpam_write_partsel_reg(msc, PART_SEL, partsel);
> +}
> +
> +static void __mpam_part_sel(u8 ris_idx, u16 partid, struct mpam_msc *msc)
> +{
> +	u32 partsel = FIELD_PREP(MPAMCFG_PART_SEL_RIS, ris_idx) |
> +		      FIELD_PREP(MPAMCFG_PART_SEL_PARTID_SEL, partid);
> +
> +	__mpam_part_sel_raw(partsel, msc);
> +}
> +
> +int mpam_register_requestor(u16 partid_max, u8 pmg_max)
> +{
> +	guard(spinlock)(&partid_max_lock);
> +	if (!partid_max_init) {
> +		mpam_partid_max = partid_max;
> +		mpam_pmg_max = pmg_max;
> +		partid_max_init = true;
> +	} else if (!partid_max_published) {
> +		mpam_partid_max = min(mpam_partid_max, partid_max);
> +		mpam_pmg_max = min(mpam_pmg_max, pmg_max);
> +	} else {
> +		/* New requestors can't lower the values */
> +		if (partid_max < mpam_partid_max || pmg_max < mpam_pmg_max)
> +			return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mpam_register_requestor);
> +

With mpam_partid_max and mpam_pmg_max initialized to USHRT_MAX and 255, this
can be:

	if (!partid_max_published) {
             ...
         } else {
             ...
         }

>   static struct mpam_vmsc *
>   mpam_vmsc_alloc(struct mpam_component *comp, struct mpam_msc *msc)
>   {
> @@ -451,9 +525,35 @@ int mpam_ris_create(struct mpam_msc *msc, u8 ris_idx,
>   	return err;
>   }
>   
> +static struct mpam_msc_ris *mpam_get_or_create_ris(struct mpam_msc *msc,
> +						   u8 ris_idx)
> +{
> +	int err;
> +	struct mpam_msc_ris *ris;
> +
> +	lockdep_assert_held(&mpam_list_lock);
> +
> +	if (!test_bit(ris_idx, &msc->ris_idxs)) {
> +		err = mpam_ris_create_locked(msc, ris_idx, MPAM_CLASS_UNKNOWN,
> +					     0, 0);
> +		if (err)
> +			return ERR_PTR(err);
> +	}
> +
> +	list_for_each_entry(ris, &msc->ris, msc_list) {
> +		if (ris->ris_idx == ris_idx)
> +			return ris;
> +	}
> +
> +	return ERR_PTR(-ENOENT);
> +}
> +
>   static int mpam_msc_hw_probe(struct mpam_msc *msc)
>   {
>   	u64 idr;
> +	u16 partid_max;
> +	u8 ris_idx, pmg_max;
> +	struct mpam_msc_ris *ris;
>   	struct device *dev = &msc->pdev->dev;
>   
>   	lockdep_assert_held(&msc->probe_lock);
> @@ -464,6 +564,40 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>   		return -EIO;
>   	}
>   
> +	/* Grab an IDR value to find out how many RIS there are */
> +	mutex_lock(&msc->part_sel_lock);
> +	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 */
> +	msc->partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr);
> +	msc->pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr);
> +
> +	for (ris_idx = 0; ris_idx <= msc->ris_max; ris_idx++) {
> +		mutex_lock(&msc->part_sel_lock);
> +		__mpam_part_sel(ris_idx, 0, msc);
> +		idr = mpam_msc_read_idr(msc);
> +		mutex_unlock(&msc->part_sel_lock);
> +
> +		partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr);
> +		pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr);
> +		msc->partid_max = min(msc->partid_max, partid_max);
> +		msc->pmg_max = min(msc->pmg_max, pmg_max);
> +
> +		mutex_lock(&mpam_list_lock);
> +		ris = mpam_get_or_create_ris(msc, ris_idx);
> +		mutex_unlock(&mpam_list_lock);
> +		if (IS_ERR(ris))
> +			return PTR_ERR(ris);
> +	}
> +
> +	spin_lock(&partid_max_lock);
> +	mpam_partid_max = min(mpam_partid_max, msc->partid_max);
> +	mpam_pmg_max = min(mpam_pmg_max, msc->pmg_max);
> +	spin_unlock(&partid_max_lock);
> +
>   	msc->probed = true;
>   
>   	return 0;
> @@ -686,10 +820,20 @@ static struct platform_driver mpam_msc_driver = {
>   
>   static void mpam_enable_once(void)
>   {
> +	/*
> +	 * Once the cpuhp callbacks have been changed, mpam_partid_max can no
> +	 * longer change.
> +	 */
> +	spin_lock(&partid_max_lock);
> +	partid_max_published = true;
> +	spin_unlock(&partid_max_lock);
> +
>   	mpam_register_cpuhp_callbacks(mpam_cpu_online, mpam_cpu_offline,
>   				      "mpam:online");
>   
> -	pr_info("MPAM enabled\n");
> +	/* Use printk() to avoid the pr_fmt adding the function name. */
> +	printk(KERN_INFO "MPAM enabled with %u PARTIDs and %u PMGs\n",
> +	       mpam_partid_max + 1, mpam_pmg_max + 1);
>   }
>   
>   void mpam_disable(struct work_struct *ignored)
> @@ -756,4 +900,6 @@ static int __init mpam_msc_driver_init(void)
>   
>   	return platform_driver_register(&mpam_msc_driver);
>   }
> +
> +/* Must occur after arm64_mpam_register_cpus() from arch_initcall() */
>   subsys_initcall(mpam_msc_driver_init);
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index 4e1538d29783..768a58a3ab27 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -49,6 +49,8 @@ struct mpam_msc {
>   	 */
>   	struct mutex		probe_lock;
>   	bool			probed;
> +	u16			partid_max;
> +	u8			pmg_max;
>   	unsigned long		ris_idxs;
>   	u32			ris_max;
>   
> @@ -138,6 +140,10 @@ struct mpam_msc_ris {
>   extern struct srcu_struct mpam_srcu;
>   extern struct list_head mpam_classes;
>   
> +/* System wide partid/pmg values */
> +extern u16 mpam_partid_max;
> +extern u8 mpam_pmg_max;
> +
>   /* Scheduled work callback to enable mpam once all MSC have been probed */
>   void mpam_enable(struct work_struct *work);
>   void mpam_disable(struct work_struct *work);
> diff --git a/include/linux/arm_mpam.h b/include/linux/arm_mpam.h
> index 5a3aab6bb1d4..c7246cfeb091 100644
> --- a/include/linux/arm_mpam.h
> +++ b/include/linux/arm_mpam.h
> @@ -49,4 +49,18 @@ static inline int mpam_ris_create(struct mpam_msc *msc, u8 ris_idx,
>   }
>   #endif
>   
> +/**
> + * mpam_register_requestor() - Register a requestor with the MPAM driver
> + * @partid_max:		The maximum PARTID value the requestor can generate.
> + * @pmg_max:		The maximum PMG value the requestor can generate.
> + *
> + * Registers a requestor with the MPAM driver to ensure the chosen system-wide
> + * minimum PARTID and PMG values will allow the requestors features to be used.
> + *
> + * Returns an error if the registration is too late, and a larger PARTID/PMG
> + * value has been advertised to user-space. In this case the requestor should
> + * not use its MPAM features. Returns 0 on success.
> + */
> +int mpam_register_requestor(u16 partid_max, u8 pmg_max);
> +
>   #endif /* __LINUX_ARM_MPAM_H */
Thanks,
Gavin