Message ID | 20200330164712.198282-1-dgilbert@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | serial: Fix double migration data | expand |
Hi On Mon, Mar 30, 2020 at 6:47 PM Dr. David Alan Gilbert (git) <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > After c9808d60281 we have both an object representing the serial-isa > device and a separate object representing the underlying common serial > uart. Both of these have vmsd's associated with them and thus the > migration stream ends up with two copies of the migration data - the > serial-isa includes the vmstate of the core serial. Besides > being wrong, it breaks backwards migration compatibility. > > Fix this by removing the dc->vmsd from the core device, so it only > gets migrated by any parent devices including it. > Add a vmstate_serial_mm so that any device that uses serial_mm_init > rather than creating a device still gets migrated. > (That doesn't fix backwards migration for serial_mm_init users, > but does seem to work forwards for ppce500). > > Fixes: c9808d60281 ('serial: realize the serial device') > Buglink: https://bugs.launchpad.net/qemu/+bug/1869426 > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/char/serial.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index 2ab8b69e03..c822a9ae6c 100644 > --- a/hw/char/serial.c > +++ b/hw/char/serial.c > @@ -1043,7 +1043,6 @@ static void serial_class_init(ObjectClass *klass, void* data) > dc->user_creatable = false; > dc->realize = serial_realize; > dc->unrealize = serial_unrealize; > - dc->vmsd = &vmstate_serial; > device_class_set_props(dc, serial_properties); > } > > @@ -1113,6 +1112,16 @@ static void serial_mm_realize(DeviceState *dev, Error **errp) > sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq); > } > > +static const VMStateDescription vmstate_serial_mm = { > + .name = "serial", > + .version_id = 3, > + .minimum_version_id = 2, > + .fields = (VMStateField[]) { > + VMSTATE_STRUCT(serial, SerialMM, 0, vmstate_serial, SerialState), > + VMSTATE_END_OF_LIST() > + } > +}; > + Why do you make it a sub-state? # qemu-system-ppc -M ppce500 -monitor stdio -serial pty in 4.2 and 5.0: "serial (8)": { "divider": "0x00d9", "rbr": "0x00", "ier": "0x00", "iir": "0xc1", "lcr": "0x03", "mcr": "0x03", "lsr": "0x60", "msr": "0xb0", "scr": "0x00", "fcr_vmstate": "0x01" }, With this patch: "serial (8)": { "serial": { "divider": "0x00d9", "rbr": "0x00", "ier": "0x00", "iir": "0xc1", "lcr": "0x03", "mcr": "0x03", "lsr": "0x60", "msr": "0xb0", "scr": "0x00", "fcr_vmstate": "0x01" } }, > SerialMM *serial_mm_init(MemoryRegion *address_space, > hwaddr base, int regshift, > qemu_irq irq, int baudbase, > @@ -1162,6 +1171,7 @@ static void serial_mm_class_init(ObjectClass *oc, void *data) > > device_class_set_props(dc, serial_mm_properties); > dc->realize = serial_mm_realize; > + dc->vmsd = &vmstate_serial_mm; > } > > static const TypeInfo serial_mm_info = { > -- > 2.25.1 > > I understand removing the serial state from SerialClass solves the double state issue for ISA. But at the same time, I think we should aim to migrate ISASerial state to SerialClass state. I can take a look if you want.
* Marc-André Lureau (marcandre.lureau@gmail.com) wrote: > Hi > > On Mon, Mar 30, 2020 at 6:47 PM Dr. David Alan Gilbert (git) > <dgilbert@redhat.com> wrote: > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > After c9808d60281 we have both an object representing the serial-isa > > device and a separate object representing the underlying common serial > > uart. Both of these have vmsd's associated with them and thus the > > migration stream ends up with two copies of the migration data - the > > serial-isa includes the vmstate of the core serial. Besides > > being wrong, it breaks backwards migration compatibility. > > > > Fix this by removing the dc->vmsd from the core device, so it only > > gets migrated by any parent devices including it. > > Add a vmstate_serial_mm so that any device that uses serial_mm_init > > rather than creating a device still gets migrated. > > (That doesn't fix backwards migration for serial_mm_init users, > > but does seem to work forwards for ppce500). > > > > Fixes: c9808d60281 ('serial: realize the serial device') > > Buglink: https://bugs.launchpad.net/qemu/+bug/1869426 > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > hw/char/serial.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/hw/char/serial.c b/hw/char/serial.c > > index 2ab8b69e03..c822a9ae6c 100644 > > --- a/hw/char/serial.c > > +++ b/hw/char/serial.c > > @@ -1043,7 +1043,6 @@ static void serial_class_init(ObjectClass *klass, void* data) > > dc->user_creatable = false; > > dc->realize = serial_realize; > > dc->unrealize = serial_unrealize; > > - dc->vmsd = &vmstate_serial; > > device_class_set_props(dc, serial_properties); > > } > > > > @@ -1113,6 +1112,16 @@ static void serial_mm_realize(DeviceState *dev, Error **errp) > > sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq); > > } > > > > +static const VMStateDescription vmstate_serial_mm = { > > + .name = "serial", > > + .version_id = 3, > > + .minimum_version_id = 2, > > + .fields = (VMStateField[]) { > > + VMSTATE_STRUCT(serial, SerialMM, 0, vmstate_serial, SerialState), > > + VMSTATE_END_OF_LIST() > > + } > > +}; > > + > > Why do you make it a sub-state? Because it's consistent with serial-isa and it's simple. > # qemu-system-ppc -M ppce500 -monitor stdio -serial pty > in 4.2 and 5.0: > "serial (8)": { > "divider": "0x00d9", > "rbr": "0x00", > "ier": "0x00", > "iir": "0xc1", > "lcr": "0x03", > "mcr": "0x03", > "lsr": "0x60", > "msr": "0xb0", > "scr": "0x00", > "fcr_vmstate": "0x01" > }, > > With this patch: > "serial (8)": { > "serial": { > "divider": "0x00d9", > "rbr": "0x00", > "ier": "0x00", > "iir": "0xc1", > "lcr": "0x03", > "mcr": "0x03", > "lsr": "0x60", > "msr": "0xb0", > "scr": "0x00", > "fcr_vmstate": "0x01" > } > }, > > > SerialMM *serial_mm_init(MemoryRegion *address_space, > > hwaddr base, int regshift, > > qemu_irq irq, int baudbase, > > @@ -1162,6 +1171,7 @@ static void serial_mm_class_init(ObjectClass *oc, void *data) > > > > device_class_set_props(dc, serial_mm_properties); > > dc->realize = serial_mm_realize; > > + dc->vmsd = &vmstate_serial_mm; > > } > > > > static const TypeInfo serial_mm_info = { > > -- > > 2.25.1 > > > > > > I understand removing the serial state from SerialClass solves the > double state issue for ISA. But at the same time, I think we should > aim to migrate ISASerial state to SerialClass state. I can take a look > if you want. I don't think there's anything wrong with having a separate layer here; the physical reality of what we have is a UART (Serial) that is on the ISA bus where the ISA bus interfacing doesn't require any extra state to be migrated. Dave > > > -- > Marc-André Lureau > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Hi On Mon, Mar 30, 2020 at 7:41 PM Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote: > > Hi > > > > On Mon, Mar 30, 2020 at 6:47 PM Dr. David Alan Gilbert (git) > > <dgilbert@redhat.com> wrote: > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > After c9808d60281 we have both an object representing the serial-isa > > > device and a separate object representing the underlying common serial > > > uart. Both of these have vmsd's associated with them and thus the > > > migration stream ends up with two copies of the migration data - the > > > serial-isa includes the vmstate of the core serial. Besides > > > being wrong, it breaks backwards migration compatibility. > > > > > > Fix this by removing the dc->vmsd from the core device, so it only > > > gets migrated by any parent devices including it. > > > Add a vmstate_serial_mm so that any device that uses serial_mm_init > > > rather than creating a device still gets migrated. > > > (That doesn't fix backwards migration for serial_mm_init users, > > > but does seem to work forwards for ppce500). > > > > > > Fixes: c9808d60281 ('serial: realize the serial device') > > > Buglink: https://bugs.launchpad.net/qemu/+bug/1869426 > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > > hw/char/serial.c | 12 +++++++++++- > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/char/serial.c b/hw/char/serial.c > > > index 2ab8b69e03..c822a9ae6c 100644 > > > --- a/hw/char/serial.c > > > +++ b/hw/char/serial.c > > > @@ -1043,7 +1043,6 @@ static void serial_class_init(ObjectClass *klass, void* data) > > > dc->user_creatable = false; > > > dc->realize = serial_realize; > > > dc->unrealize = serial_unrealize; > > > - dc->vmsd = &vmstate_serial; > > > device_class_set_props(dc, serial_properties); > > > } > > > > > > @@ -1113,6 +1112,16 @@ static void serial_mm_realize(DeviceState *dev, Error **errp) > > > sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq); > > > } > > > > > > +static const VMStateDescription vmstate_serial_mm = { > > > + .name = "serial", > > > + .version_id = 3, > > > + .minimum_version_id = 2, > > > + .fields = (VMStateField[]) { > > > + VMSTATE_STRUCT(serial, SerialMM, 0, vmstate_serial, SerialState), > > > + VMSTATE_END_OF_LIST() > > > + } > > > +}; > > > + > > > > Why do you make it a sub-state? > > Because it's consistent with serial-isa and it's simple. ok, lgtm then Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > # qemu-system-ppc -M ppce500 -monitor stdio -serial pty > > in 4.2 and 5.0: > > "serial (8)": { > > "divider": "0x00d9", > > "rbr": "0x00", > > "ier": "0x00", > > "iir": "0xc1", > > "lcr": "0x03", > > "mcr": "0x03", > > "lsr": "0x60", > > "msr": "0xb0", > > "scr": "0x00", > > "fcr_vmstate": "0x01" > > }, > > > > With this patch: > > "serial (8)": { > > "serial": { > > "divider": "0x00d9", > > "rbr": "0x00", > > "ier": "0x00", > > "iir": "0xc1", > > "lcr": "0x03", > > "mcr": "0x03", > > "lsr": "0x60", > > "msr": "0xb0", > > "scr": "0x00", > > "fcr_vmstate": "0x01" > > } > > }, > > > > > SerialMM *serial_mm_init(MemoryRegion *address_space, > > > hwaddr base, int regshift, > > > qemu_irq irq, int baudbase, > > > @@ -1162,6 +1171,7 @@ static void serial_mm_class_init(ObjectClass *oc, void *data) > > > > > > device_class_set_props(dc, serial_mm_properties); > > > dc->realize = serial_mm_realize; > > > + dc->vmsd = &vmstate_serial_mm; > > > } > > > > > > static const TypeInfo serial_mm_info = { > > > -- > > > 2.25.1 > > > > > > > > > > I understand removing the serial state from SerialClass solves the > > double state issue for ISA. But at the same time, I think we should > > aim to migrate ISASerial state to SerialClass state. I can take a look > > if you want. > > I don't think there's anything wrong with having a separate layer here; > the physical reality of what we have is a UART (Serial) that is on the > ISA bus where the ISA bus interfacing doesn't require any extra state > to be migrated. > > Dave > > > > > > > -- > > Marc-André Lureau > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > thanks!
On 30/03/20 19:56, Marc-André Lureau wrote: >>> Why do you make it a sub-state? >> Because it's consistent with serial-isa and it's simple. > ok, lgtm then > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Queued, thanks. Paolo
diff --git a/hw/char/serial.c b/hw/char/serial.c index 2ab8b69e03..c822a9ae6c 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -1043,7 +1043,6 @@ static void serial_class_init(ObjectClass *klass, void* data) dc->user_creatable = false; dc->realize = serial_realize; dc->unrealize = serial_unrealize; - dc->vmsd = &vmstate_serial; device_class_set_props(dc, serial_properties); } @@ -1113,6 +1112,16 @@ static void serial_mm_realize(DeviceState *dev, Error **errp) sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq); } +static const VMStateDescription vmstate_serial_mm = { + .name = "serial", + .version_id = 3, + .minimum_version_id = 2, + .fields = (VMStateField[]) { + VMSTATE_STRUCT(serial, SerialMM, 0, vmstate_serial, SerialState), + VMSTATE_END_OF_LIST() + } +}; + SerialMM *serial_mm_init(MemoryRegion *address_space, hwaddr base, int regshift, qemu_irq irq, int baudbase, @@ -1162,6 +1171,7 @@ static void serial_mm_class_init(ObjectClass *oc, void *data) device_class_set_props(dc, serial_mm_properties); dc->realize = serial_mm_realize; + dc->vmsd = &vmstate_serial_mm; } static const TypeInfo serial_mm_info = {