[RFC PATCH 0/8] virtio: Support for hibernation (S4)

[RFC PATCH 0/8] virtio: Support for hibernation (S4)

[Amit Shah]

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

[RFC PATCH 1/8] 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 |   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

[RFC PATCH 2/8] 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.

Signed-off-by: Amit Shah <amit.shah@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

[RFC PATCH 3/8] virtio-pci: save/restore config space across S4

[Amit Shah]

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@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

[RFC PATCH 4/8] virtio: console: Ignore port name update request if name already set

[Amit Shah]

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@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

[RFC PATCH 5/8] virtio: console: Use wait_event_freezable instead of _interruptible

[Amit Shah]

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@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

[RFC PATCH 6/8] 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 |   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

[RFC PATCH 7/8] virtio: block: 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 |   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

[RFC PATCH 8/8] 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 |   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

Re: [RFC PATCH 3/8] virtio-pci: save/restore config space across S4

[Michael S. Tsirkin]

On Thu, Jul 28, 2011 at 08:05:08PM +0530, Amit Shah wrote:
> 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@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);

Yes, this looks wrong. Status would typically be DRIVER_OK,
and it needs to be set after vq setup.
It also needs to match current state I think:
e.g. if restore failed we want to set it to FAILED.
So why not do it more or less the way probe does it?


> -- 
> 1.7.6

Re: [RFC PATCH 8/8] virtio: net: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

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@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

Re: [RFC PATCH 4/8] virtio: console: Ignore port name update request if name already set

[Michael S. Tsirkin]

On Thu, Jul 28, 2011 at 08:05:09PM +0530, Amit Shah wrote:
> We don't allow port name changes dynamically for a port.  So any
> requests by the host to change the name are ignored.

It would be nice to check the name and if
changed, print a warning.

> 
> 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@redhat.com>

Memleaks are bad. So this is a bugfix we probably should have anyway?

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

Re: [RFC PATCH 3/8] virtio-pci: save/restore config space across S4

[Amit Shah]

On (Fri) 29 Jul 2011 [07:07:58], Michael S. Tsirkin wrote:
> On Thu, Jul 28, 2011 at 08:05:08PM +0530, Amit Shah wrote:
> > 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@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);
> 
> Yes, this looks wrong. Status would typically be DRIVER_OK,
> and it needs to be set after vq setup.

Because I expect this to be carried forward from the hibernated
image.. the feature negotiation, etc., all happened the first time
around, and the driver should just be ready when the image is
restored.  I still don't know why the status is reset, and why we'd
need to replay the sequence in probe.

Of course, this patch exists in the current form only because I don't
know the answers yet (as mentioned in 0/8).  Once I know the answer,
either we'll have a better patch or a better description for this
patch.

> It also needs to match current state I think:
> e.g. if restore failed we want to set it to FAILED.

If restore fails, we return to a normal bootup sequence.  The probe
routine should handle that case normally.  However, this is something
that has to be tested to ensure it works fine.

		Amit

Re: [RFC PATCH 4/8] virtio: console: Ignore port name update request if name already set

[Amit Shah]

On (Fri) 29 Jul 2011 [07:19:58], Michael S. Tsirkin wrote:
> On Thu, Jul 28, 2011 at 08:05:09PM +0530, Amit Shah wrote:
> > We don't allow port name changes dynamically for a port.  So any
> > requests by the host to change the name are ignored.
> 
> It would be nice to check the name and if
> changed, print a warning.

But not change it, right?  I don't know what applications expect, but
I think we should keep the port names stable while they're plugged in.

> > 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@redhat.com>
> 
> Memleaks are bad. So this is a bugfix we probably should have anyway?

Yes.

		Amit

Re: [RFC PATCH 4/8] virtio: console: Ignore port name update request if name already set

[Michael S. Tsirkin]

On Fri, Jul 29, 2011 at 09:56:19AM +0530, Amit Shah wrote:
> On (Fri) 29 Jul 2011 [07:19:58], Michael S. Tsirkin wrote:
> > On Thu, Jul 28, 2011 at 08:05:09PM +0530, Amit Shah wrote:
> > > We don't allow port name changes dynamically for a port.  So any
> > > requests by the host to change the name are ignored.
> > 
> > It would be nice to check the name and if
> > changed, print a warning.
> 
> But not change it, right?

That's what I meant.

>  I don't know what applications expect, but
> I think we should keep the port names stable while they're plugged in.
> 
> > > 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@redhat.com>
> > 
> > Memleaks are bad. So this is a bugfix we probably should have anyway?
> 
> Yes.
> 
> 		Amit

Re: [RFC PATCH 8/8] virtio: net: Add freeze, restore handlers to support S4

[Amit Shah]

On (Fri) 29 Jul 2011 [07:13:35], Michael S. Tsirkin wrote:
> > +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.

Will add that.

I think most of these restore routines are incomplete; I just pulled
in the most important things from the probe routines, but before the
final submission I'll do a thorough walk-through and ensure I get all
the bits necessary.

		Amit

Re: [RFC PATCH 8/8] virtio: net: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Fri, Jul 29, 2011 at 09:58:02AM +0530, Amit Shah wrote:
> On (Fri) 29 Jul 2011 [07:13:35], Michael S. Tsirkin wrote:
> > > +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);

I guess we should schedule work on failure, as well.

> > 
> > should probably update carrier status too.
> 
> Will add that.
> 
> I think most of these restore routines are incomplete; I just pulled
> in the most important things from the probe routines, but before the
> final submission I'll do a thorough walk-through and ensure I get all
> the bits necessary.
> 
> 		Amit

In some cases, it might be a good idea to use common
code for that.

-- 
MST

Re: [RFC PATCH 8/8] virtio: net: Add freeze, restore handlers to support S4

[Amit Shah]

On (Fri) 29 Jul 2011 [07:31:16], Michael S. Tsirkin wrote:
> On Fri, Jul 29, 2011 at 09:58:02AM +0530, Amit Shah wrote:
> > On (Fri) 29 Jul 2011 [07:13:35], Michael S. Tsirkin wrote:
> > > > +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);
> 
> I guess we should schedule work on failure, as well.
> 
> > > 
> > > should probably update carrier status too.
> > 
> > Will add that.
> > 
> > I think most of these restore routines are incomplete; I just pulled
> > in the most important things from the probe routines, but before the
> > final submission I'll do a thorough walk-through and ensure I get all
> > the bits necessary.
> 
> In some cases, it might be a good idea to use common
> code for that.

Absolutely.

		Amit

Re: [RFC PATCH 0/8] virtio: Support for hibernation (S4)

[Amit Shah]

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