Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1307797 > unrolled thread

[PATCHv8 0/5] Driver for new "VMD" device

Back to article view | Back to linux.kernel

Contents

  [PATCHv8 0/5] Driver for new "VMD" device  Keith Busch <keith.busch@intel.com> - 2016-01-12 21:20 +0100
    [PATCHv8 1/5] msi: Relax msi_domain_alloc() to support parentless MSI irqdomains Keith Busch <keith.busch@intel.com> - 2016-01-12 21:20 +0100
    [PATCHv8 4/5] PCI/AER: Use 32 bit int type domains Keith Busch <keith.busch@intel.com> - 2016-01-12 21:20 +0100
    [PATCHv8 5/5] x86/PCI: Initial commit for new VMD device driver Keith Busch <keith.busch@intel.com> - 2016-01-12 21:20 +0100
    [PATCHv8 2/5] x86/IRQ: Export IRQ domain function for module use Keith Busch <keith.busch@intel.com> - 2016-01-12 21:20 +0100
    [PATCHv8 3/5] x86/PCI: Allow PCI domain specific dma ops Keith Busch <keith.busch@intel.com> - 2016-01-12 21:20 +0100
    Re: [PATCHv8 0/5] Driver for new "VMD" device Bjorn Helgaas <helgaas@kernel.org> - 2016-01-15 19:20 +0100
      Re: [PATCHv8 0/5] Driver for new "VMD" device "Veal, Bryan E." <bryan.e.veal@intel.com> - 2016-01-15 20:40 +0100
        Re: [PATCHv8 0/5] Driver for new "VMD" device Bjorn Helgaas <helgaas@kernel.org> - 2016-01-15 22:50 +0100
          Re: [PATCHv8 0/5] Driver for new "VMD" device "Veal, Bryan E." <bryan.e.veal@intel.com> - 2016-01-16 23:30 +0100
            Re: [PATCHv8 0/5] Driver for new "VMD" device Bjorn Helgaas <helgaas@kernel.org> - 2016-01-20 23:30 +0100
      Re: [PATCHv8 0/5] Driver for new "VMD" device Keith Busch <keith.busch@intel.com> - 2016-01-15 20:40 +0100
      Re: [PATCHv8 0/5] Driver for new "VMD" device Thomas Gleixner <tglx@linutronix.de> - 2016-01-15 20:50 +0100
        Re: [PATCHv8 0/5] Driver for new "VMD" device Bjorn Helgaas <bhelgaas@google.com> - 2016-01-15 21:10 +0100
          Re: [PATCHv8 0/5] Driver for new "VMD" device Thomas Gleixner <tglx@linutronix.de> - 2016-01-15 21:20 +0100
      RE: [PATCHv8 0/5] Driver for new "VMD" device "Derrick, Jonathan" <jonathan.derrick@intel.com> - 2016-01-15 20:50 +0100
        Re: [PATCHv8 0/5] Driver for new "VMD" device Keith Busch <keith.busch@intel.com> - 2016-01-15 21:00 +0100
          Re: [PATCHv8 0/5] Driver for new "VMD" device Jon Derrick <jonathan.derrick@intel.com> - 2016-01-15 21:10 +0100
        Re: [PATCHv8 0/5] Driver for new "VMD" device Bjorn Helgaas <helgaas@kernel.org> - 2016-01-15 23:10 +0100
          Re: [PATCHv8 0/5] Driver for new "VMD" device Keith Busch <keith.busch@intel.com> - 2016-01-19 16:40 +0100
            Re: [PATCHv8 0/5] Driver for new "VMD" device Christoph Hellwig <hch@infradead.org> - 2016-01-19 17:10 +0100
              Re: [PATCHv8 0/5] Driver for new "VMD" device Keith Busch <keith.busch@intel.com> - 2016-01-19 17:40 +0100
                Re: [PATCHv8 0/5] Driver for new "VMD" device "Veal, Bryan E." <bryan.e.veal@intel.com> - 2016-01-19 23:10 +0100
              Re: [PATCHv8 0/5] Driver for new "VMD" device Bjorn Helgaas <helgaas@kernel.org> - 2016-01-20 21:50 +0100
                Re: [PATCHv8 0/5] Driver for new "VMD" device Christoph Hellwig <hch@infradead.org> - 2016-01-26 17:50 +0100
                  Re: [PATCHv8 0/5] Driver for new "VMD" device "Veal, Bryan E." <bryan.e.veal@intel.com> - 2016-01-26 19:30 +0100
    Re: [PATCHv8 0/5] Driver for new "VMD" device Christoph Hellwig <hch@infradead.org> - 2016-01-17 19:00 +0100

Page 1 of 2  [1] 2  Next page →

#1307797 — [PATCHv8 0/5] Driver for new "VMD" device

Nothing changed; just contained in a single series, as requested.

Keith Busch (4):
  x86/IRQ: Export IRQ domain function for module use
  x86/PCI: Allow PCI domain specific dma ops
  PCI/AER: Use 32 bit int type domains
  x86/PCI: Initial commit for new VMD device driver

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

 MAINTAINERS                       |   6 +
 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                | 699 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/pcie/aer/aer_inject.c |  16 +-
 kernel/irq/irqdomain.c            |   1 +
 kernel/irq/msi.c                  |   8 +-
 10 files changed, 787 insertions(+), 11 deletions(-)
 create mode 100644 arch/x86/pci/vmd.c

-- 
2.6.2.307.g37023ba

[toc] | [next] | [standalone]

#1307801 — [PATCHv8 1/5] msi: Relax msi_domain_alloc() to support parentless MSI irqdomains

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 15b249e..38e89ce 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

[toc] | [prev] | [next] | [standalone]

