Commit 03fcbd9dc508 ("qdev: Check for the availability of a hotplug
controller before adding a device") says:
> The qdev_unplug() function contains a g_assert(hotplug_ctrl)
> statement, so QEMU crashes when the user tries to device_add +
> device_del a device that does not have a corresponding hotplug
> controller.
> The code in qdev_device_add() already checks whether the bus has a
> proper hotplug controller, but for devices that do not have a
> corresponding bus, here is no appropriate check available yet. In that
> case we should check whether the machine itself provides a suitable
> hotplug controller and refuse to plug the device if none is available.
However, it forgot to add the corresponding check to qdev_unplug().
Most checks are comon between the hot-plug and hot-unplug scenarios so
extract them and share the implementation, saving some code and fixing
the aforementioned bug.
Fixes: 7716b8ca74 ("qdev: HotplugHandler: Add support for unplugging BUS-less
devices")
Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
---
Changes in v3:
- Extracted checks common for hot-plug and hot-unplug into a function.
- Link to v2:
https://lore.kernel.org/r/20231210-bus-v2-1-34ebf5726...@daynix.com
Changes in v2:
- Fixed indention.
- Link to v1:
https://lore.kernel.org/r/20231202-bus-v1-1-f7540e3a8...@daynix.com
---
include/hw/qdev-core.h | 1 +
hw/core/qdev-hotplug.c | 39 +++++++++++++++++++++++++++++++++++++++
system/qdev-monitor.c | 35 ++++-------------------------------
3 files changed, 44 insertions(+), 31 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 9228e96c87e9..94ee4bb26415 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -533,6 +533,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int
alias_id,
HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
+bool qdev_hotunplug_allowed(DeviceState *dev, Error **errp);
/**
* qdev_get_hotplug_handler() - Get handler responsible for device wiring
diff --git a/hw/core/qdev-hotplug.c b/hw/core/qdev-hotplug.c
index d495d0e9c70a..7785fc52267b 100644
--- a/hw/core/qdev-hotplug.c
+++ b/hw/core/qdev-hotplug.c
@@ -12,6 +12,8 @@
#include "qemu/osdep.h"
#include "hw/qdev-core.h"
#include "hw/boards.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
{
@@ -30,12 +32,43 @@ HotplugHandler
*qdev_get_machine_hotplug_handler(DeviceState *dev)
return NULL;
}
+static bool qdev_hotplug_unplug_allowed_common(DeviceState *dev, Error **errp)
+{
+ DeviceClass *dc = DEVICE_GET_CLASS(dev);
+
+ if (!dc->hotpluggable) {
+ error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
+ object_get_typename(OBJECT(dev)));
+ return false;
+ }
+
+ if (dev->parent_bus) {
+ if (!qbus_is_hotpluggable(dev->parent_bus)) {
+ error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
+ return false;
+ }
+ } else {
+ if (!qdev_get_machine_hotplug_handler(dev)) {
+ /* No bus, no machine hotplug handler --> device is not
hotpluggable */
+ error_setg(errp, "Device '%s' can not be hotplugged on this
machine",
+ object_get_typename(OBJECT(dev)));
+ return false;
+ }
+ }
+
+ return true;
+}
+
bool qdev_hotplug_allowed(DeviceState *dev, Error **errp)
{
MachineState *machine;
MachineClass *mc;
Object *m_obj = qdev_get_machine();
+ if (!qdev_hotplug_unplug_allowed_common(dev, errp)) {
+ return false;
+ }
+
if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
machine = MACHINE(m_obj);
mc = MACHINE_GET_CLASS(machine);
@@ -47,6 +80,12 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp)
return true;
}
+bool qdev_hotunplug_allowed(DeviceState *dev, Error **errp)
+{
+ return !qdev_unplug_blocked(dev, errp) &&
+ qdev_hotplug_unplug_allowed_common(dev, errp);
+}
+
HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev)
{
if (dev->parent_bus) {
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index a13db763e5dd..b079e827ee8f 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -257,8 +257,7 @@ static DeviceClass *qdev_get_device_class(const char
**driver, Error **errp)
}
dc = DEVICE_CLASS(oc);
- if (!dc->user_creatable ||
- (phase_check(PHASE_MACHINE_READY) && !dc->hotpluggable)) {
+ if (!dc->user_creatable) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
"a pluggable device type");
return NULL;
@@ -668,11 +667,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
return NULL;
}
- if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus))
{
- error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
- return NULL;
- }
-
if (!migration_is_idle()) {
error_setg(errp, "device_add not allowed while migrating");
return NULL;
@@ -682,17 +676,8 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
dev = qdev_new(driver);
/* Check whether the hotplug is allowed by the machine */
- if (phase_check(PHASE_MACHINE_READY)) {
- if (!qdev_hotplug_allowed(dev, errp)) {
- goto err_del_dev;
- }
-
- if (!bus && !qdev_get_machine_hotplug_handler(dev)) {
- /* No bus, no machine hotplug handler --> device is not
hotpluggable */
- error_setg(errp, "Device '%s' can not be hotplugged on this
machine",
- driver);
- goto err_del_dev;
- }
+ if (phase_check(PHASE_MACHINE_READY) && !qdev_hotplug_allowed(dev, errp)) {
+ goto err_del_dev;
}
/*
@@ -899,23 +884,11 @@ static DeviceState *find_device_state(const char *id,
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)));
+ if (!qdev_hotunplug_allowed(dev, errp)) {
return;
}
---
base-commit: 5005aed8a7e728d028efb40e243ecfc2b4f3df3a
change-id: 20231202-bus-75a454c5d959
Best regards,
--
Akihiko Odaki <akihiko.od...@daynix.com>
