[kvm-unit-tests PATCH v7 00/13] PCI bus support

[kvm-unit-tests PATCH v7 00/13] PCI bus support

[Alexander Gordeev]

Hi Andrew,

This is 7th version of PCI support. Noticeable changes since v5 & v6:
  - patch "pci: Factor out pci_bar_get()" moved earlier in the series;
  - 'extern' added to public functions in lib/pci.h;
  - PCI test added to arm/unittests.cfg;

Sources are avalable at:
https://github.com/a-gordeev/kvm-unit-tests.git pci-testdev-v7

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>

Alexander Gordeev (13):
  pci: Fix coding style in generic PCI files
  pci: x86: Rename pci_config_read() to pci_config_readl()
  pci: Add 'extern' to public function declarations
  pci: x86: Add remaining PCI configuration space accessors
  pci: Factor out pci_bar_get()
  pci: Rework pci_bar_addr()
  pci: Add pci_bar_set_addr()
  pci: Add pci_dev_exists()
  pci: Add pci_print()
  pci: Add generic ECAM host support
  arm/arm64: pci: Add PCI bus operation test
  pci: Add pci-testdev PCI bus test device
  arm/arm64: pci: Add pci-testdev PCI device operation test

 arm/Makefile.common               |   6 +-
 arm/pci-test.c                    |  27 ++++
 arm/run                           |   7 +-
 arm/unittests.cfg                 |   4 +
 lib/arm/asm/pci.h                 |   1 +
 lib/arm64/asm/pci.h               |   1 +
 lib/asm-generic/pci-host-bridge.h |  26 ++++
 lib/pci-host-generic.c            | 295 ++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h            |  46 ++++++
 lib/pci-testdev.c                 | 192 +++++++++++++++++++++++++
 lib/pci.c                         | 201 +++++++++++++++++++++++---
 lib/pci.h                         |  41 +++++-
 lib/x86/asm/pci.h                 |  31 +++-
 x86/vmexit.c                      |   4 +-
 14 files changed, 851 insertions(+), 31 deletions(-)
 create mode 100644 arm/pci-test.c
 create mode 100644 lib/arm/asm/pci.h
 create mode 100644 lib/arm64/asm/pci.h
 create mode 100644 lib/asm-generic/pci-host-bridge.h
 create mode 100644 lib/pci-host-generic.c
 create mode 100644 lib/pci-host-generic.h
 create mode 100644 lib/pci-testdev.c

-- 
1.8.3.1

[kvm-unit-tests PATCH v7 01/13] pci: Fix coding style in generic PCI files

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c | 38 ++++++++++++++++++++------------------
 lib/pci.h |  3 ++-
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index 0058d70c888d..43cd0ea4f2fa 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -10,34 +10,36 @@
 /* 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_read(dev, 0);
-    if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id) {
-        return dev;
-    }
-    }
-    return PCIDEVADDR_INVALID;
+	pcidevaddr_t dev;
+
+	for (dev = 0; dev < 256; ++dev) {
+		uint32_t id = pci_config_read(dev, 0);
+
+		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
+			return dev;
+	}
+
+	return PCIDEVADDR_INVALID;
 }
 
 unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
-    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
-    if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
-        return bar & PCI_BASE_ADDRESS_IO_MASK;
-    } else {
-        return bar & PCI_BASE_ADDRESS_MEM_MASK;
-    }
+	uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+
+	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
+		return bar & PCI_BASE_ADDRESS_IO_MASK;
+	else
+		return bar & PCI_BASE_ADDRESS_MEM_MASK;
 }
 
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
 {
-    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
-    return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
+	uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+
+	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
 }
 
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 {
-    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
-    return bar;
+	return pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }
diff --git a/lib/pci.h b/lib/pci.h
index 9160cfb5950d..54fbf22d634a 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -12,8 +12,9 @@
 
 typedef uint16_t pcidevaddr_t;
 enum {
-    PCIDEVADDR_INVALID = 0xffff,
+	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);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v7 01/13] pci: Fix coding style in generic PCI files

[Thomas Huth]

On 17.08.2016 14:07, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c | 38 ++++++++++++++++++++------------------
>  lib/pci.h |  3 ++-
>  2 files changed, 22 insertions(+), 19 deletions(-)
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[kvm-unit-tests PATCH v7 02/13] pci: x86: Rename pci_config_read() to pci_config_readl()

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c         | 8 ++++----
 lib/x86/asm/pci.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index 43cd0ea4f2fa..e0b4514244f6 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -13,7 +13,7 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	pcidevaddr_t dev;
 
 	for (dev = 0; dev < 256; ++dev) {
-		uint32_t id = pci_config_read(dev, 0);
+		uint32_t id = pci_config_readl(dev, 0);
 
 		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
 			return dev;
@@ -24,7 +24,7 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 
 unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	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;
@@ -34,12 +34,12 @@ unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
 
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 
 	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
 }
 
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 {
-	return pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }
diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
index cddde41a7d97..d00438fe91e4 100644
--- a/lib/x86/asm/pci.h
+++ b/lib/x86/asm/pci.h
@@ -9,7 +9,7 @@
 #include "pci.h"
 #include "x86/asm/io.h"
 
-static inline uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)
+static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg)
 {
     uint32_t index = reg | (dev << 8) | (0x1 << 31);
     outl(index, 0xCF8);
-- 
1.8.3.1

[kvm-unit-tests PATCH v7 03/13] pci: Add 'extern' to public function declarations

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/pci.h b/lib/pci.h
index 54fbf22d634a..066fac77b237 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -15,10 +15,10 @@ 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);
-bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
-bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
+extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
+extern unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
+extern bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
+extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 
 /*
  * pci-testdev is a driver for the pci-testdev qemu pci device. The
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v7 03/13] pci: Add 'extern' to public function declarations

[Andrew Jones]

On Wed, Aug 17, 2016 at 02:07:04PM +0200, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/lib/pci.h b/lib/pci.h
> index 54fbf22d634a..066fac77b237 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -15,10 +15,10 @@ 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);
> -bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
> -bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> +extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> +extern unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
> +extern bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
> +extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
>  
>  /*
>   * pci-testdev is a driver for the pci-testdev qemu pci device. The
> -- 
> 1.8.3.1
> 

[kvm-unit-tests PATCH v7 04/13] pci: x86: Add remaining PCI configuration space accessors

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c         |  5 ++---
 lib/x86/asm/pci.h | 23 +++++++++++++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index e0b4514244f6..b05ecfa3f3da 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -13,9 +13,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	pcidevaddr_t dev;
 
 	for (dev = 0; dev < 256; ++dev) {
-		uint32_t id = pci_config_readl(dev, 0);
-
-		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
+		if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id &&
+		    pci_config_readw(dev, PCI_DEVICE_ID) == device_id)
 			return dev;
 	}
 
diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
index d00438fe91e4..821a2c1e180a 100644
--- a/lib/x86/asm/pci.h
+++ b/lib/x86/asm/pci.h
@@ -9,11 +9,30 @@
 #include "pci.h"
 #include "x86/asm/io.h"
 
+#define PCI_CONF1_ADDRESS(dev, reg)	((0x1 << 31) | (dev << 8) | reg)
+
+static inline uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    return inb(0xCFC);
+}
+
+static inline uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    return inw(0xCFC);
+}
+
 static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg)
 {
-    uint32_t index = reg | (dev << 8) | (0x1 << 31);
-    outl(index, 0xCF8);
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
     return inl(0xCFC);
 }
 
+static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    outl(val, 0xCFC);
+}
+
 #endif
-- 
1.8.3.1

[kvm-unit-tests PATCH v7 05/13] pci: Factor out pci_bar_get()

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index b05ecfa3f3da..ce481bbfadb6 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -21,9 +21,14 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	return PCIDEVADDR_INVALID;
 }
 
+static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
+{
+	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+}
+
 unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	uint32_t bar = pci_bar_get(dev, bar_num);
 
 	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
 		return bar & PCI_BASE_ADDRESS_IO_MASK;
@@ -33,12 +38,12 @@ unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
 
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	uint32_t bar = pci_bar_get(dev, bar_num);
 
 	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
 }
 
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 {
-	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	return pci_bar_get(dev, bar_num);
 }
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v7 05/13] pci: Factor out pci_bar_get()

[Thomas Huth]

On 17.08.2016 14:07, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index b05ecfa3f3da..ce481bbfadb6 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -21,9 +21,14 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
>  	return PCIDEVADDR_INVALID;
>  }
>  
> +static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
> +{
> +	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +}
> +
>  unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
>  {
> -	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +	uint32_t bar = pci_bar_get(dev, bar_num);
>  
>  	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
>  		return bar & PCI_BASE_ADDRESS_IO_MASK;
> @@ -33,12 +38,12 @@ unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
>  
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
>  {
> -	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +	uint32_t bar = pci_bar_get(dev, bar_num);
>  
>  	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
>  }
>  
>  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
>  {
> -	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +	return pci_bar_get(dev, bar_num);
>  }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Alexander Gordeev]

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@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c         | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 lib/pci.h         | 17 ++++++++++++-
 lib/x86/asm/pci.h |  6 +++++
 3 files changed, 90 insertions(+), 6 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index ce481bbfadb6..e69e828523d4 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -21,19 +21,71 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	return PCIDEVADDR_INVALID;
 }
 
+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;
+}
+
 static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
 {
 	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }
 
-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
+phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_bar_get(dev, bar_num);
+	uint32_t mask = pci_bar_mask(bar);
+	uint64_t addr = bar & mask;
 
-	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))
+		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
+
+	return pci_translate_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, and then read it back. The amount of
+ * memory can be then determined by masking the information bits,
+ * performing a bitwise NOT, and incrementing the value by 1.
+ *
+ * The following pci_bar_size_helper() and pci_bar_size() functions
+ * implement the algorithm.
+ */
+static uint32_t pci_bar_size_helper(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, size;
+
+	size = pci_bar_size_helper(dev, bar_num);
+	if (!size)
+		return 0;
+
+	bar = pci_bar_get(dev, bar_num);
+	size &= pci_bar_mask(bar);
+
+	if (pci_bar_is64(dev, bar_num)) {
+		phys_addr_t size64 = pci_bar_size_helper(dev, bar_num + 1);
+		size64 = (size64 << 32) | size;
+
+		return ~size64 + 1;
+	} else {
+		return ~size + 1;
+	}
 }
 
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
@@ -47,3 +99,14 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 {
 	return pci_bar_get(dev, bar_num);
 }
+
+bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
+{
+	uint32_t bar = pci_bar_get(dev, bar_num);
+
+	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 066fac77b237..8eec236cb123 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -16,7 +16,22 @@ enum {
 };
 
 extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
-extern unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
+
+/*
+ * @bar_num in all BAR access functions below is the index of the 32-bit
+ * register starting from the PCI_BASE_ADDRESS_0 offset.
+ *
+ * In cases where the BAR size is 64-bit, a caller should still provide
+ * @bar_num in terms of 32-bit words. For example, if a device has a 64-bit
+ * BAR#0 and a 32-bit BAR#1, then caller should provide 2 to address BAR#1,
+ * not 1.
+ *
+ * It is expected the caller is aware of the device BAR layout and never
+ * tries to address the middle of a 64-bit register.
+ */
+extern phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
+extern phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
+extern bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 extern bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 extern 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..7384b91adba1 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 dev __unused, uint64_t addr)
+{
+    return addr;
+}
+
 #endif
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Peter Xu]

