[Qemu-devel] [PATCH 00/11] virtio migration: simplify vmstate helper

[Qemu-devel] [PATCH 00/11] virtio migration: simplify vmstate helper

[Halil Pasic]

As a part of the long term  effort to convert migration to vmstate the
migration of virtio devices was recently partially switched to vmstate
starting with the outer layer (commit 5943124cc "virtio: Migration
helper function and macro" and the subsequent "virtio-*: Wrap in
vmstate" commits). This was done by introducing a vmstate based wrapper
driving the well know virtio migration process. As this transition is
still in progress, some synergies were left unexploited, and some things
can be expressed in a more vmstatish way.

Let us simplify a couple of things and get rid of some code duplication.

Did only a couple of smoke tests. Comprehensive testing is still to be done.

Halil Pasic (11):
  virtio: add VIRTIO_DEF_DEVICE_VMSD macro
  virtio-blk: convert to VIRTIO_DEF_DEVICE_VMSD
  virtio-net: convert to VIRTIO_DEF_DEVICE_VMSD
  virtio-9p: convert to VIRTIO_DEF_DEVICE_VMSD
  virtio-serial: convert to VIRTIO_DEF_DEVICE_VMSD
  virtio-gpu: do not use VMSTATE_VIRTIO_DEVICE
  virtio-input: convert to VIRTIO_DEF_DEVICE_VMSD
  virtio-scsi: convert to VIRTIO_DEF_DEVICE_VMSD
  virtio-balloon: convert to VIRTIO_DEF_DEVICE_VMSD
  virtio-rng: convert to VIRTIO_DEF_DEVICE_VMSD
  virtio: remove unused VMSTATE_VIRTIO_DEVICE

 hw/9pfs/virtio-9p-device.c  |  7 +------
 hw/block/virtio-blk.c       | 17 +----------------
 hw/char/virtio-serial-bus.c |  8 +-------
 hw/display/virtio-gpu.c     | 39 +++++++++++++++++++++++++++------------
 hw/input/virtio-input.c     | 13 +++----------
 hw/net/virtio-net.c         | 32 +++++++++++---------------------
 hw/scsi/virtio-scsi.c       | 18 +-----------------
 hw/virtio/virtio-balloon.c  |  7 +------
 hw/virtio/virtio-rng.c      | 10 ++--------
 hw/virtio/virtio.c          | 11 ++++++++++-
 include/hw/virtio/virtio.h  | 31 ++++++++++++++++++-------------
 11 files changed, 76 insertions(+), 117 deletions(-)

-- 
2.8.4

[Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Halil Pasic]

In most cases the functions passed to VMSTATE_VIRTIO_DEVICE
only call the virtio_load and virtio_save wrappers. Some include some
pre- and post- massaging too. The massaging is better expressed
as such in the VMStateDescription.

Let us introduce a new macro called VIRTIO_DEF_DEVICE_VMSD and replace
VMSTATE_VIRTIO_DEVICE with it gradually.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c         | 15 +++++++++++++++
 include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 18ce333..ca0a780 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1622,6 +1622,21 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
     virtio_save(VIRTIO_DEVICE(opaque), f);
 }
 
+/* A wrapper for use as a VMState .put function */
+void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size)
+{
+    virtio_save(VIRTIO_DEVICE(opaque), f);
+}
+
+/* A wrapper for use as a VMState .get function */
+int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
+    DeviceClass *dc = DEVICE_CLASS(VIRTIO_DEVICE_GET_CLASS(vdev));
+
+    return virtio_load(vdev, f, dc->vmsd->version_id);
+}
+
 static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
 {
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 888c8de..01de49b 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -176,6 +176,31 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
 
 void virtio_save(VirtIODevice *vdev, QEMUFile *f);
 void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size);
+void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size);
+int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size);
+
+#define VMSTATE_VIRTIO_FIELD \
+    {                                         \
+        .name = "virtio",                     \
+        .info = &(const VMStateInfo) {        \
+            .name = "virtio",                 \
+            .get = virtio_load_as_vmsi_get,   \
+            .put = virtio_save_as_vmsi_put,   \
+        },                                    \
+        .flags = VMS_SINGLE,                  \
+    }
+
+#define VIRTIO_DEF_DEVICE_VMSD(devname, v, ...) \
+    static const VMStateDescription vmstate_virtio_ ## devname = { \
+        .name = "virtio-" #devname ,          \
+        .minimum_version_id = v,              \
+        .version_id = v,                      \
+        .fields = (VMStateField[]) {          \
+            VMSTATE_VIRTIO_FIELD,             \
+            VMSTATE_END_OF_LIST()             \
+        },                                    \
+        __VA_ARGS__                           \
+    };
 
 #define VMSTATE_VIRTIO_DEVICE(devname, v, getf, putf) \
     static const VMStateDescription vmstate_virtio_ ## devname = { \
-- 
2.8.4

[Qemu-devel] [PATCH 02/12] virtio-blk: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/block/virtio-blk.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 3a6112f..c6f2838 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -803,13 +803,6 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
     }
 }
 
-static void virtio_blk_save(QEMUFile *f, void *opaque, size_t size)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
-
-    virtio_save(vdev, f);
-}
-    
 static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
@@ -828,14 +821,6 @@ static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_sbyte(f, 0);
 }
 
-static int virtio_blk_load(QEMUFile *f, void *opaque, size_t size)
-{
-    VirtIOBlock *s = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-
-    return virtio_load(vdev, f, 2);
-}
-
 static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
                                   int version_id)
 {
@@ -956,7 +941,7 @@ static void virtio_blk_instance_init(Object *obj)
                                   DEVICE(obj), NULL);
 }
 
-VMSTATE_VIRTIO_DEVICE(blk, 2, virtio_blk_load, virtio_blk_save);
+VIRTIO_DEF_DEVICE_VMSD(blk, 2)
 
 static Property virtio_blk_properties[] = {
     DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf),
-- 
2.8.4

[Qemu-devel] [PATCH 03/12] virtio-net: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6b8ae2c..101c006 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1498,17 +1498,6 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
     virtio_net_set_queues(n);
 }
 
-static void virtio_net_save(QEMUFile *f, void *opaque, size_t size)
-{
-    VirtIONet *n = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(n);
-
-    /* At this point, backend must be stopped, otherwise
-     * it might keep writing to memory. */
-    assert(!n->vhost_started);
-    virtio_save(vdev, f);
-}
-
 static void virtio_net_save_device(VirtIODevice *vdev, QEMUFile *f)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -1544,14 +1533,6 @@ static void virtio_net_save_device(VirtIODevice *vdev, QEMUFile *f)
     }
 }
 
-static int virtio_net_load(QEMUFile *f, void *opaque, size_t size)
-{
-    VirtIONet *n = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(n);
-
-    return virtio_load(vdev, f, VIRTIO_NET_VM_VERSION);
-}
-
 static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
                                   int version_id)
 {
@@ -1854,8 +1835,17 @@ static void virtio_net_instance_init(Object *obj)
                                   DEVICE(n), NULL);
 }
 
-VMSTATE_VIRTIO_DEVICE(net, VIRTIO_NET_VM_VERSION, virtio_net_load,
-                      virtio_net_save);
+static void virtio_net_pre_save(void *opaque)
+{
+    VirtIONet *n = opaque;
+
+    /* At this point, backend must be stopped, otherwise
+     * it might keep writing to memory. */
+    assert(!n->vhost_started);
+}
+
+VIRTIO_DEF_DEVICE_VMSD(net, VIRTIO_NET_VM_VERSION,
+    .pre_save = virtio_net_pre_save)
 
 static Property virtio_net_properties[] = {
     DEFINE_PROP_BIT("csum", VirtIONet, host_features, VIRTIO_NET_F_CSUM, true),
-- 
2.8.4

[Qemu-devel] [PATCH 04/12] virtio-9p: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 009b43f..f854ac7 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -97,11 +97,6 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t *config)
     g_free(cfg);
 }
 
