[Qemu-devel] [PATCH RFC 0/1] virtio: fix config_vector migration issues

[Qemu-devel] [PATCH RFC 0/1] virtio: fix config_vector migration issues

[Cornelia Huck]

We've noticed a problem when migrating guests using virtio-ccw: the
ballooner will stop working after migration. Turns out that the
config_vector field was not migrated and still remained at its initial
value on the target system - therefore no working config changed
notifications.

config_vector is located in VirtIODevice, so we assumed core would take
care of migrating the value; virtio-pci does it itself.

We cannot simply add new values to the virtio stream, so I introduced
a new, optional, subsection only to be included when config_vector has
actually been changed from its initial value. Not wanting to introduce
problems with migrating virtio-pci based machines, I've introduced a
callback for virtio-pci to override creation of this subsection.

Migration for virtio-ccw based machines seems to be fixed with this one
(ballooner continues to work on the target system), but I'd like
confirmation whether this setup makes sense for virtio-pci as well.

Cornelia Huck (1):
  virtio: migrate config_vector

 hw/virtio/virtio-pci.c         | 14 ++++++++++++++
 hw/virtio/virtio.c             | 25 +++++++++++++++++++++++++
 include/hw/virtio/virtio-bus.h |  5 +++++
 3 files changed, 44 insertions(+)

-- 
2.1.4

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

[Cornelia Huck]

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

Let's have the core handle migration of config_vector in a vmstate
subsection instead, so that we can keep it backwards compatible and
no additional code is needed if config_vector is not used at all. Also
provide a callback for virtio-pci so they can avoid introducing a
subsection that is not needed and still be compatible in both directions.

Reported-by: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/virtio/virtio-pci.c         | 14 ++++++++++++++
 hw/virtio/virtio.c             | 25 +++++++++++++++++++++++++
 include/hw/virtio/virtio-bus.h |  5 +++++
 3 files changed, 44 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 867c9d1..4959b7d 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -971,6 +971,19 @@ static void virtio_pci_device_unplugged(DeviceState *d)
     virtio_pci_stop_ioeventfd(proxy);
 }
 
+static bool virtio_pci_needs_confvec(DeviceState *d)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+
+    /*
+     * We don't want the core to create an unneeded vmstate subsection
+     * when we already migrate the config vector ourselves.
+     */
+    return (vdev->config_vector != VIRTIO_NO_VECTOR) &&
+        !msix_present(&proxy->pci_dev);
+}
+
 static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
 {
     VirtIOPCIProxy *dev = VIRTIO_PCI(pci_dev);
@@ -1505,6 +1518,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->device_plugged = virtio_pci_device_plugged;
     k->device_unplugged = virtio_pci_device_unplugged;
     k->query_nvectors = virtio_pci_query_nvectors;
+    k->needs_confvec = virtio_pci_needs_confvec;
 }
 
 static const TypeInfo virtio_pci_bus_info = {
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6985e76..3af530e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -903,6 +903,27 @@ 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->needs_confvec ?
+        k->needs_confvec(qbus->parent) :
+        (vdev->config_vector != VIRTIO_NO_VECTOR);
+}
+
+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 +937,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 {
-- 
2.1.4

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

[Michael S. Tsirkin]

On Wed, May 13, 2015 at 04:42:02PM +0200, Cornelia Huck wrote:
> 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).
> 
> Let's have the core handle migration of config_vector in a vmstate
> subsection instead, so that we can keep it backwards compatible and
> no additional code is needed if config_vector is not used at all. Also
> provide a callback for virtio-pci so they can avoid introducing a
> subsection that is not needed and still be compatible in both directions.
> 
> Reported-by: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

I'm inclined to say just fix ccw, don't touch core.
queue vectors are still saved by transports, why special-case
config_vector?

> ---
>  hw/virtio/virtio-pci.c         | 14 ++++++++++++++
>  hw/virtio/virtio.c             | 25 +++++++++++++++++++++++++
>  include/hw/virtio/virtio-bus.h |  5 +++++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 867c9d1..4959b7d 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -971,6 +971,19 @@ static void virtio_pci_device_unplugged(DeviceState *d)
>      virtio_pci_stop_ioeventfd(proxy);
>  }
>  
> +static bool virtio_pci_needs_confvec(DeviceState *d)
> +{
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +
> +    /*
> +     * We don't want the core to create an unneeded vmstate subsection
> +     * when we already migrate the config vector ourselves.
> +     */
> +    return (vdev->config_vector != VIRTIO_NO_VECTOR) &&
> +        !msix_present(&proxy->pci_dev);

As config_vector is ignored without msix,
why is it a good idea to break migration if it's wrong?

> +}
> +
>  static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      VirtIOPCIProxy *dev = VIRTIO_PCI(pci_dev);
> @@ -1505,6 +1518,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
>      k->device_plugged = virtio_pci_device_plugged;
>      k->device_unplugged = virtio_pci_device_unplugged;
>      k->query_nvectors = virtio_pci_query_nvectors;
> +    k->needs_confvec = virtio_pci_needs_confvec;
>  }
>  
>  static const TypeInfo virtio_pci_bus_info = {
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 6985e76..3af530e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -903,6 +903,27 @@ 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->needs_confvec ?
> +        k->needs_confvec(qbus->parent) :
> +        (vdev->config_vector != VIRTIO_NO_VECTOR);
> +}
> +
> +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 +937,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 {
> -- 
> 2.1.4

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

[Cornelia Huck]

On Wed, 13 May 2015 16:58:58 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, May 13, 2015 at 04:42:02PM +0200, Cornelia Huck wrote:
> > 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).
> > 
> > Let's have the core handle migration of config_vector in a vmstate
> > subsection instead, so that we can keep it backwards compatible and
> > no additional code is needed if config_vector is not used at all. Also
> > provide a callback for virtio-pci so they can avoid introducing a
> > subsection that is not needed and still be compatible in both directions.
> > 
> > Reported-by: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> I'm inclined to say just fix ccw, don't touch core.
> queue vectors are still saved by transports, why special-case
> config_vector?

