[RFC PATCHv3 0/4] Driver for new VMD device

[RFC PATCHv3 0/4] Driver for new VMD device

[Keith Busch]

Here's version 3 for the VMD device driver, and overview of what changed
from v2:

>From review discussions, we discovered potential clashes in domain
numbering (thanks, Bjorn). This new version avoids that clash by using
domain numbers outside the ACPI defined _SEG range. Domains are purely
a software concept, so there is no need to constrain domains to the
possible segment range.

Since this lets domain numbers exceed 16 bit domains, PATCH 4/4 updates
pciutils to understand this so 'lspci' and 'setpci' will work as
expected with these types of domains. This is not a kernel patch, but
wasn't sure if it needs to be sent separately or to a different list,
so it's included here.

After testing configurations with constrained resources, we discovered
pci probe made the previously and perfectly reasonable assumption that
a pci domain provides 256 buses, but that's not always the case with
these domains. PATCH 1/4 addresses the reduced resource conflicts that
may occur during the initial scan.

PATCH 2/4 provides a generic interface to allow pci domains to define
DMA operations specific to their domain. This seemed better than tying
the feature to this new VMD device. An alternative suggestion for future
consideration was to use host_bridge specific operations when those are
provided when that option is implemented.

The main VMD patch in 3/4 is updated to address reviewer comments and
a bug fixes discovered during testing. Specific fixes and updates include:

  Lockdep inversion detection with irq flow handler and msi activation;
  replaced irq flow handler's locking with rcu list.

  Out of range configuration access from constrained CFGBAR.

  Improved description in changelog and Kconfig describing the additional
  bus resource benefit this device provides.

  Proper use of 'pci_add_resource()'.

  Removed unnecessary NULL checking.

  Simplified domain enumeration and removal by calling pci-core API's
  for scan and root bus removal.

  Use "module_pci_driver()" rather than "module_init()"

  Subscribe to the "new" domain specific operations rather than defining
  this as a PCI FIXUP.

  Fixed memory leak if irq_domain creation failed.

Keith Busch (4):
  pci: skip child bus with conflicting resources
  x86/pci: allow pci domain specific dma ops
  x86/pci: Initial commit for new VMD device driver
  pciutils: Allow 32-bit domains

 arch/x86/Kconfig              |   17 ++
 arch/x86/include/asm/device.h |   10 +
 arch/x86/include/asm/vmd.h    |   10 +
 arch/x86/kernel/apic/msi.c    |   38 +++
 arch/x86/pci/Makefile         |    2 +
 arch/x86/pci/common.c         |   38 +++
 arch/x86/pci/vmd.c            |  619 +++++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c           |   10 +-
 kernel/irq/chip.c             |    1 +
 kernel/irq/irqdomain.c        |    3 +
 10 files changed, 746 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/include/asm/vmd.h
 create mode 100644 arch/x86/pci/vmd.c

-- 
1.7.10.4

[RFC PATCHv3 1/4] pci: skip child bus with conflicting resources

[Keith Busch]

And use the max bus resource from the parent rather than assume 255.

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

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8361d27..1cb3be7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -856,7 +856,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 			if (!child)
 				goto out;
 			child->primary = primary;
-			pci_bus_insert_busn_res(child, secondary, subordinate);
+			if (!pci_bus_insert_busn_res(child, secondary, subordinate)) {
+				pci_remove_bus(child);
+				goto out;
+			}
 			child->bridge_ctl = bctl;
 		}
 
@@ -896,7 +899,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 			child = pci_add_new_bus(bus, dev, max+1);
 			if (!child)
 				goto out;
-			pci_bus_insert_busn_res(child, max+1, 0xff);
+			if (!pci_bus_insert_busn_res(child, max+1, bus->busn_res.end)) {
+				pci_remove_bus(child);
+				goto out;
+			}
 		}
 		max++;
 		buses = (buses & 0xff000000)
-- 
1.7.10.4

[RFC PATCHv3 2/4] 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 dc78a4a..524183d 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;
 }
 
-- 
1.7.10.4

[RFC PATCHv3 3/4] 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 reuse 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, a chained MSI irq domain, irq_chip implementation,
and dma operations necessary to support the domain 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 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
   it provides an irq_chip for independent IRQ allocation and 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           |   17 ++
 arch/x86/include/asm/vmd.h |   10 +
 arch/x86/kernel/apic/msi.c |   38 +++
 arch/x86/pci/Makefile      |    2 +
 arch/x86/pci/vmd.c         |  619 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/irq/chip.c          |    1 +
 kernel/irq/irqdomain.c     |    3 +
 7 files changed, 690 insertions(+)
 create mode 100644 arch/x86/include/asm/vmd.h
 create mode 100644 arch/x86/pci/vmd.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 96d058a..53c53f7 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2632,6 +2632,23 @@ config PMC_ATOM
 	def_bool y
         depends on PCI
 
+config HAVE_VMDDEV
+	bool
+
+config VMDDEV
+	depends on PCI && PCI_DOMAINS && PCI_MSI && GENERIC_MSI_IRQ_DOMAIN && IRQ_DOMAIN_HIERARCHY
+	tristate "Volume Management Device Driver"
+	default N
+	select HAVE_VMDDEV
+	---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 to allow more devices than are otherwise possible
+	with a single domain. If your system provides one of these and
+	has devices attached to it, say "Y".
+
 source "net/Kconfig"
 
 source "drivers/Kconfig"
diff --git a/arch/x86/include/asm/vmd.h b/arch/x86/include/asm/vmd.h
new file mode 100644
index 0000000..35429f0
--- /dev/null
+++ b/arch/x86/include/asm/vmd.h
@@ -0,0 +1,10 @@
+#ifndef _ASM_X86_VMD_H
+#define _ASM_X86_VMD_H
+
+#ifdef CONFIG_HAVE_VMDDEV
+struct irq_domain_ops;
+struct irq_domain *vmd_create_irq_domain(struct pci_dev *dev,
+				const struct irq_domain_ops *domain_ops);
+#endif
+
+#endif /* _ASM_X86_VMD_H */
diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
index 5f1feb6..dc773de 100644
--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -22,6 +22,7 @@
 #include <asm/hw_irq.h>
 #include <asm/apic.h>
 #include <asm/irq_remapping.h>
