Re: [PATCH v2 03/12] hw/char/serial: Free struct SerialState from MemoryRegion

[BALATON Zoltan]

On Mon, 18 Dec 2023, Bernhard Beschow wrote:
> SerialState::io isn't used within TYPE_SERIAL directly. Push it to its users to
> make them the owner of the MemoryRegion.

I'm not sure this patch is needed. The users already own the SerialState 
so can use its memory region so they don't need their own. Since all of 
these need this io region putting it in SerialState saves some 
duplication. Unless I've missed some reason this might be needed.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/hw/char/serial.h   | 2 +-
> hw/char/serial-isa.c       | 7 +++++--
> hw/char/serial-pci-multi.c | 7 ++++---
> hw/char/serial-pci.c       | 7 +++++--
> hw/char/serial.c           | 4 ++--
> 5 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 8ba7eca3d6..eb4254edde 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -77,7 +77,6 @@ struct SerialState {
>     int poll_msl;
>
>     QEMUTimer *modem_status_poll;
> -    MemoryRegion io;
> };
> typedef struct SerialState SerialState;
>
> @@ -85,6 +84,7 @@ struct SerialMM {
>     SysBusDevice parent;
>
>     SerialState serial;
> +    MemoryRegion io;
>
>     uint8_t regshift;
>     uint8_t endianness;
> diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
> index 141a6cb168..2be8be980b 100644
> --- a/hw/char/serial-isa.c
> +++ b/hw/char/serial-isa.c
> @@ -26,6 +26,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/module.h"
> +#include "exec/memory.h"
> #include "sysemu/sysemu.h"
> #include "hw/acpi/acpi_aml_interface.h"
> #include "hw/char/serial.h"
> @@ -43,6 +44,7 @@ struct ISASerialState {
>     uint32_t iobase;
>     uint32_t isairq;
>     SerialState state;
> +    MemoryRegion io;
> };
>
> static const int isa_serial_io[MAX_ISA_SERIAL_PORTS] = {
> @@ -79,8 +81,9 @@ 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);
> +    memory_region_init_io(&isa->io, OBJECT(isa), &serial_io_ops, s, "serial",
> +                          8);
> +    isa_register_ioport(isadev, &isa->io, isa->iobase);
> }
>
> static void serial_isa_build_aml(AcpiDevAmlIf *adev, Aml *scope)
> diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
> index 5d65c534cb..16cb2faad7 100644
> --- a/hw/char/serial-pci-multi.c
> +++ b/hw/char/serial-pci-multi.c
> @@ -44,6 +44,7 @@ typedef struct PCIMultiSerialState {
>     uint32_t     ports;
>     char         *name[PCI_SERIAL_MAX_PORTS];
>     SerialState  state[PCI_SERIAL_MAX_PORTS];
> +    MemoryRegion io[PCI_SERIAL_MAX_PORTS];
>     uint32_t     level[PCI_SERIAL_MAX_PORTS];
>     qemu_irq     *irqs;
>     uint8_t      prog_if;
> @@ -58,7 +59,7 @@ static void multi_serial_pci_exit(PCIDevice *dev)
>     for (i = 0; i < pci->ports; i++) {
>         s = pci->state + i;
>         qdev_unrealize(DEVICE(s));
> -        memory_region_del_subregion(&pci->iobar, &s->io);
> +        memory_region_del_subregion(&pci->iobar, &pci->io[i]);
>         g_free(pci->name[i]);
>     }
>     qemu_free_irqs(pci->irqs, pci->ports);
> @@ -112,9 +113,9 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error 
> **errp)
>         }
>         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,
> +        memory_region_init_io(&pci->io[i], OBJECT(pci), &serial_io_ops, s,
>                               pci->name[i], 8);
> -        memory_region_add_subregion(&pci->iobar, 8 * i, &s->io);
> +        memory_region_add_subregion(&pci->iobar, 8 * i, &pci->io[i]);
>         pci->ports++;
>     }
> }
> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
> index 087da3059a..ab3d0e56b5 100644
> --- a/hw/char/serial-pci.c
> +++ b/hw/char/serial-pci.c
> @@ -28,6 +28,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/module.h"
> +#include "exec/memory.h"
> #include "hw/char/serial.h"
> #include "hw/irq.h"
> #include "hw/pci/pci_device.h"
> @@ -38,6 +39,7 @@
> struct PCISerialState {
>     PCIDevice dev;
>     SerialState state;
> +    MemoryRegion io;
>     uint8_t prog_if;
> };
>
> @@ -57,8 +59,9 @@ static void serial_pci_realize(PCIDevice *dev, Error **errp)
>     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
>     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);
> +    memory_region_init_io(&pci->io, OBJECT(pci), &serial_io_ops, s, "serial",
> +                          8);
> +    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->io);
> }
>
> static void serial_pci_exit(PCIDevice *dev)
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index a32eb25f58..83b642aec3 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -1045,10 +1045,10 @@ static void serial_mm_realize(DeviceState *dev, Error 
> **errp)
>         return;
>     }
>
> -    memory_region_init_io(&s->io, OBJECT(dev),
> +    memory_region_init_io(&smm->io, OBJECT(dev),
>                           &serial_mm_ops[smm->endianness], smm, "serial",
>                           8 << smm->regshift);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(smm), &s->io);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(smm), &smm->io);
>     sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq);
> }