Linux Virtualization - Jul 2011 - [RFC PATCH 0/8] virtio: Support for hibernation (S4)

Hello,

These patches are an initial attempt at supporting hibernation for
virtio drivers.

The default configuration of event_index=on doesn't work; i.e. restore
from a hibernated image only works if the devices have event_index
support turned off.  I have not yet dug into this, but is most likely
due to some state not being sync'ed.  This could be related to the
hack that is patch 3.

Each virtio driver has to be modified to add support for the
freeze/restore callbacks to delete and then re-add the vqs just before
and after hibernation.

Testing:

I've not tested S3 (suspend).

For virtio-net, ping continues working across hibernate (lost one
packet in sole testing).

For virtio-blk, disk activity continues working.

For virtio-console, port IO continues working.


Amit Shah (8):
  virtio: pci: switch to new PM API
  virtio-pci: add PM notification handlers for restore, freeze, thaw,
    poweroff
  virtio-pci: save/restore config space across S4
  virtio: console: Ignore port name update request if name already set
  virtio: console: Use wait_event_freezable instead of _interruptible
  virtio: console: Add freeze and restore handlers to support S4
  virtio: block: Add freeze, restore handlers to support S4
  virtio: net: Add freeze, restore handlers to support S4

 drivers/block/virtio_blk.c    |   24 +++++++++++
 drivers/char/virtio_console.c |   87 +++++++++++++++++++++++++++++++++++++++--
 drivers/net/virtio_net.c      |   51 ++++++++++++++++++++++++
 drivers/virtio/virtio_pci.c   |   57 +++++++++++++++++++++++++--
 include/linux/virtio.h        |    4 ++
 5 files changed, 215 insertions(+), 8 deletions(-)

-- 
1.7.6

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 at redhat.com>
---
 drivers/virtio/virtio_pci.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 4bcc8b8..3879c3e 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/list.h>
 #include <linux/pci.h>
+#include <linux/pm.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/virtio.h>
@@ -685,19 +686,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 +716,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

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

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

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3879c3e..579681f 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -704,9 +704,43 @@ 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);
+
+	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);
+
+	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

The virtio config space doesn't get saved across hibernation; we save it
locally and update it after restore.

Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/virtio/virtio_pci.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 579681f..9c37561 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -56,6 +56,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;
 };
@@ -713,6 +717,7 @@ static int virtio_pci_freeze(struct device *dev)
 	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);
 
@@ -728,6 +733,7 @@ static int virtio_pci_restore(struct device *dev)
 	drv = container_of(vp_dev->vdev.dev.driver,
 			   struct virtio_driver, driver);
 
+	vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
 	if (drv && drv->restore)
 		return drv->restore(&vp_dev->vdev);
 
-- 
1.7.6

We don't allow port name changes dynamically for a port.  So any
requests by the host to change the name are ignored.

Before this patch, if the hypervisor sent a port name while we had one
set already, we would leak memory equivalent to the size of the old
name.