+#include <asm/vmd.h>
 
 static struct irq_domain *msi_default_domain;
 
@@ -134,6 +135,43 @@ static struct msi_domain_info pci_msi_domain_info = {
 	.handler_name	= "edge",
 };
 
+#ifdef CONFIG_HAVE_VMDDEV
+static struct irq_chip pci_chained_msi_controller = {
+	.name			= "PCI-MSI-chained",
+};
+
+static struct msi_domain_info pci_chained_msi_domain_info = {
+	.flags		= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+			  MSI_FLAG_PCI_MSIX,
+	.ops		= &pci_msi_domain_ops,
+	.chip		= &pci_chained_msi_controller,
+};
+
+struct irq_domain *vmd_create_irq_domain(struct pci_dev *dev,
+				const struct irq_domain_ops *domain_ops)
+{
+	int count = 0;
+	struct msi_desc *msi_desc;
+	struct irq_domain *vmd_irqdomain, *msi_irqdomain;
+
+	if (!dev->msix_enabled)
+		return NULL;
+
+	for_each_pci_msi_entry(msi_desc, dev)
+		count += msi_desc->nvec_used;
+	vmd_irqdomain = irq_domain_add_linear(NULL, count,
+						  domain_ops, dev);
+	if (!vmd_irqdomain)
+		return NULL;
+	msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
+						  vmd_irqdomain);
+	if (!msi_irqdomain)
+		irq_domain_remove(vmd_irqdomain);
+	return msi_irqdomain;
+}
+EXPORT_SYMBOL_GPL(vmd_create_irq_domain);
+#endif
+
 void arch_init_msi_domain(struct irq_domain *parent)
 {
 	if (disable_apic)
diff --git a/arch/x86/pci/Makefile b/arch/x86/pci/Makefile
index 5c6fc35..c204079 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_VMDDEV) += 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..7d98d84
--- /dev/null
+++ b/arch/x86/pci/vmd.c
@@ -0,0 +1,619 @@
+/*
+ * 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/irqchip/chained_irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/rculist.h>
+#include <linux/rcupdate.h>
+#include <asm/device.h>
+#include <asm/irqdomain.h>
+#include <asm/hpet.h>
+#include <asm/msidef.h>
+#include <asm/vmd.h>
+
+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 pci_bus *bus;
+	struct irq_domain *irq_domain;
+	struct resource resources[3];
+
+#ifdef CONFIG_X86_DEV_DMA_OPS
+	struct dma_map_ops dma_ops;
+	struct dma_domain dma_domain;
+#endif
+};
+
+struct vmd_irq {
+	struct list_head node;
+	struct vmd_irq_list *irq;
+	unsigned int virq;
+};
+
+static DEFINE_SPINLOCK(list_lock);
+struct vmd_irq_list {
+	struct list_head irq_list;
+	unsigned int vmd_vector;
+	unsigned int index;
+};
+
+static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)
+{
+	return container_of(bus->sysdata, struct vmd_dev, sysdata);
+}
+
+static void vmd_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct vmd_irq *vmdirq = data->chip_data;
+
+	msg->address_hi = MSI_ADDR_BASE_HI;
+	msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_DEST_ID(vmdirq->irq->index);
+	msg->data = MSI_DATA_VECTOR(vmdirq->irq->vmd_vector);
+}
+
+static void vmd_msi_write_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	pci_write_msi_msg(data->irq, msg);
+}
+
+/*
+ * Function is required when using irq hierarchy domains, but we don't have a
+ * good way to not create conflicts with other devices sharing the same vector.
+ */
+int vmd_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
+								bool force)
+{
+	return 0;
+}
+
+static struct irq_chip vmd_chip = {
+	.name 			= "VMD-MSI",
+	.irq_compose_msi_msg	= vmd_msi_compose_msg,
+	.irq_write_msi_msg	= vmd_msi_write_msg,
+	.irq_set_affinity	= vmd_irq_set_affinity,
+};
+
+static void vmd_msi_free_irqs(struct irq_domain *domain,
+				 unsigned int virq, unsigned int nr_irqs)
+{
+	struct vmd_irq *vmdirq;
+	struct irq_data *irq_data;
+
+	irq_data = irq_domain_get_irq_data(domain, virq);
+	if (!irq_data)
+		return;
+	vmdirq = irq_data->chip_data;
+	kfree(vmdirq);
+}
+
+static int vmd_msi_alloc_irqs(struct irq_domain *domain, unsigned int virq,
+				 unsigned int nr_irqs, void *arg)
+{
+	struct vmd_dev *vmd;
+	struct vmd_irq *vmdirq;
+	struct irq_data *irq_data;
+	struct pci_dev *dev = domain->host_data;
+
+	irq_data = irq_domain_get_irq_data(domain, virq);
+	if (!irq_data)
+		return -EINVAL;
+
+	vmd = pci_get_drvdata(dev);
+	vmdirq = kzalloc(sizeof(*vmdirq), GFP_KERNEL);
+	if (!vmdirq)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&vmdirq->node);
+	vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];
+	vmdirq->virq = virq;
+
+	irq_domain_set_hwirq_and_chip(domain, virq, vmdirq->irq->vmd_vector,
+							&vmd_chip, vmdirq);
+	irq_set_chip(virq, &vmd_chip);
+	irq_set_handler_data(virq, vmdirq);
+	__irq_set_handler(virq, handle_simple_irq, 0, NULL);
+
+	return 0;
+}
+
+static void vmd_msi_activate(struct irq_domain *domain,
+					struct irq_data *data)
+{
+	struct msi_msg msg;
+	struct vmd_irq *vmdirq = data->chip_data;
+	struct vmd_irq_list *irq = vmdirq->irq;
+
+	BUG_ON(irq_chip_compose_msi_msg(data, &msg));
+	data->chip->irq_write_msi_msg(data, &msg);
+
+	spin_lock(&list_lock);
+	list_add_tail_rcu(&vmdirq->node, &irq->irq_list);
+	spin_unlock(&list_lock);
+}
+
+static void vmd_msi_deactivate(struct irq_domain *domain,
+				   struct irq_data *data)
+{
+	struct msi_msg msg;
+	struct vmd_irq *vmdirq = data->chip_data;
+
+	memset(&msg, 0, sizeof(msg));
+	data->chip->irq_write_msi_msg(data, &msg);
+
+	spin_lock(&list_lock);
+	list_del_rcu(&vmdirq->node);
+	spin_unlock(&list_lock);
+}
+
+static const struct irq_domain_ops vmd_domain_ops = {
+	.alloc = vmd_msi_alloc_irqs,
+	.free = vmd_msi_free_irqs,
+	.activate = vmd_msi_activate,
+	.deactivate = vmd_msi_deactivate,
+};
+
+#ifdef CONFIG_X86_DEV_DMA_OPS
+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);
+}
+
+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;
+	if (source->alloc)
+		dest->alloc = vmd_alloc;
+	if (source->free)
+		dest->free = vmd_free;
+	if (source->mmap)
+		dest->mmap = vmd_mmap;
+	if (source->get_sgtable)
+		dest->get_sgtable = vmd_get_sgtable;
+	if (source->map_page)
+		dest->map_page = vmd_map_page;
+	if (source->unmap_page)
+		dest->unmap_page = vmd_unmap_page;
+	if (source->map_sg)
+		dest->map_sg = vmd_map_sg;
+	if (source->unmap_sg)
+		dest->unmap_sg = vmd_unmap_sg;
+	if (source->sync_single_for_cpu)
+		dest->sync_single_for_cpu = vmd_sync_single_for_cpu;
+	if (source->sync_single_for_device)
+		dest->sync_single_for_device = vmd_sync_single_for_device;
+	if (source->sync_sg_for_cpu)
+		dest->sync_sg_for_cpu = vmd_sync_sg_for_cpu;
+	if (source->sync_sg_for_device)
+		dest->sync_sg_for_device = vmd_sync_sg_for_device;
+	if (source->mapping_error)
+		dest->mapping_error = vmd_mapping_error;
+	if (source->dma_supported)
+		dest->dma_supported = vmd_dma_supported;
+#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
+	if (source->get_required_mask)
+		dest->get_required_mask = vmd_get_required_mask;
+#endif
+	add_dma_domain(domain);
+}
+#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,
+};
+
+static int vmd_find_free_domain(void)
+{
+	/*
+	 * VMD Domains start at 10000h to not clash with domains defining
+	 * ACPI _SEG.
+	 */
+	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);
+	struct resource cfgbar = {
+		.name = "VMD CFGBAR",
+		.start = 0,
+		.end = (resource_size(&vmd->dev->resource[0]) >> 20) - 1,
+		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
+	};
+	struct resource membar1 = {
+		.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),
+	};
+	struct resource membar2 = {
+		.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),
+	};
+
+	vmd->resources[0] = cfgbar;
+	vmd->resources[1] = membar1;
+	vmd->resources[2] = membar2;
+
+	sd->domain = vmd_find_free_domain();
+	if (sd->domain < 0)
+		return sd->domain;
+	sd->node = pcibus_to_node(vmd->dev->bus);
+
+	vmd->irq_domain = vmd_create_irq_domain(vmd->dev, &vmd_domain_ops);
+	if (!vmd->irq_domain)
+		return -ENODEV;
+
+	vmd_setup_dma_ops(vmd);
+
+	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) {
+		irq_domain_remove(vmd->irq_domain);
+		pci_free_resource_list(&resources);
+		return -ENODEV;
+	}
+	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 void vmd_flow_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct vmd_irq_list *irqs = irq_desc_get_handler_data(desc);
+	struct vmd_irq *vmdirq;
+
+	chained_irq_enter(chip, desc);
+	rcu_read_lock();
+	list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
+		generic_handle_irq(vmdirq->virq);
+	rcu_read_unlock();
+	chained_irq_exit(chip, desc);
+}
+
+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)
+		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;
+
+		irq_set_chained_handler_and_data(vmd->msix_entries[i].vector,
+					vmd_flow_handler, &vmd->irqs[i]);
+	}
+	spin_lock_init(&vmd->cfg_lock);
+	err = vmd_enable_domain(vmd);
+	if (err)
+		return err;
+
+	pci_set_drvdata(dev, vmd);
+	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);
+}
+
+static const struct pci_device_id vmd_ids[] = {
+	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x201d),},
+	{0,}
+};
+MODULE_DEVICE_TABLE(pci, vmd_ids);
+
+struct pci_driver vmd_drv = {
+	.name		= "vmd",
+	.id_table	= vmd_ids,
+	.probe		= vmd_probe,
+	.remove		= vmd_remove,
+};
+module_pci_driver(vmd_drv);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("0.2");
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index e28169d..e566a6b 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1062,3 +1062,4 @@ int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(irq_chip_compose_msi_msg);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index dc9d27c..8303ccb 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -910,6 +910,7 @@ struct irq_data *irq_domain_get_irq_data(struct irq_domain *domain,
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(irq_domain_get_irq_data);
 
 /**
  * irq_domain_set_hwirq_and_chip - Set hwirq and irqchip of @virq at @domain
@@ -934,6 +935,7 @@ int irq_domain_set_hwirq_and_chip(struct irq_domain *domain, unsigned int virq,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(irq_domain_set_hwirq_and_chip);
 
 /**
  * irq_domain_set_info - Set the complete data for a @virq in @domain
@@ -1240,6 +1242,7 @@ struct irq_data *irq_domain_get_irq_data(struct irq_domain *domain,
 
 	return (irq_data && irq_data->domain == domain) ? irq_data : NULL;
 }
+EXPORT_SYMBOL_GPL(irq_domain_get_irq_data);
 
 /**
  * irq_domain_set_info - Set the complete data for a @virq in @domain
-- 
1.7.10.4

Re: [RFC PATCHv3 3/4] x86/pci: Initial commit for new VMD device driver

[Thomas Gleixner]

Keith!

On Tue, 27 Oct 2015, Keith Busch wrote:

I'm just looking at the interrupt part of the patch and it's way
better than the first version I looked at.

>   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 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
>   it provides an irq_chip for independent IRQ allocation and to relay
>   interrupts from VMD's interrupt handler to the appropriate device
>   driver's handler.

Is there any documentation on this available for mere mortals?

I have a faint idea how this is supposed to work, but some more
detailed information about this technology would be appreciated.
  
> +config HAVE_VMDDEV
> +	bool

Why do you need a seperate config symbol here?

We usually use HAVE_xxx to signal the availability of functionality
which is then used by some other Kconfig entry as a dependency.

> +config VMDDEV
> +	depends on PCI && PCI_DOMAINS && PCI_MSI && GENERIC_MSI_IRQ_DOMAIN && IRQ_DOMAIN_HIERARCHY

On x86 this dependency chain is not required. x86/Kconfig does:

   config PCI_DOMAINS
     def_bool y
     depends on PCI

and

    select PCI_MSI_IRQ_DOMAIN if PCI_MSI

and pci/Kconfig does:

  config PCI_MSI_IRQ_DOMAIN
    bool
    depends on PCI_MSI
    select GENERIC_MSI_IRQ_DOMAIN

kernel/irq/Kconfig has:

  config GENERIC_MSI_IRQ_DOMAIN
    bool
    select IRQ_DOMAIN_HIERARCHY

So all you really need is:

   depends on PCI_MSI

> +	tristate "Volume Management Device Driver"
> +	default N
> +	select HAVE_VMDDEV
> +	---help---
> +	Adds support for the Intel Volume Manage Device (VMD). VMD is

The help text should be indented with 2 extra spaces.

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

Missing period after domain.

> +	bus resources to allow more devices than are otherwise possible
> +	with a single domain. If your system provides one of these and
> +	has devices attached to it, say "Y".

How is one supposed to figure out that the system supports this?

> +#ifndef _ASM_X86_VMD_H
> +#define _ASM_X86_VMD_H
> +
> +#ifdef CONFIG_HAVE_VMDDEV

No need for that #ifdef

> +struct irq_domain_ops;
> +struct irq_domain *vmd_create_irq_domain(struct pci_dev *dev,
> +				const struct irq_domain_ops *domain_ops);
> +#endif

  
> +#ifdef CONFIG_HAVE_VMDDEV
> +static struct irq_chip pci_chained_msi_controller = {
> +	.name			= "PCI-MSI-chained",
> +};
> +
> +static struct msi_domain_info pci_chained_msi_domain_info = {
> +	.flags		= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +			  MSI_FLAG_PCI_MSIX,
> +	.ops		= &pci_msi_domain_ops,
> +	.chip		= &pci_chained_msi_controller,
> +};
> +
> +struct irq_domain *vmd_create_irq_domain(struct pci_dev *dev,
> +				const struct irq_domain_ops *domain_ops)

This function really wants a comment what it is doing. AFAICT it
creates a disconnected vmd_irqdomain and a pci_msi irq domain as a
child domain of that. I'm missing the overall picture ...

> +{
> +	int count = 0;
> +	struct msi_desc *msi_desc;
> +	struct irq_domain *vmd_irqdomain, *msi_irqdomain;
> +
> +	if (!dev->msix_enabled)
> +		return NULL;
> +
> +	for_each_pci_msi_entry(msi_desc, dev)
> +		count += msi_desc->nvec_used;

Is this the number of vectors which are available for the VMD device
itself?

> +	vmd_irqdomain = irq_domain_add_linear(NULL, count,
> +						  domain_ops, dev);
> +	if (!vmd_irqdomain)
> +		return NULL;
> +	msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
> +						  vmd_irqdomain);
> +	if (!msi_irqdomain)
> +		irq_domain_remove(vmd_irqdomain);
> +	return msi_irqdomain;
> +}
> +EXPORT_SYMBOL_GPL(vmd_create_irq_domain);
> +#endif

> +++ b/arch/x86/pci/vmd.c

> +struct vmd_dev {
> +	struct pci_dev *dev;
> +
> +	spinlock_t cfg_lock;
> +	char __iomem *cfgbar;

It'd be nice if you could write those structs like a table

+	struct pci_dev		*dev;
+
+	spinlock_t		cfg_lock;
+	char __iomem		*cfgbar;

That makes it way simpler to parse.

> +struct vmd_irq {
> +	struct list_head node;
> +	struct vmd_irq_list *irq;
> +	unsigned int virq;
> +};
> +
> +static DEFINE_SPINLOCK(list_lock);

Please do not put a variable declaration in the middle of struct
definitions.

> +struct vmd_irq_list {
> +	struct list_head irq_list;
> +	unsigned int vmd_vector;
> +	unsigned int index;
> +};

These structs want an explanation for the struct members. Especially
the "*irq" one in vmd_irq is irritating.

> +static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)
> +{
> +	return container_of(bus->sysdata, struct vmd_dev, sysdata);
> +}
> +
> +static void vmd_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	struct vmd_irq *vmdirq = data->chip_data;
> +
> +	msg->address_hi = MSI_ADDR_BASE_HI;
> +	msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_DEST_ID(vmdirq->irq->index);
> +	msg->data = MSI_DATA_VECTOR(vmdirq->irq->vmd_vector);

Is this the MSI msg which gets written into the VMD device or into the
device which hangs off the VMD device bus?

> +}
> +
> +static void vmd_msi_write_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	pci_write_msi_msg(data->irq, msg);

Why would you go through another lookup for the msi descriptor? What's
wrong with pci_msi_domain_write_msg() ?

> +}
> +
> +/*
> + * Function is required when using irq hierarchy domains, but we don't have a
> + * good way to not create conflicts with other devices sharing the same vector.

This is just a lame work around and suggests the user that he can set
the affinity.

You can prevent affinity setting by other means. 

Aside of that the question is whether you really want to prevent
affinity setting of all involved interrupts - the demultiplexers and
the device interrupts. Though I don't have any clue how this is going
to be used.

> + */
> +int vmd_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
> +								bool force)