-static int virtio_9p_load(QEMUFile *f, void *opaque, size_t size)
-{
-    return virtio_load(VIRTIO_DEVICE(opaque), f, 1);
-}
-
 static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -168,7 +163,7 @@ void virtio_init_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
 
 /* virtio-9p device */
 
-VMSTATE_VIRTIO_DEVICE(9p, 1, virtio_9p_load, virtio_vmstate_save);
+VIRTIO_DEF_DEVICE_VMSD(9p, 1)
 
 static Property virtio_9p_properties[] = {
     DEFINE_PROP_STRING("mount_tag", V9fsVirtioState, state.fsconf.tag),
-- 
2.8.4

[Qemu-devel] [PATCH 05/12] virtio-serial: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/char/virtio-serial-bus.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index db57a38..60e27cc 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -728,12 +728,6 @@ static int fetch_active_ports_list(QEMUFile *f,
     return 0;
 }
 
-static int virtio_serial_load(QEMUFile *f, void *opaque, size_t size)
-{
-    /* The virtio device */
-    return virtio_load(VIRTIO_DEVICE(opaque), f, 3);
-}
-
 static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f,
                                      int version_id)
 {
@@ -1075,7 +1069,7 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
 }
 
 /* Note: 'console' is used for backwards compatibility */
-VMSTATE_VIRTIO_DEVICE(console, 3, virtio_serial_load, virtio_vmstate_save);
+VIRTIO_DEF_DEVICE_VMSD(console, 3)
 
 static Property virtio_serial_properties[] = {
     DEFINE_PROP_UINT32("max_ports", VirtIOSerial, serial.max_virtserial_ports,
-- 
2.8.4

[Qemu-devel] [PATCH 06/12] virtio-gpu: do not use VMSTATE_VIRTIO_DEVICE

[Halil Pasic]

Refactor so that VMSTATE_VIRTIO_DEVICE macro which we want to remove
in the future is not used any more. The device virtio-gpu is special
because it actually does not adhere to the virtio migration schema,
because device state is last.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/display/virtio-gpu.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 7fe6ed8..8fff6cb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -990,12 +990,9 @@ static const VMStateDescription vmstate_virtio_gpu_scanouts = {
 static void virtio_gpu_save(QEMUFile *f, void *opaque, size_t size)
 {
     VirtIOGPU *g = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(g);
     struct virtio_gpu_simple_resource *res;
     int i;
 
-    virtio_save(vdev, f);
-
     /* in 2d mode we should never find unprocessed commands here */
     assert(QTAILQ_EMPTY(&g->cmdq));
 
@@ -1020,16 +1017,10 @@ static void virtio_gpu_save(QEMUFile *f, void *opaque, size_t size)
 static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size)
 {
     VirtIOGPU *g = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(g);
     struct virtio_gpu_simple_resource *res;
     struct virtio_gpu_scanout *scanout;
     uint32_t resource_id, pformat;
-    int i, ret;
-
-    ret = virtio_load(vdev, f, VIRTIO_GPU_VM_VERSION);
-    if (ret) {
-        return ret;
-    }
+    int i;
 
     resource_id = qemu_get_be32(f);
     while (resource_id != 0) {
@@ -1219,8 +1210,32 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
 #endif
 }
 
-VMSTATE_VIRTIO_DEVICE(gpu, VIRTIO_GPU_VM_VERSION, virtio_gpu_load,
-                      virtio_gpu_save);
+/*
+ * For historical reasons virtio_gpu does not adhere to virtio migration
+ * scheme as described in doc/virtio-migration.txt, in a sense that no
+ * save/load callback are provided to the core. Instead the device data
+ * is saved/loaded after the core data.
+ *
+ * Because of this we need a special vmsd.
+ */
+static const VMStateDescription vmstate_virtio_gpu = {
+    .name = "virtio-gpu",
+    .minimum_version_id = VIRTIO_GPU_VM_VERSION,
+    .version_id = VIRTIO_GPU_VM_VERSION,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_FIELD /* core */,
+        {
+            .name = "virtio-gpu",
+            .info = &(const VMStateInfo) {
+                        .name = "virtio-gpu",
+                        .get = virtio_gpu_load,
+                        .put = virtio_gpu_save,
+            },
+            .flags = VMS_SINGLE,
+        } /* device */,
+        VMSTATE_END_OF_LIST()
+    },
+};
 
 static Property virtio_gpu_properties[] = {
     DEFINE_PROP_UINT32("max_outputs", VirtIOGPU, conf.max_outputs, 1),
-- 
2.8.4

[Qemu-devel] [PATCH 07/12] virtio-input: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/input/virtio-input.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
index ccdf730..d8509fa 100644
--- a/hw/input/virtio-input.c
+++ b/hw/input/virtio-input.c
@@ -217,19 +217,12 @@ static void virtio_input_reset(VirtIODevice *vdev)
     }
 }
 
-static int virtio_input_load(QEMUFile *f, void *opaque, size_t size)
+static int virtio_input_post_load(void *opaque, int version_id)
 {
     VirtIOInput *vinput = opaque;
     VirtIOInputClass *vic = VIRTIO_INPUT_GET_CLASS(vinput);
     VirtIODevice *vdev = VIRTIO_DEVICE(vinput);
-    int ret;
 
-    ret = virtio_load(vdev, f, VIRTIO_INPUT_VM_VERSION);
-    if (ret) {
-        return ret;
-    }
-
-    /* post_load() */
     vinput->active = vdev->status & VIRTIO_CONFIG_S_DRIVER_OK;
     if (vic->change_active) {
         vic->change_active(vinput);
@@ -296,8 +289,8 @@ static void virtio_input_device_unrealize(DeviceState *dev, Error **errp)
     virtio_cleanup(vdev);
 }
 
-VMSTATE_VIRTIO_DEVICE(input, VIRTIO_INPUT_VM_VERSION, virtio_input_load,
-                      virtio_vmstate_save);
+VIRTIO_DEF_DEVICE_VMSD(input, VIRTIO_INPUT_VM_VERSION,
+    .post_load = virtio_input_post_load)
 
 static Property virtio_input_properties[] = {
     DEFINE_PROP_STRING("serial", VirtIOInput, serial),
-- 
2.8.4

[Qemu-devel] [PATCH 08/12] virtio-scsi: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/scsi/virtio-scsi.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index e596b64..acbf8cd 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -663,22 +663,6 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
     s->events_dropped = false;
 }
 
-/* The device does not have anything to save beyond the virtio data.
- * Request data is saved with callbacks from SCSI devices.
- */
-static void virtio_scsi_save(QEMUFile *f, void *opaque, size_t size)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
-    virtio_save(vdev, f);
-}
-
-static int virtio_scsi_load(QEMUFile *f, void *opaque, size_t size)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
-
-    return virtio_load(vdev, f, 1);
-}
-
 void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
                             uint32_t event, uint32_t reason)
 {
@@ -921,7 +905,7 @@ static Property virtio_scsi_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-VMSTATE_VIRTIO_DEVICE(scsi, 1, virtio_scsi_load, virtio_scsi_save);
+VIRTIO_DEF_DEVICE_VMSD(scsi, 1)
 
 static void virtio_scsi_common_class_init(ObjectClass *klass, void *data)
 {
-- 
2.8.4

[Qemu-devel] [PATCH 09/12] virtio-balloon: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/virtio/virtio-balloon.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 49a2f4a..09688b6 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -404,11 +404,6 @@ static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_be32(f, s->actual);
 }
 
-static int virtio_balloon_load(QEMUFile *f, void *opaque, size_t size)
-{
-    return virtio_load(VIRTIO_DEVICE(opaque), f, 1);
-}
-
 static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
                                       int version_id)
 {
@@ -494,7 +489,7 @@ static void virtio_balloon_instance_init(Object *obj)
                         NULL, s, NULL);
 }
 
