[kvm-unit-tests PATCH v3 0/6] pci: Complete conversion of PCI API to struct pci_dev

[kvm-unit-tests PATCH v3 0/6] pci: Complete conversion of PCI API to struct pci_dev

[Alexander Gordeev]

Hi Andrew,

The PCI BAR checks series v2 evolved to a rework of the framework
itself. Hopefully, the result will be less confusion. I do not
post an extensive sanity checks for now - let's see if the changes
would make it in.

Thanks!

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

Alexander Gordeev (6):
  pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0
  pci: Do not use 0 for unimplemented BARs in pci_dev::resource[]
  pci: Accomodate 64 bit BARs in pci_dev::resource[]
  pci: Turn struct pci_dev into device handle for PCI functions
  pci: Rework pci_bar_is_valid()
  pci: Make PCI API consistent wrt using struct pci_dev

 lib/pci-host-generic.c |  4 +--
 lib/pci.c              | 93 ++++++++++++++++++++++++++++----------------------
 lib/pci.h              |  5 ++-
 x86/intel-iommu.c      |  2 +-
 x86/vmexit.c           |  1 -
 5 files changed, 57 insertions(+), 48 deletions(-)

-- 
1.8.3.1

[kvm-unit-tests PATCH v3 1/6] pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0

[Alexander Gordeev]

This update is better be squashed into commit b8b9fcf56253b
("pci: Make all ones invalid translate address")

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-host-generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 6f2ab9e85a13..8b505b444a34 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -173,7 +173,7 @@ static bool pci_alloc_resource(struct pci_dev *dev, int bar_num, u64 *addr)
 	u64 size, pci_addr;
 	int type, i;
 
-	*addr = ~0;
+	*addr = INVALID_PHYS_ADDR;
 
 	size = pci_bar_size(dev, bar_num);
 	if (!size)
-- 
1.8.3.1

[kvm-unit-tests PATCH v3 2/6] pci: Do not use 0 for unimplemented BARs in pci_dev::resource[]

[Alexander Gordeev]

Zero could be a legitimate address inn PIO address space.
Thus, marking unimplemented BARs using zero is a bad idea.
Use INVALID_PHYS_ADDR instead.

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

diff --git a/lib/pci.c b/lib/pci.c
index 62b1c6b0b7c5..28ef5781a07a 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -336,7 +336,7 @@ void pci_scan_bars(struct pci_dev *dev)
 		dev->resource[i] = pci_bar_get_addr(dev, i);
 		if (pci_bar_is64(dev, i)) {
 			i++;
-			dev->resource[i] = (phys_addr_t)0;
+			dev->resource[i] = INVALID_PHYS_ADDR;
 		}
 	}
 }
-- 
1.8.3.1

[kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Alexander Gordeev]

Array pci_dev::resource[] is ambiguous wrt what is
actually stored in its elements and how to interpret
the index into the array.

It is simple in case a device has only 32-bit BARs -
an element pci_dev::resource[bar_num] contains the
decoded address of BAR # bar_num.

But what if a device has 64-bit BAR starting at bar_num?

Curretnly pci_dev::resource[bar_num] contains the decoded
address of the BAR, while pci_dev::resource[bar_num + 1]
contains 0. That makes meaning of (bar_num + 1) index
difficult to understand.

On the one hand it is an index of high 32-bit part of
the 64-bit address in the device itself. But why then
the element contains 0, not the high part of the address
or INVALID_PHYS_ADDRESS for example?

By placing the same 64-bit address in both bar_num and
(bar_num + 1) elements the ambiguity is less striking,
since:
  - the meaning of bar_num kept consistent with the rest
    of the functions (where it refers 32-bit BAR in terms
    of the device configuration address space);
  - pci_dev::resource[bar_num + 1] contains a valid address
    rather than vague value of 0.
  - both bar_num and (bar_num + 1) indexes refer to the
    same 64-bit BAR and therefore return the same address;
    The notion of low and high parts of a 64-bit address
    is ignored, but that is fine, since pci_dev::resource[]
    contain only full addresses;

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

diff --git a/lib/pci.c b/lib/pci.c
index 28ef5781a07a..9acc9652cb25 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -331,11 +331,14 @@ void pci_scan_bars(struct pci_dev *dev)
 	int i;
 
 	for (i = 0; i < PCI_BAR_NUM; i++) {
-		if (!pci_bar_is_valid(dev, i))
-			continue;
-		dev->resource[i] = pci_bar_get_addr(dev, i);
-		if (pci_bar_is64(dev, i)) {
-			i++;
+		if (pci_bar_size(dev, i)) {
+			dev->resource[i] = pci_bar_get_addr(dev, i);
+			if (pci_bar_is64(dev, i)) {
+				assert(i + 1 < PCI_BAR_NUM);
+				dev->resource[i + 1] = dev->resource[i];
+				i++;
+			}
+		} else {
 			dev->resource[i] = INVALID_PHYS_ADDR;
 		}
 	}
-- 
1.8.3.1

[kvm-unit-tests PATCH v3 4/6] pci: Turn struct pci_dev into device handle for PCI functions

[Alexander Gordeev]

Currently struct pci_dev is used for caching PCI device
info used by some functions. This update turns the struct
into device handle that will be used by nearly all existing
and future APIs.

As result of this change a pci_dev should be initialized
with pci_dev_init() and pci_scan_bars() becomes redundant.

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

diff --git a/lib/pci.c b/lib/pci.c
index 9acc9652cb25..aded1b1ddb77 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -90,12 +90,6 @@ bool pci_dev_exists(pcidevaddr_t dev)
 		pci_config_readw(dev, PCI_DEVICE_ID) != 0xffff);
 }
 
