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

[Qemu-devel] [PATCH v3 0/8] 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 v2：
  * Rebased on QEMU upstream
  * Add a member in MachineState first, then fix the memory leak [mjt]

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

Shannon Zhao (8):
  include/hw/boards.h: Add a member in MachineState to store irq array
  hw/ppc/ppc440_bamboo.c: Store irq array in MachineState to fix memory
    leak
  hw/sparc/leon3.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/ppc/ppc440_bamboo.c |  1 +
 hw/sh4/r2d.c           |  1 +
 hw/sparc/leon3.c       |  1 +
 include/hw/boards.h    |  1 +
 8 files changed, 19 insertions(+), 11 deletions(-)

-- 
2.0.4

[Qemu-devel] [PATCH v3 1/8] include/hw/boards.h: 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 will be used to fix the memory leak due to misuse
qemu_allocate_irqs.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 include/hw/boards.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 6379901..0dea769 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -148,6 +148,7 @@ struct MachineState {
     char *initrd_filename;
     const char *cpu_model;
     AccelState *accelerator;
+    qemu_irq *irqs;
 };
 
 #endif
-- 
2.0.4

Re: [Qemu-devel] [PATCH v3 1/8] include/hw/boards.h: Add a member in MachineState to store irq array

[Peter Crosthwaite]

On Tue, Jun 16, 2015 at 7:59 PM, Shannon Zhao <zhaoshenglong@huawei.com> 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 will be used to fix the memory leak due to misuse
> qemu_allocate_irqs.
>

This is based on an assumption that IRQ are a global concept. This (as
well as the one global address space) is something we really should
get away from. A better appoach would be to child the IRQs to the
Machine object (note that IRQs are QOM objects) for those machines
where this assumption makes sense. Then we don't have to pollute
common structs.

Regards,
Peter

> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  include/hw/boards.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 6379901..0dea769 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -148,6 +148,7 @@ struct MachineState {
>      char *initrd_filename;
>      const char *cpu_model;
>      AccelState *accelerator;
> +    qemu_irq *irqs;
>  };
>
>  #endif
> --
> 2.0.4
>
>
>

[Qemu-devel] [PATCH v3 2/8] hw/ppc/ppc440_bamboo.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/ppc/ppc440_bamboo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 778970a..944999d 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -203,6 +203,7 @@ static void bamboo_init(MachineState *machine)
     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 = pic;
 
     /* SDRAM controller */
     memset(ram_bases, 0, sizeof(ram_bases));
-- 
2.0.4

[Qemu-devel] [PATCH v3 3/8] hw/sparc/leon3.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/sparc/leon3.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 7f5dcd6..6733ebb 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -140,6 +140,7 @@ static void leon3_generic_hw_init(MachineState *machine)
 
     /* Allocate IRQ manager */
     grlib_irqmp_create(0x80000200, env, &cpu_irqs, MAX_PILS, &leon3_set_pil_in);
+    machine->irqs = cpu_irqs;
 
     env->qemu_irq_ack = leon3_irq_manager;
 
-- 
2.0.4

[Qemu-devel] [PATCH v3 4/8] 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 v3 5/8] 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 5e22ed7..7f621b2 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -261,6 +261,7 @@ static void r2d_init(MachineState *machine)
     /* Register peripherals */
     s = sh7750_init(cpu, address_space_mem);
     irq = r2d_fpga_init(address_space_mem, 0x04000000, sh7750_irl(s));
+    machine->irqs = irq;
 
     dev = qdev_create(NULL, "sh_pci");
     busdev = SYS_BUS_DEVICE(dev);
-- 
2.0.4

[Qemu-devel] [PATCH v3 6/8] 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 v3 7/8] 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 v3 8/8] 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] [PATCH v3 0/8] Add a member in MachineState to store irq array

[Michael Tokarev]

17.06.2015 05:59, Shannon Zhao wrote:
> 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 v2：
>   * Rebased on QEMU upstream
>   * Add a member in MachineState first, then fix the memory leak [mjt]

Um.  This is not what I asked.  It was a small suggestion, to add
infrastructure first use it in subsequent patches.

But the main question was what this infrastructure, what this patchset
actually brings us?  You just store the irq pointer in machine state,
that's okay, but WHAT FOR?  If your intention is to free resources,
this intention isn't met.  The end result of this whole change is a no-op,
there's nothing changed, the memory which has been allocated isn't being
freed.

So the actual question is: what's the point of this patchset?

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v3 0/8] Add a member in MachineState to store irq array

[Shannon Zhao]

On 2015/6/17 15:30, Michael Tokarev wrote:
> 17.06.2015 05:59, Shannon Zhao wrote:
>> 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 v2：
>>   * Rebased on QEMU upstream
>>   * Add a member in MachineState first, then fix the memory leak [mjt]
> 
> Um.  This is not what I asked.  It was a small suggestion, to add
> infrastructure first use it in subsequent patches.
> 
> But the main question was what this infrastructure, what this patchset
> actually brings us?  You just store the irq pointer in machine state,
> that's okay, but WHAT FOR?  If your intention is to free resources,
> this intention isn't met.  The end result of this whole change is a no-op,
> there's nothing changed, the memory which has been allocated isn't being
> freed.
> 

Ok, will rethink about.

> So the actual question is: what's the point of this patchset?
> 
> Thanks,
> 
> /mjt
> 
> 
> .
> 

-- 
Shannon