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

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

[Alexander Gordeev]

Changes since v3:
  - patches 2/6 and 3/6 merged; I amended the commit message, but kept the R-b;
  - an empty line in patch 4/6 removed;

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

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

Alexander Gordeev (5):
  pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0
  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              | 92 ++++++++++++++++++++++++++++----------------------
 lib/pci.h              |  5 ++-
 x86/intel-iommu.c      |  2 +-
 x86/vmexit.c           |  1 -
 5 files changed, 56 insertions(+), 48 deletions(-)

-- 
1.8.3.1

[kvm-unit-tests PATCH v4 1/5] 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>
Cc: Peter Xu <peterx@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 v4 2/5] 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 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.

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;

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>
Cc: Peter Xu <peterx@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index 62b1c6b0b7c5..9acc9652cb25 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -331,12 +331,15 @@ 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++;
-			dev->resource[i] = (phys_addr_t)0;
+		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 v4 3/5] 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>
Cc: Peter Xu <peterx@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c    | 30 +++++++++++++++++-------------
 lib/pci.h    |  1 -
 x86/vmexit.c |  1 -
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index 9acc9652cb25..cf33b894759d 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,23 @@ 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);
+	dev->resource[bar_num] = addr;
 
-	if (pci_bar_is64(dev, bar_num))
-		pci_config_writel(dev->bdf, off + 4,
-				  (uint32_t)(addr >> 32));
+	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 +328,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 +365,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 v4 4/5] pci: Rework pci_bar_is_valid()

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Reviewed-by: 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 cf33b894759d..fc18b254366c 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -205,7 +205,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)
@@ -224,11 +224,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;
 
@@ -308,7 +308,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 v4 5/5] 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>
Cc: Peter Xu <peterx@redhat.com>
Reviewed-by: 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 fc18b254366c..daf398100b7e 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -265,11 +265,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)
@@ -287,44 +289,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 v4 0/5] pci: Complete conversion of PCI API to struct pci_dev

[Radim Krčmář]

2017-02-28 19:08+0100, Alexander Gordeev:
> Changes since v3:
>   - patches 2/6 and 3/6 merged; I amended the commit message, but kept the R-b;
>   - an empty line in patch 4/6 removed;

Applied, thanks.

> Sources are avalable at:
> https://github.com/a-gordeev/kvm-unit-tests.git pci-fixes-v4
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> 
> Alexander Gordeev (5):
>   pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0
>   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              | 92 ++++++++++++++++++++++++++++----------------------
>  lib/pci.h              |  5 ++-
>  x86/intel-iommu.c      |  2 +-
>  x86/vmexit.c           |  1 -
>  5 files changed, 56 insertions(+), 48 deletions(-)
> 
> -- 
> 1.8.3.1
> 

[kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks

[Alexander Gordeev]

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

diff --git a/lib/pci.c b/lib/pci.c
index daf398100b7e..98fc3849d297 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar)
 		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
 }
 
-uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
+uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num)
 {
-	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 +
-				bar_num * 4);
+	CHECK_BAR_NUM(bar_num);
+
+	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }
 
-static 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, unsigned int bar_num)
 {
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
@@ -132,15 +133,26 @@ static 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)
+phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num)
 {
+	CHECK_BAR_NUM(bar_num);
+
 	return dev->resource[bar_num];
 }
 
-void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
+void pci_bar_set_addr(struct pci_dev *dev,
+		      unsigned int bar_num, phys_addr_t addr)
 {
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
+	CHECK_BAR_NUM(bar_num);
+	assert(addr != INVALID_PHYS_ADDR);
+	assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
+	if (pci_bar_is64(dev, bar_num)) {
+		assert(bar_num + 1 < PCI_BAR_NUM);
+		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);
+	}
+
 	pci_config_writel(dev->bdf, off, (uint32_t)addr);
 	dev->resource[bar_num] = addr;
 
@@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
  * The following pci_bar_size_helper() and pci_bar_size() functions
  * implement the algorithm.
  */
-static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
+static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num)
 {
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 	uint16_t bdf = dev->bdf;
@@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
 	return val;
 }
 
-phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
+phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num)
 {
 	uint32_t bar, size;
 
+	CHECK_BAR_NUM(bar_num);
+
 	size = pci_bar_size_helper(dev, bar_num);
 	if (!size)
 		return 0;
@@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
 	}
 }
 
-bool pci_bar_is_memory(struct pci_dev *dev, int bar_num)
+bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num)
 {
-	uint32_t bar = pci_bar_get(dev, bar_num);
+	uint32_t bar;
+
+	CHECK_BAR_NUM(bar_num);
+
+	bar = pci_bar_get(dev, bar_num);
 
 	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
 }
 
-bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
+bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num)
 {
+	CHECK_BAR_NUM(bar_num);
+
 	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
 }
 
-bool pci_bar_is64(struct pci_dev *dev, int bar_num)
+bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num)
 {
-	uint32_t bar = pci_bar_get(dev, bar_num);
+	uint32_t bar;
+
+	CHECK_BAR_NUM(bar_num);
+
+	bar = pci_bar_get(dev, bar_num);
 
 	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
 		return false;
@@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num)
 		      PCI_BASE_ADDRESS_MEM_TYPE_64;
 }
 
-void pci_bar_print(struct pci_dev *dev, int bar_num)
+void pci_bar_print(struct pci_dev *dev, unsigned int bar_num)
 {
 	phys_addr_t size, start, end;
 	uint32_t bar;
 
+	CHECK_BAR_NUM(bar_num);
+
 	if (!pci_bar_is_valid(dev, bar_num))
 		return;
 
diff --git a/lib/pci.h b/lib/pci.h
index 03cc0a72d48d..c770def840da 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -18,6 +18,9 @@ enum {
 #define PCI_BAR_NUM                     6
 #define PCI_DEVFN_MAX                   256
 
+#define CHECK_BAR_NUM(bar_num)	\
+	do { assert(bar_num < PCI_BAR_NUM); } while (0)
+
 #define PCI_BDF_GET_DEVFN(x)            ((x) & 0xff)
 #define PCI_BDF_GET_BUS(x)              (((x) >> 8) & 0xff)
 
@@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
  * It is expected the caller is aware of the device BAR layout and never
  * tries to address the middle of a 64-bit register.
  */
-extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num);
-extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr);
-extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num);
-extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num);
+
+extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num);
+extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num,
+			     phys_addr_t addr);
+extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num);
+extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num);
 extern uint32_t pci_bar_mask(uint32_t bar);
-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 bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num);
+extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num);
+extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num);
+extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num);
 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);
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks

[Andrew Jones]

Hi Alex,

This should be its own patch, the series you've added it to was
already committed.

