| Message ID | 20170205171526.6224-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017年02月06日 01:15, Christoph Hellwig wrote: > This lets IRQ layer handle dispatching IRQs to separate handlers for the > case where we don't have per-VQ MSI-X vectors, and allows us to greatly > simplify the code based on the assumption that we always have interrupt > vector 0 (legacy INTx or config interrupt for MSI-X) available, and > any other interrupt is request/freed throught the VQ, even if the > actual interrupt line might be shared in some cases. > > This allows removing a great deal of variables keeping track of the > interrupt state in struct virtio_pci_device, as we can now simply walk the > list of VQs and deal with per-VQ interrupt handlers there, and only treat > vector 0 special. > > Additionally clean up the VQ allocation code to properly unwind on error > instead of having a single global cleanup label, which is error prone, > and in this case also leads to more code. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Jason Wang <jasowang@redhat.com> > --- > drivers/virtio/virtio_pci_common.c | 235 ++++++++++++++++--------------------- > drivers/virtio/virtio_pci_common.h | 16 +-- > 2 files changed, 106 insertions(+), 145 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index a33767318cbf..274dc1ff09c0 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -33,10 +33,8 @@ void vp_synchronize_vectors(struct virtio_device *vdev) > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > int i; > > - if (vp_dev->intx_enabled) > - synchronize_irq(vp_dev->pci_dev->irq); > - > - for (i = 0; i < vp_dev->msix_vectors; ++i) > + synchronize_irq(pci_irq_vector(vp_dev->pci_dev, 0)); > + for (i = 1; i < vp_dev->msix_vectors; i++) > synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); > } > > @@ -99,77 +97,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque) > return vp_vring_interrupt(irq, opaque); > } > > -static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors, > - bool per_vq_vectors) > -{ > - struct virtio_pci_device *vp_dev = to_vp_device(vdev); > - const char *name = dev_name(&vp_dev->vdev.dev); > - unsigned i, v; > - int err = -ENOMEM; > - > - vp_dev->msix_vectors = nvectors; > - > - vp_dev->msix_names = kmalloc(nvectors * sizeof *vp_dev->msix_names, > - GFP_KERNEL); > - if (!vp_dev->msix_names) > - goto error; > - vp_dev->msix_affinity_masks > - = kzalloc(nvectors * sizeof *vp_dev->msix_affinity_masks, > - GFP_KERNEL); > - if (!vp_dev->msix_affinity_masks) > - goto error; > - for (i = 0; i < nvectors; ++i) > - if (!alloc_cpumask_var(&vp_dev->msix_affinity_masks[i], > - GFP_KERNEL)) > - goto error; > - > - err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors, > - PCI_IRQ_MSIX); > - if (err < 0) > - goto error; > - vp_dev->msix_enabled = 1; > - > - /* Set the vector used for configuration */ > - v = vp_dev->msix_used_vectors; > - snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names, > - "%s-config", name); > - err = request_irq(pci_irq_vector(vp_dev->pci_dev, v), > - vp_config_changed, 0, vp_dev->msix_names[v], > - vp_dev); > - if (err) > - goto error; > - ++vp_dev->msix_used_vectors; > - > - v = vp_dev->config_vector(vp_dev, v); > - /* Verify we had enough resources to assign the vector */ > - if (v == VIRTIO_MSI_NO_VECTOR) { > - err = -EBUSY; > - goto error; > - } > - > - if (!per_vq_vectors) { > - /* Shared vector for all VQs */ > - v = vp_dev->msix_used_vectors; > - snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names, > - "%s-virtqueues", name); > - err = request_irq(pci_irq_vector(vp_dev->pci_dev, v), > - vp_vring_interrupt, 0, vp_dev->msix_names[v], > - vp_dev); > - if (err) > - goto error; > - ++vp_dev->msix_used_vectors; > - } > - return 0; > -error: > - return err; > -} > - > -/* the config->del_vqs() implementation */ > -void vp_del_vqs(struct virtio_device *vdev) > +static void vp_remove_vqs(struct virtio_device *vdev) > { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > struct virtqueue *vq, *n; > - int i; > > list_for_each_entry_safe(vq, n, &vdev->vqs, list) { > if (vp_dev->msix_vector_map) { > @@ -181,35 +112,33 @@ void vp_del_vqs(struct virtio_device *vdev) > } > vp_dev->del_vq(vq); > } > +} > > - if (vp_dev->intx_enabled) { > - free_irq(vp_dev->pci_dev->irq, vp_dev); > - vp_dev->intx_enabled = 0; > - } > +/* the config->del_vqs() implementation */ > +void vp_del_vqs(struct virtio_device *vdev) > +{ > + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + int i; > > - for (i = 0; i < vp_dev->msix_used_vectors; ++i) > - free_irq(pci_irq_vector(vp_dev->pci_dev, i), vp_dev); > + if (WARN_ON_ONCE(list_empty_careful(&vdev->vqs))) > + return; > > - for (i = 0; i < vp_dev->msix_vectors; i++) > - if (vp_dev->msix_affinity_masks[i]) > - free_cpumask_var(vp_dev->msix_affinity_masks[i]); > + vp_remove_vqs(vdev); > > if (vp_dev->msix_enabled) { > + for (i = 0; i < vp_dev->msix_vectors; i++) > + free_cpumask_var(vp_dev->msix_affinity_masks[i]); > + > /* Disable the vector used for configuration */ > vp_dev->config_vector(vp_dev, VIRTIO_MSI_NO_VECTOR); > > - pci_free_irq_vectors(vp_dev->pci_dev); > - vp_dev->msix_enabled = 0; > + kfree(vp_dev->msix_affinity_masks); > + kfree(vp_dev->msix_names); > + kfree(vp_dev->msix_vector_map); > } > > - vp_dev->msix_vectors = 0; > - vp_dev->msix_used_vectors = 0; > - kfree(vp_dev->msix_names); > - vp_dev->msix_names = NULL; > - kfree(vp_dev->msix_affinity_masks); > - vp_dev->msix_affinity_masks = NULL; > - kfree(vp_dev->msix_vector_map); > - vp_dev->msix_vector_map = NULL; > + free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev); > + pci_free_irq_vectors(vp_dev->pci_dev); > } > > static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs, > @@ -219,79 +148,122 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs, > bool per_vq_vectors) > { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + const char *name = dev_name(&vp_dev->vdev.dev); > + int i, err = -ENOMEM, allocated_vectors, nvectors; > u16 msix_vec; > - int i, err, nvectors, allocated_vectors; > + > + nvectors = 1; > + for (i = 0; i < nvqs; i++) > + if (callbacks[i]) > + nvectors++; > > if (per_vq_vectors) { > - /* Best option: one for change interrupt, one per vq. */ > - nvectors = 1; > - for (i = 0; i < nvqs; ++i) > - if (callbacks[i]) > - ++nvectors; > + err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors, > + PCI_IRQ_MSIX); > } else { > - /* Second best: one for change, shared for all vqs. */ > - nvectors = 2; > + err = pci_alloc_irq_vectors(vp_dev->pci_dev, 2, 2, > + PCI_IRQ_MSIX); > } > + if (err < 0) > + return err; > > - err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors); > + vp_dev->msix_vectors = nvectors; > + vp_dev->msix_names = kmalloc_array(nvectors, > + sizeof(*vp_dev->msix_names), GFP_KERNEL); > + if (!vp_dev->msix_names) > + goto out_free_irq_vectors; > + > + vp_dev->msix_affinity_masks = kcalloc(nvectors, > + sizeof(*vp_dev->msix_affinity_masks), GFP_KERNEL); > + if (!vp_dev->msix_affinity_masks) > + goto out_free_msix_names; > + > + for (i = 0; i < nvectors; ++i) { > + if (!alloc_cpumask_var(&vp_dev->msix_affinity_masks[i], > + GFP_KERNEL)) > + goto out_free_msix_affinity_masks; > + } > + > + /* Set the vector used for configuration */ > + snprintf(vp_dev->msix_names[0], sizeof(*vp_dev->msix_names), > + "%s-config", name); > + err = request_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_config_changed, > + 0, vp_dev->msix_names[0], vp_dev); > if (err) > - goto error_find; > + goto out_free_irq_vectors; > > - if (per_vq_vectors) { > - vp_dev->msix_vector_map = kmalloc_array(nvqs, > - sizeof(*vp_dev->msix_vector_map), GFP_KERNEL); > - if (!vp_dev->msix_vector_map) > - goto error_find; > + /* Verify we had enough resources to assign the vector */ > + if (vp_dev->config_vector(vp_dev, 0) == VIRTIO_MSI_NO_VECTOR) { > + err = -EBUSY; > + goto out_free_config_irq; > } > > - allocated_vectors = vp_dev->msix_used_vectors; > + vp_dev->msix_vector_map = kmalloc_array(nvqs, > + sizeof(*vp_dev->msix_vector_map), GFP_KERNEL); > + if (!vp_dev->msix_vector_map) > + goto out_disable_config_irq; > + > + allocated_vectors = 1; /* vector 0 is the config interrupt */ > for (i = 0; i < nvqs; ++i) { > if (!names[i]) { > vqs[i] = NULL; > continue; > } > > - if (!callbacks[i]) > - msix_vec = VIRTIO_MSI_NO_VECTOR; > - else if (per_vq_vectors) > - msix_vec = allocated_vectors++; > + if (callbacks[i]) > + msix_vec = allocated_vectors; > else > - msix_vec = VP_MSIX_VQ_VECTOR; > + msix_vec = VIRTIO_MSI_NO_VECTOR; > + > vqs[i] = vp_dev->setup_vq(vp_dev, i, callbacks[i], names[i], > msix_vec); > if (IS_ERR(vqs[i])) { > err = PTR_ERR(vqs[i]); > - goto error_find; > + goto out_remove_vqs; > } > > - if (!per_vq_vectors) > - continue; > - > if (msix_vec == VIRTIO_MSI_NO_VECTOR) { > vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR; > continue; > } > > - /* allocate per-vq irq if available and necessary */ > - snprintf(vp_dev->msix_names[msix_vec], > - sizeof *vp_dev->msix_names, > - "%s-%s", > + snprintf(vp_dev->msix_names[i + 1], > + sizeof(*vp_dev->msix_names), "%s-%s", > dev_name(&vp_dev->vdev.dev), names[i]); > err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec), > - vring_interrupt, 0, > - vp_dev->msix_names[msix_vec], > - vqs[i]); > + vring_interrupt, IRQF_SHARED, > + vp_dev->msix_names[i + 1], vqs[i]); > if (err) { > /* don't free this irq on error */ > vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR; > - goto error_find; > + goto out_remove_vqs; > } > vp_dev->msix_vector_map[i] = msix_vec; > + > + if (per_vq_vectors) > + allocated_vectors++; > } > + > + vp_dev->msix_enabled = 1; > return 0; > > -error_find: > - vp_del_vqs(vdev); > +out_remove_vqs: > + vp_remove_vqs(vdev); > + kfree(vp_dev->msix_vector_map); > +out_disable_config_irq: > + vp_dev->config_vector(vp_dev, VIRTIO_MSI_NO_VECTOR); > +out_free_config_irq: > + free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev); > +out_free_msix_affinity_masks: > + for (i = 0; i < nvectors; i++) { > + if (vp_dev->msix_affinity_masks[i]) > + free_cpumask_var(vp_dev->msix_affinity_masks[i]); > + } > + kfree(vp_dev->msix_affinity_masks); > +out_free_msix_names: > + kfree(vp_dev->msix_names); > +out_free_irq_vectors: > + pci_free_irq_vectors(vp_dev->pci_dev); > return err; > } > > @@ -305,9 +277,8 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs, > err = request_irq(vp_dev->pci_dev->irq, vp_interrupt, IRQF_SHARED, > dev_name(&vdev->dev), vp_dev); > if (err) > - goto out_del_vqs; > + return err; > > - vp_dev->intx_enabled = 1; > for (i = 0; i < nvqs; ++i) { > if (!names[i]) { > vqs[i] = NULL; > @@ -317,13 +288,15 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs, > VIRTIO_MSI_NO_VECTOR); > if (IS_ERR(vqs[i])) { > err = PTR_ERR(vqs[i]); > - goto out_del_vqs; > + goto out_remove_vqs; > } > } > > return 0; > -out_del_vqs: > - vp_del_vqs(vdev); > + > +out_remove_vqs: > + vp_remove_vqs(vdev); > + free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev); > return err; > } > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > index 2038887bdf23..85593867e712 100644 > --- a/drivers/virtio/virtio_pci_common.h > +++ b/drivers/virtio/virtio_pci_common.h > @@ -66,16 +66,12 @@ struct virtio_pci_device { > > /* MSI-X support */ > int msix_enabled; > - int intx_enabled; > cpumask_var_t *msix_affinity_masks; > /* Name strings for interrupts. This size should be enough, > * and I'm too lazy to allocate each name separately. */ > char (*msix_names)[256]; > - /* Number of available vectors */ > - unsigned msix_vectors; > - /* Vectors allocated, excluding per-vq vectors if any */ > - unsigned msix_used_vectors; > - > + /* Total Number of MSI-X vectors (including per-VQ ones). */ > + int msix_vectors; > /* Map of per-VQ MSI-X vectors, may be NULL */ > unsigned *msix_vector_map; > > @@ -89,14 +85,6 @@ struct virtio_pci_device { > u16 (*config_vector)(struct virtio_pci_device *vp_dev, u16 vector); > }; > > -/* Constants for MSI-X */ > -/* Use first vector for configuration changes, second and the rest for > - * virtqueues Thus, we need at least 2 vectors for MSI. */ > -enum { > - VP_MSIX_CONFIG_VECTOR = 0, > - VP_MSIX_VQ_VECTOR = 1, > -}; > - > /* Convert a generic virtio device to our structure */ > static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) > {
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index a33767318cbf..274dc1ff09c0 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -33,10 +33,8 @@ void vp_synchronize_vectors(struct virtio_device *vdev) struct virtio_pci_device *vp_dev = to_vp_device(vdev); int i; - if (vp_dev->intx_enabled) - synchronize_irq(vp_dev->pci_dev->irq); - - for (i = 0; i < vp_dev->msix_vectors; ++i) + synchronize_irq(pci_irq_vector(vp_dev->pci_dev, 0)); + for (i = 1; i < vp_dev->msix_vectors; i++) synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); } @@ -99,77 +97,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque) return vp_vring_interrupt(irq, opaque); } -static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors, - bool per_vq_vectors) -{ - struct virtio_pci_device *vp_dev = to_vp_device(vdev); - const char *name = dev_name(&vp_dev->vdev.dev); - unsigned i, v; - int err = -ENOMEM; - - vp_dev->msix_vectors = nvectors; - - vp_dev->msix_names = kmalloc(nvectors * sizeof *vp_dev->msix_names, - GFP_KERNEL); - if (!vp_dev->msix_names) - goto error; - vp_dev->msix_affinity_masks - = kzalloc(nvectors * sizeof *vp_dev->msix_affinity_masks, - GFP_KERNEL); - if (!vp_dev->msix_affinity_masks) - goto error; - for (i = 0; i < nvectors; ++i) - if (!alloc_cpumask_var(&vp_dev->msix_affinity_masks[i], - GFP_KERNEL)) - goto error; - - err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors, - PCI_IRQ_MSIX); - if (err < 0) - goto error; - vp_dev->msix_enabled = 1; - - /* Set the vector used for configuration */ - v = vp_dev->msix_used_vectors; - snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names, - "%s-config", name); - err = request_irq(pci_irq_vector(vp_dev->pci_dev, v), - vp_config_changed, 0, vp_dev->msix_names[v], - vp_dev); - if (err) - goto error; - ++vp_dev->msix_used_vectors; - - v = vp_dev->config_vector(vp_dev, v); - /* Verify we had enough resources to assign the vector */ - if (v == VIRTIO_MSI_NO_VECTOR) { - err = -EBUSY; - goto error; - } - - if (!per_vq_vectors) { - /* Shared vector for all VQs */ - v = vp_dev->msix_used_vectors; - snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names, - "%s-virtqueues", name); - err = request_irq(pci_irq_vector(vp_dev->pci_dev, v), - vp_vring_interrupt, 0, vp_dev->msix_names[v], - vp_dev); - if (err) - goto error; - ++vp_dev->msix_used_vectors; - } - return 0; -error: - return err; -} - -/* the config->del_vqs() implementation */ -void vp_del_vqs(struct virtio_device *vdev) +static void vp_remove_vqs(struct virtio_device *vdev) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); struct virtqueue *vq, *n; - int i; list_for_each_entry_safe(vq, n, &vdev->vqs, list) { if (vp_dev->msix_vector_map) { @@ -181,35 +112,33 @@ void vp_del_vqs(struct virtio_device *vdev) } vp_dev->del_vq(vq); } +} - if (vp_dev->intx_enabled) { - free_irq(vp_dev->pci_dev->irq, vp_dev); - vp_dev->intx_enabled = 0; - } +/* the config->del_vqs() implementation */ +void vp_del_vqs(struct virtio_device *vdev) +{ + struct virtio_pci_device *vp_dev = to_vp_device(vdev); + int i; - for (i = 0; i < vp_dev->msix_used_vectors; ++i) - free_irq(pci_irq_vector(vp_dev->pci_dev, i), vp_dev); + if (WARN_ON_ONCE(list_empty_careful(&vdev->vqs))) + return; - for (i = 0; i < vp_dev->msix_vectors; i++) - if (vp_dev->msix_affinity_masks[i]) - free_cpumask_var(vp_dev->msix_affinity_masks[i]); + vp_remove_vqs(vdev); if (vp_dev->msix_enabled) { + for (i = 0; i < vp_dev->msix_vectors; i++) + free_cpumask_var(vp_dev->msix_affinity_masks[i]); + /* Disable the vector used for configuration */ vp_dev->config_vector(vp_dev, VIRTIO_MSI_NO_VECTOR); - pci_free_irq_vectors(vp_dev->pci_dev); - vp_dev->msix_enabled = 0; + kfree(vp_dev->msix_affinity_masks); + kfree(vp_dev->msix_names); + kfree(vp_dev->msix_vector_map); } - vp_dev->msix_vectors = 0; - vp_dev->msix_used_vectors = 0; - kfree(vp_dev->msix_names); - vp_dev->msix_names = NULL; - kfree(vp_dev->msix_affinity_masks); - vp_dev->msix_affinity_masks = NULL; - kfree(vp_dev->msix_vector_map); - vp_dev->msix_vector_map = NULL; + free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev); + pci_free_irq_vectors(vp_dev->pci_dev); } static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs, @@ -219,79 +148,122 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs, bool per_vq_vectors) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); + const char *name = dev_name(&vp_dev->vdev.dev); + int i, err = -ENOMEM, allocated_vectors, nvectors; u16 msix_vec; - int i, err, nvectors, allocated_vectors; + + nvectors = 1; + for (i = 0; i < nvqs; i++) + if (callbacks[i]) + nvectors++; if (per_vq_vectors) { - /* Best option: one for change interrupt, one per vq. */ - nvectors = 1; - for (i = 0; i < nvqs; ++i) - if (callbacks[i]) - ++nvectors; + err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors, + PCI_IRQ_MSIX); } else { - /* Second best: one for change, shared for all vqs. */ - nvectors = 2; + err = pci_alloc_irq_vectors(vp_dev->pci_dev, 2, 2, + PCI_IRQ_MSIX); } + if (err < 0) + return err; - err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors); + vp_dev->msix_vectors = nvectors; + vp_dev->msix_names = kmalloc_array(nvectors, + sizeof(*vp_dev->msix_names), GFP_KERNEL); + if (!vp_dev->msix_names) + goto out_free_irq_vectors; + + vp_dev->msix_affinity_masks = kcalloc(nvectors, + sizeof(*vp_dev->msix_affinity_masks), GFP_KERNEL); + if (!vp_dev->msix_affinity_masks) + goto out_free_msix_names; + + for (i = 0; i < nvectors; ++i) { + if (!alloc_cpumask_var(&vp_dev->msix_affinity_masks[i], + GFP_KERNEL)) + goto out_free_msix_affinity_masks; + } + + /* Set the vector used for configuration */ + snprintf(vp_dev->msix_names[0], sizeof(*vp_dev->msix_names), + "%s-config", name); + err = request_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_config_changed, + 0, vp_dev->msix_names[0], vp_dev); if (err) - goto error_find; + goto out_free_irq_vectors; - if (per_vq_vectors) { - vp_dev->msix_vector_map = kmalloc_array(nvqs, - sizeof(*vp_dev->msix_vector_map), GFP_KERNEL); - if (!vp_dev->msix_vector_map) - goto error_find; + /* Verify we had enough resources to assign the vector */ + if (vp_dev->config_vector(vp_dev, 0) == VIRTIO_MSI_NO_VECTOR) { + err = -EBUSY; + goto out_free_config_irq; } - allocated_vectors = vp_dev->msix_used_vectors; + vp_dev->msix_vector_map = kmalloc_array(nvqs, + sizeof(*vp_dev->msix_vector_map), GFP_KERNEL); + if (!vp_dev->msix_vector_map) + goto out_disable_config_irq; + + allocated_vectors = 1; /* vector 0 is the config interrupt */ for (i = 0; i < nvqs; ++i) { if (!names[i]) { vqs[i] = NULL; continue; } - if (!callbacks[i]) - msix_vec = VIRTIO_MSI_NO_VECTOR; - else if (per_vq_vectors) - msix_vec = allocated_vectors++; + if (callbacks[i]) + msix_vec = allocated_vectors; else - msix_vec = VP_MSIX_VQ_VECTOR; + msix_vec = VIRTIO_MSI_NO_VECTOR; + vqs[i] = vp_dev->setup_vq(vp_dev, i, callbacks[i], names[i], msix_vec); if (IS_ERR(vqs[i])) { err = PTR_ERR(vqs[i]); - goto error_find; + goto out_remove_vqs; } - if (!per_vq_vectors) - continue; - if (msix_vec == VIRTIO_MSI_NO_VECTOR) { vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR; continue; } - /* allocate per-vq irq if available and necessary */ - snprintf(vp_dev->msix_names[msix_vec], - sizeof *vp_dev->msix_names, - "%s-%s", + snprintf(vp_dev->msix_names[i + 1], + sizeof(*vp_dev->msix_names), "%s-%s", dev_name(&vp_dev->vdev.dev), names[i]); err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec), - vring_interrupt, 0, - vp_dev->msix_names[msix_vec], - vqs[i]); + vring_interrupt, IRQF_SHARED, + vp_dev->msix_names[i + 1], vqs[i]); if (err) { /* don't free this irq on error */ vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR; - goto error_find; + goto out_remove_vqs; } vp_dev->msix_vector_map[i] = msix_vec; + + if (per_vq_vectors) + allocated_vectors++; } + + vp_dev->msix_enabled = 1; return 0; -error_find: - vp_del_vqs(vdev); +out_remove_vqs: + vp_remove_vqs(vdev); + kfree(vp_dev->msix_vector_map); +out_disable_config_irq: + vp_dev->config_vector(vp_dev, VIRTIO_MSI_NO_VECTOR); +out_free_config_irq: + free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev); +out_free_msix_affinity_masks: + for (i = 0; i < nvectors; i++) { + if (vp_dev->msix_affinity_masks[i]) + free_cpumask_var(vp_dev->msix_affinity_masks[i]); + } + kfree(vp_dev->msix_affinity_masks); +out_free_msix_names: + kfree(vp_dev->msix_names); +out_free_irq_vectors: + pci_free_irq_vectors(vp_dev->pci_dev); return err; } @@ -305,9 +277,8 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs, err = request_irq(vp_dev->pci_dev->irq, vp_interrupt, IRQF_SHARED, dev_name(&vdev->dev), vp_dev); if (err) - goto out_del_vqs; + return err; - vp_dev->intx_enabled = 1; for (i = 0; i < nvqs; ++i) { if (!names[i]) { vqs[i] = NULL; @@ -317,13 +288,15 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs, VIRTIO_MSI_NO_VECTOR); if (IS_ERR(vqs[i])) { err = PTR_ERR(vqs[i]); - goto out_del_vqs; + goto out_remove_vqs; } } return 0; -out_del_vqs: - vp_del_vqs(vdev); + +out_remove_vqs: + vp_remove_vqs(vdev); + free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev); return err; } diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h index 2038887bdf23..85593867e712 100644 --- a/drivers/virtio/virtio_pci_common.h +++ b/drivers/virtio/virtio_pci_common.h @@ -66,16 +66,12 @@ struct virtio_pci_device { /* MSI-X support */ int msix_enabled; - int intx_enabled; cpumask_var_t *msix_affinity_masks; /* Name strings for interrupts. This size should be enough, * and I'm too lazy to allocate each name separately. */ char (*msix_names)[256]; - /* Number of available vectors */ - unsigned msix_vectors; - /* Vectors allocated, excluding per-vq vectors if any */ - unsigned msix_used_vectors; - + /* Total Number of MSI-X vectors (including per-VQ ones). */ + int msix_vectors; /* Map of per-VQ MSI-X vectors, may be NULL */ unsigned *msix_vector_map; @@ -89,14 +85,6 @@ struct virtio_pci_device { u16 (*config_vector)(struct virtio_pci_device *vp_dev, u16 vector); }; -/* Constants for MSI-X */ -/* Use first vector for configuration changes, second and the rest for - * virtqueues Thus, we need at least 2 vectors for MSI. */ -enum { - VP_MSIX_CONFIG_VECTOR = 0, - VP_MSIX_VQ_VECTOR = 1, -}; - /* Convert a generic virtio device to our structure */ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) {
This lets IRQ layer handle dispatching IRQs to separate handlers for the case where we don't have per-VQ MSI-X vectors, and allows us to greatly simplify the code based on the assumption that we always have interrupt vector 0 (legacy INTx or config interrupt for MSI-X) available, and any other interrupt is request/freed throught the VQ, even if the actual interrupt line might be shared in some cases. This allows removing a great deal of variables keeping track of the interrupt state in struct virtio_pci_device, as we can now simply walk the list of VQs and deal with per-VQ interrupt handlers there, and only treat vector 0 special. Additionally clean up the VQ allocation code to properly unwind on error instead of having a single global cleanup label, which is error prone, and in this case also leads to more code. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/virtio/virtio_pci_common.c | 235 ++++++++++++++++--------------------- drivers/virtio/virtio_pci_common.h | 16 +-- 2 files changed, 106 insertions(+), 145 deletions(-)