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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 16, 2011 at 05:05:19PM +0100, stefano.stabellini@eu.citrix.com wrote:
> 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/xen_platform.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 62 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> index b167eee..9f8c843 100644
> --- a/hw/xen_platform.c
> +++ b/hw/xen_platform.c
> @@ -34,6 +34,9 @@
>  #include "xen_backend.h"
>  #include "rwhandler.h"
>  #include "trace.h"
> +#include "hw/ide/internal.h"

I'm not an expert here but it looks like
you should put some code in hw/ide/xen.c
and export an API from there rather
than calling ide_bus_reset and tweaking
PCIIDEState directly.

> +#include "hw/ide/pci.h"
> +#include "hw/pci_ids.h"
>  
>  #include <xenguest.h>
>  
> @@ -76,6 +79,54 @@ static void log_writeb(PCIXenPlatformState *s, char val)
>  }
>  
>  /* Xen Platform, Fixed IOPort */
> +#define UNPLUG_ALL_IDE_DISKS 1
> +#define UNPLUG_ALL_NICS 2
> +#define UNPLUG_AUX_IDE_DISKS 4
> +
> +static int unplug_param;
> +
> +static void unplug_nic(PCIBus *b, PCIDevice *d)
> +{
> +    if (d->config[0xa] == 0 && d->config[0xb] == 2) {

Please use registers from pci_regs.h and pci_ids.h

> +        pci_unplug_device(&(d->qdev));

Can't you use qdev_unplug?
That does other useful checks and updates system state.
Also, are there non hotpluggable devices?
If not you can assert on qdev_unplug failure.

> +    }
> +}
> +
> +static void pci_unplug_nics(PCIBus *bus)
> +{
> +    pci_for_each_device(bus, 0, unplug_nic);
> +}
> +
> +static void unplug_disks(PCIBus *b, PCIDevice *d)
> +{
> +    if (d->config[0xa] == 1 && d->config[0xb] == 1) {

Same comment about hardcoded constants.

> +        PCIIDEState *pci_ide = DO_UPCAST(PCIIDEState, dev, d);
> +        DriveInfo *di;
> +        int i = 0;
> +
> +        if (unplug_param & UNPLUG_AUX_IDE_DISKS)
> +            i++;
> +
> +        for (; i < 3; i++) {
> +            di = drive_get_by_index(IF_IDE, i); 
> +            if (di != NULL && di->bdrv != NULL && di->bdrv->type != BDRV_TYPE_CDROM) {

line too long

> +                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;
> +                drive_put_ref(di);
> +            }
> +        }
> +        ide_bus_reset(&pci_ide->bus[0]);
> +        ide_bus_reset(&pci_ide->bus[1]);
> +    }
> +}
> +
> +static void pci_unplug_disks(PCIBus *bus)
> +{
> +    pci_for_each_device(bus, 0, unplug_disks);
> +}
>  
>  static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>  {
> @@ -83,10 +134,20 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
>  
>      switch (addr - XEN_PLATFORM_IOPORT) {
>      case 0:
> -        /* TODO: */
> +        unplug_param = val;
>          /* Unplug devices.  Value is a bitmask of which devices to
>             unplug, with bit 0 the IDE devices, bit 1 the network
>             devices, and bit 2 the non-primary-master IDE devices. */
> +        if (val & UNPLUG_ALL_IDE_DISKS || val & UNPLUG_AUX_IDE_DISKS) {
> +            DPRINTF("unplug disks\n");
> +            qemu_aio_flush();
> +            bdrv_flush_all();
> +            pci_unplug_disks(s->pci_dev.bus);
> +        }
> +        if (val & UNPLUG_ALL_NICS) {
> +            DPRINTF("unplug nics\n");
> +            pci_unplug_nics(s->pci_dev.bus);
> +        }
>          break;
>      case 2:
>          switch (val) {
> -- 
> 1.7.2.3
>