From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1EAE8C3DA6E for ; Wed, 20 Dec 2023 07:54:28 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rFrOi-0004iB-KU; Wed, 20 Dec 2023 02:53:44 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rFrOY-0004gO-Oj for qemu-devel@nongnu.org; Wed, 20 Dec 2023 02:53:35 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rFrOU-0005x5-M8 for qemu-devel@nongnu.org; Wed, 20 Dec 2023 02:53:32 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1703058806; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=+PBX4G9iHIKLgigmXNC+ZfT9BGo2GdbXEndjPZvY/Ek=; b=MsHI9nqspWbDl6WLGnbh0HMbdG0rArZOAGbwFjudDUTi5phkpvDY+R7K4L8aK2xgwujBy4 6XJVFHjHHOiKaLfLETQk5CCHHeMLL5pSaqRK+GAheQ/wIYtD24w5RQ5UOlhbSpe4+Qu4v5 /Dt7EVz4IVQRT+Fh3ARjqqoXV1uObf8= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-515-XNqedwWFPYaOQlJODXHgxA-1; Wed, 20 Dec 2023 02:53:23 -0500 X-MC-Unique: XNqedwWFPYaOQlJODXHgxA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B64A48B0D61; Wed, 20 Dec 2023 07:53:22 +0000 (UTC) Received: from blackfin.pond.sub.org (unknown [10.39.192.129]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 77BAC2166B31; Wed, 20 Dec 2023 07:53:22 +0000 (UTC) Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id 56E5421E6920; Wed, 20 Dec 2023 08:53:21 +0100 (CET) From: Markus Armbruster To: Akihiko Odaki Cc: Paolo Bonzini , Daniel P. =?utf-8?Q?Berrang?= =?utf-8?Q?=C3=A9?= , Eduardo Habkost , qemu-devel@nongnu.org Subject: Re: [PATCH v2] qdev: Report an error for machine without HotplugHandler In-Reply-To: (Akihiko Odaki's message of "Tue, 19 Dec 2023 21:08:45 +0900") References: <20231210-bus-v2-1-34ebf5726fa0@daynix.com> <87h6kpgrl7.fsf@pond.sub.org> <8734vzsj6k.fsf@pond.sub.org> Date: Wed, 20 Dec 2023 08:53:21 +0100 Message-ID: <87y1dpgvim.fsf@pond.sub.org> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 Received-SPF: pass client-ip=170.10.129.124; envelope-from=armbru@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.066, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Akihiko Odaki writes: > On 2023/12/18 23:02, Markus Armbruster wrote: >> Akihiko Odaki writes: >> >>> On 2023/12/11 15:51, Markus Armbruster wrote: >>>> Akihiko Odaki writes: >>>> >>>>> The HotplugHandler of the machine will be used when the parent bus does >>>>> not exist, but the machine may not have one. Report an error in such a >>>>> case instead of aborting. >>>>> >>>>> Fixes: 7716b8ca74 ("qdev: HotplugHandler: Add support for unplugging BUS-less devices") >>>>> Signed-off-by: Akihiko Odaki >>>> >>>> Do you have a reproducer for the crash? >>>> >>>>> --- >>>>> Changes in v2: >>>>> - Fixed indention. >>>>> - Link to v1: https://lore.kernel.org/r/20231202-bus-v1-1-f7540e3a8d62@daynix.com >>>>> --- >>>>> system/qdev-monitor.c | 13 ++++++++++--- >>>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c >>>>> index a13db763e5..5fe5d49c20 100644 >>>>> --- a/system/qdev-monitor.c >>>>> +++ b/system/qdev-monitor.c >>>>> @@ -927,9 +927,16 @@ void qdev_unplug(DeviceState *dev, Error **errp) >>>> void qdev_unplug(DeviceState *dev, Error **errp) >>>> { >>>> DeviceClass *dc = DEVICE_GET_CLASS(dev); >>>> HotplugHandler *hotplug_ctrl; >>>> HotplugHandlerClass *hdc; >>>> Error *local_err = NULL; >>>> if (qdev_unplug_blocked(dev, errp)) { >>>> return; >>>> } >>>> if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) { >>>> error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); >>>> return; >>>> } >>>> if (!dc->hotpluggable) { >>>> error_setg(errp, QERR_DEVICE_NO_HOTPLUG, >>>> object_get_typename(OBJECT(dev))); >>>> return; >>>> } >>>> if (!migration_is_idle() && !dev->allow_unplug_during_migration) { >>>> error_setg(errp, "device_del not allowed while migrating"); >>>> return; >>>> } >>>> >>>>> qdev_hot_removed = true; >>>>> hotplug_ctrl = qdev_get_hotplug_handler(dev); >>>>> - /* hotpluggable device MUST have HotplugHandler, if it doesn't >>>>> - * then something is very wrong with it */ >>>>> - g_assert(hotplug_ctrl); >>>>> + if (!hotplug_ctrl) { >>>>> + /* >>>>> + * hotpluggable bus MUST have HotplugHandler, if it doesn't >>>>> + * then something is very wrong with it >>>>> + */ >>>>> + assert(!dev->parent_bus); >>>>> + >>>>> + error_setg(errp, "The machine does not support hotplugging for a device without parent bus"); >>>>> + return; >>>>> + } >>>> >>>> Extended version of my question above: what are the devices where >>>> qdev_get_hotplug_handler(dev) returns null here? >>> >>> Start a VM: qemu-system-aarch64 -M virt -nographic >>> Run the following on its HMP: device_del /machine/unattached/device[0] >>> >>> It tries to unplug cortex-a15-arm-cpu and crashes. >> >> This device has no parent bus (dev->parent_bus is null), but is marked >> hot-pluggable (dc->hotpluggable is true). Question for somebody >> familiar with the hot-plug machinery: is this sane? > > Setting hotpluggable false for each device without bus_type gives the same effect, but is error-prone. Having hotpluggable = true when the device cannot be hot-plugged is *wrong*. You might be able to paper over the wrongness so the code works anyway, but nothing good can come out of lying to developers trying to understand how the code works. Three ideas to avoid the lying: 1. default hotpluggable to bus_type != NULL. 2. assert(dc->bus_type || !dc->hotpluggable) in a suitable spot. 3. Change the meaning of hotpluggable, and rename it to reflect its new meaning. Requires a careful reading of its uses. I wouldn't go there.