[v2,4/4] PortioList: Store PortioList in device state

PortioList is an abstraction used for construction of MemoryRegionPortioList
from MemoryRegionPortio. It can be used later to unmap created memory regions.
It also requires proper cleanup because some of the memory inside is allocated
dynamically.

By moving PortioList to device state we make it possible to cleanup later and
avoid leaking memory.

This change spans several target platforms.  The following testcases cover all
changed lines:
  qemu-system-ppc -M prep
  qemu-system-i386 -vga qxl
  qemu-system-i386 -M isapc -soundhw adlib -device ib700,id=watchdog0,bus=isa.0

Signed-off-by: Kirill Batuzov <batuzovk@ispras.ru>
---
 hw/audio/adlib.c        |    6 +++---
 hw/display/qxl.c        |    7 +++----
 hw/display/qxl.h        |    1 +
 hw/display/vga.c        |   12 +++++-------
 hw/display/vga_int.h    |    2 ++
 hw/dma/i82374.c         |    7 ++++---
 hw/isa/isa-bus.c        |   28 +++++++++++++++++++++++++---
 hw/ppc/prep.c           |    7 ++++---
 hw/watchdog/wdt_ib700.c |    7 ++++---
 include/hw/isa/isa.h    |    1 +
 10 files changed, 52 insertions(+), 26 deletions(-)

v1 -> v2:
 I tried adding PortioList to AdlibState, PCIQXLDevice etc like Paolo suggested.
 It worked fine for all cases except for isa_register_portio_list.
 isa_register_portio_list can be called:
  - with NULL instead of real ISADevice *,
  - several times for one device.
 Currently I put a workaroung for these cases but it is ugly. Proper solution
 would be to add another parameter PortioList * to isa_register_portio_list and
 to update all devices which use it. But it will change even more devices in
 this patch.

Il 24/04/2014 16:15, Kirill Batuzov ha scritto:
> v1 -> v2:
>  I tried adding PortioList to AdlibState, PCIQXLDevice etc like Paolo suggested.
>  It worked fine for all cases except for isa_register_portio_list.
>  isa_register_portio_list can be called:
>   - with NULL instead of real ISADevice *,
>   - several times for one device.
>  Currently I put a workaroung for these cases but it is ugly. Proper solution
>  would be to add another parameter PortioList * to isa_register_portio_list and
>  to update all devices which use it. But it will change even more devices in
>  this patch.

Can you just leave the leak for isa_register_portio_list, and add a 
FIXME comment about it?

Paolo

> diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
> index 28eed81..5dd739e 100644
> --- a/hw/audio/adlib.c
> +++ b/hw/audio/adlib.c
> @@ -86,6 +86,7 @@ typedef struct {
>  #ifndef HAS_YMF262
>      FM_OPL *opl;
>  #endif
> +    PortioList port_list;
>  } AdlibState;
>
>  static AdlibState *glob_adlib;
> @@ -293,7 +294,6 @@ static MemoryRegionPortio adlib_portio_list[] = {
>  static void adlib_realizefn (DeviceState *dev, Error **errp)
>  {
>      AdlibState *s = ADLIB(dev);
> -    PortioList *port_list = g_new(PortioList, 1);
>      struct audsettings as;
>
>      if (glob_adlib) {
> @@ -349,8 +349,8 @@ static void adlib_realizefn (DeviceState *dev, Error **errp)
>
>      adlib_portio_list[0].offset = s->port;
>      adlib_portio_list[1].offset = s->port + 8;
> -    portio_list_init (port_list, OBJECT(s), adlib_portio_list, s, "adlib");
> -    portio_list_add (port_list, isa_address_space_io(&s->parent_obj), 0);
> +    portio_list_init (&s->port_list, OBJECT(s), adlib_portio_list, s, "adlib");
> +    portio_list_add (&s->port_list, isa_address_space_io(&s->parent_obj), 0);
>  }
>
>  static Property adlib_properties[] = {
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 47bbf1f..b307b3d 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2055,7 +2055,6 @@ static int qxl_init_primary(PCIDevice *dev)
>  {
>      PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci, dev);
>      VGACommonState *vga = &qxl->vga;
> -    PortioList *qxl_vga_port_list = g_new(PortioList, 1);
>      int rc;
>
>      qxl->id = 0;
> @@ -2064,10 +2063,10 @@ static int qxl_init_primary(PCIDevice *dev)
>      vga_common_init(vga, OBJECT(dev));
>      vga_init(vga, OBJECT(dev),
>               pci_address_space(dev), pci_address_space_io(dev), false);
> -    portio_list_init(qxl_vga_port_list, OBJECT(dev), qxl_vga_portio_list,
> +    portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
>                       vga, "vga");
> -    portio_list_set_flush_coalesced(qxl_vga_port_list);
> -    portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
> +    portio_list_set_flush_coalesced(&qxl->vga_port_list);
> +    portio_list_add(&qxl->vga_port_list, pci_address_space_io(dev), 0x3b0);
>
>      vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
>      qemu_spice_display_init_common(&qxl->ssd);
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index c5de3d7..412e346 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -32,6 +32,7 @@ enum qxl_mode {
>
>  typedef struct PCIQXLDevice {
>      PCIDevice          pci;
> +    PortioList         vga_port_list;
>      SimpleSpiceDisplay ssd;
>      int                id;
>      uint32_t           debug;
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 063319d..5284920 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2351,8 +2351,6 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>  {
>      MemoryRegion *vga_io_memory;
>      const MemoryRegionPortio *vga_ports, *vbe_ports;
> -    PortioList *vga_port_list = g_new(PortioList, 1);
> -    PortioList *vbe_port_list = g_new(PortioList, 1);
>
>      qemu_register_reset(vga_reset, s);
>
> @@ -2367,13 +2365,13 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>                                          1);
>      memory_region_set_coalescing(vga_io_memory);
>      if (init_vga_ports) {
> -        portio_list_init(vga_port_list, obj, vga_ports, s, "vga");
> -        portio_list_set_flush_coalesced(vga_port_list);
> -        portio_list_add(vga_port_list, address_space_io, 0x3b0);
> +        portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
> +        portio_list_set_flush_coalesced(&s->vga_port_list);
> +        portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
>      }
>      if (vbe_ports) {
> -        portio_list_init(vbe_port_list, obj, vbe_ports, s, "vbe");
> -        portio_list_add(vbe_port_list, address_space_io, 0x1ce);
> +        portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
> +        portio_list_add(&s->vbe_port_list, address_space_io, 0x1ce);
>      }
>  }
>
> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
> index e641890..a09f14c 100644
> --- a/hw/display/vga_int.h
> +++ b/hw/display/vga_int.h
> @@ -124,6 +124,8 @@ typedef struct VGACommonState {
>      void (*get_resolution)(struct VGACommonState *s,
>                          int *pwidth,
>                          int *pheight);
> +    PortioList vga_port_list;
> +    PortioList vbe_port_list;
>      /* bochs vbe state */
>      uint16_t vbe_index;
>      uint16_t vbe_regs[VBE_DISPI_INDEX_NB];
> diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
> index dc7a767..b8ad2e6 100644
> --- a/hw/dma/i82374.c
> +++ b/hw/dma/i82374.c
> @@ -39,6 +39,7 @@ do { fprintf(stderr, "i82374 ERROR: " fmt , ## __VA_ARGS__); } while (0)
>  typedef struct I82374State {
>      uint8_t commands[8];
>      qemu_irq out;
> +    PortioList port_list;
>  } I82374State;
>
>  static const VMStateDescription vmstate_i82374 = {
> @@ -137,10 +138,10 @@ static void i82374_isa_realize(DeviceState *dev, Error **errp)
>  {
>      ISAi82374State *isa = I82374(dev);
>      I82374State *s = &isa->state;
> -    PortioList *port_list = g_new(PortioList, 1);
>
> -    portio_list_init(port_list, OBJECT(isa), i82374_portio_list, s, "i82374");
> -    portio_list_add(port_list, isa_address_space_io(&isa->parent_obj),
> +    portio_list_init(&s->port_list, OBJECT(isa), i82374_portio_list, s,
> +                     "i82374");
> +    portio_list_add(&s->port_list, isa_address_space_io(&isa->parent_obj),
>                      isa->iobase);
>
>      i82374_realize(s, errp);
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 55d0100..c3fa0ef 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -108,15 +108,37 @@ void isa_register_portio_list(ISADevice *dev, uint16_t start,
>                                const MemoryRegionPortio *pio_start,
>                                void *opaque, const char *name)
>  {
> -    PortioList *piolist = g_new(PortioList, 1);
> +    int i;
>
>      /* START is how we should treat DEV, regardless of the actual
>         contents of the portio array.  This is how the old code
>         actually handled e.g. the FDC device.  */
>      isa_init_ioport(dev, start);
>
> -    portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
> -    portio_list_add(piolist, isabus->address_space_io, start);
> +    if (dev) {
> +        i = 0;
> +        if (dev->piolist[i].owner) {
> +            i++;
> +        }
> +
> +        assert(!dev->piolist[i].owner);
> +
> +        portio_list_init(&dev->piolist[i], OBJECT(dev), pio_start, opaque,
> +                         name);
> +        portio_list_add(&dev->piolist[i], isabus->address_space_io, start);
> +    } else {
> +        /* Ideally, device should keep track of its ioports and other memory
> +           regions to be able to perform cleanup later.  But some devices
> +           register ioports without providing valid ISADevice *.  As
> +           a workaround for such devices we need to provide a local PortioList
> +           which is immediately destroyed after initialization.  */
> +
> +        PortioList piolist;
> +
> +        portio_list_init(&piolist, OBJECT(dev), pio_start, opaque, name);
> +        portio_list_add(&piolist, isabus->address_space_io, start);
> +        portio_list_destroy(&piolist);
> +    }
>  }
>
>  static void isa_device_init(Object *obj)
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index e243651..bc00ccf 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -361,6 +361,8 @@ static const MemoryRegionPortio prep_portio_list[] = {
>      PORTIO_END_OF_LIST(),
>  };
>
> +PortioList prep_port_list;
> +
>  /* PowerPC PREP hardware initialisation */
>  static void ppc_prep_init(QEMUMachineInitArgs *args)
>  {
> @@ -375,7 +377,6 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
>      CPUPPCState *env = NULL;
>      nvram_t nvram;
>      M48t59State *m48t59;
> -    PortioList *port_list = g_new(PortioList, 1);
>  #if 0
>      MemoryRegion *xcsr = g_new(MemoryRegion, 1);
>  #endif
> @@ -542,8 +543,8 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
>      cpu = POWERPC_CPU(first_cpu);
>      sysctrl->reset_irq = cpu->env.irq_inputs[PPC6xx_INPUT_HRESET];
>
> -    portio_list_init(port_list, NULL, prep_portio_list, sysctrl, "prep");
> -    portio_list_add(port_list, isa_address_space_io(isa), 0x0);
> +    portio_list_init(&prep_port_list, NULL, prep_portio_list, sysctrl, "prep");
> +    portio_list_add(&prep_port_list, isa_address_space_io(isa), 0x0);
>
>      /* PowerPC control and status register group */
>  #if 0
> diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
> index bc994a4..68b33e1 100644
> --- a/hw/watchdog/wdt_ib700.c
> +++ b/hw/watchdog/wdt_ib700.c
> @@ -42,6 +42,8 @@ typedef struct IB700state {
>      ISADevice parent_obj;
>
>      QEMUTimer *timer;
> +
> +    PortioList port_list;
>  } IB700State;
>
>  /* This is the timer.  We use a global here because the watchdog
> @@ -106,14 +108,13 @@ static const MemoryRegionPortio wdt_portio_list[] = {
>  static void wdt_ib700_realize(DeviceState *dev, Error **errp)
>  {
>      IB700State *s = IB700(dev);
> -    PortioList *port_list = g_new(PortioList, 1);
>
>      ib700_debug("watchdog init\n");
>
>      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ib700_timer_expired, s);
>
> -    portio_list_init(port_list, OBJECT(s), wdt_portio_list, s, "ib700");
> -    portio_list_add(port_list, isa_address_space_io(&s->parent_obj), 0);
> +    portio_list_init(&s->port_list, OBJECT(s), wdt_portio_list, s, "ib700");
> +    portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
>  }
>
>  static void wdt_ib700_reset(DeviceState *dev)
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index e0c749f..02df80c 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -48,6 +48,7 @@ struct ISADevice {
>      uint32_t isairq[2];
>      int nirqs;
>      int ioport_id;
> +    PortioList piolist[2];
>  };
>
>  ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space_io);
>