This scenario wasn't expected so far, but with the suspend-resume
support, we'll send the VIRTIO_CONSOLE_PORT_READY message after restore,
which can get us into this situation.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fb68b12..3c10b10 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1394,6 +1394,13 @@ static void handle_control_message(struct ports_device
*portdev,
 		break;
 	case VIRTIO_CONSOLE_PORT_NAME:
 		/*
+		 * If we woke up after hibernation, we can get this
+		 * again.  Skip it in that case.
+		 */
+		if (port->name)
+			break;
+
+		/*
 		 * Skip the size of the header and the cpkt to get the size
 		 * of the name that was sent
 		 */
-- 
1.7.6

Get ready to support suspend/resume by using the freezable calls so that
blocking read/write syscalls are handled properly across suspend/resume.

Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3c10b10..a7dfd02 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -21,6 +21,7 @@
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/freezer.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/list.h>
@@ -633,8 +634,8 @@ static ssize_t port_fops_read(struct file *filp, char __user
*ubuf,
 		if (filp->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
-		ret = wait_event_interruptible(port->waitqueue,
-					       !will_read_block(port));
+		ret = wait_event_freezable(port->waitqueue,
+					   !will_read_block(port));
 		if (ret < 0)
 			return ret;
 	}
@@ -677,8 +678,8 @@ static ssize_t port_fops_write(struct file *filp, const char
__user *ubuf,
 		if (nonblock)
 			return -EAGAIN;
 
-		ret = wait_event_interruptible(port->waitqueue,
-					       !will_write_block(port));
+		ret = wait_event_freezable(port->waitqueue,
+					   !will_write_block(port));
 		if (ret < 0)
 			return ret;
 	}
-- 
1.7.6

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 at redhat.com>
---
 drivers/char/virtio_console.c |   71 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index a7dfd02..9648cea 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -25,6 +25,7 @@
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/pm.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -1801,6 +1802,72 @@ 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;
+	struct port_buffer *buf;
+	unsigned int len;
+
+	portdev = vdev->priv;
+
+	if (use_multiport(portdev)) {
+		while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
+			free_buf(buf);
+
+		while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
+			free_buf(buf);
+	}
+	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;
+		discard_port_data(port);
+		reclaim_consumed_buffers(port);
+
+		while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
+			free_buf(buf);
+	}
+	portdev->vdev->config->del_vqs(portdev->vdev);
+	kfree(portdev->in_vqs);
+	kfree(portdev->out_vqs);
+
+	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);
+
+		/* XXX: Should this be done in 'complete' instead of
'restore'? */
+		/* 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),
@@ -1810,6 +1877,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

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 at redhat.com>
---
 drivers/block/virtio_blk.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..700ad76 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -540,6 +540,26 @@ 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)
+{
+	struct virtio_blk *vblk = vdev->priv;
+	int err = 0;
+
+	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
+	if (IS_ERR(vblk->vq))
+		err = PTR_ERR(vblk->vq);
+
+	return err;
+}
+#endif
+
 static const struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -565,6 +585,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

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 at redhat.com>
---
 drivers/net/virtio_net.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..c9aaa8f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1117,6 +1117,53 @@ 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;
+
+	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);
+
+	while (vi->pages)
+		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
+
+	return 0;
+}
+
+static int virtnet_restore(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+	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 = vdev->config->find_vqs(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];
+	}
+
+	/* Last of all, set up some receive buffers. */
+	try_fill_recv(vi, GFP_KERNEL);
+	return 0;
+}
+#endif
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -1141,6 +1188,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

On Thu, Jul 28, 2011 at 08:05:13PM +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 at redhat.com>
> ---
>  drivers/net/virtio_net.c |   51
++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0c7321c..c9aaa8f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1117,6 +1117,53 @@ 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;
> +
> +	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);
> +
> +	while (vi->pages)
> +		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
> +
> +	return 0;
> +}
> +
> +static int virtnet_restore(struct virtio_device *vdev)
> +{
> +	struct virtnet_info *vi = vdev->priv;
> +	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 = vdev->config->find_vqs(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];
> +	}
> +
> +	/* Last of all, set up some receive buffers. */
> +	try_fill_recv(vi, GFP_KERNEL);
should probably update carrier status too.
> +	return 0;
> +}
> +#endif
> +
>  static struct virtio_device_id id_table[] = {
>  	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
>  	{ 0 },
> @@ -1141,6 +1188,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

On (Thu) 28 Jul 2011 [20:05:05], Amit Shah wrote:
> Hello,
> 
> These patches are an initial attempt at supporting hibernation for
> virtio drivers.
> 
> The default configuration of event_index=on doesn't work; i.e. restore
> from a hibernated image only works if the devices have event_index
> support turned off.  I have not yet dug into this, but is most likely
> due to some state not being sync'ed.
This was due to the host forgetting which features the guest had
acked.  Adding this on top of this series makes things work fine.
I'll do more testing, refining code (sharing code with probe, etc.)
and repost as a proper series (mostly after the KVM Forum).

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 9c37561..c09d6a5 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -734,6 +734,7 @@ static int virtio_pci_restore(struct device *dev)
 			   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);
 

		Amit