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

[Bjorn Helgaas <bhelgaas@google.com>]

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
>