[PATCH v3 00/14] TYPE_SERIAL cleanup

[PATCH v3 00/14] TYPE_SERIAL cleanup

[Bernhard Beschow]

This series consists of cosmetic changes around TYPE_SERIAL. The biggest change
is having it derive from TYPE_SYS_BUS_DEVICE which 1) eliminates usage of
qemu_{un,}register_reset() and 2) stops all its users from accessing private
data. The latter allows TYPE_SERIAL to be ported to Rust in the (near) future.

v3:
* Pick up further R-b tags. Thanks!
* Add patch "hw/char/{diva-gsp,serial-pci-multi}: Fix deinitialization order"
  (Peter)
* Justify switching to serial_io_ops in certain cases in TYPE_SERIAL_MM (Peter)

v2:
* Pick up R-b tags. Thanks!
* Ad patch to free consumers of TYPE_SERIAL from setting up MemoryRegionOps;
  configure via properties instead which should resolve odd code (Peter)
* Improve wording in one commit message (Peter)
* Sort patches by moving char/serial last
-> Keep "Avoid implicit conversion when tracing" untouched for now. Can be
  omitted if deemed inappropriate.

Testing done:
* Boot mpc8544ds machine with Linux and interact with it on the serial console
* Boot q35 machine with Arch Linux and interact with it on the serial console
* Make check
* Make check-functional with all architectures enabled:
    Summary of Failures:

    238/313 func-thorough+func-aarch64-thorough+thorough - qemu:func-aarch64-tcg_plugins          ERROR             0.72s   exit status 1
    309/313 func-thorough+func-x86_64-thorough+thorough - qemu:func-x86_64-vhost_user_bridge      TIMEOUT          90.01s   killed by signal 15 SIGTERM
    311/313 func-thorough+func-x86_64-thorough+thorough - qemu:func-x86_64-rebuild_vmfd           ERROR            33.64s   exit status 1

    Ok:                257
    Fail:              2
    Skipped:           53
    Timeout:           1

Bernhard Beschow (14):
  hw/arm/Kconfig: Fix serial selection for NPCM8XX
  hw/char/{diva-gsp,serial-pci-multi}: Fix deinitialization order
  hw/arm/aspeed_ast27x0-{ssp, tsp}: Do not access SerialMM internals
    directly
  util/fifo8: Make all read-only methods const-correct
  hw/char/serial: Remove explicit cast from void pointer
  hw/char/serial: Prefer fifo8 methods over open-coding
  hw/char/serial: Reuse fifo8_num_used()
  hw/char/serial: Remove unhelpful comment
  hw/char/serial: Add constants for Line Control Register
  hw/char/serial: Remove redundant reset
  hw/char/serial: Avoid implicit conversion when tracing
  hw/char/serial: Keep MemoryRegionOps private
  hw/char/serial: Inherit from SysBusDevice
  hw/char/serial: Plug into reset framework

 include/hw/char/serial-mm.h |   3 -
 include/hw/char/serial.h    |   8 ++-
 include/qemu/fifo8.h        |  10 +--
 hw/arm/aspeed_ast27x0-ssp.c |   7 +-
 hw/arm/aspeed_ast27x0-tsp.c |   7 +-
 hw/char/diva-gsp.c          |  23 +++---
 hw/char/serial-isa.c        |  17 +++--
 hw/char/serial-mm.c         |  59 ++--------------
 hw/char/serial-pci-multi.c  |  23 +++---
 hw/char/serial-pci.c        |  11 ++-
 hw/char/serial.c            | 137 +++++++++++++++++++++++++-----------
 util/fifo8.c                |  10 +--
 hw/arm/Kconfig              |   2 +-
 hw/char/trace-events        |   4 +-
 14 files changed, 157 insertions(+), 164 deletions(-)

-- 
2.53.0

[PATCH v3 01/14] hw/arm/Kconfig: Fix serial selection for NPCM8XX

[Bernhard Beschow]

CONFIG_SERIAL selects the internal TYPE_SERIAL device which is akin to
an "IP block" that needs to be integrated with glue logic. In case of
NPCM8XX this glue logic is TYPE_SERIAL_MM which the code uses already.
Fix Kconfig to select CONFIG_SERIAL_MM which matches TYPE_SERIAL_MM.

Fixes: ae0c4d1a1290 ("hw/arm: Add NPCM8XX SoC")
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index c66c452737..20e99e52b1 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -493,7 +493,7 @@ config NPCM8XX
     select SMBUS
     select PL310  # cache controller
     select NPCM7XX
-    select SERIAL
+    select SERIAL_MM
     select SSI
     select UNIMP
 
-- 
2.53.0

[PATCH v3 02/14] hw/char/{diva-gsp, serial-pci-multi}: Fix deinitialization order

[Bernhard Beschow]

The memory region is owned by the device being unrealized, so must be
removed from the mapping before unrealizing.

Fixes: d66bbea4e0d3 ("serial: add 2x + 4x pci variant")
Fixes: 274074708455 ("hw/char: Add emulation of Diva GSP PCI management boards")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/char/diva-gsp.c         | 2 +-
 hw/char/serial-pci-multi.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
index 280d0413c6..f9aa6e326d 100644
--- a/hw/char/diva-gsp.c
+++ b/hw/char/diva-gsp.c
@@ -62,8 +62,8 @@ static void diva_pci_exit(PCIDevice *dev)
 
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
-        qdev_unrealize(DEVICE(s));
         memory_region_del_subregion(&pci->membar, &s->io);
+        qdev_unrealize(DEVICE(s));
         g_free(pci->name[i]);
     }
     qemu_free_irqs(pci->irqs, pci->ports);
diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
index 17796b93dd..7782452018 100644
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -56,8 +56,8 @@ static void multi_serial_pci_exit(PCIDevice *dev)
 
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
-        qdev_unrealize(DEVICE(s));
         memory_region_del_subregion(&pci->iobar, &s->io);
+        qdev_unrealize(DEVICE(s));
         g_free(pci->name[i]);
     }
 }
-- 
2.53.0

[PATCH v3 03/14] hw/arm/aspeed_ast27x0-{ssp, tsp}: Do not access SerialMM internals directly

[Bernhard Beschow]

SerialMM inherits from SysBusDevice and exposes the memory region by
means of sysbus_mmio_get_region(). Use that in order to avoid accessing
implementation details of SerialMM.

Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/arm/aspeed_ast27x0-ssp.c | 7 ++++---
 hw/arm/aspeed_ast27x0-tsp.c | 7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/arm/aspeed_ast27x0-ssp.c b/hw/arm/aspeed_ast27x0-ssp.c
index 9b12ba6743..8b84300e0f 100644
--- a/hw/arm/aspeed_ast27x0-ssp.c
+++ b/hw/arm/aspeed_ast27x0-ssp.c
@@ -149,6 +149,7 @@ static void aspeed_soc_ast27x0ssp_realize(DeviceState *dev_soc, Error **errp)
     AspeedCoprocessorState *s = ASPEED_COPROCESSOR(dev_soc);
     AspeedCoprocessorClass *sc = ASPEED_COPROCESSOR_GET_CLASS(s);
     DeviceState *armv7m;
+    MemoryRegion *mr;
     g_autofree char *sdram_name = NULL;
     int i;
 
@@ -230,9 +231,9 @@ static void aspeed_soc_ast27x0ssp_realize(DeviceState *dev_soc, Error **errp)
     }
 
     /* UART */
-    memory_region_init_alias(&s->uart_alias, OBJECT(s), "uart.alias",
-                             &s->uart->serial.io, 0,
-                             memory_region_size(&s->uart->serial.io));
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(s->uart), 0);
+    memory_region_init_alias(&s->uart_alias, OBJECT(s), "uart.alias", mr, 0,
+                             memory_region_size(mr));
     memory_region_add_subregion(s->memory, sc->memmap[s->uart_dev],
                                 &s->uart_alias);
     /*
diff --git a/hw/arm/aspeed_ast27x0-tsp.c b/hw/arm/aspeed_ast27x0-tsp.c
index e39d1dc171..e7c7b74491 100644
--- a/hw/arm/aspeed_ast27x0-tsp.c
+++ b/hw/arm/aspeed_ast27x0-tsp.c
@@ -149,6 +149,7 @@ static void aspeed_soc_ast27x0tsp_realize(DeviceState *dev_soc, Error **errp)
     AspeedCoprocessorState *s = ASPEED_COPROCESSOR(dev_soc);
     AspeedCoprocessorClass *sc = ASPEED_COPROCESSOR_GET_CLASS(s);
     DeviceState *armv7m;
+    MemoryRegion *mr;
     g_autofree char *sdram_name = NULL;
     int i;
 
@@ -230,9 +231,9 @@ static void aspeed_soc_ast27x0tsp_realize(DeviceState *dev_soc, Error **errp)
     }
 
     /* UART */
-    memory_region_init_alias(&s->uart_alias, OBJECT(s), "uart.alias",
-                             &s->uart->serial.io, 0,
-                             memory_region_size(&s->uart->serial.io));
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(s->uart), 0);
+    memory_region_init_alias(&s->uart_alias, OBJECT(s), "uart.alias", mr, 0,
+                             memory_region_size(mr));
     memory_region_add_subregion(s->memory, sc->memmap[s->uart_dev],
                                 &s->uart_alias);
     /*
-- 
2.53.0

[PATCH v3 04/14] util/fifo8: Make all read-only methods const-correct

[Bernhard Beschow]

Allows these methods to be used in const contexts, i.e. where the parent
of the fifo itself is const. This is in particular useful for Rust code.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/fifo8.h | 10 +++++-----
 util/fifo8.c         | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
index 4f768d4ee3..6b476b404e 100644
--- a/include/qemu/fifo8.h
+++ b/include/qemu/fifo8.h
@@ -71,7 +71,7 @@ uint8_t fifo8_pop(Fifo8 *fifo);
  *
  * Returns: The peeked data byte.
  */
-uint8_t fifo8_peek(Fifo8 *fifo);
+uint8_t fifo8_peek(const Fifo8 *fifo);
 
 /**
  * fifo8_pop_buf:
@@ -181,7 +181,7 @@ void fifo8_reset(Fifo8 *fifo);
  *
  * Returns: True if the fifo is empty, false otherwise.
  */
