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

[Eduardo Habkost <ehabkost@redhat.com>]

On Wed, Jul 11, 2018 at 09:21:48AM +0200, Thomas Huth wrote:
> 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...

Crashing device-list-properties should be a blocker bug, IMO.

Moving to realize is not the best solution, but I would prefer to
do that in 3.0 instead of leaving the device-list-properties
crash unfixed.

Another solution is to reintroduce
DeviceClass::cannot_destroy_with_object_finalize_yet (commit
08f00df4f4b8b4e38ad620477cc90cf5f73832d9), and set
cannot_destroy_with_object_finalize_yet=true on bcm2837.

-- 
Eduardo