Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp525487pxj; Thu, 10 Jun 2021 06:38:52 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzqrbxRd9zRfRuftMJ37RZFyIxXnFsnW+xYoNhHUo59cjhsYMG3XQY3zbi67Tl5ILRZPCOZ X-Received: by 2002:a05:6402:1c11:: with SMTP id ck17mr5045825edb.102.1623332332362; Thu, 10 Jun 2021 06:38:52 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1623332332; cv=none; d=google.com; s=arc-20160816; b=FyqM/bQWUY2b686xG5SD8y2CC9T59RWnqfKg2NJxNLk6lPlYB9ECDSEq8/YUdbCHAT t3La3/R/O8r3D8nc92VF6aEVz1sX+XlYa1evorpBEZuib/D4hoy425Dn2+LL/NaqHaZ7 v7JN8F8ivDfLjKgKCpR8htFW9xJmhLMAJ7N22N2k7ujypRu9uA9QR1fyf16nRsr9AbSW rpddBjFOP8FgAF9KnJe0LcAGtXEHM/42po3SBwdywgAH2/WPDMNrb9cPrnfuyg/5qN8T rdCZkn0Z2OsPmquwoB9IqqiE/eebTiBeOBlYlZZo8TNbNNuNipy06L2IXwTAFwrN9U+Y 6d8g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:user-agent:message-id:in-reply-to :date:references:subject:cc:to:from:dkim-signature; bh=25QHYDZU/33GobJcVDtQ/GuUkx9D2Ms/u7IzN+8Cqkg=; b=RqRMW1Gyknf+XGL9+F+mSpLW9Eo6QxqXziqYiLAAFrOK4b6TY8gJ6KoO8V3/5itCDm LKWiqalvZZwmC5ExqySrF+t2RGYK+0Ez3WEOZRtrieSKNB7r9TYVx1y1QRCoozodOrx9 b08oRCgHfCTjRB7NbiT5kThNcz9embPKJ8MxRyKt6N/DVtQuSCGZlfRibmVIrfNYLTuC kVzpXcwOtjCgFv7rhk6aCNVN4C5n5TmYjbtnOzmEvYdv4Xs9GNVbsZ+C8NW5hbFUc67M YW0NcbHUDW8z5saTNz+3bdGohECLE5EQjytncnUm8PRs9bXYlLDsz0+hUC2rP9VpTdE9 gzCQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=IWtVLYED; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id 7si2343806ejh.116.2021.06.10.06.38.26; Thu, 10 Jun 2021 06:38:52 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=IWtVLYED; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231169AbhFJNhN (ORCPT + 99 others); Thu, 10 Jun 2021 09:37:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33780 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231165AbhFJNhL (ORCPT ); Thu, 10 Jun 2021 09:37:11 -0400 Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 13606C0617A6; Thu, 10 Jun 2021 06:35:01 -0700 (PDT) Received: by mail-pf1-x42b.google.com with SMTP id m7so1634337pfa.10; Thu, 10 Jun 2021 06:35:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=25QHYDZU/33GobJcVDtQ/GuUkx9D2Ms/u7IzN+8Cqkg=; b=IWtVLYEDFrwtyBXWN2I0UMn125oAptlDnMrRMYqy/T8Thb6CgZyjwUh9fks114+MF/ w+g32AlBbaqXBjqvWJpbM0rA3L8lyq9VwBk+mrz9HpVH19Bs8Xj2wrw+tb2rwHOOq2EI DnO9b3YgmsMX6XAPbW/HdnN+dkkrpiIzPAzj+cf5+nlw+WUJlzIRT6s9E6kj6F5MqLDn svkN/JLoSzf9DpJHX4pbkPoDClYDrjKHiB3rsIKrrEWsw1c3Z81nsUE0xI4agGr4bKg1 tVx/WcOnKi4o5z4D7fzb1na4/bhZZIuJJcZKRDED++zWGtG8sjRJjLphVlsY+dNzN1DO 27TA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=25QHYDZU/33GobJcVDtQ/GuUkx9D2Ms/u7IzN+8Cqkg=; b=Yt7cfLvF9MbHQOS/sqzecduJXsMjhP+K1+F2HitAl5F7YpoAQ+bGnf3aUEu1SS0Bz9 5vHnc4HWy9Wtjg9+aCSR0+RHJ2IAkECrvYjcJck14wmuGl64HaRu7vv/2hyJj2e1DAt5 JiHxXCs7sZ/NIyAnvH9tES69geql6JW7L8QTA43s9e+cauca29E26KcLy59JsBeIjeXq flUNFbrVo17pgOfXcRA0uFCUZ6UHGraJWpql3zvqe39Z3DylNzABaDv6qC1lCHlVNkXb C00DCbI3bmSnAO1xXBm6QuvmNThnctuvKyBx02DxdQzj7z+1iI1810lKEbKUvV+EtnlN iqIw== X-Gm-Message-State: AOAM530btOy9XwIICaPb0Ne0S1fD2yghc/6CZ1P+1jdrwiTQuO76DUum EEI9guxRmmNqYf5VtcJGBs4= X-Received: by 2002:a63:6c84:: with SMTP id h126mr5011352pgc.54.1623332100417; Thu, 10 Jun 2021 06:35:00 -0700 (PDT) Received: from localhost (122x211x248x161.ap122.ftth.ucom.ne.jp. [122.211.248.161]) by smtp.gmail.com with ESMTPSA id k15sm2473875pff.19.2021.06.10.06.34.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Jun 2021 06:34:59 -0700 (PDT) From: Punit Agrawal To: Bjorn Helgaas Cc: robh+dt@kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, alexandru.elisei@arm.com, wqu@suse.com, robin.murphy@arm.com, pgwipeout@gmail.com, ardb@kernel.org, briannorris@chromium.org, shawn.lin@rock-chips.com, Bjorn Helgaas , Leonardo Bras Subject: Re: [PATCH v3 1/4] PCI: of: Clear 64-bit flag for non-prefetchable memory below 4GB References: <20210610002256.GA2680171@bjorn-Precision-5520> Date: Thu, 10 Jun 2021 22:34:56 +0900 In-Reply-To: <20210610002256.GA2680171@bjorn-Precision-5520> (Bjorn Helgaas's message of "Wed, 9 Jun 2021 19:22:56 -0500") Message-ID: <875yyllu67.fsf@stealth> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Bjorn, Bjorn Helgaas writes: > [+cc Leonardo] > > On Mon, Jun 07, 2021 at 08:28:53PM +0900, Punit Agrawal wrote: >> Some host bridges advertise non-prefetchable memory windows that are >> entirely located below 4GB but are marked as 64-bit address memory. >> >> Since commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource >> flags for 64-bit memory addresses"), the OF PCI range parser takes a >> stricter view and treats 64-bit address ranges as advertised while >> before such ranges were treated as 32-bit. >> >> A PCI root port modelled as a PCI-to-PCI bridge cannot forward 64-bit >> non-prefetchable memory ranges. As a result, the change in behaviour >> due to the commit causes failure to allocate 32-bit BAR from a 64-bit >> non-prefetchable window. >> >> In order to not break platforms where non-prefetchable memory ranges >> lie entirely below 4GB, clear the 64-bit flag. > > I don't think we should care about the address width DT supplies for a > host bridge window. Prior to 9d57e61bf723, I don't think we *did* > care because of_bus_pci_get_flags() threw away that information. > > My proposal for a commit log, including information about the problem > report and a "Fixes:" tag: > > Alexandru and Qu reported this resource allocation failure on > ROCKPro64 v2 and ROCK Pi 4B, both based on the RK3399: > > pci_bus 0000:00: root bus resource [mem 0xfa000000-0xfbdfffff 64bit] > pci 0000:00:00.0: PCI bridge to [bus 01] > pci 0000:00:00.0: BAR 14: no space for [mem size 0x00100000] > pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x00003fff 64bit] > > "BAR 14" is the PCI bridge's 32-bit non-prefetchable window, and our > PCI allocation code isn't smart enough to allocate it in a host > bridge window marked as 64-bit, even though this should work fine. > > A DT host bridge description includes the windows from the CPU > address space to the PCI bus space. On a few architectures > (microblaze, powerpc, sparc), the DT may also describe PCI devices > themselves, including their BARs. > > Before 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource > flags for 64-bit memory addresses"), of_bus_pci_get_flags() ignored > the fact that some DT addresses described 64-bit windows and BARs. > That was a problem because the virtio virtual NIC has a 32-bit BAR > and a 64-bit BAR, and the driver couldn't distinguish them. Many thanks for demystifying the motivation for 9d57e61bf723. Not being familiar with the usage of DT to describe PCI devices I was missing this context. > 9d57e61bf723 set IORESOURCE_MEM_64 for those 64-bit DT ranges, which > fixed the virtio driver. But it also set IORESOURCE_MEM_64 for host > bridge windows, which exposed the fact that the PCI allocator isn't > smart enough to put 32-bit resources in those 64-bit windows. > > Clear IORESOURCE_MEM_64 from host bridge windows since we don't need > that information. > > Fixes: 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource flags for 64-bit memory addresses") > Reported-at: https://lore.kernel.org/lkml/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com/ > Reported-by: Alexandru Elisei > Reported-by: Qu Wenruo Thank you for commit log - without all the pieces I was struggling to clearly describe the details. And I missed the appropriate tags as well. I've updated the commit log based on your suggestion. >> Suggested-by: Ard Biesheuvel >> Link: https://lore.kernel.org/r/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com >> Signed-off-by: Punit Agrawal >> Tested-by: Alexandru Elisei >> Cc: Bjorn Helgaas >> Cc: Rob Herring >> --- >> drivers/pci/of.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/pci/of.c b/drivers/pci/of.c >> index 85dcb7097da4..1e45186a5715 100644 >> --- a/drivers/pci/of.c >> +++ b/drivers/pci/of.c >> @@ -353,6 +353,14 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, >> dev_warn(dev, "More than one I/O resource converted for %pOF. CPU base address for old range lost!\n", >> dev_node); >> *io_base = range.cpu_addr; >> + } else if (resource_type(res) == IORESOURCE_MEM) { >> + if (!(res->flags & IORESOURCE_PREFETCH)) { >> + if (res->flags & IORESOURCE_MEM_64) >> + if (!upper_32_bits(range.pci_addr + range.size - 1)) { >> + dev_warn(dev, "Clearing 64-bit flag for non-prefetchable memory below 4GB\n"); >> + res->flags &= ~IORESOURCE_MEM_64; >> + } >> + } > > Why do we need to check IORESOURCE_PREFETCH, IORESOURCE_MEM_64, and > upper_32_bits()? If I understand this correctly, prior to > 9d57e61bf723, IORESOURCE_MEM_64 was *never* set here. Isn't something > like this sufficient? > > } else if (resource_type(res) == IORESOURCE_MEM) { > res->flags &= ~IORESOURCE_MEM_64; > } Based on the discussion in the original thread[0], I was working with the assumption that we don't want to lose the IORESOURCE_MEM_64 flag other than in the problem scenario, i.e., non-prefetchable memory below 4GB. You suggestion is simpler and also solves the issue by effectively reverting the impact of 9d57e61bf723 on BAR allocation. If there are no objections I will take this approach for the next update. To aid future readers I will also add the following comment - /* * PCI allocation cannot correctly allocate 32-bit non-prefetchable BAR * in host bridge windows marked as 64-bit. */ > I'm not sure we need a warning either. We didn't warn before > 9d57e61bf723, and there's nothing the user needs to do anyway. The warning was a nudge (probably too subtle) to get the user to upgrade their DT to drop the 64-bit marker on the host bridge window. With your suggestion, the DT change is not needed anymore - though it may still be worth dropping the 64-bit marker. Thanks, Punit [0] https://lore.kernel.org/linux-pci/CAMj1kXGF_JmuZ+rRA55-NrTQ6f20fhcHc=62AGJ71eHNU8AoBQ@mail.gmail.com/