virtio: S4 support

PATCH Superseded Amit Shah 2 participants 22 messages Nov 17, 2011

Subsystems (8)

Network drivers Block layer Char and misc drivers virtio core +4 more subsystems

lore

Share & tools

lore

Amit Shah

author

2011-11-17 11:57 UTC #0 lore raw

v3 cover virtio: S4 support

Hi,

These patches add support for S4 to virtio (pci) and all drivers.

For each driver, all vqs are removed before hibernation, and then re-created after restore. Some driver-specific uninit and init work is also done in the freeze and restore functions.

All the drivers in testing work fine:

* virtio-blk is used for the only disk in the VM, IO works fine before and after.

* virtio-console: port IO keeps working fine before and after.
* If a port is waiting for data from the host (blocking read(2) call), this works fine in both the cases: host-side connection is available or unavailable after resume. In case the host-side connection isn't available, the blocking call is terminated. If it is available, the call continues to remain in blocked state till further data arrives.

* virtio-net: ping remains active across S4.

* virtio-balloon: Works fine before and after. Forgets the ballooned value across S4. If it's desirable to maintain the ballooned value, a new config option can be created to do this.

All of these tests are run in parallel.

I have some more tests lined up on similar lines above. I'll reply here if something breaks.

Please review and apply if appropriate,

v3:
- Reset vqs before deleting them (Sasha Levin)
- Flush block queue before freeze (Rusty)
- Detach netdev before freeze (Michael S. Tsirkin)

v2:
- fix checkpatch errors/warnings

Amit Shah (11):
virtio: pci: switch to new PM API
virtio: pci: add PM notification handlers for restore, freeze, thaw,
poweroff
virtio: console: Move out vq and vq buf removal into separate
functions
virtio: console: Add freeze and restore handlers to support S4
virtio: blk: Move out vq initialization to separate function
virtio: blk: Add freeze, restore handlers to support S4
virtio: net: Move out vq initialization into separate function
virtio: net: Move out vq and vq buf removal into separate function
virtio: net: Add freeze, restore handlers to support S4
virtio: balloon: Move out vq initialization into separate function
virtio: balloon: Add freeze, restore handlers to support S4

 drivers/block/virtio_blk.c      |   55 +++++++++++++++--
 drivers/char/virtio_console.c   |  126 ++++++++++++++++++++++++++++++---------
 drivers/net/virtio_net.c        |   99 ++++++++++++++++++++++--------
 drivers/virtio/virtio_balloon.c |   68 +++++++++++++++------
 drivers/virtio/virtio_pci.c     |   66 +++++++++++++++++++-
 include/linux/virtio.h          |    4 +
 6 files changed, 336 insertions(+), 82 deletions(-)

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #1 lore raw

PATCH v3 1/11 virtio: pci: switch to new PM API

The older PM API doesn't have a way to get notifications on hibernate events. Switch to the newer one that gives us those notifications.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/virtio/virtio_pci.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

drivers/virtio/virtio_pci.c +12 -4

index 3d1bf41..5d36bc0 100644
@@ -690,19 +690,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 }
 
 #ifdef CONFIG_PM
-static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
+static int virtio_pci_suspend(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
 	pci_save_state(pci_dev);
 	pci_set_power_state(pci_dev, PCI_D3hot);
 	return 0;
 }
 
-static int virtio_pci_resume(struct pci_dev *pci_dev)
+static int virtio_pci_resume(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
 	pci_restore_state(pci_dev);
 	pci_set_power_state(pci_dev, PCI_D0);
 	return 0;
 }
+
+static const struct dev_pm_ops virtio_pci_pm_ops = {
+	.suspend = virtio_pci_suspend,
+	.resume  = virtio_pci_resume,
+};
 #endif
 
 static struct pci_driver virtio_pci_driver = {

@@ -711,8 +720,7 @@ static struct pci_driver virtio_pci_driver = {
 	.probe		= virtio_pci_probe,
 	.remove		= __devexit_p(virtio_pci_remove),
 #ifdef CONFIG_PM
-	.suspend	= virtio_pci_suspend,
-	.resume		= virtio_pci_resume,
+	.driver.pm	= &virtio_pci_pm_ops,
 #endif
 };

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #2 lore raw

PATCH v3 2/11 virtio: pci: add PM notification handlers for restore, freeze, thaw, poweroff

In reply to Amit Shah

Handle restore and freeze notification from the PM core. Expose these to individual virtio drivers that can quiesce and resume vq operations.

These functions also save device-specific data so that the device can be put in pre-suspend state after resume, and disable and enable the PCI device in the freeze and resume functions, respectively.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/virtio/virtio_pci.c |   50 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h      |    4 +++
 2 files changed, 54 insertions(+), 0 deletions(-)

drivers/virtio/virtio_pci.c +50 -0

index 5d36bc0..0db6f98 100644
@@ -55,6 +55,10 @@ struct virtio_pci_device
 	unsigned msix_vectors;
 	/* Vectors allocated, excluding per-vq vectors if any */
 	unsigned msix_used_vectors;
+
+	/* Status saved during hibernate/restore */
+	u8 saved_status;
+
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
 };

@@ -708,9 +712,55 @@ static int virtio_pci_resume(struct device *dev)
 	return 0;
 }
 
