PCI bus support

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

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

---
 lib/pci.c | 38 ++++++++++++++++++++------------------
 lib/pci.h |  3 ++-
 2 files changed, 22 insertions(+), 19 deletions(-)

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

---
 lib/pci.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

lib/pci.h +4 -4

index 54fbf22d634a..066fac77b237 100644
@@ -15,10 +15,10 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
 
-pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
-bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
-bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
+extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
+extern unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
+extern bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
+extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 
 /*
  * pci-testdev is a driver for the pci-testdev qemu pci device. The

Signature · 1 line

1.8.3.1

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

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

---
 lib/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

lib/pci.c +6 -2

index b05ecfa3f3da..ce481bbfadb6 100644
@@ -21,9 +21,14 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	return PCIDEVADDR_INVALID;
 }
 
+static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
+{
+	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+}
+
 unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	uint32_t bar = pci_bar_get(dev, bar_num);
 
 	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
 		return bar & PCI_BASE_ADDRESS_IO_MASK;

@@ -33,12 +38,12 @@ unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
 
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);

lib/pci.c +2 -1

-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
+phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_bar_get(dev, bar_num);
+	uint32_t mask = pci_bar_mask(bar);

Hi, Alex,

I see that we are adding ARM tests in the series as well, so I am thinking whether this work is a prepare work to test ARM SMMU?

Also, some comments below...

On Wed, Aug 17, 2016 at 02:07:07PM +0200, Alexander Gordeev wrote:

[...]

Quoted 5 lines

lib/pci.c +2 -1

-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
+phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_bar_get(dev, bar_num);
+	uint32_t mask = pci_bar_mask(bar);

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 44

Quoted 10 lines

lib/pci.c +5 -4

+	uint64_t addr = bar & mask;
 
-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num))
+		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
+
+	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

[...]

lib/x86/asm/pci.h static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val · line 43

Quoted 6 lines

lib/x86/asm/pci.h +6 -0

+static inline
+phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
+{
+    return addr;
+}
+

We are not using dev here (and in following patches), I don't know ARM, but at least x86 will need this to translate IOVA into PA (in other words, each device can have its own IO address space). If this codes are for common, shall we consider that as well?

Thanks.

-- peterx
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Peter Xu

Hi, Alex,

I see that we are adding ARM tests in the series as well, so I am thinking whether this work is a prepare work to test ARM SMMU?

That would be excellent. Adding Eric, he may be able to help here.

Quoted 7 lines

lib/pci.c +3 -1

-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
+phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_bar_get(dev, bar_num);
+	uint32_t mask = pci_bar_mask(bar);
+	uint64_t addr = bar & mask;

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 44

Quoted 8 lines

lib/pci.c +4 -4

-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num))
+		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
+
+	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

[...]

lib/x86/asm/pci.h static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val · line 43

Quoted 6 lines

lib/x86/asm/pci.h +6 -0

+static inline
+phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
+{
+    return addr;
+}
+

We are not using dev here (and in following patches), I don't know ARM, but at least x86 will need this to translate IOVA into PA (in other words, each device can have its own IO address space). If this codes are for common, shall we consider that as well?

Thanks.

-- peterx

Quoted 7 lines · Peter Xu

Quoted 7 lines

lib/pci.c +3 -1

-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
+phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_bar_get(dev, bar_num);
+	uint32_t mask = pci_bar_mask(bar);
+	uint64_t addr = bar & mask;

Quoted 10 lines

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 44

Quoted 8 lines

lib/pci.c +4 -4

-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num))
+		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
+
+	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

I believe, yes.

Unless we always have identity mapping between PCI address space and CPU physical address space I can not realize how could it be done otherwise. But even if we were, I would leave the translation routine for clarity.

Quoted 14 lines

[...]

lib/x86/asm/pci.h static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val · line 43

Quoted 6 lines

lib/x86/asm/pci.h +6 -0

+static inline
+phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
+{
+    return addr;
+}
+

We are not using dev here (and in following patches), I don't know ARM, but at least x86 will need this to translate IOVA into PA (in other words, each device can have its own IO address space). If this codes are for common, shall we consider that as well?

Could this function be extended or the prototype blocks the proper implementation? (I guess, I will get the answer once I look into your works).

Quoted 10 lines

Quoted 3 lines

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 44

Quoted 1 line

lib/pci.c +1 -0

+	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

I believe, yes.

Unless we always have identity mapping between PCI address space and CPU physical address space I can not realize how could it be done otherwise. But even if we were, I would leave the translation routine for clarity.

Sorry I didn't quite catch your point. Are we talking about IOMMU address remapping here? IMHO BAR addresses are from CPU's point of view. It's only used by CPU, not device. In that case, BAR address should not be translated at least by IOMMU (no matter for x86/arm or whatever).

Take Linux as example: pci_ioremap_bar() is responsible to be called for any PCI drivers to map device memory bars into kernel virtual address space. Basically it does:

void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)

{
	struct resource *res = &pdev->resource[bar];
	return ioremap_nocache(res->start, resource_size(res));
}

So as it is written: I believe we don't translate the bar address (which should be res->start). We use it as physical address.

Or, do you mean other kinds of translation that I don't aware of?

Another silly question: I see pci_translate_addr() defined in both lib/x86/asm/pci.h and lib/asm-generic/pci-host-bridge.h. How's that working?

Thanks,

-- peterx

Quoted 30 lines · Peter Xu

Quoted 10 lines

Quoted 3 lines

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 44

Quoted 1 line

lib/pci.c +1 -0

+	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

I believe, yes.

Unless we always have identity mapping between PCI address space and CPU physical address space I can not realize how could it be done otherwise. But even if we were, I would leave the translation routine for clarity.

Sorry I didn't quite catch your point. Are we talking about IOMMU address remapping here? IMHO BAR addresses are from CPU's point of view. It's only used by CPU, not device. In that case, BAR address should not be translated at least by IOMMU (no matter for x86/arm or whatever).

Take Linux as example: pci_ioremap_bar() is responsible to be called for any PCI drivers to map device memory bars into kernel virtual address space. Basically it does:

void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)

{
	struct resource *res = &pdev->resource[bar];
	return ioremap_nocache(res->start, resource_size(res));
}

So as it is written: I believe we don't translate the bar address (which should be res->start). We use it as physical address.

Quoted 1 line

Or, do you mean other kinds of translation that I don't aware of?

Yes, I mean translation from PCI bus address space to CPU physical address space. These two busses are different and hence need a translation. I assume Linux pci_dev::resource[] have translated address, but it is not what PCI devices see. Unless I do not terribly missing somethig, BAR addresses is what a device sees on its AD[0..31] pins.

Quoted 31 lines · Alexander Gordeev

Quoted 30 lines · Peter Xu

Quoted 10 lines

Quoted 3 lines

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 44

Quoted 1 line

lib/pci.c +1 -0

+	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

I believe, yes.

Unless we always have identity mapping between PCI address space and CPU physical address space I can not realize how could it be done otherwise. But even if we were, I would leave the translation routine for clarity.

Sorry I didn't quite catch your point. Are we talking about IOMMU address remapping here? IMHO BAR addresses are from CPU's point of view. It's only used by CPU, not device. In that case, BAR address should not be translated at least by IOMMU (no matter for x86/arm or whatever).

Take Linux as example: pci_ioremap_bar() is responsible to be called for any PCI drivers to map device memory bars into kernel virtual address space. Basically it does:

void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)

{
	struct resource *res = &pdev->resource[bar];
	return ioremap_nocache(res->start, resource_size(res));
}

So as it is written: I believe we don't translate the bar address (which should be res->start). We use it as physical address.

Quoted 8 lines

Quoted 1 line

Or, do you mean other kinds of translation that I don't aware of?

Yes, I mean translation from PCI bus address space to CPU physical address space. These two busses are different and hence need a translation. I assume Linux pci_dev::resource[] have translated address, but it is not what PCI devices see. Unless I do not terribly missing somethig, BAR addresses is what a device sees on its AD[0..31] pins.

I believe pci_dev::resource[] should be assigned by BIOS or something before Linux. At that time, IOMMU is possibly even not inited. So no chance for a translation at all.

Hi Peter,

Quoted 41 lines · Peter Xu

Quoted 31 lines · Alexander Gordeev

Quoted 30 lines · Peter Xu

Quoted 10 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

I believe, yes.

Unless we always have identity mapping between PCI address space and CPU physical address space I can not realize how could it be done otherwise. But even if we were, I would leave the translation routine for clarity.

Sorry I didn't quite catch your point. Are we talking about IOMMU address remapping here? IMHO BAR addresses are from CPU's point of view. It's only used by CPU, not device. In that case, BAR address should not be translated at least by IOMMU (no matter for x86/arm or whatever).

Take Linux as example: pci_ioremap_bar() is responsible to be called for any PCI drivers to map device memory bars into kernel virtual address space. Basically it does:

void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)

{
	struct resource *res = &pdev->resource[bar];
	return ioremap_nocache(res->start, resource_size(res));
}

So as it is written: I believe we don't translate the bar address (which should be res->start). We use it as physical address.

Quoted 8 lines

Quoted 1 line

Or, do you mean other kinds of translation that I don't aware of?

Yes, I mean translation from PCI bus address space to CPU physical address space. These two busses are different and hence need a translation. I assume Linux pci_dev::resource[] have translated address, but it is not what PCI devices see. Unless I do not terribly missing somethig, BAR addresses is what a device sees on its AD[0..31] pins.

Quoted 3 lines

I believe pci_dev::resource[] should be assigned by BIOS or something before Linux. At that time, IOMMU is possibly even not inited. So no chance for a translation at all.

kvm-unit-tests != Linux

kvm-unit-tests/arm doesn't have a bootloader at all (not counting QEMU initializing registers, and a handful of QEMU generated instructions that gives us a kick)

Anyway, I'm glad you're reviewing this series (my PCI skills are minimal), but you'll have to review it in the right context. In this case, more of a seabios context.

Quoted 31 lines · Alexander Gordeev

Quoted 30 lines · Peter Xu

Quoted 10 lines

Quoted 3 lines

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 44

Quoted 1 line

lib/pci.c +1 -0

+	return pci_translate_addr(dev, addr);

Raw question: do we need to translate bar addresses as well?

I believe, yes.

Unless we always have identity mapping between PCI address space and CPU physical address space I can not realize how could it be done otherwise. But even if we were, I would leave the translation routine for clarity.

Sorry I didn't quite catch your point. Are we talking about IOMMU address remapping here? IMHO BAR addresses are from CPU's point of view. It's only used by CPU, not device. In that case, BAR address should not be translated at least by IOMMU (no matter for x86/arm or whatever).

Take Linux as example: pci_ioremap_bar() is responsible to be called for any PCI drivers to map device memory bars into kernel virtual address space. Basically it does:

void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)

{
	struct resource *res = &pdev->resource[bar];
	return ioremap_nocache(res->start, resource_size(res));
}

So as it is written: I believe we don't translate the bar address (which should be res->start). We use it as physical address.

Quoted 8 lines

Quoted 1 line

Or, do you mean other kinds of translation that I don't aware of?

Yes, I mean translation from PCI bus address space to CPU physical address space. These two busses are different and hence need a translation. I assume Linux pci_dev::resource[] have translated address, but it is not what PCI devices see. Unless I do not terribly missing somethig, BAR addresses is what a device sees on its AD[0..31] pins.

I believe pci_dev::resource[] should be assigned by BIOS or something before Linux. At that time, IOMMU is possibly even not inited. So no chance for a translation at all.

If you see PCI Local Bus Spec Rev 3.0, chap 6.2.5.1:

Power-up software needs to build a consistent address map before booting the machine to an operating system. This means it has to determine how much memory is in the system, and how much address space the I/O controllers in the system require. After determining this information, power-up software can map the I/O controllers into reasonable locations and proceed with system boot. In order to do this mapping in a device independent manner, the base registers for this mapping are placed in the predefined header portion of Configuration Space.

BARs for each PCI devices should be pre-allocated during power-up software, and a consistent map is built with the knowledge of existing RAM in the system.

If you boot a VM with/without IOMMU, you'll see that BAR addresses won't change before/after enabling IOMMU.

Thanks,

-- peterx

So pci-host-generic.c is "BIOS or something" in this context. There is no contradiction with the your excerpt. Please note "Figure 1-2: PCI System Block Diagram" in the spec. The CPU and PCI busses are two physically different busses interconnected by a bridge.

A CPU data access to PCI devices initiates data access on a PCI bus, but adresses seen by the CPU and PCI devices do not necessarily match.

So in case of x86 they do (lib/x86/asm/pci.h):

static inline
phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)

{
	return addr;
}

But in case of ARM they might not (lib/asm-generic/pci-host-bridge.h):

static inline
phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)

{
	/*
	 * Assume we only have single PCI host bridge in a system.
	 */
	return pci_host_bridge_get_paddr(addr);
}

