From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3B3C1CA1006 for ; Mon, 1 Sep 2025 13:35:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=+34R6gR8daL+Q/TpM6El6UP8HBV8HIfE+YzE1CEFDLs=; b=l7WJLkq219TOUg6kLQ361yCFpF 2mu1ZGL4JmDu/Ziw/5xIrXPxXnB0JjIsmBtwl08/UrmA8dj7VzAZDNFAn6ZLYLHcc9Hsz/5SlsBx3 6iEn24uwlk3lyzV8vnqUTKRRZzgwfA3kY8U4GjfGyvHMbhtgcrPS26bdQEdaRLScOvKwaIdcpEGNZ nwTBi5GUKK97v5jSswzweuEB7xtW8EYHX10OnQvVRq/WdL+RiF5LMVAt37h/hlBHvqIRoIoYXYjvN qhALn/r9igGc6WP1lEWlU3qWBUa29vSmz6wdwe37w+ZwuChgmu3mZEmtubBGxmVKzQ5Lsg0x7/nfJ JxQJqmHg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1ut4h1-0000000ChT7-2lRV; Mon, 01 Sep 2025 13:35:31 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1ut2QN-0000000C0pr-3B9V for linux-arm-kernel@lists.infradead.org; Mon, 01 Sep 2025 11:10:13 +0000 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 DAC511A00; Mon, 1 Sep 2025 04:10:00 -0700 (PDT) Received: from e133380.arm.com (e133380.arm.com [10.1.197.68]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5B7673F6A8; Mon, 1 Sep 2025 04:10:03 -0700 (PDT) Date: Mon, 1 Sep 2025 12:09:47 +0100 From: Dave Martin To: James Morse Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, devicetree@vger.kernel.org, shameerali.kolothum.thodi@huawei.com, 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 , Rex Nie , Koba Ko , Shanker Donthineni , fenghuay@nvidia.com, baisheng.gao@unisoc.com, Jonathan Cameron , Rob Herring , Rohit Mathew , Rafael Wysocki , Len Brown , Lorenzo Pieralisi , Hanjun Guo , Sudeep Holla , Krzysztof Kozlowski , Conor Dooley , Catalin Marinas , Will Deacon , Greg Kroah-Hartman , Danilo Krummrich , Ben Horgan Subject: Re: [PATCH 12/33] arm_mpam: Add the class and component structures for ris firmware described Message-ID: References: <20250822153048.2287-1-james.morse@arm.com> <20250822153048.2287-13-james.morse@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250822153048.2287-13-james.morse@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250901_041011_878201_7BA7FF36 X-CRM114-Status: GOOD ( 58.77 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, > Subject: arm_mpam: Add the class and component structures for ris firmware described Mangled subject line? There is a fair intersection between the commit message and what the patch does, but they don't quite seem to match up. Some key issues like locking / object lifecycle management and DT parsing (a bit of which, it appears, lives here too) are not mentioned at all. In lieu of a complete rewrite, it might be best to discard the explanation of the various object types. The comment in the code speaks for itself, and looks clearer. [...] On Fri, Aug 22, 2025 at 03:29:53PM +0000, James Morse wrote: > An MSC is a container of resources, each identified by their RIS index. > Some RIS are described by firmware to provide their position in the system. > Others are discovered when the driver probes the hardware. > > To configure a resource it needs to be found by its class, e.g. 'L2'. > There are two kinds of grouping, a class is a set of components, which > are visible to user-space as there are likely to be multiple instances > of the L2 cache. (e.g. one per cluster or package) > > struct mpam_components are a set of struct mpam_vmsc. A vMSC groups the > RIS in an MSC that control the same logical piece of hardware. (e.g. L2). > This is to allow hardware implementations where two controls are presented > as different RIS. Re-combining these RIS allows their feature bits to > be or-ed. This structure is not visible outside mpam_devices.c > > struct mpam_vmsc are then a set of struct mpam_msc_ris, which are not > visible as each L2 cache may be composed of individual slices which need > to be configured the same as the hardware is not able to distribute the > configuration. > > Add support for creating and destroying these structures. > A gfp is passed as the structures may need creating when a new RIS entry > is discovered when probing the MSC. > > CC: Ben Horgan > Signed-off-by: James Morse > --- > Changes since RFC: > * removed a pr_err() debug message that crept in. > --- > drivers/resctrl/mpam_devices.c | 488 +++++++++++++++++++++++++++++++- > drivers/resctrl/mpam_internal.h | 91 ++++++ > include/linux/arm_mpam.h | 8 +- > 3 files changed, 574 insertions(+), 13 deletions(-) > > diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c > index 71a1fb1a9c75..5baf2a8786fb 100644 > --- a/drivers/resctrl/mpam_devices.c > +++ b/drivers/resctrl/mpam_devices.c > @@ -20,7 +20,6 @@ [...] > @@ -35,11 +34,483 @@ > static DEFINE_MUTEX(mpam_list_lock); > static LIST_HEAD(mpam_all_msc); > > -static struct srcu_struct mpam_srcu; > +struct srcu_struct mpam_srcu; Why expose this here? This patch makes no use of the exposed symbol. > > /* MPAM isn't available until all the MSC have been probed. */ > static u32 mpam_num_msc; > > +/* > + * An MSC is a physical container for controls and monitors, each identified by > + * their RIS index. These share a base-address, interrupts and some MMIO > + * registers. A vMSC is a virtual container for RIS in an MSC that control or > + * monitor the same thing. Members of a vMSC are all RIS in the same MSC, but > + * not all RIS in an MSC share a vMSC. > + * Components are a group of vMSC that control or monitor the same thing but > + * are from different MSC, so have different base-address, interrupts etc. > + * Classes are the set components of the same type. > + * > + * The features of a vMSC is the union of the RIS it contains. > + * The features of a Class and Component are the common subset of the vMSC > + * they contain. > + * > + * e.g. The system cache may have bandwidth controls on multiple interfaces, > + * for regulating traffic from devices independently of traffic from CPUs. > + * If these are two RIS in one MSC, they will be treated as controlling > + * different things, and will not share a vMSC/component/class. > + * > + * e.g. The L2 may have one MSC and two RIS, one for cache-controls another > + * for bandwidth. These two RIS are members of the same vMSC. > + * > + * e.g. The set of RIS that make up the L2 are grouped as a component. These > + * are sometimes termed slices. They should be configured the same, as if there > + * were only one. > + * > + * e.g. The SoC probably has more than one L2, each attached to a distinct set > + * of CPUs. All the L2 components are grouped as a class. > + * > + * When creating an MSC, struct mpam_msc is added to the all mpam_all_msc list, > + * then linked via struct mpam_ris to a vmsc, component and class. > + * The same MSC may exist under different class->component->vmsc paths, but the > + * RIS index will be unique. > + */ This description of the structures and how they relate to each other seems OK (bearing in mind that I am already familiar with this stuff -- I can't speak for other people). > +LIST_HEAD(mpam_classes); > + > +/* List of all objects that can be free()d after synchronise_srcu() */ > +static LLIST_HEAD(mpam_garbage); > + > +#define init_garbage(x) init_llist_node(&(x)->garbage.llist) [...] > +#define add_to_garbage(x) \ > +do { \ > + __typeof__(x) _x = x; \ Nit: = (x) (for the paranoid) > + (_x)->garbage.to_free = (_x); \ _x->garbage.to_free = _x; (_x is an identifier, not a macro argument. It can't get re-parsed as something else -- assuming that there is not a #define for _x, but then all bets would be off anyway.) > + llist_add(&(_x)->garbage.llist, &mpam_garbage); \ &_x->... [...] > +static void mpam_ris_destroy(struct mpam_msc_ris *ris) > +{ > + struct mpam_vmsc *vmsc = ris->vmsc; > + struct mpam_msc *msc = vmsc->msc; > + struct platform_device *pdev = msc->pdev; > + struct mpam_component *comp = vmsc->comp; > + struct mpam_class *class = comp->class; > + > + lockdep_assert_held(&mpam_list_lock); > + > + cpumask_andnot(&comp->affinity, &comp->affinity, &ris->affinity); > + cpumask_andnot(&class->affinity, &class->affinity, &ris->affinity); This is not the inverse of the cpumask_or()s in mpam_ris_create_locked(), unless the the ris associated with each class and each component have strictly disjoint affinity masks. Is that checked anywhere, or should it be impossible by construction? But, thinking about it: I wonder why we ever really need to do the teardown. If we get an error interrupt then we can just go into a sulk, spam dmesg a bit, put the hardware into the most vanilla state that we can, and refuse to manipulate it further. But this only happens in the case of a software or hardware *bug* (or, in a future world where we might implement virtualisation, an uncontainable MPAM error triggered by a guest -- for which tearing down the host MPAM would be an overreaction). Trying to cleanly tear the MPAM driver down after such an error seems a bit futile. The MPAM resctrl glue could eventually be made into a module (though not needed from day 1) -- which would allow for unloading resctrlfs if that is eventually module-ised. I think this wouldn't require the MPAM devices backend to be torn down at any point, though (?) If we can simplify or eliminate the teardown, does it simplify the locking at all? The garbage collection logic can also be dispensed with if there is never any garbage. Since MSCs etc. never disappear from the hardware, it feels like it ought not to be necessary ever to remove items from any of these lists except when trying to do a teardown (?) (Putting the hardware into a quiecent state is not the same thing as tearing down the data structures -- we do want to quiesce MPAM when shutting down the kernel, as least for the kexec scenario.) > + clear_bit(ris->ris_idx, msc->ris_idxs); > + list_del_rcu(&ris->vmsc_list); > + list_del_rcu(&ris->msc_list); > + add_to_garbage(ris); > + ris->garbage.pdev = pdev; > + > + if (list_empty(&vmsc->ris)) > + mpam_vmsc_destroy(vmsc); > +} [...] > +static int mpam_ris_create_locked(struct mpam_msc *msc, u8 ris_idx, > + enum mpam_class_types type, u8 class_id, > + int component_id, gfp_t gfp) > +{ > + int err; > + struct mpam_vmsc *vmsc; > + struct mpam_msc_ris *ris; > + struct mpam_class *class; > + struct mpam_component *comp; > + > + lockdep_assert_held(&mpam_list_lock); > + > + if (test_and_set_bit(ris_idx, msc->ris_idxs)) > + return -EBUSY; Is it impossible by construction to get in here with an out-of-range ris_idx? To avoid the callers (i.e., ACPI) needing to understand the internal limitations of this code, maybe it is worth having a check here (even if technically redundant). [...] Cheers ---Dave