-VMSTATE_VIRTIO_DEVICE(balloon, 1, virtio_balloon_load, virtio_vmstate_save);
+VIRTIO_DEF_DEVICE_VMSD(balloon, 1)
 
 static Property virtio_balloon_properties[] = {
     DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features,
-- 
2.8.4

[Qemu-devel] [PATCH 10/12] virtio-rng: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/virtio/virtio-rng.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index cd8ca10..018d44d 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -120,15 +120,9 @@ static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
     return f;
 }
 
-static int virtio_rng_load(QEMUFile *f, void *opaque, size_t size)
+static int virtio_rng_post_load(void *opaque, int version_id)
 {
     VirtIORNG *vrng = opaque;
-    int ret;
-
-    ret = virtio_load(VIRTIO_DEVICE(vrng), f, 1);
-    if (ret != 0) {
-        return ret;
-    }
 
     /* We may have an element ready but couldn't process it due to a quota
      * limit.  Make sure to try again after live migration when the quota may
@@ -216,7 +210,7 @@ static void virtio_rng_device_unrealize(DeviceState *dev, Error **errp)
     virtio_cleanup(vdev);
 }
 
-VMSTATE_VIRTIO_DEVICE(rng, 1, virtio_rng_load, virtio_vmstate_save);
+VIRTIO_DEF_DEVICE_VMSD(rng, 1, .post_load =  virtio_rng_post_load)
 
 static Property virtio_rng_properties[] = {
     /* Set a default rate limit of 2^47 bytes per minute or roughly 2TB/s.  If
-- 
2.8.4

[Qemu-devel] [PATCH 11/12] vhost-vsock: convert to VIRTIO_DEF_DEVICE_VMSD

[Halil Pasic]

Refactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD
replaces VMSTATE_VIRTIO_DEVICE.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/virtio/vhost-vsock.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index bde2456..3c2763e 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -236,17 +236,6 @@ out:
     g_free(elem);
 }
 
-static void vhost_vsock_save(QEMUFile *f, void *opaque, size_t size)
-{
-    VHostVSock *vsock = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(vsock);
-
-    /* At this point, backend must be stopped, otherwise
-     * it might keep writing to memory. */
-    assert(!vsock->vhost_dev.started);
-    virtio_save(vdev, f);
-}
-
 static void vhost_vsock_post_load_timer_cleanup(VHostVSock *vsock)
 {
     if (!vsock->post_load_timer) {
@@ -266,16 +255,19 @@ static void vhost_vsock_post_load_timer_cb(void *opaque)
     vhost_vsock_send_transport_reset(vsock);
 }
 
-static int vhost_vsock_load(QEMUFile *f, void *opaque, size_t size)
+static void vhost_vsock_pre_save(void *opaque)
+{
+    VHostVSock *vsock = opaque;
+
+    /* At this point, backend must be stopped, otherwise
+     * it might keep writing to memory. */
+    assert(!vsock->vhost_dev.started);
+}
+
+static int vhost_vsock_post_load(void *opaque, int version_id)
 {
     VHostVSock *vsock = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(vsock);
-    int ret;
-
-    ret = virtio_load(vdev, f, VHOST_VSOCK_SAVEVM_VERSION);
-    if (ret) {
-        return ret;
-    }
 
     if (virtio_queue_get_addr(vdev, 2)) {
         /* Defer transport reset event to a vm clock timer so that virtqueue
@@ -288,12 +280,11 @@ static int vhost_vsock_load(QEMUFile *f, void *opaque, size_t size)
                          vsock);
         timer_mod(vsock->post_load_timer, 1);
     }
-
     return 0;
 }
 
-VMSTATE_VIRTIO_DEVICE(vhost_vsock, VHOST_VSOCK_SAVEVM_VERSION,
-                      vhost_vsock_load, vhost_vsock_save);
+VIRTIO_DEF_DEVICE_VMSD(vhost_vsock, VHOST_VSOCK_SAVEVM_VERSION,
+    .pre_save = vhost_vsock_pre_save, .post_load = vhost_vsock_post_load)
 
 static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
 {
-- 
2.8.4

[Qemu-devel] [PATCH 12/12] virtio: remove unused VMSTATE_VIRTIO_DEVICE

[Halil Pasic]

Previously we mande sure VMSTATE_VIRTIO_DEVICE is not used any more.
Let us remove it along with the associated wrapper function.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c         |  6 ------
 include/hw/virtio/virtio.h | 20 --------------------
 2 files changed, 26 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ca0a780..6d23ba7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1617,12 +1617,6 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
 }
 
 /* A wrapper for use as a VMState .put function */
-void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
-{
-    virtio_save(VIRTIO_DEVICE(opaque), f);
-}
-
-/* A wrapper for use as a VMState .put function */
 void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size)
 {
     virtio_save(VIRTIO_DEVICE(opaque), f);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 01de49b..85b0e2f 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -175,7 +175,6 @@ bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq);
 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
 
 void virtio_save(VirtIODevice *vdev, QEMUFile *f);
-void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size);
 void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size);
 int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size);
 
@@ -202,25 +201,6 @@ int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size);
         __VA_ARGS__                           \
     };
 
-#define VMSTATE_VIRTIO_DEVICE(devname, v, getf, putf) \
-    static const VMStateDescription vmstate_virtio_ ## devname = { \
-        .name = "virtio-" #devname ,          \
-        .minimum_version_id = v,              \
-        .version_id = v,                      \
-        .fields = (VMStateField[]) {          \
-            {                                 \
-                .name = "virtio",             \
-                .info = &(const VMStateInfo) {\
-                        .name = "virtio",     \
-                        .get = getf,          \
-                        .put = putf,          \
-                    },                        \
-                .flags = VMS_SINGLE,          \
-            },                                \
-            VMSTATE_END_OF_LIST()             \
-        }                                     \
-    }
-
 int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id);
 
 void virtio_notify_config(VirtIODevice *vdev);
-- 
2.8.4

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Paolo Bonzini]

On 30/09/2016 16:19, Halil Pasic wrote:
> In most cases the functions passed to VMSTATE_VIRTIO_DEVICE
> only call the virtio_load and virtio_save wrappers. Some include some
> pre- and post- massaging too. The massaging is better expressed
> as such in the VMStateDescription.
> 
> Let us introduce a new macro called VIRTIO_DEF_DEVICE_VMSD and replace
> VMSTATE_VIRTIO_DEVICE with it gradually.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/virtio/virtio.c         | 15 +++++++++++++++
>  include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 18ce333..ca0a780 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1622,6 +1622,21 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>      virtio_save(VIRTIO_DEVICE(opaque), f);
>  }
>  
> +/* A wrapper for use as a VMState .put function */
> +void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size)
> +{
> +    virtio_save(VIRTIO_DEVICE(opaque), f);
> +}
> +
> +/* A wrapper for use as a VMState .get function */
> +int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> +    DeviceClass *dc = DEVICE_CLASS(VIRTIO_DEVICE_GET_CLASS(vdev));
> +
> +    return virtio_load(vdev, f, dc->vmsd->version_id);
> +}
> +
>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>  {
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 888c8de..01de49b 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -176,6 +176,31 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>  
>  void virtio_save(VirtIODevice *vdev, QEMUFile *f);
>  void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size);
> +void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size);
> +int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size);