Is there a reason why this function would be global?

> +{
> +	return 0;
> +}
> +
> +static struct irq_chip vmd_chip = {
> +	.name 			= "VMD-MSI",
> +	.irq_compose_msi_msg	= vmd_msi_compose_msg,
> +	.irq_write_msi_msg	= vmd_msi_write_msg,
> +	.irq_set_affinity	= vmd_irq_set_affinity,
> +};
> +
> +static void vmd_msi_free_irqs(struct irq_domain *domain,
> +				 unsigned int virq, unsigned int nr_irqs)
> +{
> +	struct vmd_irq *vmdirq;
> +	struct irq_data *irq_data;
> +
> +	irq_data = irq_domain_get_irq_data(domain, virq);
> +	if (!irq_data)
> +		return;
> +	vmdirq = irq_data->chip_data;
> +	kfree(vmdirq);

Shouldn't that be kfree_rcu()?

> +}
> +
> +static int vmd_msi_alloc_irqs(struct irq_domain *domain, unsigned int virq,
> +				 unsigned int nr_irqs, void *arg)
> +{
> +	struct vmd_dev *vmd;
> +	struct vmd_irq *vmdirq;
> +	struct irq_data *irq_data;
> +	struct pci_dev *dev = domain->host_data;
> +
> +	irq_data = irq_domain_get_irq_data(domain, virq);
> +	if (!irq_data)
> +		return -EINVAL;
> +
> +	vmd = pci_get_drvdata(dev);
> +	vmdirq = kzalloc(sizeof(*vmdirq), GFP_KERNEL);
> +	if (!vmdirq)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&vmdirq->node);
> +	vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];

