From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47872) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDVvP-0007xE-Rr for qemu-devel@nongnu.org; Wed, 12 Feb 2014 04:16:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WDVvK-0006Fq-T3 for qemu-devel@nongnu.org; Wed, 12 Feb 2014 04:16:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40809) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDVvK-0006Fg-LC for qemu-devel@nongnu.org; Wed, 12 Feb 2014 04:16:06 -0500 From: Markus Armbruster References: <1391084198-21021-1-git-send-email-armbru@redhat.com> <1391084198-21021-5-git-send-email-armbru@redhat.com> <20140203145406.GN3643@dhcp-200-207.str.redhat.com> <878utp8xh1.fsf@blackfin.pond.sub.org> <87wqh8wwpj.fsf@blackfin.pond.sub.org> <8738jpaf0b.fsf@blackfin.pond.sub.org> Date: Wed, 12 Feb 2014 10:16:00 +0100 In-Reply-To: <8738jpaf0b.fsf@blackfin.pond.sub.org> (Markus Armbruster's message of "Tue, 11 Feb 2014 15:41:08 +0100") Message-ID: <878utgpu7j.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Kevin Wolf , afaerber@suse.de, qemu-devel@nongnu.org, stefanha@redhat.com, agraf@suse.de Markus Armbruster writes: > Stefano Stabellini writes: > >> On Thu, 6 Feb 2014, Markus Armbruster wrote: >>> Stefano Stabellini writes: >>> >>> > On Wed, 5 Feb 2014, Markus Armbruster wrote: >>> >> [Note cc: Stefano] >>> >> >>> >> Kevin Wolf writes: >>> >> >>> >> > Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben: >>> >> >> It's a copy of dev->conf.bs. The copy was needed for non-qdevified >>> >> >> controllers, which lacked dev. >>> >> >> >>> >> >> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back >>> >> >> to that in the next few commits. >>> >> >> >>> >> >> Signed-off-by: Markus Armbruster >>> >> > >>> >> > So this pci_piix3_xen_ide_unplug() is what happens here: >>> >> > >>> >> >> --- a/hw/ide/piix.c >>> >> >> +++ b/hw/ide/piix.c >>> >> >> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev) >>> >> >> >>> >> >> static int pci_piix3_xen_ide_unplug(DeviceState *dev) >>> >> >> { >>> >> >> - PCIIDEState *pci_ide; >>> >> >> DriveInfo *di; >>> >> >> int i = 0; >>> >> >> >>> >> >> - pci_ide = PCI_IDE(dev); >>> >> >> - >>> >> >> for (; i < 3; i++) { >>> >> >> di = drive_get_by_index(IF_IDE, i); >>> >> >> if (di != NULL && !di->media_cd) { >>> >> >> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev) >>> >> >> bdrv_detach_dev(di->bdrv, ds); >>> >> >> } >>> >> >> bdrv_close(di->bdrv); >>> >> >> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; >>> >> >> drive_put_ref(di); >>> >> >> } >>> >> >> } >>> >> > >>> >> > Probably I'm just missing the obvious, but it seems to me that the >>> >> > copy was cleared here, while the original was left around. This was >>> >> > no problem because the original was unused anyway after device >>> >> > initialisation. >>> >> > >>> >> > Now that the copy doesn't exist any more, we can't clear it, obviously, >>> >> > but why don't we have to clear the original? Won't we still run the >>> >> > "device is attached" code branches even though the device is really >>> >> > unplugged? >>> >> >>> >> It's been a while since I wrote this. Almost 14 months, in fact. >>> >> >>> >> No other IDE controller implements DeviceClass method unplug(). I can't >>> >> remember why the normal code to detach the backend (release_drive()) >>> >> doesn't do here. Stefano, can you help? >>> > >>> > Too long to be able to remember the exact details :-/ >>> > However if you point me to a branch I can give it a try and tell you if >>> > the unplug still works as it used to. >>> >>> Series trivially rebased to current master available at >>> git://repo.or.cz/qemu/armbru.git branch kill-non-qdev-ide >> >> Unfortunately it doesn't work: I can see both sda and xvda being present >> after the system has booted. > > Thank you for testing. I'll try to clear the originals like Kevin > suggested. Hopefully, that'll pass your test. Stefano, I updated the branch, please retest it.