- config_vector is in the common VirtIODevice structure, not in
  transport-specific code
- new transports may make the same mistake
- AFAICS, there's no easy way to add transport-specific subsections -
  and simply adding config_vector in ccw would break compatibility

> 
> > ---
> >  hw/virtio/virtio-pci.c         | 14 ++++++++++++++
> >  hw/virtio/virtio.c             | 25 +++++++++++++++++++++++++
> >  include/hw/virtio/virtio-bus.h |  5 +++++
> >  3 files changed, 44 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 867c9d1..4959b7d 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -971,6 +971,19 @@ static void virtio_pci_device_unplugged(DeviceState *d)
> >      virtio_pci_stop_ioeventfd(proxy);
> >  }
> >  
> > +static bool virtio_pci_needs_confvec(DeviceState *d)
> > +{
> > +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> > +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > +
> > +    /*
> > +     * We don't want the core to create an unneeded vmstate subsection
> > +     * when we already migrate the config vector ourselves.
> > +     */
> > +    return (vdev->config_vector != VIRTIO_NO_VECTOR) &&
> > +        !msix_present(&proxy->pci_dev);
> 
> As config_vector is ignored without msix,
> why is it a good idea to break migration if it's wrong?

This should be covered by the first check, no?

> 
> > +}
> > +
> >  static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> >  {
> >      VirtIOPCIProxy *dev = VIRTIO_PCI(pci_dev);
> > @@ -1505,6 +1518,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> >      k->device_plugged = virtio_pci_device_plugged;
> >      k->device_unplugged = virtio_pci_device_unplugged;
> >      k->query_nvectors = virtio_pci_query_nvectors;
> > +    k->needs_confvec = virtio_pci_needs_confvec;
> >  }
> >  
> >  static const TypeInfo virtio_pci_bus_info = {
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 6985e76..3af530e 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -903,6 +903,27 @@ 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->needs_confvec ?
> > +        k->needs_confvec(qbus->parent) :
> > +        (vdev->config_vector != VIRTIO_NO_VECTOR);
> > +}
> > +
> > +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 +937,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 {
> > -- 
> > 2.1.4
> 

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

[Michael S. Tsirkin]

On Wed, May 13, 2015 at 05:03:35PM +0200, Cornelia Huck wrote:
> On Wed, 13 May 2015 16:58:58 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, May 13, 2015 at 04:42:02PM +0200, Cornelia Huck wrote:
> > > 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).
> > > 
> > > Let's have the core handle migration of config_vector in a vmstate
> > > subsection instead, so that we can keep it backwards compatible and
> > > no additional code is needed if config_vector is not used at all. Also
> > > provide a callback for virtio-pci so they can avoid introducing a
> > > subsection that is not needed and still be compatible in both directions.
> > > 
> > > Reported-by: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > 
> > I'm inclined to say just fix ccw, don't touch core.
> > queue vectors are still saved by transports, why special-case
> > config_vector?
> 
> - config_vector is in the common VirtIODevice structure, not in
>   transport-specific code

So is the queue vector.

> - new transports may make the same mistake

Well, we can't just pile up new interfaces that all do the same
thing slightly differently.
If you really want to migrate vectors in core, you can do

    if (k->should_save_vectors &&
	k->should_save_vectors(qbus->parent)) {
        qemu_put_be16(f, virtio_queue_vector(vdev, n));
    }

and similarly for queue vectors.

> - AFAICS, there's no easy way to add transport-specific subsections -
>   and simply adding config_vector in ccw would break compatibility

subsections break compatibility too.  The only way around that is to set
a flag to skip migrating config_vector for old machine types.


