[kvm-unit-tests PATCH v4 00/12] PCI bus support

[kvm-unit-tests PATCH v4 00/12] PCI bus support

[Alexander Gordeev]

Hi all,

This series should be applied on top of "Cleanup low-level arch code"
series which is still not included. Yet, it is ready for review as
all previous comments and suggestions are addressed.

There might be some confusion about version numbering as I posted
the previous version as RFC with no version number at all. In fact
it was 3rd version so I am labelling this series as v4. Unlike the
RFC it does not have gaps in implementation.

There are quite a lot of changes since the previous version.

I tried pci-testdev against ARM and got the device semi-operational.
It is still to investigate, but that could be addressed separately.

Most interesting - writing to IO BAR on ARM does not seem working as
a written value does not read back. Probably, ARM64 is also affected,
but again - I have not investigated it yet.

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

Alexander Gordeev (12):
  pci: Fix coding style in generic PCI files
  pci: x86: Rename pci_config_read() to pci_config_readl()
  pci: x86: Add remaining PCI configuration space accessors
  pci: Rework pci_bar_addr()
  pci: Factor out pci_bar_get()
  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    |   7 +-
 arm/pci-test.c         |  31 ++++++
 arm/run                |   7 +-
 lib/arm/asm/pci.h      |  26 +++++
 lib/arm64/asm/pci.h    |   1 +
 lib/pci-host-generic.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h |  46 ++++++++
 lib/pci-testdev.c      | 184 +++++++++++++++++++++++++++++++
 lib/pci.c              | 194 ++++++++++++++++++++++++++++----
 lib/pci.h              |  33 +++++-
 lib/x86/asm/pci.h      |  31 +++++-
 x86/vmexit.c           |   4 +-
 12 files changed, 830 insertions(+), 28 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/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 v4 01/12] pci: Fix coding style in generic PCI files

[Alexander Gordeev]

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

diff --git a/lib/pci.c b/lib/pci.c
index 0058d70c888d..bfdaebac862e 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;
+	uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	return bar;
 }
diff --git a/lib/pci.h b/lib/pci.h
index 9160cfb5950d..88dc47c1f48d 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -12,7 +12,7 @@
 
 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);
-- 
1.8.3.1

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

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:30PM +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.c | 38 ++++++++++++++++++++------------------
>  lib/pci.h |  2 +-
>  2 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 0058d70c888d..bfdaebac862e 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);
no blank line here?
> +	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;
> +	uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +	return bar;
can just do return pci_config_read(...)
>  }
> diff --git a/lib/pci.h b/lib/pci.h
> index 9160cfb5950d..88dc47c1f48d 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -12,7 +12,7 @@
>  
>  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);
> -- 
> 1.8.3.1
> 

[kvm-unit-tests PATCH v4 02/12] 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 bfdaebac862e..7ddaac639006 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)
 {
-	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;
 }
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 v4 03/12] 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>
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 7ddaac639006..b0d89bf98067 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

Re: [kvm-unit-tests PATCH v4 03/12] pci: x86: Add remaining PCI configuration space accessors

[Thomas Huth]

On 06.06.2016 14:46, 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         |  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 7ddaac639006..b0d89bf98067 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

Maybe add a #define for that magic value 0xCF8, now that you use it
multiple times?

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

Re: [kvm-unit-tests PATCH v4 03/12] pci: x86: Add remaining PCI configuration space accessors

[Alexander Gordeev]

On Tue, Jun 07, 2016 at 08:48:46AM +0200, Thomas Huth wrote:
> On 06.06.2016 14:46, 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         |  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 7ddaac639006..b0d89bf98067 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
> 
> Maybe add a #define for that magic value 0xCF8, now that you use it
> multiple times?

I used to an idea 0xCF8/0xCFC in x86 world are one of those well-
known constant that people perceive better their values rather than
any names :)

Andrew?

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

Re: [kvm-unit-tests PATCH v4 03/12] pci: x86: Add remaining PCI configuration space accessors

[Andrew Jones]

On Wed, Jun 08, 2016 at 08:21:27AM +0200, Alexander Gordeev wrote:
> On Tue, Jun 07, 2016 at 08:48:46AM +0200, Thomas Huth wrote:
> > On 06.06.2016 14:46, 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         |  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 7ddaac639006..b0d89bf98067 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
> > 
> > Maybe add a #define for that magic value 0xCF8, now that you use it
> > multiple times?
> 
> I used to an idea 0xCF8/0xCFC in x86 world are one of those well-
> known constant that people perceive better their values rather than
> any names :)
> 
> Andrew?

I agree with Alex. I think CF8 and CFC are effectively the names now
in the PC world :-)

> 
> > Anyway,
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > 
> > 
> --
> 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

[kvm-unit-tests PATCH v4 04/12] 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>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c         | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 lib/pci.h         | 16 +++++++++++++-
 lib/x86/asm/pci.h |  6 +++++
 3 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index b0d89bf98067..07c911820e20 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -21,14 +21,59 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	return PCIDEVADDR_INVALID;
 }
 
-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
+static uint32_t pci_bar_mask(uint32_t bar)
+{
+	return (bar & PCI_BASE_ADDRESS_SPACE_IO) ?
+		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
+}
+
+phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
+{
+	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
+	phys_addr_t bar = pci_config_readl(dev, off);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
+
+	if (pci_bar_is64(dev, bar_num))
+		bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;
+
+	return pci_translate_addr(dev, bar & mask);
+}
+
+/*
+ * To determine the amount of address space needed by a PCI device,
+ * one must save the original value of the BAR, write a value of
+ * all 1's to the register, then read it back. The amount of memory
+ * can then be then determined by masking the information bits,
+ * performing a bitwise NOT and incrementing the value by 1.
+ *
+ * The following pci_bar_size32() and pci_bar_size() functions do
+ * the described algorithm.
+ */
+static uint32_t pci_bar_size32(pcidevaddr_t dev, int bar_num)
+{
+	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
+	uint32_t bar, size;
+
+	bar = pci_config_readl(dev, off);
+	pci_config_writel(dev, off, ~0u);
+	size = pci_config_readl(dev, off);
+	pci_config_writel(dev, off, bar);
+
+	return size;
+}
+
+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
 
-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num)) {
+		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
+		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
+	}
+
+	return (~(size & mask)) + 1;
 }
 
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
@@ -42,3 +87,14 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 	return bar;
 }
+
+bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
+{
+	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+
+	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
+		return false;
+
+	return (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+		      PCI_BASE_ADDRESS_MEM_TYPE_64;
+}
diff --git a/lib/pci.h b/lib/pci.h
index 88dc47c1f48d..dbfe7918da37 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -15,7 +15,21 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
+
+/*
+ * BAR number in all BAR access functions below is a number of 32-bit
+ * register starting from PCI_BASE_ADDRESS_0 offset.
+ *
+ * In cases BAR size is 64-bit a caller should still provide BAR number
+ * in terms of 32-bit words. For example, if a device has 64-bit BAR#0
+ * and 32-bit BAR#1 the caller should provide 2 to address BAR#1, not 1.
+ *
+ * It is expected the caller is aware of the device BAR layout and never
+ * tries to address in the middle of a 64-bit register.
+ */
+phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
+bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 
diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
index 821a2c1e180a..813dc28b9232 100644
--- a/lib/x86/asm/pci.h
+++ b/lib/x86/asm/pci.h
@@ -35,4 +35,10 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val
     outl(val, 0xCFC);
 }
 
+static inline
+phys_addr_t pci_translate_addr(pcidevaddr_t __unused dev, uint64_t addr)
+{
+    return addr;
+}
+
 #endif
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:33PM +0200, Alexander Gordeev wrote:
> This update makes pci_bar_addr() interface 64 bit BARs aware and
> introduces a concept of PCI address translation.
> 
> An architecutre should implement pci_translate_addr() interface
> in order to provide mapping between PCI bus address and CPU
> physical address.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c         | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  lib/pci.h         | 16 +++++++++++++-
>  lib/x86/asm/pci.h |  6 +++++
>  3 files changed, 82 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index b0d89bf98067..07c911820e20 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -21,14 +21,59 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
>  	return PCIDEVADDR_INVALID;
>  }
>  
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> +static uint32_t pci_bar_mask(uint32_t bar)
> +{
> +	return (bar & PCI_BASE_ADDRESS_SPACE_IO) ?
> +		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
> +}
> +
> +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
> +{
> +	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> +	phys_addr_t bar = pci_config_readl(dev, off);
> +	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

This sign-extension trick is too subtle, IMO, and not really necessary.
We only need to apply the mask to the lower half of a 64-bit bar, i.e.

 phys_addr_t bar = pci_config_readl(dev, off);
 uint32_t mask = pci_bar_mask(bar);

 bar &= mask;

 if (pci_bar_is64(dev, bar_num))
     bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;

 return pci_translate_addr(dev, bar);

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

All this casting of size and mask is pointless. Please rework it
similar to what I did above.

>  }
>  
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
> @@ -42,3 +87,14 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
>  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
>  	return bar;
>  }
> +
> +bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
> +{
> +	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +
> +	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> +		return false;
> +
> +	return (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +		      PCI_BASE_ADDRESS_MEM_TYPE_64;
> +}
> diff --git a/lib/pci.h b/lib/pci.h
> index 88dc47c1f48d..dbfe7918da37 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -15,7 +15,21 @@ enum {
>  	PCIDEVADDR_INVALID = 0xffff,
>  };
>  pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
> +
> +/*
> + * BAR number in all BAR access functions below is a number of 32-bit
> + * register starting from PCI_BASE_ADDRESS_0 offset.
> + *
> + * In cases BAR size is 64-bit a caller should still provide BAR number
> + * in terms of 32-bit words. For example, if a device has 64-bit BAR#0
> + * and 32-bit BAR#1 the caller should provide 2 to address BAR#1, not 1.

Is this how people label bars? I.e. they call a bar following a 64-bit
bar BAR#N+1? Or do they skip numbers with the labels to match the
offsets? I.e. in this example you'd say BAR#0 (64-bit), there is no BAR#1,
and then BAR#2 (32-bit) when labeling?

> + *
> + * It is expected the caller is aware of the device BAR layout and never
> + * tries to address in the middle of a 64-bit register.
> + */
> +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
> +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
> +bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
>  
> diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
> index 821a2c1e180a..813dc28b9232 100644
> --- a/lib/x86/asm/pci.h
> +++ b/lib/x86/asm/pci.h
> @@ -35,4 +35,10 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val
>      outl(val, 0xCFC);
>  }
>  
> +static inline
> +phys_addr_t pci_translate_addr(pcidevaddr_t __unused dev, uint64_t addr)

