[PATCH v3 0/3] ARM: PCI: implement generic PCI host controller

[PATCH v3 0/3] ARM: PCI: implement generic PCI host controller

[Will Deacon]

Hi,

This is version 3 of the patches previously posted here:

  v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/229679.html
  v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/232213.html

Changes since v2 include:

  - Tightening up of the DT requirements:
    * Must have at least a non-prefetchable memory region
    * "reg" property now defines a base and size for config space
    * bus-range property supported and used in conjunction with "reg"
    * device_type must be "pci"

  - Fixed bus mapping so we don't sleep in atomic

  - Updated compatible strings and file names to get rid of "arm"

  - Added a really dumb I/O space allocator

  - Moved a bunch of ->probe() into ->setup(). Note that I've not included
    Arnd's patch to propagate ->setup() failures back to ->probe(), so despite
    the use of devm_* to manage resource allocations, I still have explicit
    cleanup code on the failure paths.

All feedback welcome.

Will


Will Deacon (3):
  ARM: mach-virt: allow PCI support to be selected
  ARM: bios32: use pci_enable_resource to enable PCI resources
  PCI: ARM: add support for generic PCI host controller

 .../devicetree/bindings/pci/host-generic-pci.txt   |  88 +++++
 arch/arm/kernel/bios32.c                           |  37 +-
 arch/arm/mach-virt/Kconfig                         |   1 +
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-host-generic.c                | 381 +++++++++++++++++++++
 6 files changed, 481 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/host-generic-pci.txt
 create mode 100644 drivers/pci/host/pci-host-generic.c

-- 
1.8.2.2

[PATCH v3 1/3] ARM: mach-virt: allow PCI support to be selected

[Will Deacon]

mach-virt can make use of virtio-pci devices, which requires the guest
kernel to have PCI support selected.

This patch selects CONFIG_MIGHT_HAVE_PCI when CONFIG_ARCH_VIRT=y.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/mach-virt/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-virt/Kconfig b/arch/arm/mach-virt/Kconfig
index 081d46929436..f40fb55574cb 100644
--- a/arch/arm/mach-virt/Kconfig
+++ b/arch/arm/mach-virt/Kconfig
@@ -8,3 +8,4 @@ config ARCH_VIRT
 	select CPU_V7
 	select SPARSE_IRQ
 	select USE_OF
+	select MIGHT_HAVE_PCI
-- 
1.8.2.2

[PATCH v3 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Will Deacon]

This patch moves bios32 over to using the generic code for enabling PCI
resources. Since the core code takes care of bridge resources too, we
can also drop the explicit IO and MEMORY enabling for them in the arch
code.

A side-effect of this change is that we no longer explicitly enable
devices when running in PCI_PROBE_ONLY mode. This stays closer to the
meaning of the option and prevents us from trying to enable devices
without any assigned resources (the core code refuses to enable
resources without parents).

Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/bios32.c | 37 +++----------------------------------
 1 file changed, 3 insertions(+), 34 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 317da88ae65b..91f48804e3bb 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -608,41 +608,10 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
  */
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx = 0; idx < 6; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1 << idx)))
-			continue;
-
-		r = dev->resource + idx;
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because"
-			       " of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
+	if (pci_has_flag(PCI_PROBE_ONLY))
+		return 0;
 