So that points to the underlying VMD device irq, right? And you spread
the device interrupts across the available VMD irqs. So in the worst
case you might end up with empty and crowded VMD irqs, right?
Shouldn't you try to fill that space in a more intelligent way?

> +	vmdirq->virq = virq;
> +
> +	irq_domain_set_hwirq_and_chip(domain, virq, vmdirq->irq->vmd_vector,
> +							&vmd_chip, vmdirq);
> +	irq_set_chip(virq, &vmd_chip);
> +	irq_set_handler_data(virq, vmdirq);
> +	__irq_set_handler(virq, handle_simple_irq, 0, NULL);

What's wrong with irq_set_handler() ?

> +
> +	return 0;
> +}
> +
> +static void vmd_msi_activate(struct irq_domain *domain,
> +					struct irq_data *data)
> +{
> +	struct msi_msg msg;
> +	struct vmd_irq *vmdirq = data->chip_data;
> +	struct vmd_irq_list *irq = vmdirq->irq;
> +
> +	BUG_ON(irq_chip_compose_msi_msg(data, &msg));
> +	data->chip->irq_write_msi_msg(data, &msg);
> +
> +	spin_lock(&list_lock);
> +	list_add_tail_rcu(&vmdirq->node, &irq->irq_list);
> +	spin_unlock(&list_lock);
> +}
> +
> +static void vmd_msi_deactivate(struct irq_domain *domain,
> +				   struct irq_data *data)
> +{
> +	struct msi_msg msg;
> +	struct vmd_irq *vmdirq = data->chip_data;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	data->chip->irq_write_msi_msg(data, &msg);
> +
> +	spin_lock(&list_lock);
> +	list_del_rcu(&vmdirq->node);
> +	spin_unlock(&list_lock);
> +}
> +
> +static const struct irq_domain_ops vmd_domain_ops = {
> +	.alloc = vmd_msi_alloc_irqs,
> +	.free = vmd_msi_free_irqs,
> +	.activate = vmd_msi_activate,
> +	.deactivate = vmd_msi_deactivate,

You nicely aligned the irq_chip above. Can you please use the same
scheme here?

> +static int vmd_enable_domain(struct vmd_dev *vmd)
> +{
> +	struct pci_sysdata *sd = &vmd->sysdata;

<SNIP>

> +	vmd->irq_domain = vmd_create_irq_domain(vmd->dev, &vmd_domain_ops);

This is the irq domain which the devices on that VMD bus are seeing, right?

> +	if (!vmd->irq_domain)
> +		return -ENODEV;

> +static void vmd_flow_handler(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct vmd_irq_list *irqs = irq_desc_get_handler_data(desc);
> +	struct vmd_irq *vmdirq;
> +
> +	chained_irq_enter(chip, desc);
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
> +		generic_handle_irq(vmdirq->virq);
> +	rcu_read_unlock();

I'm not entirely sure that the rcu protection here covers the whole
virq thing when an interrupt is torn down. Can you please explain the
whole protection scheme in detail?

> +	chained_irq_exit(chip, desc);
> +}
> +
> +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)
> +		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);

