Re: [PATCH RFC 08/15] pci: Rework pci_bar_addr()

[Thomas Huth <thuth@redhat.com>]

On 11.04.2016 13:04, Alexander Gordeev wrote:
> This update makes pci_bar_addr() interface 64 bit BARs aware
> and introduces a concept of PCI address translation. In cases
> PCI bus and CPU bus physical addresses are not indentical the
> architecutre should override pci_xslate_addr() interface to
> provide necessary mapping between the two.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/asm-generic/pci.h |  8 +++++
>  lib/pci.c             | 84 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  lib/pci.h             |  4 ++-
>  3 files changed, 87 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/asm-generic/pci.h b/lib/asm-generic/pci.h
> index 175b0497ed82..15f23079f27e 100644
> --- a/lib/asm-generic/pci.h
> +++ b/lib/asm-generic/pci.h
> @@ -14,4 +14,12 @@ static inline void pci_shutdown(void)
>  }
>  #endif
>  
> +#ifndef pci_xslate_addr
> +static inline
> +phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr)
> +{
> +	return addr;
> +}
> +#endif
> +
>  #endif
> diff --git a/lib/pci.c b/lib/pci.c
> index 88c7d2d06fc2..43e9c0c38434 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -9,23 +9,78 @@
>  /* Scan bus look for a specific device. Only bus 0 scanned for now. */
>  pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
>  {
> -	unsigned dev;
> -	for (dev = 0; dev < 256; ++dev) {
> -		uint32_t id = pci_config_readl(dev, 0);
> -		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id) {
> +	pcidevaddr_t dev;
> +
> +	for (dev = 0; dev < 256; dev++) {
> +		if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id &&
> +		    pci_config_readw(dev, PCI_DEVICE_ID) == device_id) {
>  			return dev;
>  		}
>  	}
> +
>  	return PCIDEVADDR_INVALID;
>  }
>  
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> +static phys_addr_t pci_bar_mask(uint32_t bar)
> +{
> +	return (bar & PCI_BASE_ADDRESS_SPACE_IO) ?
> +		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
> +}
> +
> +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
> +{
> +	int off;
> +	uint32_t bar;
> +	uint64_t addr, mask;
> +
> +	off = PCI_BASE_ADDRESS_0 + bar_num * 4;

Looks like you also added now support for 64-bit bars below ... but is
the above calculation still valid if there is a 64-bit bar inbetween?
E.g. what is this function supposed to do if the first bar is a 64-bit bar?
Is the caller supposed to use bar_num = 2 for the second bar, or bar_num = 1?
... maybe a comment in front of the function would be good here.

> +	bar = pci_config_readl(dev, off);
> +	mask = pci_bar_mask(bar);
> +
> +	if (pci_bar_is64(dev, bar_num)) {
> +		uint32_t addr_high = pci_config_readl(dev, off + 4);
> +		addr = (((uint64_t)addr_high << 32) | bar) & mask;
> +	} else {
> +		addr = bar & mask;
> +	}
> +
> +	return pci_xlate_addr(dev, addr);
> +}
> +
> +/*
> + * To determine the amount of address space needed by a PCI device,
> + * one must save the original value of the BAR, write a value of
> + * all 1's to the register, then read it back. The amount of memory
> + * can then be then determined by masking the information bits,
> + * performing a bitwise NOT and incrementing the value by 1.
> + *
> + * The following pci_bar_size32() and pci_bar_size() functions do
> + * the described algorithm.
> + */                                                                     
> +static uint32_t pci_bar_size32(pcidevaddr_t dev, int bar_num)
> +{
> +	int off = PCI_BASE_ADDRESS_0 + (bar_num * 4);
> +	uint32_t bar, size;
> +
> +	bar = pci_config_readl(dev, off);
> +	pci_config_writel(dev, off, ~0u);
> +	size = pci_config_readl(dev, off);
> +	pci_config_writel(dev, off, bar);
> +
> +	return size;
> +}
> +
> +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
>  {
>  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> -	if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
> -		return bar & PCI_BASE_ADDRESS_IO_MASK;
> +	uint32_t size = pci_bar_size32(dev, bar_num);
> +	phys_addr_t mask = pci_bar_mask(bar);
> +
> +	if (pci_bar_is64(dev, bar_num)) {
> +		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> +		return (~((((phys_addr_t)size_high << 32) | size) & mask)) + 1;
>  	} else {
> -		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> +		return (~((phys_addr_t)size & mask)) + 1;
>  	}
>  }

I think you could slightly simplify that function, something like:

phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
{
	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
	phys_addr_t size = pci_bar_size32(dev, bar_num);
	phys_addr_t mask = pci_bar_mask(bar);

	if (pci_bar_is64(dev, bar_num)) {
		size |= ((phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;
	}

	return (~((phys_addr_t)size & mask)) + 1;
}
  
> @@ -40,3 +95,16 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
>  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
>  	return bar;
>  }
> +
> +bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
> +{
> +	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +
> +	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> +		return false;
> +	else if ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +			PCI_BASE_ADDRESS_MEM_TYPE_64)
> +		return true;
> +	else
> +		return false;

More simple:

	else
		return ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
			PCI_BASE_ADDRESS_MEM_TYPE_64);

?

> +}
> diff --git a/lib/pci.h b/lib/pci.h
> index 90bf62bf3b6d..09b500ea19f0 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -15,7 +15,9 @@ enum {
>  	PCIDEVADDR_INVALID = 0xffff,
>  };
>  pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
> +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
> +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
> +bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);

 Thomas