Re: [PATCH v4 06/37] serial: initial qom-ification

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Wed, Nov 20, 2019 at 8:42 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 11/20/19 4:24 PM, Marc-André Lureau 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 <marcandre.lureau@redhat.com>
> > ---
> >   hw/char/serial-isa.c       |  9 +++++++++
> >   hw/char/serial-pci-multi.c | 15 +++++++++++++++
> >   hw/char/serial-pci.c       | 13 ++++++++++++-
> >   hw/char/serial.c           | 33 +++++++++++++++++++++++++++------
> >   include/hw/char/serial.h   |  7 ++++++-
> >   5 files changed, 69 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 5c553c30d0..edfbfdca9e 100644
> > --- a/hw/char/serial-pci-multi.c
> > +++ b/hw/char/serial-pci-multi.c
> > @@ -181,10 +181,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 *pms = DO_UPCAST(PCIMultiSerialState, dev, dev);
> > +    size_t i, nports = multi_serial_get_port_count(PCI_DEVICE_GET_CLASS(dev));
> > +
> > +    for (i = 0; i < nports; i++) {
> > +        object_initialize_child(o, "serial[*]", &pms->state[i],
> > +                                sizeof(pms->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 },
> > @@ -196,6 +210,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..f99b6c19e0 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 *ps = PCI_SERIAL(o);
> > +
> > +    object_initialize_child(o, "serial", &ps->state, sizeof(ps->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..e0a5bec290 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,20 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase,
> >       return s;
> >   }
> >
> > +static void serial_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->user_creatable = false;
>
> Why? Due to the "chardev" property?

No, because it's not user usable as such (it's an internal device for
serialio/serialmm and others). And making abstract wouldn't work
either.

>
> > +}
> > +
> > +static const TypeInfo serial_info = {
> > +    .name = TYPE_SERIAL,
> > +    .parent = TYPE_DEVICE,
> > +    .instance_size = sizeof(SerialState),
> > +    .class_init = serial_class_init,
> > +};
> > +
> >   /* Memory mapped interface */
> >   static uint64_t serial_mm_read(void *opaque, hwaddr addr,
> >                                  unsigned size)
> > @@ -1045,9 +1059,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 +1069,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,
> >
>
>


-- 
Marc-André Lureau