From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60406 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PsA7P-0008RM-Dp for qemu-devel@nongnu.org; Wed, 23 Feb 2011 03:30:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PsA7K-0006zn-LG for qemu-devel@nongnu.org; Wed, 23 Feb 2011 03:30:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37833) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PsA7K-0006zO-AQ for qemu-devel@nongnu.org; Wed, 23 Feb 2011 03:30:38 -0500 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH] `qdev_free` when unplug a pci device References: <1298396180-23734-1-git-send-email-wdauchy@gmail.com> <20110223025001.GC19727@valinux.co.jp> Date: Wed, 23 Feb 2011 09:30:23 +0100 In-Reply-To: <20110223025001.GC19727@valinux.co.jp> (Isaku Yamahata's message of "Wed, 23 Feb 2011 11:50:01 +0900") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: Gerd Hoffmann , William Dauchy , qemu-devel@nongnu.org Isaku Yamahata writes: > On Tue, Feb 22, 2011 at 06:36:20PM +0100, William Dauchy wrote: >> `qdev_free` when unplug a pci device >> It resolves a bug when detaching/attaching a network device >> >> # virsh detach-interface dom0 network --mac 52:54:00:f6:84:ba >> Interface detached successfully >> >> # virsh attach-interface dom0 network default --mac 52:54:00:f6:84:ba >> error: Failed to attach interface >> error: internal error unable to execute QEMU command 'device_add': Duplicate ID 'net0' for device >> >> A detached pci device was not freed of the list `qemu_device_opts` >> --- >> hw/pci.c | 5 ++++- >> 1 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/hw/pci.c b/hw/pci.c >> index 8b76cea..1e9a8f0 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -1671,13 +1671,16 @@ static int pci_unplug_device(DeviceState *qdev) >> { >> PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev); >> PCIDeviceInfo *info = container_of(qdev->info, PCIDeviceInfo, qdev); >> + int unplug; >> >> if (info->no_hotplug) { >> qerror_report(QERR_DEVICE_NO_HOTPLUG, info->qdev.name); >> return -1; >> } >> - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, >> + unplug = dev->bus->hotplug(dev->bus->hotplug_qdev, dev, >> PCI_HOTPLUG_DISABLED); >> + qdev_free(qdev); > > Good catch. > qdev_free() should be called only when unplug had succeeded. > Although piix4 unplug always successes, pcie unplug may fail. > >> + return unplug; >> } >> >> void pci_qdev_register(PCIDeviceInfo *info) I don't think this patch is correct. Let me explain. Device hot unplug is *not* guaranteed to succeed. For some buses, such as USB, it always succeeds immediately, i.e. when the device_del monitor command finishes, the device is gone. Live is good. But for PCI, device_del merely initiates the ACPI unplug rain dance. It doesn't wait for the dance to complete. Why? The dance can take an unpredictable amount of time, including forever. Problem: Subsequent device_add can fail if it reuses the qdev ID or PCI slot, and the unplug has not yet completed (race condition), or it failed. Yes, Virginia, PCI hotplug *can* fail. When unplug succeeds, the qdev is automatically destroyed. pciej_write() does that for PIIX4. Looks like pcie_cap_slot_event() does it for PCIE. Your patch adds a *second* qdev_free(). No good.