[PATCH 00/11] virtio: Support for hibernation (S4)

[PATCH 00/11] virtio: Support for hibernation (S4)

[Amit Shah]

Hello,

These patches add support for S4 to virtio (pci) and all drivers.  The
patches are based on the virtio-console patch series in Rusty's queue.

For each driver, all vqs are removed before hibernation, and then
re-created after restore.

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.  One packet is lost.
  (Current qemu.git has a regression in slirp code causing qemu
  segfault, commit 1ab74cea060 is the offender).

* 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 in all, this looks pretty good.

Please review and apply.


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      |   36 ++++++++++--
 drivers/char/virtio_console.c   |  124 ++++++++++++++++++++++++++++++---------
 drivers/net/virtio_net.c        |   98 ++++++++++++++++++++++---------
 drivers/virtio/virtio_balloon.c |   65 +++++++++++++++------
 drivers/virtio/virtio_pci.c     |   57 +++++++++++++++++-
 include/linux/virtio.h          |    4 +
 6 files changed, 301 insertions(+), 83 deletions(-)

-- 
1.7.6.2

[PATCH 01/11] virtio: pci: switch to new PM API

[Amit Shah]

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 <amit.shah@redhat.com>
---
 drivers/virtio/virtio_pci.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 4bcc8b8..6947620 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -685,19 +685,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 = {
@@ -706,8 +715,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
 };
 
-- 
1.7.6.2

[PATCH 02/11] virtio-pci: add PM notification handlers for restore, freeze, thaw, poweroff

[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.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_pci.c |   41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h      |    4 ++++
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 6947620..cc70662 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -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;
 };
@@ -703,9 +707,46 @@ 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;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+
+	vp_dev->saved_status = vp_get_status(&vp_dev->vdev);
+	if (drv && drv->freeze)
+		return drv->freeze(&vp_dev->vdev);
+
+	return 0;
+}
+
+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;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+
+	vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
+	vp_finalize_features(&vp_dev->vdev);
+	if (drv && drv->restore)
+		return drv->restore(&vp_dev->vdev);
+
+	return 0;
+}
+
 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
 
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 7108857..9afb662 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -141,6 +141,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);
-- 
1.7.6.2

Re: [PATCH 02/11] virtio-pci: add PM notification handlers for restore, freeze, thaw, poweroff

[Sasha Levin]

On Thu, 2011-09-29 at 20:55 +0530, Amit Shah wrote:
> 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.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/virtio/virtio_pci.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/virtio.h      |    4 ++++
>  2 files changed, 45 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 6947620..cc70662 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -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;
>  };
> @@ -703,9 +707,46 @@ 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;
> +
> +	drv = container_of(vp_dev->vdev.dev.driver,
> +			   struct virtio_driver, driver);
> +
> +	vp_dev->saved_status = vp_get_status(&vp_dev->vdev);
> +	if (drv && drv->freeze)
> +		return drv->freeze(&vp_dev->vdev);
> +
> +	return 0;
> +}

After drv->freeze() completes, the vq is freed from guest memory, but is
there anything that prevents from the device to keep writing into that
vq?

Shouldn't we set status to '0' before we free vqs?

-- 

Sasha.

Re: [PATCH 02/11] virtio-pci: add PM notification handlers for restore, freeze, thaw, poweroff

[Amit Shah]

On (Thu) 29 Sep 2011 [21:36:38], Sasha Levin wrote:
> 
> After drv->freeze() completes, the vq is freed from guest memory, but is
> there anything that prevents from the device to keep writing into that
> vq?
> 
> Shouldn't we set status to '0' before we free vqs?

Yes, this should be done in the driver code though, not in common
code.  I've fixed this in my tree.

Thanks,

		Amit

[PATCH 03/11] virtio: console: Move out vq and vq buf removal into separate functions

[Amit Shah]

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

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   68 ++++++++++++++++++++++++-----------------
 1 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7f2c6e5..1bef060 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1270,6 +1270,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
@@ -1277,8 +1291,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);
@@ -1299,14 +1311,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 --
@@ -1658,6 +1663,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.
@@ -1765,9 +1792,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:
@@ -1805,21 +1830,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);
 }
 
-- 
1.7.6.2

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

[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 <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   56 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1bef060..d85d827 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1845,6 +1845,58 @@ 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;
+
+	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),
@@ -1854,6 +1906,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)
-- 
1.7.6.2