I put the __unused after the argument name.

> +{
> +    return addr;

Need tab here.

> +}
> +
>  #endif
> -- 
> 1.8.3.1
>

Thanks,
drew 

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> >  {
> >  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> > +	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > +	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> >  
> > -	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > -		return bar & PCI_BASE_ADDRESS_IO_MASK;
> > -	else
> > -		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> > +	if (pci_bar_is64(dev, bar_num)) {
> > +		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> > +		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
> > +	}
> > +
> > +	return (~(size & mask)) + 1;
> 
> All this casting of size and mask is pointless. Please rework it
> similar to what I did above.

This is the most compact and straight variant I was able to come up with:

	uint32_t bar = pci_bar_get(dev, bar_num);
	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

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

	return (~(size & mask)) + 1;

The casting is needed to avoid putting explicitly all 1s into higher
bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would
not bring correct results. I really struggle to make something better
readable.

> Thanks,
> drew 

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Andrew Jones]

On Tue, Jun 07, 2016 at 08:38:55AM +0200, Alexander Gordeev wrote:
> On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> > >  {
> > >  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> > > +	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > +	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > >  
> > > -	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > > -		return bar & PCI_BASE_ADDRESS_IO_MASK;
> > > -	else
> > > -		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> > > +	if (pci_bar_is64(dev, bar_num)) {
> > > +		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> > > +		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
> > > +	}
> > > +
> > > +	return (~(size & mask)) + 1;
> > 
> > All this casting of size and mask is pointless. Please rework it
> > similar to what I did above.
> 
> This is the most compact and straight variant I was able to come up with:
> 
> 	uint32_t bar = pci_bar_get(dev, bar_num);
> 	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);

But this is wrong. If your 32-bit size was 0x80000000, then you now
say it's 0xffffffff80000000.

> 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

It might be OK to do that here, on a mask, but even if it is, then I
don't like it, because it's too subtle (like I said for the casting
in pci_bar_addr in my last reply). I don't like that it requires us to
know that masking bit 31 in a 32-bit mask means we also want to mask
63..32. That should at least be in a comment somewhere.

> 
> 	if (pci_bar_is64(dev, bar_num))
> 		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;
> 
> 	return (~(size & mask)) + 1;
> 
> The casting is needed to avoid putting explicitly all 1s into higher
> bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would
> not bring correct results. I really struggle to make something better
> readable.

It's not just about readability, it's about correctness. You shouldn't
use 32-bit sizes/masks with 64-bit data this way. Like I said in the
last reply, you should rework it like I showed you, operate on the
32-bit data with the 32-bit mask/size, and then eventually construct
64-bit data.

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Tue, Jun 07, 2016 at 09:03:18AM +0200, Andrew Jones wrote:
> On Tue, Jun 07, 2016 at 08:38:55AM +0200, Alexander Gordeev wrote:
> > On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > > > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> > > >  {
> > > >  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> > > > +	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > > +	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > > >  
> > > > -	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > > > -		return bar & PCI_BASE_ADDRESS_IO_MASK;
> > > > -	else
> > > > -		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> > > > +	if (pci_bar_is64(dev, bar_num)) {
> > > > +		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> > > > +		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
> > > > +	}
> > > > +
> > > > +	return (~(size & mask)) + 1;
> > > 
> > > All this casting of size and mask is pointless. Please rework it
> > > similar to what I did above.
> > 
> > This is the most compact and straight variant I was able to come up with:
> > 
> > 	uint32_t bar = pci_bar_get(dev, bar_num);
> > 	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> 
> But this is wrong. If your 32-bit size was 0x80000000, then you now
> say it's 0xffffffff80000000.

Hmm.. I am either terribly missing the point or we are on different
pages.

So if pci_bar_size32() returned 0x80000000 the size sign-extension
gives 0xffffffff80000000 and the mask sign extension gives (i.e.
mmio) 0xfffffffffffffff0. The AND gives 0xffffffff80000000 and the
NOT gives 0x7fffffff. Finally, 0x7fffffff + 1 gives 0x80000000.

If we do not sign-extend (or explicitly OR with 0xffffffff00000000)
size and/or mask then the return(~(size & mask)) + 1 gives a wrong
0xffffffff80000000.

> > 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> 
> It might be OK to do that here, on a mask, but even if it is, then I
> don't like it, because it's too subtle (like I said for the casting
> in pci_bar_addr in my last reply). I don't like that it requires us to
> know that masking bit 31 in a 32-bit mask means we also want to mask
> 63..32. That should at least be in a comment somewhere.

Yes, but it is not an arbitrary mask, it is an alignment mask.
We unconditionally want to mask 63..32, 31th and even lower.

> > 	if (pci_bar_is64(dev, bar_num))
> > 		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;
> > 
> > 	return (~(size & mask)) + 1;
> > 
> > The casting is needed to avoid putting explicitly all 1s into higher
> > bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would
> > not bring correct results. I really struggle to make something better
> > readable.
> 
> It's not just about readability, it's about correctness. You shouldn't
> use 32-bit sizes/masks with 64-bit data this way. Like I said in the
> last reply, you should rework it like I showed you, operate on the
> 32-bit data with the 32-bit mask/size, and then eventually construct
> 64-bit data.

It is all about those last +1 and shifting bits from lower to upper
word of 64-bit value. My hope was to off-load it to the compiler :)

> Thanks,
> drew

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Tue, Jun 07, 2016 at 12:33:54PM +0200, Alexander Gordeev wrote:
> > > 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > 
> > It might be OK to do that here, on a mask, but even if it is, then I
> > don't like it, because it's too subtle (like I said for the casting
> > in pci_bar_addr in my last reply). I don't like that it requires us to
> > know that masking bit 31 in a 32-bit mask means we also want to mask
> > 63..32. That should at least be in a comment somewhere.
> 
> Yes, but it is not an arbitrary mask, it is an alignment mask.
> We unconditionally want to mask 63..32, 31th and even lower.

Seems like making pci_bar_mask() return phys_addr_t rather than
uint32_t would make more sense.

> > > 	if (pci_bar_is64(dev, bar_num))
> > > 		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;
> > > 
> > > 	return (~(size & mask)) + 1;
> > > 
> > > The casting is needed to avoid putting explicitly all 1s into higher
> > > bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would
> > > not bring correct results. I really struggle to make something better
> > > readable.
> > 
> > It's not just about readability, it's about correctness. You shouldn't
> > use 32-bit sizes/masks with 64-bit data this way. Like I said in the
> > last reply, you should rework it like I showed you, operate on the
> > 32-bit data with the 32-bit mask/size, and then eventually construct
> > 64-bit data.
> 
> It is all about those last +1 and shifting bits from lower to upper
> word of 64-bit value. My hope was to off-load it to the compiler :)
> 
> > Thanks,
> > drew

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Andrew Jones]

On Tue, Jun 07, 2016 at 01:23:35PM +0200, Alexander Gordeev wrote:
> On Tue, Jun 07, 2016 at 12:33:54PM +0200, Alexander Gordeev wrote:
> > > > 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > > 
> > > It might be OK to do that here, on a mask, but even if it is, then I
> > > don't like it, because it's too subtle (like I said for the casting
> > > in pci_bar_addr in my last reply). I don't like that it requires us to
> > > know that masking bit 31 in a 32-bit mask means we also want to mask
> > > 63..32. That should at least be in a comment somewhere.
> > 
> > Yes, but it is not an arbitrary mask, it is an alignment mask.
> > We unconditionally want to mask 63..32, 31th and even lower.
> 
> Seems like making pci_bar_mask() return phys_addr_t rather than
> uint32_t would make more sense.

See last reply. If a BAR mask is suppose to operate on a BAR (4 bytes)
then it should be 32-bits and not applied to a 64-bit operation. And,
as the upper 32-bits of a 64-bit bar don't need any masking anyway,
then there's no point in complicating things by attempting to mask it.

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Andrew Jones]

On Tue, Jun 07, 2016 at 12:33:54PM +0200, Alexander Gordeev wrote:
> On Tue, Jun 07, 2016 at 09:03:18AM +0200, Andrew Jones wrote:
> > On Tue, Jun 07, 2016 at 08:38:55AM +0200, Alexander Gordeev wrote:
> > > On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > > > > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> > > > >  {
> > > > >  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> > > > > +	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > > > +	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > > > >  
> > > > > -	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > > > > -		return bar & PCI_BASE_ADDRESS_IO_MASK;
> > > > > -	else
> > > > > -		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> > > > > +	if (pci_bar_is64(dev, bar_num)) {
> > > > > +		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> > > > > +		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
> > > > > +	}
> > > > > +
> > > > > +	return (~(size & mask)) + 1;
> > > > 
> > > > All this casting of size and mask is pointless. Please rework it
> > > > similar to what I did above.
> > > 
> > > This is the most compact and straight variant I was able to come up with:
> > > 
> > > 	uint32_t bar = pci_bar_get(dev, bar_num);
> > > 	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > 
> > But this is wrong. If your 32-bit size was 0x80000000, then you now
> > say it's 0xffffffff80000000.
> 
> Hmm.. I am either terribly missing the point or we are on different
> pages.

Let's make sure I know what the function is supposed to do.

Based on your comment explaining how to get the PCI bar size,
it sounds like we should do these steps; let's assume a device
needs size=0x200000, and is 32-bit

 read-write1s-dance	barsz=0x00000000
 readl			barsz=0xffe0000c
 mask			barsz=0xffe00000

 barsz = ~barsz + 1	barsz=0x00200000

Now let's say it's 64-bit

 read-write1s-dance	barsz_lo=0x00000000
 readl			barsz_lo=0xffe0000c
 mask			barsz_lo=0xffe00000
 read-write1s-dance	barsz_hi=0x00000000
 readl			barsz_hi=0xffffffff
 <upper 32-bit mask is 0xffffffff, so don't bother masking at all>

 barsz = ~((phys_addr_t)barsz_hi << 32 | barsz_lo) + 1 (barsz=0x200000)

So you need 5 functions

uint32_t get_mask(int bar)
{
...
}

uint32_t read_dance(int bar)
{
  read-write1s-dance
  return readl
}

uint32_t get_size32(int bar)
{
  uint32_t size = read_dance(bar) & get_mask(bar);
  return ~size + 1;
}

uint64_t get_size64(int bar)
{
  uint64_t size = read_dance(bar) & get_mask(bar);
  uint64_t size_hi = read_dance(bar + 1);

  size |= size_hi << 32;
  return ~size + 1;
}

uint64_t get_size(int bar)
{
  return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
}

> 
> So if pci_bar_size32() returned 0x80000000 the size sign-extension
> gives 0xffffffff80000000 and the mask sign extension gives (i.e.
> mmio) 0xfffffffffffffff0. The AND gives 0xffffffff80000000 and the
> NOT gives 0x7fffffff. Finally, 0x7fffffff + 1 gives 0x80000000.

I don't see the point of using the upper 32-bits to calculate a
32-bit value from 32-bit inputs.

> 
> If we do not sign-extend (or explicitly OR with 0xffffffff00000000)
> size and/or mask then the return(~(size & mask)) + 1 gives a wrong
> 0xffffffff80000000.
> 
> > > 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > 
> > It might be OK to do that here, on a mask, but even if it is, then I
> > don't like it, because it's too subtle (like I said for the casting
> > in pci_bar_addr in my last reply). I don't like that it requires us to
> > know that masking bit 31 in a 32-bit mask means we also want to mask
> > 63..32. That should at least be in a comment somewhere.
> 
> Yes, but it is not an arbitrary mask, it is an alignment mask.
> We unconditionally want to mask 63..32, 31th and even lower.

If you use sign-extension to generate the alignment mask, then
it needs a comment. But if we can avoid the headache of 64-bit
data computing 32-bit results then let's avoid it. (Even though
you described to me how it's supposed to work, I'm still not
sure it's safe for all cases :-)

> 
> > > 	if (pci_bar_is64(dev, bar_num))
> > > 		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;
> > > 
> > > 	return (~(size & mask)) + 1;
> > > 
> > > The casting is needed to avoid putting explicitly all 1s into higher
> > > bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would
> > > not bring correct results. I really struggle to make something better
> > > readable.
> > 
> > It's not just about readability, it's about correctness. You shouldn't
> > use 32-bit sizes/masks with 64-bit data this way. Like I said in the
> > last reply, you should rework it like I showed you, operate on the
> > 32-bit data with the 32-bit mask/size, and then eventually construct
> > 64-bit data.
> 
> It is all about those last +1 and shifting bits from lower to upper
> word of 64-bit value. My hope was to off-load it to the compiler :)

I think the problem comes from trying to apply a mask to the upper bits,
even though it doesn't need one. Off-loading to the compiler is a good
idea, but we can avoid being compilers ourselves by writing short, simple
to understand functions, and then letting the compiler inline and optimize
them for us :-)

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Tue, Jun 07, 2016 at 04:08:22PM +0200, Andrew Jones wrote:
> On Tue, Jun 07, 2016 at 12:33:54PM +0200, Alexander Gordeev wrote:
> > On Tue, Jun 07, 2016 at 09:03:18AM +0200, Andrew Jones wrote:
> > > On Tue, Jun 07, 2016 at 08:38:55AM +0200, Alexander Gordeev wrote:
> > > > On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > > > > > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> > > > > >  {
> > > > > >  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> > > > > > +	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > > > > +	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > > > > >  
> > > > > > -	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > > > > > -		return bar & PCI_BASE_ADDRESS_IO_MASK;
> > > > > > -	else
> > > > > > -		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> > > > > > +	if (pci_bar_is64(dev, bar_num)) {
> > > > > > +		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> > > > > > +		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
> > > > > > +	}
> > > > > > +
> > > > > > +	return (~(size & mask)) + 1;
> > > > > 
> > > > > All this casting of size and mask is pointless. Please rework it
> > > > > similar to what I did above.
> > > > 
> > > > This is the most compact and straight variant I was able to come up with:
> > > > 
> > > > 	uint32_t bar = pci_bar_get(dev, bar_num);
> > > > 	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > 
> > > But this is wrong. If your 32-bit size was 0x80000000, then you now
> > > say it's 0xffffffff80000000.
> > 
> > Hmm.. I am either terribly missing the point or we are on different
> > pages.
> 
> Let's make sure I know what the function is supposed to do.
> 
> Based on your comment explaining how to get the PCI bar size,
> it sounds like we should do these steps; let's assume a device
> needs size=0x200000, and is 32-bit

I am not quite sure what barsz=0x00000000 at read-write1s-dance	
phase is, but otherwise it seems correct to me (well may be a nit -
the 32-bit variant denoted 64-bit mmio).

>  read-write1s-dance	barsz=0x00000000
>  readl			barsz=0xffe0000c
>  mask			barsz=0xffe00000
> 
>  barsz = ~barsz + 1	barsz=0x00200000
> 
> Now let's say it's 64-bit
> 
>  read-write1s-dance	barsz_lo=0x00000000
>  readl			barsz_lo=0xffe0000c
>  mask			barsz_lo=0xffe00000
>  read-write1s-dance	barsz_hi=0x00000000
>  readl			barsz_hi=0xffffffff
>  <upper 32-bit mask is 0xffffffff, so don't bother masking at all>
> 
>  barsz = ~((phys_addr_t)barsz_hi << 32 | barsz_lo) + 1 (barsz=0x200000)
> 
> So you need 5 functions
> 
> uint32_t get_mask(int bar)
> {
> ...
> }
> 
> uint32_t read_dance(int bar)
> {
>   read-write1s-dance
>   return readl
> }
> 
> uint32_t get_size32(int bar)
> {
>   uint32_t size = read_dance(bar) & get_mask(bar);
>   return ~size + 1;
> }
> 
> uint64_t get_size64(int bar)
> {
>   uint64_t size = read_dance(bar) & get_mask(bar);
>   uint64_t size_hi = read_dance(bar + 1);
> 
>   size |= size_hi << 32;
>   return ~size + 1;
> }
> 
> uint64_t get_size(int bar)
> {
>   return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
> }

If the above is a pseudo code or you want me to rework using these
functions? If not, below is how it could look like.

Also, looking at your get_size() I noticed a bug in my version :-)
when read_dance() is followed by readl() returns zero - that means no
(more) BAR(s).

phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
{
	uint32_t size;
	uint32_t bar;

	size = pci_bar_size32(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_size32(dev, bar_num + 1);
		size64 = (size64 << 32) | size;

		return ~size64 + 1;
	} else {
		return ~size + 1;
	}
}

> > So if pci_bar_size32() returned 0x80000000 the size sign-extension
> > gives 0xffffffff80000000 and the mask sign extension gives (i.e.
> > mmio) 0xfffffffffffffff0. The AND gives 0xffffffff80000000 and the
> > NOT gives 0x7fffffff. Finally, 0x7fffffff + 1 gives 0x80000000.
> 
> I don't see the point of using the upper 32-bits to calculate a
> 32-bit value from 32-bit inputs.

It is indeed too complicated. My point has faded away :)

> > If we do not sign-extend (or explicitly OR with 0xffffffff00000000)
> > size and/or mask then the return(~(size & mask)) + 1 gives a wrong
> > 0xffffffff80000000.
> > 
> > > > 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > > 
> > > It might be OK to do that here, on a mask, but even if it is, then I
> > > don't like it, because it's too subtle (like I said for the casting
> > > in pci_bar_addr in my last reply). I don't like that it requires us to
> > > know that masking bit 31 in a 32-bit mask means we also want to mask
> > > 63..32. That should at least be in a comment somewhere.
> > 
> > Yes, but it is not an arbitrary mask, it is an alignment mask.
> > We unconditionally want to mask 63..32, 31th and even lower.
> 
> If you use sign-extension to generate the alignment mask, then
> it needs a comment. But if we can avoid the headache of 64-bit
> data computing 32-bit results then let's avoid it. (Even though
> you described to me how it's supposed to work, I'm still not
> sure it's safe for all cases :-)
> 
> > 
> > > > 	if (pci_bar_is64(dev, bar_num))
> > > > 		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;
> > > > 
> > > > 	return (~(size & mask)) + 1;
> > > > 
> > > > The casting is needed to avoid putting explicitly all 1s into higher
> > > > bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would
> > > > not bring correct results. I really struggle to make something better
> > > > readable.
> > > 
> > > It's not just about readability, it's about correctness. You shouldn't
> > > use 32-bit sizes/masks with 64-bit data this way. Like I said in the
> > > last reply, you should rework it like I showed you, operate on the
> > > 32-bit data with the 32-bit mask/size, and then eventually construct
> > > 64-bit data.
> > 
> > It is all about those last +1 and shifting bits from lower to upper
> > word of 64-bit value. My hope was to off-load it to the compiler :)
> 
> I think the problem comes from trying to apply a mask to the upper bits,
> even though it doesn't need one. Off-loading to the compiler is a good
> idea, but we can avoid being compilers ourselves by writing short, simple
> to understand functions, and then letting the compiler inline and optimize
> them for us :-)
> 
> Thanks,
> drew

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Andrew Jones]

