Received: by 2002:a25:6193:0:0:0:0:0 with SMTP id v141csp1153919ybb; Wed, 1 Apr 2020 17:05:57 -0700 (PDT) X-Google-Smtp-Source: APiQypKd8rAsCrgVWeJrVSeTozLdJm5uY6Ckd/wGEcteX9+vg/mLLnZFXRnDGx6ovPH+4u65Z8bP X-Received: by 2002:aca:f288:: with SMTP id q130mr403794oih.33.1585785957551; Wed, 01 Apr 2020 17:05:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1585785957; cv=none; d=google.com; s=arc-20160816; b=z0C27gN5mZ/C4+OgsmA9TfOJdvULyElJwAeMAb8p8hQrCB3AHorfGwtIzJ9X/VBXeY Ukwmp0+mMuOP6Q65oZ3rUQChG06YTFbtU5ncLxWH2OSQnmwcpnmfE0IhmDx2ZPpJk1Gp b7hHxBD/eLBqPqC/lw3ozXRPvOr2XtbpVI5PqFYAE3TWu65hzWDIn4PAb1tTavfTIRDg TzquOSpoqMXEmWwoE69sW1iUA22zx7fPBZjk3CSyDWLvhtjdtNc6ISonB00P7sm3tuPE gkZV4WVnhWzEFHyrG4f6XZ+BvVLX72TcdNpR4OOPPYXqCH2vIfLoEhJJcjko18fZkJSq t4NQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=HbToDyfvISc0MzdSRiZMdo2xILcDDxUI1mnFelsEDqQ=; b=EJQYFtubmbZ6uPOZQ7d0y09tEtJCeb47D80KcGs0ZRFxGjGkmU+7MQMjQZwyEt3fre om2Ac+eMcUKLv0ocwBjaQ6xXeBlUCi4NL5NuCzGHNOJB+7qmE3eSJV37VFEyviFztOqO xydWp2Bq1GJ4BZ5EntiNV2ZbDoMoI/wfx5ydTtpPIKmMW7cy43TRof+QAAKEjG4bJT7M AYygAmAnOezokONW+mUexuKb7QbJfZ5e0BnaaXKxoMz+yc+T1XZjx2ALxA5S/wVrEaf9 RQZlw9vnZENebjSXBxx/9cu90WaL3/xc1d5OVbLtgS9Lu200MU0nubYNli58t2uCJcKu SO0Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sifive.com header.s=google header.b=LhCJsl1Z; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id k194si1445471oib.251.2020.04.01.17.05.44; Wed, 01 Apr 2020 17:05:57 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@sifive.com header.s=google header.b=LhCJsl1Z; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1733257AbgDBAFE (ORCPT + 99 others); Wed, 1 Apr 2020 20:05:04 -0400 Received: from mail-lf1-f68.google.com ([209.85.167.68]:38548 "EHLO mail-lf1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732872AbgDBAFE (ORCPT ); Wed, 1 Apr 2020 20:05:04 -0400 Received: by mail-lf1-f68.google.com with SMTP id c5so1246583lfp.5 for ; Wed, 01 Apr 2020 17:05:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=HbToDyfvISc0MzdSRiZMdo2xILcDDxUI1mnFelsEDqQ=; b=LhCJsl1ZVXkiiIaRr7trZvle/wKMDu6R1ix6l2+VcimCvMksOWsbf+l/PtbMXtJ8v2 sBNoLpvThmCkpWgEBwz7/3tB2h1PW7GguOPp8ERH8ltg0Zjr3Yf2MSlmXICXAtUFTyLw 30JJukxFnhYXHNKCnsqG7hVk04lBbvKt7VBEk1mBFCVAJs8nD7B7dAcaac4iBkIbm2KL DPe84t0uxAfvrCzlhxUYzPh3wQUHHW+nOhXwalmGsjTBgjwmp5zPoMJ1ZDdLr8LRLw6H VFbfIUzWVc6pLlbnXKeOEBEFan5KSXRlDPB7kSCiVrkrd4l4kcXQQEfEsf7Jt6Bz/OVb FWrw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=HbToDyfvISc0MzdSRiZMdo2xILcDDxUI1mnFelsEDqQ=; b=N7ppIeFmlqe1I7cQ5HmwZws2fHAabg3tXkc6vxqPifppSgBgt23vxtcXCGmt5hGCYp A83JEGEacD+xv8IOaRZvTqvA2RPTPs4RyP4hO7qZwb459uAp2XRcgHIpLVzZehymzgzi w0WNCTdFZW8vJK9DMcjgARbFbtNS4a0VffdTvAEEr31iLI2j/iyoDo4qwcK8KX8hgSkM FsiHahZArGbfKZ5QPz+ZzJrjBMN7LiW2i31VeKIbYYez38C62wtxSsS5nZSc/gJUTiSU fR2Q8LlYZEh8bhg/gWgxXGZJWPe//Zqum7TbBsgMfMA9dvu5iXqVrFoc2jfwKt2G5Yse 03Xw== X-Gm-Message-State: AGi0PuYdbZDFB0Fnyi6hpMDzRrxhBTt8uSEJBoCU4UCageuuzEo6QEmF nJ/9kegcXBHec3qnNacnwcmWB+iVJI5aDsksiNC/pQ== X-Received: by 2002:ac2:545a:: with SMTP id d26mr419914lfn.85.1585785900862; Wed, 01 Apr 2020 17:05:00 -0700 (PDT) MIME-Version: 1.0 References: <20200401202937.GA130497@google.com> In-Reply-To: From: Alan Mikhak Date: Wed, 1 Apr 2020 17:04:49 -0700 Message-ID: Subject: Re: [PATCH] PCI: Warn about MEM resource size being too big To: Bjorn Helgaas Cc: linux-kernel@vger.kernel.org, linux-pci , Jingoo Han , Gustavo Pimentel , lorenzo.pieralisi@arm.com, amurray@thegoodpenguin.co.uk, Kishon Vijay Abraham I , Paul Walmsley Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 1, 2020 at 3:02 PM Alan Mikhak wrote: > > On Wed, Apr 1, 2020 at 2:01 PM Alan Mikhak wrote: > > > > On Wed, Apr 1, 2020 at 1:29 PM Bjorn Helgaas wrote: > > > > > > On Tue, Mar 31, 2020 at 01:36:04PM -0700, Alan Mikhak wrote: > > > > On Tue, Mar 31, 2020 at 1:12 PM Bjorn Helgaas wrote: > > > > > On Mon, Mar 30, 2020 at 05:19:47PM -0700, Alan Mikhak wrote: > > > > > > Output a warning for MEM resource size with > > > > > > non-zero upper 32-bits. > > > > > > > > > > > > ATU programming functions limit the size of > > > > > > the translated region to 4GB by using a u32 size > > > > > > parameter. Function dw_pcie_prog_outbound_atu() > > > > > > does not program the upper 32-bit ATU limit > > > > > > register. This may result in undefined behavior > > > > > > for resource sizes with non-zero upper 32-bits. > > > > > > > > > > > > For example, a 128GB address space starting at > > > > > > physical CPU address of 0x2000000000 with size of > > > > > > 0x2000000000 needs the following values programmed > > > > > > into the lower and upper 32-bit limit registers: > > > > > > 0x3fffffff in the upper 32-bit limit register > > > > > > 0xffffffff in the lower 32-bit limit register > > > > > > > > > > > > Currently, only the lower 32-bit limit register is > > > > > > programmed with a value of 0xffffffff but the upper > > > > > > 32-bit limit register is not being programmed. > > > > > > As a result, the upper 32-bit limit register remains > > > > > > at its default value after reset of 0x0. This would > > > > > > be a problem for a 128GB PCIe space because in > > > > > > effect its size gets reduced to 4GB. > > > > > > > > > > > > ATU programming functions can be changed to > > > > > > specify a u64 size parameter for the translated > > > > > > region. Along with this change, the internal > > > > > > calculation of the limit address, the address of > > > > > > the last byte in the translated region, needs to > > > > > > change such that both the lower 32-bit and upper > > > > > > 32-bit limit registers can be programmed correctly. > > > > > > > > > > > > Changing the ATU programming functions is high > > > > > > impact. Without change, this issue can go > > > > > > unnoticed. A warning may prompt the user to > > > > > > look into possible issues. > > > > > > > > > > So this is basically a warning, and we could actually *fix* the > > > > > problem with more effort? I vote for the fix. > > > > > > > > The fix would impact all PCIe drivers that depend on dwc. > > > > > > Is that another way of saying "the fix would *fix* all the drivers > > > that depend on dwc"? > > > > Thanks Bjorn for your comments. > > > > Not at all. I'm not suggesting that. I'm just stating the dilemma. > > > > One option is, as you may be alluding, the *fix* would include > > modification to all drivers that depend on dwc to at least not break > > the build.. Whoever embarks on such a *fix* would have to take > > that on before submitting the patch. > > > > Another option is to produce an alternate ATU programming > > API for the Linux PCI sub-system to support u64 size. That > > way individual driver owners can choose if and when to migrate > > their drivers to the new API on their own timeline. Such an > > alternative API can also be generic to support not only > > Designware PCIe controllers but others. > > > > > > > > > I would have no way of validating such a fix without > > > > breaking it for everyone let alone the bandwidth it needs. > > > > All drivers that depend on dwc seem to be currently happy > > > > with the u32 size limit. I suggest we add the warning but > > > > keep this issue in mind for a solution that allows existing > > > > PCe drivers to phase into the fix on their own individual > > > > schedules, if they need to. > > > > > > Obviously it would *nice* to test all the drivers that depend on dwc, > > > but if you're fixing a problem, you verify the fix on your system, and > > > the relevant people review it, I don't think exhaustive testing is a > > > hard requirement, and I certainly wouldn't expect you to do it. > > > > That is a relief for whoever commits to take this on. > > > > > > > > If we want to live with a 32-bit limit, I think we should change the > > > relevant interfaces to use u32 so there's not a way to venture into > > > this region of undefined behavior. I don't think "warning + undefined > > > behavior" is a very maintainable situation. > > > > I cannot live with the 32-bit limit. I need a 64-bit solution. I had > > to implement a solution that suits my needs. I have worked > > out some of the issue. It is generic enough for my use with PCIe > > controllers from more than one vendor. But, it requires pulling a > > lot of code from Designware layer into a separate framework > > which I believe can become common for Linux PCI subsystem. > > If it gets in, others who need 64-bit can migrate over to it without > > being migrated involuntarily. > > > > The "undefined" behavior part of the problem can be fixed > separately in the function dw_pcie_prog_outbound_atu_unroll() > by modifying its internal calculation of the limit address and > programming the ATU upper limit address register. > > With the above fix in dw_pcie_prog_outbound_atu_unroll(), > the total problem reduces to the size issued being reduced > to 4G max. > Bjorn, Thinking about your comment, I decided to submit a separate patch to fix the "undefined" behavior portion of these issues by modifying dw_pcie_prog_outbound_atu_unroll(). Would the warning plus the patch for dw_pcie_prog_outbound_atu_unroll() make the remaining 32-bit size limit issue more maintainable? Regards, Alan > > > > > > > > > This limitation also means that multiple ATUs > > > > > > would need to be used to map larger regions. > > > > > > > > > > > > Signed-off-by: Alan Mikhak > > > > > > --- > > > > > > drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++++- > > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > > > > > index 395feb8ca051..37a8c71ef89a 100644 > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > > > > > @@ -325,6 +325,7 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > > > > struct pci_bus *child; > > > > > > struct pci_host_bridge *bridge; > > > > > > struct resource *cfg_res; > > > > > > + resource_size_t mem_size; > > > > > > u32 hdr_type; > > > > > > int ret; > > > > > > > > > > > > @@ -362,7 +363,10 @@ int dw_pcie_host_init(struct pcie_port *pp) > > > > > > case IORESOURCE_MEM: > > > > > > pp->mem = win->res; > > > > > > pp->mem->name = "MEM"; > > > > > > - pp->mem_size = resource_size(pp->mem); > > > > > > + mem_size = resource_size(pp->mem); > > > > > > + if (upper_32_bits(mem_size)) > > > > > > + dev_warn(dev, "MEM resource size too big\n"); > > > > > > + pp->mem_size = mem_size; > > > > > > pp->mem_bus_addr = pp->mem->start - win->offset; > > > > > > break; > > > > > > case 0: > > > > > > -- > > > > > > 2.7.4 > > > > > >