Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails

* Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails
       [not found] ` <20140915083220.GC6480@redhat.com>
@ 2014-09-15 14:10   ` Anthony Liguori
  2014-09-15 14:39     ` Michael S. Tsirkin
  0 siblings, 1 reply; 2+ messages in thread
From: Anthony Liguori @ 2014-09-15 14:10 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtualization, Anthony Liguori, Matt Wilson, Rusty Russell,
	linux-kernel

Hi Michael,

On Mon, Sep 15, 2014 at 1:32 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori wrote:
>> From: Anthony Liguori <aliguori@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@amazon.com>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: Michael Tsirkin <mst@redhat.com>
>> Signed-off-by: Anthony Liguori <aliguori@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

^ permalink raw reply	[flat|nested] 2+ messages in thread