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 8EBD4C83F17 for ; Wed, 23 Jul 2025 16:42:01 +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:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=zhyTxnonkbQLnC3fjLO8mzzlxV3OMCjGBPnUgJAU6LE=; b=mswyilQRUccR7wKYm8bHYl+B+l IBsWrcTbji87EiDKyw88OZ2VMw/Xpc1+wMF3f0LTbRj7IKYcd8N6aabhVr+Y6L4SKNp7YAAh81qZZ J191qoJRS4mF/u4Frsz7dl8LOfnAX2I6P7rm2WV0OCVoCBfZ8CLGtPJHmKaYwdhliTvFsfVVX8egq 1XiHqeCaaoMzTEy5YAW4nVGpbrSLirq3fcfOKdOYi2sj+huP7zX3m8uiwuUjo9+AScNUP5BPDCgNG kbI7/brytzNBHJp6yOqU7aggNMRLo2nDUST3wpEMpFT0/52E9RKZpmX1nfgCr0a6OVIKmxum3PVlr YwnK03kA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uecXT-00000005WNj-119X; Wed, 23 Jul 2025 16:41:55 +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 1uecV0-00000005W4Z-3Hd5 for linux-arm-kernel@lists.infradead.org; Wed, 23 Jul 2025 16:39:24 +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 D152722C7; Wed, 23 Jul 2025 09:39:15 -0700 (PDT) Received: from [10.1.196.46] (e134344.arm.com [10.1.196.46]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 70C4C3F6A8; Wed, 23 Jul 2025 09:39:18 -0700 (PDT) Message-ID: <84e52287-e46a-40c3-90b7-ab7e9f2c7431@arm.com> Date: Wed, 23 Jul 2025 17:39:17 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH 10/36] ACPI / MPAM: Parse the MPAM table To: Jonathan Cameron , James Morse Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Rob Herring , Rohit Mathew , Shanker Donthineni , Zeng Heng , Lecopzer Chen , Carl Worth , shameerali.kolothum.thodi@huawei.com, D Scott Phillips OS , 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 , Dave Martin , Koba Ko References: <20250711183648.30766-1-james.morse@arm.com> <20250711183648.30766-11-james.morse@arm.com> <20250716180725.0000452d@huawei.com> Content-Language: en-US From: Ben Horgan In-Reply-To: <20250716180725.0000452d@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250723_093922_909791_AA172E7A X-CRM114-Status: GOOD ( 53.60 ) 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 James, Jonathan, On 7/16/25 18:07, Jonathan Cameron wrote: > On Fri, 11 Jul 2025 18:36:22 +0000 > James Morse wrote: > >> Add code to parse the arm64 specific MPAM table, looking up the cache >> level from the PPTT and feeding the end result into the MPAM driver. > > Throw in a link to the spec perhaps? Particularly useful to know which > version this was written against when reviewing it. As I comment below this code checks the table revision is 1 and so we can assume it was written against version 2 of the spec. As of Monday, there is a new version hot off the press, https://developer.arm.com/documentation/den0065/3-0bet/?lang=en which introduces an "MMIO size" field to allow for disabled nodes. This should be considered here to avoid advertising msc that aren't present. > >> >> CC: Carl Worth >> Signed-off-by: James Morse >> --- >> arch/arm64/Kconfig | 1 + >> drivers/acpi/arm64/Kconfig | 3 + >> drivers/acpi/arm64/Makefile | 1 + >> drivers/acpi/arm64/mpam.c | 365 ++++++++++++++++++++++++++++++++++++ >> drivers/acpi/tables.c | 2 +- >> include/linux/arm_mpam.h | 46 +++++ >> 6 files changed, 417 insertions(+), 1 deletion(-) >> create mode 100644 drivers/acpi/arm64/mpam.c >> create mode 100644 include/linux/arm_mpam.h >> > >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile >> index 05ecde9eaabe..27b872249baa 100644 >> --- a/drivers/acpi/arm64/Makefile >> +++ b/drivers/acpi/arm64/Makefile >> @@ -6,5 +6,6 @@ obj-$(CONFIG_ACPI_GTDT) += gtdt.o >> obj-$(CONFIG_ACPI_IORT) += iort.o >> obj-$(CONFIG_ACPI_PROCESSOR_IDLE) += cpuidle.o >> obj-$(CONFIG_ARM_AMBA) += amba.o >> +obj-$(CONFIG_ACPI_MPAM) += mpam.o > > Keep it with the ACPI ones? There doesn't seem to be a lot of order in here > though so I guess maybe there is logic behind putting it here I'm missing. > >> obj-y += dma.o init.o >> obj-y += thermal_cpufreq.o >> diff --git a/drivers/acpi/arm64/mpam.c b/drivers/acpi/arm64/mpam.c >> new file mode 100644 >> index 000000000000..f4791bac9a2a >> --- /dev/null >> +++ b/drivers/acpi/arm64/mpam.c >> @@ -0,0 +1,365 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (C) 2025 Arm Ltd. >> + >> +/* Parse the MPAM ACPI table feeding the discovered nodes into the driver */ >> + >> +#define pr_fmt(fmt) "ACPI MPAM: " fmt >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +/* Flags for acpi_table_mpam_msc.*_interrupt_flags */ > > References.. I'm looking at 3.0-alpha table 5 to check this. > I can see why you might be reluctant to point at an alpha if that > is what you are using ;) > > >> +#define ACPI_MPAM_MSC_IRQ_MODE_EDGE 1 >> +#define ACPI_MPAM_MSC_IRQ_TYPE_MASK (3 << 1) > > GENMASK(3, 2) would be my preference for how to do masks in new code. > >> +#define ACPI_MPAM_MSC_IRQ_TYPE_WIRED 0 >> +#define ACPI_MPAM_MSC_IRQ_AFFINITY_PROCESSOR_CONTAINER BIT(3) >> +#define ACPI_MPAM_MSC_IRQ_AFFINITY_VALID BIT(4) >> + >> +static bool frob_irq(struct platform_device *pdev, int intid, u32 flags, >> + int *irq, u32 processor_container_uid) >> +{ >> + int sense; >> + >> + if (!intid) >> + return false; >> + >> + /* 0 in this field indicates a wired interrupt */ >> + if (flags & ACPI_MPAM_MSC_IRQ_TYPE_MASK) > I'd prefer more explicit code (and probably no comment) > > if (FIELD_GET(flags, ACPI_MPAM_MSC_IRQ_TYPE_MASK) != > ACPI_MPAM_MSC_IRQ_TYPE_WIRED) > return false; > >> + return false; >> + >> + if (flags & ACPI_MPAM_MSC_IRQ_MODE_EDGE) >> + sense = ACPI_EDGE_SENSITIVE; >> + else >> + sense = ACPI_LEVEL_SENSITIVE; > > If the spec is supposed to be using standard ACPI_* types for this field > (I don't think the connection is explicitly documented though) then > > sense = FIELD_GET(flags, ACPI_MPAM_MSC_IRQ_MODE_MASK); > Assuming a change to define the mask and rely on the ACPI defs for the values > > This one is entirely up to you. > >> + >> + /* >> + * If the GSI is in the GIC's PPI range, try and create a partitioned >> + * percpu interrupt. >> + */ >> + if (16 <= intid && intid < 32 && processor_container_uid != ~0) { >> + pr_err_once("Partitioned interrupts not supported\n"); >> + return false; >> + } >> + >> + *irq = acpi_register_gsi(&pdev->dev, intid, sense, ACPI_ACTIVE_HIGH); >> + if (*irq <= 0) { >> + pr_err_once("Failed to register interrupt 0x%x with ACPI\n", >> + intid); >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static void acpi_mpam_parse_irqs(struct platform_device *pdev, >> + struct acpi_mpam_msc_node *tbl_msc, >> + struct resource *res, int *res_idx) >> +{ >> + u32 flags, aff = ~0; >> + int irq; >> + >> + flags = tbl_msc->overflow_interrupt_flags; >> + if (flags & ACPI_MPAM_MSC_IRQ_AFFINITY_VALID && >> + flags & ACPI_MPAM_MSC_IRQ_AFFINITY_PROCESSOR_CONTAINER) >> + aff = tbl_msc->overflow_interrupt_affinity; > Just to make the two cases look the same I'd do > > else > aff = ~0; > > here as well and not initialize above. It's not quite worth using > a helper function for these two identical blocks but it's close. > >> + if (frob_irq(pdev, tbl_msc->overflow_interrupt, flags, &irq, aff)) { >> + res[*res_idx].start = irq; >> + res[*res_idx].end = irq; >> + res[*res_idx].flags = IORESOURCE_IRQ; >> + res[*res_idx].name = "overflow"; > > res[*res_idx] = DEFINE_RES_IRQ_NAMED(irq, 1, "overflow"); >> + >> + (*res_idx)++; > Can roll this in as well. > >> + } >> + >> + flags = tbl_msc->error_interrupt_flags; >> + if (flags & ACPI_MPAM_MSC_IRQ_AFFINITY_VALID && >> + flags & ACPI_MPAM_MSC_IRQ_AFFINITY_PROCESSOR_CONTAINER) >> + aff = tbl_msc->error_interrupt_affinity; >> + else >> + aff = ~0; >> + if (frob_irq(pdev, tbl_msc->error_interrupt, flags, &irq, aff)) { >> + res[*res_idx].start = irq; >> + res[*res_idx].end = irq; >> + res[*res_idx].flags = IORESOURCE_IRQ; >> + res[*res_idx].name = "error"; > > Similar to above. > >> + >> + (*res_idx)++; >> + } >> +} >> + > > >> +static bool __init parse_msc_pm_link(struct acpi_mpam_msc_node *tbl_msc, >> + struct platform_device *pdev, >> + u32 *acpi_id) >> +{ >> + bool acpi_id_valid = false; >> + struct acpi_device *buddy; >> + char hid[16], uid[16]; >> + int err; >> + >> + memset(&hid, 0, sizeof(hid)); >> + memcpy(hid, &tbl_msc->hardware_id_linked_device, >> + sizeof(tbl_msc->hardware_id_linked_device)); >> + >> + if (!strcmp(hid, ACPI_PROCESSOR_CONTAINER_HID)) { >> + *acpi_id = tbl_msc->instance_id_linked_device; >> + acpi_id_valid = true; >> + } >> + >> + err = snprintf(uid, sizeof(uid), "%u", >> + tbl_msc->instance_id_linked_device); >> + if (err < 0 || err >= sizeof(uid)) > > Does snprintf() ever return < 0 ? It's documented as returning > number of chars printed (without the NULL) so that can only be 0 or > greater. > > Can it return >= sizeof(uid) ? Looks odd. > > + return acpi_id_valid; >> + >> + buddy = acpi_dev_get_first_match_dev(hid, uid, -1); >> + if (buddy) >> + device_link_add(&pdev->dev, &buddy->dev, DL_FLAG_STATELESS); >> + >> + return acpi_id_valid; >> +} > >> +static int __init _parse_table(struct acpi_table_header *table) >> +{ >> + char *table_end, *table_offset = (char *)(table + 1); >> + struct property_entry props[4]; /* needs a sentinel */ >> + struct acpi_mpam_msc_node *tbl_msc; >> + int next_res, next_prop, err = 0; >> + struct acpi_device *companion; >> + struct platform_device *pdev; >> + enum mpam_msc_iface iface; >> + struct resource res[3]; >> + char uid[16]; >> + u32 acpi_id; >> + >> + table_end = (char *)table + table->length; >> + >> + while (table_offset < table_end) { >> + tbl_msc = (struct acpi_mpam_msc_node *)table_offset; >> + table_offset += tbl_msc->length; >> + >> + /* >> + * If any of the reserved fields are set, make no attempt to >> + * parse the msc structure. This will prevent the driver from >> + * probing all the MSC, meaning it can't discover the system >> + * wide supported partid and pmg ranges. This avoids whatever >> + * this MSC is truncating the partids and creating a screaming >> + * error interrupt. >> + */ >> + if (tbl_msc->reserved || tbl_msc->reserved1 || tbl_msc->reserved2) >> + continue; >> + >> + if (decode_interface_type(tbl_msc, &iface)) >> + continue; >> + >> + next_res = 0; >> + next_prop = 0; >> + memset(res, 0, sizeof(res)); >> + memset(props, 0, sizeof(props)); >> + >> + pdev = platform_device_alloc("mpam_msc", tbl_msc->identifier); >> + if (IS_ERR(pdev)) { > > returns NULL in at least some error cases (probably all, I'm just to lazy to check) > >> + err = PTR_ERR(pdev); >> + break; >> + } >> + >> + if (tbl_msc->length < sizeof(*tbl_msc)) { >> + err = -EINVAL; >> + break; >> + } >> + >> + /* Some power management is described in the namespace: */ >> + err = snprintf(uid, sizeof(uid), "%u", tbl_msc->identifier); >> + if (err > 0 && err < sizeof(uid)) { >> + companion = acpi_dev_get_first_match_dev("ARMHAA5C", uid, -1); >> + if (companion) >> + ACPI_COMPANION_SET(&pdev->dev, companion); >> + } >> + >> + if (iface == MPAM_IFACE_MMIO) { >> + res[next_res].name = "MPAM:MSC"; >> + res[next_res].start = tbl_msc->base_address; >> + res[next_res].end = tbl_msc->base_address + tbl_msc->mmio_size - 1; >> + res[next_res].flags = IORESOURCE_MEM; >> + next_res++; > > DEFINE_RES_MEM_NAMED()? > >> + } else if (iface == MPAM_IFACE_PCC) { >> + props[next_prop++] = PROPERTY_ENTRY_U32("pcc-channel", >> + tbl_msc->base_address); >> + next_prop++; >> + } >> + >> + acpi_mpam_parse_irqs(pdev, tbl_msc, res, &next_res); >> + err = platform_device_add_resources(pdev, res, next_res); >> + if (err) >> + break; >> + >> + props[next_prop++] = PROPERTY_ENTRY_U32("arm,not-ready-us", >> + tbl_msc->max_nrdy_usec); >> + >> + /* >> + * The MSC's CPU affinity is described via its linked power >> + * management device, but only if it points at a Processor or >> + * Processor Container. >> + */ >> + if (parse_msc_pm_link(tbl_msc, pdev, &acpi_id)) { >> + props[next_prop++] = PROPERTY_ENTRY_U32("cpu_affinity", >> + acpi_id); >> + } >> + >> + err = device_create_managed_software_node(&pdev->dev, props, >> + NULL); >> + if (err) >> + break; >> + >> + /* Come back later if you want the RIS too */ >> + err = platform_device_add_data(pdev, tbl_msc, tbl_msc->length); >> + if (err) >> + break; >> + >> + platform_device_add(pdev); > > Can fail. > >> + } >> + >> + if (err) >> + platform_device_put(pdev); >> + >> + return err; >> +} >> + >> +static struct acpi_table_header *get_table(void) >> +{ >> + struct acpi_table_header *table; >> + acpi_status status; >> + >> + if (acpi_disabled || !system_supports_mpam()) >> + return NULL; >> + >> + status = acpi_get_table(ACPI_SIG_MPAM, 0, &table); >> + if (ACPI_FAILURE(status)) >> + return NULL; >> + >> + if (table->revision != 1) >> + return NULL; Indicates that this was written against version 2 of the spec. >> + >> + return table; >> +} >> + >> +static int __init acpi_mpam_parse(void) >> +{ >> + struct acpi_table_header *mpam; >> + int err; >> + >> + mpam = get_table(); >> + if (!mpam) >> + return 0; > > Just what I was suggesting for the PPTT case in early patches. Nice :) > >> + >> + err = _parse_table(mpam); >> + acpi_put_table(mpam); >> + >> + return err; >> +} >> + >> +static int _count_msc(struct acpi_table_header *table) >> +{ >> + char *table_end, *table_offset = (char *)(table + 1); >> + struct acpi_mpam_msc_node *tbl_msc; >> + int ret = 0; > > Call it count as it only ever contains the count? > >> + >> + tbl_msc = (struct acpi_mpam_msc_node *)table_offset; >> + table_end = (char *)table + table->length; >> + >> + while (table_offset < table_end) { >> + if (tbl_msc->length < sizeof(*tbl_msc)) >> + return -EINVAL; >> + >> + ret++; > > count++ would feel more natural here. > >> + >> + table_offset += tbl_msc->length; >> + tbl_msc = (struct acpi_mpam_msc_node *)table_offset; >> + } >> + >> + return ret; >> +} > > That's all I have time for today. Will get to the rest of the series soonish. > > Jonathan > > Thanks, Ben