[PATCH 0/2] hw/char/serial: Migrate I/O serial device

[PATCH 0/2] hw/char/serial: Migrate I/O serial device

[Philippe Mathieu-Daudé]

We migrate the memory mapped device, but not the I/O one.
No particular reason, so let it be migratable.

Philippe Mathieu-Daudé (2):
  hw/char/serial: Separate and document static properties
  hw/char/serial: Allow migration of the I/O serial device

 include/hw/char/serial.h |  7 +++++--
 hw/char/serial.c         | 12 +++++++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.21.3

[PATCH 1/2] hw/char/serial: Separate and document static properties

[Philippe Mathieu-Daudé]

Add more descriptive comments to keep a clear separation
between static property vs runtime changeable.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/serial.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 535fa23a2b..d955963ef1 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -55,9 +55,7 @@ typedef struct SerialState {
        it can be reset while reading iir */
     int thr_ipending;
     qemu_irq irq;
-    CharBackend chr;
     int last_break_enable;
-    uint32_t baudbase;
     uint32_t tsr_retry;
     guint watch_tag;
     uint32_t wakeup;
@@ -77,6 +75,10 @@ typedef struct SerialState {
 
     QEMUTimer *modem_status_poll;
     MemoryRegion io;
+
+    /* Properties */
+    CharBackend chr;
+    uint32_t baudbase;
 } SerialState;
 
 typedef struct SerialMM {
@@ -84,6 +86,7 @@ typedef struct SerialMM {
 
     SerialState serial;
 
+    /* Properties */
     uint8_t regshift;
     uint8_t endianness;
 } SerialMM;
-- 
2.21.3

[PATCH 2/2] hw/char/serial: Allow migration of the I/O serial device

[Philippe Mathieu-Daudé]

The serial device mapped on the I/O bus hold a migratable
SerialState. Keep the same version range from SerialState:

 837 const VMStateDescription vmstate_serial = {
 838     .name = "serial",
 839     .version_id = 3,
 840     .minimum_version_id = 2,

Fixes: 10315a7089 ("serial: make SerialIO a sysbus device")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 9eebcb27e7..c167b584fb 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1002,12 +1002,22 @@ static void serial_io_realize(DeviceState *dev, Error **errp)
     sysbus_init_irq(SYS_BUS_DEVICE(sio), &s->irq);
 }
 
+static const VMStateDescription vmstate_serial_io = {
+    .name = "serial",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(serial, SerialIO, 0, vmstate_serial, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void serial_io_class_init(ObjectClass *klass, void* data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = serial_io_realize;
-    /* No dc->vmsd: class has no migratable state */
+    dc->vmsd = &vmstate_serial_io;
 }
 
 static void serial_io_instance_init(Object *o)
-- 
2.21.3

Re: [PATCH 2/2] hw/char/serial: Allow migration of the I/O serial device

[Paolo Bonzini]

On 03/07/20 20:58, Philippe Mathieu-Daudé wrote:
> The serial device mapped on the I/O bus hold a migratable
> SerialState. Keep the same version range from SerialState:
> 
>  837 const VMStateDescription vmstate_serial = {
>  838     .name = "serial",
>  839     .version_id = 3,
>  840     .minimum_version_id = 2,
> 
> Fixes: 10315a7089 ("serial: make SerialIO a sysbus device")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  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 9eebcb27e7..c167b584fb 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -1002,12 +1002,22 @@ static void serial_io_realize(DeviceState *dev, Error **errp)
>      sysbus_init_irq(SYS_BUS_DEVICE(sio), &s->irq);
>  }
>  
> +static const VMStateDescription vmstate_serial_io = {
> +    .name = "serial",
> +    .version_id = 3,
> +    .minimum_version_id = 2,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT(serial, SerialIO, 0, vmstate_serial, SerialState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void serial_io_class_init(ObjectClass *klass, void* data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = serial_io_realize;
> -    /* No dc->vmsd: class has no migratable state */
> +    dc->vmsd = &vmstate_serial_io;
>  }
>  
>  static void serial_io_instance_init(Object *o)
> 

Is there any difference between SerialMM and SerialIO at this point?

Paolo

Re: [PATCH 1/2] hw/char/serial: Separate and document static properties

[Peter Maydell]

On Fri, 3 Jul 2020 at 19:59, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Add more descriptive comments to keep a clear separation
> between static property vs runtime changeable.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/serial.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 535fa23a2b..d955963ef1 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -55,9 +55,7 @@ typedef struct SerialState {
>         it can be reset while reading iir */
>      int thr_ipending;
>      qemu_irq irq;
> -    CharBackend chr;
>      int last_break_enable;
> -    uint32_t baudbase;
>      uint32_t tsr_retry;
>      guint watch_tag;
>      uint32_t wakeup;
> @@ -77,6 +75,10 @@ typedef struct SerialState {
>
>      QEMUTimer *modem_status_poll;
>      MemoryRegion io;
> +
> +    /* Properties */
> +    CharBackend chr;
> +    uint32_t baudbase;
>  } SerialState;
>
>  typedef struct SerialMM {
> @@ -84,6 +86,7 @@ typedef struct SerialMM {
>
>      SerialState serial;
>
> +    /* Properties */
>      uint8_t regshift;
>      uint8_t endianness;
>  } SerialMM;

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Incidentally 'baudbase' is technically runtime updateable
via the serial_set_frequency() function, except that there
are no callers of it. We should delete that as unused code.

thanks
-- PMM

Re: [PATCH 2/2] hw/char/serial: Allow migration of the I/O serial device

[Peter Maydell]

On Tue, 7 Jul 2020 at 11:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 03/07/20 20:58, Philippe Mathieu-Daudé wrote:
> > The serial device mapped on the I/O bus hold a migratable
> > SerialState. Keep the same version range from SerialState:
> >
> >  837 const VMStateDescription vmstate_serial = {
> >  838     .name = "serial",
> >  839     .version_id = 3,
> >  840     .minimum_version_id = 2,
> >
> > Fixes: 10315a7089 ("serial: make SerialIO a sysbus device")
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  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 9eebcb27e7..c167b584fb 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -1002,12 +1002,22 @@ static void serial_io_realize(DeviceState *dev, Error **errp)
> >      sysbus_init_irq(SYS_BUS_DEVICE(sio), &s->irq);
> >  }
> >
> > +static const VMStateDescription vmstate_serial_io = {
> > +    .name = "serial",
> > +    .version_id = 3,
> > +    .minimum_version_id = 2,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_STRUCT(serial, SerialIO, 0, vmstate_serial, SerialState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static void serial_io_class_init(ObjectClass *klass, void* data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >
> >      dc->realize = serial_io_realize;
> > -    /* No dc->vmsd: class has no migratable state */
> > +    dc->vmsd = &vmstate_serial_io;
> >  }
> >
> >  static void serial_io_instance_init(Object *o)
> >
>
> Is there any difference between SerialMM and SerialIO at this point?

SerialIO insists on 1-byte wide accesses; SerialMM allows the
creator of the device to specify the spacing between registers
and the endianness. So I suppose SerialIO is kind of a subset of
SerialMM.

It looks like the only user of TYPE_SERIAL_IO is hw/mips/mipssim.c,
Adding the migration state here would be a forwards migration
compat break anyway, so I think we could just change that
machine to use TYPE_SERIAL_MM and then delete TYPE_SERIAL_IO ?

thanks
-- PMM

Re: [PATCH 1/2] hw/char/serial: Separate and document static properties

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Add more descriptive comments to keep a clear separation
> between static property vs runtime changeable.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 2/2] hw/char/serial: Allow migration of the I/O serial device

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The serial device mapped on the I/O bus hold a migratable
> SerialState. Keep the same version range from SerialState:
>
>  837 const VMStateDescription vmstate_serial = {
>  838     .name = "serial",
>  839     .version_id = 3,
>  840     .minimum_version_id = 2,
>
> Fixes: 10315a7089 ("serial: make SerialIO a sysbus device")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

In the sense that the vmstate stuff is correct.

Reviewed-by: Juan Quintela <quintela@redhat.com>

But as Peter says, it appears that it is better to just drop the whole
serial_io infrastructure if we can switch the mips simulator
implementation to use mm, no?

Later, Juan.