[PATCH v2 00/13] TYPE_SERIAL cleanup

[PATCH v2 00/13] 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.

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:
* Make check
* 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

Bernhard Beschow (13):
  hw/arm/Kconfig: Fix serial selection for NPCM8XX
  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 stale 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 v2 01/13] 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 v2 02/13] 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.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.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

Re: [PATCH v2 02/13] hw/arm/aspeed_ast27x0-{ssp, tsp}: Do not access SerialMM internals directly

[Philippe Mathieu-Daudé]

On 3/3/26 23:21, Bernhard Beschow wrote:
> 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.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/arm/aspeed_ast27x0-ssp.c | 7 ++++---
>   hw/arm/aspeed_ast27x0-tsp.c | 7 ++++---
>   2 files changed, 8 insertions(+), 6 deletions(-)

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

[PATCH v2 03/13] 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 v2 04/13] 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 v2 05/13] 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 v2 06/13] 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 v2 07/13] hw/char/serial: Remove stale comment

[Bernhard Beschow]

There is no "is_load" flag and one can tell from the method name what
the method does. Remove this stale 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 v2 08/13] 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 v2 09/13] 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 v2 10/13] 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.

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

Re: [PATCH v2 10/13] hw/char/serial: Avoid implicit conversion when tracing

[Philippe Mathieu-Daudé]

On 3/3/26 23:21, Bernhard Beschow wrote:
> 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.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/char/trace-events | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

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

[PATCH v2 11/13] 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.

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 280d0413c6..2ea60103ea 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;
         qdev_unrealize(DEVICE(s));
         memory_region_del_subregion(&pci->membar, &s->io);
-        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 17796b93dd..38e5a78e6f 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;
         qdev_unrealize(DEVICE(s));
         memory_region_del_subregion(&pci->iobar, &s->io);
-        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

Re: [PATCH v2 11/13] hw/char/serial: Keep MemoryRegionOps private

[BALATON Zoltan]

On Tue, 3 Mar 2026, Bernhard Beschow wrote:
> 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.
>
> 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;
> };

This effectively makes TYPE_SERIAL the same as TYPE_SERIAL_MM so you could 
just merge the two and have everything embed a SERIAL_MM device. But the 
idea here is to make TYPE_SERIAL a superclass of all serial devices and 
SERIAL_MM is the memory mapped version. Memory mapped devices are usually 
SysBusDevices but others are not so this seems to be trying to model 
things in the wrong way. Maybe you could just add accessor functions to 
TYPE_SERIAL to get the memory regions and use that from subclasses to 
avoid poking in the state struct and not try to reuse SysBusDevice for 
this.

Regards,
BALATON Zoltan

Re: [PATCH v2 11/13] hw/char/serial: Keep MemoryRegionOps private

[BALATON Zoltan]

On Tue, 3 Mar 2026, BALATON Zoltan wrote:
> On Tue, 3 Mar 2026, Bernhard Beschow wrote:
>> 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.
>> 
>> 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;
>> };
>
> This effectively makes TYPE_SERIAL the same as TYPE_SERIAL_MM so you could 
> just merge the two and have everything embed a SERIAL_MM device. But the idea 
> here is to make TYPE_SERIAL a superclass of all serial devices and SERIAL_MM 
> is the memory mapped version. Memory mapped devices are usually SysBusDevices 
> but others are not so this seems to be trying to model things in the wrong 
> way. Maybe you could just add accessor functions to TYPE_SERIAL to get the 
> memory regions and use that from subclasses to avoid poking in the state 
> struct and not try to reuse SysBusDevice for this.

Or instead of TYPE_SERIAL exporting memory regions it could export ops or 
functions for it (like PCI IDE or PCI does) and let subclasses implement 
the regions that call these exported functions with the register layout 
they need?

Regards,
BALATON Zoltan

Re: [PATCH v2 11/13] hw/char/serial: Keep MemoryRegionOps private

[Bernhard Beschow]

Am 4. März 2026 01:33:34 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 3 Mar 2026, BALATON Zoltan wrote:
>> On Tue, 3 Mar 2026, Bernhard Beschow wrote:
>>> 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.
>>> 
>>> 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;
>>> };
>> 
>> This effectively makes TYPE_SERIAL the same as TYPE_SERIAL_MM so you could just merge the two and have everything embed a SERIAL_MM device. But the idea here is to make TYPE_SERIAL a superclass of all serial devices and SERIAL_MM is the memory mapped version. Memory mapped devices are usually SysBusDevices but others are not so this seems to be trying to model things in the wrong way. Maybe you could just add accessor functions to TYPE_SERIAL to get the memory regions and use that from subclasses to avoid poking in the state struct and not try to reuse SysBusDevice for this.
>
>Or instead of TYPE_SERIAL exporting memory regions it could export ops or functions for it (like PCI IDE or PCI does) and let subclasses implement the regions that call these exported functions with the register layout they need?

You mean by pushing the MemoryRegion objects into the individual users, i.e. every user would provide its own io attribute? That would certainly work and I've proposed that before (I think in the series where we made serial-isa moveable). That was criticized so I went with the current approach.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