-void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf)
-{
-       memset(dev, 0, sizeof(*dev));
-       dev->bdf = bdf;
-}
-
 /* 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)
 {
@@ -122,7 +116,7 @@ uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
 				bar_num * 4);
 }
 
-phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
+static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num)
 {
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
@@ -138,15 +132,24 @@ phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
 	return phys_addr;
 }
 
+phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
+{
+	return dev->resource[bar_num];
+}
+
 void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
 {
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
 	pci_config_writel(dev->bdf, off, (uint32_t)addr);
 
-	if (pci_bar_is64(dev, bar_num))
-		pci_config_writel(dev->bdf, off + 4,
-				  (uint32_t)(addr >> 32));
+	dev->resource[bar_num] = addr;
+
+	if (pci_bar_is64(dev, bar_num)) {
+		assert(bar_num + 1 < PCI_BAR_NUM);
+		pci_config_writel(dev->bdf, off + 4, (uint32_t)(addr >> 32));
+		dev->resource[bar_num + 1] = dev->resource[bar_num];
+	}
 }
 
 /*
@@ -326,13 +329,16 @@ void pci_print(void)
 	}
 }
 
-void pci_scan_bars(struct pci_dev *dev)
+void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf)
 {
 	int i;
 
+	memset(dev, 0, sizeof(*dev));
+	dev->bdf = bdf;
+
 	for (i = 0; i < PCI_BAR_NUM; i++) {
 		if (pci_bar_size(dev, i)) {
-			dev->resource[i] = pci_bar_get_addr(dev, i);
+			dev->resource[i] = __pci_bar_get_addr(dev, i);
 			if (pci_bar_is64(dev, i)) {
 				assert(i + 1 < PCI_BAR_NUM);
 				dev->resource[i + 1] = dev->resource[i];
@@ -360,7 +366,6 @@ static void pci_cap_setup(struct pci_dev *dev, int cap_offset, int cap_id)
 
 void pci_enable_defaults(struct pci_dev *dev)
 {
-	pci_scan_bars(dev);
 	/* Enable device DMA operations */
 	pci_cmd_set_clr(dev, PCI_COMMAND_MASTER, 0);
 	pci_cap_walk(dev, pci_cap_setup);
diff --git a/lib/pci.h b/lib/pci.h
index 3da3ccc8c791..fefd9a84b307 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -28,7 +28,6 @@ struct pci_dev {
 };
 
 extern void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf);
-extern void pci_scan_bars(struct pci_dev *dev);
 extern void pci_cmd_set_clr(struct pci_dev *dev, uint16_t set, uint16_t clr);
 typedef void (*pci_cap_handler_t)(struct pci_dev *dev, int cap_offset, int cap_id);
 extern void pci_cap_walk(struct pci_dev *dev, pci_cap_handler_t handler);
diff --git a/x86/vmexit.c b/x86/vmexit.c
index 71f4d156b3ee..5b821b5eb125 100644
--- a/x86/vmexit.c
+++ b/x86/vmexit.c
@@ -519,7 +519,6 @@ int main(int ac, char **av)
 	ret = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
 	if (ret != PCIDEVADDR_INVALID) {
 		pci_dev_init(&pcidev, ret);
-		pci_scan_bars(&pcidev);
 		assert(pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_MEM));
 		assert(!pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_IO));
 		membar = pcidev.resource[PCI_TESTDEV_BAR_MEM];
-- 
1.8.3.1

[kvm-unit-tests PATCH v3 5/6] pci: Rework pci_bar_is_valid()

