From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60593) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QYa8m-00058Z-I0 for qemu-devel@nongnu.org; Mon, 20 Jun 2011 04:47:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QYa8k-0002hh-Je for qemu-devel@nongnu.org; Mon, 20 Jun 2011 04:47:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26927) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QYa8k-0002hW-27 for qemu-devel@nongnu.org; Mon, 20 Jun 2011 04:47:26 -0400 Date: Mon, 20 Jun 2011 11:47:33 +0300 From: "Michael S. Tsirkin" Message-ID: <20110620084733.GA22456@redhat.com> References: <1308240319-13949-3-git-send-email-stefano.stabellini@eu.citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1308240319-13949-3-git-send-email-stefano.stabellini@eu.citrix.com> Subject: Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: stefano.stabellini@eu.citrix.com Cc: Anthony.Perard@citrix.com, xen-devel@lists.xensource.com, qemu-devel@nongnu.org, agraf@suse.de On Thu, Jun 16, 2011 at 05:05:19PM +0100, stefano.stabellini@eu.citrix.com wrote: > From: Stefano Stabellini > > 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 > --- > 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 > > @@ -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 >