[PATCH 1/3] PCI: Introduce pci_bus_addr_t

[PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Yinghai Lu]

David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
to fit in upstream windows") broke booting on sparc/T5-8.

In the boot log, there is
  pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
  0x110204000)
but that only could happen when dma_addr_t is 32-bit.

According to David Miller, all DMA occurs behind an IOMMU and these
IOMMUs only support 32-bit addressing, therefore dma_addr_t is
32-bit on sparc64.

Let's introduce pci_bus_addr_t instead of using dma_addr_t,
and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.

Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
Fixes: commit 23b13bc76f35 ("PCI: Fail safely if we can't handle BARs larger than 4GB")
Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
Reported-by: David Ahern <david.ahern@oracle.com>
Tested-by: David Ahern <david.ahern@oracle.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: <stable@vger.kernel.org> #3.19
---
 drivers/pci/Kconfig |  4 ++++
 drivers/pci/bus.c   | 10 +++++-----
 drivers/pci/probe.c | 12 ++++++------
 include/linux/pci.h | 12 +++++++++---
 4 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 7a8f1c5..6a5a269 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -1,6 +1,10 @@
 #
 # PCI configuration
 #
+config PCI_BUS_ADDR_T_64BIT
+	def_bool y if (64BIT || X86_PAE)
+	depends on PCI
+
 config PCI_MSI
 	bool "Message Signaled Interrupts (MSI and MSI-X)"
 	depends on PCI
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 90fa3a7..6fbd3f2 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -92,11 +92,11 @@ void pci_bus_remove_resources(struct pci_bus *bus)
 }
 
 static struct pci_bus_region pci_32_bit = {0, 0xffffffffULL};
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
 static struct pci_bus_region pci_64_bit = {0,
-				(dma_addr_t) 0xffffffffffffffffULL};
-static struct pci_bus_region pci_high = {(dma_addr_t) 0x100000000ULL,
-				(dma_addr_t) 0xffffffffffffffffULL};
+				(pci_bus_addr_t) 0xffffffffffffffffULL};
+static struct pci_bus_region pci_high = {(pci_bus_addr_t) 0x100000000ULL,
+				(pci_bus_addr_t) 0xffffffffffffffffULL};
 #endif
 
 /*
@@ -200,7 +200,7 @@ int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 					  resource_size_t),
 		void *alignf_data)
 {
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
 	int rc;
 
 	if (res->flags & IORESOURCE_MEM_64) {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8d2f400..f71cb7c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -253,8 +253,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 	}
 
 	if (res->flags & IORESOURCE_MEM_64) {
-		if ((sizeof(dma_addr_t) < 8 || sizeof(resource_size_t) < 8) &&
-		    sz64 > 0x100000000ULL) {
+		if ((sizeof(pci_bus_addr_t) < 8 || sizeof(resource_size_t) < 8)
+		    && sz64 > 0x100000000ULL) {
 			res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
 			res->start = 0;
 			res->end = 0;
@@ -263,7 +263,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			goto out;
 		}
 
-		if ((sizeof(dma_addr_t) < 8) && l) {
+		if ((sizeof(pci_bus_addr_t) < 8) && l) {
 			/* Above 32-bit boundary; try to reallocate */
 			res->flags |= IORESOURCE_UNSET;
 			res->start = 0;
@@ -398,7 +398,7 @@ static void pci_read_bridge_mmio_pref(struct pci_bus *child)
 	struct pci_dev *dev = child->self;
 	u16 mem_base_lo, mem_limit_lo;
 	u64 base64, limit64;
-	dma_addr_t base, limit;
+	pci_bus_addr_t base, limit;
 	struct pci_bus_region region;
 	struct resource *res;
 
@@ -425,8 +425,8 @@ static void pci_read_bridge_mmio_pref(struct pci_bus *child)
 		}
 	}
 
-	base = (dma_addr_t) base64;
-	limit = (dma_addr_t) limit64;
+	base = (pci_bus_addr_t) base64;
+	limit = (pci_bus_addr_t) limit64;
 
 	if (base != base64) {
 		dev_err(&dev->dev, "can't handle bridge window above 4GB (bus address %#010llx)\n",
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 211e9da..6021bbe 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -573,9 +573,15 @@ int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
 int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
 		  int reg, int len, u32 val);
 
+#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
+typedef u64 pci_bus_addr_t;
+#else
+typedef u32 pci_bus_addr_t;
+#endif
+
 struct pci_bus_region {
-	dma_addr_t start;
-	dma_addr_t end;
+	pci_bus_addr_t start;
+	pci_bus_addr_t end;
 };
 
 struct pci_dynids {
@@ -1124,7 +1130,7 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
 
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
 
-static inline dma_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
+static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
 {
 	struct pci_bus_region region;
 
-- 
1.8.4.5

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Bjorn Helgaas]

On Tue, Mar 31, 2015 at 07:57:47PM -0700, Yinghai Lu wrote:
> David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
> to fit in upstream windows") broke booting on sparc/T5-8.
> 
> In the boot log, there is
>   pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
>   0x110204000)
> but that only could happen when dma_addr_t is 32-bit.
> 
> According to David Miller, all DMA occurs behind an IOMMU and these
> IOMMUs only support 32-bit addressing, therefore dma_addr_t is
> 32-bit on sparc64.
> 
> Let's introduce pci_bus_addr_t instead of using dma_addr_t,
> and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.

I propose the following doc updates (I can just fold them into this patch
if you approve):


commit 13fca18e2f1d9dd078b8dfea965718cf13a4b600
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Fri Apr 3 12:39:35 2015 -0500

    pci-bus-addr-fixups

diff --git a/Documentation/DMA-API-HOWTO.txt b/Documentation/DMA-API-HOWTO.txt
index 0f7afb2bb442..aef8cc5a677b 100644
--- a/Documentation/DMA-API-HOWTO.txt
+++ b/Documentation/DMA-API-HOWTO.txt
@@ -25,13 +25,18 @@ physical addresses.  These are the addresses in /proc/iomem.  The physical
 address is not directly useful to a driver; it must use ioremap() to map
 the space and produce a virtual address.
 
-I/O devices use a third kind of address: a "bus address" or "DMA address".
-If a device has registers at an MMIO address, or if it performs DMA to read
-or write system memory, the addresses used by the device are bus addresses.
-In some systems, bus addresses are identical to CPU physical addresses, but
-in general they are not.  IOMMUs and host bridges can produce arbitrary
+I/O devices use a third kind of address: a "bus address".  If a device has
+registers at an MMIO address, or if it performs DMA to read or write system
+memory, the addresses used by the device are bus addresses.  In some
+systems, bus addresses are identical to CPU physical addresses, but in
+general they are not.  IOMMUs and host bridges can produce arbitrary
 mappings between physical and bus addresses.
 
+From a device's point of view, DMA uses the bus address space, but it may
+be restricted to a subset of that space.  For example, even if a system
+supports 64-bit addresses for main memory and PCI BARs, it may use an IOMMU
+so devices only need to use 32-bit DMA addresses.
+
 Here's a picture and some examples:
 
                CPU                  CPU                  Bus
@@ -72,11 +77,11 @@ can use virtual address X to access the buffer, but the device itself
 cannot because DMA doesn't go through the CPU virtual memory system.
 
 In some simple systems, the device can do DMA directly to physical address
-Y.  But in many others, there is IOMMU hardware that translates bus
+Y.  But in many others, there is IOMMU hardware that translates DMA
 addresses to physical addresses, e.g., it translates Z to Y.  This is part
 of the reason for the DMA API: the driver can give a virtual address X to
 an interface like dma_map_single(), which sets up any required IOMMU
-mapping and returns the bus address Z.  The driver then tells the device to
+mapping and returns the DMA address Z.  The driver then tells the device to
 do DMA to Z, and the IOMMU maps it to the buffer at address Y in system
 RAM.
 
@@ -98,7 +103,7 @@ First of all, you should make sure
 #include <linux/dma-mapping.h>
 
 is in your driver, which provides the definition of dma_addr_t.  This type
-can hold any valid DMA or bus address for the platform and should be used
+can hold any valid DMA address for the platform and should be used
 everywhere you hold a DMA address returned from the DMA mapping functions.
 
 			 What memory is DMA'able?
@@ -316,7 +321,7 @@ There are two types of DMA mappings:
   Think of "consistent" as "synchronous" or "coherent".
 
   The current default is to return consistent memory in the low 32
-  bits of the bus space.  However, for future compatibility you should
+  bits of the DMA space.  However, for future compatibility you should
   set the consistent mask even if this default is fine for your
   driver.
 
@@ -403,7 +408,7 @@ dma_alloc_coherent() returns two values: the virtual address which you
 can use to access it from the CPU and dma_handle which you pass to the
 card.
 
-The CPU virtual address and the DMA bus address are both
+The CPU virtual address and the DMA address are both
 guaranteed to be aligned to the smallest PAGE_SIZE order which
 is greater than or equal to the requested size.  This invariant
 exists (for example) to guarantee that if you allocate a chunk
@@ -645,8 +650,8 @@ PLEASE NOTE:  The 'nents' argument to the dma_unmap_sg call must be
               dma_map_sg call.
 
 Every dma_map_{single,sg}() call should have its dma_unmap_{single,sg}()
-counterpart, because the bus address space is a shared resource and
-you could render the machine unusable by consuming all bus addresses.
+counterpart, because the DMA address space is a shared resource and
+you could render the machine unusable by consuming all DMA addresses.
 
 If you need to use the same streaming DMA region multiple times and touch
 the data in between the DMA transfers, the buffer needs to be synced
diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 52088408668a..7eba542eff7c 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -18,10 +18,10 @@ Part I - dma_ API
 To get the dma_ API, you must #include <linux/dma-mapping.h>.  This
 provides dma_addr_t and the interfaces described below.
 
-A dma_addr_t can hold any valid DMA or bus address for the platform.  It
-can be given to a device to use as a DMA source or target.  A CPU cannot
-reference a dma_addr_t directly because there may be translation between
-its physical address space and the bus address space.
+A dma_addr_t can hold any valid DMA address for the platform.  It can be
+given to a device to use as a DMA source or target.  A CPU cannot reference
+a dma_addr_t directly because there may be translation between its physical
+address space and the DMA address space.
 
 Part Ia - Using large DMA-coherent buffers
 ------------------------------------------
@@ -42,7 +42,7 @@ It returns a pointer to the allocated region (in the processor's virtual
 address space) or NULL if the allocation failed.
 
 It also returns a <dma_handle> which may be cast to an unsigned integer the
-same width as the bus and given to the device as the bus address base of
+same width as the bus and given to the device as the DMA address base of
 the region.
 
 Note: consistent memory can be expensive on some platforms, and the
@@ -193,7 +193,7 @@ dma_map_single(struct device *dev, void *cpu_addr, size_t size,
 		      enum dma_data_direction direction)
 
 Maps a piece of processor virtual memory so it can be accessed by the
-device and returns the bus address of the memory.
+device and returns the DMA address of the memory.
 
 The direction for both APIs may be converted freely by casting.
 However the dma_ API uses a strongly typed enumerator for its
@@ -212,20 +212,20 @@ contiguous piece of memory.  For this reason, memory to be mapped by
 this API should be obtained from sources which guarantee it to be
 physically contiguous (like kmalloc).
 
-Further, the bus address of the memory must be within the
+Further, the DMA address of the memory must be within the
 dma_mask of the device (the dma_mask is a bit mask of the
-addressable region for the device, i.e., if the bus address of
-the memory ANDed with the dma_mask is still equal to the bus
+addressable region for the device, i.e., if the DMA address of
+the memory ANDed with the dma_mask is still equal to the DMA
 address, then the device can perform DMA to the memory).  To
 ensure that the memory allocated by kmalloc is within the dma_mask,
 the driver may specify various platform-dependent flags to restrict
-the bus address range of the allocation (e.g., on x86, GFP_DMA
-guarantees to be within the first 16MB of available bus addresses,
+the DMA address range of the allocation (e.g., on x86, GFP_DMA
+guarantees to be within the first 16MB of available DMA addresses,
 as required by ISA devices).
 
 Note also that the above constraints on physical contiguity and
 dma_mask may not apply if the platform has an IOMMU (a device which
-maps an I/O bus address to a physical memory address).  However, to be
+maps an I/O DMA address to a physical memory address).  However, to be
 portable, device driver writers may *not* assume that such an IOMMU
 exists.
 
@@ -296,7 +296,7 @@ reduce current DMA mapping usage or delay and try again later).
 	dma_map_sg(struct device *dev, struct scatterlist *sg,
 		int nents, enum dma_data_direction direction)
 