#1307803 — [PATCHv8 4/5] PCI/AER: Use 32 bit int type domains

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..20db790 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;
+	u32 domain;
 };
 
 struct aer_error {
 	struct list_head list;
-	u16 domain;
+	u32 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, u32 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(u32 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

[toc] | [prev] | [next] | [standalone]

#1307804 — [PATCHv8 5/5] x86/PCI: Initial commit for new VMD device driver

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>
---
 MAINTAINERS                   |   6 +
 arch/x86/Kconfig              |  13 +
 arch/x86/include/asm/hw_irq.h |   5 +
 arch/x86/pci/Makefile         |   2 +
 arch/x86/pci/vmd.c            | 699 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 725 insertions(+)
 create mode 100644 arch/x86/pci/vmd.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 020a3be..d86a23c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8207,6 +8207,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/host-generic-pci.txt
 F:	drivers/pci/host/pci-host-generic.c
 
+PCI DRIVER FOR VMD
+M:	Keith Busch <keith.busch@intel.com>
+L:	linux-pci@vger.kernel.org
+S:	Supported
+F:	arch/x86/pci/vmd.c
+
 PCIE DRIVER FOR ST SPEAR13XX
 M:	Pratyush Anand <pratyush.anand@gmail.com>
 L:	linux-pci@vger.kernel.org
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 258965d..8cbcd64 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2684,6 +2684,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..1815b73 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -130,6 +130,11 @@ struct irq_alloc_info {
 			char		*uv_name;
 		};
 #endif
+#if IS_ENABLED(CONFIG_VMD)
+		struct {
+			struct msi_desc *desc;
+		};
+#endif
 	};
 };
 
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..3d6c23e
--- /dev/null
+++ b/arch/x86/pci/vmd.c
@@ -0,0 +1,699 @@
+/*
+ * 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;
+	struct vmd_dev		*vmd;
+	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 *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 struct dma_map_ops *vmd_dma_ops(struct device *dev)
+{
+	return to_vmd_dev(dev)->archdata.dma_ops;
+}
+
+static void *vmd_alloc(struct device *dev, size_t size, dma_addr_t *addr,
+				gfp_t flag, struct dma_attrs *attrs)
+{
+	return vmd_dma_ops(dev)->alloc(to_vmd_dev(dev), size, addr, flag,
+								attrs);
+}
+
+static void vmd_free(struct device *dev, size_t size, void *vaddr,
+				dma_addr_t addr, struct dma_attrs *attrs)
+{
+	return vmd_dma_ops(dev)->free(to_vmd_dev(dev), 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)
+{
+	return vmd_dma_ops(dev)->mmap(to_vmd_dev(dev), 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)
+{
+	return vmd_dma_ops(dev)->get_sgtable(to_vmd_dev(dev), 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)
+{
+	return vmd_dma_ops(dev)->map_page(to_vmd_dev(dev), 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)
+{
+	vmd_dma_ops(dev)->unmap_page(to_vmd_dev(dev), 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)
+{
+	return vmd_dma_ops(dev)->map_sg(to_vmd_dev(dev), 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)
+{
+	vmd_dma_ops(dev)->unmap_sg(to_vmd_dev(dev), 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)
+{
+	vmd_dma_ops(dev)->sync_single_for_cpu(to_vmd_dev(dev), 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)
+{
+	vmd_dma_ops(dev)->sync_single_for_device(to_vmd_dev(dev), addr, size,
+									dir);
+}
+
+static void vmd_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
+				int nents, enum dma_data_direction dir)
+{
+	vmd_dma_ops(dev)->sync_sg_for_cpu(to_vmd_dev(dev), sg, nents, dir);
+}
+
+static void vmd_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
+				int nents, enum dma_data_direction dir)
+{
+	vmd_dma_ops(dev)->sync_sg_for_device(to_vmd_dev(dev), sg, nents, dir);
+}
+
+static int vmd_mapping_error(struct device *dev, dma_addr_t addr)
+{
+	return vmd_dma_ops(dev)->mapping_error(to_vmd_dev(dev), addr);
+}
+
+static int vmd_dma_supported(struct device *dev, u64 mask)
+{
+	return vmd_dma_ops(dev)->dma_supported(to_vmd_dev(dev), mask);
+}
+
+#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
+static u64 vmd_get_required_mask(struct device *dev)
+{
+	return vmd_dma_ops(dev)->get_required_mask(to_vmd_dev(dev));
+}
+#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)	\
+	do {					\
+		if (source->fn)			\
+			dest->fn = vmd_##fn;	\
+	} while (0)
+
+static void vmd_setup_dma_ops(struct vmd_dev *vmd)
+{
+	const struct dma_map_ops *source = vmd->dev->dev.archdata.dma_ops;
+	struct dma_map_ops *dest = &vmd->dma_ops;
+	struct dma_domain *domain = &vmd->dma_domain;
+
+	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 non-posted config writes to posted memory writes. 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;
+	struct resource *res;
+	LIST_HEAD(resources);
+
+	res = &vmd->resources[0];
+	res->name = "VMD CFGBAR";
+	res->start = 0;
+	res->end = (resource_size(&vmd->dev->resource[0]) >> 20) - 1;
+	res->flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED;
+
+	res = &vmd->resources[1];
+	res->name = "VMD MEMBAR1";
+	res->start = vmd->dev->resource[2].start;
+	res->end = vmd->dev->resource[2].end;
+	res->flags = (vmd->dev->resource[2].flags & ~IORESOURCE_SIZEALIGN) &
+				(!upper_32_bits(vmd->dev->resource[2].end) ?
+						~IORESOURCE_MEM_64 : ~0);
+
+	res = &vmd->resources[2];
+	res->name = "VMD MEMBAR2";
+	res->start = vmd->dev->resource[4].start + 0x2000;
+	res->end = vmd->dev->resource[4].end;
+	res->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(&vmd->dev->dev, 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;
+	dev_info(&vmd->dev->dev, "Bound to PCI domain:%x\n",
+					vmd->sysdata.domain);
+	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.6");
-- 
2.6.2.307.g37023ba

[toc] | [prev] | [next] | [standalone]

#1307805 — [PATCHv8 2/5] x86/IRQ: Export IRQ domain function for module use

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 kernel/irq/irqdomain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8cf95de..6e655f7 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1061,6 +1061,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

[toc] | [prev] | [next] | [standalone]

#1307808 — [PATCHv8 3/5] x86/PCI: Allow PCI domain specific dma ops

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

[toc] | [prev] | [next] | [standalone]

#1310354

Hi Keith,

On Tue, Jan 12, 2016 at 01:18:05PM -0700, Keith Busch wrote:
> Nothing changed; just contained in a single series, as requested.
> 
> Keith Busch (4):
>   x86/IRQ: Export IRQ domain function for module use
>   x86/PCI: Allow PCI domain specific dma ops
>   PCI/AER: Use 32 bit int type domains
>   x86/PCI: Initial commit for new VMD device driver
> 
> Liu Jiang (1):
>   msi: Relax msi_domain_alloc() to support parentless MSI irqdomains
> 
>  MAINTAINERS                       |   6 +
>  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                | 699 ++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pcie/aer/aer_inject.c |  16 +-
>  kernel/irq/irqdomain.c            |   1 +
>  kernel/irq/msi.c                  |   8 +-
>  10 files changed, 787 insertions(+), 11 deletions(-)
>  create mode 100644 arch/x86/pci/vmd.c

I applied these to pci/host-vmd with the changes below.  Most of them
are cosmetic (rewrapping changelogs, fixing whitespace, etc.), but
there are a few I'd like you to take a close look at:

  - Added VMD_CFGBAR and similar #defines
  - Added vmd_cfg_addr() to factor out the addr computation and
    validation
  - Resource setup in vmd_enable_domain().  I suggested a temporary to
    make the lines shorter.  I had the vmd->dev->resource[n] in mind,
    but you added a temporary for vmd->resources[n].  Either way is
    fine, but I liked the look of the v7 init, so I reverted to that,
    with a temporary for vmd->dev->resource[n].
  - Flags setup in vmd_enable_domain().  This was pretty confusing,
    and I *think* what I did is equivalent, but you should verify.

I also have a more substantive question about the flags setup.  I
think you should not clear IORESOURCE_MEM_64.  The intent of
IORESOURCE_MEM_64 is to describe the *capability* of a BAR, not its
contents.  But I assume you cleared it for a reason.  vmd->resources[n]
are not BARs, so the PCI core won't assign resources to them like it
does for BAR, so we shouldn't care about IORESOURCE_MEM_64 for that
reason.  Is there some other reason IORESOURCE_MEM_64 makes a
difference there?

I'm still hoping to get this in during the merge window.

If you want to test this, I recommend using my git branch
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-vmd
instead of applying the patch below on top of your v8.  If you want
to make changes, post an incremental patch based on that branch.

Bjorn


@@ -1,114 +1,123 @@
-commit 7b1cd49008b8
+commit a0fea74a2b8f
 Author: Keith Busch <keith.busch@intel.com>
 Date:   Tue Jan 12 13:18:10 2016 -0700
 
-    x86/PCI: Initial commit for new VMD device driver
+    x86/PCI: Add driver for Intel Volume Management Device (VMD)
     
-    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.
+    The Intel Volume Management Device (VMD) is a Root Complex Integrated
+    Endpoint 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 PCIe domains
+    allow more than 256 buses in a system by letting bus numbers 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.
+    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 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
+       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
+       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 an 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 as
+       if they were issued using one of VMD's MSI-X table entries.  Each MSI
+       and MSI-X address of VMD-owned devices and ports has a special format
+       where the address refers to specific entries in the 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.
+       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
+       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.
     
+    [bhelgaas: add VMD BAR #defines, factor out vmd_cfg_addr(), whitespace]
     Signed-off-by: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit 05e97692e217
+commit 60707d6ebbbe
 Author: Keith Busch <keith.busch@intel.com>
 Date:   Tue Jan 12 13:18:09 2016 -0700
 
-    PCI/AER: Use 32 bit int type domains
+    PCI/AER: Use 32 bit PCI domain numbers
     
-    New pci device provides additional pci domains that start above what 16
-    bits can address.
+    The Intel Volume Management Device (VMD) supports 32-bit domain numbers.
+    To accommodate this, use u32 instead of u16 to store domain numbers.
     
+    [bhelgaas: changelog]
     Signed-off-by: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit 323121f05c8d
+commit cbab4ba90c61
 Author: Keith Busch <keith.busch@intel.com>
 Date:   Tue Jan 12 13:18:08 2016 -0700
 
-    x86/PCI: Allow PCI domain specific dma ops
+    x86/PCI: Allow DMA ops specific to a PCI domain
     
-    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 discovered in that domain to use them.
+    The Intel Volume Management Device (VMD) is a PCIe endpoint that acts as a
+    host bridge to another PCI domain.  When devices below the VMD perform DMA,
+    the VMD replaces their DMA source IDs with its own source ID.  Therefore,
+    those devices require special DMA ops.
     
+    Add interfaces to allow the VMD driver to set up dma_ops for the devices
+    below it.
+    
+    [bhelgaas: remove "extern", add "static", changelog]
     Signed-off-by: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit b54bc8bc78d0
+commit db6a7b7a3887
 Author: Keith Busch <keith.busch@intel.com>
 Date:   Tue Jan 12 13:18:07 2016 -0700
 
-    x86/IRQ: Export IRQ domain function for module use
+    irqdomain: Export irq_domain_set_info() for module use
+    
+    Export irq_domain_set_info() for module use.  It will be used by the Volume
+    Management Device driver.
     
+    [bhelgaas: changelog]
     Signed-off-by: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit 7c8c9dc397f2
+commit 23e42d853208
 Author: Liu Jiang <jiang.liu@linux.intel.com>
 Date:   Tue Jan 12 13:18:06 2016 -0700
 
-    msi: Relax msi_domain_alloc() to support parentless MSI irqdomains
+    genirq/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
+    Previously msi_domain_alloc() assumed MSI irqdomains always had parent
+    irqdomains, but that's not true for the new Intel VMD devices.  Relax
     msi_domain_alloc() to support parentless MSI irqdomains.
     
     Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>


diff --git a/MAINTAINERS b/MAINTAINERS
index 76369d4..ce47e08 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8216,7 +8216,7 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/host-generic-pci.txt
 F:	drivers/pci/host/pci-host-generic.c
 
-PCI DRIVER FOR VMD
+PCI DRIVER FOR INTEL VOLUME MANAGEMENT DEVICE (VMD)
 M:	Keith Busch <keith.busch@intel.com>
 L:	linux-pci@vger.kernel.org
 S:	Supported
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9a5ab69..3e6aca8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2670,13 +2670,13 @@ config VMD
 	tristate "Volume Management Device Driver"
 	default N
 	---help---
-	  Adds support for the Intel Volume Manage Device (VMD). VMD is a
+	  Adds support for the Intel Volume Management 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
+	  more 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.
+	  has devices attached to it, say Y; if you are not sure, say N.
 
 source "net/Kconfig"
 
diff --git a/arch/x86/include/asm/device.h b/arch/x86/include/asm/device.h
index 3b23897..684ed6c 100644
--- a/arch/x86/include/asm/device.h
+++ b/arch/x86/include/asm/device.h
@@ -16,8 +16,8 @@ struct dma_domain {
 	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);
+void add_dma_domain(struct dma_domain *domain);
+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 106fd13..2879efc 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -642,8 +642,8 @@ unsigned int pcibios_assign_all_busses(void)
 }
 
 #if defined(CONFIG_X86_DEV_DMA_OPS) && defined(CONFIG_PCI_DOMAINS)
-LIST_HEAD(dma_domain_list);
-DEFINE_SPINLOCK(dma_domain_list_lock);
+static LIST_HEAD(dma_domain_list);
+static DEFINE_SPINLOCK(dma_domain_list_lock);
 
 void add_dma_domain(struct dma_domain *domain)
 {
diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
index 56ef447..d57e480 100644
--- a/arch/x86/pci/vmd.c
+++ b/arch/x86/pci/vmd.c
@@ -27,20 +27,24 @@
 #include <asm/msi.h>
 #include <asm/msidef.h>
 
+#define VMD_CFGBAR	0
+#define VMD_MEMBAR1	2
+#define VMD_MEMBAR2	4
+
 /*
- * Lock for manipulating vmd irq lists.
+ * 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
+ * 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.
+ * @rcu:	RCU callback item for freeing.
  * @irq:	back pointer to parent.
- * @virq:	the virtual irq value provided to the requesting driver.
+ * @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.
+ * 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;
@@ -50,11 +54,11 @@ struct vmd_irq {
 };
 
 /**
- * struct vmd_irq_list - list of driver requested irq's mapping to a vmd vector
+ * struct vmd_irq_list - list of driver requested IRQs 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
+ * @vmd_vector:	the h/w IRQ assigned to the VMD.
+ * @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 {
@@ -92,11 +96,11 @@ static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)
 }
 
 /*
- * Drivers managing a device in a VMD domain allocate their own irqs as before,
+ * 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
+ * destination ID for the VMD MSI-X table.  The VMD 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)
@@ -110,7 +114,7 @@ static void vmd_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 }
 
 /*
- * We rely on MSI_FLAG_USE_DEF_CHIP_OPS to set the irq mask/unmask ops.
+ * 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)
 {
@@ -139,7 +143,7 @@ static void vmd_irq_disable(struct irq_data *data)
  * other devices sharing the same vector.
  */
 static int vmd_irq_set_affinity(struct irq_data *data,
-			const struct cpumask *dest, bool force)
+				const struct cpumask *dest, bool force)
 {
 	return -EINVAL;
 }
