| Message ID | 20191023173154.30051-8-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > QOM-ify further. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/char/serial.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index c839035fdd..4af8b0ce4c 100644 > --- a/hw/char/serial.c > +++ b/hw/char/serial.c > @@ -990,8 +990,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, > s->baudbase = baudbase; > qemu_chr_fe_init(&s->chr, chr, &error_abort); > serial_realize_core(s, &error_fatal); > - > - vmstate_register(NULL, base, &vmstate_serial, s); > + qdev_set_legacy_instance_id(dev, base, 2); > qdev_init_nofail(dev); Did you test whether migration still works from a QEMU version without this patch to one with it? (The migration vmstate code is too complicated for me to be able to figure out whether passing the 'dev' pointer makes a difference to whot it names the state sections and whether the 'qdev_set_legacy_instance_id' suffices to avoid problems.) thanks -- PMM
On Mon, Nov 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > > > QOM-ify further. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/char/serial.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/hw/char/serial.c b/hw/char/serial.c > > index c839035fdd..4af8b0ce4c 100644 > > --- a/hw/char/serial.c > > +++ b/hw/char/serial.c > > @@ -990,8 +990,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, > > s->baudbase = baudbase; > > qemu_chr_fe_init(&s->chr, chr, &error_abort); > > serial_realize_core(s, &error_fatal); > > - > > - vmstate_register(NULL, base, &vmstate_serial, s); > > + qdev_set_legacy_instance_id(dev, base, 2); > > qdev_init_nofail(dev); > > Did you test whether migration still works from a QEMU > version without this patch to one with it? (The migration Yes, I thought I did test correctly, but I realized testing with x86 isn't correct. So with arm/musicpal for ex, I can migrate from before->after, however after->before won't work. Is that ok? > vmstate code is too complicated for me to be able to figure > out whether passing the 'dev' pointer makes a difference > to whot it names the state sections and whether the > 'qdev_set_legacy_instance_id' suffices to avoid problems.) I don't see a way to fix after->before, because the instance id is initially 0 with the new code, and the old code expect a different value.
On Tue, 19 Nov 2019 at 10:23, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > On Mon, Nov 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > Did you test whether migration still works from a QEMU > > version without this patch to one with it? (The migration > > Yes, I thought I did test correctly, but I realized testing with x86 > isn't correct. > > So with arm/musicpal for ex, I can migrate from before->after, however > after->before won't work. Is that ok? Broadly speaking, the only case where we care about not breaking cross-version migration is where we have a versioned machine type. So musicpal doesn't matter too much. Beyond that, yes, generally before->after is more important than after->before. I have a feeling Red Hat downstream cares about after->before migration at least for x86 but you or your colleagues would know that better than me :-) > > vmstate code is too complicated for me to be able to figure > > out whether passing the 'dev' pointer makes a difference > > to whot it names the state sections and whether the > > 'qdev_set_legacy_instance_id' suffices to avoid problems.) > > I don't see a way to fix after->before, because the instance id is > initially 0 with the new code, and the old code expect a different > value. Can you explain how the instance ID stuff works? I was expecting that the result of setting the legacy instance ID would just be that the new version would always have the older setting, so if it works for old->new it would also work for new->old. But as I say I don't understand this bit of the migration code. thanks -- PMM
On Tue, Nov 19, 2019 at 2:35 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 19 Nov 2019 at 10:23, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > On Mon, Nov 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > Did you test whether migration still works from a QEMU > > > version without this patch to one with it? (The migration > > > > Yes, I thought I did test correctly, but I realized testing with x86 > > isn't correct. > > > > So with arm/musicpal for ex, I can migrate from before->after, however > > after->before won't work. Is that ok? > > Broadly speaking, the only case where we care about not > breaking cross-version migration is where we have a versioned > machine type. So musicpal doesn't matter too much. Beyond > that, yes, generally before->after is more important than > after->before. I have a feeling Red Hat downstream cares about > after->before migration at least for x86 but you or your colleagues > would know that better than me :-) > > > > vmstate code is too complicated for me to be able to figure > > > out whether passing the 'dev' pointer makes a difference > > > to whot it names the state sections and whether the > > > 'qdev_set_legacy_instance_id' suffices to avoid problems.) > > > > I don't see a way to fix after->before, because the instance id is > > initially 0 with the new code, and the old code expect a different > > value. > > Can you explain how the instance ID stuff works? I was > expecting that the result of setting the legacy instance ID > would just be that the new version would always have > the older setting, so if it works for old->new it would also > work for new->old. But as I say I don't understand this bit > of the migration code. From what I understand, the alias_id is only used in savevm.c:find_se(), and thus can only be used to match against "legacy" instance id values. On new code, instance_id is generated incrementally from 0 with calculate_new_instance_id(), based on "qdev-path/vmsd-name".
* Marc-André Lureau (marcandre.lureau@gmail.com) wrote: > On Tue, Nov 19, 2019 at 2:35 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Tue, 19 Nov 2019 at 10:23, Marc-André Lureau > > <marcandre.lureau@redhat.com> wrote: > > > On Mon, Nov 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > Did you test whether migration still works from a QEMU > > > > version without this patch to one with it? (The migration > > > > > > Yes, I thought I did test correctly, but I realized testing with x86 > > > isn't correct. > > > > > > So with arm/musicpal for ex, I can migrate from before->after, however > > > after->before won't work. Is that ok? > > > > Broadly speaking, the only case where we care about not > > breaking cross-version migration is where we have a versioned > > machine type. So musicpal doesn't matter too much. Beyond > > that, yes, generally before->after is more important than > > after->before. I have a feeling Red Hat downstream cares about > > after->before migration at least for x86 but you or your colleagues > > would know that better than me :-) > > > > > > vmstate code is too complicated for me to be able to figure > > > > out whether passing the 'dev' pointer makes a difference > > > > to whot it names the state sections and whether the > > > > 'qdev_set_legacy_instance_id' suffices to avoid problems.) > > > > > > I don't see a way to fix after->before, because the instance id is > > > initially 0 with the new code, and the old code expect a different > > > value. > > > > Can you explain how the instance ID stuff works? I was > > expecting that the result of setting the legacy instance ID > > would just be that the new version would always have > > the older setting, so if it works for old->new it would also > > work for new->old. But as I say I don't understand this bit > > of the migration code. > > From what I understand, the alias_id is only used in > savevm.c:find_se(), and thus can only be used to match against > "legacy" instance id values. On new code, instance_id is generated > incrementally from 0 with calculate_new_instance_id(), based on > "qdev-path/vmsd-name". I think there are cases here there's no qdev path that's viable; e.g. for ISA devices, the ID is set to the ISA IO base: hw/char/serial-isa.c 79: qdev_set_legacy_instance_id(dev, isa->iobase, 3); (In serial_isa_realizefn ) but to be honest I'd have to trace this out and see what values the devices are actually using to be sure. (And yes, please don't break backwards migration; otherwise I'll end up having to figure out a fix). Dave > > > -- > Marc-André Lureau > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Hi On Wed, Nov 20, 2019 at 10:54 PM Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote: > > On Tue, Nov 19, 2019 at 2:35 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Tue, 19 Nov 2019 at 10:23, Marc-André Lureau > > > <marcandre.lureau@redhat.com> wrote: > > > > On Mon, Nov 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > Did you test whether migration still works from a QEMU > > > > > version without this patch to one with it? (The migration > > > > > > > > Yes, I thought I did test correctly, but I realized testing with x86 > > > > isn't correct. > > > > > > > > So with arm/musicpal for ex, I can migrate from before->after, however > > > > after->before won't work. Is that ok? > > > > > > Broadly speaking, the only case where we care about not > > > breaking cross-version migration is where we have a versioned > > > machine type. So musicpal doesn't matter too much. Beyond > > > that, yes, generally before->after is more important than > > > after->before. I have a feeling Red Hat downstream cares about > > > after->before migration at least for x86 but you or your colleagues > > > would know that better than me :-) > > > > > > > > vmstate code is too complicated for me to be able to figure > > > > > out whether passing the 'dev' pointer makes a difference > > > > > to whot it names the state sections and whether the > > > > > 'qdev_set_legacy_instance_id' suffices to avoid problems.) > > > > > > > > I don't see a way to fix after->before, because the instance id is > > > > initially 0 with the new code, and the old code expect a different > > > > value. > > > > > > Can you explain how the instance ID stuff works? I was > > > expecting that the result of setting the legacy instance ID > > > would just be that the new version would always have > > > the older setting, so if it works for old->new it would also > > > work for new->old. But as I say I don't understand this bit > > > of the migration code. > > > > From what I understand, the alias_id is only used in > > savevm.c:find_se(), and thus can only be used to match against > > "legacy" instance id values. On new code, instance_id is generated > > incrementally from 0 with calculate_new_instance_id(), based on > > "qdev-path/vmsd-name". > > I think there are cases here there's no qdev path that's viable; > e.g. for ISA devices, the ID is set to the ISA IO base: > > hw/char/serial-isa.c > 79: qdev_set_legacy_instance_id(dev, isa->iobase, 3); > > (In serial_isa_realizefn ) > > but to be honest I'd have to trace this out and see what values the > devices are actually using to be sure. There is no qdev path, because ISA bus doesn't have get_dev_path() implemented for some reason. However, vmstate_register_with_alias_id() will use calculate_new_instance_id(se->idstr) in this case. > > (And yes, please don't break backwards migration; otherwise I'll > end up having to figure out a fix). My understanding is that qdev_set_legacy_instance_id() always broke backward migration. To keep backward migration to work, we would need a mechanism to "force" to use legacy instance id. Would it be acceptable to have a patch that does that when the original VM state uses legacy instance id? If you start the VM with the new code path, and try to migrate to the old / legacy it would fail. But migrating existing old VM back and forth between old/new would work.
diff --git a/hw/char/serial.c b/hw/char/serial.c index c839035fdd..4af8b0ce4c 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -990,8 +990,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, s->baudbase = baudbase; qemu_chr_fe_init(&s->chr, chr, &error_abort); serial_realize_core(s, &error_fatal); - - vmstate_register(NULL, base, &vmstate_serial, s); + qdev_set_legacy_instance_id(dev, base, 2); qdev_init_nofail(dev); memory_region_init_io(&s->io, NULL, &serial_io_ops, s, "serial", 8); @@ -1000,9 +999,17 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, return s; } +static void serial_class_init(ObjectClass *klass, void* data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->vmsd = &vmstate_serial; +} + static const TypeInfo serial_info = { .name = TYPE_SERIAL, .parent = TYPE_DEVICE, + .class_init = serial_class_init, .instance_size = sizeof(SerialState), }; @@ -1060,7 +1067,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space, qemu_chr_fe_init(&s->chr, chr, &error_abort); serial_realize_core(s, &error_fatal); - vmstate_register(NULL, base, &vmstate_serial, s); + qdev_set_legacy_instance_id(dev, base, 2); qdev_init_nofail(dev); memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s,
QOM-ify further. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/char/serial.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)