| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20180305161928.GA30407@e107981-ln.cambridge.arm.com>
Date: Mon, 5 Mar 2018 16:19:28 +0000
From: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To: Niklas Cassel <niklas.cassel@...s.com>
Cc: kishon@...com, Bjorn Helgaas <bhelgaas@...gle.com>,
Sekhar Nori <nsekhar@...com>,
Shawn Lin <shawn.lin@...k-chips.com>,
Cyrille Pitchen <cyrille.pitchen@...e-electrons.com>,
John Keeping <john@...anate.com>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/3] PCI: endpoint: Handle 64-bit BARs properly
On Thu, Mar 01, 2018 at 03:40:30PM +0100, Niklas Cassel wrote:
> On Wed, Feb 28, 2018 at 02:21:48PM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Feb 27, 2018 at 12:59:05PM +0100, Niklas Cassel wrote:
> > > A 64-bit BAR uses the succeeding BAR for the upper bits, therefore
> > > we cannot call pci_epc_set_bar() on a BAR that follows a 64-bit BAR.
> > >
> > > If pci_epc_set_bar() is called with flag PCI_BASE_ADDRESS_MEM_TYPE_64,
> >
> > PCI_BASE_ADDRESS_MEM_TYPE_64 is detected through a sizeof(dma_addr_t).
> >
> > I thought we decided to describe the BARs not as dma_addr_t + size but
> > as resources, which would allow you to have flags describing their
> > actual size.
> >
> > Current code has a fixed BAR size defined by the dma_addr_t type size
> > and I do not think that's what we really want.
>
> You suggested to Kishon that the bar member array should be refactored
> to be an array of struct resources:
>
> https://marc.info/?l=linux-pci&m=151851776921594&w=2
>
> That refactoring would be a good thing,
> but can't it be done after the merge of this patch?
This patch does not look right to me - a BAR size should not be
determined by sizeof(dma_addr_t) and on top of that this code is not
easy to comprehend.
> I'm guessing that one of the reasons why you want this
> refactoring is so that you can actually call pci_epc_set_bar()
> on each array member, so that you don't need my patch:
> > > + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> > > + bar++;
>
> However, if we want to omit this, I think that other changes would be needed.
>
> Let's say that resource[x] is 64-bit, then resource[x+1] must not have
> (IORESOURCE_MEM or IORESOURCE_IO) set.
>
> It is important that BAR[x+1] == 0, for a 64-bit BAR.
>
> So either pci_epc_set_bar() or epc->ops->set_bar()
> must know to not touch its BAR (or BAR mask) register
> if neither of those flags are set.
>
> It's probably easier to change pci_epc_set_bar(), rather than
> changing all epc->ops->set_bar() implementations,
> here is e.g. part of set_bar() for DesignWare:
>
> static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
> enum pci_barno bar,
> dma_addr_t bar_phys, size_t size, int flags)
> {
> int ret;
> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> enum dw_pcie_as_type as_type;
> u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>
> if (!(flags & PCI_BASE_ADDRESS_SPACE))
> as_type = DW_PCIE_AS_MEM;
> else
> as_type = DW_PCIE_AS_IO;
>
>
> So I still see a point in having this patch, at least until
> someone has refactored the code to use resource + modified pci_epc_set_bar().
But the point is that, IIUC (because again - this code path is really
confusing) it is up to the specific implementation to turn a struct
pci_epf_bar into a *real* BAR and that's implementation specific
(even though there must be a 1:1 relationship between
struct pci_epf_bar.size and the actual BAR size in the device, ie
if struct pci_epf_bar.size exceeds 32-bit space we need a 64-bit
BAR to represent it).
I assume the problem here is having a 1:1 index mapping between the
struct pci_epf.bar[6] array and the BAR index programmed into the
device. If that's the case - I see two options:
- Use struct epf_bar.size to drive the reg increment (ie reg++ if
size > 4G)
- Add a return value to struct pci_epc.ops.set_bar() that returns
whether a 64-bit BAR was set-up (which is the same __pci_read_base()
does in PCI core)
Please let me know if I have not got this right so that we can make
progress.
Thanks,
Lorenzo
> > How is (in HW I mean) actually the BAR size configured in a given EPF ?
>
> For the DesignWare PCIe controller (assuming it has been synthesized with
> Programmable Masks), to configure a BAR, you set the usual prefetch+type bits
> in the standard PCI BAR register, but then the controller also has, for each
> bar, a special BAR mask register, which dw_pcie_ep_set_bar() sets to (size-1),
> this defines which bits that will be writable by the RC (and the RC can figure
> out the size of the BAR by writing all 1's as usual).
>
> For a 64-bit Memory Space BAR of size 16 GiB (prefetchable), you would set:
> BAR[x] = 0000 0000 0000 0000 0000 0000 0000 1100
> BAR[x+1] = 0000 0000 0000 0000 0000 0000 0000 0000
>
> BAR_MASK[x] = 1111 1111 1111 1111 1111 1111 1111 1111
> BAR_MASK[x+1] = 0000 0000 0000 0000 0000 0000 0000 0011
>
>
>
> I guess that instead of patch 3/3 in this patch series,
> we could do:
>
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -136,8 +136,15 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
> return ret;
>
> dw_pcie_dbi_ro_wr_en(pci);
> - dw_pcie_writel_dbi2(pci, reg, size - 1);
> - dw_pcie_writel_dbi(pci, reg, flags);
> + if (upper_32_bits(size)) {
> + dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
> + dw_pcie_writel_dbi(pci, reg, flags);
> + dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
> + dw_pcie_writel_dbi(pci, reg + 4, 0);
> + } else {
> + dw_pcie_writel_dbi2(pci, reg, size - 1);
> + dw_pcie_writel_dbi(pci, reg, flags);
> + }
> dw_pcie_dbi_ro_wr_dis(pci);
>
> return 0;
>
> However, since I don't have access to a 64-bit CPU with loads of RAM,
> that has the DesignWare PCIe controller, that patch is completely untested.
> But if everyone agrees, we could replace 3/3 of this series with that.
>
>
> >
> > Thanks,
> > Lorenzo
> >
> > > it has to be up to the controller driver to write both BAR[x] and BAR[x+1]
> > > (and BAR_mask[x] and BAR_mask[x+1]).
> > >
> > > Signed-off-by: Niklas Cassel <niklas.cassel@...s.com>
> > > ---
> > > drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > index 64d8a17f8094..ecbf6a7750dc 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > @@ -382,6 +382,8 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> > > if (bar == test_reg_bar)
> > > return ret;
> > > }
> > > + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> > > + bar++;
> > > }
> > >
> > > return 0;
> > > --
> > > 2.14.2
> > >
Powered by blists - more mailing lists