-bool fifo8_is_empty(Fifo8 *fifo);
+bool fifo8_is_empty(const Fifo8 *fifo);
 
 /**
  * fifo8_is_full:
@@ -191,7 +191,7 @@ bool fifo8_is_empty(Fifo8 *fifo);
  *
  * Returns: True if the fifo is full, false otherwise.
  */
-bool fifo8_is_full(Fifo8 *fifo);
+bool fifo8_is_full(const Fifo8 *fifo);
 
 /**
  * fifo8_num_free:
@@ -201,7 +201,7 @@ bool fifo8_is_full(Fifo8 *fifo);
  *
  * Returns: Number of free bytes.
  */
-uint32_t fifo8_num_free(Fifo8 *fifo);
+uint32_t fifo8_num_free(const Fifo8 *fifo);
 
 /**
  * fifo8_num_used:
@@ -211,7 +211,7 @@ uint32_t fifo8_num_free(Fifo8 *fifo);
  *
  * Returns: Number of used bytes.
  */
-uint32_t fifo8_num_used(Fifo8 *fifo);
+uint32_t fifo8_num_used(const Fifo8 *fifo);
 
 extern const VMStateDescription vmstate_fifo8;
 
diff --git a/util/fifo8.c b/util/fifo8.c
index a26da66ad2..cc4f590b7a 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -71,7 +71,7 @@ uint8_t fifo8_pop(Fifo8 *fifo)
     return ret;
 }
 
-uint8_t fifo8_peek(Fifo8 *fifo)
+uint8_t fifo8_peek(const Fifo8 *fifo)
 {
     assert(fifo->num > 0);
     return fifo->data[fifo->head];
@@ -157,22 +157,22 @@ void fifo8_drop(Fifo8 *fifo, uint32_t len)
     assert(len == 0);
 }
 
-bool fifo8_is_empty(Fifo8 *fifo)
+bool fifo8_is_empty(const Fifo8 *fifo)
 {
     return (fifo->num == 0);
 }
 
-bool fifo8_is_full(Fifo8 *fifo)
+bool fifo8_is_full(const Fifo8 *fifo)
 {
     return (fifo->num == fifo->capacity);
 }
 
-uint32_t fifo8_num_free(Fifo8 *fifo)
+uint32_t fifo8_num_free(const Fifo8 *fifo)
 {
     return fifo->capacity - fifo->num;
 }
 
-uint32_t fifo8_num_used(Fifo8 *fifo)
+uint32_t fifo8_num_used(const Fifo8 *fifo)
 {
     return fifo->num;
 }
-- 
2.53.0

[PATCH v3 05/14] hw/char/serial: Remove explicit cast from void pointer

[Bernhard Beschow]

