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 <address@hidden>
> ---
>  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);
>  
>