QOMify IRQs

PATCH In review Peter Crosthwaite 4 participants 14 messages Jun 18, 2014 – Jun 27, 2014

lore

Share & tools

lore

Peter Crosthwaite

author

2014-06-18 07:54 UTC #0 lore raw

v2 cover QOMify IRQs

Hi Andreas and all,

I have done some cleanup of your WIP IRQ QOMification and have it in a hopefully ready state. Its now link safe and the allocation/freeing process is not as complex as before.

For fuller context of the motivation behind this series, please see:
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03265.html

changed since v1:
Fixed sh4 instance of "[0]" bug (Kirill review)

Regards,
Peter

Andreas Färber (3):
sdhci: Fix misuse of qemu_free_irqs()
hw: Fix qemu_allocate_irqs() leaks
irq: Slim conversion of qemu_irq to QOM

Peter Crosthwaite (1):
irq: Allocate IRQs individually

 hw/arm/omap1.c          | 14 +++++++-------
 hw/arm/omap2.c          |  2 +-
 hw/arm/pxa2xx.c         |  4 ++--
 hw/arm/spitz.c          |  4 ++--
 hw/arm/z2.c             |  2 +-
 hw/char/serial-pci.c    |  2 +-
 hw/core/irq.c           | 46 +++++++++++++++++++++++++++++-----------------
 hw/core/qdev.c          |  2 +-
 hw/dma/omap_dma.c       |  4 ++--
 hw/ide/microdrive.c     |  2 +-
 hw/ipack/ipack.c        |  2 +-
 hw/misc/cbus.c          |  6 +++---
 hw/pcmcia/pxa2xx.c      |  2 +-
 hw/sd/omap_mmc.c        |  2 +-
 hw/sd/sdhci.c           |  8 ++++----
 hw/sh4/sh7750.c         |  3 +--
 hw/timer/omap_gptimer.c |  4 ++--
 include/hw/irq.h        |  4 +++-
 18 files changed, 63 insertions(+), 50 deletions(-)

Signature · 1 line

2.0.0

Peter Crosthwaite

author

2014-06-18 07:57 UTC #1 lore raw

PATCH v2 1/4 sdhci: Fix misuse of qemu_free_irqs()

From: Andreas Färber <[email protected]>

It does a g_free() on the pointer.

Reviewed-by Peter Crosthwaite <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

Signed-off-by Andreas Färber <[email protected]>

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/sd/sdhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/sd/sdhci.c +2 -2

index e2951e6..3e13d70 100644
@@ -1184,8 +1184,8 @@ static void sdhci_uninitfn(Object *obj)
     timer_free(s->insert_timer);
     timer_del(s->transfer_timer);
     timer_free(s->transfer_timer);
-    qemu_free_irqs(&s->eject_cb);
-    qemu_free_irqs(&s->ro_cb);
+    qemu_free_irq(s->eject_cb);
+    qemu_free_irq(s->ro_cb);
 
     if (s->fifo_buffer) {
         g_free(s->fifo_buffer);

Signature · 1 line

2.0.0

Andreas Färber

2014-06-27 09:40 UTC #2 lore raw

Re: [Qemu-devel] [PATCH qom v2 1/4] sdhci: Fix misuse of qemu_free_irqs()

Am 18.06.2014 09:54, schrieb Peter Crosthwaite:

Quoted 8 lines

From: Andreas Färber <[email protected]>
It does a g_free() on the pointer.
Reviewed-by Peter Crosthwaite <[email protected]>
Reviewed-by Peter Maydell <[email protected]>
Signed-off-by Andreas Färber <[email protected]>
Signed-off-by Peter Crosthwaite <[email protected]>

Thanks for picking this up and reviewing, applied to qom-next with extended commit message:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Andreas

Signature · 2 lines

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

Peter Crosthwaite

author

2014-06-18 07:56 UTC #3 lore raw

PATCH v2 2/4 hw: Fix qemu_allocate_irqs() leaks

In reply to Peter Crosthwaite

From: Andreas Färber <[email protected]>

Replace qemu_allocate_irqs(foo, bar, 1)[0]
with qemu_allocate_irq(foo, bar, 0).

This avoids leaking the dereferenced qemu_irq *.

Cc Kirill Batuzov <[email protected]>

Cc Markus Armbruster <[email protected]>

Cc Peter Maydell <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

Signed-off-by Andreas Färber <[email protected]>

[PC Changes:
* Applied change to instance in sh4/sh7750.c
]

Signed-off-by Peter Crosthwaite <[email protected]>

---
Changed since 1:
Applied change to instance in sh4/sh7750.c (Kirill review)

 hw/arm/omap1.c          | 14 +++++++-------
 hw/arm/omap2.c          |  2 +-
 hw/arm/pxa2xx.c         |  4 ++--
 hw/arm/spitz.c          |  4 ++--
 hw/arm/z2.c             |  2 +-
 hw/core/irq.c           |  4 ++--
 hw/dma/omap_dma.c       |  4 ++--
 hw/ide/microdrive.c     |  2 +-
 hw/misc/cbus.c          |  6 +++---
 hw/pcmcia/pxa2xx.c      |  2 +-
 hw/sd/omap_mmc.c        |  2 +-
 hw/sd/sdhci.c           |  4 ++--
 hw/sh4/sh7750.c         |  3 +--
 hw/timer/omap_gptimer.c |  4 ++--
 14 files changed, 28 insertions(+), 29 deletions(-)

hw/arm/omap1.c +7 -7

index b28e052..e7cc5d7 100644
@@ -172,7 +172,7 @@ static void omap_timer_clk_update(void *opaque, int line, int on)
 static void omap_timer_clk_setup(struct omap_mpu_timer_s *timer)
 {
     omap_clk_adduser(timer->clk,
-                    qemu_allocate_irqs(omap_timer_clk_update, timer, 1)[0]);
+                    qemu_allocate_irq(omap_timer_clk_update, timer, 0));
     timer->rate = omap_clk_getrate(timer->clk);
 }

