| Message ID | 1313143181-7921-4-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/12/2011 04:59 AM, Paolo Bonzini wrote: > With the current migration format, VMS_STRUCTs with subsections > are ambiguous. The protocol cannot tell whether a 0x5 byte after > the VMS_STRUCT is a subsection or part of the parent data stream. > In the past QEMU assumed it was always a part of a subsection; after > commit eb60260 (savevm: fix corruption in vmstate_subsection_load()., > 2011-02-03) the choice depends on whether the VMS_STRUCT has subsections > defined. > > Unfortunately, this means that if a destination has no subsections > defined for the struct, it will happily read subsection data into > its own fields. And if you are "lucky" enough to stumble on a > zero byte at the right time, it will be interpreted as QEMU_VM_EOF > and migration will be interrupted. > > There is no way out of this except defining an incompatible > migration protocol with a sentinel at the end of embedded structs. > Of course, this is restricted to new machine models. > > Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> Because of the change we made for 0.15, this is no longer strictly needed. It only matters if we add a subsection to a structure, right? Regards, Anthony Liguori > --- > hw/boards.h | 3 +++ > hw/pc_piix.c | 5 +++++ > savevm.c | 24 ++++++++++++++++-------- > 3 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/hw/boards.h b/hw/boards.h > index 560dbaf..f20d58e 100644 > --- a/hw/boards.h > +++ b/hw/boards.h > @@ -5,6 +5,9 @@ > > #include "qdev.h" > > +#define QEMU_VM_FILE_VERSION_0_15 0x00000003 > +#define QEMU_VM_FILE_VERSION 0x00000004 > + > typedef void QEMUMachineInitFunc(ram_addr_t ram_size, > const char *boot_device, > const char *kernel_filename, > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 88a5f3b..8af1f6f 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -284,6 +284,7 @@ static QEMUMachine pc_machine_v0_15 = { > .desc = "Standard PC", > .init = pc_init_pci, > .max_cpus = 255, > + .migration_format = QEMU_VM_FILE_VERSION_0_15, > }; > > static QEMUMachine pc_machine_v0_13 = { > @@ -291,6 +292,7 @@ static QEMUMachine pc_machine_v0_13 = { > .desc = "Standard PC", > .init = pc_init_pci_no_kvmclock, > .max_cpus = 255, > + .migration_format = QEMU_VM_FILE_VERSION_0_15, > .compat_props = (GlobalProperty[]) { > { > .driver = "virtio-9p-pci", > @@ -330,6 +332,7 @@ static QEMUMachine pc_machine_v0_12 = { > .desc = "Standard PC", > .init = pc_init_pci_no_kvmclock, > .max_cpus = 255, > + .migration_format = QEMU_VM_FILE_VERSION_0_15, > .compat_props = (GlobalProperty[]) { > { > .driver = "virtio-serial-pci", > @@ -373,6 +376,7 @@ static QEMUMachine pc_machine_v0_11 = { > .desc = "Standard PC, qemu 0.11", > .init = pc_init_pci_no_kvmclock, > .max_cpus = 255, > + .migration_format = QEMU_VM_FILE_VERSION_0_15, > .compat_props = (GlobalProperty[]) { > { > .driver = "virtio-blk-pci", > @@ -424,6 +428,7 @@ static QEMUMachine pc_machine_v0_10 = { > .desc = "Standard PC, qemu 0.10", > .init = pc_init_pci_no_kvmclock, > .max_cpus = 255, > + .migration_format = QEMU_VM_FILE_VERSION_0_15, > .compat_props = (GlobalProperty[]) { > { > .driver = "virtio-blk-pci", > diff --git a/savevm.c b/savevm.c > index 87f2b71..a362ad7 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -158,6 +158,14 @@ void qemu_announce_self(void) > > #define IO_BUF_SIZE 32768 > > +#define QEMU_VM_EOF 0x00 > +#define QEMU_VM_SECTION_START 0x01 > +#define QEMU_VM_SECTION_PART 0x02 > +#define QEMU_VM_SECTION_END 0x03 > +#define QEMU_VM_SECTION_FULL 0x04 > +#define QEMU_VM_SUBSECTION 0x05 > +#define QEMU_VM_SUBSECTIONS_END 0x06 > + > struct QEMUFile { > QEMUFilePutBufferFunc *put_buffer; > QEMUFileGetBufferFunc *get_buffer; > @@ -1358,6 +1366,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > } > if (field->flags& VMS_STRUCT) { > ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id); > + if (qemu_current_migration_format()>= 4) { > + if (qemu_get_byte(f) != QEMU_VM_SUBSECTIONS_END) { > + return -EINVAL; > + } > + } > } else { > ret = field->info->get(f, addr, size); > > @@ -1429,6 +1442,9 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, > } > if (field->flags& VMS_STRUCT) { > vmstate_save_state(f, field->vmsd, addr); > + if (qemu_current_migration_format()>= 4) { > + qemu_put_byte(f, QEMU_VM_SUBSECTIONS_END); > + } > } else { > field->info->put(f, addr, size); > } > @@ -1458,14 +1474,6 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se) > > #define QEMU_VM_FILE_MAGIC 0x5145564d > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 > -#define QEMU_VM_FILE_VERSION 0x00000003 > - > -#define QEMU_VM_EOF 0x00 > -#define QEMU_VM_SECTION_START 0x01 > -#define QEMU_VM_SECTION_PART 0x02 > -#define QEMU_VM_SECTION_END 0x03 > -#define QEMU_VM_SECTION_FULL 0x04 > -#define QEMU_VM_SUBSECTION 0x05 > > bool qemu_savevm_state_blocked(Monitor *mon) > {
On 08/12/2011 10:11 AM, Anthony Liguori wrote: > On 08/12/2011 04:59 AM, Paolo Bonzini wrote: >> With the current migration format, VMS_STRUCTs with subsections >> are ambiguous. The protocol cannot tell whether a 0x5 byte after >> the VMS_STRUCT is a subsection or part of the parent data stream. >> In the past QEMU assumed it was always a part of a subsection; after >> commit eb60260 (savevm: fix corruption in vmstate_subsection_load()., >> 2011-02-03) the choice depends on whether the VMS_STRUCT has subsections >> defined. >> >> Unfortunately, this means that if a destination has no subsections >> defined for the struct, it will happily read subsection data into >> its own fields. And if you are "lucky" enough to stumble on a >> zero byte at the right time, it will be interpreted as QEMU_VM_EOF >> and migration will be interrupted. >> >> There is no way out of this except defining an incompatible >> migration protocol with a sentinel at the end of embedded structs. >> Of course, this is restricted to new machine models. >> >> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> > > Because of the change we made for 0.15, this is no longer strictly > needed. It only matters if we add a subsection to a structure, right? I'm definitely not saying we shouldn't do this BTW. But trying to understand whether we can position this as a strictly optional feature (iow, a capability) or whether this requires a hard break in the migration protocol. Regards, Anthony Liguori > > Regards, > > Anthony Liguori > >> --- >> hw/boards.h | 3 +++ >> hw/pc_piix.c | 5 +++++ >> savevm.c | 24 ++++++++++++++++-------- >> 3 files changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/hw/boards.h b/hw/boards.h >> index 560dbaf..f20d58e 100644 >> --- a/hw/boards.h >> +++ b/hw/boards.h >> @@ -5,6 +5,9 @@ >> >> #include "qdev.h" >> >> +#define QEMU_VM_FILE_VERSION_0_15 0x00000003 >> +#define QEMU_VM_FILE_VERSION 0x00000004 >> + >> typedef void QEMUMachineInitFunc(ram_addr_t ram_size, >> const char *boot_device, >> const char *kernel_filename, >> diff --git a/hw/pc_piix.c b/hw/pc_piix.c >> index 88a5f3b..8af1f6f 100644 >> --- a/hw/pc_piix.c >> +++ b/hw/pc_piix.c >> @@ -284,6 +284,7 @@ static QEMUMachine pc_machine_v0_15 = { >> .desc = "Standard PC", >> .init = pc_init_pci, >> .max_cpus = 255, >> + .migration_format = QEMU_VM_FILE_VERSION_0_15, >> }; >> >> static QEMUMachine pc_machine_v0_13 = { >> @@ -291,6 +292,7 @@ static QEMUMachine pc_machine_v0_13 = { >> .desc = "Standard PC", >> .init = pc_init_pci_no_kvmclock, >> .max_cpus = 255, >> + .migration_format = QEMU_VM_FILE_VERSION_0_15, >> .compat_props = (GlobalProperty[]) { >> { >> .driver = "virtio-9p-pci", >> @@ -330,6 +332,7 @@ static QEMUMachine pc_machine_v0_12 = { >> .desc = "Standard PC", >> .init = pc_init_pci_no_kvmclock, >> .max_cpus = 255, >> + .migration_format = QEMU_VM_FILE_VERSION_0_15, >> .compat_props = (GlobalProperty[]) { >> { >> .driver = "virtio-serial-pci", >> @@ -373,6 +376,7 @@ static QEMUMachine pc_machine_v0_11 = { >> .desc = "Standard PC, qemu 0.11", >> .init = pc_init_pci_no_kvmclock, >> .max_cpus = 255, >> + .migration_format = QEMU_VM_FILE_VERSION_0_15, >> .compat_props = (GlobalProperty[]) { >> { >> .driver = "virtio-blk-pci", >> @@ -424,6 +428,7 @@ static QEMUMachine pc_machine_v0_10 = { >> .desc = "Standard PC, qemu 0.10", >> .init = pc_init_pci_no_kvmclock, >> .max_cpus = 255, >> + .migration_format = QEMU_VM_FILE_VERSION_0_15, >> .compat_props = (GlobalProperty[]) { >> { >> .driver = "virtio-blk-pci", >> diff --git a/savevm.c b/savevm.c >> index 87f2b71..a362ad7 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -158,6 +158,14 @@ void qemu_announce_self(void) >> >> #define IO_BUF_SIZE 32768 >> >> +#define QEMU_VM_EOF 0x00 >> +#define QEMU_VM_SECTION_START 0x01 >> +#define QEMU_VM_SECTION_PART 0x02 >> +#define QEMU_VM_SECTION_END 0x03 >> +#define QEMU_VM_SECTION_FULL 0x04 >> +#define QEMU_VM_SUBSECTION 0x05 >> +#define QEMU_VM_SUBSECTIONS_END 0x06 >> + >> struct QEMUFile { >> QEMUFilePutBufferFunc *put_buffer; >> QEMUFileGetBufferFunc *get_buffer; >> @@ -1358,6 +1366,11 @@ int vmstate_load_state(QEMUFile *f, const >> VMStateDescription *vmsd, >> } >> if (field->flags& VMS_STRUCT) { >> ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id); >> + if (qemu_current_migration_format()>= 4) { >> + if (qemu_get_byte(f) != QEMU_VM_SUBSECTIONS_END) { >> + return -EINVAL; >> + } >> + } >> } else { >> ret = field->info->get(f, addr, size); >> >> @@ -1429,6 +1442,9 @@ void vmstate_save_state(QEMUFile *f, const >> VMStateDescription *vmsd, >> } >> if (field->flags& VMS_STRUCT) { >> vmstate_save_state(f, field->vmsd, addr); >> + if (qemu_current_migration_format()>= 4) { >> + qemu_put_byte(f, QEMU_VM_SUBSECTIONS_END); >> + } >> } else { >> field->info->put(f, addr, size); >> } >> @@ -1458,14 +1474,6 @@ static void vmstate_save(QEMUFile *f, >> SaveStateEntry *se) >> >> #define QEMU_VM_FILE_MAGIC 0x5145564d >> #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 >> -#define QEMU_VM_FILE_VERSION 0x00000003 >> - >> -#define QEMU_VM_EOF 0x00 >> -#define QEMU_VM_SECTION_START 0x01 >> -#define QEMU_VM_SECTION_PART 0x02 >> -#define QEMU_VM_SECTION_END 0x03 >> -#define QEMU_VM_SECTION_FULL 0x04 >> -#define QEMU_VM_SUBSECTION 0x05 >> >> bool qemu_savevm_state_blocked(Monitor *mon) >> { > >
On 08/12/2011 05:16 PM, Anthony Liguori wrote: >> >> Because of the change we made for 0.15, this is no longer strictly >> needed. It only matters if we add a subsection to a structure, right? Yes. We can also check the current migration format in the "needed" function for such subsections, if necessary. This is what I plan to do for the floppy subsection, skipping it for old machine types that do not use the new migration format. > I'm definitely not saying we shouldn't do this BTW. But trying to > understand whether we can position this as a strictly optional feature > (iow, a capability) or whether this requires a hard break in the > migration protocol. This what? :) Adding a subsection to a structure, or using the new migration format? If you mean using the new format only when you have subsections in a structure, then you'll need quite a bit of new code to try all VMState handlers and see if they'll need a subsection. I don't think it's worth it, also because we all hope this is a temporary solution until we have a serious format (e.g. ASN-1). Paolo
diff --git a/hw/boards.h b/hw/boards.h index 560dbaf..f20d58e 100644 --- a/hw/boards.h +++ b/hw/boards.h @@ -5,6 +5,9 @@ #include "qdev.h" +#define QEMU_VM_FILE_VERSION_0_15 0x00000003 +#define QEMU_VM_FILE_VERSION 0x00000004 + typedef void QEMUMachineInitFunc(ram_addr_t ram_size, const char *boot_device, const char *kernel_filename, diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 88a5f3b..8af1f6f 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -284,6 +284,7 @@ static QEMUMachine pc_machine_v0_15 = { .desc = "Standard PC", .init = pc_init_pci, .max_cpus = 255, + .migration_format = QEMU_VM_FILE_VERSION_0_15, }; static QEMUMachine pc_machine_v0_13 = { @@ -291,6 +292,7 @@ static QEMUMachine pc_machine_v0_13 = { .desc = "Standard PC", .init = pc_init_pci_no_kvmclock, .max_cpus = 255, + .migration_format = QEMU_VM_FILE_VERSION_0_15, .compat_props = (GlobalProperty[]) { { .driver = "virtio-9p-pci", @@ -330,6 +332,7 @@ static QEMUMachine pc_machine_v0_12 = { .desc = "Standard PC", .init = pc_init_pci_no_kvmclock, .max_cpus = 255, + .migration_format = QEMU_VM_FILE_VERSION_0_15, .compat_props = (GlobalProperty[]) { { .driver = "virtio-serial-pci", @@ -373,6 +376,7 @@ static QEMUMachine pc_machine_v0_11 = { .desc = "Standard PC, qemu 0.11", .init = pc_init_pci_no_kvmclock, .max_cpus = 255, + .migration_format = QEMU_VM_FILE_VERSION_0_15, .compat_props = (GlobalProperty[]) { { .driver = "virtio-blk-pci", @@ -424,6 +428,7 @@ static QEMUMachine pc_machine_v0_10 = { .desc = "Standard PC, qemu 0.10", .init = pc_init_pci_no_kvmclock, .max_cpus = 255, + .migration_format = QEMU_VM_FILE_VERSION_0_15, .compat_props = (GlobalProperty[]) { { .driver = "virtio-blk-pci", diff --git a/savevm.c b/savevm.c index 87f2b71..a362ad7 100644 --- a/savevm.c +++ b/savevm.c @@ -158,6 +158,14 @@ void qemu_announce_self(void) #define IO_BUF_SIZE 32768 +#define QEMU_VM_EOF 0x00 +#define QEMU_VM_SECTION_START 0x01 +#define QEMU_VM_SECTION_PART 0x02 +#define QEMU_VM_SECTION_END 0x03 +#define QEMU_VM_SECTION_FULL 0x04 +#define QEMU_VM_SUBSECTION 0x05 +#define QEMU_VM_SUBSECTIONS_END 0x06 + struct QEMUFile { QEMUFilePutBufferFunc *put_buffer; QEMUFileGetBufferFunc *get_buffer; @@ -1358,6 +1366,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, } if (field->flags & VMS_STRUCT) { ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id); + if (qemu_current_migration_format() >= 4) { + if (qemu_get_byte(f) != QEMU_VM_SUBSECTIONS_END) { + return -EINVAL; + } + } } else { ret = field->info->get(f, addr, size); @@ -1429,6 +1442,9 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, } if (field->flags & VMS_STRUCT) { vmstate_save_state(f, field->vmsd, addr); + if (qemu_current_migration_format() >= 4) { + qemu_put_byte(f, QEMU_VM_SUBSECTIONS_END); + } } else { field->info->put(f, addr, size); } @@ -1458,14 +1474,6 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se) #define QEMU_VM_FILE_MAGIC 0x5145564d #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 -#define QEMU_VM_FILE_VERSION 0x00000003 - -#define QEMU_VM_EOF 0x00 -#define QEMU_VM_SECTION_START 0x01 -#define QEMU_VM_SECTION_PART 0x02 -#define QEMU_VM_SECTION_END 0x03 -#define QEMU_VM_SECTION_FULL 0x04 -#define QEMU_VM_SUBSECTION 0x05 bool qemu_savevm_state_blocked(Monitor *mon) {
With the current migration format, VMS_STRUCTs with subsections are ambiguous. The protocol cannot tell whether a 0x5 byte after the VMS_STRUCT is a subsection or part of the parent data stream. In the past QEMU assumed it was always a part of a subsection; after commit eb60260 (savevm: fix corruption in vmstate_subsection_load()., 2011-02-03) the choice depends on whether the VMS_STRUCT has subsections defined. Unfortunately, this means that if a destination has no subsections defined for the struct, it will happily read subsection data into its own fields. And if you are "lucky" enough to stumble on a zero byte at the right time, it will be interpreted as QEMU_VM_EOF and migration will be interrupted. There is no way out of this except defining an incompatible migration protocol with a sentinel at the end of embedded structs. Of course, this is restricted to new machine models. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/boards.h | 3 +++ hw/pc_piix.c | 5 +++++ savevm.c | 24 ++++++++++++++++-------- 3 files changed, 24 insertions(+), 8 deletions(-)