-Returns: the number of bus address segments mapped (this may be shorter
+Returns: the number of DMA address segments mapped (this may be shorter
 than <nents> passed in if some elements of the scatter/gather list are
 physically or virtually adjacent and an IOMMU maps them with a single
 entry).
@@ -340,7 +340,7 @@ must be the same as those and passed in to the scatter/gather mapping
 API.
 
 Note: <nents> must be the number you passed in, *not* the number of
-bus address entries returned.
+DMA address entries returned.
 
 void
 dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
@@ -507,7 +507,7 @@ it's asked for coherent memory for this device.
 phys_addr is the CPU physical address to which the memory is currently
 assigned (this will be ioremapped so the CPU can access the region).
 
-device_addr is the bus address the device needs to be programmed
+device_addr is the DMA address the device needs to be programmed
 with to actually address this memory (this will be handed out as the
 dma_addr_t in dma_alloc_coherent()).
 
diff --git a/include/linux/types.h b/include/linux/types.h
index 6747247e3f9f..00a127e89752 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -139,12 +139,20 @@ typedef unsigned long blkcnt_t;
  */
 #define pgoff_t unsigned long
 
-/* A dma_addr_t can hold any valid DMA or bus address for the platform */
+/*
+ * A dma_addr_t can hold any valid DMA address, i.e., any address returned
+ * by the DMA API.
+ *
+ * If the DMA API only uses 32-bit addresses, dma_addr_t need only be 32
+ * bits wide.  Bus addresses, e.g., PCI BARs, may be wider than 32 bits,
+ * but drivers do memory-mapped I/O to ioremapped kernel virtual addresses,
+ * so they don't care about the size of the actual bus addresses.
+ */
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 typedef u64 dma_addr_t;
 #else
 typedef u32 dma_addr_t;
-#endif /* dma_addr_t */
+#endif
 
 #ifdef __CHECKER__
 #else

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Fri, 3 Apr 2015 13:59:39 -0500

> On Tue, Mar 31, 2015 at 07:57:47PM -0700, Yinghai Lu wrote:
>> David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
>> to fit in upstream windows") broke booting on sparc/T5-8.
>> 
>> In the boot log, there is
>>   pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
>>   0x110204000)
>> but that only could happen when dma_addr_t is 32-bit.
>> 
>> According to David Miller, all DMA occurs behind an IOMMU and these
>> IOMMUs only support 32-bit addressing, therefore dma_addr_t is
>> 32-bit on sparc64.
>> 
>> Let's introduce pci_bus_addr_t instead of using dma_addr_t,
>> and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.
> 
> I propose the following doc updates (I can just fold them into this patch
> if you approve):

No objection.

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Yinghai Lu]

On Fri, Apr 3, 2015 at 11:59 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Mar 31, 2015 at 07:57:47PM -0700, Yinghai Lu wrote:
>> David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
>> to fit in upstream windows") broke booting on sparc/T5-8.
>>
>> In the boot log, there is
>>   pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
>>   0x110204000)
>> but that only could happen when dma_addr_t is 32-bit.
>>
>> According to David Miller, all DMA occurs behind an IOMMU and these
>> IOMMUs only support 32-bit addressing, therefore dma_addr_t is
>> 32-bit on sparc64.
>>
>> Let's introduce pci_bus_addr_t instead of using dma_addr_t,
>> and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.
>
> I propose the following doc updates (I can just fold them into this patch
> if you approve):
>
>
> commit 13fca18e2f1d9dd078b8dfea965718cf13a4b600
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Fri Apr 3 12:39:35 2015 -0500
>
>     pci-bus-addr-fixups
>
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 6747247e3f9f..00a127e89752 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -139,12 +139,20 @@ typedef unsigned long blkcnt_t;
>   */
>  #define pgoff_t unsigned long
>
> -/* A dma_addr_t can hold any valid DMA or bus address for the platform */
> +/*
> + * A dma_addr_t can hold any valid DMA address, i.e., any address returned
> + * by the DMA API.
> + *
> + * If the DMA API only uses 32-bit addresses, dma_addr_t need only be 32
> + * bits wide.  Bus addresses, e.g., PCI BARs, may be wider than 32 bits,
> + * but drivers do memory-mapped I/O to ioremapped kernel virtual addresses,
> + * so they don't care about the size of the actual bus addresses.
> + */

Good to me.

Thanks

Yinghai

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Bjorn Helgaas]

On Tue, Mar 31, 2015 at 07:57:47PM -0700, Yinghai Lu wrote:
> David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
> to fit in upstream windows") broke booting on sparc/T5-8.
> 
> In the boot log, there is
>   pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
>   0x110204000)
> but that only could happen when dma_addr_t is 32-bit.
> 
> According to David Miller, all DMA occurs behind an IOMMU and these
> IOMMUs only support 32-bit addressing, therefore dma_addr_t is
> 32-bit on sparc64.
> 
> Let's introduce pci_bus_addr_t instead of using dma_addr_t,
> and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.
> 
> Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
> Fixes: commit 23b13bc76f35 ("PCI: Fail safely if we can't handle BARs larger than 4GB")
> Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
> Reported-by: David Ahern <david.ahern@oracle.com>
> Tested-by: David Ahern <david.ahern@oracle.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: <stable@vger.kernel.org> #3.19
> ---
>  drivers/pci/Kconfig |  4 ++++
>  drivers/pci/bus.c   | 10 +++++-----
>  drivers/pci/probe.c | 12 ++++++------
>  include/linux/pci.h | 12 +++++++++---
>  4 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 7a8f1c5..6a5a269 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -1,6 +1,10 @@
>  #
>  # PCI configuration
>  #
> +config PCI_BUS_ADDR_T_64BIT
> +	def_bool y if (64BIT || X86_PAE)
> +	depends on PCI

We're going to use pci_bus_addr_t in some places where we previously used
dma_addr_t, which means pci_bus_addr_t should be at least as large as
dma_addr_t.  Can you enforce that directly, e.g., with something like this?

    def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT || X86_PAE)

Unless we do something like that, I think this may break these arches by
changing the addresses in struct pci_bus_region from 64 bits to 32:

    arch/arm/Kconfig:
	XEN selects ARCH_DMA_ADDR_T_64BIT but not 64BIT, so it would not
	set PCI_BUS_ADDR_T_64BIT

    arch/arm/mach-axxia/Kconfig:
	ARCH_AXXIA selects ARCH_DMA_ADDR_T_64BIT but not 64BIT, so it
	would not set PCI_BUS_ADDR_T_64BIT

    arch/arm/mach-exynos/Kconfig:
	SOC_EXYNOS5440 selects ARCH_DMA_ADDR_T_64BIT if ARM_LPAE, but not
	64BIT, so it would not set PCI_BUS_ADDR_T_64BIT

    arch/arm/mach-highbank/Kconfig:
	ARCH_HIGHBANK selects ARCH_DMA_ADDR_T_64BIT if ARM_LPAE, but not
	64BIT, so it would not set PCI_BUS_ADDR_T_64BIT

    arch/arm/mach-shmobile/Kconfig:
	ARCH_SHMOBILE_MULTI selects ARCH_DMA_ADDR_T_64BIT if ARM_LPAE, but
	not 64BIT, so it would not set PCI_BUS_ADDR_T_64BIT

    arch/mips/Kconfig:
	sets ARCH_DMA_ADDR_T_64BIT if (HIGHMEM && ARCH_PHYS_ADDR_T_64BIT)
	|| 64BIT, so if we have (HIGHMEM && ARCH_PHYS_ADDR_T_64BIT) but not
	64BIT, it would not set PCI_BUS_ADDR_T_64BIT
	
    arch/powerpc/Kconfig:
	sets ARCH_DMA_ADDR_T_64BIT if ARCH_PHYS_ADDR_T_64BIT, which isn't
	trivially identical to (64BIT || X86_PAE), so we may not set
	PCI_BUS_ADDR_T_64BIT in all cases where we set
	ARCH_DMA_ADDR_T_64BIT

    arch/x86/Kconfig:
	sets ARCH_DMA_ADDR_T_64BIT if X86_64 || HIGHMEM64G, which isn't
	trivially identical to (64BIT || X86_PAE), so we may not set
	PCI_BUS_ADDR_T_64BIT in all cases where we set
	ARCH_DMA_ADDR_T_64BIT

