Re: [PATCH v2 09/29] arm_mpam: Add MPAM MSC register layout definitions

Jonathan Cameron
2025-09-11T15:00:36Z

On Wed, 10 Sep 2025 20:42:49 +0000
James Morse <james.morse@arm.com> wrote:

> Memory Partitioning and Monitoring (MPAM) has memory mapped devices
> (MSCs) with an identity/configuration page.
>
> Add the definitions for these registers as offset within the page(s).
Hi James,

I'm not sure why some things ended up in this patch and others didn't.
MPAMCFG_EN for example isn't here.

If doing a separate 'register defines' patch I'd do the lot as of
the current spec.

>
> Link: https://developer.arm.com/documentation/ihi0099/latest/

Maybe link a specific version? I'm not sure if I'm looking at is the same one
as you were when you wrote this. That will become worse over time. I'm definitely
seeing extra bits in a number of registers.

I'm lazy enough not to go see if the cover letter calls out a version.

Anyhow, various small things on ordering that would have made this easier to review
against the spec.

Jonathan

> Signed-off-by: James Morse <james.morse@arm.com>
> Reviewed-by: Ben Horgan <ben.horgan@arm.com>
> ---
> Changes since v1:
> * Whitespace.
> * Added constants for CASSOC and XCL.
> * Merged FLT/CTL defines.
> * Fixed MSMON_CFG_CSU_CTL_TYPE_CSU definition.
>
> Changes since RFC:
> * Renamed MSMON_CFG_MBWU_CTL_TYPE_CSU as MSMON_CFG_CSU_CTL_TYPE_CSU
> * Whitepsace churn.
> * Cite a more recent document.
> * Removed some stale feature, fixed some names etc.
> ---
> drivers/resctrl/mpam_internal.h | 267 ++++++++++++++++++++++++++++++++
> 1 file changed, 267 insertions(+)
>
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index 02e9576ece6b..109f03df46c2 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -152,4 +152,271 @@ extern struct list_head mpam_classes;
> int mpam_get_cpumask_from_cache_id(unsigned long cache_id, u32 cache_level,
> cpumask_t *affinity);
>
> +/*
> + * MPAM MSCs have the following register layout. See:
> + * Arm Memory System Resource Partitioning and Monitoring (MPAM) System
> + * Component Specification.
> + * https://developer.arm.com/documentation/ihi0099/latest/

Maybe be friendly and give some section number references.

> + */
> +#define MPAM_ARCHITECTURE_V1 0x10
> +
> +/* Memory mapped control pages: */
> +/* ID Register offsets in the memory mapped page */
> +#define MPAMF_IDR 0x0000 /* features id register */
> +#define MPAMF_MSMON_IDR 0x0080 /* performance monitoring features */

Any reason this one is out of order with respect to the addresses?

> +#define MPAMF_IMPL_IDR 0x0028 /* imp-def partitioning */
> +#define MPAMF_CPOR_IDR 0x0030 /* cache-portion partitioning */
> +#define MPAMF_CCAP_IDR 0x0038 /* cache-capacity partitioning */
> +#define MPAMF_MBW_IDR 0x0040 /* mem-bw partitioning */
> +#define MPAMF_PRI_IDR 0x0048 /* priority partitioning */
> +#define MPAMF_CSUMON_IDR 0x0088 /* cache-usage monitor */
> +#define MPAMF_MBWUMON_IDR 0x0090 /* mem-bw usage monitor */
> +#define MPAMF_PARTID_NRW_IDR 0x0050 /* partid-narrowing */
> +#define MPAMF_IIDR 0x0018 /* implementer id register */
> +#define MPAMF_AIDR 0x0020 /* architectural id register */

These 3 as well. I'm not sure what the ordering is conveying but probably easier to just
to put them in address order.

There are some other cases of this below.

> +/* MPAMF_IIDR - MPAM implementation ID register */
> +#define MPAMF_IIDR_PRODUCTID GENMASK(31, 20)
> +#define MPAMF_IIDR_PRODUCTID_SHIFT 20
> +#define MPAMF_IIDR_VARIANT GENMASK(19, 16)
> +#define MPAMF_IIDR_VARIANT_SHIFT 16
> +#define MPAMF_IIDR_REVISON GENMASK(15, 12)
> +#define MPAMF_IIDR_REVISON_SHIFT 12
> +#define MPAMF_IIDR_IMPLEMENTER GENMASK(11, 0)
> +#define MPAMF_IIDR_IMPLEMENTER_SHIFT 0
I'd expect to see FIELD_GET/ PREP rather than use of shifts. Can we drop the defines?

