Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Kevin Wolf <kwolf@redhat.com>]

Am 27.06.2011 17:34, schrieb Stefano Stabellini:
> On Mon, 27 Jun 2011, Kevin Wolf wrote:
>> hw/ide/pci.h is just as internal as internal.h is. And even if you
>> managed to access the same things without any IDE header file, I still
>> think it's not the right level of abstraction because it relies on the
>> implementation details of IDE.
>>
>> Just this line: pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; Does this
>> really look right to you to do anywhere outside IDE?
>>
>> I'm basically looking for the same as Michael who wanted to have network
>> unplug handled through qdev, just that the IDE code doesn't support
>> unplug yet.
> 
> I understand.
> 
> I created pci_piix3_xen_ide_init and moved the unplug code to
> hw/ide/piix.c so that we don't have any internal knowledge of IDE code
> in xen_platform.c any more, see below.
> 
> One remaining problem is that the generic pci unplug function is not
> what I need in this case so I have to setup my own; but I hope that in
> general the changes are going in the right direction.

Looks much better to me now. :-)

Just a few comments inline:

> diff --git a/hw/ide.h b/hw/ide.h
> index 34d9394..a490cbb 100644
> --- a/hw/ide.h
> +++ b/hw/ide.h
> @@ -13,6 +13,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq,
>  /* ide-pci.c */
>  void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>                           int secondary_ide_enabled);
> +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index c349644..8ae9ff0 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -167,6 +167,41 @@ static int pci_piix4_ide_initfn(PCIDevice *dev)
>      return pci_piix_ide_initfn(d);
>  }
>  
> +static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> +{
> +    PCIDevice *pci_dev;
> +    PCIIDEState *pci_ide;
> +    DriveInfo *di;
> +    int i = 0;
> +
> +    pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
> +    pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev);
> +
> +    for (; i < 3; i++) {
> +        di = drive_get_by_index(IF_IDE, i); 
> +        if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) {
> +            DeviceState *ds = bdrv_get_attached(di->bdrv);
> +            if (ds)
> +                bdrv_detach(di->bdrv, ds);

Missing braces

> +            bdrv_close(di->bdrv);
> +            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> +            drive_put_ref(di);
> +        }
> +    }
> +    qdev_reset_all(&(pci_ide->dev.qdev));
> +    return 0;
> +}
> +
> +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +{
> +    PCIDevice *dev;
> +
> +    dev = pci_create_simple(bus, devfn, "piix3-ide");
> +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;

Doesn't this modify the piix3-ide definition? Shouldn't matter in
practice because you can't have additional IDE controllers anyway, but
maybe worth a comment stating this.

The other, probably cleaner way of doing it would be adding another
PCIDeviceInfo for xen-ide.

Kevin