[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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index aded1b1ddb77..d9e3dfaa3726 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -206,7 +206,7 @@ bool pci_bar_is_memory(struct pci_dev *dev, int bar_num)
 
 bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
 {
-	return pci_bar_get(dev, bar_num);
+	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
 }
 
 bool pci_bar_is64(struct pci_dev *dev, int bar_num)
@@ -225,11 +225,11 @@ void pci_bar_print(struct pci_dev *dev, int bar_num)
 	phys_addr_t size, start, end;
 	uint32_t bar;
 
-	size = pci_bar_size(dev, bar_num);
-	if (!size)
+	if (!pci_bar_is_valid(dev, bar_num))
 		return;
 
 	bar = pci_bar_get(dev, bar_num);
+	size = pci_bar_size(dev, bar_num);
 	start = pci_bar_get_addr(dev, bar_num);
 	end = start + size - 1;
 
@@ -309,7 +309,7 @@ void pci_dev_print(pcidevaddr_t dev)
 		return;
 
 	for (i = 0; i < PCI_BAR_NUM; i++) {
-		if (pci_bar_size(&pci_dev, i)) {
+		if (pci_bar_is_valid(&pci_dev, i)) {
 			printf("\t");
 			pci_bar_print(&pci_dev, i);
 			printf("\n");
-- 
1.8.3.1

[kvm-unit-tests PATCH v3 6/6] pci: Make PCI API consistent wrt using struct pci_dev

[Alexander Gordeev]

Complete conversion of PCI API so all functions
that imply the underlying device does exist would
use struct pci_dev as a handle, not pcidevaddr_t.

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 |  2 +-
 lib/pci.c              | 43 +++++++++++++++++++++++--------------------
 lib/pci.h              |  4 ++--
 x86/intel-iommu.c      |  2 +-
 4 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 8b505b444a34..818150dc0a66 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -192,7 +192,7 @@ static bool pci_alloc_resource(struct pci_dev *dev, int bar_num, u64 *addr)
 
 	if (i >= host->nr_addr_spaces) {
 		printf("%s: warning: can't satisfy request for ", __func__);
-		pci_dev_print_id(dev->bdf);
+		pci_dev_print_id(dev);
 		printf(" ");
 		pci_bar_print(dev, bar_num);
 		printf("\n");
diff --git a/lib/pci.c b/lib/pci.c
index d9e3dfaa3726..cf9dc3f2792f 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -266,11 +266,13 @@ void pci_bar_print(struct pci_dev *dev, int bar_num)
 	printf("]");
 }
 
-void pci_dev_print_id(pcidevaddr_t dev)
+void pci_dev_print_id(struct pci_dev *dev)
 {
-	printf("00.%02x.%1x %04x:%04x", dev / 8, dev % 8,
-		pci_config_readw(dev, PCI_VENDOR_ID),
-		pci_config_readw(dev, PCI_DEVICE_ID));
+	pcidevaddr_t bdf = dev->bdf;
+
+	printf("00.%02x.%1x %04x:%04x", bdf / 8, bdf % 8,
+		pci_config_readw(bdf, PCI_VENDOR_ID),
+		pci_config_readw(bdf, PCI_DEVICE_ID));
 }
 
 static void pci_cap_print(struct pci_dev *dev, int cap_offset, int cap_id)
@@ -288,44 +290,45 @@ static void pci_cap_print(struct pci_dev *dev, int cap_offset, int cap_id)
 	printf("at offset 0x%02x\n", cap_offset);
 }
 
-void pci_dev_print(pcidevaddr_t dev)
+void pci_dev_print(struct pci_dev *dev)
 {
-	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);
-	struct pci_dev pci_dev;
+	pcidevaddr_t bdf = dev->bdf;
+	uint8_t header = pci_config_readb(bdf, PCI_HEADER_TYPE);
+	uint8_t progif = pci_config_readb(bdf, PCI_CLASS_PROG);
+	uint8_t subclass = pci_config_readb(bdf, PCI_CLASS_DEVICE);
+	uint8_t class = pci_config_readb(bdf, PCI_CLASS_DEVICE + 1);
 	int i;
 
-	pci_dev_init(&pci_dev, dev);
-
 	pci_dev_print_id(dev);
 	printf(" type %02x progif %02x class %02x subclass %02x\n",
 	       header, progif, class, subclass);
 
-	pci_cap_walk(&pci_dev, pci_cap_print);
+	pci_cap_walk(dev, pci_cap_print);
 
 	if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
 		return;
 
 	for (i = 0; i < PCI_BAR_NUM; i++) {
-		if (pci_bar_is_valid(&pci_dev, i)) {
+		if (pci_bar_is_valid(dev, i)) {
 			printf("\t");
-			pci_bar_print(&pci_dev, i);
+			pci_bar_print(dev, i);
 			printf("\n");
 		}
-		if (pci_bar_is64(&pci_dev, i))
+		if (pci_bar_is64(dev, i))
 			i++;
 	}
 }
 
 void pci_print(void)
 {
-	pcidevaddr_t dev;
+	pcidevaddr_t devfn;
+	struct pci_dev pci_dev;
 
-	for (dev = 0; dev < PCI_DEVFN_MAX; ++dev) {
-		if (pci_dev_exists(dev))
-			pci_dev_print(dev);
+	for (devfn = 0; devfn < PCI_DEVFN_MAX; ++devfn) {
+		if (pci_dev_exists(devfn)) {
+			pci_dev_init(&pci_dev, devfn);
+			pci_dev_print(&pci_dev);
+		}
 	}
 }
 
diff --git a/lib/pci.h b/lib/pci.h
index fefd9a84b307..03cc0a72d48d 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -63,8 +63,8 @@ extern bool pci_bar_is64(struct pci_dev *dev, int bar_num);
 extern bool pci_bar_is_memory(struct pci_dev *dev, int bar_num);
 extern bool pci_bar_is_valid(struct pci_dev *dev, int bar_num);
 extern void pci_bar_print(struct pci_dev *dev, int bar_num);
-extern void pci_dev_print_id(pcidevaddr_t dev);
-extern void pci_dev_print(pcidevaddr_t dev);
+extern void pci_dev_print_id(struct pci_dev *dev);
+extern void pci_dev_print(struct pci_dev *dev);
 extern uint8_t pci_intx_line(struct pci_dev *dev);
 void pci_msi_set_enable(struct pci_dev *dev, bool enabled);
 
diff --git a/x86/intel-iommu.c b/x86/intel-iommu.c
index a01b23e674a8..610cc655c41b 100644
--- a/x86/intel-iommu.c
+++ b/x86/intel-iommu.c
@@ -154,7 +154,7 @@ int main(int argc, char *argv[])
 		report_skip(VTD_TEST_IR_MSI);
 	} else {
 		printf("Found EDU device:\n");
-		pci_dev_print(edu_dev.pci_dev.bdf);
+		pci_dev_print(&edu_dev.pci_dev);
 		vtd_test_dmar();
 		vtd_test_ir();
 	}
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v3 4/6] pci: Turn struct pci_dev into device handle for PCI functions

[Andrew Jones]

On Mon, Feb 27, 2017 at 03:12:35PM +0100, Alexander Gordeev wrote:
> Currently struct pci_dev is used for caching PCI device
> info used by some functions. This update turns the struct
> into device handle that will be used by nearly all existing
> and future APIs.
> 
> As result of this change a pci_dev should be initialized
> with pci_dev_init() and pci_scan_bars() becomes redundant.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c    | 31 ++++++++++++++++++-------------
>  lib/pci.h    |  1 -
>  x86/vmexit.c |  1 -
>  3 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 9acc9652cb25..aded1b1ddb77 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -90,12 +90,6 @@ bool pci_dev_exists(pcidevaddr_t dev)
>  		pci_config_readw(dev, PCI_DEVICE_ID) != 0xffff);
>  }
>  
> -void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf)
> -{
> -       memset(dev, 0, sizeof(*dev));
> -       dev->bdf = bdf;
> -}
> -
>  /* 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)
>  {
> @@ -122,7 +116,7 @@ uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
>  				bar_num * 4);
>  }
>  
> -phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
> +static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num)
>  {
>  	uint32_t bar = pci_bar_get(dev, bar_num);
>  	uint32_t mask = pci_bar_mask(bar);
> @@ -138,15 +132,24 @@ phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
>  	return phys_addr;
>  }
>  
> +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
> +{
> +	return dev->resource[bar_num];
> +}
> +
>  void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
>  {
>  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
>  
>  	pci_config_writel(dev->bdf, off, (uint32_t)addr);
>

nit: I'd remove the above blank line too (not worth respinning for though)  

> -	if (pci_bar_is64(dev, bar_num))
> -		pci_config_writel(dev->bdf, off + 4,
> -				  (uint32_t)(addr >> 32));
> +	dev->resource[bar_num] = addr;
> +
> +	if (pci_bar_is64(dev, bar_num)) {
> +		assert(bar_num + 1 < PCI_BAR_NUM);
> +		pci_config_writel(dev->bdf, off + 4, (uint32_t)(addr >> 32));
> +		dev->resource[bar_num + 1] = dev->resource[bar_num];
> +	}
>  }
>  
>  /*
> @@ -326,13 +329,16 @@ void pci_print(void)
>  	}
>  }
>  
> -void pci_scan_bars(struct pci_dev *dev)
> +void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf)
>  {
>  	int i;
>  
> +	memset(dev, 0, sizeof(*dev));
> +	dev->bdf = bdf;
> +
>  	for (i = 0; i < PCI_BAR_NUM; i++) {
>  		if (pci_bar_size(dev, i)) {
> -			dev->resource[i] = pci_bar_get_addr(dev, i);
> +			dev->resource[i] = __pci_bar_get_addr(dev, i);
>  			if (pci_bar_is64(dev, i)) {
>  				assert(i + 1 < PCI_BAR_NUM);
>  				dev->resource[i + 1] = dev->resource[i];
> @@ -360,7 +366,6 @@ static void pci_cap_setup(struct pci_dev *dev, int cap_offset, int cap_id)
>  
>  void pci_enable_defaults(struct pci_dev *dev)
>  {
> -	pci_scan_bars(dev);
>  	/* Enable device DMA operations */
>  	pci_cmd_set_clr(dev, PCI_COMMAND_MASTER, 0);
>  	pci_cap_walk(dev, pci_cap_setup);
> diff --git a/lib/pci.h b/lib/pci.h
> index 3da3ccc8c791..fefd9a84b307 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -28,7 +28,6 @@ struct pci_dev {
>  };
>  
>  extern void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf);
> -extern void pci_scan_bars(struct pci_dev *dev);
>  extern void pci_cmd_set_clr(struct pci_dev *dev, uint16_t set, uint16_t clr);
>  typedef void (*pci_cap_handler_t)(struct pci_dev *dev, int cap_offset, int cap_id);
>  extern void pci_cap_walk(struct pci_dev *dev, pci_cap_handler_t handler);
> diff --git a/x86/vmexit.c b/x86/vmexit.c
> index 71f4d156b3ee..5b821b5eb125 100644
> --- a/x86/vmexit.c
> +++ b/x86/vmexit.c
> @@ -519,7 +519,6 @@ int main(int ac, char **av)
>  	ret = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
>  	if (ret != PCIDEVADDR_INVALID) {
>  		pci_dev_init(&pcidev, ret);
> -		pci_scan_bars(&pcidev);
>  		assert(pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_MEM));
>  		assert(!pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_IO));
>  		membar = pcidev.resource[PCI_TESTDEV_BAR_MEM];
> -- 
> 1.8.3.1
> 