@@ -2098,7 +2098,7 @@ static struct omap_mpuio_s *omap_mpuio_init(MemoryRegion *memory,
                           "omap-mpuio", 0x800);
     memory_region_add_subregion(memory, base, &s->iomem);
 
-    omap_clk_adduser(clk, qemu_allocate_irqs(omap_mpuio_onoff, s, 1)[0]);
+    omap_clk_adduser(clk, qemu_allocate_irq(omap_mpuio_onoff, s, 0));
 
     return s;
 }

@@ -2401,7 +2401,7 @@ static struct omap_pwl_s *omap_pwl_init(MemoryRegion *system_memory,
                           "omap-pwl", 0x800);
     memory_region_add_subregion(system_memory, base, &s->iomem);
 
-    omap_clk_adduser(clk, qemu_allocate_irqs(omap_pwl_clk_update, s, 1)[0]);
+    omap_clk_adduser(clk, qemu_allocate_irq(omap_pwl_clk_update, s, 0));
     return s;
 }

@@ -3485,8 +3485,8 @@ static void omap_mcbsp_i2s_start(void *opaque, int line, int level)
 void omap_mcbsp_i2s_attach(struct omap_mcbsp_s *s, I2SCodec *slave)
 {
     s->codec = slave;
-    slave->rx_swallow = qemu_allocate_irqs(omap_mcbsp_i2s_swallow, s, 1)[0];
-    slave->tx_start = qemu_allocate_irqs(omap_mcbsp_i2s_start, s, 1)[0];
+    slave->rx_swallow = qemu_allocate_irq(omap_mcbsp_i2s_swallow, s, 0);
+    slave->tx_start = qemu_allocate_irq(omap_mcbsp_i2s_start, s, 0);
 }
 
 /* LED Pulse Generators */

@@ -3634,7 +3634,7 @@ static struct omap_lpg_s *omap_lpg_init(MemoryRegion *system_memory,
     memory_region_init_io(&s->iomem, NULL, &omap_lpg_ops, s, "omap-lpg", 0x800);
     memory_region_add_subregion(system_memory, base, &s->iomem);
 
-    omap_clk_adduser(clk, qemu_allocate_irqs(omap_lpg_clk_update, s, 1)[0]);
+    omap_clk_adduser(clk, qemu_allocate_irq(omap_lpg_clk_update, s, 0));
 
     return s;
 }

@@ -3848,7 +3848,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
     s->sdram_size = sdram_size;
     s->sram_size = OMAP15XX_SRAM_SIZE;
 
-    s->wakeup = qemu_allocate_irqs(omap_mpu_wakeup, s, 1)[0];
+    s->wakeup = qemu_allocate_irq(omap_mpu_wakeup, s, 0);
 
     /* Clocks */
     omap_clk_init(s);

hw/arm/omap2.c +1 -1

index 36efde0..dc53a7a 100644
@@ -2260,7 +2260,7 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
     s->sdram_size = sdram_size;
     s->sram_size = OMAP242X_SRAM_SIZE;
 
-    s->wakeup = qemu_allocate_irqs(omap_mpu_wakeup, s, 1)[0];
+    s->wakeup = qemu_allocate_irq(omap_mpu_wakeup, s, 0);
 
     /* Clocks */
     omap_clk_init(s);

hw/arm/pxa2xx.c +2 -2

index 2d28a11..557e0f1 100644
@@ -2052,7 +2052,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
         fprintf(stderr, "Unable to find CPU definition\n");
         exit(1);
     }
