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;
MIME-Version: 1.0
References: <1585613987-8453-1-git-send-email-alan.mikhak@sifive.com> <20200331201225.GA19649@google.com>
In-Reply-To: <20200331201225.GA19649@google.com>
From:   Alan Mikhak <alan.mikhak@sifive.com>
Date:   Tue, 31 Mar 2020 13:36:04 -0700
Message-ID: <CABEDWGzfTDtmq==j-GcK3YYbdPX4-Ms=PDuDEiQusV78bUGvDA@mail.gmail.com>
Subject: Re: [PATCH] PCI: Warn about MEM resource size being too big
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     linux-kernel@vger.kernel.org,
        linux-pci <linux-pci@vger.kernel.org>,
        Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        lorenzo.pieralisi@arm.com, amurray@thegoodpenguin.co.uk,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Paul Walmsley <paul.walmsley@sifive.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Mar 31, 2020 at 1:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> $ git log --oneline drivers/pci/controller/dwc/pcie-designware-host.c
> 7fe71aa84b43 ("PCI: dwc: Use pci_parse_request_of_pci_ranges()")
> 1137e61dcb99 ("PCI: dwc: Fix find_next_bit() usage")
> 0b24134f7888 ("PCI: dwc: Add validation that PCIe core is set to correct mode")
> 3924bc2fd1b6 ("PCI: dwc: Group DBI registers writes requiring unlocking")
> ca98329d3b58 ("PCI: dwc: Export APIs to support .remove() implementation")
> 9d071cade30a ("PCI: dwc: Add API support to de-initialize host")
> fe23274f72f4 ("PCI: dwc: Save root bus for driver remove hooks")
>
> Please make yours match.  Please mention something about the 32-bit
> limit instead of just "being too big".
>
> Wrap the commit log to 75 columns to be consistent with the rest of
> the history.
>

Thanks Bjorn for your comments. Will correct in v2.

> 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.
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.

>
> > This limitation also means that multiple ATUs
> > would need to be used to map larger regions.
> >
> > Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
> > ---
> >  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
> >