Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
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: sta...@vger.kernel.org > Cc: Amit Shah > Signed-off-by: Michael S. Tsirkin > --- > > 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(_dev->vdev); > + > + /* Acknowledge that we've seen the device. */ > + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE; > + vp_set_status(_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(_dev->vdev, status); > + > + if (!drv) > + return 0; > + > + /* We have a driver! */ > + status |= VIRTIO_CONFIG_S_DRIVER; > + vp_set_status(_dev->vdev, status); > + > vp_finalize_features(_dev->vdev); > > - if (drv && drv->restore) > - ret = drv->restore(_dev->vdev); > + if (!drv->restore) > + return 0; So in this case DRIVER_OK will never be set? > + > + ret = drv->restore(_dev->vdev); > + if (ret) { > + status |= VIRTIO_CONFIG_S_FAILED; > + vp_set_status(_dev->vdev, status); > + return ret; > + } > > /* Finally, tell the device we're all set */ > - if (!ret) > - vp_set_status(_dev->vdev, vp_dev->saved_status); > + status |= VIRTIO_CONFIG_S_DRIVER_OK; > + vp_set_status(_dev->vdev, status); > > return ret; > } Amit -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
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: sta...@vger.kernel.org > > Cc: Amit Shah > > Signed-off-by: Michael S. Tsirkin > [...] > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
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: sta...@vger.kernel.org Cc: Amit Shah amit.s...@redhat.com Signed-off-by: Michael S. Tsirkin m...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
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: sta...@vger.kernel.org Cc: Amit Shah amit.s...@redhat.com Signed-off-by: Michael S. Tsirkin m...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
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: sta...@vger.kernel.org > Cc: Amit Shah > Signed-off-by: Michael S. Tsirkin [...] 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 signature.asc Description: This is a digitally signed message part
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
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 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: sta...@vger.kernel.org > > Cc: Amit Shah > > Signed-off-by: Michael S. Tsirkin > > --- > > > > 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(_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(_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(_dev->vdev, status); > > + > > + if (!drv) > > + return 0; > > + > > + /* We have a driver! */ > > + status |= VIRTIO_CONFIG_S_DRIVER; > > + vp_set_status(_dev->vdev, status); > > + > > vp_finalize_features(_dev->vdev); > > > > - if (drv && drv->restore) > > - ret = drv->restore(_dev->vdev); > > + if (!drv->restore) > > + return 0; > > + > > + ret = drv->restore(_dev->vdev); > > + if (ret) { > > + status |= VIRTIO_CONFIG_S_FAILED; > > + vp_set_status(_dev->vdev, status); > > + return ret; > > + } > > > > /* Finally, tell the device we're all set */ > > - if (!ret) > > - vp_set_status(_dev->vdev, vp_dev->saved_status); > > + status |= VIRTIO_CONFIG_S_DRIVER_OK; > > + vp_set_status(_dev->vdev, status); > > > > return ret; > > } > > -- > > MST > > ___ > > Virtualization mailing list > > virtualizat...@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
On Tue, Sep 23, 2014 at 3:32 AM, 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: sta...@vger.kernel.org > Cc: Amit Shah > Signed-off-by: Michael S. Tsirkin > --- > > 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(_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(_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(_dev->vdev, status); > + > + if (!drv) > + return 0; > + > + /* We have a driver! */ > + status |= VIRTIO_CONFIG_S_DRIVER; > + vp_set_status(_dev->vdev, status); > + > vp_finalize_features(_dev->vdev); > > - if (drv && drv->restore) > - ret = drv->restore(_dev->vdev); > + if (!drv->restore) > + return 0; > + > + ret = drv->restore(_dev->vdev); > + if (ret) { > + status |= VIRTIO_CONFIG_S_FAILED; > + vp_set_status(_dev->vdev, status); > + return ret; > + } > > /* Finally, tell the device we're all set */ > - if (!ret) > - vp_set_status(_dev->vdev, vp_dev->saved_status); > + status |= VIRTIO_CONFIG_S_DRIVER_OK; > + vp_set_status(_dev->vdev, status); > > return ret; > } > -- > MST > ___ > Virtualization mailing list > virtualizat...@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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: sta...@vger.kernel.org Cc: Amit Shah Signed-off-by: Michael S. Tsirkin --- 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(_dev->vdev); + + /* Acknowledge that we've seen the device. */ + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE; + vp_set_status(_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(_dev->vdev, status); + + if (!drv) + return 0; + + /* We have a driver! */ + status |= VIRTIO_CONFIG_S_DRIVER; + vp_set_status(_dev->vdev, status); + vp_finalize_features(_dev->vdev); - if (drv && drv->restore) - ret = drv->restore(_dev->vdev); + if (!drv->restore) + return 0; + + ret = drv->restore(_dev->vdev); + if (ret) { + status |= VIRTIO_CONFIG_S_FAILED; + vp_set_status(_dev->vdev, status); + return ret; + } /* Finally, tell the device we're all set */ - if (!ret) - vp_set_status(_dev->vdev, vp_dev->saved_status); + status |= VIRTIO_CONFIG_S_DRIVER_OK; + vp_set_status(_dev->vdev, status); return ret; } -- MST -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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: sta...@vger.kernel.org Cc: Amit Shah amit.s...@redhat.com Signed-off-by: Michael S. Tsirkin m...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
On Tue, Sep 23, 2014 at 3:32 AM, Michael S. Tsirkin m...@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: sta...@vger.kernel.org Cc: Amit Shah amit.s...@redhat.com Signed-off-by: Michael S. Tsirkin m...@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 virtualizat...@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
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 m...@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: sta...@vger.kernel.org Cc: Amit Shah amit.s...@redhat.com Signed-off-by: Michael S. Tsirkin m...@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 virtualizat...@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
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: sta...@vger.kernel.org Cc: Amit Shah amit.s...@redhat.com Signed-off-by: Michael S. Tsirkin m...@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 signature.asc Description: This is a digitally signed message part