Linux Virtualization - Sep 2014 - [PATCH RFC] virtio_pci: fix virtio spec compliance on restore

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 at vger.kernel.org
Cc: Amit Shah <amit.shah at redhat.com>
Signed-off-by: Michael S. Tsirkin <mst at 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

On Tue, Sep 23, 2014 at 3:32 AM, Michael S. Tsirkin <mst at 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 at vger.kernel.org
> Cc: Amit Shah <amit.shah at redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst at 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 at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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 at
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 at vger.kernel.org
> > Cc: Amit Shah <amit.shah at redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst at 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 at lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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 at vger.kernel.org
> Cc: Amit Shah <amit.shah at redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: This is a digitally signed message part
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20140924/c2c11f49/attachment.sig>

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 at vger.kernel.org
> > Cc: Amit Shah <amit.shah at redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst at 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

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 at vger.kernel.org
> Cc: Amit Shah <amit.shah at redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst at 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