On Mon, Mar 06, 2017 at 09:06:23PM +0100, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
>  lib/pci.h | 21 +++++++++++++--------
>  2 files changed, 53 insertions(+), 22 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index daf398100b7e..98fc3849d297 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar)
>  		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
>  }
>  
> -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
> +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num)
>  {
> -	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 +
> -				bar_num * 4);
> +	CHECK_BAR_NUM(bar_num);
> +
> +	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4);
>  }
>  
> -static 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, unsigned int bar_num)
>  {
>  	uint32_t bar = pci_bar_get(dev, bar_num);
>  	uint32_t mask = pci_bar_mask(bar);
> @@ -132,15 +133,26 @@ static 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)
> +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num)
>  {
> +	CHECK_BAR_NUM(bar_num);
> +
>  	return dev->resource[bar_num];
>  }
>  
> -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> +void pci_bar_set_addr(struct pci_dev *dev,
> +		      unsigned int bar_num, phys_addr_t addr)
>  {
>  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
>  
> +	CHECK_BAR_NUM(bar_num);
> +	assert(addr != INVALID_PHYS_ADDR);

Shouldn't this be something like

 assert((addr & PHYS_MASK) == addr)

But that means we need to ensure all architectures define PHYS_MASK...

> +	assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
> +	if (pci_bar_is64(dev, bar_num)) {
> +		assert(bar_num + 1 < PCI_BAR_NUM);

Use CHECK_BAR_NUM(bar_num + 1)

> +		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);

Can this ever happen without the unit test explicitly messing things up?

> +	}
> +
>  	pci_config_writel(dev->bdf, off, (uint32_t)addr);
>  	dev->resource[bar_num] = addr;
>  
> @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
>   * The following pci_bar_size_helper() and pci_bar_size() functions
>   * implement the algorithm.
>   */
> -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
> +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num)
>  {
>  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
>  	uint16_t bdf = dev->bdf;
> @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
>  	return val;
>  }
>  
> -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
> +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num)
>  {
>  	uint32_t bar, size;
>  
> +	CHECK_BAR_NUM(bar_num);
> +
>  	size = pci_bar_size_helper(dev, bar_num);
>  	if (!size)
>  		return 0;
> @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
>  	}
>  }
>  
> -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num)
> +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num)
>  {
> -	uint32_t bar = pci_bar_get(dev, bar_num);
> +	uint32_t bar;
> +
> +	CHECK_BAR_NUM(bar_num);
> +
> +	bar = pci_bar_get(dev, bar_num);

This hunk is unnecessary, pci_bar_get already calls CHECK_BAR_NUM

>  
>  	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
>  }
>  
> -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num)
>  {
> +	CHECK_BAR_NUM(bar_num);
> +
>  	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
>  }
>  
> -bool pci_bar_is64(struct pci_dev *dev, int bar_num)
> +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num)
>  {
> -	uint32_t bar = pci_bar_get(dev, bar_num);
> +	uint32_t bar;
> +
> +	CHECK_BAR_NUM(bar_num);
> +
> +	bar = pci_bar_get(dev, bar_num);

Same as above (unnecessary hunk)

>  
>  	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
>  		return false;
> @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num)
>  		      PCI_BASE_ADDRESS_MEM_TYPE_64;
>  }
>  
> -void pci_bar_print(struct pci_dev *dev, int bar_num)
> +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num)
>  {
>  	phys_addr_t size, start, end;
>  	uint32_t bar;
>  
> +	CHECK_BAR_NUM(bar_num);
> +

Unnecessary, pci_bar_is_valid already checks

>  	if (!pci_bar_is_valid(dev, bar_num))
>  		return;
>  
> diff --git a/lib/pci.h b/lib/pci.h
> index 03cc0a72d48d..c770def840da 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -18,6 +18,9 @@ enum {
>  #define PCI_BAR_NUM                     6
>  #define PCI_DEVFN_MAX                   256
>  
> +#define CHECK_BAR_NUM(bar_num)	\
> +	do { assert(bar_num < PCI_BAR_NUM); } while (0)

Just add 'bar_num >= 0 &&' to this macro and we don't need to
change bar_num to unsigned everywhere

> +
>  #define PCI_BDF_GET_DEVFN(x)            ((x) & 0xff)
>  #define PCI_BDF_GET_BUS(x)              (((x) >> 8) & 0xff)
>  
> @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
>   * It is expected the caller is aware of the device BAR layout and never
>   * tries to address the middle of a 64-bit register.
>   */
> -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num);
> -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr);
> -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num);
> -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num);
> +
> +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num);
> +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num,
> +			     phys_addr_t addr);
> +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num);
> +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num);
>  extern uint32_t pci_bar_mask(uint32_t bar);
> -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 bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num);
> +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num);
> +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num);
> +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num);
>  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);
> -- 
> 1.8.3.1
> 

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks

[Alexander Gordeev]

