[PATCH] serial: Fix double migration data

Dr. David Alan Gilbert (git) posted 1 patch 3 years, 12 months ago
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test FreeBSD passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200330164712.198282-1-dgilbert@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
hw/char/serial.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
[PATCH] serial: Fix double migration data
Posted by Dr. David Alan Gilbert (git) 3 years, 12 months ago
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()
+    }
+};
+
 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


Re: [PATCH] serial: Fix double migration data
Posted by Marc-André Lureau 3 years, 12 months ago
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

Re: [PATCH] serial: Fix double migration data
Posted by Dr. David Alan Gilbert 3 years, 12 months ago
* 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


Re: [PATCH] serial: Fix double migration data
Posted by Marc-André Lureau 3 years, 12 months ago
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!

-- 
Marc-André Lureau

Re: [PATCH] serial: Fix double migration data
Posted by Paolo Bonzini 3 years, 12 months ago
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