So this allocates the CPU facing MSI vectors, which are used to run
the demultiplex handler, right?

A few comments explaining the whole thing would be really
appropriate. I'm sure you'll appreciate them when you look at it
yourself a year from now.

> +	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;
> +
> +		irq_set_chained_handler_and_data(vmd->msix_entries[i].vector,
> +					vmd_flow_handler, &vmd->irqs[i]);
> +	}
> +	spin_lock_init(&vmd->cfg_lock);
> +	err = vmd_enable_domain(vmd);
> +	if (err)
> +		return err;

What undoes the above in case that vmd_enable_domain() fails? Same
question for all other error returns ...

> +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);

What tears down the chained handlers and the MSIx entries?

> +EXPORT_SYMBOL_GPL(irq_chip_compose_msi_msg);
> +EXPORT_SYMBOL_GPL(irq_domain_get_irq_data);
> +EXPORT_SYMBOL_GPL(irq_domain_set_hwirq_and_chip);
> +EXPORT_SYMBOL_GPL(irq_domain_get_irq_data);

This wants to be a seperate patch please.

Thanks,

	tglx

Re: [RFC PATCHv3 3/4] x86/pci: Initial commit for new VMD device driver

[Keith Busch]

On Mon, Nov 02, 2015 at 07:35:15PM +0100, Thomas Gleixner wrote:
> On Tue, 27 Oct 2015, Keith Busch wrote:

