[Qemu-devel] [PATCH 0/3] ide: ahci: fix memory leak in device unit

[Qemu-devel] [PATCH 0/3] ide: ahci: fix memory leak in device unit

[Li Qiang]

As the pci ahci can be hotplug and unplug, in the ahci unrealize
function it should free all the resource once allocated in the
realized function. This patchset first add cleanup function in
core layer and then call it in the ahci unit.

Li Qiang (3):
  ide: qdev: register ide bus unrealize function
  ide: core: add cleanup function
  ide: ahci: call cleanup function in ahci unit

 hw/ide/ahci.c             | 12 ++++++++++++
 hw/ide/core.c             |  8 ++++++++
 hw/ide/qdev.c             | 12 ++++++------
 include/hw/ide/internal.h |  1 +
 4 files changed, 27 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] ide: qdev: register ide bus unrealize function

[Li Qiang]

we have an idebus unrealize function, but it was being
registered as the unrealize function for the IDE Device,
so it was not getting invoked on device teardown because
nothing is "unrealizing" the IDE devices themselves.

Suggested-by: John Snow <jsnow@redhat.com>
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/ide/qdev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index dbaa75c..fbf7aa5 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -31,7 +31,7 @@
 /* --------------------------------- */
 
 static char *idebus_get_fw_dev_path(DeviceState *dev);
-static void idebus_unrealize(DeviceState *qdev, Error **errp);
+static void idebus_unrealize(BusState *qdev, Error **errp);
 
 static Property ide_props[] = {
     DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
@@ -43,14 +43,15 @@ static void ide_bus_class_init(ObjectClass *klass, void *data)
     BusClass *k = BUS_CLASS(klass);
 
     k->get_fw_dev_path = idebus_get_fw_dev_path;
+    k->unrealize = idebus_unrealize;
 }
 
-static void idebus_unrealize(DeviceState *qdev, Error **errp)
+static void idebus_unrealize(BusState *bus, Error **errp)
 {
-    IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
+    IDEBus *ibus = IDE_BUS(bus);
 
-    if (bus->vmstate) {
-        qemu_del_vm_change_state_handler(bus->vmstate);
+    if (ibus->vmstate) {
+        qemu_del_vm_change_state_handler(ibus->vmstate);
     }
 }
 
@@ -365,7 +366,6 @@ static void ide_device_class_init(ObjectClass *klass, void *data)
     k->init = ide_qdev_init;
     set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
     k->bus_type = TYPE_IDE_BUS;
-    k->unrealize = idebus_unrealize;
     k->props = ide_props;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/3] ide: qdev: register ide bus unrealize function

[Philippe Mathieu-Daudé]

On 03/02/2017 07:08 AM, Li Qiang wrote:
> we have an idebus unrealize function, but it was being
> registered as the unrealize function for the IDE Device,
> so it was not getting invoked on device teardown because
> nothing is "unrealizing" the IDE devices themselves.

nice catch

>
> Suggested-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/ide/qdev.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index dbaa75c..fbf7aa5 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -31,7 +31,7 @@
>  /* --------------------------------- */
>
>  static char *idebus_get_fw_dev_path(DeviceState *dev);
> -static void idebus_unrealize(DeviceState *qdev, Error **errp);
> +static void idebus_unrealize(BusState *qdev, Error **errp);
>
>  static Property ide_props[] = {
>      DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
> @@ -43,14 +43,15 @@ static void ide_bus_class_init(ObjectClass *klass, void *data)
>      BusClass *k = BUS_CLASS(klass);
>
>      k->get_fw_dev_path = idebus_get_fw_dev_path;
> +    k->unrealize = idebus_unrealize;
>  }
>
> -static void idebus_unrealize(DeviceState *qdev, Error **errp)
> +static void idebus_unrealize(BusState *bus, Error **errp)
>  {
> -    IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
> +    IDEBus *ibus = IDE_BUS(bus);

:)

>
> -    if (bus->vmstate) {
> -        qemu_del_vm_change_state_handler(bus->vmstate);
> +    if (ibus->vmstate) {
> +        qemu_del_vm_change_state_handler(ibus->vmstate);
>      }
>  }
>
> @@ -365,7 +366,6 @@ static void ide_device_class_init(ObjectClass *klass, void *data)
>      k->init = ide_qdev_init;
>      set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
>      k->bus_type = TYPE_IDE_BUS;
> -    k->unrealize = idebus_unrealize;
>      k->props = ide_props;
>  }
>
>

[Qemu-devel] [PATCH 2/3] ide: core: add cleanup function

[Li Qiang]

As the pci ahci can be hotplug and unplug, in the ahci unrealize
function it should free all the resource once allocated in the
realized function. This patch add ide_exit to free the resource.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/ide/core.c             | 8 ++++++++
 include/hw/ide/internal.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index cfa5de6..e971a94 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2603,6 +2603,14 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
     bus->dma = &ide_dma_nop;
 }
 