@@ -159,7 +163,7 @@ static irq_hw_number_t vmd_get_hwirq(struct msi_domain_info *info,
 }
 
 /*
- * XXX: We can be even smarter selecting the best irq once we solve the
+ * 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)
@@ -176,8 +180,7 @@ static struct vmd_irq_list *vmd_next_irq(struct vmd_dev *vmd)
 	return &vmd->irqs[best];
 }
 
-static int vmd_msi_init(struct irq_domain *domain,
-			struct msi_domain_info *info,
+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)
 {
@@ -191,14 +194,13 @@ static int vmd_msi_init(struct irq_domain *domain,
 	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);
+	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 msi_domain_info *info, unsigned int virq)
 {
 	struct vmd_irq *vmdirq = irq_get_chip_data(virq);
 
@@ -210,15 +212,15 @@ static void vmd_msi_free(struct irq_domain *domain,
 	kfree_rcu(vmdirq, rcu);
 }
 
-static int vmd_msi_prepare(struct irq_domain *domain,
-			struct device *dev, int nvec,
-			msi_alloc_info_t *arg)
+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;
 }
@@ -245,8 +247,8 @@ static struct msi_domain_info vmd_msi_domain_info = {
 
 #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
+ * VMD replaces the requester ID with its own.  DMA mappings for devices in a
+ * VMD domain need to be mapped for the VMD, not the device requiring
  * the mapping.
  */
 static struct device *to_vmd_dev(struct device *dev)
@@ -263,76 +265,73 @@ static struct dma_map_ops *vmd_dma_ops(struct device *dev)
 }
 
 static void *vmd_alloc(struct device *dev, size_t size, dma_addr_t *addr,
-				gfp_t flag, struct dma_attrs *attrs)
+		       gfp_t flag, struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->alloc(to_vmd_dev(dev), size, addr, flag,
-								attrs);
+				       attrs);
 }
 
 static void vmd_free(struct device *dev, size_t size, void *vaddr,
-				dma_addr_t addr, struct dma_attrs *attrs)
+		     dma_addr_t addr, struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->free(to_vmd_dev(dev), size, vaddr, addr,
-								attrs);
+				      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)
+		    void *cpu_addr, dma_addr_t addr, size_t size,
+		    struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->mmap(to_vmd_dev(dev), vma, cpu_addr, addr,
-								size, attrs);
+				      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)
+			   void *cpu_addr, dma_addr_t addr, size_t size,
+			   struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->get_sgtable(to_vmd_dev(dev), sgt, cpu_addr,
-							addr, size, attrs);
+					     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)
+			       unsigned long offset, size_t size,
+			       enum dma_data_direction dir,
+			       struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->map_page(to_vmd_dev(dev), page, offset, size,
-								dir, attrs);
+					  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)
+			   enum dma_data_direction dir, struct dma_attrs *attrs)
 {
 	vmd_dma_ops(dev)->unmap_page(to_vmd_dev(dev), 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)
+		      enum dma_data_direction dir, struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->map_sg(to_vmd_dev(dev), 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)
+			 enum dma_data_direction dir, struct dma_attrs *attrs)
 {
 	vmd_dma_ops(dev)->unmap_sg(to_vmd_dev(dev), 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)
+				    size_t size, enum dma_data_direction dir)
 {
 	vmd_dma_ops(dev)->sync_single_for_cpu(to_vmd_dev(dev), 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)
+				       size_t size, enum dma_data_direction dir)
 {
 	vmd_dma_ops(dev)->sync_single_for_device(to_vmd_dev(dev), addr, size,
-									dir);
+						 dir);
 }
 
 static void vmd_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
@@ -342,7 +341,7 @@ static void vmd_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
 }
 
 static void vmd_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
-				int nents, enum dma_data_direction dir)
+				   int nents, enum dma_data_direction dir)
 {
 	vmd_dma_ops(dev)->sync_sg_for_device(to_vmd_dev(dev), sg, nents, dir);
 }
@@ -414,20 +413,32 @@ static void vmd_teardown_dma_ops(struct vmd_dev *vmd) {}
 static void vmd_setup_dma_ops(struct vmd_dev *vmd) {}
 #endif
 
