* [PATCH] serial: Fix double migration data
@ 2020-03-30 16:47 Dr. David Alan Gilbert (git)
2020-03-30 17:29 ` Marc-André Lureau
0 siblings, 1 reply; 5+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2020-03-30 16:47 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau, pbonzini, mst
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] serial: Fix double migration data
2020-03-30 16:47 [PATCH] serial: Fix double migration data Dr. David Alan Gilbert (git)
@ 2020-03-30 17:29 ` Marc-André Lureau
2020-03-30 17:41 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 5+ messages in thread
From: Marc-André Lureau @ 2020-03-30 17:29 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: Paolo Bonzini, QEMU, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] serial: Fix double migration data
2020-03-30 17:29 ` Marc-André Lureau
@ 2020-03-30 17:41 ` Dr. David Alan Gilbert
2020-03-30 17:56 ` Marc-André Lureau
0 siblings, 1 reply; 5+ messages in thread
From: Dr. David Alan Gilbert @ 2020-03-30 17:41 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Paolo Bonzini, QEMU, Michael S. Tsirkin
* 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] serial: Fix double migration data
2020-03-30 17:41 ` Dr. David Alan Gilbert
@ 2020-03-30 17:56 ` Marc-André Lureau
2020-03-31 15:23 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Marc-André Lureau @ 2020-03-30 17:56 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Paolo Bonzini, QEMU, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] serial: Fix double migration data
2020-03-30 17:56 ` Marc-André Lureau
@ 2020-03-31 15:23 ` Paolo Bonzini
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2020-03-31 15:23 UTC (permalink / raw)
To: Marc-André Lureau, Dr. David Alan Gilbert; +Cc: QEMU, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-31 15:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30 16:47 [PATCH] serial: Fix double migration data Dr. David Alan Gilbert (git)
2020-03-30 17:29 ` Marc-André Lureau
2020-03-30 17:41 ` Dr. David Alan Gilbert
2020-03-30 17:56 ` Marc-André Lureau
2020-03-31 15:23 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).