virtio migration: simplify vmstate helper

PATCH In review Halil Pasic 2 participants 15 messages Oct 6, 2016

Share & tools

Halil Pasic

author

2016-10-06 12:56 UTC #0 lore raw

v2 cover virtio migration: simplify vmstate helper

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.

NOTE: This series is exploring the suggestions of Paolo (I did my best to do everything as requested). I still think that we are better of with a macro that with spelling out the VMStateDescription for each device separately and redundantly. The LOC balance of the previous version was -41, this version is at +14 because of the expanded macros. IMHO the readability benefit of spelling out the vmsd definitions is questionabe (but it is beneficial if using ctags or grep). I hope for a good discussion, but I can live with this version too.

v1 --> v2:
* export VMStateInfo instead of helpers
* change semantic of VMSTATE_VIRTIO_DEVICE
* drop VIRTIO_DEF_DEVICE_VMSD macro, use its expansion instead

Halil Pasic (12):
virtio: prepare change VMSTATE_VIRTIO_DEVICE macro
virtio-blk: convert VMSTATE_VIRTIO_DEVICE
virtio-net: convert VMSTATE_VIRTIO_DEVICE
virtio-9p: convert VMSTATE_VIRTIO_DEVICE
virtio-serial: convert VMSTATE_VIRTIO_DEVICE
virtio-gpu: convert VMSTATE_VIRTIO_DEVICE
virtio-input: convert VMSTATE_VIRTIO_DEVICE
virtio-scsi: convert VMSTATE_VIRTIO_DEVICE
virtio-balloon: convert VMSTATE_VIRTIO_DEVICE
virtio-rng: convert VMSTATE_VIRTIO_DEVICE
vhost-vsock: convert VMSTATE_VIRTIO_DEVICE
virtio: cleanup VMSTATE_VIRTIO_DEVICE

 hw/9pfs/virtio-9p-device.c  | 15 +++++++++------
 hw/block/virtio-blk.c       | 25 +++++++++----------------
 hw/char/virtio-serial-bus.c | 16 +++++++++-------
 hw/display/virtio-gpu.c     | 39 +++++++++++++++++++++++++++------------
 hw/input/virtio-input.c     | 21 +++++++++++----------
 hw/net/virtio-net.c         | 40 +++++++++++++++++++---------------------
 hw/scsi/virtio-scsi.c       | 26 +++++++++-----------------
 hw/virtio/vhost-vsock.c     | 42 +++++++++++++++++++++---------------------
 hw/virtio/virtio-balloon.c  | 15 +++++++++------
 hw/virtio/virtio-rng.c      | 19 +++++++++++--------
 hw/virtio/virtio.c          | 17 ++++++++++++++++-
 include/hw/virtio/virtio.h  | 27 ++++++++-------------------
 12 files changed, 158 insertions(+), 144 deletions(-)

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #1 lore raw

PATCH v2 1/12 virtio: prepare change VMSTATE_VIRTIO_DEVICE macro

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 prepare for changing the semantic of the VMSTATE_VIRTIO_DEVICE macro so that it is more similar to the other VMSTATE_*_DEVICE macros in a sense that it is a field definition.

The preprocessor conditionals are going to be removed as soon as every usage is converted to the new semantic.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/virtio/virtio.c         | 21 +++++++++++++++++++++
 include/hw/virtio/virtio.h | 16 ++++++++++++++++
 2 files changed, 37 insertions(+)

hw/virtio/virtio.c +21 -0

index 18ce333..2c807dd 100644
@@ -1622,6 +1622,27 @@ 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 */
+static void virtio_device_put(QEMUFile *f, void *opaque, size_t size)
+{
+    virtio_save(VIRTIO_DEVICE(opaque), f);
+}
+
+/* A wrapper for use as a VMState .get function */
+static int virtio_device_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);
+}
+
+const VMStateInfo  virtio_vmstate_info = {
+    .name = "virtio",
+    .get = virtio_device_get,
+    .put = virtio_device_put,
+};
+
 static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
 {
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);

include/hw/virtio/virtio.h +16 -0

index 888c8de..db4c125 100644
@@ -177,6 +177,20 @@ 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);
 