Hi, Alex,

I see that we are adding ARM tests in the series as well, so I am
thinking whether this work is a prepare work to test ARM SMMU?

Also, some comments below...

On Wed, Aug 17, 2016 at 02:07:07PM +0200, Alexander Gordeev wrote:

[...]

> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
>  {
>  	uint32_t bar = pci_bar_get(dev, bar_num);
> +	uint32_t mask = pci_bar_mask(bar);
> +	uint64_t addr = bar & mask;
>  
> -	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))
> +		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
> +
> +	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

[...]

> +static inline
> +phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
> +{
> +    return addr;
> +}
> +

We are not using dev here (and in following patches), I don't know
ARM, but at least x86 will need this to translate IOVA into PA (in
other words, each device can have its own IO address space). If this
codes are for common, shall we consider that as well?

Thanks.

-- peterx

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Andrew Jones]

On Fri, Sep 23, 2016 at 03:14:04PM +0800, Peter Xu wrote:
> Hi, Alex,
> 
> I see that we are adding ARM tests in the series as well, so I am
> thinking whether this work is a prepare work to test ARM SMMU?

That would be excellent. Adding Eric, he may be able to help here.

drew

> 
> Also, some comments below...
> 
> On Wed, Aug 17, 2016 at 02:07:07PM +0200, Alexander Gordeev wrote:
> 
> [...]
> 
> > -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> > +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
> >  {
> >  	uint32_t bar = pci_bar_get(dev, bar_num);
> > +	uint32_t mask = pci_bar_mask(bar);
> > +	uint64_t addr = bar & mask;
> >  
> > -	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))
> > +		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
> > +
> > +	return pci_translate_addr(dev, addr);
> 
> Raw question: do we need to translate bar addresses as well?
> 
> [...]
> 
> > +static inline
> > +phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
> > +{
> > +    return addr;
> > +}
> > +
> 
> We are not using dev here (and in following patches), I don't know
> ARM, but at least x86 will need this to translate IOVA into PA (in
> other words, each device can have its own IO address space). If this
> codes are for common, shall we consider that as well?
> 
> Thanks.
> 
> -- peterx
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Peter Xu]

On Fri, Sep 23, 2016 at 10:51:06AM +0200, Andrew Jones wrote:
> On Fri, Sep 23, 2016 at 03:14:04PM +0800, Peter Xu wrote:
> > Hi, Alex,
> > 
> > I see that we are adding ARM tests in the series as well, so I am
> > thinking whether this work is a prepare work to test ARM SMMU?
> 
> That would be excellent. Adding Eric, he may be able to help here.

I should say "Alex" rather than "we". :)

And I posted this question since I see that Alex is adding addr
translation logic to PCI. That's what I am trying to do for unit tests
for Intel IOMMU devices. So I asked.

Though (as mentioned in the comment) I don't know whether PCI bar
needs translation. I thought only DMA requests are required to be
translated in device address space. Please correct if I am wrong.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Fri, Sep 23, 2016 at 03:14:04PM +0800, Peter Xu wrote:
> > -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> > +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
> >  {
> >  	uint32_t bar = pci_bar_get(dev, bar_num);
> > +	uint32_t mask = pci_bar_mask(bar);
> > +	uint64_t addr = bar & mask;
> >  
> > -	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))
> > +		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
> > +
> > +	return pci_translate_addr(dev, addr);
> 
> Raw question: do we need to translate bar addresses as well?

I believe, yes.

Unless we always have identity mapping between PCI address space and
CPU physical address space I can not realize how could it be done
otherwise. But even if we were, I would leave the translation routine
for clarity.

> [...]
> 
> > +static inline
> > +phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
> > +{
> > +    return addr;
> > +}
> > +
> 
> We are not using dev here (and in following patches), I don't know
> ARM, but at least x86 will need this to translate IOVA into PA (in
> other words, each device can have its own IO address space). If this
> codes are for common, shall we consider that as well?

Could this function be extended or the prototype blocks the proper
implementation? (I guess, I will get the answer once I look into your
works).

> Thanks.
> 
> -- peterx

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Peter Xu]

On Wed, Oct 12, 2016 at 04:37:54PM +0200, Alexander Gordeev wrote:
> On Fri, Sep 23, 2016 at 03:14:04PM +0800, Peter Xu wrote:
> > > -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> > > +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
> > >  {
> > >  	uint32_t bar = pci_bar_get(dev, bar_num);
> > > +	uint32_t mask = pci_bar_mask(bar);
> > > +	uint64_t addr = bar & mask;
> > >  
> > > -	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))
> > > +		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
> > > +
> > > +	return pci_translate_addr(dev, addr);
> > 
> > Raw question: do we need to translate bar addresses as well?
> 
> I believe, yes.
> 
> Unless we always have identity mapping between PCI address space and
> CPU physical address space I can not realize how could it be done
> otherwise. But even if we were, I would leave the translation routine
> for clarity.

Sorry I didn't quite catch your point. Are we talking about IOMMU
address remapping here? IMHO BAR addresses are from CPU's point of
view. It's only used by CPU, not device. In that case, BAR address
should not be translated at least by IOMMU (no matter for x86/arm or
whatever).

Take Linux as example: pci_ioremap_bar() is responsible to be called
for any PCI drivers to map device memory bars into kernel virtual
address space. Basically it does:

void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)
{
	struct resource *res = &pdev->resource[bar];
	return ioremap_nocache(res->start, resource_size(res));
}

So as it is written: I believe we don't translate the bar address
(which should be res->start). We use it as physical address.

Or, do you mean other kinds of translation that I don't aware of?

Another silly question: I see pci_translate_addr() defined in both
lib/x86/asm/pci.h and lib/asm-generic/pci-host-bridge.h. How's that
working?

> 
> > [...]
> > 
> > > +static inline
> > > +phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
> > > +{
> > > +    return addr;
> > > +}
> > > +
> > 
> > We are not using dev here (and in following patches), I don't know
> > ARM, but at least x86 will need this to translate IOVA into PA (in
> > other words, each device can have its own IO address space). If this
> > codes are for common, shall we consider that as well?
> 
> Could this function be extended or the prototype blocks the proper
> implementation? (I guess, I will get the answer once I look into your
> works).

Yes, it can be extended.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Thu, Oct 13, 2016 at 02:40:35PM +0800, Peter Xu wrote:
> > > > +	return pci_translate_addr(dev, addr);
> > > 
> > > Raw question: do we need to translate bar addresses as well?
> > 
> > I believe, yes.
> > 
> > Unless we always have identity mapping between PCI address space and
> > CPU physical address space I can not realize how could it be done
> > otherwise. But even if we were, I would leave the translation routine
> > for clarity.
> 
> Sorry I didn't quite catch your point. Are we talking about IOMMU
> address remapping here? IMHO BAR addresses are from CPU's point of
> view. It's only used by CPU, not device. In that case, BAR address
> should not be translated at least by IOMMU (no matter for x86/arm or
> whatever).
> 
> Take Linux as example: pci_ioremap_bar() is responsible to be called
> for any PCI drivers to map device memory bars into kernel virtual
> address space. Basically it does:
> 
> void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)
> {
> 	struct resource *res = &pdev->resource[bar];
> 	return ioremap_nocache(res->start, resource_size(res));
> }
> 
> So as it is written: I believe we don't translate the bar address
> (which should be res->start). We use it as physical address.
> 
> Or, do you mean other kinds of translation that I don't aware of?

Yes, I mean translation from PCI bus address space to CPU physical
address space. These two busses are different and hence need a
translation. I assume Linux pci_dev::resource[] have translated
address, but it is not what PCI devices see. Unless I do not terribly
missing somethig, BAR addresses is what a device sees on its AD[0..31]
pins.

> Another silly question: I see pci_translate_addr() defined in both
> lib/x86/asm/pci.h and lib/asm-generic/pci-host-bridge.h. How's that
> working?

So an architecutre should implement pci_translate_addr()  in order
to provide mapping between PCI bus address and CPU physical address.
These two are the implementations.

Thanks!

> Thanks,
> 
> -- peterx

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Peter Xu]

On Thu, Oct 13, 2016 at 04:16:03PM +0200, Alexander Gordeev wrote:
> On Thu, Oct 13, 2016 at 02:40:35PM +0800, Peter Xu wrote:
> > > > > +	return pci_translate_addr(dev, addr);
> > > > 
> > > > Raw question: do we need to translate bar addresses as well?
> > > 
> > > I believe, yes.
> > > 
> > > Unless we always have identity mapping between PCI address space and
> > > CPU physical address space I can not realize how could it be done
> > > otherwise. But even if we were, I would leave the translation routine
> > > for clarity.
> > 
> > Sorry I didn't quite catch your point. Are we talking about IOMMU
> > address remapping here? IMHO BAR addresses are from CPU's point of
> > view. It's only used by CPU, not device. In that case, BAR address
> > should not be translated at least by IOMMU (no matter for x86/arm or
> > whatever).
> > 
> > Take Linux as example: pci_ioremap_bar() is responsible to be called
> > for any PCI drivers to map device memory bars into kernel virtual
> > address space. Basically it does:
> > 
> > void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)
> > {
> > 	struct resource *res = &pdev->resource[bar];
> > 	return ioremap_nocache(res->start, resource_size(res));
> > }
> > 
> > So as it is written: I believe we don't translate the bar address
> > (which should be res->start). We use it as physical address.
> > 
> > Or, do you mean other kinds of translation that I don't aware of?
> 
> Yes, I mean translation from PCI bus address space to CPU physical
> address space. These two busses are different and hence need a
> translation. I assume Linux pci_dev::resource[] have translated
> address, but it is not what PCI devices see. Unless I do not terribly
> missing somethig, BAR addresses is what a device sees on its AD[0..31]
> pins.

I believe pci_dev::resource[] should be assigned by BIOS or something
before Linux. At that time, IOMMU is possibly even not inited. So no
chance for a translation at all.

If you see PCI Local Bus Spec Rev 3.0, chap 6.2.5.1:

    Power-up software needs to build a consistent address map before
    booting the machine to an operating system. This means it has to
    determine how much memory is in the system, and how much address
    space the I/O controllers in the system require. After determining
    this information, power-up software can map the I/O controllers
    into reasonable locations and proceed with system boot. In order
    to do this mapping in a device independent manner, the base
    registers for this mapping are placed in the predefined header
    portion of Configuration Space.

BARs for each PCI devices should be pre-allocated during power-up
software, and a consistent map is built with the knowledge of existing
RAM in the system.

If you boot a VM with/without IOMMU, you'll see that BAR addresses
won't change before/after enabling IOMMU.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Andrew Jones]

Hi Peter,

