Re: [PATCH] serial: Fix double migration data

[Marc-André Lureau]

Hi

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

thanks!

-- 
Marc-André Lureau