[PATCH 05/11] virtio: blk: Move out vq initialization to separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/block/virtio_blk.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..509760b 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -337,6 +337,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;
@@ -375,12 +387,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) {
-- 
1.7.6.2

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

[Amit Shah]

Delete the vq 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 <amit.shah@redhat.com>
---
 drivers/block/virtio_blk.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 509760b..9844d2c 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -549,6 +549,19 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	kfree(vblk);
 }
 
+#ifdef CONFIG_PM
+static int virtblk_freeze(struct virtio_device *vdev)
+{
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int virtblk_restore(struct virtio_device *vdev)
+{
+	return init_vq(vdev->priv);;
+}
+#endif
+
 static const struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -574,6 +587,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)
-- 
1.7.6.2

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

[Michael S. Tsirkin]

On Thu, Sep 29, 2011 at 08:56:02PM +0530, Amit Shah wrote:
> Delete the vq 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 <amit.shah@redhat.com>
> ---
>  drivers/block/virtio_blk.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 509760b..9844d2c 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -549,6 +549,19 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
>  	kfree(vblk);
>  }
>  
> +#ifdef CONFIG_PM
> +static int virtblk_freeze(struct virtio_device *vdev)
> +{
> +	vdev->config->del_vqs(vdev);

What prevents the guest from trying to use the VQ while
this is in progress? IF it does, what prevents a crash?

> +	return 0;
> +}
> +
> +static int virtblk_restore(struct virtio_device *vdev)
> +{
> +	return init_vq(vdev->priv);;
> +}
> +#endif
> +
>  static const struct virtio_device_id id_table[] = {
>  	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
>  	{ 0 },
> @@ -574,6 +587,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)
> -- 
> 1.7.6.2

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

[Amit Shah]

On (Sun) 02 Oct 2011 [11:37:33], Michael S. Tsirkin wrote:
> On Thu, Sep 29, 2011 at 08:56:02PM +0530, Amit Shah wrote:
> > Delete the vq 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 <amit.shah@redhat.com>
> > ---
> >  drivers/block/virtio_blk.c |   17 +++++++++++++++++
> >  1 files changed, 17 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 509760b..9844d2c 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -549,6 +549,19 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> >  	kfree(vblk);
> >  }
> >  
> > +#ifdef CONFIG_PM
> > +static int virtblk_freeze(struct virtio_device *vdev)
> > +{
> > +	vdev->config->del_vqs(vdev);
> 
> What prevents the guest from trying to use the VQ while
> this is in progress? IF it does, what prevents a crash?

An oversight: Sasha pointed this out, too, and I've now fixed this by
adding a call to vdev->config->reset() before deleting vqs.

		Amit

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

[Michael S. Tsirkin]

On Mon, Oct 03, 2011 at 02:46:46PM +0530, Amit Shah wrote:
> On (Sun) 02 Oct 2011 [11:37:33], Michael S. Tsirkin wrote:
> > On Thu, Sep 29, 2011 at 08:56:02PM +0530, Amit Shah wrote:
> > > Delete the vq 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 <amit.shah@redhat.com>
> > > ---
> > >  drivers/block/virtio_blk.c |   17 +++++++++++++++++
> > >  1 files changed, 17 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > index 509760b..9844d2c 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c
> > > @@ -549,6 +549,19 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> > >  	kfree(vblk);
> > >  }
> > >  
> > > +#ifdef CONFIG_PM
> > > +static int virtblk_freeze(struct virtio_device *vdev)
> > > +{
> > > +	vdev->config->del_vqs(vdev);
> > 
> > What prevents the guest from trying to use the VQ while
> > this is in progress? IF it does, what prevents a crash?
> 
> An oversight: Sasha pointed this out, too, and I've now fixed this by
> adding a call to vdev->config->reset() before deleting vqs.
> 
> 		Amit

This stops the host from accessing the VQ, but does not stop the *guest*.

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

[Amit Shah]

On (Mon) 03 Oct 2011 [11:24:44], Michael S. Tsirkin wrote:
> On Mon, Oct 03, 2011 at 02:46:46PM +0530, Amit Shah wrote:
> > On (Sun) 02 Oct 2011 [11:37:33], Michael S. Tsirkin wrote:
> > > On Thu, Sep 29, 2011 at 08:56:02PM +0530, Amit Shah wrote:
> > > > Delete the vq 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 <amit.shah@redhat.com>
> > > > ---
> > > >  drivers/block/virtio_blk.c |   17 +++++++++++++++++
> > > >  1 files changed, 17 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > > index 509760b..9844d2c 100644
> > > > --- a/drivers/block/virtio_blk.c
> > > > +++ b/drivers/block/virtio_blk.c
> > > > @@ -549,6 +549,19 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> > > >  	kfree(vblk);
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_PM
> > > > +static int virtblk_freeze(struct virtio_device *vdev)
> > > > +{
> > > > +	vdev->config->del_vqs(vdev);
> > > 
> > > What prevents the guest from trying to use the VQ while
> > > this is in progress? IF it does, what prevents a crash?
> > 
> > An oversight: Sasha pointed this out, too, and I've now fixed this by
> > adding a call to vdev->config->reset() before deleting vqs.
> 
> This stops the host from accessing the VQ, but does not stop the *guest*.

Yes, have to ensure all pending IO is complete and nothing new is
queued.


		Amit

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

[Michael S. Tsirkin]

On Mon, Oct 03, 2011 at 03:01:05PM +0530, Amit Shah wrote:
> On (Mon) 03 Oct 2011 [11:24:44], Michael S. Tsirkin wrote:
> > On Mon, Oct 03, 2011 at 02:46:46PM +0530, Amit Shah wrote:
> > > On (Sun) 02 Oct 2011 [11:37:33], Michael S. Tsirkin wrote:
> > > > On Thu, Sep 29, 2011 at 08:56:02PM +0530, Amit Shah wrote:
> > > > > Delete the vq 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 <amit.shah@redhat.com>
> > > > > ---
> > > > >  drivers/block/virtio_blk.c |   17 +++++++++++++++++
> > > > >  1 files changed, 17 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > > > index 509760b..9844d2c 100644
> > > > > --- a/drivers/block/virtio_blk.c
> > > > > +++ b/drivers/block/virtio_blk.c
> > > > > @@ -549,6 +549,19 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> > > > >  	kfree(vblk);
> > > > >  }
> > > > >  
> > > > > +#ifdef CONFIG_PM
> > > > > +static int virtblk_freeze(struct virtio_device *vdev)
> > > > > +{
> > > > > +	vdev->config->del_vqs(vdev);
> > > > 
> > > > What prevents the guest from trying to use the VQ while
> > > > this is in progress? IF it does, what prevents a crash?
> > > 
> > > An oversight: Sasha pointed this out, too, and I've now fixed this by
> > > adding a call to vdev->config->reset() before deleting vqs.
> > 
> > This stops the host from accessing the VQ, but does not stop the *guest*.
> 
> Yes, have to ensure all pending IO is complete and nothing new is
> queued.
> 
> 
> 		Amit

reset will effectively cancel pending requests, maybe that
will help.

Also, same concern applies to all virtio devices really.

-- 
MST

[PATCH 07/11] virtio: net: Move out vq initialization into separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   47 +++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..da4c51a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -933,15 +933,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));
@@ -1014,24 +1037,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");
-- 
1.7.6.2

