Skip to contentSkip to conversation

pci: Complete conversion of PCI API to struct pci_dev

PATCH Applied Alexander Gordeev 3 participants 13 messages Feb 28, 2017 – Mar 8, 2017

Subsystems (2)
KVM Library code
lore
Share & tools
lore

Alexander Gordeev

author
#0 lore raw

v4 cover pci: Complete conversion of PCI API to struct pci_dev

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 <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>

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(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH v4 1/5 pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0

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

Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci-host-generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
lib/pci-host-generic.c +1 -1
index 6f2ab9e85a13..8b505b444a34 100644
@@ -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)
Signature · 1 line
1.8.3.1
author
#2 lore raw

PATCH v4 2/5 pci: Accomodate 64 bit BARs in pci_dev::resource[]

In reply to 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 <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
lib/pci.c +9 -6
index 62b1c6b0b7c5..9acc9652cb25 100644
@@ -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;
 		}
 	}
 }
Signature · 1 line
1.8.3.1
author
#3 lore raw

PATCH v4 3/5 pci: Turn struct pci_dev into device handle for PCI functions

In reply to 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 <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c    | 30 +++++++++++++++++-------------
 lib/pci.h    |  1 -
 x86/vmexit.c |  1 -
 3 files changed, 17 insertions(+), 15 deletions(-)
lib/pci.c +17 -13
index 9acc9652cb25..cf33b894759d 100644
@@ -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);
lib/pci.h +0 -1
index 3da3ccc8c791..fefd9a84b307 100644
@@ -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);
x86/vmexit.c +0 -1
index 71f4d156b3ee..5b821b5eb125 100644
@@ -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];
Signature · 1 line
1.8.3.1
author
#4 lore raw

PATCH v4 4/5 pci: Rework pci_bar_is_valid()

In reply to Alexander Gordeev
Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
lib/pci.c +4 -4
index cf33b894759d..fc18b254366c 100644
@@ -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");
Signature · 1 line
1.8.3.1
author
#5 lore raw

PATCH v4 5/5 pci: Make PCI API consistent wrt using struct pci_dev

In reply to 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 <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 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(-)
lib/pci-host-generic.c +1 -1
index 8b505b444a34..818150dc0a66 100644
@@ -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");
lib/pci.c +23 -20

78 lines not on this page · raw message

lib/pci.h +2 -2
index fefd9a84b307..03cc0a72d48d 100644
@@ -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);
x86/intel-iommu.c +1 -1
index a01b23e674a8..610cc655c41b 100644
@@ -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();
 	}
Signature · 1 line
1.8.3.1
Applied
#6 lore raw
In reply to Alexander Gordeev

2017-02-28 19:08+0100, Alexander Gordeev:

Quoted 3 lines

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.

Quoted 24 lines

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

Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>

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(-)
Signature · 1 line
1.8.3.1
author
#7 lore raw

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

In reply to Alexander Gordeev
Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
 lib/pci.h | 21 +++++++++++++--------
 2 files changed, 53 insertions(+), 22 deletions(-)
lib/pci.c +40 -14
index daf398100b7e..98fc3849d297 100644
@@ -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;
lib/pci.h +13 -8
index 03cc0a72d48d..c770def840da 100644
@@ -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);
Signature · 1 line
1.8.3.1
#8 lore raw

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

Hi Alex,

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

Quoted 42 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
 lib/pci.h | 21 +++++++++++++--------
 2 files changed, 53 insertions(+), 22 deletions(-)
lib/pci.c +8 -5
index daf398100b7e..98fc3849d297 100644
@@ -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];

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 149

Quoted 10 lines
lib/pci.c +4 -1
 }
 
-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...

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 152

Quoted 3 lines
lib/pci.c +3 -0
+	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)

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 153

Quoted 1 line
lib/pci.c +1 -0
+		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);

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

Quoted 31 lines
lib/pci.c +6 -2
+	}
+
 	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)
 	}
 }

lib/pci.c phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) · line 219

Quoted 10 lines
lib/pci.c +6 -2
 
-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

Quoted 11 lines
lib/pci.c +3 -1
 	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;
 }

lib/pci.c phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) · line 237

Quoted 10 lines
lib/pci.c +6 -2
 
-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)

Quoted 5 lines
lib/pci.c +0 -0
 	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;

lib/pci.c bool pci_bar_is64(struct pci_dev *dev, int bar_num) · line 252

Quoted 10 lines
lib/pci.c +3 -1
 }
 
-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

