[PATCH v2 0/5] arm/pci: add PCI bus support

[PATCH v2 0/5] arm/pci: add PCI bus support

[Alexander Gordeev]

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

Major changes since v1:
  - PCI probe and shutdown functions turned argument-free;
  - PCI bus is scanned unconditionally at probing;
  - no statements inside asserts;
  - more error messages;
  - several patches merged, reducing the set from 11 to 5;
  - GPL headers added;

Some suggestions and comments were not addressed as I presume
the new version might make the suggested changes unnecessary.

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

Alexander Gordeev (5):
  arm/pci: Probe Open Firmware Device Tree
  arm/pci: Scan PCI root bus for devices
  arm/pci: Allocate and assign memory and io space resources
  arm/pci: Add pci_find_dev() and pci_bar_addr() functions
  arm/pci: pci-testdev PCI device operation test

 arm/pci-test.c               |  25 +++
 config/config-arm-common.mak |   6 +-
 lib/pci-host-generic.c       | 521 +++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h       | 109 +++++++++
 lib/pci-testdev.c            | 189 ++++++++++++++++
 lib/pci.h                    |   9 +
 6 files changed, 858 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
 create mode 100644 lib/pci-testdev.c

-- 
1.8.3.1

[PATCH v2 1/5] arm/pci: Probe Open Firmware Device Tree

[Alexander Gordeev]

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 <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 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

diff --git a/arm/pci-test.c b/arm/pci-test.c
new file mode 100644
index 0000000..db7d048
--- /dev/null
+++ b/arm/pci-test.c
@@ -0,0 +1,21 @@
+/*
+ * PCI bus operation test
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <pci.h>
+
+int main(void)
+{
+	int ret;
+
+	ret = pci_probe();
+	report("PCI device tree probing", ret);
+
+	pci_shutdown();
+
+	return report_summary();
+}
diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
index 698555d..06ad346 100644
--- a/config/config-arm-common.mak
+++ b/config/config-arm-common.mak
@@ -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
@@ -70,3 +72,4 @@ test_cases: $(generated_files) $(tests-common) $(tests)
 
 $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o
 $(TEST_DIR)/spinlock-test.elf: $(cstart.o) $(TEST_DIR)/spinlock-test.o
+$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.o
diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
new file mode 100644
index 0000000..9bc6642
--- /dev/null
+++ b/lib/pci-host-generic.c
@@ -0,0 +1,219 @@
+/*
+ * PCI bus operation test
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+#include "devicetree.h"
+#include "asm/io.h"
+#include "pci.h"
+#include "pci-host-generic.h"
+#include <linux/pci_regs.h>
+
+struct gen_pci *gen_pci = NULL;
+
+static struct gen_pci *get_pci(void)
+{
+	return gen_pci;
+}
+
+static void set_pci(struct gen_pci *pci)
+{
+	gen_pci = pci;
+}
+
+static char *addr_space_desc[] = {
+	[PCI_RES_TYPE_CONF]		= "CONF",
+	[PCI_RES_TYPE_IO]		= "IO",
+	[PCI_RES_TYPE_MEM32]		= "MEM32",
+	[PCI_RES_TYPE_MEM64]		= "MEM64",
+	[PCI_RES_TYPE_PREFMEM32]	= "MEM32/p",
+	[PCI_RES_TYPE_PREFMEM64]	= "MEM64/p"
+};
+
+static pci_res_type_t flags_to_type(u32 of_flags)
+{
+	return ((of_flags & 0x40000000) >> 28) | ((of_flags >> 24) & 0x03);
+}
+
+static u32 from_fdt32(fdt32_t val)
+{
+	return fdt32_to_cpu(val);
+}
+
+static u64 from_fdt64(fdt32_t high, fdt32_t low)
+{
+	return ((u64)fdt32_to_cpu(high) << 32) | fdt32_to_cpu(low);
+}
+
+static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
+				     fdt32_t rcells[], int nr_rcells)
+{
+	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 {
+			as->cpu_range.start = from_fdt64(rcells[3], rcells[4]);
+			as->cpu_range.size  = from_fdt64(rcells[5], rcells[6]);
+		}
+
+		as->pci_range.size = as->cpu_range.size;
+		rcells += nr_rcells;
+	}
+}
+
+static void gen_pci_print(struct gen_pci *pci)
+{
+	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,
+		        as->pci_range.start, as->pci_range.size);
+	}
+}
+
+/*
+ * Probe DT for a generic PCI host controller
+ * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
+ * and function gen_pci_probe() in drivers/pci/host/pci-host-generic.c
+ */
+static struct gen_pci *gen_pci_probe(void)
+{
+	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) {
+		printf("No PCIe controller found");
+		return NULL;
+	}
+	assert(node >= 0);
+
+	prop = fdt_get_property(fdt, node, "device_type", &len);
+	if (!prop || len != 4 || strcmp((char*)prop->data, "pci")) {
+		printf("PCIe controller device_type is not \"pci\"");
+		return NULL;
+	}
+
+	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);
+	assert(pci != NULL);
+
+	pci->cpu_range.start	= base.addr;
+	pci->cpu_range.size	= base.size;
+	pci->bus		= bus;
+	pci->bus_max		= bus_max;
+	pci->nr_addr_spaces	= nr_addr_spaces;
+
+	pci_host_addr_space_init(&pci->addr_space[0], nr_addr_spaces,
+				 (fdt32_t*)prop->data, nr_range_cells);
+	gen_pci_print(pci);
+
+	return pci;
+}
+
+bool pci_probe(void)
+{
+	struct gen_pci *pci = get_pci();
+
+	assert(pci == NULL);
+	pci = gen_pci_probe();
+	set_pci(pci);
+
+	return (pci != NULL);
+}
+
+void pci_shutdown(void)
+{
+	struct gen_pci *pci = get_pci();
+
+	set_pci(NULL);
+	free(pci);
+}
diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
new file mode 100644
index 0000000..f4ae3e4
--- /dev/null
+++ b/lib/pci-host-generic.h
@@ -0,0 +1,73 @@
+#ifndef PCI_HOST_GENERIC_H
+#define PCI_HOST_GENERIC_H
+/*
+ * PCI host bridge supporting structures and constants
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+
+struct pci_addr_range {
+	phys_addr_t			start;
+	phys_addr_t			size;
+};
+
+struct pci_addr_space {
+	struct pci_addr_range		cpu_range;
+	struct pci_addr_range		pci_range;
+	u32				of_flags;
+};
+
+/*
+ * 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;
+	struct pci_addr_space		addr_space[];
+};
+
+typedef enum pci_res_type {
+	PCI_RES_TYPE_CONF		= 0,
+	PCI_RES_TYPE_IO			= 1,
+	PCI_RES_TYPE_MEM32		= 2,
+	PCI_RES_TYPE_MEM64		= 3,
+	PCI_RES_TYPE_PREFMEM32		= 6,
+	PCI_RES_TYPE_PREFMEM64		= 7
+} 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.
+ *
+ * For CAM, this 24-bit offset is:
+ *
+ *         cfg_offset(bus, device, function, register) =
+ *                    bus << 16 | device << 11 | function << 8 | register
+ *
+ * Whilst ECAM extends this by 4 bits to accommodate 4k of function space:
+ *
+ *         cfg_offset(bus, device, function, register) =
+ *                    bus << 20 | device << 15 | function << 12 | register
+ *
+ */
+#define PCI_ECAM_BUS_SIZE	(1 << 20)
+
+#endif
diff --git a/lib/pci.h b/lib/pci.h
index 9160cfb..80d0d04 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -39,4 +39,7 @@ struct pci_test_dev_hdr {
 	uint8_t  name[];
 };
 
