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              | 2 ++
 drivers/pci/dwc/pcie-designware-ep.c          | 5 +++++
 drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++
 3 files changed, 9 insertions(+)

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 800da09d9005..eef85820f59e 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

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

Ignore BARs that does not have a valid resource length.

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

---
Lorenzo/Bjorn: pci_resource_len() seems to fix my problem,
but is it the correct function to use here?
If BAR[x] is a 64-bit BAR, I'm assuming that pci_resource_len() on BAR[x+1] will always return 0 (since BAR[x+1] cannot have any prefetchable/type bits when BAR[x] is 64-bit).

 drivers/misc/pci_endpoint_test.c | 2 ++
 1 file changed, 2 insertions(+)

index 320276f42653..3af31bfdcfdd 100644
@@ -534,6 +534,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	}
 
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
+		if (pci_resource_len(pdev, bar) == 0)
+			continue;
 		base = pci_ioremap_bar(pdev, bar);
 		if (!base) {
 			dev_err(dev, "failed to read BAR%d\n", 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..4a0085ead1e3 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 (size > 0x100000000ULL) {
+		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

On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <[email protected]> wrote:

include/linux/sizes.h:

+SZ_4G 0x100000000ULL

#include <linux/sizes.h>

if (size > SZ_4G) {

?

With Best Regards,
Andy Shevchenko

I like this one for the readability.
Thank you.

Best regards,
Jingoo Han

I liked it too, however both variants

if (size > 0x100000000ULL) {

if (size > SZ_4G) {

result in:

drivers/pci/dwc/pcie-designware-ep.c:131:11: warning:
comparison is always false due to limited range of data type [-Wtype-limits]

when compiling with W=1 on a platform with 32-bit size_t.

The annoying thing here is that a BAR can be 64-bit, yet the parameter size is defined as a size_t, so the error will only show on 32-bit and not on 64-bit.

What do you think about:

if (upper_32_bits(size)) {
	dev_err(pci->dev, "can't handle BAR larger than 4GB\n");
	return -EINVAL;
}

That should compile without warnings for both 32-bit size_t and 64-bit size_t.

Regards,
Niklas

Oh, indeed. And it looks moving to u64 or alike is not a solution (because if would not describe real hardware in that case).

Can you derive some helper based on the code in __pci_read_base() code?

With Best Regards,
Andy Shevchenko

Hello Andy,

I guess that would be possible, however I think that simply checking upper_32_bits(size) is simpler.

If someone is ever to fix dw_pcie_ep_set_bar() so that it works with 64-bit BARs, the function will probably need to check upper_32_bits(size) anyway.

Regards,
Niklas