On Fri, Oct 14, 2016 at 02:23:55PM +0800, Peter Xu wrote:
> On Thu, Oct 13, 2016 at 04:16:03PM +0200, Alexander Gordeev wrote:
> > On Thu, Oct 13, 2016 at 02:40:35PM +0800, Peter Xu wrote:
> > > > > > +	return pci_translate_addr(dev, addr);
> > > > > 
> > > > > Raw question: do we need to translate bar addresses as well?
> > > > 
> > > > I believe, yes.
> > > > 
> > > > Unless we always have identity mapping between PCI address space and
> > > > CPU physical address space I can not realize how could it be done
> > > > otherwise. But even if we were, I would leave the translation routine
> > > > for clarity.
> > > 
> > > Sorry I didn't quite catch your point. Are we talking about IOMMU
> > > address remapping here? IMHO BAR addresses are from CPU's point of
> > > view. It's only used by CPU, not device. In that case, BAR address
> > > should not be translated at least by IOMMU (no matter for x86/arm or
> > > whatever).
> > > 
> > > Take Linux as example: pci_ioremap_bar() is responsible to be called
> > > for any PCI drivers to map device memory bars into kernel virtual
> > > address space. Basically it does:
> > > 
> > > void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)
> > > {
> > > 	struct resource *res = &pdev->resource[bar];
> > > 	return ioremap_nocache(res->start, resource_size(res));
> > > }
> > > 
> > > So as it is written: I believe we don't translate the bar address
> > > (which should be res->start). We use it as physical address.
> > > 
> > > Or, do you mean other kinds of translation that I don't aware of?
> > 
> > Yes, I mean translation from PCI bus address space to CPU physical
> > address space. These two busses are different and hence need a
> > translation. I assume Linux pci_dev::resource[] have translated
> > address, but it is not what PCI devices see. Unless I do not terribly
> > missing somethig, BAR addresses is what a device sees on its AD[0..31]
> > pins.
> 
> I believe pci_dev::resource[] should be assigned by BIOS or something
> before Linux. At that time, IOMMU is possibly even not inited. So no
> chance for a translation at all.

kvm-unit-tests != Linux

kvm-unit-tests/arm doesn't have a bootloader at all (not counting QEMU
initializing registers, and a handful of QEMU generated instructions
that gives us a kick)

Anyway, I'm glad you're reviewing this series (my PCI skills are
minimal), but you'll have to review it in the right context. In
this case, more of a seabios context.

Thanks,
drew

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Peter Xu]

On Fri, Oct 14, 2016 at 08:55:20AM +0200, Andrew Jones wrote:
> Hi Peter,
> 
> On Fri, Oct 14, 2016 at 02:23:55PM +0800, Peter Xu wrote:
> > On Thu, Oct 13, 2016 at 04:16:03PM +0200, Alexander Gordeev wrote:
> > > On Thu, Oct 13, 2016 at 02:40:35PM +0800, Peter Xu wrote:
> > > > > > > +	return pci_translate_addr(dev, addr);
> > > > > > 
> > > > > > Raw question: do we need to translate bar addresses as well?
> > > > > 
> > > > > I believe, yes.
> > > > > 
> > > > > Unless we always have identity mapping between PCI address space and
> > > > > CPU physical address space I can not realize how could it be done
> > > > > otherwise. But even if we were, I would leave the translation routine
> > > > > for clarity.
> > > > 
> > > > Sorry I didn't quite catch your point. Are we talking about IOMMU
> > > > address remapping here? IMHO BAR addresses are from CPU's point of
> > > > view. It's only used by CPU, not device. In that case, BAR address
> > > > should not be translated at least by IOMMU (no matter for x86/arm or
> > > > whatever).
> > > > 
> > > > Take Linux as example: pci_ioremap_bar() is responsible to be called
> > > > for any PCI drivers to map device memory bars into kernel virtual
> > > > address space. Basically it does:
> > > > 
> > > > void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)
> > > > {
> > > > 	struct resource *res = &pdev->resource[bar];
> > > > 	return ioremap_nocache(res->start, resource_size(res));
> > > > }
> > > > 
> > > > So as it is written: I believe we don't translate the bar address
> > > > (which should be res->start). We use it as physical address.
> > > > 
> > > > Or, do you mean other kinds of translation that I don't aware of?
> > > 
> > > Yes, I mean translation from PCI bus address space to CPU physical
> > > address space. These two busses are different and hence need a
> > > translation. I assume Linux pci_dev::resource[] have translated
> > > address, but it is not what PCI devices see. Unless I do not terribly
> > > missing somethig, BAR addresses is what a device sees on its AD[0..31]
> > > pins.
> > 
> > I believe pci_dev::resource[] should be assigned by BIOS or something
> > before Linux. At that time, IOMMU is possibly even not inited. So no
> > chance for a translation at all.
> 
> kvm-unit-tests != Linux
> 
> kvm-unit-tests/arm doesn't have a bootloader at all (not counting QEMU
> initializing registers, and a handful of QEMU generated instructions
> that gives us a kick)
> 
> Anyway, I'm glad you're reviewing this series (my PCI skills are
> minimal), but you'll have to review it in the right context. In
> this case, more of a seabios context.

Thank you for pointing out. :-)

I know little about PCI as well, just want to know the fact on how we
should treat BAR addresses. So I'd say my comments are more like
questions rather than I disagree with the changes. E.g., even if we
don't have BIOS, do we really need to translate BAR addresses on ARM?

Sorry if I brought too much noise to this thread. Looking forward to
Alex's further works.

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Fri, Oct 14, 2016 at 02:23:55PM +0800, Peter Xu wrote:
> On Thu, Oct 13, 2016 at 04:16:03PM +0200, Alexander Gordeev wrote:
> > On Thu, Oct 13, 2016 at 02:40:35PM +0800, Peter Xu wrote:
> > > > > > +	return pci_translate_addr(dev, addr);
> > > > > 
> > > > > Raw question: do we need to translate bar addresses as well?
> > > > 
> > > > I believe, yes.
> > > > 
> > > > Unless we always have identity mapping between PCI address space and
> > > > CPU physical address space I can not realize how could it be done
> > > > otherwise. But even if we were, I would leave the translation routine
> > > > for clarity.
> > > 
> > > Sorry I didn't quite catch your point. Are we talking about IOMMU
> > > address remapping here? IMHO BAR addresses are from CPU's point of
> > > view. It's only used by CPU, not device. In that case, BAR address
> > > should not be translated at least by IOMMU (no matter for x86/arm or
> > > whatever).
> > > 
> > > Take Linux as example: pci_ioremap_bar() is responsible to be called
> > > for any PCI drivers to map device memory bars into kernel virtual
> > > address space. Basically it does:
> > > 
> > > void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)
> > > {
> > > 	struct resource *res = &pdev->resource[bar];
> > > 	return ioremap_nocache(res->start, resource_size(res));
> > > }
> > > 
> > > So as it is written: I believe we don't translate the bar address
> > > (which should be res->start). We use it as physical address.
> > > 
> > > Or, do you mean other kinds of translation that I don't aware of?
> > 
> > Yes, I mean translation from PCI bus address space to CPU physical
> > address space. These two busses are different and hence need a
> > translation. I assume Linux pci_dev::resource[] have translated
> > address, but it is not what PCI devices see. Unless I do not terribly
> > missing somethig, BAR addresses is what a device sees on its AD[0..31]
> > pins.
> 
> I believe pci_dev::resource[] should be assigned by BIOS or something
> before Linux. At that time, IOMMU is possibly even not inited. So no
> chance for a translation at all.

So pci-host-generic.c is "BIOS or something" in this context. There is
no contradiction with the your excerpt. Please note "Figure 1-2: PCI
System Block Diagram" in the spec. The CPU and PCI busses are two
physically different busses interconnected by a bridge.

A CPU data access to PCI devices initiates data access on a PCI bus,
but adresses seen by the CPU and PCI devices do not necessarily match.

So in case of x86 they do (lib/x86/asm/pci.h):

static inline
phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
{
	return addr;
}

But in case of ARM they might not (lib/asm-generic/pci-host-bridge.h):

static inline
phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
{
	/*
	 * Assume we only have single PCI host bridge in a system.
	 */
	return pci_host_bridge_get_paddr(addr);
}

> If you see PCI Local Bus Spec Rev 3.0, chap 6.2.5.1:
> 
>     Power-up software needs to build a consistent address map before
>     booting the machine to an operating system. This means it has to
>     determine how much memory is in the system, and how much address
>     space the I/O controllers in the system require. After determining
>     this information, power-up software can map the I/O controllers
>     into reasonable locations and proceed with system boot. In order
>     to do this mapping in a device independent manner, the base
>     registers for this mapping are placed in the predefined header
>     portion of Configuration Space.
> 
> BARs for each PCI devices should be pre-allocated during power-up
> software, and a consistent map is built with the knowledge of existing
> RAM in the system.

Yep, see how pci_probe() / pci_alloc_resource() allocate and assign
BARs to each device. It is the case for PPC/ARM but in case of x86
it is BIOS who does it.

> If you boot a VM with/without IOMMU, you'll see that BAR addresses
> won't change before/after enabling IOMMU.

BAR addresses are in PCI bus address space. It sounds quite logical.
(I am not experienced with IOMMU address translation, though).

> Thanks,
> 
> -- peterx

Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Peter Xu]

On Fri, Oct 14, 2016 at 02:37:25PM +0200, Alexander Gordeev wrote:

[...]

> So pci-host-generic.c is "BIOS or something" in this context. There is
> no contradiction with the your excerpt. Please note "Figure 1-2: PCI
> System Block Diagram" in the spec. The CPU and PCI busses are two
> physically different busses interconnected by a bridge.
> 
> A CPU data access to PCI devices initiates data access on a PCI bus,
> but adresses seen by the CPU and PCI devices do not necessarily match.
> 
> So in case of x86 they do (lib/x86/asm/pci.h):
> 
> static inline
> phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
> {
> 	return addr;
> }
> 
> But in case of ARM they might not (lib/asm-generic/pci-host-bridge.h):
> 
> static inline
> phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
> {
> 	/*
> 	 * Assume we only have single PCI host bridge in a system.
> 	 */
> 	return pci_host_bridge_get_paddr(addr);
> }
> 
> > If you see PCI Local Bus Spec Rev 3.0, chap 6.2.5.1:
> > 
> >     Power-up software needs to build a consistent address map before
> >     booting the machine to an operating system. This means it has to
> >     determine how much memory is in the system, and how much address
> >     space the I/O controllers in the system require. After determining
> >     this information, power-up software can map the I/O controllers
> >     into reasonable locations and proceed with system boot. In order
> >     to do this mapping in a device independent manner, the base
> >     registers for this mapping are placed in the predefined header
> >     portion of Configuration Space.
> > 
> > BARs for each PCI devices should be pre-allocated during power-up
> > software, and a consistent map is built with the knowledge of existing
> > RAM in the system.
> 
> Yep, see how pci_probe() / pci_alloc_resource() allocate and assign
> BARs to each device. It is the case for PPC/ARM but in case of x86
> it is BIOS who does it.
> 
> > If you boot a VM with/without IOMMU, you'll see that BAR addresses
> > won't change before/after enabling IOMMU.
> 
> BAR addresses are in PCI bus address space. It sounds quite logical.
> (I am not experienced with IOMMU address translation, though).

Thank you for the thorough explanation. I am still not sure I fully
understand the whole thing, but looks like the translation here is not
the same as x86 IOMMU address translations, and x86 is special in this
case (BAR addresses should be the same as physical addresses in x86,
while not for ARM/PPC).

I am sorry for the noise. Looking forward to your next version. Thanks!

-- peterx

