From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50379) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbDyW-0001kO-Ml for qemu-devel@nongnu.org; Mon, 27 Jun 2011 11:43:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QbDyU-0000jz-Cv for qemu-devel@nongnu.org; Mon, 27 Jun 2011 11:43:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25564) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbDyT-0000jo-Qq for qemu-devel@nongnu.org; Mon, 27 Jun 2011 11:43:46 -0400 Message-ID: <4E08A5D3.8040400@redhat.com> Date: Mon, 27 Jun 2011 17:46:27 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1308240319-13949-3-git-send-email-stefano.stabellini@eu.citrix.com> <7424DA76-FD02-479F-B72A-D0B6EBBDE7DF@suse.de> <4DFF0C0A.8040505@redhat.com> <4E083EB9.4030806@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 Cc: "xen-devel@lists.xensource.com Devel" , "Michael S. Tsirkin" , Markus Armbruster , Alexander Graf , "qemu-devel@nongnu.org Developers" , Anthony Perard 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