A void pointer asks for being casted, so C allows for omitting the
explicit cast. Take advantage of that.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/serial.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index adbd1d1d4a..0f2e79dfba 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -715,7 +715,7 @@ static const VMStateDescription vmstate_serial_thr_ipending = {
 
 static bool serial_tsr_needed(void *opaque)
 {
-    SerialState *s = (SerialState *)opaque;
+    SerialState *s = opaque;
     return s->tsr_retry != 0;
 }
 
@@ -734,7 +734,7 @@ static const VMStateDescription vmstate_serial_tsr = {
 
 static bool serial_recv_fifo_needed(void *opaque)
 {
-    SerialState *s = (SerialState *)opaque;
+    SerialState *s = opaque;
     return !fifo8_is_empty(&s->recv_fifo);
 
 }
@@ -752,7 +752,7 @@ static const VMStateDescription vmstate_serial_recv_fifo = {
 
 static bool serial_xmit_fifo_needed(void *opaque)
 {
-    SerialState *s = (SerialState *)opaque;
+    SerialState *s = opaque;
     return !fifo8_is_empty(&s->xmit_fifo);
 }
 
@@ -769,7 +769,7 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
 
 static bool serial_fifo_timeout_timer_needed(void *opaque)
 {
-    SerialState *s = (SerialState *)opaque;
+    SerialState *s = opaque;
     return timer_pending(s->fifo_timeout_timer);
 }
 
@@ -786,7 +786,7 @@ static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
 
 static bool serial_timeout_ipending_needed(void *opaque)
 {
-    SerialState *s = (SerialState *)opaque;
+    SerialState *s = opaque;
     return s->timeout_ipending != 0;
 }
 
@@ -803,7 +803,7 @@ static const VMStateDescription vmstate_serial_timeout_ipending = {
 
 static bool serial_poll_needed(void *opaque)
 {
-    SerialState *s = (SerialState *)opaque;
+    SerialState *s = opaque;
     return s->poll_msl >= 0;
 }
 
-- 
2.53.0

[PATCH v3 06/14] hw/char/serial: Prefer fifo8 methods over open-coding

[Bernhard Beschow]

Use fifo8_is_empty() and fifo8_is_full() to improve readability of the
code.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/serial.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 0f2e79dfba..20f68fd2f8 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -239,7 +239,7 @@ static void serial_xmit(SerialState *s)
             if (s->fcr & UART_FCR_FE) {
                 assert(!fifo8_is_empty(&s->xmit_fifo));
                 s->tsr = fifo8_pop(&s->xmit_fifo);
-                if (!s->xmit_fifo.num) {
+                if (fifo8_is_empty(&s->xmit_fifo)) {
                     s->lsr |= UART_LSR_THRE;
                 }
             } else {
@@ -481,7 +481,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
             if(s->fcr & UART_FCR_FE) {
                 ret = fifo8_is_empty(&s->recv_fifo) ?
                             0 : fifo8_pop(&s->recv_fifo);
-                if (s->recv_fifo.num == 0) {
+                if (fifo8_is_empty(&s->recv_fifo)) {
                     s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
                 } else {
                     timer_mod(s->fifo_timeout_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->char_transmit_time * 4);
@@ -555,7 +555,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
 static int serial_can_receive(SerialState *s)
 {
     if(s->fcr & UART_FCR_FE) {
-        if (s->recv_fifo.num < UART_FIFO_LENGTH) {
+        if (!fifo8_is_full(&s->recv_fifo)) {
             /*
              * Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1
              * if above. If UART_FIFO_LENGTH - fifo.count is advertised the
@@ -585,7 +585,7 @@ static void serial_receive_break(SerialState *s)
 /* There's data in recv_fifo and s->rbr has not been read for 4 char transmit times */
 static void fifo_timeout_int (void *opaque) {
     SerialState *s = opaque;
-    if (s->recv_fifo.num) {
+    if (!fifo8_is_empty(&s->recv_fifo)) {
         s->timeout_ipending = 1;
         serial_update_irq(s);
     }
-- 
2.53.0

[PATCH v3 07/14] hw/char/serial: Reuse fifo8_num_used()

[Bernhard Beschow]

Avoids accessing private fields of struct Fifo8. Now, TYPE_SERIAL only
accesses struct Fifo8 through its methods.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/serial.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 20f68fd2f8..2c558cb9fc 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -128,7 +128,7 @@ static void serial_update_irq(SerialState *s)
         tmp_iir = UART_IIR_CTI;
     } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR) &&
                (!(s->fcr & UART_FCR_FE) ||
-                s->recv_fifo.num >= s->recv_fifo_itl)) {
+                fifo8_num_used(&s->recv_fifo) >= s->recv_fifo_itl)) {
         tmp_iir = UART_IIR_RDI;
     } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
         tmp_iir = UART_IIR_THRI;
@@ -563,8 +563,8 @@ static int serial_can_receive(SerialState *s)
              * the guest has a chance to respond, effectively overriding the ITL
              * that the guest has set.
              */
-            return (s->recv_fifo.num <= s->recv_fifo_itl) ?
-                        s->recv_fifo_itl - s->recv_fifo.num : 1;
+            return (fifo8_num_used(&s->recv_fifo) <= s->recv_fifo_itl) ?
+                        s->recv_fifo_itl - fifo8_num_used(&s->recv_fifo) : 1;
         } else {
             return 0;
         }
-- 
2.53.0

[PATCH v3 08/14] hw/char/serial: Remove unhelpful comment

[Bernhard Beschow]

There is no "is_load" flag and one can tell from the method name what
the method does. Remove this unhelpful comment.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/serial.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 2c558cb9fc..f73de1ae4f 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -281,9 +281,6 @@ static void serial_xmit(SerialState *s)
     s->lsr |= UART_LSR_TEMT;
 }
 
-/* Setter for FCR.
-   is_load flag means, that value is set while loading VM state
-   and interrupt should not be invoked */
 static void serial_write_fcr(SerialState *s, uint8_t val)
 {
     /* Set fcr - val only has the bits that are supposed to "stick" */
-- 
2.53.0

[PATCH v3 09/14] hw/char/serial: Add constants for Line Control Register

[Bernhard Beschow]

Substitute some magic numbers by named constants for slightly improved
readability.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/serial.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index f73de1ae4f..485b98f03f 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -39,6 +39,11 @@
 #include "hw/core/qdev-properties-system.h"
 
 #define UART_LCR_DLAB   0x80    /* Divisor latch access bit */
+#define UART_LCR_SB     0x40    /* Set break */
+#define UART_LCR_EPS    0x10    /* Even parity select */
+#define UART_LCR_PEN    0x08    /* Parity enable */
+#define UART_LCR_NSTB   0x04    /* Number of stop bits */
+#define UART_LCR_WLS    0x03    /* Word length select */
 
 #define UART_IER_MSI    0x08    /* Enable Modem status interrupt */
 #define UART_IER_RLSI   0x04    /* Enable receiver line status interrupt */
@@ -153,23 +158,23 @@ static void serial_update_parameters(SerialState *s)
 
     /* Start bit. */
     frame_size = 1;
-    if (s->lcr & 0x08) {
+    if (s->lcr & UART_LCR_PEN) {
         /* Parity bit. */
         frame_size++;
-        if (s->lcr & 0x10)
+        if (s->lcr & UART_LCR_EPS)
             parity = 'E';
         else
             parity = 'O';
     } else {
             parity = 'N';
     }
-    if (s->lcr & 0x04) {
+    if (s->lcr & UART_LCR_NSTB) {
         stop_bits = 2;
     } else {
         stop_bits = 1;
     }
 
-    data_bits = (s->lcr & 0x03) + 5;
+    data_bits = (s->lcr & UART_LCR_WLS) + 5;
     frame_size += data_bits + stop_bits;
     /* Zero divisor should give about 3500 baud */
     speed = (s->divider == 0) ? 3500 : (float) s->baudbase / s->divider;
@@ -430,7 +435,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
             int break_enable;
             s->lcr = val;
             serial_update_parameters(s);
-            break_enable = (val >> 6) & 1;
+            break_enable = !!(val & UART_LCR_SB);
             if (break_enable != s->last_break_enable) {
                 s->last_break_enable = break_enable;
                 qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
@@ -676,7 +681,7 @@ static int serial_post_load(void *opaque, int version_id)
         }
     }
 
-    s->last_break_enable = (s->lcr >> 6) & 1;
+    s->last_break_enable = !!(s->lcr & UART_LCR_SB);
     /* Initialize fcr via setter to perform essential side-effects */
     serial_write_fcr(s, s->fcr_vmstate);
     serial_update_parameters(s);
-- 
2.53.0

[PATCH v3 10/14] hw/char/serial: Remove redundant reset

[Bernhard Beschow]

There is no need to invoke the reset method in realize since the reset
framework will do so anyway before the machine starts.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/serial.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 485b98f03f..0f3469a1e8 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -934,7 +934,6 @@ static void serial_realize(DeviceState *dev, Error **errp)
                              serial_event, serial_be_change, s, NULL, true);
     fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
     fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
-    serial_reset(s);
 }
 
 static void serial_unrealize(DeviceState *dev)
-- 
2.53.0

[PATCH v3 11/14] hw/char/serial: Avoid implicit conversion when tracing

[Bernhard Beschow]

On 64 bit targets, the MemoryRegion API passes an address and a value as
uint64_t, so use that for tracing. Keep the uint8_t for reading since
this is what the device model produces. On targets with less than 64
bits, uint64_t is wide enough to avoid narrowing.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/char/trace-events | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/char/trace-events b/hw/char/trace-events
index 9e74be2c14..a3fcc77287 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -5,8 +5,8 @@ parallel_ioport_read(const char *desc, uint16_t addr, uint8_t value) "read [%s]
 parallel_ioport_write(const char *desc, uint16_t addr, uint8_t value) "write [%s] addr 0x%02x val 0x%02x"
 
 # serial.c
-serial_read(uint16_t addr, uint8_t value) "read addr 0x%02x val 0x%02x"
-serial_write(uint16_t addr, uint8_t value) "write addr 0x%02x val 0x%02x"
+serial_read(uint64_t addr, uint8_t value) "[0x%02" PRIx64 "] -> 0x%02" PRIx8
+serial_write(uint64_t addr, uint64_t value) "[0x%02" PRIx64 "] <- 0x%02" PRIx64
 serial_update_parameters(uint64_t baudrate, char parity, int data_bits, int stop_bits) "baudrate=%"PRIu64" parity='%c' data=%d stop=%d"
 
 # virtio-serial-bus.c
-- 
2.53.0

[PATCH v3 12/14] hw/char/serial: Keep MemoryRegionOps private

[Bernhard Beschow]

Rather than requiring users of TYPE_SERIAL to initialize the MMIO region
themselves, make it generic enough to be configured via properties. This
makes TYPE_SERIAL more self-contained and prepares it for being turned
into a SysBusDevice.

Note that for TYPE_SERIAL_MM, if the regshift property has the default value
zero, serial_io_ops rather than serial_mm_ops is now used. This seems to be
fine since in both cases, registers are one byte wide and there are no gaps
between registers. Therefore, neither the endianness nor the access size
should make a difference. Running `make check` and `make check-functional`
seems to confirm this.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/char/serial-mm.h |  3 --
 include/hw/char/serial.h    |  4 +-
 hw/char/diva-gsp.c          |  5 ---
 hw/char/serial-isa.c        |  1 -
 hw/char/serial-mm.c         | 51 -------------------------
 hw/char/serial-pci-multi.c  |  5 ---
 hw/char/serial-pci.c        |  1 -
 hw/char/serial.c            | 76 ++++++++++++++++++++++++++++++-------
 8 files changed, 66 insertions(+), 80 deletions(-)

diff --git a/include/hw/char/serial-mm.h b/include/hw/char/serial-mm.h
index 0076bdc061..4c18e2a609 100644
--- a/include/hw/char/serial-mm.h
+++ b/include/hw/char/serial-mm.h
@@ -39,9 +39,6 @@ struct SerialMM {
     SysBusDevice parent;
 
     SerialState serial;
-
-    uint8_t regshift;
-    uint8_t endianness;
 };
 
 SerialMM *serial_mm_init(MemoryRegion *address_space,
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index ea82ffac47..0cf641a860 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -62,6 +62,9 @@ struct SerialState {
     guint watch_tag;
     bool wakeup;
 
+    uint8_t regshift;
+    uint8_t endianness;
+
     /* Time when the last byte was successfully sent out of the tsr */
     uint64_t last_xmit_ts;
     Fifo8 recv_fifo;
@@ -80,7 +83,6 @@ struct SerialState {
 };
 
 extern const VMStateDescription vmstate_serial;
-extern const MemoryRegionOps serial_io_ops;
 
 #define TYPE_SERIAL "serial"
 OBJECT_DECLARE_SIMPLE_TYPE(SerialState, SERIAL)
diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
index f9aa6e326d..b0a2437c85 100644
--- a/hw/char/diva-gsp.c
+++ b/hw/char/diva-gsp.c
@@ -47,7 +47,6 @@ typedef struct PCIDivaSerialState {
     MemoryRegion mailboxbar;    /* for hardware mailbox */
     uint32_t     subvendor;
     uint32_t     ports;
-    char         *name[PCI_SERIAL_MAX_PORTS];
     SerialState  state[PCI_SERIAL_MAX_PORTS];
     uint32_t     level[PCI_SERIAL_MAX_PORTS];
     qemu_irq     *irqs;
@@ -64,7 +63,6 @@ static void diva_pci_exit(PCIDevice *dev)
         s = pci->state + i;
         memory_region_del_subregion(&pci->membar, &s->io);
         qdev_unrealize(DEVICE(s));
-        g_free(pci->name[i]);
     }
     qemu_free_irqs(pci->irqs, pci->ports);
 }
@@ -136,9 +134,6 @@ static void diva_pci_realize(PCIDevice *dev, Error **errp)
             return;
         }
         s->irq = pci->irqs[i];
-        pci->name[i] = g_strdup_printf("uart #%zu", i + 1);
-        memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s,
-                              pci->name[i], 8);
 
         /* calculate offset of given port based on bitmask */
         while ((portmask & BIT(0)) == 0) {
diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index a4be0492c5..3a48b2495e 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -80,7 +80,6 @@ static void serial_isa_realizefn(DeviceState *dev, Error **errp)
     qdev_realize(DEVICE(s), NULL, errp);
     qdev_set_legacy_instance_id(dev, isa->iobase, 3);
 
-    memory_region_init_io(&s->io, OBJECT(isa), &serial_io_ops, s, "serial", 8);
     isa_register_ioport(isadev, &s->io, isa->iobase);
 }
 
diff --git a/hw/char/serial-mm.c b/hw/char/serial-mm.c
index 0e0be26fa9..1dba4fc694 100644
--- a/hw/char/serial-mm.c
+++ b/hw/char/serial-mm.c
@@ -30,44 +30,6 @@
 #include "qapi/error.h"
 #include "hw/core/qdev-properties.h"
 
-static uint64_t serial_mm_read(void *opaque, hwaddr addr, unsigned size)
-{
-    SerialMM *s = SERIAL_MM(opaque);
-    return serial_io_ops.read(&s->serial, addr >> s->regshift, 1);
-}
-
-static void serial_mm_write(void *opaque, hwaddr addr,
-                            uint64_t value, unsigned size)
-{
-    SerialMM *s = SERIAL_MM(opaque);
-    value &= 255;
-    serial_io_ops.write(&s->serial, addr >> s->regshift, value, 1);
-}
-
-static const MemoryRegionOps serial_mm_ops[] = {
-    [DEVICE_NATIVE_ENDIAN] = {
-        .read = serial_mm_read,
-        .write = serial_mm_write,
-        .endianness = DEVICE_NATIVE_ENDIAN,
-        .valid.max_access_size = 8,
-        .impl.max_access_size = 8,
-    },
-    [DEVICE_LITTLE_ENDIAN] = {
-        .read = serial_mm_read,
-        .write = serial_mm_write,
-        .endianness = DEVICE_LITTLE_ENDIAN,
-        .valid.max_access_size = 8,
-        .impl.max_access_size = 8,
-    },
-    [DEVICE_BIG_ENDIAN] = {
-        .read = serial_mm_read,
-        .write = serial_mm_write,
-        .endianness = DEVICE_BIG_ENDIAN,
-        .valid.max_access_size = 8,
-        .impl.max_access_size = 8,
-    },
-};
-
 static void serial_mm_realize(DeviceState *dev, Error **errp)
 {
     SerialMM *smm = SERIAL_MM(dev);
@@ -77,9 +39,6 @@ static void serial_mm_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    memory_region_init_io(&s->io, OBJECT(dev),
-                          &serial_mm_ops[smm->endianness], smm, "serial",
-                          8 << smm->regshift);
     sysbus_init_mmio(SYS_BUS_DEVICE(smm), &s->io);
     sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq);
 }
@@ -125,20 +84,10 @@ static void serial_mm_instance_init(Object *o)
     qdev_alias_all_properties(DEVICE(&smm->serial), o);
 }
 
-static const Property serial_mm_properties[] = {
-    /*
-     * Set the spacing between adjacent memory-mapped UART registers.
-     * Each register will be at (1 << regshift) bytes after the previous one.
-     */
-    DEFINE_PROP_UINT8("regshift", SerialMM, regshift, 0),
-    DEFINE_PROP_UINT8("endianness", SerialMM, endianness, DEVICE_NATIVE_ENDIAN),
-};
-
 static void serial_mm_class_init(ObjectClass *oc, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
 
-    device_class_set_props(dc, serial_mm_properties);
     dc->realize = serial_mm_realize;
     dc->vmsd = &vmstate_serial_mm;
 }
diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
index 7782452018..eda0d43bcf 100644
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -42,7 +42,6 @@ typedef struct PCIMultiSerialState {
     PCIDevice    dev;
     MemoryRegion iobar;
     uint32_t     ports;
-    char         *name[PCI_SERIAL_MAX_PORTS];
     SerialState  state[PCI_SERIAL_MAX_PORTS];
     uint32_t     level[PCI_SERIAL_MAX_PORTS];
     IRQState     irqs[PCI_SERIAL_MAX_PORTS];
@@ -58,7 +57,6 @@ static void multi_serial_pci_exit(PCIDevice *dev)
         s = pci->state + i;
         memory_region_del_subregion(&pci->iobar, &s->io);
         qdev_unrealize(DEVICE(s));
-        g_free(pci->name[i]);
     }
 }
 
@@ -108,9 +106,6 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
             return;
         }
         s->irq = &pci->irqs[i];
-        pci->name[i] = g_strdup_printf("uart #%zu", i + 1);
-        memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s,
-                              pci->name[i], 8);
         memory_region_add_subregion(&pci->iobar, 8 * i, &s->io);
         pci->ports++;
     }
diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index d8cacc9085..9a0bf2d890 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -56,7 +56,6 @@ static void serial_pci_realize(PCIDevice *dev, Error **errp)
     pci->dev.config[PCI_INTERRUPT_PIN] = 1;
     s->irq = pci_allocate_irq(&pci->dev);
 
-    memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s, "serial", 8);
     pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
 }
 
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 0f3469a1e8..49227830e1 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -921,10 +921,67 @@ static int serial_be_change(void *opaque)
     return 0;
 }
 
