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

[Qemu-devel] [PATCH 0/2] 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 (2):
  ide: core: add cleanup function
  ide: ahci: call cleanup function in ahci unit

 hw/ide/ahci.c             |  8 ++++++++
 hw/ide/core.c             | 21 +++++++++++++++++++++
 include/hw/ide/internal.h |  2 ++
 3 files changed, 31 insertions(+)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] 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 adds two cleanup function.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 43709e5..8fe5896 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
     }
 }
 
+void ide_unregister_restart_cb(IDEBus *bus)
+{
+    if (bus->dma->ops->restart_dma) {
+        qemu_del_vm_change_state_handler(bus->vmstate);
+    }
+}
+
 static IDEDMA ide_dma_nop = {
     .ops = &ide_dma_nop_ops,
     .aiocb = NULL,
@@ -2603,6 +2610,20 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
     bus->dma = &ide_dma_nop;
 }
 
+void ide_exit(IDEBus *bus)
+{
+    int i;
+
+    for (i = 0; i < 2; i++) {
+        IDEState *s = &bus->ifs[i];
+
+        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..09b0404 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -607,8 +607,10 @@ 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(IDEBus *bus);
 void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
+void ide_unregister_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3c19bda..56f68a8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1485,6 +1485,14 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
 
 void ahci_uninit(AHCIState *s)
 {
+    int i;
+
+    for (i = 0; i < s->ports; i++) {
+        AHCIDevice *ad = &s->dev[i];
+
+        ide_unregister_restart_cb(&ad->port);
+        ide_exit(&ad->port);
+    }
     g_free(s->dev);
 }
 
-- 
1.8.3.1

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

[John Snow]

On 02/09/2017 02:04 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 patch adds two cleanup function.
> 
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
>  hw/ide/core.c             | 21 +++++++++++++++++++++
>  include/hw/ide/internal.h |  2 ++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 43709e5..8fe5896 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
>      }
>  }
>  
> +void ide_unregister_restart_cb(IDEBus *bus)
> +{
> +    if (bus->dma->ops->restart_dma) {
> +        qemu_del_vm_change_state_handler(bus->vmstate);
> +    }
> +}
> +

Doesn't this conflict with qdev.c's idebus_unrealize call?

>  static IDEDMA ide_dma_nop = {
>      .ops = &ide_dma_nop_ops,
>      .aiocb = NULL,
> @@ -2603,6 +2610,20 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
>      bus->dma = &ide_dma_nop;
>  }
>  
> +void ide_exit(IDEBus *bus)
> +{
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        IDEState *s = &bus->ifs[i];
> +
> +        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..09b0404 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -607,8 +607,10 @@ 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(IDEBus *bus);
>  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>  void ide_register_restart_cb(IDEBus *bus);
> +void ide_unregister_restart_cb(IDEBus *bus);
>  
>  void ide_exec_cmd(IDEBus *bus, uint32_t val);
>  
> 

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

[Li Qiang]

Hello John,

2017-02-10 3:42 GMT+08:00 John Snow <jsnow@redhat.com>:

>
>
> On 02/09/2017 02:04 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 patch adds two cleanup function.
> >
> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > ---
> >  hw/ide/core.c             | 21 +++++++++++++++++++++
> >  include/hw/ide/internal.h |  2 ++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 43709e5..8fe5896 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
> >      }
> >  }
> >
> > +void ide_unregister_restart_cb(IDEBus *bus)
> > +{
> > +    if (bus->dma->ops->restart_dma) {
> > +        qemu_del_vm_change_state_handler(bus->vmstate);
> > +    }
> > +}
> > +
>
> Doesn't this conflict with qdev.c's idebus_unrealize call?


As far as I can see, No conflict. Let's get a confirmation.

Hello Paolo,

Does this patch have any conflict/affect with the qdev?

Thanks.



> >  static IDEDMA ide_dma_nop = {
> >      .ops = &ide_dma_nop_ops,
> >      .aiocb = NULL,
> > @@ -2603,6 +2610,20 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
> >      bus->dma = &ide_dma_nop;
> >  }
> >
> > +void ide_exit(IDEBus *bus)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < 2; i++) {
> > +        IDEState *s = &bus->ifs[i];
> > +
> > +        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..09b0404 100644
> > --- a/include/hw/ide/internal.h
> > +++ b/include/hw/ide/internal.h
> > @@ -607,8 +607,10 @@ 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(IDEBus *bus);
> >  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int
> iobase2);
> >  void ide_register_restart_cb(IDEBus *bus);
> > +void ide_unregister_restart_cb(IDEBus *bus);
> >
> >  void ide_exec_cmd(IDEBus *bus, uint32_t val);
> >
> >
>

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