These can be called virtio_device_get and virtio_device_put, and can be
static if...

> +
> +#define VMSTATE_VIRTIO_FIELD \
> +    {                                         \
> +        .name = "virtio",                     \
> +        .info = &(const VMStateInfo) {        \
> +            .name = "virtio",                 \
> +            .get = virtio_load_as_vmsi_get,   \
> +            .put = virtio_save_as_vmsi_put,   \
> +        },                                    \
> +        .flags = VMS_SINGLE,                  \
> +    }

... you only export the VMStateInfo.

Also please define the macro like the rest of the VMSTATE macros, i.e.

#define VMSTATE_PCI_DEVICE(_field, _state) {                           \
    .name       = (stringify(_field)),                                 \
    .size       = sizeof(VirtIODevice),                                \
    .vmsd       = &vmstate_virtio_device,                              \
    .flags      = VMS_SINGLE,                                          \
    .offset     = vmstate_offset_value(_state, _field, VirtIODevice),  \
}


> +#define VIRTIO_DEF_DEVICE_VMSD(devname, v, ...) \
> +    static const VMStateDescription vmstate_virtio_ ## devname = { \
> +        .name = "virtio-" #devname ,          \
> +        .minimum_version_id = v,              \
> +        .version_id = v,                      \
> +        .fields = (VMStateField[]) {          \
> +            VMSTATE_VIRTIO_FIELD,             \
> +            VMSTATE_END_OF_LIST()             \
> +        },                                    \
> +        __VA_ARGS__                           \
> +    };

and here, don't use the macro, using its expansion everywhere instead.

Paolo

>  #define VMSTATE_VIRTIO_DEVICE(devname, v, getf, putf) \
>      static const VMStateDescription vmstate_virtio_ ## devname = { \
> 

Re: [Qemu-devel] [PATCH 00/11] virtio migration: simplify vmstate helper

[Paolo Bonzini]

On 30/09/2016 16:19, Halil Pasic wrote:
> As a part of the long term  effort to convert migration to vmstate the
> migration of virtio devices was recently partially switched to vmstate
> starting with the outer layer (commit 5943124cc "virtio: Migration
> helper function and macro" and the subsequent "virtio-*: Wrap in
> vmstate" commits). This was done by introducing a vmstate based wrapper
> driving the well know virtio migration process. As this transition is
> still in progress, some synergies were left unexploited, and some things
> can be expressed in a more vmstatish way.

Another useful thing to do is to move code out of virtio_load and into a
post_load callback of vmstate_virtio.

Paolo

> Let us simplify a couple of things and get rid of some code duplication.
> 
> Did only a couple of smoke tests. Comprehensive testing is still to be done.
> 
> Halil Pasic (11):
>   virtio: add VIRTIO_DEF_DEVICE_VMSD macro
>   virtio-blk: convert to VIRTIO_DEF_DEVICE_VMSD
>   virtio-net: convert to VIRTIO_DEF_DEVICE_VMSD
>   virtio-9p: convert to VIRTIO_DEF_DEVICE_VMSD
>   virtio-serial: convert to VIRTIO_DEF_DEVICE_VMSD
>   virtio-gpu: do not use VMSTATE_VIRTIO_DEVICE
>   virtio-input: convert to VIRTIO_DEF_DEVICE_VMSD
>   virtio-scsi: convert to VIRTIO_DEF_DEVICE_VMSD
>   virtio-balloon: convert to VIRTIO_DEF_DEVICE_VMSD
>   virtio-rng: convert to VIRTIO_DEF_DEVICE_VMSD
>   virtio: remove unused VMSTATE_VIRTIO_DEVICE
> 
>  hw/9pfs/virtio-9p-device.c  |  7 +------
>  hw/block/virtio-blk.c       | 17 +----------------
>  hw/char/virtio-serial-bus.c |  8 +-------
>  hw/display/virtio-gpu.c     | 39 +++++++++++++++++++++++++++------------
>  hw/input/virtio-input.c     | 13 +++----------
>  hw/net/virtio-net.c         | 32 +++++++++++---------------------
>  hw/scsi/virtio-scsi.c       | 18 +-----------------
>  hw/virtio/virtio-balloon.c  |  7 +------
>  hw/virtio/virtio-rng.c      | 10 ++--------
>  hw/virtio/virtio.c          | 11 ++++++++++-
>  include/hw/virtio/virtio.h  | 31 ++++++++++++++++++-------------
>  11 files changed, 76 insertions(+), 117 deletions(-)
> 

Re: [Qemu-devel] [PATCH 00/11] virtio migration: simplify vmstate helper

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 30/09/2016 16:19, Halil Pasic wrote:
> > As a part of the long term  effort to convert migration to vmstate the
> > migration of virtio devices was recently partially switched to vmstate
> > starting with the outer layer (commit 5943124cc "virtio: Migration
> > helper function and macro" and the subsequent "virtio-*: Wrap in
> > vmstate" commits). This was done by introducing a vmstate based wrapper
> > driving the well know virtio migration process. As this transition is
> > still in progress, some synergies were left unexploited, and some things
> > can be expressed in a more vmstatish way.
> 
> Another useful thing to do is to move code out of virtio_load and into a
> post_load callback of vmstate_virtio.

Yes, and I'm hoping that we'll be able to get more of virtio_load/virtio_save
into normal macros within the vmstate_virtio eventually.

So other than the minor issues Paolo suggested on the macro:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave

> 
> Paolo
> 
> > Let us simplify a couple of things and get rid of some code duplication.
> > 
> > Did only a couple of smoke tests. Comprehensive testing is still to be done.
> > 
> > Halil Pasic (11):
> >   virtio: add VIRTIO_DEF_DEVICE_VMSD macro
> >   virtio-blk: convert to VIRTIO_DEF_DEVICE_VMSD
> >   virtio-net: convert to VIRTIO_DEF_DEVICE_VMSD
> >   virtio-9p: convert to VIRTIO_DEF_DEVICE_VMSD
> >   virtio-serial: convert to VIRTIO_DEF_DEVICE_VMSD
> >   virtio-gpu: do not use VMSTATE_VIRTIO_DEVICE
> >   virtio-input: convert to VIRTIO_DEF_DEVICE_VMSD
> >   virtio-scsi: convert to VIRTIO_DEF_DEVICE_VMSD
> >   virtio-balloon: convert to VIRTIO_DEF_DEVICE_VMSD
> >   virtio-rng: convert to VIRTIO_DEF_DEVICE_VMSD
> >   virtio: remove unused VMSTATE_VIRTIO_DEVICE
> > 
> >  hw/9pfs/virtio-9p-device.c  |  7 +------
> >  hw/block/virtio-blk.c       | 17 +----------------
> >  hw/char/virtio-serial-bus.c |  8 +-------
> >  hw/display/virtio-gpu.c     | 39 +++++++++++++++++++++++++++------------
> >  hw/input/virtio-input.c     | 13 +++----------
> >  hw/net/virtio-net.c         | 32 +++++++++++---------------------
> >  hw/scsi/virtio-scsi.c       | 18 +-----------------
> >  hw/virtio/virtio-balloon.c  |  7 +------
> >  hw/virtio/virtio-rng.c      | 10 ++--------
> >  hw/virtio/virtio.c          | 11 ++++++++++-
> >  include/hw/virtio/virtio.h  | 31 ++++++++++++++++++-------------
> >  11 files changed, 76 insertions(+), 117 deletions(-)
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 00/11] virtio migration: simplify vmstate helper

[Halil Pasic]

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



On 09/30/2016 05:02 PM, Paolo Bonzini wrote:
> 
> On 30/09/2016 16:19, Halil Pasic wrote:
>> > As a part of the long term  effort to convert migration to vmstate the
>> > migration of virtio devices was recently partially switched to vmstate
>> > starting with the outer layer (commit 5943124cc "virtio: Migration
>> > helper function and macro" and the subsequent "virtio-*: Wrap in
>> > vmstate" commits). This was done by introducing a vmstate based wrapper
>> > driving the well know virtio migration process. As this transition is
>> > still in progress, some synergies were left unexploited, and some things
>> > can be expressed in a more vmstatish way.
> Another useful thing to do is to move code out of virtio_load and into a
> post_load callback of vmstate_virtio.
> 
> Paolo
> 

Hi Paolo,

thanks for the review. I agree, but I see this out of scope for this patch
series. If you like I could do something along the lines as a separate patch.

Cheers,
Halil


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 00/11] virtio migration: simplify vmstate helper

[Paolo Bonzini]

On 03/10/2016 12:04, Halil Pasic wrote:
> > Another useful thing to do is to move code out of virtio_load and into a
> > post_load callback of vmstate_virtio.
> 
> thanks for the review. I agree, but I see this out of scope for this patch
> series. If you like I could do something along the lines as a separate patch.

Yes, I agree.

Paolo

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Halil Pasic]