Quoted 12 lines

If you see PCI Local Bus Spec Rev 3.0, chap 6.2.5.1:

Power-up software needs to build a consistent address map before booting the machine to an operating system. This means it has to determine how much memory is in the system, and how much address space the I/O controllers in the system require. After determining this information, power-up software can map the I/O controllers into reasonable locations and proceed with system boot. In order to do this mapping in a device independent manner, the base registers for this mapping are placed in the predefined header portion of Configuration Space.

Quoted 3 lines

BARs for each PCI devices should be pre-allocated during power-up software, and a consistent map is built with the knowledge of existing RAM in the system.

Yep, see how pci_probe() / pci_alloc_resource() allocate and assign BARs to each device. It is the case for PPC/ARM but in case of x86 it is BIOS who does it.

Quoted 2 lines

If you boot a VM with/without IOMMU, you'll see that BAR addresses won't change before/after enabling IOMMU.

BAR addresses are in PCI bus address space. It sounds quite logical. (I am not experienced with IOMMU address translation, though).

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

---
 lib/pci-testdev.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h         |   7 ++
 2 files changed, 199 insertions(+)
 create mode 100644 lib/pci-testdev.c

From a quick skim this looks like the same as v6, so you must have just

lib/pci-testdev.c new file +7 -0

+static bool pci_testdev_one(struct pci_test_dev_hdr *test,
+			    int test_nr,
+			    struct pci_testdev_ops *ops)
+{
+	u8 width;
+	u32 count, sig, off;
+	const int nr_writes = 16;

lib/pci-testdev.c new file +13 -0

+int pci_testdev(void)
+{
+	phys_addr_t addr;
+	void __iomem *mem, *io;
+	pcidevaddr_t dev;
+	int nr_tests = 0;
+	bool ret;
+
+	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
+	if (dev == PCIDEVADDR_INVALID) {
+		printf("'pci-testdev' device is not found, "
+		       "check QEMU '-device pci-testdev' parameter\n");
+		return -1;

Some nit-picks inline...

(btw, this looks more like a test case shared by platforms rather than
a library, so shall we move it outside of lib/? I don't know.)

[...]

Quoted 7 lines

lib/pci-testdev.c new file +7 -0

+static bool pci_testdev_one(struct pci_test_dev_hdr *test,
+			    int test_nr,
+			    struct pci_testdev_ops *ops)
+{
+	u8 width;
+	u32 count, sig, off;
+	const int nr_writes = 16;

lib/pci-testdev.c line 114

Quoted 10 lines

lib/pci-testdev.c new file +10 -0

+	int i;
+
+	ops->io_writeb(test_nr, &test->test);
+	count = ops->io_readl(&test->count);
+	if (count != 0)
+		return false;
+
+	width = ops->io_readb(&test->width);
+	if (width != 1 && width != 2 && width != 4)
+		return false;

IIUC we only have 1?

lib/pci-testdev.c line 123

Quoted 9 lines

lib/pci-testdev.c new file +9 -0

+
+	sig = ops->io_readl(&test->data);
+	off = ops->io_readl(&test->offset);
+
+	for (i = 0; i < nr_writes; i++) {
+		switch (width) {
+		case 1: ops->io_writeb(sig, (void *)test + off); break;
+		case 2: ops->io_writew(sig, (void *)test + off); break;
+		case 4: ops->io_writel(sig, (void *)test + off); break;

Here as well. Could I ask why we are handling 2/4?

[...]

lib/pci-testdev.c line 157

Quoted 8 lines

lib/pci-testdev.c new file +8 -0

+static int pci_testdev_all(struct pci_test_dev_hdr *test,
+			   struct pci_testdev_ops *ops)
+{
+	int i;
+
+	for (i = 0;; i++) {
+		if (!pci_testdev_one(test, i, ops))
+			break;

Since we have defined PCI_TESTDEV_NUM_TESTS, shall we use it here to stop the loop rather than depending on a failure code from pci_testdev_one()?

[...]

Quoted 13 lines

lib/pci-testdev.c new file +13 -0

+int pci_testdev(void)
+{
+	phys_addr_t addr;
+	void __iomem *mem, *io;
+	pcidevaddr_t dev;
+	int nr_tests = 0;
+	bool ret;
+
+	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
+	if (dev == PCIDEVADDR_INVALID) {
+		printf("'pci-testdev' device is not found, "
+		       "check QEMU '-device pci-testdev' parameter\n");
+		return -1;

lib/pci-testdev.c line 186

Quoted 10 lines

lib/pci-testdev.c new file +10 -0

+	}
+
+	ret = pci_bar_is_valid(dev, 0) && pci_bar_is_valid(dev, 1);
+	assert(ret);
+
+	addr = pci_bar_get_addr(dev, 0);
+	mem = ioremap(addr, PAGE_SIZE);
+
+	addr = pci_bar_get_addr(dev, 1);
+	io = (void *)(unsigned long)addr;

x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the init part and share it? (Actually there is patch in my local tree for this, but haven't posted :)

Thanks!

-- peterx
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

lib/pci-testdev.c line 114

Quoted 3 lines

lib/pci-testdev.c new file +3 -0

+	width = ops->io_readb(&test->width);
+	if (width != 1 && width != 2 && width != 4)
+		return false;

IIUC we only have 1?

lib/pci-testdev.c line 123

Quoted 9 lines

lib/pci-testdev.c new file +9 -0

+
+	sig = ops->io_readl(&test->data);
+	off = ops->io_readl(&test->offset);
+
+	for (i = 0; i < nr_writes; i++) {
+		switch (width) {
+		case 1: ops->io_writeb(sig, (void *)test + off); break;
+		case 2: ops->io_writew(sig, (void *)test + off); break;
+		case 4: ops->io_writel(sig, (void *)test + off); break;

Here as well. Could I ask why we are handling 2/4?

[...]

lib/pci-testdev.c line 157

Quoted 8 lines

lib/pci-testdev.c new file +8 -0

+static int pci_testdev_all(struct pci_test_dev_hdr *test,
+			   struct pci_testdev_ops *ops)
+{
+	int i;
+
+	for (i = 0;; i++) {
+		if (!pci_testdev_one(test, i, ops))
+			break;

Since we have defined PCI_TESTDEV_NUM_TESTS, shall we use it here to stop the loop rather than depending on a failure code from pci_testdev_one()?

[...]

x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the init part and share it? (Actually there is patch in my local tree for this, but haven't posted :)

Thanks!

-- peterx

Quoted 5 lines · Peter Xu

lib/pci-testdev.c line 114

Quoted 3 lines

lib/pci-testdev.c new file +3 -0

+	width = ops->io_readb(&test->width);
+	if (width != 1 && width != 2 && width != 4)
+		return false;

IIUC we only have 1?

I guess it boils what *have* does mean here.

pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c implements just 1 (yet?).

Quoted 12 lines

lib/pci-testdev.c line 123

Quoted 10 lines

lib/pci-testdev.c new file +9 -0

+
+	sig = ops->io_readl(&test->data);
+	off = ops->io_readl(&test->offset);
+
+	for (i = 0; i < nr_writes; i++) {
+		switch (width) {
+		case 1: ops->io_writeb(sig, (void *)test + off); break;
+		case 2: ops->io_writew(sig, (void *)test + off); break;
+		case 4: ops->io_writel(sig, (void *)test + off); break;

Here as well. Could I ask why we are handling 2/4?

Basically, because x86 had it and this implementation mimics it.

Quoted 3 lines

x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the init part and share it? (Actually there is patch in my local tree for this, but haven't posted :)

Yep, I have x86 enabler and it is very simple. But x86 is just too different to try to generalize and we're not pursuing it right now.

Quoted 7 lines · Alexander Gordeev

Quoted 5 lines · Peter Xu

lib/pci-testdev.c line 114

Quoted 3 lines

lib/pci-testdev.c new file +3 -0

+	width = ops->io_readb(&test->width);
+	if (width != 1 && width != 2 && width != 4)
+		return false;

IIUC we only have 1?

Quoted 4 lines

I guess it boils what *have* does mean here.

pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c implements just 1 (yet?).

Do we have other possible implementations for pci-testdev protocol?

Quoted 11 lines

Quoted 10 lines

lib/pci-testdev.c line 123

Quoted 8 lines

lib/pci-testdev.c new file +8 -0

+	sig = ops->io_readl(&test->data);
+	off = ops->io_readl(&test->offset);
+
+	for (i = 0; i < nr_writes; i++) {
+		switch (width) {
+		case 1: ops->io_writeb(sig, (void *)test + off); break;
+		case 2: ops->io_writew(sig, (void *)test + off); break;
+		case 4: ops->io_writel(sig, (void *)test + off); break;

Here as well. Could I ask why we are handling 2/4?

Quoted 1 line

Basically, because x86 had it and this implementation mimics it.

Yes, actually I didn't notice that before. So I have the same question for vmexit.c. But of course I don't think this question is a blocker for the series.

Quoted 6 lines

Quoted 3 lines

x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the init part and share it? (Actually there is patch in my local tree for this, but haven't posted :)

Yep, I have x86 enabler and it is very simple. But x86 is just too different to try to generalize and we're not pursuing it right now.

Could I ask what's the difficulties? Again this is not a block for sure, so, looking forward to your next version.

Thanks,

-- peterx

Quoted 10 lines · Peter Xu

Quoted 9 lines · Alexander Gordeev

Quoted 5 lines · Peter Xu

lib/pci-testdev.c line 114

Quoted 3 lines

lib/pci-testdev.c new file +3 -0

+	width = ops->io_readb(&test->width);
+	if (width != 1 && width != 2 && width != 4)
+		return false;

IIUC we only have 1?

I guess it boils what *have* does mean here.

Quoted 4 lines

Quoted 2 lines

pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c implements just 1 (yet?).

Do we have other possible implementations for pci-testdev protocol?

I typed answer twice, but realized I do not get the question. :) Could you paraphrase, please?

Quoted 14 lines · Alexander Gordeev

Quoted 13 lines · Peter Xu

Quoted 11 lines · Alexander Gordeev

Quoted 5 lines · Peter Xu

lib/pci-testdev.c line 114

Quoted 3 lines

lib/pci-testdev.c new file +3 -0

+	width = ops->io_readb(&test->width);
+	if (width != 1 && width != 2 && width != 4)
+		return false;

IIUC we only have 1?

I guess it boils what *have* does mean here.

pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c implements just 1 (yet?).

Quoted 4 lines

Quoted 1 line

Do we have other possible implementations for pci-testdev protocol?

I typed answer twice, but realized I do not get the question. :) Could you paraphrase, please?

Sorry for not being clear. I am just wondering whether there is other implementation for pci-testdev besides the one in QEMU. It looks like a special device to test QEMU only.

Quoted 15 lines · Peter Xu

Quoted 14 lines · Alexander Gordeev

Quoted 13 lines · Peter Xu

Quoted 11 lines · Alexander Gordeev

Quoted 5 lines · Peter Xu

Review

Quoted 3 lines

(quoted diff) +3 -0

+	width = ops->io_readb(&test->width);
+	if (width != 1 && width != 2 && width != 4)
+		return false;

IIUC we only have 1?

I guess it boils what *have* does mean here.

pci-testdev protocol allows it to be any, but hw/misc/pci-testdev.c implements just 1 (yet?).

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Do we have other possible implementations for pci-testdev protocol?

I typed answer twice, but realized I do not get the question. :) Could you paraphrase, please?

Sorry for not being clear. I am just wondering whether there is other implementation for pci-testdev besides the one in QEMU. It looks like a special device to test QEMU only.

Yes. pci-testdev (most likely) has never been implemented anywhere other than in QEMU. It was written specifically for kvm-unit-tests. See

http://www.linux-kvm.org/page/KVM-unit-tests#Testdevs

for a synopsis of all testdevs. Please feel free to add more details to the pci-testdev section. Feel free to help maintain the document in any other ways too, of course :-)

Hi Andrew,

This is 7th version of PCI support. Noticeable changes since v5 & v6:
- patch "pci: Factor out pci_bar_get()" moved earlier in the series;
- 'extern' added to public functions in lib/pci.h;
- PCI test added to arm/unittests.cfg;

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

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Alexander Gordeev (13):
pci: Fix coding style in generic PCI files
pci: x86: Rename pci_config_read() to pci_config_readl()
pci: Add 'extern' to public function declarations
pci: x86: Add remaining PCI configuration space accessors
pci: Factor out pci_bar_get()
pci: Rework pci_bar_addr()
pci: Add pci_bar_set_addr()
pci: Add pci_dev_exists()
pci: Add pci_print()
pci: Add generic ECAM host support
arm/arm64: pci: Add PCI bus operation test
pci: Add pci-testdev PCI bus test device arm/arm64: pci: Add pci-testdev PCI device operation test

And now for bad news. I imported your branch and tested it. It works great over TCG, but hits the "No PCI resource found for a device" assert when running with KVM on an AArch64 server. I've only tried on a mustang so far.

Thanks,
drew

Quoted 4 lines · Andrew Jones

And now for bad news. I imported your branch and tested it. It works great over TCG, but hits the "No PCI resource found for a device" assert when running with KVM on an AArch64 server. I've only tried on a mustang so far.

Unlike TCG, KVM on an AArch64 reports pci-testdev's BAR #4 value of 0xC. It denotes to 64 bit prefetch memory, which is absent. What is more, BAR #4 is not implemented in pci-testdev. Only two BARs are expected (qemu hw/misc/pci-testdev.c):

    pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->portio);

So the assert sounds about right and the question is why BAR #4 is there.

I finally had a chance to take a look at this. There's no BAR#4 being exposed from pci-testdev. QEMU is adding a default virtio-net-pci device, which does have a BAR#4. Adjusting the QEMU env like this

export QEMU="$QEMU -nodefaults"

which removes default devices, allows the new unit test to run and pass.

kvm-unit-tests shouldn't be asserting and dying when it's presented a valid virtio-net-pci device though. It should handle the probing correctly, or at least just warn that it doesn't know how to, and then ignore it. Alex, can you consider what might need to be tweaked in this series to do that?

That said, as kvm-unit-tests isn't going to drive non-testdev devices anyway, then just removing default devices is a reasonable thing to do too. I'll submit a patch adding '-nodefaults -nodefconfig' to the arm QEMU command line.

Could I ask what's the status of this series? I see that this is still not in master (nor did I saw any further works from Alex).

Thanks,

-- peterx

Quoted 2 lines · Peter Xu

Could I ask what's the status of this series? I see that this is still not in master (nor did I saw any further works from Alex).

Hi Peter,

Sorry for the long absense.
I am going to post the updated version, hopefully soon.

kvm-unit-tests shouldn't be asserting and dying when it's presented a valid virtio-net-pci device though. It should handle the probing correctly, or at least just warn that it doesn't know how to, and then ignore it. Alex, can you consider what might need to be tweaked in this series to do that?

Thanks,
drew