[John Snow]

On 02/09/2017 08:22 PM, Li Qiang wrote:
> Hello John,
> 
> 2017-02-10 3:42 GMT+08:00 John Snow <jsnow@redhat.com
> <mailto:jsnow@redhat.com>>:
> 
> 
> 
>     On 02/09/2017 02:04 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 patch adds two cleanup function.
>     >
>     > Signed-off-by: Li Qiang <liqiang6-s@360.cn <mailto:liqiang6-s@360.cn>>
>     > ---
>     >  hw/ide/core.c             | 21 +++++++++++++++++++++
>     >  include/hw/ide/internal.h |  2 ++
>     >  2 files changed, 23 insertions(+)
>     >
>     > diff --git a/hw/ide/core.c b/hw/ide/core.c
>     > index 43709e5..8fe5896 100644
>     > --- a/hw/ide/core.c
>     > +++ b/hw/ide/core.c
>     > @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
>     >      }
>     >  }
>     >
>     > +void ide_unregister_restart_cb(IDEBus *bus)
>     > +{
>     > +    if (bus->dma->ops->restart_dma) {
>     > +        qemu_del_vm_change_state_handler(bus->vmstate);
>     > +    }
>     > +}
>     > +
> 
>     Doesn't this conflict with qdev.c's idebus_unrealize call? 
> 
> 
> As far as I can see, No conflict. Let's get a confirmation.
> 
> Hello Paolo,
> 
> Does this patch have any conflict/affect with the qdev?
> 
> Thanks.
> 

They're both deleting the same VMstate handler, so as far as I can see
this is a redundant call.

> 
> 
>     >  static IDEDMA ide_dma_nop = {
>     >      .ops = &ide_dma_nop_ops,
>     >      .aiocb = NULL,
>     > @@ -2603,6 +2610,20 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
>     >      bus->dma = &ide_dma_nop;
>     >  }
>     >
>     > +void ide_exit(IDEBus *bus)
>     > +{
>     > +    int i;
>     > +
>     > +    for (i = 0; i < 2; i++) {
>     > +        IDEState *s = &bus->ifs[i];
>     > +
>     > +        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..09b0404 100644
>     > --- a/include/hw/ide/internal.h
>     > +++ b/include/hw/ide/internal.h
>     > @@ -607,8 +607,10 @@ 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(IDEBus *bus);
>     >  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int
>     iobase2);
>     >  void ide_register_restart_cb(IDEBus *bus);
>     > +void ide_unregister_restart_cb(IDEBus *bus);
>     >
>     >  void ide_exec_cmd(IDEBus *bus, uint32_t val);
>     >
>     >
> 

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

[Li Qiang]

Hello,

2017-02-15 7:30 GMT+08:00 John Snow <jsnow@redhat.com>:

>
>
> On 02/09/2017 08:22 PM, Li Qiang wrote:
> > Hello John,
> >
> > 2017-02-10 3:42 GMT+08:00 John Snow <jsnow@redhat.com
> > <mailto:jsnow@redhat.com>>:
> >
> >
> >
> >     On 02/09/2017 02:04 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 patch adds two cleanup function.
> >     >
> >     > Signed-off-by: Li Qiang <liqiang6-s@360.cn <mailto:
> liqiang6-s@360.cn>>
> >     > ---
> >     >  hw/ide/core.c             | 21 +++++++++++++++++++++
> >     >  include/hw/ide/internal.h |  2 ++
> >     >  2 files changed, 23 insertions(+)
> >     >
> >     > diff --git a/hw/ide/core.c b/hw/ide/core.c
> >     > index 43709e5..8fe5896 100644
> >     > --- a/hw/ide/core.c
> >     > +++ b/hw/ide/core.c
> >     > @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
> >     >      }
> >     >  }
> >     >
> >     > +void ide_unregister_restart_cb(IDEBus *bus)
> >     > +{
> >     > +    if (bus->dma->ops->restart_dma) {
> >     > +        qemu_del_vm_change_state_handler(bus->vmstate);
> >     > +    }
> >     > +}
> >     > +
> >
> >     Doesn't this conflict with qdev.c's idebus_unrealize call?
> >
> >
> > As far as I can see, No conflict. Let's get a confirmation.
> >
> > Hello Paolo,
> >
> > Does this patch have any conflict/affect with the qdev?
> >
> > Thanks.
> >
>
> They're both deleting the same VMstate handler, so as far as I can see
> this is a redundant call.
>
>
IIUC, the idebus_unrealize in qdev.c is for qdev model.
But the added is for qom model.
For example, if you use 'device_add ahci,id=ahci'/'device_del ahci' in the
qmp.

