From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 1E99E2877C2; Fri, 17 Oct 2025 18:53:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1760727232; cv=none; b=HSRJDYOFbP2/dWBxe64elwsLXOPS+Q7jnjlK2/Go+TtpcrmPSQYgRJqtizy9LDc1gEvXf7zG5HN7EZtEPHMQiRKEdkPhCFFKf04NePkBHscrS3zdIbDFSfyIsXFQ5nGJ7rZeS/MJucFF6a2mbUKYXjaLUIUlRKpAvizCaWOctZw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1760727232; c=relaxed/simple; bh=PKXdSSCA3h6c9divuIMc0sHjO0iXWA3XidbmTDbmvqk=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=QOhTolRM6mpSbgCCgF32BSOR/ZnvFUTPmIabYzht9IOS0vCPtQQ91xTi+TX0ru/m0C0/dJBPZ6dzsDIynShj48LRnPtenQ0PEjceNG8efCW7FbZtjOqEO1nMLXH5YH017aBrRmskeTmx/iMvl1YnYl8USYS/iN7DMjyxfC/Mk0g= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 99EF51515; Fri, 17 Oct 2025 11:53:40 -0700 (PDT) Received: from [10.1.197.69] (eglon.cambridge.arm.com [10.1.197.69]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 709343F66E; Fri, 17 Oct 2025 11:53:43 -0700 (PDT) Message-ID: Date: Fri, 17 Oct 2025 19:53:41 +0100 Precedence: bulk X-Mailing-List: linux-acpi@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 09/29] arm_mpam: Add MPAM MSC register layout definitions To: Jonathan Cameron Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, D Scott Phillips OS , carl@os.amperecomputing.com, lcherian@marvell.com, bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com, baolin.wang@linux.alibaba.com, Jamie Iles , Xin Hao , peternewman@google.com, dfustini@baylibre.com, amitsinght@marvell.com, David Hildenbrand , Dave Martin , Koba Ko , Shanker Donthineni , fenghuay@nvidia.com, baisheng.gao@unisoc.com, Rob Herring , Rohit Mathew , Rafael Wysocki , Len Brown , Lorenzo Pieralisi , Hanjun Guo , Sudeep Holla , Catalin Marinas , Will Deacon , Greg Kroah-Hartman , Danilo Krummrich , Ben Horgan References: <20250910204309.20751-1-james.morse@arm.com> <20250910204309.20751-10-james.morse@arm.com> <20250911160031.000026c7@huawei.com> Content-Language: en-GB From: James Morse In-Reply-To: <20250911160031.000026c7@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Jonathan, On 11/09/2025 16:00, Jonathan Cameron wrote: > On Wed, 10 Sep 2025 20:42:49 +0000 > James Morse 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