+static char __iomem *vmd_cfg_addr(struct vmd_dev *vmd, struct pci_bus *bus,
+				  unsigned int devfn, int reg, int len)
+{
+	char __iomem *addr = vmd->cfgbar +
+			     (bus->number << 20) + (devfn << 12) + reg;
+
+	if ((addr - vmd->cfgbar) + len >=
+	    resource_size(&vmd->dev->resource[VMD_CFGBAR]))
+		return NULL;
+
+	return addr;
+}
+
 /*
  * 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 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;
+	char __iomem *addr = vmd_cfg_addr(vmd, bus, devfn, reg, len);
+	unsigned long flags;
+	int ret = 0;
 
-	if ((addr - vmd->cfgbar) + len >= resource_size(&vmd->dev->resource[0]))
+	if (!addr)
 		return -EFAULT;
 
 	spin_lock_irqsave(&vmd->cfg_lock, flags);
@@ -455,15 +466,14 @@ static int vmd_pci_read(struct pci_bus *bus, unsigned int devfn, int reg,
  * 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 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;
+	char __iomem *addr = vmd_cfg_addr(vmd, bus, devfn, reg, len);
+	unsigned long flags;
+	int ret = 0;
 
-	if ((addr - vmd->cfgbar) + len >= resource_size(&vmd->dev->resource[0]))
+	if (!addr)
 		return -EFAULT;
 
 	spin_lock_irqsave(&vmd->cfg_lock, flags);
@@ -494,7 +504,7 @@ static struct pci_ops vmd_ops = {
 };
 
 /*
- * VMD domains start at 10000h to not clash with domains defining ACPI _SEG.
+ * VMD domains start at 0x1000 to not clash with ACPI _SEG domains.
  */
 static int vmd_find_free_domain(void)
 {
@@ -510,37 +520,50 @@ static int vmd_enable_domain(struct vmd_dev *vmd)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
 	struct resource *res;
+	u32 upper_bits;
+	unsigned long flags;
 	LIST_HEAD(resources);
 
-	res = &vmd->resources[0];
-	res->name = "VMD CFGBAR";
-	res->start = 0;
-	res->end = (resource_size(&vmd->dev->resource[0]) >> 20) - 1;
-	res->flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED;
-
-	res = &vmd->resources[1];
-	res->name = "VMD MEMBAR1";
-	res->start = vmd->dev->resource[2].start;
-	res->end = vmd->dev->resource[2].end;
-	res->flags = (vmd->dev->resource[2].flags & ~IORESOURCE_SIZEALIGN) &
-				(!upper_32_bits(vmd->dev->resource[2].end) ?
-						~IORESOURCE_MEM_64 : ~0);
-
-	res = &vmd->resources[2];
-	res->name = "VMD MEMBAR2";
-	res->start = vmd->dev->resource[4].start + 0x2000;
-	res->end = vmd->dev->resource[4].end;
-	res->flags = (vmd->dev->resource[4].flags & ~IORESOURCE_SIZEALIGN) &
-				(!upper_32_bits(vmd->dev->resource[4].end) ?
-						~IORESOURCE_MEM_64 : ~0);
+	res = &vmd->dev->resource[VMD_CFGBAR];
+	vmd->resources[0] = (struct resource) {
+		.name  = "VMD CFGBAR",
+		.start = res->start,
+		.end   = (resource_size(res) >> 20) - 1,
+		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
+	};
+
+	res = &vmd->dev->resource[VMD_MEMBAR1];
+	upper_bits = upper_32_bits(res->end);
+	flags = res->flags & ~IORESOURCE_SIZEALIGN;
+	if (!upper_bits)
+		flags &= ~IORESOURCE_MEM_64;
+	vmd->resources[1] = (struct resource) {
+		.name  = "VMD MEMBAR1",
+		.start = res->start,
+		.end   = res->end,
+		.flags = flags,
+	};
+
+	res = &vmd->dev->resource[VMD_MEMBAR2];
+	upper_bits = upper_32_bits(res->end);
+	flags = res->flags & ~IORESOURCE_SIZEALIGN;
+	if (!upper_bits)
+		flags &= ~IORESOURCE_MEM_64;
+	vmd->resources[2] = (struct resource) {
+		.name  = "VMD MEMBAR2",
+		.start = res->start + 0x2000,
+		.end   = res->end,
+		.flags = flags,
+	};
 
 	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);
+						    NULL);
 	if (!vmd->irq_domain)
 		return -ENODEV;
 
@@ -548,7 +571,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd)
 	pci_add_resource(&resources, &vmd->resources[1]);
 	pci_add_resource(&resources, &vmd->resources[2]);
 	vmd->bus = pci_create_root_bus(&vmd->dev->dev, 0, &vmd_ops, sd,
-								&resources);
+				       &resources);
 	if (!vmd->bus) {
 		pci_free_resource_list(&resources);
 		irq_domain_remove(vmd->irq_domain);
@@ -560,8 +583,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd)
 	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");
+			       "domain"), "Can't create symlink to domain\n");
 	return 0;
 }
 
@@ -583,7 +605,7 @@ 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))
+	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20))
 		return -ENOMEM;
 
 	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
@@ -595,7 +617,7 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (err < 0)
 		return err;
 
-	vmd->cfgbar = pcim_iomap(dev, 0, 0);
+	vmd->cfgbar = pcim_iomap(dev, VMD_CFGBAR, 0);
 	if (!vmd->cfgbar)
 		return -ENOMEM;
 
@@ -609,19 +631,20 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 
 	vmd->irqs = devm_kcalloc(&dev->dev, vmd->msix_count, sizeof(*vmd->irqs),
-							GFP_KERNEL);
+				 GFP_KERNEL);
 	if (!vmd->irqs)
 		return -ENOMEM;
 
 	vmd->msix_entries = devm_kcalloc(&dev->dev, vmd->msix_count,
-					sizeof(*vmd->msix_entries), GFP_KERNEL);
+					 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);
+						vmd->msix_count);
 	if (vmd->msix_count < 0)
 		return vmd->msix_count;
 
@@ -631,7 +654,7 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		vmd->irqs[i].index = i;
 
 		err = devm_request_irq(&dev->dev, vmd->irqs[i].vmd_vector,
-				vmd_irq, 0, "vmd", &vmd->irqs[i]);
+				       vmd_irq, 0, "vmd", &vmd->irqs[i]);
 		if (err)
 			return err;
 	}
@@ -641,8 +664,9 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	err = vmd_enable_domain(vmd);
 	if (err)
 		return err;
-	dev_info(&vmd->dev->dev, "Bound to PCI domain:%x\n",
-					vmd->sysdata.domain);
+
+	dev_info(&vmd->dev->dev, "Bound to PCI domain %04x\n",
+		 vmd->sysdata.domain);
 	return 0;
 }
 

[toc] | [prev] | [next] | [standalone]

#1310403

On Fri, Jan 15, 2016 at 12:19:38PM -0600, Bjorn Helgaas wrote:
> I also have a more substantive question about the flags setup.  I
> think you should not clear IORESOURCE_MEM_64.  The intent of
> IORESOURCE_MEM_64 is to describe the *capability* of a BAR, not its
> contents.  But I assume you cleared it for a reason.  vmd->resources[n]
> are not BARs, so the PCI core won't assign resources to them like it
> does for BAR, so we shouldn't care about IORESOURCE_MEM_64 for that
> reason.  Is there some other reason IORESOURCE_MEM_64 makes a
> difference there?

Hi Bjorn & Keith:
  
I did this to fix an issue in pre-RFC code.

The flag is subtly restrictive in one specific scenario: spec-compliant
PCIe ports lack the ability to specify a 64-bit, non-prefetchable range.
IORESOURCE_MEM_64 directs the PCI subsystem to put the address into the
64-bit *prefetchable* range. Below the port, the "prefetchable" propoerty
*is* restrictive: the addresses can't be used for non-prefetchable BARs.

Thus, in the specific case where a 64-bit non-prefetchable VMD bar happens
to contain a 32-bit address, removing the IORESOURCE_MEM_64 flag allows
the address resource to be used for *any* non-prefetchable BARs (32-bit or
64-bit) downstream.  

[toc] | [prev] | [next] | [standalone]

#1310483

On Fri, Jan 15, 2016 at 11:31:03AM -0800, Veal, Bryan E. wrote:
> On Fri, Jan 15, 2016 at 12:19:38PM -0600, Bjorn Helgaas wrote:
> > I also have a more substantive question about the flags setup.  I
> > think you should not clear IORESOURCE_MEM_64.  The intent of
> > IORESOURCE_MEM_64 is to describe the *capability* of a BAR, not its
> > contents.  But I assume you cleared it for a reason.  vmd->resources[n]
> > are not BARs, so the PCI core won't assign resources to them like it
> > does for BAR, so we shouldn't care about IORESOURCE_MEM_64 for that
> > reason.  Is there some other reason IORESOURCE_MEM_64 makes a
> > difference there?
> 
> I did this to fix an issue in pre-RFC code.

Even though you found this issue before posting the RFC code, I assume
the issue is still relevant in the current code, and you still want to
clear IORESOURCE_MEM_64, right?

> The flag is subtly restrictive in one specific scenario: spec-compliant
> PCIe ports lack the ability to specify a 64-bit, non-prefetchable range.

Right; I think this is just a consequence of PCIe ports being PCI
bridges, and bridges having:

  - optional 32-bit I/O window
  - required 32-bit non-prefetchable memory window
  - optional prefetchable memory window (either 32-bit or 64-bit)

If we have a device with a 64-bit non-prefetchable BAR, we can assign
a 64-bit address if the device is on a root bus and the host bridge
has a 64-bit non-prefetchable window.  Otherwise, the device is below
a P2P bridge and we have to assign space from the 32-bit
non-prefetchable window.