On Tue, Mar 07, 2017 at 03:22:53PM +0100, Andrew Jones wrote:
> Hi Alex,
> 
> This should be its own patch, the series you've added it to was
> already committed.
> 
> On Mon, Mar 06, 2017 at 09:06:23PM +0100, Alexander Gordeev wrote:
> > Cc: Thomas Huth <thuth@redhat.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
> >  lib/pci.h | 21 +++++++++++++--------
> >  2 files changed, 53 insertions(+), 22 deletions(-)
> > 
> > diff --git a/lib/pci.c b/lib/pci.c
> > index daf398100b7e..98fc3849d297 100644
> > --- a/lib/pci.c
> > +++ b/lib/pci.c
> > @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar)
> >  		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
> >  }
> >  
> > -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
> > +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > -	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 +
> > -				bar_num * 4);
> > +	CHECK_BAR_NUM(bar_num);
> > +
> > +	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4);
> >  }
> >  
> > -static 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, unsigned int bar_num)
> >  {
> >  	uint32_t bar = pci_bar_get(dev, bar_num);
> >  	uint32_t mask = pci_bar_mask(bar);
> > @@ -132,15 +133,26 @@ static 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)
> > +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > +	CHECK_BAR_NUM(bar_num);
> > +
> >  	return dev->resource[bar_num];
> >  }
> >  
> > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> > +void pci_bar_set_addr(struct pci_dev *dev,
> > +		      unsigned int bar_num, phys_addr_t addr)
> >  {
> >  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> >  
> > +	CHECK_BAR_NUM(bar_num);
> > +	assert(addr != INVALID_PHYS_ADDR);
> 
> Shouldn't this be something like
> 
>  assert((addr & PHYS_MASK) == addr)

Nope. At this stage PCI bus is scanned and pci_dev::resource[]
is initialized with proper values. A value of INVALID_PHYS_ADDR in
dev->resource[bar_num] indicates BAR #bar_num is not implemented.
Thus, we do not want to screw up this assumption with a test case
trying to write INVALID_PHYS_ADDR to a the BAR.

> But that means we need to ensure all architectures define PHYS_MASK...
> 
> > +	assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
> > +	if (pci_bar_is64(dev, bar_num)) {
> > +		assert(bar_num + 1 < PCI_BAR_NUM);
> 
> Use CHECK_BAR_NUM(bar_num + 1)
> 
> > +		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);
> 
> Can this ever happen without the unit test explicitly messing things up?
> 
> > +	}
> > +
> >  	pci_config_writel(dev->bdf, off, (uint32_t)addr);
> >  	dev->resource[bar_num] = addr;
> >  
> > @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> >   * The following pci_bar_size_helper() and pci_bar_size() functions
> >   * implement the algorithm.
> >   */
> > -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
> > +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num)
> >  {
> >  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> >  	uint16_t bdf = dev->bdf;
> > @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
> >  	return val;
> >  }
> >  
> > -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
> > +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num)
> >  {
> >  	uint32_t bar, size;
> >  
> > +	CHECK_BAR_NUM(bar_num);
> > +
> >  	size = pci_bar_size_helper(dev, bar_num);
> >  	if (!size)
> >  		return 0;
> > @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
> >  	}
> >  }
> >  
> > -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num)
> > +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > -	uint32_t bar = pci_bar_get(dev, bar_num);
> > +	uint32_t bar;
> > +
> > +	CHECK_BAR_NUM(bar_num);
> > +
> > +	bar = pci_bar_get(dev, bar_num);
> 
> This hunk is unnecessary, pci_bar_get already calls CHECK_BAR_NUM

Right, it is superfluous. I put it here intentionally to avoid
the check to go away in case pci_bar_get() is changed somehow.
Pure paranoia. I can remove it if you want here and elsewhere.

> >  	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
> >  }
> >  
> > -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> > +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > +	CHECK_BAR_NUM(bar_num);
> > +
> >  	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
> >  }
> >  
> > -bool pci_bar_is64(struct pci_dev *dev, int bar_num)
> > +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > -	uint32_t bar = pci_bar_get(dev, bar_num);
> > +	uint32_t bar;
> > +
> > +	CHECK_BAR_NUM(bar_num);
> > +
> > +	bar = pci_bar_get(dev, bar_num);
> 
> Same as above (unnecessary hunk)
> 
> >  
> >  	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> >  		return false;
> > @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num)
> >  		      PCI_BASE_ADDRESS_MEM_TYPE_64;
> >  }
> >  
> > -void pci_bar_print(struct pci_dev *dev, int bar_num)
> > +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num)
> >  {
> >  	phys_addr_t size, start, end;
> >  	uint32_t bar;
> >  
> > +	CHECK_BAR_NUM(bar_num);
> > +
> 
> Unnecessary, pci_bar_is_valid already checks
> 
> >  	if (!pci_bar_is_valid(dev, bar_num))
> >  		return;
> >  
> > diff --git a/lib/pci.h b/lib/pci.h
> > index 03cc0a72d48d..c770def840da 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -18,6 +18,9 @@ enum {
> >  #define PCI_BAR_NUM                     6
> >  #define PCI_DEVFN_MAX                   256
> >  
> > +#define CHECK_BAR_NUM(bar_num)	\
> > +	do { assert(bar_num < PCI_BAR_NUM); } while (0)
> 
> Just add 'bar_num >= 0 &&' to this macro and we don't need to
> change bar_num to unsigned everywhere
> 
> > +
> >  #define PCI_BDF_GET_DEVFN(x)            ((x) & 0xff)
> >  #define PCI_BDF_GET_BUS(x)              (((x) >> 8) & 0xff)
> >  
> > @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> >   * It is expected the caller is aware of the device BAR layout and never
> >   * tries to address the middle of a 64-bit register.
> >   */
> > -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num);
> > -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr);
> > -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num);
> > -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num);
> > +
> > +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num);
> > +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num,
> > +			     phys_addr_t addr);
> > +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num);
> > +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num);
> >  extern uint32_t pci_bar_mask(uint32_t bar);
> > -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 bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num);
> > +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num);
> > +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num);
> > +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num);
> >  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);
> > -- 
> > 1.8.3.1
> > 
> 
> Thanks,
> drew