+static int virtio_pci_freeze(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;
+	int ret;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+
+	ret = 0;
+	vp_dev->saved_status = vp_get_status(&vp_dev->vdev);
+	if (drv && drv->freeze)
+		ret = drv->freeze(&vp_dev->vdev);
+
+	if (!ret)
+		pci_disable_device(pci_dev);
+	return ret;
+}
+
+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;
+	int ret;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+
+	ret = pci_enable_device(pci_dev);
+	if (ret)
+		return ret;
+	pci_set_master(pci_dev);
+	vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
+	vp_finalize_features(&vp_dev->vdev);
+	if (drv && drv->restore)
+		ret = drv->restore(&vp_dev->vdev);
+
+	return ret;
+}
+
 static const struct dev_pm_ops virtio_pci_pm_ops = {
 	.suspend = virtio_pci_suspend,
 	.resume  = virtio_pci_resume,
+	.freeze  = virtio_pci_freeze,
+	.thaw    = virtio_pci_restore,
+	.restore = virtio_pci_restore,
+	.poweroff = virtio_pci_suspend,
 };
 #endif

include/linux/virtio.h +4 -0

index 4c069d8..1c26416 100644
@@ -146,6 +146,10 @@ struct virtio_driver {
 	int (*probe)(struct virtio_device *dev);
 	void (*remove)(struct virtio_device *dev);
 	void (*config_changed)(struct virtio_device *dev);
+#ifdef CONFIG_PM
+	int (*freeze)(struct virtio_device *dev);
+	int (*restore)(struct virtio_device *dev);
+#endif
 };
 
 int register_virtio_driver(struct virtio_driver *drv);

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #3 lore raw

PATCH v3 3/11 virtio: console: Move out vq and vq buf removal into separate functions

In reply to Amit Shah

This common code will be shared with the PM freeze function.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/char/virtio_console.c |   68 ++++++++++++++++++++++++-----------------
 1 files changed, 40 insertions(+), 28 deletions(-)

drivers/char/virtio_console.c +40 ~~-28~~

index 8e3c46d..e14f5aa 100644
@@ -1271,6 +1271,20 @@ static void remove_port(struct kref *kref)
 	kfree(port);
 }
 
+static void remove_port_data(struct port *port)
+{
+	struct port_buffer *buf;
+
+	/* Remove unused data this port might have received. */
+	discard_port_data(port);
+
+	reclaim_consumed_buffers(port);
+
+	/* Remove buffers we queued up for the Host to send us data in. */
+	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
+		free_buf(buf);
+}
+
 /*
  * Port got unplugged.  Remove port from portdev's list and drop the
  * kref reference.  If no userspace has this port opened, it will

@@ -1278,8 +1292,6 @@ static void remove_port(struct kref *kref)
  */
 static void unplug_port(struct port *port)
 {
-	struct port_buffer *buf;
-
 	spin_lock_irq(&port->portdev->ports_lock);
 	list_del(&port->list);
 	spin_unlock_irq(&port->portdev->ports_lock);

@@ -1300,14 +1312,7 @@ static void unplug_port(struct port *port)
 		hvc_remove(port->cons.hvc);
 	}
 
-	/* Remove unused data this port might have received. */
-	discard_port_data(port);
-
-	reclaim_consumed_buffers(port);
-
-	/* Remove buffers we queued up for the Host to send us data in. */
-	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+	remove_port_data(port);
 
 	/*
 	 * We should just assume the device itself has gone off --

@@ -1659,6 +1664,28 @@ static const struct file_operations portdev_fops = {
 	.owner = THIS_MODULE,
 };
 
+static void remove_vqs(struct ports_device *portdev)
+{
+	portdev->vdev->config->del_vqs(portdev->vdev);
+	kfree(portdev->in_vqs);
+	kfree(portdev->out_vqs);
+}
+
+static void remove_controlq_data(struct ports_device *portdev)
+{
+	struct port_buffer *buf;
+	unsigned int len;
+
+	if (!use_multiport(portdev))
+		return;
+
+	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
+		free_buf(buf);
+
+	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
+		free_buf(buf);
+}
+
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.

@@ -1764,9 +1791,7 @@ free_vqs:
 	/* The host might want to notify mgmt sw about device add failure */
 	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
 			   VIRTIO_CONSOLE_DEVICE_READY, 0);
-	vdev->config->del_vqs(vdev);
-	kfree(portdev->in_vqs);
-	kfree(portdev->out_vqs);
+	remove_vqs(portdev);
 free_chrdev:
 	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
 free:

@@ -1804,21 +1829,8 @@ static void virtcons_remove(struct virtio_device *vdev)
 	 * have to just stop using the port, as the vqs are going
 	 * away.
 	 */
-	if (use_multiport(portdev)) {
-		struct port_buffer *buf;
-		unsigned int len;
-
-		while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
-			free_buf(buf);
-
-		while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
-			free_buf(buf);
-	}
-
-	vdev->config->del_vqs(vdev);
-	kfree(portdev->in_vqs);
-	kfree(portdev->out_vqs);
-
+	remove_controlq_data(portdev);
+	remove_vqs(portdev);
 	kfree(portdev);
 }

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #4 lore raw

PATCH v3 4/11 virtio: console: Add freeze and restore handlers to support S4

In reply to Amit Shah

Remove all vqs and associated buffers in the freeze callback which prepares us to go into hibernation state. On restore, re-create all the vqs and populate the input vqs with buffers to get to the pre-hibernate state.

Note: Any outstanding unconsumed buffers are discarded; which means there's a possibility of data loss in case the host or the guest didn't consume any data already present in the vqs. This can be addressed in a later patch series, perhaps in virtio common code.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/char/virtio_console.c |   58 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

drivers/char/virtio_console.c +58 -0

index e14f5aa..fd2fd6f 100644
@@ -1844,6 +1844,60 @@ static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_MULTIPORT,
 };
 
