[PATCH v3 0/3] PCI endpoint 64-bit BAR fixes

[PATCH v3 0/3] PCI endpoint 64-bit BAR fixes

[Niklas Cassel]

PCI endpoint fixes to improve the way 64-bit BARs are handled.


There are still future improvements that could be made:

pci-epf-test.c always allocates space for
6 BARs, even when using 64-bit BARs (which
really only requires us to allocate 3 BARs).

pcitest.sh will print "NOT OKAY" for BAR1,
BAR3, and BAR5 when using 64-bit BARs.
This could probably be improved to say
something like "N/A (64-bit BAR)".

Niklas Cassel (3):
  PCI: endpoint: Handle 64-bit BARs properly
  misc: pci_endpoint_test: Handle 64-bit BARs properly
  PCI: designware-ep: Return an error when requesting a too large BAR
    size

 drivers/misc/pci_endpoint_test.c              | 12 +++++++-----
 drivers/pci/dwc/pcie-designware-ep.c          |  5 +++++
 drivers/pci/endpoint/functions/pci-epf-test.c |  2 ++
 3 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.14.2

[PATCH v3 1/3] PCI: endpoint: Handle 64-bit BARs properly

[Niklas Cassel]

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,
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@axis.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

[PATCH v3 2/3] misc: pci_endpoint_test: Handle 64-bit BARs properly

[Niklas Cassel]

A 64-bit BAR uses the succeeding BAR for the upper bits,
so we cannot simply call pci_ioremap_bar() on every single BAR.

BARs succeding a 64-bit BAR will not have the IORESOURCE_MEM resource
flag set. Only call ioremap on BARs that have the IORESOURCE_MEM
resource flag set.

pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit]
pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit]
pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit]
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR1
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR3
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR5

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
Changes since v2:
Check (pci_resource_flags(pdev, bar) & IORESOURCE_MEM)
rather than (pci_resource_len(pdev, bar) == 0), thanks Bjorn.

 drivers/misc/pci_endpoint_test.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 320276f42653..fe8897e64635 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -534,12 +534,14 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	}
 
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
-		base = pci_ioremap_bar(pdev, bar);
-		if (!base) {
-			dev_err(dev, "failed to read BAR%d\n", bar);
-			WARN_ON(bar == test_reg_bar);
+		if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
+			base = pci_ioremap_bar(pdev, bar);
+			if (!base) {
+				dev_err(dev, "failed to read BAR%d\n", bar);
+				WARN_ON(bar == test_reg_bar);
+			}
+			test->bar[bar] = base;
 		}
-		test->bar[bar] = base;
 	}
 
 	test->base = test->bar[test_reg_bar];
-- 
2.14.2

[PATCH v3 3/3] PCI: designware-ep: Return an error when requesting a too large BAR size

[Niklas Cassel]

pci_epc_set_bar() can be called with flag PCI_BASE_ADDRESS_MEM_TYPE_64,
and can thus request a BAR size larger than 4 GB.

However, the pcie-designware-ep.c driver currently doesn't handle
BAR sizes larger than 4 GB. (Since we are only writing the BAR_mask[x]
register and not the BAR_mask[x+1] register.)

For now, return an error when requesting a BAR size larger than 4 GB.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-designware-ep.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 3a6feeff5f5b..efb65a7c06b8 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -126,6 +126,11 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
 	enum dw_pcie_as_type as_type;
 	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
 
+	if (upper_32_bits(size)) {
+		dev_err(pci->dev, "can't handle BAR larger than 4GB\n");
+		return -EINVAL;
+	}
+
 	if (!(flags & PCI_BASE_ADDRESS_SPACE))
 		as_type = DW_PCIE_AS_MEM;
 	else
-- 
2.14.2

Re: [PATCH v3 1/3] PCI: endpoint: Handle 64-bit BARs properly

[Lorenzo Pieralisi]

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.

How is (in HW I mean) actually the BAR size configured in a given EPF ?

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

Re: [PATCH v3 1/3] PCI: endpoint: Handle 64-bit BARs properly

[Niklas Cassel]

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?


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


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

Re: [PATCH v3 1/3] PCI: endpoint: Handle 64-bit BARs properly

[Lorenzo Pieralisi]

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