[kvm-unit-tests PATCH v7 07/13] pci: Add pci_bar_set_addr()

[Alexander Gordeev]

Because the counterpart to pci_bar_set_addr() setter is
pci_bar_addr() getter, these names become inconsistent.
Rename pci_bar_addr() to pci_bar_get_addr() also to make
the resulting names conform to each other.

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c    | 12 +++++++++++-
 lib/pci.h    |  3 ++-
 x86/vmexit.c |  4 ++--
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index e69e828523d4..7650da9ffaf2 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -32,7 +32,7 @@ static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
 	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }
 
-phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
+phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
@@ -44,6 +44,16 @@ phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 	return pci_translate_addr(dev, addr);
 }
 
+void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
+{
+	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
+
+	pci_config_writel(dev, off, (uint32_t)addr);
+
+	if (pci_bar_is64(dev, bar_num))
+		pci_config_writel(dev, off + 4, (uint32_t)(addr >> 32));
+}
+
 /*
  * 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
diff --git a/lib/pci.h b/lib/pci.h
index 8eec236cb123..1c20308c9344 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -29,7 +29,8 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
  * It is expected the caller is aware of the device BAR layout and never
  * tries to address the middle of a 64-bit register.
  */
-extern phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
+extern phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
+extern void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
 extern phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
 extern bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 extern bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
diff --git a/x86/vmexit.c b/x86/vmexit.c
index c2e1e496918d..2d99d5fdf1c2 100644
--- a/x86/vmexit.c
+++ b/x86/vmexit.c
@@ -392,10 +392,10 @@ int main(int ac, char **av)
 				continue;
 			}
 			if (pci_bar_is_memory(pcidev, i)) {
-				membar = pci_bar_addr(pcidev, i);
+				membar = pci_bar_get_addr(pcidev, i);
 				pci_test.memaddr = ioremap(membar, PAGE_SIZE);
 			} else {
-				pci_test.iobar = pci_bar_addr(pcidev, i);
+				pci_test.iobar = pci_bar_get_addr(pcidev, i);
 			}
 		}
 		printf("pci-testdev at 0x%x membar %lx iobar %x\n",
-- 
1.8.3.1

[kvm-unit-tests PATCH v7 08/13] pci: Add pci_dev_exists()

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c | 6 ++++++
 lib/pci.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/lib/pci.c b/lib/pci.c
index 7650da9ffaf2..8dea915662d5 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -7,6 +7,12 @@
 #include "pci.h"
 #include "asm/pci.h"
 
+bool pci_dev_exists(pcidevaddr_t dev)
+{
+	return (pci_config_readw(dev, PCI_VENDOR_ID) != 0xffff &&
+		pci_config_readw(dev, PCI_DEVICE_ID) != 0xffff);
+}
+
 /* 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)
 {
diff --git a/lib/pci.h b/lib/pci.h
index 1c20308c9344..1462aa2f0e1a 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -15,6 +15,7 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
 
+extern bool pci_dev_exists(pcidevaddr_t dev);
 extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 
 /*
-- 
1.8.3.1

[kvm-unit-tests PATCH v7 09/13] pci: Add pci_print()

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h |  3 +++
 2 files changed, 81 insertions(+)

diff --git a/lib/pci.c b/lib/pci.c
index 8dea915662d5..4271ae5b528e 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -126,3 +126,81 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
 	return (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
 		      PCI_BASE_ADDRESS_MEM_TYPE_64;
 }
+
+static void pci_dev_print(pcidevaddr_t dev)
+{
+	uint16_t vendor_id = pci_config_readw(dev, PCI_VENDOR_ID);
+	uint16_t device_id = pci_config_readw(dev, PCI_DEVICE_ID);
+	uint8_t header = pci_config_readb(dev, PCI_HEADER_TYPE);
+	uint8_t progif = pci_config_readb(dev, PCI_CLASS_PROG);
+	uint8_t subclass = pci_config_readb(dev, PCI_CLASS_DEVICE);
+	uint8_t class = pci_config_readb(dev, PCI_CLASS_DEVICE + 1);
+	int i;
+
+	printf("dev %2d fn %d vendor_id %04x device_id %04x type %02x "
+	       "progif %02x class %02x subclass %02x\n",
+	       dev / 8, dev % 8, vendor_id, device_id, header,
+	       progif, class, subclass);
+
+	if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
+		return;
+
+	for (i = 0; i < 6; i++) {
+		phys_addr_t size, start, end;
+		uint32_t bar;
+
+		size = pci_bar_size(dev, i);
+		if (!size)
+			continue;
+
+		start = pci_bar_get_addr(dev, i);
+		end = start + size - 1;
+
+		if (pci_bar_is64(dev, i)) {
+			printf("\tBAR#%d,%d [%" PRIx64 "-%" PRIx64 " ",
+			       i, i + 1, start, end);
+			i++;
+		} else {
+			printf("\tBAR#%d    [%02x-%02x ",
+			       i, (uint32_t)start, (uint32_t)end);
+		}
+
+		bar = pci_bar_get(dev, i);
+
+		if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
+			printf("PIO]\n");
+			continue;
+		}
+
+		printf("MEM");
+
+		switch (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) {
+		case PCI_BASE_ADDRESS_MEM_TYPE_32:
+			printf("32");
+			break;
+		case PCI_BASE_ADDRESS_MEM_TYPE_1M:
+			printf("1M");
+			break;
+		case PCI_BASE_ADDRESS_MEM_TYPE_64:
+			printf("64");
+			break;
+		default:
+			assert(0);
+		}
+
+		if (bar & PCI_BASE_ADDRESS_MEM_PREFETCH)
+			printf("/p");
+
+		printf("]\n");
+	}
+}
+
+void pci_print(void)
+{
+	pcidevaddr_t dev;
+
+	for (dev = 0; dev < 256; ++dev) {
+		if (pci_dev_exists(dev))
+			pci_dev_print(dev);
+	}
+}
diff --git a/lib/pci.h b/lib/pci.h
index 1462aa2f0e1a..fc0940adc299 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -15,6 +15,7 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
 
+extern void pci_print(void);
 extern bool pci_dev_exists(pcidevaddr_t dev);
 extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 
@@ -57,4 +58,6 @@ struct pci_test_dev_hdr {
 	uint8_t  name[];
 };
 
+#define  PCI_HEADER_TYPE_MASK		0x7f
+
 #endif /* PCI_H */
-- 
1.8.3.1

[kvm-unit-tests PATCH v7 10/13] pci: Add generic ECAM host support

[Alexander Gordeev]

Unlike x86, other architectures using generic ECAM PCI host
do not have a luxury of PCI bus initialized by a BIOS and
ready to use at start. Thus, we need allocate and assign
resources to all devices, much like an architecture's
firmware would do.

There is no any sort of resource management for memory and
io spaces, since only ones-per-BAR allocations are expected
and no deallocations at all.

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/asm-generic/pci-host-bridge.h |  26 ++++
 lib/pci-host-generic.c            | 295 ++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h            |  46 ++++++
 lib/pci.c                         |   4 +-
 lib/pci.h                         |   3 +
 5 files changed, 372 insertions(+), 2 deletions(-)
 create mode 100644 lib/asm-generic/pci-host-bridge.h
 create mode 100644 lib/pci-host-generic.c
 create mode 100644 lib/pci-host-generic.h

diff --git a/lib/asm-generic/pci-host-bridge.h b/lib/asm-generic/pci-host-bridge.h
new file mode 100644
index 000000000000..ac32b85198e9
--- /dev/null
+++ b/lib/asm-generic/pci-host-bridge.h
@@ -0,0 +1,26 @@
+#ifndef _ASM_PCI_HOST_BRIDGE_H_
+#define _ASM_PCI_HOST_BRIDGE_H_
+/*
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+
+phys_addr_t pci_host_bridge_get_paddr(uint64_t addr);
+
+static inline
+phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
+{
+	/*
+	 * Assume we only have single PCI host bridge in a system.
+	 */
+	return pci_host_bridge_get_paddr(addr);
+}
+
+uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg);
+uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg);
+uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg);
+void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val);
+
+#endif
diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
new file mode 100644
index 000000000000..4a2716a33150
--- /dev/null
+++ b/lib/pci-host-generic.c
@@ -0,0 +1,295 @@
+/*
+ * Generic PCI host controller as described in PCI Bus Binding to Open Firmware
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+#include "devicetree.h"
+#include "alloc.h"
+#include "pci.h"
+#include "asm/pci.h"
+#include "asm/io.h"
+#include "pci-host-generic.h"
+#include <linux/pci_regs.h>
+
+static struct pci_host_bridge *pci_host_bridge;
+
+static int of_flags_to_pci_type(u32 of_flags)
+{
+	static int type_map[] = {
+		[1] = PCI_BASE_ADDRESS_SPACE_IO,
+		[2] = PCI_BASE_ADDRESS_MEM_TYPE_32,
+		[3] = PCI_BASE_ADDRESS_MEM_TYPE_64
+	};
+	int idx = (of_flags >> 24) & 0x03;
+	int res;
+
+	assert(idx > 0);
+	res = type_map[idx];
+
+	if (of_flags & 0x40000000)
+		res |= PCI_BASE_ADDRESS_MEM_PREFETCH;
+
+	return res;
+}
+
+static int pci_bar_type(u32 bar)
+{
+	if (bar & PCI_BASE_ADDRESS_SPACE)
+		return PCI_BASE_ADDRESS_SPACE_IO;
+	else
+		return bar & (PCI_BASE_ADDRESS_MEM_TYPE_MASK |
+			      PCI_BASE_ADDRESS_MEM_PREFETCH);
+}
+
+/*
+ * Probe DT for a generic PCI host controller
+ * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
+ * and function gen_pci_probe() in drivers/pci/host/pci-host-generic.c
+ */
+static struct pci_host_bridge *pci_dt_probe(void)
+{
+	struct pci_host_bridge *host;
+	const void *fdt = dt_fdt();
+	const struct fdt_property *prop;
+	struct dt_pbus_reg base;
+	struct dt_device dt_dev;
+	struct dt_bus dt_bus;
+	struct pci_addr_space *as;
+	fdt32_t *data;
+	u32 bus, bus_max;
+	u32 nac, nsc, nac_root, nsc_root;
+	int nr_range_cells, nr_addr_spaces;
+	int ret, node, len, i;
+
+	if (!dt_available()) {
+		printf("No device tree found\n");
+		return NULL;
+	}
+
+	dt_bus_init_defaults(&dt_bus);
+	dt_device_init(&dt_dev, &dt_bus, NULL);
+
+	node = fdt_path_offset(fdt, "/");
+	assert(node >= 0);
+
+	ret = dt_get_nr_cells(node, &nac_root, &nsc_root);
+	assert(ret == 0);
+	assert(nac_root == 1 || nac_root == 2);
+
+	node = fdt_node_offset_by_compatible(fdt, node,
+					     "pci-host-ecam-generic");
+	if (node == -FDT_ERR_NOTFOUND) {
+		printf("No PCIe ECAM compatible controller found\n");
+		return NULL;
+	}
+	assert(node >= 0);
+
+	prop = fdt_get_property(fdt, node, "device_type", &len);
+	assert(prop && len == 4 && !strcmp((char *)prop->data, "pci"));
+
+	dt_device_bind_node(&dt_dev, node);
+	ret = dt_pbus_get_base(&dt_dev, &base);
+	assert(ret == 0);
+
+	prop = fdt_get_property(fdt, node, "bus-range", &len);
+	if (prop == NULL) {
+		assert(len == -FDT_ERR_NOTFOUND);
+		bus		= 0x00;
+		bus_max		= 0xff;
+	} else {
+		data		= (fdt32_t *)prop->data;
+		bus		= fdt32_to_cpu(data[0]);
+		bus_max		= fdt32_to_cpu(data[1]);
+		assert(bus <= bus_max);
+	}
+	assert(bus_max < base.size / (1 << PCI_ECAM_BUS_SHIFT));
+
+	ret = dt_get_nr_cells(node, &nac, &nsc);
+	assert(ret == 0);
+	assert(nac == 3 && nsc == 2);
+
+	prop = fdt_get_property(fdt, node, "ranges", &len);
+	assert(prop != NULL);
+
+	nr_range_cells = nac + nsc + nac_root;
+	nr_addr_spaces = (len / 4) / nr_range_cells;
+	assert(nr_addr_spaces);
+
+	host = malloc(sizeof(*host) +
+		      sizeof(host->addr_space[0]) * nr_addr_spaces);
+	assert(host != NULL);
+
+	host->start		= base.addr;
+	host->size		= base.size;
+	host->bus		= bus;
+	host->bus_max		= bus_max;
+	host->nr_addr_spaces	= nr_addr_spaces;
+
+	data = (fdt32_t *)prop->data;
+	as = &host->addr_space[0];
+
+	for (i = 0; i < nr_addr_spaces; i++) {
+		/*
+		 * The PCI binding encodes the PCI address with three
+		 * cells as follows:
+		 *
+		 * phys.hi  cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
+		 * phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
+		 * phys.lo  cell: llllllll llllllll llllllll llllllll
+		 *
+		 * PCI device bus address and flags are encoded into phys.high
+		 * PCI 64 bit address is encoded into phys.mid and phys.low
+		 */
+		as->type = of_flags_to_pci_type(fdt32_to_cpu(data[0]));
+		as->pci_start = ((u64)fdt32_to_cpu(data[1]) << 32) |
+				fdt32_to_cpu(data[2]);
+
+		if (nr_range_cells == 6) {
+			as->start = fdt32_to_cpu(data[3]);
+			as->size  = ((u64)fdt32_to_cpu(data[4]) << 32) |
+				    fdt32_to_cpu(data[5]);
+		} else {
+			as->start = ((u64)fdt32_to_cpu(data[3]) << 32) |
+				    fdt32_to_cpu(data[4]);
+			as->size  = ((u64)fdt32_to_cpu(data[5]) << 32) |
+				    fdt32_to_cpu(data[6]);
+		}
+
+		data += nr_range_cells;
+		as++;
+	}
+
+	return host;
+}
+
+static u64 pci_alloc_resource(u32 bar, u64 size)
+{
+	struct pci_host_bridge *host = pci_host_bridge;
+	struct pci_addr_space *as = &host->addr_space[0];
+	phys_addr_t addr;
+	u32 mask;
+	int i;
+
+	for (i = 0; i < host->nr_addr_spaces; i++) {
+		if (as->type == pci_bar_type(bar))
+			break;
+		as++;
+	}
+	if (i >= host->nr_addr_spaces) {
+		printf("No PCI resource found for a device\n");
+		assert(0);
+	}
+
+	mask = pci_bar_mask(bar);
+	size = ALIGN(size, ~mask + 1);
+	assert(as->allocated + size <= as->size);
+
+	addr = as->pci_start + as->allocated;
+	as->allocated += size;
+
+	return addr;
+}
+
+bool pci_probe(void)
+{
+	pcidevaddr_t dev;
+	u8 header;
+	u32 cmd;
+	int i;
+
+	assert(!pci_host_bridge);
+	pci_host_bridge = pci_dt_probe();
+	if (!pci_host_bridge)
+		return false;
+
+	for (dev = 0; dev < 256; dev++) {
+		if (!pci_dev_exists(dev))
+			continue;
+
+		/* We are only interested in normal PCI devices */
+		header = pci_config_readb(dev, PCI_HEADER_TYPE);
+		if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
+			continue;
+
+		cmd = PCI_COMMAND_SERR;
+
+		for (i = 0; i < 6; i++) {
+			u64 addr, size;
+			u32 bar;
+
+			size = pci_bar_size(dev, i);
+			if (!size)
+				continue;
+
+			bar = pci_bar_get(dev, i);
+			addr = pci_alloc_resource(bar, size);
+			pci_bar_set_addr(dev, i, addr);
+
+			if (pci_bar_is_memory(dev, i))
+				cmd |= PCI_COMMAND_MEMORY;
+			else
+				cmd |= PCI_COMMAND_IO;
+
+			if (pci_bar_is64(dev, i))
+				i++;
+		}
+
+		pci_config_writel(dev, PCI_COMMAND, cmd);
+	}
+
+	return true;
+}
+
+/*
+ * This function is to be called from pci_translate_addr() to provide
+ * mapping between this host bridge's PCI busses address and CPU physical
+ * address.
+ */
+phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr)
+{
+	struct pci_host_bridge *host = pci_host_bridge;
+	struct pci_addr_space *as = &host->addr_space[0];
+	int i;
+
+	for (i = 0; i < host->nr_addr_spaces; i++) {
+		if (pci_addr >= as->pci_start &&
+		    pci_addr < as->pci_start + as->size)
+			return as->start + (pci_addr - as->pci_start);
+		as++;
+	}
+
+	return 0;
+}
+
+static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn)
+{
+	return (void __iomem *)(unsigned long)
+		host->start + (devfn << PCI_ECAM_DEVFN_SHIFT);
+}
+
+u8 pci_config_readb(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readb(conf + off);
+}
+
+u16 pci_config_readw(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readw(conf + off);
+}
+
+u32 pci_config_readl(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readl(conf + off);
+}
+
+void pci_config_writel(pcidevaddr_t dev, u8 off, u32 val)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	writel(val, conf + off);
+}
diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
new file mode 100644
index 000000000000..fd30e7c74ed8
--- /dev/null
+++ b/lib/pci-host-generic.h
@@ -0,0 +1,46 @@
+#ifndef PCI_HOST_GENERIC_H
+#define PCI_HOST_GENERIC_H
+/*
+ * PCI host bridge supporting structures and constants
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+
+struct pci_addr_space {
+	phys_addr_t		pci_start;
+	phys_addr_t		start;
+	phys_addr_t		size;
+	phys_addr_t		allocated;
+	int			type;
+};
+
+struct pci_host_bridge {
+	phys_addr_t		start;
+	phys_addr_t		size;
+	int			bus;
+	int			bus_max;
+	int			nr_addr_spaces;
+	struct pci_addr_space	addr_space[];
+};
+
+/*
+ * The following constants are derived from Linux, see this source:
+ *
+ *         drivers/pci/host/pci-host-generic.c
+ *                 struct gen_pci_cfg_bus_ops::bus_shift
+ *                 int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
+ *
+ * Documentation/devicetree/bindings/pci/host-generic-pci.txt describes
+ * ECAM Configuration Space is be memory-mapped by concatenating the various
+ * components to form an offset:
+ *
+ *	cfg_offset(bus, device, function, register) =
+ *		   bus << 20 | device << 15 | function << 12 | register
+ */
+#define PCI_ECAM_BUS_SHIFT	20
+#define PCI_ECAM_DEVFN_SHIFT	12
+
+#endif
diff --git a/lib/pci.c b/lib/pci.c
index 4271ae5b528e..4bdfd803fd80 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -27,13 +27,13 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	return PCIDEVADDR_INVALID;
 }
 