+static const MemoryRegionOps serial_io_ops = {
+    .read = serial_ioport_read,
+    .write = serial_ioport_write,
+    .valid = {
+        .unaligned = 1,
+    },
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static uint64_t serial_mm_read(void *opaque, hwaddr addr, unsigned size)
+{
+    SerialState *s = opaque;
+
+    return serial_ioport_read(s, addr >> s->regshift, 1);
+}
+
+static void serial_mm_write(void *opaque, hwaddr addr,
+                            uint64_t value, unsigned size)
+{
+    SerialState *s = opaque;
+
+    serial_ioport_write(s, addr >> s->regshift, value & 0xff, 1);
+}
+
+static const MemoryRegionOps serial_mm_ops[] = {
+    [DEVICE_NATIVE_ENDIAN] = {
+        .read = serial_mm_read,
+        .write = serial_mm_write,
+        .endianness = DEVICE_NATIVE_ENDIAN,
+        .valid.max_access_size = 8,
+        .impl.max_access_size = 8,
+    },
+    [DEVICE_LITTLE_ENDIAN] = {
+        .read = serial_mm_read,
+        .write = serial_mm_write,
+        .endianness = DEVICE_LITTLE_ENDIAN,
+        .valid.max_access_size = 8,
+        .impl.max_access_size = 8,
+    },
+    [DEVICE_BIG_ENDIAN] = {
+        .read = serial_mm_read,
+        .write = serial_mm_write,
+        .endianness = DEVICE_BIG_ENDIAN,
+        .valid.max_access_size = 8,
+        .impl.max_access_size = 8,
+    },
+};
+
 static void serial_realize(DeviceState *dev, Error **errp)
 {
     SerialState *s = SERIAL(dev);
 
+    memory_region_init_io(&s->io, OBJECT(s),
+                          s->regshift ? &serial_mm_ops[s->endianness]
+                                      : &serial_io_ops,
+                          s, "serial", 8 << s->regshift);
+
     s->modem_status_poll = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) serial_update_msl, s);
 
     s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
@@ -952,23 +1009,16 @@ static void serial_unrealize(DeviceState *dev)
     qemu_unregister_reset(serial_reset, s);
 }
 
-const MemoryRegionOps serial_io_ops = {
-    .read = serial_ioport_read,
-    .write = serial_ioport_write,
-    .valid = {
-        .unaligned = 1,
-    },
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 1,
-    },
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 static const Property serial_properties[] = {
     DEFINE_PROP_CHR("chardev", SerialState, chr),
     DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200),
     DEFINE_PROP_BOOL("wakeup", SerialState, wakeup, false),
+    /*
+     * Set the spacing between adjacent memory-mapped UART registers.
+     * Each register will be at (1 << regshift) bytes after the previous one.
+     */
+    DEFINE_PROP_UINT8("regshift", SerialState, regshift, 0),
+    DEFINE_PROP_UINT8("endianness", SerialState, endianness, DEVICE_NATIVE_ENDIAN),
 };
 
 static void serial_class_init(ObjectClass *klass, const void *data)
-- 
2.53.0

[PATCH v3 13/14] hw/char/serial: Inherit from SysBusDevice

[Bernhard Beschow]

SerialState currently inherits just from DeviceState and serial devices
use SerialState as an "IP block". Since DeviceState doesn't have an API
to provide MMIO regions or IRQs, all serial devices access attributes
internal to SerialState directly. Fix this by having SerialState inherit
from SysBusDevice.

In addition, DeviceState doesn't participate in the reset framework
while SysBusDevice does. This allows for implementing reset
functionality more idiomatically.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/char/serial.h   |  4 ++--
 hw/char/diva-gsp.c         | 18 +++++++++---------
 hw/char/serial-isa.c       | 16 ++++++++++------
 hw/char/serial-mm.c        |  8 ++++----
 hw/char/serial-pci-multi.c | 18 +++++++++---------
 hw/char/serial-pci.c       | 10 +++++-----
 hw/char/serial.c           |  5 ++++-
 7 files changed, 43 insertions(+), 36 deletions(-)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 0cf641a860..2ee9e5984c 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -26,7 +26,7 @@
 #ifndef HW_SERIAL_H
 #define HW_SERIAL_H
 
-#include "hw/core/qdev.h"
+#include "hw/core/sysbus.h"
 #include "chardev/char-fe.h"
 #include "system/memory.h"
 #include "qemu/fifo8.h"