> > 
> > > ---
> > >  hw/virtio/virtio-pci.c         | 14 ++++++++++++++
> > >  hw/virtio/virtio.c             | 25 +++++++++++++++++++++++++
> > >  include/hw/virtio/virtio-bus.h |  5 +++++
> > >  3 files changed, 44 insertions(+)
> > > 
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index 867c9d1..4959b7d 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -971,6 +971,19 @@ static void virtio_pci_device_unplugged(DeviceState *d)
> > >      virtio_pci_stop_ioeventfd(proxy);
> > >  }
> > >  
> > > +static bool virtio_pci_needs_confvec(DeviceState *d)
> > > +{
> > > +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> > > +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > > +
> > > +    /*
> > > +     * We don't want the core to create an unneeded vmstate subsection
> > > +     * when we already migrate the config vector ourselves.
> > > +     */
> > > +    return (vdev->config_vector != VIRTIO_NO_VECTOR) &&
> > > +        !msix_present(&proxy->pci_dev);
> > 
> > As config_vector is ignored without msix,
> > why is it a good idea to break migration if it's wrong?
> 
> This should be covered by the first check, no?

Why should it?

> > 
> > > +}
> > > +
> > >  static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> > >  {
> > >      VirtIOPCIProxy *dev = VIRTIO_PCI(pci_dev);
> > > @@ -1505,6 +1518,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> > >      k->device_plugged = virtio_pci_device_plugged;
> > >      k->device_unplugged = virtio_pci_device_unplugged;
> > >      k->query_nvectors = virtio_pci_query_nvectors;
> > > +    k->needs_confvec = virtio_pci_needs_confvec;
> > >  }
> > >  
> > >  static const TypeInfo virtio_pci_bus_info = {
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 6985e76..3af530e 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -903,6 +903,27 @@ 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->needs_confvec ?
> > > +        k->needs_confvec(qbus->parent) :
> > > +        (vdev->config_vector != VIRTIO_NO_VECTOR);
> > > +}
> > > +
> > > +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 +937,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 {
> > > -- 
> > > 2.1.4
> > 

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

[Christian Borntraeger]

Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
>> - AFAICS, there's no easy way to add transport-specific subsections -
>>   and simply adding config_vector in ccw would break compatibility
> 
> subsections break compatibility too.  The only way around that is to set
> a flag to skip migrating config_vector for old machine types.

My main concern is about undetected compatibility issues. A subsection will 
tell the user that something went wrong. What happens if we just add a new
qemu_put_byte in the stream. Will the savevm core always detect that we have
too many or not enough bytes? If yes, adding new stuff in the stream will
always be detected in some way as error we can go with just adding
qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
Old/new QEMUs will then not be compatible - but thats probably ok as long as it
errors out.

My understanding was that we do not have a guarentee that this will be
detected all the time and having random junk in some variables is a debugging
nightmare. Is that correct?


Christian

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

[Michael S. Tsirkin]

On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
> >> - AFAICS, there's no easy way to add transport-specific subsections -
> >>   and simply adding config_vector in ccw would break compatibility
> > 
> > subsections break compatibility too.  The only way around that is to set
> > a flag to skip migrating config_vector for old machine types.
> 
> My main concern is about undetected compatibility issues. A subsection will 
> tell the user that something went wrong. What happens if we just add a new
> qemu_put_byte in the stream. Will the savevm core always detect that we have
> too many or not enough bytes? If yes, adding new stuff in the stream will
> always be detected in some way as error we can go with just adding
> qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
> Old/new QEMUs will then not be compatible - but thats probably ok as long as it
> errors out.
> 
> My understanding was that we do not have a guarentee that this will be
> detected all the time and having random junk in some variables is a debugging
> nightmare. Is that correct?
> 
> 
> Christian

It's not too bad - normally there's a bunch of strings that
helps you find out what's going on.
But if you really care about debuggability of migration streams, help move
forward dgilbert's RFC that switched to a self-delimiting format.
Just piling up random hacks in virtio seems like a wrong approach.

-- 
MST

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

[Christian Borntraeger]

Am 13.05.2015 um 23:47 schrieb Michael S. Tsirkin:
> On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
>> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
>>>> - AFAICS, there's no easy way to add transport-specific subsections -
>>>>   and simply adding config_vector in ccw would break compatibility
>>>
>>> subsections break compatibility too.  The only way around that is to set
>>> a flag to skip migrating config_vector for old machine types.
>>
>> My main concern is about undetected compatibility issues. A subsection will 
>> tell the user that something went wrong. What happens if we just add a new
>> qemu_put_byte in the stream. Will the savevm core always detect that we have
>> too many or not enough bytes? If yes, adding new stuff in the stream will
>> always be detected in some way as error we can go with just adding
>> qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
>> Old/new QEMUs will then not be compatible - but thats probably ok as long as it
>> errors out.
>>
>> My understanding was that we do not have a guarentee that this will be
>> detected all the time and having random junk in some variables is a debugging
>> nightmare. Is that correct?
>>
>>
>> Christian
> 
> It's not too bad - normally there's a bunch of strings that
> helps you find out what's going on.
> But if you really care about debuggability of migration streams, help move
> forward dgilbert's RFC that switched to a self-delimiting format.
> Just piling up random hacks in virtio seems like a wrong approach.
> 

Thats not my question. PLEASE try to understand my question.
I want a hard stop if migration changes in incompatible ways.
If adding a qemu_put_byte in virtio_ccw gets detected we can just fix
virtio_ccw AS YOU SUGGESTED. I just want to know if I can rely on that 
or not.

Christian 

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

[Michael S. Tsirkin]

On Thu, May 14, 2015 at 11:22:13AM +0200, Christian Borntraeger wrote:
> Am 13.05.2015 um 23:47 schrieb Michael S. Tsirkin:
> > On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
> >> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
> >>>> - AFAICS, there's no easy way to add transport-specific subsections -
> >>>>   and simply adding config_vector in ccw would break compatibility
> >>>
> >>> subsections break compatibility too.  The only way around that is to set
> >>> a flag to skip migrating config_vector for old machine types.
> >>
> >> My main concern is about undetected compatibility issues. A subsection will 
> >> tell the user that something went wrong. What happens if we just add a new
> >> qemu_put_byte in the stream. Will the savevm core always detect that we have
> >> too many or not enough bytes? If yes, adding new stuff in the stream will
> >> always be detected in some way as error we can go with just adding
> >> qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
> >> Old/new QEMUs will then not be compatible - but thats probably ok as long as it
> >> errors out.
> >>
> >> My understanding was that we do not have a guarentee that this will be
> >> detected all the time and having random junk in some variables is a debugging
> >> nightmare. Is that correct?
> >>
> >>
> >> Christian
> > 
> > It's not too bad - normally there's a bunch of strings that
> > helps you find out what's going on.
> > But if you really care about debuggability of migration streams, help move
> > forward dgilbert's RFC that switched to a self-delimiting format.
> > Just piling up random hacks in virtio seems like a wrong approach.
> > 
> 
> Thats not my question. PLEASE try to understand my question.
> I want a hard stop if migration changes in incompatible ways.
> If adding a qemu_put_byte in virtio_ccw gets detected we can just fix
> virtio_ccw AS YOU SUGGESTED. I just want to know if I can rely on that 
> or not.
> 
> Christian 

I answered exactly this question but let me try to spell the answer
out a bit more.

There are three answers:
1.  Yes, it's sure to get detected because everything gets shifted
    and then you get an unexpected string instead of next device name.
2.  If you want a more generic way to detect this, then please work
    on changing format for devices generally so each device
    section has a byte length attached to it. Then we know that
    when we make changes, they are detected as device will end
    earlier/later than expected.
3.  You can have a different workaround: add property "skip config vec
    on migration" and set it for old spapr machine types.
    old types continue losing config vec; new ones work better.

-- 
MST

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

[Paolo Bonzini]

On 14/05/2015 11:36, Michael S. Tsirkin wrote:
> 
> There are three answers:
> 1.  Yes, it's sure to get detected because everything gets shifted
>     and then you get an unexpected string instead of next device name.
> 2.  If you want a more generic way to detect this, then please work
>     on changing format for devices generally so each device
>     section has a byte length attached to it. Then we know that
>     when we make changes, they are detected as device will end
>     earlier/later than expected.

No need for that.  Subsections were introduced exactly to allow a change
to be compatible in the common case and incompatible in the uncommon
case.  Check out "git log -- grep subsection -- savevm.c".

> 3.  You can have a different workaround: add property "skip config vec
>     on migration" and set it for old spapr machine types.
>     old types continue losing config vec; new ones work better.

Why in the world would you want that?

Paolo

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

[Christian Borntraeger]

Am 14.05.2015 um 11:36 schrieb Michael S. Tsirkin:
> On Thu, May 14, 2015 at 11:22:13AM +0200, Christian Borntraeger wrote:
>> Am 13.05.2015 um 23:47 schrieb Michael S. Tsirkin:
>>> On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
>>>> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
>>>>>> - AFAICS, there's no easy way to add transport-specific subsections -
>>>>>>   and simply adding config_vector in ccw would break compatibility
>>>>>
>>>>> subsections break compatibility too.  The only way around that is to set
>>>>> a flag to skip migrating config_vector for old machine types.
>>>>
>>>> My main concern is about undetected compatibility issues. A subsection will 
>>>> tell the user that something went wrong. What happens if we just add a new
>>>> qemu_put_byte in the stream. Will the savevm core always detect that we have
>>>> too many or not enough bytes? If yes, adding new stuff in the stream will
>>>> always be detected in some way as error we can go with just adding
>>>> qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
>>>> Old/new QEMUs will then not be compatible - but thats probably ok as long as it
>>>> errors out.
>>>>
>>>> My understanding was that we do not have a guarentee that this will be
>>>> detected all the time and having random junk in some variables is a debugging
>>>> nightmare. Is that correct?
>>>>
>>>>
>>>> Christian
>>>
>>> It's not too bad - normally there's a bunch of strings that
>>> helps you find out what's going on.
>>> But if you really care about debuggability of migration streams, help move
>>> forward dgilbert's RFC that switched to a self-delimiting format.
>>> Just piling up random hacks in virtio seems like a wrong approach.
>>>
>>
>> Thats not my question. PLEASE try to understand my question.
>> I want a hard stop if migration changes in incompatible ways.
>> If adding a qemu_put_byte in virtio_ccw gets detected we can just fix
>> virtio_ccw AS YOU SUGGESTED. I just want to know if I can rely on that 
>> or not.
>>
>> Christian 
> 
> I answered exactly this question but let me try to spell the answer
> out a bit more.
> 
> There are three answers:
> 1.  Yes, it's sure to get detected because everything gets shifted
>     and then you get an unexpected string instead of next device name.

Ok got it. So simply adding a qemu_put/get_byte will always fail the migration and we
can just fixup virtio-ccw.c at the cost of being not migrateable between versions before/after that change. 

Thanks

Christian


> 2.  If you want a more generic way to detect this, then please work
>     on changing format for devices generally so each device
>     section has a byte length attached to it. Then we know that
>     when we make changes, they are detected as device will end
>     earlier/later than expected.
> 3.  You can have a different workaround: add property "skip config vec
>     on migration" and set it for old spapr machine types.
>     old types continue losing config vec; new ones work better.
> 

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

[Dr. David Alan Gilbert]

* Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> Am 14.05.2015 um 11:36 schrieb Michael S. Tsirkin:
> > On Thu, May 14, 2015 at 11:22:13AM +0200, Christian Borntraeger wrote:
> >> Am 13.05.2015 um 23:47 schrieb Michael S. Tsirkin:
> >>> On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
> >>>> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
> >>>>>> - AFAICS, there's no easy way to add transport-specific subsections -
> >>>>>>   and simply adding config_vector in ccw would break compatibility
> >>>>>
> >>>>> subsections break compatibility too.  The only way around that is to set
> >>>>> a flag to skip migrating config_vector for old machine types.
> >>>>
> >>>> My main concern is about undetected compatibility issues. A subsection will 
> >>>> tell the user that something went wrong. What happens if we just add a new
> >>>> qemu_put_byte in the stream. Will the savevm core always detect that we have
> >>>> too many or not enough bytes? If yes, adding new stuff in the stream will
> >>>> always be detected in some way as error we can go with just adding
> >>>> qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
> >>>> Old/new QEMUs will then not be compatible - but thats probably ok as long as it
> >>>> errors out.
> >>>>
> >>>> My understanding was that we do not have a guarentee that this will be
> >>>> detected all the time and having random junk in some variables is a debugging
> >>>> nightmare. Is that correct?
> >>>>
> >>>>
> >>>> Christian
> >>>
> >>> It's not too bad - normally there's a bunch of strings that
> >>> helps you find out what's going on.
> >>> But if you really care about debuggability of migration streams, help move
> >>> forward dgilbert's RFC that switched to a self-delimiting format.
> >>> Just piling up random hacks in virtio seems like a wrong approach.
> >>>
> >>
> >> Thats not my question. PLEASE try to understand my question.
> >> I want a hard stop if migration changes in incompatible ways.
> >> If adding a qemu_put_byte in virtio_ccw gets detected we can just fix
> >> virtio_ccw AS YOU SUGGESTED. I just want to know if I can rely on that 
> >> or not.
> >>
> >> Christian 
> > 
> > I answered exactly this question but let me try to spell the answer
> > out a bit more.
> > 
> > There are three answers:
> > 1.  Yes, it's sure to get detected because everything gets shifted
> >     and then you get an unexpected string instead of next device name.
> 
> Ok got it. So simply adding a qemu_put/get_byte will always fail the migration and we
> can just fixup virtio-ccw.c at the cost of being not migrateable between versions before/after that change. 

Gahhh!  No!   Adding an extra byte into the stream causes random horrible failures
that get very very confusing.  Yes, it will probably fail, but how it will
fail and what error you get is just guess work.
(And note, it's strictly a 'probably fail' - if you happen to land with
a zero byte where you expect the start of the next section the migration
code will think it's the end of the migration stream and blissfully start
the CPUs).

Dave

> 
> Thanks
> 
> Christian
> 
> 
> > 2.  If you want a more generic way to detect this, then please work
> >     on changing format for devices generally so each device
> >     section has a byte length attached to it. Then we know that
> >     when we make changes, they are detected as device will end
> >     earlier/later than expected.
> > 3.  You can have a different workaround: add property "skip config vec
> >     on migration" and set it for old spapr machine types.
> >     old types continue losing config vec; new ones work better.
> > 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Christian Borntraeger]

