On 2023/12/20 16:53, Markus Armbruster wrote:
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.
Hi,
I'm now revisiting this patch and now I think it is still semantically
correct.
This patch indeed prevents hotplugging a hotpluggable device and that
may sound irrational. However, we should note that the entity that
prevents hotplugging is not the device, but the machine that lacks a
hotplug handler. So we can say the device itself is hotpluggable, but
the machine is preventing hotplugging.
We already do similar in a case that a device has a parent bus.
qbus_is_hotpluggable() returns false if the parent bus lacks a hotplug
handler and prevents from hotplugging a hotpluggable device. The device
class must still have hotpluggable = true in such a case because another
instance of device may be plugged into a bus that has a hotplug handler.
I'll submit v3 soon so please check if this reasoning sounds valid for
you and review it once I submit it.
Regards,
Akihiko Odaki