On 09/30/2016 04:50 PM, Paolo Bonzini wrote:
> 
> 
> On 30/09/2016 16:19, Halil Pasic wrote:
>> In most cases the functions passed to VMSTATE_VIRTIO_DEVICE
>> only call the virtio_load and virtio_save wrappers. Some include some
>> pre- and post- massaging too. The massaging is better expressed
>> as such in the VMStateDescription.
>>
>> Let us introduce a new macro called VIRTIO_DEF_DEVICE_VMSD and replace
>> VMSTATE_VIRTIO_DEVICE with it gradually.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/virtio/virtio.c         | 15 +++++++++++++++
>>  include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 18ce333..ca0a780 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1622,6 +1622,21 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>>      virtio_save(VIRTIO_DEVICE(opaque), f);
>>  }
>>  
>> +/* A wrapper for use as a VMState .put function */
>> +void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size)
>> +{
>> +    virtio_save(VIRTIO_DEVICE(opaque), f);
>> +}
>> +
>> +/* A wrapper for use as a VMState .get function */
>> +int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size)
>> +{
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
>> +    DeviceClass *dc = DEVICE_CLASS(VIRTIO_DEVICE_GET_CLASS(vdev));
>> +
>> +    return virtio_load(vdev, f, dc->vmsd->version_id);
>> +}
>> +
>>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>>  {
>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 888c8de..01de49b 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -176,6 +176,31 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>>  
>>  void virtio_save(VirtIODevice *vdev, QEMUFile *f);
>>  void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size);
>> +void virtio_save_as_vmsi_put(QEMUFile *f, void *opaque, size_t size);
>> +int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size);
> 
> These can be called virtio_device_get and virtio_device_put, and can be
> static if...
> 
>> +
>> +#define VMSTATE_VIRTIO_FIELD \
>> +    {                                         \
>> +        .name = "virtio",                     \
>> +        .info = &(const VMStateInfo) {        \
>> +            .name = "virtio",                 \
>> +            .get = virtio_load_as_vmsi_get,   \
>> +            .put = virtio_save_as_vmsi_put,   \
>> +        },                                    \
>> +        .flags = VMS_SINGLE,                  \
>> +    }
> 
> ... you only export the VMStateInfo.
> 

Hi Paolo,

thanks I missed that, you are totally right.

> Also please define the macro like the rest of the VMSTATE macros, i.e.
> 
> #define VMSTATE_PCI_DEVICE(_field, _state) {                           \
>     .name       = (stringify(_field)),                                 \
>     .size       = sizeof(VirtIODevice),                                \
>     .vmsd       = &vmstate_virtio_device,                              \
>     .flags      = VMS_SINGLE,                                          \
>     .offset     = vmstate_offset_value(_state, _field, VirtIODevice),  \
> }
> 
> 

I'm not sure if I understand where are you coming from, but if I do, I
think I have to disagree here. I think you are coming from the normal 
device inheritance direction, where you have the parent storage object
(that is its instance data( embedded into the child, and the corresponding
parent state is supposed to get migrated as a (vmstate) field of the child.

Now if you look at the documentation of virtio migration (or the code) it is
pretty obvious that the situation is very different for virtio. Virtio migration
is kind of a template method approach (with virtio_load and virtio_save being
the template methods), and the parent (core, transport) and the child (device
specific) data are intermingled (the device data is in the middle). We can
not change this for compatibility reasons (sadly).

Thus I would find using the usual conventions rather misleading here. But
as I said, my understanding may be wrong, so if it is, please correct me.

>> +#define VIRTIO_DEF_DEVICE_VMSD(devname, v, ...) \
>> +    static const VMStateDescription vmstate_virtio_ ## devname = { \
>> +        .name = "virtio-" #devname ,          \
>> +        .minimum_version_id = v,              \
>> +        .version_id = v,                      \
>> +        .fields = (VMStateField[]) {          \
>> +            VMSTATE_VIRTIO_FIELD,             \
>> +            VMSTATE_END_OF_LIST()             \
>> +        },                                    \
>> +        __VA_ARGS__                           \
>> +    };
> 
> and here, don't use the macro, using its expansion everywhere instead.

>From the above I think it is clear, that I would like to keep this macro.
This macro is direct substitute for VMSTATE_VIRTIO_DEVICE, so I really do
not get what is the difference, and why should I use an expansion everywhere
instead, when the previous macro was fine and received no criticism during
the reviews. I think the only thing we gain with the expansion is code
duplication. If you insist on this, could you provide some background, so
I'm also able to understand the why?

Cheers,
Halil

> 
> Paolo
> 
>>  #define VMSTATE_VIRTIO_DEVICE(devname, v, getf, putf) \
>>      static const VMStateDescription vmstate_virtio_ ## devname = { \
>>
> 

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Paolo Bonzini]

