On Mon, Jun 06, 2016 at 02:46:33PM +0200, Alexander Gordeev wrote:
> This update makes pci_bar_addr() interface 64 bit BARs aware and
> introduces a concept of PCI address translation.
>
> An architecutre should implement pci_translate_addr() interface
> in order to provide mapping between PCI bus address and CPU
> physical address.
>
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> lib/pci.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> lib/pci.h | 16 +++++++++++++-
> lib/x86/asm/pci.h | 6 +++++
> 3 files changed, 82 insertions(+), 6 deletions(-)
>
> diff --git a/lib/pci.c b/lib/pci.c
> index b0d89bf98067..07c911820e20 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -21,14 +21,59 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
> return PCIDEVADDR_INVALID;
> }
>
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> +static uint32_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 = PCI_BASE_ADDRESS_0 + bar_num * 4;
> + phys_addr_t bar = pci_config_readl(dev, off);
> + phys_addr_t mask = (int32_t)pci_bar_mask(bar);
This sign-extension trick is too subtle, IMO, and not really necessary.
We only need to apply the mask to the lower half of a 64-bit bar, i.e.
phys_addr_t bar = pci_config_readl(dev, off);
uint32_t mask = pci_bar_mask(bar);
bar &= mask;
if (pci_bar_is64(dev, bar_num))
bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;
return pci_translate_addr(dev, bar);
> +
> + if (pci_bar_is64(dev, bar_num))
> + bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;
> +
> + return pci_translate_addr(dev, bar & mask);
> +}
> +
> +/*
> + * 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);
> + phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> + phys_addr_t mask = (int32_t)pci_bar_mask(bar);
>
> - if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> - return bar & PCI_BASE_ADDRESS_IO_MASK;
> - else
> - return bar & PCI_BASE_ADDRESS_MEM_MASK;
> + if (pci_bar_is64(dev, bar_num)) {
> + uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> + size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
> + }
> +
> + return (~(size & mask)) + 1;
All this casting of size and mask is pointless. Please rework it
similar to what I did above.
> }
>
> bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
> @@ -42,3 +87,14 @@ 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;
> +
> + 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 88dc47c1f48d..dbfe7918da37 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -15,7 +15,21 @@ 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);
> +
> +/*
> + * BAR number in all BAR access functions below is a number of 32-bit
> + * register starting from PCI_BASE_ADDRESS_0 offset.
> + *
> + * In cases BAR size is 64-bit a caller should still provide BAR number
> + * in terms of 32-bit words. For example, if a device has 64-bit BAR#0
> + * and 32-bit BAR#1 the caller should provide 2 to address BAR#1, not 1.
Is this how people label bars? I.e. they call a bar following a 64-bit
bar BAR#N+1? Or do they skip numbers with the labels to match the
offsets? I.e. in this example you'd say BAR#0 (64-bit), there is no BAR#1,
and then BAR#2 (32-bit) when labeling?
> + *
> + * It is expected the caller is aware of the device BAR layout and never
> + * tries to address in the middle of a 64-bit register.
> + */
> +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);
>
> diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
> index 821a2c1e180a..813dc28b9232 100644
> --- a/lib/x86/asm/pci.h
> +++ b/lib/x86/asm/pci.h
> @@ -35,4 +35,10 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val
> outl(val, 0xCFC);
> }
>
> +static inline
> +phys_addr_t pci_translate_addr(pcidevaddr_t __unused dev, uint64_t addr)
I put the __unused after the argument name.
> +{
> + return addr;
Need tab here.
> +}
> +
> #endif
> --
> 1.8.3.1
>
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