Re: [kvm-unit-tests PATCH v3 0/6] pci: Complete conversion of PCI API to struct pci_dev

[Andrew Jones]

On Mon, Feb 27, 2017 at 03:12:31PM +0100, Alexander Gordeev wrote:
> Hi Andrew,
> 
> The PCI BAR checks series v2 evolved to a rework of the framework
> itself. Hopefully, the result will be less confusion. I do not
> post an extensive sanity checks for now - let's see if the changes
> would make it in.
> 
> Thanks!
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> 
> Alexander Gordeev (6):
>   pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0
>   pci: Do not use 0 for unimplemented BARs in pci_dev::resource[]
>   pci: Accomodate 64 bit BARs in pci_dev::resource[]
>   pci: Turn struct pci_dev into device handle for PCI functions
>   pci: Rework pci_bar_is_valid()
>   pci: Make PCI API consistent wrt using struct pci_dev
> 
>  lib/pci-host-generic.c |  4 +--
>  lib/pci.c              | 93 ++++++++++++++++++++++++++++----------------------
>  lib/pci.h              |  5 ++-
>  x86/intel-iommu.c      |  2 +-
>  x86/vmexit.c           |  1 -
>  5 files changed, 57 insertions(+), 48 deletions(-)
> 
> -- 
> 1.8.3.1
>

For the series

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

Peter Xu should also take a look though (CC'ed)

Thanks,
drew

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Peter Xu]