+extern const VMStateInfo virtio_vmstate_info;
+
+#ifdef VMSTATE_VIRTIO_DEVICE_USE_NEW
+
+#define VMSTATE_VIRTIO_DEVICE \
+    {                                         \
+        .name = "virtio",                     \
+        .info = &virtio_vmstate_info,         \
+        .flags = VMS_SINGLE,                  \
+    }
+
+#else
+/* TODO remove conditional as soon as all users are converted */
+
 #define VMSTATE_VIRTIO_DEVICE(devname, v, getf, putf) \
     static const VMStateDescription vmstate_virtio_ ## devname = { \
         .name = "virtio-" #devname ,          \

@@ -196,6 +210,8 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size);
         }                                     \
     }
 
+#endif
+
 int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id);
 
 void virtio_notify_config(VirtIODevice *vdev);

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #2 lore raw

PATCH v2 2/12 virtio-blk: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/block/virtio-blk.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

hw/block/virtio-blk.c +11 ~~-16~~

index 3a6112f..d773453 100644
@@ -11,6 +11,8 @@
  *
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu-common.h"

@@ -803,13 +805,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 +823,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 +943,15 @@ static void virtio_blk_instance_init(Object *obj)
                                   DEVICE(obj), NULL);
 }
 
-VMSTATE_VIRTIO_DEVICE(blk, 2, virtio_blk_load, virtio_blk_save);
+static const VMStateDescription vmstate_virtio_blk = {
+    .name = "virtio-blk",
+    .minimum_version_id = 2,
+    .version_id = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+};
 
 static Property virtio_blk_properties[] = {
     DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf),

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #3 lore raw

PATCH v2 3/12 virtio-net: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/net/virtio-net.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

hw/net/virtio-net.c +21 ~~-21~~

index 6b8ae2c..8059457 100644
@@ -11,6 +11,8 @@
  *
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "qemu/iov.h"
 #include "hw/virtio/virtio.h"