+#ifdef CONFIG_PM
+static int virtcons_freeze(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port;
+
+	portdev = vdev->priv;
+
+	vdev->config->reset(vdev);
+
+	cancel_work_sync(&portdev->control_work);
+	remove_controlq_data(portdev);
+
+	list_for_each_entry(port, &portdev->ports, list) {
+		/*
+		 * We'll ask the host later if the new invocation has
+		 * the port opened or closed.
+		 */
+		port->host_connected = false;
+		remove_port_data(port);
+	}
+	remove_vqs(portdev);
+
+	return 0;
+}
+
+static int virtcons_restore(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port;
+	int ret;
+
+	portdev = vdev->priv;
+
+	ret = init_vqs(portdev);
+	if (ret)
+		return ret;
+
+	if (use_multiport(portdev))
+		fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+
+	list_for_each_entry(port, &portdev->ports, list) {
+		port->in_vq = portdev->in_vqs[port->id];
+		port->out_vq = portdev->out_vqs[port->id];
+
+		fill_queue(port->in_vq, &port->inbuf_lock);
+
+		/* Get port open/close status on the host */
+		send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+	}
+	return 0;
+}
+#endif
+
 static struct virtio_driver virtio_console = {
 	.feature_table = features,
 	.feature_table_size = ARRAY_SIZE(features),

@@ -1853,6 +1907,10 @@ static struct virtio_driver virtio_console = {
 	.probe =	virtcons_probe,
 	.remove =	virtcons_remove,
 	.config_changed = config_intr,
+#ifdef CONFIG_PM
+	.freeze =	virtcons_freeze,
+	.restore =	virtcons_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Michael S. Tsirkin

maintainer

2011-11-17 12:29 UTC #5 lore raw

Re: [PATCH v3 04/11] virtio: console: Add freeze and restore handlers to support S4

Quoted 22 lines · Amit Shah

Remove all vqs and associated buffers in the freeze callback which prepares us to go into hibernation state. On restore, re-create all the vqs and populate the input vqs with buffers to get to the pre-hibernate state.
Note: Any outstanding unconsumed buffers are discarded; which means there's a possibility of data loss in case the host or the guest didn't consume any data already present in the vqs. This can be addressed in a later patch series, perhaps in virtio common code.
Signed-off-by Amit Shah <[email protected]>
---
 drivers/char/virtio_console.c |   58 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)
drivers/char/virtio_console.c +0 -0
index e14f5aa..fd2fd6f 100644
@@ -1844,6 +1844,60 @@ static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_MULTIPORT,
 };