@@ -35,7 +35,7 @@
 #define UART_FIFO_LENGTH    16      /* 16550A Fifo Length */
 
 struct SerialState {
-    DeviceState parent;
+    SysBusDevice parent;
 
     uint16_t divider;
     uint8_t rbr; /* receive register */
diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
index b0a2437c85..8e467f047d 100644
--- a/hw/char/diva-gsp.c
+++ b/hw/char/diva-gsp.c
@@ -56,13 +56,13 @@ typedef struct PCIDivaSerialState {
 static void diva_pci_exit(PCIDevice *dev)
 {
     PCIDivaSerialState *pci = DO_UPCAST(PCIDivaSerialState, dev, dev);
-    SerialState *s;
     int i;
 
     for (i = 0; i < pci->ports; i++) {
-        s = pci->state + i;
-        memory_region_del_subregion(&pci->membar, &s->io);
-        qdev_unrealize(DEVICE(s));
+        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
+        memory_region_del_subregion(&pci->membar,
+                                    sysbus_mmio_get_region(sbd, 0));
+        qdev_unrealize(DEVICE(sbd));
     }
     qemu_free_irqs(pci->irqs, pci->ports);
 }
@@ -116,7 +116,6 @@ static void diva_pci_realize(PCIDevice *dev, Error **errp)
 {
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
     PCIDivaSerialState *pci = DO_UPCAST(PCIDivaSerialState, dev, dev);
-    SerialState *s;
     struct diva_info di = diva_get_diva_info(pc);
     size_t i, offset = 0;
     size_t portmask = di.omask;
@@ -128,19 +127,20 @@ static void diva_pci_realize(PCIDevice *dev, Error **errp)
     pci->irqs = qemu_allocate_irqs(multi_serial_irq_mux, pci, di.nports);
 
     for (i = 0; i < di.nports; i++) {
-        s = pci->state + i;
-        if (!qdev_realize(DEVICE(s), NULL, errp)) {
+        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
+        if (!sysbus_realize(sbd, errp)) {
             diva_pci_exit(dev);
             return;
         }
-        s->irq = pci->irqs[i];
+        sysbus_connect_irq(sbd, 0, pci->irqs[i]);
 
         /* calculate offset of given port based on bitmask */
         while ((portmask & BIT(0)) == 0) {
             offset += 8;
             portmask >>= 1;
         }
-        memory_region_add_subregion(&pci->membar, offset, &s->io);
+        memory_region_add_subregion(&pci->membar, offset,
+                                    sysbus_mmio_get_region(sbd, 0));
         offset += 8;
         portmask >>= 1;
         pci->ports++;
diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index 3a48b2495e..5eb2dc6350 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -58,7 +58,7 @@ static void serial_isa_realizefn(DeviceState *dev, Error **errp)
     static int index;
     ISADevice *isadev = ISA_DEVICE(dev);
     ISASerialState *isa = ISA_SERIAL(dev);
-    SerialState *s = &isa->state;
+    SysBusDevice *sbd = SYS_BUS_DEVICE(&isa->state);
 
     if (isa->index == -1) {
         isa->index = index;
@@ -76,11 +76,11 @@ static void serial_isa_realizefn(DeviceState *dev, Error **errp)
     }
     index++;
 
-    s->irq = isa_get_irq(isadev, isa->isairq);
-    qdev_realize(DEVICE(s), NULL, errp);
+    sysbus_realize(sbd, errp);
+    sysbus_connect_irq(sbd, 0, isa_get_irq(isadev, isa->isairq));
     qdev_set_legacy_instance_id(dev, isa->iobase, 3);
 
-    isa_register_ioport(isadev, &s->io, isa->iobase);
+    isa_register_ioport(isadev, sysbus_mmio_get_region(sbd, 0), isa->iobase);
 }
 
 static void serial_isa_build_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -187,13 +187,17 @@ void serial_hds_isa_init(ISABus *bus, int from, int to)
 void isa_serial_set_iobase(ISADevice *serial, hwaddr iobase)
 {
     ISASerialState *s = ISA_SERIAL(serial);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(&s->state);
 
     serial->ioport_id = iobase;
     s->iobase = iobase;
-    memory_region_set_address(&s->state.io, s->iobase);
+    memory_region_set_address(sysbus_mmio_get_region(sbd, 0), s->iobase);
 }
 
 void isa_serial_set_enabled(ISADevice *serial, bool enabled)
 {
-    memory_region_set_enabled(&ISA_SERIAL(serial)->state.io, enabled);
+    ISASerialState *s = ISA_SERIAL(serial);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(&s->state);
+
+    memory_region_set_enabled(sysbus_mmio_get_region(sbd, 0), enabled);
 }
diff --git a/hw/char/serial-mm.c b/hw/char/serial-mm.c
index 1dba4fc694..ba4061aa69 100644
--- a/hw/char/serial-mm.c
+++ b/hw/char/serial-mm.c
@@ -33,14 +33,14 @@
 static void serial_mm_realize(DeviceState *dev, Error **errp)
 {
     SerialMM *smm = SERIAL_MM(dev);
-    SerialState *s = &smm->serial;
+    SysBusDevice *sbd = SYS_BUS_DEVICE(&smm->serial);
 
-    if (!qdev_realize(DEVICE(s), NULL, errp)) {
+    if (!sysbus_realize(sbd, errp)) {
         return;
     }
 
-    sysbus_init_mmio(SYS_BUS_DEVICE(smm), &s->io);
-    sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq);
+    sysbus_init_mmio(SYS_BUS_DEVICE(smm), sysbus_mmio_get_region(sbd, 0));
+    sysbus_pass_irq(SYS_BUS_DEVICE(smm), SYS_BUS_DEVICE(sbd));
 }
 
 static const VMStateDescription vmstate_serial_mm = {
diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
index eda0d43bcf..f5ea7f4188 100644
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -50,13 +50,13 @@ typedef struct PCIMultiSerialState {
 static void multi_serial_pci_exit(PCIDevice *dev)
 {
     PCIMultiSerialState *pci = DO_UPCAST(PCIMultiSerialState, dev, dev);
-    SerialState *s;
     int i;
 
     for (i = 0; i < pci->ports; i++) {
-        s = pci->state + i;
-        memory_region_del_subregion(&pci->iobar, &s->io);
-        qdev_unrealize(DEVICE(s));
+        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
+        memory_region_del_subregion(&pci->iobar,
+                                    sysbus_mmio_get_region(sbd, 0));
+        qdev_unrealize(DEVICE(sbd));
     }
 }
 
@@ -91,7 +91,6 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
 {
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
     PCIMultiSerialState *pci = DO_UPCAST(PCIMultiSerialState, dev, dev);
-    SerialState *s;
     size_t i, nports = multi_serial_get_port_count(pc);
 
     pci->dev.config[PCI_CLASS_PROG] = 2; /* 16550 compatible */
@@ -100,13 +99,14 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->iobar);
 
     for (i = 0; i < nports; i++) {
-        s = pci->state + i;
-        if (!qdev_realize(DEVICE(s), NULL, errp)) {
+        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
+        if (!sysbus_realize(sbd, errp)) {
             multi_serial_pci_exit(dev);
             return;
         }
-        s->irq = &pci->irqs[i];
-        memory_region_add_subregion(&pci->iobar, 8 * i, &s->io);
+        sysbus_connect_irq(sbd, 0, &pci->irqs[i]);
+        memory_region_add_subregion(&pci->iobar, 8 * i,
+                                    sysbus_mmio_get_region(sbd, 0));
         pci->ports++;
     }
 }
diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index 9a0bf2d890..b702de8219 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -46,17 +46,18 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCISerialState, PCI_SERIAL)
 static void serial_pci_realize(PCIDevice *dev, Error **errp)
 {
     PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev);
-    SerialState *s = &pci->state;
+    SysBusDevice *sbd = SYS_BUS_DEVICE(&pci->state);
 
-    if (!qdev_realize(DEVICE(s), NULL, errp)) {
+    if (!sysbus_realize(sbd, errp)) {
         return;
     }
 
     pci->dev.config[PCI_CLASS_PROG] = 2; /* 16550 compatible */
     pci->dev.config[PCI_INTERRUPT_PIN] = 1;
-    s->irq = pci_allocate_irq(&pci->dev);
+    sysbus_connect_irq(sbd, 0, pci_allocate_irq(&pci->dev));
 
-    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
+    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
+                     sysbus_mmio_get_region(sbd, 0));
 }
 
 static void serial_pci_exit(PCIDevice *dev)
@@ -65,7 +66,6 @@ static void serial_pci_exit(PCIDevice *dev)
     SerialState *s = &pci->state;
 
     qdev_unrealize(DEVICE(s));
-    qemu_free_irq(s->irq);
 }
 
 static const VMStateDescription vmstate_pci_serial = {
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 49227830e1..fc92897376 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -991,6 +991,9 @@ static void serial_realize(DeviceState *dev, Error **errp)
                              serial_event, serial_be_change, s, NULL, true);
     fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
     fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
+
+    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io);
+    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
 }
 
 static void serial_unrealize(DeviceState *dev)
@@ -1034,7 +1037,7 @@ static void serial_class_init(ObjectClass *klass, const void *data)
 
 static const TypeInfo serial_info = {
     .name = TYPE_SERIAL,
-    .parent = TYPE_DEVICE,
+    .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SerialState),
     .class_init = serial_class_init,
 };
-- 
2.53.0

[PATCH v3 14/14] hw/char/serial: Plug into reset framework

[Bernhard Beschow]

Now that SerialState inherits from SysBusDevice we can take advantage of
having the reset method be called by the bus hierarchy rather than having
to plug and unplug it manually.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/char/serial.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index fc92897376..31ce3c7b40 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -31,7 +31,6 @@
 #include "chardev/char-serial.h"
 #include "qapi/error.h"
 #include "qemu/timer.h"
-#include "system/reset.h"
 #include "system/runstate.h"
 #include "qemu/error-report.h"
 #include "trace.h"
@@ -853,9 +852,9 @@ const VMStateDescription vmstate_serial = {
     }
 };
 
-static void serial_reset(void *opaque)
+static void serial_reset(DeviceState *dev)
 {
-    SerialState *s = opaque;
+    SerialState *s = SERIAL(dev);
 
     if (s->watch_tag > 0) {
         g_source_remove(s->watch_tag);
@@ -985,7 +984,6 @@ static void serial_realize(DeviceState *dev, Error **errp)
     s->modem_status_poll = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) serial_update_msl, s);
 
     s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
-    qemu_register_reset(serial_reset, s);
 
     qemu_chr_fe_set_handlers(&s->chr, serial_can_receive1, serial_receive1,
                              serial_event, serial_be_change, s, NULL, true);
@@ -1008,8 +1006,6 @@ static void serial_unrealize(DeviceState *dev)
 
     fifo8_destroy(&s->recv_fifo);
     fifo8_destroy(&s->xmit_fifo);
-
-    qemu_unregister_reset(serial_reset, s);
 }
 
 static const Property serial_properties[] = {
@@ -1032,6 +1028,7 @@ static void serial_class_init(ObjectClass *klass, const void *data)
     dc->user_creatable = false;
     dc->realize = serial_realize;
     dc->unrealize = serial_unrealize;
+    device_class_set_legacy_reset(dc, serial_reset);
     device_class_set_props(dc, serial_properties);
 }
 
-- 
2.53.0

Re: [PATCH v3 04/14] util/fifo8: Make all read-only methods const-correct

[Philippe Mathieu-Daudé]

On 5/3/26 23:09, Bernhard Beschow wrote:
> Allows these methods to be used in const contexts, i.e. where the parent
> of the fifo itself is const. This is in particular useful for Rust code.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/qemu/fifo8.h | 10 +++++-----
>   util/fifo8.c         | 10 +++++-----
>   2 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 06/14] hw/char/serial: Prefer fifo8 methods over open-coding

[Philippe Mathieu-Daudé]

On 5/3/26 23:09, Bernhard Beschow wrote:
> Use fifo8_is_empty() and fifo8_is_full() to improve readability of the
> code.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/char/serial.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 05/14] hw/char/serial: Remove explicit cast from void pointer

[Philippe Mathieu-Daudé]

On 5/3/26 23:09, Bernhard Beschow wrote:
> A void pointer asks for being casted, so C allows for omitting the
> explicit cast. Take advantage of that.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/char/serial.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 07/14] hw/char/serial: Reuse fifo8_num_used()

[Philippe Mathieu-Daudé]

On 5/3/26 23:09, Bernhard Beschow wrote:
> Avoids accessing private fields of struct Fifo8. Now, TYPE_SERIAL only
> accesses struct Fifo8 through its methods.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/char/serial.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 12/14] hw/char/serial: Keep MemoryRegionOps private

[Bernhard Beschow]

Am 5. März 2026 22:09:09 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>Rather than requiring users of TYPE_SERIAL to initialize the MMIO region
>themselves, make it generic enough to be configured via properties. This
>makes TYPE_SERIAL more self-contained and prepares it for being turned
>into a SysBusDevice.
>
>Note that for TYPE_SERIAL_MM, if the regshift property has the default value
>zero, serial_io_ops rather than serial_mm_ops is now used. This seems to be
>fine since in both cases, registers are one byte wide and there are no gaps
>between registers. Therefore, neither the endianness nor the access size
>should make a difference.

Hmm, the access size of 1 is now actually stricter. Before, it was possible to read or write all registers at once with one 64 bit access. I wonder if this is used or useful in practice or if the access size of 8 was chosen to avoid a combinatoric explosion of MemoryRegionOps.

Should we make a bet here and fix regressions, if any, as they happen to avoid "cargo culting"?

Best regards,
Bernhard