>  config PCI_MSI
>  	bool "Message Signaled Interrupts (MSI and MSI-X)"
>  	depends on PCI
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 90fa3a7..6fbd3f2 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -92,11 +92,11 @@ void pci_bus_remove_resources(struct pci_bus *bus)
>  }
>  
>  static struct pci_bus_region pci_32_bit = {0, 0xffffffffULL};
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> +#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
>  static struct pci_bus_region pci_64_bit = {0,
> -				(dma_addr_t) 0xffffffffffffffffULL};
> -static struct pci_bus_region pci_high = {(dma_addr_t) 0x100000000ULL,
> -				(dma_addr_t) 0xffffffffffffffffULL};
> +				(pci_bus_addr_t) 0xffffffffffffffffULL};
> +static struct pci_bus_region pci_high = {(pci_bus_addr_t) 0x100000000ULL,
> +				(pci_bus_addr_t) 0xffffffffffffffffULL};
>  #endif
>  
>  /*
> @@ -200,7 +200,7 @@ int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
>  					  resource_size_t),
>  		void *alignf_data)
>  {
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> +#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
>  	int rc;
>  
>  	if (res->flags & IORESOURCE_MEM_64) {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8d2f400..f71cb7c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -253,8 +253,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>  	}
>  
>  	if (res->flags & IORESOURCE_MEM_64) {
> -		if ((sizeof(dma_addr_t) < 8 || sizeof(resource_size_t) < 8) &&
> -		    sz64 > 0x100000000ULL) {
> +		if ((sizeof(pci_bus_addr_t) < 8 || sizeof(resource_size_t) < 8)
> +		    && sz64 > 0x100000000ULL) {
>  			res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
>  			res->start = 0;
>  			res->end = 0;
> @@ -263,7 +263,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>  			goto out;
>  		}
>  
> -		if ((sizeof(dma_addr_t) < 8) && l) {
> +		if ((sizeof(pci_bus_addr_t) < 8) && l) {
>  			/* Above 32-bit boundary; try to reallocate */
>  			res->flags |= IORESOURCE_UNSET;
>  			res->start = 0;
> @@ -398,7 +398,7 @@ static void pci_read_bridge_mmio_pref(struct pci_bus *child)
>  	struct pci_dev *dev = child->self;
>  	u16 mem_base_lo, mem_limit_lo;
>  	u64 base64, limit64;
> -	dma_addr_t base, limit;
> +	pci_bus_addr_t base, limit;
>  	struct pci_bus_region region;
>  	struct resource *res;
>  
> @@ -425,8 +425,8 @@ static void pci_read_bridge_mmio_pref(struct pci_bus *child)
>  		}
>  	}
>  
> -	base = (dma_addr_t) base64;
> -	limit = (dma_addr_t) limit64;
> +	base = (pci_bus_addr_t) base64;
> +	limit = (pci_bus_addr_t) limit64;
>  
>  	if (base != base64) {
>  		dev_err(&dev->dev, "can't handle bridge window above 4GB (bus address %#010llx)\n",
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 211e9da..6021bbe 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -573,9 +573,15 @@ int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
>  int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
>  		  int reg, int len, u32 val);
>  
> +#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
> +typedef u64 pci_bus_addr_t;
> +#else
> +typedef u32 pci_bus_addr_t;
> +#endif
> +
>  struct pci_bus_region {
> -	dma_addr_t start;
> -	dma_addr_t end;
> +	pci_bus_addr_t start;
> +	pci_bus_addr_t end;
>  };
>  
>  struct pci_dynids {
> @@ -1124,7 +1130,7 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
>  
>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
>  
> -static inline dma_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
> +static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
>  {
>  	struct pci_bus_region region;
>  
> -- 
> 1.8.4.5
> 

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Bjorn Helgaas]

[+cc Sam (commented on previous versions), Russell, linux-arm-kernel, Ralf,
linux-mips, Ben, linuxppc-dev, x86]

On Fri, Apr 03, 2015 at 02:32:34PM -0500, Bjorn Helgaas wrote:
> On Tue, Mar 31, 2015 at 07:57:47PM -0700, Yinghai Lu wrote:
> > David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
> > to fit in upstream windows") broke booting on sparc/T5-8.
> > 
> > In the boot log, there is
> >   pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
> >   0x110204000)
> > but that only could happen when dma_addr_t is 32-bit.
> > 
> > According to David Miller, all DMA occurs behind an IOMMU and these
> > IOMMUs only support 32-bit addressing, therefore dma_addr_t is
> > 32-bit on sparc64.
> > 
> > Let's introduce pci_bus_addr_t instead of using dma_addr_t,
> > and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.
> > 
> > Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
> > Fixes: commit 23b13bc76f35 ("PCI: Fail safely if we can't handle BARs larger than 4GB")
> > Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
> > Reported-by: David Ahern <david.ahern@oracle.com>
> > Tested-by: David Ahern <david.ahern@oracle.com>
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > Cc: <stable@vger.kernel.org> #3.19
> > ---
> >  drivers/pci/Kconfig |  4 ++++
> >  drivers/pci/bus.c   | 10 +++++-----
> >  drivers/pci/probe.c | 12 ++++++------
> >  include/linux/pci.h | 12 +++++++++---
> >  4 files changed, 24 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > index 7a8f1c5..6a5a269 100644
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -1,6 +1,10 @@
> >  #
> >  # PCI configuration
> >  #
> > +config PCI_BUS_ADDR_T_64BIT
> > +	def_bool y if (64BIT || X86_PAE)
> > +	depends on PCI
> 
> We're going to use pci_bus_addr_t in some places where we previously used
> dma_addr_t, which means pci_bus_addr_t should be at least as large as
> dma_addr_t.  Can you enforce that directly, e.g., with something like this?
> 
>     def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT || X86_PAE)
> 
> Unless we do something like that, I think this may break these arches by
> changing the addresses in struct pci_bus_region from 64 bits to 32:
> 
>     arch/arm/Kconfig:
> 	XEN selects ARCH_DMA_ADDR_T_64BIT but not 64BIT, so it would not
> 	set PCI_BUS_ADDR_T_64BIT
> 
>     arch/arm/mach-axxia/Kconfig:
> 	ARCH_AXXIA selects ARCH_DMA_ADDR_T_64BIT but not 64BIT, so it
> 	would not set PCI_BUS_ADDR_T_64BIT
> 
>     arch/arm/mach-exynos/Kconfig:
> 	SOC_EXYNOS5440 selects ARCH_DMA_ADDR_T_64BIT if ARM_LPAE, but not
> 	64BIT, so it would not set PCI_BUS_ADDR_T_64BIT
> 
>     arch/arm/mach-highbank/Kconfig:
> 	ARCH_HIGHBANK selects ARCH_DMA_ADDR_T_64BIT if ARM_LPAE, but not
> 	64BIT, so it would not set PCI_BUS_ADDR_T_64BIT
> 
>     arch/arm/mach-shmobile/Kconfig:
> 	ARCH_SHMOBILE_MULTI selects ARCH_DMA_ADDR_T_64BIT if ARM_LPAE, but
> 	not 64BIT, so it would not set PCI_BUS_ADDR_T_64BIT
> 
>     arch/mips/Kconfig:
> 	sets ARCH_DMA_ADDR_T_64BIT if (HIGHMEM && ARCH_PHYS_ADDR_T_64BIT)
> 	|| 64BIT, so if we have (HIGHMEM && ARCH_PHYS_ADDR_T_64BIT) but not
> 	64BIT, it would not set PCI_BUS_ADDR_T_64BIT
> 	
>     arch/powerpc/Kconfig:
> 	sets ARCH_DMA_ADDR_T_64BIT if ARCH_PHYS_ADDR_T_64BIT, which isn't
> 	trivially identical to (64BIT || X86_PAE), so we may not set
> 	PCI_BUS_ADDR_T_64BIT in all cases where we set
> 	ARCH_DMA_ADDR_T_64BIT
> 
>     arch/x86/Kconfig:
> 	sets ARCH_DMA_ADDR_T_64BIT if X86_64 || HIGHMEM64G, which isn't
> 	trivially identical to (64BIT || X86_PAE), so we may not set
> 	PCI_BUS_ADDR_T_64BIT in all cases where we set
> 	ARCH_DMA_ADDR_T_64BIT
> 
> >  config PCI_MSI
> >  	bool "Message Signaled Interrupts (MSI and MSI-X)"
> >  	depends on PCI
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 90fa3a7..6fbd3f2 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -92,11 +92,11 @@ void pci_bus_remove_resources(struct pci_bus *bus)
> >  }
> >  
> >  static struct pci_bus_region pci_32_bit = {0, 0xffffffffULL};
> > -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> > +#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
> >  static struct pci_bus_region pci_64_bit = {0,
> > -				(dma_addr_t) 0xffffffffffffffffULL};
> > -static struct pci_bus_region pci_high = {(dma_addr_t) 0x100000000ULL,
> > -				(dma_addr_t) 0xffffffffffffffffULL};
> > +				(pci_bus_addr_t) 0xffffffffffffffffULL};
> > +static struct pci_bus_region pci_high = {(pci_bus_addr_t) 0x100000000ULL,
> > +				(pci_bus_addr_t) 0xffffffffffffffffULL};
> >  #endif
> >  
> >  /*
> > @@ -200,7 +200,7 @@ int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
> >  					  resource_size_t),
> >  		void *alignf_data)
> >  {
> > -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> > +#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
> >  	int rc;
> >  
> >  	if (res->flags & IORESOURCE_MEM_64) {
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 8d2f400..f71cb7c 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -253,8 +253,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> >  	}
> >  
> >  	if (res->flags & IORESOURCE_MEM_64) {
> > -		if ((sizeof(dma_addr_t) < 8 || sizeof(resource_size_t) < 8) &&
> > -		    sz64 > 0x100000000ULL) {
> > +		if ((sizeof(pci_bus_addr_t) < 8 || sizeof(resource_size_t) < 8)
> > +		    && sz64 > 0x100000000ULL) {
> >  			res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
> >  			res->start = 0;
> >  			res->end = 0;
> > @@ -263,7 +263,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> >  			goto out;
> >  		}
> >  
> > -		if ((sizeof(dma_addr_t) < 8) && l) {
> > +		if ((sizeof(pci_bus_addr_t) < 8) && l) {
> >  			/* Above 32-bit boundary; try to reallocate */
> >  			res->flags |= IORESOURCE_UNSET;
> >  			res->start = 0;
> > @@ -398,7 +398,7 @@ static void pci_read_bridge_mmio_pref(struct pci_bus *child)
> >  	struct pci_dev *dev = child->self;
> >  	u16 mem_base_lo, mem_limit_lo;
> >  	u64 base64, limit64;
> > -	dma_addr_t base, limit;
> > +	pci_bus_addr_t base, limit;
> >  	struct pci_bus_region region;
> >  	struct resource *res;
> >  
> > @@ -425,8 +425,8 @@ static void pci_read_bridge_mmio_pref(struct pci_bus *child)
> >  		}
> >  	}
> >  
> > -	base = (dma_addr_t) base64;
> > -	limit = (dma_addr_t) limit64;
> > +	base = (pci_bus_addr_t) base64;
> > +	limit = (pci_bus_addr_t) limit64;
> >  
> >  	if (base != base64) {
> >  		dev_err(&dev->dev, "can't handle bridge window above 4GB (bus address %#010llx)\n",
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 211e9da..6021bbe 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -573,9 +573,15 @@ int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
> >  int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
> >  		  int reg, int len, u32 val);
> >  
> > +#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
> > +typedef u64 pci_bus_addr_t;
> > +#else
> > +typedef u32 pci_bus_addr_t;
> > +#endif
> > +
> >  struct pci_bus_region {
> > -	dma_addr_t start;
> > -	dma_addr_t end;
> > +	pci_bus_addr_t start;
> > +	pci_bus_addr_t end;
> >  };
> >  
> >  struct pci_dynids {
> > @@ -1124,7 +1130,7 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
> >  
> >  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
> >  
> > -static inline dma_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
> > +static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
> >  {
> >  	struct pci_bus_region region;
> >  
> > -- 
> > 1.8.4.5
> > 

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Yinghai Lu]

On Fri, Apr 3, 2015 at 1:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Sam (commented on previous versions), Russell, linux-arm-kernel, Ralf,
> linux-mips, Ben, linuxppc-dev, x86]
>
> On Fri, Apr 03, 2015 at 02:32:34PM -0500, Bjorn Helgaas wrote:
>> On Tue, Mar 31, 2015 at 07:57:47PM -0700, Yinghai Lu wrote:
>> > David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
>> > to fit in upstream windows") broke booting on sparc/T5-8.
>> >
>> > In the boot log, there is
>> >   pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
>> >   0x110204000)
>> > but that only could happen when dma_addr_t is 32-bit.
>> >
>> > According to David Miller, all DMA occurs behind an IOMMU and these
>> > IOMMUs only support 32-bit addressing, therefore dma_addr_t is
>> > 32-bit on sparc64.
>> >
>> > Let's introduce pci_bus_addr_t instead of using dma_addr_t,
>> > and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.
>> >
>> > Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
>> > Fixes: commit 23b13bc76f35 ("PCI: Fail safely if we can't handle BARs larger than 4GB")
>> > Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
>> > Reported-by: David Ahern <david.ahern@oracle.com>
>> > Tested-by: David Ahern <david.ahern@oracle.com>
>> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> > Cc: <stable@vger.kernel.org> #3.19
>> > ---
>> >  drivers/pci/Kconfig |  4 ++++
>> >  drivers/pci/bus.c   | 10 +++++-----
>> >  drivers/pci/probe.c | 12 ++++++------
>> >  include/linux/pci.h | 12 +++++++++---
>> >  4 files changed, 24 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> > index 7a8f1c5..6a5a269 100644
>> > --- a/drivers/pci/Kconfig
>> > +++ b/drivers/pci/Kconfig
>> > @@ -1,6 +1,10 @@
>> >  #
>> >  # PCI configuration
>> >  #
>> > +config PCI_BUS_ADDR_T_64BIT
>> > +   def_bool y if (64BIT || X86_PAE)
>> > +   depends on PCI
>>
>> We're going to use pci_bus_addr_t in some places where we previously used
>> dma_addr_t, which means pci_bus_addr_t should be at least as large as
>> dma_addr_t.  Can you enforce that directly, e.g., with something like this?
>>
>>     def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT || X86_PAE)

