[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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: pci: Use SIMPLE_DEV_PM_OPS macro
Jingoo Han jg1@samsung.com writes: On Tuesday, September 09, 2014 9:14 AM, Rusty Russell wrote: Jingoo Han jg1@samsung.com writes: Use SIMPLE_DEV_PM_OPS macro in order to make the code simpler. Signed-off-by: Jingoo Han jg1@samsung.com This patch is obviously wrong. It won't compile without CONFIG_PM_SLEEP. No, there is no compile issue. When, CONFIG_PM_SLEEP=n, there is no build error. My mistake. Thanks, I've applied it. It probably won't go in until the next merge window, however, since I'm travelling for this one. Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC] virtio-pci: share config interrupt between virtio devices
On Sunday 21 September 2014 13:21:06, Michael S. Tsirkin wrote: On Sun, Sep 21, 2014 at 11:36:44AM +0200, Stefan Fritsch wrote: On Sunday 21 September 2014 11:09:14, Michael S. Tsirkin wrote: On Thu, Sep 18, 2014 at 09:18:37PM +0200, Stefan Fritsch wrote: On Monday 01 September 2014 09:37:30, Michael S. Tsirkin wrote: Why do we need INT#x? How about setting IRQF_SHARED for the config interrupt while using MSI-X? You'd have to read ISR to check that the interrupt was intended for your device. The virtio 0.9.5 spec says that ISR is unused when in MSI-X mode. I don't think that you can depend on the device to set the configuration changed bit. The virtio 1.0 spec seems to have fixed that. Yes, virtio 0.9.5 has this bug. But in practice qemu always set this bit, so for qemu we could do that unconditionally. Pekka's lkvm tool doesn't unfortunately. It's easy to fix that, but it would be nicer to additionally probe for old versions of the tool, and disable IRQF_SHARED in that case. What about other implementations? I think Linux should try to conform to the spec so that all device implementations which conform to the spec just work. One implementation that comes to mind is virtualbox. But from a quick look at the source, it seems that it sets the ISR bit always, too. And it uses qemu's subsystem vendor id. But there are other implementations. For example bhyve. I couldn't find any code in bhyve that sets VTCFG_ISR_CONF_CHANGED. Maybe it doesn't generate config changed interrupts? bhyve sets subsystem vendor to 0 apparently? We could use that to detect it. My point was that there are many virtio implementations by now and you can't assume you know all of them. But maybe we should just make it a 1.0 only feature. FWIW, I think that would be the better option. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
