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

[Thomas Huth]

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?

> Also, "device_add bcm2837" is just nonsensical: there's no
> way to create an SoC device like this with device_add.
> We should not allow it (for this and all the other
> devices that device_add will never work with)...

I'm fine if we additionally add a user_creatable = false here, but the
problem then still persists, e.g. with :

echo "{'execute':'qmp_capabilities'} " \
 "{'execute':'device-list-properties', 'arguments': " \
 "{'typename':'bcm2837'}} {'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}"   | valgrind -q \
  aarch64-softmmu/qemu-system-aarch64 -M integratorcp -S -qmp stdio

The same problem also exists for some other devices which have already
been marked with user_creatable = false, e.g.:

echo "{'execute':'qmp_capabilities'} " \
 "{'execute':'device-list-properties', " \
 "'arguments':{'typename':'macio-newworld'}} " \
 "{'execute': 'human-monitor-command', 'arguments': " \
 "{'command-line': 'info qtree'}}" | valgrind -q \
 ppc64-softmmu/qemu-system-ppc64 -M mac99 -S -qmp stdio

Hmm, maybe we need a qtest that first executes "info qtree", then runs
'device-list-properties' for all devices and finally checks "info qtree"
again ... since 'device-list-properties' should not change the qtree,
the output of the "info qtree" should be the same. Currently this is not
the case :-/

 Thomas