@@ -1498,17 +1500,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 +1535,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 +1837,25 @@ 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);
+}
+
+static const VMStateDescription vmstate_virtio_net = {
+    .name = "virtio-net",
+    .minimum_version_id = VIRTIO_NET_VM_VERSION,
+    .version_id = VIRTIO_NET_VM_VERSION,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+    .pre_save = virtio_net_pre_save,
+};
 
 static Property virtio_net_properties[] = {
     DEFINE_PROP_BIT("csum", VirtIONet, host_features, VIRTIO_NET_F_CSUM, true),

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #4 lore raw

PATCH v2 4/12 virtio-9p: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/9pfs/virtio-9p-device.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

hw/9pfs/virtio-9p-device.c +11 -6

index 009b43f..c51dd9f 100644
@@ -11,6 +11,8 @@
  *
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "hw/virtio/virtio.h"
 #include "qemu/sockets.h"

@@ -97,11 +99,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 +165,15 @@ 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);
+static const VMStateDescription vmstate_virtio_9p = {
+    .name = "virtio-9p",
+    .minimum_version_id = 1,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+};
 
 static Property virtio_9p_properties[] = {
     DEFINE_PROP_STRING("mount_tag", V9fsVirtioState, state.fsconf.tag),

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #5 lore raw

PATCH v2 5/12 virtio-serial: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/char/virtio-serial-bus.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

hw/char/virtio-serial-bus.c +11 -7

index db57a38..5566e94 100644
@@ -18,6 +18,8 @@
  * GNU GPL, version 2 or (at your option) any later version.
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/iov.h"

@@ -728,12 +730,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 +1071,15 @@ 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);
+static const VMStateDescription vmstate_virtio_console = {
+    .name = "virtio-console",
+    .minimum_version_id = 3,
+    .version_id = 3,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+};
 
 static Property virtio_serial_properties[] = {
     DEFINE_PROP_UINT32("max_ports", VirtIOSerial, serial.max_virtserial_ports,

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #6 lore raw

PATCH v2 6/12 virtio-gpu: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro. 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 <[email protected]>

---
 hw/display/virtio-gpu.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

hw/display/virtio-gpu.c +29 ~~-12~~

index 7fe6ed8..4fcd63c 100644
@@ -11,6 +11,8 @@
  * See the COPYING file in the top-level directory.
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/iov.h"

@@ -990,12 +992,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 +1019,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 +1212,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_DEVICE /* 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),

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #7 lore raw

PATCH v2 7/12 virtio-input: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/input/virtio-input.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

hw/input/virtio-input.c +13 ~~-10~~

index ccdf730..5e31033 100644
@@ -4,6 +4,8 @@
  * top-level directory.
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/iov.h"

@@ -217,19 +219,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 +291,16 @@ 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);
+static const VMStateDescription vmstate_virtio_input = {
+    .name = "virtio-input",
+    .minimum_version_id = VIRTIO_INPUT_VM_VERSION,
+    .version_id = VIRTIO_INPUT_VM_VERSION,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+    .post_load = virtio_input_post_load,
+};
 
 static Property virtio_input_properties[] = {
     DEFINE_PROP_STRING("serial", VirtIOInput, serial),

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #8 lore raw

PATCH v2 8/12 virtio-scsi: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/scsi/virtio-scsi.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

hw/scsi/virtio-scsi.c +11 ~~-17~~

index e596b64..def316b 100644
@@ -13,6 +13,8 @@
  *
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "standard-headers/linux/virtio_ids.h"

@@ -663,22 +665,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 +907,15 @@ static Property virtio_scsi_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-VMSTATE_VIRTIO_DEVICE(scsi, 1, virtio_scsi_load, virtio_scsi_save);
+static const VMStateDescription vmstate_virtio_scsi = {
+    .name = "virtio-scsi",
+    .minimum_version_id = 1,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+};
 
 static void virtio_scsi_common_class_init(ObjectClass *klass, void *data)
 {

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #9 lore raw

PATCH v2 9/12 virtio-balloon: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/virtio/virtio-balloon.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

hw/virtio/virtio-balloon.c +11 -6

index 49a2f4a..1014ec6 100644
@@ -13,6 +13,8 @@
  *
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "qemu/iov.h"
 #include "qemu/timer.h"

@@ -404,11 +406,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 +491,15 @@ static void virtio_balloon_instance_init(Object *obj)
                         NULL, s, NULL);
 }
 
-VMSTATE_VIRTIO_DEVICE(balloon, 1, virtio_balloon_load, virtio_vmstate_save);
+static const VMStateDescription vmstate_virtio_balloon = {
+    .name = "virtio-balloon",
+    .minimum_version_id = 1,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+};
 
 static Property virtio_balloon_properties[] = {
     DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features,

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #10 lore raw

PATCH v2 10/12 virtio-rng: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/virtio/virtio-rng.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

hw/virtio/virtio-rng.c +13 -8

index cd8ca10..62867d1 100644
@@ -9,6 +9,8 @@
  * top-level directory.
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/iov.h"

@@ -120,15 +122,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 +212,16 @@ static void virtio_rng_device_unrealize(DeviceState *dev, Error **errp)
     virtio_cleanup(vdev);
 }
 
-VMSTATE_VIRTIO_DEVICE(rng, 1, virtio_rng_load, virtio_vmstate_save);
+static const VMStateDescription vmstate_virtio_rng = {
+    .name = "virtio-rng",
+    .minimum_version_id = 1,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+    .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

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #11 lore raw

PATCH v2 11/12 vhost-vsock: convert VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Use the new VMSTATE_VIRTIO_DEVICE macro.

Signed-off-by Halil Pasic <[email protected]>

---
 hw/virtio/vhost-vsock.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

hw/virtio/vhost-vsock.c +23 ~~-21~~

index bde2456..99cb216 100644
@@ -11,6 +11,8 @@
  * top-level directory.
  */
 
+#define VMSTATE_VIRTIO_DEVICE_USE_NEW
+
 #include <sys/ioctl.h>
 #include "qemu/osdep.h"
 #include "standard-headers/linux/virtio_vsock.h"

@@ -236,17 +238,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 +257,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;
-    VirtIODevice *vdev = VIRTIO_DEVICE(vsock);
-    int ret;
 
-    ret = virtio_load(vdev, f, VHOST_VSOCK_SAVEVM_VERSION);
-    if (ret) {
-        return ret;
-    }
+    /* 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);
 
     if (virtio_queue_get_addr(vdev, 2)) {
         /* Defer transport reset event to a vm clock timer so that virtqueue

@@ -288,12 +282,20 @@ 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);
+static const VMStateDescription vmstate_virtio_vhost_vsock = {
+    .name = "virtio-vhost_vsock",
+    .minimum_version_id = VHOST_VSOCK_SAVEVM_VERSION,
+    .version_id = VHOST_VSOCK_SAVEVM_VERSION,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+    .pre_save = vhost_vsock_pre_save,
+    .post_load = vhost_vsock_post_load,
+};
 
 static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
 {

Signature · 1 line

2.8.4

Halil Pasic

author

2016-10-06 12:56 UTC #12 lore raw

PATCH v2 12/12 virtio: cleanup VMSTATE_VIRTIO_DEVICE

In reply to Halil Pasic

Now all the usages of the old version of VMSTATE_VIRTIO_DEVICE are gone, so we can get rid of the conditionals, and the old macro.

Signed-off-by Halil Pasic <[email protected]>

---

We could also make virtio_load/save static (private) at this point. I think we should. Opinions?

---
 hw/9pfs/virtio-9p-device.c  |  2 --
 hw/block/virtio-blk.c       |  2 --
 hw/char/virtio-serial-bus.c |  2 --
 hw/display/virtio-gpu.c     |  2 --
 hw/input/virtio-input.c     |  2 --
 hw/net/virtio-net.c         |  2 --
 hw/scsi/virtio-scsi.c       |  2 --
 hw/virtio/vhost-vsock.c     |  2 --
 hw/virtio/virtio-balloon.c  |  2 --
 hw/virtio/virtio-rng.c      |  2 --
 hw/virtio/virtio.c          |  6 ------
 include/hw/virtio/virtio.h  | 27 ---------------------------
 12 files changed, 53 deletions(-)

hw/9pfs/virtio-9p-device.c +0 -2

index c51dd9f..81a4332 100644
@@ -11,8 +11,6 @@
  *
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "hw/virtio/virtio.h"
 #include "qemu/sockets.h"

hw/block/virtio-blk.c +0 -2

index d773453..3ed3b2c 100644
@@ -11,8 +11,6 @@
  *
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu-common.h"

hw/char/virtio-serial-bus.c +0 -2

index 5566e94..d4d7abd 100644
@@ -18,8 +18,6 @@
  * GNU GPL, version 2 or (at your option) any later version.
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/iov.h"

hw/display/virtio-gpu.c +0 -2

index 4fcd63c..fa6fd0e 100644
@@ -11,8 +11,6 @@
  * See the COPYING file in the top-level directory.
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/iov.h"

hw/input/virtio-input.c +0 -2

index 5e31033..b678ee9 100644
@@ -4,8 +4,6 @@
  * top-level directory.
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/iov.h"

hw/net/virtio-net.c +0 -2

index 8059457..4d33e3c 100644
@@ -11,8 +11,6 @@
  *
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "qemu/iov.h"
 #include "hw/virtio/virtio.h"

hw/scsi/virtio-scsi.c +0 -2

index def316b..4bb1495 100644
@@ -13,8 +13,6 @@
  *
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "standard-headers/linux/virtio_ids.h"

hw/virtio/vhost-vsock.c +0 -2

index 99cb216..b481562 100644
@@ -11,8 +11,6 @@
  * top-level directory.
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include <sys/ioctl.h>
 #include "qemu/osdep.h"
 #include "standard-headers/linux/virtio_vsock.h"

hw/virtio/virtio-balloon.c +0 -2

index 1014ec6..a655cf7 100644
@@ -13,8 +13,6 @@
  *
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "qemu/iov.h"
 #include "qemu/timer.h"

hw/virtio/virtio-rng.c +0 -2

index 62867d1..9639f4e 100644
@@ -9,8 +9,6 @@
  * top-level directory.
  */
 
-#define VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/iov.h"

hw/virtio/virtio.c +0 -6

index 2c807dd..db92b1d 100644
@@ -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 */
 static void virtio_device_put(QEMUFile *f, void *opaque, size_t size)
 {
     virtio_save(VIRTIO_DEVICE(opaque), f);

include/hw/virtio/virtio.h +0 ~~-27~~

index db4c125..8e4dc8c 100644
@@ -175,12 +175,9 @@ 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);
 
 extern const VMStateInfo virtio_vmstate_info;
 
-#ifdef VMSTATE_VIRTIO_DEVICE_USE_NEW
-
 #define VMSTATE_VIRTIO_DEVICE \
     {                                         \
         .name = "virtio",                     \

@@ -188,30 +185,6 @@ extern const VMStateInfo virtio_vmstate_info;
         .flags = VMS_SINGLE,                  \
     }
 
-#else
-/* TODO remove conditional as soon as all users are converted */
-
-#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()             \
-        }                                     \
-    }
-
-#endif
-
 int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id);
 
 void virtio_notify_config(VirtIODevice *vdev);

