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

Gavin Shan
2025-11-09T00:43:44Z

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