Hi On Mon, Mar 30, 2020 at 7:41 PM Dr. David Alan Gilbert <[email protected]> wrote: > > * Marc-André Lureau ([email protected]) wrote: > > Hi > > > > On Mon, Mar 30, 2020 at 6:47 PM Dr. David Alan Gilbert (git) > > <[email protected]> wrote: > > > > > > From: "Dr. David Alan Gilbert" <[email protected]> > > > > > > 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 <[email protected]> > > > --- > > > 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 <[email protected]> > > > # 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 / [email protected] / Manchester, UK > thanks! -- Marc-André Lureau