Thomas,

Thanks a bunch for the feedback! I'll reply what I can right now, and
will take more time to consider or fix the rest for the next revision.
 
> I'm just looking at the interrupt part of the patch and it's way
> better than the first version I looked at.

Thanks! Though I have to credit Gerry for the initial setup.

I think you correctly deduced the interrupt mux/demux requirement. I
don't even completely follow how the irq domain code accomplishes this,
but it works on the h/w I've been provided and I trust Gerry wouldn't
steer me wrong with the software. :)

> > +config HAVE_VMDDEV
> > +	bool
> 
> Why do you need a seperate config symbol here?

This is to split the kernel vs. module parts. "vmd_create_irq_domain"
needs to be in-kernel, but VMD may be compiled as a module, so can't use
"CONFIG_VMD_DEV".

Alternatively we could export pci_msi_domain_ops, and no additional
in-kernel dependencies are required.

> > +	bus resources to allow more devices than are otherwise possible
> > +	with a single domain. If your system provides one of these and
> > +	has devices attached to it, say "Y".
> 
> How is one supposed to figure out that the system supports this?

I'll work out more appropriate text. Basically enabling this is a
platform setting that requires purposeful intent, almost certainly not
by accident. The user ought to know prior to OS install, so maybe it
should say 'if you're not sure, say "N"'.

> > +	int count = 0;
> > +	struct msi_desc *msi_desc;
> > +	struct irq_domain *vmd_irqdomain, *msi_irqdomain;
> > +
> > +	if (!dev->msix_enabled)
> > +		return NULL;
> > +
> > +	for_each_pci_msi_entry(msi_desc, dev)
> > +		count += msi_desc->nvec_used;
> 
> Is this the number of vectors which are available for the VMD device
> itself?

Correct, this is counting the number of the VMD vectors itself. This is
not the number of vectors that may be allocated in this domain, though
they will all need to multiplex into one of these.

> > +static void vmd_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> > +{
> > +	struct vmd_irq *vmdirq = data->chip_data;
> > +
> > +	msg->address_hi = MSI_ADDR_BASE_HI;
> > +	msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_DEST_ID(vmdirq->irq->index);
> > +	msg->data = MSI_DATA_VECTOR(vmdirq->irq->vmd_vector);
> 
> Is this the MSI msg which gets written into the VMD device or into the
> device which hangs off the VMD device bus?

This is the message to be written in the "child" device hanging off
the VMD domain. The child table must be programmed with VMD vectors,
and many child devices may share the same one. In some cases, a single
device might have multiple table entries with the same vector.

The child device driver's IRQ is purely a software concept in this domain
to facilitate chaining the VMD IRQ to the appropriate handler. The h/w
will never see it.

> > +	vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];
> 
> So that points to the underlying VMD device irq, right? And you spread
> the device interrupts across the available VMD irqs. So in the worst
> case you might end up with empty and crowded VMD irqs, right?
> Shouldn't you try to fill that space in a more intelligent way?

The domain is enabled and enumerated by pci, so I thought devices will
be bound to their drivers serially, creating a more-or-less incrementing
virtual irq that should wrap in evenly.

But yes, we can be smarter about this, and the above is probably flawed
rationale, especially if considering hot-plug.

> > +static int vmd_enable_domain(struct vmd_dev *vmd)
> > +{
> > +	struct pci_sysdata *sd = &vmd->sysdata;
> 
> <SNIP>
> 
> > +	vmd->irq_domain = vmd_create_irq_domain(vmd->dev, &vmd_domain_ops);
> 
> This is the irq domain which the devices on that VMD bus are seeing, right?

Correct.
 
> > +	if (!vmd->irq_domain)
> > +		return -ENODEV;
> 
> > +static void vmd_flow_handler(struct irq_desc *desc)
> > +{
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	struct vmd_irq_list *irqs = irq_desc_get_handler_data(desc);
> > +	struct vmd_irq *vmdirq;
> > +
> > +	chained_irq_enter(chip, desc);
> > +	rcu_read_lock();
> > +	list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
> > +		generic_handle_irq(vmdirq->virq);
> > +	rcu_read_unlock();
> 
> I'm not entirely sure that the rcu protection here covers the whole
> virq thing when an interrupt is torn down. Can you please explain the
> whole protection scheme in detail?

This protects against device drivers manipulating the list through
vmd_msi_activate/deactivate. We can't spinlock the flow handler access
due to desc->lock taken through generic_handle_irq. The same lock is
held prior to vmd's msi activate/deactivate, so the lock order would be
opposite in those two paths.

I don't believe we're in danger of missing a necessary event or triggering
an unexpected event here. Is the concern that we need to synchronize rcu
instead of directly freeing the "vmdirq"? I think I see that possibility.

> > +	vmd->msix_count = pci_enable_msix_range(vmd->dev, vmd->msix_entries, 1,
> > +							vmd->msix_count);
> 
> So this allocates the CPU facing MSI vectors, which are used to run
> the demultiplex handler, right?

Correct.
 
> A few comments explaining the whole thing would be really
> appropriate. I'm sure you'll appreciate them when you look at it
> yourself a year from now.

Absolutely. It's not entirely obvious what's happening, but I think you
correctly surmised what this is about, and hopefully some of the answers
to your other questions helped with some details.

We will provide more detail in code comments for the next round.

> > +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);
> 
> What tears down the chained handlers and the MSIx entries?

Thanks, I will fix the chained handler in both cases mentioned.

The entries themselves should be freed through devres_release after this
exits, and after MSI-x is disabled via pcim_release.

Re: [RFC PATCHv3 3/4] x86/pci: Initial commit for new VMD device driver

[Thomas Gleixner]

Keith,

On Tue, 3 Nov 2015, Keith Busch wrote:
> On Mon, Nov 02, 2015 at 07:35:15PM +0100, Thomas Gleixner wrote:
> > On Tue, 27 Oct 2015, Keith Busch wrote:

> > > +config HAVE_VMDDEV
> > > +	bool
> > 
> > Why do you need a seperate config symbol here?
> 
> This is to split the kernel vs. module parts. "vmd_create_irq_domain"
> needs to be in-kernel, but VMD may be compiled as a module, so can't use
> "CONFIG_VMD_DEV".

#ifdef IS_ENABLED(CONFIG_VMD_DEV)

is true for both "m" and "y"
 
> Alternatively we could export pci_msi_domain_ops, and no additional
> in-kernel dependencies are required.

That might be not the worst choice.
 
> > > +	int count = 0;
> > > +	struct msi_desc *msi_desc;
> > > +	struct irq_domain *vmd_irqdomain, *msi_irqdomain;
> > > +
> > > +	if (!dev->msix_enabled)
> > > +		return NULL;
> > > +
> > > +	for_each_pci_msi_entry(msi_desc, dev)
> > > +		count += msi_desc->nvec_used;
> > 
> > Is this the number of vectors which are available for the VMD device
> > itself?
> 
> Correct, this is counting the number of the VMD vectors itself. This is
> not the number of vectors that may be allocated in this domain, though
> they will all need to multiplex into one of these.

But you actually limit the number of vectors in that domain:

> > > +	for_each_pci_msi_entry(msi_desc, dev)
> > > +		count += msi_desc->nvec_used;
> > > +	vmd_irqdomain = irq_domain_add_linear(NULL, count,
> > > +						  domain_ops, dev);

So vmd_irqdomain can only hand out "count" vectors. 

The vmd_irqdomain is the parent of the msi_irqdomain:

> > > +	msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
> > > +						  vmd_irqdomain);

