* Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails
2014-09-15 14:10 ` [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails Anthony Liguori
@ 2014-09-15 14:39 ` Michael S. Tsirkin
0 siblings, 0 replies; 2+ messages in thread
From: Michael S. Tsirkin @ 2014-09-15 14:39 UTC (permalink / raw)
To: Anthony Liguori
Cc: virtualization, Anthony Liguori, Matt Wilson, Rusty Russell,
linux-kernel
On Mon, Sep 15, 2014 at 07:10:45AM -0700, Anthony Liguori wrote:
> 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.
It's the style of initialization that records the current initialization
stage, and then uses that to do all cleanup in a single place
(as compared to detailed separate goto labels for each initialization
stage).
I don't mind either keeping this style or changing to another style,
but if we change it we should change it everywhere I think.
> 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