> Running `make check` and `make check-functional`
>seems to confirm this.
>
>Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>---
> include/hw/char/serial-mm.h |  3 --
> include/hw/char/serial.h    |  4 +-
> hw/char/diva-gsp.c          |  5 ---
> hw/char/serial-isa.c        |  1 -
> hw/char/serial-mm.c         | 51 -------------------------
> hw/char/serial-pci-multi.c  |  5 ---
> hw/char/serial-pci.c        |  1 -
> hw/char/serial.c            | 76 ++++++++++++++++++++++++++++++-------
> 8 files changed, 66 insertions(+), 80 deletions(-)
>
>diff --git a/include/hw/char/serial-mm.h b/include/hw/char/serial-mm.h
>index 0076bdc061..4c18e2a609 100644
>--- a/include/hw/char/serial-mm.h
>+++ b/include/hw/char/serial-mm.h
>@@ -39,9 +39,6 @@ struct SerialMM {
>     SysBusDevice parent;
> 
>     SerialState serial;
>-
>-    uint8_t regshift;
>-    uint8_t endianness;
> };
> 
> SerialMM *serial_mm_init(MemoryRegion *address_space,
>diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
>index ea82ffac47..0cf641a860 100644
>--- a/include/hw/char/serial.h
>+++ b/include/hw/char/serial.h
>@@ -62,6 +62,9 @@ struct SerialState {
>     guint watch_tag;
>     bool wakeup;
> 
>+    uint8_t regshift;
>+    uint8_t endianness;
>+
>     /* Time when the last byte was successfully sent out of the tsr */
>     uint64_t last_xmit_ts;
>     Fifo8 recv_fifo;
>@@ -80,7 +83,6 @@ struct SerialState {
> };
> 
> extern const VMStateDescription vmstate_serial;
>-extern const MemoryRegionOps serial_io_ops;
> 
> #define TYPE_SERIAL "serial"
> OBJECT_DECLARE_SIMPLE_TYPE(SerialState, SERIAL)
>diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
>index f9aa6e326d..b0a2437c85 100644
>--- a/hw/char/diva-gsp.c
>+++ b/hw/char/diva-gsp.c
>@@ -47,7 +47,6 @@ typedef struct PCIDivaSerialState {
>     MemoryRegion mailboxbar;    /* for hardware mailbox */
>     uint32_t     subvendor;
>     uint32_t     ports;
>-    char         *name[PCI_SERIAL_MAX_PORTS];
>     SerialState  state[PCI_SERIAL_MAX_PORTS];
>     uint32_t     level[PCI_SERIAL_MAX_PORTS];
>     qemu_irq     *irqs;
>@@ -64,7 +63,6 @@ static void diva_pci_exit(PCIDevice *dev)
>         s = pci->state + i;
>         memory_region_del_subregion(&pci->membar, &s->io);
>         qdev_unrealize(DEVICE(s));
>-        g_free(pci->name[i]);
>     }
>     qemu_free_irqs(pci->irqs, pci->ports);
> }
>@@ -136,9 +134,6 @@ static void diva_pci_realize(PCIDevice *dev, Error **errp)
>             return;
>         }
>         s->irq = pci->irqs[i];
>-        pci->name[i] = g_strdup_printf("uart #%zu", i + 1);
>-        memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s,
>-                              pci->name[i], 8);
> 
>         /* calculate offset of given port based on bitmask */
>         while ((portmask & BIT(0)) == 0) {
>diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
>index a4be0492c5..3a48b2495e 100644
>--- a/hw/char/serial-isa.c
>+++ b/hw/char/serial-isa.c
>@@ -80,7 +80,6 @@ static void serial_isa_realizefn(DeviceState *dev, Error **errp)
>     qdev_realize(DEVICE(s), NULL, errp);
>     qdev_set_legacy_instance_id(dev, isa->iobase, 3);
> 
>-    memory_region_init_io(&s->io, OBJECT(isa), &serial_io_ops, s, "serial", 8);
>     isa_register_ioport(isadev, &s->io, isa->iobase);
> }
> 
>diff --git a/hw/char/serial-mm.c b/hw/char/serial-mm.c
>index 0e0be26fa9..1dba4fc694 100644
>--- a/hw/char/serial-mm.c
>+++ b/hw/char/serial-mm.c
>@@ -30,44 +30,6 @@
> #include "qapi/error.h"
> #include "hw/core/qdev-properties.h"
> 
>-static uint64_t serial_mm_read(void *opaque, hwaddr addr, unsigned size)
>-{
>-    SerialMM *s = SERIAL_MM(opaque);
>-    return serial_io_ops.read(&s->serial, addr >> s->regshift, 1);
>-}
>-
>-static void serial_mm_write(void *opaque, hwaddr addr,
>-                            uint64_t value, unsigned size)
>-{
>-    SerialMM *s = SERIAL_MM(opaque);
>-    value &= 255;
>-    serial_io_ops.write(&s->serial, addr >> s->regshift, value, 1);
>-}
>-
>-static const MemoryRegionOps serial_mm_ops[] = {
>-    [DEVICE_NATIVE_ENDIAN] = {
>-        .read = serial_mm_read,
>-        .write = serial_mm_write,
>-        .endianness = DEVICE_NATIVE_ENDIAN,
>-        .valid.max_access_size = 8,
>-        .impl.max_access_size = 8,
>-    },
>-    [DEVICE_LITTLE_ENDIAN] = {
>-        .read = serial_mm_read,
>-        .write = serial_mm_write,
>-        .endianness = DEVICE_LITTLE_ENDIAN,
>-        .valid.max_access_size = 8,
>-        .impl.max_access_size = 8,
>-    },
>-    [DEVICE_BIG_ENDIAN] = {
>-        .read = serial_mm_read,
>-        .write = serial_mm_write,
>-        .endianness = DEVICE_BIG_ENDIAN,
>-        .valid.max_access_size = 8,
>-        .impl.max_access_size = 8,
>-    },
>-};
>-
> static void serial_mm_realize(DeviceState *dev, Error **errp)
> {
>     SerialMM *smm = SERIAL_MM(dev);
>@@ -77,9 +39,6 @@ static void serial_mm_realize(DeviceState *dev, Error **errp)
>         return;
>     }
> 
>-    memory_region_init_io(&s->io, OBJECT(dev),
>-                          &serial_mm_ops[smm->endianness], smm, "serial",
>-                          8 << smm->regshift);
>     sysbus_init_mmio(SYS_BUS_DEVICE(smm), &s->io);
>     sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq);
> }
>@@ -125,20 +84,10 @@ static void serial_mm_instance_init(Object *o)
>     qdev_alias_all_properties(DEVICE(&smm->serial), o);
> }
> 
>-static const Property serial_mm_properties[] = {
>-    /*
>-     * Set the spacing between adjacent memory-mapped UART registers.
>-     * Each register will be at (1 << regshift) bytes after the previous one.
>-     */
>-    DEFINE_PROP_UINT8("regshift", SerialMM, regshift, 0),
>-    DEFINE_PROP_UINT8("endianness", SerialMM, endianness, DEVICE_NATIVE_ENDIAN),
>-};
>-
> static void serial_mm_class_init(ObjectClass *oc, const void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(oc);
> 
>-    device_class_set_props(dc, serial_mm_properties);
>     dc->realize = serial_mm_realize;
>     dc->vmsd = &vmstate_serial_mm;
> }
>diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
>index 7782452018..eda0d43bcf 100644
>--- a/hw/char/serial-pci-multi.c
>+++ b/hw/char/serial-pci-multi.c
>@@ -42,7 +42,6 @@ typedef struct PCIMultiSerialState {
>     PCIDevice    dev;
>     MemoryRegion iobar;
>     uint32_t     ports;
>-    char         *name[PCI_SERIAL_MAX_PORTS];
>     SerialState  state[PCI_SERIAL_MAX_PORTS];
>     uint32_t     level[PCI_SERIAL_MAX_PORTS];
>     IRQState     irqs[PCI_SERIAL_MAX_PORTS];
>@@ -58,7 +57,6 @@ static void multi_serial_pci_exit(PCIDevice *dev)
>         s = pci->state + i;
>         memory_region_del_subregion(&pci->iobar, &s->io);
>         qdev_unrealize(DEVICE(s));
>-        g_free(pci->name[i]);
>     }
> }
> 
>@@ -108,9 +106,6 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
>             return;
>         }
>         s->irq = &pci->irqs[i];
>-        pci->name[i] = g_strdup_printf("uart #%zu", i + 1);
>-        memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s,
>-                              pci->name[i], 8);
>         memory_region_add_subregion(&pci->iobar, 8 * i, &s->io);
>         pci->ports++;
>     }
>diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
>index d8cacc9085..9a0bf2d890 100644
>--- a/hw/char/serial-pci.c
>+++ b/hw/char/serial-pci.c
>@@ -56,7 +56,6 @@ static void serial_pci_realize(PCIDevice *dev, Error **errp)
>     pci->dev.config[PCI_INTERRUPT_PIN] = 1;
>     s->irq = pci_allocate_irq(&pci->dev);
> 
>-    memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s, "serial", 8);
>     pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
> }
> 
>diff --git a/hw/char/serial.c b/hw/char/serial.c
>index 0f3469a1e8..49227830e1 100644
>--- a/hw/char/serial.c
>+++ b/hw/char/serial.c
>@@ -921,10 +921,67 @@ static int serial_be_change(void *opaque)
>     return 0;
> }
> 
>+static const MemoryRegionOps serial_io_ops = {
>+    .read = serial_ioport_read,
>+    .write = serial_ioport_write,
>+    .valid = {
>+        .unaligned = 1,
>+    },
>+    .impl = {
>+        .min_access_size = 1,
>+        .max_access_size = 1,
>+    },
>+    .endianness = DEVICE_LITTLE_ENDIAN,
>+};
>+
>+static uint64_t serial_mm_read(void *opaque, hwaddr addr, unsigned size)
>+{
>+    SerialState *s = opaque;
>+
>+    return serial_ioport_read(s, addr >> s->regshift, 1);
>+}
>+
>+static void serial_mm_write(void *opaque, hwaddr addr,
>+                            uint64_t value, unsigned size)
>+{
>+    SerialState *s = opaque;
>+
>+    serial_ioport_write(s, addr >> s->regshift, value & 0xff, 1);
>+}
>+
>+static const MemoryRegionOps serial_mm_ops[] = {
>+    [DEVICE_NATIVE_ENDIAN] = {
>+        .read = serial_mm_read,
>+        .write = serial_mm_write,
>+        .endianness = DEVICE_NATIVE_ENDIAN,
>+        .valid.max_access_size = 8,
>+        .impl.max_access_size = 8,
>+    },
>+    [DEVICE_LITTLE_ENDIAN] = {
>+        .read = serial_mm_read,
>+        .write = serial_mm_write,
>+        .endianness = DEVICE_LITTLE_ENDIAN,
>+        .valid.max_access_size = 8,
>+        .impl.max_access_size = 8,
>+    },
>+    [DEVICE_BIG_ENDIAN] = {
>+        .read = serial_mm_read,
>+        .write = serial_mm_write,
>+        .endianness = DEVICE_BIG_ENDIAN,
>+        .valid.max_access_size = 8,
>+        .impl.max_access_size = 8,
>+    },
>+};
>+
> static void serial_realize(DeviceState *dev, Error **errp)
> {
>     SerialState *s = SERIAL(dev);
> 
>+    memory_region_init_io(&s->io, OBJECT(s),
>+                          s->regshift ? &serial_mm_ops[s->endianness]
>+                                      : &serial_io_ops,
>+                          s, "serial", 8 << s->regshift);
>+
>     s->modem_status_poll = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) serial_update_msl, s);
> 
>     s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
>@@ -952,23 +1009,16 @@ static void serial_unrealize(DeviceState *dev)
>     qemu_unregister_reset(serial_reset, s);
> }
> 
>-const MemoryRegionOps serial_io_ops = {
>-    .read = serial_ioport_read,
>-    .write = serial_ioport_write,
>-    .valid = {
>-        .unaligned = 1,
>-    },
>-    .impl = {
>-        .min_access_size = 1,
>-        .max_access_size = 1,
>-    },
>-    .endianness = DEVICE_LITTLE_ENDIAN,
>-};
>-
> static const Property serial_properties[] = {
>     DEFINE_PROP_CHR("chardev", SerialState, chr),
>     DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200),
>     DEFINE_PROP_BOOL("wakeup", SerialState, wakeup, false),
>+    /*
>+     * Set the spacing between adjacent memory-mapped UART registers.
>+     * Each register will be at (1 << regshift) bytes after the previous one.
>+     */
>+    DEFINE_PROP_UINT8("regshift", SerialState, regshift, 0),
>+    DEFINE_PROP_UINT8("endianness", SerialState, endianness, DEVICE_NATIVE_ENDIAN),
> };
> 
> static void serial_class_init(ObjectClass *klass, const void *data)