-static uint32_t pci_bar_mask(uint32_t bar)
+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;
 }
 
-static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
+uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
 {
 	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }
diff --git a/lib/pci.h b/lib/pci.h
index fc0940adc299..7acbbdf2eb16 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -15,6 +15,7 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
 
+extern bool pci_probe(void);
 extern void pci_print(void);
 extern bool pci_dev_exists(pcidevaddr_t dev);
 extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
@@ -34,6 +35,8 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 extern phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
 extern void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
 extern phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
+extern uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num);
+extern uint32_t pci_bar_mask(uint32_t bar);
 extern bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 extern bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
-- 
1.8.3.1

[kvm-unit-tests PATCH v7 11/13] arm/arm64: pci: Add PCI bus operation test

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arm/Makefile.common |  5 ++++-
 arm/pci-test.c      | 19 +++++++++++++++++++
 lib/arm/asm/pci.h   |  1 +
 lib/arm64/asm/pci.h |  1 +
 4 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 arm/pci-test.c
 create mode 100644 lib/arm/asm/pci.h
 create mode 100644 lib/arm64/asm/pci.h

diff --git a/arm/Makefile.common b/arm/Makefile.common
index ccb554d9251a..97179bbea4e7 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -11,7 +11,8 @@ endif
 
 tests-common = \
 	$(TEST_DIR)/selftest.flat \
-	$(TEST_DIR)/spinlock-test.flat
+	$(TEST_DIR)/spinlock-test.flat \
+	$(TEST_DIR)/pci-test.flat
 
 all: test_cases
 
@@ -33,6 +34,8 @@ include scripts/asm-offsets.mak
 cflatobjs += lib/util.o
 cflatobjs += lib/alloc.o
 cflatobjs += lib/devicetree.o
+cflatobjs += lib/pci.o
+cflatobjs += lib/pci-host-generic.o
 cflatobjs += lib/virtio.o
 cflatobjs += lib/virtio-mmio.o
 cflatobjs += lib/chr-testdev.o