-    s->reset = qemu_allocate_irqs(pxa2xx_reset, s, 1)[0];
+    s->reset = qemu_allocate_irq(pxa2xx_reset, s, 0);
 
     /* SDRAM & Internal Memory Storage */
     memory_region_init_ram(&s->sdram, NULL, "pxa270.sdram", sdram_size);

@@ -2183,7 +2183,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
         fprintf(stderr, "Unable to find CPU definition\n");
         exit(1);
     }
-    s->reset = qemu_allocate_irqs(pxa2xx_reset, s, 1)[0];
+    s->reset = qemu_allocate_irq(pxa2xx_reset, s, 0);
 
     /* SDRAM & Internal Memory Storage */
     memory_region_init_ram(&s->sdram, NULL, "pxa255.sdram", sdram_size);

hw/arm/spitz.c +2 -2

index 5455dbf..e20d5b8 100644
@@ -744,7 +744,7 @@ static void spitz_i2c_setup(PXA2xxState *cpu)
 
     spitz_wm8750_addr(wm, 0, 0);
     qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_WM,
-                    qemu_allocate_irqs(spitz_wm8750_addr, wm, 1)[0]);
+                          qemu_allocate_irq(spitz_wm8750_addr, wm, 0));
     /* .. and to the sound interface.  */
     cpu->i2s->opaque = wm;
     cpu->i2s->codec_out = wm8750_dac_dat;

@@ -850,7 +850,7 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots)
      * wouldn't guarantee that a guest ever exits the loop.
      */
     spitz_hsync = 0;
-    lcd_hsync = qemu_allocate_irqs(spitz_lcd_hsync_handler, cpu, 1)[0];
+    lcd_hsync = qemu_allocate_irq(spitz_lcd_hsync_handler, cpu, 0);
     pxa2xx_gpio_read_notifier(cpu->gpio, lcd_hsync);
     pxa2xx_lcd_vsync_notifier(cpu->lcd, lcd_hsync);

hw/arm/z2.c +1 -1

index ab9e4c9..36b3b50 100644
@@ -363,7 +363,7 @@ static void z2_init(MachineState *machine)
     wm8750_data_req_set(wm, mpu->i2s->data_req, mpu->i2s);
 
     qdev_connect_gpio_out(mpu->gpio, Z2_GPIO_LCD_CS,
-        qemu_allocate_irqs(z2_lcd_cs, z2_lcd, 1)[0]);
+                          qemu_allocate_irq(z2_lcd_cs, z2_lcd, 0));
 
     z2_binfo.kernel_filename = kernel_filename;
     z2_binfo.kernel_cmdline = kernel_cmdline;

hw/core/irq.c +2 -2

index 03c8cb3..3d284c6 100644
@@ -102,7 +102,7 @@ qemu_irq qemu_irq_invert(qemu_irq irq)
 {
     /* The default state for IRQs is low, so raise the output now.  */
     qemu_irq_raise(irq);
-    return qemu_allocate_irqs(qemu_notirq, irq, 1)[0];
+    return qemu_allocate_irq(qemu_notirq, irq, 0);
 }
 
 static void qemu_splitirq(void *opaque, int line, int level)

@@ -117,7 +117,7 @@ qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq irq2)
     qemu_irq *s = g_malloc0(2 * sizeof(qemu_irq));
     s[0] = irq1;
     s[1] = irq2;
-    return qemu_allocate_irqs(qemu_splitirq, s, 1)[0];
+    return qemu_allocate_irq(qemu_splitirq, s, 0);
 }
 
 static void proxy_irq_handler(void *opaque, int n, int level)

hw/dma/omap_dma.c +2 -2

index 0f35c42..756a87a 100644
@@ -1660,7 +1660,7 @@ struct soc_dma_s *omap_dma_init(hwaddr base, qemu_irq *irqs,
     }
 
     omap_dma_setcaps(s);
-    omap_clk_adduser(s->clk, qemu_allocate_irqs(omap_dma_clk_update, s, 1)[0]);
+    omap_clk_adduser(s->clk, qemu_allocate_irq(omap_dma_clk_update, s, 0));
     omap_dma_reset(s->dma);
     omap_dma_clk_update(s, 0, 1);

