Re: [Qemu-devel] [PATCH RFC 1/1] virtio: migrate config_vector

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 18.05.2015 um 17:29 schrieb Cornelia Huck:
> On Mon, 18 May 2015 13:26:35 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

Had a discussion with Juan in irc regarding compatibility. As v2.3.0 is still
on v2 for the machine and v2.4.0 will have v4 we could simply go with Jasons
first approach that boild down to 

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 9ef1059..13d9dda 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1332,6 +1332,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
     SubchDev *s = dev->sch;
+    VirtIODevice *vdev = virtio_ccw_get_vdev(s);

     subch_device_save(s, f);
     if (dev->indicators != NULL) {
@@ -1355,6 +1356,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
         qemu_put_be32(f, 0);
         qemu_put_be64(f, 0UL);
     }
+    qemu_put_be16(f, vdev->config_vector);
     qemu_put_be64(f, dev->routes.adapter.ind_offset);
     qemu_put_byte(f, dev->thinint_isc);
 }
@@ -1363,6 +1365,7 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
     SubchDev *s = dev->sch;
+    VirtIODevice *vdev = virtio_ccw_get_vdev(s);
     int len;

     s->driver_data = dev;
@@ -1388,6 +1391,7 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
         qemu_get_be64(f);
         dev->summary_indicator = NULL;
     }
+    qemu_get_be16s(f, &vdev->config_vector);
     dev->routes.adapter.ind_offset = qemu_get_be64(f);
     dev->thinint_isc = qemu_get_byte(f);
     if (s->thinint_active) {


we only have to provide compatibility checks between releases. As there is no
production envrionment yet we can break compatibility and the migration code will
reject 2.3->2.4 and vice versa. 

In the future we have to be more careful, though.
Juan, Conny virtio-ccw has no version as virtio has no vmstate.
I am tempted to convert subch_device_save to use a vmstate_save_state 
instead of qemu_put*.... This would allow us to have a version for
the virtio-ccw stuff only. 
This would be an incompatible change, though. Maybe we should continue
to bump the machine version even if only the virtio migration format 
changes (which should rarely  happen).

Christian







> 
>> Would the subsection approach be more acceptable if I default needed to
>> false and have only virtio-ccw override it in a callback?
> 
> Smth like the following:
> 
> From 773a753475d9c89fb8dc789005a694d07b0cfac2 Mon Sep 17 00:00:00 2001
> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> Date: Tue, 12 May 2015 09:14:45 +0200
> Subject: [PATCH] virtio: migrate config_vector
> 
> Currently, config_vector is managed in VirtIODevice, but migration is
> handled by the transport; this led to one transport using config_vector
> migrating correctly (pci), while the other forgot it (ccw).
> 
> We don't want to simply add a value to the virtio-ccw save data (it
> may make migration failures hard to debug), and unfortunately we can't
> add optional vmstate subsections to a transport only. So let's add
> a new subsection for config_vector to the core and only let virtio-ccw
> signal via an optional callback that it wants the config_vector to be
> saved.
> 
> Reported-by: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/s390x/virtio-ccw.c          | 10 ++++++++++
>  hw/virtio/virtio.c             | 24 ++++++++++++++++++++++++
>  include/hw/virtio/virtio-bus.h |  5 +++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index c96101a..dfb4514 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1436,6 +1436,15 @@ static void virtio_ccw_device_unplugged(DeviceState *d)
>  
>      virtio_ccw_stop_ioeventfd(dev);
>  }
> +
> +static bool virtio_ccw_needs_confvec(DeviceState *d)
> +{
> +    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> +    VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> +
> +    return vdev && (vdev->config_vector != VIRTIO_NO_VECTOR);
> +}
> +
>  /**************** Virtio-ccw Bus Device Descriptions *******************/
>  
>  static Property virtio_ccw_net_properties[] = {
> @@ -1760,6 +1769,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
>      k->load_config = virtio_ccw_load_config;
>      k->device_plugged = virtio_ccw_device_plugged;
>      k->device_unplugged = virtio_ccw_device_unplugged;
> +    k->needs_confvec = virtio_ccw_needs_confvec;
>  }
>  
>  static const TypeInfo virtio_ccw_bus_info = {
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 6985e76..627be0d 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -903,6 +903,26 @@ static const VMStateDescription vmstate_virtio_device_endian = {
>      }
>  };
>  
> +static bool virtio_device_confvec_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +
> +    return k && k->needs_confvec ?
> +        k->needs_confvec(qbus->parent) : false;
> +}
> +
> +static const VMStateDescription vmstate_virtio_device_confvec = {
> +    .name = "virtio/config_vector",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16(config_vector, VirtIODevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio = {
>      .name = "virtio",
>      .version_id = 1,
> @@ -916,6 +936,10 @@ static const VMStateDescription vmstate_virtio = {
>              .vmsd = &vmstate_virtio_device_endian,
>              .needed = &virtio_device_endian_needed
>          },
> +        {
> +            .vmsd = &vmstate_virtio_device_confvec,
> +            .needed = &virtio_device_confvec_needed,
> +        },
>          { 0 }
>      }
>  };
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index a4588ca..79e6e8b 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -69,6 +69,11 @@ typedef struct VirtioBusClass {
>       * Note that changing this will break migration for this transport.
>       */
>      bool has_variable_vring_alignment;
> +    /*
> +     * (optional) Does the bus want the core to handle config_vector
> +     * migration? This is for backwards compatibility only.
> +     */
> +    bool (*needs_confvec)(DeviceState *d);
>  } VirtioBusClass;
>  
>  struct VirtioBusState {
>