[kvm-unit-tests PATCH 00/11] pci/arm: add PCI bus support

[kvm-unit-tests PATCH 00/11] pci/arm: add PCI bus support

[Alexander Gordeev]

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

Cc: Andrew Jones <drjones@redhat.com>

Alexander Gordeev (11):
  arm/pci: Device tree PCI probing
  arm/pci: PCI bus scanning
  arm/pci: Read devices BARs
  arm/pci: Allocate and assign memory/io space resources
  arm/pci: Add pci_find_dev() and pci_bar_addr() functions
  arm/pci: PCI testdev existence test
  arm/pci: PCI device operation test
  arm/pci: PCI device read/write test
  arm/pci: PCI host bridge info printing
  arm/pci: PCI devices basic info printing
  arm/pci: PCI testdev test flavour printing

 arm/pci-test.c               |  32 +++
 config/config-arm-common.mak |   6 +-
 lib/alloc.c                  |   3 -
 lib/libcflat.h               |   3 +
 lib/pci-host-generic.c       | 502 +++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h       |  37 ++++
 lib/pci-testdev.c            | 198 +++++++++++++++++
 lib/pci.h                    |  33 +++
 8 files changed, 810 insertions(+), 4 deletions(-)
 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
 create mode 100644 lib/pci.h

-- 
1.8.3.1

[kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing

[Alexander Gordeev]

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arm/pci-test.c               |  25 +++++++
 config/config-arm-common.mak |   5 +-
 lib/alloc.c                  |   3 -
 lib/libcflat.h               |   3 +
 lib/pci-host-generic.c       | 155 +++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h       |  26 ++++++++
 lib/pci.h                    |  13 ++++
 7 files changed, 226 insertions(+), 4 deletions(-)
 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.h

diff --git a/arm/pci-test.c b/arm/pci-test.c
new file mode 100644
index 0000000..8629c89
--- /dev/null
+++ b/arm/pci-test.c
@@ -0,0 +1,25 @@
+/*
+ * PCI bus operation test
+ *
+ * Copyright (C) 2015, Red Hat Inc, Andrew Jones <drjones@redhat.com>
+ * Copyright (C) 2015, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <pci.h>
+
+int main(void)
+{
+	struct pci *pci;
+
+	pci = pci_dt_probe();
+
+	report("PCI device tree probing", pci);
+	if (!pci)
+		goto done;
+
+	pci_shutdown(pci);
+
+done:
+	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/alloc.c b/lib/alloc.c
index ad67614..2f60145 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -7,9 +7,6 @@
 #include "asm/spinlock.h"
 #include "asm/io.h"
 
-#define MIN(a, b)		((a) < (b) ? (a) : (b))
-#define MAX(a, b)		((a) > (b) ? (a) : (b))
-
 #define PHYS_ALLOC_NR_REGIONS	256
 
 struct phys_alloc_region {
diff --git a/lib/libcflat.h b/lib/libcflat.h
index 9747ccd..2cb167c 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -76,4 +76,7 @@ do {									\
 		abort();						\
 } while (0)
 
+#define MIN(a, b)		((a) < (b) ? (a) : (b))
+#define MAX(a, b)		((a) > (b) ? (a) : (b))
+
 #endif
diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
new file mode 100644
index 0000000..62b5662
--- /dev/null
+++ b/lib/pci-host-generic.c
@@ -0,0 +1,155 @@
+/*
+ * Adapated from
+ *   drivers/pci/host/pci-host-generic.c
+ *
+ * Copyright (C) 2015, Red Hat Inc, Andrew Jones <drjones@redhat.com>
+ * 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>
+
+static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
+				     u32 cells[], int nr_range_cells)
+{
+	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 = fdt32_to_cpu(cells[0]);
+		as->pci_range.start = fdt64_to_cpu(*((fdt64_t*)&cells[1]));
+
+		if (nr_range_cells == 6) {
+			as->cpu_range.start = fdt32_to_cpu(cells[3]);
+			as->cpu_range.size =
+				fdt64_to_cpu(*((fdt64_t*)&cells[4]));
+		} else {
+			as->cpu_range.start =
+				fdt64_to_cpu(*((fdt64_t*)&cells[3]));;
+			as->cpu_range.size =
+				fdt64_to_cpu(*((fdt64_t*)&cells[5]));
+		}
+
+		as->pci_range.size = as->cpu_range.size;
+
+		cells += nr_range_cells;
+	}
+}
+
+/*
+ * Probe DT for a generic PCI host controller
+ * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
+ */
+static struct pci_host_bridge *pci_host_bridge_probe(void)
+{
+	struct pci_host_bridge *host;
+	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())
+		return NULL;
+
+	dt_bus_init_defaults(&dt_bus);
+	dt_device_init(&dt_dev, &dt_bus, NULL);
+
+	node = fdt_path_offset(fdt, "/");
+	assert(node >= 0);
+
+	assert(dt_get_nr_cells(node, &nac_root, &nsc_root) == 0);
+	assert(nac_root == 1 || nac_root == 2);
+
+	node = fdt_subnode_offset(fdt, node, "pcie");
+	if (node == -FDT_ERR_NOTFOUND)
+		return NULL;
+	assert(node >= 0);
+
+	prop = fdt_get_property(fdt, node, "device_type", &len);
+	if (!prop || strcmp((char*)prop->data, "pci"))
+		return NULL;
+
+	ret = fdt_node_check_compatible(fdt, node, "pci-host-ecam-generic");
+	assert(ret >= 0);
+	if (ret != 0)
+		return NULL;
+
+	dt_device_bind_node(&dt_dev, node);
+	assert(dt_pbus_get_base(&dt_dev, &base) == 0);
+
+	prop = fdt_get_property(fdt, node, "bus-range", &len);
+	if (prop == NULL) {
+		assert(len != 0);
+		bus		= 0x00;
+		bus_max		= 0xff;
+	} else {
+		u32 *data	= (u32*)prop->data;
+		bus		= fdt32_to_cpu(data[0]);
+		bus_max		= fdt32_to_cpu(data[1]);
+	}
+	bus_max = MIN(bus_max, (base.size / PCI_ECAM_BUS_SIZE) - 1);
+
+	assert(dt_get_nr_cells(node, &nac, &nsc) == 0);
+	assert(nac == 3 && nsc == 2);
+
+	assert(prop = fdt_get_property(fdt, node, "ranges", &len));
+
+	nr_range_cells = nac + nsc + nac_root;
+	nr_addr_spaces = (len / 4) / nr_range_cells;
+
+	assert(host = malloc(sizeof(*host) +
+			     sizeof(host->addr_space[0]) * nr_addr_spaces));
+
+	host->cpu_range.start	= base.addr;
+	host->cpu_range.size	= base.size;
+	host->bus		= bus;
+	host->bus_max		= bus_max;
+	host->nr_addr_spaces	= nr_addr_spaces;
+
+	pci_host_addr_space_init(&host->addr_space[0], nr_addr_spaces,
+				 (u32*)prop->data, nr_range_cells);
+
+	return host;
+}
+
+struct pci *pci_dt_probe(void)
+{
+	struct pci_host_bridge *host;
+	struct pci *pci;
+
+	host = pci_host_bridge_probe();
+	if (!host)
+		return NULL;
+
+	assert(pci = calloc(1, sizeof(*pci)));
+	pci->sysdata = host;
+
+	return pci;
+}
+
+void pci_shutdown(struct pci *pci)
+{
+	free(pci->sysdata);
+	free(pci);
+}
diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
new file mode 100644
index 0000000..097ac2d
--- /dev/null
+++ b/lib/pci-host-generic.h
@@ -0,0 +1,26 @@
+#ifndef PCI_HOST_GENERIC_H
+#define PCI_HOST_GENERIC_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;
+	phys_addr_t			alloc;
+	u32				of_flags;
+};
+
+struct pci_host_bridge {
+	struct pci_addr_range		cpu_range;
+	int				bus;
+	int				bus_max;
+	int				nr_addr_spaces;
+	struct pci_addr_space		addr_space[];
+};
+
+#define PCI_ECAM_BUS_SIZE		(1 << 20)
+
+#endif
diff --git a/lib/pci.h b/lib/pci.h
new file mode 100644
index 0000000..22b8e31
--- /dev/null
+++ b/lib/pci.h
@@ -0,0 +1,13 @@
+#ifndef PCI_H
+#define PCI_H
+
+#include "alloc.h"
+
+struct pci {
+	void *sysdata;
+};
+
+extern struct pci *pci_dt_probe(void);
+extern void pci_shutdown(struct pci *pci);
+
+#endif
-- 
1.8.3.1

[kvm-unit-tests PATCH 02/11] arm/pci: PCI bus scanning

[Alexander Gordeev]

Scan bus 0 and function 0 only for now

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arm/pci-test.c         |  4 +++
 lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h |  1 +
 lib/pci.h              |  1 +
 4 files changed, 73 insertions(+)

diff --git a/arm/pci-test.c b/arm/pci-test.c
index 8629c89..1539d3e 100644
--- a/arm/pci-test.c
+++ b/arm/pci-test.c
@@ -11,6 +11,7 @@
 int main(void)
 {
 	struct pci *pci;
+	int ret;
 
 	pci = pci_dt_probe();
 
@@ -18,6 +19,9 @@ int main(void)
 	if (!pci)
 		goto done;
 
+	ret = pci_bus_scan(pci);
+	report("PCI bus scanning detected %d devices", true, ret);
+
 	pci_shutdown(pci);
 
 done:
diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 62b5662..e479989 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -14,6 +14,49 @@
 #include "pci-host-generic.h"
 #include <linux/pci_regs.h>
 
+#define for_each_pci_dev(pci, dev, conf)		\
+	for (dev = find_next_dev(pci, -1, &conf);	\
+	     dev >= 0;					\
+	     dev = find_next_dev(pci, dev, &conf))
+
+static u8 pci_config_readb(const void *conf, int off)
+{
+	return readb(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);
+}
+
+static void* dev_conf(struct pci *pci, int dev)
+{
+	struct pci_host_bridge *host = pci->sysdata;
+	return (void*)host->cpu_range.start + (dev * 8) * PCI_ECAM_CONFIG_SIZE;
+}
+
+/* We scan bus 0 only for now */
+static int find_next_dev(struct pci *pci, int dev, void **pconf)
+{
+	struct pci_host_bridge *host = pci->sysdata;
+	int limit = MIN(32u, host->cpu_range.size / PCI_ECAM_CONFIG_SIZE);
+
+	for (dev++; dev < limit; dev++) {
+		void *conf = dev_conf(pci, dev);
+		if (pci_config_readl(conf, PCI_VENDOR_ID) != ((u32)~0)) {
+			*pconf = conf;
+			return dev;
+		}
+	}
+
+	return -1;
+}
+
 static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
 				     u32 cells[], int nr_range_cells)
 {
@@ -133,6 +176,24 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)
 	return host;
 }
 
