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

James Morse
2025-10-17T18:53:48Z

Hi Jonathan,

On 11/09/2025 16:00, Jonathan Cameron wrote:
> 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).

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

Things were added once I'd already written this, and I only updated it with 'new' features
where they were actually useful for feature parity with resctrl/Intel-RDT.

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

I've not done this because its a time sink for no benefit. The kernel doesn't use any of
the 'missing' features. While I agree it would be nice if the list were up to date - it
will become stale pretty quickly, so its not an achievable goal...

>
>>
>> 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.

>> 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.

Heh, linking to the 'latest' means those will change...

>
>> + */
>> +#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?

No - I must have been going mad!

>> +#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.

... I reckon the ones in funny places were the ones that the original FVP supported
i.e. only the mandatory ones, which wasn't particularly useful.

>> +/* 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?

Sure,

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

I think I've got that more consistent now...

>> +/* 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..

Yup, it the frequent game of spot-the-difference.
I've updated that as part of your other feedback.

>> +
>> +/*
>> + * 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

It's only defined for MSMON: "Value scaling enable",. I'll move it after the
MSMON_CFG_MBWU_CTL_TYPE_MBWU define below.

>> +#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.

When I started these were the same!

It is dealt with in common code, I don't think any of the bits that are different are
used by the driver.

Thanks,

James