diff --git a/arm/pci-test.c b/arm/pci-test.c
new file mode 100644
index 000000000000..daebdafceb60
--- /dev/null
+++ b/arm/pci-test.c
@@ -0,0 +1,19 @@
+/*
+ * PCI bus operation test
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <pci.h>
+
+int main(void)
+{
+	if (!pci_probe())
+		report_abort("PCI bus probing failed\n");
+
+	pci_print();
+
+	return report_summary();
+}
diff --git a/lib/arm/asm/pci.h b/lib/arm/asm/pci.h
new file mode 100644
index 000000000000..01ecafe2b6d8
--- /dev/null
+++ b/lib/arm/asm/pci.h
@@ -0,0 +1 @@
+#include "asm-generic/pci-host-bridge.h"
diff --git a/lib/arm64/asm/pci.h b/lib/arm64/asm/pci.h
new file mode 100644
index 000000000000..f70ef560e2ab
--- /dev/null
+++ b/lib/arm64/asm/pci.h
@@ -0,0 +1 @@
+#include "../../arm/asm/pci.h"
-- 
1.8.3.1

[kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-testdev.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h         |   7 ++
 2 files changed, 199 insertions(+)
 create mode 100644 lib/pci-testdev.c

diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
new file mode 100644
index 000000000000..ad482d3291c7
--- /dev/null
+++ b/lib/pci-testdev.c
@@ -0,0 +1,192 @@
+/*
+ * QEMU "pci-testdev" PCI test device
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "pci.h"
+#include "asm/io.h"
+
+struct pci_testdev_ops {
+	u8 (*io_readb)(const volatile void *addr);
+	u16 (*io_readw)(const volatile void *addr);
+	u32 (*io_readl)(const volatile void *addr);
+	void (*io_writeb)(u8 value, volatile void *addr);
+	void (*io_writew)(u16 value, volatile void *addr);
+	void (*io_writel)(u32 value, volatile void *addr);
+};
+
+static u8 pio_readb(const volatile void *addr)
+{
+	return inb((unsigned long)addr);
+}
+
+static u16 pio_readw(const volatile void *addr)
+{
+	return inw((unsigned long)addr);
+}
+
+static u32 pio_readl(const volatile void *addr)
+{
+	return inl((unsigned long)addr);
+}
+
+static void pio_writeb(u8 value, volatile void *addr)
+{
+	outb(value, (unsigned long)addr);
+}
+
+static void pio_writew(u16 value, volatile void *addr)
+{
+	outw(value, (unsigned long)addr);
+}
+
+static void pio_writel(u32 value, volatile void *addr)
+{
+	outl(value, (unsigned long)addr);
+}
+
+static struct pci_testdev_ops pci_testdev_io_ops = {
+	.io_readb	= pio_readb,
+	.io_readw	= pio_readw,
+	.io_readl	= pio_readl,
+	.io_writeb	= pio_writeb,
+	.io_writew	= pio_writew,
+	.io_writel	= pio_writel
+};
+
+static u8 mmio_readb(const volatile void *addr)
+{
+	return *(const volatile u8 __force *)addr;
+}
+
+static u16 mmio_readw(const volatile void *addr)
+{
+	return *(const volatile u16 __force *)addr;
+}
+
+static u32 mmio_readl(const volatile void *addr)
+{
+	return *(const volatile u32 __force *)addr;
+}
+
+static void mmio_writeb(u8 value, volatile void *addr)
+{
+	*(volatile u8 __force *)addr = value;
+}
+
+static void mmio_writew(u16 value, volatile void *addr)
+{
+	*(volatile u16 __force *)addr = value;
+}
+
+static void mmio_writel(u32 value, volatile void *addr)
+{
+	*(volatile u32 __force *)addr = value;
+}
+
+static struct pci_testdev_ops pci_testdev_mem_ops = {
+	.io_readb	= mmio_readb,
+	.io_readw	= mmio_readw,
+	.io_readl	= mmio_readl,
+	.io_writeb	= mmio_writeb,
+	.io_writew	= mmio_writew,
+	.io_writel	= mmio_writel
+};
+
+static bool pci_testdev_one(struct pci_test_dev_hdr *test,
+			    int test_nr,
+			    struct pci_testdev_ops *ops)
+{
+	u8 width;
+	u32 count, sig, off;
+	const int nr_writes = 16;
+	int i;
+
+	ops->io_writeb(test_nr, &test->test);
+	count = ops->io_readl(&test->count);
+	if (count != 0)
+		return false;
+
+	width = ops->io_readb(&test->width);
+	if (width != 1 && width != 2 && width != 4)
+		return false;
+
+	sig = ops->io_readl(&test->data);
+	off = ops->io_readl(&test->offset);
+
+	for (i = 0; i < nr_writes; i++) {
+		switch (width) {
+		case 1: ops->io_writeb(sig, (void *)test + off); break;
+		case 2: ops->io_writew(sig, (void *)test + off); break;
+		case 4: ops->io_writel(sig, (void *)test + off); break;
+		}
+	}
+
+	count = ops->io_readl(&test->count);
+	if (!count)
+		return true;
+
+	return (int)count == nr_writes;
+}
+
+void pci_testdev_print(struct pci_test_dev_hdr *test,
+		       struct pci_testdev_ops *ops)
+{
+	bool io = (ops == &pci_testdev_io_ops);
+	int i;
+
+	printf("pci-testdev %3s: ", io ? "io" : "mem");
+	for (i = 0;; ++i) {
+		char c = ops->io_readb(&test->name[i]);
+		if (!c)
+			break;
+		printf("%c", c);
+	}
+	printf("\n");
+}
+
+static int pci_testdev_all(struct pci_test_dev_hdr *test,
+			   struct pci_testdev_ops *ops)
+{
+	int i;
+
+	for (i = 0;; i++) {
+		if (!pci_testdev_one(test, i, ops))
+			break;
+		pci_testdev_print(test, ops);
+	}
+
+	return i;
+}
+
+int pci_testdev(void)
+{
+	phys_addr_t addr;
+	void __iomem *mem, *io;
+	pcidevaddr_t dev;
+	int nr_tests = 0;
+	bool ret;
+
+	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
+	if (dev == PCIDEVADDR_INVALID) {
+		printf("'pci-testdev' device is not found, "
+		       "check QEMU '-device pci-testdev' parameter\n");
+		return -1;
+	}
+
+	ret = pci_bar_is_valid(dev, 0) && pci_bar_is_valid(dev, 1);
+	assert(ret);
+
+	addr = pci_bar_get_addr(dev, 0);
+	mem = ioremap(addr, PAGE_SIZE);
+
+	addr = pci_bar_get_addr(dev, 1);
+	io = (void *)(unsigned long)addr;
+
+	nr_tests += pci_testdev_all(mem, &pci_testdev_mem_ops);
+	nr_tests += pci_testdev_all(io, &pci_testdev_io_ops);
+
+	return nr_tests;
+}
diff --git a/lib/pci.h b/lib/pci.h
index 7acbbdf2eb16..40e11a892783 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -41,6 +41,8 @@ extern bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 extern bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 
+int pci_testdev(void);
+
 /*
  * pci-testdev is a driver for the pci-testdev qemu pci device. The
  * device enables testing mmio and portio exits, and measuring their
@@ -49,7 +51,12 @@ extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 #define PCI_VENDOR_ID_REDHAT		0x1b36
 #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
 
+/*
+ * pci-testdev supports at least three types of tests (via mmio and
+ * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd
+ */
 #define PCI_TESTDEV_NUM_BARS		2
+#define PCI_TESTDEV_NUM_TESTS		3
 
 struct pci_test_dev_hdr {
 	uint8_t  test;
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Andrew Jones]

On Wed, Aug 17, 2016 at 02:07:13PM +0200, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-testdev.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci.h         |   7 ++
>  2 files changed, 199 insertions(+)
>  create mode 100644 lib/pci-testdev.c
> 

>From a quick skim this looks like the same as v6, so you must have just
forgotten to pick up my r-b. Anyway, here it is again

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Peter Xu]

Some nit-picks inline...

