On 19/02/2015 14:05, Markus Armbruster wrote:
> Andreas Färber <afaer...@suse.de> 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 <g_free>,
> 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