+int pci_bus_scan(struct pci *pci)
+{
+	void *conf;
+	int dev;
+	int nr_dev = 0;
+
+	for_each_pci_dev(pci, dev, 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;
+}
+
 struct pci *pci_dt_probe(void)
 {
 	struct pci_host_bridge *host;
@@ -150,6 +211,12 @@ struct pci *pci_dt_probe(void)
 
 void pci_shutdown(struct pci *pci)
 {
+	void *conf;
+	int dev;
+
+	for_each_pci_dev(pci, dev, conf)
+		pci_config_writel(PCI_COMMAND_INTX_DISABLE, conf, PCI_COMMAND);
+
 	free(pci->sysdata);
 	free(pci);
 }
diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
index 097ac2d..c14d32d 100644
--- a/lib/pci-host-generic.h
+++ b/lib/pci-host-generic.h
@@ -22,5 +22,6 @@ struct pci_host_bridge {
 };
 
 #define PCI_ECAM_BUS_SIZE		(1 << 20)
+#define PCI_ECAM_CONFIG_SIZE		(1 << 12)
 
 #endif
diff --git a/lib/pci.h b/lib/pci.h
index 22b8e31..bc4f2d2 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -8,6 +8,7 @@ struct pci {
 };
 
 extern struct pci *pci_dt_probe(void);
+extern int pci_bus_scan(struct pci *pci);
 extern void pci_shutdown(struct pci *pci);
 
 #endif
-- 
1.8.3.1

[kvm-unit-tests PATCH 03/11] arm/pci: Read devices BARs

[Alexander Gordeev]

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-host-generic.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h | 10 ++++++
 2 files changed, 100 insertions(+)

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index e479989..b783d29 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -176,9 +176,91 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)
 	return host;
 }
 
+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;
+}
+
 int pci_bus_scan(struct pci *pci)
 {
 	void *conf;
+	pci_res_type_t type;
+	bool is64;
+	int bar;
 	int dev;
 	int nr_dev = 0;
 
@@ -187,6 +269,14 @@ int pci_bus_scan(struct pci *pci)
 		if (pci_config_readb(conf, PCI_HEADER_TYPE) !=
 					   PCI_HEADER_TYPE_NORMAL)
 			continue;
+
+		for (bar = 0; bar < PCI_HEADER_TYPE_NORMAL_NUM_BARS; bar++) {
+			if (!pci_get_bar(conf, bar, &type, NULL, NULL, &is64))
+				break;
+			if (is64)
+				bar++;
+		}
+
 		pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
 		nr_dev++;
 	}
diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
index c14d32d..132dc5f 100644
--- a/lib/pci-host-generic.h
+++ b/lib/pci-host-generic.h
@@ -21,7 +21,17 @@ struct pci_host_bridge {
 	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;
+
 #define PCI_ECAM_BUS_SIZE		(1 << 20)
 #define PCI_ECAM_CONFIG_SIZE		(1 << 12)
+#define PCI_HEADER_TYPE_NORMAL_NUM_BARS	6	/* # of BARs in PCI function */
 
 #endif
-- 
1.8.3.1

[kvm-unit-tests PATCH 04/11] arm/pci: Allocate and assign memory/io space resources

[Alexander Gordeev]

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

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index b783d29..f3161f8 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -19,6 +19,11 @@
 	     dev >= 0;					\
 	     dev = find_next_dev(pci, dev, &conf))
 