+void ide_exit(IDEState *s)
+{
+    timer_del(s->sector_write_timer);
+    timer_free(s->sector_write_timer);
+    qemu_vfree(s->smart_selftest_data);
+    qemu_vfree(s->io_buffer);
+}
+
 static const MemoryRegionPortio ide_portio_list[] = {
     { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
     { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 88dc118..482a951 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -607,6 +607,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    uint32_t cylinders, uint32_t heads, uint32_t secs,
                    int chs_trans);
 void ide_init2(IDEBus *bus, qemu_irq irq);
+void ide_exit(IDEState *s);
 void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] ide: ahci: call cleanup function in ahci unit

[Li Qiang]

This can avoid memory leak when hotunplug the ahci device.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/ide/ahci.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 6a17acf..f60826d 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1485,6 +1485,18 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
 
 void ahci_uninit(AHCIState *s)
 {
+    int i, j;
+
+    for (i = 0; i < s->ports; i++) {
+        AHCIDevice *ad = &s->dev[i];
+
+        for (j = 0; j < 2; j++) {
+            IDEState *s = &ad->port.ifs[j];
+
+            ide_exit(s);
+        }
+    }
+
     g_free(s->dev);
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] ide: ahci: fix memory leak in device unit

[John Snow]

On 03/02/2017 05:08 AM, Li Qiang wrote:
> As the pci ahci can be hotplug and unplug, in the ahci unrealize
> function it should free all the resource once allocated in the
> realized function. This patchset first add cleanup function in
> core layer and then call it in the ahci unit.
> 
> Li Qiang (3):
>   ide: qdev: register ide bus unrealize function
>   ide: core: add cleanup function
>   ide: ahci: call cleanup function in ahci unit
> 
>  hw/ide/ahci.c             | 12 ++++++++++++
>  hw/ide/core.c             |  8 ++++++++
>  hw/ide/qdev.c             | 12 ++++++------
>  include/hw/ide/internal.h |  1 +
>  4 files changed, 27 insertions(+), 6 deletions(-)
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [PATCH 0/3] ide: ahci: fix memory leak in device unit

[Li Qiang]

Hello John,

Does this patch go to upstream?

Thanks.

2017-03-04 6:48 GMT+08:00 John Snow <jsnow@redhat.com>:

>
>
> On 03/02/2017 05:08 AM, Li Qiang wrote:
> > As the pci ahci can be hotplug and unplug, in the ahci unrealize
> > function it should free all the resource once allocated in the
> > realized function. This patchset first add cleanup function in
> > core layer and then call it in the ahci unit.
> >
> > Li Qiang (3):
> >   ide: qdev: register ide bus unrealize function
> >   ide: core: add cleanup function
> >   ide: ahci: call cleanup function in ahci unit
> >
> >  hw/ide/ahci.c             | 12 ++++++++++++
> >  hw/ide/core.c             |  8 ++++++++
> >  hw/ide/qdev.c             | 12 ++++++------
> >  include/hw/ide/internal.h |  1 +
> >  4 files changed, 27 insertions(+), 6 deletions(-)
> >
>
> Thanks, applied to my IDE tree:
>
> https://github.com/jnsnow/qemu/commits/ide
> https://github.com/jnsnow/qemu.git
>
> --js
>

Re: [Qemu-devel] [PATCH 0/3] ide: ahci: fix memory leak in device unit

[John Snow]

On 03/13/2017 10:41 PM, Li Qiang wrote:
> Hello John,
> 
> Does this patch go to upstream?
> 
> Thanks.
> 

Yes, it will be included for 2.9. I usually send my PRs on Friday, but I
was out of town so it got delayed. It qualifies as a bugfix though, so
it can still go in.

--js

> 2017-03-04 6:48 GMT+08:00 John Snow <jsnow@redhat.com
> <mailto:jsnow@redhat.com>>:
> 
> 
> 
>     On 03/02/2017 05:08 AM, Li Qiang wrote:
>     > As the pci ahci can be hotplug and unplug, in the ahci unrealize
>     > function it should free all the resource once allocated in the
>     > realized function. This patchset first add cleanup function in
>     > core layer and then call it in the ahci unit.
>     >
>     > Li Qiang (3):
>     >   ide: qdev: register ide bus unrealize function
>     >   ide: core: add cleanup function
>     >   ide: ahci: call cleanup function in ahci unit
>     >
>     >  hw/ide/ahci.c             | 12 ++++++++++++
>     >  hw/ide/core.c             |  8 ++++++++
>     >  hw/ide/qdev.c             | 12 ++++++------
>     >  include/hw/ide/internal.h |  1 +
>     >  4 files changed, 27 insertions(+), 6 deletions(-)
>     >
> 
>     Thanks, applied to my IDE tree:
> 
>     https://github.com/jnsnow/qemu/commits/ide
>     <https://github.com/jnsnow/qemu/commits/ide>
>     https://github.com/jnsnow/qemu.git <https://github.com/jnsnow/qemu.git>
> 
>     --js
> 
>