+extern bool pci_probe(void);
+extern void pci_shutdown(void);
+
 #endif /* PCI_H */
-- 
1.8.3.1

[PATCH v2 2/5] arm/pci: Scan PCI root bus for devices

[Alexander Gordeev]

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 <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-host-generic.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/pci-host-generic.h | 34 +++++++++++++++++++++++
 2 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 9bc6642..fd3bb34 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -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)
+{
+	for (dev++; dev < PCI_NUM_DEVICES; dev++) {
+		void *conf = dev_conf(pci, dev);
+
+		if (pci_config_readl(conf, PCI_VENDOR_ID) != ((u32)~0))
+			return dev;
+	}
+
+	return -1;
+}
+
 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);
+	u8 header = pci_config_readb(conf, PCI_HEADER_TYPE);
+	u8 progif = pci_config_readb(conf, PCI_CLASS_PROG);
+	u8 subcl = pci_config_readb(conf, PCI_CLASS_DEVICE);
+	u8 class = pci_config_readb(conf, PCI_CLASS_DEVICE + 1);
+
+	printf("conf %p vendor_id %04x device_id %04x type %d "
+	       "progif %02x class %02x subcl %02x\n",
+	       conf, vendor_id, device_id, header,
+	       progif, class, subcl);
+}
+
+static int pci_bus_scan(struct gen_pci *pci)
+{
+	int dev;
+	int nr_dev = 0;
+
+	if (!pci)
+		return -1;
+
+	for_each_pci_dev(pci, dev) {
+		void *conf = dev_conf(pci, dev);
+		pci_dev_print(conf);
+
+		/* We are only interested in normal PCI devices */
+		if (pci_config_readb(conf, PCI_HEADER_TYPE) !=
+					   PCI_HEADER_TYPE_NORMAL)
+			continue;
+
+		pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
+		nr_dev++;
+	}
+
+	return nr_dev;
+}
+
 bool pci_probe(void)
 {
 	struct gen_pci *pci = get_pci();
 
 	assert(pci == NULL);
 	pci = gen_pci_probe();
+	if (!pci)
+		goto probe;
 	set_pci(pci);
 
-	return (pci != NULL);
+	if (pci_bus_scan(pci) < 0)
+		goto scan;
+
+	return true;
+
+scan:
+	pci_shutdown();
+
+probe:
+	return false;
 }
 
 void pci_shutdown(void)
 {
 	struct gen_pci *pci = get_pci();
+	int dev;
+
+	if (!pci)
+		return;
+
+	for_each_pci_dev(pci, dev) {
+		void *conf = dev_conf(pci, dev);
+
+		pci_config_writel(PCI_COMMAND_INTX_DISABLE, conf, PCI_COMMAND);
+	}
 
 	set_pci(NULL);
 	free(pci);
diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
index f4ae3e4..1d86b43 100644
--- a/lib/pci-host-generic.h
+++ b/lib/pci-host-generic.h
@@ -69,5 +69,39 @@ 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 for_each_pci_dev(pci, dev)		\
+	for (dev = find_next_dev(pci, -1);	\
+	     dev >= 0;				\
+	     dev = find_next_dev(pci, dev))
+
+static void* dev_conf(struct gen_pci *pci, int dev)
+{
+	return (void*)pci->cpu_range.start + (dev * 8) * PCI_ECAM_CONFIG_SIZE;
+}
+
+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);
+}
+
+static u32 pci_config_readl(const void *conf, int off)
+{
+	return readl(conf + off);
+}
+
+static void pci_config_writel(u32 val, void *conf, int off)
+{
+	writel(val, conf + off);
+}
+
+extern int find_next_dev(struct gen_pci *pci, int dev);
 
 #endif
-- 
1.8.3.1

[PATCH v2 3/5] arm/pci: Allocate and assign memory and io space resources

[Alexander Gordeev]

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 <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-host-generic.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/pci-host-generic.h |   2 +
 2 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index fd3bb34..167d0db 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -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;