On 03/10/2016 12:36, Halil Pasic wrote:
>> > #define VMSTATE_PCI_DEVICE(_field, _state) {                           \
>> >     .name       = (stringify(_field)),                                 \
>> >     .size       = sizeof(VirtIODevice),                                \
>> >     .vmsd       = &vmstate_virtio_device,                              \
>> >     .flags      = VMS_SINGLE,                                          \
>> >     .offset     = vmstate_offset_value(_state, _field, VirtIODevice),  \
>> > }
>> > 
>> > 
> I'm not sure if I understand where are you coming from, but if I do, I
> think I have to disagree here. I think you are coming from the normal 
> device inheritance direction, where you have the parent storage object
> (that is its instance data( embedded into the child, and the corresponding
> parent state is supposed to get migrated as a (vmstate) field of the child.
> 
> Now if you look at the documentation of virtio migration (or the code) it is
> pretty obvious that the situation is very different for virtio. Virtio migration
> is kind of a template method approach (with virtio_load and virtio_save being
> the template methods), and the parent (core, transport) and the child (device
> specific) data are intermingled (the device data is in the middle). We can
> not change this for compatibility reasons (sadly).

Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
differently for no particular reason.  Your macro is already doing
exactly the same as other VMSTATE_* macros, just with different
conventions for the arguments.  I don't see any advantage in changing that.

Regarding VIRTIO_DEF_DEVICE_VMSD, it's true that virtio migration is
currently done differently and we will definitely have to do things
somewhat different due to compatibility, but we can at least evolve
towards having a normal VMStateDescription (virtio-gpu is already more
similar to this).  Having everything hidden behind a magic macro makes
things harder to follow than other vmstate definitions.  It's okay when
you have a very small veneer as in commit 5943124cc, but in my opinion
having field definitions inside macro arguments is already too much.

Paolo

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Halil Pasic]

Hi Paolo,

I'm sorry, but I do not get it quite yet, or more exactly I have the
feeling I did not manage to bring my point over. So I will try with
more details.

On 10/03/2016 01:29 PM, Paolo Bonzini wrote:
> 
> 
> On 03/10/2016 12:36, Halil Pasic wrote:
>>>> #define VMSTATE_PCI_DEVICE(_field, _state) {

This was probably supposed to be VMSTATE_VIRTIO_DEVICE.
                           \
>>>>     .name       = (stringify(_field)),                                 \
>>>>     .size       = sizeof(VirtIODevice),                                \
>>>>     .vmsd       = &vmstate_virtio_device,                              \

This one does not exist at least very tricky to write because of the peculiarities
of virtio migration. This one would need to migrate the transport stuff too. And
the specialized device to, which is not normal.

>>>>     .flags      = VMS_SINGLE,                                          \
>>>>     .offset     = vmstate_offset_value(_state, _field, VirtIODevice),  \

This is useless if we keep virtio_save and virtio_load.

>>>> }
>>>>
>>>>
>> I'm not sure if I understand where are you coming from, but if I do, I
>> think I have to disagree here. I think you are coming from the normal 
>> device inheritance direction, where you have the parent storage object
>> (that is its instance data( embedded into the child, and the corresponding
>> parent state is supposed to get migrated as a (vmstate) field of the child.
>>
>> Now if you look at the documentation of virtio migration (or the code) it is
>> pretty obvious that the situation is very different for virtio. Virtio migration
>> is kind of a template method approach (with virtio_load and virtio_save being
>> the template methods), and the parent (core, transport) and the child (device
>> specific) data are intermingled (the device data is in the middle). We can
>> not change this for compatibility reasons (sadly).
> 
> Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
> differently for no particular reason.  Your macro is already doing
> exactly the same as other VMSTATE_* macros, just with different
> conventions for the arguments.  I don't see any advantage in changing that.

In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have
(a self contained) parent (in sense of inheritance) device, which is embedded
as _field in the specialized device and is migrated by the vmstatedescription
of the parent. The rest of the specialized devices state is represented by
the other fields.

VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and
virtio_save are called at the right time with the right arguments. The specialized
device is then migrated by the save/load callbacks of the device class, or
the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed
to be the only field, if the virtio device adheres to the virtio-migration
document. VMSTATE_VIRTIO_FIELD has no arguments because it is
a virtual field and does not depend on the offset stuff.

To summarize currently I have no idea how to write up the vmstate
field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your
expectations. 
> 
> Regarding VIRTIO_DEF_DEVICE_VMSD, it's true that virtio migration is
> currently done differently and we will definitely have to do things
> somewhat different due to compatibility, but we can at least evolve
> towards having a normal VMStateDescription (virtio-gpu is already more
> similar to this).  

Virtio-gpu does not adhere to the virtio-migration document because it saves
the specialized device state after the virtio_save is done (that is
after the common virtio subsections). This is however more like the normal
approach -- first you save the parent, then the child.


> Having everything hidden behind a magic macro makes
> things harder to follow than other vmstate definitions.  

Again in my opinion the virtio migration is different that the rest of the
vmstate migration stuff, and it's ugly to some extent. So the idea was
to make it look different and hide the ugliness behind the macro. 
If you insist i will create a version where the macros are expanded so
it's easier to say if this improves or worsens the readability.

> It's okay when
> you have a very small veneer as in commit 5943124cc, but in my opinion
> having field definitions inside macro arguments is already too much.

I think this is matter of taste, and your taste matters more ;). I do 
agree that the variadic for the massaging functions is more complicated
that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea
was that we end up with more readable code on the caller-side, but if you
prefer function pointers and NULLs if no callback is needed needed 
(most cases), I can live with that.

Sorry again for my thick head. 

Cheers,
Halil

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Paolo Bonzini]

On 03/10/2016 15:34, Halil Pasic wrote:
> Hi Paolo,
> 
> I'm sorry, but I do not get it quite yet, or more exactly I have the
> feeling I did not manage to bring my point over. So I will try with
> more details.
> 
> On 10/03/2016 01:29 PM, Paolo Bonzini wrote:
>>
>>
>> On 03/10/2016 12:36, Halil Pasic wrote:
>>>>> #define VMSTATE_PCI_DEVICE(_field, _state) {
> 
> This was probably supposed to be VMSTATE_VIRTIO_DEVICE.

Yes.

>>>>>     .name       = (stringify(_field)),                                 \
>>>>>     .size       = sizeof(VirtIODevice),                                \
>>>>>     .vmsd       = &vmstate_virtio_device,                              \
> 
> This one does not exist at least very tricky to write because of the peculiarities
> of virtio migration. This one would need to migrate the transport stuff too. And
> the specialized device to, which is not normal.

This is my own typo - this should be .info.  Sorry.

>> Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
>> differently for no particular reason.  Your macro is already doing
>> exactly the same as other VMSTATE_* macros, just with different
>> conventions for the arguments.  I don't see any advantage in changing that.
> 
> In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have
> (a self contained) parent (in sense of inheritance) device, which is embedded
> as _field in the specialized device and is migrated by the vmstatedescription
> of the parent. The rest of the specialized devices state is represented by
> the other fields.
> 
> VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and
> virtio_save are called at the right time with the right arguments. The specialized
> device is then migrated by the save/load callbacks of the device class, or
> the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed
> to be the only field, if the virtio device adheres to the virtio-migration
> document. VMSTATE_VIRTIO_FIELD has no arguments because it is
> a virtual field and does not depend on the offset stuff.
> 
> To summarize currently I have no idea how to write up the vmstate
> field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your
> expectations. 

As above.

>> Having everything hidden behind a magic macro makes
>> things harder to follow than other vmstate definitions.  
> 
> Again in my opinion the virtio migration is different that the rest of the
> vmstate migration stuff, and it's ugly to some extent. So the idea was
> to make it look different and hide the ugliness behind the macro. 
> If you insist i will create a version where the macros are expanded so
> it's easier to say if this improves or worsens the readability.

I agree it is not exactly the same as the other devices.  But in my
opinion it's not different-enough to do everything completely more, and
in the future we should aim at making it less different.

> I think this is matter of taste, and your taste matters more ;). I do 
> agree that the variadic for the massaging functions is more complicated
> that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea
> was that we end up with more readable code on the caller-side, but if you
> prefer function pointers and NULLs if no callback is needed needed 
> (most cases), I can live with that.

Well, the third possibility would be expanding the VMStateDescription
definition, :) where .post_load becomes just yet another initializer.

Paolo

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Halil Pasic]

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