Am 14.05.2015 um 19:00 schrieb Dr. David Alan Gilbert:
> * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
>> Am 14.05.2015 um 11:36 schrieb Michael S. Tsirkin:
>>> On Thu, May 14, 2015 at 11:22:13AM +0200, Christian Borntraeger wrote:
>>>> Am 13.05.2015 um 23:47 schrieb Michael S. Tsirkin:
>>>>> On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
>>>>>> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
>>>>>>>> - AFAICS, there's no easy way to add transport-specific subsections -
>>>>>>>>   and simply adding config_vector in ccw would break compatibility
>>>>>>>
>>>>>>> subsections break compatibility too.  The only way around that is to set
>>>>>>> a flag to skip migrating config_vector for old machine types.
>>>>>>
>>>>>> My main concern is about undetected compatibility issues. A subsection will 
>>>>>> tell the user that something went wrong. What happens if we just add a new
>>>>>> qemu_put_byte in the stream. Will the savevm core always detect that we have
>>>>>> too many or not enough bytes? If yes, adding new stuff in the stream will
>>>>>> always be detected in some way as error we can go with just adding
>>>>>> qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
>>>>>> Old/new QEMUs will then not be compatible - but thats probably ok as long as it
>>>>>> errors out.
>>>>>>
>>>>>> My understanding was that we do not have a guarentee that this will be
>>>>>> detected all the time and having random junk in some variables is a debugging
>>>>>> nightmare. Is that correct?
>>>>>>
>>>>>>
>>>>>> Christian
>>>>>
>>>>> It's not too bad - normally there's a bunch of strings that
>>>>> helps you find out what's going on.
>>>>> But if you really care about debuggability of migration streams, help move
>>>>> forward dgilbert's RFC that switched to a self-delimiting format.
>>>>> Just piling up random hacks in virtio seems like a wrong approach.
>>>>>
>>>>
>>>> Thats not my question. PLEASE try to understand my question.
>>>> I want a hard stop if migration changes in incompatible ways.
>>>> If adding a qemu_put_byte in virtio_ccw gets detected we can just fix
>>>> virtio_ccw AS YOU SUGGESTED. I just want to know if I can rely on that 
>>>> or not.
>>>>
>>>> Christian 
>>>
>>> I answered exactly this question but let me try to spell the answer
>>> out a bit more.
>>>
>>> There are three answers:
>>> 1.  Yes, it's sure to get detected because everything gets shifted
>>>     and then you get an unexpected string instead of next device name.
>>
>> Ok got it. So simply adding a qemu_put/get_byte will always fail the migration and we
>> can just fixup virtio-ccw.c at the cost of being not migrateable between versions before/after that change. 
> 
> Gahhh!  No!   Adding an extra byte into the stream causes random horrible failures
> that get very very confusing.  Yes, it will probably fail, but how it will
> fail and what error you get is just guess work.
> (And note, it's strictly a 'probably fail' - if you happen to land with
> a zero byte where you expect the start of the next section the migration
> code will think it's the end of the migration stream and blissfully start
> the CPUs).

