arm/pci: add PCI bus support

This (and following PCI bus scan) commit is a glimpse of Linux implementation with almost all PCI data structures omitted - none of pci_host_bridge, pci_bus, pci_dev and pci_bus_resource are adopted. While gen_pci is just name- compatible and is basically a root pointer to underlying data describing PCI bus hierarchy in a minimalistic way.

The Device Tree is expected to conform to PCI Bus Binding to Open Firmware with any deviation treated as a fatal errors.

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

---
 arm/pci-test.c               |  21 +++++
 config/config-arm-common.mak |   5 +-
 lib/pci-host-generic.c       | 219 +++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h       |  73 +++++++++++++++
 lib/pci.h                    |   3 +
 5 files changed, 320 insertions(+), 1 deletion(-)
 create mode 100644 arm/pci-test.c
 create mode 100644 lib/pci-host-generic.c
 create mode 100644 lib/pci-host-generic.h

arm/pci-test.c new file +5 -0

new file mode 100644
index 0000000..db7d048
@@ -0,0 +1,21 @@
+/*
+ * PCI bus operation test
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *

arm/pci-test.c new file +3 -0

+
+	return report_summary();
+}

config/config-arm-common.mak +3 -1

index 698555d..06ad346 100644
@@ -11,7 +11,8 @@ endif
 
 tests-common = \
 	$(TEST_DIR)/selftest.flat \
-	$(TEST_DIR)/spinlock-test.flat
+	$(TEST_DIR)/spinlock-test.flat \
+	$(TEST_DIR)/pci-test.flat
 
 all: test_cases
 

@@ -29,6 +30,7 @@ include config/asm-offsets.mak
 
 cflatobjs += lib/alloc.o
 cflatobjs += lib/devicetree.o
+cflatobjs += lib/pci-host-generic.o
 cflatobjs += lib/virtio.o
 cflatobjs += lib/virtio-mmio.o
 cflatobjs += lib/chr-testdev.o

lib/pci-host-generic.c new file +5 -0

new file mode 100644
index 0000000..9bc6642
@@ -0,0 +1,219 @@
+/*
+ * PCI bus operation test
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *

lib/pci-host-generic.c new file +4 -0

+
+static char *addr_space_desc[] = {
+	[PCI_RES_TYPE_CONF]		= "CONF",
+	[PCI_RES_TYPE_IO]		= "IO",

lib/pci-host-generic.c new file +23 -0

+{
+	int i;
+
+	/*
+	 * The PCI binding claims the numerical representation of a PCI
+	 * address consists of three cells, encoded as follows:
+	 *
+	 * phys.hi  cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
+	 * phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
+	 * phys.lo  cell: llllllll llllllll llllllll llllllll
+	 *
+	 * PCI device bus address and flags are encoded into phys.high
+	 * PCI 64 bit address is encoded into phys.mid and phys.low
+	 */
+
+	for (i = 0; i < nr_as; i++, as++) {
+		as->of_flags = from_fdt32(rcells[0]);
+		as->pci_range.start = from_fdt64(rcells[1], rcells[2]);
+
+		if (nr_rcells == 6) {
+			as->cpu_range.start = from_fdt32(rcells[3]);
+			as->cpu_range.size  = from_fdt64(rcells[4], rcells[5]);
+		} else {

lib/pci-host-generic.c new file +15 -0

+{
+	struct pci_addr_space *as = &pci->addr_space[0];
+	int i;
+
+	printf("PCIe start %016llx size %016llx "
+	       "bus %02x bus_max %02x #spaces %d\n\n",
+		pci->cpu_range.start, pci->cpu_range.size,
+		pci->bus, pci->bus_max, pci->nr_addr_spaces);
+
+	for (i = 0; i < pci->nr_addr_spaces; i++, as++) {
+		printf("%s address space:\n"
+		       "CPU: start %016llx size %016llx\n"
+		       "PCI: start %016llx size %016llx\n\n",
+			addr_space_desc[flags_to_type(as->of_flags)],
+			as->cpu_range.start, as->cpu_range.size,

lib/pci-host-generic.c new file +29 -0

+{
+	struct gen_pci *pci;
+	const void *fdt = dt_fdt();
+	const struct fdt_property *prop;
+	struct dt_pbus_reg base;
+	struct dt_device dt_dev;
+	struct dt_bus dt_bus;
+	u32 bus, bus_max;
+	u32 nac, nsc, nac_root, nsc_root;
+	u32 nr_range_cells, nr_addr_spaces;
+	int ret, node, len;
+
+	if (!dt_available()) {
+		printf("No device tree found");
+		return NULL;
+	}
+
+	dt_bus_init_defaults(&dt_bus);
+	dt_device_init(&dt_dev, &dt_bus, NULL);
+
+	node = fdt_path_offset(fdt, "/");
+	assert(node >= 0);
+
+	ret = dt_get_nr_cells(node, &nac_root, &nsc_root);
+	assert(ret == 0);
+	assert(nac_root == 1 || nac_root == 2);
+
+	node = fdt_subnode_offset(fdt, node, "pcie");
+	if (node == -FDT_ERR_NOTFOUND) {

lib/pci-host-generic.c new file +37 -0

+
+	ret = fdt_node_check_compatible(fdt, node, "pci-host-ecam-generic");
+	assert(ret >= 0);
+	if (ret != 0) {
+		printf("PCIe controller is not ECAM compatible");
+		return NULL;
+	}
+
+	dt_device_bind_node(&dt_dev, node);
+	ret = dt_pbus_get_base(&dt_dev, &base);
+	assert(ret == 0);
+
+	prop = fdt_get_property(fdt, node, "bus-range", &len);
+	if (prop == NULL) {
+		assert(len == -FDT_ERR_NOTFOUND);
+		bus		= 0x00;
+		bus_max		= 0xff;
+	} else {
+		fdt32_t *data	= (fdt32_t*)prop->data;
+		bus		= fdt32_to_cpu(data[0]);
+		bus_max		= fdt32_to_cpu(data[1]);
+		assert(bus <= bus_max);
+	}
+	assert(bus_max < base.size / PCI_ECAM_BUS_SIZE);
+
+	ret = dt_get_nr_cells(node, &nac, &nsc);
+	assert(ret == 0);
+	assert(nac == 3 && nsc == 2);
+
+	prop = fdt_get_property(fdt, node, "ranges", &len);
+	assert(prop != NULL);
+
+	nr_range_cells = nac + nsc + nac_root;
+	nr_addr_spaces = (len / 4) / nr_range_cells;
+
+	pci = malloc(sizeof(*pci) +
+		     sizeof(pci->addr_space[0]) * nr_addr_spaces);

lib/pci-host-generic.c new file +1 -0

+}

lib/pci-host-generic.h new file +0 -0

new file mode 100644
index 0000000..f4ae3e4

lib/pci-host-generic.h new file +13 -0

+};
+
+/*
+ * See drivers/pci/host/pci-host-generic.c for the idea of what gen_pci
+ * structure is. Although it brings the very same semantics as Linux,
+ * it completely misses the kernel's data design in a bid to keep it as
+ * simple as possible.
+ */
+struct gen_pci {
+	struct pci_addr_range		cpu_range;
+	int				bus;
+	int				bus_max;
+	int				nr_addr_spaces;

lib/pci-host-generic.h new file +15 -0

+} pci_res_type_t;
+
+/*
+ * The following constants are derived from Linux, see this source:
+ *
+ *         drivers/pci/host/pci-host-generic.c
+ *                 struct gen_pci_cfg_bus_ops::bus_shift
+ *                 int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
+ *
+ * Documentation/devicetree/bindings/pci/host-generic-pci.txt excerpt:
+ *
+ * Configuration Space is assumed to be memory-mapped (as opposed to being
+ * accessed via an ioport) and laid out with a direct correspondence to the
+ * geography of a PCI bus address by concatenating the various components to
+ * form an offset.

lib/pci.h +1 -0

+
 #endif /* PCI_H */

Signature · 1 line

1.8.3.1

Scan bus 0 only and function 0 only on each device.

Operations pci_config_read*/pci_config_write* are just wrappers over read*/write* accessors and only needed to emphasize PCI configuration space access.

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

---
 lib/pci-host-generic.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/pci-host-generic.h | 34 +++++++++++++++++++++++
 2 files changed, 107 insertions(+), 1 deletion(-)

lib/pci-host-generic.c +1 -0

index 9bc6642..fd3bb34 100644
@@ -38,6 +38,18 @@ static pci_res_type_t flags_to_type(u32 of_flags)
 	return ((of_flags & 0x40000000) >> 28) | ((of_flags >> 24) & 0x03);
 }
 
+int find_next_dev(struct gen_pci *pci, int dev)

lib/pci-host-generic.c +5 -0

+
 static u32 from_fdt32(fdt32_t val)
 {
 	return fdt32_to_cpu(val);

@@ -199,20 +211,80 @@ static struct gen_pci *gen_pci_probe(void)
 	return pci;
 }
 
+static void pci_dev_print(void *conf)
+{
+	u16 vendor_id = pci_config_readw(conf, PCI_VENDOR_ID);
+	u16 device_id = pci_config_readw(conf, PCI_DEVICE_ID);

lib/pci-host-generic.c +4 -0

+
+		/* We are only interested in normal PCI devices */
+		if (pci_config_readb(conf, PCI_HEADER_TYPE) !=
+					   PCI_HEADER_TYPE_NORMAL)

lib/pci-host-generic.h +11 -0

+}
+
+static u8 pci_config_readb(const void *conf, int off)
+{
+	return readb(conf + off);
+}
+
+static u16 pci_config_readw(const void *conf, int off)
+{
+	return readw(conf + off);
+}

lib/pci-host-generic.h +2 -0

+
+extern int find_next_dev(struct gen_pci *pci, int dev);
 
 #endif

Signature · 1 line

1.8.3.1

Unlike x86, ARM and PPC kvm-unit-tests do not have a luxury of PCI bus initialized by a BIOS and ready to use at start. Thus, we need allocate and assign resources to all devices.

There is no any sort of resource management for memory and io spaces, since only one-per-BAR allocations are expected between calls to pci_probe() and pci_shutdown(), and no deallocations at all.

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

---
 lib/pci-host-generic.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/pci-host-generic.h |   2 +
 2 files changed, 172 insertions(+), 1 deletion(-)

lib/pci-host-generic.c +14 -0

index fd3bb34..167d0db 100644
@@ -211,6 +211,138 @@ static struct gen_pci *gen_pci_probe(void)
 	return pci;
 }
 
+static void pci_get_bar_addr_size(void *conf, int bar, u32 *addr, u32 *size)
+{
+	int off = PCI_BASE_ADDRESS_0 + (bar * 4);
+
+	*addr = pci_config_readl(conf, off);
+	pci_config_writel(~0, conf, off);
+	*size = pci_config_readl(conf, off);
+	pci_config_writel(*addr, conf, off);
+}
+
+static bool pci_get_bar(void *conf, int bar, pci_res_type_t *type,
+			u64 *addr, u64 *size, bool *is64)
+{
+	u32 addr_low, size_low;

lib/pci-host-generic.c +43 -0

+	pci_get_bar_addr_size(conf, bar, &addr_low, &size_low);
+
+	if (addr_low & PCI_BASE_ADDRESS_SPACE_IO) {
+		mask = PCI_BASE_ADDRESS_IO_MASK;
+		*type = PCI_RES_TYPE_IO;
+		*is64 = false;
+	} else {
+		mask = PCI_BASE_ADDRESS_MEM_MASK;
+		if ((addr_low & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+				PCI_BASE_ADDRESS_MEM_TYPE_64) {
+			if (addr_low & PCI_BASE_ADDRESS_MEM_PREFETCH)
+				*type = PCI_RES_TYPE_PREFMEM64;
+			else
+				*type = PCI_RES_TYPE_MEM64;
+			*is64 = true;
+		} else {
+			if (addr_low & PCI_BASE_ADDRESS_MEM_PREFETCH)
+				*type = PCI_RES_TYPE_PREFMEM32;
+			else
+				*type = PCI_RES_TYPE_MEM32;
+			*is64 = false;
+		}
+	}
+
+	if (*is64) {
+		u32 addr_high, size_high;
+		u64 size64;
+
+		assert(bar < 5);
+		pci_get_bar_addr_size(conf, bar + 1, &addr_high, &size_high);
+
+		size64 = (~((((u64)size_high << 32) | size_low) & mask)) + 1;
+		if (!size64)
+			return false;
+
+		if (size)
+			*size = size64;
+		if (addr)
+			*addr = (((u64)addr_high << 32) | addr_low) & mask;
+	} else {
+		u32 size32;
+
+		size32 = (~(size_low & (u32)mask)) + 1;

lib/pci-host-generic.c +16 -0

+
+static struct pci_addr_space *pci_find_res(struct gen_pci *pci,
+					   pci_res_type_t type)
+{
+	struct pci_addr_space *as = &pci->addr_space[0];
+	int i;
+
+	for (i = 0; i < pci->nr_addr_spaces; i++, as++) {
+		if (flags_to_type(as->of_flags) == type)
+			return as;
+	}
+
+	return NULL;
+}
+
+static phys_addr_t pci_align_res_size(phys_addr_t size, pci_res_type_t type)

lib/pci-host-generic.c +5 -0

+
+static phys_addr_t pci_alloc_res(struct gen_pci *pci,
+				 pci_res_type_t type, u64 size)
+{
+	struct pci_addr_space *res;

lib/pci-host-generic.c +7 -0

+
+	return addr;
+}
+
 static void pci_dev_print(void *conf)
 {
 	u16 vendor_id = pci_config_readw(conf, PCI_VENDOR_ID);

@@ -226,9 +358,29 @@ static void pci_dev_print(void *conf)
 	       progif, class, subcl);
 }
 
+static void pci_dev_bar_print(int bar, pci_res_type_t type,
+			      phys_addr_t addr, u64 size, bool is64)
+{

lib/pci-host-generic.c +7 -0

+
 static int pci_bus_scan(struct gen_pci *pci)
 {
 	int dev;
+	phys_addr_t addr;
+	u64 size;
+	pci_res_type_t type;
+	u32 cmd = PCI_COMMAND_SERR;
+	bool is64;
+	int bar;
 	int nr_dev = 0;
 
 	if (!pci)

lib/pci-host-generic.c +0 -0

 		nr_dev++;
 	}
 

lib/pci-host-generic.h +2 -0

index 1d86b43..0f69d71 100644
@@ -17,6 +17,7 @@ struct pci_addr_range {
 struct pci_addr_space {
 	struct pci_addr_range		cpu_range;
 	struct pci_addr_range		pci_range;
+	phys_addr_t			alloc;
 	u32				of_flags;
 };
 

@@ -71,6 +72,7 @@ typedef enum pci_res_type {
 #define PCI_ECAM_BUS_SIZE	(1 << 20)
 #define PCI_NUM_DEVICES		(PCI_ECAM_BUS_SIZE / (1 << 15))
 #define PCI_ECAM_CONFIG_SIZE	(1 << 12)
+#define PCI_HEADER_TYPE_NORMAL_NUM_BARS	6	/* # of BARs in PCI function */
 
 #define for_each_pci_dev(pci, dev)		\
 	for (dev = find_next_dev(pci, -1);	\

Signature · 1 line

1.8.3.1

These two functions is a prerequisite for the following pci-testdev test.

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

---
 lib/pci-host-generic.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

lib/pci-host-generic.c +0 -0

index 167d0db..9bbf232 100644

lib/pci-host-generic.c +17 -0

+
+	for_each_pci_dev(pci, dev) {
+		void *conf = dev_conf(pci, dev);
+
+		if (vendor_id == pci_config_readw(conf, PCI_VENDOR_ID) &&
+		    device_id == pci_config_readw(conf, PCI_DEVICE_ID))
+			return encode_addr(0, dev, 0);
+	}
+
+	return PCIDEVADDR_INVALID;
+}
+
+unsigned long pci_bar_addr(pcidevaddr_t bdf, int bar)
+{
+	struct gen_pci *pci = get_pci();
+	void *conf;
+	struct pci_addr_space *res;

lib/pci-host-generic.c +1 -0

+
 bool pci_probe(void)
 {
 	struct gen_pci *pci = get_pci();

Signature · 1 line

1.8.3.1

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

---
 arm/pci-test.c               |   4 +
 config/config-arm-common.mak |   1 +
 lib/pci-testdev.c            | 189 +++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h                    |   6 ++
 4 files changed, 200 insertions(+)
 create mode 100644 lib/pci-testdev.c

arm/pci-test.c +0 -0

index db7d048..2ec6156 100644

arm/pci-test.c +1 -0

+
 	pci_shutdown();
 
 	return report_summary();

config/config-arm-common.mak +1 -0

index 06ad346..f2e0ad1 100644
@@ -31,6 +31,7 @@ include config/asm-offsets.mak
 cflatobjs += lib/alloc.o
 cflatobjs += lib/devicetree.o
 cflatobjs += lib/pci-host-generic.o
+cflatobjs += lib/pci-testdev.o
 cflatobjs += lib/virtio.o
 cflatobjs += lib/virtio-mmio.o
 cflatobjs += lib/chr-testdev.o

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

new file mode 100644
index 0000000..1567228
@@ -0,0 +1,189 @@
+/*
+ * QEMU "pci-testdev" PCI test device
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "pci.h"
+#include "asm/io.h"
+
+struct pci_testdev_ops {
+	u8 (*read_byte)(const volatile void *addr);
+	u16 (*read_word)(const volatile void *addr);
+	u32 (*read_long)(const volatile void *addr);
+	void (*write_byte)(u8 value, volatile void *addr);
+	void (*write_word)(u16 value, volatile void *addr);
+	void (*write_long)(u32 value, volatile void *addr);
+};
+
+static u8 ioreadb(const volatile void *addr)
+{
+	return readb(addr);
+}
+
+static u16 ioreadw(const volatile void *addr)
+{
+	return readw(addr);
+}
+
+static u32 ioreadl(const volatile void *addr)
+{
+	return readl(addr);
+}
+
+static void iowriteb(u8 value, volatile void *addr)
+{
+	writeb(value, addr);
+}

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

+
+static u8 memreadb(const volatile void *addr)
+{
+	return *(const volatile u8 __force *)addr;
+}
+
+static u16 memreadw(const volatile void *addr)
+{
+	return *(const volatile u16 __force *)addr;
+}
+
+static u32 memreadl(const volatile void *addr)
+{
+	return *(const volatile u32 __force *)addr;
+}
+
+static void memwriteb(u8 value, volatile void *addr)
+{
+	*(volatile u8 __force *)addr = value;
+}
+
+static void memwritew(u16 value, volatile void *addr)
+{
+	*(volatile u16 __force *)addr = value;
+}
+
+static void memwritel(u32 value, volatile void *addr)
+{
+	*(volatile u32 __force *)addr = value;

lib/pci-testdev.c new file +28 -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;
+	int i;
+
+	ops->write_byte(test_nr, &test->test);
+	count = ops->read_long(&test->count);
+	if (count != 0)
+		return false;
+
+	width = ops->read_byte(&test->width);
+	if ((width != 1) && (width != 2) && (width != 4))
+		return false;
+
+	sig = ops->read_long(&test->data);
+	off = ops->read_long(&test->offset);
+
+	for (i = 0; i < nr_writes; i++) {
+		switch (width) {
+			case 1: ops->write_byte(sig, (void*)test + off); break;
+			case 2: ops->write_word(sig, (void*)test + off); break;
+			case 4: ops->write_long(sig, (void*)test + off); break;
+		}

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

+{
+	bool io = (ops == &pci_testdev_io_ops);
+	int i;
+
+	printf("pci-testdev %3s: ", io ? "io" : "mem");
+	for (i = 0;; ++i) {
+		char c = ops->read_byte(&test->name[i]);
+		if (!c)
+			break;
+		printf("%c", c);
+	}
+	printf("\n");
+
+}
+
+static int pci_testdev_all(struct pci_test_dev_hdr *test,
+			   struct pci_testdev_ops *ops)
+{
+	int i;
+

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

+{
+	unsigned long addr;
+	void __iomem *mem, *io;
+	pcidevaddr_t dev;
+	int nr_tests = 0;
+
+	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
+	if (dev == PCIDEVADDR_INVALID)
+		return -1;
+
+	addr = pci_bar_addr(dev, 1);
+	if (addr == ~0ul)
+		return -1;
+	io = (void*)addr;
+
+	addr = pci_bar_addr(dev, 0);
+	if (addr == ~0ul)
+		return -1;
+	mem = ioremap(addr, 0);
+
+	nr_tests += pci_testdev_all(mem, &pci_testdev_mem_ops);
+	nr_tests += pci_testdev_all(io, &pci_testdev_io_ops);
+
+	return nr_tests;
+}

lib/pci.h +6 -0

index 80d0d04..4a0d305 100644
@@ -27,7 +27,12 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 #define PCI_VENDOR_ID_REDHAT		0x1b36
 #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
 
+/*
+ * pci-testdev supports at least three types of tests (via mmio and
+ * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd
+ */
 #define PCI_TESTDEV_NUM_BARS		2
+#define PCI_TESTDEV_NUM_TESTS		3
 
 struct pci_test_dev_hdr {
 	uint8_t  test;

@@ -41,5 +46,6 @@ struct pci_test_dev_hdr {
 
 extern bool pci_probe(void);
 extern void pci_shutdown(void);
+extern int pci_testdev(void);
 
 #endif /* PCI_H */

Signature · 1 line

1.8.3.1

Should still keep 'arm' out of this summary, because the few lines added to arm/pci-test.c should just be squashed into the ones in the first patch and posted as a separate patch.

Quoted 16 lines · Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

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

---
 arm/pci-test.c               |   4 +
 config/config-arm-common.mak |   1 +
 lib/pci-testdev.c            | 189 +++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h                    |   6 ++
 4 files changed, 200 insertions(+)
 create mode 100644 lib/pci-testdev.c

arm/pci-test.c +0 -0

index db7d048..2ec6156 100644

arm/pci-test.c int main(void) · line 20

Quoted 6 lines

arm/pci-test.c +3 -0

@@ -15,6 +15,10 @@ int main(void)
 	ret = pci_probe();
 	report("PCI device tree probing", ret);
 
+	ret = pci_testdev();
+	report("PCI test device passed %d tests",
+	       ret >= PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS, ret);

Why '>=', aren't we expecting exactly '=='? Wait, I think I see. If more tests are added to QEMU, then this will just work. Right?

Quoted 60 lines

arm/pci-test.c +1 -0

+
 	pci_shutdown();
 
 	return report_summary();

config/config-arm-common.mak +1 -0

index 06ad346..f2e0ad1 100644
@@ -31,6 +31,7 @@ include config/asm-offsets.mak
 cflatobjs += lib/alloc.o
 cflatobjs += lib/devicetree.o
 cflatobjs += lib/pci-host-generic.o
+cflatobjs += lib/pci-testdev.o
 cflatobjs += lib/virtio.o
 cflatobjs += lib/virtio-mmio.o
 cflatobjs += lib/chr-testdev.o

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

new file mode 100644
index 0000000..1567228
@@ -0,0 +1,189 @@
+/*
+ * QEMU "pci-testdev" PCI test device
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "pci.h"
+#include "asm/io.h"
+
+struct pci_testdev_ops {
+	u8 (*read_byte)(const volatile void *addr);
+	u16 (*read_word)(const volatile void *addr);
+	u32 (*read_long)(const volatile void *addr);
+	void (*write_byte)(u8 value, volatile void *addr);
+	void (*write_word)(u16 value, volatile void *addr);
+	void (*write_long)(u32 value, volatile void *addr);
+};
+
+static u8 ioreadb(const volatile void *addr)
+{
+	return readb(addr);
+}
+
+static u16 ioreadw(const volatile void *addr)
+{
+	return readw(addr);
+}
+
+static u32 ioreadl(const volatile void *addr)
+{
+	return readl(addr);
+}
+
+static void iowriteb(u8 value, volatile void *addr)
+{
+	writeb(value, addr);
+}

lib/pci-testdev.c line 48

Quoted 10 lines

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

+
+static void iowritew(u16 value, volatile void *addr)
+{
+	writew(value, addr);
+}
+
+static void iowritel(u32 value, volatile void *addr)
+{
+	writel(value, addr);
+}

You can't put these MMIO reads and writes in here. How would x86 use it? Leave it to the unit test to define read_byte, write_byte, etc. They know how their arch works.

lib/pci-testdev.c line 57

Quoted 9 lines

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

+
+static struct pci_testdev_ops pci_testdev_io_ops = {
+	.read_byte	= ioreadb,
+	.read_word	= ioreadw,
+	.read_long	= ioreadl,
+	.write_byte	= iowriteb,
+	.write_word	= iowritew,
+	.write_long	= iowritel
+};

This should also be left to the unittest to define, and then register with the pci-testdev driver.

Quoted 29 lines

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

+
+static u8 memreadb(const volatile void *addr)
+{
+	return *(const volatile u8 __force *)addr;
+}
+
+static u16 memreadw(const volatile void *addr)
+{
+	return *(const volatile u16 __force *)addr;
+}
+
+static u32 memreadl(const volatile void *addr)
+{
+	return *(const volatile u32 __force *)addr;
+}
+
+static void memwriteb(u8 value, volatile void *addr)
+{
+	*(volatile u8 __force *)addr = value;
+}
+
+static void memwritew(u16 value, volatile void *addr)
+{
+	*(volatile u16 __force *)addr = value;
+}
+
+static void memwritel(u32 value, volatile void *addr)
+{
+	*(volatile u32 __force *)addr = value;

lib/pci-testdev.c line 96

Quoted 10 lines

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

+}
+
+static struct pci_testdev_ops pci_testdev_mem_ops = {
+	.read_byte	= memreadb,
+	.read_word	= memreadw,
+	.read_long	= memreadl,
+	.write_byte	= memwriteb,
+	.write_word	= memwritew,
+	.write_long	= memwritel
+};

These are pretty safe, since they'll most likely be the same across arches, but there's no reason to do it either, since we don't do the I/O ones.

So basically just move most of the above into arm/pci-test.c in the arm patch.

Quoted 28 lines

lib/pci-testdev.c new file +28 -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;
+	int i;
+
+	ops->write_byte(test_nr, &test->test);
+	count = ops->read_long(&test->count);
+	if (count != 0)
+		return false;
+
+	width = ops->read_byte(&test->width);
+	if ((width != 1) && (width != 2) && (width != 4))
+		return false;
+
+	sig = ops->read_long(&test->data);
+	off = ops->read_long(&test->offset);
+
+	for (i = 0; i < nr_writes; i++) {
+		switch (width) {
+			case 1: ops->write_byte(sig, (void*)test + off); break;
+			case 2: ops->write_word(sig, (void*)test + off); break;
+			case 4: ops->write_long(sig, (void*)test + off); break;
+		}

lib/pci-testdev.c line 134

Quoted 10 lines

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

+	}
+
+	if ((int)ops->read_long(&test->count) != nr_writes)
+		return false;
+
+	return true;
+}
+
+void pci_testdev_print(struct pci_test_dev_hdr *test,
+		       struct pci_testdev_ops *ops)

Why no 'static'?

Quoted 20 lines

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

+{
+	bool io = (ops == &pci_testdev_io_ops);
+	int i;
+
+	printf("pci-testdev %3s: ", io ? "io" : "mem");
+	for (i = 0;; ++i) {
+		char c = ops->read_byte(&test->name[i]);
+		if (!c)
+			break;
+		printf("%c", c);
+	}
+	printf("\n");
+
+}
+
+static int pci_testdev_all(struct pci_test_dev_hdr *test,
+			   struct pci_testdev_ops *ops)
+{
+	int i;
+

lib/pci-testdev.c line 164

Quoted 10 lines

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

+	for (i = 0;; i++) {
+		if (!pci_testdev_one(test, i, ops))
+			break;
+		pci_testdev_print(test, ops);
+	}
+
+	return i;
+}
+
+int pci_testdev(void)

So this should take pointers to the two ops structs as arguments.

Quoted 48 lines

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

+{
+	unsigned long addr;
+	void __iomem *mem, *io;
+	pcidevaddr_t dev;
+	int nr_tests = 0;
+
+	dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
+	if (dev == PCIDEVADDR_INVALID)
+		return -1;
+
+	addr = pci_bar_addr(dev, 1);
+	if (addr == ~0ul)
+		return -1;
+	io = (void*)addr;
+
+	addr = pci_bar_addr(dev, 0);
+	if (addr == ~0ul)
+		return -1;
+	mem = ioremap(addr, 0);
+
+	nr_tests += pci_testdev_all(mem, &pci_testdev_mem_ops);
+	nr_tests += pci_testdev_all(io, &pci_testdev_io_ops);
+
+	return nr_tests;
+}

lib/pci.h +6 -0

index 80d0d04..4a0d305 100644
@@ -27,7 +27,12 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 #define PCI_VENDOR_ID_REDHAT		0x1b36
 #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
 
+/*
+ * pci-testdev supports at least three types of tests (via mmio and
+ * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd
+ */
 #define PCI_TESTDEV_NUM_BARS		2
+#define PCI_TESTDEV_NUM_TESTS		3
 
 struct pci_test_dev_hdr {
 	uint8_t  test;

@@ -41,5 +46,6 @@ struct pci_test_dev_hdr {
 
 extern bool pci_probe(void);
 extern void pci_shutdown(void);
+extern int pci_testdev(void);
 

Quoted 4 lines

lib/pci.h +0 -0

 #endif /* PCI_H */

Signature · 1 line

1.8.3.1

This looks pretty good, except for needing to move the MMIO access out.

This series extends the kvm-unit-tests/arm framework to support PCI.