+static pci_res_type_t flags_to_type(u32 of_flags)
+{
+	return ((of_flags & 0x40000000) >> 28) | ((of_flags >> 24) & 0x03);
+}
+
 static u8 pci_config_readb(const void *conf, int off)
 {
 	return readb(conf + off);
@@ -255,11 +260,66 @@ static bool pci_get_bar(void *conf, int bar, pci_res_type_t *type,
 	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 pci_host_bridge *host,
+					   pci_res_type_t type)
+{
+	struct pci_addr_space *as = &host->addr_space[0];
+	int i;
+
+	for (i = 0; i < host->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 pci_host_bridge *host,
+				 pci_res_type_t type, u64 size)
+{
+	struct pci_addr_space *res;
+	phys_addr_t addr;
+
+	assert(res = pci_find_res(host, type));
+
+	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;
+}
+
 int pci_bus_scan(struct pci *pci)
 {
 	void *conf;
+	phys_addr_t addr;
+	u64 size;
 	pci_res_type_t type;
 	bool is64;
+	u32 cmd = PCI_COMMAND_SERR;
 	int bar;
 	int dev;
 	int nr_dev = 0;
@@ -271,13 +331,20 @@ int pci_bus_scan(struct pci *pci)
 			continue;
 
 		for (bar = 0; bar < PCI_HEADER_TYPE_NORMAL_NUM_BARS; bar++) {
-			if (!pci_get_bar(conf, bar, &type, NULL, NULL, &is64))
+			if (!pci_get_bar(conf, bar, &type, NULL, &size, &is64))
 				break;
+			addr = pci_alloc_res(pci->sysdata, type, size);
+			pci_set_bar(conf, bar, addr, is64);
 			if (is64)
 				bar++;
+
+			if (type == PCI_RES_TYPE_IO)
+				cmd |= PCI_COMMAND_IO;
+			else
+				cmd |= PCI_COMMAND_MEMORY;
 		}
 
-		pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
+		pci_config_writel(cmd, conf, PCI_COMMAND);
 		nr_dev++;
 	}
 
-- 
1.8.3.1

[kvm-unit-tests PATCH 05/11] arm/pci: Add pci_find_dev() and pci_bar_addr() functions

[Alexander Gordeev]

This updat is a reminiscence of x86 implementation - to
provision a possible future common PCI interface.

Make pcidevaddr_t as int, not u16. There is no good reason
to limit it to u16 while omitting properly adjusted bit fields.

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-host-generic.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h              | 18 +++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index f3161f8..3d9c271 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -29,6 +29,11 @@ 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);
@@ -351,6 +356,55 @@ int pci_bus_scan(struct 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(struct pci *pci, u16 vendor_id, u16 device_id)
+{
+	void *conf;
+	int dev;
+
+	for_each_pci_dev(pci, dev, conf) {
+		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;
+}
+
+phys_addr_t pci_bar_addr(struct pci *pci, pcidevaddr_t bdf, int bar)
+{
+	void *conf;
+	struct pci_addr_space *res;
+	pci_res_type_t type;
+	phys_addr_t addr;
+	bool is64;
+	int bus, dev, fn;
+
+	decode_addr(bdf, &bus, &dev, &fn);
+	assert(!bus && !fn);	/* We support bus 0 and function 0 only */
+
+	conf = dev_conf(pci, dev);
+	assert(pci_config_readb(conf, PCI_HEADER_TYPE) ==
+				      PCI_HEADER_TYPE_NORMAL);
+
+	assert(pci_get_bar(conf, bar, &type, &addr, NULL, &is64));
+	assert(res = pci_find_res(pci->sysdata, type));
+
+	return res->cpu_range.start + (addr - res->pci_range.start);
+}
+
 struct pci *pci_dt_probe(void)
 {
 	struct pci_host_bridge *host;
diff --git a/lib/pci.h b/lib/pci.h
index bc4f2d2..7d9baad 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -7,8 +7,26 @@ struct pci {
 	void *sysdata;
 };
 
+/*
+ * Make it a reminiscence of x86 implementation - to provision a possible
+ * future merge.
+ *
+ * Unlike x86, PCIDEVADDR_INVALID is -1, not 0. PCI bus-device-function
+ * of 00.00:0 is a legitimate PCI address, so x86 needs a fix.
+ *
+ * Also, make pcidevaddr_t int, not u16. There is no good reason to limit
+ * it to u16 while omitting properly adjusted bit fields.
+ *
+ */
+typedef enum {
+	PCIDEVADDR_INVALID = -1
+} pcidevaddr_t;
+
 extern struct pci *pci_dt_probe(void);
 extern int pci_bus_scan(struct pci *pci);
 extern void pci_shutdown(struct pci *pci);
 
+extern pcidevaddr_t pci_find_dev(struct pci *pci, u16 vendor_id, u16 device_id);
+extern phys_addr_t pci_bar_addr(struct pci *pci, pcidevaddr_t bdf, int bar_nr);
+
 #endif
-- 
1.8.3.1

[kvm-unit-tests PATCH 06/11] arm/pci: PCI testdev existence test

[Alexander Gordeev]

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arm/pci-test.c               |  3 +++
 config/config-arm-common.mak |  1 +
 lib/pci-testdev.c            | 31 +++++++++++++++++++++++++++++++
 lib/pci.h                    |  1 +
 4 files changed, 36 insertions(+)
 create mode 100644 lib/pci-testdev.c

diff --git a/arm/pci-test.c b/arm/pci-test.c
index 1539d3e..695acf9 100644
--- a/arm/pci-test.c
+++ b/arm/pci-test.c
@@ -22,6 +22,9 @@ int main(void)
 	ret = pci_bus_scan(pci);
 	report("PCI bus scanning detected %d devices", true, ret);
 
+	ret = pci_testdev(pci);
+	report("PCI test device", ret);
+
 	pci_shutdown(pci);
 
 done:
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..a7e0995
--- /dev/null
+++ b/lib/pci-testdev.c
@@ -0,0 +1,31 @@
+/*
+ * QEMU "pci-testdev" PCI test device
+ *
+ * Copyright (C) 2015, 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"
+
+#define PCI_VENDOR_ID_REDHAT		0x1b36
+#define PCI_DEVICE_ID_REDHAT_TEST	0x0005
+
+bool pci_testdev(struct pci *pci)
+{
+	phys_addr_t addr;
+	pcidevaddr_t dev = pci_find_dev(pci,
+					PCI_VENDOR_ID_REDHAT,
+					PCI_DEVICE_ID_REDHAT_TEST);
+
+	if (dev == PCIDEVADDR_INVALID)
+		return false;
+
+	addr = pci_bar_addr(pci, dev, 0);
+	assert(addr);
+
+	addr = pci_bar_addr(pci, dev, 1);
+	assert(addr);
+
+	return true;
+}
diff --git a/lib/pci.h b/lib/pci.h
index 7d9baad..6af5599 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -24,6 +24,7 @@ typedef enum {
 
 extern struct pci *pci_dt_probe(void);
 extern int pci_bus_scan(struct pci *pci);
+extern bool pci_testdev(struct pci *pci);
 extern void pci_shutdown(struct pci *pci);
 
 extern pcidevaddr_t pci_find_dev(struct pci *pci, u16 vendor_id, u16 device_id);
-- 
1.8.3.1

[kvm-unit-tests PATCH 07/11] arm/pci: PCI device operation test

[Alexander Gordeev]

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arm/pci-test.c    |   2 +-
 lib/pci-testdev.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 lib/pci.h         |   2 +-
 3 files changed, 138 insertions(+), 5 deletions(-)

diff --git a/arm/pci-test.c b/arm/pci-test.c
index 695acf9..44e2857 100644
--- a/arm/pci-test.c
+++ b/arm/pci-test.c
@@ -23,7 +23,7 @@ int main(void)
 	report("PCI bus scanning detected %d devices", true, ret);
 
 	ret = pci_testdev(pci);
-	report("PCI test device", ret);
+	report("PCI test device passed %d tests", (ret >= 6), ret);
 
 	pci_shutdown(pci);
 
diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
index a7e0995..de97f82 100644
--- a/lib/pci-testdev.c
+++ b/lib/pci-testdev.c
@@ -11,21 +11,154 @@
 #define PCI_VENDOR_ID_REDHAT		0x1b36
 #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
 
-bool pci_testdev(struct pci *pci)
+struct pci_test_dev_hdr {
+	u8 test;
+	u8 width;
+	u8 pad0[2];
+	u32 offset;
+	u32 data;
+	u32 count;
+	u8 name[];
+};
+
+struct pci_testdev_ops {
+	u8 (*read8)(const volatile void *addr);
+	u16 (*read16)(const volatile void *addr);
+	u32 (*read32)(const volatile void *addr);
+	void (*write8)(u8 value, volatile void *addr);
+	void (*write16)(u16 value, volatile void *addr);
+	void (*write32)(u32 value, volatile void *addr);
+};
+
+static u8 ioread8(const volatile void *addr)
+{
+	return readb(addr);
+}
+
+static u16 ioread16(const volatile void *addr)
+{
+	return readw(addr);
+}
+
+static u32 ioread32(const volatile void *addr)
+{
+	return readl(addr);
+}
+
+static void iowrite8(u8 value, volatile void *addr)
+{
+	writeb(value, addr);
+}
+
+static void iowrite16(u16 value, volatile void *addr)
+{
+	writew(value, addr);
+}
+
+static void iowrite32(u32 value, volatile void *addr)
+{
+	writel(value, addr);
+}
+
+static struct pci_testdev_ops pci_testdev_io_ops = {
+	.read8		= ioread8,
+	.read16		= ioread16,
+	.read32		= ioread32,
+	.write8		= iowrite8,
+	.write16	= iowrite16,
+	.write32	= iowrite32
+};
+
+static u8 memread8(const volatile void *addr)
+{
+	return *(const volatile u8 __force *)addr;
+}
+
+static u16 memread16(const volatile void *addr)
+{
+	return *(const volatile u16 __force *)addr;
+}
+
+static u32 memread32(const volatile void *addr)
+{
+	return *(const volatile u32 __force *)addr;
+}
+
+static void memwrite8(u8 value, volatile void *addr)
+{
+	*(volatile u8 __force *)addr = value;
+}
+
+static void memwrite16(u16 value, volatile void *addr)
+{
+	*(volatile u16 __force *)addr = value;
+}
+
+static void memwrite32(u32 value, volatile void *addr)
+{
+	*(volatile u32 __force *)addr = value;
+}
+
+static struct pci_testdev_ops pci_testdev_mem_ops = {
+	.read8		= memread8,
+	.read16		= memread16,
+	.read32		= memread32,
+	.write8		= memwrite8,
+	.write16	= memwrite16,
+	.write32	= memwrite32
+};
+
+static bool pci_testdev_one(struct pci_test_dev_hdr *test,
+			    int test_nr,
+			    struct pci_testdev_ops *ops)
+{
+	u8 width;
+
+	ops->write8(test_nr, &test->test);
+	assert(ops->read32(&test->count) == 0);
+
+	width = ops->read8(&test->width);
+	if ((width != 1) && (width != 2) && (width != 4))
+		return false;
+
+	return true;
+}
+
+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;
+	}
+
+	return i;
+}
+
+int pci_testdev(struct pci *pci)
 {
 	phys_addr_t addr;
+	void __iomem *mem, *io;
+	int nr_tests = 0;
 	pcidevaddr_t dev = pci_find_dev(pci,
 					PCI_VENDOR_ID_REDHAT,
 					PCI_DEVICE_ID_REDHAT_TEST);
 
 	if (dev == PCIDEVADDR_INVALID)
-		return false;
+		return 0;
 
 	addr = pci_bar_addr(pci, dev, 0);
 	assert(addr);
+	mem = ioremap(addr, 0);
 
 	addr = pci_bar_addr(pci, dev, 1);
 	assert(addr);
+	io = (void*)addr;
 
-	return true;
+	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 6af5599..0cb0e27 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -24,7 +24,7 @@ typedef enum {
 
 extern struct pci *pci_dt_probe(void);
 extern int pci_bus_scan(struct pci *pci);
-extern bool pci_testdev(struct pci *pci);
+extern int pci_testdev(struct pci *pci);
 extern void pci_shutdown(struct pci *pci);
 
 extern pcidevaddr_t pci_find_dev(struct pci *pci, u16 vendor_id, u16 device_id);
-- 
1.8.3.1

[kvm-unit-tests PATCH 08/11] arm/pci: PCI device read/write test

[Alexander Gordeev]

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-testdev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
index de97f82..dd6a5ac 100644
--- a/lib/pci-testdev.c
+++ b/lib/pci-testdev.c
@@ -113,6 +113,9 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
 			    struct pci_testdev_ops *ops)
 {
 	u8 width;
+	u32 sig, off;
+	const int nr_writes = 16;
+	int i;
 
 	ops->write8(test_nr, &test->test);
 	assert(ops->read32(&test->count) == 0);
@@ -121,6 +124,19 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
 	if ((width != 1) && (width != 2) && (width != 4))
 		return false;
 
+	sig = ops->read32(&test->data);
+	off = ops->read32(&test->offset);
+
+	for (i = 0; i < nr_writes; i++) {
+		switch (width) {
+			case 1: ops->write8 (sig, (void*)test + off); break;
+			case 2: ops->write16(sig, (void*)test + off); break;
+			case 4: ops->write32(sig, (void*)test + off); break;
+		}
+	}
+
+	assert((int)ops->read32(&test->count) == nr_writes);
+
 	return true;
 }
 
-- 
1.8.3.1

[kvm-unit-tests PATCH 09/11] arm/pci: PCI host bridge info printing

[Alexander Gordeev]

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

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 3d9c271..2d218a4 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -19,6 +19,15 @@
 	     dev >= 0;					\
 	     dev = find_next_dev(pci, dev, &conf))
 
+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);
@@ -105,6 +114,28 @@ static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
 	}
 }
 
+static void pci_host_bridge_print(struct pci_host_bridge *host)
+{
+	printf("PCIe start %016llx size %016llx "
+	       "bus %02x bus_max %02x #spaces %d\n\n",
+		host->cpu_range.start, host->cpu_range.size,
+		host->bus, host->bus_max, host->nr_addr_spaces);
+}
+
+static void pci_address_spaces_print(struct pci_addr_space as[], int nr_as)
+{
+	int i;
+
+	for (i = 0; i < nr_as; 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
@@ -180,9 +211,13 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)
 	host->bus_max		= bus_max;
 	host->nr_addr_spaces	= nr_addr_spaces;
 
+	pci_host_bridge_print(host);
+
 	pci_host_addr_space_init(&host->addr_space[0], nr_addr_spaces,
 				 (u32*)prop->data, nr_range_cells);
 
+	pci_address_spaces_print(&host->addr_space[0], host->nr_addr_spaces);
+
 	return host;
 }
 
-- 
1.8.3.1

[kvm-unit-tests PATCH 10/11] arm/pci: PCI devices basic info printing

[Alexander Gordeev]

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

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 2d218a4..50cf09a 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -352,6 +352,35 @@ static phys_addr_t pci_alloc_res(struct pci_host_bridge *host,
 	return addr;
 }
 
+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 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);
+	}
+}
+
 int pci_bus_scan(struct pci *pci)
 {
 	void *conf;
@@ -365,6 +394,8 @@ int pci_bus_scan(struct pci *pci)
 	int nr_dev = 0;
 
 	for_each_pci_dev(pci, dev, conf) {
+		pci_dev_print(conf);
+
 		/* We are only interested in normal PCI devices */
 		if (pci_config_readb(conf, PCI_HEADER_TYPE) !=
 					   PCI_HEADER_TYPE_NORMAL)
@@ -375,6 +406,9 @@ int pci_bus_scan(struct pci *pci)
 				break;
 			addr = pci_alloc_res(pci->sysdata, type, size);
 			pci_set_bar(conf, bar, addr, is64);
+
+			pci_dev_bar_print(bar, type, addr, size, is64);
+
 			if (is64)
 				bar++;
 
-- 
1.8.3.1

[kvm-unit-tests PATCH 11/11] arm/pci: PCI testdev test flavour printing

[Alexander Gordeev]

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-testdev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
index dd6a5ac..f7f291f 100644
--- a/lib/pci-testdev.c
+++ b/lib/pci-testdev.c
@@ -140,6 +140,23 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
 	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->read8(&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)
 {
@@ -148,6 +165,7 @@ static int pci_testdev_all(struct pci_test_dev_hdr *test,
 	for (i = 0;; i++) {
 		if (!pci_testdev_one(test, i, ops))
 			break;
+		pci_testdev_print(test, ops);
 	}
 
 	return i;
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:48PM +0100, Alexander Gordeev wrote:
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  arm/pci-test.c               |  25 +++++++
>  config/config-arm-common.mak |   5 +-
>  lib/alloc.c                  |   3 -
>  lib/libcflat.h               |   3 +
>  lib/pci-host-generic.c       | 155 +++++++++++++++++++++++++++++++++++++++++++
>  lib/pci-host-generic.h       |  26 ++++++++
>  lib/pci.h                    |  13 ++++
>  7 files changed, 226 insertions(+), 4 deletions(-)
>  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.h
> 
> diff --git a/arm/pci-test.c b/arm/pci-test.c
> new file mode 100644
> index 0000000..8629c89
> --- /dev/null
> +++ b/arm/pci-test.c
> @@ -0,0 +1,25 @@
> +/*
> + * PCI bus operation test
> + *
> + * Copyright (C) 2015, Red Hat Inc, Andrew Jones <drjones@redhat.com>

This has changed enough from the starter code I sent you that I don't
need my name here.

> + * Copyright (C) 2015, Red Hat Inc, Alexander Gordeev <agordeev@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */

should explicitly include <libcflat.h> too

> +#include <pci.h>
> +
> +int main(void)
> +{
> +	struct pci *pci;
> +
> +	pci = pci_dt_probe();

It would be better to only expose a 'pci_probe' to the unit tests.
Internally pci_probe then determines it should use pci_dt_probe
(unconditionally atm...)

> +
> +	report("PCI device tree probing", pci);
> +	if (!pci)
> +		goto done;
> +
> +	pci_shutdown(pci);

>From this patch it looks like we could call this function pci_free,
and call it safely with a NULL pci parameter, but it appears some
later patch will change that.

> +
> +done:
> +	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/alloc.c b/lib/alloc.c
> index ad67614..2f60145 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -7,9 +7,6 @@
>  #include "asm/spinlock.h"
>  #include "asm/io.h"
>  
> -#define MIN(a, b)		((a) < (b) ? (a) : (b))
> -#define MAX(a, b)		((a) > (b) ? (a) : (b))
> -
>  #define PHYS_ALLOC_NR_REGIONS	256
>  
>  struct phys_alloc_region {
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 9747ccd..2cb167c 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -76,4 +76,7 @@ do {									\
>  		abort();						\
>  } while (0)
>  
> +#define MIN(a, b)		((a) < (b) ? (a) : (b))
> +#define MAX(a, b)		((a) > (b) ? (a) : (b))
> +
>  #endif
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> new file mode 100644
> index 0000000..62b5662
> --- /dev/null
> +++ b/lib/pci-host-generic.c
> @@ -0,0 +1,155 @@
> +/*
> + * Adapated from
> + *   drivers/pci/host/pci-host-generic.c
> + *
> + * Copyright (C) 2015, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> + * 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>
> +
> +static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
> +				     u32 cells[], int nr_range_cells)
                                         ^ range_cells might be better
> +{
> +	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 = fdt32_to_cpu(cells[0]);
> +		as->pci_range.start = fdt64_to_cpu(*((fdt64_t*)&cells[1]));

This is kind of yucky. I'd rather see something like
  (fdt32_to_cpu(cells[1]) << 32) | fdt32_to_cpu(cells[2])

same comment for the next few assignments below

> +
> +		if (nr_range_cells == 6) {
> +			as->cpu_range.start = fdt32_to_cpu(cells[3]);
> +			as->cpu_range.size =
> +				fdt64_to_cpu(*((fdt64_t*)&cells[4]));
> +		} else {
> +			as->cpu_range.start =
> +				fdt64_to_cpu(*((fdt64_t*)&cells[3]));;
> +			as->cpu_range.size =
> +				fdt64_to_cpu(*((fdt64_t*)&cells[5]));
> +		}
> +
> +		as->pci_range.size = as->cpu_range.size;
> +
> +		cells += nr_range_cells;
> +	}
> +}
> +
> +/*
> + * Probe DT for a generic PCI host controller
> + * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
> + */
> +static struct pci_host_bridge *pci_host_bridge_probe(void)

This function probes DT, but there's no _dt_ in the name.

> +{
> +	struct pci_host_bridge *host;
> +	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())
> +		return NULL;
> +
> +	dt_bus_init_defaults(&dt_bus);
> +	dt_device_init(&dt_dev, &dt_bus, NULL);
> +
> +	node = fdt_path_offset(fdt, "/");
> +	assert(node >= 0);
> +
> +	assert(dt_get_nr_cells(node, &nac_root, &nsc_root) == 0);

Sorry the starter code I sent you had statements inside asserts().
We avoid those now, see commit 18ab6cadf

> +	assert(nac_root == 1 || nac_root == 2);
> +
> +	node = fdt_subnode_offset(fdt, node, "pcie");
> +	if (node == -FDT_ERR_NOTFOUND)

It would be good to print that the expected node is missing so
the user knows what's up.

> +		return NULL;
> +	assert(node >= 0);
> +
> +	prop = fdt_get_property(fdt, node, "device_type", &len);
> +	if (!prop || strcmp((char*)prop->data, "pci"))

You should check that len is big enough to do the strcmp without an
overflow. Also, again an error message to the user that the DT is
missing an expected property would be good.

> +		return NULL;
> +
> +	ret = fdt_node_check_compatible(fdt, node, "pci-host-ecam-generic");
> +	assert(ret >= 0);
> +	if (ret != 0)

need error message

> +		return NULL;
> +
> +	dt_device_bind_node(&dt_dev, node);
> +	assert(dt_pbus_get_base(&dt_dev, &base) == 0);
> +
> +	prop = fdt_get_property(fdt, node, "bus-range", &len);
> +	if (prop == NULL) {
> +		assert(len != 0);

Let's change this to assert(len == -FDT_ERR_NOTFOUND)

> +		bus		= 0x00;
> +		bus_max		= 0xff;

Should we just use 'base.size / PCI_ECAM_BUS_SIZE - 1' here for
bus_max, and then change the MIN() below to an assert, checking
that bus_max matches what's expected for the size on the other
path?

> +	} else {
> +		u32 *data	= (u32*)prop->data;
> +		bus		= fdt32_to_cpu(data[0]);
> +		bus_max		= fdt32_to_cpu(data[1]);
> +	}
> +	bus_max = MIN(bus_max, (base.size / PCI_ECAM_BUS_SIZE) - 1);
> +
> +	assert(dt_get_nr_cells(node, &nac, &nsc) == 0);
> +	assert(nac == 3 && nsc == 2);
> +
> +	assert(prop = fdt_get_property(fdt, node, "ranges", &len));
> +
> +	nr_range_cells = nac + nsc + nac_root;
> +	nr_addr_spaces = (len / 4) / nr_range_cells;
> +
> +	assert(host = malloc(sizeof(*host) +
> +			     sizeof(host->addr_space[0]) * nr_addr_spaces));
> +
> +	host->cpu_range.start	= base.addr;
> +	host->cpu_range.size	= base.size;
> +	host->bus		= bus;
> +	host->bus_max		= bus_max;
> +	host->nr_addr_spaces	= nr_addr_spaces;
> +
> +	pci_host_addr_space_init(&host->addr_space[0], nr_addr_spaces,
> +				 (u32*)prop->data, nr_range_cells);
> +
> +	return host;
> +}
> +
> +struct pci *pci_dt_probe(void)
> +{
> +	struct pci_host_bridge *host;
> +	struct pci *pci;
> +
> +	host = pci_host_bridge_probe();
> +	if (!host)
> +		return NULL;
> +
> +	assert(pci = calloc(1, sizeof(*pci)));
> +	pci->sysdata = host;
> +
> +	return pci;
> +}
> +
> +void pci_shutdown(struct pci *pci)
> +{
> +	free(pci->sysdata);
> +	free(pci);
> +}
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> new file mode 100644
> index 0000000..097ac2d
> --- /dev/null
> +++ b/lib/pci-host-generic.h
> @@ -0,0 +1,26 @@
> +#ifndef PCI_HOST_GENERIC_H
> +#define PCI_HOST_GENERIC_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;
> +	phys_addr_t			alloc;

alloc isn't used in this patch, but guess it will be later

> +	u32				of_flags;
> +};
> +
> +struct pci_host_bridge {
> +	struct pci_addr_range		cpu_range;
> +	int				bus;
> +	int				bus_max;
> +	int				nr_addr_spaces;
> +	struct pci_addr_space		addr_space[];
> +};
> +
> +#define PCI_ECAM_BUS_SIZE		(1 << 20)

Why not put this struct declaration (and supporting structs) in pci.h?
Linux has struct pci_host_bridge in include/linux/pci.h

> +
> +#endif
> diff --git a/lib/pci.h b/lib/pci.h
> new file mode 100644
> index 0000000..22b8e31
> --- /dev/null
> +++ b/lib/pci.h
> @@ -0,0 +1,13 @@
> +#ifndef PCI_H
> +#define PCI_H

This file needs its copyright/gpl header. I also like to document
the lib here in the header file, e.g. see lib/devicetree.h and
lib/alloc.h

> +
> +#include "alloc.h"
> +
> +struct pci {
> +	void *sysdata;

The choice of sysdata for the name appears to come from the Linux
kernel. Why not also call 'struct pci' 'struct pci_dev' for full
consistency?

> +};
> +
> +extern struct pci *pci_dt_probe(void);
> +extern void pci_shutdown(struct pci *pci);
> +
> +#endif
> -- 
> 1.8.3.1
> 

Re: [kvm-unit-tests PATCH 02/11] arm/pci: PCI bus scanning

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:49PM +0100, Alexander Gordeev wrote:
> Scan bus 0 and function 0 only for now
> 
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  arm/pci-test.c         |  4 +++
>  lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci-host-generic.h |  1 +
>  lib/pci.h              |  1 +
>  4 files changed, 73 insertions(+)
> 
> diff --git a/arm/pci-test.c b/arm/pci-test.c
> index 8629c89..1539d3e 100644
> --- a/arm/pci-test.c
> +++ b/arm/pci-test.c
> @@ -11,6 +11,7 @@
>  int main(void)
>  {
>  	struct pci *pci;
> +	int ret;
>  
>  	pci = pci_dt_probe();
>  
> @@ -18,6 +19,9 @@ int main(void)
>  	if (!pci)
>  		goto done;
>  
> +	ret = pci_bus_scan(pci);
> +	report("PCI bus scanning detected %d devices", true, ret);

How many devices are expected in this test? You can pass the expected
number in on the command line, and then make this a "real" test report
with

  report(..., ret == atol(argv[0]), ret)

> +
>  	pci_shutdown(pci);
>  
>  done:
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 62b5662..e479989 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -14,6 +14,49 @@
>  #include "pci-host-generic.h"
>  #include <linux/pci_regs.h>
>  
> +#define for_each_pci_dev(pci, dev, conf)		\
> +	for (dev = find_next_dev(pci, -1, &conf);	\
> +	     dev >= 0;					\
> +	     dev = find_next_dev(pci, dev, &conf))

Why do we need to pass both a dev index and a conf
pointer? I see below you can get the later from the
former easily enough.

> +
> +static u8 pci_config_readb(const void *conf, int off)
> +{
> +	return readb(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);
> +}

These read/write wrappers don't look all that useful to me. They would
be more useful if they took a 'struct pci[_dev] like Linux's
pci_read_config_byte/word/dword do. In fact, stealing those from Linux
makes sense.

> +
> +static void* dev_conf(struct pci *pci, int dev)
> +{
> +	struct pci_host_bridge *host = pci->sysdata;
> +	return (void*)host->cpu_range.start + (dev * 8) * PCI_ECAM_CONFIG_SIZE;
> +}
> +
> +/* We scan bus 0 only for now */
> +static int find_next_dev(struct pci *pci, int dev, void **pconf)
> +{
> +	struct pci_host_bridge *host = pci->sysdata;
> +	int limit = MIN(32u, host->cpu_range.size / PCI_ECAM_CONFIG_SIZE);

32 is the pci max devices per bus limit, right? Assuming a sane DT, is it
possible to have host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32?

> +
> +	for (dev++; dev < limit; dev++) {
> +		void *conf = dev_conf(pci, dev);
> +		if (pci_config_readl(conf, PCI_VENDOR_ID) != ((u32)~0)) {
> +			*pconf = conf;
> +			return dev;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
>  static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
>  				     u32 cells[], int nr_range_cells)
>  {
> @@ -133,6 +176,24 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)
>  	return host;
>  }
>  
> +int pci_bus_scan(struct pci *pci)
> +{
> +	void *conf;
> +	int dev;
> +	int nr_dev = 0;
> +
> +	for_each_pci_dev(pci, dev, 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);

Shouldn't the driver (unit test) be the one that writes SERR? Also,
doesn't this clear everything in the Command register except SERR? If
a scan gets run again after driver/unittest has started using a device
then this write may mess things up.

> +		nr_dev++;
> +	}
> +
> +	return nr_dev;
> +}
> +
>  struct pci *pci_dt_probe(void)
>  {
>  	struct pci_host_bridge *host;
> @@ -150,6 +211,12 @@ struct pci *pci_dt_probe(void)
>  
>  void pci_shutdown(struct pci *pci)
>  {
> +	void *conf;
> +	int dev;
> +
> +	for_each_pci_dev(pci, dev, conf)
> +		pci_config_writel(PCI_COMMAND_INTX_DISABLE, conf, PCI_COMMAND);

I think I know why this is here, but I don't think that kvm-unit-tests
needs to bother with it.

> +
>  	free(pci->sysdata);
>  	free(pci);
>  }
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> index 097ac2d..c14d32d 100644
> --- a/lib/pci-host-generic.h
> +++ b/lib/pci-host-generic.h
> @@ -22,5 +22,6 @@ struct pci_host_bridge {
>  };
>  
>  #define PCI_ECAM_BUS_SIZE		(1 << 20)
> +#define PCI_ECAM_CONFIG_SIZE		(1 << 12)

Are these also defined in the Linux kernel? I'm too lazy to pull up the
PCI spec right now, so I'd rather confirm the sizes by looking them up
in Linux. If they are defined there somewhere, then please use the same
names here. Otherwise, I guess references to some documentation or
something would be nice for pci dummies like me.

>  
>  #endif
> diff --git a/lib/pci.h b/lib/pci.h
> index 22b8e31..bc4f2d2 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -8,6 +8,7 @@ struct pci {
>  };
>  
>  extern struct pci *pci_dt_probe(void);
> +extern int pci_bus_scan(struct pci *pci);
>  extern void pci_shutdown(struct pci *pci);
>  
>  #endif
> -- 
> 1.8.3.1
> 

Re: [kvm-unit-tests PATCH 05/11] arm/pci: Add pci_find_dev() and pci_bar_addr() functions

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:52PM +0100, Alexander Gordeev wrote:
> This updat is a reminiscence of x86 implementation - to
> provision a possible future common PCI interface.

This comment inspired me to refactor the x86 code into common code.
I'll post that series in a few minutes for you to review. If x86
people are OK with it, then I think we should rebase your series on
it. I'm particular interested in learning if the last two patches
of this series can be greatly simplified by pretty much just
implementing the DT pcie-host-bridge discovery and a
lib/arm/asm/pci.h:pci_config_read() function.

> 
> Make pcidevaddr_t as int, not u16. There is no good reason
> to limit it to u16 while omitting properly adjusted bit fields.

pcidevaddr_t is OK as a u16, afaict it's just a device index
for a single bus (bus=0). So it has a max of 256. You could
redefine it to include the bus number too, but I'm not sure it's
necessary for simple kvm-unit-tests.

> 
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci.h              | 18 +++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index f3161f8..3d9c271 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -29,6 +29,11 @@ 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);
> @@ -351,6 +356,55 @@ int pci_bus_scan(struct 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(struct pci *pci, u16 vendor_id, u16 device_id)
> +{
> +	void *conf;
> +	int dev;
> +
> +	for_each_pci_dev(pci, dev, conf) {
> +		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;
> +}
> +
> +phys_addr_t pci_bar_addr(struct pci *pci, pcidevaddr_t bdf, int bar)
> +{
> +	void *conf;
> +	struct pci_addr_space *res;
> +	pci_res_type_t type;
> +	phys_addr_t addr;
> +	bool is64;
> +	int bus, dev, fn;
> +
> +	decode_addr(bdf, &bus, &dev, &fn);
> +	assert(!bus && !fn);	/* We support bus 0 and function 0 only */
> +
> +	conf = dev_conf(pci, dev);
> +	assert(pci_config_readb(conf, PCI_HEADER_TYPE) ==
> +				      PCI_HEADER_TYPE_NORMAL);
> +
> +	assert(pci_get_bar(conf, bar, &type, &addr, NULL, &is64));
> +	assert(res = pci_find_res(pci->sysdata, type));
> +
> +	return res->cpu_range.start + (addr - res->pci_range.start);
> +}
> +
>  struct pci *pci_dt_probe(void)
>  {
>  	struct pci_host_bridge *host;
> diff --git a/lib/pci.h b/lib/pci.h
> index bc4f2d2..7d9baad 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -7,8 +7,26 @@ struct pci {
>  	void *sysdata;
>  };
>  
> +/*
> + * Make it a reminiscence of x86 implementation - to provision a possible
> + * future merge.
> + *
> + * Unlike x86, PCIDEVADDR_INVALID is -1, not 0. PCI bus-device-function
> + * of 00.00:0 is a legitimate PCI address, so x86 needs a fix.

x86 gets away with this because the pci-testdev is never at addr 00.0,
that's where its host bridge is. I agree that it would be cleaner to use
an invalid dev-addr (unsigned > 255) for "INVALID" though. How about 0xffff?

> + *
> + * Also, make pcidevaddr_t int, not u16. There is no good reason to limit
> + * it to u16 while omitting properly adjusted bit fields.
> + *
> + */
> +typedef enum {
> +	PCIDEVADDR_INVALID = -1
> +} pcidevaddr_t;
> +
>  extern struct pci *pci_dt_probe(void);
>  extern int pci_bus_scan(struct pci *pci);
>  extern void pci_shutdown(struct pci *pci);
>  
> +extern pcidevaddr_t pci_find_dev(struct pci *pci, u16 vendor_id, u16 device_id);
> +extern phys_addr_t pci_bar_addr(struct pci *pci, pcidevaddr_t bdf, int bar_nr);
> +
>  #endif
> -- 
> 1.8.3.1
> 

Re: [kvm-unit-tests PATCH 07/11] arm/pci: PCI device operation test

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:54PM +0100, Alexander Gordeev wrote:
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  arm/pci-test.c    |   2 +-
>  lib/pci-testdev.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  lib/pci.h         |   2 +-
>  3 files changed, 138 insertions(+), 5 deletions(-)
> 
> diff --git a/arm/pci-test.c b/arm/pci-test.c
> index 695acf9..44e2857 100644
> --- a/arm/pci-test.c
> +++ b/arm/pci-test.c
> @@ -23,7 +23,7 @@ int main(void)
>  	report("PCI bus scanning detected %d devices", true, ret);
>  
>  	ret = pci_testdev(pci);
> -	report("PCI test device", ret);
> +	report("PCI test device passed %d tests", (ret >= 6), ret);
>  
>  	pci_shutdown(pci);
>  
> diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> index a7e0995..de97f82 100644
> --- a/lib/pci-testdev.c
> +++ b/lib/pci-testdev.c
> @@ -11,21 +11,154 @@
>  #define PCI_VENDOR_ID_REDHAT		0x1b36
>  #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
>  
> -bool pci_testdev(struct pci *pci)
> +struct pci_test_dev_hdr {
> +	u8 test;
> +	u8 width;
> +	u8 pad0[2];
> +	u32 offset;
> +	u32 data;
> +	u32 count;
> +	u8 name[];
> +};
> +
> +struct pci_testdev_ops {
> +	u8 (*read8)(const volatile void *addr);
> +	u16 (*read16)(const volatile void *addr);
> +	u32 (*read32)(const volatile void *addr);
> +	void (*write8)(u8 value, volatile void *addr);
> +	void (*write16)(u16 value, volatile void *addr);
> +	void (*write32)(u32 value, volatile void *addr);

Please use the standard 'b', 'w', 'l' in these names.

> +};
> +
> +static u8 ioread8(const volatile void *addr)
> +{
> +	return readb(addr);
> +}
> +
> +static u16 ioread16(const volatile void *addr)
> +{
> +	return readw(addr);
> +}
> +
> +static u32 ioread32(const volatile void *addr)
> +{
> +	return readl(addr);
> +}
> +
> +static void iowrite8(u8 value, volatile void *addr)
> +{
> +	writeb(value, addr);
> +}
> +
> +static void iowrite16(u16 value, volatile void *addr)
> +{
> +	writew(value, addr);
> +}
> +
> +static void iowrite32(u32 value, volatile void *addr)
> +{
> +	writel(value, addr);
> +}
> +
> +static struct pci_testdev_ops pci_testdev_io_ops = {
> +	.read8		= ioread8,
> +	.read16		= ioread16,
> +	.read32		= ioread32,
> +	.write8		= iowrite8,
> +	.write16	= iowrite16,
> +	.write32	= iowrite32
> +};
> +
> +static u8 memread8(const volatile void *addr)
> +{
> +	return *(const volatile u8 __force *)addr;
> +}
> +
> +static u16 memread16(const volatile void *addr)
> +{
> +	return *(const volatile u16 __force *)addr;
> +}
> +
> +static u32 memread32(const volatile void *addr)
> +{
> +	return *(const volatile u32 __force *)addr;
> +}
> +
> +static void memwrite8(u8 value, volatile void *addr)
> +{
> +	*(volatile u8 __force *)addr = value;
> +}
> +
> +static void memwrite16(u16 value, volatile void *addr)
> +{
> +	*(volatile u16 __force *)addr = value;
> +}
> +
> +static void memwrite32(u32 value, volatile void *addr)
> +{
> +	*(volatile u32 __force *)addr = value;
> +}
> +
> +static struct pci_testdev_ops pci_testdev_mem_ops = {
> +	.read8		= memread8,
> +	.read16		= memread16,
> +	.read32		= memread32,
> +	.write8		= memwrite8,
> +	.write16	= memwrite16,
> +	.write32	= memwrite32
> +};
> +
> +static bool pci_testdev_one(struct pci_test_dev_hdr *test,
> +			    int test_nr,
> +			    struct pci_testdev_ops *ops)
> +{
> +	u8 width;
> +
> +	ops->write8(test_nr, &test->test);
> +	assert(ops->read32(&test->count) == 0);
> +
> +	width = ops->read8(&test->width);
> +	if ((width != 1) && (width != 2) && (width != 4))
> +		return false;
> +
> +	return true;
> +}
> +
> +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;
> +	}
> +
> +	return i;
> +}
> +
> +int pci_testdev(struct pci *pci)
>  {
>  	phys_addr_t addr;
> +	void __iomem *mem, *io;
> +	int nr_tests = 0;
>  	pcidevaddr_t dev = pci_find_dev(pci,
>  					PCI_VENDOR_ID_REDHAT,
>  					PCI_DEVICE_ID_REDHAT_TEST);
>  
>  	if (dev == PCIDEVADDR_INVALID)
> -		return false;
> +		return 0;
>  
>  	addr = pci_bar_addr(pci, dev, 0);
>  	assert(addr);
> +	mem = ioremap(addr, 0);
>  
>  	addr = pci_bar_addr(pci, dev, 1);
>  	assert(addr);
> +	io = (void*)addr;
>  
> -	return true;
> +	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 6af5599..0cb0e27 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -24,7 +24,7 @@ typedef enum {
>  
>  extern struct pci *pci_dt_probe(void);
>  extern int pci_bus_scan(struct pci *pci);
> -extern bool pci_testdev(struct pci *pci);
> +extern int pci_testdev(struct pci *pci);
>  extern void pci_shutdown(struct pci *pci);
>  
>  extern pcidevaddr_t pci_find_dev(struct pci *pci, u16 vendor_id, u16 device_id);
> -- 
> 1.8.3.1
>

This patch can be squashed into the last one.

Thanks,
drew

Re: [kvm-unit-tests PATCH 08/11] arm/pci: PCI device read/write test

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:55PM +0100, Alexander Gordeev wrote:
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-testdev.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> index de97f82..dd6a5ac 100644
> --- a/lib/pci-testdev.c
> +++ b/lib/pci-testdev.c
> @@ -113,6 +113,9 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
>  			    struct pci_testdev_ops *ops)
>  {
>  	u8 width;
> +	u32 sig, off;
> +	const int nr_writes = 16;
> +	int i;
>  
>  	ops->write8(test_nr, &test->test);
>  	assert(ops->read32(&test->count) == 0);
> @@ -121,6 +124,19 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
>  	if ((width != 1) && (width != 2) && (width != 4))
>  		return false;
>  
> +	sig = ops->read32(&test->data);
> +	off = ops->read32(&test->offset);
> +
> +	for (i = 0; i < nr_writes; i++) {
> +		switch (width) {
> +			case 1: ops->write8 (sig, (void*)test + off); break;
> +			case 2: ops->write16(sig, (void*)test + off); break;
> +			case 4: ops->write32(sig, (void*)test + off); break;
> +		}
> +	}
> +
> +	assert((int)ops->read32(&test->count) == nr_writes);

Are you sure we want to assert here? What about outputting a message and
then returning false instead?

> +
>  	return true;
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [kvm-unit-tests PATCH 08/11] arm/pci: PCI device read/write test

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:55PM +0100, Alexander Gordeev wrote:
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-testdev.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> index de97f82..dd6a5ac 100644
> --- a/lib/pci-testdev.c
> +++ b/lib/pci-testdev.c
> @@ -113,6 +113,9 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
>  			    struct pci_testdev_ops *ops)
>  {
>  	u8 width;
> +	u32 sig, off;
> +	const int nr_writes = 16;
> +	int i;
>  
>  	ops->write8(test_nr, &test->test);
>  	assert(ops->read32(&test->count) == 0);
> @@ -121,6 +124,19 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
>  	if ((width != 1) && (width != 2) && (width != 4))
>  		return false;
>  
> +	sig = ops->read32(&test->data);
> +	off = ops->read32(&test->offset);
> +
> +	for (i = 0; i < nr_writes; i++) {
> +		switch (width) {
> +			case 1: ops->write8 (sig, (void*)test + off); break;
> +			case 2: ops->write16(sig, (void*)test + off); break;
> +			case 4: ops->write32(sig, (void*)test + off); break;
> +		}
> +	}
> +
> +	assert((int)ops->read32(&test->count) == nr_writes);
> +
>  	return true;
>  }

Forgot to mention that this could also be squashed into the last two
patches.

drew

Re: [kvm-unit-tests PATCH 09/11] arm/pci: PCI host bridge info printing

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:56PM +0100, Alexander Gordeev wrote:
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 3d9c271..2d218a4 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -19,6 +19,15 @@
>  	     dev >= 0;					\
>  	     dev = find_next_dev(pci, dev, &conf))
>  
> +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);
> @@ -105,6 +114,28 @@ static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
>  	}
>  }
>  
> +static void pci_host_bridge_print(struct pci_host_bridge *host)
> +{
> +	printf("PCIe start %016llx size %016llx "
> +	       "bus %02x bus_max %02x #spaces %d\n\n",
> +		host->cpu_range.start, host->cpu_range.size,
> +		host->bus, host->bus_max, host->nr_addr_spaces);
> +}
> +
> +static void pci_address_spaces_print(struct pci_addr_space as[], int nr_as)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_as; 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
> @@ -180,9 +211,13 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)
>  	host->bus_max		= bus_max;
>  	host->nr_addr_spaces	= nr_addr_spaces;
>  
> +	pci_host_bridge_print(host);
> +
>  	pci_host_addr_space_init(&host->addr_space[0], nr_addr_spaces,
>  				 (u32*)prop->data, nr_range_cells);
>  
> +	pci_address_spaces_print(&host->addr_space[0], host->nr_addr_spaces);
> +
>  	return host;
>  }

Looks good

drew

Re: [kvm-unit-tests PATCH 10/11] arm/pci: PCI devices basic info printing

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:57PM +0100, Alexander Gordeev wrote:
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 2d218a4..50cf09a 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -352,6 +352,35 @@ static phys_addr_t pci_alloc_res(struct pci_host_bridge *host,
>  	return addr;
>  }
>  
> +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 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);
> +	}
> +}
> +
>  int pci_bus_scan(struct pci *pci)
>  {
>  	void *conf;
> @@ -365,6 +394,8 @@ int pci_bus_scan(struct pci *pci)
>  	int nr_dev = 0;
>  
>  	for_each_pci_dev(pci, dev, conf) {
> +		pci_dev_print(conf);
> +
>  		/* We are only interested in normal PCI devices */
>  		if (pci_config_readb(conf, PCI_HEADER_TYPE) !=
>  					   PCI_HEADER_TYPE_NORMAL)
> @@ -375,6 +406,9 @@ int pci_bus_scan(struct pci *pci)
>  				break;
>  			addr = pci_alloc_res(pci->sysdata, type, size);
>  			pci_set_bar(conf, bar, addr, is64);
> +
> +			pci_dev_bar_print(bar, type, addr, size, is64);
> +
>  			if (is64)
>  				bar++;
>  
> -- 
> 1.8.3.1
>

I don't think we'll want to print on every bus scan. We should have a
special pci_dump_devices or something that can be called if needed.

Otherwise looks good

drew

Re: [kvm-unit-tests PATCH 11/11] arm/pci: PCI testdev test flavour printing

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:58PM +0100, Alexander Gordeev wrote:
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-testdev.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> index dd6a5ac..f7f291f 100644
> --- a/lib/pci-testdev.c
> +++ b/lib/pci-testdev.c
> @@ -140,6 +140,23 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
>  	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->read8(&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)
>  {
> @@ -148,6 +165,7 @@ static int pci_testdev_all(struct pci_test_dev_hdr *test,
>  	for (i = 0;; i++) {
>  		if (!pci_testdev_one(test, i, ops))
>  			break;
> +		pci_testdev_print(test, ops);
>  	}
>  
>  	return i;
> -- 
> 1.8.3.1
>

This can be squashed into 6/11-8/11

Thanks,
drew 

Re: [kvm-unit-tests PATCH 00/11] pci/arm: add PCI bus support

[Andrew Jones]

On Sat, Jan 09, 2016 at 01:22:47PM +0100, Alexander Gordeev wrote:
> This series extends the kvm-unit-tests/arm framework to support PCI.
> 
> Cc: Andrew Jones <drjones@redhat.com>
> 
> Alexander Gordeev (11):
>   arm/pci: Device tree PCI probing
>   arm/pci: PCI bus scanning
>   arm/pci: Read devices BARs
>   arm/pci: Allocate and assign memory/io space resources
>   arm/pci: Add pci_find_dev() and pci_bar_addr() functions
>   arm/pci: PCI testdev existence test
>   arm/pci: PCI device operation test
>   arm/pci: PCI device read/write test
>   arm/pci: PCI host bridge info printing
>   arm/pci: PCI devices basic info printing
>   arm/pci: PCI testdev test flavour printing
> 
>  arm/pci-test.c               |  32 +++
>  config/config-arm-common.mak |   6 +-
>  lib/alloc.c                  |   3 -
>  lib/libcflat.h               |   3 +
>  lib/pci-host-generic.c       | 502 +++++++++++++++++++++++++++++++++++++++++++
>  lib/pci-host-generic.h       |  37 ++++
>  lib/pci-testdev.c            | 198 +++++++++++++++++
>  lib/pci.h                    |  33 +++
>  8 files changed, 810 insertions(+), 4 deletions(-)
>  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
>  create mode 100644 lib/pci.h
> 
> -- 
> 1.8.3.1
>

Nice start! I think we can reduce some of it down a bit though by
integrating with x86 right now. I'll post a series that starts that
effort. If x86 people like it, then you can rebase on that, do some
patch squashing, and post again. Soon we'll have pci-testdev vmexit
tests for ARM!

Thanks,
drew

Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing

[Alexander Gordeev]

On Wed, Jan 13, 2016 at 04:13:07PM +0100, Andrew Jones wrote:
> > +	assert(dt_get_nr_cells(node, &nac_root, &nsc_root) == 0);
> 
> Sorry the starter code I sent you had statements inside asserts().
> We avoid those now, see commit 18ab6cadf

Hmm.. the commit does not appear addressing the described
in changelog condition '...if somebody introduces a switch to
"#define assert(...) /*nothing*/"...' as to-be-void asserts
are not followed by return value checks (even when seemingly
possible). If it worth following this pattern?

Thanks!

Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing

[Andrew Jones]

On Thu, Jan 28, 2016 at 04:17:19PM +0100, Alexander Gordeev wrote:
> On Wed, Jan 13, 2016 at 04:13:07PM +0100, Andrew Jones wrote:
> > > +	assert(dt_get_nr_cells(node, &nac_root, &nsc_root) == 0);
> > 
> > Sorry the starter code I sent you had statements inside asserts().
> > We avoid those now, see commit 18ab6cadf
> 
> Hmm.. the commit does not appear addressing the described
> in changelog condition '...if somebody introduces a switch to
> "#define assert(...) /*nothing*/"...' as to-be-void asserts
> are not followed by return value checks (even when seemingly
> possible). If it worth following this pattern?
>

The asserts on return values that we've left are only there out
of paranoia. It's highly unlikely the return codes will not be
their expected values, but if they were, then it'd be good to
halt immediately, rather than continue on making the issue more
difficult to debug.

Now, it's also highly unlikely we'll ever run kvm-unit-tests with
a no-op assert() function, but I'm glad Thomas suggested we avoid
embedding actual functionality in them, as it's now the more correct
way to use assert().

Thanks,
drew

Re: [kvm-unit-tests PATCH 02/11] arm/pci: PCI bus scanning

[Alexander Gordeev]

On Wed, Jan 13, 2016 at 04:58:46PM +0100, Andrew Jones wrote:
> On Sat, Jan 09, 2016 at 01:22:49PM +0100, Alexander Gordeev wrote:
> > Scan bus 0 and function 0 only for now
> > 
> > Cc: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  arm/pci-test.c         |  4 +++
> >  lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/pci-host-generic.h |  1 +
> >  lib/pci.h              |  1 +
> >  4 files changed, 73 insertions(+)
> > 
> > diff --git a/arm/pci-test.c b/arm/pci-test.c
> > index 8629c89..1539d3e 100644
> > --- a/arm/pci-test.c
> > +++ b/arm/pci-test.c
> > @@ -11,6 +11,7 @@
> >  int main(void)
> >  {
> >  	struct pci *pci;
> > +	int ret;
> >  
> >  	pci = pci_dt_probe();
> >  
> > @@ -18,6 +19,9 @@ int main(void)
> >  	if (!pci)
> >  		goto done;
> >  
> > +	ret = pci_bus_scan(pci);
> > +	report("PCI bus scanning detected %d devices", true, ret);
> 
> How many devices are expected in this test? You can pass the expected
> number in on the command line, and then make this a "real" test report
> with
> 
>   report(..., ret == atol(argv[0]), ret)

What if a machine's PCI hierarchy changed? Such test would break then.

Also, this report is not entirely dummy. It ensures PCI configuration
space access operates, devices do respond and generally the bus is fine.

> > +
> >  	pci_shutdown(pci);
> >  
> >  done:
> > diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> > index 62b5662..e479989 100644
> > --- a/lib/pci-host-generic.c
> > +++ b/lib/pci-host-generic.c
> > @@ -14,6 +14,49 @@
> >  #include "pci-host-generic.h"
> >  #include <linux/pci_regs.h>
> >  
> > +#define for_each_pci_dev(pci, dev, conf)		\
> > +	for (dev = find_next_dev(pci, -1, &conf);	\
> > +	     dev >= 0;					\
> > +	     dev = find_next_dev(pci, dev, &conf))
> 
> Why do we need to pass both a dev index and a conf
> pointer? I see below you can get the later from the
> former easily enough.

No particular reason apart from the fact each for_each_pci_dev()
would be followed by dev_conf() call - so I paired it to save
few lines. I have no preference here, though.

> > +
> > +static u8 pci_config_readb(const void *conf, int off)
> > +{
> > +	return readb(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);
> > +}
> 
> These read/write wrappers don't look all that useful to me. They would
> be more useful if they took a 'struct pci[_dev] like Linux's
> pci_read_config_byte/word/dword do. In fact, stealing those from Linux
> makes sense.

I put those ones only to emphasize PCI reads/writes as opposed to
direct read*/write* accesses. Matching the Linux names is probably
confusing.

Bringing Linux-like pci_read_config_byte/word/dword seems as an
overkill to me as it entails pulling the whole data hierarchy
pci_host_bridge->pci_bus->pci_dev.

By contrast, if we do not pull the whole hierarchy then we could
introduce our own minimalistic data structure(s) and get off with
it easily.

> > +
> > +static void* dev_conf(struct pci *pci, int dev)
> > +{
> > +	struct pci_host_bridge *host = pci->sysdata;
> > +	return (void*)host->cpu_range.start + (dev * 8) * PCI_ECAM_CONFIG_SIZE;
> > +}
> > +
> > +/* We scan bus 0 only for now */
> > +static int find_next_dev(struct pci *pci, int dev, void **pconf)
> > +{
> > +	struct pci_host_bridge *host = pci->sysdata;
> > +	int limit = MIN(32u, host->cpu_range.size / PCI_ECAM_CONFIG_SIZE);
> 
> 32 is the pci max devices per bus limit, right? Assuming a sane DT, is it
> possible to have host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32?

The point here is not host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32
but rather host->cpu_range.size/PCI_ECAM_CONFIG_SIZE < 32.

But you are right - a bus with less than 32 device slots is likely
insane.

> > +
> > +	for (dev++; dev < limit; dev++) {
> > +		void *conf = dev_conf(pci, dev);
> > +		if (pci_config_readl(conf, PCI_VENDOR_ID) != ((u32)~0)) {
> > +			*pconf = conf;
> > +			return dev;
> > +		}
> > +	}
> > +
> > +	return -1;
> > +}
> > +
> >  static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
> >  				     u32 cells[], int nr_range_cells)
> >  {
> > @@ -133,6 +176,24 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)
> >  	return host;
> >  }
> >  
> > +int pci_bus_scan(struct pci *pci)
> > +{
> > +	void *conf;
> > +	int dev;
> > +	int nr_dev = 0;
> > +
> > +	for_each_pci_dev(pci, dev, 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);
> 
> Shouldn't the driver (unit test) be the one that writes SERR? Also,
> doesn't this clear everything in the Command register except SERR?

I do not thing a driver is in charge. It is a minimal devices
initialization that is expected from firmware/BIOS AFAICT. A driver
should inherit the properly initialized bus with allocated io/memory
buffers to use.

> If a scan gets run again after driver/unittest has started using a device
> then this write may mess things up.

PCI bus scan is normally done once to initialize hierarchy. I am
struggling to imagine a usecase when it is called repeatedly,
especially in such a small framework.

> > +		nr_dev++;
> > +	}
> > +
> > +	return nr_dev;
> > +}
> > +
> >  struct pci *pci_dt_probe(void)
> >  {
> >  	struct pci_host_bridge *host;
> > @@ -150,6 +211,12 @@ struct pci *pci_dt_probe(void)
> >  
> >  void pci_shutdown(struct pci *pci)
> >  {
> > +	void *conf;
> > +	int dev;
> > +
> > +	for_each_pci_dev(pci, dev, conf)
> > +		pci_config_writel(PCI_COMMAND_INTX_DISABLE, conf, PCI_COMMAND);
> 
> I think I know why this is here, but I don't think that kvm-unit-tests
> needs to bother with it.

Well, it is not about interrupts, rather about disabling devices
access to access io/memory buffers, which is writing 0 to the
command regiester. PCI_COMMAND_INTX_DISABLE is just a good one
to write as well.

BTW, this is the reason I named this routine pci_shutdown(), not
pci_free().

> > +
> >  	free(pci->sysdata);
> >  	free(pci);
> >  }
> > diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> > index 097ac2d..c14d32d 100644
> > --- a/lib/pci-host-generic.h
> > +++ b/lib/pci-host-generic.h
> > @@ -22,5 +22,6 @@ struct pci_host_bridge {
> >  };
> >  
> >  #define PCI_ECAM_BUS_SIZE		(1 << 20)
> > +#define PCI_ECAM_CONFIG_SIZE		(1 << 12)
> 
> Are these also defined in the Linux kernel? I'm too lazy to pull up the
> PCI spec right now, so I'd rather confirm the sizes by looking them up
> in Linux. If they are defined there somewhere, then please use the same
> names here. Otherwise, I guess references to some documentation or
> something would be nice for pci dummies like me.
> 
> >  
> >  #endif
> > diff --git a/lib/pci.h b/lib/pci.h
> > index 22b8e31..bc4f2d2 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -8,6 +8,7 @@ struct pci {
> >  };
> >  
> >  extern struct pci *pci_dt_probe(void);
> > +extern int pci_bus_scan(struct pci *pci);
> >  extern void pci_shutdown(struct pci *pci);
> >  
> >  #endif
> > -- 
> > 1.8.3.1
> > 

Re: [kvm-unit-tests PATCH 02/11] arm/pci: PCI bus scanning

[Andrew Jones]

On Tue, Feb 02, 2016 at 10:34:01AM +0100, Alexander Gordeev wrote:
> On Wed, Jan 13, 2016 at 04:58:46PM +0100, Andrew Jones wrote:
> > On Sat, Jan 09, 2016 at 01:22:49PM +0100, Alexander Gordeev wrote:
> > > Scan bus 0 and function 0 only for now
> > > 
> > > Cc: Andrew Jones <drjones@redhat.com>
> > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > > ---
> > >  arm/pci-test.c         |  4 +++
> > >  lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/pci-host-generic.h |  1 +
> > >  lib/pci.h              |  1 +
> > >  4 files changed, 73 insertions(+)
> > > 
> > > diff --git a/arm/pci-test.c b/arm/pci-test.c
> > > index 8629c89..1539d3e 100644
> > > --- a/arm/pci-test.c
> > > +++ b/arm/pci-test.c
> > > @@ -11,6 +11,7 @@
> > >  int main(void)
> > >  {
> > >  	struct pci *pci;
> > > +	int ret;
> > >  
> > >  	pci = pci_dt_probe();
> > >  
> > > @@ -18,6 +19,9 @@ int main(void)
> > >  	if (!pci)
> > >  		goto done;
> > >  
> > > +	ret = pci_bus_scan(pci);
> > > +	report("PCI bus scanning detected %d devices", true, ret);
> > 
> > How many devices are expected in this test? You can pass the expected
> > number in on the command line, and then make this a "real" test report
> > with
> > 
> >   report(..., ret == atol(argv[0]), ret)
> 
> What if a machine's PCI hierarchy changed? Such test would break then.

How would the number *of* detected devices change if we never pass more
or less devices on the qemu command line? I realize the PCI addresses
may change.

> 
> Also, this report is not entirely dummy. It ensures PCI configuration
> space access operates, devices do respond and generally the bus is fine.
> 
> > > +
> > >  	pci_shutdown(pci);
> > >  
> > >  done:
> > > diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> > > index 62b5662..e479989 100644
> > > --- a/lib/pci-host-generic.c
> > > +++ b/lib/pci-host-generic.c
> > > @@ -14,6 +14,49 @@
> > >  #include "pci-host-generic.h"
> > >  #include <linux/pci_regs.h>
> > >  
> > > +#define for_each_pci_dev(pci, dev, conf)		\
> > > +	for (dev = find_next_dev(pci, -1, &conf);	\
> > > +	     dev >= 0;					\
> > > +	     dev = find_next_dev(pci, dev, &conf))
> > 
> > Why do we need to pass both a dev index and a conf
> > pointer? I see below you can get the later from the
> > former easily enough.
> 
> No particular reason apart from the fact each for_each_pci_dev()
> would be followed by dev_conf() call - so I paired it to save
> few lines. I have no preference here, though.

Then please drop one or the other.

> 
> > > +
> > > +static u8 pci_config_readb(const void *conf, int off)
> > > +{
> > > +	return readb(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);
> > > +}
> > 
> > These read/write wrappers don't look all that useful to me. They would
> > be more useful if they took a 'struct pci[_dev] like Linux's
> > pci_read_config_byte/word/dword do. In fact, stealing those from Linux
> > makes sense.
> 
> I put those ones only to emphasize PCI reads/writes as opposed to
> direct read*/write* accesses. Matching the Linux names is probably
> confusing.
> 
> Bringing Linux-like pci_read_config_byte/word/dword seems as an
> overkill to me as it entails pulling the whole data hierarchy
> pci_host_bridge->pci_bus->pci_dev.
> 
> By contrast, if we do not pull the whole hierarchy then we could
> introduce our own minimalistic data structure(s) and get off with
> it easily.

Yes, that's what I was thinking. We already do have our own minimalistic
data structure 'struct pci[_dev]'. However, now that I've made x86 pci
code common you'll either just need to create a lib/arm/asm/pci.h with
a pci_config_read like in lib/x86/asm/pci.h, or perhaps expand on that
if you need to read different sizes, by adding the _byte/word/...
variants.

> 
> > > +
> > > +static void* dev_conf(struct pci *pci, int dev)
> > > +{
> > > +	struct pci_host_bridge *host = pci->sysdata;
> > > +	return (void*)host->cpu_range.start + (dev * 8) * PCI_ECAM_CONFIG_SIZE;
> > > +}
> > > +
> > > +/* We scan bus 0 only for now */
> > > +static int find_next_dev(struct pci *pci, int dev, void **pconf)
> > > +{
> > > +	struct pci_host_bridge *host = pci->sysdata;
> > > +	int limit = MIN(32u, host->cpu_range.size / PCI_ECAM_CONFIG_SIZE);
> > 
> > 32 is the pci max devices per bus limit, right? Assuming a sane DT, is it
> > possible to have host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32?
> 
> The point here is not host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32
> but rather host->cpu_range.size/PCI_ECAM_CONFIG_SIZE < 32.
> 
> But you are right - a bus with less than 32 device slots is likely
> insane.
> 
> > > +
> > > +	for (dev++; dev < limit; dev++) {
> > > +		void *conf = dev_conf(pci, dev);
> > > +		if (pci_config_readl(conf, PCI_VENDOR_ID) != ((u32)~0)) {
> > > +			*pconf = conf;
> > > +			return dev;
> > > +		}
> > > +	}
> > > +
> > > +	return -1;
> > > +}
> > > +
> > >  static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
> > >  				     u32 cells[], int nr_range_cells)
> > >  {
> > > @@ -133,6 +176,24 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)
> > >  	return host;
> > >  }
> > >  
> > > +int pci_bus_scan(struct pci *pci)
> > > +{
> > > +	void *conf;
> > > +	int dev;
> > > +	int nr_dev = 0;
> > > +
> > > +	for_each_pci_dev(pci, dev, 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);
> > 
> > Shouldn't the driver (unit test) be the one that writes SERR? Also,
> > doesn't this clear everything in the Command register except SERR?
> 
> I do not thing a driver is in charge. It is a minimal devices
> initialization that is expected from firmware/BIOS AFAICT. A driver
> should inherit the properly initialized bus with allocated io/memory
> buffers to use.

OK, does QEMU not provide that already?

> 
> > If a scan gets run again after driver/unittest has started using a device
> > then this write may mess things up.
> 
> PCI bus scan is normally done once to initialize hierarchy. I am
> struggling to imagine a usecase when it is called repeatedly,
> especially in such a small framework.

A unit test can do whatever it likes. I'd like the initial setup to be
useful, but minimal, and easily modifiable.

> 
> > > +		nr_dev++;
> > > +	}
> > > +
> > > +	return nr_dev;
> > > +}
> > > +
> > >  struct pci *pci_dt_probe(void)
> > >  {
> > >  	struct pci_host_bridge *host;
> > > @@ -150,6 +211,12 @@ struct pci *pci_dt_probe(void)
> > >  
> > >  void pci_shutdown(struct pci *pci)
> > >  {
> > > +	void *conf;
> > > +	int dev;
> > > +
> > > +	for_each_pci_dev(pci, dev, conf)
> > > +		pci_config_writel(PCI_COMMAND_INTX_DISABLE, conf, PCI_COMMAND);
> > 
> > I think I know why this is here, but I don't think that kvm-unit-tests
> > needs to bother with it.
> 
> Well, it is not about interrupts, rather about disabling devices
> access to access io/memory buffers, which is writing 0 to the
> command regiester. PCI_COMMAND_INTX_DISABLE is just a good one
> to write as well.
> 
> BTW, this is the reason I named this routine pci_shutdown(), not
> pci_free().

I'm currently thinking any sort of shutdown is overkill for
kvm-unit-tests, but I'll accept anything you can make a good
case for :-)

> 
> > > +
> > >  	free(pci->sysdata);
> > >  	free(pci);
> > >  }
> > > diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> > > index 097ac2d..c14d32d 100644
> > > --- a/lib/pci-host-generic.h
> > > +++ b/lib/pci-host-generic.h
> > > @@ -22,5 +22,6 @@ struct pci_host_bridge {
> > >  };
> > >  
> > >  #define PCI_ECAM_BUS_SIZE		(1 << 20)
> > > +#define PCI_ECAM_CONFIG_SIZE		(1 << 12)
> > 
> > Are these also defined in the Linux kernel? I'm too lazy to pull up the
> > PCI spec right now, so I'd rather confirm the sizes by looking them up
> > in Linux. If they are defined there somewhere, then please use the same
> > names here. Otherwise, I guess references to some documentation or
> > something would be nice for pci dummies like me.
> > 
> > >  
> > >  #endif
> > > diff --git a/lib/pci.h b/lib/pci.h
> > > index 22b8e31..bc4f2d2 100644
> > > --- a/lib/pci.h
> > > +++ b/lib/pci.h
> > > @@ -8,6 +8,7 @@ struct pci {
> > >  };
> > >  
> > >  extern struct pci *pci_dt_probe(void);
> > > +extern int pci_bus_scan(struct pci *pci);
> > >  extern void pci_shutdown(struct pci *pci);
> > >  
> > >  #endif
> > > -- 
> > > 1.8.3.1
> > > 

Re: [kvm-unit-tests PATCH 08/11] arm/pci: PCI device read/write test

[Alexander Gordeev]

On Fri, Jan 15, 2016 at 04:33:50PM +0100, Andrew Jones wrote:
> On Sat, Jan 09, 2016 at 01:22:55PM +0100, Alexander Gordeev wrote:
> > Cc: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  lib/pci-testdev.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> > index de97f82..dd6a5ac 100644
> > --- a/lib/pci-testdev.c
> > +++ b/lib/pci-testdev.c
> > @@ -113,6 +113,9 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
> >  			    struct pci_testdev_ops *ops)
> >  {
> >  	u8 width;
> > +	u32 sig, off;
> > +	const int nr_writes = 16;
> > +	int i;
> >  
> >  	ops->write8(test_nr, &test->test);
> >  	assert(ops->read32(&test->count) == 0);
> > @@ -121,6 +124,19 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,
> >  	if ((width != 1) && (width != 2) && (width != 4))
> >  		return false;
> >  
> > +	sig = ops->read32(&test->data);
> > +	off = ops->read32(&test->offset);
> > +
> > +	for (i = 0; i < nr_writes; i++) {
> > +		switch (width) {
> > +			case 1: ops->write8 (sig, (void*)test + off); break;
> > +			case 2: ops->write16(sig, (void*)test + off); break;
> > +			case 4: ops->write32(sig, (void*)test + off); break;
> > +		}
> > +	}
> > +
> > +	assert((int)ops->read32(&test->count) == nr_writes);
> 
> Are you sure we want to assert here? What about outputting a message and
> then returning false instead?

Nice catch.

> > +
> >  	return true;
> >  }
> >  
> > -- 
> > 1.8.3.1
> > 