As Conny is away today, I will drive the dicussion a bit further :-)
So we really want a feature that detects this change and prevents migration.
I think its totally  fine to not be able to migrate between todays QEMUs and
a fixed version for  s390 as there no supported environment today I am aware
of. What would be the preferred way to go?
a: Connies approach of a subsection that is only migrated if necessary 
(config vector != 0xffff)
b: change virtio-ccw.c with put/get_be16 and make a new version of the
s390-ccw machine? The old version will set a property to not migrate the config
vector. (like Michaels 2nd suggestion)
c: ?

Christian

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

[Michael S. Tsirkin]

On Fri, May 15, 2015 at 09:08:07AM +0200, Christian Borntraeger wrote:
> Am 14.05.2015 um 19:00 schrieb Dr. David Alan Gilbert:
> > * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> >> Am 14.05.2015 um 11:36 schrieb Michael S. Tsirkin:
> >>> On Thu, May 14, 2015 at 11:22:13AM +0200, Christian Borntraeger wrote:
> >>>> Am 13.05.2015 um 23:47 schrieb Michael S. Tsirkin:
> >>>>> On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
> >>>>>> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
> >>>>>>>> - AFAICS, there's no easy way to add transport-specific subsections -
> >>>>>>>>   and simply adding config_vector in ccw would break compatibility
> >>>>>>>
> >>>>>>> subsections break compatibility too.  The only way around that is to set
> >>>>>>> a flag to skip migrating config_vector for old machine types.
> >>>>>>
> >>>>>> My main concern is about undetected compatibility issues. A subsection will 
> >>>>>> tell the user that something went wrong. What happens if we just add a new
> >>>>>> qemu_put_byte in the stream. Will the savevm core always detect that we have
> >>>>>> too many or not enough bytes? If yes, adding new stuff in the stream will
> >>>>>> always be detected in some way as error we can go with just adding
> >>>>>> qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
> >>>>>> Old/new QEMUs will then not be compatible - but thats probably ok as long as it
> >>>>>> errors out.
> >>>>>>
> >>>>>> My understanding was that we do not have a guarentee that this will be
> >>>>>> detected all the time and having random junk in some variables is a debugging
> >>>>>> nightmare. Is that correct?
> >>>>>>
> >>>>>>
> >>>>>> Christian
> >>>>>
> >>>>> It's not too bad - normally there's a bunch of strings that
> >>>>> helps you find out what's going on.
> >>>>> But if you really care about debuggability of migration streams, help move
> >>>>> forward dgilbert's RFC that switched to a self-delimiting format.
> >>>>> Just piling up random hacks in virtio seems like a wrong approach.
> >>>>>
> >>>>
> >>>> Thats not my question. PLEASE try to understand my question.
> >>>> I want a hard stop if migration changes in incompatible ways.
> >>>> If adding a qemu_put_byte in virtio_ccw gets detected we can just fix
> >>>> virtio_ccw AS YOU SUGGESTED. I just want to know if I can rely on that 
> >>>> or not.
> >>>>
> >>>> Christian 
> >>>
> >>> I answered exactly this question but let me try to spell the answer
> >>> out a bit more.
> >>>
> >>> There are three answers:
> >>> 1.  Yes, it's sure to get detected because everything gets shifted
> >>>     and then you get an unexpected string instead of next device name.
> >>
> >> Ok got it. So simply adding a qemu_put/get_byte will always fail the migration and we
> >> can just fixup virtio-ccw.c at the cost of being not migrateable between versions before/after that change. 
> > 
> > Gahhh!  No!   Adding an extra byte into the stream causes random horrible failures
> > that get very very confusing.  Yes, it will probably fail, but how it will
> > fail and what error you get is just guess work.
> > (And note, it's strictly a 'probably fail' - if you happen to land with
> > a zero byte where you expect the start of the next section the migration
> > code will think it's the end of the migration stream and blissfully start
> > the CPUs).
> 
> As Conny is away today, I will drive the dicussion a bit further :-)
> So we really want a feature that detects this change and prevents migration.
> I think its totally  fine to not be able to migrate between todays QEMUs and
> a fixed version for  s390 as there no supported environment today I am aware
> of. What would be the preferred way to go?
> a: Connies approach of a subsection that is only migrated if necessary 
> (config vector != 0xffff)
> b: change virtio-ccw.c with put/get_be16 and make a new version of the
> s390-ccw machine? The old version will set a property to not migrate the config
> vector. (like Michaels 2nd suggestion)
> c: ?