Re: [PATCH v2 11/13] hw/char/serial: Keep MemoryRegionOps private

[Bernhard Beschow]

Am 3. März 2026 22:56:20 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 3 Mar 2026, Bernhard Beschow wrote:
>> 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.
>> 
>> 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;
>> };
>
>This effectively makes TYPE_SERIAL the same as TYPE_SERIAL_MM so you could just merge the two and have everything embed a SERIAL_MM device.

I've tried having TYPE_SERIAL_MM inherit from the SysBus-TYPE_SERIAL. This resulted in failing migration tests, so I stayed with composition.

> But the idea here is to make TYPE_SERIAL a superclass of all serial devices and SERIAL_MM is the memory mapped version. Memory mapped devices are usually SysBusDevices but others are not so this seems to be trying to model things in the wrong way. Maybe you could just add accessor functions to TYPE_SERIAL to get the memory regions and use that from subclasses to avoid poking in the state struct and not try to reuse SysBusDevice for this.

After ruling out inheritance, adding just accessor methods would basically reinvent SysBusDevice without the benefit of the reset tree.

My goal is to rewrite TYPE_SERIAL in Rust which requires it to be accessed only via methods. Not Plugging into the reset tree would be an inconvenience I'd like to avoid. The rewritten version already works except for migration. I didn't "advertise" this too much in the cover letter since these changes should be useful on their own.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

Re: [PATCH v2 11/13] hw/char/serial: Keep MemoryRegionOps private

[Peter Maydell]

On Tue, 3 Mar 2026 at 22:22, Bernhard Beschow <shentey@gmail.com> wrote:
>
> 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.
>
> 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(-)



This does leave TYPE_SERIAL_MM a bit of a do-nothing
extra wrapper, as Balaton says. However I don't think
it's being used by any versioned machine types, so we
could maybe look after this at the possibility of dropping
the wrapper (by having serial_mm_init() create a TYPE_SERIAL
directly, and dropping TYPE_SERIAL_MM entirely). I don't
think we strictly need to do that, though -- being able
to clean up the reset handling here is worthwhile in
itself.

I do have one question:

> +    memory_region_init_io(&s->io, OBJECT(s),
> +                          s->regshift ? &serial_mm_ops[s->endianness]
> +                                      : &serial_io_ops,
> +                          s, "serial", 8 << s->regshift);

This means that users of serial_mm_init() which pass 0 for
regshift now get serial_io_ops rather than serial_mm_ops.
That affects a handful of machines. I'm not sure if
this makes a guest-visible difference (very possibly not),
but for a "refactoring" type patch like this I'd rather
err on the safe side and keep the same MemoryRegionOps
if we can. This could be awkward, though, because we
definitely have users of TYPE_SERIAL_MM which rely on
the default regshift property value being 0. Maybe we
can have the handful of places that want the serial_io_ops
do an explicit setting of some property to request that?

The other approach is to analyse the code and satisfy
ourselves that serial_io_ops and serial_mm_ops with
regshift == 0 really do have exactly identical behaviour,
and note that in the commit message.

thanks
-- PMM

Re: [PATCH v2 11/13] hw/char/serial: Keep MemoryRegionOps private

[Bernhard Beschow]

Am 5. März 2026 10:03:12 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Tue, 3 Mar 2026 at 22:22, Bernhard Beschow <shentey@gmail.com> wrote:
>>
>> 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.
>>
>> 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(-)
>

Hi Peter,

>
>
>This does leave TYPE_SERIAL_MM a bit of a do-nothing
>extra wrapper, as Balaton says. However I don't think
>it's being used by any versioned machine types,

Most prominently, the riscv and loongson virt machines use it, but aren't versioned yet AFAICS.

> so we
>could maybe look after this at the possibility of dropping
>the wrapper (by having serial_mm_init() create a TYPE_SERIAL
>directly, and dropping TYPE_SERIAL_MM entirely). I don't
>think we strictly need to do that, though -- being able
>to clean up the reset handling here is worthwhile in
>itself.
>
>I do have one question:
>
>> +    memory_region_init_io(&s->io, OBJECT(s),
>> +                          s->regshift ? &serial_mm_ops[s->endianness]
>> +                                      : &serial_io_ops,
>> +                          s, "serial", 8 << s->regshift);
>
>This means that users of serial_mm_init() which pass 0 for
>regshift now get serial_io_ops rather than serial_mm_ops.

This is correct.

>That affects a handful of machines. I'm not sure if
>this makes a guest-visible difference (very possibly not),
>but for a "refactoring" type patch like this I'd rather
>err on the safe side and keep the same MemoryRegionOps
>if we can. This could be awkward, though, because we
>definitely have users of TYPE_SERIAL_MM which rely on
>the default regshift property value being 0. Maybe we
>can have the handful of places that want the serial_io_ops
>do an explicit setting of some property to request that?
>
>The other approach is to analyse the code and satisfy
>ourselves that serial_io_ops and serial_mm_ops with
>regshift == 0 really do have exactly identical behaviour,
>and note that in the commit message.

I've already sent v3 to fix the lifetime issue you pointed out and attempted to take the last approach from above. But then realized that a justification in the commit message isn't as straight forward as anticipated. I've left some thoughts there...