On Mon, Feb 27, 2017 at 03:12:34PM +0100, Alexander Gordeev wrote:
> Array pci_dev::resource[] is ambiguous wrt what is
> actually stored in its elements and how to interpret
> the index into the array.
> 
> It is simple in case a device has only 32-bit BARs -
> an element pci_dev::resource[bar_num] contains the
> decoded address of BAR # bar_num.
> 
> But what if a device has 64-bit BAR starting at bar_num?
> 
> Curretnly pci_dev::resource[bar_num] contains the decoded
> address of the BAR, while pci_dev::resource[bar_num + 1]
> contains 0. That makes meaning of (bar_num + 1) index
> difficult to understand.
> 
> On the one hand it is an index of high 32-bit part of
> the 64-bit address in the device itself. But why then
> the element contains 0, not the high part of the address
> or INVALID_PHYS_ADDRESS for example?
> 
> By placing the same 64-bit address in both bar_num and
> (bar_num + 1) elements the ambiguity is less striking,
> since:
>   - the meaning of bar_num kept consistent with the rest
>     of the functions (where it refers 32-bit BAR in terms
>     of the device configuration address space);
>   - pci_dev::resource[bar_num + 1] contains a valid address
>     rather than vague value of 0.
>   - both bar_num and (bar_num + 1) indexes refer to the
>     same 64-bit BAR and therefore return the same address;
>     The notion of low and high parts of a 64-bit address
>     is ignored, but that is fine, since pci_dev::resource[]
>     contain only full addresses;

IIUC for a general PCI device driver, it should know which bars are
used for specific device, the type of the bar (whether it would be
64bit), for what purpose, etc.. Then, the driver should just avoid
touching the other bar (in our case, bar_num+1). So here I don't quite
catch why we need to explicitly have res[bar_num+1] contain the same
content as res[bar_num]. Do we really have a use case?

And, looks like this patch has just overwritten previous patch (patch
2). IMHO I would slightly prefer that one since INVALID_PHYS_ADDR at
least showed that we fetched something wrong.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Andrew Jones]

On Tue, Feb 28, 2017 at 03:02:25PM +0800, Peter Xu wrote:
> On Mon, Feb 27, 2017 at 03:12:34PM +0100, Alexander Gordeev wrote:
> > Array pci_dev::resource[] is ambiguous wrt what is
> > actually stored in its elements and how to interpret
> > the index into the array.
> > 
> > It is simple in case a device has only 32-bit BARs -
> > an element pci_dev::resource[bar_num] contains the
> > decoded address of BAR # bar_num.
> > 
> > But what if a device has 64-bit BAR starting at bar_num?
> > 
> > Curretnly pci_dev::resource[bar_num] contains the decoded
> > address of the BAR, while pci_dev::resource[bar_num + 1]
> > contains 0. That makes meaning of (bar_num + 1) index
> > difficult to understand.
> > 
> > On the one hand it is an index of high 32-bit part of
> > the 64-bit address in the device itself. But why then
> > the element contains 0, not the high part of the address
> > or INVALID_PHYS_ADDRESS for example?
> > 
> > By placing the same 64-bit address in both bar_num and
> > (bar_num + 1) elements the ambiguity is less striking,
> > since:
> >   - the meaning of bar_num kept consistent with the rest
> >     of the functions (where it refers 32-bit BAR in terms
> >     of the device configuration address space);
> >   - pci_dev::resource[bar_num + 1] contains a valid address
> >     rather than vague value of 0.
> >   - both bar_num and (bar_num + 1) indexes refer to the
> >     same 64-bit BAR and therefore return the same address;
> >     The notion of low and high parts of a 64-bit address
> >     is ignored, but that is fine, since pci_dev::resource[]
> >     contain only full addresses;
> 
> IIUC for a general PCI device driver, it should know which bars are
> used for specific device, the type of the bar (whether it would be
> 64bit), for what purpose, etc.. Then, the driver should just avoid
> touching the other bar (in our case, bar_num+1). So here I don't quite
> catch why we need to explicitly have res[bar_num+1] contain the same
> content as res[bar_num]. Do we really have a use case?

kvm-unit-test drivers can be unit tests. I prefer that common code
provide asserts allowing unit test writers to quickly determine
when their test has been incorrectly written, rather than forcing
them to read the common code in order to debug the issue. Currently
it's not possible to ensure a driver (unit test) does not attempt
to use invalid bars or 64bit bars in invalid ways. This series
prepares for some asserts to get sprinkled into the common code
and/or offer a better "is valid" API.

> 
> And, looks like this patch has just overwritten previous patch (patch
> 2). IMHO I would slightly prefer that one since INVALID_PHYS_ADDR at
> least showed that we fetched something wrong.

Not really. The INVALID_PHYS_ADDR is still assigned to unused bars
in the else {} below.

Thanks,
drew

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Peter Xu]

