qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH v3] PortioList: Store PortioList in device state
Date: Tue, 29 Apr 2014 16:02:30 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0

Il 29/04/2014 15:38, Kirill Batuzov ha scritto:
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.

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..b28981b 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -108,15 +108,20 @@ 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);
+    PortioList piolist;

     /* 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);
+    /* FIXME: the device should store created PortioList in its state.  Note
+       that DEV can be NULL here and that single device can register several
+       portio lists.  Current implementation is leaking memory allocated
+       in portio_list_init.  The leak is not critical because it happens only
+       at initialization time.  */
+    portio_list_init(&piolist, OBJECT(dev), pio_start, opaque, name);
+    portio_list_add(&piolist, isabus->address_space_io, start);
 }

 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)


Reviewed-by: Paolo Bonzini <address@hidden>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]