c. Set old machine as non-migrateable.

> Christian


I prefer b or c.

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

[Cornelia Huck]

On Fri, 15 May 2015 09:13:46 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, May 15, 2015 at 09:08:07AM +0200, Christian Borntraeger wrote:
> > Am 14.05.2015 um 19:00 schrieb Dr. David Alan Gilbert:
> > > * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> > >> Am 14.05.2015 um 11:36 schrieb Michael S. Tsirkin:
> > >>> On Thu, May 14, 2015 at 11:22:13AM +0200, Christian Borntraeger wrote:
> > >>>> Am 13.05.2015 um 23:47 schrieb Michael S. Tsirkin:
> > >>>>> On Wed, May 13, 2015 at 08:57:00PM +0200, Christian Borntraeger wrote:
> > >>>>>> Am 13.05.2015 um 18:14 schrieb Michael S. Tsirkin:
> > >>>>>>>> - AFAICS, there's no easy way to add transport-specific subsections -
> > >>>>>>>>   and simply adding config_vector in ccw would break compatibility
> > >>>>>>>
> > >>>>>>> subsections break compatibility too.  The only way around that is to set
> > >>>>>>> a flag to skip migrating config_vector for old machine types.
> > >>>>>>
> > >>>>>> My main concern is about undetected compatibility issues. A subsection will 
> > >>>>>> tell the user that something went wrong. What happens if we just add a new
> > >>>>>> qemu_put_byte in the stream. Will the savevm core always detect that we have
> > >>>>>> too many or not enough bytes? If yes, adding new stuff in the stream will
> > >>>>>> always be detected in some way as error we can go with just adding
> > >>>>>> qemu_put_be16/qemu_get_be16 in virtio_ccw_save_config/virtio_ccw_load_config.
> > >>>>>> Old/new QEMUs will then not be compatible - but thats probably ok as long as it
> > >>>>>> errors out.
> > >>>>>>
> > >>>>>> My understanding was that we do not have a guarentee that this will be
> > >>>>>> detected all the time and having random junk in some variables is a debugging
> > >>>>>> nightmare. Is that correct?
> > >>>>>>
> > >>>>>>
> > >>>>>> Christian
> > >>>>>
> > >>>>> It's not too bad - normally there's a bunch of strings that
> > >>>>> helps you find out what's going on.
> > >>>>> But if you really care about debuggability of migration streams, help move
> > >>>>> forward dgilbert's RFC that switched to a self-delimiting format.
> > >>>>> Just piling up random hacks in virtio seems like a wrong approach.
> > >>>>>
> > >>>>
> > >>>> Thats not my question. PLEASE try to understand my question.
> > >>>> I want a hard stop if migration changes in incompatible ways.
> > >>>> If adding a qemu_put_byte in virtio_ccw gets detected we can just fix
> > >>>> virtio_ccw AS YOU SUGGESTED. I just want to know if I can rely on that 
> > >>>> or not.
> > >>>>
> > >>>> Christian 
> > >>>
> > >>> I answered exactly this question but let me try to spell the answer
> > >>> out a bit more.
> > >>>
> > >>> There are three answers:
> > >>> 1.  Yes, it's sure to get detected because everything gets shifted
> > >>>     and then you get an unexpected string instead of next device name.
> > >>
> > >> Ok got it. So simply adding a qemu_put/get_byte will always fail the migration and we
> > >> can just fixup virtio-ccw.c at the cost of being not migrateable between versions before/after that change. 
> > > 
> > > Gahhh!  No!   Adding an extra byte into the stream causes random horrible failures
> > > that get very very confusing.  Yes, it will probably fail, but how it will
> > > fail and what error you get is just guess work.
> > > (And note, it's strictly a 'probably fail' - if you happen to land with
> > > a zero byte where you expect the start of the next section the migration
> > > code will think it's the end of the migration stream and blissfully start
> > > the CPUs).
> > 
> > As Conny is away today, I will drive the dicussion a bit further :-)
> > So we really want a feature that detects this change and prevents migration.
> > I think its totally  fine to not be able to migrate between todays QEMUs and
> > a fixed version for  s390 as there no supported environment today I am aware
> > of. What would be the preferred way to go?
> > a: Connies approach of a subsection that is only migrated if necessary 
> > (config vector != 0xffff)