Signature · 1 line

2.8.4

Paolo Bonzini

2016-10-06 15:30 UTC #13 lore raw

In reply to Halil Pasic

Quoted 12 lines · Halil Pasic

Let us simplify a couple of things and get rid of some code duplication.
NOTE: This series is exploring the suggestions of Paolo (I did my best to do everything as requested). I still think that we are better of with a macro that with spelling out the VMStateDescription for each device separately and redundantly. The LOC balance of the previous version was -41, this version is at +14 because of the expanded macros. IMHO the readability benefit of spelling out the vmsd definitions is questionabe (but it is beneficial if using ctags or grep). I hope for a good discussion, but I can live with this version too.

Quoted 4 lines

v1 --> v2:
* export VMStateInfo instead of helpers
* change semantic of VMSTATE_VIRTIO_DEVICE
* drop VIRTIO_DEF_DEVICE_VMSD macro, use its expansion instead

Yes, this is what I meant... I personally like that everything is spelled out in

+static const VMStateDescription vmstate_virtio_blk = {

+    .name = "virtio-blk",
+    .minimum_version_id = 2,
+    .version_id = 2,
+    .fields = (VMStateField[]) {

+ VMSTATE_VIRTIO_DEVICE,
+ VMSTATE_END_OF_LIST()
+ },
+};

