Message-ID: <ad7c715d-227b-4f49-8f8b-fe4763c1cef6@arm.com>
Date: Fri, 17 Oct 2025 19:53:41 +0100
From: James Morse <james.morse@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-acpi@...r.kernel.org,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Dave Martin <dave.martin@....com>,
 Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 fenghuay@...dia.com, baisheng.gao@...soc.com, Rob Herring <robh@...nel.org>,
 Rohit Mathew <rohit.mathew@....com>, Rafael Wysocki <rafael@...nel.org>,
 Len Brown <lenb@...nel.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Hanjun Guo <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>, Ben Horgan <ben.horgan@....com>
Subject: Re: [PATCH v2 09/29] arm_mpam: Add MPAM MSC register layout
 definitions

Hi Jonathan,

On 11/09/2025 16:00, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 20:42:49 +0000
> James Morse <james.morse@....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

Powered by blists - more mailing lists