| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5a5cae94-b56c-4d4b-5ce5-516193e8962f@redhat.com>
Date: Tue, 7 Feb 2017 15:17:54 +0800
From: Jason Wang <jasowang@...hat.com>
To: Christoph Hellwig <hch@....de>, mst@...hat.com
Cc: axboe@...nel.dk, pbonzini@...hat.com,
virtualization@...ts.linux-foundation.org,
linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/9] virtio_pci: use shared interrupts for virtqueues
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@....de>
Reviewed-by: Jason Wang <jasowang@...hat.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)
> {
Powered by blists - more mailing lists