Best regards,
Bernhard

>
>thanks
>-- PMM

[PATCH v2 12/13] 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.

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 2ea60103ea..55c42effe9 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;
-        qdev_unrealize(DEVICE(s));
-        memory_region_del_subregion(&pci->membar, &s->io);
+        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
+        qdev_unrealize(DEVICE(sbd));
+        memory_region_del_subregion(&pci->membar,
+                                    sysbus_mmio_get_region(sbd, 0));
     }
     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 38e5a78e6f..4e51d14111 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;
-        qdev_unrealize(DEVICE(s));
-        memory_region_del_subregion(&pci->iobar, &s->io);
+        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
+        qdev_unrealize(DEVICE(sbd));
+        memory_region_del_subregion(&pci->iobar,
+                                    sysbus_mmio_get_region(sbd, 0));
     }
 }
 
@@ -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

QOM parent type "sys-bus-device" vs. "device" (was: [PATCH v2 12/13] hw/char/serial: Inherit from SysBusDevice)

[Markus Armbruster]

Bernhard Beschow <shentey@gmail.com> writes:

> 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.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

We might be at a crossroads here.

Paul Brook designed qdev around "a device plugs into some bus, and may
provide buses".  To make this work, he added a main system bus to serve
as root of a single tree.

The parent of a device in that tree is the bus it plugs into, and the
parent of a bus is the device that provides it, except for the main
system bus, which has no parent.  "info qtree" shows this tree.

Note a machine had just this one system bus back then.

The system bus isn't something hardware people would call a bus.

The qtree is a qdev thing.  It is *not* the QOM composition tree ("info
qom-tree /").

We actually abandoned "a device plugs into some bus" a long time ago.
Moe on that below.

Qdev was later rebased onto QOM, as follows.

A bus QOM type is a subtype of "bus".

A device QOM type is a subtype of "device".

If a device type has a @bus_type, then it plugs into a bus of that QOM
type.  Such a concrete device is commonly[*] not a direct subtype of
"device".  Instead, it's a subtype of the abstract device that plugs
into that bus type.  Example: "isa-serial" is a direct subtype of
"isa-device", and it inherits its bus type "ISA" from there.

If a device has no @bus_type, it doesn't plug into any bus.  It doesn't
show up in "info qtree" (it does in "info qom-tree", of course).
Example: "serial" is a direct subtype of "device".

This lets us build devices from components without having to make up
buses to connect them.  Example: "isa-serial" has a direct QOM child of
type "serial".  Good!

Except infrastructure useful to such bus-less devices lives in sysbus,
where bus-less devices can't access it.  This tempts us to make devices
sysbus devices just to be able to use the infrastructure, and to create
additional sysbuses.

We can elect to go down this path further: make more sysbus devices.
Create more sysbuses.  If this is the right path, then bus-less devices
were a mistake, and we should consider eliminating them.

Or we can pick the other branch: put the infrastructure where bus-less
devices can use it.  If this is the right path, then sysbus is
obsolescent.

Or we can elect to muddle on without giving it much thought.  Always an
option :)


[...]


[*] Are there any exceptions?  I don't know.

Re: QOM parent type "sys-bus-device" vs. "device" (was: [PATCH v2 12/13] hw/char/serial: Inherit from SysBusDevice)

[Peter Maydell]