Pick an order for reg field definitions. Until here they've been low to high.

> +/* Error conditions in accessing memory mapped registers */
> +#define MPAM_ERRCODE_NONE 0
> +#define MPAM_ERRCODE_PARTID_SEL_RANGE 1
> +#define MPAM_ERRCODE_REQ_PARTID_RANGE 2
> +#define MPAM_ERRCODE_MSMONCFG_ID_RANGE 3
> +#define MPAM_ERRCODE_REQ_PMG_RANGE 4
> +#define MPAM_ERRCODE_MONITOR_RANGE 5
> +#define MPAM_ERRCODE_INTPARTID_RANGE 6
> +#define MPAM_ERRCODE_UNEXPECTED_INTERNAL 7

Seems there are more in latest spec..
> +
> +/*
> + * MSMON_CFG_CSU_CTL - Memory system performance monitor configure cache storage
> + * usage monitor control register
> + * MSMON_CFG_MBWU_CTL - Memory system performance monitor configure memory
> + * bandwidth usage monitor control register
> + */
> +#define MSMON_CFG_x_CTL_TYPE GENMASK(7, 0)
> +#define MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L BIT(15)
> +#define MSMON_CFG_x_CTL_MATCH_PARTID BIT(16)
> +#define MSMON_CFG_x_CTL_MATCH_PMG BIT(17)
> +#define MSMON_CFG_x_CTL_SCLEN BIT(19)
On the spec I'm looking at this is reserved in CSU_CTL

> +#define MSMON_CFG_x_CTL_SUBTYPE GENMASK(22, 20)
> +#define MSMON_CFG_x_CTL_OFLOW_FRZ BIT(24)
> +#define MSMON_CFG_x_CTL_OFLOW_INTR BIT(25)
> +#define MSMON_CFG_x_CTL_OFLOW_STATUS BIT(26)
> +#define MSMON_CFG_x_CTL_CAPT_RESET BIT(27)
> +#define MSMON_CFG_x_CTL_CAPT_EVNT GENMASK(30, 28)
> +#define MSMON_CFG_x_CTL_EN BIT(31)

I guess this combining of definitions will show some advante in common code
later but right now it seems confusing given not all bits are present in both.

> +
> +#define MSMON_CFG_MBWU_CTL_TYPE_MBWU 0x42
> +#define MSMON_CFG_CSU_CTL_TYPE_CSU 0x43
> +
> +/*
> + * MSMON_CFG_CSU_FLT - Memory system performance monitor configure cache storage
> + * usage monitor filter register
> + * MSMON_CFG_MBWU_FLT - Memory system performance monitor configure memory
> + * bandwidth usage monitor filter register
> + */
> +#define MSMON_CFG_x_FLT_PARTID GENMASK(15, 0)
> +#define MSMON_CFG_x_FLT_PMG GENMASK(23, 16)
> +
> +#define MSMON_CFG_MBWU_FLT_RWBW GENMASK(31, 30)
> +#define MSMON_CFG_CSU_FLT_XCL BIT(31)
> +
> +/*
> + * MSMON_CSU - Memory system performance monitor cache storage usage monitor
> + * register
> + * MSMON_CSU_CAPTURE - Memory system performance monitor cache storage usage
> + * capture register
> + * MSMON_MBWU - Memory system performance monitor memory bandwidth usage
> + * monitor register
> + * MSMON_MBWU_CAPTURE - Memory system performance monitor memory bandwidth usage
> + * capture register
> + */
> +#define MSMON___VALUE GENMASK(30, 0)
> +#define MSMON___NRDY BIT(31)
> +#define MSMON___NRDY_L BIT(63)
> +#define MSMON___L_VALUE GENMASK(43, 0)
> +#define MSMON___LWD_VALUE GENMASK(62, 0)
> +
> +/*
> + * MSMON_CAPT_EVNT - Memory system performance monitoring capture event
> + * generation register
> + */
> +#define MSMON_CAPT_EVNT_NOW BIT(0)
> +
> #endif /* MPAM_INTERNAL_H */