Re: [PATCH v3 06/33] serial: initial qom-ification

[Marc-André Lureau]

On Wed, Oct 23, 2019 at 9:06 PM Marc-André Lureau
<address@hidden> wrote:
>
> Make SerialState a device (the following patches will introduce IO/MM
> sysbus serial devices)
>
> None of the serial_{,mm}_init() callers actually free the returned
> value (even if they did, it would be quite harmless), so we can change
> the object allocation at will.
>
> However, the devices that embed SerialState must now have their field
> QOM-initialized manually (isa, pci, pci-multi).
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/char/serial-isa.c       |  9 +++++++++
>  hw/char/serial-pci-multi.c | 15 +++++++++++++++
>  hw/char/serial-pci.c       | 13 ++++++++++++-
>  hw/char/serial.c           | 25 +++++++++++++++++++------
>  include/hw/char/serial.h   |  7 ++++++-
>  5 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
> index 9e31c51bb6..9a5928b3ee 100644
> --- a/hw/char/serial-isa.c
> +++ b/hw/char/serial-isa.c
> @@ -111,10 +111,19 @@ static void serial_isa_class_initfn(ObjectClass *klass, 
> void *data)
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
>
> +static void serial_isa_initfn(Object *o)
> +{
> +    ISASerialState *self = ISA_SERIAL(o);
> +
> +    object_initialize_child(o, "serial", &self->state, sizeof(self->state),
> +                            TYPE_SERIAL, &error_abort, NULL);
> +}
> +
>  static const TypeInfo serial_isa_info = {
>      .name          = TYPE_ISA_SERIAL,
>      .parent        = TYPE_ISA_DEVICE,
>      .instance_size = sizeof(ISASerialState),
> +    .instance_init = serial_isa_initfn,
>      .class_init    = serial_isa_class_initfn,
>  };
>
> diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
> index 6fa1cc6225..3485bdad87 100644
> --- a/hw/char/serial-pci-multi.c
> +++ b/hw/char/serial-pci-multi.c
> @@ -182,10 +182,24 @@ static void 
> multi_4x_serial_pci_class_initfn(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
>
> +static void multi_serial_init(Object *o)
> +{
> +    PCIDevice *dev = PCI_DEVICE(o);
> +    PCIMultiSerialState *self = DO_UPCAST(PCIMultiSerialState, dev, dev);
> +    int i, nr_ports = multi_serial_get_nr_ports(PCI_DEVICE_GET_CLASS(dev));
> +
> +    for (i = 0; i < nr_ports; i++) {
> +        object_initialize_child(o, "serial[*]", &self->state[i],
> +                                sizeof(self->state[i]),
> +                                TYPE_SERIAL, &error_abort, NULL);
> +    }
> +}
> +
>  static const TypeInfo multi_2x_serial_pci_info = {
>      .name          = "pci-serial-2x",
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(PCIMultiSerialState),
> +    .instance_init = multi_serial_init,
>      .class_init    = multi_2x_serial_pci_class_initfn,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> @@ -197,6 +211,7 @@ static const TypeInfo multi_4x_serial_pci_info = {
>      .name          = "pci-serial-4x",
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(PCIMultiSerialState),
> +    .instance_init = multi_serial_init,
>      .class_init    = multi_4x_serial_pci_class_initfn,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
> index cb9b76e22b..a33264a1fb 100644
> --- a/hw/char/serial-pci.c
> +++ b/hw/char/serial-pci.c
> @@ -40,6 +40,8 @@ typedef struct PCISerialState {
>      uint8_t prog_if;
>  } PCISerialState;
>
> +#define TYPE_PCI_SERIAL "pci-serial"
> +#define PCI_SERIAL(s) OBJECT_CHECK(PCISerialState, (s), TYPE_PCI_SERIAL)
>
>  static void serial_pci_realize(PCIDevice *dev, Error **errp)
>  {
> @@ -103,10 +105,19 @@ static void serial_pci_class_initfn(ObjectClass *klass, 
> void *data)
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
>
> +static void serial_pci_init(Object *o)
> +{
> +    PCISerialState *self = PCI_SERIAL(o);
> +
> +    object_initialize_child(o, "serial", &self->state, sizeof(self->state),
> +                            TYPE_SERIAL, &error_abort, NULL);
> +}
> +
>  static const TypeInfo serial_pci_info = {
> -    .name          = "pci-serial",
> +    .name          = TYPE_PCI_SERIAL,
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(PCISerialState),
> +    .instance_init = serial_pci_init,
>      .class_init    = serial_pci_class_initfn,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index b4aa250950..c839035fdd 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -983,9 +983,8 @@ const MemoryRegionOps serial_io_ops = {
>  SerialState *serial_init(int base, qemu_irq irq, int baudbase,
>                           Chardev *chr, MemoryRegion *system_io)
>  {
> -    SerialState *s;
> -
> -    s = g_malloc0(sizeof(SerialState));
> +    DeviceState *dev = DEVICE(object_new(TYPE_SERIAL));
> +    SerialState *s = SERIAL(dev);
>
>      s->irq = irq;
>      s->baudbase = baudbase;
> @@ -993,6 +992,7 @@ SerialState *serial_init(int base, qemu_irq irq, int 
> baudbase,
>      serial_realize_core(s, &error_fatal);
>
>      vmstate_register(NULL, base, &vmstate_serial, s);
> +    qdev_init_nofail(dev);
>
>      memory_region_init_io(&s->io, NULL, &serial_io_ops, s, "serial", 8);
>      memory_region_add_subregion(system_io, base, &s->io);
> @@ -1000,6 +1000,12 @@ SerialState *serial_init(int base, qemu_irq irq, int 
> baudbase,
>      return s;
>  }
>
> +static const TypeInfo serial_info = {
> +    .name = TYPE_SERIAL,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(SerialState),
> +};
> +

I added a class_init to set user_creatable=false: the base device is
useless when created from command line.

>  /* Memory mapped interface */
>  static uint64_t serial_mm_read(void *opaque, hwaddr addr,
>                                 unsigned size)
> @@ -1045,9 +1051,8 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
>                              qemu_irq irq, int baudbase,
>                              Chardev *chr, enum device_endian end)
>  {
> -    SerialState *s;
> -
> -    s = g_malloc0(sizeof(SerialState));
> +    DeviceState *dev = DEVICE(object_new(TYPE_SERIAL));
> +    SerialState *s = SERIAL(dev);
>
>      s->it_shift = it_shift;
>      s->irq = irq;
> @@ -1056,9 +1061,17 @@ SerialState *serial_mm_init(MemoryRegion 
> *address_space,
>
>      serial_realize_core(s, &error_fatal);
>      vmstate_register(NULL, base, &vmstate_serial, s);
> +    qdev_init_nofail(dev);
>
>      memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s,
>                            "serial", 8 << it_shift);
>      memory_region_add_subregion(address_space, base, &s->io);
>      return s;
>  }
> +
> +static void serial_register_types(void)
> +{
> +    type_register_static(&serial_info);
> +}
> +
> +type_init(serial_register_types)
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 8be3d8a4f9..180cc7c24e 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -30,10 +30,13 @@
>  #include "exec/memory.h"
>  #include "qemu/fifo8.h"
>  #include "chardev/char.h"
> +#include "hw/sysbus.h"
>
>  #define UART_FIFO_LENGTH    16      /* 16550A Fifo Length */
>
>  typedef struct SerialState {
> +    DeviceState parent;
> +
>      uint16_t divider;
>      uint8_t rbr; /* receive register */
>      uint8_t thr; /* transmit holding register */
> @@ -84,7 +87,9 @@ void serial_realize_core(SerialState *s, Error **errp);
>  void serial_exit_core(SerialState *s);
>  void serial_set_frequency(SerialState *s, uint32_t frequency);
>
> -/* legacy pre qom */
> +#define TYPE_SERIAL "serial"
> +#define SERIAL(s) OBJECT_CHECK(SerialState, (s), TYPE_SERIAL)
> +
>  SerialState *serial_init(int base, qemu_irq irq, int baudbase,
>                           Chardev *chr, MemoryRegion *system_io);
>  SerialState *serial_mm_init(MemoryRegion *address_space,
> --
> 2.23.0.606.g08da6496b6
>
>


-- 
Marc-André Lureau