then should use

def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT)

Thanks

Yinghai

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Bjorn Helgaas]

On Fri, Apr 3, 2015 at 10:34 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Apr 3, 2015 at 1:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc Sam (commented on previous versions), Russell, linux-arm-kernel, Ralf,
>> linux-mips, Ben, linuxppc-dev, x86]
>>
>> On Fri, Apr 03, 2015 at 02:32:34PM -0500, Bjorn Helgaas wrote:
>>> On Tue, Mar 31, 2015 at 07:57:47PM -0700, Yinghai Lu wrote:
>>> > David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
>>> > to fit in upstream windows") broke booting on sparc/T5-8.
>>> >
>>> > In the boot log, there is
>>> >   pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
>>> >   0x110204000)
>>> > but that only could happen when dma_addr_t is 32-bit.
>>> >
>>> > According to David Miller, all DMA occurs behind an IOMMU and these
>>> > IOMMUs only support 32-bit addressing, therefore dma_addr_t is
>>> > 32-bit on sparc64.
>>> >
>>> > Let's introduce pci_bus_addr_t instead of using dma_addr_t,
>>> > and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.
>>> >
>>> > Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
>>> > Fixes: commit 23b13bc76f35 ("PCI: Fail safely if we can't handle BARs larger than 4GB")
>>> > Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
>>> > Reported-by: David Ahern <david.ahern@oracle.com>
>>> > Tested-by: David Ahern <david.ahern@oracle.com>
>>> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>> > Cc: <stable@vger.kernel.org> #3.19
>>> > ---
>>> >  drivers/pci/Kconfig |  4 ++++
>>> >  drivers/pci/bus.c   | 10 +++++-----
>>> >  drivers/pci/probe.c | 12 ++++++------
>>> >  include/linux/pci.h | 12 +++++++++---
>>> >  4 files changed, 24 insertions(+), 14 deletions(-)
>>> >
>>> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>>> > index 7a8f1c5..6a5a269 100644
>>> > --- a/drivers/pci/Kconfig
>>> > +++ b/drivers/pci/Kconfig
>>> > @@ -1,6 +1,10 @@
>>> >  #
>>> >  # PCI configuration
>>> >  #
>>> > +config PCI_BUS_ADDR_T_64BIT
>>> > +   def_bool y if (64BIT || X86_PAE)
>>> > +   depends on PCI
>>>
>>> We're going to use pci_bus_addr_t in some places where we previously used
>>> dma_addr_t, which means pci_bus_addr_t should be at least as large as
>>> dma_addr_t.  Can you enforce that directly, e.g., with something like this?
>>>
>>>     def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT || X86_PAE)
>
> then should use
>
> def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT)

OK, would you mind updating this series, incorporating the doc
updates, and reposting it?

I think there's still an unresolved question about the OF parsing code.

Bjorn

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Rob Herring]

On Sat, Apr 4, 2015 at 7:46 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Apr 3, 2015 at 10:34 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Apr 3, 2015 at 1:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> [+cc Sam (commented on previous versions), Russell, linux-arm-kernel, Ralf,
>>> linux-mips, Ben, linuxppc-dev, x86]
>>>
>>> On Fri, Apr 03, 2015 at 02:32:34PM -0500, Bjorn Helgaas wrote:
>>>> On Tue, Mar 31, 2015 at 07:57:47PM -0700, Yinghai Lu wrote:
>>>> > David Ahern found commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows
>>>> > to fit in upstream windows") broke booting on sparc/T5-8.
>>>> >
>>>> > In the boot log, there is
>>>> >   pci 0000:06:00.0: reg 0x184: can't handle BAR above 4GB (bus address
>>>> >   0x110204000)
>>>> > but that only could happen when dma_addr_t is 32-bit.
>>>> >
>>>> > According to David Miller, all DMA occurs behind an IOMMU and these
>>>> > IOMMUs only support 32-bit addressing, therefore dma_addr_t is
>>>> > 32-bit on sparc64.
>>>> >
>>>> > Let's introduce pci_bus_addr_t instead of using dma_addr_t,
>>>> > and pci_bus_addr_t will be 64-bit on 64-bit platform or X86_PAE.
>>>> >
>>>> > Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
>>>> > Fixes: commit 23b13bc76f35 ("PCI: Fail safely if we can't handle BARs larger than 4GB")
>>>> > Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
>>>> > Reported-by: David Ahern <david.ahern@oracle.com>
>>>> > Tested-by: David Ahern <david.ahern@oracle.com>
>>>> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>> > Cc: <stable@vger.kernel.org> #3.19
>>>> > ---
>>>> >  drivers/pci/Kconfig |  4 ++++
>>>> >  drivers/pci/bus.c   | 10 +++++-----
>>>> >  drivers/pci/probe.c | 12 ++++++------
>>>> >  include/linux/pci.h | 12 +++++++++---
>>>> >  4 files changed, 24 insertions(+), 14 deletions(-)
>>>> >
>>>> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>>>> > index 7a8f1c5..6a5a269 100644
>>>> > --- a/drivers/pci/Kconfig
>>>> > +++ b/drivers/pci/Kconfig
>>>> > @@ -1,6 +1,10 @@
>>>> >  #
>>>> >  # PCI configuration
>>>> >  #
>>>> > +config PCI_BUS_ADDR_T_64BIT
>>>> > +   def_bool y if (64BIT || X86_PAE)
>>>> > +   depends on PCI
>>>>
>>>> We're going to use pci_bus_addr_t in some places where we previously used
>>>> dma_addr_t, which means pci_bus_addr_t should be at least as large as
>>>> dma_addr_t.  Can you enforce that directly, e.g., with something like this?
>>>>
>>>>     def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT || X86_PAE)
>>
>> then should use
>>
>> def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT)
>
> OK, would you mind updating this series, incorporating the doc
> updates, and reposting it?
>
> I think there's still an unresolved question about the OF parsing code.

Got a pointer to what that is? I'll take a guess. Generally, we make
the parsing code independent of the kernel addr sizes and use u64
types. The DT sizes and kernel sizes are not always aligned. For
example, an LPAE capable platform running a non-LPAE kernel build.

Rob

>
> Bjorn
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Bjorn Helgaas]

On Sat, Apr 4, 2015 at 2:48 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Sat, Apr 4, 2015 at 7:46 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:

>> I think there's still an unresolved question about the OF parsing code.
>
> Got a pointer to what that is? I'll take a guess. Generally, we make
> the parsing code independent of the kernel addr sizes and use u64
> types. The DT sizes and kernel sizes are not always aligned. For
> example, an LPAE capable platform running a non-LPAE kernel build.

Yep: http://lkml.kernel.org/r/1427857069-6789-3-git-send-email-yinghai@kernel.org

Yinghai made a change to the sparc OF parsing code.  The question is
whether a similar change should be made to clones of that code (two
others in arch/sparc, one in arch/powerpc, and one in drivers/of).

Bjorn

Re: [PATCH 1/3] PCI: Introduce pci_bus_addr_t

[Rob Herring]

On Sat, Apr 4, 2015 at 10:25 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sat, Apr 4, 2015 at 2:48 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On Sat, Apr 4, 2015 at 7:46 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>>> I think there's still an unresolved question about the OF parsing code.
>>
>> Got a pointer to what that is? I'll take a guess. Generally, we make
>> the parsing code independent of the kernel addr sizes and use u64
>> types. The DT sizes and kernel sizes are not always aligned. For
>> example, an LPAE capable platform running a non-LPAE kernel build.
>
> Yep: http://lkml.kernel.org/r/1427857069-6789-3-git-send-email-yinghai@kernel.org
>
> Yinghai made a change to the sparc OF parsing code.  The question is
> whether a similar change should be made to clones of that code (two
> others in arch/sparc, one in arch/powerpc, and one in drivers/of).

Yes, it appears to me that is needed. At least the drivers/of/ code
does not distinguish 32 and 64 bit ATM.

Rob

[PATCH 2/3] sparc/PCI: Add mem64 resource parsing for root bus

[Yinghai Lu]

Found "no compatible bridge window" warning in boot log from T5-8.

pci 0000:00:01.0: can't claim BAR 15 [mem 0x100000000-0x4afffffff pref]: no compatible bridge window

That resource is above 4G, but does not get offset correctly as
root bus only report io and mem32.

pci_sun4v f02dbcfc: PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [io  0x804000000000-0x80400fffffff] (bus address [0x0000-0xfffffff])
pci_bus 0000:00: root bus resource [mem 0x800000000000-0x80007effffff] (bus address [0x00000000-0x7effffff])
pci_bus 0000:00: root bus resource [bus 00-77]

Add mem64 handling in pci_common for sparc, so we can have 64bit resource
registered for root bus at first.

After patch, will have:
pci_sun4v f02dbcfc: PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [io  0x804000000000-0x80400fffffff] (bus address [0x0000-0xfffffff])
pci_bus 0000:00: root bus resource [mem 0x800000000000-0x80007effffff] (bus address [0x00000000-0x7effffff])
pci_bus 0000:00: root bus resource [mem 0x800100000000-0x8007ffffffff] (bus address [0x100000000-0x7ffffffff])
pci_bus 0000:00: root bus resource [bus 00-77]

Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
Reported-by: David Ahern <david.ahern@oracle.com>
Tested-by: David Ahern <david.ahern@oracle.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: <stable@vger.kernel.org> #3.19
---
 arch/sparc/kernel/pci.c        |  7 ++++++-
 arch/sparc/kernel/pci_common.c | 15 +++++++++++++--
 arch/sparc/kernel/pci_impl.h   |  1 +
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
index 9ce5afe..04ce3ac 100644
--- a/arch/sparc/kernel/pci.c
+++ b/arch/sparc/kernel/pci.c
@@ -185,8 +185,10 @@ static unsigned long pci_parse_of_flags(u32 addr0)
 
 	if (addr0 & 0x02000000) {
 		flags = IORESOURCE_MEM | PCI_BASE_ADDRESS_SPACE_MEMORY;
-		flags |= (addr0 >> 22) & PCI_BASE_ADDRESS_MEM_TYPE_64;
 		flags |= (addr0 >> 28) & PCI_BASE_ADDRESS_MEM_TYPE_1M;
+		if (addr0 & 0x01000000)
+			flags |= IORESOURCE_MEM_64
+				 | PCI_BASE_ADDRESS_MEM_TYPE_64;
 		if (addr0 & 0x40000000)
 			flags |= IORESOURCE_PREFETCH
 				 | PCI_BASE_ADDRESS_MEM_PREFETCH;
@@ -663,6 +665,9 @@ struct pci_bus *pci_scan_one_pbm(struct pci_pbm_info *pbm,
 				pbm->io_space.start);
 	pci_add_resource_offset(&resources, &pbm->mem_space,
 				pbm->mem_space.start);
+	if (pbm->mem64_space.flags)
+		pci_add_resource_offset(&resources, &pbm->mem64_space,
+					pbm->mem_space.start);
 	pbm->busn.start = pbm->pci_first_busno;
 	pbm->busn.end	= pbm->pci_last_busno;
 	pbm->busn.flags	= IORESOURCE_BUS;
diff --git a/arch/sparc/kernel/pci_common.c b/arch/sparc/kernel/pci_common.c
index 944a065..a859a86 100644
--- a/arch/sparc/kernel/pci_common.c
+++ b/arch/sparc/kernel/pci_common.c
@@ -406,6 +406,7 @@ void pci_determine_mem_io_space(struct pci_pbm_info *pbm)
 	}
 
 	num_pbm_ranges = i / sizeof(*pbm_ranges);