On 10/03/2016 05:24 PM, Paolo Bonzini wrote:
> 
> 
> On 03/10/2016 15:34, Halil Pasic wrote:
>> Hi Paolo,
>>
>> I'm sorry, but I do not get it quite yet, or more exactly I have the
>> feeling I did not manage to bring my point over. So I will try with
>> more details.
>>
>> On 10/03/2016 01:29 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 03/10/2016 12:36, Halil Pasic wrote:
>>>>>> #define VMSTATE_PCI_DEVICE(_field, _state) {
>>
>> This was probably supposed to be VMSTATE_VIRTIO_DEVICE.
> 
> Yes.
> 
>>>>>>     .name       = (stringify(_field)),                                 \
>>>>>>     .size       = sizeof(VirtIODevice),                                \
>>>>>>     .vmsd       = &vmstate_virtio_device,                              \
>>
>> This one does not exist at least very tricky to write because of the peculiarities
>> of virtio migration. This one would need to migrate the transport stuff too. And
>> the specialized device to, which is not normal.
> 
> This is my own typo - this should be .info.  Sorry.
> 
>>> Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
>>> differently for no particular reason.  Your macro is already doing
>>> exactly the same as other VMSTATE_* macros, just with different
>>> conventions for the arguments.  I don't see any advantage in changing that.
>>
>> In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have
>> (a self contained) parent (in sense of inheritance) device, which is embedded
>> as _field in the specialized device and is migrated by the vmstatedescription
>> of the parent. The rest of the specialized devices state is represented by
>> the other fields.
>>
>> VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and
>> virtio_save are called at the right time with the right arguments. The specialized
>> device is then migrated by the save/load callbacks of the device class, or
>> the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed
>> to be the only field, if the virtio device adheres to the virtio-migration
>> document. VMSTATE_VIRTIO_FIELD has no arguments because it is
>> a virtual field and does not depend on the offset stuff.
>>
>> To summarize currently I have no idea how to write up the vmstate
>> field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your
>> expectations. 
> 
> As above.
> 
>>> Having everything hidden behind a magic macro makes
>>> things harder to follow than other vmstate definitions.  
>>
>> Again in my opinion the virtio migration is different that the rest of the
>> vmstate migration stuff, and it's ugly to some extent. So the idea was
>> to make it look different and hide the ugliness behind the macro. 
>> If you insist i will create a version where the macros are expanded so
>> it's easier to say if this improves or worsens the readability.
> 
> I agree it is not exactly the same as the other devices.  But in my
> opinion it's not different-enough to do everything completely more, and
> in the future we should aim at making it less different.
> 
>> I think this is matter of taste, and your taste matters more ;). I do 
>> agree that the variadic for the massaging functions is more complicated
>> that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea
>> was that we end up with more readable code on the caller-side, but if you
>> prefer function pointers and NULLs if no callback is needed needed 
>> (most cases), I can live with that.
> 
> Well, the third possibility would be expanding the VMStateDescription
> definition, :) where .post_load becomes just yet another initializer.
> 
> Paolo
> 

Hi Paolo,

clear now. I will come back with a v2 with the suggestions implemented.
Thank you very much for your time.

Cheers,
Halil


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 03/10/2016 15:34, Halil Pasic wrote:
> > Hi Paolo,
> > 
> > I'm sorry, but I do not get it quite yet, or more exactly I have the
> > feeling I did not manage to bring my point over. So I will try with
> > more details.
> > 
> > On 10/03/2016 01:29 PM, Paolo Bonzini wrote:
> >>
> >>
> >> On 03/10/2016 12:36, Halil Pasic wrote:
> >>>>> #define VMSTATE_PCI_DEVICE(_field, _state) {
> > 
> > This was probably supposed to be VMSTATE_VIRTIO_DEVICE.
> 
> Yes.
> 
> >>>>>     .name       = (stringify(_field)),                                 \
> >>>>>     .size       = sizeof(VirtIODevice),                                \
> >>>>>     .vmsd       = &vmstate_virtio_device,                              \
> > 
> > This one does not exist at least very tricky to write because of the peculiarities
> > of virtio migration. This one would need to migrate the transport stuff too. And
> > the specialized device to, which is not normal.
> 
> This is my own typo - this should be .info.  Sorry.
> 
> >> Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
> >> differently for no particular reason.  Your macro is already doing
> >> exactly the same as other VMSTATE_* macros, just with different
> >> conventions for the arguments.  I don't see any advantage in changing that.
> > 
> > In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have
> > (a self contained) parent (in sense of inheritance) device, which is embedded
> > as _field in the specialized device and is migrated by the vmstatedescription
> > of the parent. The rest of the specialized devices state is represented by
> > the other fields.
> > 
> > VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and
> > virtio_save are called at the right time with the right arguments. The specialized
> > device is then migrated by the save/load callbacks of the device class, or
> > the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed
> > to be the only field, if the virtio device adheres to the virtio-migration
> > document. VMSTATE_VIRTIO_FIELD has no arguments because it is
> > a virtual field and does not depend on the offset stuff.
> > 
> > To summarize currently I have no idea how to write up the vmstate
> > field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your
> > expectations. 
> 
> As above.
> 
> >> Having everything hidden behind a magic macro makes
> >> things harder to follow than other vmstate definitions.  
> > 
> > Again in my opinion the virtio migration is different that the rest of the
> > vmstate migration stuff, and it's ugly to some extent. So the idea was
> > to make it look different and hide the ugliness behind the macro. 
> > If you insist i will create a version where the macros are expanded so
> > it's easier to say if this improves or worsens the readability.
> 
> I agree it is not exactly the same as the other devices.  But in my
> opinion it's not different-enough to do everything completely more, and
> in the future we should aim at making it less different.
> 
> > I think this is matter of taste, and your taste matters more ;). I do 
> > agree that the variadic for the massaging functions is more complicated
> > that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea
> > was that we end up with more readable code on the caller-side, but if you
> > prefer function pointers and NULLs if no callback is needed needed 
> > (most cases), I can live with that.
> 
> Well, the third possibility would be expanding the VMStateDescription
> definition, :) where .post_load becomes just yet another initializer.


Hmm, I actually disagree here; I like the macros that Halil has got;
to me the important thing is that in most cases they're trivially short
and in the slightly weirder cases they're not much bigger.
The virtio-input conversion especially is nice and simple.
The only weird case is virtio-gpu, and well virtio-gpu is the one that's
odd here (compared to the rest of virtio).

I'd say we take these macros (apart from anything minor) - I'd anticipate
they'd evolve slowly as we move more and more chunks to VMState.

Dave


> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Paolo Bonzini]

On 05/10/2016 16:29, Dr. David Alan Gilbert wrote:
> The virtio-input conversion especially is nice and simple.
> The only weird case is virtio-gpu, and well virtio-gpu is the one that's
> odd here (compared to the rest of virtio).

Though virtio-gpu would be the one that could become nicer without the
macros. :)

What I don't like about the macros is that they don't allow you to
extend the VMStateDescription.  However, if you agree with Halil your
opinion counts more.

Paolo

> I'd say we take these macros (apart from anything minor) - I'd anticipate
> they'd evolve slowly as we move more and more chunks to VMState.

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 05/10/2016 16:29, Dr. David Alan Gilbert wrote:
> > The virtio-input conversion especially is nice and simple.
> > The only weird case is virtio-gpu, and well virtio-gpu is the one that's
> > odd here (compared to the rest of virtio).
> 
> Though virtio-gpu would be the one that could become nicer without the
> macros. :)

Yes, it would.

> What I don't like about the macros is that they don't allow you to
> extend the VMStateDescription.

Hmm so would this work:

add an 'extra_field'  that we normally leave empty:

#define VIRTIO_DEF_DEVICE_VMSD(devname, v, extra_field, ...) \
    static const VMStateDescription vmstate_virtio_ ## devname = { \
        .name = "virtio-" #devname ,          \
        .minimum_version_id = v,              \
        .version_id = v,                      \
        .fields = (VMStateField[]) {          \
            VMSTATE_VIRTIO_FIELD,             \
            extra_field                       \
            VMSTATE_END_OF_LIST()             \
        },                                    \
        __VA_ARGS__                           \
    };

so the normal case would gain messy commas:

    VIRTIO_DEF_DEVICE_VMSD(console,3, /* empty */)
