Driver for new VMD device

From review discussions, we discovered potential clashes in domain

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

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

arch/x86/kernel/apic/msi.c +8 -0

+struct irq_domain_ops;
+struct irq_domain *vmd_create_irq_domain(struct pci_dev *dev,
+				const struct irq_domain_ops *domain_ops);
+#endif
 
 
+#ifdef CONFIG_HAVE_VMDDEV
+static struct irq_chip pci_chained_msi_controller = {
+	.name			= "PCI-MSI-chained",
+};

arch/x86/kernel/apic/msi.c +10 -0

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

arch/x86/pci/vmd.c new file +12 -0

+{
+	return 0;
+}
+
+static struct irq_chip vmd_chip = {
+	.name 			= "VMD-MSI",
+	.irq_compose_msi_msg	= vmd_msi_compose_msg,
+	.irq_write_msi_msg	= vmd_msi_write_msg,
+	.irq_set_affinity	= vmd_irq_set_affinity,
+};
+
+static void vmd_msi_free_irqs(struct irq_domain *domain,

arch/x86/pci/vmd.c new file +11 -0

+}
+
+static int vmd_msi_alloc_irqs(struct irq_domain *domain, unsigned int virq,
+				 unsigned int nr_irqs, void *arg)
+{
+	struct vmd_dev *vmd;
+	struct vmd_irq *vmdirq;
+	struct irq_data *irq_data;
+	struct pci_dev *dev = domain->host_data;
+
+	irq_data = irq_domain_get_irq_data(domain, virq);

arch/x86/pci/vmd.c new file +28 -0

+
+	return 0;
+}
+
+static void vmd_msi_activate(struct irq_domain *domain,
+					struct irq_data *data)
+{
+	struct msi_msg msg;
+	struct vmd_irq *vmdirq = data->chip_data;
+	struct vmd_irq_list *irq = vmdirq->irq;
+
+	BUG_ON(irq_chip_compose_msi_msg(data, &msg));
+	data->chip->irq_write_msi_msg(data, &msg);
+
+	spin_lock(&list_lock);
+	list_add_tail_rcu(&vmdirq->node, &irq->irq_list);
+	spin_unlock(&list_lock);
+}
+
+static void vmd_msi_deactivate(struct irq_domain *domain,
+				   struct irq_data *data)
+{
+	struct msi_msg msg;
+	struct vmd_irq *vmdirq = data->chip_data;
+
+	memset(&msg, 0, sizeof(msg));
+	data->chip->irq_write_msi_msg(data, &msg);
+

arch/x86/pci/vmd.c new file +3 -0

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

arch/x86/pci/vmd.c new file +3 -0

+	if (!vmd->irq_domain)
+		return -ENODEV;
 
+static void vmd_flow_handler(struct irq_desc *desc)

arch/x86/pci/vmd.c new file +37 -0

+	chained_irq_exit(chip, desc);
+}
+
+static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	struct vmd_dev *vmd;
+	int i, err;
+
+	if (resource_size(&dev->resource[0]) < (1 << 20))
+		return -ENOMEM;
+
+	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
+	if (!vmd)
+		return -ENOMEM;
+
+	vmd->dev = dev;
+	err = pcim_enable_device(dev);
+	if (err < 0)
+		return err;
+
+	vmd->cfgbar = pcim_iomap(dev, 0, 0);
+	if (!vmd->cfgbar)
+		return -ENOMEM;
+
+	pci_set_master(dev);
+	if (dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(64)) &&
+	    dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32)))
+		return -ENODEV;
+
+	vmd->msix_count = pci_msix_vec_count(dev);
+	if (!vmd->msix_count)
+		return -ENODEV;
+
+	vmd->irqs = devm_kcalloc(&dev->dev, vmd->msix_count, sizeof(*vmd->irqs),
+							GFP_KERNEL);
+	if (!vmd->irqs)
+		return -ENOMEM;

arch/x86/pci/vmd.c new file +5 -0

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

On Tue, 27 Oct 2015, Keith Busch wrote:

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

