[Qemu-devel] [PATCH v2 0/7] Add a member in MachineState to store irq array

[Qemu-devel] [PATCH v2 0/7] Add a member in MachineState to store irq array

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

These are relevant to memory leak in machine init function. Here we add
a member in MachineState to store irq array returned from
qemu_allocate_irqs.

PS. These patches are split from my previous patchset [1] since they are
relevant to MachineState.

Thanks,
Shannon

[1] [PATCH 00/29] Fix memory leak relevant to calling qemu_allocate_irqs

changes since v1：
  * Add a member in MachineState to store irq array [Peter]

Shannon Zhao (7):
  hw/ppc/ppc440_bamboo.c: Add a member in MachineState to store irq
    array
  hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory
    leak
  hw/m68k/an5206.c: Store irq array in MachineState to fix memory leak
  hw/sh4/r2d.c: Store irq array in MachineState to fix memory leak
  hw/arm/palm.c: Store irq array in MachineState to fix memory leak
  hw/arm/spitz.c: Store irq array in MachineState to fix memory leak
  hw/arm/tosa.c: Store irq array in MachineState to fix memory leak

 hw/arm/palm.c          |  6 ++++--
 hw/arm/spitz.c         |  8 +++++---
 hw/arm/tosa.c          | 10 +++++-----
 hw/m68k/an5206.c       |  2 +-
 hw/mips/mips_jazz.c    | 28 ++++++++++++++--------------
 hw/ppc/ppc440_bamboo.c | 17 +++++++++--------
 hw/sh4/r2d.c           | 15 +++++++--------
 include/hw/boards.h    |  1 +
 8 files changed, 46 insertions(+), 41 deletions(-)

-- 
2.0.4

[Qemu-devel] [PATCH v2 1/7] hw/ppc/ppc440_bamboo.c: Add a member in MachineState to store irq array

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Here we add a member in MachineState to store the irq array returned
from qemu_allocate_irqs. Then these irq arrays will be free before QEMU
exit and it fixes the memory leak spotted by valgrind.

valgrind complains about:
==6366== 4,936 (256 direct, 4,680 indirect) bytes in 1 blocks are definitely lost in loss record 3,245 of 3,271
==6366==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==6366==    by 0x336F47: malloc_and_trace (vl.c:2556)
==6366==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
==6366==    by 0x3C093B: qemu_extend_irqs (irq.c:55)
==6366==    by 0x3C09C7: qemu_allocate_irqs (irq.c:64)
==6366==    by 0x265091: ppcuic_init (ppc4xx_devs.c:317)
==6366==    by 0x26A90A: bamboo_init (ppc440_bamboo.c:205)
==6366==    by 0x33B312: main (vl.c:4249)

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/ppc/ppc440_bamboo.c | 17 +++++++++--------
 include/hw/boards.h    |  1 +
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 778970a..f533b06 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -170,7 +170,6 @@ static void bamboo_init(MachineState *machine)
         = g_malloc(PPC440EP_SDRAM_NR_BANKS * sizeof(*ram_memories));
     hwaddr ram_bases[PPC440EP_SDRAM_NR_BANKS];
     hwaddr ram_sizes[PPC440EP_SDRAM_NR_BANKS];
-    qemu_irq *pic;
     qemu_irq *irqs;
     PCIBus *pcibus;
     PowerPCCPU *cpu;
@@ -202,7 +201,7 @@ static void bamboo_init(MachineState *machine)
     irqs = g_malloc0(sizeof(qemu_irq) * PPCUIC_OUTPUT_NB);
     irqs[PPCUIC_OUTPUT_INT] = ((qemu_irq *)env->irq_inputs)[PPC40x_INPUT_INT];
     irqs[PPCUIC_OUTPUT_CINT] = ((qemu_irq *)env->irq_inputs)[PPC40x_INPUT_CINT];
-    pic = ppcuic_init(env, irqs, 0x0C0, 0, 1);
+    machine->irqs = ppcuic_init(env, irqs, 0x0C0, 0, 1);
 
     /* SDRAM controller */
     memset(ram_bases, 0, sizeof(ram_bases));