The qemu will call 'pci_ich9_ahci_realize'/'pci_ich9_uninit'.





> >
> >
> >     >  static IDEDMA ide_dma_nop = {
> >     >      .ops = &ide_dma_nop_ops,
> >     >      .aiocb = NULL,
> >     > @@ -2603,6 +2610,20 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
> >     >      bus->dma = &ide_dma_nop;
> >     >  }
> >     >
> >     > +void ide_exit(IDEBus *bus)
> >     > +{
> >     > +    int i;
> >     > +
> >     > +    for (i = 0; i < 2; i++) {
> >     > +        IDEState *s = &bus->ifs[i];
> >     > +
> >     > +        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..09b0404 100644
> >     > --- a/include/hw/ide/internal.h
> >     > +++ b/include/hw/ide/internal.h
> >     > @@ -607,8 +607,10 @@ 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(IDEBus *bus);
> >     >  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int
> >     iobase2);
> >     >  void ide_register_restart_cb(IDEBus *bus);
> >     > +void ide_unregister_restart_cb(IDEBus *bus);
> >     >
> >     >  void ide_exec_cmd(IDEBus *bus, uint32_t val);
> >     >
> >     >
> >
>

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

[John Snow]

On 02/15/2017 04:26 AM, Li Qiang wrote:
> Hello,
> 
> 2017-02-15 7:30 GMT+08:00 John Snow <jsnow@redhat.com
> <mailto:jsnow@redhat.com>>:
> 
> 
> 
>     On 02/09/2017 08:22 PM, Li Qiang wrote:
>     > Hello John,
>     >
>     > 2017-02-10 3:42 GMT+08:00 John Snow <jsnow@redhat.com <mailto:jsnow@redhat.com>
>     > <mailto:jsnow@redhat.com <mailto:jsnow@redhat.com>>>:
>     >
>     >
>     >
>     >     On 02/09/2017 02:04 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 patch adds two cleanup function.
>     >     >
>     >     > Signed-off-by: Li Qiang <liqiang6-s@360.cn
>     <mailto:liqiang6-s@360.cn> <mailto:liqiang6-s@360.cn
>     <mailto:liqiang6-s@360.cn>>>
>     >     > ---
>     >     >  hw/ide/core.c             | 21 +++++++++++++++++++++
>     >     >  include/hw/ide/internal.h |  2 ++
>     >     >  2 files changed, 23 insertions(+)
>     >     >
>     >     > diff --git a/hw/ide/core.c b/hw/ide/core.c
>     >     > index 43709e5..8fe5896 100644
>     >     > --- a/hw/ide/core.c
>     >     > +++ b/hw/ide/core.c
>     >     > @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
>     >     >      }
>     >     >  }
>     >     >
>     >     > +void ide_unregister_restart_cb(IDEBus *bus)
>     >     > +{
>     >     > +    if (bus->dma->ops->restart_dma) {
>     >     > +        qemu_del_vm_change_state_handler(bus->vmstate);
>     >     > +    }
>     >     > +}
>     >     > +
>     >
>     >     Doesn't this conflict with qdev.c's idebus_unrealize call?
>     >
>     >
>     > As far as I can see, No conflict. Let's get a confirmation.
>     >
>     > Hello Paolo,
>     >
>     > Does this patch have any conflict/affect with the qdev?
>     >
>     > Thanks.
>     >
> 
>     They're both deleting the same VMstate handler, so as far as I can see
>     this is a redundant call.
> 
> 
> IIUC, the idebus_unrealize in qdev.c is for qdev model.
> But the added is for qom model.
> For example, if you use 'device_add ahci,id=ahci'/'device_del ahci' in
> the qmp.
> 
> The qemu will call 'pci_ich9_ahci_realize'/'pci_ich9_uninit'.
> 
> 
>  