On Wed, 4 Mar 2026 at 12:25, Markus Armbruster <armbru@redhat.com> wrote:
> We might be at a crossroads here.
>
> Paul Brook designed qdev around "a device plugs into some bus, and may
> provide buses".  To make this work, he added a main system bus to serve
> as root of a single tree.
>
> The parent of a device in that tree is the bus it plugs into, and the
> parent of a bus is the device that provides it, except for the main
> system bus, which has no parent.  "info qtree" shows this tree.
>
> Note a machine had just this one system bus back then.
>
> The system bus isn't something hardware people would call a bus.
>
> The qtree is a qdev thing.  It is *not* the QOM composition tree ("info
> qom-tree /").
>
> We actually abandoned "a device plugs into some bus" a long time ago.
> Moe on that below.
>
> Qdev was later rebased onto QOM, as follows.
>
> A bus QOM type is a subtype of "bus".
>
> A device QOM type is a subtype of "device".
>
> If a device type has a @bus_type, then it plugs into a bus of that QOM
> type.  Such a concrete device is commonly[*] not a direct subtype of
> "device".  Instead, it's a subtype of the abstract device that plugs
> into that bus type.  Example: "isa-serial" is a direct subtype of
> "isa-device", and it inherits its bus type "ISA" from there.
>
> If a device has no @bus_type, it doesn't plug into any bus.  It doesn't
> show up in "info qtree" (it does in "info qom-tree", of course).
> Example: "serial" is a direct subtype of "device".
>
> This lets us build devices from components without having to make up
> buses to connect them.  Example: "isa-serial" has a direct QOM child of
> type "serial".  Good!
>
> Except infrastructure useful to such bus-less devices lives in sysbus,
> where bus-less devices can't access it.  This tempts us to make devices
> sysbus devices just to be able to use the infrastructure, and to create
> additional sysbuses.

There is only one sysbus -- it is impossible to create more than one,
and there wouldn't be much point in creating a second one anyway.

> We can elect to go down this path further: make more sysbus devices.
> Create more sysbuses.  If this is the right path, then bus-less devices
> were a mistake, and we should consider eliminating them.
>
> Or we can pick the other branch: put the infrastructure where bus-less
> devices can use it.  If this is the right path, then sysbus is
> obsolescent.

I do think that at some point it would be good to move all that
stuff into the base "device" class, and drop sysbus. The real
problem, though, is reset. Currently reset propagates along the
qtree (the tree of buses). So something (like TYPE_SERIAL, and
also like CPU objects) that is a direct child of TYPE_DEVICE does
not get automatically reset. The major overriding reason why
(as it currently stands) I favour making things sysbus devices
and not plain devices is that devices that don't get reset are
either broken or need all their users to take extra steps that
most don't take, or do in a weird way. We can't just push the
reset handling into TYPE_DEVICE because by definition it is tied
to "being a thing on a bus, i.e. the sysbus".

Disentangling reset is extremely painful -- I don't even know
what it ought to be doing. Maybe propagating along the QOM tree?
So for now, we muddle along.

-- PMM

Re: QOM parent type "sys-bus-device" vs. "device" (was: [PATCH v2 12/13] hw/char/serial: Inherit from SysBusDevice)

[BALATON Zoltan]

On Wed, 4 Mar 2026, Peter Maydell wrote:
> On Wed, 4 Mar 2026 at 12:25, Markus Armbruster <armbru@redhat.com> wrote:
>> We might be at a crossroads here.
>>
>> Paul Brook designed qdev around "a device plugs into some bus, and may
>> provide buses".  To make this work, he added a main system bus to serve
>> as root of a single tree.
>>
>> The parent of a device in that tree is the bus it plugs into, and the
>> parent of a bus is the device that provides it, except for the main
>> system bus, which has no parent.  "info qtree" shows this tree.
>>
>> Note a machine had just this one system bus back then.
>>
>> The system bus isn't something hardware people would call a bus.
>>
>> The qtree is a qdev thing.  It is *not* the QOM composition tree ("info
>> qom-tree /").
>>
>> We actually abandoned "a device plugs into some bus" a long time ago.
>> Moe on that below.
>>
>> Qdev was later rebased onto QOM, as follows.
>>
>> A bus QOM type is a subtype of "bus".
>>
>> A device QOM type is a subtype of "device".
>>
>> If a device type has a @bus_type, then it plugs into a bus of that QOM
>> type.  Such a concrete device is commonly[*] not a direct subtype of
>> "device".  Instead, it's a subtype of the abstract device that plugs
>> into that bus type.  Example: "isa-serial" is a direct subtype of
>> "isa-device", and it inherits its bus type "ISA" from there.
>>
>> If a device has no @bus_type, it doesn't plug into any bus.  It doesn't
>> show up in "info qtree" (it does in "info qom-tree", of course).
>> Example: "serial" is a direct subtype of "device".
>>
>> This lets us build devices from components without having to make up
>> buses to connect them.  Example: "isa-serial" has a direct QOM child of
>> type "serial".  Good!
>>
>> Except infrastructure useful to such bus-less devices lives in sysbus,
>> where bus-less devices can't access it.  This tempts us to make devices
>> sysbus devices just to be able to use the infrastructure, and to create
>> additional sysbuses.
>
> There is only one sysbus -- it is impossible to create more than one,
> and there wouldn't be much point in creating a second one anyway.
>
>> We can elect to go down this path further: make more sysbus devices.
>> Create more sysbuses.  If this is the right path, then bus-less devices
>> were a mistake, and we should consider eliminating them.
>>
>> Or we can pick the other branch: put the infrastructure where bus-less
>> devices can use it.  If this is the right path, then sysbus is
>> obsolescent.
>
> I do think that at some point it would be good to move all that
> stuff into the base "device" class, and drop sysbus. The real
> problem, though, is reset. Currently reset propagates along the
> qtree (the tree of buses). So something (like TYPE_SERIAL, and
> also like CPU objects) that is a direct child of TYPE_DEVICE does
> not get automatically reset. The major overriding reason why
> (as it currently stands) I favour making things sysbus devices
> and not plain devices is that devices that don't get reset are
> either broken or need all their users to take extra steps that
> most don't take, or do in a weird way. We can't just push the
> reset handling into TYPE_DEVICE because by definition it is tied
> to "being a thing on a bus, i.e. the sysbus".
>
> Disentangling reset is extremely painful -- I don't even know
> what it ought to be doing. Maybe propagating along the QOM tree?
> So for now, we muddle along.

This suggests maybe we need a way to represent qtree in QOM. So while QOM 
devices are organised by parent-child relationship in the qom-tree they 
could have standardised properties that tell which bus they are connected 
to (or buses have properties that list devices connected to them, 
whichever is easier to walk) then the reset or info qtree could select 
devices based on these properties. Could that replace the qtree of sysbus 
devices and thus allow merging SysBusDevice into Device? There are link 
properties in QOM to connect objects so it should be possible to represent 
object relationships with such properties not just by their parent-child 
(which I think are also represented by properties but I did not check it).

Regards,
BALATON Zoltan

Re: QOM parent type "sys-bus-device" vs. "device"

[Markus Armbruster via]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 4 Mar 2026 at 12:25, Markus Armbruster <armbru@redhat.com> wrote:
>> We might be at a crossroads here.
>>
>> Paul Brook designed qdev around "a device plugs into some bus, and may
>> provide buses".  To make this work, he added a main system bus to serve
>> as root of a single tree.
>>
>> The parent of a device in that tree is the bus it plugs into, and the
>> parent of a bus is the device that provides it, except for the main
>> system bus, which has no parent.  "info qtree" shows this tree.
>>
>> Note a machine had just this one system bus back then.
>>
>> The system bus isn't something hardware people would call a bus.
>>
>> The qtree is a qdev thing.  It is *not* the QOM composition tree ("info
>> qom-tree /").
>>
>> We actually abandoned "a device plugs into some bus" a long time ago.
>> Moe on that below.
>>
>> Qdev was later rebased onto QOM, as follows.
>>
>> A bus QOM type is a subtype of "bus".
>>
>> A device QOM type is a subtype of "device".
>>
>> If a device type has a @bus_type, then it plugs into a bus of that QOM
>> type.  Such a concrete device is commonly[*] not a direct subtype of
>> "device".  Instead, it's a subtype of the abstract device that plugs
>> into that bus type.  Example: "isa-serial" is a direct subtype of
>> "isa-device", and it inherits its bus type "ISA" from there.
>>
>> If a device has no @bus_type, it doesn't plug into any bus.  It doesn't
>> show up in "info qtree" (it does in "info qom-tree", of course).
>> Example: "serial" is a direct subtype of "device".
>>
>> This lets us build devices from components without having to make up
>> buses to connect them.  Example: "isa-serial" has a direct QOM child of
>> type "serial".  Good!
>>
>> Except infrastructure useful to such bus-less devices lives in sysbus,
>> where bus-less devices can't access it.  This tempts us to make devices
>> sysbus devices just to be able to use the infrastructure, and to create
>> additional sysbuses.
>
> There is only one sysbus -- it is impossible to create more than one,
> and there wouldn't be much point in creating a second one anyway.

I was almost entirely mistaken there.  However, you're not quite right,
either :)

We *can* create more than one system bus, and we in fact do: abstract
PCI device "macio" contains a "macio-bus", which is a subtype of the
system bus type.  Its concrete subtypes are "macio-oldworld" and
"macio-newworld".  To see the "macio-bus" in "info qtree", try machine
"g3beige".

But this is the rare exception.  The rule is us doing something
differently undesirable.

Consider ARM machine "ast2700fc".  Its core are three devices:
"ast2700-a2", "aspeed27x0ssp-coprocessor", and
"aspeed27x0tsp-coprocessor".  All three contain component devices.  The
QOM composition tree shows them as "/machine/ca35", "/machine/ssp", and
"/machine/tsp", i.e. they are direct children of the machine object.

Here's a closer look at a small part of the composition tree:

    /machine (ast2700fc-machine)
      [...]
      /ca35 (ast2700-a2)
        [...]
        /ioexp[0] (aspeed.ast1700)
          [...]
          /uart (serial-mm)
            /serial (serial)
            /serial[0] (memory-region)

I'll refer back to this later.

A few components end up in the "/machine/unattached" orphanage instead
due to sloppy modelling.

My point is: the QOM composition tree actually reflects how devices are
built from components, except for the parts where we got sloppy, which
should be fixable.

The qtree (the thing shown by "info qtree") shows a different picture.
The three core devices are bus-less, and therefore not in the qtree.
Their components, however, are mostly sysbus devices, and therefore in
the qtree right under the root.  Here are the parts of the qtree that
correspond to the QOM composition tree snippet above:

bus: main-system-bus
  type System
  [...]
  dev: serial-mm, id ""
    gpio-out "sysbus-irq" 1
    regshift = 2 (0x2)
    endianness = 2 (0x2)
    mmio ffffffffffffffff/0000000000000020
  dev: aspeed.ast1700, id ""
    board-idx = 0 (0x0)
    silicon-rev = 100794627 (0x6020103)
    dram = "/machine/ca35/ca35-dram[0]"
    mmio 0000000030000000/0000000001000000

The "aspeed.ast1700" and its "serial-mm" component are siblings.  The
latter's "serial" component is bus-less, and therefore not in the qtree.
Bernhard's patches make it a sysbus device instead, and therefore add it
to the qtree as another sibling.

My point is: the qtree does not reflect how devices are built from
components / are wired together, at least not where sysbus devices are
involved.

>> We can elect to go down this path further: make more sysbus devices.
>> Create more sysbuses.  If this is the right path, then bus-less devices
>> were a mistake, and we should consider eliminating them.
>>
>> Or we can pick the other branch: put the infrastructure where bus-less
>> devices can use it.  If this is the right path, then sysbus is
>> obsolescent.
>
> I do think that at some point it would be good to move all that
> stuff into the base "device" class, and drop sysbus. The real
> problem, though, is reset. Currently reset propagates along the
> qtree (the tree of buses).

Unfortunately, the qtree is far removed from reality for sysbus devices.

Case in point: "aspeed.ast1700" and its component "serial-mm" are
siblings in the qtree.  In what order will they be reset?

Physical hardware has reset lines.  Devices generally wire up their
components' reset pins to their own.  I believe the sane way to model
this is a reset tree.

>                            So something (like TYPE_SERIAL, and
> also like CPU objects) that is a direct child of TYPE_DEVICE does
> not get automatically reset. The major overriding reason why
> (as it currently stands) I favour making things sysbus devices
> and not plain devices is that devices that don't get reset are
> either broken or need all their users to take extra steps that
> most don't take, or do in a weird way. We can't just push the
> reset handling into TYPE_DEVICE because by definition it is tied
> to "being a thing on a bus, i.e. the sysbus".
>
> Disentangling reset is extremely painful -- I don't even know
> what it ought to be doing. Maybe propagating along the QOM tree?

I think this would make more sense.  Unlike the qtree, the QOM
composition tree contains all devices, and reflects the actual
composition.  It's certainly closer to the real reset tree than the
qtree is.  Is it close enough?  I don't know.  If yes, there's our reset
tree.  If no, I guess we could still use it as a base, with manually
corrected reset lines where the QOM composition tree is off.

> So for now, we muddle along.
>
> -- PMM

Re: QOM parent type "sys-bus-device" vs. "device"

[Markus Armbruster via qemu development]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 4 Mar 2026 at 12:25, Markus Armbruster <armbru@redhat.com> wrote:
>> We might be at a crossroads here.
>>
>> Paul Brook designed qdev around "a device plugs into some bus, and may
>> provide buses".  To make this work, he added a main system bus to serve
>> as root of a single tree.
>>
>> The parent of a device in that tree is the bus it plugs into, and the
>> parent of a bus is the device that provides it, except for the main
>> system bus, which has no parent.  "info qtree" shows this tree.
>>
>> Note a machine had just this one system bus back then.
>>
>> The system bus isn't something hardware people would call a bus.
>>
>> The qtree is a qdev thing.  It is *not* the QOM composition tree ("info
>> qom-tree /").
>>
>> We actually abandoned "a device plugs into some bus" a long time ago.
>> Moe on that below.
>>
>> Qdev was later rebased onto QOM, as follows.
>>
>> A bus QOM type is a subtype of "bus".
>>
>> A device QOM type is a subtype of "device".
>>
>> If a device type has a @bus_type, then it plugs into a bus of that QOM
>> type.  Such a concrete device is commonly[*] not a direct subtype of
>> "device".  Instead, it's a subtype of the abstract device that plugs
>> into that bus type.  Example: "isa-serial" is a direct subtype of
>> "isa-device", and it inherits its bus type "ISA" from there.
>>
>> If a device has no @bus_type, it doesn't plug into any bus.  It doesn't
>> show up in "info qtree" (it does in "info qom-tree", of course).
>> Example: "serial" is a direct subtype of "device".
>>
>> This lets us build devices from components without having to make up
>> buses to connect them.  Example: "isa-serial" has a direct QOM child of
>> type "serial".  Good!
>>
>> Except infrastructure useful to such bus-less devices lives in sysbus,
>> where bus-less devices can't access it.  This tempts us to make devices
>> sysbus devices just to be able to use the infrastructure, and to create
>> additional sysbuses.
>
> There is only one sysbus -- it is impossible to create more than one,
> and there wouldn't be much point in creating a second one anyway.

I was almost entirely mistaken there.  However, you're not quite right,
either :)

We *can* create more than one system bus, and we in fact do: abstract
PCI device "macio" contains a "macio-bus", which is a subtype of the
system bus type.  Its concrete subtypes are "macio-oldworld" and
"macio-newworld".  To see the "macio-bus" in "info qtree", try machine
"g3beige".

But this is the rare exception.  The rule is us doing something
differently undesirable.

Consider ARM machine "ast2700fc".  Its core are three devices:
"ast2700-a2", "aspeed27x0ssp-coprocessor", and
"aspeed27x0tsp-coprocessor".  All three contain component devices.  The
QOM composition tree shows them as "/machine/ca35", "/machine/ssp", and
"/machine/tsp", i.e. they are direct children of the machine object.

Here's a closer look at a small part of the composition tree:

    /machine (ast2700fc-machine)
      [...]
      /ca35 (ast2700-a2)
        [...]
        /ioexp[0] (aspeed.ast1700)
          [...]
          /uart (serial-mm)
            /serial (serial)
            /serial[0] (memory-region)

I'll refer back to this later.

A few components end up in the "/machine/unattached" orphanage instead
due to sloppy modelling.

My point is: the QOM composition tree actually reflects how devices are
built from components, except for the parts where we got sloppy, which
should be fixable.

The qtree (the thing shown by "info qtree") shows a different picture.
The three core devices are bus-less, and therefore not in the qtree.
Their components, however, are mostly sysbus devices, and therefore in
the qtree right under the root.  Here are the parts of the qtree that
correspond to the QOM composition tree snippet above:

bus: main-system-bus
  type System
  [...]
  dev: serial-mm, id ""
    gpio-out "sysbus-irq" 1
    regshift = 2 (0x2)
    endianness = 2 (0x2)
    mmio ffffffffffffffff/0000000000000020
  dev: aspeed.ast1700, id ""
    board-idx = 0 (0x0)
    silicon-rev = 100794627 (0x6020103)
    dram = "/machine/ca35/ca35-dram[0]"
    mmio 0000000030000000/0000000001000000

The "aspeed.ast1700" and its "serial-mm" component are siblings.  The
latter's "serial" component is bus-less, and therefore not in the qtree.
Bernhard's patches make it a sysbus device instead, and therefore add it
to the qtree as another sibling.

My point is: the qtree does not reflect how devices are built from
components / are wired together, at least not where sysbus devices are
involved.

>> We can elect to go down this path further: make more sysbus devices.
>> Create more sysbuses.  If this is the right path, then bus-less devices
>> were a mistake, and we should consider eliminating them.
>>
>> Or we can pick the other branch: put the infrastructure where bus-less
>> devices can use it.  If this is the right path, then sysbus is
>> obsolescent.
>
> I do think that at some point it would be good to move all that
> stuff into the base "device" class, and drop sysbus. The real
> problem, though, is reset. Currently reset propagates along the
> qtree (the tree of buses).

Unfortunately, the qtree is far removed from reality for sysbus devices.

Case in point: "aspeed.ast1700" and its component "serial-mm" are
siblings in the qtree.  In what order will they be reset?

Physical hardware has reset lines.  Devices generally wire up their
components' reset pins to their own.  I believe the sane way to model
this is a reset tree.

>                            So something (like TYPE_SERIAL, and
> also like CPU objects) that is a direct child of TYPE_DEVICE does
> not get automatically reset. The major overriding reason why
> (as it currently stands) I favour making things sysbus devices
> and not plain devices is that devices that don't get reset are
> either broken or need all their users to take extra steps that
> most don't take, or do in a weird way. We can't just push the
> reset handling into TYPE_DEVICE because by definition it is tied
> to "being a thing on a bus, i.e. the sysbus".
>
> Disentangling reset is extremely painful -- I don't even know
> what it ought to be doing. Maybe propagating along the QOM tree?

I think this would make more sense.  Unlike the qtree, the QOM
composition tree contains all devices, and reflects the actual
composition.  It's certainly closer to the real reset tree than the
qtree is.  Is it close enough?  I don't know.  If yes, there's our reset
tree.  If no, I guess we could still use it as a base, with manually
corrected reset lines where the QOM composition tree is off.

> So for now, we muddle along.
>
> -- PMM

Re: QOM parent type "sys-bus-device" vs. "device"

[Peter Maydell]

On Thu, 5 Mar 2026 at 09:25, Markus Armbruster <armbru@redhat.com> wrote:
> > Disentangling reset is extremely painful -- I don't even know
> > what it ought to be doing. Maybe propagating along the QOM tree?
>
> I think this would make more sense.  Unlike the qtree, the QOM
> composition tree contains all devices, and reflects the actual
> composition.  It's certainly closer to the real reset tree than the
> qtree is.  Is it close enough?  I don't know.  If yes, there's our reset
> tree.  If no, I guess we could still use it as a base, with manually
> corrected reset lines where the QOM composition tree is off.

Yes. We really don't want to add more boilerplate requirements
to how you write "container" type devices like SoC objects.
It's already bad enough that you have to have an instance_init
that manually calls instance_init on all your subcomponents,
and a realize that calls realize on all of them. If we add
a requirement that you need to have reset methods for 3 phases
that call reset on all your subcomponents, people are going to
forget. We need the default to be "do the thing that's right
almost every time", not "do the thing that's wrong".

(And then of course there is the "how do we get there from
here?" question :-))

thanks
-- PMM

Re: QOM parent type "sys-bus-device" vs. "device"

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 5 Mar 2026 at 09:25, Markus Armbruster <armbru@redhat.com> wrote:
>> > Disentangling reset is extremely painful -- I don't even know
>> > what it ought to be doing. Maybe propagating along the QOM tree?
>>
>> I think this would make more sense.  Unlike the qtree, the QOM
>> composition tree contains all devices, and reflects the actual
>> composition.  It's certainly closer to the real reset tree than the
>> qtree is.  Is it close enough?  I don't know.  If yes, there's our reset
>> tree.  If no, I guess we could still use it as a base, with manually
>> corrected reset lines where the QOM composition tree is off.

A walk to the farmers market jogged my memory: the QOM composition tree
is in fact off for user-created devices.  These go into
/machine/peripheral and /machine/peripheral-anon.

Most of them plug into a bus, and reset should flow through that bus,
not through their peripheral container.

A few don't, and I can't say how reset is supposed to work then.

Onboard device can also be connected via some bus.  For instance,
machine "pc" contains sysbus device "i440FX-pcihost", which provides a
PCI bus.  It also contains PCI device "PIIX3" (the south bridge)
connected via that PCI bus.

The QOM composition tree has "i440FX-pcihost" and its PCI bus in the
right place, and "PIIX3" in the orphanage:

    /machine (pc-i440fx-11.0-machine)
      /i440fx (i440FX-pcihost)
        /pci.0 (PCI)
      /unattached (container)
        /device[3] (PIIX3)
        /sysbus (System)

Say we fixed that like so

    /machine (pc-i440fx-11.0-machine)
      /i440fx (i440FX-pcihost)
        /pci.0 (PCI)
        /piix3 (PIIX3)

then reset flowing along the composition tree is still problematic: we
probably want to reset the PCI bus before the devices plugged into it.

So maybe reset should flow along the composition tree to buses, from bus
to devices plugged into it, from such a device again along the
composition tree, and so forth.

> Yes. We really don't want to add more boilerplate requirements
> to how you write "container" type devices like SoC objects.
> It's already bad enough that you have to have an instance_init
> that manually calls instance_init on all your subcomponents,
> and a realize that calls realize on all of them. If we add
> a requirement that you need to have reset methods for 3 phases
> that call reset on all your subcomponents, people are going to
> forget. We need the default to be "do the thing that's right
> almost every time", not "do the thing that's wrong".

Good interfaces make doing the right thing easier than doing the wrong
thing.  We clearly failed there.

> (And then of course there is the "how do we get there from
> here?" question :-))

Good question.  Next question?  SCNR!

I'm afraid I don't understand how reset works *now* well enough to come
up with a workable plan.

In other words, I don't have a clear idea of "here", and I fear we
together don't have a sufficiently clear idea of "there".

What can we do to improve on both?

Re: [PATCH v2 12/13] hw/char/serial: Inherit from SysBusDevice

[Peter Maydell]

On Tue, 3 Mar 2026 at 22:22, Bernhard Beschow <shentey@gmail.com> 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.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>



> diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
> index 2ea60103ea..55c42effe9 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;
> -        qdev_unrealize(DEVICE(s));
> -        memory_region_del_subregion(&pci->membar, &s->io);
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
> +        qdev_unrealize(DEVICE(sbd));
> +        memory_region_del_subregion(&pci->membar,
> +                                    sysbus_mmio_get_region(sbd, 0));
>      }
>      qemu_free_irqs(pci->irqs, pci->ports);
>  }

The ordering here looks a little odd -- is it really OK to
keep using sbd (passing it to sysbus_mmio_get_region()) after
we have unrealized the device ? Remove the region from the membar
first and then unrealize would seem a more natural order.
Perhaps the MR refcounts here save us, though.

> diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
> index 38e5a78e6f..4e51d14111 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;
> -        qdev_unrealize(DEVICE(s));
> -        memory_region_del_subregion(&pci->iobar, &s->io);
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
> +        qdev_unrealize(DEVICE(sbd));
> +        memory_region_del_subregion(&pci->iobar,
> +                                    sysbus_mmio_get_region(sbd, 0));
>      }
>  }

