PCI endpoint 64-bit BAR fixes
Subsystems (3)
v2 cover 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(+)
Signature · 1 line
2.14.2
PATCH v2 1/3 PCI: endpoint: Handle 64-bit BARs properly
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(+)
drivers/pci/endpoint/functions/pci-epf-test.c +2 -0
@@ -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 ;
Signature · 1 line
2.14.2
Re: [PATCH v2 1/3] PCI: endpoint: Handle 64-bit BARs properly
Hi,
Quoted 4 lines · 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 flagPCI_BASE_ADDRESS_MEM_TYPE_64,
Not related to $patch. But I have a query on when PCI_BASE_ADDRESS_MEM_TYPE_64 should be set. Whether if the size is > 4G or if the address can be mapped anywhere in the 64-bit PCIe address space or both?
Thanks
Kishon
Quoted 22 lines
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 <[email protected]>--- drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++ 1 file changed, 2 insertions(+)drivers/pci/endpoint/functions/pci-epf-test.c +2
-0@@ -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 ;
Re: [PATCH v2 1/3] PCI: endpoint: Handle 64-bit BARs properly
Quoted 11 lines · Kishon Vijay Abraham I
Hi,
Quoted 4 lines · 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 flagPCI_BASE_ADDRESS_MEM_TYPE_64,Not related to $patch. But I have a query on when
PCI_BASE_ADDRESS_MEM_TYPE_64should be set. Whether if the size is > 4G or if the address can be mapped anywhere in the 64-bit PCIe address space or both?
Hello Kishon,
Since 32-bit BARs work fine on 64-bit CPUs,
and since 64-bit BARs work fine on 32 bit CPUs
(as long as we assign them an address <4G,
and their (combined) size is not too big),
perhaps the best way would be if pci-epf-test always defaults to 32-bit BARs, and a module parameter says if we should test 64-bit BARs.
Just because a 64-bit BAR can be assigned an address >4G,
and have a size >4G, doesn't mean that we have to give it those properties.
This way we can have some testing of 64-bit BARs on 32-bit CPUs, even though more extensive testing (e.g. having a BAR with a size >4G) would require a 64-bit CPU.
Regards,
Niklas
Quoted 25 lines
Thanks
KishonQuoted 22 lines
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 <[email protected]>--- drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++ 1 file changed, 2 insertions(+)drivers/pci/endpoint/functions/pci-epf-test.c +2
-0@@ -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 ;
Re: [PATCH v2 1/3] PCI: endpoint: Handle 64-bit BARs properly
In reply to Kishon Vijay Abraham IQuoted 6 lines · Kishon Vijay Abraham I
Hi,
Quoted 3 lines · 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.
Quoted 6 lines
Quoted 1 line
If
pci_epc_set_bar()is called with flagPCI_BASE_ADDRESS_MEM_TYPE_64,Not related to $patch. But I have a query on when
PCI_BASE_ADDRESS_MEM_TYPE_64should be set. Whether if the size is > 4G or if the address can be mapped anywhere in the 64-bit PCIe address space or both?
In general, PCI_BASE_ADDRESS_MEM_TYPE_64 should be set if the BAR is 64 bits wide. IORESOURCE_MEM_64 is similar.
It doesn't matter what the current value of the BAR is.
It's easy to look at the current address or size of a resource if we need to know where it is or how big it is. But if we lose track of the width of the BAR register, we have no way to know whether it's *capable* of holding a 64-bit value.
Quoted 22 lines
Quoted 22 lines
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 <[email protected]>--- drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++ 1 file changed, 2 insertions(+)drivers/pci/endpoint/functions/pci-epf-test.c +2
-0@@ -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 ;
Re: [PATCH v2 1/3] PCI: endpoint: Handle 64-bit BARs properly
Hi Bjorn,
Quoted 9 lines · Bjorn Helgaas
Quoted 8 lines · Kishon Vijay Abraham I
Hi,
Quoted 4 lines · 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 flagPCI_BASE_ADDRESS_MEM_TYPE_64,
Quoted 7 lines
Quoted 4 lines
Not related to $patch. But I have a query on when
PCI_BASE_ADDRESS_MEM_TYPE_64should be set. Whether if the size is > 4G or if the address can be mapped anywhere in the 64-bit PCIe address space or both?In general,
PCI_BASE_ADDRESS_MEM_TYPE_64should be set if the BAR is 64 bits wide.IORESOURCE_MEM_64is similar.
okay, if the HW support 64bit BAR, 64 bit flag should be set and not based on size or anything else?
Thanks
Kishon
Re: [PATCH v2 1/3] PCI: endpoint: Handle 64-bit BARs properly
Quoted 17 lines · Kishon Vijay Abraham I
Hi Bjorn,
Quoted 14 lines · Bjorn Helgaas
Quoted 12 lines · Kishon Vijay Abraham I
Hi,
Quoted 4 lines · 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 flagPCI_BASE_ADDRESS_MEM_TYPE_64,Not related to $patch. But I have a query on when
PCI_BASE_ADDRESS_MEM_TYPE_64should be set. Whether if the size is > 4G or if the address can be mapped anywhere in the 64-bit PCIe address space or both?
Quoted 5 lines
Quoted 2 lines
In general,
PCI_BASE_ADDRESS_MEM_TYPE_64should be set if the BAR is 64 bits wide.IORESOURCE_MEM_64is similar.okay, if the HW support 64bit BAR, 64 bit flag should be set and not based on size or anything else?
Yes, I completely agree with Bjorn. Actually it would be a good idea to make the struct pci_epf->bar member array an array of struct resources to simplify its handling.
Thanks,
Lorenzo
PATCH v2 2/3 misc: pci_endpoint_test: Handle 64-bit BARs properly
In reply to Niklas CasselA 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(+)
drivers/misc/pci_endpoint_test.c +2 -0
@@ -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);
Signature · 1 line
2.14.2
Re: [PATCH v2 2/3] misc: pci_endpoint_test: Handle 64-bit BARs properly
Quoted 31 lines · 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.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: [???
0x00000000flags0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR1 pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [???0x00000000flags0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR3 pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [???0x00000000flags0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR5Signed-off-by Niklas Cassel <[email protected]>---
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 thatpci_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(+)drivers/misc/pci_endpoint_test.c +0
-0
drivers/misc/pci_endpoint_test.c static int pci_endpoint_test_probe(struct pci_dev *pdev, · line 538
Quoted 5 lines
drivers/misc/pci_endpoint_test.c +2
-0@@ -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 ;
Should not it be handled by checking the resource flags as you loop through the bar counter and incrementing the bar counter (+1) if IORESOURCE_MEM_64 is detected ?
I would like to get Bjorn's point of view on this since it is definitely more comprehensive than mine.
Thanks,
Lorenzo
Quoted 6 lines
drivers/misc/pci_endpoint_test.c +0
-0base =pci_ioremap_bar (pdev, bar);if (!base) {dev_err (dev,"failed to read BAR%d\n" , bar);Signature · 1 line
2.14.2
Re: [PATCH v2 2/3] misc: pci_endpoint_test: Handle 64-bit BARs properly
Quoted 34 lines · Lorenzo Pieralisi
Quoted 33 lines · 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.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: [???
0x00000000flags0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR1 pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [???0x00000000flags0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR3 pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [???0x00000000flags0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR5Signed-off-by Niklas Cassel <[email protected]>---
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 thatpci_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(+)drivers/misc/pci_endpoint_test.c +0
-0@@ -534,6 +534,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, }
Quoted 7 lines
drivers/misc/pci_endpoint_test.c static int pci_endpoint_test_probe(struct pci_dev *pdev, · line 538
Quoted 3 lines
drivers/misc/pci_endpoint_test.c +2
-0for (bar =BAR_0 ; bar <=BAR_5 ; bar++) { +if (pci_resource_len (pdev, bar) ==0 ) +continue ;Should not it be handled by checking the resource flags as you loop through the bar counter and incrementing the bar counter (+1) if
IORESOURCE_MEM_64is detected ?
I agree, pci_resource_len() is the wrong thing here. The length (actually the entire resource[x]) *should* be zero if the slot corresponds to the upper bits of a 64-bit BAR, but I think it would be more natural to do this:
if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM)
base = pci_ioremap_bar(pdev, bar);You *could* check for IORESOURCE_MEM_64 and increment "bar" if you find it, but I don't think that's really idiomatic, and it builds in a little bit of unnecessary knowledge about how the PCI core maps BAR registers to the resource[] array.
Quoted 6 lines
Quoted 6 lines
drivers/misc/pci_endpoint_test.c +0
-0base =pci_ioremap_bar (pdev, bar);if (!base) {dev_err (dev,"failed to read BAR%d\n" , bar);Signature · 1 line
2.14.2
PATCH v2 3/3 PCI: designware-ep: Return an error when requesting a too large BAR size
In reply to Niklas Casselpci_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.
---
Changes since v1:
Use upper_32_bits() to avoid build warning on systems with 32-bit size_t.
drivers/pci/dwc/pcie-designware-ep.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/pci/dwc/pcie-designware-ep.c +5 -0
@@ -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
Signature · 1 line
2.14.2