On Tue, Feb 28, 2017 at 10:02:53AM +0100, Andrew Jones wrote:
> On Tue, Feb 28, 2017 at 03:02:25PM +0800, Peter Xu wrote:
> > On Mon, Feb 27, 2017 at 03:12:34PM +0100, Alexander Gordeev wrote:
> > > Array pci_dev::resource[] is ambiguous wrt what is
> > > actually stored in its elements and how to interpret
> > > the index into the array.
> > > 
> > > It is simple in case a device has only 32-bit BARs -
> > > an element pci_dev::resource[bar_num] contains the
> > > decoded address of BAR # bar_num.
> > > 
> > > But what if a device has 64-bit BAR starting at bar_num?
> > > 
> > > Curretnly pci_dev::resource[bar_num] contains the decoded
> > > address of the BAR, while pci_dev::resource[bar_num + 1]
> > > contains 0. That makes meaning of (bar_num + 1) index
> > > difficult to understand.
> > > 
> > > On the one hand it is an index of high 32-bit part of
> > > the 64-bit address in the device itself. But why then
> > > the element contains 0, not the high part of the address
> > > or INVALID_PHYS_ADDRESS for example?
> > > 
> > > By placing the same 64-bit address in both bar_num and
> > > (bar_num + 1) elements the ambiguity is less striking,
> > > since:
> > >   - the meaning of bar_num kept consistent with the rest
> > >     of the functions (where it refers 32-bit BAR in terms
> > >     of the device configuration address space);
> > >   - pci_dev::resource[bar_num + 1] contains a valid address
> > >     rather than vague value of 0.
> > >   - both bar_num and (bar_num + 1) indexes refer to the
> > >     same 64-bit BAR and therefore return the same address;
> > >     The notion of low and high parts of a 64-bit address
> > >     is ignored, but that is fine, since pci_dev::resource[]
> > >     contain only full addresses;
> > 
> > IIUC for a general PCI device driver, it should know which bars are
> > used for specific device, the type of the bar (whether it would be
> > 64bit), for what purpose, etc.. Then, the driver should just avoid
> > touching the other bar (in our case, bar_num+1). So here I don't quite
> > catch why we need to explicitly have res[bar_num+1] contain the same
> > content as res[bar_num]. Do we really have a use case?
> 
> kvm-unit-test drivers can be unit tests. I prefer that common code
> provide asserts allowing unit test writers to quickly determine
> when their test has been incorrectly written, rather than forcing
> them to read the common code in order to debug the issue. Currently
> it's not possible to ensure a driver (unit test) does not attempt
> to use invalid bars or 64bit bars in invalid ways. This series
> prepares for some asserts to get sprinkled into the common code
> and/or offer a better "is valid" API.

Yeah. I agree that for unit tests we should just crash the test when
something wrong happened. But would this patch help this purpose? Both
res[num] and res[num+1] are written with the same address, then how
would the test writter know that "fetching address from num+1 is
actually wrong"?

> 
> > 
> > And, looks like this patch has just overwritten previous patch (patch
> > 2). IMHO I would slightly prefer that one since INVALID_PHYS_ADDR at
> > least showed that we fetched something wrong.
> 
> Not really. The INVALID_PHYS_ADDR is still assigned to unused bars
> in the else {} below.

Ah yes. Sorry I didn't notice that. Then I'll prefer touching up
subject of patch 2 from:

 "pci: Do not use 0 for unimplemented BARs in pci_dev::resource[]"

into:

 "pci: Do not use 0 for high dword of 64bit BARs"

 (or something similar)

since actually before this patch we just skipped unimplemented bars,
while this patch set them up with INVALID_PHYS_ADDR.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Andrew Jones]

On Tue, Feb 28, 2017 at 05:31:29PM +0800, Peter Xu wrote:
> On Tue, Feb 28, 2017 at 10:02:53AM +0100, Andrew Jones wrote:
> > On Tue, Feb 28, 2017 at 03:02:25PM +0800, Peter Xu wrote:
> > > On Mon, Feb 27, 2017 at 03:12:34PM +0100, Alexander Gordeev wrote:
> > > > Array pci_dev::resource[] is ambiguous wrt what is
> > > > actually stored in its elements and how to interpret
> > > > the index into the array.
> > > > 
> > > > It is simple in case a device has only 32-bit BARs -
> > > > an element pci_dev::resource[bar_num] contains the
> > > > decoded address of BAR # bar_num.
> > > > 
> > > > But what if a device has 64-bit BAR starting at bar_num?
> > > > 
> > > > Curretnly pci_dev::resource[bar_num] contains the decoded
> > > > address of the BAR, while pci_dev::resource[bar_num + 1]
> > > > contains 0. That makes meaning of (bar_num + 1) index
> > > > difficult to understand.
> > > > 
> > > > On the one hand it is an index of high 32-bit part of
> > > > the 64-bit address in the device itself. But why then
> > > > the element contains 0, not the high part of the address
> > > > or INVALID_PHYS_ADDRESS for example?
> > > > 
> > > > By placing the same 64-bit address in both bar_num and
> > > > (bar_num + 1) elements the ambiguity is less striking,
> > > > since:
> > > >   - the meaning of bar_num kept consistent with the rest
> > > >     of the functions (where it refers 32-bit BAR in terms
> > > >     of the device configuration address space);
> > > >   - pci_dev::resource[bar_num + 1] contains a valid address
> > > >     rather than vague value of 0.
> > > >   - both bar_num and (bar_num + 1) indexes refer to the
> > > >     same 64-bit BAR and therefore return the same address;
> > > >     The notion of low and high parts of a 64-bit address
> > > >     is ignored, but that is fine, since pci_dev::resource[]
> > > >     contain only full addresses;
> > > 
> > > IIUC for a general PCI device driver, it should know which bars are
> > > used for specific device, the type of the bar (whether it would be
> > > 64bit), for what purpose, etc.. Then, the driver should just avoid
> > > touching the other bar (in our case, bar_num+1). So here I don't quite
> > > catch why we need to explicitly have res[bar_num+1] contain the same
> > > content as res[bar_num]. Do we really have a use case?
> > 
> > kvm-unit-test drivers can be unit tests. I prefer that common code
> > provide asserts allowing unit test writers to quickly determine
> > when their test has been incorrectly written, rather than forcing
> > them to read the common code in order to debug the issue. Currently
> > it's not possible to ensure a driver (unit test) does not attempt
> > to use invalid bars or 64bit bars in invalid ways. This series
> > prepares for some asserts to get sprinkled into the common code
> > and/or offer a better "is valid" API.
> 
> Yeah. I agree that for unit tests we should just crash the test when
> something wrong happened. But would this patch help this purpose? Both
> res[num] and res[num+1] are written with the same address, then how
> would the test writter know that "fetching address from num+1 is
> actually wrong"?