@@ -212,14 +211,16 @@ static void bamboo_init(MachineState *machine)
                                    ram_bases, ram_sizes,
                                    ppc440ep_sdram_bank_sizes);
     /* XXX 440EP's ECC interrupts are on UIC1, but we've only created UIC0. */
-    ppc4xx_sdram_init(env, pic[14], PPC440EP_SDRAM_NR_BANKS, ram_memories,
-                      ram_bases, ram_sizes, 1);
+    ppc4xx_sdram_init(env, machine->irqs[14], PPC440EP_SDRAM_NR_BANKS,
+                      ram_memories, ram_bases, ram_sizes, 1);
 
     /* PCI */
     dev = sysbus_create_varargs(TYPE_PPC4xx_PCI_HOST_BRIDGE,
                                 PPC440EP_PCI_CONFIG,
-                                pic[pci_irq_nrs[0]], pic[pci_irq_nrs[1]],
-                                pic[pci_irq_nrs[2]], pic[pci_irq_nrs[3]],
+                                machine->irqs[pci_irq_nrs[0]],
+                                machine->irqs[pci_irq_nrs[1]],
+                                machine->irqs[pci_irq_nrs[2]],
+                                machine->irqs[pci_irq_nrs[3]],
                                 NULL);
     pcibus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
     if (!pcibus) {
@@ -232,12 +233,12 @@ static void bamboo_init(MachineState *machine)
     memory_region_add_subregion(get_system_memory(), PPC440EP_PCI_IO, isa);
 
     if (serial_hds[0] != NULL) {
-        serial_mm_init(address_space_mem, 0xef600300, 0, pic[0],
+        serial_mm_init(address_space_mem, 0xef600300, 0, machine->irqs[0],
                        PPC_SERIAL_MM_BAUDBASE, serial_hds[0],
                        DEVICE_BIG_ENDIAN);
     }
     if (serial_hds[1] != NULL) {
-        serial_mm_init(address_space_mem, 0xef600400, 0, pic[1],
+        serial_mm_init(address_space_mem, 0xef600400, 0, machine->irqs[1],
                        PPC_SERIAL_MM_BAUDBASE, serial_hds[1],
                        DEVICE_BIG_ENDIAN);
     }
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 1f11881..18a0f93 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -160,6 +160,7 @@ struct MachineState {
     char *initrd_filename;
     const char *cpu_model;
     AccelState *accelerator;
+    qemu_irq *irqs;
 };
 
 #endif
-- 
2.0.4

[Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/mips/mips_jazz.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 2c153e0..259458b 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -135,7 +135,7 @@ static void mips_jazz_init(MachineState *machine,
     MIPSCPU *cpu;
     CPUClass *cc;
     CPUMIPSState *env;
-    qemu_irq *rc4030, *i8259;
+    qemu_irq *i8259;
     rc4030_dma *dmas;
     void* rc4030_opaque;
     MemoryRegion *isa_mem = g_new(MemoryRegion, 1);
@@ -218,7 +218,7 @@ static void mips_jazz_init(MachineState *machine,
     cpu_mips_clock_init(env);
 
     /* Chipset */
-    rc4030_opaque = rc4030_init(env->irq[6], env->irq[3], &rc4030, &dmas,
+    rc4030_opaque = rc4030_init(env->irq[6], env->irq[3], &machine->irqs, &dmas,
                                 address_space);
     memory_region_init_io(dma_dummy, NULL, &dma_dummy_ops, NULL, "dummy_dma", 0x1000);
     memory_region_add_subregion(address_space, 0x8000d000, dma_dummy);
@@ -246,7 +246,7 @@ static void mips_jazz_init(MachineState *machine,
         sysbus = SYS_BUS_DEVICE(dev);
         sysbus_mmio_map(sysbus, 0, 0x60080000);
         sysbus_mmio_map(sysbus, 1, 0x40000000);
-        sysbus_connect_irq(sysbus, 0, rc4030[3]);
+        sysbus_connect_irq(sysbus, 0, machine->irqs[3]);
         {
             /* Simple ROM, so user doesn't have to provide one */
             MemoryRegion *rom_mr = g_new(MemoryRegion, 1);
@@ -272,8 +272,8 @@ static void mips_jazz_init(MachineState *machine,
         if (!nd->model)
             nd->model = g_strdup("dp83932");
         if (strcmp(nd->model, "dp83932") == 0) {
-            dp83932_init(nd, 0x80001000, 2, get_system_memory(), rc4030[4],
-                         rc4030_opaque, rc4030_dma_memory_rw);
+            dp83932_init(nd, 0x80001000, 2, get_system_memory(),
+                         machine->irqs[4], rc4030_opaque, rc4030_dma_memory_rw);
             break;
         } else if (is_help_option(nd->model)) {
             fprintf(stderr, "qemu: Supported NICs: dp83932\n");
@@ -287,7 +287,7 @@ static void mips_jazz_init(MachineState *machine,
     /* SCSI adapter */
     esp_init(0x80002000, 0,
              rc4030_dma_read, rc4030_dma_write, dmas[0],
-             rc4030[5], &esp_reset, &dma_enable);
+             machine->irqs[5], &esp_reset, &dma_enable);
 
     /* Floppy */
     if (drive_get_max_bus(IF_FLOPPY) >= MAX_FD) {
@@ -297,7 +297,7 @@ static void mips_jazz_init(MachineState *machine,
     for (n = 0; n < MAX_FD; n++) {
         fds[n] = drive_get(IF_FLOPPY, 0, n);
     }
-    fdctrl_init_sysbus(rc4030[1], 0, 0x80003000, fds);
+    fdctrl_init_sysbus(machine->irqs[1], 0, 0x80003000, fds);
 
     /* Real time clock */
     rtc_init(isa_bus, 1980, NULL);
@@ -305,25 +305,25 @@ static void mips_jazz_init(MachineState *machine,
     memory_region_add_subregion(address_space, 0x80004000, rtc);
 
     /* Keyboard (i8042) */
-    i8042_mm_init(rc4030[6], rc4030[7], i8042, 0x1000, 0x1);
+    i8042_mm_init(machine->irqs[6], machine->irqs[7], i8042, 0x1000, 0x1);
     memory_region_add_subregion(address_space, 0x80005000, i8042);
 
     /* Serial ports */
     if (serial_hds[0]) {
-        serial_mm_init(address_space, 0x80006000, 0, rc4030[8], 8000000/16,
-                       serial_hds[0], DEVICE_NATIVE_ENDIAN);
+        serial_mm_init(address_space, 0x80006000, 0, machine->irqs[8],
+                       8000000/16, serial_hds[0], DEVICE_NATIVE_ENDIAN);
     }
     if (serial_hds[1]) {
-        serial_mm_init(address_space, 0x80007000, 0, rc4030[9], 8000000/16,
-                       serial_hds[1], DEVICE_NATIVE_ENDIAN);
+        serial_mm_init(address_space, 0x80007000, 0, machine->irqs[9],
+                       8000000/16, serial_hds[1], DEVICE_NATIVE_ENDIAN);
     }
 
     /* Parallel port */
     if (parallel_hds[0])
-        parallel_mm_init(address_space, 0x80008000, 0, rc4030[0],
+        parallel_mm_init(address_space, 0x80008000, 0, machine->irqs[0],
                          parallel_hds[0]);
 
-    /* FIXME: missing Jazz sound at 0x8000c000, rc4030[2] */
+    /* FIXME: missing Jazz sound at 0x8000c000, machine->irqs[2] */
 
     /* NVRAM */
     dev = qdev_create(NULL, "ds1225y");
-- 
2.0.4

[Qemu-devel] [PATCH v2 3/7] hw/m68k/an5206.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/m68k/an5206.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/m68k/an5206.c b/hw/m68k/an5206.c
index f63ab2b..6ebf2d2 100644
--- a/hw/m68k/an5206.c
+++ b/hw/m68k/an5206.c
@@ -58,7 +58,7 @@ static void an5206_init(MachineState *machine)
     vmstate_register_ram_global(sram);
     memory_region_add_subregion(address_space_mem, AN5206_RAMBAR_ADDR, sram);
 
-    mcf5206_init(address_space_mem, AN5206_MBAR_ADDR, cpu);
+    machine->irqs = mcf5206_init(address_space_mem, AN5206_MBAR_ADDR, cpu);
 
     /* Load kernel.  */
     if (!kernel_filename) {
-- 
2.0.4

[Qemu-devel] [PATCH v2 4/7] hw/sh4/r2d.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/sh4/r2d.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 4221060..39edefd 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -230,7 +230,6 @@ static void r2d_init(MachineState *machine)
     ResetData *reset_info;
     struct SH7750State *s;
     MemoryRegion *sdram = g_new(MemoryRegion, 1);
-    qemu_irq *irq;
     DriveInfo *dinfo;
     int i;
     DeviceState *dev;
@@ -260,7 +259,7 @@ static void r2d_init(MachineState *machine)
     memory_region_add_subregion(address_space_mem, SDRAM_BASE, sdram);
     /* Register peripherals */
     s = sh7750_init(cpu, address_space_mem);
-    irq = r2d_fpga_init(address_space_mem, 0x04000000, sh7750_irl(s));
+    machine->irqs = r2d_fpga_init(address_space_mem, 0x04000000, sh7750_irl(s));
 
     dev = qdev_create(NULL, "sh_pci");
     busdev = SYS_BUS_DEVICE(dev);
@@ -268,19 +267,19 @@ static void r2d_init(MachineState *machine)
     pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci"));
     sysbus_mmio_map(busdev, 0, P4ADDR(0x1e200000));
     sysbus_mmio_map(busdev, 1, A7ADDR(0x1e200000));
-    sysbus_connect_irq(busdev, 0, irq[PCI_INTA]);
-    sysbus_connect_irq(busdev, 1, irq[PCI_INTB]);
-    sysbus_connect_irq(busdev, 2, irq[PCI_INTC]);
-    sysbus_connect_irq(busdev, 3, irq[PCI_INTD]);
+    sysbus_connect_irq(busdev, 0, machine->irqs[PCI_INTA]);
+    sysbus_connect_irq(busdev, 1, machine->irqs[PCI_INTB]);
+    sysbus_connect_irq(busdev, 2, machine->irqs[PCI_INTC]);
+    sysbus_connect_irq(busdev, 3, machine->irqs[PCI_INTD]);
 
     sm501_init(address_space_mem, 0x10000000, SM501_VRAM_SIZE,
-               irq[SM501], serial_hds[2]);
+               machine->irqs[SM501], serial_hds[2]);
 
     /* onboard CF (True IDE mode, Master only). */
     dinfo = drive_get(IF_IDE, 0, 0);
     dev = qdev_create(NULL, "mmio-ide");
     busdev = SYS_BUS_DEVICE(dev);
-    sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
+    sysbus_connect_irq(busdev, 0, machine->irqs[CF_IDE]);
     qdev_prop_set_uint32(dev, "shift", 1);
     qdev_init_nofail(dev);
     sysbus_mmio_map(busdev, 0, 0x14001000);
-- 
2.0.4

[Qemu-devel] [PATCH v2 5/7] hw/arm/palm.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/arm/palm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/arm/palm.c b/hw/arm/palm.c
index 7f1cfb8..6fe28e5 100644
--- a/hw/arm/palm.c
+++ b/hw/arm/palm.c
@@ -156,7 +156,7 @@ static void palmte_onoff_gpios(void *opaque, int line, int level)
     }
 }
 
-static void palmte_gpio_setup(struct omap_mpu_state_s *cpu)
+static qemu_irq *palmte_gpio_setup(struct omap_mpu_state_s *cpu)
 {
     qemu_irq *misc_gpio;
 
@@ -183,6 +183,8 @@ static void palmte_gpio_setup(struct omap_mpu_state_s *cpu)
     qemu_irq_raise(omap_mpuio_in_get(cpu->mpuio)[6]);
     qemu_irq_raise(omap_mpuio_in_get(cpu->mpuio)[7]);
     qemu_irq_raise(omap_mpuio_in_get(cpu->mpuio)[11]);
+
+    return misc_gpio;
 }
 
 static struct arm_boot_info palmte_binfo = {
@@ -236,7 +238,7 @@ static void palmte_init(MachineState *machine)
 
     qemu_add_kbd_event_handler(palmte_button_event, mpu);
 
-    palmte_gpio_setup(mpu);
+    machine->irqs = palmte_gpio_setup(mpu);
 
     /* Setup initial (reset) machine state */
     if (nb_option_roms) {
-- 
2.0.4

[Qemu-devel] [PATCH v2 6/7] hw/arm/spitz.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/arm/spitz.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 5bf032a..454919d 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -812,8 +812,8 @@ static void spitz_out_switch(void *opaque, int line, int level)
 #define SPITZ_SCP2_BACKLIGHT_ON		8
 #define SPITZ_SCP2_MIC_BIAS		9
 
-static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
-                DeviceState *scp0, DeviceState *scp1)
+static qemu_irq *spitz_scoop_gpio_setup(PXA2xxState *cpu,
+                     DeviceState *scp0, DeviceState *scp1)
 {
     qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, cpu, 8);
 
@@ -828,6 +828,8 @@ static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
     }
 
     qdev_connect_gpio_out(scp0, SPITZ_SCP_ADC_TEMP_ON, outsignals[6]);
+
+    return outsignals;
 }
 
 #define SPITZ_GPIO_HSYNC		22
@@ -928,7 +930,7 @@ static void spitz_common_init(MachineState *machine,
         scp1 = sysbus_create_simple("scoop", 0x08800040, NULL);
     }
 
-    spitz_scoop_gpio_setup(mpu, scp0, scp1);
+    machine->irqs = spitz_scoop_gpio_setup(mpu, scp0, scp1);
 
     spitz_gpio_setup(mpu, (model == akita) ? 1 : 2);
 
-- 
2.0.4

[Qemu-devel] [PATCH v2 7/7] hw/arm/tosa.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/arm/tosa.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 73572eb..4711514 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -85,10 +85,8 @@ static void tosa_out_switch(void *opaque, int line, int level)
 }
 
 
-static void tosa_gpio_setup(PXA2xxState *cpu,
-                DeviceState *scp0,
-                DeviceState *scp1,
-                TC6393xbState *tmio)
+static qemu_irq *tosa_gpio_setup(PXA2xxState *cpu, DeviceState *scp0,
+                                 DeviceState *scp1, TC6393xbState *tmio)
 {
     qemu_irq *outsignals = qemu_allocate_irqs(tosa_out_switch, cpu, 4);
     /* MMC/SD host */
@@ -117,6 +115,8 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
 
     /* UDC Vbus */
     qemu_irq_raise(qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_USB_IN));
+
+    return outsignals;
 }
 
 static uint32_t tosa_ssp_tansfer(SSISlave *dev, uint32_t value)
@@ -238,7 +238,7 @@ static void tosa_init(MachineState *machine)
     scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
     scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
 
-    tosa_gpio_setup(mpu, scp0, scp1, tmio);
+    machine->irqs = tosa_gpio_setup(mpu, scp0, scp1, tmio);
 
     tosa_microdrive_attach(mpu);
 
-- 
2.0.4

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 1/7] hw/ppc/ppc440_bamboo.c: Add a member in MachineState to store irq array

[Michael Tokarev]

30.05.2015 10:54, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Here we add a member in MachineState to store the irq array returned
> from qemu_allocate_irqs. Then these irq arrays will be free before QEMU
> exit and it fixes the memory leak spotted by valgrind.

Where the new member (machine->irqs) is being freed?  Can we add the
generic machinery for it first, and next convert all places to it one
by one?

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Michael Tokarev]

30.05.2015 10:54, Shannon Zhao пишет:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/mips/mips_jazz.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
> index 2c153e0..259458b 100644
> --- a/hw/mips/mips_jazz.c
> +++ b/hw/mips/mips_jazz.c
> @@ -135,7 +135,7 @@ static void mips_jazz_init(MachineState *machine,
>      MIPSCPU *cpu;
>      CPUClass *cc;
>      CPUMIPSState *env;
> -    qemu_irq *rc4030, *i8259;
> +    qemu_irq *i8259;

Hm. Why do you only cover rc4030, not i8259?

Besides, in order to keep the changes smaller, I think it is okay to
keep the variables like that, here and in the rest of the function,
and only add assignment of it to machine->irqs.  This way, we also
keep semantic names of the variables, rc4030[i] is easier to understand
than machine->irqs[i], the former's more specific.

BTW, there's also cpu_exit_irq in this function whose allocation also
suffers from qemu_allocate_irqs(..., 1) API abuse.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 1/7] hw/ppc/ppc440_bamboo.c: Add a member in MachineState to store irq array

[Shannon Zhao]

On 2015/6/4 22:12, Michael Tokarev wrote:
> 30.05.2015 10:54, Shannon Zhao wrote:
>> >From: Shannon Zhao<shannon.zhao@linaro.org>
>> >
>> >Here we add a member in MachineState to store the irq array returned
>> >from qemu_allocate_irqs. Then these irq arrays will be free before QEMU
>> >exit and it fixes the memory leak spotted by valgrind.
> Where the new member (machine->irqs) is being freed?

The machine->irqs will not be freed until the machine dies. Since 
machine init function only runs once and the allocated memories could be 
freed when QEMU exits.

> Can we add the
> generic machinery for it first, and next convert all places to it one
> by one?

-- 
Shannon

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

On 2015/6/4 22:18, Michael Tokarev wrote:
> 30.05.2015 10:54, Shannon Zhao пишет:
>> >From: Shannon Zhao<shannon.zhao@linaro.org>
>> >
>> >Signed-off-by: Shannon Zhao<zhaoshenglong@huawei.com>
>> >Signed-off-by: Shannon Zhao<shannon.zhao@linaro.org>
>> >---
>> >  hw/mips/mips_jazz.c | 28 ++++++++++++++--------------
>> >  1 file changed, 14 insertions(+), 14 deletions(-)
>> >
>> >diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
>> >index 2c153e0..259458b 100644
>> >--- a/hw/mips/mips_jazz.c
>> >+++ b/hw/mips/mips_jazz.c
>> >@@ -135,7 +135,7 @@ static void mips_jazz_init(MachineState *machine,
>> >      MIPSCPU *cpu;
>> >      CPUClass *cc;
>> >      CPUMIPSState *env;
>> >-    qemu_irq *rc4030, *i8259;
>> >+    qemu_irq *i8259;
> Hm. Why do you only cover rc4030, not i8259?
>

As i8259 is stored in ISABus->irqs by function isa_bus_irqs.

void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
{
     if (!bus) {
         hw_error("Can't set isa irqs with no isa bus present.");
     }
     bus->irqs = irqs;
}

> Besides, in order to keep the changes smaller, I think it is okay to
> keep the variables like that, here and in the rest of the function,
> and only add assignment of it to machine->irqs.  This way, we also
> keep semantic names of the variables, rc4030[i] is easier to understand
> than machine->irqs[i], the former's more specific.
>
Agree.

> BTW, there's also cpu_exit_irq in this function whose allocation also
> suffers from qemu_allocate_irqs(..., 1) API abuse.

Yeah, missed this one.

-- 
Shannon

Re: [Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Michael Tokarev]

04.06.2015 17:30, Shannon Zhao wrote:

>> BTW, there's also cpu_exit_irq in this function whose allocation also
>> suffers from qemu_allocate_irqs(..., 1) API abuse.
> 
> Yeah, missed this one.

This one isn't that simple, because it passes the result to DMA_init
which expects a pointer, even if it uses only one element of the
array.

/mjt

Re: [Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

On 2015/6/4 22:32, Michael Tokarev wrote:
> 04.06.2015 17:30, Shannon Zhao wrote:
>
>>> >>BTW, there's also cpu_exit_irq in this function whose allocation also
>>> >>suffers from qemu_allocate_irqs(..., 1) API abuse.
>> >
>> >Yeah, missed this one.
> This one isn't that simple, because it passes the result to DMA_init
> which expects a pointer, even if it uses only one element of the
> array.

Yeah, but I think something like below would work.

     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
     DMA_init(0, cpu_exit_irq);

-- 
Shannon

Re: [Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Michael Tokarev]

04.06.2015 17:51, Shannon Zhao wrote:
> Yeah, but I think something like below would work.
> 
>     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>     DMA_init(0, cpu_exit_irq);

NO!!! :)

/mjt

Re: [Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

On 2015/6/4 22:57, Michael Tokarev wrote:
> 04.06.2015 17:51, Shannon Zhao wrote:
>> >Yeah, but I think something like below would work.
>> >
>> >     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>> >     DMA_init(0, cpu_exit_irq);
> NO!!!:)

Hmm...really?

This is used by DMA_schedule.

void DMA_schedule(int nchan)
{
     struct dma_cont *d = &dma_controllers[nchan > 3];

     qemu_irq_pulse(*d->cpu_request_exit);
}

-- 
Shannon

Re: [Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Michael Tokarev]

04.06.2015 18:09, Shannon Zhao wrote:
> On 2015/6/4 22:57, Michael Tokarev wrote:
>> 04.06.2015 17:51, Shannon Zhao wrote:
>>> >Yeah, but I think something like below would work.
>>> >
>>> >     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>>> >     DMA_init(0, cpu_exit_irq);
>> NO!!!:)
> 
> Hmm...really?

Really-really :)

You're getting an address of a temporary local variable
returned from qemu_allocate_irq, and store that address
in some pointer to be used later.  This address is not
valid once we return from this function.

/mjt

Re: [Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

On 2015/6/4 22:57, Michael Tokarev wrote:
> 04.06.2015 17:51, Shannon Zhao wrote:
>> >Yeah, but I think something like below would work.
>> >
>> >     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>> >     DMA_init(0, cpu_exit_irq);
> NO!!!:)

Oh, will rethink about it. But I think maybe it's unnecessary to fix it 
as it actually requires a pointer which stores qemu_irq.

-- 
Shannon

Re: [Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Michael Tokarev]

04.06.2015 18:14, Michael Tokarev wrote:
> 04.06.2015 18:09, Shannon Zhao wrote:
>> On 2015/6/4 22:57, Michael Tokarev wrote:
>>> 04.06.2015 17:51, Shannon Zhao wrote:
>>>>> Yeah, but I think something like below would work.
>>>>>
>>>>>     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>>>>>     DMA_init(0, cpu_exit_irq);
>>> NO!!!:)
>>
>> Hmm...really?
> 
> Really-really :)
> 
> You're getting an address of a temporary local variable
> returned from qemu_allocate_irq,

If this is valid at all, to which I'm not sure.

Even for this:

 int foo();

 int *var = &foo();

/mjt\

Re: [Qemu-devel] [PATCH v2 2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

[Shannon Zhao]

On 2015/6/4 23:17, Shannon Zhao wrote:
>
>
> On 2015/6/4 22:57, Michael Tokarev wrote:
>> 04.06.2015 17:51, Shannon Zhao wrote:
>>> >Yeah, but I think something like below would work.
>>> >
>>> >     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>>> >     DMA_init(0, cpu_exit_irq);
>> NO!!!:)
>
> Oh, will rethink about it. But I think maybe it's unnecessary to fix it
> as it actually requires a pointer which stores qemu_irq.

And if we want to use qemu_allocate_irq here, it will be something like:

cpu_exit_irq = g_new(qemu_irq, 1);
cpu_exit_irq[0] = qemu_allocate_irq(cpu_request_exit, NULL, 0);
DMA_init(0, cpu_exit_irq);

This is what exactly qemu_allocate_irqs does.

Or we modify the DMA_init to make it take qemu_irq as parameter not 
qemu_irq *

-- 
Shannon