arch/x86/Kconfig config PMC_ATOM · line 2636

Quoted 2 lines

arch/x86/Kconfig +2 -0

+config HAVE_VMDDEV
+	bool

Why do you need a seperate config symbol here?

arch/x86/Kconfig config PMC_ATOM · line 2650

Quoted 3 lines

arch/x86/Kconfig +3 -0

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

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

Quoted 4 lines

arch/x86/kernel/apic/msi.c +4 -0

+	int count = 0;
+	struct msi_desc *msi_desc;
+	struct irq_domain *vmd_irqdomain, *msi_irqdomain;
+

arch/x86/kernel/apic/msi.c static struct msi_domain_info pci_msi_domain_info = { · line 161

Quoted 5 lines

arch/x86/kernel/apic/msi.c +5 -0

+	if (!dev->msix_enabled)
+		return NULL;
+
+	for_each_pci_msi_entry(msi_desc, dev)
+		count += msi_desc->nvec_used;

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

arch/x86/pci/vmd.c line 75

Quoted 7 lines

arch/x86/pci/vmd.c new file +7 -0

+static void vmd_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct vmd_irq *vmdirq = data->chip_data;
+
+	msg->address_hi = MSI_ADDR_BASE_HI;
+	msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_DEST_ID(vmdirq->irq->index);
+	msg->data = MSI_DATA_VECTOR(vmdirq->irq->vmd_vector);

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

arch/x86/pci/vmd.c line 131

Quoted 1 line

arch/x86/pci/vmd.c new file +1 -0

+	vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];

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

Quoted 3 lines

arch/x86/pci/vmd.c new file +3 -0

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

<SNIP>

arch/x86/pci/vmd.c line 489

Quoted 1 line

arch/x86/pci/vmd.c new file +1 -0

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

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

Quoted 4 lines

arch/x86/pci/vmd.c new file +3 -0

+	if (!vmd->irq_domain)
+		return -ENODEV;
 
+static void vmd_flow_handler(struct irq_desc *desc)

arch/x86/pci/vmd.c line 523

Quoted 10 lines

arch/x86/pci/vmd.c new file +10 -0

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

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

arch/x86/pci/vmd.c line 570

Quoted 2 lines

arch/x86/pci/vmd.c new file +2 -0

+	vmd->msix_count = pci_enable_msix_range(vmd->dev, vmd->msix_entries, 1,
+							vmd->msix_count);

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

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

Quoted 4 lines

arch/x86/pci/vmd.c new file +4 -0