So far this is all standard PCI stuff, not VMD- or even PCIe-specific.

> IORESOURCE_MEM_64 directs the PCI subsystem to put the address into the
> 64-bit *prefetchable* range. 

This is where I get confused.  IORESOURCE_MEM_64 *should* mean "the
hardware register associated with this resource can accommodate a
64-bit value."  If we're using IORESOURCE_MEM_64 to decide whether to
use a prefetchable vs. a non-prefetchable window, that sounds broken.

Can you point me to the relevant code, and maybe give an example?  I'm
pretty sure the code doesn't completely match the spec, and maybe this
is a case where we have to set the flags non-intuitively to get the
desired result.

> Below the port, the "prefetchable" propoerty
> *is* restrictive: the addresses can't be used for non-prefetchable BARs.
> 
> Thus, in the specific case where a 64-bit non-prefetchable VMD bar happens
> to contain a 32-bit address, removing the IORESOURCE_MEM_64 flag allows
> the address resource to be used for *any* non-prefetchable BARs (32-bit or
> 64-bit) downstream.  

If I understand correctly, these VMD BARs (VMD_MEMBAR1 and
VMD_MEMBAR2) effectively become the host bridge windows available for
devices below the VMD.

I infer that if the VMD host bridge window is non-prefetchable and has
IORESOURCE_MEM_64 set, we won't put a 32-bit non-prefetchable BAR in
that window.  That sounds like a bug, but let me be the first to admit
that I don't understand our PCI resource allocation code.

BTW, I forgot to ask about the 0x2000 offset applied to VMD_MEMBAR2.
Can we add a comment about what that offset is doing?

BTW2, is one of these (VMD_MEMBAR1 and VMD_MEMBAR2) prefetchable and
the other non-prefetchable?  If so, a comment would really help.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1311062

On Fri, Jan 15, 2016 at 03:49:02PM -0600, Bjorn Helgaas wrote:
> Even though you found this issue before posting the RFC code, I assume
> the issue is still relevant in the current code, and you still want to
> clear IORESOURCE_MEM_64, right?

Yes.

> This is where I get confused.  IORESOURCE_MEM_64 *should* mean "the
> hardware register associated with this resource can accommodate a
> 64-bit value."  If we're using IORESOURCE_MEM_64 to decide whether to
> use a prefetchable vs. a non-prefetchable window, that sounds broken.
> 
> Can you point me to the relevant code, and maybe give an example?  I'm
> pretty sure the code doesn't completely match the spec, and maybe this
> is a case where we have to set the flags non-intuitively to get the
> desired result.
> 
> > Below the port, the "prefetchable" propoerty
> > *is* restrictive: the addresses can't be used for non-prefetchable BARs.
> > 
> > Thus, in the specific case where a 64-bit non-prefetchable VMD bar happens
> > to contain a 32-bit address, removing the IORESOURCE_MEM_64 flag allows
> > the address resource to be used for *any* non-prefetchable BARs (32-bit or
> > 64-bit) downstream.  
> 
> If I understand correctly, these VMD BARs (VMD_MEMBAR1 and
> VMD_MEMBAR2) effectively become the host bridge windows available for
> devices below the VMD.
> 
> I infer that if the VMD host bridge window is non-prefetchable and has
> IORESOURCE_MEM_64 set, we won't put a 32-bit non-prefetchable BAR in
> that window.  That sounds like a bug, but let me be the first to admit
> that I don't understand our PCI resource allocation code.

I don't think anything is broken. You are correct that the MEMBARs are
used as a host bridge window. The reason to clear the flag is a side
effect of that.

For BARs, the flags describe capabilities. For resources, they are
interpreted as restrictions.

If VMD has a 32-bit resource in a 64-bit non-prefetchable BAR, without
clearing the flag, it yields a host bridge resource, and thus root bus
resource, with IORESOURCE_MEM_64 set.

Downstream of VMD, the root port's 32-bit non-prefetchable base/limit
registers can't handle the 64-bit resource, but the 64-bit prefetchable
window can, so that's where it ends up. (See pci_bus_alloc_resource().)

Downstream of the root port, the resource is now "upcast" to
IORESOURCE_PREFETCH, which can't be used in a non-prefetchable BAR.

> BTW, I forgot to ask about the 0x2000 offset applied to VMD_MEMBAR2.
> Can we add a comment about what that offset is doing?

I'll rely on Keith to add the comments. This range is reserved for VMD's
MSI-X table and PBA.

> BTW2, is one of these (VMD_MEMBAR1 and VMD_MEMBAR2) prefetchable and
> the other non-prefetchable?  If so, a comment would really help.

BIOS can configure the types pre-boot, but having one of each type is
the only real reason to need both BARs.

[toc] | [prev] | [next] | [standalone]

#1313624

On Sat, Jan 16, 2016 at 02:19:38PM -0800, Veal, Bryan E. wrote:
> On Fri, Jan 15, 2016 at 03:49:02PM -0600, Bjorn Helgaas wrote:
> > Even though you found this issue before posting the RFC code, I assume
> > the issue is still relevant in the current code, and you still want to
> > clear IORESOURCE_MEM_64, right?
> 
> Yes.
> 
> > This is where I get confused.  IORESOURCE_MEM_64 *should* mean "the
> > hardware register associated with this resource can accommodate a
> > 64-bit value."  If we're using IORESOURCE_MEM_64 to decide whether to
> > use a prefetchable vs. a non-prefetchable window, that sounds broken.
> > 
> > Can you point me to the relevant code, and maybe give an example?  I'm
> > pretty sure the code doesn't completely match the spec, and maybe this
> > is a case where we have to set the flags non-intuitively to get the
> > desired result.
> > 
> > > Below the port, the "prefetchable" propoerty
> > > *is* restrictive: the addresses can't be used for non-prefetchable BARs.
> > > 
> > > Thus, in the specific case where a 64-bit non-prefetchable VMD bar happens
> > > to contain a 32-bit address, removing the IORESOURCE_MEM_64 flag allows
> > > the address resource to be used for *any* non-prefetchable BARs (32-bit or
> > > 64-bit) downstream.  
> > 
> > If I understand correctly, these VMD BARs (VMD_MEMBAR1 and
> > VMD_MEMBAR2) effectively become the host bridge windows available for
> > devices below the VMD.
> > 
> > I infer that if the VMD host bridge window is non-prefetchable and has
> > IORESOURCE_MEM_64 set, we won't put a 32-bit non-prefetchable BAR in
> > that window.  That sounds like a bug, but let me be the first to admit
> > that I don't understand our PCI resource allocation code.
> 
> I don't think anything is broken. You are correct that the MEMBARs are
> used as a host bridge window. The reason to clear the flag is a side
> effect of that.
> 
> For BARs, the flags describe capabilities. For resources, they are
> interpreted as restrictions.
> 
> If VMD has a 32-bit resource in a 64-bit non-prefetchable BAR, without
> clearing the flag, it yields a host bridge resource, and thus root bus
> resource, with IORESOURCE_MEM_64 set.
> 
> Downstream of VMD, the root port's 32-bit non-prefetchable base/limit
> registers can't handle the 64-bit resource, but the 64-bit prefetchable
> window can, so that's where it ends up. (See pci_bus_alloc_resource().)

OK, I think I finally found the critical comment, which is in
__pci_assign_resource():

  Even if a 64-bit prefetchable bridge window is below 4GB, we can't
  put a 32-bit prefetchable resource in it because pbus_size_mem()
  assumes a 64-bit window will contain no 32-bit resources.  If we
  assign things differently than they were sized, not everything will
  fit.

There's no reason we can't put a Root Port's 32-bit non-prefetchable
window inside a 64-bit VMD window that happens to be below 4GB,
*except* for the fact that pbus_size_mem() assumes we won't do that.

The VMD code needs a reference to that comment.

I guess you're relying on BIOS to assign your non-prefetchable VMD BAR
below 4GB even though it's a 64-bit BAR?  If Linux assigned that BAR,
e.g., after a hot-add of a VMD, we might put it above 4GB, and then
Root Ports downstream from the VMD would not be able to use any
non-prefetchable space.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1310405

On Fri, Jan 15, 2016 at 12:19:38PM -0600, Bjorn Helgaas wrote:
> I applied these to pci/host-vmd with the changes below.  Most of them
> are cosmetic (rewrapping changelogs, fixing whitespace, etc.), but
> there are a few I'd like you to take a close look at:
> 
>   - Added VMD_CFGBAR and similar #defines
>   - Added vmd_cfg_addr() to factor out the addr computation and
>     validation
>   - Resource setup in vmd_enable_domain().  I suggested a temporary to
>     make the lines shorter.  I had the vmd->dev->resource[n] in mind,
>     but you added a temporary for vmd->resources[n].  Either way is
>     fine, but I liked the look of the v7 init, so I reverted to that,
>     with a temporary for vmd->dev->resource[n].
>   - Flags setup in vmd_enable_domain().  This was pretty confusing,
>     and I *think* what I did is equivalent, but you should verify.

