* [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-14 15:41 [Qemu-devel] [PATCH RFC 0/8] virtio: migrate new properties Greg Kurz
@ 2014-05-14 15:41 ` Greg Kurz
2014-05-15 6:04 ` Amit Shah
2014-05-14 15:41 ` [Qemu-devel] [PATCH RFC 2/8] virtio-net: migrate subsections Greg Kurz
` (6 subsequent siblings)
7 siblings, 1 reply; 63+ messages in thread
From: Greg Kurz @ 2014-05-14 15:41 UTC (permalink / raw)
To: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini
Cc: Juan Quintela, Fam Zheng, Alexander Graf, Andreas Färber,
qemu-devel
There is a need to add some more fields to VirtIODevice that should be
migrated (broken status, endianness). The problem is that we do not
want to break compatibility while adding a new feature... This issue has
been addressed in the generic VMState code with the use of optional
subsections. As a *temporary* alternative to port the whole virtio
migration code to VMState, this patch mimics a similar subsectionning
ability for virtio.
Since each virtio device is streamed in its own section, the idea is to
stream subsections between the end of the device section and the start
of the next sections. This allows an older QEMU to complain and exit
when fed with subsections:
Unknown savevm section type 5
Error -22 while loading VM state
All users of virtio_load()/virtio_save() need to be patched because the
subsections are streamed AFTER the device itself.
Suggested-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/virtio/virtio.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
include/hw/virtio/virtio.h | 4 +++
2 files changed, 69 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index aeabf3a..f4eaa3f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -19,6 +19,7 @@
#include "hw/virtio/virtio.h"
#include "qemu/atomic.h"
#include "hw/virtio/virtio-bus.h"
+#include "migration/migration.h"
/*
* The alignment to use between consumer and producer parts of vring.
@@ -833,6 +834,70 @@ void virtio_notify_config(VirtIODevice *vdev)
virtio_notify_vector(vdev, vdev->config_vector);
}
+static const struct VirtIOSubsectionDescStruct {
+ const char *name;
+ int version_id;
+ void (*save)(VirtIODevice *vdev, QEMUFile *f);
+ int (*load)(VirtIODevice *vdev, QEMUFile *f);
+} virtio_subsection[] = {
+ { .name = NULL }
+};
+
+void virtio_save_subsections(VirtIODevice *vdev, QEMUFile *f)
+{
+ int i;
+
+ for (i = 0; virtio_subsection[i].name; i++) {
+ const char *name = virtio_subsection[i].name;
+ uint8_t len = strlen(name);
+
+ qemu_put_byte(f, QEMU_VM_SUBSECTION);
+ qemu_put_byte(f, len);
+ qemu_put_buffer(f, (uint8_t *) name, len);
+ qemu_put_be32(f, virtio_subsection[i].version_id);
+ (*virtio_subsection[i].save)(vdev, f);
+ }
+}
+
+int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
+{
+ while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
+ char idstr[256];
+ uint8_t len, size;
+ int i;
+
+ len = qemu_peek_byte(f, 1);
+ size = qemu_peek_buffer(f, (uint8_t *) idstr, len, 2);
+ if (size != len) {
+ break;
+ }
+
+ idstr[size] = 0;
+
+ for (i = 0; virtio_subsection[i].name; i++) {
+ if (strcmp(virtio_subsection[i].name, idstr) == 0) {
+ uint32_t version_id;
+ int ret;
+
+ qemu_file_skip(f, 1); /* subsection */
+ qemu_file_skip(f, 1); /* len */
+ qemu_file_skip(f, len); /* idstr */
+
+ version_id = qemu_get_be32(f);
+ if (version_id > virtio_subsection[i].version_id) {
+ return -EINVAL;
+ }
+ ret = (*virtio_subsection[i].load)(vdev, f);
+ if (ret) {
+ return ret;
+ }
+ }
+ }
+ }
+
+ return 0;
+}
+
void virtio_save(VirtIODevice *vdev, QEMUFile *f)
{
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3e54e90..82f852f 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -186,6 +186,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f);
int virtio_load(VirtIODevice *vdev, QEMUFile *f);
+void virtio_save_subsections(VirtIODevice *vdev, QEMUFile *f);
+
+int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f);
+
void virtio_notify_config(VirtIODevice *vdev);
void virtio_queue_set_notification(VirtQueue *vq, int enable);
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-14 15:41 ` [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream Greg Kurz
@ 2014-05-15 6:04 ` Amit Shah
2014-05-15 6:23 ` Michael S. Tsirkin
2014-05-15 6:49 ` Greg Kurz
0 siblings, 2 replies; 63+ messages in thread
From: Amit Shah @ 2014-05-15 6:04 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Michael S. Tsirkin,
Juan Quintela, Alexander Graf, qemu-devel, Stefan Hajnoczi,
Paolo Bonzini, Andreas Färber
On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> There is a need to add some more fields to VirtIODevice that should be
> migrated (broken status, endianness). The problem is that we do not
> want to break compatibility while adding a new feature... This issue has
> been addressed in the generic VMState code with the use of optional
> subsections. As a *temporary* alternative to port the whole virtio
> migration code to VMState, this patch mimics a similar subsectionning
> ability for virtio.
>
> Since each virtio device is streamed in its own section, the idea is to
> stream subsections between the end of the device section and the start
> of the next sections. This allows an older QEMU to complain and exit
> when fed with subsections:
>
> Unknown savevm section type 5
> Error -22 while loading VM state
Please make this configurable -- either via configure or device
properties. That avoids having to break existing configurations that
work without this patch.
> All users of virtio_load()/virtio_save() need to be patched because the
> subsections are streamed AFTER the device itself.
Since all have the same fixup, I'm wondering if a new section can be
added to the virtio-bus itself, which gets propagated to all devices
upon load in the dest.
Amit
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 6:04 ` Amit Shah
@ 2014-05-15 6:23 ` Michael S. Tsirkin
2014-05-15 6:46 ` Amit Shah
2014-05-15 6:49 ` Greg Kurz
1 sibling, 1 reply; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 6:23 UTC (permalink / raw)
To: Amit Shah
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Juan Quintela,
Alexander Graf, qemu-devel, Stefan Hajnoczi, Paolo Bonzini,
Andreas Färber, Greg Kurz
On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > There is a need to add some more fields to VirtIODevice that should be
> > migrated (broken status, endianness). The problem is that we do not
> > want to break compatibility while adding a new feature... This issue has
> > been addressed in the generic VMState code with the use of optional
> > subsections. As a *temporary* alternative to port the whole virtio
> > migration code to VMState, this patch mimics a similar subsectionning
> > ability for virtio.
> >
> > Since each virtio device is streamed in its own section, the idea is to
> > stream subsections between the end of the device section and the start
> > of the next sections. This allows an older QEMU to complain and exit
> > when fed with subsections:
> >
> > Unknown savevm section type 5
> > Error -22 while loading VM state
>
> Please make this configurable -- either via configure or device
> properties. That avoids having to break existing configurations that
> work without this patch.
>
> > All users of virtio_load()/virtio_save() need to be patched because the
> > subsections are streamed AFTER the device itself.
>
> Since all have the same fixup, I'm wondering if a new section can be
> added to the virtio-bus itself, which gets propagated to all devices
> upon load in the dest.
>
> Amit
This calls for a way for devices to inherit properties from the bus,
which doesn't exist ATM.
Fine but let's not hold up this patchset because of this.
--
MST
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 6:23 ` Michael S. Tsirkin
@ 2014-05-15 6:46 ` Amit Shah
2014-05-15 7:04 ` Greg Kurz
2014-05-15 7:14 ` Michael S. Tsirkin
0 siblings, 2 replies; 63+ messages in thread
From: Amit Shah @ 2014-05-15 6:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Juan Quintela,
Alexander Graf, qemu-devel, Stefan Hajnoczi, Paolo Bonzini,
Andreas Färber, Greg Kurz
On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> > On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > > There is a need to add some more fields to VirtIODevice that should be
> > > migrated (broken status, endianness). The problem is that we do not
> > > want to break compatibility while adding a new feature... This issue has
> > > been addressed in the generic VMState code with the use of optional
> > > subsections. As a *temporary* alternative to port the whole virtio
> > > migration code to VMState, this patch mimics a similar subsectionning
> > > ability for virtio.
BTW Greg, do you plan on working on vmstate for virtio?
> > > Since each virtio device is streamed in its own section, the idea is to
> > > stream subsections between the end of the device section and the start
> > > of the next sections. This allows an older QEMU to complain and exit
> > > when fed with subsections:
> > >
> > > Unknown savevm section type 5
> > > Error -22 while loading VM state
> >
> > Please make this configurable -- either via configure or device
> > properties. That avoids having to break existing configurations that
> > work without this patch.
> >
> > > All users of virtio_load()/virtio_save() need to be patched because the
> > > subsections are streamed AFTER the device itself.
> >
> > Since all have the same fixup, I'm wondering if a new section can be
> > added to the virtio-bus itself, which gets propagated to all devices
> > upon load in the dest.
>
> This calls for a way for devices to inherit properties from the bus,
> which doesn't exist ATM.
> Fine but let's not hold up this patchset because of this.
No, only suggestion is to add a migration section in the bus, and then
it's easier to do this in the post-migrate functions for each device
-- so only one new section gets introduced instead of all devices
being modified to send a new subsection.
Amit
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 6:46 ` Amit Shah
@ 2014-05-15 7:04 ` Greg Kurz
2014-05-15 9:20 ` Andreas Färber
2014-05-17 18:29 ` Michael S. Tsirkin
2014-05-15 7:14 ` Michael S. Tsirkin
1 sibling, 2 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-15 7:04 UTC (permalink / raw)
To: Amit Shah
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Michael S. Tsirkin, Alexander Graf, qemu-devel, Anthony Liguori,
Paolo Bonzini, Andreas Färber
On Thu, 15 May 2014 12:16:35 +0530
Amit Shah <amit.shah@redhat.com> wrote:
> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> > On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> > > On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > > > There is a need to add some more fields to VirtIODevice that should be
> > > > migrated (broken status, endianness). The problem is that we do not
> > > > want to break compatibility while adding a new feature... This issue has
> > > > been addressed in the generic VMState code with the use of optional
> > > > subsections. As a *temporary* alternative to port the whole virtio
> > > > migration code to VMState, this patch mimics a similar subsectionning
> > > > ability for virtio.
>
> BTW Greg, do you plan on working on vmstate for virtio?
>
Yes.
> > > > Since each virtio device is streamed in its own section, the idea is to
> > > > stream subsections between the end of the device section and the start
> > > > of the next sections. This allows an older QEMU to complain and exit
> > > > when fed with subsections:
> > > >
> > > > Unknown savevm section type 5
> > > > Error -22 while loading VM state
> > >
> > > Please make this configurable -- either via configure or device
> > > properties. That avoids having to break existing configurations that
> > > work without this patch.
> > >
> > > > All users of virtio_load()/virtio_save() need to be patched because the
> > > > subsections are streamed AFTER the device itself.
> > >
> > > Since all have the same fixup, I'm wondering if a new section can be
> > > added to the virtio-bus itself, which gets propagated to all devices
> > > upon load in the dest.
> >
> > This calls for a way for devices to inherit properties from the bus,
> > which doesn't exist ATM.
> > Fine but let's not hold up this patchset because of this.
>
> No, only suggestion is to add a migration section in the bus, and then
> it's easier to do this in the post-migrate functions for each device
> -- so only one new section gets introduced instead of all devices
> being modified to send a new subsection.
>
The main problem I see is that virtio sucks: as you see in patch 8, we have
to be careful not to call vring or virtqueue stuff before the device knows
its endianness or it breaks... I need to study how the virtio-bus gets
migrated to ensure the endian section is streamed before the devices.
> Amit
>
Thanks.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 7:04 ` Greg Kurz
@ 2014-05-15 9:20 ` Andreas Färber
2014-05-15 9:52 ` Michael S. Tsirkin
2014-05-15 10:08 ` Greg Kurz
2014-05-17 18:29 ` Michael S. Tsirkin
1 sibling, 2 replies; 63+ messages in thread
From: Andreas Färber @ 2014-05-15 9:20 UTC (permalink / raw)
To: Greg Kurz, Amit Shah
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Michael S. Tsirkin, Alexander Graf, qemu-devel, Anthony Liguori,
Paolo Bonzini
Am 15.05.2014 09:04, schrieb Greg Kurz:
> On Thu, 15 May 2014 12:16:35 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
>> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
>>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
>>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
>>>>> Since each virtio device is streamed in its own section, the idea is to
>>>>> stream subsections between the end of the device section and the start
>>>>> of the next sections. This allows an older QEMU to complain and exit
>>>>> when fed with subsections:
>>>>>
>>>>> Unknown savevm section type 5
>>>>> Error -22 while loading VM state
>>>>
>>>> Please make this configurable -- either via configure or device
>>>> properties. That avoids having to break existing configurations that
>>>> work without this patch.
Since backwards migration is not supported upstream, wouldn't it be
easiest to just add support for the subsection marker and skipping to
the end of section in that downstream?
>>>>> All users of virtio_load()/virtio_save() need to be patched because the
>>>>> subsections are streamed AFTER the device itself.
IMO this is calling for inversion of control - i.e. let virtio devices
call generic load/save functions that then dispatch to device-specific
code and let us add common stuff in a central place without forgetting
to add calls in some new device.
>>>> Since all have the same fixup, I'm wondering if a new section can be
>>>> added to the virtio-bus itself, which gets propagated to all devices
>>>> upon load in the dest.
>>>
>>> This calls for a way for devices to inherit properties from the bus,
>>> which doesn't exist ATM.
>>> Fine but let's not hold up this patchset because of this.
>>
>> No, only suggestion is to add a migration section in the bus, and then
>> it's easier to do this in the post-migrate functions for each device
>> -- so only one new section gets introduced instead of all devices
>> being modified to send a new subsection.
>>
>
> The main problem I see is that virtio sucks: as you see in patch 8, we have
> to be careful not to call vring or virtqueue stuff before the device knows
> its endianness or it breaks... I need to study how the virtio-bus gets
> migrated to ensure the endian section is streamed before the devices.
There is no ordering guarantee. The state needs to be migrated in the
device or bus where it sits, if post-load processing is required; i.e.,
if it's in VirtIODevice then something like this series, if it were on
VirtioBus exclusively (device asking bus for its endianness each time
and does not do post-load stuff) then endianness could be migrated as a
new bus section. Not sure if that would help the "broken" state though?
Would touch on Stefan's alias properties for anything but virtio-mmio.
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 9:20 ` Andreas Färber
@ 2014-05-15 9:52 ` Michael S. Tsirkin
2014-05-15 9:58 ` Andreas Färber
2014-05-15 12:33 ` Markus Armbruster
2014-05-15 10:08 ` Greg Kurz
1 sibling, 2 replies; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 9:52 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Greg Kurz
On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
> Am 15.05.2014 09:04, schrieb Greg Kurz:
> > On Thu, 15 May 2014 12:16:35 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> >> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> >>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> >>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> >>>>> Since each virtio device is streamed in its own section, the idea is to
> >>>>> stream subsections between the end of the device section and the start
> >>>>> of the next sections. This allows an older QEMU to complain and exit
> >>>>> when fed with subsections:
> >>>>>
> >>>>> Unknown savevm section type 5
> >>>>> Error -22 while loading VM state
> >>>>
> >>>> Please make this configurable -- either via configure or device
> >>>> properties. That avoids having to break existing configurations that
> >>>> work without this patch.
>
> Since backwards migration is not supported upstream, wouldn't it be
> easiest to just add support for the subsection marker and skipping to
> the end of section in that downstream?
Backwards and forwards migration need to be supported,
customers told us repeatedly. So some downstreams support this
and not supporting it upstream just means downstreams need
to do their own thing.
As importantly, ping-pong migration is the only
reliable way to stress migration.
So if we want to test cross-version we need it to work
both way.
Finally, the real issue and difficulty with cross-version migration is
making VM behave in a backwards compatible way. Serializing in a
compatible way is a trivial problem, or would be if the code wasn't a
mess :) Once you do the hard part, breaking migration because of the
trivial serialization issue is just silly. And special-casing forward
migration does not make code simpler, it really only leads to
proliferation of hacks and lack of symmetry.
So yes it's a useful feature, and no not supporting it does
not help anyway.
> >>>>> All users of virtio_load()/virtio_save() need to be patched because the
> >>>>> subsections are streamed AFTER the device itself.
>
> IMO this is calling for inversion of control - i.e. let virtio devices
> call generic load/save functions that then dispatch to device-specific
> code and let us add common stuff in a central place without forgetting
> to add calls in some new device.
>
> >>>> Since all have the same fixup, I'm wondering if a new section can be
> >>>> added to the virtio-bus itself, which gets propagated to all devices
> >>>> upon load in the dest.
> >>>
> >>> This calls for a way for devices to inherit properties from the bus,
> >>> which doesn't exist ATM.
> >>> Fine but let's not hold up this patchset because of this.
> >>
> >> No, only suggestion is to add a migration section in the bus, and then
> >> it's easier to do this in the post-migrate functions for each device
> >> -- so only one new section gets introduced instead of all devices
> >> being modified to send a new subsection.
> >>
> >
> > The main problem I see is that virtio sucks: as you see in patch 8, we have
> > to be careful not to call vring or virtqueue stuff before the device knows
> > its endianness or it breaks... I need to study how the virtio-bus gets
> > migrated to ensure the endian section is streamed before the devices.
>
> There is no ordering guarantee. The state needs to be migrated in the
> device or bus where it sits, if post-load processing is required; i.e.,
> if it's in VirtIODevice then something like this series, if it were on
> VirtioBus exclusively (device asking bus for its endianness each time
> and does not do post-load stuff) then endianness could be migrated as a
> new bus section. Not sure if that would help the "broken" state though?
>
> Would touch on Stefan's alias properties for anything but virtio-mmio.
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 9:52 ` Michael S. Tsirkin
@ 2014-05-15 9:58 ` Andreas Färber
2014-05-15 10:03 ` Michael S. Tsirkin
2014-05-15 12:33 ` Markus Armbruster
1 sibling, 1 reply; 63+ messages in thread
From: Andreas Färber @ 2014-05-15 9:58 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Greg Kurz
Am 15.05.2014 11:52, schrieb Michael S. Tsirkin:
> On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
>> Am 15.05.2014 09:04, schrieb Greg Kurz:
>>> On Thu, 15 May 2014 12:16:35 +0530
>>> Amit Shah <amit.shah@redhat.com> wrote:
>>>> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
>>>>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
>>>>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
>>>>>>> Since each virtio device is streamed in its own section, the idea is to
>>>>>>> stream subsections between the end of the device section and the start
>>>>>>> of the next sections. This allows an older QEMU to complain and exit
>>>>>>> when fed with subsections:
>>>>>>>
>>>>>>> Unknown savevm section type 5
>>>>>>> Error -22 while loading VM state
>>>>>>
>>>>>> Please make this configurable -- either via configure or device
>>>>>> properties. That avoids having to break existing configurations that
>>>>>> work without this patch.
>>
>> Since backwards migration is not supported upstream, wouldn't it be
>> easiest to just add support for the subsection marker and skipping to
>> the end of section in that downstream?
>
> Backwards and forwards migration need to be supported,
> customers told us repeatedly. So some downstreams support this
> and not supporting it upstream just means downstreams need
> to do their own thing.
>
> As importantly, ping-pong migration is the only
> reliable way to stress migration.
>
> So if we want to test cross-version we need it to work
> both way.
>
> Finally, the real issue and difficulty with cross-version migration is
> making VM behave in a backwards compatible way. Serializing in a
> compatible way is a trivial problem, or would be if the code wasn't a
> mess :) Once you do the hard part, breaking migration because of the
> trivial serialization issue is just silly. And special-casing forward
> migration does not make code simpler, it really only leads to
> proliferation of hacks and lack of symmetry.
>
> So yes it's a useful feature, and no not supporting it does
> not help anyway.
It seems you misunderstand. I was not saying it's not useful.
My point is that VMStateSubsections added in newer versions (and thus
not existing in older versions) need to be handled for any
VMState-converted devices. So why can't we make that work for virtio too?
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 9:58 ` Andreas Färber
@ 2014-05-15 10:03 ` Michael S. Tsirkin
2014-05-15 10:11 ` Andreas Färber
0 siblings, 1 reply; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 10:03 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Greg Kurz
On Thu, May 15, 2014 at 11:58:25AM +0200, Andreas Färber wrote:
> Am 15.05.2014 11:52, schrieb Michael S. Tsirkin:
> > On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
> >> Am 15.05.2014 09:04, schrieb Greg Kurz:
> >>> On Thu, 15 May 2014 12:16:35 +0530
> >>> Amit Shah <amit.shah@redhat.com> wrote:
> >>>> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> >>>>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> >>>>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> >>>>>>> Since each virtio device is streamed in its own section, the idea is to
> >>>>>>> stream subsections between the end of the device section and the start
> >>>>>>> of the next sections. This allows an older QEMU to complain and exit
> >>>>>>> when fed with subsections:
> >>>>>>>
> >>>>>>> Unknown savevm section type 5
> >>>>>>> Error -22 while loading VM state
> >>>>>>
> >>>>>> Please make this configurable -- either via configure or device
> >>>>>> properties. That avoids having to break existing configurations that
> >>>>>> work without this patch.
> >>
> >> Since backwards migration is not supported upstream, wouldn't it be
> >> easiest to just add support for the subsection marker and skipping to
> >> the end of section in that downstream?
> >
> > Backwards and forwards migration need to be supported,
> > customers told us repeatedly. So some downstreams support this
> > and not supporting it upstream just means downstreams need
> > to do their own thing.
> >
> > As importantly, ping-pong migration is the only
> > reliable way to stress migration.
> >
> > So if we want to test cross-version we need it to work
> > both way.
> >
> > Finally, the real issue and difficulty with cross-version migration is
> > making VM behave in a backwards compatible way. Serializing in a
> > compatible way is a trivial problem, or would be if the code wasn't a
> > mess :) Once you do the hard part, breaking migration because of the
> > trivial serialization issue is just silly. And special-casing forward
> > migration does not make code simpler, it really only leads to
> > proliferation of hacks and lack of symmetry.
> >
> > So yes it's a useful feature, and no not supporting it does
> > not help anyway.
>
> It seems you misunderstand. I was not saying it's not useful.
>
> My point is that VMStateSubsections added in newer versions (and thus
> not existing in older versions) need to be handled for any
> VMState-converted devices. So why can't we make that work for virtio too?
>
> Andreas
Sure.
AFAIK the way to this works is by adding an "field_exists" callback,
and not sending the section when we are in a compat mode.
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 10:03 ` Michael S. Tsirkin
@ 2014-05-15 10:11 ` Andreas Färber
2014-05-15 10:16 ` Michael S. Tsirkin
0 siblings, 1 reply; 63+ messages in thread
From: Andreas Färber @ 2014-05-15 10:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Greg Kurz
Am 15.05.2014 12:03, schrieb Michael S. Tsirkin:
> On Thu, May 15, 2014 at 11:58:25AM +0200, Andreas Färber wrote:
>> Am 15.05.2014 11:52, schrieb Michael S. Tsirkin:
>>> On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
>>>> Am 15.05.2014 09:04, schrieb Greg Kurz:
>>>>> On Thu, 15 May 2014 12:16:35 +0530
>>>>> Amit Shah <amit.shah@redhat.com> wrote:
>>>>>> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
>>>>>>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
>>>>>>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
>>>>>>>>> Since each virtio device is streamed in its own section, the idea is to
>>>>>>>>> stream subsections between the end of the device section and the start
>>>>>>>>> of the next sections. This allows an older QEMU to complain and exit
>>>>>>>>> when fed with subsections:
>>>>>>>>>
>>>>>>>>> Unknown savevm section type 5
>>>>>>>>> Error -22 while loading VM state
>>>>>>>>
>>>>>>>> Please make this configurable -- either via configure or device
>>>>>>>> properties. That avoids having to break existing configurations that
>>>>>>>> work without this patch.
>>>>
>>>> Since backwards migration is not supported upstream, wouldn't it be
>>>> easiest to just add support for the subsection marker and skipping to
>>>> the end of section in that downstream?
>>>
>>> Backwards and forwards migration need to be supported,
>>> customers told us repeatedly. So some downstreams support this
>>> and not supporting it upstream just means downstreams need
>>> to do their own thing.
>>>
>>> As importantly, ping-pong migration is the only
>>> reliable way to stress migration.
>>>
>>> So if we want to test cross-version we need it to work
>>> both way.
>>>
>>> Finally, the real issue and difficulty with cross-version migration is
>>> making VM behave in a backwards compatible way. Serializing in a
>>> compatible way is a trivial problem, or would be if the code wasn't a
>>> mess :) Once you do the hard part, breaking migration because of the
>>> trivial serialization issue is just silly. And special-casing forward
>>> migration does not make code simpler, it really only leads to
>>> proliferation of hacks and lack of symmetry.
>>>
>>> So yes it's a useful feature, and no not supporting it does
>>> not help anyway.
>>
>> It seems you misunderstand. I was not saying it's not useful.
>>
>> My point is that VMStateSubsections added in newer versions (and thus
>> not existing in older versions) need to be handled for any
>> VMState-converted devices. So why can't we make that work for virtio too?
>
> Sure.
> AFAIK the way to this works is by adding an "field_exists" callback,
> and not sending the section when we are in a compat mode.
OK, but upstream always sends the latest version today. So isn't that
just two ifs that you would need to add in save and load functions added
here for the downstream? x86_64 is unaffected from ppc's endianness
changes and you still do ppc64 BE, so there's no real functional problem
for RHEL, is there?
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 10:11 ` Andreas Färber
@ 2014-05-15 10:16 ` Michael S. Tsirkin
2014-05-15 12:00 ` Andreas Färber
0 siblings, 1 reply; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 10:16 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Greg Kurz
On Thu, May 15, 2014 at 12:11:12PM +0200, Andreas Färber wrote:
> Am 15.05.2014 12:03, schrieb Michael S. Tsirkin:
> > On Thu, May 15, 2014 at 11:58:25AM +0200, Andreas Färber wrote:
> >> Am 15.05.2014 11:52, schrieb Michael S. Tsirkin:
> >>> On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
> >>>> Am 15.05.2014 09:04, schrieb Greg Kurz:
> >>>>> On Thu, 15 May 2014 12:16:35 +0530
> >>>>> Amit Shah <amit.shah@redhat.com> wrote:
> >>>>>> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> >>>>>>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> >>>>>>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> >>>>>>>>> Since each virtio device is streamed in its own section, the idea is to
> >>>>>>>>> stream subsections between the end of the device section and the start
> >>>>>>>>> of the next sections. This allows an older QEMU to complain and exit
> >>>>>>>>> when fed with subsections:
> >>>>>>>>>
> >>>>>>>>> Unknown savevm section type 5
> >>>>>>>>> Error -22 while loading VM state
> >>>>>>>>
> >>>>>>>> Please make this configurable -- either via configure or device
> >>>>>>>> properties. That avoids having to break existing configurations that
> >>>>>>>> work without this patch.
> >>>>
> >>>> Since backwards migration is not supported upstream, wouldn't it be
> >>>> easiest to just add support for the subsection marker and skipping to
> >>>> the end of section in that downstream?
> >>>
> >>> Backwards and forwards migration need to be supported,
> >>> customers told us repeatedly. So some downstreams support this
> >>> and not supporting it upstream just means downstreams need
> >>> to do their own thing.
> >>>
> >>> As importantly, ping-pong migration is the only
> >>> reliable way to stress migration.
> >>>
> >>> So if we want to test cross-version we need it to work
> >>> both way.
> >>>
> >>> Finally, the real issue and difficulty with cross-version migration is
> >>> making VM behave in a backwards compatible way. Serializing in a
> >>> compatible way is a trivial problem, or would be if the code wasn't a
> >>> mess :) Once you do the hard part, breaking migration because of the
> >>> trivial serialization issue is just silly. And special-casing forward
> >>> migration does not make code simpler, it really only leads to
> >>> proliferation of hacks and lack of symmetry.
> >>>
> >>> So yes it's a useful feature, and no not supporting it does
> >>> not help anyway.
> >>
> >> It seems you misunderstand. I was not saying it's not useful.
> >>
> >> My point is that VMStateSubsections added in newer versions (and thus
> >> not existing in older versions) need to be handled for any
> >> VMState-converted devices. So why can't we make that work for virtio too?
> >
> > Sure.
> > AFAIK the way to this works is by adding an "field_exists" callback,
> > and not sending the section when we are in a compat mode.
>
> OK, but upstream always sends the latest version today.
> So isn't that
> just two ifs that you would need to add in save and load functions added
> here for the downstream? x86_64 is unaffected from ppc's endianness
> changes and you still do ppc64 BE, so there's no real functional problem
> for RHEL, is there?
>
> Andreas
I'm sorry I don't understand what you are saying here.
Simply put, if you specify a compatible -M then your
device should behave, and migrate, exactly like and old
qemu did.
With *any* change, there's always the temptation to say
"oh but old behaviour was too buggy we should just ignore it".
Seems to make sense in each case but does not scale,
you end up with breakages in all version without exception.
So this should be a very high bar to overcome.
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 10:16 ` Michael S. Tsirkin
@ 2014-05-15 12:00 ` Andreas Färber
2014-05-15 12:20 ` Michael S. Tsirkin
2014-05-15 13:49 ` Greg Kurz
0 siblings, 2 replies; 63+ messages in thread
From: Andreas Färber @ 2014-05-15 12:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Greg Kurz
Am 15.05.2014 12:16, schrieb Michael S. Tsirkin:
> On Thu, May 15, 2014 at 12:11:12PM +0200, Andreas Färber wrote:
>> Am 15.05.2014 12:03, schrieb Michael S. Tsirkin:
>>> On Thu, May 15, 2014 at 11:58:25AM +0200, Andreas Färber wrote:
>>>> Am 15.05.2014 11:52, schrieb Michael S. Tsirkin:
>>>>> On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
>>>>>> Am 15.05.2014 09:04, schrieb Greg Kurz:
>>>>>>> On Thu, 15 May 2014 12:16:35 +0530
>>>>>>> Amit Shah <amit.shah@redhat.com> wrote:
>>>>>>>> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
>>>>>>>>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
>>>>>>>>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
>>>>>>>>>>> Since each virtio device is streamed in its own section, the idea is to
>>>>>>>>>>> stream subsections between the end of the device section and the start
>>>>>>>>>>> of the next sections. This allows an older QEMU to complain and exit
>>>>>>>>>>> when fed with subsections:
>>>>>>>>>>>
>>>>>>>>>>> Unknown savevm section type 5
>>>>>>>>>>> Error -22 while loading VM state
>>>>>>>>>>
>>>>>>>>>> Please make this configurable -- either via configure or device
>>>>>>>>>> properties. That avoids having to break existing configurations that
>>>>>>>>>> work without this patch.
>>>>>>
>>>>>> Since backwards migration is not supported upstream, wouldn't it be
>>>>>> easiest to just add support for the subsection marker and skipping to
>>>>>> the end of section in that downstream?
>>>>>
>>>>> Backwards and forwards migration need to be supported,
>>>>> customers told us repeatedly. So some downstreams support this
>>>>> and not supporting it upstream just means downstreams need
>>>>> to do their own thing.
>>>>>
>>>>> As importantly, ping-pong migration is the only
>>>>> reliable way to stress migration.
>>>>>
>>>>> So if we want to test cross-version we need it to work
>>>>> both way.
>>>>>
>>>>> Finally, the real issue and difficulty with cross-version migration is
>>>>> making VM behave in a backwards compatible way. Serializing in a
>>>>> compatible way is a trivial problem, or would be if the code wasn't a
>>>>> mess :) Once you do the hard part, breaking migration because of the
>>>>> trivial serialization issue is just silly. And special-casing forward
>>>>> migration does not make code simpler, it really only leads to
>>>>> proliferation of hacks and lack of symmetry.
>>>>>
>>>>> So yes it's a useful feature, and no not supporting it does
>>>>> not help anyway.
>>>>
>>>> It seems you misunderstand. I was not saying it's not useful.
>>>>
>>>> My point is that VMStateSubsections added in newer versions (and thus
>>>> not existing in older versions) need to be handled for any
>>>> VMState-converted devices. So why can't we make that work for virtio too?
>>>
>>> Sure.
>>> AFAIK the way to this works is by adding an "field_exists" callback,
>>> and not sending the section when we are in a compat mode.
>>
>> OK, but upstream always sends the latest version today.
>> So isn't that
>> just two ifs that you would need to add in save and load functions added
>> here for the downstream? x86_64 is unaffected from ppc's endianness
>> changes and you still do ppc64 BE, so there's no real functional problem
>> for RHEL, is there?
>
> I'm sorry I don't understand what you are saying here.
> Simply put, if you specify a compatible -M then your
> device should behave, and migrate, exactly like and old
> qemu did.
Whatever the version_id fields are for, upstream QEMU *always* saves the
newest version_id format that it knows. There is no mechanism that I'm
aware of in upstream QEMU for migrating device fields dependent on -M.
So a device in QEMU only migrates exactly like an old QEMU did if
neither fields nor subsections were added.
Subsections are usually migrated based on whether the state differs from
the default state (didn't check whether the final patch does this
correctly here, but doesn't matter for 1/8 concept). So for x86 the
subsection should *never* get written and thus not be a problem for you.
For ppc64 it should not get written either, unless you care about
migration from SLES12 or upstream ppc64le to RHEL ppc64.
As I understood the IRC discussion Alex and me had with Greg about this,
this is copying the exact code VMState uses to write its subsections, so
there would be no forward migration problems at all once we convert to
VMState and VMStateSubsections. The only question remaining is how does
your downstream react when it encounters a subsection marker for
subsections it doesn't know - which is not something upstream can solve
for you unless you contribute backwards migration support upstream.
Don't forget that Greg is introducing subsection support *because of*
your backwards migration wish, so telling him not to add subsections
because of backwards migration is really weird! Either subsections work
with your downstream or they don't; if they don't, then you have much
larger problems than can be solved by blocking this one section for ppc.
Therefore I was saying if your downstream forgot to handle the case of a
non-VMState device having subsections, then it may be easier to fix
exactly that than make Greg jump through more hoops here for a
theoretical problem you are unlikely to encounter on your downstream.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 12:00 ` Andreas Färber
@ 2014-05-15 12:20 ` Michael S. Tsirkin
2014-05-15 13:47 ` Markus Armbruster
2014-05-15 13:49 ` Greg Kurz
1 sibling, 1 reply; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 12:20 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Greg Kurz
On Thu, May 15, 2014 at 02:00:15PM +0200, Andreas Färber wrote:
> Am 15.05.2014 12:16, schrieb Michael S. Tsirkin:
> > On Thu, May 15, 2014 at 12:11:12PM +0200, Andreas Färber wrote:
> >> Am 15.05.2014 12:03, schrieb Michael S. Tsirkin:
> >>> On Thu, May 15, 2014 at 11:58:25AM +0200, Andreas Färber wrote:
> >>>> Am 15.05.2014 11:52, schrieb Michael S. Tsirkin:
> >>>>> On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
> >>>>>> Am 15.05.2014 09:04, schrieb Greg Kurz:
> >>>>>>> On Thu, 15 May 2014 12:16:35 +0530
> >>>>>>> Amit Shah <amit.shah@redhat.com> wrote:
> >>>>>>>> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> >>>>>>>>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> >>>>>>>>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> >>>>>>>>>>> Since each virtio device is streamed in its own section, the idea is to
> >>>>>>>>>>> stream subsections between the end of the device section and the start
> >>>>>>>>>>> of the next sections. This allows an older QEMU to complain and exit
> >>>>>>>>>>> when fed with subsections:
> >>>>>>>>>>>
> >>>>>>>>>>> Unknown savevm section type 5
> >>>>>>>>>>> Error -22 while loading VM state
> >>>>>>>>>>
> >>>>>>>>>> Please make this configurable -- either via configure or device
> >>>>>>>>>> properties. That avoids having to break existing configurations that
> >>>>>>>>>> work without this patch.
> >>>>>>
> >>>>>> Since backwards migration is not supported upstream, wouldn't it be
> >>>>>> easiest to just add support for the subsection marker and skipping to
> >>>>>> the end of section in that downstream?
> >>>>>
> >>>>> Backwards and forwards migration need to be supported,
> >>>>> customers told us repeatedly. So some downstreams support this
> >>>>> and not supporting it upstream just means downstreams need
> >>>>> to do their own thing.
> >>>>>
> >>>>> As importantly, ping-pong migration is the only
> >>>>> reliable way to stress migration.
> >>>>>
> >>>>> So if we want to test cross-version we need it to work
> >>>>> both way.
> >>>>>
> >>>>> Finally, the real issue and difficulty with cross-version migration is
> >>>>> making VM behave in a backwards compatible way. Serializing in a
> >>>>> compatible way is a trivial problem, or would be if the code wasn't a
> >>>>> mess :) Once you do the hard part, breaking migration because of the
> >>>>> trivial serialization issue is just silly. And special-casing forward
> >>>>> migration does not make code simpler, it really only leads to
> >>>>> proliferation of hacks and lack of symmetry.
> >>>>>
> >>>>> So yes it's a useful feature, and no not supporting it does
> >>>>> not help anyway.
> >>>>
> >>>> It seems you misunderstand. I was not saying it's not useful.
> >>>>
> >>>> My point is that VMStateSubsections added in newer versions (and thus
> >>>> not existing in older versions) need to be handled for any
> >>>> VMState-converted devices. So why can't we make that work for virtio too?
> >>>
> >>> Sure.
> >>> AFAIK the way to this works is by adding an "field_exists" callback,
> >>> and not sending the section when we are in a compat mode.
> >>
> >> OK, but upstream always sends the latest version today.
> >> So isn't that
> >> just two ifs that you would need to add in save and load functions added
> >> here for the downstream? x86_64 is unaffected from ppc's endianness
> >> changes and you still do ppc64 BE, so there's no real functional problem
> >> for RHEL, is there?
> >
> > I'm sorry I don't understand what you are saying here.
> > Simply put, if you specify a compatible -M then your
> > device should behave, and migrate, exactly like and old
> > qemu did.
>
> Whatever the version_id fields are for, upstream QEMU *always* saves the
> newest version_id format that it knows. There is no mechanism that I'm
> aware of in upstream QEMU for migrating device fields dependent on -M.
> So a device in QEMU only migrates exactly like an old QEMU did if
> neither fields nor subsections were added.
Ah you are talking about version ids.
Yes, they can not support cross-version migration.
For this reason these should only be incremented as the last resort,
normally new fields (possibly subsections) should be added,
with field_exists callback checking for compatibility mode.
> Subsections are usually migrated based on whether the state differs from
> the default state (didn't check whether the final patch does this
> correctly here, but doesn't matter for 1/8 concept).
Yes, this would be fine, but it's not what's implemented IIUC.
> So for x86 the
> subsection should *never* get written and thus not be a problem for you.
> For ppc64 it should not get written either, unless you care about
> migration from SLES12 or upstream ppc64le to RHEL ppc64.
> As I understood the IRC discussion Alex and me had with Greg about this,
> this is copying the exact code VMState uses to write its subsections, so
> there would be no forward migration problems at all once we convert to
> VMState and VMStateSubsections. The only question remaining is how does
> your downstream react when it encounters a subsection marker for
> subsections it doesn't know - which is not something upstream can solve
> for you unless you contribute backwards migration support upstream.
>
> Don't forget that Greg is introducing subsection support *because of*
> your backwards migration wish, so telling him not to add subsections
> because of backwards migration is really weird!
I don't really care whether it's a subsection or just plain
qemu_put_byte, but I'm not saying not to use subsection
either.
I am merely saying this should have "exists" callback checking
for compability mode for old machine types.
> Either subsections work
> with your downstream or they don't; if they don't, then you have much
> larger problems than can be solved by blocking this one section for ppc.
> Therefore I was saying if your downstream forgot to handle the case of a
> non-VMState device having subsections, then it may be easier to fix
> exactly that than make Greg jump through more hoops here for a
> theoretical problem you are unlikely to encounter on your downstream.
>
> Andreas
I only mentioned downstream because it's a requirement coming from
users most of whom never interact with upstream.
There shouldn't be any difference and people building clusters
using upstream qemu will have same needs: clusters can't be
completely brought down for version upgrades, so need to be able to run
a mix of versions, and be able to migrate in any direction
while in the process.
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 12:20 ` Michael S. Tsirkin
@ 2014-05-15 13:47 ` Markus Armbruster
0 siblings, 0 replies; 63+ messages in thread
From: Markus Armbruster @ 2014-05-15 13:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Juan Quintela,
Alexander Graf, qemu-devel, Stefan Hajnoczi, Amit Shah,
Paolo Bonzini, Andreas Färber, Greg Kurz
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, May 15, 2014 at 02:00:15PM +0200, Andreas Färber wrote:
[...]
>> Either subsections work
>> with your downstream or they don't; if they don't, then you have much
>> larger problems than can be solved by blocking this one section for ppc.
>> Therefore I was saying if your downstream forgot to handle the case of a
>> non-VMState device having subsections, then it may be easier to fix
>> exactly that than make Greg jump through more hoops here for a
>> theoretical problem you are unlikely to encounter on your downstream.
>>
>> Andreas
>
> I only mentioned downstream because it's a requirement coming from
> users most of whom never interact with upstream.
> There shouldn't be any difference and people building clusters
> using upstream qemu will have same needs: clusters can't be
> completely brought down for version upgrades, so need to be able to run
> a mix of versions, and be able to migrate in any direction
> while in the process.
Okay, I'll have world peace, a cluster, and a pony, please.
Seriously, no matter how much your cluster management software wants to
migrate backwards, it's not going to work reliably, except in very
narrow cases, at least not today.
If you want to migrate freely, stick to an ultra-stable (downstream)
branch of QEMU. When that branch EOLs, scrap your cluster and start
over.
If you want to upgrade QEMU in more interesting ways, keep track of what
your cluster nodes are running, and migrate only forward.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 12:00 ` Andreas Färber
2014-05-15 12:20 ` Michael S. Tsirkin
@ 2014-05-15 13:49 ` Greg Kurz
1 sibling, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-15 13:49 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Michael S. Tsirkin, Alexander Graf, qemu-devel, Anthony Liguori,
Amit Shah, Paolo Bonzini
On Thu, 15 May 2014 14:00:15 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 15.05.2014 12:16, schrieb Michael S. Tsirkin:
> > On Thu, May 15, 2014 at 12:11:12PM +0200, Andreas Färber wrote:
> >> Am 15.05.2014 12:03, schrieb Michael S. Tsirkin:
> >>> On Thu, May 15, 2014 at 11:58:25AM +0200, Andreas Färber wrote:
> >>>> Am 15.05.2014 11:52, schrieb Michael S. Tsirkin:
> >>>>> On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
> >>>>>> Am 15.05.2014 09:04, schrieb Greg Kurz:
> >>>>>>> On Thu, 15 May 2014 12:16:35 +0530
> >>>>>>> Amit Shah <amit.shah@redhat.com> wrote:
> >>>>>>>> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> >>>>>>>>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> >>>>>>>>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> >>>>>>>>>>> Since each virtio device is streamed in its own section, the idea is to
> >>>>>>>>>>> stream subsections between the end of the device section and the start
> >>>>>>>>>>> of the next sections. This allows an older QEMU to complain and exit
> >>>>>>>>>>> when fed with subsections:
> >>>>>>>>>>>
> >>>>>>>>>>> Unknown savevm section type 5
> >>>>>>>>>>> Error -22 while loading VM state
> >>>>>>>>>>
> >>>>>>>>>> Please make this configurable -- either via configure or device
> >>>>>>>>>> properties. That avoids having to break existing configurations that
> >>>>>>>>>> work without this patch.
> >>>>>>
> >>>>>> Since backwards migration is not supported upstream, wouldn't it be
> >>>>>> easiest to just add support for the subsection marker and skipping to
> >>>>>> the end of section in that downstream?
> >>>>>
> >>>>> Backwards and forwards migration need to be supported,
> >>>>> customers told us repeatedly. So some downstreams support this
> >>>>> and not supporting it upstream just means downstreams need
> >>>>> to do their own thing.
> >>>>>
> >>>>> As importantly, ping-pong migration is the only
> >>>>> reliable way to stress migration.
> >>>>>
> >>>>> So if we want to test cross-version we need it to work
> >>>>> both way.
> >>>>>
> >>>>> Finally, the real issue and difficulty with cross-version migration is
> >>>>> making VM behave in a backwards compatible way. Serializing in a
> >>>>> compatible way is a trivial problem, or would be if the code wasn't a
> >>>>> mess :) Once you do the hard part, breaking migration because of the
> >>>>> trivial serialization issue is just silly. And special-casing forward
> >>>>> migration does not make code simpler, it really only leads to
> >>>>> proliferation of hacks and lack of symmetry.
> >>>>>
> >>>>> So yes it's a useful feature, and no not supporting it does
> >>>>> not help anyway.
> >>>>
> >>>> It seems you misunderstand. I was not saying it's not useful.
> >>>>
> >>>> My point is that VMStateSubsections added in newer versions (and thus
> >>>> not existing in older versions) need to be handled for any
> >>>> VMState-converted devices. So why can't we make that work for virtio too?
> >>>
> >>> Sure.
> >>> AFAIK the way to this works is by adding an "field_exists" callback,
> >>> and not sending the section when we are in a compat mode.
> >>
> >> OK, but upstream always sends the latest version today.
> >> So isn't that
> >> just two ifs that you would need to add in save and load functions added
> >> here for the downstream? x86_64 is unaffected from ppc's endianness
> >> changes and you still do ppc64 BE, so there's no real functional problem
> >> for RHEL, is there?
> >
> > I'm sorry I don't understand what you are saying here.
> > Simply put, if you specify a compatible -M then your
> > device should behave, and migrate, exactly like and old
> > qemu did.
>
> Whatever the version_id fields are for, upstream QEMU *always* saves the
> newest version_id format that it knows. There is no mechanism that I'm
> aware of in upstream QEMU for migrating device fields dependent on -M.
> So a device in QEMU only migrates exactly like an old QEMU did if
> neither fields nor subsections were added.
>
> Subsections are usually migrated based on whether the state differs from
> the default state (didn't check whether the final patch does this
> correctly here, but doesn't matter for 1/8 concept). So for x86 the
For this first round, subsections are sent unconditionally. I will change
that.
> subsection should *never* get written and thus not be a problem for you.
> For ppc64 it should not get written either, unless you care about
> migration from SLES12 or upstream ppc64le to RHEL ppc64.
> As I understood the IRC discussion Alex and me had with Greg about this,
> this is copying the exact code VMState uses to write its subsections, so
> there would be no forward migration problems at all once we convert to
> VMState and VMStateSubsections. The only question remaining is how does
> your downstream react when it encounters a subsection marker for
> subsections it doesn't know - which is not something upstream can solve
> for you unless you contribute backwards migration support upstream.
>
> Don't forget that Greg is introducing subsection support *because of*
> your backwards migration wish, so telling him not to add subsections
> because of backwards migration is really weird! Either subsections work
> with your downstream or they don't; if they don't, then you have much
> larger problems than can be solved by blocking this one section for ppc.
> Therefore I was saying if your downstream forgot to handle the case of a
> non-VMState device having subsections, then it may be easier to fix
> exactly that than make Greg jump through more hoops here for a
> theoretical problem you are unlikely to encounter on your downstream.
>
> Andreas
>
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 9:52 ` Michael S. Tsirkin
2014-05-15 9:58 ` Andreas Färber
@ 2014-05-15 12:33 ` Markus Armbruster
2014-05-15 12:58 ` Michael S. Tsirkin
1 sibling, 1 reply; 63+ messages in thread
From: Markus Armbruster @ 2014-05-15 12:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Juan Quintela,
Alexander Graf, qemu-devel, Stefan Hajnoczi, Amit Shah,
Paolo Bonzini, Andreas Färber, Greg Kurz
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
>> Am 15.05.2014 09:04, schrieb Greg Kurz:
>> > On Thu, 15 May 2014 12:16:35 +0530
>> > Amit Shah <amit.shah@redhat.com> wrote:
>> >> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
>> >>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
>> >>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
>> >>>>> Since each virtio device is streamed in its own section, the idea is to
>> >>>>> stream subsections between the end of the device section and the start
>> >>>>> of the next sections. This allows an older QEMU to complain and exit
>> >>>>> when fed with subsections:
>> >>>>>
>> >>>>> Unknown savevm section type 5
>> >>>>> Error -22 while loading VM state
>> >>>>
>> >>>> Please make this configurable -- either via configure or device
>> >>>> properties. That avoids having to break existing configurations that
>> >>>> work without this patch.
>>
>> Since backwards migration is not supported upstream, wouldn't it be
>> easiest to just add support for the subsection marker and skipping to
>> the end of section in that downstream?
>
> Backwards and forwards migration need to be supported,
> customers told us repeatedly.
Can I have world peace and a pony with that?
Given the current state of things, attempting to support backward
migration is trying to run before you can walk. We need to put
migration on a more solid footing first.
The migration format is crap, and needs to be replaced.
Reasoning on migration compatibility is entirely manual.
Systematic testing of migration compatibility is done downstream.
Fortunately, there's progress being made on all of the above. Let's not
sabotage it by biting off yet another mouthful.
> So some downstreams support this
> and not supporting it upstream just means downstreams need
> to do their own thing.
>
> As importantly, ping-pong migration is the only
> reliable way to stress migration.
>
> So if we want to test cross-version we need it to work
> both way.
Non sequitur.
> Finally, the real issue and difficulty with cross-version migration is
> making VM behave in a backwards compatible way. Serializing in a
> compatible way is a trivial problem, or would be if the code wasn't a
> mess :)
However, it is.
> Once you do the hard part, breaking migration because of the
> trivial serialization issue is just silly. And special-casing forward
> migration does not make code simpler, it really only leads to
> proliferation of hacks and lack of symmetry.
Bold claim; citation needed.
> So yes it's a useful feature, and no not supporting it does
> not help anyway.
Nobody denies reliable backward migration would be useful. However,
attempting to do every useful feature at once just because they're all
useful is foolish.
Treating backward migration as strictly secondary concern while we're up
to the ass in other alligators *can* help, by letting us focus on the
said other alligators.
I'm not opposed to coding things in ways that help backward migration.
Speaking of "support", however, is clearly premature and misleading.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 12:33 ` Markus Armbruster
@ 2014-05-15 12:58 ` Michael S. Tsirkin
2014-05-15 13:35 ` Greg Kurz
0 siblings, 1 reply; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 12:58 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Juan Quintela,
Alexander Graf, qemu-devel, Stefan Hajnoczi, Amit Shah,
Paolo Bonzini, Andreas Färber, Greg Kurz
On Thu, May 15, 2014 at 02:33:47PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
> >> Am 15.05.2014 09:04, schrieb Greg Kurz:
> >> > On Thu, 15 May 2014 12:16:35 +0530
> >> > Amit Shah <amit.shah@redhat.com> wrote:
> >> >> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> >> >>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> >> >>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> >> >>>>> Since each virtio device is streamed in its own section, the idea is to
> >> >>>>> stream subsections between the end of the device section and the start
> >> >>>>> of the next sections. This allows an older QEMU to complain and exit
> >> >>>>> when fed with subsections:
> >> >>>>>
> >> >>>>> Unknown savevm section type 5
> >> >>>>> Error -22 while loading VM state
> >> >>>>
> >> >>>> Please make this configurable -- either via configure or device
> >> >>>> properties. That avoids having to break existing configurations that
> >> >>>> work without this patch.
> >>
> >> Since backwards migration is not supported upstream, wouldn't it be
> >> easiest to just add support for the subsection marker and skipping to
> >> the end of section in that downstream?
> >
> > Backwards and forwards migration need to be supported,
> > customers told us repeatedly.
>
> Can I have world peace and a pony with that?
>
> Given the current state of things, attempting to support backward
> migration is trying to run before you can walk. We need to put
> migration on a more solid footing first.
>
> The migration format is crap, and needs to be replaced.
>
> Reasoning on migration compatibility is entirely manual.
>
> Systematic testing of migration compatibility is done downstream.
>
> Fortunately, there's progress being made on all of the above. Let's not
> sabotage it by biting off yet another mouthful.
>
> > So some downstreams support this
> > and not supporting it upstream just means downstreams need
> > to do their own thing.
> >
> > As importantly, ping-pong migration is the only
> > reliable way to stress migration.
> >
> > So if we want to test cross-version we need it to work
> > both way.
>
> Non sequitur.
>
> > Finally, the real issue and difficulty with cross-version migration is
> > making VM behave in a backwards compatible way. Serializing in a
> > compatible way is a trivial problem, or would be if the code wasn't a
> > mess :)
>
> However, it is.
>
> > Once you do the hard part, breaking migration because of the
> > trivial serialization issue is just silly. And special-casing forward
> > migration does not make code simpler, it really only leads to
> > proliferation of hacks and lack of symmetry.
>
> Bold claim; citation needed.
You are asking for examples of ugly assymetry?
It's easy: grep for .load_state_old.
You have here a bunch of functions loading format that qemu
can no longer produce, with any set of flags.
The only way to make them run is to install two qemu versions side by side,
save from old one and load in the new one.
What, would you guess, is the chance that they actually work?
I'm going to send a patch removing all this stuff, it's
effectively dead code, but this is just one, biggest example.
> > So yes it's a useful feature, and no not supporting it does
> > not help anyway.
>
> Nobody denies reliable backward migration would be useful. However,
> attempting to do every useful feature at once just because they're all
> useful is foolish.
>
> Treating backward migration as strictly secondary concern while we're up
> to the ass in other alligators *can* help, by letting us focus on the
> said other alligators.
>
> I'm not opposed to coding things in ways that help backward migration.
> Speaking of "support", however, is clearly premature and misleading.
I agree it's a secondary concern upstream and your comments really apply
to migration generally. We can't claim that it's properly supported by
the upstream QEMU.
I am merely asking that we don't break cross-version migration
intentionally. I and others also try fix it when we notice it's broken.
In particular I'm not asking that submitters test it.
--
MST
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 12:58 ` Michael S. Tsirkin
@ 2014-05-15 13:35 ` Greg Kurz
0 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-15 13:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Juan Quintela,
qemu-devel, Markus Armbruster, Alexander Graf, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini, Andreas Färber
On Thu, 15 May 2014 15:58:16 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, May 15, 2014 at 02:33:47PM +0200, Markus Armbruster wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >
> > > On Thu, May 15, 2014 at 11:20:18AM +0200, Andreas Färber wrote:
> > >> Am 15.05.2014 09:04, schrieb Greg Kurz:
> > >> > On Thu, 15 May 2014 12:16:35 +0530
> > >> > Amit Shah <amit.shah@redhat.com> wrote:
> > >> >> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> > >> >>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> > >> >>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > >> >>>>> Since each virtio device is streamed in its own section, the idea is to
> > >> >>>>> stream subsections between the end of the device section and the start
> > >> >>>>> of the next sections. This allows an older QEMU to complain and exit
> > >> >>>>> when fed with subsections:
> > >> >>>>>
> > >> >>>>> Unknown savevm section type 5
> > >> >>>>> Error -22 while loading VM state
> > >> >>>>
> > >> >>>> Please make this configurable -- either via configure or device
> > >> >>>> properties. That avoids having to break existing configurations that
> > >> >>>> work without this patch.
> > >>
> > >> Since backwards migration is not supported upstream, wouldn't it be
> > >> easiest to just add support for the subsection marker and skipping to
> > >> the end of section in that downstream?
> > >
> > > Backwards and forwards migration need to be supported,
> > > customers told us repeatedly.
> >
> > Can I have world peace and a pony with that?
> >
> > Given the current state of things, attempting to support backward
> > migration is trying to run before you can walk. We need to put
> > migration on a more solid footing first.
> >
> > The migration format is crap, and needs to be replaced.
> >
> > Reasoning on migration compatibility is entirely manual.
> >
> > Systematic testing of migration compatibility is done downstream.
> >
> > Fortunately, there's progress being made on all of the above. Let's not
> > sabotage it by biting off yet another mouthful.
> >
> > > So some downstreams support this
> > > and not supporting it upstream just means downstreams need
> > > to do their own thing.
> > >
> > > As importantly, ping-pong migration is the only
> > > reliable way to stress migration.
> > >
> > > So if we want to test cross-version we need it to work
> > > both way.
> >
> > Non sequitur.
> >
> > > Finally, the real issue and difficulty with cross-version migration is
> > > making VM behave in a backwards compatible way. Serializing in a
> > > compatible way is a trivial problem, or would be if the code wasn't a
> > > mess :)
> >
> > However, it is.
> >
> > > Once you do the hard part, breaking migration because of the
> > > trivial serialization issue is just silly. And special-casing forward
> > > migration does not make code simpler, it really only leads to
> > > proliferation of hacks and lack of symmetry.
> >
> > Bold claim; citation needed.
>
> You are asking for examples of ugly assymetry?
> It's easy: grep for .load_state_old.
> You have here a bunch of functions loading format that qemu
> can no longer produce, with any set of flags.
> The only way to make them run is to install two qemu versions side by side,
> save from old one and load in the new one.
> What, would you guess, is the chance that they actually work?
>
> I'm going to send a patch removing all this stuff, it's
> effectively dead code, but this is just one, biggest example.
>
>
> > > So yes it's a useful feature, and no not supporting it does
> > > not help anyway.
> >
> > Nobody denies reliable backward migration would be useful. However,
> > attempting to do every useful feature at once just because they're all
> > useful is foolish.
> >
> > Treating backward migration as strictly secondary concern while we're up
> > to the ass in other alligators *can* help, by letting us focus on the
> > said other alligators.
> >
> > I'm not opposed to coding things in ways that help backward migration.
> > Speaking of "support", however, is clearly premature and misleading.
>
>
> I agree it's a secondary concern upstream and your comments really apply
> to migration generally. We can't claim that it's properly supported by
> the upstream QEMU.
>
> I am merely asking that we don't break cross-version migration
> intentionally. I and others also try fix it when we notice it's broken.
> In particular I'm not asking that submitters test it.
>
Michael,
I fully understand your concern and I will take some time to study backward
migration. This being said, the task is not that simple and I will need
several rounds to have the job done...
Cheers.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 9:20 ` Andreas Färber
2014-05-15 9:52 ` Michael S. Tsirkin
@ 2014-05-15 10:08 ` Greg Kurz
2014-05-15 10:12 ` Michael S. Tsirkin
` (2 more replies)
1 sibling, 3 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-15 10:08 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Michael S. Tsirkin, Alexander Graf, qemu-devel, Anthony Liguori,
Amit Shah, Paolo Bonzini
On Thu, 15 May 2014 11:20:18 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 15.05.2014 09:04, schrieb Greg Kurz:
> > On Thu, 15 May 2014 12:16:35 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> >> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> >>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> >>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> >>>>> Since each virtio device is streamed in its own section, the idea is to
> >>>>> stream subsections between the end of the device section and the start
> >>>>> of the next sections. This allows an older QEMU to complain and exit
> >>>>> when fed with subsections:
> >>>>>
> >>>>> Unknown savevm section type 5
> >>>>> Error -22 while loading VM state
> >>>>
> >>>> Please make this configurable -- either via configure or device
> >>>> properties. That avoids having to break existing configurations that
> >>>> work without this patch.
>
> Since backwards migration is not supported upstream, wouldn't it be
> easiest to just add support for the subsection marker and skipping to
> the end of section in that downstream?
>
Not sure I understand well... Do you suggest to stream the markers first,
then the device, then the subsections ? And then there would be a way
we can have the subsections restored before the device ?
> >>>>> All users of virtio_load()/virtio_save() need to be patched because the
> >>>>> subsections are streamed AFTER the device itself.
>
> IMO this is calling for inversion of control - i.e. let virtio devices
> call generic load/save functions that then dispatch to device-specific
> code and let us add common stuff in a central place without forgetting
> to add calls in some new device.
>
That makes a lot of sense.
> >>>> Since all have the same fixup, I'm wondering if a new section can be
> >>>> added to the virtio-bus itself, which gets propagated to all devices
> >>>> upon load in the dest.
> >>>
> >>> This calls for a way for devices to inherit properties from the bus,
> >>> which doesn't exist ATM.
> >>> Fine but let's not hold up this patchset because of this.
> >>
> >> No, only suggestion is to add a migration section in the bus, and then
> >> it's easier to do this in the post-migrate functions for each device
> >> -- so only one new section gets introduced instead of all devices
> >> being modified to send a new subsection.
> >>
> >
> > The main problem I see is that virtio sucks: as you see in patch 8, we have
> > to be careful not to call vring or virtqueue stuff before the device knows
> > its endianness or it breaks... I need to study how the virtio-bus gets
> > migrated to ensure the endian section is streamed before the devices.
>
> There is no ordering guarantee. The state needs to be migrated in the
> device or bus where it sits, if post-load processing is required; i.e.,
> if it's in VirtIODevice then something like this series, if it were on
> VirtioBus exclusively (device asking bus for its endianness each time
> and does not do post-load stuff) then endianness could be migrated as a
> new bus section. Not sure if that would help the "broken" state though?
>
IIRW the "broken" state was proposed as a per-device property...
Fam,
Do you have plans about the "broken" property ? Is it still needed ?
> Would touch on Stefan's alias properties for anything but virtio-mmio.
>
OMG... maybe I should hold on then.
> Regards,
> Andreas
>
Thanks !
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 10:08 ` Greg Kurz
@ 2014-05-15 10:12 ` Michael S. Tsirkin
2014-05-15 10:21 ` Greg Kurz
2014-05-15 10:16 ` Greg Kurz
2014-05-16 9:14 ` Fam Zheng
2 siblings, 1 reply; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 10:12 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Andreas Färber
On Thu, May 15, 2014 at 12:08:26PM +0200, Greg Kurz wrote:
> On Thu, 15 May 2014 11:20:18 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> > Am 15.05.2014 09:04, schrieb Greg Kurz:
> > > On Thu, 15 May 2014 12:16:35 +0530
> > > Amit Shah <amit.shah@redhat.com> wrote:
> > >> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> > >>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> > >>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > >>>>> Since each virtio device is streamed in its own section, the idea is to
> > >>>>> stream subsections between the end of the device section and the start
> > >>>>> of the next sections. This allows an older QEMU to complain and exit
> > >>>>> when fed with subsections:
> > >>>>>
> > >>>>> Unknown savevm section type 5
> > >>>>> Error -22 while loading VM state
> > >>>>
> > >>>> Please make this configurable -- either via configure or device
> > >>>> properties. That avoids having to break existing configurations that
> > >>>> work without this patch.
> >
> > Since backwards migration is not supported upstream, wouldn't it be
> > easiest to just add support for the subsection marker and skipping to
> > the end of section in that downstream?
> >
>
> Not sure I understand well... Do you suggest to stream the markers first,
> then the device, then the subsections ? And then there would be a way
> we can have the subsections restored before the device ?
>
> > >>>>> All users of virtio_load()/virtio_save() need to be patched because the
> > >>>>> subsections are streamed AFTER the device itself.
> >
> > IMO this is calling for inversion of control - i.e. let virtio devices
> > call generic load/save functions that then dispatch to device-specific
> > code and let us add common stuff in a central place without forgetting
> > to add calls in some new device.
> >
>
> That makes a lot of sense.
>
> > >>>> Since all have the same fixup, I'm wondering if a new section can be
> > >>>> added to the virtio-bus itself, which gets propagated to all devices
> > >>>> upon load in the dest.
> > >>>
> > >>> This calls for a way for devices to inherit properties from the bus,
> > >>> which doesn't exist ATM.
> > >>> Fine but let's not hold up this patchset because of this.
> > >>
> > >> No, only suggestion is to add a migration section in the bus, and then
> > >> it's easier to do this in the post-migrate functions for each device
> > >> -- so only one new section gets introduced instead of all devices
> > >> being modified to send a new subsection.
> > >>
> > >
> > > The main problem I see is that virtio sucks: as you see in patch 8, we have
> > > to be careful not to call vring or virtqueue stuff before the device knows
> > > its endianness or it breaks... I need to study how the virtio-bus gets
> > > migrated to ensure the endian section is streamed before the devices.
> >
> > There is no ordering guarantee. The state needs to be migrated in the
> > device or bus where it sits, if post-load processing is required; i.e.,
> > if it's in VirtIODevice then something like this series, if it were on
> > VirtioBus exclusively (device asking bus for its endianness each time
> > and does not do post-load stuff) then endianness could be migrated as a
> > new bus section. Not sure if that would help the "broken" state though?
> >
>
> IIRW the "broken" state was proposed as a per-device property...
>
> Fam,
>
> Do you have plans about the "broken" property ? Is it still needed ?
>
> > Would touch on Stefan's alias properties for anything but virtio-mmio.
> >
>
> OMG... maybe I should hold on then.
No need to wait imho.
Can this be made even simpler - call this stuff
from virtio_save/virtio_load?
Why not?
> > Regards,
> > Andreas
> >
>
> Thanks !
>
> --
> Gregory Kurz kurzgreg@fr.ibm.com
> gkurz@linux.vnet.ibm.com
> Software Engineer @ IBM/Meiosys http://www.ibm.com
> Tel +33 (0)562 165 496
>
> "Anarchy is about taking complete responsibility for yourself."
> Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 10:12 ` Michael S. Tsirkin
@ 2014-05-15 10:21 ` Greg Kurz
0 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-15 10:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Andreas Färber
On Thu, 15 May 2014 13:12:12 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, May 15, 2014 at 12:08:26PM +0200, Greg Kurz wrote:
> > On Thu, 15 May 2014 11:20:18 +0200
> > Andreas Färber <afaerber@suse.de> wrote:
> > > Am 15.05.2014 09:04, schrieb Greg Kurz:
> > > > On Thu, 15 May 2014 12:16:35 +0530
> > > > Amit Shah <amit.shah@redhat.com> wrote:
> > > >> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> > > >>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> > > >>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > > >>>>> Since each virtio device is streamed in its own section, the idea is to
> > > >>>>> stream subsections between the end of the device section and the start
> > > >>>>> of the next sections. This allows an older QEMU to complain and exit
> > > >>>>> when fed with subsections:
> > > >>>>>
> > > >>>>> Unknown savevm section type 5
> > > >>>>> Error -22 while loading VM state
> > > >>>>
> > > >>>> Please make this configurable -- either via configure or device
> > > >>>> properties. That avoids having to break existing configurations that
> > > >>>> work without this patch.
> > >
> > > Since backwards migration is not supported upstream, wouldn't it be
> > > easiest to just add support for the subsection marker and skipping to
> > > the end of section in that downstream?
> > >
> >
> > Not sure I understand well... Do you suggest to stream the markers first,
> > then the device, then the subsections ? And then there would be a way
> > we can have the subsections restored before the device ?
> >
> > > >>>>> All users of virtio_load()/virtio_save() need to be patched because the
> > > >>>>> subsections are streamed AFTER the device itself.
> > >
> > > IMO this is calling for inversion of control - i.e. let virtio devices
> > > call generic load/save functions that then dispatch to device-specific
> > > code and let us add common stuff in a central place without forgetting
> > > to add calls in some new device.
> > >
> >
> > That makes a lot of sense.
> >
> > > >>>> Since all have the same fixup, I'm wondering if a new section can be
> > > >>>> added to the virtio-bus itself, which gets propagated to all devices
> > > >>>> upon load in the dest.
> > > >>>
> > > >>> This calls for a way for devices to inherit properties from the bus,
> > > >>> which doesn't exist ATM.
> > > >>> Fine but let's not hold up this patchset because of this.
> > > >>
> > > >> No, only suggestion is to add a migration section in the bus, and then
> > > >> it's easier to do this in the post-migrate functions for each device
> > > >> -- so only one new section gets introduced instead of all devices
> > > >> being modified to send a new subsection.
> > > >>
> > > >
> > > > The main problem I see is that virtio sucks: as you see in patch 8, we have
> > > > to be careful not to call vring or virtqueue stuff before the device knows
> > > > its endianness or it breaks... I need to study how the virtio-bus gets
> > > > migrated to ensure the endian section is streamed before the devices.
> > >
> > > There is no ordering guarantee. The state needs to be migrated in the
> > > device or bus where it sits, if post-load processing is required; i.e.,
> > > if it's in VirtIODevice then something like this series, if it were on
> > > VirtioBus exclusively (device asking bus for its endianness each time
> > > and does not do post-load stuff) then endianness could be migrated as a
> > > new bus section. Not sure if that would help the "broken" state though?
> > >
> >
> > IIRW the "broken" state was proposed as a per-device property...
> >
> > Fam,
> >
> > Do you have plans about the "broken" property ? Is it still needed ?
> >
> > > Would touch on Stefan's alias properties for anything but virtio-mmio.
> > >
> >
> > OMG... maybe I should hold on then.
>
> No need to wait imho.
> Can this be made even simpler - call this stuff
> from virtio_save/virtio_load?
>
Andreas already suggested this inversion of control.
> Why not?
>
No reason indeed. I'll rewrite the code that way ! :)
>
> > > Regards,
> > > Andreas
> > >
> >
> > Thanks !
> >
> > --
> > Gregory Kurz kurzgreg@fr.ibm.com
> > gkurz@linux.vnet.ibm.com
> > Software Engineer @ IBM/Meiosys http://www.ibm.com
> > Tel +33 (0)562 165 496
> >
> > "Anarchy is about taking complete responsibility for yourself."
> > Alan Moore.
>
Thnaks !
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 10:08 ` Greg Kurz
2014-05-15 10:12 ` Michael S. Tsirkin
@ 2014-05-15 10:16 ` Greg Kurz
2014-05-16 9:14 ` Fam Zheng
2 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-15 10:16 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Michael S. Tsirkin,
Juan Quintela, Alexander Graf, qemu-devel, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini, Andreas Färber
On Thu, 15 May 2014 12:08:26 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Thu, 15 May 2014 11:20:18 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> > Am 15.05.2014 09:04, schrieb Greg Kurz:
> > > On Thu, 15 May 2014 12:16:35 +0530
> > > Amit Shah <amit.shah@redhat.com> wrote:
> > >> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> > >>> On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> > >>>> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > >>>>> Since each virtio device is streamed in its own section, the idea is to
> > >>>>> stream subsections between the end of the device section and the start
> > >>>>> of the next sections. This allows an older QEMU to complain and exit
> > >>>>> when fed with subsections:
> > >>>>>
> > >>>>> Unknown savevm section type 5
> > >>>>> Error -22 while loading VM state
> > >>>>
> > >>>> Please make this configurable -- either via configure or device
> > >>>> properties. That avoids having to break existing configurations that
> > >>>> work without this patch.
> >
> > Since backwards migration is not supported upstream, wouldn't it be
> > easiest to just add support for the subsection marker and skipping to
> > the end of section in that downstream?
> >
>
> Not sure I understand well... Do you suggest to stream the markers first,
> then the device, then the subsections ? And then there would be a way
> we can have the subsections restored before the device ?
>
Heh just got the clarification thanks to Michael's mail... Sorry for the
confusion and noise. :P
> > >>>>> All users of virtio_load()/virtio_save() need to be patched because the
> > >>>>> subsections are streamed AFTER the device itself.
> >
> > IMO this is calling for inversion of control - i.e. let virtio devices
> > call generic load/save functions that then dispatch to device-specific
> > code and let us add common stuff in a central place without forgetting
> > to add calls in some new device.
> >
>
> That makes a lot of sense.
>
> > >>>> Since all have the same fixup, I'm wondering if a new section can be
> > >>>> added to the virtio-bus itself, which gets propagated to all devices
> > >>>> upon load in the dest.
> > >>>
> > >>> This calls for a way for devices to inherit properties from the bus,
> > >>> which doesn't exist ATM.
> > >>> Fine but let's not hold up this patchset because of this.
> > >>
> > >> No, only suggestion is to add a migration section in the bus, and then
> > >> it's easier to do this in the post-migrate functions for each device
> > >> -- so only one new section gets introduced instead of all devices
> > >> being modified to send a new subsection.
> > >>
> > >
> > > The main problem I see is that virtio sucks: as you see in patch 8, we have
> > > to be careful not to call vring or virtqueue stuff before the device knows
> > > its endianness or it breaks... I need to study how the virtio-bus gets
> > > migrated to ensure the endian section is streamed before the devices.
> >
> > There is no ordering guarantee. The state needs to be migrated in the
> > device or bus where it sits, if post-load processing is required; i.e.,
> > if it's in VirtIODevice then something like this series, if it were on
> > VirtioBus exclusively (device asking bus for its endianness each time
> > and does not do post-load stuff) then endianness could be migrated as a
> > new bus section. Not sure if that would help the "broken" state though?
> >
>
> IIRW the "broken" state was proposed as a per-device property...
>
> Fam,
>
> Do you have plans about the "broken" property ? Is it still needed ?
>
> > Would touch on Stefan's alias properties for anything but virtio-mmio.
> >
>
> OMG... maybe I should hold on then.
>
> > Regards,
> > Andreas
> >
>
> Thanks !
>
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 10:08 ` Greg Kurz
2014-05-15 10:12 ` Michael S. Tsirkin
2014-05-15 10:16 ` Greg Kurz
@ 2014-05-16 9:14 ` Fam Zheng
2014-05-16 9:22 ` Andreas Färber
2 siblings, 1 reply; 63+ messages in thread
From: Fam Zheng @ 2014-05-16 9:14 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Juan Quintela,
Alexander Graf, qemu-devel, Stefan Hajnoczi, Amit Shah,
Paolo Bonzini, Andreas Färber
On Thu, 05/15 12:08, Greg Kurz wrote:
> > > The main problem I see is that virtio sucks: as you see in patch 8, we have
> > > to be careful not to call vring or virtqueue stuff before the device knows
> > > its endianness or it breaks... I need to study how the virtio-bus gets
> > > migrated to ensure the endian section is streamed before the devices.
> >
> > There is no ordering guarantee. The state needs to be migrated in the
> > device or bus where it sits, if post-load processing is required; i.e.,
> > if it's in VirtIODevice then something like this series, if it were on
> > VirtioBus exclusively (device asking bus for its endianness each time
> > and does not do post-load stuff) then endianness could be migrated as a
> > new bus section. Not sure if that would help the "broken" state though?
> >
>
> IIRW the "broken" state was proposed as a per-device property...
Yes.
>
> Fam,
>
> Do you have plans about the "broken" property ? Is it still needed ?
It is on my todo list. We will need it. I hope the next revision of virtio
specification has a proper device status bit that the "broken" property can
utilize.
Thanks,
Fam
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-16 9:14 ` Fam Zheng
@ 2014-05-16 9:22 ` Andreas Färber
2014-05-16 9:40 ` Fam Zheng
0 siblings, 1 reply; 63+ messages in thread
From: Andreas Färber @ 2014-05-16 9:22 UTC (permalink / raw)
To: Fam Zheng, Greg Kurz
Cc: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Juan Quintela,
qemu-devel, Alexander Graf, Stefan Hajnoczi, Amit Shah,
Paolo Bonzini
Am 16.05.2014 11:14, schrieb Fam Zheng:
> On Thu, 05/15 12:08, Greg Kurz wrote:
>>>> The main problem I see is that virtio sucks: as you see in patch 8, we have
>>>> to be careful not to call vring or virtqueue stuff before the device knows
>>>> its endianness or it breaks... I need to study how the virtio-bus gets
>>>> migrated to ensure the endian section is streamed before the devices.
>>>
>>> There is no ordering guarantee. The state needs to be migrated in the
>>> device or bus where it sits, if post-load processing is required; i.e.,
>>> if it's in VirtIODevice then something like this series, if it were on
>>> VirtioBus exclusively (device asking bus for its endianness each time
>>> and does not do post-load stuff) then endianness could be migrated as a
>>> new bus section. Not sure if that would help the "broken" state though?
>>>
>>
>> IIRW the "broken" state was proposed as a per-device property...
>
> Yes.
Sure, and that makes sense, but we do have a 1:1 relation of bus/device,
or does virtio-mmio support more? If device doesn't work for some
reason, we could (mis)use the bus as fallback then.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-16 9:22 ` Andreas Färber
@ 2014-05-16 9:40 ` Fam Zheng
2014-05-16 9:48 ` Greg Kurz
0 siblings, 1 reply; 63+ messages in thread
From: Fam Zheng @ 2014-05-16 9:40 UTC (permalink / raw)
To: Andreas Färber
Cc: Kevin Wolf, Stefan Hajnoczi, Juan Quintela, Michael S. Tsirkin,
qemu-devel, Alexander Graf, Anthony Liguori, Amit Shah,
Paolo Bonzini, Greg Kurz
On Fri, 05/16 11:22, Andreas Färber wrote:
> Am 16.05.2014 11:14, schrieb Fam Zheng:
> > On Thu, 05/15 12:08, Greg Kurz wrote:
> >>>> The main problem I see is that virtio sucks: as you see in patch 8, we have
> >>>> to be careful not to call vring or virtqueue stuff before the device knows
> >>>> its endianness or it breaks... I need to study how the virtio-bus gets
> >>>> migrated to ensure the endian section is streamed before the devices.
> >>>
> >>> There is no ordering guarantee. The state needs to be migrated in the
> >>> device or bus where it sits, if post-load processing is required; i.e.,
> >>> if it's in VirtIODevice then something like this series, if it were on
> >>> VirtioBus exclusively (device asking bus for its endianness each time
> >>> and does not do post-load stuff) then endianness could be migrated as a
> >>> new bus section. Not sure if that would help the "broken" state though?
> >>>
> >>
> >> IIRW the "broken" state was proposed as a per-device property...
> >
> > Yes.
>
> Sure, and that makes sense, but we do have a 1:1 relation of bus/device,
> or does virtio-mmio support more? If device doesn't work for some
> reason, we could (mis)use the bus as fallback then.
>
FWIW, I just realized that "broken" may be loaded from device status bit in the
future, so we don't need to migrate the field separately:
https://tools.oasis-open.org/issues/browse/VIRTIO-98
Fam
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-16 9:40 ` Fam Zheng
@ 2014-05-16 9:48 ` Greg Kurz
0 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-16 9:48 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Stefan Hajnoczi, Juan Quintela, Michael S. Tsirkin,
qemu-devel, Alexander Graf, Anthony Liguori, Amit Shah,
Paolo Bonzini, Andreas Färber
On Fri, 16 May 2014 17:40:00 +0800
Fam Zheng <famz@redhat.com> wrote:
> On Fri, 05/16 11:22, Andreas Färber wrote:
> > Am 16.05.2014 11:14, schrieb Fam Zheng:
> > > On Thu, 05/15 12:08, Greg Kurz wrote:
> > >>>> The main problem I see is that virtio sucks: as you see in patch 8, we have
> > >>>> to be careful not to call vring or virtqueue stuff before the device knows
> > >>>> its endianness or it breaks... I need to study how the virtio-bus gets
> > >>>> migrated to ensure the endian section is streamed before the devices.
> > >>>
> > >>> There is no ordering guarantee. The state needs to be migrated in the
> > >>> device or bus where it sits, if post-load processing is required; i.e.,
> > >>> if it's in VirtIODevice then something like this series, if it were on
> > >>> VirtioBus exclusively (device asking bus for its endianness each time
> > >>> and does not do post-load stuff) then endianness could be migrated as a
> > >>> new bus section. Not sure if that would help the "broken" state though?
> > >>>
> > >>
> > >> IIRW the "broken" state was proposed as a per-device property...
> > >
> > > Yes.
> >
> > Sure, and that makes sense, but we do have a 1:1 relation of bus/device,
> > or does virtio-mmio support more? If device doesn't work for some
> > reason, we could (mis)use the bus as fallback then.
> >
>
> FWIW, I just realized that "broken" may be loaded from device status bit in the
> future, so we don't need to migrate the field separately:
>
> https://tools.oasis-open.org/issues/browse/VIRTIO-98
>
> Fam
>
Oh, if you are targetting virtio 1.0... all the virtio endian stuff I am
working on is for legacy virtio. Unless you plan to do something for
legacy, I guess our objectives differ.
Cheers.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 7:04 ` Greg Kurz
2014-05-15 9:20 ` Andreas Färber
@ 2014-05-17 18:29 ` Michael S. Tsirkin
1 sibling, 0 replies; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-17 18:29 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Juan Quintela,
Alexander Graf, qemu-devel, Anthony Liguori, Amit Shah,
Paolo Bonzini, Andreas Färber
On Thu, May 15, 2014 at 09:04:49AM +0200, Greg Kurz wrote:
> On Thu, 15 May 2014 12:16:35 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> > On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> > > On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> > > > On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > > > > There is a need to add some more fields to VirtIODevice that should be
> > > > > migrated (broken status, endianness). The problem is that we do not
> > > > > want to break compatibility while adding a new feature... This issue has
> > > > > been addressed in the generic VMState code with the use of optional
> > > > > subsections. As a *temporary* alternative to port the whole virtio
> > > > > migration code to VMState, this patch mimics a similar subsectionning
> > > > > ability for virtio.
> >
> > BTW Greg, do you plan on working on vmstate for virtio?
> >
>
> Yes.
>
> > > > > Since each virtio device is streamed in its own section, the idea is to
> > > > > stream subsections between the end of the device section and the start
> > > > > of the next sections. This allows an older QEMU to complain and exit
> > > > > when fed with subsections:
> > > > >
> > > > > Unknown savevm section type 5
> > > > > Error -22 while loading VM state
> > > >
> > > > Please make this configurable -- either via configure or device
> > > > properties. That avoids having to break existing configurations that
> > > > work without this patch.
> > > >
> > > > > All users of virtio_load()/virtio_save() need to be patched because the
> > > > > subsections are streamed AFTER the device itself.
> > > >
> > > > Since all have the same fixup, I'm wondering if a new section can be
> > > > added to the virtio-bus itself, which gets propagated to all devices
> > > > upon load in the dest.
> > >
> > > This calls for a way for devices to inherit properties from the bus,
> > > which doesn't exist ATM.
> > > Fine but let's not hold up this patchset because of this.
> >
> > No, only suggestion is to add a migration section in the bus, and then
> > it's easier to do this in the post-migrate functions for each device
> > -- so only one new section gets introduced instead of all devices
> > being modified to send a new subsection.
> >
>
> The main problem I see is that virtio sucks: as you see in patch 8, we have
> to be careful not to call vring or virtqueue stuff before the device knows
> its endianness or it breaks...
I see.
I think it's all not a big deal.
People here suggested many ways to deal with it, but IMHO the
only thing that does matter is the functionality.
Functionality-wise I think the only two things that were mentioned were
- decent chance that migration to a wrong machine
version fails gracefully
- migrate in a compatible way with correct legacy machine version
> I need to study how the virtio-bus gets
> migrated to ensure the endian section is streamed before the devices.
>
> > Amit
> >
>
> Thanks.
>
> --
> Gregory Kurz kurzgreg@fr.ibm.com
> gkurz@linux.vnet.ibm.com
> Software Engineer @ IBM/Meiosys http://www.ibm.com
> Tel +33 (0)562 165 496
>
> "Anarchy is about taking complete responsibility for yourself."
> Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 6:46 ` Amit Shah
2014-05-15 7:04 ` Greg Kurz
@ 2014-05-15 7:14 ` Michael S. Tsirkin
1 sibling, 0 replies; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 7:14 UTC (permalink / raw)
To: Amit Shah
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Juan Quintela,
Alexander Graf, qemu-devel, Stefan Hajnoczi, Paolo Bonzini,
Andreas Färber, Greg Kurz
On Thu, May 15, 2014 at 12:16:35PM +0530, Amit Shah wrote:
> On (Thu) 15 May 2014 [09:23:51], Michael S. Tsirkin wrote:
> > On Thu, May 15, 2014 at 11:34:25AM +0530, Amit Shah wrote:
> > > On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > > > There is a need to add some more fields to VirtIODevice that should be
> > > > migrated (broken status, endianness). The problem is that we do not
> > > > want to break compatibility while adding a new feature... This issue has
> > > > been addressed in the generic VMState code with the use of optional
> > > > subsections. As a *temporary* alternative to port the whole virtio
> > > > migration code to VMState, this patch mimics a similar subsectionning
> > > > ability for virtio.
>
> BTW Greg, do you plan on working on vmstate for virtio?
>
> > > > Since each virtio device is streamed in its own section, the idea is to
> > > > stream subsections between the end of the device section and the start
> > > > of the next sections. This allows an older QEMU to complain and exit
> > > > when fed with subsections:
> > > >
> > > > Unknown savevm section type 5
> > > > Error -22 while loading VM state
> > >
> > > Please make this configurable -- either via configure or device
> > > properties. That avoids having to break existing configurations that
> > > work without this patch.
> > >
> > > > All users of virtio_load()/virtio_save() need to be patched because the
> > > > subsections are streamed AFTER the device itself.
> > >
> > > Since all have the same fixup, I'm wondering if a new section can be
> > > added to the virtio-bus itself, which gets propagated to all devices
> > > upon load in the dest.
> >
> > This calls for a way for devices to inherit properties from the bus,
> > which doesn't exist ATM.
> > Fine but let's not hold up this patchset because of this.
>
> No, only suggestion is to add a migration section in the bus, and then
> it's easier to do this in the post-migrate functions for each device
> -- so only one new section gets introduced instead of all devices
> being modified to send a new subsection.
>
> Amit
I don't mind but the gain isn't very big here.
--
MST
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 6:04 ` Amit Shah
2014-05-15 6:23 ` Michael S. Tsirkin
@ 2014-05-15 6:49 ` Greg Kurz
2014-05-15 6:55 ` Amit Shah
2014-05-15 7:12 ` Michael S. Tsirkin
1 sibling, 2 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-15 6:49 UTC (permalink / raw)
To: Amit Shah
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Michael S. Tsirkin,
Juan Quintela, Alexander Graf, qemu-devel, Stefan Hajnoczi,
Paolo Bonzini, Andreas Färber
On Thu, 15 May 2014 11:34:25 +0530
Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > There is a need to add some more fields to VirtIODevice that should be
> > migrated (broken status, endianness). The problem is that we do not
> > want to break compatibility while adding a new feature... This issue has
> > been addressed in the generic VMState code with the use of optional
> > subsections. As a *temporary* alternative to port the whole virtio
> > migration code to VMState, this patch mimics a similar subsectionning
> > ability for virtio.
> >
> > Since each virtio device is streamed in its own section, the idea is to
> > stream subsections between the end of the device section and the start
> > of the next sections. This allows an older QEMU to complain and exit
> > when fed with subsections:
> >
> > Unknown savevm section type 5
> > Error -22 while loading VM state
>
> Please make this configurable -- either via configure or device
> properties. That avoids having to break existing configurations that
> work without this patch.
>
Hmmm... you mean we support migration from a newer QEMU to an older one ?
> > All users of virtio_load()/virtio_save() need to be patched because the
> > subsections are streamed AFTER the device itself.
>
> Since all have the same fixup, I'm wondering if a new section can be
> added to the virtio-bus itself, which gets propagated to all devices
> upon load in the dest.
>
That would be nice if possible. I will have a closer look.
> Amit
>
Thanks.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 6:49 ` Greg Kurz
@ 2014-05-15 6:55 ` Amit Shah
2014-05-15 7:12 ` Michael S. Tsirkin
1 sibling, 0 replies; 63+ messages in thread
From: Amit Shah @ 2014-05-15 6:55 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Michael S. Tsirkin,
Juan Quintela, Alexander Graf, qemu-devel, Stefan Hajnoczi,
Paolo Bonzini, Andreas Färber
On (Thu) 15 May 2014 [08:49:48], Greg Kurz wrote:
> On Thu, 15 May 2014 11:34:25 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
>
> > On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > > There is a need to add some more fields to VirtIODevice that should be
> > > migrated (broken status, endianness). The problem is that we do not
> > > want to break compatibility while adding a new feature... This issue has
> > > been addressed in the generic VMState code with the use of optional
> > > subsections. As a *temporary* alternative to port the whole virtio
> > > migration code to VMState, this patch mimics a similar subsectionning
> > > ability for virtio.
> > >
> > > Since each virtio device is streamed in its own section, the idea is to
> > > stream subsections between the end of the device section and the start
> > > of the next sections. This allows an older QEMU to complain and exit
> > > when fed with subsections:
> > >
> > > Unknown savevm section type 5
> > > Error -22 while loading VM state
> >
> > Please make this configurable -- either via configure or device
> > properties. That avoids having to break existing configurations that
> > work without this patch.
> >
>
> Hmmm... you mean we support migration from a newer QEMU to an older one ?
Well, not really "support", since many things don't work anyway, but
let's make that easier on downstreams that may want to.
> > > All users of virtio_load()/virtio_save() need to be patched because the
> > > subsections are streamed AFTER the device itself.
> >
> > Since all have the same fixup, I'm wondering if a new section can be
> > added to the virtio-bus itself, which gets propagated to all devices
> > upon load in the dest.
> >
>
> That would be nice if possible. I will have a closer look.
Thanks!
Amit
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream
2014-05-15 6:49 ` Greg Kurz
2014-05-15 6:55 ` Amit Shah
@ 2014-05-15 7:12 ` Michael S. Tsirkin
1 sibling, 0 replies; 63+ messages in thread
From: Michael S. Tsirkin @ 2014-05-15 7:12 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, Anthony Liguori, Juan Quintela,
Alexander Graf, qemu-devel, Stefan Hajnoczi, Amit Shah,
Paolo Bonzini, Andreas Färber
On Thu, May 15, 2014 at 08:49:48AM +0200, Greg Kurz wrote:
> On Thu, 15 May 2014 11:34:25 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
>
> > On (Wed) 14 May 2014 [17:41:38], Greg Kurz wrote:
> > > There is a need to add some more fields to VirtIODevice that should be
> > > migrated (broken status, endianness). The problem is that we do not
> > > want to break compatibility while adding a new feature... This issue has
> > > been addressed in the generic VMState code with the use of optional
> > > subsections. As a *temporary* alternative to port the whole virtio
> > > migration code to VMState, this patch mimics a similar subsectionning
> > > ability for virtio.
> > >
> > > Since each virtio device is streamed in its own section, the idea is to
> > > stream subsections between the end of the device section and the start
> > > of the next sections. This allows an older QEMU to complain and exit
> > > when fed with subsections:
> > >
> > > Unknown savevm section type 5
> > > Error -22 while loading VM state
> >
> > Please make this configurable -- either via configure or device
> > properties. That avoids having to break existing configurations that
> > work without this patch.
> >
>
> Hmmm... you mean we support migration from a newer QEMU to an older one ?
In theory yes, that's why we have the -M switch.
This area isn't well tested unfortunately, but there are
downstreams that test and productize it.
> > > All users of virtio_load()/virtio_save() need to be patched because the
> > > subsections are streamed AFTER the device itself.
> >
> > Since all have the same fixup, I'm wondering if a new section can be
> > added to the virtio-bus itself, which gets propagated to all devices
> > upon load in the dest.
> >
>
> That would be nice if possible. I will have a closer look.
>
> > Amit
> >
>
> Thanks.
>
> --
> Gregory Kurz kurzgreg@fr.ibm.com
> gkurz@linux.vnet.ibm.com
> Software Engineer @ IBM/Meiosys http://www.ibm.com
> Tel +33 (0)562 165 496
>
> "Anarchy is about taking complete responsibility for yourself."
> Alan Moore.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [Qemu-devel] [PATCH RFC 2/8] virtio-net: migrate subsections
2014-05-14 15:41 [Qemu-devel] [PATCH RFC 0/8] virtio: migrate new properties Greg Kurz
2014-05-14 15:41 ` [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream Greg Kurz
@ 2014-05-14 15:41 ` Greg Kurz
2014-05-14 15:41 ` [Qemu-devel] [PATCH RFC 3/8] virtio-blk: " Greg Kurz
` (5 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-14 15:41 UTC (permalink / raw)
To: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini
Cc: Juan Quintela, Fam Zheng, Alexander Graf, Andreas Färber,
qemu-devel
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/net/virtio-net.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index fd23c46..4004d02 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1300,6 +1300,8 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
if ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & vdev->guest_features) {
qemu_put_be64(f, n->curr_guest_offloads);
}
+
+ virtio_save_subsections(vdev, f);
}
static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
@@ -1396,6 +1398,11 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
n->curr_guest_offloads = virtio_net_supported_guest_offloads(n);
}
+ ret = virtio_load_subsections(vdev, f);
+ if (ret) {
+ return ret;
+ }
+
if (peer_has_vnet_hdr(n)) {
virtio_net_apply_guest_offloads(n);
}
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [Qemu-devel] [PATCH RFC 3/8] virtio-blk: migrate subsections
2014-05-14 15:41 [Qemu-devel] [PATCH RFC 0/8] virtio: migrate new properties Greg Kurz
2014-05-14 15:41 ` [Qemu-devel] [PATCH RFC 1/8] virtio: add subsections to the migration stream Greg Kurz
2014-05-14 15:41 ` [Qemu-devel] [PATCH RFC 2/8] virtio-net: migrate subsections Greg Kurz
@ 2014-05-14 15:41 ` Greg Kurz
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 4/8] virtio-scsi: " Greg Kurz
` (4 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-14 15:41 UTC (permalink / raw)
To: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini
Cc: Juan Quintela, Fam Zheng, Alexander Graf, Andreas Färber,
qemu-devel
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/block/virtio-blk.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8a568e5..5a0d224 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -600,6 +600,8 @@ static void virtio_blk_save(QEMUFile *f, void *opaque)
req = req->next;
}
qemu_put_sbyte(f, 0);
+
+ virtio_save_subsections(vdev, f);
}
static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
@@ -628,7 +630,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
req->elem.out_num, 0);
}
- return 0;
+ return virtio_load_subsections(vdev, f);
}
static void virtio_blk_resize(void *opaque)
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [Qemu-devel] [PATCH RFC 4/8] virtio-scsi: migrate subsections
2014-05-14 15:41 [Qemu-devel] [PATCH RFC 0/8] virtio: migrate new properties Greg Kurz
` (2 preceding siblings ...)
2014-05-14 15:41 ` [Qemu-devel] [PATCH RFC 3/8] virtio-blk: " Greg Kurz
@ 2014-05-14 15:42 ` Greg Kurz
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 5/8] virtio-serial: " Greg Kurz
` (3 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-14 15:42 UTC (permalink / raw)
To: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini
Cc: Juan Quintela, Fam Zheng, Alexander Graf, Andreas Färber,
qemu-devel
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/scsi/virtio-scsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b0d7517..989e4ed 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -471,6 +471,7 @@ static void virtio_scsi_save(QEMUFile *f, void *opaque)
{
VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
virtio_save(vdev, f);
+ virtio_save_subsections(vdev, f);
}
static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
@@ -482,7 +483,7 @@ static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
if (ret) {
return ret;
}
- return 0;
+ return virtio_load_subsections(vdev, f);
}
static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [Qemu-devel] [PATCH RFC 5/8] virtio-serial: migrate subsections
2014-05-14 15:41 [Qemu-devel] [PATCH RFC 0/8] virtio: migrate new properties Greg Kurz
` (3 preceding siblings ...)
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 4/8] virtio-scsi: " Greg Kurz
@ 2014-05-14 15:42 ` Greg Kurz
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 6/8] virtio-balloon: " Greg Kurz
` (2 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-14 15:42 UTC (permalink / raw)
To: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini
Cc: Juan Quintela, Fam Zheng, Alexander Graf, Andreas Färber,
qemu-devel
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/char/virtio-serial-bus.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..98f32fd 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -522,12 +522,13 @@ static void vser_reset(VirtIODevice *vdev)
static void virtio_serial_save(QEMUFile *f, void *opaque)
{
VirtIOSerial *s = VIRTIO_SERIAL(opaque);
+ VirtIODevice *vdev = VIRTIO_DEVICE(s);
VirtIOSerialPort *port;
uint32_t nr_active_ports;
unsigned int i, max_nr_ports;
/* The virtio device */
- virtio_save(VIRTIO_DEVICE(s), f);
+ virtio_save(vdev, f);
/* The config space */
qemu_put_be16s(f, &s->config.cols);
@@ -573,6 +574,8 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
sizeof(port->elem));
}
}
+
+ virtio_save_subsections(vdev, f);
}
static void virtio_serial_post_load_timer_cb(void *opaque)
@@ -667,6 +670,7 @@ static int fetch_active_ports_list(QEMUFile *f, int version_id,
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
VirtIOSerial *s = VIRTIO_SERIAL(opaque);
+ VirtIODevice *vdev = VIRTIO_DEVICE(s);
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
@@ -676,7 +680,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
}
/* The virtio device */
- ret = virtio_load(VIRTIO_DEVICE(s), f);
+ ret = virtio_load(vdev, f);
if (ret) {
return ret;
}
@@ -716,7 +720,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
return ret;
}
}
- return 0;
+ return virtio_load_subsections(vdev, f);
}
static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [Qemu-devel] [PATCH RFC 6/8] virtio-balloon: migrate subsections
2014-05-14 15:41 [Qemu-devel] [PATCH RFC 0/8] virtio: migrate new properties Greg Kurz
` (4 preceding siblings ...)
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 5/8] virtio-serial: " Greg Kurz
@ 2014-05-14 15:42 ` Greg Kurz
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 7/8] virtio-rng: " Greg Kurz
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice Greg Kurz
7 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-14 15:42 UTC (permalink / raw)
To: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini
Cc: Juan Quintela, Fam Zheng, Alexander Graf, Andreas Färber,
qemu-devel
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/virtio/virtio-balloon.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a470a0b..41f7ae1 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -316,6 +316,8 @@ static void virtio_balloon_save(QEMUFile *f, void *opaque)
qemu_put_be32(f, s->num_pages);
qemu_put_be32(f, s->actual);
+
+ virtio_save_subsections(vdev, f);
}
static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
@@ -334,7 +336,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
s->num_pages = qemu_get_be32(f);
s->actual = qemu_get_be32(f);
- return 0;
+ return virtio_load_subsections(vdev, f);
}
static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [Qemu-devel] [PATCH RFC 7/8] virtio-rng: migrate subsections
2014-05-14 15:41 [Qemu-devel] [PATCH RFC 0/8] virtio: migrate new properties Greg Kurz
` (5 preceding siblings ...)
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 6/8] virtio-balloon: " Greg Kurz
@ 2014-05-14 15:42 ` Greg Kurz
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice Greg Kurz
7 siblings, 0 replies; 63+ messages in thread
From: Greg Kurz @ 2014-05-14 15:42 UTC (permalink / raw)
To: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini
Cc: Juan Quintela, Fam Zheng, Alexander Graf, Andreas Färber,
qemu-devel
While we are here, we also check virtio_load() return value.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/virtio/virtio-rng.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index b6ab361..0371103 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -103,17 +103,28 @@ static void virtio_rng_save(QEMUFile *f, void *opaque)
VirtIODevice *vdev = opaque;
virtio_save(vdev, f);
+ virtio_save_subsections(vdev, f);
}
static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
{
VirtIORNG *vrng = opaque;
VirtIODevice *vdev = VIRTIO_DEVICE(vrng);
+ int ret;
if (version_id != 1) {
return -EINVAL;
}
- virtio_load(vdev, f);
+
+ ret = virtio_load(vdev, f);
+ if (ret) {
+ return ret;
+ }
+
+ ret = virtio_load_subsections(vdev, f);
+ if (ret) {
+ 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
2014-05-14 15:41 [Qemu-devel] [PATCH RFC 0/8] virtio: migrate new properties Greg Kurz
` (6 preceding siblings ...)
2014-05-14 15:42 ` [Qemu-devel] [PATCH RFC 7/8] virtio-rng: " Greg Kurz
@ 2014-05-14 15:42 ` Greg Kurz
[not found] ` <5384A8D2.8050104@redhat.com>
7 siblings, 1 reply; 63+ messages in thread
From: Greg Kurz @ 2014-05-14 15:42 UTC (permalink / raw)
To: Kevin Wolf, Anthony Liguori, Michael S. Tsirkin, Stefan Hajnoczi,
Amit Shah, Paolo Bonzini
Cc: Juan Quintela, Fam Zheng, Alexander Graf, Andreas Färber,
qemu-devel
Some CPU families can dynamically change their endianness. This means we
can have little endian ppc or big endian arm guests for example. This has
an impact on legacy virtio data structures since they are target endian.
We hence introduce a new property to track the endianness of each virtio
device. It is reasonnably assumed that endianness won't change while the
device is in use : we hence capture the device endianness when it gets
reset.
Of course this property must be part of the migration stream as most of
the virtio code will depend on it. It is hence migrated in a subsection
to preserve compatibility with older migration streams. The tricky part
is that the endianness gets known quite late and we must ensure no access
is made to virtio data before that. It is for example invalid to call
vring_avail_idx() as does the actual migration code: the VQ indexes sanity
checks had to be moved from virtio_load() to virtio_load_subsections()
because of that.
In fact, we have two conditions where the endianness property is stale:
- from initialization time (virtio_init) to first reset time (virtio_reset)
- from incoming migration time (virtio_load) to the time the property
is restored (virtio_load_device_endian)
We enforce some paranoia by making the endianness a 3-value enum so
that we can poison it when it should not be used.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
exec.c | 8 +-----
hw/virtio/virtio-pci.c | 11 +++-----
hw/virtio/virtio.c | 64 ++++++++++++++++++++++++++++++++++++--------
include/exec/cpu-common.h | 1 +
include/hw/virtio/virtio.h | 13 +++++++++
5 files changed, 72 insertions(+), 25 deletions(-)
diff --git a/exec.c b/exec.c
index 91513c6..4e83588 100644
--- a/exec.c
+++ b/exec.c
@@ -2740,13 +2740,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
#endif
#if !defined(CONFIG_USER_ONLY)
-
-/*
- * A helper function for the _utterly broken_ virtio device model to find out if
- * it's running on a big endian machine. Don't do this at home kids!
- */
-bool virtio_is_big_endian(void);
-bool virtio_is_big_endian(void)
+bool target_words_bigendian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
return true;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..2ffceb8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests. */
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
break;
case 2:
val = virtio_config_readw(vdev, addr);
- if (virtio_is_big_endian()) {
+ if (virtio_is_big_endian(vdev)) {
val = bswap16(val);
}
break;
case 4:
val = virtio_config_readl(vdev, addr);
- if (virtio_is_big_endian()) {
+ if (virtio_is_big_endian(vdev)) {
val = bswap32(val);
}
break;
@@ -443,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
virtio_config_writeb(vdev, addr, val);
break;
case 2:
- if (virtio_is_big_endian()) {
+ if (virtio_is_big_endian(vdev)) {
val = bswap16(val);
}
virtio_config_writew(vdev, addr, val);
break;
case 4:
- if (virtio_is_big_endian()) {
+ if (virtio_is_big_endian(vdev)) {
val = bswap32(val);
}
virtio_config_writel(vdev, addr, val);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f4eaa3f..9018b6c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -547,6 +547,12 @@ void virtio_reset(void *opaque)
virtio_set_status(vdev, 0);
+ if (target_words_bigendian()) {
+ vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
+ } else {
+ vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
+ }
+
if (k->reset) {
k->reset(vdev);
}
@@ -834,12 +840,28 @@ void virtio_notify_config(VirtIODevice *vdev)
virtio_notify_vector(vdev, vdev->config_vector);
}
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+ qemu_put_byte(f, vdev->device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+ vdev->device_endian = qemu_get_byte(f);
+ return 0;
+}
+
static const struct VirtIOSubsectionDescStruct {
const char *name;
int version_id;
void (*save)(VirtIODevice *vdev, QEMUFile *f);
int (*load)(VirtIODevice *vdev, QEMUFile *f);
} virtio_subsection[] = {
+ { .name = "virtio/is_big_endian",
+ .version_id = 1,
+ .save = virtio_save_device_endian,
+ .load = virtio_load_device_endian,
+ },
{ .name = NULL }
};
@@ -861,6 +883,8 @@ void virtio_save_subsections(VirtIODevice *vdev, QEMUFile *f)
int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
{
+ int i;
+
while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
char idstr[256];
uint8_t len, size;
@@ -895,6 +919,26 @@ int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
}
}
+ for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+ if (vdev->vq[i].vring.num == 0) {
+ break;
+ }
+
+ if (vdev->vq[i].pa) {
+ uint16_t nheads;
+ nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
+ /* Check it isn't doing strange things with descriptor numbers. */
+ if (nheads > vdev->vq[i].vring.num) {
+ error_report("VQ %d size 0x%x Guest index 0x%x "
+ "inconsistent with Host index 0x%x: delta 0x%x",
+ i, vdev->vq[i].vring.num,
+ vring_avail_idx(&vdev->vq[i]),
+ vdev->vq[i].last_avail_idx, nheads);
+ return -1;
+ }
+ }
+ }
+
return 0;
}
@@ -962,6 +1006,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+ /* We poison the endianness to ensure it does not get used before
+ * subsections have been loaded.
+ */
+ vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
+
if (k->load_config) {
ret = k->load_config(qbus->parent, f);
if (ret)
@@ -995,18 +1044,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
vdev->vq[i].notification = true;
if (vdev->vq[i].pa) {
- uint16_t nheads;
virtqueue_init(&vdev->vq[i]);
- nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
- /* Check it isn't doing very strange things with descriptor numbers. */
- if (nheads > vdev->vq[i].vring.num) {
- error_report("VQ %d size 0x%x Guest index 0x%x "
- "inconsistent with Host index 0x%x: delta 0x%x",
- i, vdev->vq[i].vring.num,
- vring_avail_idx(&vdev->vq[i]),
- vdev->vq[i].last_avail_idx, nheads);
- return -1;
- }
} else if (vdev->vq[i].last_avail_idx) {
error_report("VQ %d address 0x0 "
"inconsistent with Host index 0x%x",
@@ -1078,6 +1116,10 @@ void virtio_init(VirtIODevice *vdev, const char *name,
}
vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
vdev);
+ /* We poison the endianness to ensure it does not get used before
+ * the device is reset.
+ */
+ vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
}
hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index a21b65a..eb798c1 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -122,4 +122,5 @@ void qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
#endif
+bool target_words_bigendian(void);
#endif /* !CPU_COMMON_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 82f852f..0012470 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -104,6 +104,12 @@ typedef struct VirtQueueElement
#define VIRTIO_DEVICE(obj) \
OBJECT_CHECK(VirtIODevice, (obj), TYPE_VIRTIO_DEVICE)
+enum virtio_device_endian {
+ VIRTIO_DEVICE_ENDIAN_UNKNOWN,
+ VIRTIO_DEVICE_ENDIAN_LITTLE,
+ VIRTIO_DEVICE_ENDIAN_BIG,
+};
+
struct VirtIODevice
{
DeviceState parent_obj;
@@ -121,6 +127,7 @@ struct VirtIODevice
bool vm_running;
VMChangeStateEntry *vmstate;
char *bus_name;
+ enum virtio_device_endian device_endian;
};
typedef struct VirtioDeviceClass {
@@ -257,4 +264,10 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
bool set_handler);
void virtio_queue_notify_vq(VirtQueue *vq);
void virtio_irq(VirtQueue *vq);
+
+static inline bool virtio_is_big_endian(VirtIODevice *vdev)
+{
+ assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+ return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
#endif
^ permalink raw reply related [flat|nested] 63+ messages in thread