[PATCHv6 0/7] Driver for new "VMD" device

[PATCHv6 0/7] Driver for new "VMD" device

[Keith Busch]

v5 -> v6:

  Fixed kernel doc.

  Fixed S-o-B on PATCH 1/7.

  Added driver power management to save and restoring VMD pci state.

  Allow VMD domains to be accisble to aer_inject, which requires a minor
  change to use 32-bit pci domains.

  Changed child bus resource conflict detection on walking the pci
  domain. If the bus resource aperture is not large enough, we can
  enumerate only a sub-tree of the topology. A bridge device with
  subordinate outside the range should not be allocated. Checking for the
  conflict after the child is allocated retains pointer to the removed
  subordinate bus. The way to fix that is to call "pci_remove_bus_device"
  instead, but I don't think we want to remove the bridge dev since it
  is accessible, albeit not very useful as a bridge device.

Keith Busch (6):
  pci: child bus alloc fix on constrained resource
  Export msi and irq functions for module use
  x86-pci: allow pci domain specific dma ops
  x86/pci: Initial commit for new VMD device driver
  aer_inject: Use 32 bit int type domains
  pciutils: Allow 32-bit domains

Liu Jiang (1):
  msi: Relax msi_domain_alloc() to support parentless MSI irqdomains

 arch/x86/Kconfig                  |  13 +
 arch/x86/include/asm/device.h     |  10 +
 arch/x86/include/asm/hw_irq.h     |   5 +
 arch/x86/pci/Makefile             |   2 +
 arch/x86/pci/common.c             |  38 +++
 arch/x86/pci/vmd.c                | 695 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/msi.c                 |   2 +
 drivers/pci/pcie/aer/aer_inject.c |  16 +-
 drivers/pci/probe.c               |   6 +
 kernel/irq/irqdomain.c            |   1 +
 kernel/irq/msi.c                  |   8 +-
 11 files changed, 785 insertions(+), 11 deletions(-)
 create mode 100644 arch/x86/pci/vmd.c

-- 
2.6.2.307.g37023ba

[PATCHv6 1/7] msi: Relax msi_domain_alloc() to support parentless MSI irqdomains

[Keith Busch]

From: Liu Jiang <jiang.liu@linux.intel.com>

Previously msi_domain_alloc() assumes MSI irqdomains always have parent
irqdomains, but that's not true for the new Intel VMD devices. So relax
msi_domain_alloc() to support parentless MSI irqdomains.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 kernel/irq/msi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 6b0c0b7..5e15cb4 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -109,9 +109,11 @@ static int msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	if (irq_find_mapping(domain, hwirq) > 0)
 		return -EEXIST;
 