Thanks for the cleanups. All the new changes look good to me, and I will
test your tree today to confirm no regressions.
 
> I'm still hoping to get this in during the merge window.

Thanks a bunch. This would be great timing to align the hardware
availability with various software and OEM vendors.
 
> If you want to test this, I recommend using my git branch
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-vmd
> instead of applying the patch below on top of your v8.  If you want
> to make changes, post an incremental patch based on that branch.

I'll give this a test today.

[toc] | [prev] | [next] | [standalone]

#1310408

On Fri, 15 Jan 2016, Bjorn Helgaas wrote:
> +    [bhelgaas: add VMD BAR #defines, factor out vmd_cfg_addr(), whitespace]
>      Signed-off-by: Keith Busch <keith.busch@intel.com>
>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Just for the record. You forgot my Acked-bys
 
Other than that the changes look good!

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1310436

On Fri, Jan 15, 2016 at 11:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 15 Jan 2016, Bjorn Helgaas wrote:
>> +    [bhelgaas: add VMD BAR #defines, factor out vmd_cfg_addr(), whitespace]
>>      Signed-off-by: Keith Busch <keith.busch@intel.com>
>>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Just for the record. You forgot my Acked-bys

Sorry, I added them to the first two patches:

  irqdomain: Export irq_domain_set_info() for module use
  genirq/MSI: Relax msi_domain_alloc() to support parentless MSI irqdomains

Let me know if you intended to ack any of the others, too.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1310439

On Fri, 15 Jan 2016, Bjorn Helgaas wrote:

> On Fri, Jan 15, 2016 at 11:44 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Fri, 15 Jan 2016, Bjorn Helgaas wrote:
> >> +    [bhelgaas: add VMD BAR #defines, factor out vmd_cfg_addr(), whitespace]
> >>      Signed-off-by: Keith Busch <keith.busch@intel.com>
> >>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Just for the record. You forgot my Acked-bys
> 
> Sorry, I added them to the first two patches:
> 
>   irqdomain: Export irq_domain_set_info() for module use
>   genirq/MSI: Relax msi_domain_alloc() to support parentless MSI irqdomains
> 
> Let me know if you intended to ack any of the others, too.

The vmd driver itself (at least for the irq related parts). But you don't have
to redo it.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1310417

Hi folks,

The VMD driver can be built as a module, so the following symbols need to be exported:
pci_msi_create_irq_domain
msi_desc_to_pci_dev
I'll post a follow-up patch shortly which applies to pci/host-vmd

Additionally, what are your thoughts of moving the menuconfig option from the root-tier to 'Processor Type and Features'?

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org] 
Sent: Friday, January 15, 2016 11:20 AM
To: Busch, Keith <keith.busch@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>; x86@kernel.org; linux-pci@vger.kernel.org; Thomas Gleixner <tglx@linutronix.de>; Bjorn Helgaas <bhelgaas@google.com>; Williams, Dan J <dan.j.williams@intel.com>; Veal, Bryan E <bryan.e.veal@intel.com>; Derrick, Jonathan <jonathan.derrick@intel.com>
Subject: Re: [PATCHv8 0/5] Driver for new "VMD" device

Hi Keith,

On Tue, Jan 12, 2016 at 01:18:05PM -0700, Keith Busch wrote:
> Nothing changed; just contained in a single series, as requested.
> 
> Keith Busch (4):
>   x86/IRQ: Export IRQ domain function for module use
>   x86/PCI: Allow PCI domain specific dma ops
>   PCI/AER: Use 32 bit int type domains
>   x86/PCI: Initial commit for new VMD device driver
> 
> Liu Jiang (1):
>   msi: Relax msi_domain_alloc() to support parentless MSI irqdomains
> 
>  MAINTAINERS                       |   6 +
>  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                | 699 ++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pcie/aer/aer_inject.c |  16 +-
>  kernel/irq/irqdomain.c            |   1 +
>  kernel/irq/msi.c                  |   8 +-
>  10 files changed, 787 insertions(+), 11 deletions(-)  create mode 
> 100644 arch/x86/pci/vmd.c

I applied these to pci/host-vmd with the changes below.  Most of them are cosmetic (rewrapping changelogs, fixing whitespace, etc.), but there are a few I'd like you to take a close look at:

  - Added VMD_CFGBAR and similar #defines
  - Added vmd_cfg_addr() to factor out the addr computation and
    validation
  - Resource setup in vmd_enable_domain().  I suggested a temporary to
    make the lines shorter.  I had the vmd->dev->resource[n] in mind,
    but you added a temporary for vmd->resources[n].  Either way is
    fine, but I liked the look of the v7 init, so I reverted to that,
    with a temporary for vmd->dev->resource[n].
  - Flags setup in vmd_enable_domain().  This was pretty confusing,
    and I *think* what I did is equivalent, but you should verify.

I also have a more substantive question about the flags setup.  I think you should not clear IORESOURCE_MEM_64.  The intent of
IORESOURCE_MEM_64 is to describe the *capability* of a BAR, not its contents.  But I assume you cleared it for a reason.  vmd->resources[n] are not BARs, so the PCI core won't assign resources to them like it does for BAR, so we shouldn't care about IORESOURCE_MEM_64 for that reason.  Is there some other reason IORESOURCE_MEM_64 makes a difference there?

I'm still hoping to get this in during the merge window.

If you want to test this, I recommend using my git branch https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-vmd
instead of applying the patch below on top of your v8.  If you want to make changes, post an incremental patch based on that branch.

Bjorn


@@ -1,114 +1,123 @@
-commit 7b1cd49008b8
+commit a0fea74a2b8f
 Author: Keith Busch <keith.busch@intel.com>
 Date:   Tue Jan 12 13:18:10 2016 -0700
 
-    x86/PCI: Initial commit for new VMD device driver
+    x86/PCI: Add driver for Intel Volume Management Device (VMD)
     
-    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.
+    The Intel Volume Management Device (VMD) is a Root Complex Integrated
+    Endpoint 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 PCIe domains
+    allow more than 256 buses in a system by letting bus numbers 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.
+    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 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
+       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
+       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 an 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 as
+       if they were issued using one of VMD's MSI-X table entries.  Each MSI
+       and MSI-X address of VMD-owned devices and ports has a special format
+       where the address refers to specific entries in the 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.
+       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
+       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.
     
+    [bhelgaas: add VMD BAR #defines, factor out vmd_cfg_addr(), 
+ whitespace]
     Signed-off-by: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit 05e97692e217
+commit 60707d6ebbbe
 Author: Keith Busch <keith.busch@intel.com>
 Date:   Tue Jan 12 13:18:09 2016 -0700
 
-    PCI/AER: Use 32 bit int type domains
+    PCI/AER: Use 32 bit PCI domain numbers
     
-    New pci device provides additional pci domains that start above what 16
-    bits can address.
+    The Intel Volume Management Device (VMD) supports 32-bit domain numbers.
+    To accommodate this, use u32 instead of u16 to store domain numbers.
     
+    [bhelgaas: changelog]
     Signed-off-by: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit 323121f05c8d
+commit cbab4ba90c61
 Author: Keith Busch <keith.busch@intel.com>
 Date:   Tue Jan 12 13:18:08 2016 -0700
 
-    x86/PCI: Allow PCI domain specific dma ops
+    x86/PCI: Allow DMA ops specific to a PCI domain
     
-    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 discovered in that domain to use them.
+    The Intel Volume Management Device (VMD) is a PCIe endpoint that acts as a
+    host bridge to another PCI domain.  When devices below the VMD perform DMA,
+    the VMD replaces their DMA source IDs with its own source ID.  Therefore,
+    those devices require special DMA ops.
     
+    Add interfaces to allow the VMD driver to set up dma_ops for the devices
+    below it.
+    
+    [bhelgaas: remove "extern", add "static", changelog]
     Signed-off-by: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit b54bc8bc78d0
+commit db6a7b7a3887
 Author: Keith Busch <keith.busch@intel.com>
 Date:   Tue Jan 12 13:18:07 2016 -0700
 
-    x86/IRQ: Export IRQ domain function for module use
+    irqdomain: Export irq_domain_set_info() for module use
+    
+    Export irq_domain_set_info() for module use.  It will be used by the Volume
+    Management Device driver.
     
+    [bhelgaas: changelog]
     Signed-off-by: Keith Busch <keith.busch@intel.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit 7c8c9dc397f2
+commit 23e42d853208
 Author: Liu Jiang <jiang.liu@linux.intel.com>
 Date:   Tue Jan 12 13:18:06 2016 -0700
 
-    msi: Relax msi_domain_alloc() to support parentless MSI irqdomains
+    genirq/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
+    Previously msi_domain_alloc() assumed MSI irqdomains always had parent
+    irqdomains, but that's not true for the new Intel VMD devices.  
+ Relax
     msi_domain_alloc() to support parentless MSI irqdomains.
     
     Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>


diff --git a/MAINTAINERS b/MAINTAINERS
index 76369d4..ce47e08 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8216,7 +8216,7 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/host-generic-pci.txt
 F:	drivers/pci/host/pci-host-generic.c
 
-PCI DRIVER FOR VMD
+PCI DRIVER FOR INTEL VOLUME MANAGEMENT DEVICE (VMD)
 M:	Keith Busch <keith.busch@intel.com>
 L:	linux-pci@vger.kernel.org
 S:	Supported
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 9a5ab69..3e6aca8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2670,13 +2670,13 @@ config VMD
 	tristate "Volume Management Device Driver"
 	default N
 	---help---
-	  Adds support for the Intel Volume Manage Device (VMD). VMD is a
+	  Adds support for the Intel Volume Management 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
+	  more 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.
+	  has devices attached to it, say Y; if you are not sure, say N.
 
 source "net/Kconfig"
 
diff --git a/arch/x86/include/asm/device.h b/arch/x86/include/asm/device.h index 3b23897..684ed6c 100644
--- a/arch/x86/include/asm/device.h
+++ b/arch/x86/include/asm/device.h
@@ -16,8 +16,8 @@ struct dma_domain {
 	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);
+void add_dma_domain(struct dma_domain *domain); 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 106fd13..2879efc 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -642,8 +642,8 @@ unsigned int pcibios_assign_all_busses(void)  }
 
 #if defined(CONFIG_X86_DEV_DMA_OPS) && defined(CONFIG_PCI_DOMAINS) -LIST_HEAD(dma_domain_list); -DEFINE_SPINLOCK(dma_domain_list_lock);
+static LIST_HEAD(dma_domain_list);
+static DEFINE_SPINLOCK(dma_domain_list_lock);
 
 void add_dma_domain(struct dma_domain *domain)  { diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c index 56ef447..d57e480 100644
--- a/arch/x86/pci/vmd.c
+++ b/arch/x86/pci/vmd.c
@@ -27,20 +27,24 @@
 #include <asm/msi.h>
 #include <asm/msidef.h>
 
+#define VMD_CFGBAR	0
+#define VMD_MEMBAR1	2
+#define VMD_MEMBAR2	4
+
 /*
- * Lock for manipulating vmd irq lists.
+ * 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
+ * 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.
+ * @rcu:	RCU callback item for freeing.
  * @irq:	back pointer to parent.
- * @virq:	the virtual irq value provided to the requesting driver.
+ * @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.
+ * 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;
@@ -50,11 +54,11 @@ struct vmd_irq {
 };
 
 /**
- * struct vmd_irq_list - list of driver requested irq's mapping to a vmd vector
+ * struct vmd_irq_list - list of driver requested IRQs 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
+ * @vmd_vector:	the h/w IRQ assigned to the VMD.
+ * @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 {
@@ -92,11 +96,11 @@ static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)  }
 
 /*
- * Drivers managing a device in a VMD domain allocate their own irqs as before,
+ * 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
+ * destination ID for the VMD MSI-X table.  The VMD 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) @@ -110,7 +114,7 @@ static void vmd_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)  }
 
 /*
- * We rely on MSI_FLAG_USE_DEF_CHIP_OPS to set the irq mask/unmask ops.
+ * 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)  { @@ -139,7 +143,7 @@ static void vmd_irq_disable(struct irq_data *data)
  * other devices sharing the same vector.
  */
 static int vmd_irq_set_affinity(struct irq_data *data,
-			const struct cpumask *dest, bool force)
+				const struct cpumask *dest, bool force)
 {
 	return -EINVAL;
 }
@@ -159,7 +163,7 @@ static irq_hw_number_t vmd_get_hwirq(struct msi_domain_info *info,  }
 
 /*
- * XXX: We can be even smarter selecting the best irq once we solve the
+ * 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) @@ -176,8 +180,7 @@ static struct vmd_irq_list *vmd_next_irq(struct vmd_dev *vmd)
 	return &vmd->irqs[best];
 }
 
-static int vmd_msi_init(struct irq_domain *domain,
-			struct msi_domain_info *info,
+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)
 {
@@ -191,14 +194,13 @@ static int vmd_msi_init(struct irq_domain *domain,
 	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);
+	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 msi_domain_info *info, unsigned int virq)
 {
 	struct vmd_irq *vmdirq = irq_get_chip_data(virq);
 
@@ -210,15 +212,15 @@ static void vmd_msi_free(struct irq_domain *domain,
 	kfree_rcu(vmdirq, rcu);
 }
 
-static int vmd_msi_prepare(struct irq_domain *domain,
-			struct device *dev, int nvec,
-			msi_alloc_info_t *arg)
+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;
 }
@@ -245,8 +247,8 @@ static struct msi_domain_info vmd_msi_domain_info = {
 
 #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
+ * VMD replaces the requester ID with its own.  DMA mappings for 
+ devices in a
+ * VMD domain need to be mapped for the VMD, not the device requiring
  * the mapping.
  */
 static struct device *to_vmd_dev(struct device *dev) @@ -263,76 +265,73 @@ static struct dma_map_ops *vmd_dma_ops(struct device *dev)  }
 
 static void *vmd_alloc(struct device *dev, size_t size, dma_addr_t *addr,
-				gfp_t flag, struct dma_attrs *attrs)
+		       gfp_t flag, struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->alloc(to_vmd_dev(dev), size, addr, flag,
-								attrs);
+				       attrs);
 }
 
 static void vmd_free(struct device *dev, size_t size, void *vaddr,
-				dma_addr_t addr, struct dma_attrs *attrs)
+		     dma_addr_t addr, struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->free(to_vmd_dev(dev), size, vaddr, addr,
-								attrs);
+				      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)
+		    void *cpu_addr, dma_addr_t addr, size_t size,
+		    struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->mmap(to_vmd_dev(dev), vma, cpu_addr, addr,
-								size, attrs);
+				      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)
+			   void *cpu_addr, dma_addr_t addr, size_t size,
+			   struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->get_sgtable(to_vmd_dev(dev), sgt, cpu_addr,
-							addr, size, attrs);
+					     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)
+			       unsigned long offset, size_t size,
+			       enum dma_data_direction dir,
+			       struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->map_page(to_vmd_dev(dev), page, offset, size,
-								dir, attrs);
+					  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)
+			   enum dma_data_direction dir, struct dma_attrs *attrs)
 {
 	vmd_dma_ops(dev)->unmap_page(to_vmd_dev(dev), 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)
+		      enum dma_data_direction dir, struct dma_attrs *attrs)
 {
 	return vmd_dma_ops(dev)->map_sg(to_vmd_dev(dev), 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)
+			 enum dma_data_direction dir, struct dma_attrs *attrs)
 {
 	vmd_dma_ops(dev)->unmap_sg(to_vmd_dev(dev), 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)
+				    size_t size, enum dma_data_direction dir)
 {
 	vmd_dma_ops(dev)->sync_single_for_cpu(to_vmd_dev(dev), 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)
+				       size_t size, enum dma_data_direction dir)
 {
 	vmd_dma_ops(dev)->sync_single_for_device(to_vmd_dev(dev), addr, size,
-									dir);
+						 dir);
 }
 
 static void vmd_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg, @@ -342,7 +341,7 @@ static void vmd_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,  }
 
 static void vmd_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