Would the subsection approach be more acceptable if I default needed to
false and have only virtio-ccw override it in a callback?

> > b: change virtio-ccw.c with put/get_be16 and make a new version of the
> > s390-ccw machine? The old version will set a property to not migrate the config
> > vector. (like Michaels 2nd suggestion)

I'm still not sure mucking with individual values is a good idea.

> > c: ?
> 
> c. Set old machine as non-migrateable.

Overkill?

> 
> I prefer b or c.

I still prefer a :) - mainly because I think introducing subsections
makes it more obvious what is happening. It's a pity I can't introduce
a subsection in ccw alone, but have to go to the core for that.

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

[Cornelia Huck]

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

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

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

[Christian Borntraeger]

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

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

[Michael S. Tsirkin]

On Wed, Jun 03, 2015 at 01:59:15PM +0200, Christian Borntraeger wrote:
> 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. 

Let's do this for now.


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

I discussed another idea on IRC, which is to add section length
at the beginning of each section. Will simplify debugging as well.
Juan, you agreed with this idea, right?


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

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

[Paolo Bonzini]

On 13/05/2015 18:14, Michael S. Tsirkin wrote:
> > - AFAICS, there's no easy way to add transport-specific subsections -
> >   and simply adding config_vector in ccw would break compatibility
> 
> subsections break compatibility too.  The only way around that is to set
> a flag to skip migrating config_vector for old machine types.

