Linux Virtualization - Sep 2014 - [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails

From: Anthony Liguori <aliguori at amazon.com>

If MSI-X initialization fails after setting msix_enabled = 1, then
the device is left in an inconsistent state.  This would normally
only happen if there was a bug in the device emulation but it still
should be handled correctly.

Cc: Matt Wilson <msw at amazon.com>
Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: Michael Tsirkin <mst at redhat.com>
Signed-off-by: Anthony Liguori <aliguori at amazon.com>
---
 drivers/virtio/virtio_pci.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 9cbac33..3d2c2a5 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -357,7 +357,7 @@ static int vp_request_msix_vectors(struct virtio_device
*vdev, int nvectors,
 	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
 	if (v == VIRTIO_MSI_NO_VECTOR) {
 		err = -EBUSY;
-		goto error;
+		goto error_msix_used;
 	}
 
 	if (!per_vq_vectors) {
@@ -369,11 +369,15 @@ static int vp_request_msix_vectors(struct virtio_device
*vdev, int nvectors,
 				  vp_vring_interrupt, 0, vp_dev->msix_names[v],
 				  vp_dev);
 		if (err)
-			goto error;
+			goto error_msix_used;
 		++vp_dev->msix_used_vectors;
 	}
 	return 0;
+error_msix_used:
+	v = --vp_dev->msix_used_vectors;
+	free_irq(vp_dev->msix_entries[v].vector, vp_dev);
 error:
+	vp_dev->msix_enabled = 0;
 	vp_free_vectors(vdev);
 	return err;
 }
-- 
1.7.9.5

On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori
wrote:
> From: Anthony Liguori <aliguori at amazon.com>
> 
> If MSI-X initialization fails after setting msix_enabled = 1, then
> the device is left in an inconsistent state.  This would normally
> only happen if there was a bug in the device emulation but it still
> should be handled correctly.
> 
> Cc: Matt Wilson <msw at amazon.com>
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: Michael Tsirkin <mst at redhat.com>
> Signed-off-by: Anthony Liguori <aliguori at amazon.com>
Reviewed-by: Matt Wilson <msw at amazon.com>

--msw
> ---
>  drivers/virtio/virtio_pci.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 9cbac33..3d2c2a5 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -357,7 +357,7 @@ static int vp_request_msix_vectors(struct virtio_device
*vdev, int nvectors,
>  	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
>  	if (v == VIRTIO_MSI_NO_VECTOR) {
>  		err = -EBUSY;
> -		goto error;
> +		goto error_msix_used;
>  	}
>  
>  	if (!per_vq_vectors) {
> @@ -369,11 +369,15 @@ static int vp_request_msix_vectors(struct
virtio_device *vdev, int nvectors,
>  				  vp_vring_interrupt, 0, vp_dev->msix_names[v],
>  				  vp_dev);
>  		if (err)
> -			goto error;
> +			goto error_msix_used;
>  		++vp_dev->msix_used_vectors;
>  	}
>  	return 0;
> +error_msix_used:
> +	v = --vp_dev->msix_used_vectors;
> +	free_irq(vp_dev->msix_entries[v].vector, vp_dev);
>  error:
> +	vp_dev->msix_enabled = 0;
>  	vp_free_vectors(vdev);
>  	return err;
>  }

On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori
wrote:
> From: Anthony Liguori <aliguori at amazon.com>
> 
> If MSI-X initialization fails after setting msix_enabled = 1, then
> the device is left in an inconsistent state.  This would normally
> only happen if there was a bug in the device emulation but it still
> should be handled correctly.
This might happen if host runs out of resources when trying
to map VQs to vectors, so doesn't have to be a bug.

But I don't see what the problem is:
msix_used_vectors reflects the number of used vectors
and msix_enabled is set, thus vp_free_vectors
will free all IRQs and then disable MSIX.

Where is the inconsistency you speak about?

> 
> Cc: Matt Wilson <msw at amazon.com>
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: Michael Tsirkin <mst at redhat.com>
> Signed-off-by: Anthony Liguori <aliguori at amazon.com>
> ---
>  drivers/virtio/virtio_pci.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 9cbac33..3d2c2a5 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -357,7 +357,7 @@ static int vp_request_msix_vectors(struct virtio_device
*vdev, int nvectors,
>  	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
>  	if (v == VIRTIO_MSI_NO_VECTOR) {
>  		err = -EBUSY;
> -		goto error;
> +		goto error_msix_used;
>  	}
>  
>  	if (!per_vq_vectors) {
> @@ -369,11 +369,15 @@ static int vp_request_msix_vectors(struct
virtio_device *vdev, int nvectors,
>  				  vp_vring_interrupt, 0, vp_dev->msix_names[v],
>  				  vp_dev);
>  		if (err)
> -			goto error;
> +			goto error_msix_used;
>  		++vp_dev->msix_used_vectors;
>  	}
>  	return 0;
> +error_msix_used:
> +	v = --vp_dev->msix_used_vectors;
> +	free_irq(vp_dev->msix_entries[v].vector, vp_dev);
>  error:
> +	vp_dev->msix_enabled = 0;
As far as I can see, if you do this, guest will not call
pci_disable_msix thus leaving the device with MSIX enabled.
I'm not sure this won't break drivers if they then 
try to use the device without MSIX, and it
definitely seems less elegant than reverting the
device to the original state.

>  	vp_free_vectors(vdev);
>  	return err;
>  }
> -- 
> 1.7.9.5

Hi Michael,

On Mon, Sep 15, 2014 at 1:32 AM, Michael S. Tsirkin <mst at redhat.com>
wrote:
> On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori wrote:
>> From: Anthony Liguori <aliguori at amazon.com>
>>
>> If MSI-X initialization fails after setting msix_enabled = 1, then
>> the device is left in an inconsistent state.  This would normally
>> only happen if there was a bug in the device emulation but it still
>> should be handled correctly.
>
> This might happen if host runs out of resources when trying
> to map VQs to vectors, so doesn't have to be a bug.
>
> But I don't see what the problem is:
> msix_used_vectors reflects the number of used vectors
> and msix_enabled is set, thus vp_free_vectors
> will free all IRQs and then disable MSIX.
>
> Where is the inconsistency you speak about?
I missed the fact that vp_free_vectors() conditionally sets
msix_enabled=0.  It seems a bit cludgy especially since it is called
both before and after setting msix_enabled=1.

I ran into a number of weird problems due to read/write reordering
that was causing this code path to fail.  The impact was
non-deterministic.  I'll go back and try to better understand what
code path is causing it.
>> Cc: Matt Wilson <msw at amazon.com>
>> Cc: Rusty Russell <rusty at rustcorp.com.au>
>> Cc: Michael Tsirkin <mst at redhat.com>
>> Signed-off-by: Anthony Liguori <aliguori at amazon.com>
>> ---
>>  drivers/virtio/virtio_pci.c |    8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
>> index 9cbac33..3d2c2a5 100644
>> --- a/drivers/virtio/virtio_pci.c
>> +++ b/drivers/virtio/virtio_pci.c
>> @@ -357,7 +357,7 @@ static int vp_request_msix_vectors(struct
virtio_device *vdev, int nvectors,
>>       v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
>>       if (v == VIRTIO_MSI_NO_VECTOR) {
>>               err = -EBUSY;
>> -             goto error;
>> +             goto error_msix_used;
>>       }
>>
>>       if (!per_vq_vectors) {
>> @@ -369,11 +369,15 @@ static int vp_request_msix_vectors(struct
virtio_device *vdev, int nvectors,
>>                                 vp_vring_interrupt, 0,
vp_dev->msix_names[v],
>>                                 vp_dev);
>>               if (err)
>> -                     goto error;
>> +                     goto error_msix_used;
>>               ++vp_dev->msix_used_vectors;
>>       }
>>       return 0;
>> +error_msix_used:
>> +     v = --vp_dev->msix_used_vectors;
>> +     free_irq(vp_dev->msix_entries[v].vector, vp_dev);
>>  error:
>> +     vp_dev->msix_enabled = 0;
>
> As far as I can see, if you do this, guest will not call
> pci_disable_msix thus leaving the device with MSIX enabled.
I don't understand this comment.  How is the work done in this path
any different from what's done in vp_free_vectors()?

Regards,

Anthony Liguori
> I'm not sure this won't break drivers if they then
> try to use the device without MSIX, and it
> definitely seems less elegant than reverting the
> device to the original state.
>
>
>>       vp_free_vectors(vdev);
>>       return err;
>>  }
>> --
>> 1.7.9.5