-				int nents, enum dma_data_direction dir)
+				   int nents, enum dma_data_direction dir)
 {
 	vmd_dma_ops(dev)->sync_sg_for_device(to_vmd_dev(dev), sg, nents, dir);  } @@ -414,20 +413,32 @@ static void vmd_teardown_dma_ops(struct vmd_dev *vmd) {}  static void vmd_setup_dma_ops(struct vmd_dev *vmd) {}  #endif
 
+static char __iomem *vmd_cfg_addr(struct vmd_dev *vmd, struct pci_bus *bus,
+				  unsigned int devfn, int reg, int len) {
+	char __iomem *addr = vmd->cfgbar +
+			     (bus->number << 20) + (devfn << 12) + reg;
+
+	if ((addr - vmd->cfgbar) + len >=
+	    resource_size(&vmd->dev->resource[VMD_CFGBAR]))
+		return NULL;
+
+	return addr;
+}
+
 /*
  * 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 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;
+	char __iomem *addr = vmd_cfg_addr(vmd, bus, devfn, reg, len);
+	unsigned long flags;
+	int ret = 0;
 
-	if ((addr - vmd->cfgbar) + len >= resource_size(&vmd->dev->resource[0]))
+	if (!addr)
 		return -EFAULT;
 
 	spin_lock_irqsave(&vmd->cfg_lock, flags); @@ -455,15 +466,14 @@ static int vmd_pci_read(struct pci_bus *bus, unsigned int devfn, int reg,
  * 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 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;
+	char __iomem *addr = vmd_cfg_addr(vmd, bus, devfn, reg, len);
+	unsigned long flags;
+	int ret = 0;
 
-	if ((addr - vmd->cfgbar) + len >= resource_size(&vmd->dev->resource[0]))
+	if (!addr)
 		return -EFAULT;
 
 	spin_lock_irqsave(&vmd->cfg_lock, flags); @@ -494,7 +504,7 @@ static struct pci_ops vmd_ops = {  };
 
 /*
- * VMD domains start at 10000h to not clash with domains defining ACPI _SEG.
+ * VMD domains start at 0x1000 to not clash with ACPI _SEG domains.
  */
 static int vmd_find_free_domain(void)
 {
@@ -510,37 +520,50 @@ static int vmd_enable_domain(struct vmd_dev *vmd)  {
 	struct pci_sysdata *sd = &vmd->sysdata;
 	struct resource *res;
+	u32 upper_bits;
+	unsigned long flags;
 	LIST_HEAD(resources);
 
-	res = &vmd->resources[0];
-	res->name = "VMD CFGBAR";
-	res->start = 0;
-	res->end = (resource_size(&vmd->dev->resource[0]) >> 20) - 1;
-	res->flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED;
-
-	res = &vmd->resources[1];
-	res->name = "VMD MEMBAR1";
-	res->start = vmd->dev->resource[2].start;
-	res->end = vmd->dev->resource[2].end;
-	res->flags = (vmd->dev->resource[2].flags & ~IORESOURCE_SIZEALIGN) &
-				(!upper_32_bits(vmd->dev->resource[2].end) ?
-						~IORESOURCE_MEM_64 : ~0);
-
-	res = &vmd->resources[2];
-	res->name = "VMD MEMBAR2";
-	res->start = vmd->dev->resource[4].start + 0x2000;
-	res->end = vmd->dev->resource[4].end;
-	res->flags = (vmd->dev->resource[4].flags & ~IORESOURCE_SIZEALIGN) &
-				(!upper_32_bits(vmd->dev->resource[4].end) ?
-						~IORESOURCE_MEM_64 : ~0);
+	res = &vmd->dev->resource[VMD_CFGBAR];
+	vmd->resources[0] = (struct resource) {
+		.name  = "VMD CFGBAR",
+		.start = res->start,
+		.end   = (resource_size(res) >> 20) - 1,
+		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
+	};
+
+	res = &vmd->dev->resource[VMD_MEMBAR1];
+	upper_bits = upper_32_bits(res->end);
+	flags = res->flags & ~IORESOURCE_SIZEALIGN;
+	if (!upper_bits)
+		flags &= ~IORESOURCE_MEM_64;
+	vmd->resources[1] = (struct resource) {
+		.name  = "VMD MEMBAR1",
+		.start = res->start,
+		.end   = res->end,
+		.flags = flags,
+	};
+
+	res = &vmd->dev->resource[VMD_MEMBAR2];
+	upper_bits = upper_32_bits(res->end);
+	flags = res->flags & ~IORESOURCE_SIZEALIGN;
+	if (!upper_bits)
+		flags &= ~IORESOURCE_MEM_64;
+	vmd->resources[2] = (struct resource) {
+		.name  = "VMD MEMBAR2",
+		.start = res->start + 0x2000,
+		.end   = res->end,
+		.flags = flags,
+	};
 
 	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);
+						    NULL);
 	if (!vmd->irq_domain)
 		return -ENODEV;
 
