On 09.07.2018 23:42, Peter Maydell wrote:
> On 9 July 2018 at 22:03, Thomas Huth <th...@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?
> 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
