Keith!
On Tue, 27 Oct 2015, Keith Busch wrote:
I'm just looking at the interrupt part of the patch and it's way
better than the first version I looked at.
> 4) Interrupts: Part of VMD's BAR 4 is reserved for VMD's MSI-X Table and
> PBA. MSIs from VMD domain devices and ports are remapped to appear if
> they were issued using one of VMD's MSI-X table entries. Each MSI and
> MSI-X addresses of VMD-owned devices and ports have a special format
> where the address refers specific entries in VMD's MSI-X table. As with
> DMA, the interrupt source id is translated to VMD's bus-device-function.
>
> The driver provides its own MSI and MSI-X configuration functions
> specific to how MSI messages are used within the VMD domain, and
> it provides an irq_chip for independent IRQ allocation and to relay
> interrupts from VMD's interrupt handler to the appropriate device
> driver's handler.
Is there any documentation on this available for mere mortals?
I have a faint idea how this is supposed to work, but some more
detailed information about this technology would be appreciated.
> +config HAVE_VMDDEV
> + bool
Why do you need a seperate config symbol here?
We usually use HAVE_xxx to signal the availability of functionality
which is then used by some other Kconfig entry as a dependency.
> +config VMDDEV
> + depends on PCI && PCI_DOMAINS && PCI_MSI && GENERIC_MSI_IRQ_DOMAIN && IRQ_DOMAIN_HIERARCHY
On x86 this dependency chain is not required. x86/Kconfig does:
config PCI_DOMAINS
def_bool y
depends on PCI
and
select PCI_MSI_IRQ_DOMAIN if PCI_MSI
and pci/Kconfig does:
config PCI_MSI_IRQ_DOMAIN
bool
depends on PCI_MSI
select GENERIC_MSI_IRQ_DOMAIN
kernel/irq/Kconfig has:
config GENERIC_MSI_IRQ_DOMAIN
bool
select IRQ_DOMAIN_HIERARCHY
So all you really need is:
depends on PCI_MSI
> + tristate "Volume Management Device Driver"
> + default N
> + select HAVE_VMDDEV
> + ---help---
> + Adds support for the Intel Volume Manage Device (VMD). VMD is
The help text should be indented with 2 extra spaces.
> + a secondary PCI host bridge that allows PCI Express root ports,
> + and devices attached to them, to be removed from the default PCI
> + domain and placed within the VMD domain This provides additional
Missing period after domain.
> + bus resources to allow more devices than are otherwise possible
> + with a single domain. If your system provides one of these and
> + has devices attached to it, say "Y".
How is one supposed to figure out that the system supports this?
> +#ifndef _ASM_X86_VMD_H
> +#define _ASM_X86_VMD_H
> +
> +#ifdef CONFIG_HAVE_VMDDEV
No need for that #ifdef
> +struct irq_domain_ops;
> +struct irq_domain *vmd_create_irq_domain(struct pci_dev *dev,
> + const struct irq_domain_ops *domain_ops);
> +#endif
> +#ifdef CONFIG_HAVE_VMDDEV
> +static struct irq_chip pci_chained_msi_controller = {
> + .name = "PCI-MSI-chained",
> +};
> +
> +static struct msi_domain_info pci_chained_msi_domain_info = {
> + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> + MSI_FLAG_PCI_MSIX,
> + .ops = &pci_msi_domain_ops,
> + .chip = &pci_chained_msi_controller,
> +};
> +
> +struct irq_domain *vmd_create_irq_domain(struct pci_dev *dev,
> + const struct irq_domain_ops *domain_ops)
This function really wants a comment what it is doing. AFAICT it
creates a disconnected vmd_irqdomain and a pci_msi irq domain as a
child domain of that. I'm missing the overall picture ...
> +{
> + int count = 0;
> + struct msi_desc *msi_desc;
> + struct irq_domain *vmd_irqdomain, *msi_irqdomain;
> +
> + if (!dev->msix_enabled)
> + return NULL;
> +
> + for_each_pci_msi_entry(msi_desc, dev)
> + count += msi_desc->nvec_used;
Is this the number of vectors which are available for the VMD device
itself?
> + vmd_irqdomain = irq_domain_add_linear(NULL, count,
> + domain_ops, dev);
> + if (!vmd_irqdomain)
> + return NULL;
> + msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
> + vmd_irqdomain);
> + if (!msi_irqdomain)
> + irq_domain_remove(vmd_irqdomain);
> + return msi_irqdomain;
> +}
> +EXPORT_SYMBOL_GPL(vmd_create_irq_domain);
> +#endif
> +++ b/arch/x86/pci/vmd.c
> +struct vmd_dev {
> + struct pci_dev *dev;
> +
> + spinlock_t cfg_lock;
> + char __iomem *cfgbar;
It'd be nice if you could write those structs like a table
+ struct pci_dev *dev;
+
+ spinlock_t cfg_lock;
+ char __iomem *cfgbar;
That makes it way simpler to parse.
> +struct vmd_irq {
> + struct list_head node;
> + struct vmd_irq_list *irq;
> + unsigned int virq;
> +};
> +
> +static DEFINE_SPINLOCK(list_lock);
Please do not put a variable declaration in the middle of struct
definitions.
> +struct vmd_irq_list {
> + struct list_head irq_list;
> + unsigned int vmd_vector;
> + unsigned int index;
> +};
These structs want an explanation for the struct members. Especially
the "*irq" one in vmd_irq is irritating.
> +static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)
> +{
> + return container_of(bus->sysdata, struct vmd_dev, sysdata);
> +}
> +
> +static void vmd_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> + struct vmd_irq *vmdirq = data->chip_data;
> +
> + msg->address_hi = MSI_ADDR_BASE_HI;
> + msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_DEST_ID(vmdirq->irq->index);
> + msg->data = MSI_DATA_VECTOR(vmdirq->irq->vmd_vector);
Is this the MSI msg which gets written into the VMD device or into the
device which hangs off the VMD device bus?
> +}
> +
> +static void vmd_msi_write_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> + pci_write_msi_msg(data->irq, msg);
Why would you go through another lookup for the msi descriptor? What's
wrong with pci_msi_domain_write_msg() ?
> +}
> +
> +/*
> + * Function is required when using irq hierarchy domains, but we don't have a
> + * good way to not create conflicts with other devices sharing the same vector.
This is just a lame work around and suggests the user that he can set
the affinity.
You can prevent affinity setting by other means.
Aside of that the question is whether you really want to prevent
affinity setting of all involved interrupts - the demultiplexers and
the device interrupts. Though I don't have any clue how this is going
to be used.
> + */
> +int vmd_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
> + bool force)
Is there a reason why this function would be global?
> +{
> + return 0;
> +}
> +
> +static struct irq_chip vmd_chip = {
> + .name = "VMD-MSI",
> + .irq_compose_msi_msg = vmd_msi_compose_msg,
> + .irq_write_msi_msg = vmd_msi_write_msg,
> + .irq_set_affinity = vmd_irq_set_affinity,
> +};
> +
> +static void vmd_msi_free_irqs(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> +{
> + struct vmd_irq *vmdirq;
> + struct irq_data *irq_data;
> +
> + irq_data = irq_domain_get_irq_data(domain, virq);
> + if (!irq_data)
> + return;
> + vmdirq = irq_data->chip_data;
> + kfree(vmdirq);
Shouldn't that be kfree_rcu()?
> +}
> +
> +static int vmd_msi_alloc_irqs(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *arg)
> +{
> + struct vmd_dev *vmd;
> + struct vmd_irq *vmdirq;
> + struct irq_data *irq_data;
> + struct pci_dev *dev = domain->host_data;
> +
> + irq_data = irq_domain_get_irq_data(domain, virq);
> + if (!irq_data)
> + return -EINVAL;
> +
> + vmd = pci_get_drvdata(dev);
> + vmdirq = kzalloc(sizeof(*vmdirq), GFP_KERNEL);
> + if (!vmdirq)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&vmdirq->node);
> + vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];
So that points to the underlying VMD device irq, right? And you spread
the device interrupts across the available VMD irqs. So in the worst
case you might end up with empty and crowded VMD irqs, right?
Shouldn't you try to fill that space in a more intelligent way?
> + vmdirq->virq = virq;
> +
> + irq_domain_set_hwirq_and_chip(domain, virq, vmdirq->irq->vmd_vector,
> + &vmd_chip, vmdirq);
> + irq_set_chip(virq, &vmd_chip);
> + irq_set_handler_data(virq, vmdirq);
> + __irq_set_handler(virq, handle_simple_irq, 0, NULL);
What's wrong with irq_set_handler() ?
> +
> + return 0;
> +}
> +
> +static void vmd_msi_activate(struct irq_domain *domain,
> + struct irq_data *data)
> +{
> + struct msi_msg msg;
> + struct vmd_irq *vmdirq = data->chip_data;
> + struct vmd_irq_list *irq = vmdirq->irq;
> +
> + BUG_ON(irq_chip_compose_msi_msg(data, &msg));
> + data->chip->irq_write_msi_msg(data, &msg);
> +
> + spin_lock(&list_lock);
> + list_add_tail_rcu(&vmdirq->node, &irq->irq_list);
> + spin_unlock(&list_lock);
> +}
> +
> +static void vmd_msi_deactivate(struct irq_domain *domain,
> + struct irq_data *data)
> +{
> + struct msi_msg msg;
> + struct vmd_irq *vmdirq = data->chip_data;
> +
> + memset(&msg, 0, sizeof(msg));
> + data->chip->irq_write_msi_msg(data, &msg);
> +
> + spin_lock(&list_lock);
> + list_del_rcu(&vmdirq->node);
> + spin_unlock(&list_lock);
> +}
> +
> +static const struct irq_domain_ops vmd_domain_ops = {
> + .alloc = vmd_msi_alloc_irqs,
> + .free = vmd_msi_free_irqs,
> + .activate = vmd_msi_activate,
> + .deactivate = vmd_msi_deactivate,
You nicely aligned the irq_chip above. Can you please use the same
scheme here?
> +static int vmd_enable_domain(struct vmd_dev *vmd)
> +{
> + struct pci_sysdata *sd = &vmd->sysdata;
<SNIP>
> + vmd->irq_domain = vmd_create_irq_domain(vmd->dev, &vmd_domain_ops);
This is the irq domain which the devices on that VMD bus are seeing, right?
> + if (!vmd->irq_domain)
> + return -ENODEV;
> +static void vmd_flow_handler(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct vmd_irq_list *irqs = irq_desc_get_handler_data(desc);
> + struct vmd_irq *vmdirq;
> +
> + chained_irq_enter(chip, desc);
> + rcu_read_lock();
> + list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
> + generic_handle_irq(vmdirq->virq);
> + rcu_read_unlock();
I'm not entirely sure that the rcu protection here covers the whole
virq thing when an interrupt is torn down. Can you please explain the
whole protection scheme in detail?
> + chained_irq_exit(chip, desc);
> +}
> +
> +static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
> +{
> + struct vmd_dev *vmd;
> + int i, err;
> +
> + if (resource_size(&dev->resource[0]) < (1 << 20))
> + return -ENOMEM;
> +
> + vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
> + if (!vmd)
> + return -ENOMEM;
> +
> + vmd->dev = dev;
> + err = pcim_enable_device(dev);
> + if (err < 0)
> + return err;
> +
> + vmd->cfgbar = pcim_iomap(dev, 0, 0);
> + if (!vmd->cfgbar)
> + return -ENOMEM;
> +
> + pci_set_master(dev);
> + if (dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(64)) &&
> + dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32)))
> + return -ENODEV;
> +
> + vmd->msix_count = pci_msix_vec_count(dev);
> + if (!vmd->msix_count)
> + return -ENODEV;
> +
> + vmd->irqs = devm_kcalloc(&dev->dev, vmd->msix_count, sizeof(*vmd->irqs),
> + GFP_KERNEL);
> + if (!vmd->irqs)
> + return -ENOMEM;
> +
> + vmd->msix_entries = devm_kcalloc(&dev->dev, vmd->msix_count,
> + sizeof(*vmd->msix_entries), GFP_KERNEL);
> + if(!vmd->msix_entries)
> + return -ENOMEM;
> + for (i = 0; i < vmd->msix_count; i++)
> + vmd->msix_entries[i].entry = i;
> +
> + vmd->msix_count = pci_enable_msix_range(vmd->dev, vmd->msix_entries, 1,
> + vmd->msix_count);
So this allocates the CPU facing MSI vectors, which are used to run
the demultiplex handler, right?
A few comments explaining the whole thing would be really
appropriate. I'm sure you'll appreciate them when you look at it
yourself a year from now.
> + if (vmd->msix_count < 0)
> + return vmd->msix_count;
> +
> + for (i = 0; i < vmd->msix_count; i++) {
> + INIT_LIST_HEAD(&vmd->irqs[i].irq_list);
> + vmd->irqs[i].vmd_vector = vmd->msix_entries[i].vector;
> + vmd->irqs[i].index = i;
> +
> + irq_set_chained_handler_and_data(vmd->msix_entries[i].vector,
> + vmd_flow_handler, &vmd->irqs[i]);
> + }
> + spin_lock_init(&vmd->cfg_lock);
> + err = vmd_enable_domain(vmd);
> + if (err)
> + return err;
What undoes the above in case that vmd_enable_domain() fails? Same
question for all other error returns ...
> +static void vmd_remove(struct pci_dev *dev)
> +{
> + struct vmd_dev *vmd = pci_get_drvdata(dev);
> +
> + pci_set_drvdata(dev, NULL);
> + sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
> + pci_stop_root_bus(vmd->bus);
> + pci_remove_root_bus(vmd->bus);
> + vmd_teardown_dma_ops(vmd);
> + irq_domain_remove(vmd->irq_domain);
What tears down the chained handlers and the MSIx entries?
> +EXPORT_SYMBOL_GPL(irq_chip_compose_msi_msg);
> +EXPORT_SYMBOL_GPL(irq_domain_get_irq_data);
> +EXPORT_SYMBOL_GPL(irq_domain_set_hwirq_and_chip);
> +EXPORT_SYMBOL_GPL(irq_domain_get_irq_data);
This wants to be a seperate patch please.
Thanks,
tglx
On Mon, Nov 02, 2015 at 07:35:15PM +0100, Thomas Gleixner wrote:
> On Tue, 27 Oct 2015, Keith Busch wrote:
Thomas,
Thanks a bunch for the feedback! I'll reply what I can right now, and
will take more time to consider or fix the rest for the next revision.
> I'm just looking at the interrupt part of the patch and it's way
> better than the first version I looked at.
Thanks! Though I have to credit Gerry for the initial setup.
I think you correctly deduced the interrupt mux/demux requirement. I
don't even completely follow how the irq domain code accomplishes this,
but it works on the h/w I've been provided and I trust Gerry wouldn't
steer me wrong with the software. :)
> > +config HAVE_VMDDEV
> > + bool
>
> Why do you need a seperate config symbol here?
This is to split the kernel vs. module parts. "vmd_create_irq_domain"
needs to be in-kernel, but VMD may be compiled as a module, so can't use
"CONFIG_VMD_DEV".
Alternatively we could export pci_msi_domain_ops, and no additional
in-kernel dependencies are required.
> > + bus resources to allow more devices than are otherwise possible
> > + with a single domain. If your system provides one of these and
> > + has devices attached to it, say "Y".
>
> How is one supposed to figure out that the system supports this?
I'll work out more appropriate text. Basically enabling this is a
platform setting that requires purposeful intent, almost certainly not
by accident. The user ought to know prior to OS install, so maybe it
should say 'if you're not sure, say "N"'.
> > + int count = 0;
> > + struct msi_desc *msi_desc;
> > + struct irq_domain *vmd_irqdomain, *msi_irqdomain;
> > +
> > + if (!dev->msix_enabled)
> > + return NULL;
> > +
> > + for_each_pci_msi_entry(msi_desc, dev)
> > + count += msi_desc->nvec_used;
>
> Is this the number of vectors which are available for the VMD device
> itself?
Correct, this is counting the number of the VMD vectors itself. This is
not the number of vectors that may be allocated in this domain, though
they will all need to multiplex into one of these.
> > +static void vmd_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> > +{
> > + struct vmd_irq *vmdirq = data->chip_data;
> > +
> > + msg->address_hi = MSI_ADDR_BASE_HI;
> > + msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_DEST_ID(vmdirq->irq->index);
> > + msg->data = MSI_DATA_VECTOR(vmdirq->irq->vmd_vector);
>
> Is this the MSI msg which gets written into the VMD device or into the
> device which hangs off the VMD device bus?
This is the message to be written in the "child" device hanging off
the VMD domain. The child table must be programmed with VMD vectors,
and many child devices may share the same one. In some cases, a single
device might have multiple table entries with the same vector.
The child device driver's IRQ is purely a software concept in this domain
to facilitate chaining the VMD IRQ to the appropriate handler. The h/w
will never see it.
> > + vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];
>
> So that points to the underlying VMD device irq, right? And you spread
> the device interrupts across the available VMD irqs. So in the worst
> case you might end up with empty and crowded VMD irqs, right?
> Shouldn't you try to fill that space in a more intelligent way?
The domain is enabled and enumerated by pci, so I thought devices will
be bound to their drivers serially, creating a more-or-less incrementing
virtual irq that should wrap in evenly.
But yes, we can be smarter about this, and the above is probably flawed
rationale, especially if considering hot-plug.
> > +static int vmd_enable_domain(struct vmd_dev *vmd)
> > +{
> > + struct pci_sysdata *sd = &vmd->sysdata;
>
> <SNIP>
>
> > + vmd->irq_domain = vmd_create_irq_domain(vmd->dev, &vmd_domain_ops);
>
> This is the irq domain which the devices on that VMD bus are seeing, right?
Correct.
> > + if (!vmd->irq_domain)
> > + return -ENODEV;
>
> > +static void vmd_flow_handler(struct irq_desc *desc)
> > +{
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct vmd_irq_list *irqs = irq_desc_get_handler_data(desc);
> > + struct vmd_irq *vmdirq;
> > +
> > + chained_irq_enter(chip, desc);
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
> > + generic_handle_irq(vmdirq->virq);
> > + rcu_read_unlock();
>
> I'm not entirely sure that the rcu protection here covers the whole
> virq thing when an interrupt is torn down. Can you please explain the
> whole protection scheme in detail?
This protects against device drivers manipulating the list through
vmd_msi_activate/deactivate. We can't spinlock the flow handler access
due to desc->lock taken through generic_handle_irq. The same lock is
held prior to vmd's msi activate/deactivate, so the lock order would be
opposite in those two paths.
I don't believe we're in danger of missing a necessary event or triggering
an unexpected event here. Is the concern that we need to synchronize rcu
instead of directly freeing the "vmdirq"? I think I see that possibility.
> > + vmd->msix_count = pci_enable_msix_range(vmd->dev, vmd->msix_entries, 1,
> > + vmd->msix_count);
>
> So this allocates the CPU facing MSI vectors, which are used to run
> the demultiplex handler, right?
Correct.
> A few comments explaining the whole thing would be really
> appropriate. I'm sure you'll appreciate them when you look at it
> yourself a year from now.
Absolutely. It's not entirely obvious what's happening, but I think you
correctly surmised what this is about, and hopefully some of the answers
to your other questions helped with some details.
We will provide more detail in code comments for the next round.
> > +static void vmd_remove(struct pci_dev *dev)
> > +{
> > + struct vmd_dev *vmd = pci_get_drvdata(dev);
> > +
> > + pci_set_drvdata(dev, NULL);
> > + sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
> > + pci_stop_root_bus(vmd->bus);
> > + pci_remove_root_bus(vmd->bus);
> > + vmd_teardown_dma_ops(vmd);
> > + irq_domain_remove(vmd->irq_domain);
>
> What tears down the chained handlers and the MSIx entries?
Thanks, I will fix the chained handler in both cases mentioned.
The entries themselves should be freed through devres_release after this
exits, and after MSI-x is disabled via pcim_release.
Keith,
On Tue, 3 Nov 2015, Keith Busch wrote:
> On Mon, Nov 02, 2015 at 07:35:15PM +0100, Thomas Gleixner wrote:
> > On Tue, 27 Oct 2015, Keith Busch wrote:
> > > +config HAVE_VMDDEV
> > > + bool
> >
> > Why do you need a seperate config symbol here?
>
> This is to split the kernel vs. module parts. "vmd_create_irq_domain"
> needs to be in-kernel, but VMD may be compiled as a module, so can't use
> "CONFIG_VMD_DEV".
#ifdef IS_ENABLED(CONFIG_VMD_DEV)
is true for both "m" and "y"
> Alternatively we could export pci_msi_domain_ops, and no additional
> in-kernel dependencies are required.
That might be not the worst choice.
> > > + int count = 0;
> > > + struct msi_desc *msi_desc;
> > > + struct irq_domain *vmd_irqdomain, *msi_irqdomain;
> > > +
> > > + if (!dev->msix_enabled)
> > > + return NULL;
> > > +
> > > + for_each_pci_msi_entry(msi_desc, dev)
> > > + count += msi_desc->nvec_used;
> >
> > Is this the number of vectors which are available for the VMD device
> > itself?
>
> Correct, this is counting the number of the VMD vectors itself. This is
> not the number of vectors that may be allocated in this domain, though
> they will all need to multiplex into one of these.
But you actually limit the number of vectors in that domain:
> > > + for_each_pci_msi_entry(msi_desc, dev)
> > > + count += msi_desc->nvec_used;
> > > + vmd_irqdomain = irq_domain_add_linear(NULL, count,
> > > + domain_ops, dev);
So vmd_irqdomain can only hand out "count" vectors.
The vmd_irqdomain is the parent of the msi_irqdomain:
> > > + msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
> > > + vmd_irqdomain);
But that parent limitation does not matter simply because your
msi_irqdomain does not follow down the hierarchy in the allocation
path.
So we can avoid the vmd_irqdomain creation completely. It's just
wasting memory and has no value at all. Creating the msi domain with a
NULL parent is possible.
> > > + vmdirq->irq = &vmd->irqs[virq % vmd->msix_count];
> >
> > So that points to the underlying VMD device irq, right? And you spread
> > the device interrupts across the available VMD irqs. So in the worst
> > case you might end up with empty and crowded VMD irqs, right?
> > Shouldn't you try to fill that space in a more intelligent way?
>
> The domain is enabled and enumerated by pci, so I thought devices will
> be bound to their drivers serially, creating a more-or-less incrementing
> virtual irq that should wrap in evenly.
>
> But yes, we can be smarter about this, and the above is probably flawed
> rationale, especially if considering hot-plug.
Not only hotplug. It also depends on the init ordering and the
population of the interrupt descriptor space.
> > > +static void vmd_flow_handler(struct irq_desc *desc)
> > > +{
> > > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > > + struct vmd_irq_list *irqs = irq_desc_get_handler_data(desc);
> > > + struct vmd_irq *vmdirq;
> > > +
> > > + chained_irq_enter(chip, desc);
> > > + rcu_read_lock();
> > > + list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
> > > + generic_handle_irq(vmdirq->virq);
> > > + rcu_read_unlock();
> >
> > I'm not entirely sure that the rcu protection here covers the whole
> > virq thing when an interrupt is torn down. Can you please explain the
> > whole protection scheme in detail?
>
> This protects against device drivers manipulating the list through
> vmd_msi_activate/deactivate. We can't spinlock the flow handler access
> due to desc->lock taken through generic_handle_irq. The same lock is
> held prior to vmd's msi activate/deactivate, so the lock order would be
> opposite in those two paths.
>
> I don't believe we're in danger of missing a necessary event or triggering
> an unexpected event here. Is the concern that we need to synchronize rcu
> instead of directly freeing the "vmdirq"? I think I see that possibility.
synchronize rcu does not help. You surely need to kfree_rcu()
"vmdirq", but that alone is not sufficient. You also need to think
about the underlying irq descriptor, which is not freed via rcu. Lets
look at it:
CPU0 CPU1
driver_exit()
free_irq(D)
lock(irqdesc(D))
irq_shutdown(D)
irq_domain_deactivate_irq(D) vmd_flow_handler()
vmd_msi_deactivate() rcu_read_lock()
lock() vmdirq = list_entry(...)
list_del_rcu() ---> NMI
unlock()
unlock(irqdesc(D))
synchronize_irq(D)
pci_misx_disable(device)
irq_domain_free_irqs()
vmd_msi_free_irqs()
kfree_rcu(vmdirq) <--- NMI done
D = vmdirq->virq; <- Protected by RCU
generic_handle_irq(D)
desc = irqdesc(D)
irq_free_descs()
kfree(irqdesc(D))
desc->handle_irq(desc) <-- KABOOOOM!
Subtle, isn't it? It's extremly unlikely, but when it happens you have
no chance to ever debug that.
The good news for you is that this is a general issue and you can
ignore it for now. The bad news for me is, that I have another urgent
issue on my ever growing todo list.
> > > +static void vmd_remove(struct pci_dev *dev)
> > > +{
> > > + struct vmd_dev *vmd = pci_get_drvdata(dev);
> > > +
> > > + pci_set_drvdata(dev, NULL);
> > > + sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
> > > + pci_stop_root_bus(vmd->bus);
> > > + pci_remove_root_bus(vmd->bus);
> > > + vmd_teardown_dma_ops(vmd);
> > > + irq_domain_remove(vmd->irq_domain);
> >
> > What tears down the chained handlers and the MSIx entries?
>
> Thanks, I will fix the chained handler in both cases mentioned.
>
> The entries themselves should be freed through devres_release after this
> exits, and after MSI-x is disabled via pcim_release.
Ok.
Thanks,
tglx
On Tue, Nov 03, 2015 at 12:42:02PM +0100, Thomas Gleixner wrote:
> On Tue, 3 Nov 2015, Keith Busch wrote:
> > > > + msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
> > > > + vmd_irqdomain);
>
> But that parent limitation does not matter simply because your
> msi_irqdomain does not follow down the hierarchy in the allocation
> path.
>
> So we can avoid the vmd_irqdomain creation completely. It's just
> wasting memory and has no value at all. Creating the msi domain with a
> NULL parent is possible.
I'm having trouble following the hierarchy and didn't understand the
connection between the parent and msi comain. It's still new to me,
but I don't think a NULL parent is allowable with msi domains:
pci_msi_setup_msi_irqs()
pci_msi_domain_alloc_irqs()
msi_domain_alloc_irqs()
__irq_domain_alloc_irqs()
irq_domain_alloc_irqs_recursive()
msi_domain_alloc()
irq_domain_alloc_irqs_parent()
The last call returns -ENOSYS since there parent is NULL. Was the
intension to allow no parent, or do I still need to allocate one to
achieve the desired chaining?
On Wed, 4 Nov 2015, Keith Busch wrote:
> On Tue, Nov 03, 2015 at 12:42:02PM +0100, Thomas Gleixner wrote:
> > On Tue, 3 Nov 2015, Keith Busch wrote:
> > > > > + msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
> > > > > + vmd_irqdomain);
> >
> > But that parent limitation does not matter simply because your
> > msi_irqdomain does not follow down the hierarchy in the allocation
> > path.
> >
> > So we can avoid the vmd_irqdomain creation completely. It's just
> > wasting memory and has no value at all. Creating the msi domain with a
> > NULL parent is possible.
>
> I'm having trouble following the hierarchy and didn't understand the
> connection between the parent and msi comain. It's still new to me,
> but I don't think a NULL parent is allowable with msi domains:
>
> pci_msi_setup_msi_irqs()
> pci_msi_domain_alloc_irqs()
> msi_domain_alloc_irqs()
> __irq_domain_alloc_irqs()
> irq_domain_alloc_irqs_recursive()
> msi_domain_alloc()
> irq_domain_alloc_irqs_parent()
>
> The last call returns -ENOSYS since there parent is NULL. Was the
> intension to allow no parent, or do I still need to allocate one to
> achieve the desired chaining?
Hmm, seems I missed that part. But that's a fixable problem. Jiang?
Thanks,
tglx
On 2015/11/4 23:14, Thomas Gleixner wrote:
> On Wed, 4 Nov 2015, Keith Busch wrote:
>
>> On Tue, Nov 03, 2015 at 12:42:02PM +0100, Thomas Gleixner wrote:
>>> On Tue, 3 Nov 2015, Keith Busch wrote:
>>>>>> + msi_irqdomain = pci_msi_create_irq_domain(NULL, &pci_chained_msi_domain_info,
>>>>>> + vmd_irqdomain);
>>>
>>> But that parent limitation does not matter simply because your
>>> msi_irqdomain does not follow down the hierarchy in the allocation
>>> path.
>>>
>>> So we can avoid the vmd_irqdomain creation completely. It's just
>>> wasting memory and has no value at all. Creating the msi domain with a
>>> NULL parent is possible.
>>
>> I'm having trouble following the hierarchy and didn't understand the
>> connection between the parent and msi comain. It's still new to me,
>> but I don't think a NULL parent is allowable with msi domains:
>>
>> pci_msi_setup_msi_irqs()
>> pci_msi_domain_alloc_irqs()
>> msi_domain_alloc_irqs()
>> __irq_domain_alloc_irqs()
>> irq_domain_alloc_irqs_recursive()
>> msi_domain_alloc()
>> irq_domain_alloc_irqs_parent()
>>
>> The last call returns -ENOSYS since there parent is NULL. Was the
>> intension to allow no parent, or do I still need to allocate one to
>> achieve the desired chaining?
>
> Hmm, seems I missed that part. But that's a fixable problem. Jiang?
Hi Keith,
Could you please try the attached patch?
Thanks!
Gerry
>
> Thanks,
>
> tglx
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Thu, Nov 05, 2015 at 02:35:31PM +0800, Jiang Liu wrote:
> Hi Keith,
> Could you please try the attached patch?
> Thanks!
> Gerry
Thanks! I anticipated this and tested the same thing yesterday, and it
is successful.
I'll apply to the series and send a new revision hopefully today. Not
requiring a parent simplifies the driver since we don't define
irq_domain_ops now, and the functionality they provided moves to the
irq_chip.
> From: Liu Jiang <[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(-)
>
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 7e6512b9dc1f..e4d3d707efff 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -109,9 +109,11 @@ static int msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
> if (irq_find_mapping(domain, hwirq) > 0)
> return -EEXIST;
>
> - ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
> - if (ret < 0)
> - return ret;
> + if (domain->parent) {
> + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
> + if (ret < 0)
> + return ret;
> + }
>
> for (i = 0; i < nr_irqs; i++) {
> ret = ops->msi_init(domain, info, virq + i, hwirq + i, arg);
> --
> 1.7.10.4