+	u64 mask;
+
+	/*
+	 * To determine the amount of address space needed by a PCI device,
+	 * one must save the original value of the BAR, write a value of
+	 * all 1's to the register, then read it back. The amount of memory
+	 * can then be then determined by masking the information bits,
+	 * performing a bitwise NOT and incrementing the value by 1.
+	 * Use pci_get_bar_addr_size() helper to facilitate that algorithm.
+	 */
+	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;
+		if (!size32)
+			return false;
+
+		if (size)
+			*size = size32;
+		if (addr)
+			*addr = addr_low & mask;
+	}
+
+	return true;
+}
+
+static void pci_set_bar(void *conf, int bar, phys_addr_t addr, bool is64)
+{
+	int off = PCI_BASE_ADDRESS_0 + (bar * 4);
+
+	pci_config_writel(addr, conf, off);
+	if (is64)
+		pci_config_writel(addr >> 32, conf, off + 4);
+}
+
+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)
+{
+	phys_addr_t mask;
+
+	if (type == PCI_RES_TYPE_IO)
+		mask = PCI_BASE_ADDRESS_IO_MASK;
+	else
+		mask = PCI_BASE_ADDRESS_MEM_MASK;
+
+	return (size + ~mask) & mask;
+}
+
+static phys_addr_t pci_alloc_res(struct gen_pci *pci,
+				 pci_res_type_t type, u64 size)
+{
+	struct pci_addr_space *res;
+	phys_addr_t addr;
+
+	res = pci_find_res(pci, type);
+	assert(res != NULL);
+
+	size = pci_align_res_size(size, type);
+	assert(res->alloc + size <= res->pci_range.size);
+
+	addr = res->pci_range.start + res->alloc;
+	res->alloc += size;
+
+	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)
+{
+	const char *desc = addr_space_desc[type];
+
+	if (is64) {
+		printf("\tBAR#%d,%d [%-7s %02x-%02x]\n",
+			bar, bar + 1, desc, addr, addr + size - 1);
+	} else {
+		printf("\tBAR#%d    [%-7s %02x-%02x]\n",
+			bar, desc, addr, addr + size - 1);
+	}
+}
+
 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)
@@ -243,7 +395,24 @@ static int pci_bus_scan(struct gen_pci *pci)
 					   PCI_HEADER_TYPE_NORMAL)
 			continue;
 
-		pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
+		for (bar = 0; bar < PCI_HEADER_TYPE_NORMAL_NUM_BARS; bar++) {
+			if (!pci_get_bar(conf, bar, &type, NULL, &size, &is64))
+				break;
+
+			addr = pci_alloc_res(pci, type, size);
+			pci_set_bar(conf, bar, addr, is64);
+			pci_dev_bar_print(bar, type, addr, size, is64);
+
+			if (is64)
+				bar++;
+
+			if (type == PCI_RES_TYPE_IO)
+				cmd |= PCI_COMMAND_IO;
+			else
+				cmd |= PCI_COMMAND_MEMORY;
+		}
+
+		pci_config_writel(cmd, conf, PCI_COMMAND);
 		nr_dev++;
 	}
 
diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
index 1d86b43..0f69d71 100644
--- a/lib/pci-host-generic.h
+++ b/lib/pci-host-generic.h
@@ -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);	\
-- 
1.8.3.1

[PATCH v2 4/5] arm/pci: Add pci_find_dev() and pci_bar_addr() functions

[Alexander Gordeev]

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

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-host-generic.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 167d0db..9bbf232 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -419,6 +419,67 @@ static int pci_bus_scan(struct gen_pci *pci)
 	return nr_dev;
 }
 
+static pcidevaddr_t encode_addr(int bus, int dev, int fn)
+{
+	assert(bus < 256 && dev < 32 && fn < 8);
+	return bus << 16 | dev << 11 | fn;
+}
+
+static void decode_addr(pcidevaddr_t bdf, int *bus, int *dev, int *fn)
+{
+	*bus = (bdf >> 16) & 0xff;
+	*dev = (bdf >> 11) & 0x1f;
+	*fn  = (bdf >>  8) & 0x03;
+}
+
+pcidevaddr_t pci_find_dev(u16 vendor_id, u16 device_id)
+{
+	struct gen_pci *pci = get_pci();
+	int dev;
+
+	if (!pci)
+		return PCIDEVADDR_INVALID;
+
+	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;
+	pci_res_type_t type;
+	phys_addr_t addr;
+	bool is64;
+	int ret, bus, dev, fn;
+
+	if (!pci)
+		return ~0;
+
+	decode_addr(bdf, &bus, &dev, &fn);
+	assert(!bus && !fn);	/* We support bus 0 and function 0 only */
+
+	conf = dev_conf(pci, dev);
+	ret = pci_config_readb(conf, PCI_HEADER_TYPE);
+	assert(ret == PCI_HEADER_TYPE_NORMAL);
+
+	ret = pci_get_bar(conf, bar, &type, &addr, NULL, &is64);
+	assert(ret);
+
+	res = pci_find_res(pci, type);
+	assert(res);
+
+	return res->cpu_range.start + (addr - res->pci_range.start);
+}
+
 bool pci_probe(void)
 {
 	struct gen_pci *pci = get_pci();
-- 
1.8.3.1

[PATCH v2 5/5] arm/pci: pci-testdev PCI device operation test

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 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

diff --git a/arm/pci-test.c b/arm/pci-test.c
index db7d048..2ec6156 100644
--- a/arm/pci-test.c
+++ b/arm/pci-test.c
@@ -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);
+
 	pci_shutdown();
 
 	return report_summary();
diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
index 06ad346..f2e0ad1 100644
--- a/config/config-arm-common.mak
+++ b/config/config-arm-common.mak
@@ -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
diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
new file mode 100644
index 0000000..1567228
--- /dev/null
+++ b/lib/pci-testdev.c
@@ -0,0 +1,189 @@
+/*
+ * QEMU "pci-testdev" PCI test device
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * 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);
+}
+
+static void iowritew(u16 value, volatile void *addr)
+{
+	writew(value, addr);
+}
+
+static void iowritel(u32 value, volatile void *addr)
+{
+	writel(value, addr);
+}
+
+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
+};
+
+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;
+}
+
+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
+};
+
+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;
+		}
+	}
+
+	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)
+{
+	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;
+
+	for (i = 0;; i++) {
+		if (!pci_testdev_one(test, i, ops))
+			break;
+		pci_testdev_print(test, ops);
+	}
+
+	return i;
+}
+
+int pci_testdev(void)
+{
+	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;
+}
diff --git a/lib/pci.h b/lib/pci.h
index 80d0d04..4a0d305 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -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 */
-- 
1.8.3.1