On Tue, Jun 07, 2016 at 10:00:42PM +0200, Alexander Gordeev wrote:
> On Tue, Jun 07, 2016 at 04:08:22PM +0200, Andrew Jones wrote:
> > On Tue, Jun 07, 2016 at 12:33:54PM +0200, Alexander Gordeev wrote:
> > > On Tue, Jun 07, 2016 at 09:03:18AM +0200, Andrew Jones wrote:
> > > > On Tue, Jun 07, 2016 at 08:38:55AM +0200, Alexander Gordeev wrote:
> > > > > On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > > > > > > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> > > > > > >  {
> > > > > > >  	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> > > > > > > +	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > > > > > +	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> > > > > > >  
> > > > > > > -	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > > > > > > -		return bar & PCI_BASE_ADDRESS_IO_MASK;
> > > > > > > -	else
> > > > > > > -		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> > > > > > > +	if (pci_bar_is64(dev, bar_num)) {
> > > > > > > +		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
> > > > > > > +		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return (~(size & mask)) + 1;
> > > > > > 
> > > > > > All this casting of size and mask is pointless. Please rework it
> > > > > > similar to what I did above.
> > > > > 
> > > > > This is the most compact and straight variant I was able to come up with:
> > > > > 
> > > > > 	uint32_t bar = pci_bar_get(dev, bar_num);
> > > > > 	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
> > > > 
> > > > But this is wrong. If your 32-bit size was 0x80000000, then you now
> > > > say it's 0xffffffff80000000.
> > > 
> > > Hmm.. I am either terribly missing the point or we are on different
> > > pages.
> > 
> > Let's make sure I know what the function is supposed to do.
> > 
> > Based on your comment explaining how to get the PCI bar size,
> > it sounds like we should do these steps; let's assume a device
> > needs size=0x200000, and is 32-bit
> 
> I am not quite sure what barsz=0x00000000 at read-write1s-dance	

barsz is initialized to zero in the sequence in order to start the
variable descriptions with something, but it's not necessary to do
so with real code.

> phase is, but otherwise it seems correct to me (well may be a nit -
> the 32-bit variant denoted 64-bit mmio).

Not sure what you mean, but it doesn't matter for this :-)

> 
> >  read-write1s-dance	barsz=0x00000000
> >  readl			barsz=0xffe0000c
> >  mask			barsz=0xffe00000
> > 
> >  barsz = ~barsz + 1	barsz=0x00200000
> > 
> > Now let's say it's 64-bit
> > 
> >  read-write1s-dance	barsz_lo=0x00000000
> >  readl			barsz_lo=0xffe0000c
> >  mask			barsz_lo=0xffe00000
> >  read-write1s-dance	barsz_hi=0x00000000
> >  readl			barsz_hi=0xffffffff
> >  <upper 32-bit mask is 0xffffffff, so don't bother masking at all>
> > 
> >  barsz = ~((phys_addr_t)barsz_hi << 32 | barsz_lo) + 1 (barsz=0x200000)
> > 
> > So you need 5 functions
> > 
> > uint32_t get_mask(int bar)
> > {
> > ...
> > }
> > 
> > uint32_t read_dance(int bar)
> > {
> >   read-write1s-dance
> >   return readl
> > }
> > 
> > uint32_t get_size32(int bar)
> > {
> >   uint32_t size = read_dance(bar) & get_mask(bar);
> >   return ~size + 1;
> > }
> > 
> > uint64_t get_size64(int bar)
> > {
> >   uint64_t size = read_dance(bar) & get_mask(bar);
> >   uint64_t size_hi = read_dance(bar + 1);
> > 
> >   size |= size_hi << 32;
> >   return ~size + 1;
> > }
> > 
> > uint64_t get_size(int bar)
> > {
> >   return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
> > }
> 
> If the above is a pseudo code or you want me to rework using these
> functions?

Yes and no. You can do what you like, but pci_bar_size32 should
return a size (like my get_size32), not a 2's complement of size,
otherwise the name is wrong.

> If not, below is how it could look like.
> 
> Also, looking at your get_size() I noticed a bug in my version :-)
> when read_dance() is followed by readl() returns zero - that means no
> (more) BAR(s).
> 
> phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> {
> 	uint32_t size;
> 	uint32_t bar;
> 
> 	size = pci_bar_size32(dev, bar_num);
> 	if (!size)
> 		return 0;

Should callers ever call pci_bar_size on a bar_num that will result
in zero, "no more BARs"? If not, then you can assert here. Otherwise
will all callers know what zero means, and check for it?

> 
> 	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_size32(dev, bar_num + 1);
> 		size64 = (size64 << 32) | size;
> 
> 		return ~size64 + 1;
> 	} else {
> 		return ~size + 1;
> 	}
> }
>

Otherwise I think it's right.

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Wed, Jun 08, 2016 at 01:59:59PM +0200, Andrew Jones wrote:
> > > So you need 5 functions
> > > 
> > > uint32_t get_mask(int bar)
> > > {
> > > ...
> > > }
> > > 
> > > uint32_t read_dance(int bar)
> > > {
> > >   read-write1s-dance
> > >   return readl
> > > }
> > > 
> > > uint32_t get_size32(int bar)
> > > {
> > >   uint32_t size = read_dance(bar) & get_mask(bar);
> > >   return ~size + 1;
> > > }
> > > 
> > > uint64_t get_size64(int bar)
> > > {
> > >   uint64_t size = read_dance(bar) & get_mask(bar);
> > >   uint64_t size_hi = read_dance(bar + 1);
> > > 
> > >   size |= size_hi << 32;
> > >   return ~size + 1;
> > > }
> > > 
> > > uint64_t get_size(int bar)
> > > {
> > >   return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
> > > }
> > 
> > If the above is a pseudo code or you want me to rework using these
> > functions?
> 
> Yes and no. You can do what you like, but pci_bar_size32 should
> return a size (like my get_size32), not a 2's complement of size,
> otherwise the name is wrong.

Right. So my pci_bar_size32 is your read_dance. Want to use that name
or i.e. pci_bar_size_complement?

> > If not, below is how it could look like.
> > 
> > Also, looking at your get_size() I noticed a bug in my version :-)
> > when read_dance() is followed by readl() returns zero - that means no
> > (more) BAR(s).
> > 
> > phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> > {
> > 	uint32_t size;
> > 	uint32_t bar;
> > 
> > 	size = pci_bar_size32(dev, bar_num);
> > 	if (!size)
> > 		return 0;
> 
> Should callers ever call pci_bar_size on a bar_num that will result
> in zero, "no more BARs"? If not, then you can assert here. Otherwise
> will all callers know what zero means, and check for it?

Yes, it is an indication a BAR is implemented in HW. But I misled you :/
I remember some code stopped on a zero-sized BAR, but I do not recall
what code it was. There is nothing in specs about stopping and both
Seabios and Linux just continue probing. So I will update the breaks to
continues.

> Thanks,
> drew

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Andrew Jones]

On Thu, Jun 09, 2016 at 10:41:02PM +0200, Alexander Gordeev wrote:
> On Wed, Jun 08, 2016 at 01:59:59PM +0200, Andrew Jones wrote:
> > > > So you need 5 functions
> > > > 
> > > > uint32_t get_mask(int bar)
> > > > {
> > > > ...
> > > > }
> > > > 
> > > > uint32_t read_dance(int bar)
> > > > {
> > > >   read-write1s-dance
> > > >   return readl
> > > > }
> > > > 
> > > > uint32_t get_size32(int bar)
> > > > {
> > > >   uint32_t size = read_dance(bar) & get_mask(bar);
> > > >   return ~size + 1;
> > > > }
> > > > 
> > > > uint64_t get_size64(int bar)
> > > > {
> > > >   uint64_t size = read_dance(bar) & get_mask(bar);
> > > >   uint64_t size_hi = read_dance(bar + 1);
> > > > 
> > > >   size |= size_hi << 32;
> > > >   return ~size + 1;
> > > > }
> > > > 
> > > > uint64_t get_size(int bar)
> > > > {
> > > >   return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
> > > > }
> > > 
> > > If the above is a pseudo code or you want me to rework using these
> > > functions?
> > 
> > Yes and no. You can do what you like, but pci_bar_size32 should
> > return a size (like my get_size32), not a 2's complement of size,
> > otherwise the name is wrong.
> 
> Right. So my pci_bar_size32 is your read_dance. Want to use that name
> or i.e. pci_bar_size_complement?

read-dance was more of joke name, but I am starting to like it :-)

pci_bar_size_complement isn't right, as it returns an unmasked value,
not the real complement.

pci_bar_size_helper()? pci_bar_size_read_dance()?

Whatever you like :-)


> 
> > > If not, below is how it could look like.
> > > 
> > > Also, looking at your get_size() I noticed a bug in my version :-)
> > > when read_dance() is followed by readl() returns zero - that means no
> > > (more) BAR(s).
> > > 
> > > phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> > > {
> > > 	uint32_t size;
> > > 	uint32_t bar;
> > > 
> > > 	size = pci_bar_size32(dev, bar_num);
> > > 	if (!size)
> > > 		return 0;
> > 
> > Should callers ever call pci_bar_size on a bar_num that will result
> > in zero, "no more BARs"? If not, then you can assert here. Otherwise
> > will all callers know what zero means, and check for it?
> 
> Yes, it is an indication a BAR is implemented in HW. But I misled you :/
> I remember some code stopped on a zero-sized BAR, but I do not recall
> what code it was. There is nothing in specs about stopping and both
> Seabios and Linux just continue probing. So I will update the breaks to
> continues.
> 
> > Thanks,
> > drew
> --
> 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 v4 04/12] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > +/*
> > + * BAR number in all BAR access functions below is a number of 32-bit
> > + * register starting from PCI_BASE_ADDRESS_0 offset.
> > + *
> > + * In cases BAR size is 64-bit a caller should still provide BAR number
> > + * in terms of 32-bit words. For example, if a device has 64-bit BAR#0
> > + * and 32-bit BAR#1 the caller should provide 2 to address BAR#1, not 1.
> 
> Is this how people label bars? I.e. they call a bar following a 64-bit
> bar BAR#N+1? Or do they skip numbers with the labels to match the
> offsets? I.e. in this example you'd say BAR#0 (64-bit), there is no BAR#1,
> and then BAR#2 (32-bit) when labeling?

I do not think there is an agreed notation here. I would guess, when it
comes to a particular device people label registers as described in
the device specification. But I can easily imagine specifications using
both ways.

> Thanks,
> drew 

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Alexander Gordeev]

On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
> > index 821a2c1e180a..813dc28b9232 100644
> > --- a/lib/x86/asm/pci.h
> > +++ b/lib/x86/asm/pci.h
> > @@ -35,4 +35,10 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val
> >      outl(val, 0xCFC);
> >  }
> >  
> > +static inline
> > +phys_addr_t pci_translate_addr(pcidevaddr_t __unused dev, uint64_t addr)
> 
> I put the __unused after the argument name.
> 
> > +{
> > +    return addr;
> 
> Need tab here.

It will differ then from the coding style of the other functions
in this header (and elsewhere in x86).

> > +}
> > +
> >  #endif
> > -- 
> > 1.8.3.1
> >
> 
> Thanks,
> drew 

Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr()

[Andrew Jones]

On Fri, Jun 10, 2016 at 08:56:59PM +0200, Alexander Gordeev wrote:
> On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote:
> > > diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
> > > index 821a2c1e180a..813dc28b9232 100644
> > > --- a/lib/x86/asm/pci.h
> > > +++ b/lib/x86/asm/pci.h
> > > @@ -35,4 +35,10 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val
> > >      outl(val, 0xCFC);
> > >  }
> > >  
> > > +static inline
> > > +phys_addr_t pci_translate_addr(pcidevaddr_t __unused dev, uint64_t addr)
> > 
> > I put the __unused after the argument name.
> > 
> > > +{
> > > +    return addr;
> > 
> > Need tab here.
> 
> It will differ then from the coding style of the other functions
> in this header (and elsewhere in x86).

ah, nevermind then
> 
> > > +}
> > > +
> > >  #endif
> > > -- 
> > > 1.8.3.1
> > >
> > 
> > Thanks,
> > drew 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alexander Gordeev]

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

diff --git a/lib/pci.c b/lib/pci.c
index 07c911820e20..d092e22b8804 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -27,14 +27,18 @@ static uint32_t pci_bar_mask(uint32_t bar)
 		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);
+}
+
 phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
-	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
-	phys_addr_t bar = pci_config_readl(dev, off);
+	phys_addr_t bar = pci_bar_get(dev, bar_num);
 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
 
 	if (pci_bar_is64(dev, bar_num))
-		bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;
+		bar |= (phys_addr_t)pci_bar_get(dev, bar_num + 1) << 32;
 
 	return pci_translate_addr(dev, bar & mask);
 }
@@ -64,7 +68,7 @@ static uint32_t pci_bar_size32(pcidevaddr_t dev, int bar_num)
 
 phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	uint32_t bar = pci_bar_get(dev, bar_num);
 	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
 
@@ -78,19 +82,19 @@ phys_addr_t pci_bar_size(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)
 {
-	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;
 }
 
 bool pci_bar_is64(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 false;
-- 
1.8.3.1

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

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:34PM +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.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 07c911820e20..d092e22b8804 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -27,14 +27,18 @@ static uint32_t pci_bar_mask(uint32_t bar)
>  		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);
> +}
> +
>  phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
>  {
> -	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> -	phys_addr_t bar = pci_config_readl(dev, off);
> +	phys_addr_t bar = pci_bar_get(dev, bar_num);
>  	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
>  
>  	if (pci_bar_is64(dev, bar_num))
> -		bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;
> +		bar |= (phys_addr_t)pci_bar_get(dev, bar_num + 1) << 32;
>  
>  	return pci_translate_addr(dev, bar & mask);
>  }
> @@ -64,7 +68,7 @@ static uint32_t pci_bar_size32(pcidevaddr_t dev, int bar_num)
>  
>  phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
>  {
> -	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> +	uint32_t bar = pci_bar_get(dev, bar_num);
>  	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
>  	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
>  
> @@ -78,19 +82,19 @@ phys_addr_t pci_bar_size(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)
>  {
> -	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;
>  }
>  
>  bool pci_bar_is64(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));

Useless () around bar_num here

>  
>  	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
>  		return false;
> -- 
> 1.8.3.1
>

You probably could have introduced this function earlier in the
series, allowing you to immediately use it on new functions, but
it doesn't really matter. 

Besides the nit above,

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

[kvm-unit-tests PATCH v4 06/12] 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>
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 d092e22b8804..c9b2daa6a4ce 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)
 {
 	phys_addr_t bar = pci_bar_get(dev, bar_num);
 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
@@ -43,6 +43,16 @@ phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 	return pci_translate_addr(dev, bar & mask);
 }
 
+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 dbfe7918da37..a51d2ff52cec 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -27,7 +27,8 @@ 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 in the middle of a 64-bit register.
  */
-phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
+phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
+void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
 phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
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

Re: [kvm-unit-tests PATCH v4 06/12] pci: Add pci_bar_set_addr()

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:35PM +0200, Alexander Gordeev wrote:
> 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>
> 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 d092e22b8804..c9b2daa6a4ce 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)
>  {
>  	phys_addr_t bar = pci_bar_get(dev, bar_num);
>  	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
> @@ -43,6 +43,16 @@ phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
>  	return pci_translate_addr(dev, bar & mask);
>  }
>  
> +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 dbfe7918da37..a51d2ff52cec 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -27,7 +27,8 @@ 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 in the middle of a 64-bit register.
>   */
> -phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
> +phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
> +void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
>  phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
> 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
>

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

[kvm-unit-tests PATCH v4 07/12] pci: Add pci_dev_exists()

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: 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 c9b2daa6a4ce..5c107e7b4f4e 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) != (uint16_t)~0 &&
+		pci_config_readw(dev, PCI_DEVICE_ID) != (uint16_t)~0);
+}
+
 /* 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 a51d2ff52cec..db8296b2dfa7 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -14,6 +14,7 @@ typedef uint16_t pcidevaddr_t;
 enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
+bool pci_dev_exists(pcidevaddr_t dev);
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 
 /*
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v4 07/12] pci: Add pci_dev_exists()

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:36PM +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.c | 6 ++++++
>  lib/pci.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index c9b2daa6a4ce..5c107e7b4f4e 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) != (uint16_t)~0 &&
> +		pci_config_readw(dev, PCI_DEVICE_ID) != (uint16_t)~0);

I'd prefer 0xffff, it's even 6 less characters :-)

> +}
> +
>  /* 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 a51d2ff52cec..db8296b2dfa7 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -14,6 +14,7 @@ typedef uint16_t pcidevaddr_t;
>  enum {
>  	PCIDEVADDR_INVALID = 0xffff,
>  };

a blank line here would be nice

> +bool pci_dev_exists(pcidevaddr_t dev);
>  pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
>  
>  /*
> -- 
> 1.8.3.1

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

[kvm-unit-tests PATCH v4 08/12] pci: Add pci_print()

[Alexander Gordeev]

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

diff --git a/lib/pci.c b/lib/pci.c
index 5c107e7b4f4e..e715a3e51cc4 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -118,3 +118,82 @@ 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 start, end;
+		uint32_t bar;
+
+		if (!pci_bar_is_valid(dev, i))
+			break;
+
+		start = pci_bar_get_addr(dev, i);
+		end = start + pci_bar_size(dev, i) - 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_config_readw(dev, PCI_VENDOR_ID) != (uint16_t)~0 &&
+		    pci_config_readw(dev, PCI_DEVICE_ID) != (uint16_t)~0) {
+			pci_dev_print(dev);
+		}
+	}
+}
diff --git a/lib/pci.h b/lib/pci.h
index db8296b2dfa7..e9911db92320 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -14,6 +14,7 @@ typedef uint16_t pcidevaddr_t;
 enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
+void pci_print(void);
 bool pci_dev_exists(pcidevaddr_t dev);
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 
@@ -55,4 +56,6 @@ struct pci_test_dev_hdr {
 	uint8_t  name[];
 };
 
+#define  PCI_HEADER_TYPE_MASK		0x7f
+
 #endif /* PCI_H */
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v4 08/12] pci: Add pci_print()

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:37PM +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.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci.h |  3 +++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 5c107e7b4f4e..e715a3e51cc4 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -118,3 +118,82 @@ 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 start, end;
> +		uint32_t bar;
> +
> +		if (!pci_bar_is_valid(dev, i))
> +			break;
> +
> +		start = pci_bar_get_addr(dev, i);
> +		end = start + pci_bar_size(dev, i) - 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_config_readw(dev, PCI_VENDOR_ID) != (uint16_t)~0 &&
> +		    pci_config_readw(dev, PCI_DEVICE_ID) != (uint16_t)~0) {

Should use pci_dev_exists() here. Or, could also just unconditionally
call pci_dev_print here with a "if (!pci_dev_exists(dev)) return" in
pci_dev_print.

> +			pci_dev_print(dev);
> +		}
> +	}
> +}
> diff --git a/lib/pci.h b/lib/pci.h
> index db8296b2dfa7..e9911db92320 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -14,6 +14,7 @@ typedef uint16_t pcidevaddr_t;
>  enum {
>  	PCIDEVADDR_INVALID = 0xffff,
>  };
> +void pci_print(void);
>  bool pci_dev_exists(pcidevaddr_t dev);
>  pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
>  
> @@ -55,4 +56,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 v4 09/12] 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>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-host-generic.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h |  46 ++++++++
 lib/pci.c              |   4 +-
 lib/pci.h              |   3 +
 4 files changed, 345 insertions(+), 2 deletions(-)
 create mode 100644 lib/pci-host-generic.c
 create mode 100644 lib/pci-host-generic.h

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
new file mode 100644
index 000000000000..f01913fca053
--- /dev/null
+++ b/lib/pci-host-generic.c
@@ -0,0 +1,294 @@
+/*
+ * 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 claims the numerical representation of a PCI
+		 * address consists of three cells, encoded 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)
+				break;
+
+			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++, as++) {
+		if (pci_addr >= as->pci_start &&
+		    pci_addr < as->pci_start + as->size)
+			return as->start + (pci_addr - as->pci_start);
+	}
+
+	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 e715a3e51cc4..41a44d04d475 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 e9911db92320..37b282169a4e 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -15,6 +15,7 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
 void pci_print(void);
+bool pci_probe(void);
 bool pci_dev_exists(pcidevaddr_t dev);
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 
@@ -32,6 +33,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
 phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
+uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num);
+uint32_t pci_bar_mask(uint32_t bar);
 bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v4 09/12] pci: Add generic ECAM host support

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:38PM +0200, Alexander Gordeev wrote:
> 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>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci-host-generic.h |  46 ++++++++
>  lib/pci.c              |   4 +-
>  lib/pci.h              |   3 +
>  4 files changed, 345 insertions(+), 2 deletions(-)
>  create mode 100644 lib/pci-host-generic.c
>  create mode 100644 lib/pci-host-generic.h
> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> new file mode 100644
> index 000000000000..f01913fca053
> --- /dev/null
> +++ b/lib/pci-host-generic.c
> @@ -0,0 +1,294 @@
> +/*
> + * 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));

Maybe assert(bus_max + 1 == base.size / (1 << PCI_ECAM_BUS_SHIFT)) ?
Or is there a reason to have a larger address space than necessary?

> +
> +	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 claims the numerical representation of a PCI
> +		 * address consists of three cells, encoded as follows:

nit: No need to say 'claims', "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++;

nit: could have put this as++ up with the i++ like you do below

> +	}
> +	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)
> +				break;
> +
> +			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++, as++) {
> +		if (pci_addr >= as->pci_start &&
> +		    pci_addr < as->pci_start + as->size)
> +			return as->start + (pci_addr - as->pci_start);
> +	}
> +
> +	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 e715a3e51cc4..41a44d04d475 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 e9911db92320..37b282169a4e 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -15,6 +15,7 @@ enum {
>  	PCIDEVADDR_INVALID = 0xffff,
>  };
>  void pci_print(void);
> +bool pci_probe(void);
>  bool pci_dev_exists(pcidevaddr_t dev);
>  pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
>  
> @@ -32,6 +33,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
>  phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
>  void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
>  phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
> +uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num);
> +uint32_t pci_bar_mask(uint32_t bar);
>  bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> -- 
> 1.8.3.1
>

Looks great! Only a couple nits, so

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

Re: [kvm-unit-tests PATCH v4 09/12] pci: Add generic ECAM host support

[Alexander Gordeev]

On Mon, Jun 06, 2016 at 06:27:40PM +0200, Andrew Jones wrote:
> On Mon, Jun 06, 2016 at 02:46:38PM +0200, Alexander Gordeev wrote:
> > 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>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  lib/pci-host-generic.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/pci-host-generic.h |  46 ++++++++
> >  lib/pci.c              |   4 +-
> >  lib/pci.h              |   3 +
> >  4 files changed, 345 insertions(+), 2 deletions(-)
> >  create mode 100644 lib/pci-host-generic.c
> >  create mode 100644 lib/pci-host-generic.h
> > 
> > diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> > new file mode 100644
> > index 000000000000..f01913fca053
> > --- /dev/null
> > +++ b/lib/pci-host-generic.c
> > @@ -0,0 +1,294 @@
> > +/*
> > + * 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));
> 
> Maybe assert(bus_max + 1 == base.size / (1 << PCI_ECAM_BUS_SHIFT)) ?
> Or is there a reason to have a larger address space than necessary?

I can easily imagine a FW that does not bind number of busses vs
address space size, for whatever reason. But if we align with the
current QEMU implementation then == is better.

> > +	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 claims the numerical representation of a PCI
> > +		 * address consists of three cells, encoded as follows:
> 
> nit: No need to say 'claims', "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++;
> 
> nit: could have put this as++ up with the i++ like you do below
> 
> > +	}
> > +	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)
> > +				break;
> > +
> > +			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++, as++) {
> > +		if (pci_addr >= as->pci_start &&
> > +		    pci_addr < as->pci_start + as->size)
> > +			return as->start + (pci_addr - as->pci_start);
> > +	}
> > +
> > +	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 e715a3e51cc4..41a44d04d475 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 e9911db92320..37b282169a4e 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -15,6 +15,7 @@ enum {
> >  	PCIDEVADDR_INVALID = 0xffff,
> >  };
> >  void pci_print(void);
> > +bool pci_probe(void);
> >  bool pci_dev_exists(pcidevaddr_t dev);
> >  pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> >  
> > @@ -32,6 +33,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> >  phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
> >  void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
> >  phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
> > +uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num);
> > +uint32_t pci_bar_mask(uint32_t bar);
> >  bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
> >  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
> >  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> > -- 
> > 1.8.3.1
> >
> 
> Looks great! Only a couple nits, so
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [kvm-unit-tests PATCH v4 09/12] pci: Add generic ECAM host support

[Alexander Gordeev]

On Mon, Jun 06, 2016 at 06:27:40PM +0200, Andrew Jones wrote:
> On Mon, Jun 06, 2016 at 02:46:38PM +0200, Alexander Gordeev wrote:
> > +	for (i = 0; i < nr_addr_spaces; i++) {
> > +		/*
> > +		 * The PCI binding claims the numerical representation of a PCI
> > +		 * address consists of three cells, encoded as follows:
> 
> nit: No need to say 'claims', "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++;

	(2)

> > +	}
> > +
> > +	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++;
> 
> nit: could have put this as++ up with the i++ like you do below

Actually, I forgot to make it below (1) the same way as here :-)
The reason is there are three cycles looping over address spaces
and I wanted to make them alike. I chose (2) as the sample since
these two assignents read better:

	data += nr_range_cells;
	as++;

Will repost whatever you like, IOW ;)

> > +	}
> > +	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)
> > +				break;
> > +
> > +			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++, as++) {

	(1)

> > +		if (pci_addr >= as->pci_start &&
> > +		    pci_addr < as->pci_start + as->size)
> > +			return as->start + (pci_addr - as->pci_start);
> > +	}
> > +
> > +	return 0;
> > +}

Re: [kvm-unit-tests PATCH v4 09/12] pci: Add generic ECAM host support

[Andrew Jones]

On Sat, Jun 11, 2016 at 10:10:07PM +0200, Alexander Gordeev wrote:
> On Mon, Jun 06, 2016 at 06:27:40PM +0200, Andrew Jones wrote:
> > On Mon, Jun 06, 2016 at 02:46:38PM +0200, Alexander Gordeev wrote:
> > > +	for (i = 0; i < nr_addr_spaces; i++) {
> > > +		/*
> > > +		 * The PCI binding claims the numerical representation of a PCI
> > > +		 * address consists of three cells, encoded as follows:
> > 
> > nit: No need to say 'claims', "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++;
> 
> 	(2)
> 
> > > +	}
> > > +
> > > +	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++;
> > 
> > nit: could have put this as++ up with the i++ like you do below
> 
> Actually, I forgot to make it below (1) the same way as here :-)
> The reason is there are three cycles looping over address spaces
> and I wanted to make them alike. I chose (2) as the sample since
> these two assignents read better:
> 
> 	data += nr_range_cells;
> 	as++;
> 
> Will repost whatever you like, IOW ;)

whatever you like. it was just a nit.

> 
> > > +	}
> > > +	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)
> > > +				break;
> > > +
> > > +			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++, as++) {
> 
> 	(1)
> 
> > > +		if (pci_addr >= as->pci_start &&
> > > +		    pci_addr < as->pci_start + as->size)
> > > +			return as->start + (pci_addr - as->pci_start);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> --
> 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

[kvm-unit-tests PATCH v4 10/12] arm/arm64: pci: Add PCI bus operation test

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arm/Makefile.common |  6 +++++-
 arm/pci-test.c      | 21 +++++++++++++++++++++
 lib/arm/asm/pci.h   | 26 ++++++++++++++++++++++++++
 lib/arm64/asm/pci.h |  1 +
 4 files changed, 53 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 a786fcf94154..e27a3fd276ff 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
@@ -73,3 +76,4 @@ generated_files = $(asm-offsets)
 test_cases: $(generated_files) $(tests-common) $(tests)
 
 $(TEST_DIR)/selftest.o $(cstart.o): $(asm-offsets)
+$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.o
diff --git a/arm/pci-test.c b/arm/pci-test.c
new file mode 100644
index 000000000000..fde5495dd626
--- /dev/null
+++ b/arm/pci-test.c
@@ -0,0 +1,21 @@
+/*
+ * 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)
+{
+	int ret = pci_probe();
+
+	report("PCI bus probing", ret);
+
+	if (ret)
+		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..8263821ad511
--- /dev/null
+++ b/lib/arm/asm/pci.h
@@ -0,0 +1,26 @@
+#ifndef _ASMARM_PCI_H_
+#define _ASMARM_PCI_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 __unused dev, 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/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

Re: [kvm-unit-tests PATCH v4 10/12] arm/arm64: pci: Add PCI bus operation test

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:39PM +0200, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  arm/Makefile.common |  6 +++++-
>  arm/pci-test.c      | 21 +++++++++++++++++++++
>  lib/arm/asm/pci.h   | 26 ++++++++++++++++++++++++++
>  lib/arm64/asm/pci.h |  1 +
>  4 files changed, 53 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 a786fcf94154..e27a3fd276ff 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
> @@ -73,3 +76,4 @@ generated_files = $(asm-offsets)
>  test_cases: $(generated_files) $(tests-common) $(tests)
>  
>  $(TEST_DIR)/selftest.o $(cstart.o): $(asm-offsets)
> +$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.o

You don't need this line anymore. See 4c6b5d0b1c "arm/arm64:
Makefile cleanup"

> diff --git a/arm/pci-test.c b/arm/pci-test.c
> new file mode 100644
> index 000000000000..fde5495dd626
> --- /dev/null
> +++ b/arm/pci-test.c
> @@ -0,0 +1,21 @@
> +/*
> + * 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)
> +{
> +	int ret = pci_probe();
> +
> +	report("PCI bus probing", ret);
> +
> +	if (ret)
> +		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..8263821ad511
> --- /dev/null
> +++ b/lib/arm/asm/pci.h
> @@ -0,0 +1,26 @@
> +#ifndef _ASMARM_PCI_H_
> +#define _ASMARM_PCI_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 __unused dev, uint64_t addr)

__unused after dev

> +{
> +	/*
> +	 * 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

The code in lib/arm/asm/pci.h could probably be in a generic
include instead, and then included by lib/arm/asm/pci.h, but
I'm OK with this until we add ppc64 support.

> 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
>

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 10/12] arm/arm64: pci: Add PCI bus operation test

[Alexander Gordeev]

On Mon, Jun 06, 2016 at 06:39:40PM +0200, Andrew Jones wrote:
> > diff --git a/lib/arm/asm/pci.h b/lib/arm/asm/pci.h
> > new file mode 100644
> > index 000000000000..8263821ad511
> > --- /dev/null
> > +++ b/lib/arm/asm/pci.h
> > @@ -0,0 +1,26 @@
> > +#ifndef _ASMARM_PCI_H_
> > +#define _ASMARM_PCI_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 __unused dev, uint64_t addr)
> 
> __unused after dev
> 
> > +{
> > +	/*
> > +	 * 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
> 
> The code in lib/arm/asm/pci.h could probably be in a generic
> include instead, and then included by lib/arm/asm/pci.h, but
> I'm OK with this until we add ppc64 support.

You mean moving the PCI accessor declarations to asm-generic/pci.h?
But a generic include is rather for default implementations, not
just declarations that still whould have be defined somewhere?

> > 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
> >
> 
> Thanks,
> drew

Re: [kvm-unit-tests PATCH v4 10/12] arm/arm64: pci: Add PCI bus operation test

[Andrew Jones]

On Wed, Jun 08, 2016 at 08:53:42AM +0200, Alexander Gordeev wrote:
> On Mon, Jun 06, 2016 at 06:39:40PM +0200, Andrew Jones wrote:
> > > diff --git a/lib/arm/asm/pci.h b/lib/arm/asm/pci.h
> > > new file mode 100644
> > > index 000000000000..8263821ad511
> > > --- /dev/null
> > > +++ b/lib/arm/asm/pci.h
> > > @@ -0,0 +1,26 @@
> > > +#ifndef _ASMARM_PCI_H_
> > > +#define _ASMARM_PCI_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 __unused dev, uint64_t addr)
> > 
> > __unused after dev
> > 
> > > +{
> > > +	/*
> > > +	 * 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
> > 
> > The code in lib/arm/asm/pci.h could probably be in a generic
> > include instead, and then included by lib/arm/asm/pci.h, but
> > I'm OK with this until we add ppc64 support.
> 
> You mean moving the PCI accessor declarations to asm-generic/pci.h?
> But a generic include is rather for default implementations, not
> just declarations that still whould have be defined somewhere?

Right, but the implementation of pci_translate_addr above looks pretty
generic :-) Also putting the declarations somewhere common avoids
every arch that needs them from adding them. Arches that don't want
them, x86, simply don't include the common header. The common header
doesn't need to be named asm-generic/pci.h. It can be asm/pci-host-bridge.h
(or whatever). Since arm and ppc need the host bridge code for pci, then
it makes sense for them to include host bridge headers from their
asm/pci.h header.

Thanks,
drew

[kvm-unit-tests PATCH v4 11/12] 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 | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h         |   7 +++
 2 files changed, 191 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..32a4abe117b6
--- /dev/null
+++ b/lib/pci-testdev.c
@@ -0,0 +1,184 @@
+/*
+ * 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;
+		}
+	}
+
+	return (int)ops->io_readl(&test->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;
+
+	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
+	if (dev == PCIDEVADDR_INVALID)
+		return -1;
+
+	if (!pci_bar_is_valid(dev, 0) || !pci_bar_is_valid(dev, 1))
+		return -1;
+
+	addr = pci_bar_get_addr(dev, 0);
+	mem = ioremap(addr, 0);
+
+	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 37b282169a4e..ee64ab315030 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -39,6 +39,8 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 
+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
@@ -47,7 +49,12 @@ 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 v4 11/12] pci: Add pci-testdev PCI bus test device

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:40PM +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 | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci.h         |   7 +++
>  2 files changed, 191 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..32a4abe117b6
> --- /dev/null
> +++ b/lib/pci-testdev.c
> @@ -0,0 +1,184 @@
> +/*
> + * 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))

nit: no need for all the ()'s

> +		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;
> +		}
> +	}
> +
> +	return (int)ops->io_readl(&test->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;
> +
> +	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
> +	if (dev == PCIDEVADDR_INVALID)
> +		return -1;
> +
> +	if (!pci_bar_is_valid(dev, 0) || !pci_bar_is_valid(dev, 1))
> +		return -1;
> +
> +	addr = pci_bar_get_addr(dev, 0);
> +	mem = ioremap(addr, 0);
> +
> +	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 37b282169a4e..ee64ab315030 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -39,6 +39,8 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
>  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
>  
> +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
> @@ -47,7 +49,12 @@ 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

Just one nit. Looks great.

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

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

[Alexander Gordeev]

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

diff --git a/arm/Makefile.common b/arm/Makefile.common
index e27a3fd276ff..8d49203468cd 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 fde5495dd626..be228320e8c7 100644
--- a/arm/pci-test.c
+++ b/arm/pci-test.c
@@ -13,9 +13,19 @@ int main(void)
 	int ret = pci_probe();
 
 	report("PCI bus probing", ret);
+	if (!ret)
+		goto done;
 
-	if (ret)
-		pci_print();
+	pci_print();
 
+	if (pci_find_dev(PCI_VENDOR_ID_REDHAT,
+			 PCI_DEVICE_ID_REDHAT_TEST) == PCIDEVADDR_INVALID)
+		goto done;
+
+	ret = pci_testdev();
+	report("PCI test device passed %d tests",
+		ret >= PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS, ret);
+
+done:
 	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 "$@"
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v4 12/12] arm/arm64: pci: Add pci-testdev PCI device operation test

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:41PM +0200, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  arm/Makefile.common |  1 +
>  arm/pci-test.c      | 14 ++++++++++++--
>  arm/run             |  7 ++++++-
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index e27a3fd276ff..8d49203468cd 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 fde5495dd626..be228320e8c7 100644
> --- a/arm/pci-test.c
> +++ b/arm/pci-test.c
> @@ -13,9 +13,19 @@ int main(void)
>  	int ret = pci_probe();
>  
>  	report("PCI bus probing", ret);
> +	if (!ret)
> +		goto done;
>  
> -	if (ret)
> -		pci_print();
> +	pci_print();
>  
> +	if (pci_find_dev(PCI_VENDOR_ID_REDHAT,
> +			 PCI_DEVICE_ID_REDHAT_TEST) == PCIDEVADDR_INVALID)
> +		goto done;
> +
> +	ret = pci_testdev();
> +	report("PCI test device passed %d tests",
> +		ret >= PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS, ret);
> +
> +done:
>  	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 "$@"
> -- 
> 1.8.3.1
>

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

Re: [kvm-unit-tests PATCH v4 00/12] PCI bus support

[Andrew Jones]

On Mon, Jun 06, 2016 at 02:46:29PM +0200, Alexander Gordeev wrote:
> Hi all,
> 
> This series should be applied on top of "Cleanup low-level arch code"
> series which is still not included. Yet, it is ready for review as
> all previous comments and suggestions are addressed.
> 
> There might be some confusion about version numbering as I posted
> the previous version as RFC with no version number at all. In fact
> it was 3rd version so I am labelling this series as v4. Unlike the
> RFC it does not have gaps in implementation.
> 
> There are quite a lot of changes since the previous version.
> 
> I tried pci-testdev against ARM and got the device semi-operational.
> It is still to investigate, but that could be addressed separately.

I'd like to either have this resolved, or at least understood it
before we accept the series. If you drop your pci-testdev
implementation (patch 11/12) into x86/vmexit.c, in order to replace
the implementation there, does pci-testdev still work? I.e. have
you checked if x86 works both with its current implementation and
yours?

> 
> Most interesting - writing to IO BAR on ARM does not seem working as
> a written value does not read back. Probably, ARM64 is also affected,
> but again - I have not investigated it yet.

Maybe add some tracing to QEMU to see if the writes are making there.


Anyway, this series has really improved! I'm glad to see it coming
together!

Thanks,
drew

> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> 
> Alexander Gordeev (12):
>   pci: Fix coding style in generic PCI files
>   pci: x86: Rename pci_config_read() to pci_config_readl()
>   pci: x86: Add remaining PCI configuration space accessors
>   pci: Rework pci_bar_addr()
>   pci: Factor out pci_bar_get()
>   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    |   7 +-
>  arm/pci-test.c         |  31 ++++++
>  arm/run                |   7 +-
>  lib/arm/asm/pci.h      |  26 +++++
>  lib/arm64/asm/pci.h    |   1 +
>  lib/pci-host-generic.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci-host-generic.h |  46 ++++++++
>  lib/pci-testdev.c      | 184 +++++++++++++++++++++++++++++++
>  lib/pci.c              | 194 ++++++++++++++++++++++++++++----
>  lib/pci.h              |  33 +++++-
>  lib/x86/asm/pci.h      |  31 +++++-
>  x86/vmexit.c           |   4 +-
>  12 files changed, 830 insertions(+), 28 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/pci-host-generic.c
>  create mode 100644 lib/pci-host-generic.h
>  create mode 100644 lib/pci-testdev.c
> 
> -- 
> 1.8.3.1
> 

Re: [kvm-unit-tests PATCH v4 00/12] PCI bus support

[Alexander Gordeev]

On Mon, Jun 06, 2016 at 07:11:09PM +0200, Andrew Jones wrote:
> On Mon, Jun 06, 2016 at 02:46:29PM +0200, Alexander Gordeev wrote:
> > Hi all,
> > 
> > This series should be applied on top of "Cleanup low-level arch code"
> > series which is still not included. Yet, it is ready for review as
> > all previous comments and suggestions are addressed.
> > 
> > There might be some confusion about version numbering as I posted
> > the previous version as RFC with no version number at all. In fact
> > it was 3rd version so I am labelling this series as v4. Unlike the
> > RFC it does not have gaps in implementation.
> > 
> > There are quite a lot of changes since the previous version.
> > 
> > I tried pci-testdev against ARM and got the device semi-operational.
> > It is still to investigate, but that could be addressed separately.
> 
> I'd like to either have this resolved, or at least understood it
> before we accept the series. If you drop your pci-testdev
> implementation (patch 11/12) into x86/vmexit.c, in order to replace
> the implementation there, does pci-testdev still work? I.e. have
> you checked if x86 works both with its current implementation and
> yours?

pci-testdev does not work. It does not work for x86/vmexit.c as well,
but unlike my implementation x86/vmexit.c does not check status of
device writes and therefore fails to notice that the writes do not
take place for "wildcard-eventfd" and "datamatch-eventfd" cases.
CONFIG_EVENTFD is set, so I am not sure what is happening here.

pci-testdev also does not implement 16 and 32 bit accesses, although
x86/vmexit.c and pci/pci-testdev.c do have code to test these two.
It is not a bug though.

> > Most interesting - writing to IO BAR on ARM does not seem working as
> > a written value does not read back. Probably, ARM64 is also affected,
> > but again - I have not investigated it yet.
> 
> Maybe add some tracing to QEMU to see if the writes are making there.
> 
> 
> Anyway, this series has really improved! I'm glad to see it coming
> together!
> 
> Thanks,
> drew
> 
> > 
> > Cc: Thomas Huth <thuth@redhat.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > 
> > Alexander Gordeev (12):
> >   pci: Fix coding style in generic PCI files
> >   pci: x86: Rename pci_config_read() to pci_config_readl()
> >   pci: x86: Add remaining PCI configuration space accessors
> >   pci: Rework pci_bar_addr()
> >   pci: Factor out pci_bar_get()
> >   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    |   7 +-
> >  arm/pci-test.c         |  31 ++++++
> >  arm/run                |   7 +-
> >  lib/arm/asm/pci.h      |  26 +++++
> >  lib/arm64/asm/pci.h    |   1 +
> >  lib/pci-host-generic.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/pci-host-generic.h |  46 ++++++++
> >  lib/pci-testdev.c      | 184 +++++++++++++++++++++++++++++++
> >  lib/pci.c              | 194 ++++++++++++++++++++++++++++----
> >  lib/pci.h              |  33 +++++-
> >  lib/x86/asm/pci.h      |  31 +++++-
> >  x86/vmexit.c           |   4 +-
> >  12 files changed, 830 insertions(+), 28 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/pci-host-generic.c
> >  create mode 100644 lib/pci-host-generic.h
> >  create mode 100644 lib/pci-testdev.c
> > 
> > -- 
> > 1.8.3.1
> > 

Re: [kvm-unit-tests PATCH v4 00/12] PCI bus support

[Alexander Gordeev]

On Tue, Jun 21, 2016 at 09:02:39AM +0200, Alexander Gordeev wrote:
> > > I tried pci-testdev against ARM and got the device semi-operational.
> > > It is still to investigate, but that could be addressed separately.
> > 
> > I'd like to either have this resolved, or at least understood it
> > before we accept the series. If you drop your pci-testdev
> > implementation (patch 11/12) into x86/vmexit.c, in order to replace
> > the implementation there, does pci-testdev still work? I.e. have
> > you checked if x86 works both with its current implementation and
> > yours?
> 
> pci-testdev does not work. It does not work for x86/vmexit.c as well,
> but unlike my implementation x86/vmexit.c does not check status of
> device writes and therefore fails to notice that the writes do not
> take place for "wildcard-eventfd" and "datamatch-eventfd" cases.
> CONFIG_EVENTFD is set, so I am not sure what is happening here.

So I could define PCI_TESTDEV_NUM_TESTS to 1 and hence skip failing
tests. But on the other hand these failures are exposing a problem
in QEMU and therefore fulfilling kvm-unit-tests purpose :)

> pci-testdev also does not implement 16 and 32 bit accesses, although
> x86/vmexit.c and pci/pci-testdev.c do have code to test these two.
> It is not a bug though.

Re: [kvm-unit-tests PATCH v4 00/12] PCI bus support

[Andrew Jones]

On Mon, Jun 27, 2016 at 02:59:10PM +0200, Alexander Gordeev wrote:
> On Tue, Jun 21, 2016 at 09:02:39AM +0200, Alexander Gordeev wrote:
> > > > I tried pci-testdev against ARM and got the device semi-operational.
> > > > It is still to investigate, but that could be addressed separately.
> > > 
> > > I'd like to either have this resolved, or at least understood it
> > > before we accept the series. If you drop your pci-testdev
> > > implementation (patch 11/12) into x86/vmexit.c, in order to replace
> > > the implementation there, does pci-testdev still work? I.e. have
> > > you checked if x86 works both with its current implementation and
> > > yours?
> > 
> > pci-testdev does not work. It does not work for x86/vmexit.c as well,
> > but unlike my implementation x86/vmexit.c does not check status of
> > device writes and therefore fails to notice that the writes do not
> > take place for "wildcard-eventfd" and "datamatch-eventfd" cases.
> > CONFIG_EVENTFD is set, so I am not sure what is happening here.
> 
> So I could define PCI_TESTDEV_NUM_TESTS to 1 and hence skip failing
> tests. But on the other hand these failures are exposing a problem
> in QEMU and therefore fulfilling kvm-unit-tests purpose :)

Can you collect enough details to report it on qemu-devel?

Thanks,
drew

> 
> > pci-testdev also does not implement 16 and 32 bit accesses, although
> > x86/vmexit.c and pci/pci-testdev.c do have code to test these two.
> > It is not a bug though.
> --
> 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 v4 00/12] PCI bus support

[Alexander Gordeev]

On Mon, Jun 06, 2016 at 07:11:09PM +0200, Andrew Jones wrote:
> I'd like to either have this resolved, or at least understood it
> before we accept the series. If you drop your pci-testdev
> implementation (patch 11/12) into x86/vmexit.c, in order to replace
> the implementation there, does pci-testdev still work? I.e. have
> you checked if x86 works both with its current implementation and
> yours?

What if I post for inclusion updated patches 1-10/12 and continue
working on barriers/io/pci-testdev?

> Thanks,
> drew

Re: [kvm-unit-tests PATCH v4 00/12] PCI bus support

[Andrew Jones]

On Tue, Jun 28, 2016 at 12:54:51PM +0200, Alexander Gordeev wrote:
> On Mon, Jun 06, 2016 at 07:11:09PM +0200, Andrew Jones wrote:
> > I'd like to either have this resolved, or at least understood it
> > before we accept the series. If you drop your pci-testdev
> > implementation (patch 11/12) into x86/vmexit.c, in order to replace
> > the implementation there, does pci-testdev still work? I.e. have
> > you checked if x86 works both with its current implementation and
> > yours?
> 
> What if I post for inclusion updated patches 1-10/12 and continue
> working on barriers/io/pci-testdev?

Hmm.. the only way I can test the early patches without pci-testdev
is to dump the data structures, but that doesn't really test anything
other than the DT parsing. I'd rather we wait.

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 00/12] PCI bus support

[Alexander Gordeev]

On Tue, Jun 28, 2016 at 01:18:15PM +0200, Andrew Jones wrote:
> On Tue, Jun 28, 2016 at 12:54:51PM +0200, Alexander Gordeev wrote:
> > On Mon, Jun 06, 2016 at 07:11:09PM +0200, Andrew Jones wrote:
> > > I'd like to either have this resolved, or at least understood it
> > > before we accept the series. If you drop your pci-testdev
> > > implementation (patch 11/12) into x86/vmexit.c, in order to replace
> > > the implementation there, does pci-testdev still work? I.e. have
> > > you checked if x86 works both with its current implementation and
> > > yours?
> > 
> > What if I post for inclusion updated patches 1-10/12 and continue
> > working on barriers/io/pci-testdev?
> 
> Hmm.. the only way I can test the early patches without pci-testdev
> is to dump the data structures, but that doesn't really test anything
> other than the DT parsing. I'd rather we wait.

It also involves what would be PCI configuration space cycles: PCI bus
scanning and PCI devices inquiring and parsing. Not only DT.

> Thanks,
> drew

Re: [kvm-unit-tests PATCH v4 00/12] PCI bus support

[Andrew Jones]

On Tue, Jun 28, 2016 at 01:28:55PM +0200, Alexander Gordeev wrote:
> On Tue, Jun 28, 2016 at 01:18:15PM +0200, Andrew Jones wrote:
> > On Tue, Jun 28, 2016 at 12:54:51PM +0200, Alexander Gordeev wrote:
> > > On Mon, Jun 06, 2016 at 07:11:09PM +0200, Andrew Jones wrote:
> > > > I'd like to either have this resolved, or at least understood it
> > > > before we accept the series. If you drop your pci-testdev
> > > > implementation (patch 11/12) into x86/vmexit.c, in order to replace
> > > > the implementation there, does pci-testdev still work? I.e. have
> > > > you checked if x86 works both with its current implementation and
> > > > yours?
> > > 
> > > What if I post for inclusion updated patches 1-10/12 and continue
> > > working on barriers/io/pci-testdev?
> > 
> > Hmm.. the only way I can test the early patches without pci-testdev
> > is to dump the data structures, but that doesn't really test anything
> > other than the DT parsing. I'd rather we wait.
> 
> It also involves what would be PCI configuration space cycles: PCI bus
> scanning and PCI devices inquiring and parsing. Not only DT.

But how do I know any of that stuff is working?

> 
> > Thanks,
> > drew
> --
> 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 v4 00/12] PCI bus support

[Alexander Gordeev]

On Tue, Jun 28, 2016 at 01:32:47PM +0200, Andrew Jones wrote:
> > > Hmm.. the only way I can test the early patches without pci-testdev
> > > is to dump the data structures, but that doesn't really test anything
> > > other than the DT parsing. I'd rather we wait.
> > 
> > It also involves what would be PCI configuration space cycles: PCI bus
> > scanning and PCI devices inquiring and parsing. Not only DT.
> 
> But how do I know any of that stuff is working?

Patch 10/12:

int main(void)
{
	int ret = pci_probe();

	report("PCI bus probing", ret);

	if (ret)
		pci_print();

	return report_summary();
}

> > 
> > > Thanks,
> > > drew

Re: [kvm-unit-tests PATCH v4 00/12] PCI bus support

[Andrew Jones]

On Tue, Jun 28, 2016 at 01:56:53PM +0200, Alexander Gordeev wrote:
> On Tue, Jun 28, 2016 at 01:32:47PM +0200, Andrew Jones wrote:
> > > > Hmm.. the only way I can test the early patches without pci-testdev
> > > > is to dump the data structures, but that doesn't really test anything
> > > > other than the DT parsing. I'd rather we wait.
> > > 
> > > It also involves what would be PCI configuration space cycles: PCI bus
> > > scanning and PCI devices inquiring and parsing. Not only DT.
> > 
> > But how do I know any of that stuff is working?
> 
> Patch 10/12:
> 
> int main(void)
> {
> 	int ret = pci_probe();
> 
> 	report("PCI bus probing", ret);
> 
> 	if (ret)
> 		pci_print();
> 
> 	return report_summary();
> }

Well, if you plug some random PCI devices into kvm-unit-tests,
and do the pci_print, and then compare with an lspci in a guest
with the same devices, and get the equivalent output, then I'd
be pretty convinced, but, even then, what's the point? Without
the pci-testdev (the only pci device we're currently targeting)
then none of this series is really necessary.

Thanks,
drew

> 
> > > 
> > > > Thanks,
> > > > drew
> --
> 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 v4 00/12] PCI bus support

[Alexander Gordeev]

On Tue, Jun 28, 2016 at 02:38:12PM +0200, Andrew Jones wrote:
> On Tue, Jun 28, 2016 at 01:56:53PM +0200, Alexander Gordeev wrote:
> > On Tue, Jun 28, 2016 at 01:32:47PM +0200, Andrew Jones wrote:
> > > > > Hmm.. the only way I can test the early patches without pci-testdev
> > > > > is to dump the data structures, but that doesn't really test anything
> > > > > other than the DT parsing. I'd rather we wait.
> > > > 
> > > > It also involves what would be PCI configuration space cycles: PCI bus
> > > > scanning and PCI devices inquiring and parsing. Not only DT.
> > > 
> > > But how do I know any of that stuff is working?
> > 
> > Patch 10/12:
> > 
> > int main(void)
> > {
> > 	int ret = pci_probe();
> > 
> > 	report("PCI bus probing", ret);
> > 
> > 	if (ret)
> > 		pci_print();
> > 
> > 	return report_summary();
> > }
> 
> Well, if you plug some random PCI devices into kvm-unit-tests,
> and do the pci_print, and then compare with an lspci in a guest
> with the same devices, and get the equivalent output, then I'd
> be pretty convinced, but, even then, what's the point? Without
> the pci-testdev (the only pci device we're currently targeting)
> then none of this series is really necessary.

My point is even without pci-testdev the above is of some value,
because it only misses MMIO/PIO test and (a custom) pci-testdev
protocol - which is not a PCI test per se.

So even without pci-testdev PCI configuration space is checked,
reading/writing to devices' BARs etc. All of those is part of
QEMU PCI framwork, not pci-testdev implementation.

But surely, I am not sure how valuable this test alone is.

> Thanks,
> drew
> 
> > 
> > > > 
> > > > > Thanks,
> > > > > drew
> > --
> > 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