I don't know if fetching an address from a high bar is wrong or not.
How is it specified? Is it possible to tell the difference between
an unused bar and a high bar when checking for a valid bar? I like
Alex's approach, but if it's in violation of the spec, then I agree
it needs to be changed. I defer to you PCI people to sort it out :-)

Thanks,
drew

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Alexander Gordeev]

On Tue, Feb 28, 2017 at 03:02:25PM +0800, Peter Xu wrote:
> On Mon, Feb 27, 2017 at 03:12:34PM +0100, Alexander Gordeev wrote:
> > Array pci_dev::resource[] is ambiguous wrt what is
> > actually stored in its elements and how to interpret
> > the index into the array.
> > 
> > It is simple in case a device has only 32-bit BARs -
> > an element pci_dev::resource[bar_num] contains the
> > decoded address of BAR # bar_num.
> > 
> > But what if a device has 64-bit BAR starting at bar_num?
> > 
> > Curretnly pci_dev::resource[bar_num] contains the decoded
> > address of the BAR, while pci_dev::resource[bar_num + 1]
> > contains 0. That makes meaning of (bar_num + 1) index
> > difficult to understand.
> > 
> > On the one hand it is an index of high 32-bit part of
> > the 64-bit address in the device itself. But why then
> > the element contains 0, not the high part of the address
> > or INVALID_PHYS_ADDRESS for example?
> > 
> > By placing the same 64-bit address in both bar_num and
> > (bar_num + 1) elements the ambiguity is less striking,
> > since:
> >   - the meaning of bar_num kept consistent with the rest
> >     of the functions (where it refers 32-bit BAR in terms
> >     of the device configuration address space);
> >   - pci_dev::resource[bar_num + 1] contains a valid address
> >     rather than vague value of 0.
> >   - both bar_num and (bar_num + 1) indexes refer to the
> >     same 64-bit BAR and therefore return the same address;
> >     The notion of low and high parts of a 64-bit address
> >     is ignored, but that is fine, since pci_dev::resource[]
> >     contain only full addresses;
> 
> IIUC for a general PCI device driver, it should know which bars are
> used for specific device, the type of the bar (whether it would be
> 64bit), for what purpose, etc.. Then, the driver should just avoid
> touching the other bar (in our case, bar_num+1). So here I don't quite
> catch why we need to explicitly have res[bar_num+1] contain the same
> content as res[bar_num]. Do we really have a use case?

Yes - see how pci_bar_is_valid() is implemented:

bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
{
	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
}

On the one hand a testcase should not address a 64-bit BAR using
high part BAR number. Particularly, when it tries to obtan BAR
size or address. On the other hand a testcase should be able to
access a low and high parts separately if it needs to for whatever
reason. The rest of the API allows that as well.

But we do not have any checks wrt high/low parts right now nor
this series introduces them yet. It is really about data design.

pci_dev::resource[] contains decoded 32/64-bit BAR addresses,
not low/high parts of underlying 32-bit device BARs. Yet,
indexes into this array correspond to raw 32-bit BARs in the
PCI device configuration space. Thus, a question arises -
what should be stored in array elements that correspond to
high-parts of 64-bit BARs? Zero is particularly bad choice,
because:
  - it is a valid address in PIO address space, so it can not
    stand for "no value" or NULL or whatever marker could be
    used to indicate a high part;
  - the high part of underlying 64-bit address is (could be)
    non-zero. So there is inconsistency also;

So both implementation and data-desine wise the best solution
I see for now is simply store the same 64-bit address for both
indexes of a 64-bit BAR.

> And, looks like this patch has just overwritten previous patch (patch
> 2). IMHO I would slightly prefer that one since INVALID_PHYS_ADDR at
> least showed that we fetched something wrong.
> 
> Thanks,
> 
> -- peterx

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Peter Xu]

