Hi Michael,
On Mon, Sep 15, 2014 at 1:32 AM, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori wrote:
>> From: Anthony Liguori <aliguori@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Cc: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
>> Cc: Michael Tsirkin <mst@xxxxxxxxxx>
>> Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
