qemu-kvm missing some msix capability check

qemu-kvm missing some msix capability check

[Amit Shah]

Hello,

Using recent qemu-kvm userspace with a slightly older kernel module I
get this when using the virtio-net device:

kvm_msix_add: kvm_get_irq_route_gsi failed: No space left on device

... and the guest doesn't use the net device.

This goes away when using a newer kvm module.

		Amit

Re: qemu-kvm missing some msix capability check

[Michael S. Tsirkin]

On Fri, Jul 17, 2009 at 06:34:40PM +0530, Amit Shah wrote:
> Hello,
> 
> Using recent qemu-kvm userspace with a slightly older kernel module I
> get this when using the virtio-net device:
> 
> kvm_msix_add: kvm_get_irq_route_gsi failed: No space left on device
> 
> ... and the guest doesn't use the net device.
> 
> This goes away when using a newer kvm module.
> 
> 		Amit

Could you verify if the following helps please?

---

virtio: retry on vector assignment failure

virtio currently fails to find any vqs if the device supports msi-x but
fails to assign it to a queue.  Turns out, this actually happens for old
host kernels which can't allocate a gsi for the vector. As a result
guest does not use virtio net.  Fix this by disabling msi on
such failures and falling back on regular interrupts.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 9dcc368..567c972 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -273,26 +273,35 @@ static void vp_free_vectors(struct virtio_device *vdev)
 }
 
 static int vp_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
-			  int *options, int noptions)
+			  int nvectors)
 {
-	int i;
-	for (i = 0; i < noptions; ++i)
-		if (!pci_enable_msix(dev, entries, options[i]))
-			return options[i];
-	return -EBUSY;
+	int err = pci_enable_msix(dev, entries, nvectors);
+	if (err > 0)
+		err = -ENOSPC;
+	return err;
 }
 
-static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
+static int vp_request_irq(struct virtio_device *vdev)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	int err;
+	/* Can't allocate enough MSI-X vectors, use regular interrupt */
+	vp_dev->msix_vectors = 0;
+	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
+			  IRQF_SHARED, dev_name(&vp_dev->vdev.dev), vp_dev);
+	if (err)
+		return err;
+	vp_dev->intx_enabled = 1;
+	return 0;
+}
+
+static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs,
+			      int nvectors)
 {
 	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;
-	/* We want at most one vector per queue and one for config changes.
-	 * Fallback to separate vectors for config and a shared for queues.
-	 * Finally fall back to regular interrupts. */
-	int options[] = { max_vqs + 1, 2 };
-	int nvectors = max(options[0], options[1]);
 
 	vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
 				       GFP_KERNEL);
@@ -307,37 +316,29 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 		vp_dev->msix_entries[i].entry = i;
 
 	err = vp_enable_msix(vp_dev->pci_dev, vp_dev->msix_entries,
-			     options, ARRAY_SIZE(options));
-	if (err < 0) {
-		/* Can't allocate enough MSI-X vectors, use regular interrupt */
-		vp_dev->msix_vectors = 0;
-		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
-				  IRQF_SHARED, name, vp_dev);
-		if (err)
-			goto error_irq;
-		vp_dev->intx_enabled = 1;
-	} else {
-		vp_dev->msix_vectors = err;
-		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(vp_dev->msix_entries[v].vector,
-				  vp_config_changed, 0, vp_dev->msix_names[v],
-				  vp_dev);
-		if (err)
-			goto error_irq;
-		++vp_dev->msix_used_vectors;
-
-		iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-		/* Verify we had enough resources to assign the vector */
-		v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-		if (v == VIRTIO_MSI_NO_VECTOR) {
-			err = -EBUSY;
-			goto error_irq;
-		}
+			     nvectors);
+	if (err)
+		goto error_enable;
+	vp_dev->msix_vectors = nvectors;
+	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(vp_dev->msix_entries[v].vector,
+			  vp_config_changed, 0, vp_dev->msix_names[v],
+			  vp_dev);
+	if (err)
+		goto error_irq;
+	++vp_dev->msix_used_vectors;
+
+	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
+	/* Verify we had enough resources to assign the vector */
+	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
+	if (v == VIRTIO_MSI_NO_VECTOR) {
+		err = -EBUSY;
+		goto error_irq;
 	}
 
 	if (vp_dev->msix_vectors && vp_dev->msix_vectors != max_vqs + 1) {
@@ -355,9 +356,12 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 	return 0;
 error_irq:
 	vp_free_vectors(vdev);
+error_enable:
 	kfree(vp_dev->msix_names);
+	vp_dev->msix_names = NULL;
 error_names:
 	kfree(vp_dev->msix_entries);
+	vp_dev->msix_entries = NULL;
 error_entries:
 	return err;
 }