@@ -2082,7 +2082,7 @@ struct soc_dma_s *omap_dma4_init(hwaddr base, qemu_irq *irqs,
     s->intr_update = omap_dma_interrupts_4_update;
 
     omap_dma_setcaps(s);
-    omap_clk_adduser(s->clk, qemu_allocate_irqs(omap_dma_clk_update, s, 1)[0]);
+    omap_clk_adduser(s->clk, qemu_allocate_irq(omap_dma_clk_update, s, 0));
     omap_dma_reset(s->dma);
     omap_dma_clk_update(s, 0, !!s->dma->freq);

hw/ide/microdrive.c +1 -1

index f24946d..2d70ddb 100644
@@ -593,7 +593,7 @@ static void microdrive_realize(DeviceState *dev, Error **errp)
 {
     MicroDriveState *md = MICRODRIVE(dev);
 
-    ide_init2(&md->bus, qemu_allocate_irqs(md_set_irq, md, 1)[0]);
+    ide_init2(&md->bus, qemu_allocate_irq(md_set_irq, md, 0));
 }
 
 static void microdrive_init(Object *obj)

hw/misc/cbus.c +3 -3

index 29b467b..495d507 100644
@@ -135,9 +135,9 @@ CBus *cbus_init(qemu_irq dat)
     CBusPriv *s = (CBusPriv *) g_malloc0(sizeof(*s));
 
     s->dat_out = dat;
-    s->cbus.clk = qemu_allocate_irqs(cbus_clk, s, 1)[0];
-    s->cbus.dat = qemu_allocate_irqs(cbus_dat, s, 1)[0];
-    s->cbus.sel = qemu_allocate_irqs(cbus_sel, s, 1)[0];
+    s->cbus.clk = qemu_allocate_irq(cbus_clk, s, 0);
+    s->cbus.dat = qemu_allocate_irq(cbus_dat, s, 0);
+    s->cbus.sel = qemu_allocate_irq(cbus_sel, s, 0);
 
     s->sel = 1;
     s->clk = 0;

hw/pcmcia/pxa2xx.c +1 -1

index 96f3774..55e8a2a 100644
@@ -195,7 +195,7 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
     memory_region_add_subregion(&s->container_mem, 0x0c000000,
                                 &s->common_iomem);
 
-    s->slot.irq = qemu_allocate_irqs(pxa2xx_pcmcia_set_irq, s, 1)[0];
+    s->slot.irq = qemu_allocate_irq(pxa2xx_pcmcia_set_irq, s, 0);
 
     object_property_add_link(obj, "card", TYPE_PCMCIA_CARD,
                              (Object **)&s->card,

hw/sd/omap_mmc.c +1 -1

index 937a478..6c92149 100644
@@ -625,7 +625,7 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
         exit(1);
     }
 
-    s->cdet = qemu_allocate_irqs(omap_mmc_cover_cb, s, 1)[0];
+    s->cdet = qemu_allocate_irq(omap_mmc_cover_cb, s, 0);
     sd_set_cb(s->card, NULL, s->cdet);
 
     return s;

hw/sd/sdhci.c +2 -2

index 3e13d70..b5a9eee 100644
@@ -1168,8 +1168,8 @@ static void sdhci_initfn(Object *obj)
     if (s->card == NULL) {
         exit(1);
     }
-    s->eject_cb = qemu_allocate_irqs(sdhci_insert_eject_cb, s, 1)[0];
-    s->ro_cb = qemu_allocate_irqs(sdhci_card_readonly_cb, s, 1)[0];
+    s->eject_cb = qemu_allocate_irq(sdhci_insert_eject_cb, s, 0);
+    s->ro_cb = qemu_allocate_irq(sdhci_card_readonly_cb, s, 0);
     sd_set_cb(s->card, s->ro_cb, s->eject_cb);
 
     s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);

hw/sh4/sh7750.c +1 -2

index 4a39357..9ccd770 100644
@@ -838,6 +838,5 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 qemu_irq sh7750_irl(SH7750State *s)
 {
     sh_intc_toggle_source(sh_intc_source(&s->intc, IRL), 1, 0); /* enable */
-    return qemu_allocate_irqs(sh_intc_set_irl, sh_intc_source(&s->intc, IRL),
-                               1)[0];
+    return qemu_allocate_irq(sh_intc_set_irl, sh_intc_source(&s->intc, IRL), 1);
 }

hw/timer/omap_gptimer.c +2 -2

index 016207f..b7f3d49 100644
@@ -227,7 +227,7 @@ static void omap_gp_timer_clk_update(void *opaque, int line, int on)
 static void omap_gp_timer_clk_setup(struct omap_gp_timer_s *timer)
 {
     omap_clk_adduser(timer->clk,
-                    qemu_allocate_irqs(omap_gp_timer_clk_update, timer, 1)[0]);
+                     qemu_allocate_irq(omap_gp_timer_clk_update, timer, 0));
     timer->rate = omap_clk_getrate(timer->clk);
 }

@@ -476,7 +476,7 @@ struct omap_gp_timer_s *omap_gp_timer_init(struct omap_target_agent_s *ta,
     s->clk = fclk;
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, omap_gp_timer_tick, s);
     s->match = timer_new_ns(QEMU_CLOCK_VIRTUAL, omap_gp_timer_match, s);
-    s->in = qemu_allocate_irqs(omap_gp_timer_input, s, 1)[0];
+    s->in = qemu_allocate_irq(omap_gp_timer_input, s, 0);
     omap_gp_timer_reset(s);
     omap_gp_timer_clk_setup(s);