Re: [PATCH v2 1/5] arm/pci: Probe Open Firmware Device Tree

[Andrew Jones]

Hi Alex,

$SUBJECT (arm/pci: Probe Open Firmware Device Tree) is too broad.
This patch adds DT probing for a specific device, a PCIe host
bridge. Even more specifically, a pci-host-ecam-generic compatible
one. Also, combing arm patches and pci probing patches isn't
necessary. We should add pci probing independently of arm (as it
can be used by other architectures) and then add arm patches that
make use it.

On Thu, Feb 11, 2016 at 09:25:01AM +0100, Alexander Gordeev wrote:
> 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 <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  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
> 
> diff --git a/arm/pci-test.c b/arm/pci-test.c
> new file mode 100644
> index 0000000..db7d048
> --- /dev/null
> +++ b/arm/pci-test.c
> @@ -0,0 +1,21 @@
> +/*
> + * PCI bus operation test
> + *
> + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include <libcflat.h>
> +#include <pci.h>
> +
> +int main(void)
> +{
> +	int ret;
> +
> +	ret = pci_probe();

I think the probe function should return an object. In this case a
"pci" object, which will actually be a member of a pci_host_bridge
object. The pci object will be the object that lib/pci.[ch] functions
target, whereas

struct pci_host_bridge *host = container_of(pci, struct pci_host_bridge, pci)

is the object that is targeted by lib/pci-host-generic.[ch] functions.

I was going to suggest we drop your set_pci/get_pci functions, but in
order to integrate with x86, I guess we should try to avoid the need
for passing around pci object pointers in the lib/pci.[ch] functions.
Let's instead move set_pci/get_pci into lib/pci.h, allowing a global
pci object to be set for those functions to operate on.

> +	report("PCI device tree probing", ret);

The report string shouldn't report messages based on the internals
of the function it calls. If the implementation of pci_probe were to
change, i.e. no longer using a DT, then the message would be wrong.

> +
> +	pci_shutdown();

This call should be implemented in lib/pci.c using pci_config_read,write.
You'll need to add pci_config_write to lib/x86/asm/pci.h too (at least
a stub).

> +
> +	return report_summary();
> +}
> diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
> index 698555d..06ad346 100644
> --- a/config/config-arm-common.mak
> +++ b/config/config-arm-common.mak
> @@ -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
> @@ -70,3 +72,4 @@ test_cases: $(generated_files) $(tests-common) $(tests)
>  
>  $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o
>  $(TEST_DIR)/spinlock-test.elf: $(cstart.o) $(TEST_DIR)/spinlock-test.o
> +$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.o

As mentioned at the top, all the above arm changes should be in a separate
patch.

> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> new file mode 100644
> index 0000000..9bc6642
> --- /dev/null
> +++ b/lib/pci-host-generic.c
> @@ -0,0 +1,219 @@
> +/*
> + * PCI bus operation test
> + *
> + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
> +#include "devicetree.h"
> +#include "asm/io.h"
> +#include "pci.h"
> +#include "pci-host-generic.h"
> +#include <linux/pci_regs.h>
> +
> +struct gen_pci *gen_pci = NULL;

This can be just 'struct pci', or 'pci_gen'. It's a global reference
so it's preferably prefixed by its domain.

> +
> +static struct gen_pci *get_pci(void)
> +{
> +	return gen_pci;
> +}
> +
> +static void set_pci(struct gen_pci *pci)
> +{
> +	gen_pci = pci;
> +}

The above should move to lib/pci.* Also please reverse the object/verb
in the name, i.e. pci_get, pci_set.

> +
> +static char *addr_space_desc[] = {
> +	[PCI_RES_TYPE_CONF]		= "CONF",
> +	[PCI_RES_TYPE_IO]		= "IO",
> +	[PCI_RES_TYPE_MEM32]		= "MEM32",
> +	[PCI_RES_TYPE_MEM64]		= "MEM64",
> +	[PCI_RES_TYPE_PREFMEM32]	= "MEM32/p",
> +	[PCI_RES_TYPE_PREFMEM64]	= "MEM64/p"
> +};
> +
> +static pci_res_type_t flags_to_type(u32 of_flags)
> +{
> +	return ((of_flags & 0x40000000) >> 28) | ((of_flags >> 24) & 0x03);
> +}

The above flags and pci-res-types are DT specific. I'm not sure why you
opted to keep the flags, rather than decode them into fields (as I did
in the starter code I sent you). By decoding them we keep all DT stuff
in a single DT-probe function, rather than letting it ooze out into the
rest of the PCI code.

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

Sorry, but this is pointless, and drops the important 'to_cpu' part of
the function name, allowing us to know we're doing endian conversions,
if necessary.

> +
> +static u64 from_fdt64(fdt32_t high, fdt32_t low)
> +{
> +	return ((u64)fdt32_to_cpu(high) << 32) | fdt32_to_cpu(low);
> +}

This has some more value, but for as little as we use it, I guess
I'd rather not introduce the helper.

> +
> +static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
> +				     fdt32_t rcells[], int nr_rcells)

This function is only called from one place, so why not just embed it at
the callsite? And, without "_dt_" in the name it's another example of DT
leakage into PCI general.

> +{
> +	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 {
> +			as->cpu_range.start = from_fdt64(rcells[3], rcells[4]);
> +			as->cpu_range.size  = from_fdt64(rcells[5], rcells[6]);
> +		}
> +
> +		as->pci_range.size = as->cpu_range.size;
> +		rcells += nr_rcells;
> +	}
> +}
> +
> +static void gen_pci_print(struct gen_pci *pci)

This is a useful function to make public, but needs to be in lib/pci.*,
and it won't need to take a pci object pointer, because there we'll use
the global one set by pci_set()

> +{
> +	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,
> +		        as->pci_range.start, as->pci_range.size);
> +	}
> +}
> +
> +/*
> + * Probe DT for a generic PCI host controller
> + * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
> + * and function gen_pci_probe() in drivers/pci/host/pci-host-generic.c
> + */
> +static struct gen_pci *gen_pci_probe(void)