Re: [kvm-unit-tests PATCH 07/11] arm/pci: PCI device operation test

[Alexander Gordeev]

On Fri, Jan 15, 2016 at 04:32:11PM +0100, Andrew Jones wrote:
> > +struct pci_testdev_ops {
> > +	u8 (*read8)(const volatile void *addr);
> > +	u16 (*read16)(const volatile void *addr);
> > +	u32 (*read32)(const volatile void *addr);
> > +	void (*write8)(u8 value, volatile void *addr);
> > +	void (*write16)(u16 value, volatile void *addr);
> > +	void (*write32)(u32 value, volatile void *addr);
> 
> Please use the standard 'b', 'w', 'l' in these names.

I did not use in the first place as it conflicts with standard
read*/write* macros. Would [read|write]_[byte|word|long] work
for you?

> Thanks,
> drew

Re: [kvm-unit-tests PATCH 07/11] arm/pci: PCI device operation test

[Andrew Jones]

On Thu, Feb 04, 2016 at 01:18:38PM +0100, Alexander Gordeev wrote:
> On Fri, Jan 15, 2016 at 04:32:11PM +0100, Andrew Jones wrote:
> > > +struct pci_testdev_ops {
> > > +	u8 (*read8)(const volatile void *addr);
> > > +	u16 (*read16)(const volatile void *addr);
> > > +	u32 (*read32)(const volatile void *addr);
> > > +	void (*write8)(u8 value, volatile void *addr);
> > > +	void (*write16)(u16 value, volatile void *addr);
> > > +	void (*write32)(u32 value, volatile void *addr);
> > 
> > Please use the standard 'b', 'w', 'l' in these names.
> 
> I did not use in the first place as it conflicts with standard
> read*/write* macros. Would [read|write]_[byte|word|long] work
> for you?

Oh yeah... dang macros. _byte/word/dword like the pci_config_read/write
function in Linux would work.

drew

Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing

[Alexander Gordeev]

On Wed, Jan 13, 2016 at 04:13:07PM +0100, Andrew Jones wrote:
> > diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> > new file mode 100644
> > index 0000000..097ac2d
> > --- /dev/null
> > +++ b/lib/pci-host-generic.h
> > @@ -0,0 +1,26 @@
> > +#ifndef PCI_HOST_GENERIC_H
> > +#define PCI_HOST_GENERIC_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;
> > +	phys_addr_t			alloc;
> 
> alloc isn't used in this patch, but guess it will be later
> 
> > +	u32				of_flags;
> > +};
> > +
> > +struct pci_host_bridge {
> > +	struct pci_addr_range		cpu_range;
> > +	int				bus;
> > +	int				bus_max;
> > +	int				nr_addr_spaces;
> > +	struct pci_addr_space		addr_space[];
> > +};
> > +
> > +#define PCI_ECAM_BUS_SIZE		(1 << 20)
> 
> Why not put this struct declaration (and supporting structs) in pci.h?
> Linux has struct pci_host_bridge in include/linux/pci.h

I think we do not need any of these structures to get public.

My initial attempt was also an assessment of an idea to
generalize PCI code across architectures. But my current
judgement - an outcome is not worth it. With x86 PCI access
so much different and only bus 0 function 0 "hierarchy" we
only need the simplest required routines.

Hence, I would leave these in pci-host-generic.h (just not
to blow pci-host-generic.c too big).

As result, just these routines would end up in pci.h:

	bool pci_probe(void);
	void pci_free(void);	/* pci_shutdown() ? */ 
	int pci_bus_scan(void);
	int pci_testdev(void);

Thoughts?

> > +
> > +#endif
> > diff --git a/lib/pci.h b/lib/pci.h
> > new file mode 100644
> > index 0000000..22b8e31
> > --- /dev/null
> > +++ b/lib/pci.h
> > @@ -0,0 +1,13 @@
> > +#ifndef PCI_H
> > +#define PCI_H
> 
> This file needs its copyright/gpl header. I also like to document
> the lib here in the header file, e.g. see lib/devicetree.h and
> lib/alloc.h
> 
> > +
> > +#include "alloc.h"
> > +
> > +struct pci {
> > +	void *sysdata;
> 
> The choice of sysdata for the name appears to come from the Linux
> kernel. Why not also call 'struct pci' 'struct pci_dev' for full
> consistency?
> 
> > +};
> > +
> > +extern struct pci *pci_dt_probe(void);
> > +extern void pci_shutdown(struct pci *pci);
> > +
> > +#endif
> > -- 
> > 1.8.3.1
> > 

Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing

[Andrew Jones]

On Fri, Feb 05, 2016 at 12:48:50PM +0100, Alexander Gordeev wrote:
> On Wed, Jan 13, 2016 at 04:13:07PM +0100, Andrew Jones wrote:
> > > diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> > > new file mode 100644
> > > index 0000000..097ac2d
> > > --- /dev/null
> > > +++ b/lib/pci-host-generic.h
> > > @@ -0,0 +1,26 @@
> > > +#ifndef PCI_HOST_GENERIC_H
> > > +#define PCI_HOST_GENERIC_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;
> > > +	phys_addr_t			alloc;
> > 
> > alloc isn't used in this patch, but guess it will be later
> > 
> > > +	u32				of_flags;
> > > +};
> > > +
> > > +struct pci_host_bridge {
> > > +	struct pci_addr_range		cpu_range;
> > > +	int				bus;
> > > +	int				bus_max;
> > > +	int				nr_addr_spaces;
> > > +	struct pci_addr_space		addr_space[];
> > > +};
> > > +
> > > +#define PCI_ECAM_BUS_SIZE		(1 << 20)
> > 
> > Why not put this struct declaration (and supporting structs) in pci.h?
> > Linux has struct pci_host_bridge in include/linux/pci.h
> 
> I think we do not need any of these structures to get public.
> 
> My initial attempt was also an assessment of an idea to
> generalize PCI code across architectures. But my current
> judgement - an outcome is not worth it. With x86 PCI access
> so much different and only bus 0 function 0 "hierarchy" we
> only need the simplest required routines.
> 
> Hence, I would leave these in pci-host-generic.h (just not
> to blow pci-host-generic.c too big).
> 
> As result, just these routines would end up in pci.h:
> 
> 	bool pci_probe(void);
> 	void pci_free(void);	/* pci_shutdown() ? */ 
> 	int pci_bus_scan(void);
> 	int pci_testdev(void);
> 
> Thoughts?

