From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48679) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fe5a3-0007I4-G0 for qemu-devel@nongnu.org; Fri, 13 Jul 2018 17:26:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fe5a2-0002WW-H4 for qemu-devel@nongnu.org; Fri, 13 Jul 2018 17:26:23 -0400 Date: Fri, 13 Jul 2018 18:26:13 -0300 From: Eduardo Habkost Message-ID: <20180713212613.GU31657@localhost.localdomain> References: <1531470464-21522-1-git-send-email-thuth@redhat.com> <1531470464-21522-4-git-send-email-thuth@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1531470464-21522-4-git-send-email-thuth@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 03/16] hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported machines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: qemu-devel@nongnu.org, Peter Maydell , Paolo Bonzini , qemu-arm@nongnu.org, Markus Armbruster , Beniamino Galvani , Subbaraya Sundeep , Alistair Francis , "Edgar E. Iglesias" , Andreas =?iso-8859-1?Q?F=E4rber?= On Fri, Jul 13, 2018 at 10:27:31AM +0200, Thomas Huth 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 qdev_set_parent_bus() from instance_init adds a link to the child devices > which is not valid anymore after the bcm2837 instance has been destroyed. > Unfortunately, the child devices do not get destroyed / unlinked correctly > because both object_initialize() and object_property_add_child() increase > the reference count of the child objects by one, but only one reference > is dropped when the parent gets removed. So let's use the new functions > object_initialize_child() and sysbus_init_child_obj() instead to create > the objects, which will take care of creating the child objects with the > correct reference count of one. > > Signed-off-by: Thomas Huth Reviewed-by: Eduardo Habkost The usage of &error_abort in code that can be triggered from device-list-properties still makes me nervous, but that's a separate issue. > --- > hw/arm/bcm2836.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > index 6805a7d..af97b2f 100644 > --- a/hw/arm/bcm2836.c > +++ b/hw/arm/bcm2836.c > @@ -51,25 +51,19 @@ static void bcm2836_init(Object *obj) > int n; > > for (n = 0; n < BCM283X_NCPUS; n++) { > - object_initialize(&s->cpus[n], sizeof(s->cpus[n]), > - info->cpu_type); > - object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), > - &error_abort); > + object_initialize_child(obj, "cpu[*]", &s->cpus[n], sizeof(s->cpus[n]), > + info->cpu_type, &error_abort); > } > > - object_initialize(&s->control, sizeof(s->control), TYPE_BCM2836_CONTROL); > - object_property_add_child(obj, "control", OBJECT(&s->control), NULL); > - qdev_set_parent_bus(DEVICE(&s->control), sysbus_get_default()); > + sysbus_init_child_obj(obj, "control", &s->control, sizeof(s->control), > + TYPE_BCM2836_CONTROL); > > - object_initialize(&s->peripherals, sizeof(s->peripherals), > - TYPE_BCM2835_PERIPHERALS); > - object_property_add_child(obj, "peripherals", OBJECT(&s->peripherals), > - &error_abort); > + sysbus_init_child_obj(obj, "peripherals", &s->peripherals, > + sizeof(s->peripherals), TYPE_BCM2835_PERIPHERALS); > object_property_add_alias(obj, "board-rev", OBJECT(&s->peripherals), > "board-rev", &error_abort); > object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals), > "vcram-size", &error_abort); > - qdev_set_parent_bus(DEVICE(&s->peripherals), sysbus_get_default()); > } > > static void bcm2836_realize(DeviceState *dev, Error **errp) > -- > 1.8.3.1 > -- Eduardo