Re: [Qemu-devel] bus_unparent() can go into infinite loop

[Paolo Bonzini]

On 19/02/2015 14:05, Markus Armbruster wrote:
> Andreas Färber <address@hidden> 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