On Tue, Feb 28, 2017 at 10:51:52AM +0100, Alexander Gordeev wrote:
> On Tue, Feb 28, 2017 at 03:02:25PM +0800, Peter Xu wrote:
> > On Mon, Feb 27, 2017 at 03:12:34PM +0100, Alexander Gordeev wrote:
> > > Array pci_dev::resource[] is ambiguous wrt what is
> > > actually stored in its elements and how to interpret
> > > the index into the array.
> > > 
> > > It is simple in case a device has only 32-bit BARs -
> > > an element pci_dev::resource[bar_num] contains the
> > > decoded address of BAR # bar_num.
> > > 
> > > But what if a device has 64-bit BAR starting at bar_num?
> > > 
> > > Curretnly pci_dev::resource[bar_num] contains the decoded
> > > address of the BAR, while pci_dev::resource[bar_num + 1]
> > > contains 0. That makes meaning of (bar_num + 1) index
> > > difficult to understand.
> > > 
> > > On the one hand it is an index of high 32-bit part of
> > > the 64-bit address in the device itself. But why then
> > > the element contains 0, not the high part of the address
> > > or INVALID_PHYS_ADDRESS for example?
> > > 
> > > By placing the same 64-bit address in both bar_num and
> > > (bar_num + 1) elements the ambiguity is less striking,
> > > since:
> > >   - the meaning of bar_num kept consistent with the rest
> > >     of the functions (where it refers 32-bit BAR in terms
> > >     of the device configuration address space);
> > >   - pci_dev::resource[bar_num + 1] contains a valid address
> > >     rather than vague value of 0.
> > >   - both bar_num and (bar_num + 1) indexes refer to the
> > >     same 64-bit BAR and therefore return the same address;
> > >     The notion of low and high parts of a 64-bit address
> > >     is ignored, but that is fine, since pci_dev::resource[]
> > >     contain only full addresses;
> > 
> > IIUC for a general PCI device driver, it should know which bars are
> > used for specific device, the type of the bar (whether it would be
> > 64bit), for what purpose, etc.. Then, the driver should just avoid
> > touching the other bar (in our case, bar_num+1). So here I don't quite
> > catch why we need to explicitly have res[bar_num+1] contain the same
> > content as res[bar_num]. Do we really have a use case?
> 
> Yes - see how pci_bar_is_valid() is implemented:
> 
> bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> {
> 	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
> }
> 
> On the one hand a testcase should not address a 64-bit BAR using
> high part BAR number. Particularly, when it tries to obtan BAR
> size or address. On the other hand a testcase should be able to
> access a low and high parts separately if it needs to for whatever
> reason. The rest of the API allows that as well.
> 
> But we do not have any checks wrt high/low parts right now nor
> this series introduces them yet. It is really about data design.
> 
> pci_dev::resource[] contains decoded 32/64-bit BAR addresses,
> not low/high parts of underlying 32-bit device BARs. Yet,
> indexes into this array correspond to raw 32-bit BARs in the
> PCI device configuration space. Thus, a question arises -
> what should be stored in array elements that correspond to
> high-parts of 64-bit BARs? Zero is particularly bad choice,
> because:
>   - it is a valid address in PIO address space, so it can not
>     stand for "no value" or NULL or whatever marker could be
>     used to indicate a high part;
>   - the high part of underlying 64-bit address is (could be)
>     non-zero. So there is inconsistency also;
> 
> So both implementation and data-desine wise the best solution
> I see for now is simply store the same 64-bit address for both
> indexes of a 64-bit BAR.

I see. Could I ask why we cannot just use INVALID_PHYS_ADDR for
res[bar_num+1] when bar_num is 64bit (just like what patch 2 did)?

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Alexander Gordeev]

On Tue, Feb 28, 2017 at 05:55:01PM +0800, Peter Xu wrote:
> > bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> > {
> > 	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
> > }

(*)

> I see. Could I ask why we cannot just use INVALID_PHYS_ADDR for
> res[bar_num+1] when bar_num is 64bit (just like what patch 2 did)?

INVALID_PHYS_ADDR is already used to mark unimplemented BARs (*).

INVALID_PHYS_ADDR is also confusing if we think of index into
high-part of 64-bit address. A high-part is apparently valid.

> Thanks,
> 
> -- peterx

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Peter Xu]

On Tue, Feb 28, 2017 at 11:22:14AM +0100, Alexander Gordeev wrote:
> On Tue, Feb 28, 2017 at 05:55:01PM +0800, Peter Xu wrote:
> > > bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> > > {
> > > 	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
> > > }
> 
> (*)
> 
> > I see. Could I ask why we cannot just use INVALID_PHYS_ADDR for
> > res[bar_num+1] when bar_num is 64bit (just like what patch 2 did)?
> 
> INVALID_PHYS_ADDR is already used to mark unimplemented BARs (*).
> 
> INVALID_PHYS_ADDR is also confusing if we think of index into
> high-part of 64-bit address. A high-part is apparently valid.

Okay. Then both work for me. Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Peter Xu]

On Tue, Feb 28, 2017 at 11:22:14AM +0100, Alexander Gordeev wrote:
> On Tue, Feb 28, 2017 at 05:55:01PM +0800, Peter Xu wrote:

[...]

> INVALID_PHYS_ADDR is also confusing if we think of index into
> high-part of 64-bit address. A high-part is apparently valid.

Btw, if this stands, then maybe we can consider drop patch 2, since
that patch did exactly the thing mentioned here... Thanks,

-- peterx

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Alexander Gordeev]

On Tue, Feb 28, 2017 at 06:41:50PM +0800, Peter Xu wrote:
> On Tue, Feb 28, 2017 at 11:22:14AM +0100, Alexander Gordeev wrote:
> > On Tue, Feb 28, 2017 at 05:55:01PM +0800, Peter Xu wrote:
> 
> [...]
> 
> > INVALID_PHYS_ADDR is also confusing if we think of index into
> > high-part of 64-bit address. A high-part is apparently valid.
> 
> Btw, if this stands, then maybe we can consider drop patch 2, since
> that patch did exactly the thing mentioned here... Thanks,

Hmm.. that sounds about right. Andrew?

> -- peterx

Re: [kvm-unit-tests PATCH v3 3/6] pci: Accomodate 64 bit BARs in pci_dev::resource[]

[Andrew Jones]

On Tue, Feb 28, 2017 at 12:11:57PM +0100, Alexander Gordeev wrote:
> On Tue, Feb 28, 2017 at 06:41:50PM +0800, Peter Xu wrote:
> > On Tue, Feb 28, 2017 at 11:22:14AM +0100, Alexander Gordeev wrote:
> > > On Tue, Feb 28, 2017 at 05:55:01PM +0800, Peter Xu wrote:
> > 
> > [...]
> > 
> > > INVALID_PHYS_ADDR is also confusing if we think of index into
> > > high-part of 64-bit address. A high-part is apparently valid.
> > 
> > Btw, if this stands, then maybe we can consider drop patch 2, since
> > that patch did exactly the thing mentioned here... Thanks,
> 
> Hmm.. that sounds about right. Andrew?

yup

> 
> > -- peterx