But that parent limitation does not matter simply because your
msi_irqdomain does not follow down the hierarchy in the allocation
path.

So we can avoid the vmd_irqdomain creation completely. It's just
wasting memory and has no value at all. Creating the msi domain with a
NULL parent is possible.

> > > +	vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];
> > 
> > So that points to the underlying VMD device irq, right? And you spread
> > the device interrupts across the available VMD irqs. So in the worst
> > case you might end up with empty and crowded VMD irqs, right?
> > Shouldn't you try to fill that space in a more intelligent way?
> 
> The domain is enabled and enumerated by pci, so I thought devices will
> be bound to their drivers serially, creating a more-or-less incrementing
> virtual irq that should wrap in evenly.
> 
> But yes, we can be smarter about this, and the above is probably flawed
> rationale, especially if considering hot-plug.

Not only hotplug. It also depends on the init ordering and the
population of the interrupt descriptor space.
 
> > > +static void vmd_flow_handler(struct irq_desc *desc)
> > > +{
> > > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > > +	struct vmd_irq_list *irqs = irq_desc_get_handler_data(desc);
> > > +	struct vmd_irq *vmdirq;
> > > +
> > > +	chained_irq_enter(chip, desc);
> > > +	rcu_read_lock();
> > > +	list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
> > > +		generic_handle_irq(vmdirq->virq);
> > > +	rcu_read_unlock();
> > 
> > I'm not entirely sure that the rcu protection here covers the whole
> > virq thing when an interrupt is torn down. Can you please explain the
> > whole protection scheme in detail?
> 
> This protects against device drivers manipulating the list through
> vmd_msi_activate/deactivate. We can't spinlock the flow handler access
> due to desc->lock taken through generic_handle_irq. The same lock is
> held prior to vmd's msi activate/deactivate, so the lock order would be
> opposite in those two paths.
> 
> I don't believe we're in danger of missing a necessary event or triggering
> an unexpected event here. Is the concern that we need to synchronize rcu
> instead of directly freeing the "vmdirq"? I think I see that possibility.

synchronize rcu does not help. You surely need to kfree_rcu()
"vmdirq", but that alone is not sufficient. You also need to think
about the underlying irq descriptor, which is not freed via rcu. Lets
look at it:

CPU0					CPU1

driver_exit()
 free_irq(D)
  lock(irqdesc(D))
  irq_shutdown(D)
    irq_domain_deactivate_irq(D)	vmd_flow_handler()
      vmd_msi_deactivate()		  rcu_read_lock()
        lock()				  vmdirq = list_entry(...)
        list_del_rcu()			---> NMI  
	unlock()
  unlock(irqdesc(D))
  synchronize_irq(D)			    
 pci_misx_disable(device)
  irq_domain_free_irqs()
    vmd_msi_free_irqs()			  
      kfree_rcu(vmdirq)			<--- NMI done  
					  D = vmdirq->virq; <- Protected by RCU
					  generic_handle_irq(D)
					    desc = irqdesc(D)
    irq_free_descs()
     kfree(irqdesc(D))
					    desc->handle_irq(desc) <-- KABOOOOM!

Subtle, isn't it? It's extremly unlikely, but when it happens you have
no chance to ever debug that.

The good news for you is that this is a general issue and you can
ignore it for now. The bad news for me is, that I have another urgent
issue on my ever growing todo list.

> > > +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);
> > 
> > What tears down the chained handlers and the MSIx entries?
> 
> Thanks, I will fix the chained handler in both cases mentioned.
> 
> The entries themselves should be freed through devres_release after this
> exits, and after MSI-x is disabled via pcim_release.
 
Ok.

Thanks,

	tglx

Re: [RFC PATCHv3 3/4] x86/pci: Initial commit for new VMD device driver

[Keith Busch]