I'm sorry, I still don't understand. Do you have some reproducer or case
where I can verify that this leaks?

It doesn't look as if you can hot-add or hot-remove an AHCI device right
now anyway, have you tested this?

Further, if the two calls AREN'T in conflict, I'd rather find some
cleanup mechanism that handles all the unrealize/uninit cases together
instead of having separate cleanup pathways.

--js

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

[John Snow]

On 02/09/2017 02:04 AM, Li Qiang wrote:
> This can avoid memory leak when hotunplug the ahci device.
> 
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
>  hw/ide/ahci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 3c19bda..56f68a8 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1485,6 +1485,14 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
>  
>  void ahci_uninit(AHCIState *s)
>  {
> +    int i;
> +
> +    for (i = 0; i < s->ports; i++) {
> +        AHCIDevice *ad = &s->dev[i];
> +
> +        ide_unregister_restart_cb(&ad->port);
> +        ide_exit(&ad->port);
> +    }
>      g_free(s->dev);
>  }
>  
> 

I couldn't actually prove to myself that ahci_uninit runs on an unplug
request. Did you observe improvements to the memory profile after
introducing this patch?

I don't think anyone has taken the care to plumb AHCI to behave well
when hotplugged.

--js

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

[John Snow]

On 02/28/2017 07:35 PM, John Snow wrote:
> 
> 
> On 02/09/2017 02:04 AM, Li Qiang wrote:
>> This can avoid memory leak when hotunplug the ahci device.
>>
>> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
>> ---
>>  hw/ide/ahci.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 3c19bda..56f68a8 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -1485,6 +1485,14 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
>>  
>>  void ahci_uninit(AHCIState *s)
>>  {
>> +    int i;
>> +
>> +    for (i = 0; i < s->ports; i++) {
>> +        AHCIDevice *ad = &s->dev[i];
>> +
>> +        ide_unregister_restart_cb(&ad->port);
>> +        ide_exit(&ad->port);
>> +    }
>>      g_free(s->dev);
>>  }
>>  
>>
> 
> I couldn't actually prove to myself that ahci_uninit runs on an unplug
> request. Did you observe improvements to the memory profile after
> introducing this patch?
> 
> I don't think anyone has taken the care to plumb AHCI to behave well
> when hotplugged.
> 

Ah, my mistake actually! It's just that it requires guest cooperation.
It works perfectly well as long as Linux is ready to cooperate with the
request.

--js

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

[John Snow]

On 02/09/2017 02:04 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 patch adds two cleanup function.
> 

So, the peculiarities of the current arrangement of QDEV realization and
unrealization is a bit of a mystery to me, so I'm hoping my suggestions
here make sense.

> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
>  hw/ide/core.c             | 21 +++++++++++++++++++++
>  include/hw/ide/internal.h |  2 ++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 43709e5..8fe5896 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
>      }
>  }
>  
> +void ide_unregister_restart_cb(IDEBus *bus)
> +{
> +    if (bus->dma->ops->restart_dma) {
> +        qemu_del_vm_change_state_handler(bus->vmstate);
> +    }
> +}
> +

This works perfectly well, though I think the function is named
incorrectly -- this should be an AHCI function, as it is AHCI's job to
unregister the IDEBus it created (not IDE's -- the bus belongs to the
HBA, not the IDE device.)

However, we DO have the IDEBus unrealize code in qdev.c that should be
handling this for us. Can we rename this function and just have it set
the "realized" property of the IDEBus to false to handle this cleanup
for us?

I'm not well versed in qdev code management, but it definitely feels
wrong to have the cleanup in two places.