but I understand that other's mileages may vary...

Paolo

Halil Pasic

author

2016-10-06 17:05 UTC #14 lore raw

Quoted 24 lines · Paolo Bonzini

Quoted 12 lines · Halil Pasic
Let us simplify a couple of things and get rid of some code duplication.
NOTE: This series is exploring the suggestions of Paolo (I did my best to do everything as requested). I still think that we are better of with a macro that with spelling out the VMStateDescription for each device separately and redundantly. The LOC balance of the previous version was -41, this version is at +14 because of the expanded macros. IMHO the readability benefit of spelling out the vmsd definitions is questionabe (but it is beneficial if using ctags or grep). I hope for a good discussion, but I can live with this version too.
Quoted 4 lines
v1 --> v2:
* export VMStateInfo instead of helpers
* change semantic of VMSTATE_VIRTIO_DEVICE
* drop VIRTIO_DEF_DEVICE_VMSD macro, use its expansion instead
Yes, this is what I meant... I personally like that everything is spelled out in

Review

Quoted 9 lines

(quoted diff) +9 -0

+static const VMStateDescription vmstate_virtio_blk = {
+    .name = "virtio-blk",
+    .minimum_version_id = 2,
+    .version_id = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+};

Is a valid point :). There is one important thing that is not spelled out here, and that's the state specific to virtio_blk. In my opinion this definition suggests that virtio_blk has no state beyond its base's (in therms of inheritance, that is virtio core+transport) state, and this is wrong. That is probably the main reason why I prefer the 'magic macro' (others are my preference for the concise, and for making similar stuff look similar, and dissimilar things look different).

This however can be remedied if we do:
in virtio.h:
extern const VMStateField virtio_vmstate_fields[]; in virtio.c:
const VMStateField virtio_vmstate_fields[] = {
VMSTATE_VIRTIO_DEVICE, VMSTATE_END_OF_LIST()
};

in virtio-blk.c:
static const VMStateDescription vmstate_virtio_blk = {
.name = "virtio-blk",
.minimum_version_id = 2,
.version_id = 2,
.fields = virtio_vmstate_fields,
};

And would need to fix const correctness for VMStateField in vmstate.c and vmstate.h.

I have never thought of this because of the const but right
how I feel like I like this option the best:
* it works with grep and ctags
* its absolutely flexible
* the oddity of irtio_vmstate_fields can be documented at
the declaration/definition site
* we could later use VMSTATE_VIRTIO_DEVICE with the usual VMSTATE_PCI_DEVICE like semantic for the new devices once we establish a new migration schema in which the the derived specifies its state using the fields member of its vmsd
* redundancy is minimal:
** having separate control over minimum_version_id and version_id
seems appropriate to me and having it written out improves readability
** establishing a naming convention for the vmsd is not so important and
the one line we can spare there with the macro is not worth it

What do you thing guys should I make a v3 along this path?

Cheers,
Halil