[PATCH v4 12/37] serial: start making SerialMM a sysbus device

Marc-André Lureau posted 37 patches 6 years, 2 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, BALATON Zoltan <balaton@eik.bme.hu>, KONRAD Frederic <frederic.konrad@adacore.com>, Corey Minyard <cminyard@mvista.com>, Magnus Damm <magnus.damm@gmail.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Laurent Vivier <laurent@vivier.eu>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Paul Burton <pburton@wavecomp.com>, Aleksandar Markovic <amarkovic@wavecomp.com>, Eduardo Habkost <ehabkost@redhat.com>, Aurelien Jarno <aurelien@aurel32.net>, Jason Wang <jasowang@redhat.com>, Fabien Chouteau <chouteau@adacore.com>, Richard Henderson <rth@twiddle.net>
There is a newer version of this series
[PATCH v4 12/37] serial: start making SerialMM a sysbus device
Posted by Marc-André Lureau 6 years, 2 months ago
Memory mapped serial device is in fact a sysbus device. The following
patches will make use of sysbus facilities for resource and
registration. In particular, "serial-mm: use sysbus facilities" will
move internal serial realization to serial_mm_realize callback to
follow qdev best practices.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/char/omap_uart.c      |  2 +-
 hw/char/serial.c         | 47 ++++++++++++++++++++++++++++------------
 hw/mips/boston.c         |  2 +-
 hw/mips/mips_malta.c     |  2 +-
 include/hw/char/serial.h | 20 ++++++++++++-----
 5 files changed, 51 insertions(+), 22 deletions(-)

diff --git a/hw/char/omap_uart.c b/hw/char/omap_uart.c
index 13e4f43c4c..e8da933378 100644
--- a/hw/char/omap_uart.c
+++ b/hw/char/omap_uart.c
@@ -27,7 +27,7 @@
 struct omap_uart_s {
     MemoryRegion iomem;
     hwaddr base;
-    SerialState *serial; /* TODO */
+    SerialMM *serial; /* TODO */
     struct omap_target_agent_s *ta;
     omap_clk fclk;
     qemu_irq irq;
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 2d8471dbbb..cb9cbd5146 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1031,16 +1031,16 @@ static const TypeInfo serial_info = {
 static uint64_t serial_mm_read(void *opaque, hwaddr addr,
                                unsigned size)
 {
-    SerialState *s = opaque;
-    return serial_ioport_read(s, addr >> s->it_shift, 1);
+    SerialMM *s = SERIAL_MM(opaque);
+    return serial_ioport_read(&s->serial, addr >> s->it_shift, 1);
 }
 
 static void serial_mm_write(void *opaque, hwaddr addr,
                             uint64_t value, unsigned size)
 {
-    SerialState *s = opaque;
+    SerialMM *s = SERIAL_MM(opaque);
     value &= 255;
-    serial_ioport_write(s, addr >> s->it_shift, value, 1);
+    serial_ioport_write(&s->serial, addr >> s->it_shift, value, 1);
 }
 
 static const MemoryRegionOps serial_mm_ops[3] = {
@@ -1067,30 +1067,49 @@ static const MemoryRegionOps serial_mm_ops[3] = {
     },
 };
 
-SerialState *serial_mm_init(MemoryRegion *address_space,
+SerialMM *serial_mm_init(MemoryRegion *address_space,
                             hwaddr base, int it_shift,
                             qemu_irq irq, int baudbase,
                             Chardev *chr, enum device_endian end)
 {
-    DeviceState *dev = DEVICE(object_new(TYPE_SERIAL));
-    SerialState *s = SERIAL(dev);
+    SerialMM *smm = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM));
+    SerialState *s = &smm->serial;
 
-    s->it_shift = it_shift;
+    smm->it_shift = it_shift;
     s->irq = irq;
-    qdev_prop_set_uint32(dev, "baudbase", baudbase);
-    qdev_prop_set_chr(dev, "chardev", chr);
-    qdev_set_legacy_instance_id(dev, base, 2);
-    qdev_init_nofail(dev);
+    qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase);
+    qdev_prop_set_chr(DEVICE(s), "chardev", chr);
+    qdev_set_legacy_instance_id(DEVICE(s), base, 2);
+
+    qdev_init_nofail(DEVICE(s));
+    qdev_init_nofail(DEVICE(smm));
 
-    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s,
+    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], smm,
                           "serial", 8 << it_shift);
     memory_region_add_subregion(address_space, base, &s->io);
