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.0On 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
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.