@@ -499,24 +503,24 @@ static void vp_del_vqs(struct virtio_device *vdev)
 
 	vp_free_vectors(vdev);
 	kfree(vp_dev->msix_names);
+	vp_dev->msix_names = NULL;
 	kfree(vp_dev->msix_entries);
+	vp_dev->msix_entries = NULL;
 }
 
-/* the config->find_vqs() implementation */
-static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
-		       struct virtqueue *vqs[],
-		       vq_callback_t *callbacks[],
-		       const char *names[])
+static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
+			      struct virtqueue *vqs[],
+			      vq_callback_t *callbacks[],
+			      const char *names[],
+			      int max_vectors,
+			      int nvectors)
 {
-	int vectors = 0;
 	int i, err;
 
-	/* How many vectors would we like? */
-	for (i = 0; i < nvqs; ++i)
-		if (callbacks[i])
-			++vectors;
-
-	err = vp_request_vectors(vdev, vectors);
+	if (nvectors)
+		err = vp_request_vectors(vdev, max_vectors, nvectors);
+	else
+		err = vp_request_irq(vdev);
 	if (err)
 		goto error_request;
 
@@ -534,6 +538,34 @@ error_request:
 	return PTR_ERR(vqs[i]);
 }
 
+/* the config->find_vqs() implementation */
+static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
+		       struct virtqueue *vqs[],
+		       vq_callback_t *callbacks[],
+		       const char *names[])
+{
+	/* We want at most one vector per queue and one for config changes.
+	 * Fallback to separate vectors for config and a shared for queues.
+	 * Finally fall back to regular interrupts. */
+	int options[] = { 1, 2, 0 };
+	int vectors = 0;
+	int i, uninitialized_var(err);
+
+	/* How many vectors would we like? */
+	for (i = 0; i < nvqs; ++i)
+		if (callbacks[i])
+			++vectors;
+	options[0] = vectors + 1;
+
+	for (i = 0; i < ARRAY_SIZE(options); ++i) {
+		err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+					 vectors, options[i]);
+		if (!err)
+			break;
+	}
+	return err;
+}
+
 static struct virtio_config_ops virtio_pci_config_ops = {
 	.get		= vp_get,
 	.set		= vp_set,

Re: qemu-kvm missing some msix capability check

[Amit Shah]

On (Tue) Jul 21 2009 [19:54:00], Michael S. Tsirkin wrote:
> On Fri, Jul 17, 2009 at 06:34:40PM +0530, Amit Shah wrote:
> > Hello,
> > 
> > Using recent qemu-kvm userspace with a slightly older kernel module I
> > get this when using the virtio-net device:
> > 
> > kvm_msix_add: kvm_get_irq_route_gsi failed: No space left on device
> > 
> > ... and the guest doesn't use the net device.
> > 
> > This goes away when using a newer kvm module.
> > 
> > 		Amit
> 
> Could you verify if the following helps please?

What is this based on? Fails to apply on kvm/master.

		Amit

Re: qemu-kvm missing some msix capability check

[Michael S. Tsirkin]

On Tue, Jul 21, 2009 at 10:42:19PM +0530, Amit Shah wrote:
> On (Tue) Jul 21 2009 [19:54:00], Michael S. Tsirkin wrote:
> > On Fri, Jul 17, 2009 at 06:34:40PM +0530, Amit Shah wrote:
> > > Hello,
> > > 
> > > Using recent qemu-kvm userspace with a slightly older kernel module I
> > > get this when using the virtio-net device:
> > > 
> > > kvm_msix_add: kvm_get_irq_route_gsi failed: No space left on device
> > > 
> > > ... and the guest doesn't use the net device.
> > > 
> > > This goes away when using a newer kvm module.
> > > 
> > > 		Amit
> > 
> > Could you verify if the following helps please?
> 
> What is this based on? Fails to apply on kvm/master.
> 
> 		Amit

84a3c0818fe9d7a1e34c188d6182793f213a6a66

Re: qemu-kvm missing some msix capability check

[Michael S. Tsirkin]

On Tue, Jul 21, 2009 at 10:42:19PM +0530, Amit Shah wrote:
> On (Tue) Jul 21 2009 [19:54:00], Michael S. Tsirkin wrote:
> > On Fri, Jul 17, 2009 at 06:34:40PM +0530, Amit Shah wrote:
> > > Hello,
> > > 
> > > Using recent qemu-kvm userspace with a slightly older kernel module I
> > > get this when using the virtio-net device:
> > > 
> > > kvm_msix_add: kvm_get_irq_route_gsi failed: No space left on device
> > > 
> > > ... and the guest doesn't use the net device.
> > > 
> > > This goes away when using a newer kvm module.
> > > 
> > > 		Amit
> > 
> > Could you verify if the following helps please?
> 
> What is this based on? Fails to apply on kvm/master.
> 
> 		Amit

Sorry, this is on top of 2 patches I just posted that fix othe rbugs in
virtio.

Re: qemu-kvm missing some msix capability check

[Amit Shah]

On (Tue) Jul 21 2009 [19:54:00], Michael S. Tsirkin wrote:
> On Fri, Jul 17, 2009 at 06:34:40PM +0530, Amit Shah wrote:
> > Hello,
> > 
> > Using recent qemu-kvm userspace with a slightly older kernel module I
> > get this when using the virtio-net device:

I was getting this with a very recent kernel in the guest with an older
host kernel.

2.6.31-rc3 in the guest and 2.6.29 (F11) kernel in the host.

> > kvm_msix_add: kvm_get_irq_route_gsi failed: No space left on device
> > 
> > ... and the guest doesn't use the net device.
> > 
> > This goes away when using a newer kvm module.
> > 
> > 		Amit
> 
> Could you verify if the following helps please?

This worked for me with your other patches:

guest kernel (on top of Avi's kvm/master):
1. [PATCH 1/2] virtio: Fix memory leak on device removal
2. [PATCH 2/2] virtio: fix double free_irq]

qemu-kvm (on top of Avi's qemu-kvm/master):
1. [PATCHv2] qemu-kvm: routing table update thinko fix
2. [PATCH 2/2] qemu-kvm: broken MSI routing work-around
3. [PATCH] qemu-kvm: fix error handling in msix vector add

Tested-by: Amit Shah <amit.shah@redhat.com>

> ---
> 
> virtio: retry on vector assignment failure
> 
> virtio currently fails to find any vqs if the device supports msi-x but
> fails to assign it to a queue.  Turns out, this actually happens for old
> host kernels which can't allocate a gsi for the vector. As a result
> guest does not use virtio net.  Fix this by disabling msi on
> such failures and falling back on regular interrupts.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 9dcc368..567c972 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -273,26 +273,35 @@ static void vp_free_vectors(struct virtio_device *vdev)
>  }
>  
>  static int vp_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
> -			  int *options, int noptions)
> +			  int nvectors)
>  {
> -	int i;
> -	for (i = 0; i < noptions; ++i)
> -		if (!pci_enable_msix(dev, entries, options[i]))
> -			return options[i];
> -	return -EBUSY;
> +	int err = pci_enable_msix(dev, entries, nvectors);
> +	if (err > 0)
> +		err = -ENOSPC;
> +	return err;
>  }
>  
> -static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
> +static int vp_request_irq(struct virtio_device *vdev)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +	int err;
> +	/* Can't allocate enough MSI-X vectors, use regular interrupt */
> +	vp_dev->msix_vectors = 0;
> +	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
> +			  IRQF_SHARED, dev_name(&vp_dev->vdev.dev), vp_dev);
> +	if (err)
> +		return err;
> +	vp_dev->intx_enabled = 1;
> +	return 0;
> +}
> +
> +static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs,
> +			      int nvectors)
>  {
>  	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;
> -	/* We want at most one vector per queue and one for config changes.
> -	 * Fallback to separate vectors for config and a shared for queues.
> -	 * Finally fall back to regular interrupts. */
> -	int options[] = { max_vqs + 1, 2 };
> -	int nvectors = max(options[0], options[1]);
>  
>  	vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
>  				       GFP_KERNEL);
> @@ -307,37 +316,29 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
>  		vp_dev->msix_entries[i].entry = i;
>  
>  	err = vp_enable_msix(vp_dev->pci_dev, vp_dev->msix_entries,
> -			     options, ARRAY_SIZE(options));
> -	if (err < 0) {
> -		/* Can't allocate enough MSI-X vectors, use regular interrupt */
> -		vp_dev->msix_vectors = 0;
> -		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
> -				  IRQF_SHARED, name, vp_dev);
> -		if (err)
> -			goto error_irq;
> -		vp_dev->intx_enabled = 1;
> -	} else {
> -		vp_dev->msix_vectors = err;
> -		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(vp_dev->msix_entries[v].vector,
> -				  vp_config_changed, 0, vp_dev->msix_names[v],
> -				  vp_dev);
> -		if (err)
> -			goto error_irq;
> -		++vp_dev->msix_used_vectors;
> -
> -		iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -		/* Verify we had enough resources to assign the vector */
> -		v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -		if (v == VIRTIO_MSI_NO_VECTOR) {
> -			err = -EBUSY;
> -			goto error_irq;
> -		}
> +			     nvectors);
> +	if (err)
> +		goto error_enable;
> +	vp_dev->msix_vectors = nvectors;
> +	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(vp_dev->msix_entries[v].vector,
> +			  vp_config_changed, 0, vp_dev->msix_names[v],
> +			  vp_dev);
> +	if (err)
> +		goto error_irq;
> +	++vp_dev->msix_used_vectors;
> +
> +	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> +	/* Verify we had enough resources to assign the vector */
> +	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> +	if (v == VIRTIO_MSI_NO_VECTOR) {
> +		err = -EBUSY;
> +		goto error_irq;
>  	}
>  
>  	if (vp_dev->msix_vectors && vp_dev->msix_vectors != max_vqs + 1) {
> @@ -355,9 +356,12 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
>  	return 0;
>  error_irq:
>  	vp_free_vectors(vdev);
> +error_enable:
>  	kfree(vp_dev->msix_names);
> +	vp_dev->msix_names = NULL;
>  error_names:
>  	kfree(vp_dev->msix_entries);
> +	vp_dev->msix_entries = NULL;
>  error_entries:
>  	return err;
>  }
> @@ -499,24 +503,24 @@ static void vp_del_vqs(struct virtio_device *vdev)
>  
>  	vp_free_vectors(vdev);
>  	kfree(vp_dev->msix_names);
> +	vp_dev->msix_names = NULL;
>  	kfree(vp_dev->msix_entries);
> +	vp_dev->msix_entries = NULL;
>  }
>  
> -/* the config->find_vqs() implementation */
> -static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> -		       struct virtqueue *vqs[],
> -		       vq_callback_t *callbacks[],
> -		       const char *names[])
> +static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> +			      struct virtqueue *vqs[],
> +			      vq_callback_t *callbacks[],
> +			      const char *names[],
> +			      int max_vectors,
> +			      int nvectors)
>  {
> -	int vectors = 0;
>  	int i, err;
>  
> -	/* How many vectors would we like? */
> -	for (i = 0; i < nvqs; ++i)
> -		if (callbacks[i])
> -			++vectors;
> -
> -	err = vp_request_vectors(vdev, vectors);
> +	if (nvectors)
> +		err = vp_request_vectors(vdev, max_vectors, nvectors);
> +	else
> +		err = vp_request_irq(vdev);
>  	if (err)
>  		goto error_request;
>  
> @@ -534,6 +538,34 @@ error_request:
>  	return PTR_ERR(vqs[i]);
>  }
>  
> +/* the config->find_vqs() implementation */
> +static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> +		       struct virtqueue *vqs[],
> +		       vq_callback_t *callbacks[],
> +		       const char *names[])
> +{
> +	/* We want at most one vector per queue and one for config changes.
> +	 * Fallback to separate vectors for config and a shared for queues.
> +	 * Finally fall back to regular interrupts. */
> +	int options[] = { 1, 2, 0 };
> +	int vectors = 0;
> +	int i, uninitialized_var(err);
> +
> +	/* How many vectors would we like? */
> +	for (i = 0; i < nvqs; ++i)
> +		if (callbacks[i])
> +			++vectors;
> +	options[0] = vectors + 1;
> +
> +	for (i = 0; i < ARRAY_SIZE(options); ++i) {
> +		err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
> +					 vectors, options[i]);
> +		if (!err)
> +			break;
> +	}
> +	return err;
> +}
> +
>  static struct virtio_config_ops virtio_pci_config_ops = {
>  	.get		= vp_get,
>  	.set		= vp_set,

		Amit