+	memset(&pbm->mem64_space, 0, sizeof(struct resource));
 
 	for (i = 0; i < num_pbm_ranges; i++) {
 		const struct linux_prom_pci_ranges *pr = &pbm_ranges[i];
@@ -451,7 +452,11 @@ void pci_determine_mem_io_space(struct pci_pbm_info *pbm)
 			break;
 
 		case 3:
-			/* XXX 64-bit MEM handling XXX */
+			/* 64-bit MEM handling */
+			pbm->mem64_space.start = a;
+			pbm->mem64_space.end = a + size - 1UL;
+			pbm->mem64_space.flags = IORESOURCE_MEM;
+			break;
 
 		default:
 			break;
@@ -465,15 +470,21 @@ void pci_determine_mem_io_space(struct pci_pbm_info *pbm)
 		prom_halt();
 	}
 
-	printk("%s: PCI IO[%llx] MEM[%llx]\n",
+	printk("%s: PCI IO[%llx] MEM[%llx]",
 	       pbm->name,
 	       pbm->io_space.start,
 	       pbm->mem_space.start);
+	if (pbm->mem64_space.flags)
+		printk(" MEM64[%llx]",
+		       pbm->mem64_space.start);
+	printk("\n");
 
 	pbm->io_space.name = pbm->mem_space.name = pbm->name;
 
 	request_resource(&ioport_resource, &pbm->io_space);
 	request_resource(&iomem_resource, &pbm->mem_space);
+	if (pbm->mem64_space.flags)
+		request_resource(&iomem_resource, &pbm->mem64_space);
 
 	pci_register_legacy_regions(&pbm->io_space,
 				    &pbm->mem_space);
diff --git a/arch/sparc/kernel/pci_impl.h b/arch/sparc/kernel/pci_impl.h
index 75803c7..37222ca 100644
--- a/arch/sparc/kernel/pci_impl.h
+++ b/arch/sparc/kernel/pci_impl.h
@@ -97,6 +97,7 @@ struct pci_pbm_info {
 	/* PBM I/O and Memory space resources. */
 	struct resource			io_space;
 	struct resource			mem_space;
+	struct resource			mem64_space;
 	struct resource			busn;
 
 	/* Base of PCI Config space, can be per-PBM or shared. */
-- 
1.8.4.5

Re: [PATCH 2/3] sparc/PCI: Add mem64 resource parsing for root bus

[Bjorn Helgaas]

[+cc Ben, linuxppc-dev, Grant, Rob, devicetree]

On Tue, Mar 31, 2015 at 07:57:48PM -0700, Yinghai Lu wrote:
> Found "no compatible bridge window" warning in boot log from T5-8.
> 
> pci 0000:00:01.0: can't claim BAR 15 [mem 0x100000000-0x4afffffff pref]: no compatible bridge window
> 
> That resource is above 4G, but does not get offset correctly as
> root bus only report io and mem32.
> 
> pci_sun4v f02dbcfc: PCI host bridge to bus 0000:00
> pci_bus 0000:00: root bus resource [io  0x804000000000-0x80400fffffff] (bus address [0x0000-0xfffffff])
> pci_bus 0000:00: root bus resource [mem 0x800000000000-0x80007effffff] (bus address [0x00000000-0x7effffff])
> pci_bus 0000:00: root bus resource [bus 00-77]
> 
> Add mem64 handling in pci_common for sparc, so we can have 64bit resource
> registered for root bus at first.
> 
> After patch, will have:
> pci_sun4v f02dbcfc: PCI host bridge to bus 0000:00
> pci_bus 0000:00: root bus resource [io  0x804000000000-0x80400fffffff] (bus address [0x0000-0xfffffff])
> pci_bus 0000:00: root bus resource [mem 0x800000000000-0x80007effffff] (bus address [0x00000000-0x7effffff])
> pci_bus 0000:00: root bus resource [mem 0x800100000000-0x8007ffffffff] (bus address [0x100000000-0x7ffffffff])
> pci_bus 0000:00: root bus resource [bus 00-77]
> 
> Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
> Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
> Reported-by: David Ahern <david.ahern@oracle.com>
> Tested-by: David Ahern <david.ahern@oracle.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: <stable@vger.kernel.org> #3.19
> ---
>  arch/sparc/kernel/pci.c        |  7 ++++++-
>  arch/sparc/kernel/pci_common.c | 15 +++++++++++++--
>  arch/sparc/kernel/pci_impl.h   |  1 +
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
> index 9ce5afe..04ce3ac 100644
> --- a/arch/sparc/kernel/pci.c
> +++ b/arch/sparc/kernel/pci.c
> @@ -185,8 +185,10 @@ static unsigned long pci_parse_of_flags(u32 addr0)
>  
>  	if (addr0 & 0x02000000) {
>  		flags = IORESOURCE_MEM | PCI_BASE_ADDRESS_SPACE_MEMORY;
> -		flags |= (addr0 >> 22) & PCI_BASE_ADDRESS_MEM_TYPE_64;
>  		flags |= (addr0 >> 28) & PCI_BASE_ADDRESS_MEM_TYPE_1M;
> +		if (addr0 & 0x01000000)
> +			flags |= IORESOURCE_MEM_64
> +				 | PCI_BASE_ADDRESS_MEM_TYPE_64;
>  		if (addr0 & 0x40000000)
>  			flags |= IORESOURCE_PREFETCH
>  				 | PCI_BASE_ADDRESS_MEM_PREFETCH;

This function is very similar to these:

  drivers/of/address.c			of_bus_pci_get_flags()
  arch/powerpc/kernel/pci_of_scan.c	pci_parse_of_flags()
  arch/sparc/kernel/of_device_32.c	of_bus_pci_get_flags()
  arch/sparc/kernel/of_device_64.c	of_bus_pci_get_flags()

Should they get a similar change?  They're all so similar that it would be
nice to consolidate them, but I think that would be out of scope for this
patch.

> @@ -663,6 +665,9 @@ struct pci_bus *pci_scan_one_pbm(struct pci_pbm_info *pbm,
>  				pbm->io_space.start);
>  	pci_add_resource_offset(&resources, &pbm->mem_space,
>  				pbm->mem_space.start);
> +	if (pbm->mem64_space.flags)
> +		pci_add_resource_offset(&resources, &pbm->mem64_space,
> +					pbm->mem_space.start);
>  	pbm->busn.start = pbm->pci_first_busno;
>  	pbm->busn.end	= pbm->pci_last_busno;
>  	pbm->busn.flags	= IORESOURCE_BUS;
> diff --git a/arch/sparc/kernel/pci_common.c b/arch/sparc/kernel/pci_common.c
> index 944a065..a859a86 100644
> --- a/arch/sparc/kernel/pci_common.c
> +++ b/arch/sparc/kernel/pci_common.c
> @@ -406,6 +406,7 @@ void pci_determine_mem_io_space(struct pci_pbm_info *pbm)
>  	}
>  
>  	num_pbm_ranges = i / sizeof(*pbm_ranges);
> +	memset(&pbm->mem64_space, 0, sizeof(struct resource));
>  
>  	for (i = 0; i < num_pbm_ranges; i++) {
>  		const struct linux_prom_pci_ranges *pr = &pbm_ranges[i];
> @@ -451,7 +452,11 @@ void pci_determine_mem_io_space(struct pci_pbm_info *pbm)
>  			break;
>  
>  		case 3:
> -			/* XXX 64-bit MEM handling XXX */
> +			/* 64-bit MEM handling */
> +			pbm->mem64_space.start = a;
> +			pbm->mem64_space.end = a + size - 1UL;
> +			pbm->mem64_space.flags = IORESOURCE_MEM;
> +			break;
>  
>  		default:
>  			break;
> @@ -465,15 +470,21 @@ void pci_determine_mem_io_space(struct pci_pbm_info *pbm)
>  		prom_halt();
>  	}
>  
> -	printk("%s: PCI IO[%llx] MEM[%llx]\n",
> +	printk("%s: PCI IO[%llx] MEM[%llx]",
>  	       pbm->name,
>  	       pbm->io_space.start,
>  	       pbm->mem_space.start);
> +	if (pbm->mem64_space.flags)
> +		printk(" MEM64[%llx]",
> +		       pbm->mem64_space.start);
> +	printk("\n");
>  
>  	pbm->io_space.name = pbm->mem_space.name = pbm->name;
>  
>  	request_resource(&ioport_resource, &pbm->io_space);
>  	request_resource(&iomem_resource, &pbm->mem_space);
> +	if (pbm->mem64_space.flags)
> +		request_resource(&iomem_resource, &pbm->mem64_space);
>  
>  	pci_register_legacy_regions(&pbm->io_space,
>  				    &pbm->mem_space);
> diff --git a/arch/sparc/kernel/pci_impl.h b/arch/sparc/kernel/pci_impl.h
> index 75803c7..37222ca 100644
> --- a/arch/sparc/kernel/pci_impl.h
> +++ b/arch/sparc/kernel/pci_impl.h
> @@ -97,6 +97,7 @@ struct pci_pbm_info {
>  	/* PBM I/O and Memory space resources. */
>  	struct resource			io_space;
>  	struct resource			mem_space;
> +	struct resource			mem64_space;
>  	struct resource			busn;
>  
>  	/* Base of PCI Config space, can be per-PBM or shared. */
> -- 
> 1.8.4.5
> 

[PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Yinghai Lu]

We still get "no compatible bridge window" warning on sparc T5-8
after we add support for 64bit resource parsing for root bus.

 PCI: scan_bus[/pci@300/pci@1/pci@0/pci@6] bus no 8
 PCI: Claiming 0000:00:01.0: Resource 15: 0000800100000000..00008004afffffff [220c]
 PCI: Claiming 0000:01:00.0: Resource 15: 0000800100000000..00008004afffffff [220c]
 PCI: Claiming 0000:02:04.0: Resource 15: 0000800100000000..000080012fffffff [220c]
 PCI: Claiming 0000:03:00.0: Resource 15: 0000800100000000..000080012fffffff [220c]
 PCI: Claiming 0000:04:06.0: Resource 14: 0000800100000000..000080010fffffff [220c]
 PCI: Claiming 0000:05:00.0: Resource 0: 0000800100000000..0000800100001fff [204]
 pci 0000:05:00.0: can't claim BAR 0 [mem 0x800100000000-0x800100001fff]: no compatible bridge window

All the bridges 64-bit resource have pref bit, but the device resource does not
have pref set, then we can not find parent for the device resource,
as we can not put non-pref mem under pref mem.

According to pcie spec errta
https://www.pcisig.com/specifications/pciexpress/base2/PCIe_Base_r2.1_Errata_08Jun10.pdf
page 13, in some case it is ok to mark some as pref.

Only set pref for 64bit mmio when the entire path from the host to the adapter is
over PCI Express.

Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
Reported-by: David Ahern <david.ahern@oracle.com>
Tested-by: David Ahern <david.ahern@oracle.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: <stable@vger.kernel.org> #3.19
---
 drivers/pci/probe.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index f71cb7c..4e0cd46 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1507,6 +1507,53 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_enable_acs(dev);
 }
 
+static bool pci_up_path_over_pcie(struct pci_bus *bus)
+{
+	if (!bus)
+		return true;
+
+	if (bus->self && !pci_is_pcie(bus->self))
+		return false;
+
+	return pci_up_path_over_pcie(bus->parent);
+}
+
+/*
+ * According to
+ * https://www.pcisig.com/specifications/pciexpress/base2/PCIe_Base_r2.1_Errata_08Jun10.pdf
+ * page 13, system firmware could put some 64bit non-pref under 64bit pref,
+ * on some cases.
+ * Let's set pref bit for 64bit mmio when entire path from the host to
+ * the adapter is over PCI Express.
+ */
+static void set_pcie_64bit_pref(struct pci_dev *dev)
+{
+	int i;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	if (!pci_up_path_over_pcie(dev->bus))
+		return;
+
+	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
+		struct resource *res = &dev->resource[i];
+		enum pci_bar_type type;
+		int reg;
+
+		if (!(res->flags & IORESOURCE_MEM_64))
+			continue;
+
+		if (res->flags & IORESOURCE_PREFETCH)
+			continue;
+
+		reg = pci_resource_bar(dev, i, &type);
+		dev_printk(KERN_DEBUG, &dev->dev, "reg 0x%x %pR + pref\n",
+			   reg, res);
+		res->flags |= IORESOURCE_PREFETCH;
+	}
+}
+
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
 	int ret;
@@ -1536,6 +1583,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	/* Initialize various capabilities */
 	pci_init_capabilities(dev);
 
+	/* After pcie_cap is assigned and sriov bar is probed */
+	set_pcie_64bit_pref(dev);
+
 	/*
 	 * Add the device to our list of discovered devices
 	 * and the bus list for fixup functions, etc.
-- 
1.8.4.5

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Bjorn Helgaas]

On Tue, Mar 31, 2015 at 07:57:49PM -0700, Yinghai Lu wrote:
> We still get "no compatible bridge window" warning on sparc T5-8
> after we add support for 64bit resource parsing for root bus.
> 
>  PCI: scan_bus[/pci@300/pci@1/pci@0/pci@6] bus no 8
>  PCI: Claiming 0000:00:01.0: Resource 15: 0000800100000000..00008004afffffff [220c]
>  PCI: Claiming 0000:01:00.0: Resource 15: 0000800100000000..00008004afffffff [220c]
>  PCI: Claiming 0000:02:04.0: Resource 15: 0000800100000000..000080012fffffff [220c]
>  PCI: Claiming 0000:03:00.0: Resource 15: 0000800100000000..000080012fffffff [220c]
>  PCI: Claiming 0000:04:06.0: Resource 14: 0000800100000000..000080010fffffff [220c]
>  PCI: Claiming 0000:05:00.0: Resource 0: 0000800100000000..0000800100001fff [204]
>  pci 0000:05:00.0: can't claim BAR 0 [mem 0x800100000000-0x800100001fff]: no compatible bridge window
> 
> All the bridges 64-bit resource have pref bit, but the device resource does not
> have pref set, then we can not find parent for the device resource,
> as we can not put non-pref mem under pref mem.
> 
> According to pcie spec errta
> https://www.pcisig.com/specifications/pciexpress/base2/PCIe_Base_r2.1_Errata_08Jun10.pdf
> page 13, in some case it is ok to mark some as pref.

That implementation note is included in the PCIe spec r3.0, sec 7.5.2.1.  I
think that's a better reference to cite.

The most straightforward way to read the implementation note is that it is
safe for the *device*, i.e., the hardware, to set the Prefetchable bit in
some BARs, even if those BARs don't actually support prefetching.  But I
don't think the device can really tell when it would be safe, because the
device can't tell whether the entire path leading to it is over PCIe.

I don't think it makes sense for *software* to set the Prefetchable bit in
a BAR, because the spec says bits 0-3 (including Prefetchable) are supposed
to be read-only (conventional PCI spec r3.0, sec 6.2.5.1, p226).

If the intent were to suggest that system software could *treat* a
non-prefetchable BAR as though it were prefetchable, that would make sense,
and it would have been easy to write the implementation note to actually
say that.

I guess I'm OK with doing this as in your patch below even though it really
doesn't match the language in the spec, because I can't see any other
useful way to interpret the spec.

But this is a general change that affects all platforms, and it's late in
the cycle for something as invasive as this.  I'd rather include your patch
in the v4.1 merge window, and revert d63e2e1f3df9 ("sparc/PCI: Clip bridge
windows to fit in upstream windows") for v4.0.

Bjorn

> Only set pref for 64bit mmio when the entire path from the host to the adapter is
> over PCI Express.
> 
> Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
> Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
> Reported-by: David Ahern <david.ahern@oracle.com>
> Tested-by: David Ahern <david.ahern@oracle.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: <stable@vger.kernel.org> #3.19
> ---
>  drivers/pci/probe.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f71cb7c..4e0cd46 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1507,6 +1507,53 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	pci_enable_acs(dev);
>  }
>  
> +static bool pci_up_path_over_pcie(struct pci_bus *bus)
> +{
> +	if (!bus)
> +		return true;
> +
> +	if (bus->self && !pci_is_pcie(bus->self))
> +		return false;
> +
> +	return pci_up_path_over_pcie(bus->parent);
> +}
> +
> +/*
> + * According to
> + * https://www.pcisig.com/specifications/pciexpress/base2/PCIe_Base_r2.1_Errata_08Jun10.pdf
> + * page 13, system firmware could put some 64bit non-pref under 64bit pref,
> + * on some cases.
> + * Let's set pref bit for 64bit mmio when entire path from the host to
> + * the adapter is over PCI Express.
> + */
> +static void set_pcie_64bit_pref(struct pci_dev *dev)
> +{
> +	int i;
> +
> +	if (!pci_is_pcie(dev))
> +		return;
> +
> +	if (!pci_up_path_over_pcie(dev->bus))
> +		return;
> +
> +	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
> +		struct resource *res = &dev->resource[i];
> +		enum pci_bar_type type;
> +		int reg;
> +
> +		if (!(res->flags & IORESOURCE_MEM_64))
> +			continue;
> +
> +		if (res->flags & IORESOURCE_PREFETCH)
> +			continue;
> +
> +		reg = pci_resource_bar(dev, i, &type);
> +		dev_printk(KERN_DEBUG, &dev->dev, "reg 0x%x %pR + pref\n",
> +			   reg, res);
> +		res->flags |= IORESOURCE_PREFETCH;
> +	}
> +}
> +
>  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  {
>  	int ret;
> @@ -1536,6 +1583,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  	/* Initialize various capabilities */
>  	pci_init_capabilities(dev);
>  
> +	/* After pcie_cap is assigned and sriov bar is probed */
> +	set_pcie_64bit_pref(dev);
> +
>  	/*
>  	 * Add the device to our list of discovered devices
>  	 * and the bus list for fixup functions, etc.
> -- 
> 1.8.4.5
> 

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Yinghai Lu]

On Mon, Apr 6, 2015 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> But this is a general change that affects all platforms, and it's late in
> the cycle for something as invasive as this.  I'd rather include your patch
> in the v4.1 merge window, and revert d63e2e1f3df9 ("sparc/PCI: Clip bridge
> windows to fit in upstream windows") for v4.0.

How about applying first two in this patchset for v4.0, and leaving
this one for v4.1?

Thanks

Yinghai

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Bjorn Helgaas]

On Mon, Apr 06, 2015 at 03:35:40PM -0700, Yinghai Lu wrote:
> On Mon, Apr 6, 2015 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 
> > But this is a general change that affects all platforms, and it's late in
> > the cycle for something as invasive as this.  I'd rather include your patch
> > in the v4.1 merge window, and revert d63e2e1f3df9 ("sparc/PCI: Clip bridge
> > windows to fit in upstream windows") for v4.0.
> 
> How about applying first two in this patchset for v4.0, and leaving
> this one for v4.1?

For "[PATCH 1/3] PCI: Introduce pci_bus_addr_t", I'm waiting for an updated
version with Kconfig tweaks so we don't break other arches.

For "[PATCH 2/3] sparc/PCI: Add mem64 resource parsing for root bus", I'm
waiting for a version that fixes the other of_bus_pci_get_flags() and
pci_parse_of_flags() implementations at the same time (or an explanation
about why we should fix only the arch/sparc version).  I don't want to fix
one place and leave the same bug in other places.

Bjorn

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Yinghai Lu]

On Mon, Apr 6, 2015 at 3:49 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> For "[PATCH 1/3] PCI: Introduce pci_bus_addr_t", I'm waiting for an updated
> version with Kconfig tweaks so we don't break other arches.

I was thinking that you will update it manually.

>
> For "[PATCH 2/3] sparc/PCI: Add mem64 resource parsing for root bus", I'm
> waiting for a version that fixes the other of_bus_pci_get_flags() and
> pci_parse_of_flags() implementations at the same time (or an explanation
> about why we should fix only the arch/sparc version).  I don't want to fix
> one place and leave the same bug in other places.

I don't even know if other arch like powerpc support 64-bit bus address.

No one from powerpc reported a problem, why should we mess it up now?

I would like to see someone get access those three kinds of machine that support
of to unify of support code.

Thanks

Yinghai

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Bjorn Helgaas]

On Mon, Apr 6, 2015 at 8:13 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Apr 6, 2015 at 3:49 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> For "[PATCH 1/3] PCI: Introduce pci_bus_addr_t", I'm waiting for an updated
>> version with Kconfig tweaks so we don't break other arches.
>
> I was thinking that you will update it manually.

I asked you for an updated version, incorporating the documentation
updates, to make sure I got everything you intended.  But I did go
ahead and do it manually for you.

>> For "[PATCH 2/3] sparc/PCI: Add mem64 resource parsing for root bus", I'm
>> waiting for a version that fixes the other of_bus_pci_get_flags() and
>> pci_parse_of_flags() implementations at the same time (or an explanation
>> about why we should fix only the arch/sparc version).  I don't want to fix
>> one place and leave the same bug in other places.
>
> I don't even know if other arch like powerpc support 64-bit bus address.
>
> No one from powerpc reported a problem, why should we mess it up now?
>
> I would like to see someone get access those three kinds of machine that support
> of to unify of support code.

Of course changes there should be tested on all the affected machines.
I opened https://bugzilla.kernel.org/show_bug.cgi?id=96241 and
assigned it to you as a reminder that there is nearly identical code
in several other places that may have the same issue.

I pushed these two changes to for-linus.  I'll work on the third
tomorrow.  The current changelog is very sparc64-centric, and it needs
to be much more explicit about how the change will affect every arch.

Bjorn

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Yinghai Lu]

On Mon, Apr 6, 2015 at 8:43 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Apr 6, 2015 at 8:13 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Mon, Apr 6, 2015 at 3:49 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>> For "[PATCH 1/3] PCI: Introduce pci_bus_addr_t", I'm waiting for an updated
>>> version with Kconfig tweaks so we don't break other arches.
>>
>> I was thinking that you will update it manually.
>
> I asked you for an updated version, incorporating the documentation
> updates, to make sure I got everything you intended.  But I did go
> ahead and do it manually for you.

Good.

http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=for-linus&id=f70899ff889a38f9697d3c153aaacaed25f501c3

Please consider to split that into two patches.
First one include changes in : include/linux/types.h and
Documentation/DMA-API-HOWTO.txt.
and you should be author for that.


>
>>> For "[PATCH 2/3] sparc/PCI: Add mem64 resource parsing for root bus", I'm
>>> waiting for a version that fixes the other of_bus_pci_get_flags() and
>>> pci_parse_of_flags() implementations at the same time (or an explanation
>>> about why we should fix only the arch/sparc version).  I don't want to fix
>>> one place and leave the same bug in other places.
>>
>> I don't even know if other arch like powerpc support 64-bit bus address.
>>
>> No one from powerpc reported a problem, why should we mess it up now?
>>
>> I would like to see someone get access those three kinds of machine that support
>> of to unify of support code.
>
> Of course changes there should be tested on all the affected machines.
> I opened https://bugzilla.kernel.org/show_bug.cgi?id=96241 and
> assigned it to you as a reminder that there is nearly identical code
> in several other places that may have the same issue.

ok, will on that to have three patches cover them.

>
> I pushed these two changes to for-linus.  I'll work on the third
> tomorrow.  The current changelog is very sparc64-centric, and it needs
> to be much more explicit about how the change will affect every arch.

Sure. That will affect all platform.

Thanks

Yinghai

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Bjorn Helgaas]

On Tue, Apr 7, 2015 at 12:23 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Apr 6, 2015 at 8:43 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Mon, Apr 6, 2015 at 8:13 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Mon, Apr 6, 2015 at 3:49 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>>
>>>> For "[PATCH 1/3] PCI: Introduce pci_bus_addr_t", I'm waiting for an updated
>>>> version with Kconfig tweaks so we don't break other arches.
>>>
>>> I was thinking that you will update it manually.
>>
>> I asked you for an updated version, incorporating the documentation
>> updates, to make sure I got everything you intended.  But I did go
>> ahead and do it manually for you.
>
> Good.
>
> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=for-linus&id=f70899ff889a38f9697d3c153aaacaed25f501c3
>
> Please consider to split that into two patches.
> First one include changes in : include/linux/types.h and
> Documentation/DMA-API-HOWTO.txt.
> and you should be author for that.

If you want something different, send patches.  I don't have time to
try to figure out your intent and do the work for you.  That's why I
asked you for an updated series in the first place.

Bjorn

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Mon, 6 Apr 2015 17:06:38 -0500

> But this is a general change that affects all platforms, and it's late in
> the cycle for something as invasive as this.  I'd rather include your patch
> in the v4.1 merge window, and revert d63e2e1f3df9 ("sparc/PCI: Clip bridge
> windows to fit in upstream windows") for v4.0.

I would kindly ask that we not proceed this way and use the change
which implements the fix properly.

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Bjorn Helgaas]

On Mon, Apr 6, 2015 at 7:35 PM, David Miller <davem@davemloft.net> wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Mon, 6 Apr 2015 17:06:38 -0500
>
>> But this is a general change that affects all platforms, and it's late in
>> the cycle for something as invasive as this.  I'd rather include your patch
>> in the v4.1 merge window, and revert d63e2e1f3df9 ("sparc/PCI: Clip bridge
>> windows to fit in upstream windows") for v4.0.
>
> I would kindly ask that we not proceed this way and use the change
> which implements the fix properly.

I reworked the changelog and pushed this to my for-linus branch for
v4.0.  I'll ask Linus to pull it tomorrow.

https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=for-linus&id=1a3ec5e7b00dcd9cac24efe3d65bfccf82597ce5

Thanks for your help.

Bjorn

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Bjorn Helgaas]

[+cc Ben]

On Tue, Apr 07, 2015 at 11:48:44AM -0500, Bjorn Helgaas wrote:
> On Mon, Apr 6, 2015 at 7:35 PM, David Miller <davem@davemloft.net> wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > Date: Mon, 6 Apr 2015 17:06:38 -0500
> >
> >> But this is a general change that affects all platforms, and it's late in
> >> the cycle for something as invasive as this.  I'd rather include your patch
> >> in the v4.1 merge window, and revert d63e2e1f3df9 ("sparc/PCI: Clip bridge
> >> windows to fit in upstream windows") for v4.0.
> >
> > I would kindly ask that we not proceed this way and use the change
> > which implements the fix properly.
> 
> I reworked the changelog and pushed this to my for-linus branch for
> v4.0.  I'll ask Linus to pull it tomorrow.
> 
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=for-linus&id=1a3ec5e7b00dcd9cac24efe3d65bfccf82597ce5

Based on Ben's concerns, I dropped Yinghai's three patches for now:

  PCI: Add pci_bus_addr_t
  sparc/PCI: Parse 64-bit host bridge windows from OF
  PCI: Allow non-prefetchable PCIe BARs in prefetchable windows

and reverted d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in
upstream windows") instead.

I pushed this to my for-linus branch, which I intend to merge for v4.0.
You can browse it here to see if you agree:

  https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=for-linus

Dave, I know you weren't happy with this proposal the first time around,
but I'm not sure why.  If you have a better idea, let me know.

Bjorn

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Wed, 8 Apr 2015 10:47:59 -0500

> Dave, I know you weren't happy with this proposal the first time around,
> but I'm not sure why.  If you have a better idea, let me know.

At this point getting things working again is more important I guess,
so I'm fine with this for now.

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Benjamin Herrenschmidt]

On Wed, 2015-04-08 at 10:47 -0500, Bjorn Helgaas wrote:

> Based on Ben's concerns, I dropped Yinghai's three patches for now:
> 
>   PCI: Add pci_bus_addr_t
>   sparc/PCI: Parse 64-bit host bridge windows from OF
>   PCI: Allow non-prefetchable PCIe BARs in prefetchable windows
> 
> and reverted d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in
> upstream windows") instead.
> 
> I pushed this to my for-linus branch, which I intend to merge for v4.0.
> You can browse it here to see if you agree:

Thanks Bjorn. We can fix Yinghai patch for 4.2, it would be indeed handy
even for us to be able to support putting 64-bit NP BARs in prefetch
windows (For some SR-IOV adapters for example) too, but we need to do it
right.

Cheers,
Ben.

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

On Wed, Apr 8, 2015 at 2:12 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:

> Thanks Bjorn. We can fix Yinghai patch for 4.2, it would be indeed handy
> even for us to be able to support putting 64-bit NP BARs in prefetch
> windows (For some SR-IOV adapters for example) too, but we need to do it
> right.

Please check if you are ok with attached.

        Yinghai

[-- Attachment #2: pcie_pref_64bit_mem_v3.patch --]
[-- Type: text/x-patch, Size: 8785 bytes --]

Subject: [PATCH v3] PCI: Set under_pref for mem64 resource of pcie device

We still get "no compatible bridge window" warning on sparc T5-8
after we add support for 64bit resource parsing for root bus.

 PCI: scan_bus[/pci@300/pci@1/pci@0/pci@6] bus no 8
 PCI: Claiming 0000:00:01.0: Resource 15: 0000800100000000..00008004afffffff [220c]
 PCI: Claiming 0000:01:00.0: Resource 15: 0000800100000000..00008004afffffff [220c]
 PCI: Claiming 0000:02:04.0: Resource 15: 0000800100000000..000080012fffffff [220c]
 PCI: Claiming 0000:03:00.0: Resource 15: 0000800100000000..000080012fffffff [220c]
 PCI: Claiming 0000:04:06.0: Resource 14: 0000800100000000..000080010fffffff [220c]
 PCI: Claiming 0000:05:00.0: Resource 0: 0000800100000000..0000800100001fff [204]
 pci 0000:05:00.0: can't claim BAR 0 [mem 0x800100000000-0x800100001fff]: no compatible bridge window

All the bridges 64-bit resource have pref bit, but the device resource does not
have pref set, then we can not find parent for the device resource,
as we can not put non-pref mem under pref mem.

According to pcie spec errta
https://www.pcisig.com/specifications/pciexpress/base2/PCIe_Base_r2.1_Errata_08Jun10.pdf
page 13, in some case it is ok to mark some as pref.

Only set pref for 64bit mmio when the entire path from the host to the adapter is
over PCI Express.

Fixes: commit d63e2e1f3df9 ("sparc/PCI: Clip bridge windows to fit in upstream windows")
Link: http://lkml.kernel.org/r/CAE9FiQU1gJY1LYrxs+ma5LCTEEe4xmtjRG0aXJ9K_Tsu+m9Wuw@mail.gmail.com
Reported-by: David Ahern <david.ahern@oracle.com>
Tested-by: David Ahern <david.ahern@oracle.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc:  <stable@vger.kernel.org> #3.19

---
-v2: set pref for mmio 64 when whole path is PCI Express, according to David Miller.
-v3: don't set pref directly, change to UNDER_PREF, and set PREF before
     sizing and assign resource, and cleart PREF afterwards. requested by BenH.
---


---
 drivers/pci/bus.c       |    9 +++++++-
 drivers/pci/pci.h       |   11 ++++++++++
 drivers/pci/probe.c     |   50 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/setup-bus.c |    8 ++++++-
 drivers/pci/setup-res.c |   10 +++++++++
 include/linux/ioport.h  |    2 +
 6 files changed, 88 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/pci/bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/bus.c
+++ linux-2.6/drivers/pci/bus.c
@@ -139,6 +139,8 @@ static int pci_bus_alloc_from_region(str
 
 	type_mask |= IORESOURCE_TYPE_BITS;
 
+	pci_set_pref_under_pref(res);
+
 	pci_bus_for_each_resource(bus, r, i) {
 		if (!r)
 			continue;
@@ -170,9 +172,14 @@ static int pci_bus_alloc_from_region(str
 		/* Ok, try it out.. */
 		ret = allocate_resource(r, res, size, min, max,
 					align, alignf, alignf_data);
-		if (ret == 0)
+		if (ret == 0) {
+			pci_clear_pref_under_pref(res);
 			return 0;
+		}
 	}
+
+	pci_clear_pref_under_pref(res);
+
 	return -ENOMEM;
 }
 
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -216,6 +216,17 @@ void __pci_bus_assign_resources(const st
 				struct list_head *fail_head);
 bool pci_bus_clip_resource(struct pci_dev *dev, int idx);
 
