On Tue, 3 Mar 2026 at 22:22, Bernhard Beschow <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
thanks
-- PMM
