Probing MPAM is convoluted. MSCs that are integrated with a CPU may
only be accessible from those CPUs, and they may not be online.
Touching the hardware early is pointless as MPAM can't be used until
the system-wide common values for num_partid and num_pmg have been
discovered.
Start with driver probe/remove and mapping the MSC.
CC: Carl Worth <carl@os.amperecomputing.com>
Signed-off-by: James Morse <james.morse@arm.com>
---
Changes since v1:
* Avoid selecting driver on other architectrues.
* Removed PCC support stub.
* Use for_each_available_child_of_node_scoped() and of_property_read_reg()
* Clarified a comment.
* Stopped using mpam_num_msc as an id,a and made it atomic.
* Size of -1 returned from cache_of_calculate_id()
* Renamed some struct members.
* Made a bunch of pr_err() dev_err_ocne().
* Used more cleanup magic.
* Inlined a print message.
* Fixed error propagation from mpam_dt_parse_resources().
* Moved cache accessibility checks earlier.
Changes since RFC:
* Check for status=broken DT devices.
* Moved all the files around.
* Made Kconfig symbols depend on EXPERT
---
arch/arm64/Kconfig | 1 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/resctrl/Kconfig | 14 +++
drivers/resctrl/Makefile | 4 +
drivers/resctrl/mpam_devices.c | 180 ++++++++++++++++++++++++++++++++
drivers/resctrl/mpam_internal.h | 65 ++++++++++++
7 files changed, 267 insertions(+)
create mode 100644 drivers/resctrl/Kconfig
create mode 100644 drivers/resctrl/Makefile
create mode 100644 drivers/resctrl/mpam_devices.c
create mode 100644 drivers/resctrl/mpam_internal.h
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6487c511bdc6..93e563e1cce4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2062,6 +2062,7 @@ config ARM64_TLB_RANGE
config ARM64_MPAM
bool "Enable support for MPAM"
+ select ARM64_MPAM_DRIVER if EXPERT
select ACPI_MPAM if ACPI
help
Memory System Resource Partitioning and Monitoring (MPAM) is an
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 4915a63866b0..3054b50a2f4c 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -251,4 +251,6 @@ source "drivers/hte/Kconfig"
source "drivers/cdx/Kconfig"
+source "drivers/resctrl/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index b5749cf67044..f41cf4eddeba 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -194,5 +194,6 @@ obj-$(CONFIG_HTE) += hte/
obj-$(CONFIG_DRM_ACCEL) += accel/
obj-$(CONFIG_CDX_BUS) += cdx/
obj-$(CONFIG_DPLL) += dpll/
+obj-y += resctrl/
obj-$(CONFIG_S390) += s390/
diff --git a/drivers/resctrl/Kconfig b/drivers/resctrl/Kconfig
new file mode 100644
index 000000000000..c30532a3a3a4
--- /dev/null
+++ b/drivers/resctrl/Kconfig
@@ -0,0 +1,14 @@
+menuconfig ARM64_MPAM_DRIVER
+ bool "MPAM driver"
+ depends on ARM64 && ARM64_MPAM && EXPERT
+ help
+ MPAM driver for System IP, e,g. caches and memory controllers.
+
+if ARM64_MPAM_DRIVER
+config ARM64_MPAM_DRIVER_DEBUG
+ bool "Enable debug messages from the MPAM driver"
+ depends on ARM64_MPAM_DRIVER
+ help
+ Say yes here to enable debug messages from the MPAM driver.
+
+endif
diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile
new file mode 100644
index 000000000000..92b48fa20108
--- /dev/null
+++ b/drivers/resctrl/Makefile
@@ -0,0 +1,4 @@
+obj-$(CONFIG_ARM64_MPAM_DRIVER) += mpam.o
+mpam-y += mpam_devices.o
+
+cflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG) += -DDEBUG
diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
new file mode 100644
index 000000000000..efc4738e3b4d
--- /dev/null
+++ b/drivers/resctrl/mpam_devices.c
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2025 Arm Ltd.
+
+#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
+
+#include <linux/acpi.h>
+#include <linux/arm_mpam.h>
+#include <linux/cacheinfo.h>
+#include <linux/cpu.h>
+#include <linux/cpumask.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/gfp.h>
+#include <linux/list.h>
+#include <linux/lockdep.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/srcu.h>
+#include <linux/types.h>
+
+#include "mpam_internal.h"
+
+/*
+ * mpam_list_lock protects the SRCU lists when writing. Once the
+ * mpam_enabled key is enabled these lists are read-only,
+ * unless the error interrupt disables the driver.
+ */
+static DEFINE_MUTEX(mpam_list_lock);
+static LIST_HEAD(mpam_all_msc);
+
+static struct srcu_struct mpam_srcu;
+
+/*
+ * Number of MSCs that have been probed. Once all MSC have been probed MPAM
+ * can be enabled.
+ */
+static atomic_t mpam_num_msc;
+
+/*
+ * An MSC can control traffic from a set of CPUs, but may only be accessible
+ * from a (hopefully wider) set of CPUs. The common reason for this is power
+ * management. If all the CPUs in a cluster are in PSCI:CPU_SUSPEND, the
+ * corresponding cache may also be powered off. By making accesses from
+ * one of those CPUs, we ensure this isn't the case.
+ */
+static int update_msc_accessibility(struct mpam_msc *msc)
+{
+ u32 affinity_id;
+ int err;
+
+ err = device_property_read_u32(&msc->pdev->dev, "cpu_affinity",
+ &affinity_id);
+ if (err)
+ cpumask_copy(&msc->accessibility, cpu_possible_mask);
+ else
+ acpi_pptt_get_cpus_from_container(affinity_id,
+ &msc->accessibility);
+
+ return 0;
+
+ return err;
+}
+
+static int fw_num_msc;
+
+static void mpam_msc_drv_remove(struct platform_device *pdev)
+{
+ struct mpam_msc *msc = platform_get_drvdata(pdev);
+
+ if (!msc)
+ return;
+
+ mutex_lock(&mpam_list_lock);
+ platform_set_drvdata(pdev, NULL);
+ list_del_rcu(&msc->all_msc_list);
+ synchronize_srcu(&mpam_srcu);
+ mutex_unlock(&mpam_list_lock);
+}
+
+static int mpam_msc_drv_probe(struct platform_device *pdev)
+{
+ int err;
+ struct mpam_msc *msc;
+ struct resource *msc_res;
+ struct device *dev = &pdev->dev;
+ void *plat_data = pdev->dev.platform_data;
+
+ mutex_lock(&mpam_list_lock);
+ do {
+ msc = devm_kzalloc(&pdev->dev, sizeof(*msc), GFP_KERNEL);
+ if (!msc) {
+ err = -ENOMEM;
+ break;
+ }
+
+ mutex_init(&msc->probe_lock);
+ mutex_init(&msc->part_sel_lock);
+ mutex_init(&msc->outer_mon_sel_lock);
+ raw_spin_lock_init(&msc->inner_mon_sel_lock);
+ msc->id = pdev->id;
+ msc->pdev = pdev;
+ INIT_LIST_HEAD_RCU(&msc->all_msc_list);
+ INIT_LIST_HEAD_RCU(&msc->ris);
+
+ err = update_msc_accessibility(msc);
+ if (err)
+ break;
+ if (cpumask_empty(&msc->accessibility)) {
+ dev_err_once(dev, "MSC is not accessible from any CPU!");
+ err = -EINVAL;
+ break;
+ }
+
+ if (device_property_read_u32(&pdev->dev, "pcc-channel",
+ &msc->pcc_subspace_id))
+ msc->iface = MPAM_IFACE_MMIO;
+ else
+ msc->iface = MPAM_IFACE_PCC;
+
+ if (msc->iface == MPAM_IFACE_MMIO) {
+ void __iomem *io;
+
+ io = devm_platform_get_and_ioremap_resource(pdev, 0,
+ &msc_res);
+ if (IS_ERR(io)) {
+ dev_err_once(dev, "Failed to map MSC base address\n");
+ err = PTR_ERR(io);
+ break;
+ }
+ msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
+ msc->mapped_hwpage = io;
+ }
+
+ list_add_rcu(&msc->all_msc_list, &mpam_all_msc);
+ platform_set_drvdata(pdev, msc);
+ } while (0);
+ mutex_unlock(&mpam_list_lock);
+
+ if (!err) {
+ /* Create RIS entries described by firmware */
+ err = acpi_mpam_parse_resources(msc, plat_data);
+ }
+
+ if (err && msc)
+ mpam_msc_drv_remove(pdev);
+
+ if (!err && atomic_add_return(1, &mpam_num_msc) == fw_num_msc)
+ pr_info("Discovered all MSC\n");
+
+ return err;
+}
+
+static struct platform_driver mpam_msc_driver = {
+ .driver = {
+ .name = "mpam_msc",
+ },
+ .probe = mpam_msc_drv_probe,
+ .remove = mpam_msc_drv_remove,
+};
+
+static int __init mpam_msc_driver_init(void)
+{
+ if (!system_supports_mpam())
+ return -EOPNOTSUPP;
+
+ init_srcu_struct(&mpam_srcu);
+
+ fw_num_msc = acpi_mpam_count_msc();
+
+ if (fw_num_msc <= 0) {
+ pr_err("No MSC devices found in firmware\n");
+ return -EINVAL;
+ }
+
+ return platform_driver_register(&mpam_msc_driver);
+}
+subsys_initcall(mpam_msc_driver_init);
diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
new file mode 100644
index 000000000000..7c63d590fc98
--- /dev/null
+++ b/drivers/resctrl/mpam_internal.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+// Copyright (C) 2025 Arm Ltd.
+
+#ifndef MPAM_INTERNAL_H
+#define MPAM_INTERNAL_H
+
+#include <linux/arm_mpam.h>
+#include <linux/cpumask.h>
+#include <linux/io.h>
+#include <linux/mailbox_client.h>
+#include <linux/mutex.h>
+#include <linux/resctrl.h>
+#include <linux/sizes.h>
+
+struct mpam_msc {
+ /* member of mpam_all_msc */
+ struct list_head all_msc_list;
+
+ int id;
+ struct platform_device *pdev;
+
+ /* Not modified after mpam_is_enabled() becomes true */
+ enum mpam_msc_iface iface;
+ u32 pcc_subspace_id;
+ struct mbox_client pcc_cl;
+ struct pcc_mbox_chan *pcc_chan;
+ u32 nrdy_usec;
+ cpumask_t accessibility;
+
+ /*
+ * probe_lock is only taken during discovery. After discovery these
+ * properties become read-only and the lists are protected by SRCU.
+ */
+ struct mutex probe_lock;
+ unsigned long ris_idxs;
+ u32 ris_max;
+
+ /* mpam_msc_ris of this component */
+ struct list_head ris;
+
+ /*
+ * part_sel_lock protects access to the MSC hardware registers that are
+ * affected by MPAMCFG_PART_SEL. (including the ID registers that vary
+ * by RIS).
+ * If needed, take msc->probe_lock first.
+ */
+ struct mutex part_sel_lock;
+
+ /*
+ * mon_sel_lock protects access to the MSC hardware registers that are
+ * affected by MPAMCFG_MON_SEL.
+ * If needed, take msc->probe_lock first.
+ */
+ struct mutex outer_mon_sel_lock;
+ raw_spinlock_t inner_mon_sel_lock;
+ unsigned long inner_mon_sel_flags;
+
+ void __iomem *mapped_hwpage;
+ size_t mapped_hwpage_sz;
+};
+
+int mpam_get_cpumask_from_cache_id(unsigned long cache_id, u32 cache_level,
+ cpumask_t *affinity);
+
+#endif /* MPAM_INTERNAL_H */
--
2.39.5Hi James, On 9/10/25 21:42, James Morse wrote: > Probing MPAM is convoluted. MSCs that are integrated with a CPU may > only be accessible from those CPUs, and they may not be online. > Touching the hardware early is pointless as MPAM can't be used until > the system-wide common values for num_partid and num_pmg have been > discovered. > > Start with driver probe/remove and mapping the MSC. > > CC: Carl Worth <carl@os.amperecomputing.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile > new file mode 100644 > index 000000000000..92b48fa20108 > --- /dev/null > +++ b/drivers/resctrl/Makefile > @@ -0,0 +1,4 @@ > +obj-$(CONFIG_ARM64_MPAM_DRIVER) += mpam.o > +mpam-y += mpam_devices.o > + > +cflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG) += -DDEBUG s/cflags/ccflags/ Thanks, Ben
Hi Ben, On 17/09/2025 12:03, Ben Horgan wrote: > On 9/10/25 21:42, James Morse wrote: >> Probing MPAM is convoluted. MSCs that are integrated with a CPU may >> only be accessible from those CPUs, and they may not be online. >> Touching the hardware early is pointless as MPAM can't be used until >> the system-wide common values for num_partid and num_pmg have been >> discovered. >> >> Start with driver probe/remove and mapping the MSC. >> diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile >> new file mode 100644 >> index 000000000000..92b48fa20108 >> --- /dev/null >> +++ b/drivers/resctrl/Makefile >> @@ -0,0 +1,4 @@ >> +obj-$(CONFIG_ARM64_MPAM_DRIVER) += mpam.o >> +mpam-y += mpam_devices.o >> + >> +cflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG) += -DDEBUG > > s/cflags/ccflags/ Fixed, thanks, James
On Wed, 10 Sep 2025 20:42:47 +0000
James Morse <james.morse@arm.com> wrote:
> Probing MPAM is convoluted. MSCs that are integrated with a CPU may
> only be accessible from those CPUs, and they may not be online.
> Touching the hardware early is pointless as MPAM can't be used until
> the system-wide common values for num_partid and num_pmg have been
> discovered.
>
> Start with driver probe/remove and mapping the MSC.
>
> CC: Carl Worth <carl@os.amperecomputing.com>
> Signed-off-by: James Morse <james.morse@arm.com>
Hi James,
Various comments inline. You can ignore the do/while(0)
one but I'll probably forget and send more grumpy comments about it ;)
Jonathan
> ---
> Changes since v1:
> * Avoid selecting driver on other architectrues.
> * Removed PCC support stub.
> * Use for_each_available_child_of_node_scoped() and of_property_read_reg()
> * Clarified a comment.
> * Stopped using mpam_num_msc as an id,a and made it atomic.
> * Size of -1 returned from cache_of_calculate_id()
> * Renamed some struct members.
> * Made a bunch of pr_err() dev_err_ocne().
> * Used more cleanup magic.
> * Inlined a print message.
> * Fixed error propagation from mpam_dt_parse_resources().
> * Moved cache accessibility checks earlier.
>
> Changes since RFC:
> * Check for status=broken DT devices.
> * Moved all the files around.
> * Made Kconfig symbols depend on EXPERT
> ---
> arch/arm64/Kconfig | 1 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/resctrl/Kconfig | 14 +++
> drivers/resctrl/Makefile | 4 +
> drivers/resctrl/mpam_devices.c | 180 ++++++++++++++++++++++++++++++++
> drivers/resctrl/mpam_internal.h | 65 ++++++++++++
> 7 files changed, 267 insertions(+)
> create mode 100644 drivers/resctrl/Kconfig
> create mode 100644 drivers/resctrl/Makefile
> create mode 100644 drivers/resctrl/mpam_devices.c
> create mode 100644 drivers/resctrl/mpam_internal.h
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 6487c511bdc6..93e563e1cce4 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -2062,6 +2062,7 @@ config ARM64_TLB_RANGE
>
> config ARM64_MPAM
> bool "Enable support for MPAM"
> + select ARM64_MPAM_DRIVER if EXPERT
To me that wants a comment as it's unusual.
> select ACPI_MPAM if ACPI
> help
> Memory System Resource Partitioning and Monitoring (MPAM) is an
> diff --git a/drivers/resctrl/Kconfig b/drivers/resctrl/Kconfig
> new file mode 100644
> index 000000000000..c30532a3a3a4
> --- /dev/null
> +++ b/drivers/resctrl/Kconfig
> @@ -0,0 +1,14 @@
> +menuconfig ARM64_MPAM_DRIVER
> + bool "MPAM driver"
> + depends on ARM64 && ARM64_MPAM && EXPERT
> + help
> + MPAM driver for System IP, e,g. caches and memory controllers.
> +
> +if ARM64_MPAM_DRIVER
> +config ARM64_MPAM_DRIVER_DEBUG
> + bool "Enable debug messages from the MPAM driver"
> + depends on ARM64_MPAM_DRIVER
The depends on should make the if unnecessary.
> + help
> + Say yes here to enable debug messages from the MPAM driver.
> +
> +endif
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> new file mode 100644
> index 000000000000..efc4738e3b4d
> --- /dev/null
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2025 Arm Ltd.
> +
> +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
> +
> +#include <linux/acpi.h>
> +#include <linux/arm_mpam.h>
> +#include <linux/cacheinfo.h>
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/gfp.h>
> +#include <linux/list.h>
> +#include <linux/lockdep.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/srcu.h>
> +#include <linux/types.h>
> +/*
> + * An MSC can control traffic from a set of CPUs, but may only be accessible
> + * from a (hopefully wider) set of CPUs. The common reason for this is power
> + * management. If all the CPUs in a cluster are in PSCI:CPU_SUSPEND, the
> + * corresponding cache may also be powered off. By making accesses from
> + * one of those CPUs, we ensure this isn't the case.
> + */
> +static int update_msc_accessibility(struct mpam_msc *msc)
> +{
> + u32 affinity_id;
> + int err;
> +
> + err = device_property_read_u32(&msc->pdev->dev, "cpu_affinity",
> + &affinity_id);
> + if (err)
> + cpumask_copy(&msc->accessibility, cpu_possible_mask);
> + else
> + acpi_pptt_get_cpus_from_container(affinity_id,
> + &msc->accessibility);
> +
> + return 0;
> +
> + return err;
Curious. I'd do a build test after each patch before v3. A couple of
places would have failed or given helpful warnings so far.
> +}
> +
> +static int mpam_msc_drv_probe(struct platform_device *pdev)
> +{
> + int err;
> + struct mpam_msc *msc;
> + struct resource *msc_res;
> + struct device *dev = &pdev->dev;
> + void *plat_data = pdev->dev.platform_data;
> +
> + mutex_lock(&mpam_list_lock);
> + do {
I might well have moaned about this before, but I really dislike a do while(0)
if it doesn't fit on my screen (and my eyesight is poor so that's not this
many lines). To me a non trivial case of this is almost always a place
where a '_do' function would have made it more readable.
I'm also not a fan of scoped_guard() plus breaks because it feels like
it is dependent on an implementation detail but maybe it's clearer than this.
> + msc = devm_kzalloc(&pdev->dev, sizeof(*msc), GFP_KERNEL);
> + if (!msc) {
> + err = -ENOMEM;
> + break;
> + }
> +
> + mutex_init(&msc->probe_lock);
> + mutex_init(&msc->part_sel_lock);
> + mutex_init(&msc->outer_mon_sel_lock);
> + raw_spin_lock_init(&msc->inner_mon_sel_lock);
> + msc->id = pdev->id;
> + msc->pdev = pdev;
> + INIT_LIST_HEAD_RCU(&msc->all_msc_list);
> + INIT_LIST_HEAD_RCU(&msc->ris);
> +
> + err = update_msc_accessibility(msc);
> + if (err)
> + break;
> + if (cpumask_empty(&msc->accessibility)) {
> + dev_err_once(dev, "MSC is not accessible from any CPU!");
> + err = -EINVAL;
> + break;
> + }
> +
> + if (device_property_read_u32(&pdev->dev, "pcc-channel",
> + &msc->pcc_subspace_id))
> + msc->iface = MPAM_IFACE_MMIO;
> + else
> + msc->iface = MPAM_IFACE_PCC;
> +
> + if (msc->iface == MPAM_IFACE_MMIO) {
> + void __iomem *io;
> +
> + io = devm_platform_get_and_ioremap_resource(pdev, 0,
> + &msc_res);
> + if (IS_ERR(io)) {
> + dev_err_once(dev, "Failed to map MSC base address\n");
> + err = PTR_ERR(io);
> + break;
> + }
> + msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
> + msc->mapped_hwpage = io;
> + }
> +
> + list_add_rcu(&msc->all_msc_list, &mpam_all_msc);
> + platform_set_drvdata(pdev, msc);
> + } while (0);
> + mutex_unlock(&mpam_list_lock);
> +
> + if (!err) {
> + /* Create RIS entries described by firmware */
> + err = acpi_mpam_parse_resources(msc, plat_data);
> + }
> +
> + if (err && msc)
> + mpam_msc_drv_remove(pdev);
Is it worth bothering to remove? We failed probe anyway if we got here
and it's not expected to happen on real systems so I'd just leave it around
so that you can exit early above.
I'm also not following why the msc check is relevant if you do want to do
this. Can only get here without msc if the allocation failed. Why would
we leave the driver loaded in only that case?
> +
> + if (!err && atomic_add_return(1, &mpam_num_msc) == fw_num_msc)
> + pr_info("Discovered all MSC\n");
> +
> + return err;
> +}
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> new file mode 100644
> index 000000000000..7c63d590fc98
> --- /dev/null
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +// Copyright (C) 2025 Arm Ltd.
> +
> +#ifndef MPAM_INTERNAL_H
> +#define MPAM_INTERNAL_H
> +
> +#include <linux/arm_mpam.h>
> +#include <linux/cpumask.h>
> +#include <linux/io.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mutex.h>
spinlock.h
> +#include <linux/resctrl.h>
Not spotting anything rsctl yet. So maybe this belongs later.
> +#include <linux/sizes.h>
> +
> +struct mpam_msc {
> + /* member of mpam_all_msc */
> + struct list_head all_msc_list;
> +
> + int id;
I'd follow (approx) include what you use principles to make later header
shuffling easier. So a forward def for this.
> + struct platform_device *pdev;
> +
> + /* Not modified after mpam_is_enabled() becomes true */
> + enum mpam_msc_iface iface;
> + u32 pcc_subspace_id;
> + struct mbox_client pcc_cl;
> + struct pcc_mbox_chan *pcc_chan;
Forward def or include acpi/pcc.h
> + u32 nrdy_usec;
> + cpumask_t accessibility;
> +
> + /*
> + * probe_lock is only taken during discovery. After discovery these
> + * properties become read-only and the lists are protected by SRCU.
> + */
> + struct mutex probe_lock;
> + unsigned long ris_idxs;
> + u32 ris_max;
> +
> + /* mpam_msc_ris of this component */
> + struct list_head ris;
> +
> + /*
> + * part_sel_lock protects access to the MSC hardware registers that are
> + * affected by MPAMCFG_PART_SEL. (including the ID registers that vary
> + * by RIS).
> + * If needed, take msc->probe_lock first.
> + */
> + struct mutex part_sel_lock;
> +
> + /*
> + * mon_sel_lock protects access to the MSC hardware registers that are
> + * affected by MPAMCFG_MON_SEL.
> + * If needed, take msc->probe_lock first.
> + */
> + struct mutex outer_mon_sel_lock;
> + raw_spinlock_t inner_mon_sel_lock;
> + unsigned long inner_mon_sel_flags;
> +
> + void __iomem *mapped_hwpage;
> + size_t mapped_hwpage_sz;
> +};
> +
> +int mpam_get_cpumask_from_cache_id(unsigned long cache_id, u32 cache_level,
> + cpumask_t *affinity);
Where is this?
> +
> +#endif /* MPAM_INTERNAL_H */
Hi Jonathan,
On 11/09/2025 14:35, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 20:42:47 +0000
> James Morse <james.morse@arm.com> wrote:
>
>> Probing MPAM is convoluted. MSCs that are integrated with a CPU may
>> only be accessible from those CPUs, and they may not be online.
>> Touching the hardware early is pointless as MPAM can't be used until
>> the system-wide common values for num_partid and num_pmg have been
>> discovered.
>>
>> Start with driver probe/remove and mapping the MSC.
> Hi James,
>
> Various comments inline. You can ignore the do/while(0)
> one but I'll probably forget and send more grumpy comments about it ;)
I got exposed to this quite a lot in some other project - its better than a goto.
Due to the heavy exposure, I don't think its odd - I'm just used to seeing it.
(not sure its quite Stockholm syndrome yet)
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 6487c511bdc6..93e563e1cce4 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -2062,6 +2062,7 @@ config ARM64_TLB_RANGE
>>
>> config ARM64_MPAM
>> bool "Enable support for MPAM"
>> + select ARM64_MPAM_DRIVER if EXPERT
>
> To me that wants a comment as it's unusual.
Sure - "# does nothing yet".
This is to stop it being enabled in distros as it can't be used until the resctrl part is
added.
>> diff --git a/drivers/resctrl/Kconfig b/drivers/resctrl/Kconfig
>> new file mode 100644
>> index 000000000000..c30532a3a3a4
>> --- /dev/null
>> +++ b/drivers/resctrl/Kconfig
>> @@ -0,0 +1,14 @@
>> +menuconfig ARM64_MPAM_DRIVER
>> + bool "MPAM driver"
>> + depends on ARM64 && ARM64_MPAM && EXPERT
>> + help
>> + MPAM driver for System IP, e,g. caches and memory controllers.
>> +
>> +if ARM64_MPAM_DRIVER
>> +config ARM64_MPAM_DRIVER_DEBUG
>> + bool "Enable debug messages from the MPAM driver"
>> + depends on ARM64_MPAM_DRIVER
>
> The depends on should make the if unnecessary.
Yup, that was a recent change and I didn't spot the duplication.
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> new file mode 100644
>> index 000000000000..efc4738e3b4d
>> --- /dev/null
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -0,0 +1,180 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2025 Arm Ltd.
>> +
>> +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/arm_mpam.h>
>> +#include <linux/cacheinfo.h>
>> +#include <linux/cpu.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
>> +#include <linux/gfp.h>
>> +#include <linux/list.h>
>> +#include <linux/lockdep.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/printk.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/srcu.h>
>> +#include <linux/types.h>
>
>> +/*
>> + * An MSC can control traffic from a set of CPUs, but may only be accessible
>> + * from a (hopefully wider) set of CPUs. The common reason for this is power
>> + * management. If all the CPUs in a cluster are in PSCI:CPU_SUSPEND, the
>> + * corresponding cache may also be powered off. By making accesses from
>> + * one of those CPUs, we ensure this isn't the case.
>> + */
>> +static int update_msc_accessibility(struct mpam_msc *msc)
>> +{
>> + u32 affinity_id;
>> + int err;
>> +
>> + err = device_property_read_u32(&msc->pdev->dev, "cpu_affinity",
>> + &affinity_id);
>> + if (err)
>> + cpumask_copy(&msc->accessibility, cpu_possible_mask);
>> + else
>> + acpi_pptt_get_cpus_from_container(affinity_id,
>> + &msc->accessibility);
>> +
>> + return 0;
>> +
>> + return err;
>
> Curious. I'd do a build test after each patch before v3. A couple of
> places would have failed or given helpful warnings so far.
Oddly - the compiler is quite happy with this. Not sure why.
I've fixed it to return err.
This was due to the last second rip-out of the DT support.
>> +}
>
>> +
>> +static int mpam_msc_drv_probe(struct platform_device *pdev)
>> +{
>> + int err;
>> + struct mpam_msc *msc;
>> + struct resource *msc_res;
>> + struct device *dev = &pdev->dev;
>> + void *plat_data = pdev->dev.platform_data;
>> +
>> + mutex_lock(&mpam_list_lock);
>> + do {
>
> I might well have moaned about this before, but I really dislike a do while(0)
> if it doesn't fit on my screen (and my eyesight is poor so that's not this
> many lines). To me a non trivial case of this is almost always a place
> where a '_do' function would have made it more readable.
I guess I'm very used to seeing this pattern, so I don't notice it.
I've change this to a do_ thing with all this shoved in another function.
> I'm also not a fan of scoped_guard() plus breaks because it feels like
> it is dependent on an implementation detail but maybe it's clearer than this.
(i.e. the cleanup stuff? I'd always prefer to see the free/unlock call)
>> + msc = devm_kzalloc(&pdev->dev, sizeof(*msc), GFP_KERNEL);
>> + if (!msc) {
>> + err = -ENOMEM;
>> + break;
>> + }
>> +
>> + mutex_init(&msc->probe_lock);
>> + mutex_init(&msc->part_sel_lock);
>> + mutex_init(&msc->outer_mon_sel_lock);
>> + raw_spin_lock_init(&msc->inner_mon_sel_lock);
>> + msc->id = pdev->id;
>> + msc->pdev = pdev;
>> + INIT_LIST_HEAD_RCU(&msc->all_msc_list);
>> + INIT_LIST_HEAD_RCU(&msc->ris);
>> +
>> + err = update_msc_accessibility(msc);
>> + if (err)
>> + break;
>> + if (cpumask_empty(&msc->accessibility)) {
>> + dev_err_once(dev, "MSC is not accessible from any CPU!");
>> + err = -EINVAL;
>> + break;
>> + }
>> +
>> + if (device_property_read_u32(&pdev->dev, "pcc-channel",
>> + &msc->pcc_subspace_id))
>> + msc->iface = MPAM_IFACE_MMIO;
>> + else
>> + msc->iface = MPAM_IFACE_PCC;
>> +
>> + if (msc->iface == MPAM_IFACE_MMIO) {
>> + void __iomem *io;
>> +
>> + io = devm_platform_get_and_ioremap_resource(pdev, 0,
>> + &msc_res);
>> + if (IS_ERR(io)) {
>> + dev_err_once(dev, "Failed to map MSC base address\n");
>> + err = PTR_ERR(io);
>> + break;
>> + }
>> + msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
>> + msc->mapped_hwpage = io;
>> + }
>> +
>> + list_add_rcu(&msc->all_msc_list, &mpam_all_msc);
>> + platform_set_drvdata(pdev, msc);
>> + } while (0);
>> + mutex_unlock(&mpam_list_lock);
>> +
>> + if (!err) {
>> + /* Create RIS entries described by firmware */
>> + err = acpi_mpam_parse_resources(msc, plat_data);
>> + }
>> +
>> + if (err && msc)
>> + mpam_msc_drv_remove(pdev);
>
> Is it worth bothering to remove? We failed probe anyway if we got here
> and it's not expected to happen on real systems so I'd just leave it around
> so that you can exit early above.
Symmetry and the principle of least surprise.
Ideally the probing code wouldn't need to unwind what it did, it can rely on the remove
code to do the necessary. It was a patch from Carl Worth that did this as the
unwind/remove code was duplication.
I agree there is no error that requires it here, but later patches add things like debugfs
entries that need removing before the struct mpam_msc memory can be free'd - potentially
by the devm stuff if the probe function returns an error due to the firmware tables 'RIS'
description being wonky.
> I'm also not following why the msc check is relevant if you do want to do
> this. Can only get here without msc if the allocation failed. Why would
> we leave the driver loaded in only that case?
Simply because the remove/destroy code goes dereferencing msc, so that is the one case
that mustn't get in there. With your do_ version of the above this got simplified.
>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
>> new file mode 100644
>> index 000000000000..7c63d590fc98
>> --- /dev/null
>> +++ b/drivers/resctrl/mpam_internal.h
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +// Copyright (C) 2025 Arm Ltd.
>> +
>> +#ifndef MPAM_INTERNAL_H
>> +#define MPAM_INTERNAL_H
>> +
>> +#include <linux/arm_mpam.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/io.h>
>> +#include <linux/mailbox_client.h>
>> +#include <linux/mutex.h>
>
> spinlock.h
Fixed,
>> +#include <linux/resctrl.h>
>
> Not spotting anything rsctl yet. So maybe this belongs later.
There shouldn't be anything that depends on resctrl in this series - looks like
this is a 2018 era bug in the way I carved this up!
>> +#include <linux/sizes.h>
>> +
>> +struct mpam_msc {
>> + /* member of mpam_all_msc */
>> + struct list_head all_msc_list;
>> +
>> + int id;
>
> I'd follow (approx) include what you use principles to make later header
> shuffling easier. So a forward def for this.
-ENOPARSE
I'm sure I'll work this out from your later comments.
>> + struct platform_device *pdev;
>> +
>> + /* Not modified after mpam_is_enabled() becomes true */
>> + enum mpam_msc_iface iface;
>> + u32 pcc_subspace_id;
>> + struct mbox_client pcc_cl;
>> + struct pcc_mbox_chan *pcc_chan;
>
> Forward def or include acpi/pcc.h
The PCC code got pulled later, and out of this series. I missed these bits.
>> + u32 nrdy_usec;
>> + cpumask_t accessibility;
>> +
>> + /*
>> + * probe_lock is only taken during discovery. After discovery these
>> + * properties become read-only and the lists are protected by SRCU.
>> + */
>> + struct mutex probe_lock;
>> + unsigned long ris_idxs;
>> + u32 ris_max;
>> +
>> + /* mpam_msc_ris of this component */
>> + struct list_head ris;
>> +
>> + /*
>> + * part_sel_lock protects access to the MSC hardware registers that are
>> + * affected by MPAMCFG_PART_SEL. (including the ID registers that vary
>> + * by RIS).
>> + * If needed, take msc->probe_lock first.
>> + */
>> + struct mutex part_sel_lock;
>> +
>> + /*
>> + * mon_sel_lock protects access to the MSC hardware registers that are
>> + * affected by MPAMCFG_MON_SEL.
>> + * If needed, take msc->probe_lock first.
>> + */
>> + struct mutex outer_mon_sel_lock;
>> + raw_spinlock_t inner_mon_sel_lock;
>> + unsigned long inner_mon_sel_flags;
>> +
>> + void __iomem *mapped_hwpage;
>> + size_t mapped_hwpage_sz;
>> +};
>> +
>> +int mpam_get_cpumask_from_cache_id(unsigned long cache_id, u32 cache_level,
>> + cpumask_t *affinity);
>
> Where is this?
Ugh, more bits of DT - this one is non-obvious because of the name.
Thanks,
James
> >> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> >> new file mode 100644
> >> index 000000000000..7c63d590fc98
> >> --- /dev/null
> >> +++ b/drivers/resctrl/mpam_internal.h
> >> @@ -0,0 +1,65 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +// Copyright (C) 2025 Arm Ltd.
> >> +
> >> +#ifndef MPAM_INTERNAL_H
> >> +#define MPAM_INTERNAL_H
> >> +
> >> +#include <linux/arm_mpam.h>
> >> +#include <linux/cpumask.h>
> >> +#include <linux/io.h>
> >> +#include <linux/mailbox_client.h>
> >> +#include <linux/mutex.h>
> >
> > spinlock.h
>
> Fixed,
>
>
> >> +#include <linux/resctrl.h>
> >
> > Not spotting anything rsctl yet. So maybe this belongs later.
>
> There shouldn't be anything that depends on resctrl in this series - looks like
> this is a 2018 era bug in the way I carved this up!
>
>
> >> +#include <linux/sizes.h>
> >> +
> >> +struct mpam_msc {
> >> + /* member of mpam_all_msc */
> >> + struct list_head all_msc_list;
> >> +
> >> + int id;
> >
> > I'd follow (approx) include what you use principles to make later header
> > shuffling easier. So a forward def for this.
>
> -ENOPARSE
>
> I'm sure I'll work this out from your later comments.
I missed on the comment (I think). Would have made more sense a line later.
Add a forwards def
struct platform_device;
as no reason to include the appropriate header
(and you didn't anwyay).
© 2016 - 2025 Red Hat, Inc.