pci: Complete conversion of PCI API to struct pci_dev

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

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

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

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

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

By placing the same 64-bit address in both bar_num and (bar_num + 1) elements the ambiguity is less striking, since:
- the meaning of bar_num kept consistent with the rest of the functions (where it refers 32-bit BAR in terms of the device configuration address space);
- pci_dev::resource[bar_num + 1] contains a valid address rather than vague value of 0.
- both bar_num and (bar_num + 1) indexes refer to the

    same 64-bit BAR and therefore return the same address;
    The notion of low and high parts of a 64-bit address
    is ignored, but that is fine, since pci_dev::resource[]
    contain only full addresses;

Quoted 29 lines · Alexander Gordeev

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

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

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

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

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

By placing the same 64-bit address in both bar_num and (bar_num + 1) elements the ambiguity is less striking, since:
- the meaning of bar_num kept consistent with the rest of the functions (where it refers 32-bit BAR in terms of the device configuration address space);
- pci_dev::resource[bar_num + 1] contains a valid address rather than vague value of 0.
- both bar_num and (bar_num + 1) indexes refer to the

Quoted 4 lines

    same 64-bit BAR and therefore return the same address;
    The notion of low and high parts of a 64-bit address
    is ignored, but that is fine, since pci_dev::resource[]
    contain only full addresses;

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

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

Quoted 35 lines · Peter Xu

Quoted 29 lines · Alexander Gordeev

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

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

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

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

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

By placing the same 64-bit address in both bar_num and (bar_num + 1) elements the ambiguity is less striking, since:
- the meaning of bar_num kept consistent with the rest of the functions (where it refers 32-bit BAR in terms of the device configuration address space);
- pci_dev::resource[bar_num + 1] contains a valid address rather than vague value of 0.
- both bar_num and (bar_num + 1) indexes refer to the

Quoted 4 lines

    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;

Quoted 6 lines

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

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

Quoted 5 lines

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

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

Quoted 43 lines · Andrew Jones

Quoted 35 lines · Peter Xu

Quoted 29 lines · Alexander Gordeev

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

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

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

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

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

By placing the same 64-bit address in both bar_num and (bar_num + 1) elements the ambiguity is less striking, since:
- the meaning of bar_num kept consistent with the rest of the functions (where it refers 32-bit BAR in terms of the device configuration address space);
- pci_dev::resource[bar_num + 1] contains a valid address rather than vague value of 0.
- both bar_num and (bar_num + 1) indexes refer to the

Quoted 4 lines

    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;

Quoted 6 lines

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

Quoted 8 lines

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

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

Quoted 29 lines · Alexander Gordeev

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

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

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

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

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

By placing the same 64-bit address in both bar_num and (bar_num + 1) elements the ambiguity is less striking, since:
- the meaning of bar_num kept consistent with the rest of the functions (where it refers 32-bit BAR in terms of the device configuration address space);
- pci_dev::resource[bar_num + 1] contains a valid address rather than vague value of 0.
- both bar_num and (bar_num + 1) indexes refer to the

Quoted 4 lines

    same 64-bit BAR and therefore return the same address;
    The notion of low and high parts of a 64-bit address
    is ignored, but that is fine, since pci_dev::resource[]
    contain only full addresses;

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

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

Thanks,

-- peterx

Quoted 35 lines · Peter Xu

Quoted 29 lines · Alexander Gordeev

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

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

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

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

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

By placing the same 64-bit address in both bar_num and (bar_num + 1) elements the ambiguity is less striking, since:
- the meaning of bar_num kept consistent with the rest of the functions (where it refers 32-bit BAR in terms of the device configuration address space);
- pci_dev::resource[bar_num + 1] contains a valid address rather than vague value of 0.
- both bar_num and (bar_num + 1) indexes refer to the

Quoted 4 lines

    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;

Quoted 6 lines

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

Yes - see how pci_bar_is_valid() is implemented:

bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)

{
	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
}

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

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

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

    non-zero. So there is inconsistency also;

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

Quoted 4 lines

bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)

{
	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
}

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

Thanks,

-- peterx

Quoted 4 lines · Peter Xu

Quoted 4 lines

bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)

{
	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
}

(*)

Quoted 2 lines

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

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

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

On Tue, Feb 28, 2017 at 05:55:01PM +0800, Peter Xu wrote:

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

Quoted 1 line · Alexander Gordeev

On Tue, Feb 28, 2017 at 05:55:01PM +0800, Peter Xu wrote:

[...]

Quoted 2 lines

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

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

-- peterx

Quoted 5 lines · Peter Xu

Quoted 1 line · Alexander Gordeev

On Tue, Feb 28, 2017 at 05:55:01PM +0800, Peter Xu wrote:

[...]

Quoted 5 lines

Quoted 2 lines

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

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

Hmm.. that sounds about right. Andrew?

Quoted 2 lines

-- peterx

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

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c    | 31 ++++++++++++++++++-------------
 lib/pci.h    |  1 -
 x86/vmexit.c |  1 -
 3 files changed, 18 insertions(+), 15 deletions(-)

lib/pci.c +2 -7

index 9acc9652cb25..aded1b1ddb77 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,24 @@ phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
 	return phys_addr;
 }
 
+phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)

lib/pci.c +12 -6

-	if (pci_bar_is64(dev, bar_num))
-		pci_config_writel(dev->bdf, off + 4,
-				  (uint32_t)(addr >> 32));
+	dev->resource[bar_num] = addr;
+
+	if (pci_bar_is64(dev, bar_num)) {
+		assert(bar_num + 1 < PCI_BAR_NUM);
+		pci_config_writel(dev->bdf, off + 4, (uint32_t)(addr >> 32));
+		dev->resource[bar_num + 1] = dev->resource[bar_num];
+	}
 }
 
 /*

@@ -326,13 +329,16 @@ void pci_print(void)
 	}
 }
 
-void pci_scan_bars(struct pci_dev *dev)
+void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf)
 {
 	int i;
 
+	memset(dev, 0, sizeof(*dev));
+	dev->bdf = bdf;
+
 	for (i = 0; i < PCI_BAR_NUM; i++) {
 		if (pci_bar_size(dev, i)) {
-			dev->resource[i] = pci_bar_get_addr(dev, i);
+			dev->resource[i] = __pci_bar_get_addr(dev, i);
 			if (pci_bar_is64(dev, i)) {
 				assert(i + 1 < PCI_BAR_NUM);
 				dev->resource[i + 1] = dev->resource[i];

@@ -360,7 +366,6 @@ static void pci_cap_setup(struct pci_dev *dev, int cap_offset, int cap_id)
 
 void pci_enable_defaults(struct pci_dev *dev)
 {
-	pci_scan_bars(dev);
 	/* Enable device DMA operations */
 	pci_cmd_set_clr(dev, PCI_COMMAND_MASTER, 0);
 	pci_cap_walk(dev, pci_cap_setup);

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

Hi Andrew,

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

Thanks!

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

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

Signature · 1 line

1.8.3.1