Re: [PATCH 1/5] arm_mpam: Parse the rest of the ACPI table

Re: [PATCH 1/5] arm_mpam: Parse the rest of the ACPI table

[Ben Horgan]

Hi Andre,

On 4/29/26 15:13, Andre Przywara wrote:
> From: James Morse <james.morse@arm.com>
> 
> The MPAM ACPI table lists the MPAM MSCs and indicates which resources
> in the system they control. Not everything this table can describe is
> supported by resctrl, e.g. memory-side-caches.
> 
> Add the additional table parsing to avoid reporting these as 'unknown'
> to the MPAM driver. This allows class+component hierarchys to be built.
> 
> Until resctrl has support for any of these resources, users would be
> in-kernel managers of a resource/PARTID or perf to query bandwidth
> counters on a resource resctrl is unaware of.

Does this patch give us anything needed for MPAM-Fb? It seems to just add
parsing for things we don't support in the MPAM driver.

Thanks,

Ben

> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/acpi/arm64/mpam.c | 91 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 88 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/mpam.c b/drivers/acpi/arm64/mpam.c
> index 84963a20c3e7..99c2bdbb3314 100644
> --- a/drivers/acpi/arm64/mpam.c
> +++ b/drivers/acpi/arm64/mpam.c
> @@ -95,17 +95,51 @@ static void acpi_mpam_parse_irqs(struct platform_device *pdev,
>  		res[(*res_idx)++] = DEFINE_RES_IRQ_NAMED(irq, "error");
>  }
>  
> -static int acpi_mpam_parse_resource(struct mpam_msc *msc,
> +#define UUID_MPAM_INTERCONNECT_TABLE		"fe2bd645-033b-49e6-9479-2e0b8b21d1cd"
> +
> +struct acpi_mpam_interconnect_descriptor_table {
> +	u8	type_uuid[16];
> +	u32	num_descriptors;
> +};
> +
> +struct acpi_mpam_interconnect_descriptor {
> +	u32	source_id;
> +	u32	destination_id;
> +	u8	link_type;
> +	u8	reserved[3];
> +};
> +
> +static int acpi_mpam_parse_resource(struct acpi_mpam_msc_node *tbl_msc,
> +				    struct mpam_msc *msc,
>  				    struct acpi_mpam_resource_node *res)
>  {
> +	struct acpi_mpam_interconnect_descriptor_table *tbl_int_tbl;
> +	struct acpi_mpam_interconnect_descriptor *tbl_int;
> +	guid_t int_tbl_uuid, spec_uuid;
>  	int level, nid;
>  	u32 cache_id;
> +	off_t offset;
>  
> +	/*
> +	 * Class IDs are somewhat arbitrary, but need to be co-ordinated.
> +	 * 0-N are caches,
> +	 * 64, 65: Interconnect, but ideally these would appear between the
> +	 *     classes the controls are adjacent to.
> +	 * 128: SMMU,
> +	 * 192-192+level: Memory Side Caches, nothing checks that N is a
> +	 *                small number.
> +	 * 255: Memory Controllers
> +	 *
> +	 * ACPI devices would need a class id allocated based on the _HID.
> +	 *
> +	 * Classes that the mpam driver can't currently plumb into resctrl
> +	 * are registered as UNKNOWN.
> +	 */
>  	switch (res->locator_type) {
>  	case ACPI_MPAM_LOCATION_TYPE_PROCESSOR_CACHE:
>  		cache_id = res->locator.cache_locator.cache_reference;
>  		level = find_acpi_cache_level_from_id(cache_id);
> -		if (level <= 0) {
> +		if (level <= 0 || level >= 64) {
>  			pr_err_once("Bad level (%d) for cache with id %u\n", level, cache_id);
>  			return -EINVAL;
>  		}
> @@ -120,6 +154,57 @@ static int acpi_mpam_parse_resource(struct mpam_msc *msc,
>  		}
>  		return mpam_ris_create(msc, res->ris_index, MPAM_CLASS_MEMORY,
>  				       MPAM_CLASS_ID_DEFAULT, nid);
> +	case ACPI_MPAM_LOCATION_TYPE_SMMU:
> +		return mpam_ris_create(msc, res->ris_index, MPAM_CLASS_UNKNOWN,
> +				       128, res->locator.smmu_locator.smmu_interface);
> +	case ACPI_MPAM_LOCATION_TYPE_MEMORY_CACHE:
> +		cache_id = res->locator.mem_cache_locator.reference;
> +		level = res->locator.mem_cache_locator.level;
> +		if (192 + level >= 255) {
> +			pr_err_once("Bad level for memory side cache with reference %u\n",
> +				    cache_id);
> +			return -EINVAL;
> +		}
> +
> +		return mpam_ris_create(msc, res->ris_index, MPAM_CLASS_CACHE,
> +				       192 + level, cache_id);
> +
> +	case ACPI_MPAM_LOCATION_TYPE_INTERCONNECT:
> +		/* Find the descriptor table, and check it lands in the parent msc */
> +		offset = res->locator.interconnect_ifc_locator.inter_connect_desc_tbl_off;
> +		if (offset >= tbl_msc->length) {
> +			pr_err_once("Bad offset for interconnect descriptor on msc %u\n",
> +				    tbl_msc->identifier);
> +			return -EINVAL;
> +		}
> +		tbl_int_tbl = ACPI_ADD_PTR(struct acpi_mpam_interconnect_descriptor_table,
> +					   tbl_msc, offset);
> +		guid_parse(UUID_MPAM_INTERCONNECT_TABLE, &spec_uuid);
> +		import_guid(&int_tbl_uuid, tbl_int_tbl->type_uuid);
> +		if (guid_equal(&spec_uuid, &int_tbl_uuid)) {
> +			pr_err_once("Bad UUID for interconnect descriptor on msc %u\n",
> +				    tbl_msc->identifier);
> +			return -EINVAL;
> +		}
> +
> +		offset += sizeof(*tbl_int_tbl);
> +		offset += tbl_int_tbl->num_descriptors * sizeof(*tbl_int);
> +		if (offset >= tbl_msc->length) {
> +			pr_err_once("Bad num_descriptors for interconnect descriptor on msc %u\n",
> +				    tbl_msc->identifier);
> +			return -EINVAL;
> +		}
> +
> +		tbl_int = ACPI_ADD_PTR(struct acpi_mpam_interconnect_descriptor,
> +				       tbl_int_tbl, sizeof(*tbl_int_tbl));
> +		cache_id = tbl_int->source_id;
> +
> +		/* Unknown link type? */
> +		if (tbl_int->link_type != 0 && tbl_int->link_type == 1)
> +			return 0;
> +
> +		return mpam_ris_create(msc, res->ris_index, MPAM_CLASS_UNKNOWN,
> +				       64 + tbl_int->link_type, cache_id);
>  	default:
>  		/* These get discovered later and are treated as unknown */
>  		return 0;
> @@ -150,7 +235,7 @@ int acpi_mpam_parse_resources(struct mpam_msc *msc,
>  			return -EINVAL;
>  		}
>  
> -		err = acpi_mpam_parse_resource(msc, resource);
> +		err = acpi_mpam_parse_resource(tbl_msc, msc, resource);
>  		if (err)
>  			return err;
>  

Re: [PATCH 2/5] arm_mpam: Split the locking around the mon_sel registers

[Ben Horgan]

Hi Andre,

On 4/29/26 15:13, Andre Przywara wrote:
> From: James Morse <james.morse@arm.com>
> 
> The MSC MON_SEL register needs to be accessed from hardirq for the overflow
> interrupt, and when taking an IPI to access these registers on platforms
> where MSC are not accesible from every CPU. This makes an irqsave
> spinlock the obvious lock to protect these registers. On systems with SCMI
> mailboxes it must be able to sleep, meaning a mutex must be used. The
> SCMI platforms can't support an overflow interrupt.
> 
> Clearly these two can't exist for one MSC at the same time.
> 
> Split the existing helper into a raw spinlock and a mutex, named inner
> and outer. The outer lock must be taken in an a pre-emptible context
> befroe the inner lock can be taken. On systems with SCMI mailboxes
> where the MON_SEL accesses must sleep - the inner lock will fail tobe
> taken if the caller is unable to sleep.
> This will allow callers to fail withuot having to explicitly check
> the interface type of each MSC.

Given that we have no support for perf with MPAM can we simplify the locking and
avoid adding this complexity until it's required? Would switching to just a mutex
for the mon_sel_lock work?

Thanks,

Ben

> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  drivers/resctrl/mpam_devices.c  | 56 ++++++++++++++++++++-------
>  drivers/resctrl/mpam_internal.h | 67 ++++++++++++++++++++++++---------
>  2 files changed, 91 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 41b14344b16f..3be09b0c46ae 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -735,7 +735,7 @@ static bool _mpam_ris_hw_probe_hw_nrdy(struct mpam_msc_ris *ris, u32 mon_reg)
>  	bool can_set, can_clear;
>  	struct mpam_msc *msc = ris->vmsc->msc;
>  
> -	if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> +	if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(msc)))
>  		return false;
>  
>  	mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) |
> @@ -749,7 +749,7 @@ static bool _mpam_ris_hw_probe_hw_nrdy(struct mpam_msc_ris *ris, u32 mon_reg)
>  	_mpam_write_monsel_reg(msc, mon_reg, 0);
>  	now = _mpam_read_monsel_reg(msc, mon_reg);
>  	can_clear = !(now & MSMON___NRDY);
> -	mpam_mon_sel_unlock(msc);
> +	mpam_mon_sel_inner_unlock(msc);
>  
>  	return (!can_set || !can_clear);
>  }
> @@ -873,7 +873,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>  					mpam_set_feature(mpam_feat_msmon_csu_xcl, props);
>  
>  				/* Is NRDY hardware managed? */
> +				mpam_mon_sel_outer_lock(msc);
>  				hw_managed = mpam_ris_hw_probe_hw_nrdy(ris, CSU);
> +				mpam_mon_sel_outer_unlock(msc);
>  				if (hw_managed)
>  					mpam_set_feature(mpam_feat_msmon_csu_hw_nrdy, props);
>  			}
> @@ -907,7 +909,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>  				}
>  
>  				/* Is NRDY hardware managed? */
> +				mpam_mon_sel_outer_lock(msc);
>  				hw_managed = mpam_ris_hw_probe_hw_nrdy(ris, MBWU);
> +				mpam_mon_sel_outer_unlock(msc);
>  				if (hw_managed)
>  					mpam_set_feature(mpam_feat_msmon_mbwu_hw_nrdy, props);
>  
> @@ -1201,7 +1205,7 @@ static void __ris_msmon_read(void *arg)
>  	struct mpam_msc *msc = m->ris->vmsc->msc;
>  	u32 mon_sel, ctl_val, flt_val, cur_ctl, cur_flt;
>  
> -	if (!mpam_mon_sel_lock(msc)) {
> +	if (!mpam_mon_sel_inner_lock(msc)) {
>  		m->err = -EIO;
>  		return;
>  	}
> @@ -1301,7 +1305,7 @@ static void __ris_msmon_read(void *arg)
>  	default:
>  		m->err = -EINVAL;
>  	}
> -	mpam_mon_sel_unlock(msc);
> +	mpam_mon_sel_inner_unlock(msc);
>  
>  	if (nrdy)
>  		m->err = -EBUSY;
> @@ -1323,6 +1327,7 @@ static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
>  		struct mpam_msc *msc = vmsc->msc;
>  		struct mpam_msc_ris *ris;
>  
> +		mpam_mon_sel_outer_lock(msc);
>  		list_for_each_entry_srcu(ris, &vmsc->ris, vmsc_list,
>  					 srcu_read_lock_held(&mpam_srcu)) {
>  			arg->ris = ris;
> @@ -1341,6 +1346,7 @@ static int _msmon_read(struct mpam_component *comp, struct mon_read *arg)
>  			if (err)
>  				any_err = err;
>  		}
> +		mpam_mon_sel_outer_unlock(msc);
>  	}
>  
>  	return any_err;
> @@ -1423,18 +1429,20 @@ void mpam_msmon_reset_mbwu(struct mpam_component *comp, struct mon_cfg *ctx)
>  			continue;
>  
>  		msc = vmsc->msc;
> +		mpam_mon_sel_outer_lock(msc);
>  		list_for_each_entry_srcu(ris, &vmsc->ris, vmsc_list,
>  					 srcu_read_lock_held(&mpam_srcu)) {
>  			if (!mpam_has_feature(mpam_feat_msmon_mbwu, &ris->props))
>  				continue;
>  
> -			if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> +			if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(msc)))
>  				continue;
>  
>  			ris->mbwu_state[ctx->mon].correction = 0;
>  			ris->mbwu_state[ctx->mon].reset_on_next_read = true;
> -			mpam_mon_sel_unlock(msc);
> +			mpam_mon_sel_inner_unlock(msc);
>  		}
> +		mpam_mon_sel_outer_unlock(msc);
>  	}
>  }
>  
> @@ -1635,8 +1643,11 @@ static int mpam_restore_mbwu_state(void *_ris)
>  	u64 val;
>  	struct mon_read mwbu_arg;
>  	struct mpam_msc_ris *ris = _ris;
> +	struct mpam_msc *msc = ris->vmsc->msc;
>  	struct mpam_class *class = ris->vmsc->comp->class;
>  
> +	mpam_mon_sel_outer_lock(msc);
> +
>  	for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>  		if (ris->mbwu_state[i].enabled) {
>  			mwbu_arg.ris = ris;
> @@ -1648,10 +1659,12 @@ static int mpam_restore_mbwu_state(void *_ris)
>  		}
>  	}
>  
> +	mpam_mon_sel_outer_unlock(msc);
> +
>  	return 0;
>  }
>  
> -/* Call with MSC cfg_lock held */
> +/* Call with MSC cfg_lock and outer mon_sel lock held */
>  static int mpam_save_mbwu_state(void *arg)
>  {
>  	int i;
> @@ -1666,7 +1679,7 @@ static int mpam_save_mbwu_state(void *arg)
>  		mbwu_state = &ris->mbwu_state[i];
>  		cfg = &mbwu_state->cfg;
>  
> -		if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
> +		if (WARN_ON_ONCE(!mpam_mon_sel_inner_lock(msc)))
>  			return -EIO;
>  
>  		mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, i) |
> @@ -1691,7 +1704,7 @@ static int mpam_save_mbwu_state(void *arg)
>  		cfg->partid = FIELD_GET(MSMON_CFG_x_FLT_PARTID, cur_flt);
>  		mbwu_state->correction += val;
>  		mbwu_state->enabled = FIELD_GET(MSMON_CFG_x_CTL_EN, cur_ctl);
> -		mpam_mon_sel_unlock(msc);
> +		mpam_mon_sel_inner_unlock(msc);
>  	}
>  
>  	return 0;
> @@ -1773,6 +1786,7 @@ static void mpam_reprogram_msc(struct mpam_msc *msc)
>  	mpam_assert_partid_sizes_fixed();
>  
>  	mutex_lock(&msc->cfg_lock);
> +	mpam_mon_sel_outer_lock(msc);
>  	list_for_each_entry_srcu(ris, &msc->ris, msc_list,
>  				 srcu_read_lock_held(&mpam_srcu)) {
>  		if (!mpam_is_enabled() && !ris->in_reset_state) {
> @@ -1797,6 +1811,7 @@ static void mpam_reprogram_msc(struct mpam_msc *msc)
>  		if (mpam_has_feature(mpam_feat_msmon_mbwu, &ris->props))
>  			mpam_touch_msc(msc, &mpam_restore_mbwu_state, ris);
>  	}
> +	mpam_mon_sel_outer_unlock(msc);
>  	mutex_unlock(&msc->cfg_lock);
>  }
>  
> @@ -1886,6 +1901,7 @@ static int mpam_cpu_offline(unsigned int cpu)
>  			struct mpam_msc_ris *ris;
>  
>  			mutex_lock(&msc->cfg_lock);
> +			mpam_mon_sel_outer_lock(msc);
>  			list_for_each_entry_srcu(ris, &msc->ris, msc_list,
>  						 srcu_read_lock_held(&mpam_srcu)) {
>  				mpam_touch_msc(msc, &mpam_reset_ris, ris);
> @@ -1899,6 +1915,7 @@ static int mpam_cpu_offline(unsigned int cpu)
>  				if (mpam_is_enabled())
>  					mpam_touch_msc(msc, &mpam_save_mbwu_state, ris);
>  			}
> +			mpam_mon_sel_outer_unlock(msc);
>  			mutex_unlock(&msc->cfg_lock);
>  		}
>  	}
> @@ -2589,11 +2606,13 @@ static void __destroy_component_cfg(struct mpam_component *comp)
>  	list_for_each_entry(vmsc, &comp->vmsc, comp_list) {
>  		msc = vmsc->msc;
>  
> -		if (mpam_mon_sel_lock(msc)) {
> +		mpam_mon_sel_outer_lock(msc);
> +		if (mpam_mon_sel_inner_lock(msc)) {
>  			list_for_each_entry(ris, &vmsc->ris, vmsc_list)
>  				add_to_garbage(ris->mbwu_state);
> -			mpam_mon_sel_unlock(msc);
> +			mpam_mon_sel_inner_unlock(msc);
>  		}
> +		mpam_mon_sel_outer_unlock(msc);
>  	}
>  }
>  
> @@ -2640,6 +2659,7 @@ static int __allocate_component_cfg(struct mpam_component *comp)
>  	mpam_reset_component_cfg(comp);
>  
>  	list_for_each_entry(vmsc, &comp->vmsc, comp_list) {
> +		int err = 0;
>  		struct mpam_msc *msc;
>  		struct mpam_msc_ris *ris;
>  		struct msmon_mbwu_state *mbwu_state;
> @@ -2648,6 +2668,7 @@ static int __allocate_component_cfg(struct mpam_component *comp)
>  			continue;
>  
>  		msc = vmsc->msc;
> +		mpam_mon_sel_outer_lock(msc);
>  		list_for_each_entry(ris, &vmsc->ris, vmsc_list) {
>  			if (!ris->props.num_mbwu_mon)
>  				continue;
> @@ -2656,16 +2677,21 @@ static int __allocate_component_cfg(struct mpam_component *comp)
>  						  ris->props.num_mbwu_mon);
>  			if (!mbwu_state) {
>  				__destroy_component_cfg(comp);
> -				return -ENOMEM;
> +				err = -ENOMEM;
> +				break;
>  			}
>  
>  			init_garbage(&mbwu_state[0].garbage);
>  
> -			if (mpam_mon_sel_lock(msc)) {
> +			if (mpam_mon_sel_inner_lock(msc)) {
>  				ris->mbwu_state = mbwu_state;
> -				mpam_mon_sel_unlock(msc);
> +				mpam_mon_sel_inner_unlock(msc);
>  			}
>  		}
> +		mpam_mon_sel_outer_unlock(msc);
> +
> +		if (err)
> +			return err;
>  	}
>  
>  	return 0;
> @@ -2918,12 +2944,14 @@ int mpam_apply_config(struct mpam_component *comp, u16 partid,
>  		msc = vmsc->msc;
>  
>  		mutex_lock(&msc->cfg_lock);
> +		mpam_mon_sel_outer_lock(msc);
>  		list_for_each_entry_srcu(ris, &vmsc->ris, vmsc_list,
>  					 srcu_read_lock_held(&mpam_srcu)) {
>  			arg.ris = ris;
>  			mpam_touch_msc(msc, __write_config, &arg);
>  			ris->in_reset_state = false;
>  		}
> +		mpam_mon_sel_outer_unlock(msc);
>  		mutex_unlock(&msc->cfg_lock);
>  	}
>  
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index 1914aefdcba9..80213af10a64 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -116,16 +116,20 @@ struct mpam_msc {
>  	/*
>  	 * mon_sel_lock protects access to the MSC hardware registers that are
>  	 * affected by MPAMCFG_MON_SEL, and the mbwu_state.
> -	 * Access to mon_sel is needed from both process and interrupt contexts,
> -	 * but is complicated by firmware-backed platforms that can't make any
> -	 * access unless they can sleep.
> -	 * Always use the mpam_mon_sel_lock() helpers.
> -	 * Accesses to mon_sel need to be able to fail if they occur in the wrong
> -	 * context.
> +	 * Both the 'inner' and 'outer' must be taken.
> +	 * For real MMIO MSC, the outer lock is unnecessary - but keeps the
> +	 * code common with:
> +	 * Firmware backed MSC need to sleep when accessing the MSC, which
> +	 * means some code-paths will always fail. For these MSC the outer
> +	 * lock is providing the protection, and the inner lock fails to
> +	 * be taken if the task is unable to sleep.
> +	 *
>  	 * If needed, take msc->probe_lock first.
>  	 */
> -	raw_spinlock_t		_mon_sel_lock;
> -	unsigned long		_mon_sel_flags;
> +	struct mutex		outer_mon_sel_lock;
> +	bool			outer_lock_held;
> +	raw_spinlock_t		inner_mon_sel_lock;
> +	unsigned long		inner_mon_sel_flags;
>  
>  	void __iomem		*mapped_hwpage;
>  	size_t			mapped_hwpage_sz;
> @@ -137,29 +141,56 @@ struct mpam_msc {
>  };
>  
>  /* Returning false here means accesses to mon_sel must fail and report an error. */
> -static inline bool __must_check mpam_mon_sel_lock(struct mpam_msc *msc)
> +static inline bool __must_check mpam_mon_sel_inner_lock(struct mpam_msc *msc)
>  {
> -	/* Locking will require updating to support a firmware backed interface */
> -	if (WARN_ON_ONCE(msc->iface != MPAM_IFACE_MMIO))
> -		return false;
> +	/*
> +	 * The outer lock may be taken by a CPU that then issues an IPI to run
> +	 * a helper that takes the inner lock. lockdep can't help us here.
> +	 */
> +	WARN_ON_ONCE(!READ_ONCE(msc->outer_lock_held));
> +
> +	if (msc->iface == MPAM_IFACE_MMIO) {
> +		raw_spin_lock_irqsave(&msc->inner_mon_sel_lock, msc->inner_mon_sel_flags);
> +		return true;
> +	}
> +
> +	/* Accesses must fail if we are not pre-emptible */
> +	return !!preemptible();
> +}
>  
> -	raw_spin_lock_irqsave(&msc->_mon_sel_lock, msc->_mon_sel_flags);
> -	return true;
> +static inline void mpam_mon_sel_inner_unlock(struct mpam_msc *msc)
> +{
> +	WARN_ON_ONCE(!READ_ONCE(msc->outer_lock_held));
> +
> +	if (msc->iface == MPAM_IFACE_MMIO)
> +		raw_spin_unlock_irqrestore(&msc->inner_mon_sel_lock, msc->inner_mon_sel_flags);
> +}
> +
> +static inline void mpam_mon_sel_outer_lock(struct mpam_msc *msc)
> +{
> +	mutex_lock(&msc->outer_mon_sel_lock);
> +	msc->outer_lock_held = true;
>  }
>  
> -static inline void mpam_mon_sel_unlock(struct mpam_msc *msc)
> +static inline void mpam_mon_sel_outer_unlock(struct mpam_msc *msc)
>  {
> -	raw_spin_unlock_irqrestore(&msc->_mon_sel_lock, msc->_mon_sel_flags);
> +	msc->outer_lock_held = false;
> +	mutex_unlock(&msc->outer_mon_sel_lock);
>  }
>  
>  static inline void mpam_mon_sel_lock_held(struct mpam_msc *msc)
>  {
> -	lockdep_assert_held_once(&msc->_mon_sel_lock);
> +	WARN_ON_ONCE(!READ_ONCE(msc->outer_lock_held));
> +	if (msc->iface == MPAM_IFACE_MMIO)
> +		lockdep_assert_held_once(&msc->inner_mon_sel_lock);
> +	else
> +		lockdep_assert_preemption_enabled();
>  }
>  
>  static inline void mpam_mon_sel_lock_init(struct mpam_msc *msc)
>  {
> -	raw_spin_lock_init(&msc->_mon_sel_lock);
> +	raw_spin_lock_init(&msc->inner_mon_sel_lock);
> +	mutex_init(&msc->outer_mon_sel_lock);
>  }
>  
>  /* Bits for mpam features bitmaps */

Re: [PATCH 3/5] arm_mpam: add MPAM-Fb MSC firmware access support

[Ben Horgan]

Hi Andre,

On 4/29/26 15:13, Andre Przywara wrote:
> The Arm MPAM Firmware-backed (Fb) Profile document[1] describes an
> alternative way of accessing the "Memory System Components" (MSC) in an
> MPAM enabled system.
> Normally the MSCs are MMIO mapped, but in some implementations this
> might not be possible (MSC located outside of the local socket, MSC
> mapped secure-only) or desirable (direct MMIO access too slow or needs
> to be mediated through a control processor). MPAM-fb standardises a
> protocol to abstract MSC accesses, building on the SCMI protocol.
> 
> Add functions that do an MSC read or write access by redirecting the
> request through a firmware interface. For now this done via an ACPI
> PCC shared memory and mailbox combination.
> 
> Since the protocol used is only a small subset of the full SCMI spec,
> and the SCMI protocol has no full ACPI support anyway, open-code the
> SCMI message generation and handshake, for just the fields we need.
> 
> [1] https://developer.arm.com/documentation/den0144/latest
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/resctrl/Makefile        |   2 +-
>  drivers/resctrl/mpam_devices.c  |  16 +++-
>  drivers/resctrl/mpam_fb.c       | 158 ++++++++++++++++++++++++++++++++
>  drivers/resctrl/mpam_fb.h       |  17 ++++
>  drivers/resctrl/mpam_internal.h |   5 +
>  include/linux/arm_mpam.h        |   2 +-
>  6 files changed, 196 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/resctrl/mpam_fb.c
>  create mode 100644 drivers/resctrl/mpam_fb.h
> 
> diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile
> index 4f6d0e81f9b8..097c036724e9 100644
> --- a/drivers/resctrl/Makefile
> +++ b/drivers/resctrl/Makefile
> @@ -1,5 +1,5 @@
>  obj-$(CONFIG_ARM64_MPAM_DRIVER)			+= mpam.o
> -mpam-y						+= mpam_devices.o
> +mpam-y						+= mpam_devices.o mpam_fb.o
>  mpam-$(CONFIG_ARM64_MPAM_RESCTRL_FS)		+= mpam_resctrl.o
>  
>  ccflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG)	+= -DDEBUG
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 3be09b0c46ae..90b49bfd5caa 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -28,6 +28,7 @@
>  #include <linux/workqueue.h>
>  
>  #include "mpam_internal.h"
> +#include "mpam_fb.h"
>  
>  /* Values for the T241 errata workaround */
>  #define T241_CHIPS_MAX			4
> @@ -175,6 +176,13 @@ static u32 __mpam_read_reg(struct mpam_msc *msc, u16 reg)
>  {
>  	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
>  
> +	if (msc->iface == MPAM_IFACE_PCC) {
> +		u32 ret;
> +
> +		mpam_fb_send_read_request(msc, reg, &ret);
> +		return ret;

On failure, aren't we returning ret while it's still uninitialized?

In general, we need some strategy for handling errors. Are errors rare and serious enough
that we can just disable mpam on error, or can we, while keeping any state correct, propagate
the error up to the user interface?

Thanks,

Ben

> +	}
> +
>  	return readl_relaxed(msc->mapped_hwpage + reg);
>  }
>  
> @@ -188,10 +196,14 @@ static inline u32 _mpam_read_partsel_reg(struct mpam_msc *msc, u16 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);
> +	if (msc->iface == MPAM_IFACE_PCC) {
> +		mpam_fb_send_write_request(msc, reg, val);
> +	} else {
> +		WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
> +		writel_relaxed(val, msc->mapped_hwpage + reg);
> +	}
>  }
>  
>  static inline void _mpam_write_partsel_reg(struct mpam_msc *msc, u16 reg, u32 val)
> diff --git a/drivers/resctrl/mpam_fb.c b/drivers/resctrl/mpam_fb.c
> new file mode 100644
> index 000000000000..bfb5798c74b0
> --- /dev/null
> +++ b/drivers/resctrl/mpam_fb.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2024 Arm Ltd.
> +
> +#include <linux/arm_mpam.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/gfp.h>
> +#include <linux/iopoll.h>
> +#include <linux/list.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/processor.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <acpi/pcc.h>
> +
> +#include <asm/mpam.h>
> +
> +#include "mpam_fb.h"
> +
> +#define MPAM_FB_PROTOCOL_ID	0x1a
> +#define MPAM_MSC_ATTRIBUTES_CMD	0x3
> +#define MPAM_MSC_READ_CMD	0x4
> +#define MPAM_MSC_WRITE_CMD	0x5
> +
> +#define MPAM_MSC_PROT_ID_MASK	GENMASK(17, 10)
> +#define MPAM_MSC_TOKEN_MASK	GENMASK(27, 18)
> +
> +#define SCMI_CHAN_RSVD_OFS	0x00
> +#define SCMI_CHAN_STATUS_OFS	0x04
> +#define SCMI_CHAN_STATUS_FREE_BIT	BIT(0)
> +#define SCMI_CHAN_FLAGS_OFS	0x10
> +#define SCMI_CHAN_FLAGS_IRQ		BIT(0)
> +#define SCMI_MSG_LENGTH_OFS	0x14
> +#define SCMI_MSG_HEADER_OFS	0x18
> +#define SCMI_MSG_PAYLOAD_OFS	0x1c
> +
> +#define MPAM_READ_MSG_SIZE	(SCMI_MSG_PAYLOAD_OFS + 3 * sizeof(u32))
> +#define MPAM_WRITE_MSG_SIZE	(SCMI_MSG_PAYLOAD_OFS + 4 * sizeof(u32))
> +
> +static atomic_t mpam_fb_token = ATOMIC_INIT(0);
> +
> +static int mpam_fb_build_read_message(int msc_id, int reg, unsigned int token,
> +				      void __iomem *msg_buf)
> +{
> +	writel_relaxed(SCMI_CHAN_FLAGS_IRQ, msg_buf + SCMI_CHAN_FLAGS_OFS);
> +	writel_relaxed(MPAM_READ_MSG_SIZE, msg_buf + SCMI_MSG_LENGTH_OFS);
> +	writel_relaxed(MPAM_MSC_READ_CMD |
> +		       FIELD_PREP(MPAM_MSC_TOKEN_MASK, token) |
> +		       FIELD_PREP(MPAM_MSC_PROT_ID_MASK, MPAM_FB_PROTOCOL_ID),
> +		       msg_buf + SCMI_MSG_HEADER_OFS);
> +
> +	writel_relaxed(cpu_to_le32(msc_id), msg_buf + SCMI_MSG_PAYLOAD_OFS);
> +	writel_relaxed(0, msg_buf + SCMI_MSG_PAYLOAD_OFS + 0x4);
> +	writel_relaxed(cpu_to_le32(reg), msg_buf + SCMI_MSG_PAYLOAD_OFS + 0x8);
> +
> +	return MPAM_READ_MSG_SIZE;
> +}
> +
> +static int mpam_fb_build_write_message(int msc_id, int reg, u32 val,
> +				       unsigned int token,
> +				       void __iomem *msg_buf)
> +{
> +	writel_relaxed(MPAM_WRITE_MSG_SIZE, msg_buf + SCMI_MSG_LENGTH_OFS);
> +	writel_relaxed(MPAM_MSC_WRITE_CMD |
> +		       FIELD_PREP(MPAM_MSC_TOKEN_MASK, token) |
> +		       FIELD_PREP(MPAM_MSC_PROT_ID_MASK, MPAM_FB_PROTOCOL_ID),
> +		       msg_buf + SCMI_MSG_HEADER_OFS);
> +
> +	writel_relaxed(cpu_to_le32(msc_id), msg_buf + SCMI_MSG_PAYLOAD_OFS);
> +	writel_relaxed(0, msg_buf + SCMI_MSG_PAYLOAD_OFS + 0x4);
> +	writel_relaxed(cpu_to_le32(reg), msg_buf + SCMI_MSG_PAYLOAD_OFS + 0x8);
> +	writel_relaxed(cpu_to_le32(val), msg_buf + SCMI_MSG_PAYLOAD_OFS + 0xc);
> +
> +	return MPAM_WRITE_MSG_SIZE;
> +}
> +
> +#define SCMI_CHANNEL_FREE	true
> +#define SCMI_CHANNEL_BUSY	false
> +static int mpam_fb_wait_for_channel(struct pcc_mbox_chan *chan,
> +				    bool free)
> +{
> +	u32 status = free ? SCMI_CHAN_STATUS_FREE_BIT : 0;
> +	u32 val;
> +
> +	/*
> +	 * The channel should really be free always at this point, as we take
> +	 * a lock for every read or write request. Check the free bit anyway,
> +	 * for good measure and to catch corner cases.
> +	 */
> +	return readl_poll_timeout(chan->shmem + SCMI_CHAN_STATUS_OFS, val,
> +				  (val & SCMI_CHAN_STATUS_FREE_BIT) == status,
> +				  1, 10000);
> +}
> +
> +static int mpam_fb_send_request(struct mpam_msc *msc, u16 reg, u32 *result,
> +				bool is_write)
> +{
> +	unsigned int token = atomic_inc_return(&mpam_fb_token);
> +	struct pcc_mbox_chan *chan = msc->pcc_chan;
> +	u32 status;
> +	int ret;
> +
> +	guard(mutex)(&msc->pcc_chan_lock);
> +	ret = mpam_fb_wait_for_channel(chan, SCMI_CHANNEL_FREE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Clear error bit and mark the channel as belonging to the callee */
> +	writel(0, chan->shmem + SCMI_CHAN_STATUS_OFS);
> +
> +	if (is_write)
> +		ret = mpam_fb_build_write_message(msc->mpam_fb_msc_id, reg,
> +						  *result, token, chan->shmem);
> +	else
> +		ret = mpam_fb_build_read_message(msc->mpam_fb_msc_id, reg,
> +						 token, chan->shmem);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mbox_send_message(chan->mchan, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mpam_fb_wait_for_channel(chan, SCMI_CHANNEL_FREE);
> +	if (ret)
> +		return ret;
> +
> +	status = readl(chan->shmem + SCMI_MSG_HEADER_OFS);
> +	if (FIELD_GET(MPAM_MSC_TOKEN_MASK, status) != token)
> +		return -ETIMEDOUT;
> +
> +	ret = readl(chan->shmem + SCMI_MSG_PAYLOAD_OFS + 0x0);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!is_write)
> +		*result = readl(chan->shmem + SCMI_MSG_PAYLOAD_OFS + 0x4);
> +
> +	return 0;
> +}
> +
> +int mpam_fb_send_read_request(struct mpam_msc *msc, u16 reg, u32 *result)
> +{
> +	return mpam_fb_send_request(msc, reg, result, false);
> +}
> +
> +int mpam_fb_send_write_request(struct mpam_msc *msc, u16 reg, u32 value)
> +{
> +	return mpam_fb_send_request(msc, reg, &value, true);
> +}
> diff --git a/drivers/resctrl/mpam_fb.h b/drivers/resctrl/mpam_fb.h
> new file mode 100644
> index 000000000000..c3d1a239e16f
> --- /dev/null
> +++ b/drivers/resctrl/mpam_fb.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +// Copyright (C) 2024-2025 Arm Ltd.
> +
> +#ifndef MPAM_FB_H_
> +#define MPAM_FB_H_
> +
> +#include <linux/types.h>
> +#include "mpam_internal.h"
> +
> +#define SCMI_MSG_PAYLOAD_OFS	0x1c
> +#define MPAM_WRITE_MSG_SIZE	(SCMI_MSG_PAYLOAD_OFS + 4 * sizeof(u32))
> +#define MPAM_FB_MAX_MSG_SIZE	MPAM_WRITE_MSG_SIZE
> +
> +int mpam_fb_send_read_request(struct mpam_msc *msc, u16 reg, u32 *result);
> +int mpam_fb_send_write_request(struct mpam_msc *msc, u16 reg, u32 value);
> +
> +#endif
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index 80213af10a64..3f524cd4fc81 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -11,6 +11,7 @@
>  #include <linux/io.h>
>  #include <linux/jump_label.h>
>  #include <linux/llist.h>
> +#include <linux/mailbox_client.h>
>  #include <linux/mutex.h>
>  #include <linux/resctrl.h>
>  #include <linux/spinlock.h>
> @@ -66,6 +67,10 @@ struct mpam_msc {
>  
>  	/* Not modified after mpam_is_enabled() becomes true */
>  	enum mpam_msc_iface	iface;
> +	struct mbox_client	pcc_cl;
> +	struct pcc_mbox_chan	*pcc_chan;
> +	struct mutex		pcc_chan_lock; /* only one message at a time */
> +	int			mpam_fb_msc_id;	/* in its own name space */
>  	u32			nrdy_usec;
>  	cpumask_t		accessibility;
>  	bool			has_extd_esr;
> diff --git a/include/linux/arm_mpam.h b/include/linux/arm_mpam.h
> index f92a36187a52..002f56e15362 100644
> --- a/include/linux/arm_mpam.h
> +++ b/include/linux/arm_mpam.h
> @@ -12,7 +12,7 @@ struct mpam_msc;
>  
>  enum mpam_msc_iface {
>  	MPAM_IFACE_MMIO,	/* a real MPAM MSC */
> -	MPAM_IFACE_PCC,		/* a fake MPAM MSC */
> +	MPAM_IFACE_PCC,		/* using the MPAM-Fb firmware redirection */
>  };
>  
>  enum mpam_class_types {

Re: [PATCH 5/5] arm_mpam: detect and enable MPAM-Fb PCC support

[Ben Horgan]

Hi Andre,

On 4/29/26 15:13, Andre Przywara wrote:
> The Arm MPAM-Fb specification [1] describes a protocol to access MSC
> registers through a firmware interface. This requires a shared memory
> region to hold the message, and a mailbox to trigger the access.
> For ACPI this is wrapped as a PCC channel, described using existing
> ACPI abstractions.
> 
> Add code to parse those PCC table descriptions associated with an MSC,
> and store the parsed information in the MSC struct.
> This will be used by the MPAM-Fb access wrapper code.
> 
> [1] https://developer.arm.com/documentation/den0144/latest
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/acpi/arm64/mpam.c      |  2 ++
>  drivers/resctrl/mpam_devices.c | 46 +++++++++++++++++++++++++++++++---
>  2 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/mpam.c b/drivers/acpi/arm64/mpam.c
> index 99c2bdbb3314..edb4d10e8dc3 100644
> --- a/drivers/acpi/arm64/mpam.c
> +++ b/drivers/acpi/arm64/mpam.c
> @@ -341,6 +341,8 @@ static struct platform_device * __init acpi_mpam_parse_msc(struct acpi_mpam_msc_
>  	} else if (iface == MPAM_IFACE_PCC) {
>  		props[next_prop++] = PROPERTY_ENTRY_U32("pcc-channel",
>  							tbl_msc->base_address);
> +		props[next_prop++] = PROPERTY_ENTRY_U32("msc-id",
> +							tbl_msc->identifier);
>  	}
>  
>  	acpi_mpam_parse_irqs(pdev, tbl_msc, res, &next_res);
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 62aa04cb6905..6f0d0959d3a4 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -19,6 +19,7 @@
>  #include <linux/irqdesc.h>
>  #include <linux/list.h>
>  #include <linux/lockdep.h>
> +#include <linux/mailbox_client.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/printk.h>
> @@ -27,6 +28,9 @@
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>  
> +#include <acpi/pcc.h>
> +#include <acpi/acpi_io.h>
> +
>  #include "mpam_internal.h"
>  #include "mpam_fb.h"
>  
> @@ -1042,7 +1046,8 @@ static u64 mpam_msc_read_mbwu_l(struct mpam_msc *msc)
>  
>  	mpam_mon_sel_lock_held(msc);
>  
> -	WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);
> +	if (msc->iface == MPAM_IFACE_MMIO)
> +		WARN_ON_ONCE((MSMON_MBWU_L + sizeof(u64)) > msc->mapped_hwpage_sz);

This should be in a different patch.

>  	WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(), &msc->accessibility));
>  
>  	mbwu_l_high2 = __mpam_read_reg(msc, MSMON_MBWU_L + 4);
> @@ -2042,10 +2047,15 @@ static void mpam_msc_drv_remove(struct platform_device *pdev)
>  	mpam_free_garbage();
>  }
>  
> +static void mpam_pcc_rx_callback(struct mbox_client *cl, void *msg)
> +{
> +	/* TODO: wake up tasks blocked on this MSC's PCC channel */
> +}
> +
>  static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>  {
>  	int err;
> -	u32 tmp;
> +	u32 pcc_subspace_id;
>  	struct mpam_msc *msc;
>  	struct resource *msc_res;
>  	struct device *dev = &pdev->dev;
> @@ -2090,7 +2100,8 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	if (device_property_read_u32(&pdev->dev, "pcc-channel", &tmp))
> +	if (device_property_read_u32(&pdev->dev, "pcc-channel",
> +				     &pcc_subspace_id))
>  		msc->iface = MPAM_IFACE_MMIO;
>  	else
>  		msc->iface = MPAM_IFACE_PCC;
> @@ -2106,6 +2117,35 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>  		}
>  		msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
>  		msc->mapped_hwpage = io;
> +	} else if (msc->iface == MPAM_IFACE_PCC) {
> +		u32 msc_id;
> +
> +		msc->pcc_cl.dev = &pdev->dev;
> +		msc->pcc_cl.rx_callback = mpam_pcc_rx_callback;
> +		msc->pcc_cl.tx_block = false;

How do we make sure that, for instance, a MON_SEL write has completed before we start reading
the associated counters? Is there an ordering guarantee?

Thanks,

Ben

> +		msc->pcc_cl.tx_tout = 1000; /* 1s */
> +		msc->pcc_cl.knows_txdone = false;
> +
> +		if (device_property_read_u32(&pdev->dev, "msc-id", &msc_id)) {
> +			pr_err("missing MPAM-Fb MSC identifier\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +		msc->mpam_fb_msc_id = msc_id;
> +
> +		msc->pcc_chan = pcc_mbox_request_channel(&msc->pcc_cl,
> +							 pcc_subspace_id);
> +		if (IS_ERR(msc->pcc_chan)) {
> +			pr_err("Failed to request MSC PCC channel\n");
> +			return (void *)msc->pcc_chan;
> +		}
> +
> +		if (msc->pcc_chan->shmem_size < MPAM_FB_MAX_MSG_SIZE) {
> +			pr_err("MPAM-Fb PCC channel size too small.\n");
> +			pcc_mbox_free_channel(msc->pcc_chan);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +
> +		mutex_init(&msc->pcc_chan_lock);
>  	} else {
>  		return ERR_PTR(-EINVAL);
>  	}