I mentioned this before. This function should have a "_dt_" in the name.

> +{
> +	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) {
> +		printf("No PCIe controller found");
> +		return NULL;
> +	}
> +	assert(node >= 0);
> +
> +	prop = fdt_get_property(fdt, node, "device_type", &len);
> +	if (!prop || len != 4 || strcmp((char*)prop->data, "pci")) {
> +		printf("PCIe controller device_type is not \"pci\"");
> +		return NULL;
> +	}

The above error-return-null can be an assert. host-generic-pci.txt says
device_type "Must be" "pci".

> +
> +	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);
> +	assert(pci != NULL);
> +
> +	pci->cpu_range.start	= base.addr;
> +	pci->cpu_range.size	= base.size;
> +	pci->bus		= bus;
> +	pci->bus_max		= bus_max;
> +	pci->nr_addr_spaces	= nr_addr_spaces;
> +
> +	pci_host_addr_space_init(&pci->addr_space[0], nr_addr_spaces,
> +				 (fdt32_t*)prop->data, nr_range_cells);

The above are host-bridge properties, so they should go in a
pci_host_bridge object. I guess so far we don't have any pci object
state. Well, except for the bus number, which I guess can be maintained
redundantly. And, maybe the address ranges are pci object properties?

> +	gen_pci_print(pci);

I don't think we should always print on every unittest boot. The
print function should be available for unittest debug though.

> +
> +	return pci;

return &host->pci;

> +}
> +
> +bool pci_probe(void)
> +{
> +	struct gen_pci *pci = get_pci();
> +
> +	assert(pci == NULL);
> +	pci = gen_pci_probe();
> +	set_pci(pci);
> +
> +	return (pci != NULL);

return pci;

> +}
> +
> +void pci_shutdown(void)
> +{
> +	struct gen_pci *pci = get_pci();
> +
> +	set_pci(NULL);
> +	free(pci);

nit: normally free comes before the NULL setting.

> +}
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> new file mode 100644
> index 0000000..f4ae3e4
> --- /dev/null
> +++ b/lib/pci-host-generic.h
> @@ -0,0 +1,73 @@
> +#ifndef PCI_HOST_GENERIC_H
> +#define PCI_HOST_GENERIC_H
> +/*
> + * PCI host bridge supporting structures and constants
> + *
> + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"

This should include "alloc.h" for phys_addr_t. Actually, I'm tempted to
move that typedef to libcflat.h, as we have several files now that only
include alloc.h for the typedef.

> +
> +struct pci_addr_range {
> +	phys_addr_t			start;
> +	phys_addr_t			size;
> +};
> +
> +struct pci_addr_space {
> +	struct pci_addr_range		cpu_range;
> +	struct pci_addr_range		pci_range;
> +	u32				of_flags;

As mentioned above, we don't want OF specific flags in our common pci
data structures.

> +};
> +
> +/*
> + * 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;
> +	struct pci_addr_space		addr_space[];
> +};
> +
> +typedef enum pci_res_type {
> +	PCI_RES_TYPE_CONF		= 0,
> +	PCI_RES_TYPE_IO			= 1,
> +	PCI_RES_TYPE_MEM32		= 2,
> +	PCI_RES_TYPE_MEM64		= 3,
> +	PCI_RES_TYPE_PREFMEM32		= 6,
> +	PCI_RES_TYPE_PREFMEM64		= 7

These values are also OF specific, i.e. 6 and 7 are the merged OF
prefetch and memtype flags. I'd rather not do that. prefetchable
(cacheable) can be a boolean, and aren't there already memtype defines
in linux/pci_regs.h we can use for a type field?

> +} 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.
> + *
> + * For CAM, this 24-bit offset is:
> + *
> + *         cfg_offset(bus, device, function, register) =
> + *                    bus << 16 | device << 11 | function << 8 | register
> + *
> + * Whilst ECAM extends this by 4 bits to accommodate 4k of function space:
> + *
> + *         cfg_offset(bus, device, function, register) =
> + *                    bus << 20 | device << 15 | function << 12 | register

This is a nice comment, but I think it should go above some function that
implements it.

> + *
> + */
> +#define PCI_ECAM_BUS_SIZE	(1 << 20)


How about creating a PCI_ECAM_BUS_SHIFT 20 define instead. I presume
we'll want to use it in a config_offset() function too. Well, we assume
the bus is always zero, so maybe we'd shortcut that, but still...

> +
> +#endif
> diff --git a/lib/pci.h b/lib/pci.h
> index 9160cfb..80d0d04 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -39,4 +39,7 @@ struct pci_test_dev_hdr {
>  	uint8_t  name[];
>  };
>  
> +extern bool pci_probe(void);
> +extern void pci_shutdown(void);

Yes, this is the right place to declare these functions.

> +
>  #endif /* PCI_H */
> -- 
> 1.8.3.1
>

Thanks,
drew 

Re: [PATCH v2 2/5] arm/pci: Scan PCI root bus for devices

[Andrew Jones]

'arm' shouldn't be in the subject of this patch.

On Thu, Feb 11, 2016 at 09:25:02AM +0100, Alexander Gordeev wrote:
> 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 <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/pci-host-generic.h | 34 +++++++++++++++++++++++
>  2 files changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 9bc6642..fd3bb34 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -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)
> +{
> +	for (dev++; dev < PCI_NUM_DEVICES; dev++) {
> +		void *conf = dev_conf(pci, dev);
> +
> +		if (pci_config_readl(conf, PCI_VENDOR_ID) != ((u32)~0))
> +			return dev;
> +	}
> +
> +	return -1;
> +}

