Re: [PATCH] virtio-pci: Add MSI support

From: Manivannan Sadhasivam
Date: Fri Jul 12 2024 - 10:32:22 EST


+ Jason Wang (email got truncated).

On Fri, Jul 12, 2024 at 07:59:14PM +0530, Manivannan Sadhasivam wrote:
> Virtio spec has so far only supported MSI-X and INTX for receiving the
> interrupts from the virtio device on PCI transport. But this becomes a
> limiting factor for devices supporting only MSI (plus INTX emulation) as
> they have to use the legacy INTX emulation which is limited to one IRQ per
> PCIe function.
>
> But this now addressed with the help of a proposal to the Virtio spec
> adding MSI support [1]. Based on that, let's implement MSI support in the
> virtio-pci driver.
>
> The Virtio spec proposal reuses the existing MSI-X infrastructure, like the
> config_msix_vector/queue_msix_vector fields of the Virito common config
> structure. Following that, MSI support in virtio-pci driver is also added
> on top of the existing MSI-X implementation and it mostly reuses the MSI-X
> code base. The existing vp_find_vqs_msix() API is modified to support MSI
> along with MSI-X.
>
> The preference for interrupt allocation is still given to MSI-X as per the
> spec. The driver will try to allocate MSI only if both of the MSI-X
> allocations (one vector for each queue and 2 vectors) fails. As like MSI-X,
> driver will try to allocate one MSI vector for each queue first, and if
> that fails, it will try to allocate 2 vectors (one for config queue and one
> shared for queues). If both of them fails, driver will fallback to the
> legacy INTX as usual.
>
> For keeping the changes minimal, existing 'virtio_pci_device::msix_enabled'
> flag is used to indicate the status of MSI and MSI-X. Rest of the MSI-X
> functionalities such as IRQ affinity are also reused for MSI (but the
> affinity setting really depends on the underlying IRQCHIP controller).
>
> [1] https://lore.kernel.org/virtio-comment/20240712140144.12066-1-manivannan.sadhasivam@xxxxxxxxxx/
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> ---
> drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++------
> drivers/virtio/virtio_pci_common.h | 2 +-
> 2 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index f6b0b00e4599..6f80b0c46c5f 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -100,11 +100,11 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> }
>
> static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> - bool per_vq_vectors, struct irq_affinity *desc)
> + bool per_vq_vectors, struct irq_affinity *desc,
> + unsigned int flags)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> const char *name = dev_name(&vp_dev->vdev.dev);
> - unsigned int flags = PCI_IRQ_MSIX;
> unsigned int i, v;
> int err = -ENOMEM;
>
> @@ -288,7 +288,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> struct virtqueue *vqs[], vq_callback_t *callbacks[],
> const char * const names[], bool per_vq_vectors,
> const bool *ctx,
> - struct irq_affinity *desc)
> + struct irq_affinity *desc, unsigned int flags)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> u16 msix_vec;
> @@ -310,7 +310,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> }
>
> err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors,
> - per_vq_vectors ? desc : NULL);
> + per_vq_vectors ? desc : NULL, flags);
> if (err)
> goto error_find;
>
> @@ -407,11 +407,23 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> int err;
>
> /* Try MSI-X with one vector per queue. */
> - err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, true, ctx, desc);
> + err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, true, ctx,
> + desc, PCI_IRQ_MSIX);
> if (!err)
> return 0;
> /* Fallback: MSI-X with one vector for config, one shared for queues. */
> - err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx, desc);
> + err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx,
> + desc, PCI_IRQ_MSIX);
> + if (!err)
> + return 0;
> + /* Try MSI with one vector per queue. */
> + err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, true, ctx,
> + desc, PCI_IRQ_MSI);
> + if (!err)
> + return 0;
> + /* Fallback: MSI with one vector for config, one shared for queues. */
> + err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx,
> + desc, PCI_IRQ_MSI);
> if (!err)
> return 0;
> /* Is there an interrupt? If not give up. */
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 7fef52bee455..a5062ca85f3b 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -77,7 +77,7 @@ struct virtio_pci_device {
>
> struct virtio_pci_admin_vq admin_vq;
>
> - /* MSI-X support */
> + /* MSI/MSI-X support */
> int msix_enabled;
> int intx_enabled;
> cpumask_var_t *msix_affinity_masks;
> --
> 2.25.1
>

--
மணிவண்ணன் சதாசிவம்