That's expected.  Subsections break compatibility because sometimes
breaking migration is better than leaving a broken guest after migration.

Paolo

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

[Michael S. Tsirkin]

On Thu, May 14, 2015 at 10:24:15AM +0200, Paolo Bonzini wrote:
> 
> 
> On 13/05/2015 18:14, Michael S. Tsirkin wrote:
> > > - AFAICS, there's no easy way to add transport-specific subsections -
> > >   and simply adding config_vector in ccw would break compatibility
> > 
> > subsections break compatibility too.  The only way around that is to set
> > a flag to skip migrating config_vector for old machine types.
> 
> That's expected.  Subsections break compatibility because sometimes
> breaking migration is better than leaving a broken guest after migration.
> 
> Paolo

Right, and what happens is that some devices work kind of okay without
config interrupts? Christian - is this why you are trying to preserve
the broken behaviour for cross-version migration?

-- 
MST

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

[Paolo Bonzini]

On 14/05/2015 11:56, Michael S. Tsirkin wrote:
> > That's expected.  Subsections break compatibility because sometimes
> > breaking migration is better than leaving a broken guest after migration.
> 
> Right, and what happens is that some devices work kind of okay without
> config interrupts? Christian - is this why you are trying to preserve
> the broken behaviour for cross-version migration?

If you really want that, the hook should be in the virtio device class.
Keying on the machine type is almost never the answer, at least upstream.

Distros can always override it if they know what they're getting into.

Paolo

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

[Michael S. Tsirkin]

On Thu, May 14, 2015 at 12:04:05PM +0200, Paolo Bonzini wrote:
> 
> 
> On 14/05/2015 11:56, Michael S. Tsirkin wrote:
> > > That's expected.  Subsections break compatibility because sometimes
> > > breaking migration is better than leaving a broken guest after migration.
> > 
> > Right, and what happens is that some devices work kind of okay without
> > config interrupts? Christian - is this why you are trying to preserve
> > the broken behaviour for cross-version migration?
> 
> If you really want that, the hook should be in the virtio device class.
> Keying on the machine type is almost never the answer, at least upstream.
> Distros can always override it if they know what they're getting into.
> 
> Paolo

I beg to disagree.  Check out PC_COMPAT_ macros.  But if you are saying
migration with virtio-ccw is just too broken ATM, so we shouldn't care
about its migration compatibility upstream, then I can buy that.

-- 
MST

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

[Paolo Bonzini]

On 14/05/2015 12:07, Michael S. Tsirkin wrote:
> > If you really want that, the hook should be in the virtio device class.
> > Keying on the machine type is almost never the answer, at least upstream.
> > Distros can always override it if they know what they're getting into.
> 
> I beg to disagree.  Check out PC_COMPAT_ macros.

I'm talking specifically about migration subsections.  None of them are
keyed on the machine type downstream, as far as I know.

> But if you are saying
> migration with virtio-ccw is just too broken ATM, so we shouldn't care
> about its migration compatibility upstream, then I can buy that.

Yes, I'm also saying that (and Christian must agree or they would not
have posted this patch).

Paolo

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

[Christian Borntraeger]

Am 14.05.2015 um 12:09 schrieb Paolo Bonzini:
> 
> 
> On 14/05/2015 12:07, Michael S. Tsirkin wrote:
>>> If you really want that, the hook should be in the virtio device class.
>>> Keying on the machine type is almost never the answer, at least upstream.
>>> Distros can always override it if they know what they're getting into.
>>
>> I beg to disagree.  Check out PC_COMPAT_ macros.
> 
> I'm talking specifically about migration subsections.  None of them are
> keyed on the machine type downstream, as far as I know.
> 
>> But if you are saying
>> migration with virtio-ccw is just too broken ATM, so we shouldn't care
>> about its migration compatibility upstream, then I can buy that.
> 
> Yes, I'm also saying that (and Christian must agree or they would not
> have posted this patch).

So some background: Migration works most of the time but it will fail 
if config changes happen (like virtio-balloon). When trying to fix this Jason
came up with a patch that added qemu_put_be16(f, vdev->config_vector) in the 
virtio_ccw save_config. As this does not use vmstates and versions we were not
sure about how to ensure compatibility. So Conny came up with this proposal to
detect and reject migration from an old (broken) version.

Christian

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

[Paolo Bonzini]

On 13/05/2015 16:42, Cornelia Huck wrote:
> +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->needs_confvec ?
> +        k->needs_confvec(qbus->parent) :
> +        (vdev->config_vector != VIRTIO_NO_VECTOR);
> +}

This can call k->needs_confvec unconditionally, if the default
implementation is moved to a separate function.

Paolo