Re: [kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks

[Andrew Jones]

On Tue, Mar 07, 2017 at 08:39:06PM +0100, Alexander Gordeev wrote:
> On Tue, Mar 07, 2017 at 03:22:53PM +0100, Andrew Jones wrote:
> > Hi Alex,
> > 
> > This should be its own patch, the series you've added it to was
> > already committed.
> > 
> > On Mon, Mar 06, 2017 at 09:06:23PM +0100, Alexander Gordeev wrote:
> > > Cc: Thomas Huth <thuth@redhat.com>
> > > Cc: Andrew Jones <drjones@redhat.com>
> > > Cc: Peter Xu <peterx@redhat.com>
> > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > > ---
> > >  lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
> > >  lib/pci.h | 21 +++++++++++++--------
> > >  2 files changed, 53 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/lib/pci.c b/lib/pci.c
> > > index daf398100b7e..98fc3849d297 100644
> > > --- a/lib/pci.c
> > > +++ b/lib/pci.c
> > > @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar)
> > >  		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
> > >  }
> > >  
> > > -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
> > > +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num)
> > >  {
> > > -	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 +
> > > -				bar_num * 4);
> > > +	CHECK_BAR_NUM(bar_num);
> > > +
> > > +	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4);
> > >  }
> > >  
> > > -static 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, unsigned int bar_num)
> > >  {
> > >  	uint32_t bar = pci_bar_get(dev, bar_num);
> > >  	uint32_t mask = pci_bar_mask(bar);
> > > @@ -132,15 +133,26 @@ static 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)
> > > +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num)
> > >  {
> > > +	CHECK_BAR_NUM(bar_num);
> > > +
> > >  	return dev->resource[bar_num];
> > >  }
> > >  
> > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> > > +void pci_bar_set_addr(struct pci_dev *dev,
> > > +		      unsigned int bar_num, phys_addr_t addr)
> > >  {
> > >  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> > >  
> > > +	CHECK_BAR_NUM(bar_num);
> > > +	assert(addr != INVALID_PHYS_ADDR);
> > 
> > Shouldn't this be something like
> > 
> >  assert((addr & PHYS_MASK) == addr)
> 
> Nope. At this stage PCI bus is scanned and pci_dev::resource[]
> is initialized with proper values. A value of INVALID_PHYS_ADDR in
> dev->resource[bar_num] indicates BAR #bar_num is not implemented.
> Thus, we do not want to screw up this assumption with a test case
> trying to write INVALID_PHYS_ADDR to a the BAR.

Of course, by why would any other invalid phys addr be OK? Hmm, maybe
it should be allowed for test case purposes.

> 
> > But that means we need to ensure all architectures define PHYS_MASK...
> > 
> > > +	assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
> > > +	if (pci_bar_is64(dev, bar_num)) {
> > > +		assert(bar_num + 1 < PCI_BAR_NUM);
> > 
> > Use CHECK_BAR_NUM(bar_num + 1)
> > 
> > > +		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);
> > 
> > Can this ever happen without the unit test explicitly messing things up?

No answer for this. I don't see the point of an assert that can never
fail.

> > 
> > > +	}
> > > +
> > >  	pci_config_writel(dev->bdf, off, (uint32_t)addr);
> > >  	dev->resource[bar_num] = addr;
> > >  
> > > @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> > >   * The following pci_bar_size_helper() and pci_bar_size() functions
> > >   * implement the algorithm.
> > >   */
> > > -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
> > > +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num)
> > >  {
> > >  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> > >  	uint16_t bdf = dev->bdf;
> > > @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
> > >  	return val;
> > >  }
> > >  
> > > -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
> > > +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num)
> > >  {
> > >  	uint32_t bar, size;
> > >  
> > > +	CHECK_BAR_NUM(bar_num);
> > > +
> > >  	size = pci_bar_size_helper(dev, bar_num);
> > >  	if (!size)
> > >  		return 0;
> > > @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
> > >  	}
> > >  }
> > >  
> > > -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num)
> > > +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num)
> > >  {
> > > -	uint32_t bar = pci_bar_get(dev, bar_num);
> > > +	uint32_t bar;
> > > +
> > > +	CHECK_BAR_NUM(bar_num);
> > > +
> > > +	bar = pci_bar_get(dev, bar_num);
> > 
> > This hunk is unnecessary, pci_bar_get already calls CHECK_BAR_NUM
> 
> Right, it is superfluous. I put it here intentionally to avoid
> the check to go away in case pci_bar_get() is changed somehow.
> Pure paranoia. I can remove it if you want here and elsewhere.

I'd prefer it be removed. No need for the clutter.

> 
> > >  	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
> > >  }
> > >  
> > > -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> > > +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num)
> > >  {
> > > +	CHECK_BAR_NUM(bar_num);
> > > +
> > >  	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
> > >  }
> > >  
> > > -bool pci_bar_is64(struct pci_dev *dev, int bar_num)
> > > +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num)
> > >  {
> > > -	uint32_t bar = pci_bar_get(dev, bar_num);
> > > +	uint32_t bar;
> > > +
> > > +	CHECK_BAR_NUM(bar_num);
> > > +
> > > +	bar = pci_bar_get(dev, bar_num);
> > 
> > Same as above (unnecessary hunk)
> > 
> > >  
> > >  	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > >  		return false;
> > > @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num)
> > >  		      PCI_BASE_ADDRESS_MEM_TYPE_64;
> > >  }
> > >  
> > > -void pci_bar_print(struct pci_dev *dev, int bar_num)
> > > +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num)
> > >  {
> > >  	phys_addr_t size, start, end;
> > >  	uint32_t bar;
> > >  
> > > +	CHECK_BAR_NUM(bar_num);
> > > +
> > 
> > Unnecessary, pci_bar_is_valid already checks
> > 
> > >  	if (!pci_bar_is_valid(dev, bar_num))
> > >  		return;
> > >  
> > > diff --git a/lib/pci.h b/lib/pci.h
> > > index 03cc0a72d48d..c770def840da 100644
> > > --- a/lib/pci.h
> > > +++ b/lib/pci.h
> > > @@ -18,6 +18,9 @@ enum {
> > >  #define PCI_BAR_NUM                     6
> > >  #define PCI_DEVFN_MAX                   256
> > >  
> > > +#define CHECK_BAR_NUM(bar_num)	\
> > > +	do { assert(bar_num < PCI_BAR_NUM); } while (0)
> > 
> > Just add 'bar_num >= 0 &&' to this macro and we don't need to
> > change bar_num to unsigned everywhere
> > 
> > > +
> > >  #define PCI_BDF_GET_DEVFN(x)            ((x) & 0xff)
> > >  #define PCI_BDF_GET_BUS(x)              (((x) >> 8) & 0xff)
> > >  
> > > @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> > >   * It is expected the caller is aware of the device BAR layout and never
> > >   * tries to address the middle of a 64-bit register.
> > >   */
> > > -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num);
> > > -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr);
> > > -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num);
> > > -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num);
> > > +
> > > +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num);
> > > +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num,
> > > +			     phys_addr_t addr);
> > > +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num);
> > > +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num);
> > >  extern uint32_t pci_bar_mask(uint32_t bar);
> > > -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 bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num);
> > > +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num);
> > > +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num);
> > > +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num);
> > >  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);
> > > -- 
> > > 1.8.3.1
> > > 
> > 
> > Thanks,
> > drew

