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

[Kevin Wolf <kwolf@redhat.com>]

Am 28.06.2011 13:29, schrieb stefano.stabellini@eu.citrix.com:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> The unplug protocol is necessary to support PV drivers in the guest: the
> drivers expect to be able to "unplug" emulated disks and nics before
> initializing the Xen PV interfaces.
> It is responsibility of the guest to make sure that the unplug is done
> before the emulated devices or the PV interface start to be used.
> 
> We use pci_for_each_device to walk the PCI bus, identify the devices and
> disks that we want to disable and dynamically unplug them.
> 
> 
> Changes in v2:
> 
> - use PCI_CLASS constants;
> 
> - replace pci_unplug_device with qdev_unplug;
> 
> - do not import hw/ide/internal.h in xen_platform.c;
> 
> 
> Changes in v3:
> 
> - introduce piix3-ide-xen, that support hot-unplug;
> 
> - move the unplug code to hw/ide/piix.c;
> 
> - just call qdev_unplug from xen_platform.c to unplug the IDE disks;
> 
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/ide.h          |    1 +
>  hw/ide/piix.c     |   41 +++++++++++++++++++++++++++++++++++++++++
>  hw/pc_piix.c      |    6 +++++-
>  hw/xen_platform.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 89 insertions(+), 2 deletions(-)
> 
> 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..50ab7c5 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -167,6 +167,42 @@ 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);
> +            }
> +            bdrv_close(di->bdrv);
> +            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;

Have you tested if this is enough if the guest tries to continue using
the device? I don't know of any case where it's not sufficient, just
trying to make sure that it's really true in practice.

> +            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-xen");
> +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;

Can't this be moved into the PCIDeviceInfo now that we have a separate
one for Xen?

Kevin