[PATCH 08/11] virtio: net: Move out vq and vq buf removal into separate function

[Amit Shah]

The remove and PM freeze functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index da4c51a..dcd4b01 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1103,24 +1103,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);
-- 
1.7.6.2

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

[Amit Shah]

Remove all the vqs on hibernation and re-create them after restoring
from a hibernated image.  This keeps networking working across
hibernation.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dcd4b01..d4391c0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1131,6 +1131,30 @@ 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;
+
+	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);
+	return 0;
+}
+#endif
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -1155,6 +1179,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)
-- 
1.7.6.2

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

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_balloon.c |   48 ++++++++++++++++++++++++--------------
 1 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index e058ace..571d443 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -274,32 +274,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];
@@ -316,6 +305,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)) {
-- 
1.7.6.2

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

[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 <amit.shah@redhat.com>
---
 drivers/virtio/virtio_balloon.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 571d443..3ff955f9 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -362,6 +362,19 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
+#ifdef CONFIG_PM
+static int virtballoon_freeze(struct virtio_device *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,
@@ -376,6 +389,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)
-- 
1.7.6.2

Re: [PATCH 00/11] virtio: Support for hibernation (S4)

[Amit Shah]

On (Thu) 29 Sep 2011 [20:55:56], Amit Shah wrote:
> Hello,
> 
> These patches add support for S4 to virtio (pci) and all drivers.  The
> patches are based on the virtio-console patch series in Rusty's queue.

Some patches have checkpatch warnings, re-sending those as v2.

		Amit

Re: [PATCH 00/11] virtio: Support for hibernation (S4)

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Thu, 29 Sep 2011 21:19:25 +0530, Amit Shah said:
> On (Thu) 29 Sep 2011 [20:55:56], Amit Shah wrote:
> > Hello,
> > 
> > These patches add support for S4 to virtio (pci) and all drivers.  The
> > patches are based on the virtio-console patch series in Rusty's queue.
> 
> Some patches have checkpatch warnings, re-sending those as v2.

When doing something like this, don't say "some" - say "patches 4,5, 6, and 9".
Otherwise we're looking at 11 patches from the first set, 4 replacements, and
no way to tell if we've missed a 5th replacement patch.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 00/11] virtio: Support for hibernation (S4)

[Amit Shah]

On (Thu) 29 Sep 2011 [12:48:47], Valdis.Kletnieks@vt.edu wrote:
> On Thu, 29 Sep 2011 21:19:25 +0530, Amit Shah said:
> > On (Thu) 29 Sep 2011 [20:55:56], Amit Shah wrote:
> > > Hello,
> > > 
> > > These patches add support for S4 to virtio (pci) and all drivers.  The
> > > patches are based on the virtio-console patch series in Rusty's queue.
> > 
> > Some patches have checkpatch warnings, re-sending those as v2.
> 
> When doing something like this, don't say "some" - say "patches 4,5, 6, and 9".
> Otherwise we're looking at 11 patches from the first set, 4 replacements, and
> no way to tell if we've missed a 5th replacement patch.

Makes sense, will do that in the future, thanks.

I've sent out v2 of patches 4, 5, 6 and 9.

		Amit

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

[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 <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   55 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1bef060..e0b3a28 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1845,6 +1845,57 @@ 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;
+
+	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),
@@ -1854,6 +1905,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)
-- 
1.7.6.2

[PATCH v2 05/11] virtio: blk: Move out vq initialization to separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/block/virtio_blk.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..717664a 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -337,6 +337,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;
@@ -375,12 +387,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) {
-- 
1.7.6.2

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

[Amit Shah]

Delete the vq 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 <amit.shah@redhat.com>
---
 drivers/block/virtio_blk.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 717664a..b01930c 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -549,6 +549,19 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	kfree(vblk);
 }
 
+#ifdef CONFIG_PM
+static int virtblk_freeze(struct virtio_device *vdev)
+{
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int virtblk_restore(struct virtio_device *vdev)
+{
+	return init_vq(vdev->priv);
+}
+#endif
+
 static const struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -574,6 +587,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)
-- 
1.7.6.2

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

[Amit Shah]

Remove all the vqs on hibernation and re-create them after restoring
from a hibernated image.  This keeps networking working across
hibernation.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dcd4b01..8b9ed43 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1131,6 +1131,30 @@ 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;
+
+	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);
+	return 0;
+}
+#endif
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -1155,6 +1179,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)
-- 
1.7.6.2

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

[Michael S. Tsirkin]

On Thu, Sep 29, 2011 at 09:19:40PM +0530, Amit Shah wrote:
> Remove all the vqs on hibernation and re-create them after restoring
> from a hibernated image.  This keeps networking working across
> hibernation.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index dcd4b01..8b9ed43 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1131,6 +1131,30 @@ 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;

I'm guessing we need to do something like netif_device_detach here,
otherwise guest might be in the process of using the vq for transmit at
this point.

I think we also must make sure NAPI RX handler is not in progress.

We also might need to mask interrupts from the device
to prevent TX or RX from getting rescheduled?


> +
> +	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);
> +	return 0;
> +}
> +#endif
> +
>  static struct virtio_device_id id_table[] = {
>  	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
>  	{ 0 },
> @@ -1155,6 +1179,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)
> -- 
> 1.7.6.2

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

[Amit Shah]

On (Sun) 02 Oct 2011 [11:33:26], Michael S. Tsirkin wrote:
> On Thu, Sep 29, 2011 at 09:19:40PM +0530, Amit Shah wrote:
> > Remove all the vqs on hibernation and re-create them after restoring
> > from a hibernated image.  This keeps networking working across
> > hibernation.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
> >  1 files changed, 28 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index dcd4b01..8b9ed43 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1131,6 +1131,30 @@ 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;
> 
> I'm guessing we need to do something like netif_device_detach here,
> otherwise guest might be in the process of using the vq for transmit at
> this point.

Done.

> I think we also must make sure NAPI RX handler is not in progress.

