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 X-Spam-Level: X-Spam-Status: No, score=-10.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32E6FC433E3 for ; Fri, 21 Aug 2020 13:49:32 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 045B120578 for ; Fri, 21 Aug 2020 13:49:32 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="tvkIuKHi"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="GkneyOgR" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 045B120578 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Message-ID:Subject:To:From: Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References:List-Owner; bh=EWNIkUywWjYwXj7t7GSFPvBUSoY+cXDN7pj54A/ccYs=; b=tvkIuKHiUsbctxS17IWj/T4W1 vYWTlezjOeAsgVmnMBNQ5U20sGWx4k2V9gdsl4J9/MZzIH373P+4swzKJRbWPH8vYoJkcatkMMW2H dxY5+p6fvLlL7nQpxJGEsl1LuNGGPJsMGpGXqU1csi/06LQRHpEt5X460fSLvd6TTs3SqrkHlKLkN /xDzMPoq2bY+E7pRimh7Qyr9gjY1XU1Njl8ho3BhioHt4Q7R0mGLXdC0Pl1rRWsMXzfdgmEcMpZE6 03SNm9khc6RZAbLO2hNCRvS7bToum7SBDYegK0ZIbEIqqcbqAZpJ+ppcFV2ZHJA3L+KRrA4t7v6GV U6KX6BzfA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k97OS-00010E-NH; Fri, 21 Aug 2020 13:47:44 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k97NC-00006z-Ek for linux-arm-kernel@lists.infradead.org; Fri, 21 Aug 2020 13:46:29 +0000 Received: from localhost (104.sub-72-107-126.myvzw.com [72.107.126.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 34E7A20578; Fri, 21 Aug 2020 13:46:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1598017583; bh=eR4savNfPpi1Bov9kuM6We12VX5PsJEnD49TjFI45vg=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=GkneyOgRoOluuwCS8xddIUouzrh/qxThxKNQqmUwYR7Jnj2FNd7CmaGUCo9Y5LNWH EO7HofDa6BDrqn5DFQtjbw35eaJgRLSAij7dSUApb/jB2fCp5ifAMxfdRww/kcoeSJ 88rBxQ9cvp6v3DCLnmkaTHi5lqEzwBvg2yvXhMoo= Date: Fri, 21 Aug 2020 08:46:22 -0500 From: Bjorn Helgaas To: Jonathan Cameron Subject: Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3 Message-ID: <20200821134622.GA1620197@bjorn-Precision-5520> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200821135901.0000260b@Huawei.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200821_094628_154006_E4DD440A X-CRM114-Status: GOOD ( 43.35 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: rafael@kernel.org, Lorenzo Pieralisi , linux-acpi@vger.kernel.org, linux-api@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, linuxarm@huawei.com, Keith Busch , linux-mm@kvack.org, Ingo Molnar , Brice Goglin , Bjorn Helgaas , Thomas Gleixner , Dan Williams , linux-arm-kernel@lists.infradead.org, Sean V Kelley Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Aug 21, 2020 at 01:59:01PM +0100, Jonathan Cameron wrote: > On Fri, 21 Aug 2020 07:13:56 -0500 > Bjorn Helgaas wrote: > > > [+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report > > heterogeneous memory")] > > > > On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote: > > > On Thu, 20 Aug 2020 17:21:29 -0500 > > > Bjorn Helgaas wrote: > > > > > > > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote: > > > > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure > > > > > changed substantially. One of those changes was that the flag > > > > > for "Memory Proximity Domain field is valid" was deprecated. > > > > > > > > > > This was because the field "Proximity Domain for the Memory" > > > > > became a required field and hence having a validity flag makes > > > > > no sense. > > > > > > > > > > So the correct logic is to always assume the field is there. > > > > > Current code assumes it never is. > > > > > > > > > > Signed-off-by: Jonathan Cameron > > > > > --- > > > > > drivers/acpi/numa/hmat.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c > > > > > index 2c32cfb72370..07cfe50136e0 100644 > > > > > --- a/drivers/acpi/numa/hmat.c > > > > > +++ b/drivers/acpi/numa/hmat.c > > > > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade > > > > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n", > > > > > p->flags, p->processor_PD, p->memory_PD); > > > > > > > > > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) { > > > > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) { > > > > > > > > I hope/assume the spec is written in such a way that p->memory_PD is > > > > required for any revision > 1? So maybe this should be: > > > > > > > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || > > > > hmat_revision > 1) { > > > > I should have said simply: > > > > if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID) > > > > We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless > > we already know it's revision 1. > > > > And unless there was a revision 0 of HMAT, there's no need to look for > > hmat_revison > 1. > > It needs to stay as an or statement as you had the first time. > The field is always valid for hmat_revision > 1, and valid for > hmat_revision == 1 with the flag set. You could express it as > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID) || (hmat_revision != 1)) > > but that seems more confusing to me. Oh, you're right, sorry! There are two questions here: 1) In what order should we test "p->flags & ACPI_HMAT_MEMORY_PD_VALID" and "hmat_revision == 1"? ACPI_HMAT_MEMORY_PD_VALID is defined only when "hmat_revision == 1", so I think we should test the revision first. When "hmat_revision == 2", ACPI_HMAT_MEMORY_PD_VALID is reserved, so we shouldn't test it, even if we later check the revision and discard the result of the flag test. This is a tiny thing, admittedly, but I think it follows the spec more clearly. 2) Do we need to test hmat_revision for anything other than 1? Yes, you're right, see below. > > > Good point. We have existing protections elsewhere against > > > hmat_revision being anything other than 1 or 2, so we should aim to > > > keep that in only one place. > > > > I think the "Ignoring HMAT: Unknown revision" test in hmat_init(), > > added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous > > memory"), is a mistake. > > > > And I think hmat_normalize() has a similar mistake in that it tests > > explicitly for hmat_revision == 2 when it should accept 2 AND anything > > later. > > > > We should assume that future spec revisions will be backwards > > compatible. Otherwise we're forced to make kernel changes when we > > otherwise would not have to. > > I disagree with this. There is no rule in ACPI about maintaining > backwards compatibility. The assumption is that the version number > will always be checked. The meaning of fields changed between > version 1 and version 2 so it would be bold to assume that won't > happen in the future! There *is* a rule about maintaining backwards compatibility. ACPI v6.3, sec 5.2.2, says: All versions of the ACPI tables must maintain backward compatibility. To accomplish this, modifications of the tables consist of redefinition of previously reserved fields and values plus appending data to the 1.0 tables. Modifications of the ACPI tables require that the version numbers of the modified tables be incremented. > HMAT is an optional table, so if someone boots up an old kernel > they are probably better off failing to use it at all than > misinterpreting it. An old kernel tests: if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) target = find_mem_target(p->memory_PD); which is fine on old firmware. On new firmware (hmat_revision == 2), it will ignore p->memory_PD. That is probably a problem, but I think we should check for that at the place where we need a memory_PD and don't find one. That's more general than sanity checking a revision. A new kernel that tests: if ((hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID) || hmat_revision > 1) target = find_mem_target(p->memory_PD); will do the right thing on both old and new firmware. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel