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

[Manivannan Sadhasivam]

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

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