Signature · 1 line

2.0.0

Kirill Batuzov

Reviewed-by on v2 2/4

2014-06-18 14:03 UTC #4 lore raw

Re: [Qemu-devel] [PATCH qom v2 2/4] hw: Fix qemu_allocate_irqs() leaks

Quoted 21 lines · Peter Crosthwaite

From: Andreas Färber <[email protected]>
Replace qemu_allocate_irqs(foo, bar, 1)[0]
with qemu_allocate_irq(foo, bar, 0).
This avoids leaking the dereferenced qemu_irq *.
Cc Kirill Batuzov <[email protected]>
Cc Markus Armbruster <[email protected]>
Cc Peter Maydell <[email protected]>
Reviewed-by Peter Crosthwaite <[email protected]>
Reviewed-by Peter Maydell <[email protected]>
Signed-off-by Andreas Färber <[email protected]>
[PC Changes:
* Applied change to instance in sh4/sh7750.c
]
Signed-off-by Peter Crosthwaite <[email protected]>
---
Changed since 1:
Applied change to instance in sh4/sh7750.c (Kirill review)

Quoted 16 lines

 hw/arm/omap1.c          | 14 +++++++-------
 hw/arm/omap2.c          |  2 +-
 hw/arm/pxa2xx.c         |  4 ++--
 hw/arm/spitz.c          |  4 ++--
 hw/arm/z2.c             |  2 +-
 hw/core/irq.c           |  4 ++--
 hw/dma/omap_dma.c       |  4 ++--
 hw/ide/microdrive.c     |  2 +-
 hw/misc/cbus.c          |  6 +++---
 hw/pcmcia/pxa2xx.c      |  2 +-
 hw/sd/omap_mmc.c        |  2 +-
 hw/sd/sdhci.c           |  4 ++--
 hw/sh4/sh7750.c         |  3 +--
 hw/timer/omap_gptimer.c |  4 ++--
 14 files changed, 28 insertions(+), 29 deletions(-)

Reviewed-by Kirill Batuzov <[email protected]>

Signature · 1 line

Kirill

Andreas Färber

2014-06-27 09:46 UTC #5 lore raw

Re: [Qemu-devel] [PATCH qom v2 2/4] hw: Fix qemu_allocate_irqs() leaks

In reply to Peter Crosthwaite

Am 18.06.2014 09:55, schrieb Peter Crosthwaite:

Quoted 20 lines

From: Andreas Färber <[email protected]>
Replace qemu_allocate_irqs(foo, bar, 1)[0]
with qemu_allocate_irq(foo, bar, 0).
This avoids leaking the dereferenced qemu_irq *.
Cc Kirill Batuzov <[email protected]>
Cc Markus Armbruster <[email protected]>
Cc Peter Maydell <[email protected]>
Reviewed-by Peter Crosthwaite <[email protected]>
Reviewed-by Peter Maydell <[email protected]>
Signed-off-by Andreas Färber <[email protected]>
[PC Changes:
* Applied change to instance in sh4/sh7750.c
]
Signed-off-by Peter Crosthwaite <[email protected]>
---
Changed since 1:
Applied change to instance in sh4/sh7750.c (Kirill review)

[...]

hw/sh4/sh7750.c SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem) · line 841

Quoted 11 lines

hw/sh4/sh7750.c +1 -2

index 4a39357..9ccd770 100644
@@ -838,6 +838,5 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 qemu_irq sh7750_irl(SH7750State *s)
 {
     sh_intc_toggle_source(sh_intc_source(&s->intc, IRL), 1, 0); /* enable */
-    return qemu_allocate_irqs(sh_intc_set_irl, sh_intc_source(&s->intc, IRL),
-                               1)[0];
+    return qemu_allocate_irq(sh_intc_set_irl, sh_intc_source(&s->intc, IRL), 1);

Thanks for catching this, my grep expression failed due to the line break. But shouldn't this be 0 due to the zero-based index, as per my commit message? Will fix up unless I hear objections.

Regards,
Andreas

Quoted 1 line

hw/sh4/sh7750.c +0 -0
 }

Signature · 2 lines

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

Peter Crosthwaite

author

2014-06-27 10:21 UTC #6 lore raw

Re: [Qemu-devel] [PATCH qom v2 2/4] hw: Fix qemu_allocate_irqs() leaks

Quoted 22 lines · Andreas Färber

