From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50801) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YOTaK-0000g9-9r for qemu-devel@nongnu.org; Thu, 19 Feb 2015 11:04:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YOTaF-0004eK-3E for qemu-devel@nongnu.org; Thu, 19 Feb 2015 11:04:16 -0500 Received: from mail-wi0-x22b.google.com ([2a00:1450:400c:c05::22b]:44942) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YOTaE-0004e2-P6 for qemu-devel@nongnu.org; Thu, 19 Feb 2015 11:04:11 -0500 Received: by mail-wi0-f171.google.com with SMTP id hi2so49081441wib.4 for ; Thu, 19 Feb 2015 08:04:09 -0800 (PST) Sender: Paolo Bonzini Message-ID: <54E60972.8000200@redhat.com> Date: Thu, 19 Feb 2015 17:04:02 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <87r3tmi3ig.fsf@blackfin.pond.sub.org> <54E5D6A4.1050307@suse.de> <87mw4agifx.fsf@blackfin.pond.sub.org> In-Reply-To: <87mw4agifx.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] bus_unparent() can go into infinite loop List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Bandan Das , Peter Maydell , Alexander Graf , qemu-devel@nongnu.org, "Michael S. Tsirkin" On 19/02/2015 14:05, Markus Armbruster wrote: > Andreas Färber writes: > >> Am 19.02.2015 um 11:45 schrieb Markus Armbruster: >>> Reproducer (don't ask): >>> >>> $ qemu-system-arm -M virt -S -display none -drive >>> if=scsi,id=foo,bus=1,file=tmp.qcow2 -device nec-usb-xhci -device >>> usb-storage,drive=foo -device virtio-scsi-pci >>> qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: >>> Property 'scsi-disk.drive' can't take value 'foo', it's in use >>> qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: >>> Setting drive property failed >>> qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed >>> >>> Nevermind the silly error reporting, I got patches to clean that up. >> >> I'm actually more confused about the use of PCI devices with the virt >> machine. Does that now feature Alex' PCI controller by default? >> Otherwise there would be no bus for those devices. > > "info qtree" shows a PCIE bus: > > dev: gpex-pcihost, id "" > gpio-out "sysbus-irq" 4 > mmio ffffffffffffffff/0000000010000000 > mmio ffffffffffffffff/ffffffffffffffff > mmio 000000003eff0000/0000000000010000 > bus: pcie.0 > type PCIE > dev: gpex-root, id "" > addr = 00.0 > romfile = "" > rombar = 1 (0x1) > multifunction = false > command_serr_enable = true > class Host bridge, addr 00:00.0, pci id 1b36:0008 (sub 1af4:1100) > >> Is this on master or on top of your PCI realize changes or anything? > > Unadulterated master (commit cd2d554). > >>> Stuck in bus_unparent()'s loop: >>> >>> while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) { >>> DeviceState *dev = kid->child; >>> object_unparent(OBJECT(dev)); >>> } >> >> Logically speaking, unparenting on QOM level has nothing to with the bus >> children list. >> The parent may well be /machine/{unassigned,peripheral{,-anon}} >> container objects rather than some BusState object, the latter usually >> has link<> properties only for its qdev-level "children". >> >> However I vaguely recall that we shoehorned the unparenting callback to >> invoke unrealizing the device. What might happen here is that after >> realizing the device failed, realized == false; realized = false in the >> unparenting path becomes a no-op then. I.e., the realize error handling >> may need to be reviewed to not just return but to undo any changes such >> as attaching to some bus (or MemoryRegion, VMState, etc.). > > (gdb) p *dev > $2 = {parent_obj = {class = 0x555556282140, free = 0x7ffff64dcf70 , > properties = {tqh_first = 0x55555669d860, tqh_last = 0x5555566a1d90}, > ref = 1, parent = 0x0}, id = 0x0, realized = false, > pending_deleted_event = false, opts = 0x0, hotplugged = 0, > parent_bus = 0x555556450060, gpios = {lh_first = 0x0}, child_bus = { > lh_first = 0x0}, num_child_bus = 0, instance_id_alias = -1, > alias_required_for_version = 0} > > This is right before object_unparent(). The bug is that the device is not given a parent at all by scsi_bus_legacy_add_drive, unlike for example qdev_device_add. There is a "safety" net that adds the drive to the QOM tree in qdev_init, but it isn't enough if you abort creation of the device before qdev_init. This applies to qdev_create and its callers isa_create, usb_create, and also to qdev_try_create/isa_try_create. I tried "git grep -lw FUNC_NAME | xargs grep object_unparent" and there should be no other problematic case than scsi_bus_legacy_add_drive. Patch will follow. Paolo