@@ -548,7 +571,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd)
 	pci_add_resource(&resources, &vmd->resources[1]);
 	pci_add_resource(&resources, &vmd->resources[2]);
 	vmd->bus = pci_create_root_bus(&vmd->dev->dev, 0, &vmd_ops, sd,
-								&resources);
+				       &resources);
 	if (!vmd->bus) {
 		pci_free_resource_list(&resources);
 		irq_domain_remove(vmd->irq_domain);
@@ -560,8 +583,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd)
 	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");
+			       "domain"), "Can't create symlink to domain\n");
 	return 0;
 }
 
@@ -583,7 +605,7 @@ 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))
+	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20))
 		return -ENOMEM;
 
 	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL); @@ -595,7 +617,7 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (err < 0)
 		return err;
 
-	vmd->cfgbar = pcim_iomap(dev, 0, 0);
+	vmd->cfgbar = pcim_iomap(dev, VMD_CFGBAR, 0);
 	if (!vmd->cfgbar)
 		return -ENOMEM;
 
@@ -609,19 +631,20 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 
 	vmd->irqs = devm_kcalloc(&dev->dev, vmd->msix_count, sizeof(*vmd->irqs),
-							GFP_KERNEL);
+				 GFP_KERNEL);
 	if (!vmd->irqs)
 		return -ENOMEM;
 
 	vmd->msix_entries = devm_kcalloc(&dev->dev, vmd->msix_count,
-					sizeof(*vmd->msix_entries), GFP_KERNEL);
+					 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);
+						vmd->msix_count);
 	if (vmd->msix_count < 0)
 		return vmd->msix_count;
 
@@ -631,7 +654,7 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		vmd->irqs[i].index = i;
 
 		err = devm_request_irq(&dev->dev, vmd->irqs[i].vmd_vector,
-				vmd_irq, 0, "vmd", &vmd->irqs[i]);
+				       vmd_irq, 0, "vmd", &vmd->irqs[i]);
 		if (err)
 			return err;
 	}
@@ -641,8 +664,9 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	err = vmd_enable_domain(vmd);
 	if (err)
 		return err;
-	dev_info(&vmd->dev->dev, "Bound to PCI domain:%x\n",
-					vmd->sysdata.domain);
+
+	dev_info(&vmd->dev->dev, "Bound to PCI domain %04x\n",
+		 vmd->sysdata.domain);
 	return 0;
 }
 

[toc] | [prev] | [next] | [standalone]

#1310424

On Fri, Jan 15, 2016 at 11:48:11AM -0800, Derrick, Jonathan wrote:
> Hi folks,
> 
> The VMD driver can be built as a module, so the following symbols need to be exported:
> pci_msi_create_irq_domain
> msi_desc_to_pci_dev

Hi Jon,

Your tree is a little old. It's added in commit a4289dc2ec3a5:

 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a4289dc2ec3a5821076a78ee9678909b4eff297e

But since you mention it, this is not in the pci repo yet either, but
it's a clean cherry-pick for us to test.

[toc] | [prev] | [next] | [standalone]

#1310433

On Fri, Jan 15, 2016 at 07:54:08PM +0000, Keith Busch wrote:
> On Fri, Jan 15, 2016 at 11:48:11AM -0800, Derrick, Jonathan wrote:
> > Hi folks,
> > 
> > The VMD driver can be built as a module, so the following symbols need to be exported:
> > pci_msi_create_irq_domain
> > msi_desc_to_pci_dev
> 
> Hi Jon,
> 
> Your tree is a little old. It's added in commit a4289dc2ec3a5:
> 
>  https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a4289dc2ec3a5821076a78ee9678909b4eff297e
> 
> But since you mention it, this is not in the pci repo yet either, but
> it's a clean cherry-pick for us to test.

Aha, good catch

[toc] | [prev] | [next] | [standalone]

#1310504

On Fri, Jan 15, 2016 at 07:48:11PM +0000, Derrick, Jonathan wrote:
> Additionally, what are your thoughts of moving the menuconfig option from the root-tier to 'Processor Type and Features'?

I don't know how Intel is going to market this feature, but from a
developer point of view, the code looks more like the PCI host bridge
drivers in drivers/pci/host than it does like a processor feature.
Most of those drivers are specific to a particular architecture or
SoC.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1312337

On Fri, Jan 15, 2016 at 04:06:31PM -0600, Bjorn Helgaas wrote:
> On Fri, Jan 15, 2016 at 07:48:11PM +0000, Derrick, Jonathan wrote:
> > Additionally, what are your thoughts of moving the menuconfig option from the root-tier to 'Processor Type and Features'?
> 
> I don't know how Intel is going to market this feature, but from a
> developer point of view, the code looks more like the PCI host bridge
> drivers in drivers/pci/host than it does like a processor feature.
> Most of those drivers are specific to a particular architecture or
> SoC.

We had tighter dependencies on x86 in earlier revisions of this driver. It
probably now looks more sensible to be in drivers/pci/host instead of
arch specific.

Do you want us to make this change and resend the series? Or can we
provide patches for in tree development? We'll also add the requested
code comments to explain more about the device.

BTW, we completed tests with your pci/host-vmd branch, and that was
successful.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web