Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Peter Maydell]

On 9 July 2018 at 23:03, Thomas Huth <address@hidden> wrote:
> On 09.07.2018 23:42, Peter Maydell wrote:
>> On 9 July 2018 at 22:03, Thomas Huth <address@hidden> wrote:
>>> When trying to "device_add bcm2837" on a machine that is not suitable for
>>> this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":
>>>
>>> echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
>>>  "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " \
>>>  "'arguments': {'command-line': 'info qtree'}}" | \
>>>  aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp 
>>> stdio
>>>
>>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
>>>  "package": "build-all"}, "capabilities": []}}
>>> {"return": {}}
>>> {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
>>>  hotplugged on this machine"}}
>>> Segmentation fault (core dumped)
>>>
>>> The problem is that qdev_set_parent_bus() from instance_init adds a link
>>> to the child devices which is not valid anymore after the device init
>>> failed. Thus the qdev_set_parent_bus() must rather be done in the realize
>>> function instead.
>>
>> Yuck. The real problem here is that we're still requiring the
>> code that creates these QOM devices to manually set the parent
>> in the first place. It's not surprising that we don't get it right
>> (either parenting in the wrong place or not at all). I'd much
>> rather see us fix that properly than keep papering over places
>> where we get it wrong.
>
> Sorry, I'm still not an expert in all this QOM stuff yet ... so what do
> you exactly recommend to do instead?

I'm not clear either, but I don't think that what we're
currently doing can be right.

thanks
-- PMM