drivers/char/virtio_console.c static unsigned int features[] = { · line 1855

Quoted 10 lines

drivers/char/virtio_console.c +9 -0

 
+#ifdef CONFIG_PM
+static int virtcons_freeze(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port;
+
+	portdev = vdev->priv;
+
+	vdev->config->reset(vdev);

This does a reset but that's not a guarantee that
interrupt is not running on another CPU.

drivers/char/virtio_console.c static unsigned int features[] = { · line 1857

Quoted 2 lines

drivers/char/virtio_console.c +2 -0
+
+	cancel_work_sync(&portdev->control_work);

And then work can get scheduled after this point.

drivers/char/virtio_console.c static unsigned int features[] = { · line 1858

Quoted 1 line

drivers/char/virtio_console.c +1 -0
+	remove_controlq_data(portdev);

And after this point this will lead to a use after free.

Quoted 58 lines

drivers/char/virtio_console.c +46 -0

+
+	list_for_each_entry(port, &portdev->ports, list) {
+		/*
+		 * We'll ask the host later if the new invocation has
+		 * the port opened or closed.
+		 */
+		port->host_connected = false;
+		remove_port_data(port);
+	}
+	remove_vqs(portdev);
+
+	return 0;
+}
+
+static int virtcons_restore(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port;
+	int ret;
+
+	portdev = vdev->priv;
+
+	ret = init_vqs(portdev);
+	if (ret)
+		return ret;
+
+	if (use_multiport(portdev))
+		fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+
+	list_for_each_entry(port, &portdev->ports, list) {
+		port->in_vq = portdev->in_vqs[port->id];
+		port->out_vq = portdev->out_vqs[port->id];
+
+		fill_queue(port->in_vq, &port->inbuf_lock);
+
+		/* Get port open/close status on the host */
+		send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+	}
+	return 0;
+}
+#endif
+
 static struct virtio_driver virtio_console = {
 	.feature_table = features,
 	.feature_table_size = ARRAY_SIZE(features),

@@ -1853,6 +1907,10 @@ static struct virtio_driver virtio_console = {
 	.probe =	virtcons_probe,
 	.remove =	virtcons_remove,
 	.config_changed = config_intr,
+#ifdef CONFIG_PM
+	.freeze =	virtcons_freeze,
+	.restore =	virtcons_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #6 lore raw

PATCH v3 5/11 virtio: blk: Move out vq initialization to separate function

In reply to Amit Shah

The probe and PM restore functions will share this code.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/block/virtio_blk.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

drivers/block/virtio_blk.c +14 -5

index 4d0b70a..467f218 100644
@@ -349,6 +349,18 @@ static void virtblk_config_changed(struct virtio_device *vdev)
 	queue_work(virtblk_wq, &vblk->config_work);
 }
 
+static int init_vq(struct virtio_blk *vblk)
+{
+	int err = 0;
+
+	/* We expect one virtqueue, for output. */
+	vblk->vq = virtio_find_single_vq(vblk->vdev, blk_done, "requests");
+	if (IS_ERR(vblk->vq))
+		err = PTR_ERR(vblk->vq);
+
+	return err;
+}
+
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;

@@ -390,12 +402,9 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	sg_init_table(vblk->sg, vblk->sg_elems);
 	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
 
-	/* We expect one virtqueue, for output. */
-	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
-	if (IS_ERR(vblk->vq)) {
-		err = PTR_ERR(vblk->vq);
+	err = init_vq(vblk);
+	if (err)
 		goto out_free_vblk;
-	}
 
 	vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
 	if (!vblk->pool) {

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #7 lore raw

PATCH v3 6/11 virtio: blk: Add freeze, restore handlers to support S4

In reply to Amit Shah

Delete the vq and flush any pending requests from the block queue on the freeze callback to prepare for hibernation.

Re-create the vq in the restore callback to resume normal function.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/block/virtio_blk.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

drivers/block/virtio_blk.c +36 -0

index 467f218..f73fb2d 100644
@@ -568,6 +568,38 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	ida_simple_remove(&vd_index_ida, index);
 }
 
+#ifdef CONFIG_PM
+static int virtblk_freeze(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+
+	flush_work(&vblk->config_work);
+	spin_lock_irq(vblk->disk->queue->queue_lock);
+	blk_stop_queue(vblk->disk->queue);
+	spin_unlock_irq(vblk->disk->queue->queue_lock);
+	blk_sync_queue(vblk->disk->queue);
+
+	/* Stop all the virtqueues. */
+	vdev->config->reset(vdev);
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int virtblk_restore(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+	int ret;
+
+	ret = init_vq(vdev->priv);
+	if (!ret) {
+		spin_lock_irq(vblk->disk->queue->queue_lock);
+		blk_start_queue(vblk->disk->queue);
+		spin_unlock_irq(vblk->disk->queue->queue_lock);
+	}
+	return ret;
+}
+#endif
+
 static const struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
 	{ 0 },

@@ -593,6 +625,10 @@ static struct virtio_driver __refdata virtio_blk = {
 	.probe			= virtblk_probe,
 	.remove			= __devexit_p(virtblk_remove),
 	.config_changed		= virtblk_config_changed,
+#ifdef CONFIG_PM
+	.freeze			= virtblk_freeze,
+	.restore		= virtblk_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:58 UTC #8 lore raw

PATCH v3 6/11 virtio: blk: Add freeze, restore handlers to support S4

In reply to Amit Shah

Delete the vq and flush any pending requests from the block queue on the freeze callback to prepare for hibernation.

Re-create the vq in the restore callback to resume normal function.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/block/virtio_blk.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

drivers/block/virtio_blk.c +36 -0

index 467f218..f73fb2d 100644
@@ -568,6 +568,38 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	ida_simple_remove(&vd_index_ida, index);
 }
 
+#ifdef CONFIG_PM
+static int virtblk_freeze(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+
+	flush_work(&vblk->config_work);
+	spin_lock_irq(vblk->disk->queue->queue_lock);
+	blk_stop_queue(vblk->disk->queue);
+	spin_unlock_irq(vblk->disk->queue->queue_lock);
+	blk_sync_queue(vblk->disk->queue);
+
+	/* Stop all the virtqueues. */
+	vdev->config->reset(vdev);
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int virtblk_restore(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+	int ret;
+
+	ret = init_vq(vdev->priv);
+	if (!ret) {
+		spin_lock_irq(vblk->disk->queue->queue_lock);
+		blk_start_queue(vblk->disk->queue);
+		spin_unlock_irq(vblk->disk->queue->queue_lock);
+	}
+	return ret;
+}
+#endif
+
 static const struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
 	{ 0 },

@@ -593,6 +625,10 @@ static struct virtio_driver __refdata virtio_blk = {
 	.probe			= virtblk_probe,
 	.remove			= __devexit_p(virtblk_remove),
 	.config_changed		= virtblk_config_changed,
+#ifdef CONFIG_PM
+	.freeze			= virtblk_freeze,
+	.restore		= virtblk_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Michael S. Tsirkin

maintainer

2011-11-17 12:27 UTC #9 lore raw

Re: [PATCH v3 06/11] virtio: blk: Add freeze, restore handlers to support S4

Quoted 15 lines · Amit Shah

Delete the vq and flush any pending requests from the block queue on the freeze callback to prepare for hibernation.
Re-create the vq in the restore callback to resume normal function.
Signed-off-by Amit Shah <[email protected]>
---
 drivers/block/virtio_blk.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)
drivers/block/virtio_blk.c +0 -0
index 467f218..f73fb2d 100644

drivers/block/virtio_blk.c static void __devexit virtblk_remove(struct virtio_device *vdev) · line 576

Quoted 9 lines

drivers/block/virtio_blk.c +6 -0

@@ -568,6 +568,38 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	ida_simple_remove(&vd_index_ida, index);
 }
 
+#ifdef CONFIG_PM
+static int virtblk_freeze(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+
+	flush_work(&vblk->config_work);

What if a new config_work is scheduled after this point? The right thing to do seems to be to flush after disabling interrupts.

drivers/block/virtio_blk.c static void __devexit virtblk_remove(struct virtio_device *vdev) · line 584

Quoted 8 lines

drivers/block/virtio_blk.c +8 -0

+	spin_lock_irq(vblk->disk->queue->queue_lock);
+	blk_stop_queue(vblk->disk->queue);
+	spin_unlock_irq(vblk->disk->queue->queue_lock);
+	blk_sync_queue(vblk->disk->queue);
+
+	/* Stop all the virtqueues. */
+	vdev->config->reset(vdev);
+	vdev->config->del_vqs(vdev);

I think del_vqs should be followed by reset:
del_vqs assumes the device in a state find put it in.

Quoted 34 lines

drivers/block/virtio_blk.c +22 -0

+	return 0;
+}
+
+static int virtblk_restore(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+	int ret;
+
+	ret = init_vq(vdev->priv);
+	if (!ret) {
+		spin_lock_irq(vblk->disk->queue->queue_lock);
+		blk_start_queue(vblk->disk->queue);
+		spin_unlock_irq(vblk->disk->queue->queue_lock);
+	}
+	return ret;
+}
+#endif
+
 static const struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
 	{ 0 },

@@ -593,6 +625,10 @@ static struct virtio_driver __refdata virtio_blk = {
 	.probe			= virtblk_probe,
 	.remove			= __devexit_p(virtblk_remove),
 	.config_changed		= virtblk_config_changed,
+#ifdef CONFIG_PM
+	.freeze			= virtblk_freeze,
+	.restore		= virtblk_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #10 lore raw

PATCH v3 7/11 virtio: net: Move out vq initialization into separate function

In reply to Amit Shah

The probe and PM restore functions will share this code.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/net/virtio_net.c |   47 +++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 19 deletions(-)

drivers/net/virtio_net.c +28 ~~-19~~

index 6ee8410..6baa563 100644
@@ -954,15 +954,38 @@ static void virtnet_config_changed(struct virtio_device *vdev)
 	virtnet_update_status(vi);
 }
 
+static int init_vqs(struct virtnet_info *vi)
+{
+	struct virtqueue *vqs[3];
+	vq_callback_t *callbacks[] = { skb_recv_done, skb_xmit_done, NULL};
+	const char *names[] = { "input", "output", "control" };
+	int nvqs, err;
+
+	/* We expect two virtqueues, receive then send,
+	 * and optionally control. */
+	nvqs = virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ? 3 : 2;
+
+	err = vi->vdev->config->find_vqs(vi->vdev, nvqs, vqs, callbacks, names);
+	if (err)
+		return err;
+
+	vi->rvq = vqs[0];
+	vi->svq = vqs[1];
+
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
+		vi->cvq = vqs[2];
+
+		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
+			vi->dev->features |= NETIF_F_HW_VLAN_FILTER;
+	}
+	return 0;
+}
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int err;
 	struct net_device *dev;
 	struct virtnet_info *vi;
-	struct virtqueue *vqs[3];
-	vq_callback_t *callbacks[] = { skb_recv_done, skb_xmit_done, NULL};
-	const char *names[] = { "input", "output", "control" };
-	int nvqs;
 
 	/* Allocate ourselves a network device with room for our info */
 	dev = alloc_etherdev(sizeof(struct virtnet_info));

@@ -1034,24 +1057,10 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
 		vi->mergeable_rx_bufs = true;
 
-	/* We expect two virtqueues, receive then send,
-	 * and optionally control. */
-	nvqs = virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ? 3 : 2;
-
-	err = vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names);
+	err = init_vqs(vi);
 	if (err)
 		goto free_stats;
 
-	vi->rvq = vqs[0];
-	vi->svq = vqs[1];
-
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
-		vi->cvq = vqs[2];
-
-		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
-			dev->features |= NETIF_F_HW_VLAN_FILTER;
-	}
-
 	err = register_netdev(dev);
 	if (err) {
 		pr_debug("virtio_net: registering device failed\n");

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #11 lore raw

PATCH v3 8/11 virtio: net: Move out vq and vq buf removal into separate function

In reply to Amit Shah

The remove and PM freeze functions will share this code.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/net/virtio_net.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

drivers/net/virtio_net.c +14 -9

index 6baa563..fbff37a 100644
@@ -1123,24 +1123,29 @@ static void free_unused_bufs(struct virtnet_info *vi)
 	BUG_ON(vi->num != 0);
 }
 
-static void __devexit virtnet_remove(struct virtio_device *vdev)
+static void remove_vq_common(struct virtnet_info *vi)
 {
-	struct virtnet_info *vi = vdev->priv;
-
-	/* Stop all the virtqueues. */
-	vdev->config->reset(vdev);
-
-
-	unregister_netdev(vi->dev);
 	cancel_delayed_work_sync(&vi->refill);
 
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);
 
-	vdev->config->del_vqs(vi->vdev);
+	vi->vdev->config->del_vqs(vi->vdev);
 
 	while (vi->pages)
 		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
+}
+
+static void __devexit virtnet_remove(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	/* Stop all the virtqueues. */
+	vdev->config->reset(vdev);
+
+	unregister_netdev(vi->dev);
+
+	remove_vq_common(vi);
 
 	free_percpu(vi->stats);
 	free_netdev(vi->dev);

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #12 lore raw

PATCH v3 9/11 virtio: net: Add freeze, restore handlers to support S4

In reply to Amit Shah

Remove all the vqs and detach from the netdev on hibernation.

Re-create vqs after restoring from a hibernated image and re-attach the netdev. This keeps networking working across hibernation.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/net/virtio_net.c |   37 ++++++++++++++++++++++++++++++++++---
 1 files changed, 34 insertions(+), 3 deletions(-)

drivers/net/virtio_net.c +34 -3

index fbff37a..167b555 100644
@@ -1127,6 +1127,9 @@ static void remove_vq_common(struct virtnet_info *vi)
 {
 	cancel_delayed_work_sync(&vi->refill);
 
+	/* Stop all the virtqueues. */
+	vi->vdev->config->reset(vi->vdev);
+
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);

@@ -1140,9 +1143,6 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;
 
-	/* Stop all the virtqueues. */
-	vdev->config->reset(vdev);
-
 	unregister_netdev(vi->dev);
 
 	remove_vq_common(vi);

@@ -1151,6 +1151,33 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
 	free_netdev(vi->dev);
 }
 
+#ifdef CONFIG_PM
+static int virtnet_freeze(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	netif_device_detach(vi->dev);
+	remove_vq_common(vi);
+
+	return 0;
+}
+
+static int virtnet_restore(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+	int err;
+
+	err = init_vqs(vi);
+	if (err)
+		return err;
+
+	try_fill_recv(vi, GFP_KERNEL);
+
+	netif_device_attach(vi->dev);
+	return 0;
+}
+#endif
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
 	{ 0 },

@@ -1175,6 +1202,10 @@ static struct virtio_driver virtio_net_driver = {
 	.probe =	virtnet_probe,
 	.remove =	__devexit_p(virtnet_remove),
 	.config_changed = virtnet_config_changed,
+#ifdef CONFIG_PM
+	.freeze =	virtnet_freeze,
+	.restore =	virtnet_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Michael S. Tsirkin

maintainer

2011-11-17 12:17 UTC #13 lore raw

Re: [PATCH v3 09/11] virtio: net: Add freeze, restore handlers to support S4

Quoted 35 lines · Amit Shah

Remove all the vqs and detach from the netdev on hibernation.

Re-create vqs after restoring from a hibernated image and re-attach the netdev. This keeps networking working across hibernation.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/net/virtio_net.c |   37 ++++++++++++++++++++++++++++++++++---
 1 files changed, 34 insertions(+), 3 deletions(-)

drivers/net/virtio_net.c +3 -3

index fbff37a..167b555 100644
@@ -1127,6 +1127,9 @@ static void remove_vq_common(struct virtnet_info *vi)
 {
 	cancel_delayed_work_sync(&vi->refill);
 
+	/* Stop all the virtqueues. */
+	vi->vdev->config->reset(vi->vdev);
+
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);

@@ -1140,9 +1143,6 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;
 
-	/* Stop all the virtqueues. */
-	vdev->config->reset(vdev);
-
 	unregister_netdev(vi->dev);
 
 	remove_vq_common(vi);

drivers/net/virtio_net.c static void __devexit virtnet_remove(struct virtio_device *vdev) · line 1160

Quoted 10 lines

drivers/net/virtio_net.c +7 -0

@@ -1151,6 +1151,33 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
 	free_netdev(vi->dev);
 }
 
+#ifdef CONFIG_PM
+static int virtnet_freeze(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	netif_device_detach(vi->dev);
+	remove_vq_common(vi);

This stops TX in progress, if any, but not RX which might use the RX VQ. Then remove_vq_common might delete this VQ while it's still in use.

So I think we need to call something like napi_disable. However, the subtle twist is that we need to call that *after interrupts have been disabled*. Otherwise we might schedule another napi callback.

Quoted 36 lines

drivers/net/virtio_net.c +24 -0

+
+	return 0;
+}
+
+static int virtnet_restore(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+	int err;
+
+	err = init_vqs(vi);
+	if (err)
+		return err;
+
+	try_fill_recv(vi, GFP_KERNEL);
+
+	netif_device_attach(vi->dev);
+	return 0;
+}
+#endif
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
 	{ 0 },

@@ -1175,6 +1202,10 @@ static struct virtio_driver virtio_net_driver = {
 	.probe =	virtnet_probe,
 	.remove =	__devexit_p(virtnet_remove),
 	.config_changed = virtnet_config_changed,
+#ifdef CONFIG_PM
+	.freeze =	virtnet_freeze,
+	.restore =	virtnet_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 12:27 UTC #14 lore raw

Re: [PATCH v3 09/11] virtio: net: Add freeze, restore handlers to support S4

Quoted 14 lines · Michael S. Tsirkin

drivers/net/virtio_net.c static void __devexit virtnet_remove(struct virtio_device *vdev) · line 1160
Quoted 7 lines · Amit Shah
drivers/net/virtio_net.c +7 -0
+#ifdef CONFIG_PM
+static int virtnet_freeze(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	netif_device_detach(vi->dev);
+	remove_vq_common(vi);
This stops TX in progress, if any, but not RX which might use the RX VQ. Then remove_vq_common might delete this VQ while it's still in use.

Quoted 4 lines

So I think we need to call something like napi_disable. However, the subtle twist is that we need to call that *after interrupts have been disabled*. Otherwise we might schedule another napi callback.

resetting the vqs will mean the host won't pass us any data in the vqs. Plus we're removing the vqs altogether. Also, we're disabling the pci device in virtio_pci.c, so all of these actions will take care of that, isn't it?

In addition, once the vqs are taken off, there's no chance for any other rx to happen, so napi_disable() after plugging off vqs doesn't make sense.

Amit

Michael S. Tsirkin

maintainer

2011-11-17 12:31 UTC #15 lore raw

Re: [PATCH v3 09/11] virtio: net: Add freeze, restore handlers to support S4

Quoted 20 lines · Amit Shah

Quoted 14 lines · Michael S. Tsirkin
drivers/net/virtio_net.c static void __devexit virtnet_remove(struct virtio_device *vdev) · line 1160
Quoted 7 lines · Amit Shah
drivers/net/virtio_net.c +7 -0
+#ifdef CONFIG_PM
+static int virtnet_freeze(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	netif_device_detach(vi->dev);
+	remove_vq_common(vi);
This stops TX in progress, if any, but not RX which might use the RX VQ. Then remove_vq_common might delete this VQ while it's still in use.
Quoted 4 lines
So I think we need to call something like napi_disable. However, the subtle twist is that we need to call that *after interrupts have been disabled*. Otherwise we might schedule another napi callback.

Quoted 4 lines

resetting the vqs will mean the host won't pass us any data in the vqs. Plus we're removing the vqs altogether. Also, we're disabling the pci device in virtio_pci.c, so all of these actions will take care of that, isn't it?

I don't think so.

Quoted 5 lines

In addition, once the vqs are taken off, there's no chance for any other rx to happen, so napi_disable() after plugging off vqs doesn't make sense.
Amit

Yes but napi might have been scheduled before remove_vq_common was called, and run afterwards.

Signature · 1 line

MST

Amit Shah

author

2011-11-17 11:57 UTC #16 lore raw

PATCH v3 10/11 virtio: balloon: Move out vq initialization into separate function

In reply to Amit Shah

The probe and PM restore functions will share this code.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/virtio/virtio_balloon.c |   48 ++++++++++++++++++++++++--------------
 1 files changed, 30 insertions(+), 18 deletions(-)

drivers/virtio/virtio_balloon.c +30 ~~-18~~

index 94fd738..1ff3cf4 100644
@@ -275,32 +275,21 @@ static int balloon(void *_vballoon)
 	return 0;
 }
 
-static int virtballoon_probe(struct virtio_device *vdev)
+static int init_vqs(struct virtio_balloon *vb)
 {
-	struct virtio_balloon *vb;
 	struct virtqueue *vqs[3];
 	vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
 	const char *names[] = { "inflate", "deflate", "stats" };
 	int err, nvqs;
 
-	vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
-	if (!vb) {
-		err = -ENOMEM;
-		goto out;
-	}
-
-	INIT_LIST_HEAD(&vb->pages);
-	vb->num_pages = 0;
-	init_waitqueue_head(&vb->config_change);
-	vb->vdev = vdev;
-	vb->need_stats_update = 0;
-
-	/* We expect two virtqueues: inflate and deflate,
-	 * and optionally stat. */
+	/*
+	 * We expect two virtqueues: inflate and deflate, and
+	 * optionally stat.
+	 */
 	nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
-	err = vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names);
+	err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names);
 	if (err)
-		goto out_free_vb;
+		return err;
 
 	vb->inflate_vq = vqs[0];
 	vb->deflate_vq = vqs[1];

@@ -317,6 +306,29 @@ static int virtballoon_probe(struct virtio_device *vdev)
 			BUG();
 		virtqueue_kick(vb->stats_vq);
 	}
+	return 0;
+}
+
+static int virtballoon_probe(struct virtio_device *vdev)
+{
+	struct virtio_balloon *vb;
+	int err;
+
+	vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
+	if (!vb) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&vb->pages);
+	vb->num_pages = 0;
+	init_waitqueue_head(&vb->config_change);
+	vb->vdev = vdev;
+	vb->need_stats_update = 0;
+
+	err = init_vqs(vb);
+	if (err)
+		goto out_free_vb;
 
 	vb->thread = kthread_run(balloon, vb, "vballoon");
 	if (IS_ERR(vb->thread)) {

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 11:57 UTC #17 lore raw

PATCH v3 11/11 virtio: balloon: Add freeze, restore handlers to support S4

In reply to Amit Shah

Delete the vqs on the freeze callback to prepare for hibernation. Re-create the vqs in the restore callback to resume normal function.

Signed-off-by Amit Shah <[email protected]>

---
 drivers/virtio/virtio_balloon.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

drivers/virtio/virtio_balloon.c +20 -0

index 1ff3cf4..90149d1 100644
@@ -363,6 +363,22 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
+#ifdef CONFIG_PM
+static int virtballoon_freeze(struct virtio_device *vdev)
+{
+	/* Now we reset the device so we can clean up the queues. */
+	vdev->config->reset(vdev);
+
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int virtballoon_restore(struct virtio_device *vdev)
+{
+	return init_vqs(vdev->priv);
+}
+#endif
+
 static unsigned int features[] = {
 	VIRTIO_BALLOON_F_MUST_TELL_HOST,
 	VIRTIO_BALLOON_F_STATS_VQ,

@@ -377,6 +393,10 @@ static struct virtio_driver virtio_balloon_driver = {
 	.probe =	virtballoon_probe,
 	.remove =	__devexit_p(virtballoon_remove),
 	.config_changed = virtballoon_changed,
+#ifdef CONFIG_PM
+	.freeze	=	virtballoon_freeze,
+	.restore =	virtballoon_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Michael S. Tsirkin

maintainer

2011-11-17 12:23 UTC #18 lore raw

Re: [PATCH v3 11/11] virtio: balloon: Add freeze, restore handlers to support S4

Quoted 13 lines · Amit Shah

Delete the vqs on the freeze callback to prepare for hibernation. Re-create the vqs in the restore callback to resume normal function.
Signed-off-by Amit Shah <[email protected]>
---
 drivers/virtio/virtio_balloon.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
drivers/virtio/virtio_balloon.c +0 -0
index 1ff3cf4..90149d1 100644

drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 371

Quoted 9 lines

drivers/virtio/virtio_balloon.c +6 -0

@@ -363,6 +363,22 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
+#ifdef CONFIG_PM
+static int virtballoon_freeze(struct virtio_device *vdev)
+{
+	/* Now we reset the device so we can clean up the queues. */
+	vdev->config->reset(vdev);
+

This is a weird thing to do. We normally delete vqs then reset. For example, I don't know whether we guarantee what happens to MSI-X vectors assigned meanwhile if you reset. IIRC qemu doesn't use MSI-X for the balloon, but nothing prevents it.

drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 372

Quoted 1 line

drivers/virtio/virtio_balloon.c +1 -0
+	vdev->config->del_vqs(vdev);

What prevents requests being submitted at this point? I see all kind of handling of freezing in the balloon thread ... maybe that gives us some guarantees, but if yes
it makes sense to add a comment to point this out.

Quoted 25 lines

drivers/virtio/virtio_balloon.c +13 -0

+	return 0;
+}
+
+static int virtballoon_restore(struct virtio_device *vdev)
+{
+	return init_vqs(vdev->priv);
+}
+#endif
+
 static unsigned int features[] = {
 	VIRTIO_BALLOON_F_MUST_TELL_HOST,
 	VIRTIO_BALLOON_F_STATS_VQ,

@@ -377,6 +393,10 @@ static struct virtio_driver virtio_balloon_driver = {
 	.probe =	virtballoon_probe,
 	.remove =	__devexit_p(virtballoon_remove),
 	.config_changed = virtballoon_changed,
+#ifdef CONFIG_PM
+	.freeze	=	virtballoon_freeze,
+	.restore =	virtballoon_restore,
+#endif
 };
 
 static int __init init(void)

Signature · 1 line

1.7.7.1

Amit Shah

author

2011-11-17 12:29 UTC #19 lore raw

Re: [PATCH v3 11/11] virtio: balloon: Add freeze, restore handlers to support S4

Quoted 17 lines · Michael S. Tsirkin

Quoted 16 lines · Amit Shah
Delete the vqs on the freeze callback to prepare for hibernation. Re-create the vqs in the restore callback to resume normal function.
Signed-off-by Amit Shah <[email protected]>
---
 drivers/virtio/virtio_balloon.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
drivers/virtio/virtio_balloon.c +0 -0
index 1ff3cf4..90149d1 100644
@@ -363,6 +363,22 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 

Quoted 8 lines

drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 371
Quoted 6 lines
drivers/virtio/virtio_balloon.c +6 -0
+#ifdef CONFIG_PM
+static int virtballoon_freeze(struct virtio_device *vdev)
+{
+	/* Now we reset the device so we can clean up the queues. */
+	vdev->config->reset(vdev);
+
This is a weird thing to do. We normally delete vqs then reset.

No, all the drivers first reset, then delete.

Quoted 11 lines

For example, I don't know whether we guarantee what happens to MSI-X vectors assigned meanwhile if you reset. IIRC qemu doesn't use MSI-X for the balloon, but nothing prevents it.
drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 372
Quoted 1 line
drivers/virtio/virtio_balloon.c +1 -0
+	vdev->config->del_vqs(vdev);
What prevents requests being submitted at this point? I see all kind of handling of freezing in the balloon thread ... maybe that gives us some guarantees, but if yes
it makes sense to add a comment to point this out.

Once reset, the host won't write anything to the vqs anyway..

Amit

Michael S. Tsirkin

maintainer

2011-11-17 12:35 UTC #20 lore raw

Re: [PATCH v3 11/11] virtio: balloon: Add freeze, restore handlers to support S4

Quoted 27 lines · Amit Shah

Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Amit Shah
Delete the vqs on the freeze callback to prepare for hibernation. Re-create the vqs in the restore callback to resume normal function.
Signed-off-by Amit Shah <[email protected]>
---
 drivers/virtio/virtio_balloon.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
drivers/virtio/virtio_balloon.c +0 -0
index 1ff3cf4..90149d1 100644
@@ -363,6 +363,22 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
Quoted 8 lines
drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 371
Quoted 6 lines
drivers/virtio/virtio_balloon.c +6 -0
+#ifdef CONFIG_PM
+static int virtballoon_freeze(struct virtio_device *vdev)
+{
+	/* Now we reset the device so we can clean up the queues. */
+	vdev->config->reset(vdev);
+
This is a weird thing to do. We normally delete vqs then reset.

Quoted 1 line

No, all the drivers first reset, then delete.

BTW, reset does not flush callbacks at the moment. It probably should.

Quoted 12 lines

Quoted 11 lines
For example, I don't know whether we guarantee what happens to MSI-X vectors assigned meanwhile if you reset. IIRC qemu doesn't use MSI-X for the balloon, but nothing prevents it.
drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 372
Quoted 1 line
drivers/virtio/virtio_balloon.c +1 -0
+	vdev->config->del_vqs(vdev);
What prevents requests being submitted at this point? I see all kind of handling of freezing in the balloon thread ... maybe that gives us some guarantees, but if yes
it makes sense to add a comment to point this out.

Quoted 3 lines

Once reset, the host won't write anything to the vqs anyway..
Amit

But thread might have already been scheduled, handling previous requests.

Signature · 1 line

MST

Michael S. Tsirkin

maintainer

2011-11-17 13:02 UTC #21 lore raw

Re: [PATCH v3 11/11] virtio: balloon: Add freeze, restore handlers to support S4

In reply to Amit Shah

Quoted 27 lines · Amit Shah

Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Amit Shah
Delete the vqs on the freeze callback to prepare for hibernation. Re-create the vqs in the restore callback to resume normal function.
Signed-off-by Amit Shah <[email protected]>
---
 drivers/virtio/virtio_balloon.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
drivers/virtio/virtio_balloon.c +0 -0
index 1ff3cf4..90149d1 100644
@@ -363,6 +363,22 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
Quoted 8 lines
drivers/virtio/virtio_balloon.c static void __devexit virtballoon_remove(struct virtio_device *vdev) · line 371
Quoted 6 lines
drivers/virtio/virtio_balloon.c +6 -0
+#ifdef CONFIG_PM
+static int virtballoon_freeze(struct virtio_device *vdev)
+{
+	/* Now we reset the device so we can clean up the queues. */
+	vdev->config->reset(vdev);
+
This is a weird thing to do. We normally delete vqs then reset.

Quoted 1 line

No, all the drivers first reset, then delete.

Interesting. I note that for PCI, reset actually just did an I/O write, which in PCI is really posted, that is it
can complete on CPU before the device has received it.

Further, interrupts might have been pending on the CPU, so device might get used after reset.

Patch coming.

Signature · 1 line

MST