Similarly here.

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

thanks
-- PMM

Re: [PATCH v2 12/13] hw/char/serial: Inherit from SysBusDevice

[Bernhard Beschow]

Am 5. März 2026 10:07:23 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Tue, 3 Mar 2026 at 22:22, Bernhard Beschow <shentey@gmail.com> 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.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>
>
>
>> diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
>> index 2ea60103ea..55c42effe9 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;
>> -        qdev_unrealize(DEVICE(s));
>> -        memory_region_del_subregion(&pci->membar, &s->io);
>> +        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
>> +        qdev_unrealize(DEVICE(sbd));
>> +        memory_region_del_subregion(&pci->membar,
>> +                                    sysbus_mmio_get_region(sbd, 0));
>>      }
>>      qemu_free_irqs(pci->irqs, pci->ports);
>>  }
>
>The ordering here looks a little odd -- is it really OK to
>keep using sbd (passing it to sysbus_mmio_get_region()) after
>we have unrealized the device ? Remove the region from the membar
>first and then unrealize would seem a more natural order.

Indeed. Will add a patch before this one.

Thanks,
Bermhard

>Perhaps the MR refcounts here save us, though.
>
>> diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
>> index 38e5a78e6f..4e51d14111 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;
>> -        qdev_unrealize(DEVICE(s));
>> -        memory_region_del_subregion(&pci->iobar, &s->io);
>> +        SysBusDevice *sbd = SYS_BUS_DEVICE(pci->state + i);
>> +        qdev_unrealize(DEVICE(sbd));
>> +        memory_region_del_subregion(&pci->iobar,
>> +                                    sysbus_mmio_get_region(sbd, 0));
>>      }
>>  }
>
>Similarly here.
>
>Otherwise
>Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
>thanks
>-- PMM

[PATCH v2 13/13] hw/char/serial: Plug into reset framework

[Bernhard Beschow]

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

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 v2 13/13] hw/char/serial: Plug into reset framework

[Peter Maydell]

On Tue, 3 Mar 2026 at 22:22, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Now that SerialState inherits from SysBusDevice we can take advantage of
> having the reset method called by the bus hierarchy rather than having
> to plug and unplug it manually.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/char/serial.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>

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

thanks
-- PMM