Whichever you prefer. We either "bloat" pci.h or we "bloat" the lib
directory with another file :-) Actually, I still prefer pci.h, because
if arm unit tests will always need both of them, then they'll always
need two #include lines... OTOH, if you don't think unit tests will
need these structures, then they could just be defined right in
pci-host-generic.c (or pci.c if you consolidate the source files).

Thanks,
drew

> 
> > > +
> > > +#endif
> > > diff --git a/lib/pci.h b/lib/pci.h
> > > new file mode 100644
> > > index 0000000..22b8e31
> > > --- /dev/null
> > > +++ b/lib/pci.h
> > > @@ -0,0 +1,13 @@
> > > +#ifndef PCI_H
> > > +#define PCI_H
> > 
> > This file needs its copyright/gpl header. I also like to document
> > the lib here in the header file, e.g. see lib/devicetree.h and
> > lib/alloc.h
> > 
> > > +
> > > +#include "alloc.h"
> > > +
> > > +struct pci {
> > > +	void *sysdata;
> > 
> > The choice of sysdata for the name appears to come from the Linux
> > kernel. Why not also call 'struct pci' 'struct pci_dev' for full
> > consistency?
> > 
> > > +};
> > > +
> > > +extern struct pci *pci_dt_probe(void);
> > > +extern void pci_shutdown(struct pci *pci);
> > > +
> > > +#endif
> > > -- 
> > > 1.8.3.1
> > >