PCI endpoint 64-bit BAR fixes

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

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

---
 drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++
 1 file changed, 2 insertions(+)

index 64d8a17f8094..ecbf6a7750dc 100644
@@ -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

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

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:

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

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:

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

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.

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

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

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

index 320276f42653..fe8897e64635 100644
@@ -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

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.

---
 drivers/pci/dwc/pcie-designware-ep.c | 5 +++++
 1 file changed, 5 insertions(+)

index 3a6feeff5f5b..efb65a7c06b8 100644
@@ -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