On Tue, Nov 03, 2015 at 12:42:02PM +0100, Thomas Gleixner wrote:
> On Tue, 3 Nov 2015, Keith Busch wrote:
> > > > +	msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
> > > > +						  vmd_irqdomain);
> 
> But that parent limitation does not matter simply because your
> msi_irqdomain does not follow down the hierarchy in the allocation
> path.
> 
> So we can avoid the vmd_irqdomain creation completely. It's just
> wasting memory and has no value at all. Creating the msi domain with a
> NULL parent is possible.

I'm having trouble following the hierarchy and didn't understand the
connection between the parent and msi comain. It's still new to me,
but I don't think a NULL parent is allowable with msi domains:

 pci_msi_setup_msi_irqs()
  pci_msi_domain_alloc_irqs()
   msi_domain_alloc_irqs()
    __irq_domain_alloc_irqs()
     irq_domain_alloc_irqs_recursive()
      msi_domain_alloc()
       irq_domain_alloc_irqs_parent()

The last call returns -ENOSYS since there parent is NULL. Was the
intension to allow no parent, or do I still need to allocate one to
achieve the desired chaining?

Re: [RFC PATCHv3 3/4] x86/pci: Initial commit for new VMD device driver

[Thomas Gleixner]

On Wed, 4 Nov 2015, Keith Busch wrote:

> On Tue, Nov 03, 2015 at 12:42:02PM +0100, Thomas Gleixner wrote:
> > On Tue, 3 Nov 2015, Keith Busch wrote:
> > > > > +	msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
> > > > > +						  vmd_irqdomain);
> > 
> > But that parent limitation does not matter simply because your
> > msi_irqdomain does not follow down the hierarchy in the allocation
> > path.
> > 
> > So we can avoid the vmd_irqdomain creation completely. It's just
> > wasting memory and has no value at all. Creating the msi domain with a
> > NULL parent is possible.
> 
> I'm having trouble following the hierarchy and didn't understand the
> connection between the parent and msi comain. It's still new to me,
> but I don't think a NULL parent is allowable with msi domains:
> 
>  pci_msi_setup_msi_irqs()
>   pci_msi_domain_alloc_irqs()
>    msi_domain_alloc_irqs()
>     __irq_domain_alloc_irqs()
>      irq_domain_alloc_irqs_recursive()
>       msi_domain_alloc()
>        irq_domain_alloc_irqs_parent()
> 
> The last call returns -ENOSYS since there parent is NULL. Was the
> intension to allow no parent, or do I still need to allocate one to
> achieve the desired chaining?

Hmm, seems I missed that part. But that's a fixable problem. Jiang?

Thanks,

	tglx
 

Re: [RFC PATCHv3 3/4] x86/pci: Initial commit for new VMD device driver

[Jiang Liu]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]



On 2015/11/4 23:14, Thomas Gleixner wrote:
> On Wed, 4 Nov 2015, Keith Busch wrote:
> 
>> On Tue, Nov 03, 2015 at 12:42:02PM +0100, Thomas Gleixner wrote:
>>> On Tue, 3 Nov 2015, Keith Busch wrote:
>>>>>> +	msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
>>>>>> +						  vmd_irqdomain);
>>>
>>> But that parent limitation does not matter simply because your
>>> msi_irqdomain does not follow down the hierarchy in the allocation
>>> path.
>>>
>>> So we can avoid the vmd_irqdomain creation completely. It's just
>>> wasting memory and has no value at all. Creating the msi domain with a
>>> NULL parent is possible.
>>
>> I'm having trouble following the hierarchy and didn't understand the
>> connection between the parent and msi comain. It's still new to me,
>> but I don't think a NULL parent is allowable with msi domains:
>>
>>  pci_msi_setup_msi_irqs()
>>   pci_msi_domain_alloc_irqs()
>>    msi_domain_alloc_irqs()
>>     __irq_domain_alloc_irqs()
>>      irq_domain_alloc_irqs_recursive()
>>       msi_domain_alloc()
>>        irq_domain_alloc_irqs_parent()
>>
>> The last call returns -ENOSYS since there parent is NULL. Was the
>> intension to allow no parent, or do I still need to allocate one to
>> achieve the desired chaining?
> 
> Hmm, seems I missed that part. But that's a fixable problem. Jiang?
Hi Keith,
	Could you please try the attached patch?
Thanks!
Gerry

> 
> Thanks,
> 
> 	tglx
>  
> --
> 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/
> 

[-- Attachment #2: 0001-msi-Relax-msi_domain_alloc-to-support-parentless-MSI.patch --]
[-- Type: text/x-patch, Size: 1266 bytes --]

>From 6db1568564f8edee626d1c168b22ed5502abc6eb Mon Sep 17 00:00:00 2001
From: Liu Jiang <jiang.liu@linux.intel.com>
Date: Thu, 5 Nov 2015 11:25:07 +0800
Subject: [PATCH] msi: Relax msi_domain_alloc() to support parentless MSI
 irqdomains

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: Liu Jiang <jiang.liu@linux.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 7e6512b9dc1f..e4d3d707efff 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);
-- 
1.7.10.4

Re: [RFC PATCHv3 3/4] x86/pci: Initial commit for new VMD device driver

[Keith Busch]

On Thu, Nov 05, 2015 at 02:35:31PM +0800, Jiang Liu wrote:
> Hi Keith,
> 	Could you please try the attached patch?
> Thanks!
> Gerry

Thanks! I anticipated this and tested the same thing yesterday, and it
is successful.

I'll apply to the series and send a new revision hopefully today. Not
requiring a parent simplifies the driver since we don't define
irq_domain_ops now, and the functionality they provided moves to the
irq_chip.



> From: Liu Jiang <jiang.liu@linux.intel.com>
> Date: Thu, 5 Nov 2015 11:25:07 +0800
> Subject: [PATCH] msi: Relax msi_domain_alloc() to support parentless MSI
>  irqdomains
> 
> 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: Liu Jiang <jiang.liu@linux.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 7e6512b9dc1f..e4d3d707efff 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);
> -- 
> 1.7.10.4

[RFC PATCHv3 4/4] 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