* [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails @ 2014-09-15 3:23 Anthony Liguori 2014-09-15 3:43 ` Matt Wilson 2014-09-15 8:32 ` Michael S. Tsirkin 0 siblings, 2 replies; 5+ messages in thread From: Anthony Liguori @ 2014-09-15 3:23 UTC (permalink / raw) To: virtualization; +Cc: kernel, Matt Wilson, Anthony Liguori, Michael Tsirkin 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. 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; vp_free_vectors(vdev); return err; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails 2014-09-15 3:23 [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails Anthony Liguori @ 2014-09-15 3:43 ` Matt Wilson 2014-09-15 8:32 ` Michael S. Tsirkin 1 sibling, 0 replies; 5+ messages in thread From: Matt Wilson @ 2014-09-15 3:43 UTC (permalink / raw) To: Anthony Liguori Cc: Anthony Liguori, Michael Tsirkin, kernel, virtualization, Matt Wilson 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. > > 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> Reviewed-by: Matt Wilson <msw@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; > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails 2014-09-15 3:23 [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails Anthony Liguori 2014-09-15 3:43 ` Matt Wilson @ 2014-09-15 8:32 ` Michael S. Tsirkin 2014-09-15 14:10 ` Anthony Liguori 1 sibling, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2014-09-15 8:32 UTC (permalink / raw) To: Anthony Liguori; +Cc: kernel, Matt Wilson, Anthony Liguori, virtualization 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? > > 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'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] 5+ messages in thread
* Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails 2014-09-15 8:32 ` Michael S. Tsirkin @ 2014-09-15 14:10 ` Anthony Liguori 2014-09-15 14:39 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Anthony Liguori @ 2014-09-15 14:10 UTC (permalink / raw) To: Michael S. Tsirkin Cc: linux-kernel, Matt Wilson, Anthony Liguori, virtualization 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] 5+ messages in thread
* Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails 2014-09-15 14:10 ` Anthony Liguori @ 2014-09-15 14:39 ` Michael S. Tsirkin 0 siblings, 0 replies; 5+ messages in thread From: Michael S. Tsirkin @ 2014-09-15 14:39 UTC (permalink / raw) To: Anthony Liguori Cc: linux-kernel, Matt Wilson, Anthony Liguori, virtualization 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] 5+ messages in thread
end of thread, other threads:[~2014-09-15 14:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-15 3:23 [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails Anthony Liguori 2014-09-15 3:43 ` Matt Wilson 2014-09-15 8:32 ` Michael S. Tsirkin 2014-09-15 14:10 ` Anthony Liguori 2014-09-15 14:39 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).