find_next_dev, used by for_each_pci_dev, seems a bit crufty to me. Why
not just have

for (i = 0; i < 256; ++i)
   if ([get pci-vendor-id] == 0xffff)
      continue;

everywhere you need to iterate the devices?

> +
>  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);
> +	u8 header = pci_config_readb(conf, PCI_HEADER_TYPE);
> +	u8 progif = pci_config_readb(conf, PCI_CLASS_PROG);
> +	u8 subcl = pci_config_readb(conf, PCI_CLASS_DEVICE);
> +	u8 class = pci_config_readb(conf, PCI_CLASS_DEVICE + 1);
> +
> +	printf("conf %p vendor_id %04x device_id %04x type %d "
> +	       "progif %02x class %02x subcl %02x\n",
> +	       conf, vendor_id, device_id, header,
> +	       progif, class, subcl);
> +}

This should also be in pci.c, and globally accessible for unittests.
It also needs to be written to use the global 'struct pci *pci'
object.

> +
> +static int pci_bus_scan(struct gen_pci *pci)
> +{
> +	int dev;
> +	int nr_dev = 0;
> +
> +	if (!pci)
> +		return -1;

This check is unnecessary, as this is a private function. We don't
have any other error conditions, so this should be a void function.

> +
> +	for_each_pci_dev(pci, dev) {
> +		void *conf = dev_conf(pci, dev);
> +		pci_dev_print(conf);

We shouldn't print on every unittest boot. But providing the means to
print is good. Perhaps we need a compile-time flag providing a verbose
mode?

> +
> +		/* We are only interested in normal PCI devices */
> +		if (pci_config_readb(conf, PCI_HEADER_TYPE) !=
> +					   PCI_HEADER_TYPE_NORMAL)
> +			continue;
> +
> +		pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
> +		nr_dev++;
> +	}
> +
> +	return nr_dev;
> +}
> +
>  bool pci_probe(void)

This should return the pci pointer.

>  {
>  	struct gen_pci *pci = get_pci();

Don't need the get_pci() call, we have access to the private
pointer directly.

>  
>  	assert(pci == NULL);
>  	pci = gen_pci_probe();
> +	if (!pci)
> +		goto probe;

return NULL;

>  	set_pci(pci);
>  
> -	return (pci != NULL);
> +	if (pci_bus_scan(pci) < 0)
> +		goto scan;

This is a void function, no need for goto.

return pci;

No need for labels below.
> +
> +	return true;
> +
> +scan:
> +	pci_shutdown();
> +
> +probe:
> +	return false;
>  }
>  
>  void pci_shutdown(void)
>  {
>  	struct gen_pci *pci = get_pci();

get_pci not needed.

> +	int dev;
> +
> +	if (!pci)
> +		return;
> +
> +	for_each_pci_dev(pci, dev) {
> +		void *conf = dev_conf(pci, dev);
> +
> +		pci_config_writel(PCI_COMMAND_INTX_DISABLE, conf, PCI_COMMAND);
> +	}

Should be written in pci.c using pci_config_read/write functions.

>  
>  	set_pci(NULL);
>  	free(pci);
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> index f4ae3e4..1d86b43 100644
> --- a/lib/pci-host-generic.h
> +++ b/lib/pci-host-generic.h
> @@ -69,5 +69,39 @@ typedef enum pci_res_type {
>   *
>   */
>  #define PCI_ECAM_BUS_SIZE	(1 << 20)
> +#define PCI_NUM_DEVICES		(PCI_ECAM_BUS_SIZE / (1 << 15))

Weird way to write 32 :-)

> +#define PCI_ECAM_CONFIG_SIZE	(1 << 12)
> +
> +#define for_each_pci_dev(pci, dev)		\
> +	for (dev = find_next_dev(pci, -1);	\
> +	     dev >= 0;				\
> +	     dev = find_next_dev(pci, dev))
> +
> +static void* dev_conf(struct gen_pci *pci, int dev)

This function needs pci_ prefixing it, and I would name it 
something like pci_get_config

> +{
> +	return (void*)pci->cpu_range.start + (dev * 8) * PCI_ECAM_CONFIG_SIZE;

Wait. This should be the implementation of that nice comment about
cfg_offset, right? Shouldn't it be

static void *pci_get_conf(struct gen_pci *pci, int dev, int register)
{
  pci_host_bridge *host = container_of(pci, struct pci_host_bridge, pci);
  unsigned offset = /* bus << 20 | */ dev << 15 | /* function << 12 | */ register;
  return host->cpu_range.start + offset;
}

Of course (dev * 8 * 4096) results in the same thing (not including
register) as (dev << 15)...

(I actually would rather not have the extra cpu_range part in the
 pci_host_bridge either. I don't think it gains much in readability,
 to just doing host->addr, or some such.)

> +}
> +
> +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);
> +}
> +
> +static u32 pci_config_readl(const void *conf, int off)
> +{
> +	return readl(conf + off);
> +}
> +
> +static void pci_config_writel(u32 val, void *conf, int off)
> +{
> +	writel(val, conf + off);
> +}

There's no point to these wrappers. We need a way to get the config
address, like "pci_get_config", but then we can just use readl/writel
directly from within lib/pci-host-generic.c. lib/pci.c should always
us something like pci_config_read(), which is implemented in the arch-
specific header lib/<ARCH>/asm/pci.h, and thus can be something that
uses readl/writel directly too when we're an arch using pci-host-bridge.

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

Thanks,
drew 

Re: [PATCH v2 3/5] arm/pci: Allocate and assign memory and io space resources

[Andrew Jones]

Again. arm has nothing to do with this patch, it shouldn't be in
the summary.

