Re: [PATCH] serial: Fix double migration data

[Marc-André Lureau]

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?

# 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