Re: [kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks

[Alexander Gordeev]

On Wed, Mar 08, 2017 at 11:10:26AM +0100, Andrew Jones wrote:
> > > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> > > > +void pci_bar_set_addr(struct pci_dev *dev,
> > > > +		      unsigned int bar_num, phys_addr_t addr)
> > > >  {
> > > >  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> > > >  
> > > > +	CHECK_BAR_NUM(bar_num);
> > > > +	assert(addr != INVALID_PHYS_ADDR);
> > > 
> > > Shouldn't this be something like
> > > 
> > >  assert((addr & PHYS_MASK) == addr)
> > 
> > Nope. At this stage PCI bus is scanned and pci_dev::resource[]
> > is initialized with proper values. A value of INVALID_PHYS_ADDR in
> > dev->resource[bar_num] indicates BAR #bar_num is not implemented.
> > Thus, we do not want to screw up this assumption with a test case
> > trying to write INVALID_PHYS_ADDR to a the BAR.
> 
> Of course, by why would any other invalid phys addr be OK? Hmm, maybe
> it should be allowed for test case purposes.

Yeah, I think it should be allowed.

> > > But that means we need to ensure all architectures define PHYS_MASK...
> > > 
> > > > +	assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
> > > > +	if (pci_bar_is64(dev, bar_num)) {
> > > > +		assert(bar_num + 1 < PCI_BAR_NUM);
> > > 
> > > Use CHECK_BAR_NUM(bar_num + 1)
> > > 
> > > > +		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);
> > > 
> > > Can this ever happen without the unit test explicitly messing things up?

No, unless there is really something wrong with the code.

> No answer for this. I don't see the point of an assert that can never
> fail.

So this is code integrity assert, not test case input check.
Do you want me to remove it?

> > > Thanks,
> > > drew

Re: [kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks

[Andrew Jones]

On Wed, Mar 08, 2017 at 12:40:59PM +0100, Alexander Gordeev wrote:
> On Wed, Mar 08, 2017 at 11:10:26AM +0100, Andrew Jones wrote:
> > > > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> > > > > +void pci_bar_set_addr(struct pci_dev *dev,
> > > > > +		      unsigned int bar_num, phys_addr_t addr)
> > > > >  {
> > > > >  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> > > > >  
> > > > > +	CHECK_BAR_NUM(bar_num);
> > > > > +	assert(addr != INVALID_PHYS_ADDR);
> > > > 
> > > > Shouldn't this be something like
> > > > 
> > > >  assert((addr & PHYS_MASK) == addr)
> > > 
> > > Nope. At this stage PCI bus is scanned and pci_dev::resource[]
> > > is initialized with proper values. A value of INVALID_PHYS_ADDR in
> > > dev->resource[bar_num] indicates BAR #bar_num is not implemented.
> > > Thus, we do not want to screw up this assumption with a test case
> > > trying to write INVALID_PHYS_ADDR to a the BAR.
> > 
> > Of course, by why would any other invalid phys addr be OK? Hmm, maybe
> > it should be allowed for test case purposes.
> 
> Yeah, I think it should be allowed.
> 
> > > > But that means we need to ensure all architectures define PHYS_MASK...
> > > > 
> > > > > +	assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
> > > > > +	if (pci_bar_is64(dev, bar_num)) {
> > > > > +		assert(bar_num + 1 < PCI_BAR_NUM);
> > > > 
> > > > Use CHECK_BAR_NUM(bar_num + 1)
> > > > 
> > > > > +		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);
> > > > 
> > > > Can this ever happen without the unit test explicitly messing things up?
> 
> No, unless there is really something wrong with the code.
> 
> > No answer for this. I don't see the point of an assert that can never
> > fail.
> 
> So this is code integrity assert, not test case input check.
> Do you want me to remove it?

Yes please. We don't generally defend the lib code from itself. We use
asserts for anything the user could influence and for some test case
inputs, in order to help the test writer avoid going down obviously
buggy paths. Although, like above, we also try to give the test writer
freedom to do crazy things, as that's the whole point of a testsuite :-)

Thanks,
drew