-	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
-	if (ret < 0)
-		return ret;
+	if (domain->parent) {
+		ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
+		if (ret < 0)
+			return ret;
+	}
 
 	for (i = 0; i < nr_irqs; i++) {
 		ret = ops->msi_init(domain, info, virq + i, hwirq + i, arg);
-- 
2.6.2.307.g37023ba

[PATCHv6 2/7] pci: child bus alloc fix on constrained resource

[Keith Busch]

Does not allocate a child bus if the new bus number does not fit in the
parent's bus resource window.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/probe.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index edb1984..6e29f7a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -704,6 +704,12 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	int i;
 	int ret;
 
+	if (busnr > parent->busn_res.end) {
+		dev_printk(KERN_DEBUG, &parent->dev,
+			  "can not alloc bus:%d under %pR\n", busnr,
+			  &parent->busn_res);
+		return NULL;
+	}
 	/*
 	 * Allocate a new bus, and inherit stuff from the parent..
 	 */
-- 
2.6.2.307.g37023ba

[PATCHv6 3/7] Export msi and irq functions for module use

[Keith Busch]

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/msi.c      | 2 ++
 kernel/irq/irqdomain.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 53e4632..0fec654 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1126,6 +1126,7 @@ struct pci_dev *msi_desc_to_pci_dev(struct msi_desc *desc)
 {
 	return to_pci_dev(desc->dev);
 }
+EXPORT_SYMBOL_GPL(msi_desc_to_pci_dev);
 
 void *msi_desc_to_pci_sysdata(struct msi_desc *desc)
 {
@@ -1285,6 +1286,7 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
 	domain->bus_token = DOMAIN_BUS_PCI_MSI;
 	return domain;
 }
+EXPORT_SYMBOL_GPL(pci_msi_create_irq_domain);
 
 /**
  * pci_msi_domain_alloc_irqs - Allocate interrupts for @dev in @domain
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 22aa961..ca05cc8 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1058,6 +1058,7 @@ void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
 	__irq_set_handler(virq, handler, 0, handler_name);
 	irq_set_handler_data(virq, handler_data);
 }
+EXPORT_SYMBOL(irq_domain_set_info);
 
 /**
  * irq_domain_reset_irq_data - Clear hwirq, chip and chip_data in @irq_data
-- 
2.6.2.307.g37023ba

[PATCHv6 4/7] x86-pci: allow pci domain specific dma ops

[Keith Busch]

New x86 pci h/w will require dma operations specific to that domain. This
patch allows those domains to register their operations, and sets devices
as they are discovere3d in that domain to use them.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 arch/x86/include/asm/device.h | 10 ++++++++++
 arch/x86/pci/common.c         | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/arch/x86/include/asm/device.h b/arch/x86/include/asm/device.h
index 03dd729..3b23897 100644
--- a/arch/x86/include/asm/device.h
+++ b/arch/x86/include/asm/device.h
@@ -10,6 +10,16 @@ struct dev_archdata {
 #endif
 };
 
+#if defined(CONFIG_X86_DEV_DMA_OPS) && defined(CONFIG_PCI_DOMAINS)
+struct dma_domain {
+	struct list_head node;
+	struct dma_map_ops *dma_ops;
+	int domain_nr;
+};
+extern void add_dma_domain(struct dma_domain *domain);
+extern void del_dma_domain(struct dma_domain *domain);
+#endif
+
 struct pdev_archdata {
 };
 
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index eccd4d9..106fd13 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -641,6 +641,43 @@ unsigned int pcibios_assign_all_busses(void)
 	return (pci_probe & PCI_ASSIGN_ALL_BUSSES) ? 1 : 0;
 }
 
+#if defined(CONFIG_X86_DEV_DMA_OPS) && defined(CONFIG_PCI_DOMAINS)
+LIST_HEAD(dma_domain_list);
+DEFINE_SPINLOCK(dma_domain_list_lock);
+
+void add_dma_domain(struct dma_domain *domain)
+{
+	spin_lock(&dma_domain_list_lock);
+	list_add(&domain->node, &dma_domain_list);
+	spin_unlock(&dma_domain_list_lock);
+}
+EXPORT_SYMBOL_GPL(add_dma_domain);
+
+void del_dma_domain(struct dma_domain *domain)
+{
+	spin_lock(&dma_domain_list_lock);
+	list_del(&domain->node);
+	spin_unlock(&dma_domain_list_lock);
+}
+EXPORT_SYMBOL_GPL(del_dma_domain);
+
+static void set_dma_domain_ops(struct pci_dev *pdev)
+{
+	struct dma_domain *domain;
+
+	spin_lock(&dma_domain_list_lock);
+	list_for_each_entry(domain, &dma_domain_list, node) {
+		if (pci_domain_nr(pdev->bus) == domain->domain_nr) {
+			pdev->dev.archdata.dma_ops = domain->dma_ops;
+			break;
+		}
+	}
+	spin_unlock(&dma_domain_list_lock);
+}
+#else
+static void set_dma_domain_ops(struct pci_dev *pdev) {}
+#endif
+
 int pcibios_add_device(struct pci_dev *dev)
 {
 	struct setup_data *data;
@@ -670,6 +707,7 @@ int pcibios_add_device(struct pci_dev *dev)
 		pa_data = data->next;
 		iounmap(data);
 	}
+	set_dma_domain_ops(dev);
 	return 0;
 }
 
-- 
2.6.2.307.g37023ba

[PATCHv6 5/7] x86/pci: Initial commit for new VMD device driver

[Keith Busch]

The Intel Volume Management Device (VMD) is an integrated endpoint on the
platform's PCIe root complex that acts as a host bridge to a secondary
PCIe domain. BIOS can reassign one or more root ports to appear within
a VMD domain instead of the primary domain. The immediate benefit is
that additional PCI-e domains allow more than 256 buses in a system by
letting bus number be reused across different domains.

VMD domains do not define ACPI _SEG, so to avoid domain clashing with
host bridges defining this segment, VMD domains start at 0x10000 which
is greater than the highest possible 16-bit ACPI defined _SEG.

This driver enumerates and enables the domain using the root bus
configuration interface provided by the PCI subsystem. The driver
provides configuration space accessor functions (pci_ops), bus and
memory resources, an MSI irq domain with irq_chip implementation, and
dma operations necessary to use devices in through the VMD endpoint's
interface.

VMD routes I/O as follows:

   1) Configuration Space: BAR 0 ("CFGBAR") of VMD provides the base
   address and size for configuration space register access to VMD-owned
   root ports. It works similarly to MMCONFIG for extended configuration
   space. Bus numbering is independent and does not conflict with the
   primary domain.

   2) MMIO Space: BARs 2 and 4 ("MEMBAR1" and "MEMBAR2") of VMD provide
   the base address, size, and type for MMIO register access. These
   addresses are not translated by VMD hardware; they are simply
   reservations to be distributed to root ports' memory base/limit
   registers and subdivided among devices downstream.

   3) DMA: To interact appropriately with IOMMU, the source ID DMA read
   and write requests are translated to the bus-device-function of the
   VMD endpoint. Otherwise, DMA operates normally without VMD-specific
   address translation.

   4) Interrupts: Part of VMD's BAR 4 is reserved for VMD's MSI-X Table
   and PBA. MSIs from VMD domain devices and ports are remapped to appear
   if they were issued using one of VMD's MSI-X table entries. Each MSI
   and MSI-X addresses of VMD-owned devices and ports have a special
   format where the address refers to specific entries in VMD's MSI-X
   table. As with DMA, the interrupt source id is translated to VMD's
   bus-device-function.

   The driver provides its own MSI and MSI-X configuration functions
   specific to how MSI messages are used within the VMD domain,
   and provides an irq_chip for independent IRQ allocation to relay
   interrupts from VMD's interrupt handler to the appropriate device
   driver's handler.

   5) Errors: PCIe error message are intercepted by the root ports
   normally (e.g. AER), except with VMD, system errors (i.e. firmware
   first) are disabled by default. AER and hotplug interrupts are
   translated in the same way as endpoint interrupts.

   6) VMD does not support INTx interrupts or IO ports. Devices or drivers
   requiring these features should either not be placed below VMD-owned
   root ports, or VMD should be disabled by BIOS for such endpoints.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 arch/x86/Kconfig              |  13 +
 arch/x86/include/asm/hw_irq.h |   5 +
 arch/x86/pci/Makefile         |   2 +
 arch/x86/pci/vmd.c            | 695 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 715 insertions(+)
 create mode 100644 arch/x86/pci/vmd.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f..9a5ab69 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2665,6 +2665,19 @@ config PMC_ATOM
 	def_bool y
         depends on PCI
 
+config VMD
+	depends on PCI_MSI
+	tristate "Volume Management Device Driver"
+	default N
+	---help---
+	  Adds support for the Intel Volume Manage Device (VMD). VMD is a
+	  secondary PCI host bridge that allows PCI Express root ports,
+	  and devices attached to them, to be removed from the default
+	  PCI domain and placed within the VMD domain. This provides
+	  additional bus resources than are otherwise possible with a
+	  single domain. If you know your system provides one of these and
+	  have devices attached to it, say Y; if you are not sure, say N.
+
 source "net/Kconfig"
 
 source "drivers/Kconfig"
diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
index 1e3408e..79c35c4 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -108,6 +108,11 @@ struct irq_alloc_info {
 			struct IO_APIC_route_entry *ioapic_entry;
 		};
 #endif
+#if IS_ENABLED(CONFIG_VMD)
+		struct {
+			struct msi_desc	*desc;
+		};
+#endif
 #ifdef	CONFIG_DMAR_TABLE
 		struct {
 			int		dmar_id;
diff --git a/arch/x86/pci/Makefile b/arch/x86/pci/Makefile
index 5c6fc35..97062a6 100644
--- a/arch/x86/pci/Makefile
+++ b/arch/x86/pci/Makefile
@@ -23,6 +23,8 @@ obj-y				+= bus_numa.o
 obj-$(CONFIG_AMD_NB)		+= amd_bus.o
 obj-$(CONFIG_PCI_CNB20LE_QUIRK)	+= broadcom_bus.o
 
+obj-$(CONFIG_VMD) += vmd.o
+
 ifeq ($(CONFIG_PCI_DEBUG),y)
 EXTRA_CFLAGS += -DDEBUG
 endif
diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
new file mode 100644
index 0000000..d9b1c8d
--- /dev/null
+++ b/arch/x86/pci/vmd.c
@@ -0,0 +1,695 @@
+/*
+ * Volume Management Device driver
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ */
+
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/pci.h>
+#include <linux/rculist.h>
+#include <linux/rcupdate.h>
+
+#include <asm/irqdomain.h>
+#include <asm/device.h>
+#include <asm/msi.h>
+#include <asm/msidef.h>
+
+/*
+ * Lock for manipulating vmd irq lists.
+ */
+static DEFINE_RAW_SPINLOCK(list_lock);
+
+/**
+ * struct vmd_irq - private data to map driver irq to the VMD shared vector
+ * @node:	list item for parent traversal.
+ * @rcu:	rcu callback item for freeing.
+ * @irq: 	back pointer to parent.
+ * @virq:	the virtual irq value provided to the requesting driver.
+ *
+ * Every MSI/MSI-x irq requested for a device in a VMD domain will be mapped to
+ * a VMD irq using this structure.
+ */
+struct vmd_irq {
+	struct list_head	node;
+	struct rcu_head		rcu;
+	struct vmd_irq_list	*irq;
+	unsigned int		virq;
+};
+
+/**
+ * struct vmd_irq_list - list of driver requested irq's mapping to a vmd vector
+ * @irq_list:	the list of irq's the VMD one demuxes to.
+ * @vmd_vector:	the h/w irq assigned to the VMD device.
+ * @index:	index into the VMD MSI-x table; used for message routing.
+ * @count:	number of child irqs assigned to this vector; used to track
+ * 		sharing.
+ */
+struct vmd_irq_list {
+	struct list_head	irq_list;
+	unsigned int		vmd_vector;
+	unsigned int		index;
+	unsigned int		count;
+};
+
+struct vmd_dev {
+	struct pci_dev		*dev;
+
+	spinlock_t		cfg_lock;
+	char __iomem		*cfgbar;
+
+	int msix_count;
+	struct msix_entry	*msix_entries;
+	struct vmd_irq_list	*irqs;
+
+	struct pci_sysdata	sysdata;
+	struct resource		resources[3];
+	struct irq_domain	*irq_domain;
+	struct pci_bus		*bus;
+
+#ifdef CONFIG_X86_DEV_DMA_OPS
+	struct dma_map_ops	dma_ops;
+	struct dma_domain	dma_domain;
+#endif
+};
+
+static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)
+{
+	return container_of(bus->sysdata, struct vmd_dev, sysdata);
+}
+
+/*
+ * Drivers managing a device in a VMD domain allocate their own irqs as before,
+ * but the MSI entry for the hardware it's driving will be programmed with a
+ * destination id for the VMD MSI-x table. The VMD device muxes interrupts in
+ * its domain into one of its own, and the VMD driver de-muxes these for the
+ * handlers sharing that VMD irq. The vmd irq_domain provides the operations
+ * and irq_chip to set this up.
+ */
+static void vmd_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct vmd_irq *vmdirq = data->chip_data;
+	struct vmd_irq_list *irq = vmdirq->irq;
+
+	msg->address_hi = MSI_ADDR_BASE_HI;
+	msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_DEST_ID(irq->index);
+	msg->data = 0;
+}
+
+/*
+ * We rely on MSI_FLAG_USE_DEF_CHIP_OPS to set the irq mask/unmask ops.
+ */
+static void vmd_irq_enable(struct irq_data *data)
+{
+	struct vmd_irq *vmdirq = data->chip_data;
+
+	raw_spin_lock(&list_lock);
+	list_add_tail_rcu(&vmdirq->node, &vmdirq->irq->irq_list);
+	raw_spin_unlock(&list_lock);
+
+	data->chip->irq_unmask(data);
+}
+
+static void vmd_irq_disable(struct irq_data *data)
+{
+	struct vmd_irq *vmdirq = data->chip_data;
+
+	data->chip->irq_mask(data);
+
+	raw_spin_lock(&list_lock);
+	list_del_rcu(&vmdirq->node);
+	raw_spin_unlock(&list_lock);
+}
+
+/*
+ * XXX: Stubbed until we develop acceptable way to not create conflicts with
+ * other devices sharing the same vector.
+ */
+static int vmd_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
+								bool force)
+{
+	return -EINVAL;
+}
+
+static struct irq_chip vmd_msi_controller = {
+	.name			= "VMD-MSI",
+	.irq_enable		= vmd_irq_enable,
+	.irq_disable		= vmd_irq_disable,
+	.irq_compose_msi_msg	= vmd_compose_msi_msg,
+	.irq_set_affinity	= vmd_irq_set_affinity,
+};
+
+static irq_hw_number_t vmd_get_hwirq(struct msi_domain_info *info,
+				     msi_alloc_info_t *arg)
+{
+	return 0;
+}
+
+/*
+ * XXX: We can be even smarter selecting the best irq once we solve the
+ * affinity problem.
+ */
+static struct vmd_irq_list *vmd_next_irq(struct vmd_dev *vmd)
+{
+	int i, best = 0;
+
+	raw_spin_lock(&list_lock);
+	for (i = 1; i < vmd->msix_count; i++)
+		if (vmd->irqs[i].count < vmd->irqs[best].count)
+			best = i;
+	vmd->irqs[best].count++;
+	raw_spin_unlock(&list_lock);
+
+	return &vmd->irqs[best];
+}
+
+static int vmd_msi_init(struct irq_domain *domain,
+			struct msi_domain_info *info,
+			unsigned int virq, irq_hw_number_t hwirq,
+			msi_alloc_info_t *arg)
+{
+	struct vmd_dev *vmd = vmd_from_bus(msi_desc_to_pci_dev(arg->desc)->bus);
+	struct vmd_irq *vmdirq = kzalloc(sizeof(*vmdirq), GFP_KERNEL);
+
+	if (!vmdirq)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&vmdirq->node);
+	vmdirq->irq = vmd_next_irq(vmd);
+	vmdirq->virq = virq;
+
+	irq_domain_set_info(domain, virq, vmdirq->irq->vmd_vector,
+			info->chip, vmdirq, handle_simple_irq, vmd, NULL);
+	return 0;
+}
+
+static void vmd_msi_free(struct irq_domain *domain,
+			struct msi_domain_info *info,
+			unsigned int virq)
+{
+	struct vmd_irq *vmdirq = irq_get_chip_data(virq);
+
+	/* XXX: Potential optimization to rebalance */
+	raw_spin_lock(&list_lock);
+	vmdirq->irq->count--;
+	raw_spin_unlock(&list_lock);
+
+	kfree_rcu(vmdirq, rcu);
+}
+
+static int vmd_msi_prepare(struct irq_domain *domain,
+			struct device *dev, int nvec,
+			msi_alloc_info_t *arg)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct vmd_dev *vmd = vmd_from_bus(pdev->bus);
+
+	if (nvec > vmd->msix_count)
+		return vmd->msix_count;
+	memset(arg, 0, sizeof(*arg));
+	return 0;
+}
+
+static void vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
+{
+	arg->desc = desc;
+}
+
+static struct msi_domain_ops vmd_msi_domain_ops = {
+	.get_hwirq	= vmd_get_hwirq,
+	.msi_init	= vmd_msi_init,
+	.msi_free	= vmd_msi_free,
+	.msi_prepare	= vmd_msi_prepare,
+	.set_desc	= vmd_set_desc,
+};
+
+static struct msi_domain_info vmd_msi_domain_info = {
+	.flags		= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+			  MSI_FLAG_PCI_MSIX,
+	.ops		= &vmd_msi_domain_ops,
+	.chip		= &vmd_msi_controller,
+};
+
+#ifdef CONFIG_X86_DEV_DMA_OPS
+/*
+ * VMD replaces the requester id with its own. DMA mappings for devices in a
+ * VMD domain need to be mapped for the VMD device, not the device requiring
+ * the mapping.
+ */
+static struct device *dev_to_vmd_dev(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct vmd_dev *vmd = vmd_from_bus(pdev->bus);
+	return &vmd->dev->dev;
+}
+
+static void *vmd_alloc(struct device *dev, size_t size, dma_addr_t *addr,
+				gfp_t flag, struct dma_attrs *attrs)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	return vdev->archdata.dma_ops->alloc(vdev, size, addr, flag, attrs);
+}
+
+static void vmd_free(struct device *dev, size_t size, void *vaddr,
+				dma_addr_t addr, struct dma_attrs *attrs)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	vdev->archdata.dma_ops->free(vdev, size, vaddr, addr, attrs);
+}
+
+static int vmd_mmap(struct device *dev, struct vm_area_struct *vma,
+				void *cpu_addr, dma_addr_t addr,
+				size_t size, struct dma_attrs *attrs)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	return vdev->archdata.dma_ops->mmap(vdev, vma, cpu_addr, addr, size,
+									attrs);
+}
+
+static int vmd_get_sgtable(struct device *dev, struct sg_table *sgt,
+				void *cpu_addr, dma_addr_t addr,
+				size_t size, struct dma_attrs *attrs)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	return vdev->archdata.dma_ops->get_sgtable(vdev, sgt, cpu_addr, addr,
+								size, attrs);
+}
+
+static dma_addr_t vmd_map_page(struct device *dev, struct page *page,
+				unsigned long offset, size_t size,
+				enum dma_data_direction dir,
+				struct dma_attrs *attrs)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	return vdev->archdata.dma_ops->map_page(vdev, page, offset, size, dir,
+									attrs);
+}
+
+static void vmd_unmap_page(struct device *dev, dma_addr_t addr, size_t size,
+				enum dma_data_direction dir,
+				struct dma_attrs *attrs)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	vdev->archdata.dma_ops->unmap_page(vdev, addr, size, dir, attrs);
+}
+
+static int vmd_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+				enum dma_data_direction dir,
+				struct dma_attrs *attrs)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	return vdev->archdata.dma_ops->map_sg(vdev, sg, nents, dir, attrs);
+}
+
+static void vmd_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
+				enum dma_data_direction dir,
+				struct dma_attrs *attrs)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	vdev->archdata.dma_ops->unmap_sg(vdev, sg, nents, dir, attrs);
+}
+
+static void vmd_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
+				size_t size, enum dma_data_direction dir)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	vdev->archdata.dma_ops->sync_single_for_cpu(vdev, addr, size, dir);
+}
+
+static void vmd_sync_single_for_device(struct device *dev, dma_addr_t addr,
+				size_t size, enum dma_data_direction dir)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	vdev->archdata.dma_ops->sync_single_for_device(vdev, addr, size, dir);
+}
+
+static void vmd_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
+				int nents, enum dma_data_direction dir)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	vdev->archdata.dma_ops->sync_sg_for_cpu(vdev, sg, nents, dir);
+}
+
+static void vmd_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
+				int nents, enum dma_data_direction dir)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	vdev->archdata.dma_ops->sync_sg_for_device(vdev, sg, nents, dir);
+}
+
+static int vmd_mapping_error(struct device *dev, dma_addr_t addr)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	return vdev->archdata.dma_ops->mapping_error(vdev, addr);
+}
+
+static int vmd_dma_supported(struct device *dev, u64 mask)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	return vdev->archdata.dma_ops->dma_supported(vdev, mask);
+}
+
+#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
+u64 vmd_get_required_mask(struct device *dev)
+{
+	struct device *vdev = dev_to_vmd_dev(dev);
+	return vdev->archdata.dma_ops->get_required_mask(vdev);
+}
+#endif
+
+static void vmd_teardown_dma_ops(struct vmd_dev *vmd)
+{
+	struct dma_domain *domain = &vmd->dma_domain;
+
+	if (vmd->dev->dev.archdata.dma_ops)
+		del_dma_domain(domain);
+}
+
+#define ASSIGN_VMD_DMA_OPS(source, dest, fn) \
+	if (source->fn) dest->fn = vmd_##fn;
+static void vmd_setup_dma_ops(struct vmd_dev *vmd)
+{
+	struct dma_domain *domain = &vmd->dma_domain;
+	struct dma_map_ops *source = vmd->dev->dev.archdata.dma_ops;
+	struct dma_map_ops *dest = &vmd->dma_ops;
+
+	domain->domain_nr = vmd->sysdata.domain;
+	domain->dma_ops = dest;
+
+	if (!source)
+		return;
+	ASSIGN_VMD_DMA_OPS(source, dest, alloc);
+	ASSIGN_VMD_DMA_OPS(source, dest, free);
+	ASSIGN_VMD_DMA_OPS(source, dest, mmap);
+	ASSIGN_VMD_DMA_OPS(source, dest, get_sgtable);
+	ASSIGN_VMD_DMA_OPS(source, dest, map_page);
+	ASSIGN_VMD_DMA_OPS(source, dest, unmap_page);
+	ASSIGN_VMD_DMA_OPS(source, dest, map_sg);
+	ASSIGN_VMD_DMA_OPS(source, dest, unmap_sg);
+	ASSIGN_VMD_DMA_OPS(source, dest, sync_single_for_cpu);
+	ASSIGN_VMD_DMA_OPS(source, dest, sync_single_for_device);
+	ASSIGN_VMD_DMA_OPS(source, dest, sync_sg_for_cpu);
+	ASSIGN_VMD_DMA_OPS(source, dest, sync_sg_for_device);
+	ASSIGN_VMD_DMA_OPS(source, dest, mapping_error);
+	ASSIGN_VMD_DMA_OPS(source, dest, dma_supported);
+#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
+	ASSIGN_VMD_DMA_OPS(source, dest, get_required_mask);
+#endif
+	add_dma_domain(domain);
+}
+#undef ASSIGN_VMD_DMA_OPS
+#else
+static void vmd_teardown_dma_ops(struct vmd_dev *vmd) {}
+static void vmd_setup_dma_ops(struct vmd_dev *vmd) {}
+#endif
+
+/*
+ * CPU may deadlock if config space is not serialized on some versions of this
+ * hardware, so all config space access is done under a spinlock.
+ */
+static int vmd_pci_read(struct pci_bus *bus, unsigned int devfn, int reg,
+							int len, u32 *value)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct vmd_dev *vmd = vmd_from_bus(bus);
+	char __iomem *addr = vmd->cfgbar + (bus->number << 20) +
+						(devfn << 12) + reg;
+
+	if ((addr - vmd->cfgbar) + len >= resource_size(&vmd->dev->resource[0]))
+		return -EFAULT;
+
+	spin_lock_irqsave(&vmd->cfg_lock, flags);
+	switch (len) {
+	case 1:
+		*value = readb(addr);
+		break;
+	case 2:
+		*value = readw(addr);
+		break;
+	case 4:
+		*value = readl(addr);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	spin_unlock_irqrestore(&vmd->cfg_lock, flags);
+	return ret;
+}
+
+/*
+ * VMD h/w converts posted config writes to non-posted. The read-back in this
+ * function forces the completion so it returns only after the config space was
+ * written, as expected.
+ */
+static int vmd_pci_write(struct pci_bus *bus, unsigned int devfn, int reg,
+							int len, u32 value)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct vmd_dev *vmd = vmd_from_bus(bus);
+	char __iomem *addr = vmd->cfgbar + (bus->number << 20) +
+						(devfn << 12) + reg;
+
+	if ((addr - vmd->cfgbar) + len >= resource_size(&vmd->dev->resource[0]))
+		return -EFAULT;
+
+	spin_lock_irqsave(&vmd->cfg_lock, flags);
+	switch (len) {
+	case 1:
+		writeb(value, addr);
+		readb(addr);
+		break;
+	case 2:
+		writew(value, addr);
+		readw(addr);
+		break;
+	case 4:
+		writel(value, addr);
+		readl(addr);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	spin_unlock_irqrestore(&vmd->cfg_lock, flags);
+	return ret;
+}
+
+static struct pci_ops vmd_ops = {
+	.read		= vmd_pci_read,
+	.write		= vmd_pci_write,
+};
+
+/*
+ * VMD domains start at 10000h to not clash with domains defining ACPI _SEG.
+ */
+static int vmd_find_free_domain(void)
+{
+	int domain = 0xffff;
+	struct pci_bus *bus = NULL;
+
+	while ((bus = pci_find_next_bus(bus)) != NULL)
+		domain = max_t(int, domain, pci_domain_nr(bus));
+	return domain + 1;
+}
+
+static int vmd_enable_domain(struct vmd_dev *vmd)
+{
+	struct pci_sysdata *sd = &vmd->sysdata;
+	LIST_HEAD(resources);
+
+	vmd->resources[0] = (struct resource) {
+		.name	= "VMD CFGBAR",
+		.start	= 0,
+		.end	= (resource_size(&vmd->dev->resource[0]) >> 20) - 1,
+		.flags	= IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
+	};
+	vmd->resources[1] = (struct resource) {
+		.name	= "VMD MEMBAR1",
+		.start	= vmd->dev->resource[2].start,
+		.end	= vmd->dev->resource[2].end,
+		.flags	= (vmd->dev->resource[2].flags & ~IORESOURCE_SIZEALIGN) &
+				(!upper_32_bits(vmd->dev->resource[2].end) ?
+							~IORESOURCE_MEM_64 : ~0),
+	};
+	vmd->resources[2] = (struct resource) {
+		.name	= "VMD MEMBAR2",
+		.start	= vmd->dev->resource[4].start + 0x2000,
+		.end	= vmd->dev->resource[4].end,
+		.flags	= (vmd->dev->resource[4].flags & ~IORESOURCE_SIZEALIGN) &
+				(!upper_32_bits(vmd->dev->resource[4].end) ?
+							~IORESOURCE_MEM_64 : ~0),
+	};
+
+	sd->domain = vmd_find_free_domain();
+	if (sd->domain < 0)
+		return sd->domain;
+	sd->node = pcibus_to_node(vmd->dev->bus);
+
+	vmd->irq_domain = pci_msi_create_irq_domain(NULL, &vmd_msi_domain_info,
+									NULL);
+	if (!vmd->irq_domain)
+		return -ENODEV;
+
+	pci_add_resource(&resources, &vmd->resources[0]);
+	pci_add_resource(&resources, &vmd->resources[1]);
+	pci_add_resource(&resources, &vmd->resources[2]);
+	vmd->bus = pci_create_root_bus(NULL, 0, &vmd_ops, sd, &resources);
+	if (!vmd->bus) {
+		pci_free_resource_list(&resources);
+		irq_domain_remove(vmd->irq_domain);
+		return -ENODEV;
+	}
+
+	vmd_setup_dma_ops(vmd);
+	dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
+	pci_rescan_bus(vmd->bus);
+
+	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj, "domain"),
+			"Can't create symlink to domain\n");
+	return 0;
+}
+
+static irqreturn_t vmd_irq(int irq, void *data)
+{
+	struct vmd_irq_list *irqs = data;
+	struct vmd_irq *vmdirq;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
+		generic_handle_irq(vmdirq->virq);
+	rcu_read_unlock();
+
+	return IRQ_HANDLED;
+}
+
+static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	struct vmd_dev *vmd;
+	int i, err;
+
+	if (resource_size(&dev->resource[0]) < (1 << 20))
+		return -ENOMEM;
+
+	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
+	if (!vmd)
+		return -ENOMEM;
+
+	vmd->dev = dev;
+	err = pcim_enable_device(dev);
+	if (err < 0)
+		return err;
+
+	vmd->cfgbar = pcim_iomap(dev, 0, 0);
+	if (!vmd->cfgbar)
+		return -ENOMEM;
+
+	pci_set_master(dev);
+	if (dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(64)) &&
+	    dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32)))
+		return -ENODEV;
+
+	vmd->msix_count = pci_msix_vec_count(dev);
+	if (vmd->msix_count < 0)
+		return -ENODEV;
+
+	vmd->irqs = devm_kcalloc(&dev->dev, vmd->msix_count, sizeof(*vmd->irqs),
+							GFP_KERNEL);
+	if (!vmd->irqs)
+		return -ENOMEM;
+
+	vmd->msix_entries = devm_kcalloc(&dev->dev, vmd->msix_count,
+					sizeof(*vmd->msix_entries), GFP_KERNEL);
+	if(!vmd->msix_entries)
+		return -ENOMEM;
+	for (i = 0; i < vmd->msix_count; i++)
+		vmd->msix_entries[i].entry = i;
+
+	vmd->msix_count = pci_enable_msix_range(vmd->dev, vmd->msix_entries, 1,
+							vmd->msix_count);
+	if (vmd->msix_count < 0)
+		return vmd->msix_count;
+
+	for (i = 0; i < vmd->msix_count; i++) {
+		INIT_LIST_HEAD(&vmd->irqs[i].irq_list);
+		vmd->irqs[i].vmd_vector = vmd->msix_entries[i].vector;
+		vmd->irqs[i].index = i;
+
+		err = devm_request_irq(&dev->dev, vmd->irqs[i].vmd_vector,
+				vmd_irq, 0, "vmd", &vmd->irqs[i]);
+		if (err)
+			return err;
+	}
+	spin_lock_init(&vmd->cfg_lock);
+	pci_set_drvdata(dev, vmd);
+	err = vmd_enable_domain(vmd);
+	if (err)
+		return err;
+	return 0;
+}
+
+static void vmd_remove(struct pci_dev *dev)
+{
+	struct vmd_dev *vmd = pci_get_drvdata(dev);
+
+	pci_set_drvdata(dev, NULL);
+	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
+	pci_stop_root_bus(vmd->bus);
+	pci_remove_root_bus(vmd->bus);
+	vmd_teardown_dma_ops(vmd);
+	irq_domain_remove(vmd->irq_domain);
+}
+
+#ifdef CONFIG_PM
+static int vmd_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_save_state(pdev);
+	return 0;
+}
+
+static int vmd_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_restore_state(pdev);
+	return 0;
+}
+#endif
+static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
+
+static const struct pci_device_id vmd_ids[] = {
+	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x201d),},
+	{0,}
+};
+MODULE_DEVICE_TABLE(pci, vmd_ids);
+
+static struct pci_driver vmd_drv = {
+	.name		= "vmd",
+	.id_table	= vmd_ids,
+	.probe		= vmd_probe,
+	.remove		= vmd_remove,
+	.driver		= {
+		.pm	= &vmd_dev_pm_ops,
+	},
+};
+module_pci_driver(vmd_drv);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("0.5");
-- 
2.6.2.307.g37023ba

