qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 4/4] PortioList: Store PortioList in device s

From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH v2 4/4] PortioList: Store PortioList in device state
Date: Mon, 28 Apr 2014 14:17:41 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 
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);
reply via email to
[Prev in Thread] Current Thread [Next in Thread]