* [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
@ 2014-09-23 10:32 Michael S. Tsirkin
2014-09-23 16:06 ` Eric Northup
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2014-09-23 10:32 UTC (permalink / raw)
To: linux-kernel; +Cc: stable, Amit Shah, Rusty Russell, virtualization
On restore, virtio pci does the following:
+ set features
+ init vqs etc - device can be used at this point!
+ set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
This is in violation of the virtio spec, which
requires the following order:
- ACKNOWLEDGE
- DRIVER
- init vqs
- DRIVER_OK
Cc: stable@vger.kernel.org
Cc: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Lightly tested.
Will repost as non-RFC once testing is done, sending
out now for early flames/comments.
Thanks!
drivers/virtio/virtio_pci.c | 36 ++++++++++++++++++++++++++++++++----
1 file changed, 32 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 58f7e45..58cbf6e 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -785,6 +785,7 @@ static int virtio_pci_restore(struct device *dev)
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
struct virtio_driver *drv;
+ unsigned status = 0;
int ret;
drv = container_of(vp_dev->vdev.dev.driver,
@@ -795,14 +796,41 @@ static int virtio_pci_restore(struct device *dev)
return ret;
pci_set_master(pci_dev);
+ /* We always start by resetting the device, in case a previous
+ * driver messed it up. */
+ vp_reset(&vp_dev->vdev);
+
+ /* Acknowledge that we've seen the device. */
+ status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
+ vp_set_status(&vp_dev->vdev, status);
+
+ /* Maybe driver failed before freeze.
+ * Restore the failed status, for debugging. */
+ status |= vp_dev->saved_status & VIRTIO_CONFIG_S_FAILED;
+ vp_set_status(&vp_dev->vdev, status);
+
+ if (!drv)
+ return 0;
+
+ /* We have a driver! */
+ status |= VIRTIO_CONFIG_S_DRIVER;
+ vp_set_status(&vp_dev->vdev, status);
+
vp_finalize_features(&vp_dev->vdev);
- if (drv && drv->restore)
- ret = drv->restore(&vp_dev->vdev);
+ if (!drv->restore)
+ return 0;
+
+ ret = drv->restore(&vp_dev->vdev);
+ if (ret) {
+ status |= VIRTIO_CONFIG_S_FAILED;
+ vp_set_status(&vp_dev->vdev, status);
+ return ret;
+ }
/* Finally, tell the device we're all set */
- if (!ret)
- vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
+ status |= VIRTIO_CONFIG_S_DRIVER_OK;
+ vp_set_status(&vp_dev->vdev, status);
return ret;
}
--
MST
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
2014-09-23 10:32 [PATCH RFC] virtio_pci: fix virtio spec compliance on restore Michael S. Tsirkin
@ 2014-09-23 16:06 ` Eric Northup
2014-09-23 18:02 ` Michael S. Tsirkin
2014-09-24 1:27 ` Ben Hutchings
2014-09-24 12:27 ` Amit Shah
2 siblings, 1 reply; 6+ messages in thread
From: Eric Northup @ 2014-09-23 16:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Linux Kernel Mailing List, Amit Shah, stable, lf-virt
On Tue, Sep 23, 2014 at 3:32 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On restore, virtio pci does the following:
> + set features
> + init vqs etc - device can be used at this point!
> + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
>
> This is in violation of the virtio spec, which
> requires the following order:
> - ACKNOWLEDGE
> - DRIVER
> - init vqs
> - DRIVER_OK
>
> Cc: stable@vger.kernel.org
> Cc: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Lightly tested.
> Will repost as non-RFC once testing is done, sending
> out now for early flames/comments.
>
> Thanks!
>
> drivers/virtio/virtio_pci.c | 36 ++++++++++++++++++++++++++++++++----
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 58f7e45..58cbf6e 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -785,6 +785,7 @@ static int virtio_pci_restore(struct device *dev)
> struct pci_dev *pci_dev = to_pci_dev(dev);
> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> struct virtio_driver *drv;
> + unsigned status = 0;
> int ret;
>
> drv = container_of(vp_dev->vdev.dev.driver,
> @@ -795,14 +796,41 @@ static int virtio_pci_restore(struct device *dev)
> return ret;
>
> pci_set_master(pci_dev);
> + /* We always start by resetting the device, in case a previous
> + * driver messed it up. */
> + vp_reset(&vp_dev->vdev);
This should happen before enabling PCI bus mastering.
> +
> + /* Acknowledge that we've seen the device. */
> + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
> + vp_set_status(&vp_dev->vdev, status);
> +
> + /* Maybe driver failed before freeze.
> + * Restore the failed status, for debugging. */
> + status |= vp_dev->saved_status & VIRTIO_CONFIG_S_FAILED;
> + vp_set_status(&vp_dev->vdev, status);
> +
> + if (!drv)
> + return 0;
> +
> + /* We have a driver! */
> + status |= VIRTIO_CONFIG_S_DRIVER;
> + vp_set_status(&vp_dev->vdev, status);
> +
> vp_finalize_features(&vp_dev->vdev);
>
> - if (drv && drv->restore)
> - ret = drv->restore(&vp_dev->vdev);
> + if (!drv->restore)
> + return 0;
> +
> + ret = drv->restore(&vp_dev->vdev);
> + if (ret) {
> + status |= VIRTIO_CONFIG_S_FAILED;
> + vp_set_status(&vp_dev->vdev, status);
> + return ret;
> + }
>
> /* Finally, tell the device we're all set */
> - if (!ret)
> - vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
> + status |= VIRTIO_CONFIG_S_DRIVER_OK;
> + vp_set_status(&vp_dev->vdev, status);
>
> return ret;
> }
> --
> MST
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
2014-09-23 16:06 ` Eric Northup
@ 2014-09-23 18:02 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2014-09-23 18:02 UTC (permalink / raw)
To: Eric Northup; +Cc: Linux Kernel Mailing List, Amit Shah, stable, lf-virt
On Tue, Sep 23, 2014 at 09:06:03AM -0700, Eric Northup wrote:
> On Tue, Sep 23, 2014 at 3:32 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On restore, virtio pci does the following:
> > + set features
> > + init vqs etc - device can be used at this point!
> > + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
> >
> > This is in violation of the virtio spec, which
> > requires the following order:
> > - ACKNOWLEDGE
> > - DRIVER
> > - init vqs
> > - DRIVER_OK
> >
> > Cc: stable@vger.kernel.org
> > Cc: Amit Shah <amit.shah@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Lightly tested.
> > Will repost as non-RFC once testing is done, sending
> > out now for early flames/comments.
> >
> > Thanks!
> >
> > drivers/virtio/virtio_pci.c | 36 ++++++++++++++++++++++++++++++++----
> > 1 file changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > index 58f7e45..58cbf6e 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -785,6 +785,7 @@ static int virtio_pci_restore(struct device *dev)
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > struct virtio_driver *drv;
> > + unsigned status = 0;
> > int ret;
> >
> > drv = container_of(vp_dev->vdev.dev.driver,
> > @@ -795,14 +796,41 @@ static int virtio_pci_restore(struct device *dev)
> > return ret;
> >
> > pci_set_master(pci_dev);
> > + /* We always start by resetting the device, in case a previous
> > + * driver messed it up. */
> > + vp_reset(&vp_dev->vdev);
>
> This should happen before enabling PCI bus mastering.
this is the order of events in initialization,
it seems better to be consistent.
> > +
> > + /* Acknowledge that we've seen the device. */
> > + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
> > + vp_set_status(&vp_dev->vdev, status);
> > +
> > + /* Maybe driver failed before freeze.
> > + * Restore the failed status, for debugging. */
> > + status |= vp_dev->saved_status & VIRTIO_CONFIG_S_FAILED;
> > + vp_set_status(&vp_dev->vdev, status);
> > +
> > + if (!drv)
> > + return 0;
> > +
> > + /* We have a driver! */
> > + status |= VIRTIO_CONFIG_S_DRIVER;
> > + vp_set_status(&vp_dev->vdev, status);
> > +
> > vp_finalize_features(&vp_dev->vdev);
> >
> > - if (drv && drv->restore)
> > - ret = drv->restore(&vp_dev->vdev);
> > + if (!drv->restore)
> > + return 0;
> > +
> > + ret = drv->restore(&vp_dev->vdev);
> > + if (ret) {
> > + status |= VIRTIO_CONFIG_S_FAILED;
> > + vp_set_status(&vp_dev->vdev, status);
> > + return ret;
> > + }
> >
> > /* Finally, tell the device we're all set */
> > - if (!ret)
> > - vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
> > + status |= VIRTIO_CONFIG_S_DRIVER_OK;
> > + vp_set_status(&vp_dev->vdev, status);
> >
> > return ret;
> > }
> > --
> > MST
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
2014-09-23 10:32 [PATCH RFC] virtio_pci: fix virtio spec compliance on restore Michael S. Tsirkin
2014-09-23 16:06 ` Eric Northup
@ 2014-09-24 1:27 ` Ben Hutchings
2014-09-24 11:59 ` Michael S. Tsirkin
2014-09-24 12:27 ` Amit Shah
2 siblings, 1 reply; 6+ messages in thread
From: Ben Hutchings @ 2014-09-24 1:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, stable, Amit Shah, Rusty Russell, virtualization
[-- Attachment #1: Type: text/plain, Size: 780 bytes --]
On Tue, 2014-09-23 at 13:32 +0300, Michael S. Tsirkin wrote:
> On restore, virtio pci does the following:
> + set features
> + init vqs etc - device can be used at this point!
> + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
>
> This is in violation of the virtio spec, which
> requires the following order:
> - ACKNOWLEDGE
> - DRIVER
> - init vqs
> - DRIVER_OK
>
> Cc: stable@vger.kernel.org
> Cc: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
[...]
What concrete problem does this fix, such that it should be applied to
stable branches?
Ben.
--
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
- Albert Einstein
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
2014-09-24 1:27 ` Ben Hutchings
@ 2014-09-24 11:59 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2014-09-24 11:59 UTC (permalink / raw)
To: Ben Hutchings
Cc: linux-kernel, stable, Amit Shah, Rusty Russell, virtualization
On Wed, Sep 24, 2014 at 02:27:27AM +0100, Ben Hutchings wrote:
> On Tue, 2014-09-23 at 13:32 +0300, Michael S. Tsirkin wrote:
> > On restore, virtio pci does the following:
> > + set features
> > + init vqs etc - device can be used at this point!
> > + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
> >
> > This is in violation of the virtio spec, which
> > requires the following order:
> > - ACKNOWLEDGE
> > - DRIVER
> > - init vqs
> > - DRIVER_OK
> >
> > Cc: stable@vger.kernel.org
> > Cc: Amit Shah <amit.shah@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> [...]
>
> What concrete problem does this fix, such that it should be applied to
> stable branches?
>
> Ben.
It will break with hypervisors that assume spec compliant behaviour.
I would like this applied to stable branches so hypervisors don't
need to support broken behaviour forever.
--
MST
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
2014-09-23 10:32 [PATCH RFC] virtio_pci: fix virtio spec compliance on restore Michael S. Tsirkin
2014-09-23 16:06 ` Eric Northup
2014-09-24 1:27 ` Ben Hutchings
@ 2014-09-24 12:27 ` Amit Shah
2 siblings, 0 replies; 6+ messages in thread
From: Amit Shah @ 2014-09-24 12:27 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, stable, Rusty Russell, virtualization
On (Tue) 23 Sep 2014 [13:32:22], Michael S. Tsirkin wrote:
> On restore, virtio pci does the following:
> + set features
> + init vqs etc - device can be used at this point!
> + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
>
> This is in violation of the virtio spec, which
> requires the following order:
> - ACKNOWLEDGE
> - DRIVER
> - init vqs
> - DRIVER_OK
>
> Cc: stable@vger.kernel.org
> Cc: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Lightly tested.
> Will repost as non-RFC once testing is done, sending
> out now for early flames/comments.
What tests are you running?
> Thanks!
>
> drivers/virtio/virtio_pci.c | 36 ++++++++++++++++++++++++++++++++----
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 58f7e45..58cbf6e 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -785,6 +785,7 @@ static int virtio_pci_restore(struct device *dev)
> struct pci_dev *pci_dev = to_pci_dev(dev);
> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> struct virtio_driver *drv;
> + unsigned status = 0;
> int ret;
>
> drv = container_of(vp_dev->vdev.dev.driver,
> @@ -795,14 +796,41 @@ static int virtio_pci_restore(struct device *dev)
> return ret;
>
> pci_set_master(pci_dev);
> + /* We always start by resetting the device, in case a previous
> + * driver messed it up. */
> + vp_reset(&vp_dev->vdev);
> +
> + /* Acknowledge that we've seen the device. */
> + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
> + vp_set_status(&vp_dev->vdev, status);
> +
> + /* Maybe driver failed before freeze.
> + * Restore the failed status, for debugging. */
> + status |= vp_dev->saved_status & VIRTIO_CONFIG_S_FAILED;
> + vp_set_status(&vp_dev->vdev, status);
> +
> + if (!drv)
> + return 0;
> +
> + /* We have a driver! */
> + status |= VIRTIO_CONFIG_S_DRIVER;
> + vp_set_status(&vp_dev->vdev, status);
> +
> vp_finalize_features(&vp_dev->vdev);
>
> - if (drv && drv->restore)
> - ret = drv->restore(&vp_dev->vdev);
> + if (!drv->restore)
> + return 0;
So in this case DRIVER_OK will never be set?
> +
> + ret = drv->restore(&vp_dev->vdev);
> + if (ret) {
> + status |= VIRTIO_CONFIG_S_FAILED;
> + vp_set_status(&vp_dev->vdev, status);
> + return ret;
> + }
>
> /* Finally, tell the device we're all set */
> - if (!ret)
> - vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
> + status |= VIRTIO_CONFIG_S_DRIVER_OK;
> + vp_set_status(&vp_dev->vdev, status);
>
> return ret;
> }
Amit
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-24 12:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-23 10:32 [PATCH RFC] virtio_pci: fix virtio spec compliance on restore Michael S. Tsirkin
2014-09-23 16:06 ` Eric Northup
2014-09-23 18:02 ` Michael S. Tsirkin
2014-09-24 1:27 ` Ben Hutchings
2014-09-24 11:59 ` Michael S. Tsirkin
2014-09-24 12:27 ` Amit Shah
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).