[PATCHv6 6/7] aer_inject: Use 32 bit int type domains

[Keith Busch]

New pci device provides additional pci domains that start above what 16
bits can address.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/aer/aer_inject.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index 182224a..24b191e 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -41,12 +41,12 @@ struct aer_error_inj {
 	u32 header_log1;
 	u32 header_log2;
 	u32 header_log3;
-	u16 domain;
+	int domain;
 };
 
 struct aer_error {
 	struct list_head list;
-	u16 domain;
+	int domain;
 	unsigned int bus;
 	unsigned int devfn;
 	int pos_cap_err;
@@ -74,7 +74,7 @@ static LIST_HEAD(pci_bus_ops_list);
 /* Protect einjected and pci_bus_ops_list */
 static DEFINE_SPINLOCK(inject_lock);
 
-static void aer_error_init(struct aer_error *err, u16 domain,
+static void aer_error_init(struct aer_error *err, int domain,
 			   unsigned int bus, unsigned int devfn,
 			   int pos_cap_err)
 {
@@ -86,7 +86,7 @@ static void aer_error_init(struct aer_error *err, u16 domain,
 }
 
 /* inject_lock must be held before calling */
-static struct aer_error *__find_aer_error(u16 domain, unsigned int bus,
+static struct aer_error *__find_aer_error(int domain, unsigned int bus,
 					  unsigned int devfn)
 {
 	struct aer_error *err;
@@ -106,7 +106,7 @@ static struct aer_error *__find_aer_error_by_dev(struct pci_dev *dev)
 	int domain = pci_domain_nr(dev->bus);
 	if (domain < 0)
 		return NULL;
-	return __find_aer_error((u16)domain, dev->bus->number, dev->devfn);
+	return __find_aer_error(domain, dev->bus->number, dev->devfn);
 }
 
 /* inject_lock must be held before calling */
@@ -196,7 +196,7 @@ static int pci_read_aer(struct pci_bus *bus, unsigned int devfn, int where,
 	domain = pci_domain_nr(bus);
 	if (domain < 0)
 		goto out;
-	err = __find_aer_error((u16)domain, bus->number, devfn);
+	err = __find_aer_error(domain, bus->number, devfn);
 	if (!err)
 		goto out;
 
@@ -228,7 +228,7 @@ static int pci_write_aer(struct pci_bus *bus, unsigned int devfn, int where,
 	domain = pci_domain_nr(bus);
 	if (domain < 0)
 		goto out;
-	err = __find_aer_error((u16)domain, bus->number, devfn);
+	err = __find_aer_error(domain, bus->number, devfn);
 	if (!err)
 		goto out;
 
@@ -329,7 +329,7 @@ static int aer_inject(struct aer_error_inj *einj)
 	u32 sever, cor_mask, uncor_mask, cor_mask_orig = 0, uncor_mask_orig = 0;
 	int ret = 0;
 
-	dev = pci_get_domain_bus_and_slot((int)einj->domain, einj->bus, devfn);
+	dev = pci_get_domain_bus_and_slot(einj->domain, einj->bus, devfn);
 	if (!dev)
 		return -ENODEV;
 	rpdev = pcie_find_root_port(dev);
-- 
2.6.2.307.g37023ba

[PATCHv5 7/7] pciutils: Allow 32-bit domains

[Keith Busch]

PCI-e segments will continue to use the lower 16 bits as required by
ACPI. Special domains may use the full 32-bits.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 lib/filter.c |    2 +-
 lib/pci.h    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/filter.c b/lib/filter.c
index d4254a0..075dc2f 100644
--- a/lib/filter.c
+++ b/lib/filter.c
@@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
 	  if (str[0] && strcmp(str, "*"))
 	    {
 	      long int x = strtol(str, &e, 16);
-	      if ((e && *e) || (x < 0 || x > 0xffff))
+	      if ((e && *e) || (x < 0))
 		return "Invalid domain number";
 	      f->domain = x;
 	    }
diff --git a/lib/pci.h b/lib/pci.h
index 10ba831..7e42765 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev
 
 struct pci_dev {
   struct pci_dev *next;			/* Next device in the chain */
-  u16 domain;				/* PCI domain (host bridge) */
+  int32_t domain;			/* PCI domain (host bridge) */
   u8 bus, dev, func;			/* Bus inside domain, device and function */
 
   /* These fields are set by pci_fill_info() */
-- 
1.7.10.4

Re: [PATCHv6 0/7] Driver for new "VMD" device

[Thomas Gleixner]

On Mon, 7 Dec 2015, Keith Busch wrote:

So this looks really good now (at least the irq related parts).

Bjorn, how should we handle this?

I can take it all through tip or just apply the irq/msi related parts
and provide you a branch to pull them from. Either way is fine.

Thanks,

	tglx

Re: [PATCHv5 7/7] pciutils: Allow 32-bit domains

[Andy Shevchenko]

On Mon, Dec 7, 2015 at 11:32 PM, Keith Busch <keith.busch@intel.com> wrote:
> PCI-e segments will continue to use the lower 16 bits as required by
> ACPI. Special domains may use the full 32-bits.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  lib/filter.c |    2 +-
>  lib/pci.h    |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/filter.c b/lib/filter.c
> index d4254a0..075dc2f 100644
> --- a/lib/filter.c
> +++ b/lib/filter.c
> @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
>           if (str[0] && strcmp(str, "*"))
>             {
>               long int x = strtol(str, &e, 16);
> -             if ((e && *e) || (x < 0 || x > 0xffff))
> +             if ((e && *e) || (x < 0))
>                 return "Invalid domain number";
>               f->domain = x;
>             }
> diff --git a/lib/pci.h b/lib/pci.h
> index 10ba831..7e42765 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev
>
>  struct pci_dev {
>    struct pci_dev *next;                        /* Next device in the chain */
> -  u16 domain;                          /* PCI domain (host bridge) */

> +  int32_t domain;                      /* PCI domain (host bridge) */

Why not u32 ?

>    u8 bus, dev, func;                   /* Bus inside domain, device and function */
>
>    /* These fields are set by pci_fill_info() */
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCHv5 7/7] pciutils: Allow 32-bit domains

[Bjorn Helgaas]

Hi Keith,

On Mon, Dec 07, 2015 at 02:32:29PM -0700, Keith Busch wrote:
> PCI-e segments will continue to use the lower 16 bits as required by
> ACPI. Special domains may use the full 32-bits.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  lib/filter.c |    2 +-
>  lib/pci.h    |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/filter.c b/lib/filter.c
> index d4254a0..075dc2f 100644
> --- a/lib/filter.c
> +++ b/lib/filter.c
> @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
>  	  if (str[0] && strcmp(str, "*"))
>  	    {
>  	      long int x = strtol(str, &e, 16);
> -	      if ((e && *e) || (x < 0 || x > 0xffff))
> +	      if ((e && *e) || (x < 0))

Just out of curiosity (I don't maintain pciutils; Martin would apply
this one), is there some part of the PCI or PCI firmware spec that is
relevant to this change?  Maybe this is connected to parsing things
exported by the kernel and not directly tied to PCI at the spec level.

Whatever it is, a pointer to the producer of the information you're
consuming here would help us understand and review the patch.

>  		return "Invalid domain number";
>  	      f->domain = x;
>  	    }
> diff --git a/lib/pci.h b/lib/pci.h
> index 10ba831..7e42765 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev
>  
>  struct pci_dev {
>    struct pci_dev *next;			/* Next device in the chain */
> -  u16 domain;				/* PCI domain (host bridge) */
> +  int32_t domain;			/* PCI domain (host bridge) */
>    u8 bus, dev, func;			/* Bus inside domain, device and function */
>  
>    /* These fields are set by pci_fill_info() */
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 2/7] pci: child bus alloc fix on constrained resource

[Bjorn Helgaas]

Hi Keith,

On Mon, Dec 07, 2015 at 02:32:24PM -0700, Keith Busch wrote:
> Does not allocate a child bus if the new bus number does not fit in the
> parent's bus resource window.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/probe.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index edb1984..6e29f7a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -704,6 +704,12 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  	int i;
>  	int ret;
>  
> +	if (busnr > parent->busn_res.end) {
> +		dev_printk(KERN_DEBUG, &parent->dev,
> +			  "can not alloc bus:%d under %pR\n", busnr,
> +			  &parent->busn_res);
> +		return NULL;

Can you take a look at 1820ffdccb9b ("PCI: Make sure bus number resources
stay within their parents bounds") and 12d8706963f0 ("Revert "PCI: Make
sure bus number resources stay within their parents bounds"")?

This is implemented differently, but it seems like it might expose the same
problem we found with 1820ffdccb9b.

If you could take a look and confirm that "no, this does something
differently than 1820ffdccb9b did" or "yes, this might expose that problem
again," that would help.

Bjorn

> +	}
>  	/*
>  	 * Allocate a new bus, and inherit stuff from the parent..
>  	 */
> -- 
> 2.6.2.307.g37023ba
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv5 7/7] pciutils: Allow 32-bit domains

[Keith Busch]

On Thu, Dec 17, 2015 at 11:15:45AM -0600, Bjorn Helgaas wrote:
> > @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
> >  	  if (str[0] && strcmp(str, "*"))
> >  	    {
> >  	      long int x = strtol(str, &e, 16);
> > -	      if ((e && *e) || (x < 0 || x > 0xffff))
> > +	      if ((e && *e) || (x < 0))
> 
> Just out of curiosity (I don't maintain pciutils; Martin would apply
> this one), is there some part of the PCI or PCI firmware spec that is
> relevant to this change?  Maybe this is connected to parsing things
> exported by the kernel and not directly tied to PCI at the spec level.
>
> Whatever it is, a pointer to the producer of the information you're
> consuming here would help us understand and review the patch.

Hi Bjorn,

This is not tied to anything defined in PCI spec. Domain numbers being
a software construct (ACPI6, §6.5.6), we don't need to constrain the
representation. ACPI defines 16-bit segments, and domains provided by
this new host bridge do not define _SEG, so this series proposes domain
numbers outside the ACPI reachable range to avoid potential clashes.

The pciutils patch just synchronizes the essential tooling software with
the kernel software's new representation.

Re: [PATCHv6 2/7] pci: child bus alloc fix on constrained resource

[Bjorn Helgaas]

On Mon, Dec 07, 2015 at 02:32:24PM -0700, Keith Busch wrote:
> Does not allocate a child bus if the new bus number does not fit in the
> parent's bus resource window.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Other nits: please make this similar to previous changelogs in
capitalization, sentence structure, etc.  If we figure out that this
is safe, and I end up applying it, I can just tweak it myself, but if
you post it again, something like this is what I'm looking for:

  PCI: Don't allocate child bus outside parent bus number range

  Don't allocate a child bus if the new bus number does not fit in the
  parent's bus resource window.

The next patch is missing a subject line:

  PCI/MSI: Export MSI and IRQ functions for use by modules

  Export the following functions for use by modules:
  
    msi_desc_to_pci_dev()
    pci_msi_create_irq_domain()
    irq_domain_set_info()

And the following:

  x86/PCI: Allow PCI domain-specific DMA ops

I know this seems pedantic, but try to use "x86/PCI" consistently
because I think it's distracting to have the boiler-plate be different
in non-useful ways, e.g.,

  510fb6779eed x86/pci: Initial commit for new VMD device driver
  510fb6779eed x86/pci: Initial commit for new VMD device driver
  b7a48e92f508 x86-pci: allow pci domain specific dma ops
  8affb487d4a4 x86/PCI: Don't alloc pcibios-irq when MSI is enabled
  991de2e59090 PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()

Bjorn

> ---
>  drivers/pci/probe.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index edb1984..6e29f7a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -704,6 +704,12 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  	int i;
>  	int ret;
>  
> +	if (busnr > parent->busn_res.end) {
> +		dev_printk(KERN_DEBUG, &parent->dev,
> +			  "can not alloc bus:%d under %pR\n", busnr,
> +			  &parent->busn_res);
> +		return NULL;
> +	}
>  	/*
>  	 * Allocate a new bus, and inherit stuff from the parent..
>  	 */
> -- 
> 2.6.2.307.g37023ba
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 6/7] aer_inject: Use 32 bit int type domains

[Bjorn Helgaas]

PCI/AER: Increase domain type from 16 to 32 bits

On Mon, Dec 07, 2015 at 02:32:28PM -0700, Keith Busch wrote:
> New pci device provides additional pci domains that start above what 16
> bits can address.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/pcie/aer/aer_inject.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
> index 182224a..24b191e 100644
> --- a/drivers/pci/pcie/aer/aer_inject.c
> +++ b/drivers/pci/pcie/aer/aer_inject.c
> @@ -41,12 +41,12 @@ struct aer_error_inj {
>  	u32 header_log1;
>  	u32 header_log2;
>  	u32 header_log3;
> -	u16 domain;
> +	int domain;

If you want 32 bits explicitly, why don't you use u32 here?

>  };
>  
>  struct aer_error {
>  	struct list_head list;
> -	u16 domain;
> +	int domain;
>  	unsigned int bus;
>  	unsigned int devfn;
>  	int pos_cap_err;
> @@ -74,7 +74,7 @@ static LIST_HEAD(pci_bus_ops_list);
>  /* Protect einjected and pci_bus_ops_list */
>  static DEFINE_SPINLOCK(inject_lock);
>  
> -static void aer_error_init(struct aer_error *err, u16 domain,
> +static void aer_error_init(struct aer_error *err, int domain,
>  			   unsigned int bus, unsigned int devfn,
>  			   int pos_cap_err)
>  {
> @@ -86,7 +86,7 @@ static void aer_error_init(struct aer_error *err, u16 domain,
>  }
>  
>  /* inject_lock must be held before calling */
> -static struct aer_error *__find_aer_error(u16 domain, unsigned int bus,
> +static struct aer_error *__find_aer_error(int domain, unsigned int bus,
>  					  unsigned int devfn)
>  {
>  	struct aer_error *err;
> @@ -106,7 +106,7 @@ static struct aer_error *__find_aer_error_by_dev(struct pci_dev *dev)
>  	int domain = pci_domain_nr(dev->bus);
>  	if (domain < 0)
>  		return NULL;
> -	return __find_aer_error((u16)domain, dev->bus->number, dev->devfn);
> +	return __find_aer_error(domain, dev->bus->number, dev->devfn);
>  }
>  
>  /* inject_lock must be held before calling */
> @@ -196,7 +196,7 @@ static int pci_read_aer(struct pci_bus *bus, unsigned int devfn, int where,
>  	domain = pci_domain_nr(bus);
>  	if (domain < 0)
>  		goto out;
> -	err = __find_aer_error((u16)domain, bus->number, devfn);
> +	err = __find_aer_error(domain, bus->number, devfn);
>  	if (!err)
>  		goto out;
>  
> @@ -228,7 +228,7 @@ static int pci_write_aer(struct pci_bus *bus, unsigned int devfn, int where,
>  	domain = pci_domain_nr(bus);
>  	if (domain < 0)
>  		goto out;
> -	err = __find_aer_error((u16)domain, bus->number, devfn);
> +	err = __find_aer_error(domain, bus->number, devfn);
>  	if (!err)
>  		goto out;
>  
> @@ -329,7 +329,7 @@ static int aer_inject(struct aer_error_inj *einj)
>  	u32 sever, cor_mask, uncor_mask, cor_mask_orig = 0, uncor_mask_orig = 0;
>  	int ret = 0;
>  
> -	dev = pci_get_domain_bus_and_slot((int)einj->domain, einj->bus, devfn);
> +	dev = pci_get_domain_bus_and_slot(einj->domain, einj->bus, devfn);
>  	if (!dev)
>  		return -ENODEV;
>  	rpdev = pcie_find_root_port(dev);
> -- 
> 2.6.2.307.g37023ba
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 2/7] pci: child bus alloc fix on constrained resource

[Keith Busch]

On Thu, Dec 17, 2015 at 11:27:18AM -0600, Bjorn Helgaas wrote:
> On Mon, Dec 07, 2015 at 02:32:24PM -0700, Keith Busch wrote:
> > +	if (busnr > parent->busn_res.end) {
> > +		dev_printk(KERN_DEBUG, &parent->dev,
> > +			  "can not alloc bus:%d under %pR\n", busnr,
> > +			  &parent->busn_res);
> > +		return NULL;
> 
> Can you take a look at 1820ffdccb9b ("PCI: Make sure bus number resources
> stay within their parents bounds") and 12d8706963f0 ("Revert "PCI: Make
> sure bus number resources stay within their parents bounds"")?
> 
> This is implemented differently, but it seems like it might expose the same
> problem we found with 1820ffdccb9b.
> 
> If you could take a look and confirm that "no, this does something
> differently than 1820ffdccb9b did" or "yes, this might expose that problem
> again," that would help.

Thank you for the references. I think 1820ffdccb9b had the check too
late, creating the child bus with resource conflicts. That'd probably
make it appear unavailable. But I believe this new proposal will still
break the same setup for a different reason.

We can live with this issue for now if we want to drop this patch from
the series. The only way we encounter a problem is when the config space
aperture is artificially constrained, so I can request the feature be
put on hold while a better fix is developed.

Re: [PATCHv6 5/7] x86/pci: Initial commit for new VMD device driver

[Bjorn Helgaas]

On Mon, Dec 07, 2015 at 02:32:27PM -0700, Keith Busch wrote:
> The Intel Volume Management Device (VMD) is an integrated endpoint on the
> platform's PCIe root complex that acts as a host bridge to a secondary
> PCIe domain. BIOS can reassign one or more root ports to appear within
> a VMD domain instead of the primary domain. The immediate benefit is
> that additional PCI-e domains allow more than 256 buses in a system by
> letting bus number be reused across different domains.

> +/*
> + * VMD h/w converts posted config writes to non-posted. The read-back in this
> + * function forces the completion so it returns only after the config space was
> + * written, as expected.

This comment sounds backwards:

  posted writes don't wait for completion
  non-posted writes do wait for completion

If the hardware converts to non-posted writes, you shouldn't need a
read-back.  It seems like you would need the read-back if the hardware
converted non-posted to posted.

> + */
> +static int vmd_pci_write(struct pci_bus *bus, unsigned int devfn, int reg,
> +							int len, u32 value)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +	struct vmd_dev *vmd = vmd_from_bus(bus);
> +	char __iomem *addr = vmd->cfgbar + (bus->number << 20) +
> +						(devfn << 12) + reg;
> +
> +	if ((addr - vmd->cfgbar) + len >= resource_size(&vmd->dev->resource[0]))
> +		return -EFAULT;
> +
> +	spin_lock_irqsave(&vmd->cfg_lock, flags);
> +	switch (len) {
> +	case 1:
> +		writeb(value, addr);
> +		readb(addr);
> +		break;
> +	case 2:
> +		writew(value, addr);
> +		readw(addr);
> +		break;
> +	case 4:
> +		writel(value, addr);
> +		readl(addr);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +	spin_unlock_irqrestore(&vmd->cfg_lock, flags);
> +	return ret;
> +}

> +static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
> +{
> +	struct vmd_dev *vmd;
> +	int i, err;
> +
> +	if (resource_size(&dev->resource[0]) < (1 << 20))
> +		return -ENOMEM;
> +
> +	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
> +	if (!vmd)
> +		return -ENOMEM;
> +
> +	vmd->dev = dev;
> +	err = pcim_enable_device(dev);
> +	if (err < 0)
> +		return err;
> +
> +	vmd->cfgbar = pcim_iomap(dev, 0, 0);
> +	if (!vmd->cfgbar)
> +		return -ENOMEM;
> +
> +	pci_set_master(dev);
> +	if (dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(64)) &&
> +	    dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32)))
> +		return -ENODEV;
> +
> +	vmd->msix_count = pci_msix_vec_count(dev);
> +	if (vmd->msix_count < 0)
> +		return -ENODEV;
> +
> +	vmd->irqs = devm_kcalloc(&dev->dev, vmd->msix_count, sizeof(*vmd->irqs),
> +							GFP_KERNEL);
> +	if (!vmd->irqs)
> +		return -ENOMEM;
> +
> +	vmd->msix_entries = devm_kcalloc(&dev->dev, vmd->msix_count,
> +					sizeof(*vmd->msix_entries), GFP_KERNEL);
> +	if(!vmd->msix_entries)
> +		return -ENOMEM;
> +	for (i = 0; i < vmd->msix_count; i++)
> +		vmd->msix_entries[i].entry = i;
> +
> +	vmd->msix_count = pci_enable_msix_range(vmd->dev, vmd->msix_entries, 1,
> +							vmd->msix_count);
> +	if (vmd->msix_count < 0)
> +		return vmd->msix_count;
> +
> +	for (i = 0; i < vmd->msix_count; i++) {
> +		INIT_LIST_HEAD(&vmd->irqs[i].irq_list);
> +		vmd->irqs[i].vmd_vector = vmd->msix_entries[i].vector;
> +		vmd->irqs[i].index = i;
> +
> +		err = devm_request_irq(&dev->dev, vmd->irqs[i].vmd_vector,
> +				vmd_irq, 0, "vmd", &vmd->irqs[i]);
> +		if (err)
> +			return err;
> +	}
> +	spin_lock_init(&vmd->cfg_lock);
> +	pci_set_drvdata(dev, vmd);

Seems like it might be nice to have something in dmesg that would connect
this PCI device to the new PCI domain.  It's a new, unusual topology and a
hint might help everybody understand what's going on.

> +	err = vmd_enable_domain(vmd);
> +	if (err)
> +		return err;
> +	return 0;
> +}

Bjorn

Re: [PATCHv6 6/7] aer_inject: Use 32 bit int type domains

[Keith Busch]

On Thu, Dec 17, 2015 at 11:46:15AM -0600, Bjorn Helgaas wrote:
> On Mon, Dec 07, 2015 at 02:32:28PM -0700, Keith Busch wrote:
> > -	u16 domain;
> > +	int domain;
> 
> If you want 32 bits explicitly, why don't you use u32 here?

It matches the types already defined in struct pci_dev and the
pci_domain_nr return value.

Would you prefer changing all of them to u32, just aer_inject's, or
leave as-is?

Re: [PATCHv6 5/7] x86/pci: Initial commit for new VMD device driver

[Keith Busch]

On Thu, Dec 17, 2015 at 12:14:48PM -0600, Bjorn Helgaas wrote:
> On Mon, Dec 07, 2015 at 02:32:27PM -0700, Keith Busch wrote:
> > +/*
> > + * VMD h/w converts posted config writes to non-posted. The read-back in this
> > + * function forces the completion so it returns only after the config space was
> > + * written, as expected.
> 
> This comment sounds backwards:
> 
>   posted writes don't wait for completion
>   non-posted writes do wait for completion
> 
> If the hardware converts to non-posted writes, you shouldn't need a
> read-back.  It seems like you would need the read-back if the hardware
> converted non-posted to posted.

Oops, the comment has it backwards. Non-posted config writes become
posted memory write requests with this h/w.
 
> Seems like it might be nice to have something in dmesg that would connect
> this PCI device to the new PCI domain.  It's a new, unusual topology and a
> hint might help everybody understand what's going on.

Sounds good, will add.

In addition that, I'd like to mention this patch links a new domain's
root bus kobject under the VMD end-point's so the sysfs hierarchy captures
this as well.

Re: [PATCHv5 7/7] pciutils: Allow 32-bit domains

[Bjorn Helgaas]

On Thu, Dec 17, 2015 at 05:34:46PM +0000, Keith Busch wrote:
> On Thu, Dec 17, 2015 at 11:15:45AM -0600, Bjorn Helgaas wrote:
> > > @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
> > >  	  if (str[0] && strcmp(str, "*"))
> > >  	    {
> > >  	      long int x = strtol(str, &e, 16);
> > > -	      if ((e && *e) || (x < 0 || x > 0xffff))
> > > +	      if ((e && *e) || (x < 0))
> > 
> > Just out of curiosity (I don't maintain pciutils; Martin would apply
> > this one), is there some part of the PCI or PCI firmware spec that is
> > relevant to this change?  Maybe this is connected to parsing things
> > exported by the kernel and not directly tied to PCI at the spec level.
> >
> > Whatever it is, a pointer to the producer of the information you're
> > consuming here would help us understand and review the patch.
> 
> Hi Bjorn,
> 
> This is not tied to anything defined in PCI spec. Domain numbers being
> a software construct (ACPI6, §6.5.6), we don't need to constrain the
> representation. ACPI defines 16-bit segments, and domains provided by
> this new host bridge do not define _SEG, so this series proposes domain
> numbers outside the ACPI reachable range to avoid potential clashes.
> 
> The pciutils patch just synchronizes the essential tooling software with
> the kernel software's new representation.

That's what I figured.  It'd be useful to know exactly what is on the
other end of this, e.g., a Linux /proc or /sys file or whatever it is.

Your changelog assumes a lot of implicit knowledge about Linux, VMD,
and the previous patches in this series.  But pciutils is not
Linux-specific, and it's maintained completely separately from Linux.

This patch needs to supply enough explicit context that it makes sense
all by itself, apart from the kernel series.

Bjorn

Re: [PATCHv5 7/7] pciutils: Allow 32-bit domains

[Martin Mares]

Hello!

> PCI-e segments will continue to use the lower 16 bits as required by
> ACPI. Special domains may use the full 32-bits.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  lib/filter.c |    2 +-
>  lib/pci.h    |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/filter.c b/lib/filter.c
> index d4254a0..075dc2f 100644
> --- a/lib/filter.c
> +++ b/lib/filter.c
> @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
>  	  if (str[0] && strcmp(str, "*"))
>  	    {
>  	      long int x = strtol(str, &e, 16);
> -	      if ((e && *e) || (x < 0 || x > 0xffff))
> +	      if ((e && *e) || (x < 0))
>  		return "Invalid domain number";
>  	      f->domain = x;
>  	    }
> diff --git a/lib/pci.h b/lib/pci.h
> index 10ba831..7e42765 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev
>  
>  struct pci_dev {
>    struct pci_dev *next;			/* Next device in the chain */
> -  u16 domain;				/* PCI domain (host bridge) */
> +  int32_t domain;			/* PCI domain (host bridge) */
>    u8 bus, dev, func;			/* Bus inside domain, device and function */

This is definitely not enough. Try grepping the source for "domain" :-)

At least the following places need updating, too:

  o  struct pci_filter and operations on it

  o  Format strings for printing domains at various places

  o  ABI compability ... changing a field in the middle of struct pci_dev
     (or pci_filter) is going to break ABI, so you either need to change the
     structures in a backward-compatible way, or to use ABI versioning.

Also, we should decide on what type the domain should have -- currently, some
places use "int", others use u16, and your patch introduces int32_t. I would
prefer u32 myself, but especially in the filters we should be careful about
how to encode "any domain".

			Have a nice new year
						Martin

Re: [PATCHv5 7/7] pciutils: Allow 32-bit domains

[Keith Busch]

Hi, thanks for the feedback. I've a few follow up questions.

On Sun, Jan 03, 2016 at 03:11:24PM +0100, Martin Mares wrote:
> This is definitely not enough. Try grepping the source for "domain" :-)
>
> At least the following places need updating, too:
> 
>   o  struct pci_filter and operations on it

Not sure I follow. struct pci_filter's domain was already a 32-bit int.
 
>   o  Format strings for printing domains at various places

Are you wanting a %04x for 16 bit domains and %08x for 32 bit ones? The
%04x specifier still works with 32-bit values.

We just need a bit so this new h/w can't collide with ACPI _SEG defined
domains. I don't know of any real need for the full 32-bits; we'd do
fine using only 17 bits, so thought the leading 0's wasn't useful.

>   o  ABI compability ... changing a field in the middle of struct pci_dev
>      (or pci_filter) is going to break ABI, so you either need to change the
>      structures in a backward-compatible way, or to use ABI versioning.

It looks like there's a 16-bit gap after device_class. Would it be
acceptable place the domain's upper 16 bits in there to keep ABI
compatibility?

> Also, we should decide on what type the domain should have -- currently, some
> places use "int", others use u16, and your patch introduces int32_t. I would
> prefer u32 myself, but especially in the filters we should be careful about
> how to encode "any domain".

I left it as a signed int to allow a negative number for "any", and that's
also what the linux kernel uses.

Re: [PATCHv5 7/7] pciutils: Allow 32-bit domains

[Martin Mares]

Hi!

> On Sun, Jan 03, 2016 at 03:11:24PM +0100, Martin Mares wrote:
> > This is definitely not enough. Try grepping the source for "domain" :-)
> >
> > At least the following places need updating, too:
> > 
> >   o  struct pci_filter and operations on it
> 
> Not sure I follow. struct pci_filter's domain was already a 32-bit int.

Yes, but it has to encode a "don't care" value, too.

However, if 17 bits are sufficient, then let us define that the domain
number is currently 31-bit and keep -1 as "don't care".

In any case, it would be nice to make the sysfs back-end check
that the number provided by the kernel fits in 31 bits.

> >   o  Format strings for printing domains at various places
> 
> Are you wanting a %04x for 16 bit domains and %08x for 32 bit ones? The
> %04x specifier still works with 32-bit values.

After some more thought, keeping %04x will be better.

> >   o  ABI compability ... changing a field in the middle of struct pci_dev
> >      (or pci_filter) is going to break ABI, so you either need to change the
> >      structures in a backward-compatible way, or to use ABI versioning.
> 
> It looks like there's a 16-bit gap after device_class. Would it be
> acceptable place the domain's upper 16 bits in there to keep ABI
> compatibility?

This would mean that all new programs supporting 32-bit domains
would have to combine both fields to get the full domain number.

I think we can just rename the 16-bit field to domain_16 (where only the
lower 16 bits will be stored) and add a new 32-bit domain field at the end of
the public part of the structure. Since the structures are always allocated by
libpci, that will work. Old programs will fail silently on machines with
large domain numbers, but that is hopefully acceptable.

Also, we should add a new version of some basic function call,
for example pci_init, so that new programs will require a new version
of the ABI.

				Martin