Am 18.06.2014 09:55, schrieb Peter Crosthwaite:
Quoted 20 lines
From: Andreas Färber <[email protected]>
Replace qemu_allocate_irqs(foo, bar, 1)[0]
with qemu_allocate_irq(foo, bar, 0).
This avoids leaking the dereferenced qemu_irq *.
Cc Kirill Batuzov <[email protected]>
Cc Markus Armbruster <[email protected]>
Cc Peter Maydell <[email protected]>
Reviewed-by Peter Crosthwaite <[email protected]>
Reviewed-by Peter Maydell <[email protected]>
Signed-off-by Andreas Färber <[email protected]>
[PC Changes:
* Applied change to instance in sh4/sh7750.c
]
Signed-off-by Peter Crosthwaite <[email protected]>
---
Changed since 1:
Applied change to instance in sh4/sh7750.c (Kirill review)
[...]

Quoted 16 lines

hw/sh4/sh7750.c SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem) · line 841
Quoted 11 lines
hw/sh4/sh7750.c +1 -2
index 4a39357..9ccd770 100644
@@ -838,6 +838,5 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 qemu_irq sh7750_irl(SH7750State *s)
 {
     sh_intc_toggle_source(sh_intc_source(&s->intc, IRL), 1, 0); /* enable */
-    return qemu_allocate_irqs(sh_intc_set_irl, sh_intc_source(&s->intc, IRL),
-                               1)[0];
+    return qemu_allocate_irq(sh_intc_set_irl, sh_intc_source(&s->intc, IRL), 1);
Thanks for catching this, my grep expression failed due to the line break. But shouldn't this be 0 due to the zero-based index, as per my commit message? Will fix up unless I hear objections.

Yep, sorry.

Regards,
Peter

Quoted 9 lines

Regards,
Andreas
hw/sh4/sh7750.c SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem) · line 842
Quoted 1 line
hw/sh4/sh7750.c +0 -0
 }
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Peter Crosthwaite

author

2014-06-18 07:57 UTC #7 lore raw

PATCH v2 3/4 irq: Allocate IRQs individually

In reply to Peter Crosthwaite

Allocate each IRQ individually on array allocations. This prepares for QOMification of IRQs, where pointers to individual IRQs may be taken and handed around for usage as QOM Links. The g_renew scheme used here is too fragile and would break all existing links should an IRQ list be extended.

We now have to pass the IRQ count to qemu_free_irqs(). We have so few call sites however, so this change is reasonably trivial.

Cc [email protected]

Reviewed-by Peter Maydell <[email protected]>

Acked-by Alberto Garcia <[email protected]>

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/char/serial-pci.c |  2 +-
 hw/core/irq.c        | 20 +++++++-------------
 hw/core/qdev.c       |  2 +-
 hw/ipack/ipack.c     |  2 +-
 include/hw/irq.h     |  2 +-
 5 files changed, 11 insertions(+), 17 deletions(-)

hw/char/serial-pci.c +1 -1

index 6c25296..f53bb9c 100644
@@ -152,7 +152,7 @@ static void multi_serial_pci_exit(PCIDevice *dev)
         g_free(pci->name[i]);
     }
     memory_region_destroy(&pci->iobar);
-    qemu_free_irqs(pci->irqs);
+    qemu_free_irqs(pci->irqs, pci->ports);
 }
 
 static const VMStateDescription vmstate_pci_serial = {

hw/core/irq.c +7 ~~-13~~

index 3d284c6..bc982a7 100644
@@ -42,23 +42,14 @@ qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
                            void *opaque, int n)
 {
     qemu_irq *s;
-    struct IRQState *p;
     int i;
 
     if (!old) {
         n_old = 0;
     }
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
-    p = old ? g_renew(struct IRQState, s[0], n + n_old) :
-                g_new(struct IRQState, n);
-    for (i = 0; i < n + n_old; i++) {
-        if (i >= n_old) {
-            p->handler = handler;
-            p->opaque = opaque;
-            p->n = i;
-        }
-        s[i] = p;
-        p++;
+    for (i = n_old; i < n + n_old; i++) {
+        s[i] = qemu_allocate_irq(handler, opaque, i);
     }
     return s;
 }

@@ -80,9 +71,12 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
     return irq;
 }
 
-void qemu_free_irqs(qemu_irq *s)
+void qemu_free_irqs(qemu_irq *s, int n)
 {
-    g_free(s[0]);
+    int i;
+    for (i = 0; i < n; i++) {
+        qemu_free_irq(s[i]);
+    }
     g_free(s);
 }

hw/core/qdev.c +1 -1

index e65a5aa..02377c7 100644
@@ -916,7 +916,7 @@ static void device_finalize(Object *obj)
 
     QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
         QLIST_REMOVE(ngl, node);