>  static IDEDMA ide_dma_nop = {
>      .ops = &ide_dma_nop_ops,
>      .aiocb = NULL,
> @@ -2603,6 +2610,20 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
>      bus->dma = &ide_dma_nop;
>  }
>  
> +void ide_exit(IDEBus *bus)
> +{
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        IDEState *s = &bus->ifs[i];
> +
> +        timer_del(s->sector_write_timer);
> +        timer_free(s->sector_write_timer);
> +        qemu_vfree(s->smart_selftest_data);
> +        qemu_vfree(s->io_buffer);

I would prefer a function that cleans up a single IDE device, and the
caller (which has knowledge of the HBA and the buses it owns) loops as
appropriate. (In this case, ahci_uninit or ahci_unrealize or whichever.)

It's correct otherwise, though.

> +    }
> +}
> +
>  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..09b0404 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -607,8 +607,10 @@ 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(IDEBus *bus);
>  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>  void ide_register_restart_cb(IDEBus *bus);
> +void ide_unregister_restart_cb(IDEBus *bus);
>  
>  void ide_exec_cmd(IDEBus *bus, uint32_t val);
>  
> 

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

[Li Qiang]

Hello John, Paolo,

2017-03-01 8:47 GMT+08:00 John Snow <jsnow@redhat.com>:

>
>
> On 02/09/2017 02:04 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 patch adds two cleanup function.
> >
>
> So, the peculiarities of the current arrangement of QDEV realization and
> unrealization is a bit of a mystery to me, so I'm hoping my suggestions
> here make sense.
>
> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > ---
> >  hw/ide/core.c             | 21 +++++++++++++++++++++
> >  include/hw/ide/internal.h |  2 ++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 43709e5..8fe5896 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
> >      }
> >  }
> >
> > +void ide_unregister_restart_cb(IDEBus *bus)
> > +{
> > +    if (bus->dma->ops->restart_dma) {
> > +        qemu_del_vm_change_state_handler(bus->vmstate);
> > +    }
> > +}
> > +
>
> This works perfectly well, though I think the function is named
> incorrectly -- this should be an AHCI function, as it is AHCI's job to
> unregister the IDEBus it created (not IDE's -- the bus belongs to the
> HBA, not the IDE device.)
>
> However, we DO have the IDEBus unrealize code in qdev.c that should be
> handling this for us. Can we rename this function and just have it set
> the "realized" property of the IDEBus to false to handle this cleanup
> for us?
>
> I'm not well versed in qdev code management, but it definitely feels
> wrong to have the cleanup in two places.
>

 Agree, but I'm not familiar with qdev too, that's why we need Paolo's help.

@Paolo,
Could you please have a look at this issue?


> >  static IDEDMA ide_dma_nop = {
> >      .ops = &ide_dma_nop_ops,
> >      .aiocb = NULL,
> > @@ -2603,6 +2610,20 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
> >      bus->dma = &ide_dma_nop;
> >  }
> >
> > +void ide_exit(IDEBus *bus)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < 2; i++) {
> > +        IDEState *s = &bus->ifs[i];
> > +
> > +        timer_del(s->sector_write_timer);
> > +        timer_free(s->sector_write_timer);
> > +        qemu_vfree(s->smart_selftest_data);
> > +        qemu_vfree(s->io_buffer);
>
> I would prefer a function that cleans up a single IDE device, and the
> caller (which has knowledge of the HBA and the buses it owns) loops as
> appropriate. (In this case, ahci_uninit or ahci_unrealize or whichever.)
>
> It's correct otherwise, though.
>
> > +    }
> > +}
> > +
> >  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..09b0404 100644
> > --- a/include/hw/ide/internal.h
> > +++ b/include/hw/ide/internal.h
> > @@ -607,8 +607,10 @@ 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(IDEBus *bus);
> >  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int
> iobase2);
> >  void ide_register_restart_cb(IDEBus *bus);
> > +void ide_unregister_restart_cb(IDEBus *bus);
> >
> >  void ide_exec_cmd(IDEBus *bus, uint32_t val);
> >
> >
>

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

[John Snow]