Re: [PATCH v3 09/14] hw/char/serial: Add constants for Line Control Register

[Philippe Mathieu-Daudé]

On 5/3/26 23:09, Bernhard Beschow wrote:
> Substitute some magic numbers by named constants for slightly improved
> readability.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/char/serial.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)


> @@ -153,23 +158,23 @@ static void serial_update_parameters(SerialState *s)
>   
>       /* Start bit. */
>       frame_size = 1;
> -    if (s->lcr & 0x08) {
> +    if (s->lcr & UART_LCR_PEN) {
>           /* Parity bit. */
>           frame_size++;
> -        if (s->lcr & 0x10)
> +        if (s->lcr & UART_LCR_EPS)

FYI checkpatch is complaining because you are changing code
that doesn't follow our coding style:

ERROR: braces {} are necessary for all arms of this statement
#42: FILE: hw/char/serial.c:164:
+        if (s->lcr & UART_LCR_EPS)
[...]
          else
[...]

Next time better use a preliminary commit addressing style.

>               parity = 'E';
>           else
>               parity = 'O';
>       } else {
>               parity = 'N';
>       }
> -    if (s->lcr & 0x04) {
> +    if (s->lcr & UART_LCR_NSTB) {
>           stop_bits = 2;
>       } else {
>           stop_bits = 1;
>       }

Re: [PATCH v3 00/14] TYPE_SERIAL cleanup

[Philippe Mathieu-Daudé]

On 5/3/26 23:08, Bernhard Beschow wrote:
> This series consists of cosmetic changes around TYPE_SERIAL. The biggest change
> is having it derive from TYPE_SYS_BUS_DEVICE which 1) eliminates usage of
> qemu_{un,}register_reset() and 2) stops all its users from accessing private
> data. The latter allows TYPE_SERIAL to be ported to Rust in the (near) future.


> Bernhard Beschow (14):
>    hw/arm/Kconfig: Fix serial selection for NPCM8XX
>    hw/char/{diva-gsp,serial-pci-multi}: Fix deinitialization order
>    hw/arm/aspeed_ast27x0-{ssp, tsp}: Do not access SerialMM internals
>      directly
>    util/fifo8: Make all read-only methods const-correct
>    hw/char/serial: Remove explicit cast from void pointer
>    hw/char/serial: Prefer fifo8 methods over open-coding
>    hw/char/serial: Reuse fifo8_num_used()
>    hw/char/serial: Remove unhelpful comment
>    hw/char/serial: Add constants for Line Control Register
>    hw/char/serial: Remove redundant reset
>    hw/char/serial: Avoid implicit conversion when tracing

Queueing 1-11, thanks!

>    hw/char/serial: Keep MemoryRegionOps private
>    hw/char/serial: Inherit from SysBusDevice
>    hw/char/serial: Plug into reset framework

Re: [PATCH v3 09/14] hw/char/serial: Add constants for Line Control Register

[Bernhard Beschow]