-        qemu_free_irqs(ngl->in);
+        qemu_free_irqs(ngl->in, ngl->num_in);
         g_free(ngl->name);
         g_free(ngl);
         /* ngl->out irqs are owned by the other end and should not be freed

hw/ipack/ipack.c +1 -1

index ef032e6..59bfe28 100644
@@ -66,7 +66,7 @@ static void ipack_device_unrealize(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_free_irqs(idev->irq);
+    qemu_free_irqs(idev->irq, 2);
 }
 
 static Property ipack_device_props[] = {

include/hw/irq.h +1 -1

index d08bc02..9f34c96 100644
@@ -42,7 +42,7 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n);
 qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
                                 void *opaque, int n);
 
-void qemu_free_irqs(qemu_irq *s);
+void qemu_free_irqs(qemu_irq *s, int n);
 void qemu_free_irq(qemu_irq irq);
 
 /* Returns a new IRQ with opposite polarity.  */

Signature · 1 line

2.0.0

Peter Crosthwaite

author

2014-06-18 07:57 UTC #8 lore raw

PATCH v2 4/4 irq: Slim conversion of qemu_irq to QOM

In reply to Peter Crosthwaite

From: Andreas Färber <[email protected]>

As a prequel to any big Pin refactoring plans, do an in-place conversion of qemu_irq to an Object, so that we can reference it in link<> properties.

Reviewed-by Peter Maydell <[email protected]>

Signed-off-by Andreas Färber <[email protected]>

[ PC Changes:
* Removed array-alloctor ref counting logic (limit changes just to
* single IRQ allocator)
* Removed WIP marking from subject line
]

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/irq.c    | 22 ++++++++++++++++++++--
 include/hw/irq.h |  2 ++
 2 files changed, 22 insertions(+), 2 deletions(-)

hw/core/irq.c +20 -2

index bc982a7..cffced0 100644
@@ -23,8 +23,13 @@
  */
 #include "qemu-common.h"
 #include "hw/irq.h"
