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

[James Morse <james.morse@arm.com>]

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