+static inline void pci_set_pref_under_pref(struct resource *res)
+{
+	if (res->flags & IORESOURCE_UNDER_PREF)
+		res->flags |= IORESOURCE_PREFETCH;
+}
+static inline void pci_clear_pref_under_pref(struct resource *res)
+{
+	if (res->flags & IORESOURCE_UNDER_PREF)
+		res->flags &= ~IORESOURCE_PREFETCH;
+}
+
 /**
  * pci_ari_enabled - query ARI forwarding status
  * @bus: the PCI bus
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -1508,6 +1508,53 @@ static void pci_init_capabilities(struct
 	pci_enable_acs(dev);
 }
 
+static bool pci_up_path_over_pcie(struct pci_bus *bus)
+{
+	if (!bus)
+		return true;
+
+	if (bus->self && !pci_is_pcie(bus->self))
+		return false;
+
+	return pci_up_path_over_pcie(bus->parent);
+}
+
+/*
+ * According to
+ * https://www.pcisig.com/specifications/pciexpress/base2/PCIe_Base_r2.1_Errata_08Jun10.pdf
+ * page 13, system firmware could put some 64bit non-pref under 64bit pref,
+ * on some cases.
+ * Let's set pref bit for 64bit mmio when entire path from the host to
+ * the adapter is over PCI Express.
+ */
+static void set_pcie_64bit_under_pref(struct pci_dev *dev)
+{
+	int i;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	if (!pci_up_path_over_pcie(dev->bus))
+		return;
+
+	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
+		struct resource *res = &dev->resource[i];
+		enum pci_bar_type type;
+		int reg;
+
+		if (!(res->flags & IORESOURCE_MEM_64))
+			continue;
+
+		if (res->flags & IORESOURCE_PREFETCH)
+			continue;
+
+		reg = pci_resource_bar(dev, i, &type);
+		dev_printk(KERN_DEBUG, &dev->dev, "reg 0x%x %pR + under_pref\n",
+			   reg, res);
+		res->flags |= IORESOURCE_UNDER_PREF;
+	}
+}
+
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
 	int ret;