How to do that?  napi_disable() / napi_enable() doesn't seem right
(and it doesn't work, too).  pci_disable_device() in the suspend
routine may work?

> We also might need to mask interrupts from the device
> to prevent TX or RX from getting rescheduled?

pci_disable_device() will help this too, right?

		Amit

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

[Michael S. Tsirkin]

On Tue, Nov 15, 2011 at 05:59:36PM +0530, Amit Shah wrote:
> On (Sun) 02 Oct 2011 [11:33:26], Michael S. Tsirkin wrote:
> > On Thu, Sep 29, 2011 at 09:19:40PM +0530, Amit Shah wrote:
> > > Remove all the vqs on hibernation and re-create them after restoring
> > > from a hibernated image.  This keeps networking working across
> > > hibernation.
> > > 
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
> > >  1 files changed, 28 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index dcd4b01..8b9ed43 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -1131,6 +1131,30 @@ 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;
> > 
> > I'm guessing we need to do something like netif_device_detach here,
> > otherwise guest might be in the process of using the vq for transmit at
> > this point.
> 
> Done.
> 
> > I think we also must make sure NAPI RX handler is not in progress.
> 
> How to do that?  napi_disable() / napi_enable() doesn't seem right
> (and it doesn't work, too).  pci_disable_device() in the suspend
> routine may work?
> 
> > We also might need to mask interrupts from the device
> > to prevent TX or RX from getting rescheduled?
> 
> pci_disable_device() will help this too, right?
> 
> 		Amit

No, why would it help?

-- 
MST

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

[Amit Shah]

On (Tue) 15 Nov 2011 [14:51:27], Michael S. Tsirkin wrote:
> On Tue, Nov 15, 2011 at 05:59:36PM +0530, Amit Shah wrote:
> > On (Sun) 02 Oct 2011 [11:33:26], Michael S. Tsirkin wrote:
> > > On Thu, Sep 29, 2011 at 09:19:40PM +0530, Amit Shah wrote:
> > > > Remove all the vqs on hibernation and re-create them after restoring
> > > > from a hibernated image.  This keeps networking working across
> > > > hibernation.
> > > > 
> > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > ---
> > > >  drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
> > > >  1 files changed, 28 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index dcd4b01..8b9ed43 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -1131,6 +1131,30 @@ 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;
> > > 
> > > I'm guessing we need to do something like netif_device_detach here,
> > > otherwise guest might be in the process of using the vq for transmit at
> > > this point.
> > 
> > Done.
> > 
> > > I think we also must make sure NAPI RX handler is not in progress.
> > 
> > How to do that?  napi_disable() / napi_enable() doesn't seem right
> > (and it doesn't work, too).  pci_disable_device() in the suspend
> > routine may work?
> > 
> > > We also might need to mask interrupts from the device
> > > to prevent TX or RX from getting rescheduled?
> > 
> > pci_disable_device() will help this too, right?
> > 
> > 		Amit
> 
> No, why would it help?

IRQs will be disabled after the call to pci_disable_device(), isn't
that sufficient?

		Amit

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

[Michael S. Tsirkin]

On Tue, Nov 15, 2011 at 07:33:46PM +0530, Amit Shah wrote:
> On (Tue) 15 Nov 2011 [14:51:27], Michael S. Tsirkin wrote:
> > On Tue, Nov 15, 2011 at 05:59:36PM +0530, Amit Shah wrote:
> > > On (Sun) 02 Oct 2011 [11:33:26], Michael S. Tsirkin wrote:
> > > > On Thu, Sep 29, 2011 at 09:19:40PM +0530, Amit Shah wrote:
> > > > > Remove all the vqs on hibernation and re-create them after restoring
> > > > > from a hibernated image.  This keeps networking working across
> > > > > hibernation.
> > > > > 
> > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > > ---
> > > > >  drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
> > > > >  1 files changed, 28 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index dcd4b01..8b9ed43 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -1131,6 +1131,30 @@ 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;
> > > > 
> > > > I'm guessing we need to do something like netif_device_detach here,
> > > > otherwise guest might be in the process of using the vq for transmit at
> > > > this point.
> > > 
> > > Done.
> > > 
> > > > I think we also must make sure NAPI RX handler is not in progress.
> > > 
> > > How to do that?  napi_disable() / napi_enable() doesn't seem right
> > > (and it doesn't work, too).  pci_disable_device() in the suspend
> > > routine may work?
> > > 
> > > > We also might need to mask interrupts from the device
> > > > to prevent TX or RX from getting rescheduled?
> > > 
> > > pci_disable_device() will help this too, right?
> > > 
> > > 		Amit
> > 
> > No, why would it help?
> 
> IRQs will be disabled after the call to pci_disable_device(),
> isn't that sufficient?
> 
> 		Amit

They will?
 * pci_disable_device - Disable PCI device after use
 * @dev: PCI device to be disabled
 *
 * Signal to the system that the PCI device is not in use by the system
 * anymore.  This only involves disabling PCI bus-mastering, if active.
 *
 * Note we don't actually disable the device until all callers of
 * pci_enable_device() have called pci_disable_device().


-- 
MST

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

[Amit Shah]

On (Tue) 15 Nov 2011 [16:23:00], Michael S. Tsirkin wrote:
> On Tue, Nov 15, 2011 at 07:33:46PM +0530, Amit Shah wrote:
> > On (Tue) 15 Nov 2011 [14:51:27], Michael S. Tsirkin wrote:
> > > On Tue, Nov 15, 2011 at 05:59:36PM +0530, Amit Shah wrote:
> > > > On (Sun) 02 Oct 2011 [11:33:26], Michael S. Tsirkin wrote:
> > > > > On Thu, Sep 29, 2011 at 09:19:40PM +0530, Amit Shah wrote:
> > > > > > Remove all the vqs on hibernation and re-create them after restoring
> > > > > > from a hibernated image.  This keeps networking working across
> > > > > > hibernation.
> > > > > > 
> > > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > > > ---
> > > > > >  drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
> > > > > >  1 files changed, 28 insertions(+), 0 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > index dcd4b01..8b9ed43 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -1131,6 +1131,30 @@ 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;
> > > > > 
> > > > > I'm guessing we need to do something like netif_device_detach here,
> > > > > otherwise guest might be in the process of using the vq for transmit at
> > > > > this point.
> > > > 
> > > > Done.
> > > > 
> > > > > I think we also must make sure NAPI RX handler is not in progress.
> > > > 
> > > > How to do that?  napi_disable() / napi_enable() doesn't seem right
> > > > (and it doesn't work, too).  pci_disable_device() in the suspend
> > > > routine may work?
> > > > 
> > > > > We also might need to mask interrupts from the device
> > > > > to prevent TX or RX from getting rescheduled?
> > > > 
> > > > pci_disable_device() will help this too, right?
> > > > 
> > > 
> > > No, why would it help?
> > 
> > IRQs will be disabled after the call to pci_disable_device(),
> > isn't that sufficient?
> > 
> 
> They will?
>  * pci_disable_device - Disable PCI device after use
>  * @dev: PCI device to be disabled
>  *
>  * Signal to the system that the PCI device is not in use by the system
>  * anymore.  This only involves disabling PCI bus-mastering, if active.
>  *
>  * Note we don't actually disable the device until all callers of
>  * pci_enable_device() have called pci_disable_device().

You mean multiple devices could have called pci_enable_device()?  Not
likely to happen, at least in case of our virtio devices... only we
claim ownership over them.  I don't think that'll change.

		Amit

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

[Michael S. Tsirkin]

On Tue, Nov 15, 2011 at 08:01:49PM +0530, Amit Shah wrote:
> On (Tue) 15 Nov 2011 [16:23:00], Michael S. Tsirkin wrote:
> > On Tue, Nov 15, 2011 at 07:33:46PM +0530, Amit Shah wrote:
> > > On (Tue) 15 Nov 2011 [14:51:27], Michael S. Tsirkin wrote:
> > > > On Tue, Nov 15, 2011 at 05:59:36PM +0530, Amit Shah wrote:
> > > > > On (Sun) 02 Oct 2011 [11:33:26], Michael S. Tsirkin wrote:
> > > > > > On Thu, Sep 29, 2011 at 09:19:40PM +0530, Amit Shah wrote:
> > > > > > > Remove all the vqs on hibernation and re-create them after restoring
> > > > > > > from a hibernated image.  This keeps networking working across
> > > > > > > hibernation.
> > > > > > > 
> > > > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > > > > ---
> > > > > > >  drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
> > > > > > >  1 files changed, 28 insertions(+), 0 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > index dcd4b01..8b9ed43 100644
> > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > @@ -1131,6 +1131,30 @@ 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;
> > > > > > 
> > > > > > I'm guessing we need to do something like netif_device_detach here,
> > > > > > otherwise guest might be in the process of using the vq for transmit at
> > > > > > this point.
> > > > > 
> > > > > Done.
> > > > > 
> > > > > > I think we also must make sure NAPI RX handler is not in progress.
> > > > > 
> > > > > How to do that?  napi_disable() / napi_enable() doesn't seem right
> > > > > (and it doesn't work, too).  pci_disable_device() in the suspend
> > > > > routine may work?
> > > > > 
> > > > > > We also might need to mask interrupts from the device
> > > > > > to prevent TX or RX from getting rescheduled?
> > > > > 
> > > > > pci_disable_device() will help this too, right?
> > > > > 
> > > > 
> > > > No, why would it help?
> > > 
> > > IRQs will be disabled after the call to pci_disable_device(),
> > > isn't that sufficient?
> > > 
> > 
> > They will?
> >  * pci_disable_device - Disable PCI device after use
> >  * @dev: PCI device to be disabled
> >  *
> >  * Signal to the system that the PCI device is not in use by the system
> >  * anymore.  This only involves disabling PCI bus-mastering, if active.
> >  *
> >  * Note we don't actually disable the device until all callers of
> >  * pci_enable_device() have called pci_disable_device().
> 
> You mean multiple devices could have called pci_enable_device()?  Not
> likely to happen, at least in case of our virtio devices... only we
> claim ownership over them.  I don't think that'll change.
> 
> 		Amit

I simply mean that pci_disable_device does not seem to disable
interrupts.

-- 
MST

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

[Amit Shah]

On (Tue) 15 Nov 2011 [16:36:20], Michael S. Tsirkin wrote:
> On Tue, Nov 15, 2011 at 08:01:49PM +0530, Amit Shah wrote:
> > On (Tue) 15 Nov 2011 [16:23:00], Michael S. Tsirkin wrote:
> > > On Tue, Nov 15, 2011 at 07:33:46PM +0530, Amit Shah wrote:
> > > > On (Tue) 15 Nov 2011 [14:51:27], Michael S. Tsirkin wrote:
> > > > > On Tue, Nov 15, 2011 at 05:59:36PM +0530, Amit Shah wrote:
> > > > > > On (Sun) 02 Oct 2011 [11:33:26], Michael S. Tsirkin wrote:
> > > > > > > On Thu, Sep 29, 2011 at 09:19:40PM +0530, Amit Shah wrote:
> > > > > > > > Remove all the vqs on hibernation and re-create them after restoring
> > > > > > > > from a hibernated image.  This keeps networking working across
> > > > > > > > hibernation.
> > > > > > > > 
> > > > > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > > > > > ---
> > > > > > > >  drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++++
> > > > > > > >  1 files changed, 28 insertions(+), 0 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > index dcd4b01..8b9ed43 100644
> > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > @@ -1131,6 +1131,30 @@ 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;
> > > > > > > 
> > > > > > > I'm guessing we need to do something like netif_device_detach here,
> > > > > > > otherwise guest might be in the process of using the vq for transmit at
> > > > > > > this point.
> > > > > > 
> > > > > > Done.
> > > > > > 
> > > > > > > I think we also must make sure NAPI RX handler is not in progress.
> > > > > > 
> > > > > > How to do that?  napi_disable() / napi_enable() doesn't seem right
> > > > > > (and it doesn't work, too).  pci_disable_device() in the suspend
> > > > > > routine may work?
> > > > > > 
> > > > > > > We also might need to mask interrupts from the device
> > > > > > > to prevent TX or RX from getting rescheduled?
> > > > > > 
> > > > > > pci_disable_device() will help this too, right?
> > > > > > 
> > > > > 
> > > > > No, why would it help?
> > > > 
> > > > IRQs will be disabled after the call to pci_disable_device(),
> > > > isn't that sufficient?
> > > > 
> > > 
> > > They will?
> > >  * pci_disable_device - Disable PCI device after use
> > >  * @dev: PCI device to be disabled
> > >  *
> > >  * Signal to the system that the PCI device is not in use by the system
> > >  * anymore.  This only involves disabling PCI bus-mastering, if active.
> > >  *
> > >  * Note we don't actually disable the device until all callers of
> > >  * pci_enable_device() have called pci_disable_device().
> > 
> > You mean multiple devices could have called pci_enable_device()?  Not
> > likely to happen, at least in case of our virtio devices... only we
> > claim ownership over them.  I don't think that'll change.
> > 
> 
> I simply mean that pci_disable_device does not seem to disable
> interrupts.

As far as I know, all our irqs are allocated for vqs, and they'll be
freed when the vqs are freed too.

Are there any others that need special handling?

		Amit

Re: [PATCH 00/11] virtio: Support for hibernation (S4)

[Michael S. Tsirkin]

On Thu, Sep 29, 2011 at 08:55:56PM +0530, Amit Shah wrote:
> Hello,
> 
> These patches add support for S4 to virtio (pci) and all drivers.  The
> patches are based on the virtio-console patch series in Rusty's queue.
> 
> For each driver, all vqs are removed before hibernation, and then
> re-created after restore.
> 
> 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.  One packet is lost.
>   (Current qemu.git has a regression in slirp code causing qemu
>   segfault, commit 1ab74cea060 is the offender).
> 
> * 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 in all, this looks pretty good.
> 
> Please review and apply.

So a general comment is that we need to make sure
VQs are not used by guest or host while
they are deleted.

For example remove path in exiting virtio net does:
        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);


unregister_netdev is done before del_vqs
which makes sure guest leaves the device alone, including
napi and tx. PM will need something like this too if it
removes the VQs, otherwise any use of 'vq->' might cause a crash.

As a side note: above code is not 100% robust in theory as guest could
try using VQs after reset, and we never documented that it's ok to
access such VQs, but in practice it's safe with existing hosts, accesses
are simnply discarded.  Maybe we should just document this assumption in
the spec?

Another theoretical concern with existing code is that interrupt
handler for the device might be in progress while we do
unregister_netdev, since these are not ordered wrt
io write that does the reset.
Probably reset in virtio pci should also flush all interrupt handlers?

Any thoughts?

> 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      |   36 ++++++++++--
>  drivers/char/virtio_console.c   |  124 ++++++++++++++++++++++++++++++---------
>  drivers/net/virtio_net.c        |   98 ++++++++++++++++++++++---------
>  drivers/virtio/virtio_balloon.c |   65 +++++++++++++++------
>  drivers/virtio/virtio_pci.c     |   57 +++++++++++++++++-
>  include/linux/virtio.h          |    4 +
>  6 files changed, 301 insertions(+), 83 deletions(-)
> 
> -- 
> 1.7.6.2

Re: [PATCH 00/11] virtio: Support for hibernation (S4)

[Rusty Russell]

On Sun, 2 Oct 2011 11:49:21 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Sep 29, 2011 at 08:55:56PM +0530, Amit Shah wrote:
> > Hello,
> > 
> > These patches add support for S4 to virtio (pci) and all drivers.  The
> > patches are based on the virtio-console patch series in Rusty's queue.
> > 
> > For each driver, all vqs are removed before hibernation, and then
> > re-created after restore.
> > 
> > All the drivers in testing work fine:
> > 
> > * virtio-blk is used for the only disk in the VM, IO works fine before
> >   and after.

I'm not familiar with the suspend code, but:

1) Does it already ensure there are no outstanding I/O requests?  If
   not, we want to restore them when we unfreeze.

2) Does it stop more I/O from reaching do_virtblk_request during freeze?
   If not, we need to.

If we need to save and restore requests, I don't think we should do this
on a per-driver basis, but try to do it in the core.

Thanks,
Rusty.

Re: [PATCH 00/11] virtio: Support for hibernation (S4)

[Amit Shah]

On (Tue) 04 Oct 2011 [10:36:05], Rusty Russell wrote:
> On Sun, 2 Oct 2011 11:49:21 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Sep 29, 2011 at 08:55:56PM +0530, Amit Shah wrote:
> > > Hello,
> > > 
> > > These patches add support for S4 to virtio (pci) and all drivers.  The
> > > patches are based on the virtio-console patch series in Rusty's queue.
> > > 
> > > For each driver, all vqs are removed before hibernation, and then
> > > re-created after restore.
> > > 
> > > All the drivers in testing work fine:
> > > 
> > > * virtio-blk is used for the only disk in the VM, IO works fine before
> > >   and after.
> 
> I'm not familiar with the suspend code, but:
> 
> 1) Does it already ensure there are no outstanding I/O requests?  If
>    not, we want to restore them when we unfreeze.

For the blk code, I added blk_drain_queue() and related calls to the
freeze handler, that was missed earlier.

> 2) Does it stop more I/O from reaching do_virtblk_request during freeze?
>    If not, we need to.

Added blk_stop_queue() and blk_start_queue() calls to ensure this.

> If we need to save and restore requests, I don't think we should do this
> on a per-driver basis, but try to do it in the core.

Agreed, however currently we don't save and restore any pending vq
items.  If it is necessary, we should do it in common code (earlier
version of this patchset did that).  I think we can get better results
by flushing the vq instead of trying to save/restore elements.

		Amit