Re: [Qemu-devel] [PATCH v3] PortioList: Store PortioList in device state

[Andreas Färber]

Am 29.04.2014 15:38, schrieb Kirill Batuzov:
> 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 ot 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 <address@hidden>
> ---
>  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        |   11 ++++++++---
>  hw/ppc/prep.c           |    7 ++++---
>  hw/watchdog/wdt_ib700.c |    7 ++++---
>  9 files changed, 34 insertions(+), 26 deletions(-)
> 
> I'm sending this as a separate patch. With 2 of 4 patches already applied and
> two remaining patches addressing absolutely different issues in different
> parts of code it was no longer reasonable to submit them as series.
> 
> v1 -> v2:
>  - Add PortioList to device state structures.
>  - Put a workaround for isa_regiter_portio_list for which the above solution
>    was not appilcable.
> 
> v2 -> v3:
>  - Revert workaround for isa_register_portio_list. The amount of memory leaked
>    is not large enough to justify potentially broken code.

Seeing no objections from Jan or Hervé and an Rb from Paolo, I'll pick
up this cross-devices patch for qom-next.

> 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;

I've made this variable static.

https://github.com/afaerber/qemu-cpu/commits/qom-next

Thanks,
Andreas

> +
>  /* 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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg