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

[Thomas Huth <thuth@redhat.com>]

On 10.07.2018 08:50, Peter Maydell wrote:
> On 9 July 2018 at 23:03, Thomas Huth <thuth@redhat.com> wrote:
>> On 09.07.2018 23:42, Peter Maydell wrote:
>>> On 9 July 2018 at 22:03, Thomas Huth <thuth@redhat.com> 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.

Hm, ok, so how to continue here now? Shall we at least mark the
bcm2836/7 devices with user_creatable=false, so that users can not crash
their QEMU so easily with device_add? The problem with introspection via
device-list-properties would still continue to exist, but I think that's
less likely used in practice... otherwise we could still move the
qdev_set_parent_bus() calls to the realize() function instead, and just
add a big fat FIXME comment in front of the code block, so that we
remember to clean that up one day...

 Thomas