Re: [PATCH v2 11/13] hw/char/serial: Keep MemoryRegionOps private
Am 5. März 2026 10:03:12 UTC schrieb Peter Maydell : >On Tue, 3 Mar 2026 at 22:22, 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 >> --- >> 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
Re: [PATCH v2 11/13] hw/char/serial: Keep MemoryRegionOps private
On Tue, 3 Mar 2026 at 22:22, 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 > --- > 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
Am 4. März 2026 01:33:34 UTC schrieb 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
>>> ---
>>> 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
Am 3. März 2026 22:56:20 UTC schrieb 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
>> ---
>> 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
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
---
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
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
---
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