On Thu, Feb 11, 2016 at 09:25:03AM +0100, Alexander Gordeev wrote:
> 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 <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/pci-host-generic.h |   2 +
>  2 files changed, 172 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index fd3bb34..167d0db 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -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;
> +	u64 mask;
> +
> +	/*
> +	 * To determine the amount of address space needed by a PCI device,
> +	 * one must save the original value of the BAR, write a value of
> +	 * all 1's to the register, then read it back. The amount of memory
> +	 * can then be then determined by masking the information bits,
> +	 * performing a bitwise NOT and incrementing the value by 1.
> +	 * Use pci_get_bar_addr_size() helper to facilitate that algorithm.
> +	 */

I think this comment would be better up where the function is defined.

> +	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;
> +		if (!size32)
> +			return false;
> +
> +		if (size)
> +			*size = size32;
> +		if (addr)
> +			*addr = addr_low & mask;
> +	}
> +
> +	return true;

I think this function could use some refactoring - lots of if-elses. Not
conflating the prefetch flag and the memtype would probably help.

> +}
> +
> +static void pci_set_bar(void *conf, int bar, phys_addr_t addr, bool is64)
> +{
> +	int off = PCI_BASE_ADDRESS_0 + (bar * 4);
> +
> +	pci_config_writel(addr, conf, off);
> +	if (is64)
> +		pci_config_writel(addr >> 32, conf, off + 4);
> +}

I'd rather we don't introduce small helper functions like this one for
just one use.

> +
> +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)
> +{
> +	phys_addr_t mask;
> +
> +	if (type == PCI_RES_TYPE_IO)
> +		mask = PCI_BASE_ADDRESS_IO_MASK;
> +	else
> +		mask = PCI_BASE_ADDRESS_MEM_MASK;
> +
> +	return (size + ~mask) & mask;
> +}

Another unnecessary (IMO) helper function.

> +
> +static phys_addr_t pci_alloc_res(struct gen_pci *pci,
> +				 pci_res_type_t type, u64 size)
> +{
> +	struct pci_addr_space *res;
> +	phys_addr_t addr;
> +
> +	res = pci_find_res(pci, type);
> +	assert(res != NULL);
> +
> +	size = pci_align_res_size(size, type);
> +	assert(res->alloc + size <= res->pci_range.size);
> +
> +	addr = res->pci_range.start + res->alloc;
> +	res->alloc += size;

I don't really like the name 'alloc'. It's also the free addresses
base. So how about 'free', or 'base'?

> +
> +	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)
> +{
> +	const char *desc = addr_space_desc[type];
> +
> +	if (is64) {
> +		printf("\tBAR#%d,%d [%-7s %02x-%02x]\n",
> +			bar, bar + 1, desc, addr, addr + size - 1);
> +	} else {
> +		printf("\tBAR#%d    [%-7s %02x-%02x]\n",
> +			bar, desc, addr, addr + size - 1);
> +	}
> +}

Another useful print routine that should be made public and put in
lib/pci.c

> +
>  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)
> @@ -243,7 +395,24 @@ static int pci_bus_scan(struct gen_pci *pci)
>  					   PCI_HEADER_TYPE_NORMAL)
>  			continue;
>  
> -		pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
> +		for (bar = 0; bar < PCI_HEADER_TYPE_NORMAL_NUM_BARS; bar++) {

Do we really need a define (with a long name) for normal-num-bars?

> +			if (!pci_get_bar(conf, bar, &type, NULL, &size, &is64))
> +				break;
> +
> +			addr = pci_alloc_res(pci, type, size);
> +			pci_set_bar(conf, bar, addr, is64);
> +			pci_dev_bar_print(bar, type, addr, size, is64);

Don't print on each unittest boot.

> +
> +			if (is64)
> +				bar++;
> +
> +			if (type == PCI_RES_TYPE_IO)
> +				cmd |= PCI_COMMAND_IO;
> +			else
> +				cmd |= PCI_COMMAND_MEMORY;

Hmm... you're setting cmd here, but then you loop again before you use
it, so it may get changed. I think you should have the writel's directly
in here.

> +		}
> +
> +		pci_config_writel(cmd, conf, PCI_COMMAND);

I'm not sure what you want to do here. Do we really need the
PCI_COMMAND_SERR at all? You weren't doing it for non-normal,
and now it's not happening for normal either.

>  		nr_dev++;
>  	}
>  
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> index 1d86b43..0f69d71 100644
> --- a/lib/pci-host-generic.h
> +++ b/lib/pci-host-generic.h
> @@ -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);	\
> -- 
> 1.8.3.1

Thanks,
drew

Re: [PATCH v2 4/5] arm/pci: Add pci_find_dev() and pci_bar_addr() functions

[Andrew Jones]

No 'arm' in subject please.