-    return s;
+
+    return smm;
+}
+
+static void serial_mm_instance_init(Object *o)
+{
+    SerialMM *smm = SERIAL_MM(o);
+
+    object_initialize_child(o, "serial", &smm->serial, sizeof(smm->serial),
+                            TYPE_SERIAL, &error_abort, NULL);
 }
 
+static const TypeInfo serial_mm_info = {
+    .name = TYPE_SERIAL_MM,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_init = serial_mm_instance_init,
+    .instance_size = sizeof(SerialMM),
+};
+
 static void serial_register_types(void)
 {
     type_register_static(&serial_info);
+    type_register_static(&serial_mm_info);
 }
 
 type_init(serial_register_types)
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index ca7d813a52..23fdd5ec6a 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -50,7 +50,7 @@ typedef struct {
 
     MachineState *mach;
     MIPSCPSState cps;
-    SerialState *uart;
+    SerialMM *uart;
 
     CharBackend lcd_display;
     char lcd_content[8];
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 92e9ca5bfa..89b5e3123a 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -83,7 +83,7 @@ typedef struct {
     uint32_t i2csel;
     CharBackend display;
     char display_text[9];
-    SerialState *uart;
+    SerialMM *uart;
     bool display_inited;
 } MaltaFPGAState;
 
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 548944b06a..730165347c 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -57,7 +57,6 @@ typedef struct SerialState {
     qemu_irq irq;
     CharBackend chr;
     int last_break_enable;
-    int it_shift;
     uint32_t baudbase;
     uint32_t tsr_retry;
     guint watch_tag;
@@ -80,6 +79,14 @@ typedef struct SerialState {
     MemoryRegion io;
 } SerialState;
 
+typedef struct SerialMM {
+    SysBusDevice parent;
+
+    SerialState serial;
+
+    int it_shift;
+} SerialMM;
+
 extern const VMStateDescription vmstate_serial;
 extern const MemoryRegionOps serial_io_ops;
 
@@ -88,12 +95,15 @@ void serial_set_frequency(SerialState *s, uint32_t frequency);
 #define TYPE_SERIAL "serial"
 #define SERIAL(s) OBJECT_CHECK(SerialState, (s), TYPE_SERIAL)
 
+#define TYPE_SERIAL_MM "serial-mm"
+#define SERIAL_MM(s) OBJECT_CHECK(SerialMM, (s), TYPE_SERIAL_MM)
+
 SerialState *serial_init(int base, qemu_irq irq, int baudbase,
                          Chardev *chr, MemoryRegion *system_io);
-SerialState *serial_mm_init(MemoryRegion *address_space,
-                            hwaddr base, int it_shift,
-                            qemu_irq irq, int baudbase,
-                            Chardev *chr, enum device_endian end);
+SerialMM *serial_mm_init(MemoryRegion *address_space,
+                         hwaddr base, int it_shift,
+                         qemu_irq irq, int baudbase,
+                         Chardev *chr, enum device_endian end);
 
 /* serial-isa.c */
 
-- 
2.24.0


Re: [PATCH v4 12/37] serial: start making SerialMM a sysbus device
Posted by Peter Maydell 6 years, 2 months ago
On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Memory mapped serial device is in fact a sysbus device. The following
> patches will make use of sysbus facilities for resource and
> registration. In particular, "serial-mm: use sysbus facilities" will
> move internal serial realization to serial_mm_realize callback to
> follow qdev best practices.

What goes wrong if you just put the realize of smm->serial in
the right place to start with ?

thanks
-- PMM

Re: [PATCH v4 12/37] serial: start making SerialMM a sysbus device
Posted by Marc-André Lureau 6 years, 2 months ago
On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Memory mapped serial device is in fact a sysbus device. The following
> > patches will make use of sysbus facilities for resource and
> > registration. In particular, "serial-mm: use sysbus facilities" will
> > move internal serial realization to serial_mm_realize callback to
> > follow qdev best practices.
>
> What goes wrong if you just put the realize of smm->serial in
> the right place to start with ?

You mean squash the following patches?
Sometime I go too fast, sometime it's too slow.

Decide what you prefer, this doesnt' matter much to me.


-- 
Marc-André Lureau

Re: [PATCH v4 12/37] serial: start making SerialMM a sysbus device
Posted by Peter Maydell 6 years, 2 months ago
On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> > >
> > > Memory mapped serial device is in fact a sysbus device. The following
> > > patches will make use of sysbus facilities for resource and
> > > registration. In particular, "serial-mm: use sysbus facilities" will
> > > move internal serial realization to serial_mm_realize callback to
> > > follow qdev best practices.
> >
> > What goes wrong if you just put the realize of smm->serial in
> > the right place to start with ?
>
> You mean squash the following patches?

No, I meant just having this patch have

static void serial_mm_realize(DeviceState *dev, Error **errp)
{
    SerialMM *smm = SERIAL_MM(dev);
    SerialState *s = &smm->serial;

    object_property_set_bool(OBJECT(dev), true, "realized", &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }
}

and

+ dc->realize = serial_mm_realize;

rather than manually doing the qdev_init_nofail()
in serial_mm_init(). This seems to me like an integral
part of the change to doing the init of the subdevice in the
init method, so it would be better unless there's a reason
why it breaks something. The rest of patch 15 (which is
what currently makes the equivalent change to realize) is all
about passing through the properties and exposing the
sysbus MMIO/irq regions and should stay a separate patch.

(setting the 'realized' property is better in a realize method
than using qdev_init_nofail() because it means we can propagate
any error outward rather than killing qemu.)

thanks
-- PMM

Re: [PATCH v4 12/37] serial: start making SerialMM a sysbus device
Posted by Marc-André Lureau 6 years, 2 months ago
Hi

On Thu, Nov 21, 2019 at 10:24 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau
> > > <marcandre.lureau@redhat.com> wrote:
> > > >
> > > > Memory mapped serial device is in fact a sysbus device. The following
> > > > patches will make use of sysbus facilities for resource and
> > > > registration. In particular, "serial-mm: use sysbus facilities" will
> > > > move internal serial realization to serial_mm_realize callback to
> > > > follow qdev best practices.
> > >
> > > What goes wrong if you just put the realize of smm->serial in
> > > the right place to start with ?
> >
> > You mean squash the following patches?
>
> No, I meant just having this patch have
>
> static void serial_mm_realize(DeviceState *dev, Error **errp)
> {
>     SerialMM *smm = SERIAL_MM(dev);
>     SerialState *s = &smm->serial;
>
>     object_property_set_bool(OBJECT(dev), true, "realized", &err);
>     if (err) {
>         error_propagate(errp, err);
>         return;
>     }
> }
>
> and
>
> + dc->realize = serial_mm_realize;
>
> rather than manually doing the qdev_init_nofail()
> in serial_mm_init(). This seems to me like an integral
> part of the change to doing the init of the subdevice in the
> init method, so it would be better unless there's a reason
> why it breaks something. The rest of patch 15 (which is
> what currently makes the equivalent change to realize) is all
> about passing through the properties and exposing the
> sysbus MMIO/irq regions and should stay a separate patch.
>
> (setting the 'realized' property is better in a realize method
> than using qdev_init_nofail() because it means we can propagate
> any error outward rather than killing qemu.)

Ok, but I implemented realize and moved serial init in "serial: make
SerialIO a sysbus device".

I propose to add another patch to follow your suggestion to use
set_boot("realize", true, errp) on top.

-- 
Marc-André Lureau

Re: [PATCH v4 12/37] serial: start making SerialMM a sysbus device
Posted by Peter Maydell 6 years, 2 months ago
On Thu, 21 Nov 2019 at 18:51, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Nov 21, 2019 at 10:24 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau
> > <marcandre.lureau@gmail.com> wrote:
> > >
> > > On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > >
> > > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau
> > > > <marcandre.lureau@redhat.com> wrote:
> > > > >
> > > > > Memory mapped serial device is in fact a sysbus device. The following
> > > > > patches will make use of sysbus facilities for resource and
> > > > > registration. In particular, "serial-mm: use sysbus facilities" will
> > > > > move internal serial realization to serial_mm_realize callback to
> > > > > follow qdev best practices.
> > > >
> > > > What goes wrong if you just put the realize of smm->serial in
> > > > the right place to start with ?
> > >
> > > You mean squash the following patches?
> >
> > No, I meant just having this patch have
> >
> > static void serial_mm_realize(DeviceState *dev, Error **errp)
> > {
> >     SerialMM *smm = SERIAL_MM(dev);
> >     SerialState *s = &smm->serial;
> >
> >     object_property_set_bool(OBJECT(dev), true, "realized", &err);
> >     if (err) {
> >         error_propagate(errp, err);
> >         return;
> >     }
> > }
> >
> > and
> >
> > + dc->realize = serial_mm_realize;
> >
> > rather than manually doing the qdev_init_nofail()
> > in serial_mm_init(). This seems to me like an integral
> > part of the change to doing the init of the subdevice in the
> > init method, so it would be better unless there's a reason
> > why it breaks something. The rest of patch 15 (which is
> > what currently makes the equivalent change to realize) is all
> > about passing through the properties and exposing the
> > sysbus MMIO/irq regions and should stay a separate patch.
> >
> > (setting the 'realized' property is better in a realize method
> > than using qdev_init_nofail() because it means we can propagate
> > any error outward rather than killing qemu.)
>
> Ok, but I implemented realize and moved serial init in "serial: make
> SerialIO a sysbus device".

That patch is for the TYPE_SERIAL_IO device, isn't it? It
changes both serial_io_instance_init and serial_io_realize
to do the instance init and realize of the TYPE_SERIAL device,
so it doesn't have the same "only doing half a change" issue
that this patch has for TYPE_SERIAL_MM.

thanks
-- PMM

Re: [PATCH v4 12/37] serial: start making SerialMM a sysbus device
Posted by Marc-André Lureau 6 years, 2 months ago
On Fri, Nov 22, 2019 at 2:11 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 21 Nov 2019 at 18:51, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Thu, Nov 21, 2019 at 10:24 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau
> > > <marcandre.lureau@gmail.com> wrote:
> > > >
> > > > On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > > >
> > > > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau
> > > > > <marcandre.lureau@redhat.com> wrote:
> > > > > >
> > > > > > Memory mapped serial device is in fact a sysbus device. The following
> > > > > > patches will make use of sysbus facilities for resource and
> > > > > > registration. In particular, "serial-mm: use sysbus facilities" will
> > > > > > move internal serial realization to serial_mm_realize callback to
> > > > > > follow qdev best practices.
> > > > >
> > > > > What goes wrong if you just put the realize of smm->serial in
> > > > > the right place to start with ?
> > > >
> > > > You mean squash the following patches?
> > >
> > > No, I meant just having this patch have
> > >
> > > static void serial_mm_realize(DeviceState *dev, Error **errp)
> > > {
> > >     SerialMM *smm = SERIAL_MM(dev);
> > >     SerialState *s = &smm->serial;
> > >
> > >     object_property_set_bool(OBJECT(dev), true, "realized", &err);
> > >     if (err) {
> > >         error_propagate(errp, err);
> > >         return;
> > >     }
> > > }
> > >
> > > and
> > >
> > > + dc->realize = serial_mm_realize;
> > >
> > > rather than manually doing the qdev_init_nofail()
> > > in serial_mm_init(). This seems to me like an integral
> > > part of the change to doing the init of the subdevice in the
> > > init method, so it would be better unless there's a reason
> > > why it breaks something. The rest of patch 15 (which is
> > > what currently makes the equivalent change to realize) is all
> > > about passing through the properties and exposing the
> > > sysbus MMIO/irq regions and should stay a separate patch.
> > >
> > > (setting the 'realized' property is better in a realize method
> > > than using qdev_init_nofail() because it means we can propagate
> > > any error outward rather than killing qemu.)
> >
> > Ok, but I implemented realize and moved serial init in "serial: make
> > SerialIO a sysbus device".
>
> That patch is for the TYPE_SERIAL_IO device, isn't it? It
> changes both serial_io_instance_init and serial_io_realize
> to do the instance init and realize of the TYPE_SERIAL device,
> so it doesn't have the same "only doing half a change" issue
> that this patch has for TYPE_SERIAL_MM.
>

Ok, I got it, thanks.




-- 
Marc-André Lureau