| Message ID | 1469027314-31655-27-git-send-email-ehabkost@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote: > From: Igor Mammedov <imammedo@redhat.com> > > instance_id is generated by last_used_id + 1 for a given device type > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2] > When CPU in the middle is hot-removed and migration started > APICs with instance_ids 0 and 2 are transferred in migration stream. > However target starts with 2 CPUs and APICs' instance_ids are > generated from scratch [0, 1] hence migration fails with error > Unknown savevm section or instance 'apic' 2 > > Fix issue by manually registering APIC's vmsd with apic_id as > instance_id, in this case instance_id on target will always > match instance_id on source as apic_id is the same for a given > cpu instance. > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> After these patches, the static checker complains about missing sections: Section "apic-common" does not exist in dest Section "apic" does not exist in dest Section "kvm-apic" does not exist in dest This will break migration from older versions. Amit
On Tue, 26 Jul 2016 10:41:38 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote: > > From: Igor Mammedov <imammedo@redhat.com> > > > > instance_id is generated by last_used_id + 1 for a given device type > > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2] > > When CPU in the middle is hot-removed and migration started > > APICs with instance_ids 0 and 2 are transferred in migration stream. > > However target starts with 2 CPUs and APICs' instance_ids are > > generated from scratch [0, 1] hence migration fails with error > > Unknown savevm section or instance 'apic' 2 > > > > Fix issue by manually registering APIC's vmsd with apic_id as > > instance_id, in this case instance_id on target will always > > match instance_id on source as apic_id is the same for a given > > cpu instance. > > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > After these patches, the static checker complains about missing > sections: > > Section "apic-common" does not exist in dest > Section "apic" does not exist in dest > Section "kvm-apic" does not exist in dest It works for me, could you post reproducing commands? > > This will break migration from older versions. > > Amit
On Tue, 26 Jul 2016 10:41:38 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote: > > From: Igor Mammedov <imammedo@redhat.com> > > > > instance_id is generated by last_used_id + 1 for a given device type > > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2] > > When CPU in the middle is hot-removed and migration started > > APICs with instance_ids 0 and 2 are transferred in migration stream. > > However target starts with 2 CPUs and APICs' instance_ids are > > generated from scratch [0, 1] hence migration fails with error > > Unknown savevm section or instance 'apic' 2 > > > > Fix issue by manually registering APIC's vmsd with apic_id as > > instance_id, in this case instance_id on target will always > > match instance_id on source as apic_id is the same for a given > > cpu instance. > > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > After these patches, the static checker complains about missing > sections: > > Section "apic-common" does not exist in dest > Section "apic" does not exist in dest > Section "kvm-apic" does not exist in dest > > This will break migration from older versions. Still can't reproduce: here is my CLI on SRC: qemu-system-x86_64-v2.6.0 \ -snapshot -enable-kvm -smp 6,maxcpus=6 -m 256M rhel72.img -monitor stdio -M pc-i440fx-2.6 -nodefaults monitor# stop monitor# migrate "exec:gzip -c > STATEFILE.gz" ^C CLI on DST: qemu-system-x86_64-v2.7.0-rc0 \ -snapshot -enable-kvm -smp 6,maxcpus=6 -m 256M rhel72.img -monitor stdio -M pc-i440fx-2.6 -nodefaults -incoming "exec: gzip -c -d STATEFILE.gz" But I've found issue with I2C, which breaks migration for me with: (qemu) qemu-system-x86_64: Missing section footer for i2c_bus qemu-system-x86_64: load of migration failed: Invalid argument Which is bisects to: commit 2293c27faddf9547dd8b52423caa6e85844eec3a Author: KONRAD Frederic <fred.konrad@greensocs.com> Date: Tue Jun 14 15:59:14 2016 +0100 i2c: implement broadcast write hacking migration hunks of it to old VMState fixes I2C issue, and no apic related issues are noticed. > > Amit >
On (Tue) 26 Jul 2016 [10:00:49], Igor Mammedov wrote: > On Tue, 26 Jul 2016 10:41:38 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote: > > > From: Igor Mammedov <imammedo@redhat.com> > > > > > > instance_id is generated by last_used_id + 1 for a given device type > > > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2] > > > When CPU in the middle is hot-removed and migration started > > > APICs with instance_ids 0 and 2 are transferred in migration stream. > > > However target starts with 2 CPUs and APICs' instance_ids are > > > generated from scratch [0, 1] hence migration fails with error > > > Unknown savevm section or instance 'apic' 2 > > > > > > Fix issue by manually registering APIC's vmsd with apic_id as > > > instance_id, in this case instance_id on target will always > > > match instance_id on source as apic_id is the same for a given > > > cpu instance. > > > > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > After these patches, the static checker complains about missing > > sections: > > > > Section "apic-common" does not exist in dest > > Section "apic" does not exist in dest > > Section "kvm-apic" does not exist in dest > It works for me, could you post reproducing commands? This was flagged by a nightly run of the static checker when this series was pulled. On a 'before' tree, ie one w/o the patches, do this: qemu -dump-vmstate before.json and for after: qemu -dump-vmstate after.json then, python ./scripts/vmstate-static-checker.py -s before.json -d after.json and that shows the output from above. Amit
On (Tue) 26 Jul 2016 [11:41:33], Igor Mammedov wrote: > On Tue, 26 Jul 2016 10:41:38 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote: > > > From: Igor Mammedov <imammedo@redhat.com> > > > > > > instance_id is generated by last_used_id + 1 for a given device type > > > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2] > > > When CPU in the middle is hot-removed and migration started > > > APICs with instance_ids 0 and 2 are transferred in migration stream. > > > However target starts with 2 CPUs and APICs' instance_ids are > > > generated from scratch [0, 1] hence migration fails with error > > > Unknown savevm section or instance 'apic' 2 > > > > > > Fix issue by manually registering APIC's vmsd with apic_id as > > > instance_id, in this case instance_id on target will always > > > match instance_id on source as apic_id is the same for a given > > > cpu instance. > > > > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > After these patches, the static checker complains about missing > > sections: > > > > Section "apic-common" does not exist in dest > > Section "apic" does not exist in dest > > Section "kvm-apic" does not exist in dest > > > > This will break migration from older versions. > Still can't reproduce: > here is my CLI on SRC: > qemu-system-x86_64-v2.6.0 \ > -snapshot -enable-kvm -smp 6,maxcpus=6 -m 256M rhel72.img -monitor stdio -M pc-i440fx-2.6 -nodefaults > > monitor# stop > monitor# migrate "exec:gzip -c > STATEFILE.gz" > ^C > > CLI on DST: > qemu-system-x86_64-v2.7.0-rc0 \ > -snapshot -enable-kvm -smp 6,maxcpus=6 -m 256M rhel72.img -monitor stdio -M pc-i440fx-2.6 -nodefaults -incoming "exec: gzip -c -d STATEFILE.gz" I'll check. > But I've found issue with I2C, which breaks migration for me with: > > (qemu) qemu-system-x86_64: Missing section footer for i2c_bus > qemu-system-x86_64: load of migration failed: Invalid argument > > Which is bisects to: > > commit 2293c27faddf9547dd8b52423caa6e85844eec3a > Author: KONRAD Frederic <fred.konrad@greensocs.com> > Date: Tue Jun 14 15:59:14 2016 +0100 > > i2c: implement broadcast write > > hacking migration hunks of it to old VMState fixes I2C issue, > and no apic related issues are noticed. Yea, the i2c change will also break migration: adding a field ('broadcast') without updating version info. i2c doesn't appear at all in the json output, so the script didn't catch it. I'll check why. Amit
On Tue, 26 Jul 2016 17:17:47 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Tue) 26 Jul 2016 [10:00:49], Igor Mammedov wrote: > > On Tue, 26 Jul 2016 10:41:38 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote: > > > > From: Igor Mammedov <imammedo@redhat.com> > > > > > > > > instance_id is generated by last_used_id + 1 for a given device type > > > > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2] > > > > When CPU in the middle is hot-removed and migration started > > > > APICs with instance_ids 0 and 2 are transferred in migration stream. > > > > However target starts with 2 CPUs and APICs' instance_ids are > > > > generated from scratch [0, 1] hence migration fails with error > > > > Unknown savevm section or instance 'apic' 2 > > > > > > > > Fix issue by manually registering APIC's vmsd with apic_id as > > > > instance_id, in this case instance_id on target will always > > > > match instance_id on source as apic_id is the same for a given > > > > cpu instance. > > > > > > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > > > After these patches, the static checker complains about missing > > > sections: > > > > > > Section "apic-common" does not exist in dest > > > Section "apic" does not exist in dest > > > Section "kvm-apic" does not exist in dest > > It works for me, could you post reproducing commands? > > This was flagged by a nightly run of the static checker when this > series was pulled. On a 'before' tree, ie one w/o the patches, do > this: > > qemu -dump-vmstate before.json > > and for after: > > qemu -dump-vmstate after.json > > then, > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json I don't think it is valid comparison though, as it compares default PC machines. In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference which is expected due to instance_id change. You shouldn't see it when comparing same machine types. > and that shows the output from above. > > > Amit
On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote: > > This was flagged by a nightly run of the static checker when this > > series was pulled. On a 'before' tree, ie one w/o the patches, do > > this: > > > > qemu -dump-vmstate before.json > > > > and for after: > > > > qemu -dump-vmstate after.json > > > > then, > > > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json > I don't think it is valid comparison though, as it compares default PC machines. > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference > which is expected due to instance_id change. > > You shouldn't see it when comparing same machine types. No, this is comparing the git tree just before and after the series is applied. Amit
On Tue, 26 Jul 2016 18:41:22 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote: > > > This was flagged by a nightly run of the static checker when this > > > series was pulled. On a 'before' tree, ie one w/o the patches, do > > > this: > > > > > > qemu -dump-vmstate before.json > > > > > > and for after: > > > > > > qemu -dump-vmstate after.json > > > > > > then, > > > > > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json > > I don't think it is valid comparison though, as it compares default PC machines. > > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference > > which is expected due to instance_id change. > > > > You shouldn't see it when comparing same machine types. > > No, this is comparing the git tree just before and after the series is > applied. I'd say it's expected change introduced by this commit, it should be fine as it doesn't affect other machine types and 2.7 will be released with it. I really don't see an issue here, care to point it out? > > Amit
On Tue, 26 Jul 2016 18:41:22 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote: > > > This was flagged by a nightly run of the static checker when this > > > series was pulled. On a 'before' tree, ie one w/o the patches, do > > > this: > > > > > > qemu -dump-vmstate before.json > > > > > > and for after: > > > > > > qemu -dump-vmstate after.json > > > > > > then, > > > > > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json > > I don't think it is valid comparison though, as it compares default PC machines. > > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference > > which is expected due to instance_id change. > > > > You shouldn't see it when comparing same machine types. > > No, this is comparing the git tree just before and after the series is > applied. I've checked dump_vmstate_json_to_file() implementation and it looks like it dumps only dc->vmsd enabled devices. In this patch vmstate registration has been moved to to apic_comon_realize() that's why dump_vmstate_json_to_file() doesn't dump apics anymore and you see the change > > Amit >
On Tue, Jul 26, 2016 at 04:16:04PM +0200, Igor Mammedov wrote: > On Tue, 26 Jul 2016 18:41:22 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote: > > > > This was flagged by a nightly run of the static checker when this > > > > series was pulled. On a 'before' tree, ie one w/o the patches, do > > > > this: > > > > > > > > qemu -dump-vmstate before.json > > > > > > > > and for after: > > > > > > > > qemu -dump-vmstate after.json > > > > > > > > then, > > > > > > > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json > > > I don't think it is valid comparison though, as it compares default PC machines. > > > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference > > > which is expected due to instance_id change. > > > > > > You shouldn't see it when comparing same machine types. > > > > No, this is comparing the git tree just before and after the series is > > applied. > I've checked dump_vmstate_json_to_file() implementation and it looks like > it dumps only dc->vmsd enabled devices. > > In this patch vmstate registration has been moved to to apic_comon_realize() > that's why dump_vmstate_json_to_file() doesn't dump apics anymore and you see the change So, can anything break because of this difference, or is this just a limitation of -dump-vmstate?
On Tue, 26 Jul 2016 16:19:28 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Jul 26, 2016 at 04:16:04PM +0200, Igor Mammedov wrote: > > On Tue, 26 Jul 2016 18:41:22 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote: > > > > > This was flagged by a nightly run of the static checker when this > > > > > series was pulled. On a 'before' tree, ie one w/o the patches, do > > > > > this: > > > > > > > > > > qemu -dump-vmstate before.json > > > > > > > > > > and for after: > > > > > > > > > > qemu -dump-vmstate after.json > > > > > > > > > > then, > > > > > > > > > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json > > > > I don't think it is valid comparison though, as it compares default PC machines. > > > > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference > > > > which is expected due to instance_id change. > > > > > > > > You shouldn't see it when comparing same machine types. > > > > > > No, this is comparing the git tree just before and after the series is > > > applied. > > I've checked dump_vmstate_json_to_file() implementation and it looks like > > it dumps only dc->vmsd enabled devices. > > > > In this patch vmstate registration has been moved to to apic_comon_realize() > > that's why dump_vmstate_json_to_file() doesn't dump apics anymore and you see the change > > So, can anything break because of this difference, or is this > just a limitation of -dump-vmstate? it's limitation of -dump-vmstate, there shouldn't be anything breaking because of the change.
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c index 0bb9ad5..14ac43c 100644 --- a/hw/intc/apic_common.c +++ b/hw/intc/apic_common.c @@ -294,11 +294,14 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id) return 0; } +static const VMStateDescription vmstate_apic_common; + static void apic_common_realize(DeviceState *dev, Error **errp) { APICCommonState *s = APIC_COMMON(dev); APICCommonClass *info; static DeviceState *vapic; + int instance_id = s->id; info = APIC_COMMON_GET_CLASS(s); info->realize(dev, errp); @@ -313,6 +316,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp) info->enable_tpr_reporting(s, true); } + if (s->legacy_instance_id) { + instance_id = -1; + } + vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common, + s, -1, 0); } static void apic_common_unrealize(DeviceState *dev, Error **errp) @@ -320,6 +328,7 @@ static void apic_common_unrealize(DeviceState *dev, Error **errp) APICCommonState *s = APIC_COMMON(dev); APICCommonClass *info = APIC_COMMON_GET_CLASS(s); + vmstate_unregister(NULL, &vmstate_apic_common, s); info->unrealize(dev, errp); if (apic_report_tpr_access && info->enable_tpr_reporting) { @@ -422,6 +431,8 @@ static Property apic_properties_common[] = { DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14), DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT, true), + DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState, legacy_instance_id, + false), DEFINE_PROP_END_OF_LIST(), }; @@ -429,7 +440,6 @@ static void apic_common_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); - dc->vmsd = &vmstate_apic_common; dc->reset = apic_reset_common; dc->props = apic_properties_common; dc->realize = apic_common_realize; diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h index e549a62..06c4e9f 100644 --- a/include/hw/i386/apic_internal.h +++ b/include/hw/i386/apic_internal.h @@ -183,6 +183,7 @@ struct APICCommonState { uint32_t vapic_control; DeviceState *vapic; hwaddr vapic_paddr; /* note: persistence via kvmvapic */ + bool legacy_instance_id; }; typedef struct VAPICState { diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 47411cb..bc937b9 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -382,6 +382,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); .driver = TYPE_X86_CPU,\ .property = "fill-mtrr-mask",\ .value = "off",\ + },\ + {\ + .driver = "apic",\ + .property = "legacy-instance-id",\ + .value = "on",\ }, #define PC_COMPAT_2_5 \