Am 8. März 2026 21:55:24 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 5/3/26 23:09, Bernhard Beschow wrote:
>> Substitute some magic numbers by named constants for slightly improved
>> readability.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/char/serial.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>
>
>> @@ -153,23 +158,23 @@ static void serial_update_parameters(SerialState *s)
>>         /* Start bit. */
>>       frame_size = 1;
>> -    if (s->lcr & 0x08) {
>> +    if (s->lcr & UART_LCR_PEN) {
>>           /* Parity bit. */
>>           frame_size++;
>> -        if (s->lcr & 0x10)
>> +        if (s->lcr & UART_LCR_EPS)
>
>FYI checkpatch is complaining because you are changing code
>that doesn't follow our coding style:
>
>ERROR: braces {} are necessary for all arms of this statement
>#42: FILE: hw/char/serial.c:164:
>+        if (s->lcr & UART_LCR_EPS)
>[...]
>         else
>[...]
>
>Next time better use a preliminary commit addressing style.

I need to check my configuration. Thanks for queueing and sorry for the inconvenience!

Best regards,
Bernhard

>
>>               parity = 'E';
>>           else
>>               parity = 'O';
>>       } else {
>>               parity = 'N';
>>       }
>> -    if (s->lcr & 0x04) {
>> +    if (s->lcr & UART_LCR_NSTB) {
>>           stop_bits = 2;
>>       } else {
>>           stop_bits = 1;
>>       }

Re: [PATCH v3 13/14] hw/char/serial: Inherit from SysBusDevice

[Paolo Bonzini]

On 3/5/26 23:09, Bernhard Beschow wrote:
> SerialState currently inherits just from DeviceState and serial devices
> use SerialState as an "IP block". Since DeviceState doesn't have an API
> to provide MMIO regions or IRQs, all serial devices access attributes
> internal to SerialState directly. Fix this by having SerialState inherit
> from SysBusDevice.
> 
> In addition, DeviceState doesn't participate in the reset framework
> while SysBusDevice does. This allows for implementing reset
> functionality more idiomatically.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

The lack of support for automatically resetting children other than 
buses is a pity indeed, but I don't think that this patch is correct.

SysBusDevice is for devices connected directly to the board; a device 
should not have any children objects connected to a bus (unless the bus 
is part of the device itself, as is the case for usb-storage for example).

Unfortunately I don't have a better solution, other than adding 
accessors to SerialState that retrieve the MMIO or connect the IRQ.

Paolo

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/char/serial.h   |  4 ++--
>   hw/char/diva-gsp.c         | 18 +++++++++---------
>   hw/char/serial-isa.c       | 16 ++++++++++------
>   hw/char/serial-mm.c        |  8 ++++----
>   hw/char/serial-pci-multi.c | 18 +++++++++---------
>   hw/char/serial-pci.c       | 10 +++++-----
>   hw/char/serial.c           |  5 ++++-
>   7 files changed, 43 insertions(+), 36 deletions(-)
> 
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 0cf641a860..2ee9e5984c 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -26,7 +26,7 @@
>   #ifndef HW_SERIAL_H
>   #define HW_SERIAL_H
>   
> -#include "hw/core/qdev.h"
> +#include "hw/core/sysbus.h"
>   #include "chardev/char-fe.h"
>   #include "system/memory.h"
>   #include "qemu/fifo8.h"
> @@ -35,7 +35,7 @@
>   #define UART_FIFO_LENGTH    16      /* 16550A Fifo Length */
>   
>   struct SerialState {
> -    DeviceState parent;
> +    SysBusDevice parent;
>   
>       uint16_t divider;
>       uint8_t rbr; /* receive register */
> diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
> index b0a2437c85..8e467f047d 100644
> --- a/hw/char/diva-gsp.c
> +++ b/hw/char/diva-gsp.c
> @@ -56,13 +56,13 @@ typedef struct PCIDivaSerialState {
>   static void diva_pci_exit(PCIDevice *dev)
>   {
>       PCIDivaSerialState *pci = DO_UPCAST(PCIDivaSerialState, dev, dev);
> -    SerialState *s;
>       int i;
>   
>       for (i = 0; i < pci->ports; i++) {
> -        s = pci->state + i;
> -        memory_region_del_subregion(&pci->membar, &s->io);
> -        qdev_unrealize(DEVICE(s));
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
> +        memory_region_del_subregion(&pci->membar,
> +                                    sysbus_mmio_get_region(sbd, 0));
> +        qdev_unrealize(DEVICE(sbd));
>       }
>       qemu_free_irqs(pci->irqs, pci->ports);
>   }
> @@ -116,7 +116,6 @@ static void diva_pci_realize(PCIDevice *dev, Error **errp)
>   {
>       PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
>       PCIDivaSerialState *pci = DO_UPCAST(PCIDivaSerialState, dev, dev);
> -    SerialState *s;
>       struct diva_info di = diva_get_diva_info(pc);
>       size_t i, offset = 0;
>       size_t portmask = di.omask;
> @@ -128,19 +127,20 @@ static void diva_pci_realize(PCIDevice *dev, Error **errp)
>       pci->irqs = qemu_allocate_irqs(multi_serial_irq_mux, pci, di.nports);
>   
>       for (i = 0; i < di.nports; i++) {
> -        s = pci->state + i;
> -        if (!qdev_realize(DEVICE(s), NULL, errp)) {
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
> +        if (!sysbus_realize(sbd, errp)) {
>               diva_pci_exit(dev);
>               return;
>           }
> -        s->irq = pci->irqs[i];
> +        sysbus_connect_irq(sbd, 0, pci->irqs[i]);
>   
>           /* calculate offset of given port based on bitmask */
>           while ((portmask & BIT(0)) == 0) {
>               offset += 8;
>               portmask >>= 1;
>           }
> -        memory_region_add_subregion(&pci->membar, offset, &s->io);
> +        memory_region_add_subregion(&pci->membar, offset,
> +                                    sysbus_mmio_get_region(sbd, 0));
>           offset += 8;
>           portmask >>= 1;
>           pci->ports++;
> diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
> index 3a48b2495e..5eb2dc6350 100644
> --- a/hw/char/serial-isa.c
> +++ b/hw/char/serial-isa.c
> @@ -58,7 +58,7 @@ static void serial_isa_realizefn(DeviceState *dev, Error **errp)
>       static int index;
>       ISADevice *isadev = ISA_DEVICE(dev);
>       ISASerialState *isa = ISA_SERIAL(dev);
> -    SerialState *s = &isa->state;
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(&isa->state);
>   
>       if (isa->index == -1) {
>           isa->index = index;
> @@ -76,11 +76,11 @@ static void serial_isa_realizefn(DeviceState *dev, Error **errp)
>       }
>       index++;
>   
> -    s->irq = isa_get_irq(isadev, isa->isairq);
> -    qdev_realize(DEVICE(s), NULL, errp);
> +    sysbus_realize(sbd, errp);
> +    sysbus_connect_irq(sbd, 0, isa_get_irq(isadev, isa->isairq));
>       qdev_set_legacy_instance_id(dev, isa->iobase, 3);
>   
> -    isa_register_ioport(isadev, &s->io, isa->iobase);
> +    isa_register_ioport(isadev, sysbus_mmio_get_region(sbd, 0), isa->iobase);
>   }
>   
>   static void serial_isa_build_aml(AcpiDevAmlIf *adev, Aml *scope)
> @@ -187,13 +187,17 @@ void serial_hds_isa_init(ISABus *bus, int from, int to)
>   void isa_serial_set_iobase(ISADevice *serial, hwaddr iobase)
>   {
>       ISASerialState *s = ISA_SERIAL(serial);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(&s->state);
>   
>       serial->ioport_id = iobase;
>       s->iobase = iobase;
> -    memory_region_set_address(&s->state.io, s->iobase);
> +    memory_region_set_address(sysbus_mmio_get_region(sbd, 0), s->iobase);
>   }
>   
>   void isa_serial_set_enabled(ISADevice *serial, bool enabled)
>   {
> -    memory_region_set_enabled(&ISA_SERIAL(serial)->state.io, enabled);
> +    ISASerialState *s = ISA_SERIAL(serial);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(&s->state);
> +
> +    memory_region_set_enabled(sysbus_mmio_get_region(sbd, 0), enabled);
>   }
> diff --git a/hw/char/serial-mm.c b/hw/char/serial-mm.c
> index 1dba4fc694..ba4061aa69 100644
> --- a/hw/char/serial-mm.c
> +++ b/hw/char/serial-mm.c
> @@ -33,14 +33,14 @@
>   static void serial_mm_realize(DeviceState *dev, Error **errp)
>   {
>       SerialMM *smm = SERIAL_MM(dev);
> -    SerialState *s = &smm->serial;
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(&smm->serial);
>   
> -    if (!qdev_realize(DEVICE(s), NULL, errp)) {
> +    if (!sysbus_realize(sbd, errp)) {
>           return;
>       }
>   
> -    sysbus_init_mmio(SYS_BUS_DEVICE(smm), &s->io);
> -    sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(smm), sysbus_mmio_get_region(sbd, 0));
> +    sysbus_pass_irq(SYS_BUS_DEVICE(smm), SYS_BUS_DEVICE(sbd));
>   }
>   
>   static const VMStateDescription vmstate_serial_mm = {
> diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
> index eda0d43bcf..f5ea7f4188 100644
> --- a/hw/char/serial-pci-multi.c
> +++ b/hw/char/serial-pci-multi.c
> @@ -50,13 +50,13 @@ typedef struct PCIMultiSerialState {
>   static void multi_serial_pci_exit(PCIDevice *dev)
>   {
>       PCIMultiSerialState *pci = DO_UPCAST(PCIMultiSerialState, dev, dev);
> -    SerialState *s;
>       int i;
>   
>       for (i = 0; i < pci->ports; i++) {
> -        s = pci->state + i;
> -        memory_region_del_subregion(&pci->iobar, &s->io);
> -        qdev_unrealize(DEVICE(s));
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
> +        memory_region_del_subregion(&pci->iobar,
> +                                    sysbus_mmio_get_region(sbd, 0));
> +        qdev_unrealize(DEVICE(sbd));
>       }
>   }
>   
> @@ -91,7 +91,6 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
>   {
>       PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
>       PCIMultiSerialState *pci = DO_UPCAST(PCIMultiSerialState, dev, dev);
> -    SerialState *s;
>       size_t i, nports = multi_serial_get_port_count(pc);
>   
>       pci->dev.config[PCI_CLASS_PROG] = 2; /* 16550 compatible */
> @@ -100,13 +99,14 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
>       pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->iobar);
>   
>       for (i = 0; i < nports; i++) {
> -        s = pci->state + i;
> -        if (!qdev_realize(DEVICE(s), NULL, errp)) {
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
> +        if (!sysbus_realize(sbd, errp)) {
>               multi_serial_pci_exit(dev);
>               return;
>           }
> -        s->irq = &pci->irqs[i];
> -        memory_region_add_subregion(&pci->iobar, 8 * i, &s->io);
> +        sysbus_connect_irq(sbd, 0, &pci->irqs[i]);
> +        memory_region_add_subregion(&pci->iobar, 8 * i,
> +                                    sysbus_mmio_get_region(sbd, 0));
>           pci->ports++;
>       }
>   }
> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
> index 9a0bf2d890..b702de8219 100644
> --- a/hw/char/serial-pci.c
> +++ b/hw/char/serial-pci.c
> @@ -46,17 +46,18 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCISerialState, PCI_SERIAL)
>   static void serial_pci_realize(PCIDevice *dev, Error **errp)
>   {
>       PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev);
> -    SerialState *s = &pci->state;
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(&pci->state);
>   
> -    if (!qdev_realize(DEVICE(s), NULL, errp)) {
> +    if (!sysbus_realize(sbd, errp)) {
>           return;
>       }
>   
>       pci->dev.config[PCI_CLASS_PROG] = 2; /* 16550 compatible */
>       pci->dev.config[PCI_INTERRUPT_PIN] = 1;
> -    s->irq = pci_allocate_irq(&pci->dev);
> +    sysbus_connect_irq(sbd, 0, pci_allocate_irq(&pci->dev));
>   
> -    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
> +    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
> +                     sysbus_mmio_get_region(sbd, 0));
>   }
>   
>   static void serial_pci_exit(PCIDevice *dev)
> @@ -65,7 +66,6 @@ static void serial_pci_exit(PCIDevice *dev)
>       SerialState *s = &pci->state;
>   
>       qdev_unrealize(DEVICE(s));
> -    qemu_free_irq(s->irq);
>   }
>   
>   static const VMStateDescription vmstate_pci_serial = {
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 49227830e1..fc92897376 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -991,6 +991,9 @@ static void serial_realize(DeviceState *dev, Error **errp)
>                                serial_event, serial_be_change, s, NULL, true);
>       fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
>       fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
> +
> +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io);
> +    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
>   }
>   
>   static void serial_unrealize(DeviceState *dev)
> @@ -1034,7 +1037,7 @@ static void serial_class_init(ObjectClass *klass, const void *data)
>   
>   static const TypeInfo serial_info = {
>       .name = TYPE_SERIAL,
> -    .parent = TYPE_DEVICE,
> +    .parent = TYPE_SYS_BUS_DEVICE,
>       .instance_size = sizeof(SerialState),
>       .class_init = serial_class_init,
>   };

Re: [PATCH v3 13/14] hw/char/serial: Inherit from SysBusDevice

[Peter Maydell]

On Mon, 9 Mar 2026 at 08:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 3/5/26 23:09, Bernhard Beschow wrote:
> > SerialState currently inherits just from DeviceState and serial devices
> > use SerialState as an "IP block". Since DeviceState doesn't have an API
> > to provide MMIO regions or IRQs, all serial devices access attributes
> > internal to SerialState directly. Fix this by having SerialState inherit
> > from SysBusDevice.
> >
> > In addition, DeviceState doesn't participate in the reset framework
> > while SysBusDevice does. This allows for implementing reset
> > functionality more idiomatically.
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> The lack of support for automatically resetting children other than
> buses is a pity indeed, but I don't think that this patch is correct.
>
> SysBusDevice is for devices connected directly to the board; a device
> should not have any children objects connected to a bus (unless the bus
> is part of the device itself, as is the case for usb-storage for example).

We also use it for various other things beyond that, though:
notably we use it extensively in SoC objects, which have
child objects which are almost always sysbus devices.
Very few devices really connect directly to a board.

I actively advise against people creating new devices that
directly inherit from TYPE_DEVICE. If you also advise against
creating devices that inherit from TYPE_SYSBUS_DEVICE that
aren't directly connected to the board, then we are jointly
saying "don't create this device" for quite a lot of devices :-)

-- PMM