+#include "qom/object.h"
+
+#define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)
 
 struct IRQState {
+    Object parent_obj;
+
     qemu_irq_handler handler;
     void *opaque;
     int n;

@@ -63,7 +68,7 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
 {
     struct IRQState *irq;
 
-    irq = g_new(struct IRQState, 1);
+    irq = IRQ(object_new(TYPE_IRQ));
     irq->handler = handler;
     irq->opaque = opaque;
     irq->n = n;

@@ -82,7 +87,7 @@ void qemu_free_irqs(qemu_irq *s, int n)
 
 void qemu_free_irq(qemu_irq irq)
 {
-    g_free(irq);
+    object_unref(OBJECT(irq));
 }
 
 static void qemu_notirq(void *opaque, int line, int level)

@@ -144,3 +149,16 @@ void qemu_irq_intercept_out(qemu_irq **gpio_out, qemu_irq_handler handler, int n
     qemu_irq *old_irqs = *gpio_out;
     *gpio_out = qemu_allocate_irqs(handler, old_irqs, n);
 }
+
+static const TypeInfo irq_type_info = {
+   .name = TYPE_IRQ,
+   .parent = TYPE_OBJECT,
+   .instance_size = sizeof(struct IRQState),
+};
+
+static void irq_register_types(void)
+{
+    type_register_static(&irq_type_info);
+}
+
+type_init(irq_register_types)

include/hw/irq.h +2 -0

index 9f34c96..6f874f5 100644
@@ -3,6 +3,8 @@
 
 /* Generic IRQ/GPIO pin infrastructure.  */
 
+#define TYPE_IRQ "irq"
+
 typedef struct IRQState *qemu_irq;
 
 typedef void (*qemu_irq_handler)(void *opaque, int n, int level);

Signature · 1 line

2.0.0

Paolo Bonzini

Acked-by on v2 4/4

2014-06-18 14:40 UTC #9 lore raw

Re: [Qemu-devel] [PATCH qom v2 4/4] irq: Slim conversion of qemu_irq to QOM

Il 18/06/2014 09:57, Peter Crosthwaite ha scritto:

Quoted 10 lines

hw/core/irq.c +1 -1

@@ -63,7 +68,7 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
 {
     struct IRQState *irq;
 
-    irq = g_new(struct IRQState, 1);
+    irq = IRQ(object_new(TYPE_IRQ));
     irq->handler = handler;
     irq->opaque = opaque;
     irq->n = n;

hw/core/irq.c void qemu_free_irqs(qemu_irq *s, int n) · line 93

Quoted 8 lines

hw/core/irq.c +1 -1

@@ -82,7 +87,7 @@ void qemu_free_irqs(qemu_irq *s, int n)
 
 void qemu_free_irq(qemu_irq irq)
 {
-    g_free(irq);
+    object_unref(OBJECT(irq));
 }
 
 static void qemu_notirq(void *opaque, int line, int level)

If the next step is to add an "owner" like the one in MemoryRegion, and change occurrences of qemu_free_irq to object_unparent, then

Acked-by Paolo Bonzini <[email protected]>

Paolo

Peter Crosthwaite

author

2014-06-19 04:57 UTC #10 lore raw

Re: [Qemu-devel] [PATCH qom v2 4/4] irq: Slim conversion of qemu_irq to QOM

Quoted 20 lines · Paolo Bonzini

Il 18/06/2014 09:57, Peter Crosthwaite ha scritto:

Quoted 18 lines

hw/core/irq.c +1 -1

@@ -63,7 +68,7 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler,
 void *opaque, int n)
 {
     struct IRQState *irq;
 
-    irq = g_new(struct IRQState, 1);
+    irq = IRQ(object_new(TYPE_IRQ));
     irq->handler = handler;
     irq->opaque = opaque;

     irq->n = n;

hw/core/irq.c +1 -1

@@ -82,7 +87,7 @@ void qemu_free_irqs(qemu_irq *s, int n)
 
 void qemu_free_irq(qemu_irq irq)
 {
-    g_free(irq);
+    object_unref(OBJECT(irq));
 }

Quoted 5 lines

hw/core/irq.c void qemu_free_irqs(qemu_irq *s, int n) · line 93
Quoted 1 line
hw/core/irq.c +0 -0
 static void qemu_notirq(void *opaque, int line, int level)
If the next step is to add an "owner" like the one in MemoryRegion, and change occurrences of qemu_free_irq to object_unparent,

Sure, I guess its a tree wide much like the one for Memory API though. Can we do it as follow up though and sneak this through for 2.1?

Regards,
Peter

Quoted 6 lines

then
Acked-by Paolo Bonzini <[email protected]>
Paolo

Paolo Bonzini

2014-06-19 05:47 UTC #11 lore raw

Re: [Qemu-devel] [PATCH qom v2 4/4] irq: Slim conversion of qemu_irq to QOM

Il 19/06/2014 06:57, Peter Crosthwaite ha scritto:

Quoted 5 lines

Quoted 3 lines
Quoted 3 lines
If the next step is to add an "owner" like the one in MemoryRegion, and change occurrences of qemu_free_irq to object_unparent,
Sure, I guess its a tree wide much like the one for Memory API though. Can we do it as follow up though and sneak this through for 2.1?

Sure.

Paolo

Peter Crosthwaite

author

2014-06-25 09:39 UTC #12 lore raw

In reply to Peter Crosthwaite

Ping!

This is fully reviewed and should be rdy for a merge. I'd like to see this through for 2.1.

Regards,
Peter

Quoted 47 lines · Peter Crosthwaite

Hi Andreas and all,
I have done some cleanup of your WIP IRQ QOMification and have it in a hopefully ready state. Its now link safe and the allocation/freeing process is not as complex as before.
For fuller context of the motivation behind this series, please see:
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03265.html
changed since v1:
Fixed sh4 instance of "[0]" bug (Kirill review)
Regards,
Peter
Andreas Färber (3):
sdhci: Fix misuse of qemu_free_irqs()
hw: Fix qemu_allocate_irqs() leaks
irq: Slim conversion of qemu_irq to QOM
Peter Crosthwaite (1):
irq: Allocate IRQs individually
 hw/arm/omap1.c          | 14 +++++++-------
 hw/arm/omap2.c          |  2 +-
 hw/arm/pxa2xx.c         |  4 ++--
 hw/arm/spitz.c          |  4 ++--
 hw/arm/z2.c             |  2 +-
 hw/char/serial-pci.c    |  2 +-
 hw/core/irq.c           | 46 +++++++++++++++++++++++++++++-----------------
 hw/core/qdev.c          |  2 +-
 hw/dma/omap_dma.c       |  4 ++--
 hw/ide/microdrive.c     |  2 +-
 hw/ipack/ipack.c        |  2 +-
 hw/misc/cbus.c          |  6 +++---
 hw/pcmcia/pxa2xx.c      |  2 +-
 hw/sd/omap_mmc.c        |  2 +-
 hw/sd/sdhci.c           |  8 ++++----
 hw/sh4/sh7750.c         |  3 +--
 hw/timer/omap_gptimer.c |  4 ++--
 include/hw/irq.h        |  4 +++-
 18 files changed, 63 insertions(+), 50 deletions(-)
--
2.0.0

Andreas Färber

2014-06-27 09:53 UTC #13 lore raw

Am 25.06.2014 11:39, schrieb Peter Crosthwaite:

Quoted 4 lines

Ping!
This is fully reviewed and should be rdy for a merge. I'd like to see this through for 2.1.

I have been very wary of applying the QOM conversion without full device test coverage, similar to realization. People actually testing this conversion would've been more reaffirming than a bit of review - the hardfreeze can but does not necessarily uncover all corner cases. But time is running out, so I intend to apply the series unless I discover issues.

qtests for missing devices or statistics of how incomplete our coverage actually is appreciated as always.

Regards,
Andreas

Signature · 2 lines

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