@@ -1538,6 +1585,9 @@ void pci_device_add(struct pci_dev *dev,
 	/* Initialize various capabilities */
 	pci_init_capabilities(dev);
 
+	/* After pcie_cap is assigned and sriov bar is probed */
+	set_pcie_64bit_under_pref(dev);
+
 	/*
 	 * Add the device to our list of discovered devices
 	 * and the bus list for fixup functions, etc.
Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -977,10 +977,13 @@ static int pbus_size_mem(struct pci_bus
 			struct resource *r = &dev->resource[i];
 			resource_size_t r_size;
 
+			pci_set_pref_under_pref(r);
 			if (r->parent || ((r->flags & mask) != type &&
 					  (r->flags & mask) != type2 &&
-					  (r->flags & mask) != type3))
+					  (r->flags & mask) != type3)) {
+				pci_clear_pref_under_pref(r);
 				continue;
+			}
 			r_size = resource_size(r);
 #ifdef CONFIG_PCI_IOV
 			/* put SRIOV requested res to the optional list */
@@ -989,6 +992,7 @@ static int pbus_size_mem(struct pci_bus
 				r->end = r->start - 1;
 				add_to_list(realloc_head, dev, r, r_size, 0/* don't care */);
 				children_add_size += r_size;
+				pci_clear_pref_under_pref(r);
 				continue;
 			}
 #endif
@@ -1018,6 +1022,8 @@ static int pbus_size_mem(struct pci_bus
 
 			if (realloc_head)
 				children_add_size += get_res_add_size(realloc_head, r);
+
+			pci_clear_pref_under_pref(r);
 		}
 	}
 