lib/pci.h enum { · line 22

Quoted 13 lines
lib/pci.c +0 -0
 	if (!pci_bar_is_valid(dev, bar_num))
 		return;
lib/pci.h +2 -0
index 03cc0a72d48d..c770def840da 100644
@@ -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

Quoted 33 lines
lib/pci.h +11 -8
+
 #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);
Signature · 1 line
1.8.3.1

Thanks,
drew

author
#9 lore raw

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

Quoted 56 lines · Andrew Jones

Hi Alex,

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

Quoted 50 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
 lib/pci.h | 21 +++++++++++++--------
 2 files changed, 53 insertions(+), 22 deletions(-)
lib/pci.c +10 -6
index daf398100b7e..98fc3849d297 100644
@@ -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;
Quoted 6 lines

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 149

Quoted 2 lines
lib/pci.c +2 -0
+	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.

Quoted 44 lines

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

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 152

Quoted 3 lines
lib/pci.c +3 -0
+	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)

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 153

Quoted 1 line
lib/pci.c +1 -0
+		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);

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

Quoted 32 lines
lib/pci.c +6 -2
+	}
+
 	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)
 	}
 }
Quoted 11 lines

lib/pci.c phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) · line 219

Quoted 9 lines
lib/pci.c +6 -2
-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.

Quoted 94 lines
Quoted 10 lines
lib/pci.c +3 -1
 	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;
 }

lib/pci.c phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) · line 237

Quoted 10 lines
lib/pci.c +6 -2
 
-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)

Quoted 5 lines
lib/pci.c +0 -0
 	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;

lib/pci.c bool pci_bar_is64(struct pci_dev *dev, int bar_num) · line 252

Quoted 10 lines
lib/pci.c +3 -1
 }
 
-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

lib/pci.h enum { · line 22

Quoted 13 lines
lib/pci.c +0 -0
 	if (!pci_bar_is_valid(dev, bar_num))
 		return;
lib/pci.h +2 -0
index 03cc0a72d48d..c770def840da 100644
@@ -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

Quoted 33 lines
lib/pci.h +11 -8
+
 #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);
Signature · 1 line
1.8.3.1

Thanks,
drew

#10 lore raw

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

Quoted 62 lines · Alexander Gordeev
Quoted 61 lines · Andrew Jones

Hi Alex,

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

Quoted 42 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Cc Peter Xu <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
 lib/pci.h | 21 +++++++++++++--------
 2 files changed, 53 insertions(+), 22 deletions(-)
lib/pci.c +8 -5
index daf398100b7e..98fc3849d297 100644
@@ -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];

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 149

Quoted 10 lines
lib/pci.c +4 -1
 }
 
-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

Quoted 7 lines
Quoted 1 line

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.

Quoted 9 lines
Quoted 9 lines

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

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 152

Quoted 3 lines
lib/pci.c +3 -0
+	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)

Quoted 3 lines
Quoted 3 lines

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 153

Quoted 1 line
lib/pci.c +1 -0
+		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.

Quoted 45 lines
Quoted 33 lines
Quoted 33 lines
lib/pci.c +6 -2
+	}
+
 	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)
 	}
 }
Quoted 11 lines

lib/pci.c phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) · line 219

Quoted 9 lines
lib/pci.c +6 -2
-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

Quoted 3 lines

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.

Quoted 95 lines
Quoted 95 lines
Quoted 11 lines
lib/pci.c +3 -1
 	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;
 }

lib/pci.c phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) · line 237

Quoted 10 lines
lib/pci.c +6 -2
 
-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)

Quoted 5 lines
lib/pci.c +0 -0
 	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;

lib/pci.c bool pci_bar_is64(struct pci_dev *dev, int bar_num) · line 252

Quoted 10 lines
lib/pci.c +3 -1
 }
 
-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

lib/pci.h enum { · line 22

Quoted 13 lines
lib/pci.c +0 -0
 	if (!pci_bar_is_valid(dev, bar_num))
 		return;
lib/pci.h +2 -0
index 03cc0a72d48d..c770def840da 100644
@@ -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

Quoted 33 lines
lib/pci.h +11 -8
+
 #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);
Signature · 1 line
1.8.3.1

Thanks,
drew

author
#11 lore raw

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

Quoted 13 lines · Andrew Jones
Quoted 13 lines
Quoted 12 lines

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 149

Quoted 8 lines
lib/pci.c +4 -1
-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)

Quoted 8 lines
Quoted 5 lines

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.

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines

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

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 152

Quoted 3 lines
lib/pci.c +3 -0
+	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)

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 153

Quoted 1 line
lib/pci.c +1 -0
+		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.

Quoted 2 lines

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?

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Thanks,
drew

#12 lore raw

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

Quoted 37 lines · Alexander Gordeev
Quoted 13 lines · Andrew Jones
Quoted 13 lines
Quoted 12 lines

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 149

Quoted 8 lines
lib/pci.c +4 -1
-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)

Quoted 8 lines
Quoted 5 lines

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.

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines

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

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 152

Quoted 3 lines
lib/pci.c +3 -0
+	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)

lib/pci.c static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) · line 153

Quoted 1 line
lib/pci.c +1 -0
+		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);

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

Quoted 7 lines

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

Quoted 2 lines

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