(btw, this looks more like a test case shared by platforms rather than
 a library, so shall we move it outside of lib/? I don't know.)

On Wed, Aug 17, 2016 at 02:07:13PM +0200, Alexander Gordeev wrote:

[...]

> +static bool pci_testdev_one(struct pci_test_dev_hdr *test,
> +			    int test_nr,
> +			    struct pci_testdev_ops *ops)
> +{
> +	u8 width;
> +	u32 count, sig, off;
> +	const int nr_writes = 16;
> +	int i;
> +
> +	ops->io_writeb(test_nr, &test->test);
> +	count = ops->io_readl(&test->count);
> +	if (count != 0)
> +		return false;
> +
> +	width = ops->io_readb(&test->width);
> +	if (width != 1 && width != 2 && width != 4)
> +		return false;

IIUC we only have 1?

> +
> +	sig = ops->io_readl(&test->data);
> +	off = ops->io_readl(&test->offset);
> +
> +	for (i = 0; i < nr_writes; i++) {
> +		switch (width) {
> +		case 1: ops->io_writeb(sig, (void *)test + off); break;
> +		case 2: ops->io_writew(sig, (void *)test + off); break;
> +		case 4: ops->io_writel(sig, (void *)test + off); break;

Here as well. Could I ask why we are handling 2/4?

[...]

> +static int pci_testdev_all(struct pci_test_dev_hdr *test,
> +			   struct pci_testdev_ops *ops)
> +{
> +	int i;
> +
> +	for (i = 0;; i++) {
> +		if (!pci_testdev_one(test, i, ops))
> +			break;

Since we have defined PCI_TESTDEV_NUM_TESTS, shall we use it here to
stop the loop rather than depending on a failure code from
pci_testdev_one()?

[...]

> +int pci_testdev(void)
> +{
> +	phys_addr_t addr;
> +	void __iomem *mem, *io;
> +	pcidevaddr_t dev;
> +	int nr_tests = 0;
> +	bool ret;
> +
> +	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
> +	if (dev == PCIDEVADDR_INVALID) {
> +		printf("'pci-testdev' device is not found, "
> +		       "check QEMU '-device pci-testdev' parameter\n");
> +		return -1;
> +	}
> +
> +	ret = pci_bar_is_valid(dev, 0) && pci_bar_is_valid(dev, 1);
> +	assert(ret);
> +
> +	addr = pci_bar_get_addr(dev, 0);
> +	mem = ioremap(addr, PAGE_SIZE);
> +
> +	addr = pci_bar_get_addr(dev, 1);
> +	io = (void *)(unsigned long)addr;

x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the
init part and share it? (Actually there is patch in my local tree for
this, but haven't posted :)

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Andrew Jones]

On Fri, Sep 23, 2016 at 03:25:42PM +0800, Peter Xu wrote:
> Some nit-picks inline...
> 
> (btw, this looks more like a test case shared by platforms rather than
>  a library, so shall we move it outside of lib/? I don't know.)

We only have lib/ for shared code right now. I don't think we need
to add a new directory for shared testcode vs. libcode yet, as this
is the first case. If it becomes a more common thing to do, though,
then I agree that it may be nicer to have a new directory for it.

Thanks,
drew

> 
> On Wed, Aug 17, 2016 at 02:07:13PM +0200, Alexander Gordeev wrote:
> 
> [...]
> 
> > +static bool pci_testdev_one(struct pci_test_dev_hdr *test,
> > +			    int test_nr,
> > +			    struct pci_testdev_ops *ops)
> > +{
> > +	u8 width;
> > +	u32 count, sig, off;
> > +	const int nr_writes = 16;
> > +	int i;
> > +
> > +	ops->io_writeb(test_nr, &test->test);
> > +	count = ops->io_readl(&test->count);
> > +	if (count != 0)
> > +		return false;
> > +
> > +	width = ops->io_readb(&test->width);
> > +	if (width != 1 && width != 2 && width != 4)
> > +		return false;
> 
> IIUC we only have 1?
> 
> > +
> > +	sig = ops->io_readl(&test->data);
> > +	off = ops->io_readl(&test->offset);
> > +
> > +	for (i = 0; i < nr_writes; i++) {
> > +		switch (width) {
> > +		case 1: ops->io_writeb(sig, (void *)test + off); break;
> > +		case 2: ops->io_writew(sig, (void *)test + off); break;
> > +		case 4: ops->io_writel(sig, (void *)test + off); break;
> 
> Here as well. Could I ask why we are handling 2/4?
> 
> [...]
> 
> > +static int pci_testdev_all(struct pci_test_dev_hdr *test,
> > +			   struct pci_testdev_ops *ops)
> > +{
> > +	int i;
> > +
> > +	for (i = 0;; i++) {
> > +		if (!pci_testdev_one(test, i, ops))
> > +			break;
> 
> Since we have defined PCI_TESTDEV_NUM_TESTS, shall we use it here to
> stop the loop rather than depending on a failure code from
> pci_testdev_one()?
> 
> [...]
> 
> > +int pci_testdev(void)
> > +{
> > +	phys_addr_t addr;
> > +	void __iomem *mem, *io;
> > +	pcidevaddr_t dev;
> > +	int nr_tests = 0;
> > +	bool ret;
> > +
> > +	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
> > +	if (dev == PCIDEVADDR_INVALID) {
> > +		printf("'pci-testdev' device is not found, "
> > +		       "check QEMU '-device pci-testdev' parameter\n");
> > +		return -1;
> > +	}
> > +
> > +	ret = pci_bar_is_valid(dev, 0) && pci_bar_is_valid(dev, 1);
> > +	assert(ret);
> > +
> > +	addr = pci_bar_get_addr(dev, 0);
> > +	mem = ioremap(addr, PAGE_SIZE);
> > +
> > +	addr = pci_bar_get_addr(dev, 1);
> > +	io = (void *)(unsigned long)addr;
> 
> x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the
> init part and share it? (Actually there is patch in my local tree for
> this, but haven't posted :)
> 
> Thanks!
> 
> -- peterx
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Alexander Gordeev]

On Fri, Sep 23, 2016 at 03:25:42PM +0800, Peter Xu wrote:
> > +	width = ops->io_readb(&test->width);
> > +	if (width != 1 && width != 2 && width != 4)
> > +		return false;
> 
> IIUC we only have 1?

I guess it boils what *have* does mean here.

pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c
implements just 1 (yet?).

> > +
> > +	sig = ops->io_readl(&test->data);
> > +	off = ops->io_readl(&test->offset);
> > +
> > +	for (i = 0; i < nr_writes; i++) {
> > +		switch (width) {
> > +		case 1: ops->io_writeb(sig, (void *)test + off); break;
> > +		case 2: ops->io_writew(sig, (void *)test + off); break;
> > +		case 4: ops->io_writel(sig, (void *)test + off); break;
> 
> Here as well. Could I ask why we are handling 2/4?

Basically, because x86 had it and this implementation mimics it.

> [...]
> 
> > +static int pci_testdev_all(struct pci_test_dev_hdr *test,
> > +			   struct pci_testdev_ops *ops)
> > +{
> > +	int i;
> > +
> > +	for (i = 0;; i++) {
> > +		if (!pci_testdev_one(test, i, ops))
> > +			break;
> 
> Since we have defined PCI_TESTDEV_NUM_TESTS, shall we use it here to
> stop the loop rather than depending on a failure code from
> pci_testdev_one()?

No, I think we indeed need to inquiry the device this way and
go ahead and test if it reported the size is supported.

> [...]

> x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the
> init part and share it? (Actually there is patch in my local tree for
> this, but haven't posted :)

Yep, I have x86 enabler and it is very simple. But x86 is just too
different to try to generalize and we're not pursuing it right now.

> Thanks!
> 
> -- peterx

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Peter Xu]

On Wed, Oct 12, 2016 at 06:54:28PM +0200, Alexander Gordeev wrote:
> On Fri, Sep 23, 2016 at 03:25:42PM +0800, Peter Xu wrote:
> > > +	width = ops->io_readb(&test->width);
> > > +	if (width != 1 && width != 2 && width != 4)
> > > +		return false;
> > 
> > IIUC we only have 1?
> 
> I guess it boils what *have* does mean here.
> 
> pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c
> implements just 1 (yet?).

Do we have other possible implementations for pci-testdev protocol?

> 
> > > +
> > > +	sig = ops->io_readl(&test->data);
> > > +	off = ops->io_readl(&test->offset);
> > > +
> > > +	for (i = 0; i < nr_writes; i++) {
> > > +		switch (width) {
> > > +		case 1: ops->io_writeb(sig, (void *)test + off); break;
> > > +		case 2: ops->io_writew(sig, (void *)test + off); break;
> > > +		case 4: ops->io_writel(sig, (void *)test + off); break;
> > 
> > Here as well. Could I ask why we are handling 2/4?
> 
> Basically, because x86 had it and this implementation mimics it.

Yes, actually I didn't notice that before. So I have the same question
for vmexit.c. But of course I don't think this question is a blocker
for the series.

[...]

> > x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the
> > init part and share it? (Actually there is patch in my local tree for
> > this, but haven't posted :)
> 
> Yep, I have x86 enabler and it is very simple. But x86 is just too
> different to try to generalize and we're not pursuing it right now.

Could I ask what's the difficulties? Again this is not a block for
sure, so, looking forward to your next version.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Alexander Gordeev]

On Thu, Oct 13, 2016 at 02:52:49PM +0800, Peter Xu wrote:
> On Wed, Oct 12, 2016 at 06:54:28PM +0200, Alexander Gordeev wrote:
> > On Fri, Sep 23, 2016 at 03:25:42PM +0800, Peter Xu wrote:
> > > > +	width = ops->io_readb(&test->width);
> > > > +	if (width != 1 && width != 2 && width != 4)
> > > > +		return false;
> > > 
> > > IIUC we only have 1?
> > 
> > I guess it boils what *have* does mean here.
> > 
> > pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c
> > implements just 1 (yet?).
> 
> Do we have other possible implementations for pci-testdev protocol?

I typed answer twice, but realized I do not get the question. :)
Could you paraphrase, please?

> > > > +	sig = ops->io_readl(&test->data);
> > > > +	off = ops->io_readl(&test->offset);
> > > > +
> > > > +	for (i = 0; i < nr_writes; i++) {
> > > > +		switch (width) {
> > > > +		case 1: ops->io_writeb(sig, (void *)test + off); break;
> > > > +		case 2: ops->io_writew(sig, (void *)test + off); break;
> > > > +		case 4: ops->io_writel(sig, (void *)test + off); break;
> > > 
> > > Here as well. Could I ask why we are handling 2/4?
> > 
> > Basically, because x86 had it and this implementation mimics it.
> 
> Yes, actually I didn't notice that before. So I have the same question
> for vmexit.c. But of course I don't think this question is a blocker
> for the series.

I am not sure about x86, but I do not see any problem either.

> > > x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the
> > > init part and share it? (Actually there is patch in my local tree for
> > > this, but haven't posted :)
> > 
> > Yep, I have x86 enabler and it is very simple. But x86 is just too
> > different to try to generalize and we're not pursuing it right now.
> 
> Could I ask what's the difficulties? Again this is not a block for
> sure, so, looking forward to your next version.

Well, x86 is a series of tests, very self-contained and hence very
implementation-oriented. By contrast, this version is rather stand-
alone and does not really fit.

So if one embarks to generalize, then it would be x86 wide, not this
test alone, AFAICT.

> Thanks,
> 
> -- peterx

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Peter Xu]

On Thu, Oct 13, 2016 at 03:16:49PM +0200, Alexander Gordeev wrote:
> On Thu, Oct 13, 2016 at 02:52:49PM +0800, Peter Xu wrote:
> > On Wed, Oct 12, 2016 at 06:54:28PM +0200, Alexander Gordeev wrote:
> > > On Fri, Sep 23, 2016 at 03:25:42PM +0800, Peter Xu wrote:
> > > > > +	width = ops->io_readb(&test->width);
> > > > > +	if (width != 1 && width != 2 && width != 4)
> > > > > +		return false;
> > > > 
> > > > IIUC we only have 1?
> > > 
> > > I guess it boils what *have* does mean here.
> > > 
> > > pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c
> > > implements just 1 (yet?).
> > 
> > Do we have other possible implementations for pci-testdev protocol?
> 
> I typed answer twice, but realized I do not get the question. :)
> Could you paraphrase, please?

Sorry for not being clear. I am just wondering whether there is other
implementation for pci-testdev besides the one in QEMU. It looks like
a special device to test QEMU only.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Andrew Jones]

On Fri, Oct 14, 2016 at 01:01:48PM +0800, Peter Xu wrote:
> On Thu, Oct 13, 2016 at 03:16:49PM +0200, Alexander Gordeev wrote:
> > On Thu, Oct 13, 2016 at 02:52:49PM +0800, Peter Xu wrote:
> > > On Wed, Oct 12, 2016 at 06:54:28PM +0200, Alexander Gordeev wrote:
> > > > On Fri, Sep 23, 2016 at 03:25:42PM +0800, Peter Xu wrote:
> > > > > > +	width = ops->io_readb(&test->width);
> > > > > > +	if (width != 1 && width != 2 && width != 4)
> > > > > > +		return false;
> > > > > 
> > > > > IIUC we only have 1?
> > > > 
> > > > I guess it boils what *have* does mean here.
> > > > 
> > > > pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c
> > > > implements just 1 (yet?).
> > > 
> > > Do we have other possible implementations for pci-testdev protocol?
> > 
> > I typed answer twice, but realized I do not get the question. :)
> > Could you paraphrase, please?
> 
> Sorry for not being clear. I am just wondering whether there is other
> implementation for pci-testdev besides the one in QEMU. It looks like
> a special device to test QEMU only.

Yes. pci-testdev (most likely) has never been implemented anywhere other
than in QEMU. It was written specifically for kvm-unit-tests. See

http://www.linux-kvm.org/page/KVM-unit-tests#Testdevs

for a synopsis of all testdevs. Please feel free to add more details to
the pci-testdev section. Feel free to help maintain the document in any
other ways too, of course :-)

drew

Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus test device

[Peter Xu]

On Fri, Oct 14, 2016 at 09:07:07AM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 01:01:48PM +0800, Peter Xu wrote:
> > On Thu, Oct 13, 2016 at 03:16:49PM +0200, Alexander Gordeev wrote:
> > > On Thu, Oct 13, 2016 at 02:52:49PM +0800, Peter Xu wrote:
> > > > On Wed, Oct 12, 2016 at 06:54:28PM +0200, Alexander Gordeev wrote:
> > > > > On Fri, Sep 23, 2016 at 03:25:42PM +0800, Peter Xu wrote:
> > > > > > > +	width = ops->io_readb(&test->width);
> > > > > > > +	if (width != 1 && width != 2 && width != 4)
> > > > > > > +		return false;
> > > > > > 
> > > > > > IIUC we only have 1?
> > > > > 
> > > > > I guess it boils what *have* does mean here.
> > > > > 
> > > > > pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c
> > > > > implements just 1 (yet?).
> > > > 
> > > > Do we have other possible implementations for pci-testdev protocol?
> > > 
> > > I typed answer twice, but realized I do not get the question. :)
> > > Could you paraphrase, please?
> > 
> > Sorry for not being clear. I am just wondering whether there is other
> > implementation for pci-testdev besides the one in QEMU. It looks like
> > a special device to test QEMU only.
> 
> Yes. pci-testdev (most likely) has never been implemented anywhere other
> than in QEMU. It was written specifically for kvm-unit-tests. See
> 
> http://www.linux-kvm.org/page/KVM-unit-tests#Testdevs
> 
> for a synopsis of all testdevs. Please feel free to add more details to
> the pci-testdev section. Feel free to help maintain the document in any
> other ways too, of course :-)

Thanks, Drew.

I am trying to use edu device for testing Intel IOMMU. I'll try to
post RFC patches first though. When needed, I can update the document
for edu device (though I still don't know how to do that).

-- peterx

[kvm-unit-tests PATCH v7 13/13] arm/arm64: pci: Add pci-testdev PCI device operation test

[Alexander Gordeev]

Suggested-by: Andrew Jones <drjones@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arm/Makefile.common | 1 +
 arm/pci-test.c      | 8 ++++++++
 arm/run             | 7 ++++++-
 arm/unittests.cfg   | 4 ++++
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arm/Makefile.common b/arm/Makefile.common
index 97179bbea4e7..f37b5c2a3de4 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -36,6 +36,7 @@ cflatobjs += lib/alloc.o
 cflatobjs += lib/devicetree.o
 cflatobjs += lib/pci.o
 cflatobjs += lib/pci-host-generic.o
+cflatobjs += lib/pci-testdev.o
 cflatobjs += lib/virtio.o
 cflatobjs += lib/virtio-mmio.o
 cflatobjs += lib/chr-testdev.o
diff --git a/arm/pci-test.c b/arm/pci-test.c
index daebdafceb60..10a367de5357 100644
--- a/arm/pci-test.c
+++ b/arm/pci-test.c
@@ -8,12 +8,20 @@
 #include <libcflat.h>
 #include <pci.h>
 
+#define NR_TESTS (PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS)
+
 int main(void)
 {
+	int ret;
+
 	if (!pci_probe())
 		report_abort("PCI bus probing failed\n");
 
 	pci_print();
 
+	ret = pci_testdev();
+	report("PCI test device passed %d/%d tests",
+		ret >= NR_TESTS, ret > 0 ? ret : 0, NR_TESTS);
+
 	return report_summary();
 }
diff --git a/arm/run b/arm/run
index a2f35ef6a7e6..1ee6231599d6 100755
--- a/arm/run
+++ b/arm/run
@@ -67,8 +67,13 @@ fi
 chr_testdev='-device virtio-serial-device'
 chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
 
+pci_testdev=
+if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
+	pci_testdev="-device pci-testdev"
+fi
+
 M+=",accel=$ACCEL"
-command="$qemu $M -cpu $processor $chr_testdev"
+command="$qemu $M -cpu $processor $chr_testdev $pci_testdev"
 command+=" -display none -serial stdio -kernel"
 command="$(timeout_cmd) $command"
 echo $command "$@"
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index ffd12e5794aa..20ffa0493c4c 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -51,3 +51,7 @@ file = selftest.flat
 smp = $MAX_SMP
 extra_params = -append 'smp'
 groups = selftest
+
+[pci-test]
+file = pci-test.flat
+groups = pci
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v7 00/13] PCI bus support

[Andrew Jones]

On Wed, Aug 17, 2016 at 02:07:01PM +0200, Alexander Gordeev wrote:
> Hi Andrew,
> 
> This is 7th version of PCI support. Noticeable changes since v5 & v6:
>   - patch "pci: Factor out pci_bar_get()" moved earlier in the series;
>   - 'extern' added to public functions in lib/pci.h;
>   - PCI test added to arm/unittests.cfg;
> 
> Sources are avalable at:
> https://github.com/a-gordeev/kvm-unit-tests.git pci-testdev-v7

This branch has two additional commits on it (for x86). I've dropped
them from my branch import.

> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> 
> Alexander Gordeev (13):
>   pci: Fix coding style in generic PCI files
>   pci: x86: Rename pci_config_read() to pci_config_readl()
>   pci: Add 'extern' to public function declarations
>   pci: x86: Add remaining PCI configuration space accessors
>   pci: Factor out pci_bar_get()
>   pci: Rework pci_bar_addr()
>   pci: Add pci_bar_set_addr()
>   pci: Add pci_dev_exists()
>   pci: Add pci_print()
>   pci: Add generic ECAM host support
>   arm/arm64: pci: Add PCI bus operation test
>   pci: Add pci-testdev PCI bus test device
>   arm/arm64: pci: Add pci-testdev PCI device operation test

I think patch 12 should be patch 11, and patch 13 should be squashed
into the current 11. Current 11 by itself is pretty pointless.

And now for bad news. I imported your branch and tested it. It works
great over TCG, but hits the "No PCI resource found for a device"
assert when running with KVM on an AArch64 server. I've only tried on
a mustang so far.

Thanks,
drew

Re: [kvm-unit-tests PATCH v7 00/13] PCI bus support

[Alexander Gordeev]

On Wed, Aug 17, 2016 at 04:26:54PM +0200, Andrew Jones wrote:
> And now for bad news. I imported your branch and tested it. It works
> great over TCG, but hits the "No PCI resource found for a device"
> assert when running with KVM on an AArch64 server. I've only tried on
> a mustang so far.

Unlike TCG, KVM on an AArch64 reports pci-testdev's BAR #4 value of 0xC.
It denotes to 64 bit prefetch memory, which is absent. What is more,
BAR #4 is not implemented in pci-testdev. Only two BARs are expected
(qemu hw/misc/pci-testdev.c):

    pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);

So the assert sounds about right and the question is why BAR #4 is there.

> Thanks,
> drew

Re: [kvm-unit-tests PATCH v7 00/13] PCI bus support

[Andrew Jones]

On Tue, Aug 23, 2016 at 08:28:06PM +0200, Alexander Gordeev wrote:
> On Wed, Aug 17, 2016 at 04:26:54PM +0200, Andrew Jones wrote:
> > And now for bad news. I imported your branch and tested it. It works
> > great over TCG, but hits the "No PCI resource found for a device"
> > assert when running with KVM on an AArch64 server. I've only tried on
> > a mustang so far.
> 
> Unlike TCG, KVM on an AArch64 reports pci-testdev's BAR #4 value of 0xC.
> It denotes to 64 bit prefetch memory, which is absent. What is more,
> BAR #4 is not implemented in pci-testdev. Only two BARs are expected
> (qemu hw/misc/pci-testdev.c):
> 
>     pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>     pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);
> 
> So the assert sounds about right and the question is why BAR #4 is there.

I finally had a chance to take a look at this. There's no BAR#4 being
exposed from pci-testdev. QEMU is adding a default virtio-net-pci device,
which does have a BAR#4. Adjusting the QEMU env like this

 export QEMU="$QEMU -nodefaults"

which removes default devices, allows the new unit test to run and pass.

kvm-unit-tests shouldn't be asserting and dying when it's presented a
valid virtio-net-pci device though. It should handle the probing
correctly, or at least just warn that it doesn't know how to, and then
ignore it. Alex, can you consider what might need to be tweaked in this
series to do that?

That said, as kvm-unit-tests isn't going to drive non-testdev devices
anyway, then just removing default devices is a reasonable thing to do
too. I'll submit a patch adding '-nodefaults -nodefconfig' to the arm
QEMU command line.

Thanks,
drew

Re: [kvm-unit-tests PATCH v7 00/13] PCI bus support

[Peter Xu]

On Thu, Sep 22, 2016 at 01:10:04PM +0200, Andrew Jones wrote:

[...]

> I finally had a chance to take a look at this. There's no BAR#4 being
> exposed from pci-testdev. QEMU is adding a default virtio-net-pci device,
> which does have a BAR#4. Adjusting the QEMU env like this
> 
>  export QEMU="$QEMU -nodefaults"
> 
> which removes default devices, allows the new unit test to run and pass.
> 
> kvm-unit-tests shouldn't be asserting and dying when it's presented a
> valid virtio-net-pci device though. It should handle the probing
> correctly, or at least just warn that it doesn't know how to, and then
> ignore it. Alex, can you consider what might need to be tweaked in this
> series to do that?
> 
> That said, as kvm-unit-tests isn't going to drive non-testdev devices
> anyway, then just removing default devices is a reasonable thing to do
> too. I'll submit a patch adding '-nodefaults -nodefconfig' to the arm
> QEMU command line.

Could I ask what's the status of this series? I see that this is still
not in master (nor did I saw any further works from Alex).

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v7 00/13] PCI bus support

[Alexander Gordeev]

On Wed, Sep 28, 2016 at 02:33:09PM +0800, Peter Xu wrote:
> Could I ask what's the status of this series? I see that this is still
> not in master (nor did I saw any further works from Alex).

Hi Peter,

Sorry for the long absense.
I am going to post the updated version, hopefully soon.

> Thanks,
> 
> -- peterx

Re: [kvm-unit-tests PATCH v7 00/13] PCI bus support

[Peter Xu]

On Wed, Oct 12, 2016 at 10:00:44AM +0200, Alexander Gordeev wrote:
> On Wed, Sep 28, 2016 at 02:33:09PM +0800, Peter Xu wrote:
> > Could I ask what's the status of this series? I see that this is still
> > not in master (nor did I saw any further works from Alex).
> 
> Hi Peter,
> 
> Sorry for the long absense.
> I am going to post the updated version, hopefully soon.

I have some patches that may have overlapped changes with yours. Will
see how to rebase to it. Thanks for the update!

-- peterx

Re: [kvm-unit-tests PATCH v7 00/13] PCI bus support

[Alexander Gordeev]

On Thu, Sep 22, 2016 at 01:10:04PM +0200, Andrew Jones wrote:
> kvm-unit-tests shouldn't be asserting and dying when it's presented a
> valid virtio-net-pci device though. It should handle the probing
> correctly, or at least just warn that it doesn't know how to, and then
> ignore it. Alex, can you consider what might need to be tweaked in this
> series to do that?

If the tweak below works for you?

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 4a2716a33150..a2a2b902f154 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -165,11 +165,10 @@ static struct pci_host_bridge *pci_dt_probe(void)
 	return host;
 }
 
-static u64 pci_alloc_resource(u32 bar, u64 size)
+static bool pci_alloc_resource(u64 *addr, u32 bar, u64 size)
 {
 	struct pci_host_bridge *host = pci_host_bridge;
 	struct pci_addr_space *as = &host->addr_space[0];
-	phys_addr_t addr;
 	u32 mask;
 	int i;
 
@@ -180,17 +179,17 @@ static u64 pci_alloc_resource(u32 bar, u64 size)
 	}
 	if (i >= host->nr_addr_spaces) {
 		printf("No PCI resource found for a device\n");
-		assert(0);
+		return false;
 	}
 
 	mask = pci_bar_mask(bar);
 	size = ALIGN(size, ~mask + 1);
 	assert(as->allocated + size <= as->size);
 
-	addr = as->pci_start + as->allocated;
+	*addr = as->pci_start + as->allocated;
 	as->allocated += size;
 
-	return addr;
+	return true;
 }
 
 bool pci_probe(void)
@@ -225,13 +224,14 @@ bool pci_probe(void)
 				continue;
 
 			bar = pci_bar_get(dev, i);
-			addr = pci_alloc_resource(bar, size);
-			pci_bar_set_addr(dev, i, addr);
-
-			if (pci_bar_is_memory(dev, i))
-				cmd |= PCI_COMMAND_MEMORY;
-			else
-				cmd |= PCI_COMMAND_IO;
+			if (pci_alloc_resource(&addr, bar, size)) {
+				pci_bar_set_addr(dev, i, addr);
+
+				if (pci_bar_is_memory(dev, i))
+					cmd |= PCI_COMMAND_MEMORY;
+				else
+					cmd |= PCI_COMMAND_IO;
+			}
 
 			if (pci_bar_is64(dev, i))
 				i++;
-- 
1.8.3.1



> Thanks,
> drew