Index: linux-2.6/drivers/pci/setup-res.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-res.c
+++ linux-2.6/drivers/pci/setup-res.c
@@ -116,11 +116,14 @@ int pci_claim_resource(struct pci_dev *d
 		return -EINVAL;
 	}
 
+	pci_set_pref_under_pref(res);
+
 	root = pci_find_parent_resource(dev, res);
 	if (!root) {
 		dev_info(&dev->dev, "can't claim BAR %d %pR: no compatible bridge window\n",
 			 resource, res);
 		res->flags |= IORESOURCE_UNSET;
+		pci_clear_pref_under_pref(res);
 		return -EINVAL;
 	}
 
@@ -129,9 +132,12 @@ int pci_claim_resource(struct pci_dev *d
 		dev_info(&dev->dev, "can't claim BAR %d %pR: address conflict with %s %pR\n",
 			 resource, res, conflict->name, conflict);
 		res->flags |= IORESOURCE_UNSET;
+		pci_clear_pref_under_pref(res);
 		return -EBUSY;
 	}
 
+	pci_clear_pref_under_pref(res);
+
 	return 0;
 }
 EXPORT_SYMBOL(pci_claim_resource);
@@ -250,9 +256,11 @@ static int __pci_assign_resource(struct
 static int _pci_assign_resource(struct pci_dev *dev, int resno,
 				resource_size_t size, resource_size_t min_align)
 {
+	struct resource *res = dev->resource + resno;
 	struct pci_bus *bus;
 	int ret;
 
+	pci_set_pref_under_pref(res);
 	bus = dev->bus;
 	while ((ret = __pci_assign_resource(bus, dev, resno, size, min_align))) {
 		if (!bus->parent || !bus->self->transparent)
@@ -260,6 +268,8 @@ static int _pci_assign_resource(struct p
 		bus = bus->parent;
 	}
 
+	pci_clear_pref_under_pref(res);
+
 	return ret;
 }
 
Index: linux-2.6/include/linux/ioport.h
===================================================================
--- linux-2.6.orig/include/linux/ioport.h
+++ linux-2.6/include/linux/ioport.h
@@ -49,6 +49,8 @@ struct resource {
 #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
 #define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
 
+#define IORESOURCE_UNDER_PREF	0x00800000	/* non-pref could under pref */
+
 #define IORESOURCE_EXCLUSIVE	0x08000000	/* Userland may not map this resource */
 #define IORESOURCE_DISABLED	0x10000000
 #define IORESOURCE_UNSET	0x20000000	/* No address assigned yet */

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Benjamin Herrenschmidt]

On Wed, 2015-04-08 at 17:06 -0700, Yinghai Lu wrote:
> On Wed, Apr 8, 2015 at 2:12 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> 
> > Thanks Bjorn. We can fix Yinghai patch for 4.2, it would be indeed handy
> > even for us to be able to support putting 64-bit NP BARs in prefetch
> > windows (For some SR-IOV adapters for example) too, but we need to do it
> > right.
> 
> Please check if you are ok with attached.

I'll let Bjorn be the final judge here but I am not fan of the way you
set/clear/set/clear the IORESOURCE_PREFETCH bit with
pci_set_pref_under_pref(). It's error prone and confusing, the code is
already barely readable as it is ...

I would rather you replace those various masks compares with a helper
that does something like pci_resource_compatible(parent_res, child_res),
which has the logic to test. That or a helper that does something like
pci_calc_compatible_res_flags() which returns a "flags" that has
PREFETCH set, which you use in place of res->flags in the various
allocation path.

As-is, your patch looks like a band-aid and smells like a hack :-)

Ben.

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Yinghai Lu]

On Wed, Apr 8, 2015 at 8:17 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2015-04-08 at 17:06 -0700, Yinghai Lu wrote:

> I'll let Bjorn be the final judge here but I am not fan of the way you
> set/clear/set/clear the IORESOURCE_PREFETCH bit with
> pci_set_pref_under_pref(). It's error prone and confusing, the code is
> already barely readable as it is ...

We only use them to wrapping final pci_claim*, sizing, and assigning.
that is limited places.

>
> I would rather you replace those various masks compares with a helper
> that does something like pci_resource_compatible(parent_res, child_res),
> which has the logic to test. That or a helper that does something like
> pci_calc_compatible_res_flags() which returns a "flags" that has
> PREFETCH set, which you use in place of res->flags in the various
> allocation path.

That way should be more intrusive to current code, as we are using
type_mask checking to share the code among
parent(pref)/child(pref), parent(no-pref)/child(pref), and
parent(no-pref)/child(pref)

        Yinghai

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Benjamin Herrenschmidt]

On Wed, 2015-04-08 at 21:11 -0700, Yinghai Lu wrote:
> That way should be more intrusive to current code, as we are using
> type_mask checking to share the code among
> parent(pref)/child(pref), parent(no-pref)/child(pref), and
> parent(no-pref)/child(pref)

That's fine, as long as the helper knows which one is the parent and
which one is the child.

In fact, as "cute" as the mask trick is, I think it would generally make
the code more self explanatory if it explicitly tested for the
combinations that are supported, ie something like

	if (parent_is_pref && !child_is_pref && child->pcie_only)
		...
	else if (!parent_is_pref)
		...
etc...

The impact in performance would be in the noise and the logic of the
algorithm a LOT more explicit.

Cheers,
Ben.

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Bjorn Helgaas]

On Wed, Apr 8, 2015 at 10:17 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2015-04-08 at 17:06 -0700, Yinghai Lu wrote:
>> On Wed, Apr 8, 2015 at 2:12 PM, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>>
>> > Thanks Bjorn. We can fix Yinghai patch for 4.2, it would be indeed handy
>> > even for us to be able to support putting 64-bit NP BARs in prefetch
>> > windows (For some SR-IOV adapters for example) too, but we need to do it
>> > right.
>>
>> Please check if you are ok with attached.
>
> I'll let Bjorn be the final judge here but I am not fan of the way you
> set/clear/set/clear the IORESOURCE_PREFETCH bit with
> pci_set_pref_under_pref(). It's error prone and confusing, the code is
> already barely readable as it is ...
>
> I would rather you replace those various masks compares with a helper
> that does something like pci_resource_compatible(parent_res, child_res),
> which has the logic to test. That or a helper that does something like
> pci_calc_compatible_res_flags() which returns a "flags" that has
> PREFETCH set, which you use in place of res->flags in the various
> allocation path.

I'm not planning to review this until after the merge window opens,
but I took a quick glance, and I agree with Ben.  I don't want to add
a new IORESOURCE_ flag.  I think a pci_resource_compatible() helper is
a great idea.

I am absolutely not in favor of "minimally intrusive" as a goal.
"Minimally intrusive" sounds good but it is often used to justify
clever hacks which end up being an anti-maintainer strategy in the
long term.  "Maximum readability" is what I'm looking for.

Bjorn

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Benjamin Herrenschmidt]

On Wed, 2015-04-08 at 23:26 -0500, Bjorn Helgaas wrote:
> I'm not planning to review this until after the merge window opens,
> but I took a quick glance, and I agree with Ben.  I don't want to add
> a new IORESOURCE_ flag.  I think a pci_resource_compatible() helper is
> a great idea.

So the new resource flag was handy here still regardless of the
implementation choice because otherwise, we have to do the whole tree
walk to check for "PCI Express only path".

*But*, this is a property of the device as a whole, not of the resource,
so we could instead have a pci_dev flag established at probe time that
indicates that the path to a given device is PCIe only.

The cool thing is that can be very easily done by simply propagating
from the parent. If the parent has it and the device is PCIe, then set
it, the only break would happen on PCIe to PCI bridges, which is exactly
what we are after.

That way, you avoid the special resource flag alltogether.

> I am absolutely not in favor of "minimally intrusive" as a goal.
> "Minimally intrusive" sounds good but it is often used to justify
> clever hacks which end up being an anti-maintainer strategy in the
> long term.  "Maximum readability" is what I'm looking for.

Yup, I agree. The code is too hard to understand already which makes it
bug prone in the long run.

Cheers,
Ben.

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Yinghai Lu]

On Thu, Apr 9, 2015 at 1:54 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2015-04-08 at 23:26 -0500, Bjorn Helgaas wrote:
>> I'm not planning to review this until after the merge window opens,
>> but I took a quick glance, and I agree with Ben.  I don't want to add
>> a new IORESOURCE_ flag.  I think a pci_resource_compatible() helper is
>> a great idea.
>
> So the new resource flag was handy here still regardless of the
> implementation choice because otherwise, we have to do the whole tree
> walk to check for "PCI Express only path".
>
> *But*, this is a property of the device as a whole, not of the resource,
> so we could instead have a pci_dev flag established at probe time that
> indicates that the path to a given device is PCIe only.

> That way, you avoid the special resource flag alltogether.

in the assign path: pci_bus_alloc_resource() does not take dev pointer.

should we make

int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,

to

int pci_bus_alloc_resource(struct pci_bus *bus, struct pci_dev *dev,
struct resource *res,

?

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Benjamin Herrenschmidt]

On Thu, 2015-04-09 at 11:31 -0700, Yinghai Lu wrote:
> On Thu, Apr 9, 2015 at 1:54 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Wed, 2015-04-08 at 23:26 -0500, Bjorn Helgaas wrote:
> >> I'm not planning to review this until after the merge window opens,
> >> but I took a quick glance, and I agree with Ben.  I don't want to add
> >> a new IORESOURCE_ flag.  I think a pci_resource_compatible() helper is
> >> a great idea.
> >
> > So the new resource flag was handy here still regardless of the
> > implementation choice because otherwise, we have to do the whole tree
> > walk to check for "PCI Express only path".
> >
> > *But*, this is a property of the device as a whole, not of the resource,
> > so we could instead have a pci_dev flag established at probe time that
> > indicates that the path to a given device is PCIe only.
> 
> > That way, you avoid the special resource flag alltogether.
> 
> in the assign path: pci_bus_alloc_resource() does not take dev pointer.
> 
> should we make
> 
> int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
> 
> to
> 
> int pci_bus_alloc_resource(struct pci_bus *bus, struct pci_dev *dev,
> struct resource *res,

Do you need to pass bus if you have dev ?

Do we have any caller that doesn't have dev available ?

Cheers,
Ben.

> ?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device

[Yinghai Lu]

On Thu, Apr 9, 2015 at 4:31 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>> should we make
>>
>> int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
>>
>> to
>>
>> int pci_bus_alloc_resource(struct pci_bus *bus, struct pci_dev *dev,
>> struct resource *res,
>
> Do you need to pass bus if you have dev ?

in following path:

_pci_assign_resource only take dev, and it will go up for parent bus

        while ((ret = __pci_assign_resource(bus, dev, resno, size,
                                                min_align, fit))) {
                if (!bus->parent || !bus->self->transparent)
                        break;
                bus = bus->parent;
        }

and __pci_assign_resource will call pci_bus_alloc_resource.

so it is could go up several levels to use upper bus there.