On 02/28/2017 08:03 PM, Li Qiang wrote:
> Hello John, Paolo,
> 
> 2017-03-01 8:47 GMT+08:00 John Snow <jsnow@redhat.com
> <mailto:jsnow@redhat.com>>:
> 
> 
> 
>     On 02/09/2017 02:04 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 patch adds two cleanup function.
>     >
> 
>     So, the peculiarities of the current arrangement of QDEV realization and
>     unrealization is a bit of a mystery to me, so I'm hoping my suggestions
>     here make sense.
> 
>     > Signed-off-by: Li Qiang <liqiang6-s@360.cn <mailto:liqiang6-s@360.cn>>
>     > ---
>     >  hw/ide/core.c             | 21 +++++++++++++++++++++
>     >  include/hw/ide/internal.h |  2 ++
>     >  2 files changed, 23 insertions(+)
>     >
>     > diff --git a/hw/ide/core.c b/hw/ide/core.c
>     > index 43709e5..8fe5896 100644
>     > --- a/hw/ide/core.c
>     > +++ b/hw/ide/core.c
>     > @@ -2586,6 +2586,13 @@ void ide_register_restart_cb(IDEBus *bus)
>     >      }
>     >  }
>     >
>     > +void ide_unregister_restart_cb(IDEBus *bus)
>     > +{
>     > +    if (bus->dma->ops->restart_dma) {
>     > +        qemu_del_vm_change_state_handler(bus->vmstate);
>     > +    }
>     > +}
>     > +
> 
>     This works perfectly well, though I think the function is named
>     incorrectly -- this should be an AHCI function, as it is AHCI's job to
>     unregister the IDEBus it created (not IDE's -- the bus belongs to the
>     HBA, not the IDE device.)
> 
>     However, we DO have the IDEBus unrealize code in qdev.c that should be
>     handling this for us. Can we rename this function and just have it set
>     the "realized" property of the IDEBus to false to handle this cleanup
>     for us?
> 
>     I'm not well versed in qdev code management, but it definitely feels
>     wrong to have the cleanup in two places.
> 
> 
>  Agree, but I'm not familiar with qdev too, that's why we need Paolo's help.
> 
> @Paolo,
> Could you please have a look at this issue?
> 
> 

OK, so here's what it looks like to me from here:

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, AFAIK.

Perhaps what we want is this, which does actually do the proper bus
teardown:

-static void idebus_unrealize(DeviceState *qdev, Error **errp);
+static void idebus_unrealize(BusState *bus, Error **errp);

 static Property ide_props[] = {
     DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
@@ -43,14 +43,17 @@ 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);
-
-    if (bus->vmstate) {
-        qemu_del_vm_change_state_handler(bus->vmstate);
+    IDEBus *ibus = IDE_BUS(bus);
+    if (ibus->vmstate) {
+        qemu_del_vm_change_state_handler(ibus->vmstate);
     }
 }
@@ -365,7 +369,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;
 }



This way the bus teardown calls the existing handler. If you integrate
this into your patches and recreate ide_exit as a per-device teardown
function, I'll merge this.

Thanks for your patience and the bug report,
--John

>     >  static IDEDMA ide_dma_nop = {
>     >      .ops = &ide_dma_nop_ops,
>     >      .aiocb = NULL,
>     > @@ -2603,6 +2610,20 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
>     >      bus->dma = &ide_dma_nop;
>     >  }
>     >
>     > +void ide_exit(IDEBus *bus)
>     > +{
>     > +    int i;
>     > +
>     > +    for (i = 0; i < 2; i++) {
>     > +        IDEState *s = &bus->ifs[i];
>     > +
>     > +        timer_del(s->sector_write_timer);
>     > +        timer_free(s->sector_write_timer);
>     > +        qemu_vfree(s->smart_selftest_data);
>     > +        qemu_vfree(s->io_buffer);
> 
>     I would prefer a function that cleans up a single IDE device, and the
>     caller (which has knowledge of the HBA and the buses it owns) loops as
>     appropriate. (In this case, ahci_uninit or ahci_unrealize or whichever.)
> 
>     It's correct otherwise, though.
> 
>     > +    }
>     > +}
>     > +
>     >  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..09b0404 100644
>     > --- a/include/hw/ide/internal.h
>     > +++ b/include/hw/ide/internal.h
>     > @@ -607,8 +607,10 @@ 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(IDEBus *bus);
>     >  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int
>     iobase2);
>     >  void ide_register_restart_cb(IDEBus *bus);
>     > +void ide_unregister_restart_cb(IDEBus *bus);
>     >
>     >  void ide_exec_cmd(IDEBus *bus, uint32_t val);
>     >
>     >
> 
> 

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

[Paolo Bonzini]

On 01/03/2017 21:20, John Snow 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, AFAIK.

I agree.  Thanks for looking at it!

Paolo