+static void vmd_remove(struct pci_dev *dev)
+{
+	struct vmd_dev *vmd = pci_get_drvdata(dev);
+

arch/x86/pci/vmd.c line 600

Quoted 6 lines

arch/x86/pci/vmd.c new file +6 -0

+	pci_set_drvdata(dev, NULL);
+	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
+	pci_stop_root_bus(vmd->bus);
+	pci_remove_root_bus(vmd->bus);
+	vmd_teardown_dma_ops(vmd);
+	irq_domain_remove(vmd->irq_domain);

What tears down the chained handlers and the MSIx entries?

Quoted 6 lines · Thomas Gleixner

arch/x86/Kconfig config PMC_ATOM · line 2636

Quoted 2 lines · Keith Busch

arch/x86/Kconfig +2 -0

+config HAVE_VMDDEV
+	bool

Why do you need a seperate config symbol here?

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

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

Quoted 4 lines

Quoted 4 lines

arch/x86/kernel/apic/msi.c +4 -0

+	int count = 0;
+	struct msi_desc *msi_desc;
+	struct irq_domain *vmd_irqdomain, *msi_irqdomain;
+

Quoted 8 lines

arch/x86/kernel/apic/msi.c static struct msi_domain_info pci_msi_domain_info = { · line 161

Quoted 5 lines

arch/x86/kernel/apic/msi.c +5 -0

+	if (!dev->msix_enabled)
+		return NULL;
+
+	for_each_pci_msi_entry(msi_desc, dev)
+		count += msi_desc->nvec_used;

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

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

Quoted 4 lines

arch/x86/kernel/apic/msi.c static struct msi_domain_info pci_msi_domain_info = { · line 163

Quoted 4 lines

arch/x86/kernel/apic/msi.c +4 -0

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

Quoted 2 lines

arch/x86/kernel/apic/msi.c static struct msi_domain_info pci_msi_domain_info = { · line 167

Quoted 2 lines

arch/x86/kernel/apic/msi.c +2 -0

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

Quoted 6 lines

arch/x86/pci/vmd.c line 131

Quoted 1 line

arch/x86/pci/vmd.c new file +1 -0

+	vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];

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

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

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

Quoted 7 lines

Quoted 7 lines

arch/x86/pci/vmd.c new file +7 -0

+static void vmd_flow_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct vmd_irq_list *irqs = irq_desc_get_handler_data(desc);
+	struct vmd_irq *vmdirq;
+
+	chained_irq_enter(chip, desc);

Quoted 8 lines

arch/x86/pci/vmd.c line 523

Quoted 4 lines

arch/x86/pci/vmd.c new file +4 -0

+	rcu_read_lock();
+	list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
+		generic_handle_irq(vmdirq->virq);
+	rcu_read_unlock();

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

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

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

Quoted 4 lines

Quoted 4 lines

arch/x86/pci/vmd.c new file +4 -0

+static void vmd_remove(struct pci_dev *dev)
+{
+	struct vmd_dev *vmd = pci_get_drvdata(dev);
+

Quoted 8 lines

arch/x86/pci/vmd.c line 600

Quoted 6 lines

arch/x86/pci/vmd.c new file +6 -0

+	pci_set_drvdata(dev, NULL);
+	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
+	pci_stop_root_bus(vmd->bus);
+	pci_remove_root_bus(vmd->bus);
+	vmd_teardown_dma_ops(vmd);
+	irq_domain_remove(vmd->irq_domain);

What tears down the chained handlers and the MSIx entries?

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

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

Quoted 2 lines · Keith Busch

Quoted 2 lines

arch/x86/kernel/apic/msi.c static struct msi_domain_info pci_msi_domain_info = { · line 167

Quoted 2 lines

arch/x86/kernel/apic/msi.c +2 -0

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

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

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

Quoted 11 lines · Thomas Gleixner

Quoted 2 lines · Keith Busch

Quoted 2 lines

arch/x86/kernel/apic/msi.c static struct msi_domain_info pci_msi_domain_info = { · line 167

Quoted 2 lines

arch/x86/kernel/apic/msi.c +2 -0

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

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

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

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

pci_msi_setup_msi_irqs()
pci_msi_domain_alloc_irqs()
msi_domain_alloc_irqs()

    __irq_domain_alloc_irqs()
     irq_domain_alloc_irqs_recursive()
      msi_domain_alloc()
       irq_domain_alloc_irqs_parent()

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

Quoted 25 lines · Keith Busch

Quoted 11 lines · Thomas Gleixner

Quoted 2 lines · Keith Busch

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

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

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

pci_msi_setup_msi_irqs()
pci_msi_domain_alloc_irqs()
msi_domain_alloc_irqs()

    __irq_domain_alloc_irqs()
     irq_domain_alloc_irqs_recursive()
      msi_domain_alloc()
       irq_domain_alloc_irqs_parent()

Quoted 3 lines

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

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

Thanks,

tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From 6db1568564f8edee626d1c168b22ed5502abc6eb Mon Sep 17 00:00:00 2001

Hi Keith,
Could you please try the attached patch?
Thanks!
Gerry

From: Liu Jiang <[email protected]>
Date: Thu, 5 Nov 2015 11:25:07 +0800
Subject: [PATCH] msi: Relax msi_domain_alloc() to support parentless MSI
irqdomains

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

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
 kernel/irq/msi.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

kernel/irq/msi.c +5 -3

index 7e6512b9dc1f..e4d3d707efff 100644
@@ -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);

Signature · 1 line

1.7.10.4