On 2023/12/21 1:46, Zhao Liu wrote:
Hi Markus,
On Wed, Dec 20, 2023 at 08:53:21AM +0100, Markus Armbruster wrote:
Date: Wed, 20 Dec 2023 08:53:21 +0100
From: Markus Armbruster <arm...@redhat.com>
Subject: Re: [PATCH v2] qdev: Report an error for machine without
HotplugHandler
Akihiko Odaki <akihiko.od...@daynix.com> writes:
On 2023/12/18 23:02, Markus Armbruster wrote:
Akihiko Odaki <akihiko.od...@daynix.com> writes:
On 2023/12/11 15:51, Markus Armbruster wrote:
Akihiko Odaki <akihiko.od...@daynix.com> 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 <akihiko.od...@daynix.com>
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-f7540e3a8...@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.
I don't have an idea to achieve that. Currently bus_type is set after
hotpluggable.
2. assert(dc->bus_type || !dc->hotpluggable) in a suitable spot.
It results in abortion and doesn't improve the situation.
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.
I don't have an idea for such a naming.
So I'm stuck with the current proposal. It suppresses abortion at least.
Any alternative idea is welcome.
What about 4 (or maybe 3.1) - droping this hotpluggable flag and just use a
helper (like qbus) to check if device is hotpluggable?
This removes the confusion of that flag and also reduces the number of
configuration items for DeviceState that require developer attention.
A simple helper is as follows:
Some devices simply doesn't support hotplugging even if the bus
supports. virtio-gpu-pci doesn't support hotplugging because the display
infrastructure cannot handle hotplugging, for example.
Regards,
Akihiko Odaki