On Thu, Feb 11, 2016 at 09:25:04AM +0100, Alexander Gordeev wrote:
> These two functions is a prerequisite for the following
> pci-testdev test.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 167d0db..9bbf232 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -419,6 +419,67 @@ static int pci_bus_scan(struct gen_pci *pci)
>  	return nr_dev;
>  }
>  
> +static pcidevaddr_t encode_addr(int bus, int dev, int fn)
> +{
> +	assert(bus < 256 && dev < 32 && fn < 8);
> +	return bus << 16 | dev << 11 | fn;

Wait. This is for CAM, not ECAM (and it's missing the fn shift).
ECAM should be

        return bus << 20 | dev << 15 | fn << 12 | register;

> +}
> +
> +static void decode_addr(pcidevaddr_t bdf, int *bus, int *dev, int *fn)
> +{
> +	*bus = (bdf >> 16) & 0xff;
> +	*dev = (bdf >> 11) & 0x1f;
> +	*fn  = (bdf >>  8) & 0x03;

Here you have the fn shift, but this is still CAM. And what about
register?

> +}
> +
> +pcidevaddr_t pci_find_dev(u16 vendor_id, u16 device_id)

We have this function already in pci.c. We need to implement a
new pci_config_read, rather than a new pci_find_dev.

ARM's and PowerPC's pci_config_read will call into a pci-host-generic
config function that will use pci_get() to get the pci object, which
will lead to the pci-host-bridge object, which will be used by readl.

> +{
> +	struct gen_pci *pci = get_pci();
> +	int dev;
> +
> +	if (!pci)
> +		return PCIDEVADDR_INVALID;

Not that this function should be here anyway, but in cases like this
I think an error message + abort() is better. The unittest shouldn't
be allowed to continue if it tries to use something like pci_find_dev
without running pci_probe first.

> +
> +	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;
> +	pci_res_type_t type;
> +	phys_addr_t addr;
> +	bool is64;
> +	int ret, bus, dev, fn;
> +
> +	if (!pci)
> +		return ~0;
> +
> +	decode_addr(bdf, &bus, &dev, &fn);
> +	assert(!bus && !fn);	/* We support bus 0 and function 0 only */

Why take a 'bdf' instead of just a dev if only bar=0 and fn=0 are supported.

> +
> +	conf = dev_conf(pci, dev);
> +	ret = pci_config_readb(conf, PCI_HEADER_TYPE);
> +	assert(ret == PCI_HEADER_TYPE_NORMAL);
> +
> +	ret = pci_get_bar(conf, bar, &type, &addr, NULL, &is64);
> +	assert(ret);
> +
> +	res = pci_find_res(pci, type);
> +	assert(res);
> +
> +	return res->cpu_range.start + (addr - res->pci_range.start);
> +}

I don't understand everything here. So this is where the translation
happens, I take it. Is it normal to have to search the resources for
the type? The pci_bar_addr function we already have in pci.c checks
the BAR with PCI_BASE_ADDRESS_SPACE_IO and knows what type it is. With
the translation going on, it does seem like it'd be hard to merge with
what x86 is using. However, I can't help but think something is wrong
if they're so different, or is this a PCI vs. PCIe thing that requires
a new function?

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

Thanks,
drew

Re: [PATCH v2 5/5] arm/pci: pci-testdev PCI device operation test

[Andrew Jones]

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.

On Thu, Feb 11, 2016 at 09:25:05AM +0100, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  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
> 
> diff --git a/arm/pci-test.c b/arm/pci-test.c
> index db7d048..2ec6156 100644
> --- a/arm/pci-test.c
> +++ b/arm/pci-test.c
> @@ -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?

> +
>  	pci_shutdown();
>  
>  	return report_summary();
> diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
> index 06ad346..f2e0ad1 100644
> --- a/config/config-arm-common.mak
> +++ b/config/config-arm-common.mak
> @@ -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
> diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> new file mode 100644
> index 0000000..1567228
> --- /dev/null
> +++ b/lib/pci-testdev.c
> @@ -0,0 +1,189 @@
> +/*
> + * QEMU "pci-testdev" PCI test device
> + *
> + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
> + *
> + * 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);
> +}
> +
> +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.

> +
> +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.

> +
> +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;
> +}
> +
> +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.

> +
> +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;
> +		}
> +	}
> +
> +	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'?

> +{
> +	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;
> +
> +	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.

> +{
> +	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;
> +}
> diff --git a/lib/pci.h b/lib/pci.h
> index 80d0d04..4a0d305 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -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 */
> -- 
> 1.8.3.1
>

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

Thanks,
drew

Re: [PATCH v2 5/5] arm/pci: pci-testdev PCI device operation test

[Andrew Jones]

On Mon, Feb 15, 2016 at 09:58:37PM +0100, Andrew Jones wrote:
> 
> 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.
> 
> On Thu, Feb 11, 2016 at 09:25:05AM +0100, Alexander Gordeev wrote:
> > Cc: Thomas Huth <thuth@redhat.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  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
> > 
> > diff --git a/arm/pci-test.c b/arm/pci-test.c
> > index db7d048..2ec6156 100644
> > --- a/arm/pci-test.c
> > +++ b/arm/pci-test.c
> > @@ -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?
> 
> > +
> >  	pci_shutdown();
> >  
> >  	return report_summary();
> > diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
> > index 06ad346..f2e0ad1 100644
> > --- a/config/config-arm-common.mak
> > +++ b/config/config-arm-common.mak
> > @@ -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
> > diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> > new file mode 100644
> > index 0000000..1567228
> > --- /dev/null
> > +++ b/lib/pci-testdev.c
> > @@ -0,0 +1,189 @@
> > +/*
> > + * QEMU "pci-testdev" PCI test device
> > + *
> > + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
> > + *
> > + * 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);
> > +}
> > +
> > +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.
> 
> > +
> > +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.
> 
> > +
> > +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;
> > +}
> > +
> > +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.
> 
> > +
> > +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;
> > +		}
> > +	}
> > +
> > +	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'?
> 
> > +{
> > +	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;
> > +
> > +	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.
> 
> > +{
> > +	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;
> > +}
> > diff --git a/lib/pci.h b/lib/pci.h
> > index 80d0d04..4a0d305 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -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 */
> > -- 
> > 1.8.3.1
> >
> 
> This looks pretty good, except for needing to move the MMIO access out.

Wait. Where's the arm/run script changes needed? They should be in the
arm patch of course, but since this version of the series doesn't have
an arm patch, I'd expect them here. We need something like

pci_testdev=
if $qemu $M -device '?' 2>&1 |  grep pci-testdev > /dev/null; then
       pci_testdev="-device pci-testdev"
fi
...
command="$qemu $M -cpu $processor $chr_testdev $pci_testdev"

in arm/run.

Re: [PATCH v2 0/5] arm/pci: add PCI bus support

[Andrew Jones]

On Thu, Feb 11, 2016 at 09:25:00AM +0100, Alexander Gordeev wrote:
> This series extends the kvm-unit-tests/arm framework to support PCI.

Hi Alex,

Thanks for continuing to work on getting this PCI support into kvm-
unit-tests. I look forward to your next version.

drew