-	/*
-	 * Bridges (eg, cardbus bridges) need to be fully enabled
-	 */
-	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
-		cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
-
-	if (cmd != old_cmd) {
-		printk("PCI: enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
+	return pci_enable_resources(dev, mask);
 }
 
 int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
-- 
1.8.2.2

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

This patch adds support for a generic PCI host controller, such as a
firmware-initialised device with static windows or an emulation by
something such as kvmtool.

The controller itself has no configuration registers and has its address
spaces described entirely by the device-tree (using the bindings from
ePAPR). Both CAM and ECAM are supported for Config Space accesses.

Corresponding documentation is added for the DT binding.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 .../devicetree/bindings/pci/host-generic-pci.txt   |  88 +++++
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-host-generic.c                | 381 +++++++++++++++++++++
 4 files changed, 477 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/host-generic-pci.txt
 create mode 100644 drivers/pci/host/pci-host-generic.c

diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
new file mode 100644
index 000000000000..03e1640a7602
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
@@ -0,0 +1,88 @@
+* Generic PCI host controller
+
+Firmware-initialised PCI host controllers and PCI emulations, such as the
+virtio-pci implementations found in kvmtool and other para-virtualised
+systems, do not require driver support for complexities such as regulator
+and clock management. In fact, the controller may not even require the
+configuration of a control interface by the operating system, instead
+presenting a set of fixed windows describing a subset of IO, Memory and
+Configuration Spaces.
+
+Such a controller can be described purely in terms of the standardized device
+tree bindings communicated in pci.txt:
+
+- compatible     : Must be "pci-host-cam-generic" or "pci-host-ecam-generic"
+                   depending on the layout of configuration space (CAM vs
+                   ECAM respectively).
+
+- device_type    : Must be "pci".
+
+- ranges         : As described in IEEE Std 1275-1994, but must provide
+                   at least a definition of non-prefetchable memory. One
+                   or both of prefetchable Memory and IO Space may also
+                   be provided.
+
+- bus-range      : Optional property (also described in IEEE Std 1275-1994)
+                   to indicate the range of bus numbers for this controller.
+                   If absent, defaults to <0 255> (i.e. all buses).
+
+- #address-cells : Must be 3.
+
+- #size-cells    : Must be 2.
+
+- reg            : The Configuration Space base address and size, as accessed
+                   from the parent bus.
+
+Configuration Space is assumed to be memory-mapped (as opposed to being
+accessed via an ioport) and laid out with a direct correspondence to the
+geography of a PCI bus address by concatenating the various components to
+form an offset.
+
+For CAM, this 24-bit offset is:
+
+        cfg_offset(bus, device, function, register) =
+                   bus << 16 | device << 11 | function << 8 | register
+
+Whilst ECAM extends this by 4 bits to accomodate 4k of function space:
+
+        cfg_offset(bus, device, function, register) =
+                   bus << 20 | device << 15 | function << 12 | register
+
+Interrupt mapping is exactly as described in `Open Firmware Recommended
+Practice: Interrupt Mapping' and requires the following properties:
+
+- #interrupt-cells   : Must be 1
+
+- interrupt-map      : <see aforementioned specification>
+
+- interrupt-map-mask : <see aforementioned specification>
+
+
+Example:
+
+pci {
+    compatible = "pci-host-cam-generic"
+    device_type = "pci";
+    #address-cells = <3>;
+    #size-cells = <2>;
+    bus-range = <0x0 0x1>;
+
+    // CPU_PHYSICAL(2)  SIZE(2)
+    reg = <0x0 0x40000000  0x0 0x1000000>;
+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
+             <0x2000000 0x0 0x41000000  0x0 0x41000000  0x0 0x3f000000>;
+
+
+    #interrupt-cells = <0x1>;
+
+    // PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(3)
+    interrupt-map = <  0x0 0x0 0x0  0x1  &gic  0x0 0x4 0x1
+                     0x800 0x0 0x0  0x1  &gic  0x0 0x5 0x1
+                    0x1000 0x0 0x0  0x1  &gic  0x0 0x6 0x1
+                    0x1800 0x0 0x0  0x1  &gic  0x0 0x7 0x1>;
+
+    // PCI_DEVICE(3)  INT#(1)
+    interrupt-map-mask = <0xf800 0x0 0x0  0x7>;
+}
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 47d46c6d8468..f91637d47065 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -33,4 +33,11 @@ config PCI_RCAR_GEN2
 	  There are 3 internal PCI controllers available with a single
 	  built-in EHCI/OHCI host controller present on each one.
 
+config PCI_HOST_GENERIC
+	bool "Generic PCI host controller"
+	depends on ARM && OF
+	help
+	  Say Y here if you want to support a simple generic PCI host
+	  controller, such as the one emulated by kvmtool.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 13fb3333aa05..bd1bf1ab4ac8 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
 obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
 obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
 obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
+obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
new file mode 100644
index 000000000000..d843b546a8a1
--- /dev/null
+++ b/drivers/pci/host/pci-host-generic.c
@@ -0,0 +1,381 @@
+/*
+ * Simple, generic PCI host controller driver targetting firmware-initialised
+ * systems and virtual machines (e.g. the PCI emulation provided by kvmtool).
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (C) 2014 ARM Limited
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/platform_device.h>
+
+struct gen_pci_cfg_bus_ops {
+	u32 bus_shift;
+	void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int);
+};
+
+struct gen_pci_cfg_windows {
+	struct resource				res;
+	struct resource				bus_range;
+	void __iomem				**win;
+
+	const struct gen_pci_cfg_bus_ops	*ops;
+};
+
+struct gen_pci {
+	struct pci_host_bridge			host;
+	struct gen_pci_cfg_windows		cfg;
+};
+
+static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
+					     unsigned int devfn,
+					     int where)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+	resource_size_t idx = bus->number - pci->cfg.bus_range.start;
+
+	return pci->cfg.win[idx] + ((devfn << 8) | where);
+}
+
+static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = {
+	.bus_shift	= 16,
+	.map_bus	= gen_pci_map_cfg_bus_cam,
+};
+
+static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
+					      unsigned int devfn,
+					      int where)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+	resource_size_t idx = bus->number - pci->cfg.bus_range.start;
+
+	return pci->cfg.win[idx] + ((devfn << 12) | where);
+}
+
+static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = {
+	.bus_shift	= 20,
+	.map_bus	= gen_pci_map_cfg_bus_ecam,
+};
+
+static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn,
+				int where, int size, u32 *val)
+{
+	void __iomem *addr;
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+
+	addr = pci->cfg.ops->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		*val = readb(addr);
+		break;
+	case 2:
+		*val = readw(addr);
+		break;
+	default:
+		*val = readl(addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 val)
+{
+	void __iomem *addr;
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+
+	addr = pci->cfg.ops->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		writeb(val, addr);
+		break;
+	case 2:
+		writew(val, addr);
+		break;
+	default:
+		writel(val, addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops gen_pci_ops = {
+	.read	= gen_pci_config_read,
+	.write	= gen_pci_config_write,
+};
+
+static struct pci_host_bridge_window *
+gen_pci_alloc_init_window(struct device *dev, struct of_pci_range *range,
+			  struct device_node *np)
+{
+	struct pci_host_bridge_window *win;
+
+	win = devm_kzalloc(dev, sizeof(*win), GFP_KERNEL);
+	if (!win)
+		return NULL;
+
+	win->res = devm_kmalloc(dev, sizeof(*win->res), GFP_KERNEL);
+	if (!win->res)
+		goto out_free_win;
+
+	of_pci_range_to_resource(range, np, win->res);
+	INIT_LIST_HEAD(&win->list);
+	return win;
+
+out_free_win:
+	devm_kfree(dev, win);
+	return NULL;
+}
+
+static void gen_pci_free_window(struct device *dev,
+				struct pci_host_bridge_window *win)
+{
+	devm_kfree(dev, win->res);
+	devm_kfree(dev, win);
+}
+
+static int gen_pci_alloc_io_offset(u32 sz, resource_size_t *offset)
+{
+	static DECLARE_BITMAP(io_map, (IO_SPACE_LIMIT + 1) / SZ_64K);
+	int idx, num_wins;
+
+	if (sz > SZ_64K)
+		return -ENOSPC;
+
+	num_wins = (IO_SPACE_LIMIT + 1) / SZ_64K;
+	idx = 0;
+	do {
+		idx = find_next_zero_bit(io_map, num_wins, idx);
+		if (idx == num_wins)
+			return -ENOSPC;
+	} while (test_and_set_bit(idx, io_map));
+
+	*offset = idx * SZ_64K;
+	return 0;
+}
+
+static const struct of_device_id gen_pci_of_match[] = {
+	{ .compatible = "pci-host-cam-generic",
+	  .data = &gen_pci_cfg_cam_bus_ops },
+
+	{ .compatible = "pci-host-ecam-generic",
+	  .data = &gen_pci_cfg_ecam_bus_ops },
+
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gen_pci_of_match);
+
+static int gen_pci_setup(int nr, struct pci_sys_data *sys)
+{
+	int err, res_valid;
+	u8 bus_max;
+	struct pci_host_bridge_window *win, *tmp;
+	struct resource *bus_range;
+	resource_size_t busn;
+	const char *type;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	const struct of_device_id *of_id;
+	struct gen_pci *pci = sys->private_data;
+	struct device *dev = pci->host.dev.parent;
+	struct device_node *np = dev->of_node;
+
+	type = of_get_property(np, "device_type", NULL);
+	if (!type || strcmp(type, "pci")) {
+		dev_err(dev, "invalid \"device_type\" %s\n", type);
+		return -EINVAL;
+	}
+
+	if (of_pci_range_parser_init(&parser, np)) {
+		dev_err(dev, "missing \"ranges\" property\n");
+		return -EINVAL;
+	}
+
+	if (of_pci_parse_bus_range(np, &pci->cfg.bus_range))
+		pci->cfg.bus_range = (struct resource) {
+			.name	= np->name,
+			.start	= 0,
+			.end	= 0xff,
+			.flags	= IORESOURCE_BUS,
+		};
+
+	err = of_address_to_resource(np, 0, &pci->cfg.res);
+	if (err) {
+		dev_err(dev, "missing \"reg\" property\n");
+		return err;
+	}
+
+	pci->cfg.win = devm_kcalloc(dev, resource_size(&pci->cfg.bus_range),
+				    sizeof(*pci->cfg.win), GFP_KERNEL);
+	if (!pci->cfg.win)
+		return -ENOMEM;
+
+	of_id = of_match_node(gen_pci_of_match, np);
+	pci->cfg.ops = of_id->data;
+	INIT_LIST_HEAD(&pci->host.windows);
+
+	/* Limit the bus-range to fit within reg */
+	bus_max = pci->cfg.bus_range.start +
+		  (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
+	pci->cfg.bus_range.end = min_t(resource_size_t, pci->cfg.bus_range.end,
+				       bus_max);
+
+	/* Create windows from the ranges property */
+	for_each_of_pci_range(&parser, &range) {
+		resource_size_t offset;
+		u32 restype = range.flags & IORESOURCE_TYPE_BITS;
+
+		if (restype == IORESOURCE_IO) {
+			err = gen_pci_alloc_io_offset(range.size, &offset);
+			if (err) {
+				dev_warn(dev,
+					 "failed to allocate IO window of %lld bytes\n",
+					 range.size);
+				continue;
+			}
+		} else if (restype == IORESOURCE_MEM) {
+			offset = range.cpu_addr - range.pci_addr;
+		} else {
+			dev_warn(dev,
+				"ignoring unknown/unsupported resource type %x\n",
+				 restype);
+			continue;
+		}
+
+		win = gen_pci_alloc_init_window(dev, &range, np);
+		if (!win) {
+			err = -ENOMEM;
+			goto out_free_res;
+		}
+
+		win->offset = offset;
+		list_add(&win->list, &pci->host.windows);
+	}
+
+	/* Map our Configuration Space windows */
+	if (!request_mem_region(pci->cfg.res.start,
+				resource_size(&pci->cfg.res),
+				"Configuration Space"))
+		goto out_free_res;
+
+	bus_range = &pci->cfg.bus_range;
+	for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
+		u32 idx = busn - bus_range->start;
+		u32 sz = 1 << pci->cfg.ops->bus_shift;
+
+		pci->cfg.win[idx] = devm_ioremap(dev,
+						 pci->cfg.res.start + busn * sz,
+						 sz);
+		if (!pci->cfg.win[idx]) {
+			err = -ENOMEM;
+			goto out_unmap_cfg;
+		}
+	}
+
+	/* Register our I/O and Memory resources */
+	res_valid = 0;
+	list_for_each_entry(win, &pci->host.windows, list) {
+		struct resource *parent;
+
+		if (resource_type(win->res) == IORESOURCE_IO) {
+			parent = &ioport_resource;
+			err = pci_ioremap_io(win->offset, win->res->start);
+			if (err)
+				goto out_release_res;
+		} else {
+			res_valid |= !(win->res->flags & IORESOURCE_PREFETCH);
+			parent = &iomem_resource;
+		}
+
+		err = request_resource(parent, win->res);
+		if (err)
+			goto out_release_res;
+
+		pci_add_resource_offset(&sys->resources, win->res, win->offset);
+	}
+
+	if (!res_valid) {
+		dev_err(dev, "non-prefetchable memory resource required\n");
+		err = -EINVAL;
+		goto out_release_res;
+	}
+
+	/* Register bus resource */
+	pci_add_resource(&sys->resources, bus_range);
+	return 1;
+
+out_release_res:
+	list_for_each_entry_continue_reverse(win, &pci->host.windows, list)
+		release_resource(win->res);
+out_unmap_cfg:
+	while (busn-- > bus_range->start)
+		devm_iounmap(dev, pci->cfg.win[busn-bus_range->start]);
+	release_resource(&pci->cfg.res);
+out_free_res:
+	list_for_each_entry_safe(win, tmp, &pci->host.windows, list)
+		gen_pci_free_window(dev, win);
+	devm_kfree(dev, pci->cfg.win);
+	return err;
+}
+
+static int gen_pci_probe(struct platform_device *pdev)
+{
+	struct hw_pci hw;
+	struct gen_pci *pci;
+	struct device *dev = &pdev->dev;
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->host.dev.parent = dev;
+	hw = (struct hw_pci) {
+		.nr_controllers	= 1,
+		.private_data	= (void **)&pci,
+		.setup		= gen_pci_setup,
+		.map_irq	= of_irq_parse_and_map_pci,
+		.ops		= &gen_pci_ops,
+	};
+
+	pci_common_init_dev(dev, &hw);
+	return 0;
+}
+
+static struct platform_driver gen_pci_driver = {
+	.driver = {
+		.name = "pci-host-generic",
+		.owner = THIS_MODULE,
+		.of_match_table = gen_pci_of_match,
+	},
+	.probe = gen_pci_probe,
+};
+module_platform_driver(gen_pci_driver);
+
+MODULE_DESCRIPTION("Generic PCI host driver");
+MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
+MODULE_LICENSE("GPLv2");
-- 
1.8.2.2

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Tuesday 18 February 2014 12:20:43 Will Deacon wrote:

> +static int gen_pci_alloc_io_offset(u32 sz, resource_size_t *offset)
> +{
> +	static DECLARE_BITMAP(io_map, (IO_SPACE_LIMIT + 1) / SZ_64K);
> +	int idx, num_wins;
> +
> +	if (sz > SZ_64K)
> +		return -ENOSPC;
> +
> +	num_wins = (IO_SPACE_LIMIT + 1) / SZ_64K;
> +	idx = 0;
> +	do {
> +		idx = find_next_zero_bit(io_map, num_wins, idx);
> +		if (idx == num_wins)
> +			return -ENOSPC;
> +	} while (test_and_set_bit(idx, io_map));
> +
> +	*offset = idx * SZ_64K;
> +	return 0;
> +}

Sicne you're always starting the search at 0 and you never free
the map, this is essentially the same as remembering the last number
that was used and using the next one, right?

You should also pass the rang->pci_addr here to calculate the
offset in case the pci_addr is not zero.

> +	/* Register our I/O and Memory resources */
> +	res_valid = 0;
> +	list_for_each_entry(win, &pci->host.windows, list) {
> +		struct resource *parent;
> +
> +		if (resource_type(win->res) == IORESOURCE_IO) {
> +			parent = &ioport_resource;
> +			err = pci_ioremap_io(win->offset, win->res->start);

and consequently pass the pci_addr rather than the offset here. How about
moving the pci_ioremap_io() call into gen_pci_alloc_io_offset()?

	Arnd

[PATCH v3 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Russell King - ARM Linux]

On Tue, Feb 18, 2014 at 12:20:42PM +0000, Will Deacon wrote:
> This patch moves bios32 over to using the generic code for enabling PCI
> resources. Since the core code takes care of bridge resources too, we
> can also drop the explicit IO and MEMORY enabling for them in the arch
> code.
> 
> A side-effect of this change is that we no longer explicitly enable
> devices when running in PCI_PROBE_ONLY mode. This stays closer to the
> meaning of the option and prevents us from trying to enable devices
> without any assigned resources (the core code refuses to enable
> resources without parents).
> 
> Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Tested-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Tested acceptably fine here with crudbus-from-hell.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Jason Gunthorpe]

On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:
> +
> +    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
> +    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool
output. For example purposes any value will do.

Regards,
Jason

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 06:21:42PM +0000, Jason Gunthorpe wrote:
> On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:
> > +
> > +    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
> > +    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
>                                            ^^^^^^^^^^^^^^
> 
> This probably shouldn't be 0 in the example, nor in your kvm tool
> output. For example purposes any value will do.

Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since
there's an 8250 down there. That means we have:

0x0    - 0x6200  : Weird PC stuff
0x6200 - 0x10000 : PCI IO space

Should I just change everything to be offset by 0x6200?

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Tuesday 18 February 2014 18:44:20 Will Deacon wrote:
> On Tue, Feb 18, 2014 at 06:21:42PM +0000, Jason Gunthorpe wrote:
> > On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:
> > > +
> > > +    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
> > > +    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
> >                                            ^^^^^^^^^^^^^^
> > 
> > This probably shouldn't be 0 in the example, nor in your kvm tool
> > output. For example purposes any value will do.
> 
> Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since
> there's an 8250 down there. That means we have:
> 
> 0x0    - 0x6200  : Weird PC stuff
> 0x6200 - 0x10000 : PCI IO space
> 
> Should I just change everything to be offset by 0x6200?

Do you have a list of the stuff in there?

If that is actually port I/O, leaving it in there may be best,
but I agree the example should be changed.

	Arnd

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 06:47:15PM +0000, Arnd Bergmann wrote:
> On Tuesday 18 February 2014 18:44:20 Will Deacon wrote:
> > On Tue, Feb 18, 2014 at 06:21:42PM +0000, Jason Gunthorpe wrote:
> > > On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:
> > > > +
> > > > +    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
> > > > +    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
> > >                                            ^^^^^^^^^^^^^^
> > > 
> > > This probably shouldn't be 0 in the example, nor in your kvm tool
> > > output. For example purposes any value will do.
> > 
> > Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since
> > there's an 8250 down there. That means we have:
> > 
> > 0x0    - 0x6200  : Weird PC stuff
> > 0x6200 - 0x10000 : PCI IO space
> > 
> > Should I just change everything to be offset by 0x6200?
> 
> Do you have a list of the stuff in there?

kvmtool provides an i8042, pci-shmem, an rtc, 8250 and vesa at the usual PC
offsets. On ARM, I regularly use the 8250 (there are even DT bindings for
it) and MarcZ got the rtc working for fun. Not tried the others though.

> If that is actually port I/O, leaving it in there may be best,
> but I agree the example should be changed.

Yup, it is port I/O. I'll make something up for the example.

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Jason Gunthorpe]

On Tue, Feb 18, 2014 at 06:44:20PM +0000, Will Deacon wrote:
> On Tue, Feb 18, 2014 at 06:21:42PM +0000, Jason Gunthorpe wrote:
> > On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:
> > > +
> > > +    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
> > > +    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
> >                                            ^^^^^^^^^^^^^^
> > 
> > This probably shouldn't be 0 in the example, nor in your kvm tool
> > output. For example purposes any value will do.
> 
> Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since
> there's an 8250 down there. That means we have:
> 
> 0x0    - 0x6200  : Weird PC stuff
> 0x6200 - 0x10000 : PCI IO space
> 
> Should I just change everything to be offset by 0x6200?

Just to be clear, kvm has reserved the first 64k of physical address
space for this IO window? The 0 is fine then, but it isn't typical of
real hardware.

Regarding the 0x6200.. There are two conflicting issues there :(
 - You really don't want to let the PCI core assign resources to that
   range, it probably won't work.
 - You probably need that range mapped into the ARM IO window, and the
   PCI driver is the thing doing that

The PCI core already has a black out for low IO ports, I think it
already covers the usual PC suspects so you are probably fine with
what you have for KVM.

Jason

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 06:59:47PM +0000, Jason Gunthorpe wrote:
> On Tue, Feb 18, 2014 at 06:44:20PM +0000, Will Deacon wrote:
> > On Tue, Feb 18, 2014 at 06:21:42PM +0000, Jason Gunthorpe wrote:
> > > On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:
> > > > +
> > > > +    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
> > > > +    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
> > >                                            ^^^^^^^^^^^^^^
> > > 
> > > This probably shouldn't be 0 in the example, nor in your kvm tool
> > > output. For example purposes any value will do.
> > 
> > Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since
> > there's an 8250 down there. That means we have:
> > 
> > 0x0    - 0x6200  : Weird PC stuff
> > 0x6200 - 0x10000 : PCI IO space
> > 
> > Should I just change everything to be offset by 0x6200?
> 
> Just to be clear, kvm has reserved the first 64k of physical address
> space for this IO window? The 0 is fine then, but it isn't typical of
> real hardware.

Yup, that's spot on.

> Regarding the 0x6200.. There are two conflicting issues there :(
>  - You really don't want to let the PCI core assign resources to that
>    range, it probably won't work.

Right, with kvmtool we don't support resource assignment (the BARs are fixed)
so everything is PCI_PROBE_ONLY.

>  - You probably need that range mapped into the ARM IO window, and the
>    PCI driver is the thing doing that
> 
> The PCI core already has a black out for low IO ports, I think it
> already covers the usual PC suspects so you are probably fine with
> what you have for KVM.

Great, so I'll just update the example then. Thanks!

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 01:46:44PM +0000, Arnd Bergmann wrote:
> On Tuesday 18 February 2014 12:20:43 Will Deacon wrote:
> 
> > +static int gen_pci_alloc_io_offset(u32 sz, resource_size_t *offset)
> > +{
> > +	static DECLARE_BITMAP(io_map, (IO_SPACE_LIMIT + 1) / SZ_64K);
> > +	int idx, num_wins;
> > +
> > +	if (sz > SZ_64K)
> > +		return -ENOSPC;
> > +
> > +	num_wins = (IO_SPACE_LIMIT + 1) / SZ_64K;
> > +	idx = 0;
> > +	do {
> > +		idx = find_next_zero_bit(io_map, num_wins, idx);
> > +		if (idx == num_wins)
> > +			return -ENOSPC;
> > +	} while (test_and_set_bit(idx, io_map));
> > +
> > +	*offset = idx * SZ_64K;
> > +	return 0;
> > +}
> 
> Sicne you're always starting the search at 0 and you never free
> the map, this is essentially the same as remembering the last number
> that was used and using the next one, right?

Yes, I can implement that easily enough. I was wondering if there was a case
for allowing windows to be freed, but it doesn't feel especially compelling.

> You should also pass the rang->pci_addr here to calculate the
> offset in case the pci_addr is not zero.
> 
> > +	/* Register our I/O and Memory resources */
> > +	res_valid = 0;
> > +	list_for_each_entry(win, &pci->host.windows, list) {
> > +		struct resource *parent;
> > +
> > +		if (resource_type(win->res) == IORESOURCE_IO) {
> > +			parent = &ioport_resource;
> > +			err = pci_ioremap_io(win->offset, win->res->start);
> 
> and consequently pass the pci_addr rather than the offset here. How about
> moving the pci_ioremap_io() call into gen_pci_alloc_io_offset()?

Can do.

Thanks for the feedback.

Will

[PATCH v3 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Will Deacon]

On Tue, Feb 18, 2014 at 03:41:39PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 18, 2014 at 12:20:42PM +0000, Will Deacon wrote:
> > This patch moves bios32 over to using the generic code for enabling PCI
> > resources. Since the core code takes care of bridge resources too, we
> > can also drop the explicit IO and MEMORY enabling for them in the arch
> > code.
> > 
> > A side-effect of this change is that we no longer explicitly enable
> > devices when running in PCI_PROBE_ONLY mode. This stays closer to the
> > meaning of the option and prevents us from trying to enable devices
> > without any assigned resources (the core code refuses to enable
> > resources without parents).
> > 
> > Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Tested-by: Jingoo Han <jg1.han@samsung.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> Tested acceptably fine here with crudbus-from-hell.
> 
> Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

Cheers Russell.

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Tuesday 18 February 2014 18:51:50 Will Deacon wrote:
> 
> kvmtool provides an i8042, pci-shmem, an rtc, 8250 and vesa at the usual PC
> offsets. On ARM, I regularly use the 8250 (there are even DT bindings for
> it) and MarcZ got the rtc working for fun. Not tried the others though.

Ah, interesting. But which ones are between 0x1000-0x6200? The ones
you list should all be in the ISA range between 0x0-0xfff as far
as I can tell, which we keep clear of by setting PCIBIOS_MIN_IO
to 0x1000.

	Arnd

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 07:11:25PM +0000, Arnd Bergmann wrote:
> On Tuesday 18 February 2014 18:51:50 Will Deacon wrote:
> > 
> > kvmtool provides an i8042, pci-shmem, an rtc, 8250 and vesa at the usual PC
> > offsets. On ARM, I regularly use the 8250 (there are even DT bindings for
> > it) and MarcZ got the rtc working for fun. Not tried the others though.
> 
> Ah, interesting. But which ones are between 0x1000-0x6200? The ones
> you list should all be in the ISA range between 0x0-0xfff as far
> as I can tell, which we keep clear of by setting PCIBIOS_MIN_IO
> to 0x1000.

I think there's just a hole between 0x1000 - 0x6200.

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:
> 
> > Regarding the 0x6200.. There are two conflicting issues there 
> >  - You really don't want to let the PCI core assign resources to that
> >    range, it probably won't work.
> 
> Right, with kvmtool we don't support resource assignment (the BARs are fixed)
> so everything is PCI_PROBE_ONLY.

Ok, I looked at the source now and can confirm:

* 0x0-0x1000 are used for lots of legacy ISA devices.
* PCI devices get assigned IO addresses in 0x400 steps starting at 0x6200.
* There are three PCI drivers doing this: VESA, PCI-SHMEM and virtio-pci.

Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we
have a standard DT property for that? On PowerPC we already specified
"linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems
reasonable to use in architecture independent code as well.

	Arnd

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 07:32:33PM +0000, Arnd Bergmann wrote:
> On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:
> > 
> > > Regarding the 0x6200.. There are two conflicting issues there 
> > >  - You really don't want to let the PCI core assign resources to that
> > >    range, it probably won't work.
> > 
> > Right, with kvmtool we don't support resource assignment (the BARs are fixed)
> > so everything is PCI_PROBE_ONLY.
> 
> Ok, I looked at the source now and can confirm:
> 
> * 0x0-0x1000 are used for lots of legacy ISA devices.
> * PCI devices get assigned IO addresses in 0x400 steps starting at 0x6200.
> * There are three PCI drivers doing this: VESA, PCI-SHMEM and virtio-pci.
> 
> Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we
> have a standard DT property for that? On PowerPC we already specified
> "linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems
> reasonable to use in architecture independent code as well.

For arch/arm/ it's done on the command line (pci=firmware) but yes, for the
generic driver it sounds like a good idea to put this in the device-tree.
I'll look at adding the ppc properties in the next version.

Cheers,

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 07:36:54PM +0000, Will Deacon wrote:
> On Tue, Feb 18, 2014 at 07:32:33PM +0000, Arnd Bergmann wrote:
> > On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:
> > > 
> > > > Regarding the 0x6200.. There are two conflicting issues there 
> > > >  - You really don't want to let the PCI core assign resources to that
> > > >    range, it probably won't work.
> > > 
> > > Right, with kvmtool we don't support resource assignment (the BARs are fixed)
> > > so everything is PCI_PROBE_ONLY.
> > 
> > Ok, I looked at the source now and can confirm:
> > 
> > * 0x0-0x1000 are used for lots of legacy ISA devices.
> > * PCI devices get assigned IO addresses in 0x400 steps starting at 0x6200.
> > * There are three PCI drivers doing this: VESA, PCI-SHMEM and virtio-pci.
> > 
> > Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we
> > have a standard DT property for that? On PowerPC we already specified
> > "linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems
> > reasonable to use in architecture independent code as well.
> 
> For arch/arm/ it's done on the command line (pci=firmware) but yes, for the
> generic driver it sounds like a good idea to put this in the device-tree.
> I'll look at adding the ppc properties in the next version.

Damn, I spoke too soon. PowerPC puts this information in the /chosen node, so
it applies to all PCI host controllers in the system. That then maps nicely
to pci_{add,clear}_flags which are global properties in Linux.

It sounds like this should really be per-controller. What do you think?

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Tuesday 18 February 2014 18:44:20 Will Deacon wrote:
> On Tue, Feb 18, 2014 at 06:21:42PM +0000, Jason Gunthorpe wrote:
> > On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:
> > > +
> > > +    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
> > > +    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
> >                                            ^^^^^^^^^^^^^^
> > 
> > This probably shouldn't be 0 in the example, nor in your kvm tool
> > output. For example purposes any value will do.
> 
> Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since
> there's an 8250 down there. That means we have:
> 
> 0x0    - 0x6200  : Weird PC stuff
> 0x6200 - 0x10000 : PCI IO space
> 
> Should I just change everything to be offset by 0x6200?

Some more comments about this:

* After I looked at the kvmtool code some more, I think it would be nice to
  actually move the I/O space window to a nonzero location, e.g. between the
  PCI space and the AXI space. It's really confusing the way it is, and easy
  to introduce bugs if you have any code that accidentally relies on the
  numbers matching up. For all I can tell, kvmtool should just work fine
  with any definition of ARM_IOPORT_AREA, as long as you apply the trivial
  patch below.

* pci_get_io_space_block() is a very confusing name for a function that
  allocates a memory space block in kvmtool. It seems that KVM_IOPORT_AREA
  is a misnomer for the same reason. Note that PowerPC sets KVM_IOPORT_AREA
  to zero, but has the I/O space window at SPAPR_PCI_IO_WIN_ADDR, 
  which is non-zero.

* It's interesting that you also support I/O space access to the PCI config
  space through PC-Style ports 0cf8-0cff. Not sure if that helps at all,
  but it is another standard way to probe the bus that could easily be
  implemented as an alternative to CAM and ECAM, in a generic PCI host
  driver.

	Arnd

diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c
index b017994..caf9a57 100644
--- a/tools/kvm/arm/kvm-cpu.c
+++ b/tools/kvm/arm/kvm-cpu.c
@@ -105,7 +105,7 @@ bool kvm_cpu__emulate_mmio(struct kvm *kvm, u64 phys_addr, u8 *data, u32 len,
 		return kvm__emulate_mmio(kvm, phys_addr, data, len, is_write);
 	} else if (arm_addr_in_ioport_region(phys_addr)) {
 		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
-		u16 port = phys_addr & USHRT_MAX;
+		u16 port = (phys_addr - ARM_IOPORT_AREA) & USHRT_MAX;
 		return kvm__emulate_io(kvm, port, data, direction, len, 1);
 	} else if (arm_addr_in_pci_region(phys_addr)) {
 		return kvm__emulate_mmio(kvm, phys_addr, data, len, is_write);

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Tuesday 18 February 2014 19:57:32 Will Deacon wrote:
> On Tue, Feb 18, 2014 at 07:36:54PM +0000, Will Deacon wrote:
> > On Tue, Feb 18, 2014 at 07:32:33PM +0000, Arnd Bergmann wrote:
> > > On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:
> > > Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we
> > > have a standard DT property for that? On PowerPC we already specified
> > > "linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems
> > > reasonable to use in architecture independent code as well.
> > 
> > For arch/arm/ it's done on the command line (pci=firmware) but yes, for the
> > generic driver it sounds like a good idea to put this in the device-tree.
> > I'll look at adding the ppc properties in the next version.
> 
> Damn, I spoke too soon. PowerPC puts this information in the /chosen node, so
> it applies to all PCI host controllers in the system. That then maps nicely
> to pci_{add,clear}_flags which are global properties in Linux.
> 
> It sounds like this should really be per-controller. What do you think?

I don't have a strong opinion either way. I think the reason for having it
in the /chosen node is so that a second-stage boot loader can easily
override it, which is not a concern for us. In particular, the properties
are only used on rtas-pci, which means we access the config space to
firmware runtime abstraction services rather than hardware registers.

Apparently the embedded powerpc platforms don't support these flags at
all, which I also didn't notice at first.

	Arnd

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 08:15:12PM +0000, Arnd Bergmann wrote:
> On Tuesday 18 February 2014 18:44:20 Will Deacon wrote:
> > On Tue, Feb 18, 2014 at 06:21:42PM +0000, Jason Gunthorpe wrote:
> > > On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:
> > > > +
> > > > +    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
> > > > +    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
> > >                                            ^^^^^^^^^^^^^^
> > > 
> > > This probably shouldn't be 0 in the example, nor in your kvm tool
> > > output. For example purposes any value will do.
> > 
> > Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since
> > there's an 8250 down there. That means we have:
> > 
> > 0x0    - 0x6200  : Weird PC stuff
> > 0x6200 - 0x10000 : PCI IO space
> > 
> > Should I just change everything to be offset by 0x6200?
> 
> Some more comments about this:
> 
> * After I looked at the kvmtool code some more, I think it would be nice to
>   actually move the I/O space window to a nonzero location, e.g. between the
>   PCI space and the AXI space. It's really confusing the way it is, and easy
>   to introduce bugs if you have any code that accidentally relies on the
>   numbers matching up. For all I can tell, kvmtool should just work fine
>   with any definition of ARM_IOPORT_AREA, as long as you apply the trivial
>   patch below.

Well, I think we need a bit more than that otherwise our I/O BARs won't be
initialised correctly.

> * pci_get_io_space_block() is a very confusing name for a function that
>   allocates a memory space block in kvmtool. It seems that KVM_IOPORT_AREA
>   is a misnomer for the same reason. Note that PowerPC sets KVM_IOPORT_AREA
>   to zero, but has the I/O space window at SPAPR_PCI_IO_WIN_ADDR, 
>   which is non-zero.

Agreed about pci_get_io_space_block being a misnomer, that's probably due
some cleanup. I don't think KVM_IOPORT_AREA is too bad though (it's zero for
PowerPC because I added it and don't have a platform to test on).

> * It's interesting that you also support I/O space access to the PCI config
>   space through PC-Style ports 0cf8-0cff. Not sure if that helps at all,
>   but it is another standard way to probe the bus that could easily be
>   implemented as an alternative to CAM and ECAM, in a generic PCI host
>   driver.

I won't add that initially, but if somebody needs it then it's easy to
extend what we currently have.

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 08:19:21PM +0000, Arnd Bergmann wrote:
> On Tuesday 18 February 2014 19:57:32 Will Deacon wrote:
> > On Tue, Feb 18, 2014 at 07:36:54PM +0000, Will Deacon wrote:
> > > On Tue, Feb 18, 2014 at 07:32:33PM +0000, Arnd Bergmann wrote:
> > > > On Tuesday 18 February 2014 19:09:29 Will Deacon wrote:
> > > > Regarding the PCI_PROBE_ONLY flag, how do you set that? Should we
> > > > have a standard DT property for that? On PowerPC we already specified
> > > > "linux,pci-probe-only" and "linux,pci-assign-all-buses", which seems
> > > > reasonable to use in architecture independent code as well.
> > > 
> > > For arch/arm/ it's done on the command line (pci=firmware) but yes, for the
> > > generic driver it sounds like a good idea to put this in the device-tree.
> > > I'll look at adding the ppc properties in the next version.
> > 
> > Damn, I spoke too soon. PowerPC puts this information in the /chosen node, so
> > it applies to all PCI host controllers in the system. That then maps nicely
> > to pci_{add,clear}_flags which are global properties in Linux.
> > 
> > It sounds like this should really be per-controller. What do you think?
> 
> I don't have a strong opinion either way. I think the reason for having it
> in the /chosen node is so that a second-stage boot loader can easily
> override it, which is not a concern for us. In particular, the properties
> are only used on rtas-pci, which means we access the config space to
> firmware runtime abstraction services rather than hardware registers.
> 
> Apparently the embedded powerpc platforms don't support these flags at
> all, which I also didn't notice at first.

Even in the rtas case there's a:

	#ifdef CONFIG_PPC32 /* Will be made generic soon */

for linux,pci-assign-all-buses. I think I'll just go for
linux,pci-probe-only initially, especially as PCI_REASSIGN_ALL_BUS isn't
actually used anywhere outside of arch/powerpc/.

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Tue, Feb 18, 2014 at 07:10:23PM +0000, Will Deacon wrote:
> On Tue, Feb 18, 2014 at 01:46:44PM +0000, Arnd Bergmann wrote:
> > On Tuesday 18 February 2014 12:20:43 Will Deacon wrote:
> > > +	/* Register our I/O and Memory resources */
> > > +	res_valid = 0;
> > > +	list_for_each_entry(win, &pci->host.windows, list) {
> > > +		struct resource *parent;
> > > +
> > > +		if (resource_type(win->res) == IORESOURCE_IO) {
> > > +			parent = &ioport_resource;
> > > +			err = pci_ioremap_io(win->offset, win->res->start);
> > 
> > and consequently pass the pci_addr rather than the offset here. How about
> > moving the pci_ioremap_io() call into gen_pci_alloc_io_offset()?

I've probably just confused myself, but passing the pci_addr to
pci_ioremap_io doesn't make sense to me. My understanding is that:

  cpu = bus + offset

In the case of I/O, the offset is really:

  offset = (PCI_IO_VIRT_BASE - bus) + window

where window is determined by the simple allocator I wrote.

Now, the __io macro takes care of PCI_IO_VIRT_BASE so we don't actually care
about that when adding the PCI I/O resources, instead we'll just pass:

  offset = window - bus

and then pci_ioremap_io will just want the window offset, since that's added
directly on to PCI_IO_VIRT_BASE for the ioremap_page_range.

If I call pci_ioremap_io(range->pci_addr, ...) then I'm going to trip a BUG_ON
unless the pci_addr is within IO_SPACE_LIMIT.

Will

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Wednesday 19 February 2014 11:07:19 Will Deacon wrote:
> On Tue, Feb 18, 2014 at 07:10:23PM +0000, Will Deacon wrote:
> > On Tue, Feb 18, 2014 at 01:46:44PM +0000, Arnd Bergmann wrote:
> > > On Tuesday 18 February 2014 12:20:43 Will Deacon wrote:
> > > > + /* Register our I/O and Memory resources */
> > > > + res_valid = 0;
> > > > + list_for_each_entry(win, &pci->host.windows, list) {
> > > > +         struct resource *parent;
> > > > +
> > > > +         if (resource_type(win->res) == IORESOURCE_IO) {
> > > > +                 parent = &ioport_resource;
> > > > +                 err = pci_ioremap_io(win->offset, win->res->start);
> > > 
> > > and consequently pass the pci_addr rather than the offset here. How about
> > > moving the pci_ioremap_io() call into gen_pci_alloc_io_offset()?
> 
> I've probably just confused myself, but passing the pci_addr to
> pci_ioremap_io doesn't make sense to me. 

I think the confusion is that there are two different things we call
offset here. The calculation of the offset you pass into
pci_ioremap_io() is correct AFAICT now, but it's not what you are
supposed to pass into pci_add_resource_offset() or what we normally
put into pci_host_bridge_window->offset.

> My understanding is that:
> 
>   cpu = bus + offset

Right. This would be the case for mem_offset.

> In the case of I/O, the offset is really:
> 
>   offset = (PCI_IO_VIRT_BASE - bus) + window

I can't seem to make sense of this calculation. PCI_IO_VIRT_BASE
is a pointer to the start of the virtual window. You can add offsets
to the pointer, but subtracting a number from it is not a well-defined
operation.

> where window is determined by the simple allocator I wrote.

And your allocator calls it offset, which is what confused me.

> Now, the __io macro takes care of PCI_IO_VIRT_BASE so we don't actually care
> about that when adding the PCI I/O resources, instead we'll just pass:
> 
>   offset = window - bus

Yes, this would be the io_offset that you pass into pci_add_resource_offset().

> and then pci_ioremap_io will just want the window offset, since that's added
> directly on to PCI_IO_VIRT_BASE for the ioremap_page_range.

Yes.

> If I call pci_ioremap_io(range->pci_addr, ...) then I'm going to trip a BUG_ON
> unless the pci_addr is within IO_SPACE_LIMIT.

range->pci_addr is what you call 'bus' above. Since you want 'window', you
have to pass 'bus'+'offset', or 'range->pci_addr + io_offset'. Normally, one
of the two would be zero, while the other is equal to 'window'.

	Arnd

[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

Hi Arnd,

On Wed, Feb 19, 2014 at 02:17:56PM +0000, Arnd Bergmann wrote:
> On Wednesday 19 February 2014 11:07:19 Will Deacon wrote:
> > I've probably just confused myself, but passing the pci_addr to
> > pci_ioremap_io doesn't make sense to me. 
> 
> I think the confusion is that there are two different things we call
> offset here. The calculation of the offset you pass into
> pci_ioremap_io() is correct AFAICT now, but it's not what you are
> supposed to pass into pci_add_resource_offset() or what we normally
> put into pci_host_bridge_window->offset.
> 
> > My understanding is that:
> > 
> >   cpu = bus + offset
> 
> Right. This would be the case for mem_offset.
> 
> > In the case of I/O, the offset is really:
> > 
> >   offset = (PCI_IO_VIRT_BASE - bus) + window
> 
> I can't seem to make sense of this calculation. PCI_IO_VIRT_BASE
> is a pointer to the start of the virtual window. You can add offsets
> to the pointer, but subtracting a number from it is not a well-defined
> operation.
> 
> > where window is determined by the simple allocator I wrote.
> 
> And your allocator calls it offset, which is what confused me.
> 
> > Now, the __io macro takes care of PCI_IO_VIRT_BASE so we don't actually care
> > about that when adding the PCI I/O resources, instead we'll just pass:
> > 
> >   offset = window - bus
> 
> Yes, this would be the io_offset that you pass into pci_add_resource_offset().
> 
> > and then pci_ioremap_io will just want the window offset, since that's added
> > directly on to PCI_IO_VIRT_BASE for the ioremap_page_range.
> 
> Yes.
> 
> > If I call pci_ioremap_io(range->pci_addr, ...) then I'm going to trip a BUG_ON
> > unless the pci_addr is within IO_SPACE_LIMIT.
> 
> range->pci_addr is what you call 'bus' above. Since you want 'window', you
> have to pass 'bus'+'offset', or 'range->pci_addr + io_offset'. Normally, one
> of the two would be zero, while the other is equal to 'window'.

Good, so we're in agreement! The issue indeed seems to be that there are
multiple names for the same things :)

Will

[PATCH v3 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Will Deacon]

On Tue, Feb 18, 2014 at 03:41:39PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 18, 2014 at 12:20:42PM +0000, Will Deacon wrote:
> > This patch moves bios32 over to using the generic code for enabling PCI
> > resources. Since the core code takes care of bridge resources too, we
> > can also drop the explicit IO and MEMORY enabling for them in the arch
> > code.
> > 
> > A side-effect of this change is that we no longer explicitly enable
> > devices when running in PCI_PROBE_ONLY mode. This stays closer to the
> > meaning of the option and prevents us from trying to enable devices
> > without any assigned resources (the core code refuses to enable
> > resources without parents).
> > 
> > Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Tested-by: Jingoo Han <jg1.han@samsung.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> Tested acceptably fine here with crudbus-from-hell.
> 
> Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

Damn, I just found a problem with this patch when PCI_PROBE_ONLY is set.

The problem is that bios32.c won't create a resource hierarchy for the
firmware-initialised resources, so we have a bunch of orphaned resources
that we can't pass to pci_enable_resources (since it checks r->parent).

This means that, unless firmware *enables* all of the resources, Linux won't
be able to enable them. I think this is stronger than simply not
re-assigning devices like the PCI_PROBE_ONLY flag intends.

I can see two solutions:

  (1) Just drop this patch and stick with the old code (which doesn't check
      r->parent). The downside is we still don't have any resource
      hierarchy, so /proc/iomem doesn't contain bus information.

  (2) Walk over the bus claiming the resources that we find. Patch below.

Any thoughts?

Will

--->8

commit 802062290eee961362d3090a50cf851412a63314
Author: Will Deacon <will.deacon@arm.com>
Date:   Wed Feb 19 23:29:30 2014 +0000

    ARM: bios32: claim firmware-initialised resources when PCI_PROBE_ONLY
    
    When initialising a PCI bus with the PCI_PROBE_ONLY flag set, we don't
    (re-)assign any firmware-initialised resources, leading to an incomplete
    resource hierarchy and devices with orphaned resources.
    
    If we try to enable these orphaned resources using pci_enable_resources,
    we will get back -EINVAL since a valid ->parent pointer is required.
    Consequently, pcibios_enable_device doesn't attempt to enable devices
    when PCI_PROBE_ONLY is set. This has the unfortunate effect of requiring
    the firmware to *enable* all of the devices!
    
    This patch fixes the problem by building up a resource hierarchy from
    the firmware-initialised resources when PCI_PROBE_ONLY is set.
    
    Signed-off-by: Will Deacon <will.deacon@arm.com>

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 91f48804e3bb..d65c76a4d7ad 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -514,6 +514,35 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 	}
 }
 
+static void pcibios_claim_one_bus(struct pci_bus *bus)
+{
+	struct pci_dev *dev;
+	struct pci_bus *child_bus;
+
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		int i;
+
+		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+			struct resource *r = &dev->resource[i];
+
+			if (r->parent || !r->start || !r->flags)
+				continue;
+
+			pr_debug("PCI: Claiming %s: "
+				 "Resource %d: %016llx..%016llx [%x]\n",
+				 pci_name(dev), i,
+				 (unsigned long long)r->start,
+				 (unsigned long long)r->end,
+				 (unsigned int)r->flags);
+
+			pci_claim_resource(dev, i);
+		}
+	}
+
+	list_for_each_entry(child_bus, &bus->children, node)
+		pcibios_claim_one_bus(child_bus);
+}
+
 void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 {
 	struct pci_sys_data *sys;
@@ -541,6 +570,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 			 * Assign resources.
 			 */
 			pci_bus_assign_resources(bus);
+		} else {
+			pcibios_claim_one_bus(bus);
 		}
 
 		/*
@@ -608,9 +639,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
  */
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	if (pci_has_flag(PCI_PROBE_ONLY))
-		return 0;
-
 	return pci_enable_resources(dev, mask);
 }
 

[PATCH v3 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Bjorn Helgaas]

On Thu, Feb 20, 2014 at 4:12 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Feb 18, 2014 at 03:41:39PM +0000, Russell King - ARM Linux wrote:
>> On Tue, Feb 18, 2014 at 12:20:42PM +0000, Will Deacon wrote:
>> > This patch moves bios32 over to using the generic code for enabling PCI
>> > resources. Since the core code takes care of bridge resources too, we
>> > can also drop the explicit IO and MEMORY enabling for them in the arch
>> > code.
>> >
>> > A side-effect of this change is that we no longer explicitly enable
>> > devices when running in PCI_PROBE_ONLY mode. This stays closer to the
>> > meaning of the option and prevents us from trying to enable devices
>> > without any assigned resources (the core code refuses to enable
>> > resources without parents).
>> >
>> > Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> > Tested-by: Jingoo Han <jg1.han@samsung.com>
>> > Signed-off-by: Will Deacon <will.deacon@arm.com>
>>
>> Tested acceptably fine here with crudbus-from-hell.
>>
>> Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
>
> Damn, I just found a problem with this patch when PCI_PROBE_ONLY is set.
>
> The problem is that bios32.c won't create a resource hierarchy for the
> firmware-initialised resources, so we have a bunch of orphaned resources
> that we can't pass to pci_enable_resources (since it checks r->parent).
>
> This means that, unless firmware *enables* all of the resources, Linux won't
> be able to enable them. I think this is stronger than simply not
> re-assigning devices like the PCI_PROBE_ONLY flag intends.

By "firmware enabling resources," do you mean firmware assigning
addresses in the BARs and turning on the PCI_COMMAND_MEMORY (or _IO)
bits?

I'd like to make the generic code ignore BAR values if
PCI_COMMAND_MEMORY (or _IO) is cleared.  When those bits are cleared,
I don't think there's a good way to determine whether the address in a
BAR is valid.

It sounds like you want PCI_PROBE_ONLY to mean that we should try to
use the existing BAR contents even if PCI_COMMAND_MEMORY is cleared.
Is that right?  Maybe we could try to use the existing BAR address if
we can actually claim it, i.e., if it is inside one of the upstream
bridge windows.

> I can see two solutions:
>
>   (1) Just drop this patch and stick with the old code (which doesn't check
>       r->parent). The downside is we still don't have any resource
>       hierarchy, so /proc/iomem doesn't contain bus information.
>
>   (2) Walk over the bus claiming the resources that we find. Patch below.
>
> Any thoughts?

I really want to nuke all the arch-specific "claim resource" code
because it isn't arch-dependent.

> commit 802062290eee961362d3090a50cf851412a63314
> Author: Will Deacon <will.deacon@arm.com>
> Date:   Wed Feb 19 23:29:30 2014 +0000
>
>     ARM: bios32: claim firmware-initialised resources when PCI_PROBE_ONLY
>
>     When initialising a PCI bus with the PCI_PROBE_ONLY flag set, we don't
>     (re-)assign any firmware-initialised resources, leading to an incomplete
>     resource hierarchy and devices with orphaned resources.
>
>     If we try to enable these orphaned resources using pci_enable_resources,
>     we will get back -EINVAL since a valid ->parent pointer is required.
>     Consequently, pcibios_enable_device doesn't attempt to enable devices
>     when PCI_PROBE_ONLY is set. This has the unfortunate effect of requiring
>     the firmware to *enable* all of the devices!
>
>     This patch fixes the problem by building up a resource hierarchy from
>     the firmware-initialised resources when PCI_PROBE_ONLY is set.
>
>     Signed-off-by: Will Deacon <will.deacon@arm.com>
>
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 91f48804e3bb..d65c76a4d7ad 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -514,6 +514,35 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>         }
>  }
>
> +static void pcibios_claim_one_bus(struct pci_bus *bus)
> +{
> +       struct pci_dev *dev;
> +       struct pci_bus *child_bus;
> +
> +       list_for_each_entry(dev, &bus->devices, bus_list) {
> +               int i;
> +
> +               for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> +                       struct resource *r = &dev->resource[i];
> +
> +                       if (r->parent || !r->start || !r->flags)
> +                               continue;
> +
> +                       pr_debug("PCI: Claiming %s: "
> +                                "Resource %d: %016llx..%016llx [%x]\n",
> +                                pci_name(dev), i,
> +                                (unsigned long long)r->start,
> +                                (unsigned long long)r->end,
> +                                (unsigned int)r->flags);
> +
> +                       pci_claim_resource(dev, i);
> +               }
> +       }
> +
> +       list_for_each_entry(child_bus, &bus->children, node)
> +               pcibios_claim_one_bus(child_bus);
> +}
> +
>  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  {
>         struct pci_sys_data *sys;
> @@ -541,6 +570,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>                          * Assign resources.
>                          */
>                         pci_bus_assign_resources(bus);
> +               } else {
> +                       pcibios_claim_one_bus(bus);
>                 }
>
>                 /*
> @@ -608,9 +639,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>   */
>  int pcibios_enable_device(struct pci_dev *dev, int mask)
>  {
> -       if (pci_has_flag(PCI_PROBE_ONLY))
> -               return 0;
> -
>         return pci_enable_resources(dev, mask);
>  }
>

[PATCH v3 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Jason Gunthorpe]

On Thu, Feb 20, 2014 at 12:39:16PM -0700, Bjorn Helgaas wrote:
> > Damn, I just found a problem with this patch when PCI_PROBE_ONLY is set.
> >
> > The problem is that bios32.c won't create a resource hierarchy for the
> > firmware-initialised resources, so we have a bunch of orphaned resources
> > that we can't pass to pci_enable_resources (since it checks r->parent).
> >
> > This means that, unless firmware *enables* all of the resources, Linux won't
> > be able to enable them. I think this is stronger than simply not
> > re-assigning devices like the PCI_PROBE_ONLY flag intends.
> 
> By "firmware enabling resources," do you mean firmware assigning
> addresses in the BARs and turning on the PCI_COMMAND_MEMORY (or _IO)
> bits?
> 
> I'd like to make the generic code ignore BAR values if
> PCI_COMMAND_MEMORY (or _IO) is cleared.  When those bits are cleared,
> I don't think there's a good way to determine whether the address in a
> BAR is valid.

I think this is pretty smart, PROBE_ONLY really should mean
'everything is perfect, do not touch it' and if the device isn't
enabled, well.. It isn't enabled, the firmware should have done it.

Will, this if for kvmtool right? Keeping the patch as is and instead
changing kvmtool to enable the devices seems like a good option?

Alternatively, there are OF ways to communicate the BAR assignment
through the DT. I actually have no idea how this information is used
by the core code, but it might let you avoid the probe_only by
specifying the desired BAR value through the DT.

Regards,
Jason

[PATCH v3 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Will Deacon]

Hi Jason, Bjorn,

Thanks for the comments.

On Fri, Feb 21, 2014 at 12:36:19AM +0000, Jason Gunthorpe wrote:
> On Thu, Feb 20, 2014 at 12:39:16PM -0700, Bjorn Helgaas wrote:
> > > Damn, I just found a problem with this patch when PCI_PROBE_ONLY is set.
> > >
> > > The problem is that bios32.c won't create a resource hierarchy for the
> > > firmware-initialised resources, so we have a bunch of orphaned resources
> > > that we can't pass to pci_enable_resources (since it checks r->parent).
> > >
> > > This means that, unless firmware *enables* all of the resources, Linux won't
> > > be able to enable them. I think this is stronger than simply not
> > > re-assigning devices like the PCI_PROBE_ONLY flag intends.
> > 
> > By "firmware enabling resources," do you mean firmware assigning
> > addresses in the BARs and turning on the PCI_COMMAND_MEMORY (or _IO)
> > bits?
> > 
> > I'd like to make the generic code ignore BAR values if
> > PCI_COMMAND_MEMORY (or _IO) is cleared.  When those bits are cleared,
> > I don't think there's a good way to determine whether the address in a
> > BAR is valid.
> 
> I think this is pretty smart, PROBE_ONLY really should mean
> 'everything is perfect, do not touch it' and if the device isn't
> enabled, well.. It isn't enabled, the firmware should have done it.

Yes, that's one (sane) interpretation of the PROBE_ONLY flag and I'm happy
to run with it if we all agree. We'll need some extra code paths to assign
disabled resources when PROBE_ONLY is passed, but that can come later.

> Will, this if for kvmtool right? Keeping the patch as is and instead
> changing kvmtool to enable the devices seems like a good option?

Sure, I can do that easily enough. I just wanted to make sure that we agree
on PROBE_ONLY before I start hacking kvmtool. I'll drop this additional
patch.

Cheers,

Will