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

[PATCH v2 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              | 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

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

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

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
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(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 320276f42653..3af31bfdcfdd 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -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

[PATCH v2 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>
---
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(+)

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 v2 1/3] PCI: endpoint: Handle 64-bit BARs properly

[Kishon Vijay Abraham I]

Hi,

On Thursday 08 February 2018 06:03 PM, 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,

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

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

[Niklas Cassel]

On Thu, Feb 08, 2018 at 06:17:32PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Thursday 08 February 2018 06:03 PM, 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,
> 
> 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?

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

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

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

[Bjorn Helgaas]

On Thu, Feb 08, 2018 at 06:17:32PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Thursday 08 February 2018 06:03 PM, 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,
> 
> 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?

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.

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

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

[Kishon Vijay Abraham I]

Hi Bjorn,

On Friday 09 February 2018 03:27 AM, Bjorn Helgaas wrote:
> On Thu, Feb 08, 2018 at 06:17:32PM +0530, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 08 February 2018 06:03 PM, 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,
>>
>> 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?
> 
> In general, PCI_BASE_ADDRESS_MEM_TYPE_64 should be set if the BAR is
> 64 bits wide.  IORESOURCE_MEM_64 is 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

[Lorenzo Pieralisi]

On Fri, Feb 09, 2018 at 06:14:49PM +0530, Kishon Vijay Abraham I wrote:
> Hi Bjorn,
> 
> On Friday 09 February 2018 03:27 AM, Bjorn Helgaas wrote:
> > On Thu, Feb 08, 2018 at 06:17:32PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi,
> >>
> >> On Thursday 08 February 2018 06:03 PM, 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,
> >>
> >> 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?
> > 
> > In general, PCI_BASE_ADDRESS_MEM_TYPE_64 should be set if the BAR is
> > 64 bits wide.  IORESOURCE_MEM_64 is 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

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

[Lorenzo Pieralisi]

On Thu, Feb 08, 2018 at 01:33:45PM +0100, Niklas Cassel wrote:
> 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
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
> 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(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 320276f42653..3af31bfdcfdd 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -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

>  		base = pci_ioremap_bar(pdev, bar);
>  		if (!base) {
>  			dev_err(dev, "failed to read BAR%d\n", bar);
> -- 
> 2.14.2
> 

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

[Bjorn Helgaas]

On Mon, Feb 26, 2018 at 05:26:18PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Feb 08, 2018 at 01:33:45PM +0100, Niklas Cassel wrote:
> > 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
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> > ---
> > 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(+)
> > 
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index 320276f42653..3af31bfdcfdd 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -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 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.

> >  		base = pci_ioremap_bar(pdev, bar);
> >  		if (!base) {
> >  			dev_err(dev, "failed to read BAR%d\n", bar);
> > -- 
> > 2.14.2
> >