the case with pre/post would be:
    VIRTIO_DEF_DEVICE_VMSD(vhost_vsock, VHOST_VSOCK_SAVEVM_VERSION, /* empty */,
    .pre_save = vhost_vsock_pre_save, .post_load = vhost_vsock_post_load)

.....

Define a macro for this field so it's passed as one entry:

#define VMSTATE_VIRTIO_GPU_FIELD \
         {                                        \
             .name = "virtio-gpu",                \
             .info = &(const VMStateInfo) {       \
                         .name = "virtio-gpu",    \
                         .get = virtio_gpu_load,  \
                         .put = virtio_gpu_save,  \
             },                                   \
             .flags = VMS_SINGLE,                 \
         } /* device */,

VIRTIO_DEF_DEVICE_VMSD(virtio-gpu, VIRTIO_GPU_VM_VERSION, VMSTATE_VIRTIO_GPU_FIELD)

>  However, if you agree with Halil your
> opinion counts more.

Well I think Halil's stuff moves it in the right direction;
with everything in VIRTIO_DEF_DEVICE_VMSD it means we can move things
out of virtio_load/save and into corresponding members in VIRTIO_DEF_DEVICE_VMSD's
 .fields array (before or after VMSTATE_VIRTIO_FIELD) without having
to change any of the devices. Eventually virtio_load/save disappear.

Dave

> 
> Paolo
> 
> > I'd say we take these macros (apart from anything minor) - I'd anticipate
> > they'd evolve slowly as we move more and more chunks to VMState.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Paolo Bonzini]

On 05/10/2016 21:00, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>
>>
>> On 05/10/2016 16:29, Dr. David Alan Gilbert wrote:
>>> The virtio-input conversion especially is nice and simple.
>>> The only weird case is virtio-gpu, and well virtio-gpu is the one that's
>>> odd here (compared to the rest of virtio).
>>
>> Though virtio-gpu would be the one that could become nicer without the
>> macros. :)
> 
> Yes, it would.
> 
>> What I don't like about the macros is that they don't allow you to
>> extend the VMStateDescription.
> 
> Hmm so would this work:
> 
> add an 'extra_field'  that we normally leave empty:
> 
> #define VIRTIO_DEF_DEVICE_VMSD(devname, v, extra_field, ...) \
>     static const VMStateDescription vmstate_virtio_ ## devname = { \
>         .name = "virtio-" #devname ,          \
>         .minimum_version_id = v,              \
>         .version_id = v,                      \
>         .fields = (VMStateField[]) {          \
>             VMSTATE_VIRTIO_FIELD,             \
>             extra_field                       \
>             VMSTATE_END_OF_LIST()             \
>         },                                    \
>         __VA_ARGS__                           \
>     };
> 
> so the normal case would gain messy commas:
> 
>     VIRTIO_DEF_DEVICE_VMSD(console,3, /* empty */)
> the case with pre/post would be:
>     VIRTIO_DEF_DEVICE_VMSD(vhost_vsock, VHOST_VSOCK_SAVEVM_VERSION, /* empty */,
>     .pre_save = vhost_vsock_pre_save, .post_load = vhost_vsock_post_load)
> 
> .....
> 
> Define a macro for this field so it's passed as one entry:
> 
> #define VMSTATE_VIRTIO_GPU_FIELD \
>          {                                        \
>              .name = "virtio-gpu",                \
>              .info = &(const VMStateInfo) {       \
>                          .name = "virtio-gpu",    \
>                          .get = virtio_gpu_load,  \
>                          .put = virtio_gpu_save,  \
>              },                                   \
>              .flags = VMS_SINGLE,                 \
>          } /* device */,
> 
> VIRTIO_DEF_DEVICE_VMSD(virtio-gpu, VIRTIO_GPU_VM_VERSION, VMSTATE_VIRTIO_GPU_FIELD)

I would just expand the macro in the virtio-gpu case.  For now what
Halil did is fine.

Paolo

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Halil Pasic]

On 10/05/2016 05:52 PM, Paolo Bonzini wrote:
> 
> On 05/10/2016 16:29, Dr. David Alan Gilbert wrote:
>> > The virtio-input conversion especially is nice and simple.
>> > The only weird case is virtio-gpu, and well virtio-gpu is the one that's
>> > odd here (compared to the rest of virtio).
> Though virtio-gpu would be the one that could become nicer without the
> macros. :)
> 
> What I don't like about the macros is that they don't allow you to
> extend the VMStateDescription.  However, if you agree with Halil your
> opinion counts more.
> 
> Paolo
> 

Hi guys!

Paolo you probably mean the fields part of the VMStateDescription. In my
opinion it is an important question if we should want the ability to
extend there. I was basing my assumptions on the series "[RFC 0/6]
converting some of virtio to VMState" by Dave. This series designates
VirtioDeviceClass.vmsd as the place for the state description of a
specialized device which would normally belong to where Paolo wants it.
The trouble in my opinion comes from the need to maintain compatibility
which basically means put the data exactly the way it was.

I do agree with Paolo that for new devices where this constrain is
non-existent we could and probably should get rid of the virtio
migration is very special stuff and do it normally. But for that in my
opinion we first need to figure out what is the new way of virtio
migration, document it, and provide an internal API for it. If we are
going to do that, I think it will actually turn out beneficial, that if
we name the weird (legacy) stuff differently because we can then name
the new not-so-weird stuff according to the normal conventions.

IMHO couple of questions to be answered before drafting the new approach
are:
* Is the proxy device (basically the transport) migrating itself or
is it migrated by the transport agnostic device (e.g. virtio-net)? In my
opinion these are two separate QOM/QDEV devices so they migrate
separately.  But this would arise some problems.
* How is inheritance and VMState is supposed to work together, including
versioning?

In the meanwhile I have re-implemented the series following Paolo's
suggestions (at least I hope) so I will send it out as a v2 shortly so we
can make a direct comparison.

Cheers,
Halil

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Halil Pasic]

On 10/05/2016 09:00 PM, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>
>>
>> On 05/10/2016 16:29, Dr. David Alan Gilbert wrote:
>>> The virtio-input conversion especially is nice and simple.
>>> The only weird case is virtio-gpu, and well virtio-gpu is the one that's
>>> odd here (compared to the rest of virtio).
>>
>> Though virtio-gpu would be the one that could become nicer without the
>> macros. :)
> 
> Yes, it would.
> 
>> What I don't like about the macros is that they don't allow you to
>> extend the VMStateDescription.

[..]
 
>>  However, if you agree with Halil your
>> opinion counts more.
> 
> Well I think Halil's stuff moves it in the right direction;
> with everything in VIRTIO_DEF_DEVICE_VMSD it means we can move things
> out of virtio_load/save and into corresponding members in VIRTIO_DEF_DEVICE_VMSD's
>  .fields array (before or after VMSTATE_VIRTIO_FIELD) without having
> to change any of the devices. Eventually virtio_load/save disappear.
>

I think this would end up messy if we want to preserve the current
behavior of virtio_load/save, and more importantly compatibility. AFAIU
we would have a 'synthetic' field for the configuration/transport
migration first, then a field dealing with the state in VirtIODevice
excluding config_vector and subsections (the state of virtio-core), then
we would have a sequence of fields dealing with the device specific
state of the transport agnostic virtio device (e.g. virtio-net,
virtio-gpu) that is emulate load/save from VirtIODeviceClass, then we
would have to take care of the virtio core subsections and possibly
device specific subsections (or have yet another synthetic field for
device specific subsections). This is actually the first thing I wanted
to do in response to "[RFC 0/6] converting some of virtio to VMState"
but after some thinking, concluded for myself that it will end up too
messy and way less readable that what Dave did there, without any
real gain to make the increased complexity justified.

Or am I mistaken here?

Halil