* [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
@ 2017-10-03 16:46 Thomas Huth
2017-10-03 18:49 ` Eduardo Habkost
2017-10-04 11:36 ` Igor Mammedov
0 siblings, 2 replies; 10+ messages in thread
From: Thomas Huth @ 2017-10-03 16:46 UTC (permalink / raw)
To: qemu-devel, Eduardo Habkost
Cc: Markus Armbruster, Dr. David Alan Gilbert, Igor Mammedov, Paolo Bonzini
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. This could be
provoked for a couple of devices in the past (see commit 4c93950659487c7ad
or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
controller when they are suitable for device_add.
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,
there is no appropriate check available. 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.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
This is the follow-up patch from my earlier try "hw/core/qdev: Do not
allow hot-plugging without hotplug controller" ... AFAICS the function
qdev_device_add() is now the right spot to do the check.
hw/core/qdev.c | 28 ++++++++++++++++++++--------
include/hw/qdev-core.h | 1 +
qdev-monitor.c | 9 +++++++++
3 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 606ab53..a953ec9 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
dev->alias_required_for_version = required_for_version;
}
+HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
+{
+ MachineState *machine;
+ MachineClass *mc;
+ Object *m_obj = qdev_get_machine();
+
+ if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
+ machine = MACHINE(m_obj);
+ mc = MACHINE_GET_CLASS(machine);
+ if (mc->get_hotplug_handler) {
+ return mc->get_hotplug_handler(machine, dev);
+ }
+ }
+
+ return NULL;
+}
+
HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
{
- HotplugHandler *hotplug_ctrl = NULL;
+ HotplugHandler *hotplug_ctrl;
if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
hotplug_ctrl = dev->parent_bus->hotplug_handler;
- } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
- MachineState *machine = MACHINE(qdev_get_machine());
- MachineClass *mc = MACHINE_GET_CLASS(machine);
-
- if (mc->get_hotplug_handler) {
- hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
- }
+ } else {
+ hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
}
return hotplug_ctrl;
}
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 0891461..5aa536d 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
void qdev_init_nofail(DeviceState *dev);
void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
int required_for_version);
+HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
void qdev_unplug(DeviceState *dev, Error **errp);
void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 8fd6df9..2891dde 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
return NULL;
}
+ /* In case we don't have a bus, there must be a machine hotplug handler */
+ if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
+ error_setg(errp, "Device '%s' can not be hotplugged on this machine",
+ driver);
+ object_unparent(OBJECT(dev));
+ object_unref(OBJECT(dev));
+ return NULL;
+ }
+
dev->opts = opts;
object_property_set_bool(OBJECT(dev), true, "realized", &err);
if (err != NULL) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-03 16:46 [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device Thomas Huth
@ 2017-10-03 18:49 ` Eduardo Habkost
2017-10-05 8:36 ` Igor Mammedov
2017-10-04 11:36 ` Igor Mammedov
1 sibling, 1 reply; 10+ messages in thread
From: Eduardo Habkost @ 2017-10-03 18:49 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Markus Armbruster, Dr. David Alan Gilbert,
Igor Mammedov, Paolo Bonzini
On Tue, Oct 03, 2017 at 06:46:02PM +0200, Thomas Huth wrote:
> 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. This could be
> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
> or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
> controller when they are suitable for device_add.
> 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,
> there is no appropriate check available. 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.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> This is the follow-up patch from my earlier try "hw/core/qdev: Do not
> allow hot-plugging without hotplug controller" ... AFAICS the function
> qdev_device_add() is now the right spot to do the check.
>
> hw/core/qdev.c | 28 ++++++++++++++++++++--------
> include/hw/qdev-core.h | 1 +
> qdev-monitor.c | 9 +++++++++
> 3 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 606ab53..a953ec9 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> dev->alias_required_for_version = required_for_version;
> }
>
> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
> +{
> + MachineState *machine;
> + MachineClass *mc;
> + Object *m_obj = qdev_get_machine();
> +
> + if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> + machine = MACHINE(m_obj);
> + mc = MACHINE_GET_CLASS(machine);
> + if (mc->get_hotplug_handler) {
> + return mc->get_hotplug_handler(machine, dev);
> + }
> + }
> +
> + return NULL;
> +}
> +
> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> {
> - HotplugHandler *hotplug_ctrl = NULL;
> + HotplugHandler *hotplug_ctrl;
>
> if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> hotplug_ctrl = dev->parent_bus->hotplug_handler;
> - } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> - MachineState *machine = MACHINE(qdev_get_machine());
> - MachineClass *mc = MACHINE_GET_CLASS(machine);
> -
> - if (mc->get_hotplug_handler) {
> - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> - }
> + } else {
> + hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
> }
> return hotplug_ctrl;
> }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 0891461..5aa536d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
> void qdev_init_nofail(DeviceState *dev);
> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> int required_for_version);
> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> void qdev_unplug(DeviceState *dev, Error **errp);
> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 8fd6df9..2891dde 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> return NULL;
> }
>
> + /* In case we don't have a bus, there must be a machine hotplug handler */
> + if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
> + error_setg(errp, "Device '%s' can not be hotplugged on this machine",
> + driver);
> + object_unparent(OBJECT(dev));
Isn't it better to check qdev_get_machine_hotplug_handler()
earlier (before the qdev_set_parent_bus() and qdev_set_id()
lines), so object_unparent() isn't necessary?
(We probably don't need to call object_unparent() here, already,
because bus is NULL. But moving the check before the "if (bus)
qdev_set_parent_bus()" statement would make this more obvious).
I would prefer to eventually make
MachineClass::get_hotplug_handler() get a typename or
DeviceClass* argument instead of DeviceState*, so we don't even
create the device object. But I don't think it's a requirement
for this bug fix.
> + object_unref(OBJECT(dev));
> + return NULL;
> + }
> +
> dev->opts = opts;
> object_property_set_bool(OBJECT(dev), true, "realized", &err);
> if (err != NULL) {
> --
> 1.8.3.1
>
--
Eduardo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-03 16:46 [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device Thomas Huth
2017-10-03 18:49 ` Eduardo Habkost
@ 2017-10-04 11:36 ` Igor Mammedov
2017-10-04 19:29 ` Thomas Huth
1 sibling, 1 reply; 10+ messages in thread
From: Igor Mammedov @ 2017-10-04 11:36 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Eduardo Habkost, Paolo Bonzini, Markus Armbruster,
Dr. David Alan Gilbert, Peter Maydell
On Tue, 3 Oct 2017 18:46:02 +0200
Thomas Huth <thuth@redhat.com> wrote:
> 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. This could be
> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
> or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
> controller when they are suitable for device_add.
> 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,
> there is no appropriate check available. 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.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> This is the follow-up patch from my earlier try "hw/core/qdev: Do not
> allow hot-plugging without hotplug controller" ... AFAICS the function
> qdev_device_add() is now the right spot to do the check.
>
> hw/core/qdev.c | 28 ++++++++++++++++++++--------
> include/hw/qdev-core.h | 1 +
> qdev-monitor.c | 9 +++++++++
> 3 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 606ab53..a953ec9 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> dev->alias_required_for_version = required_for_version;
> }
>
> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
> +{
> + MachineState *machine;
> + MachineClass *mc;
> + Object *m_obj = qdev_get_machine();
> +
> + if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> + machine = MACHINE(m_obj);
> + mc = MACHINE_GET_CLASS(machine);
> + if (mc->get_hotplug_handler) {
> + return mc->get_hotplug_handler(machine, dev);
> + }
> + }
> +
> + return NULL;
> +}
> +
> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> {
> - HotplugHandler *hotplug_ctrl = NULL;
> + HotplugHandler *hotplug_ctrl;
>
> if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> hotplug_ctrl = dev->parent_bus->hotplug_handler;
> - } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> - MachineState *machine = MACHINE(qdev_get_machine());
> - MachineClass *mc = MACHINE_GET_CLASS(machine);
> -
> - if (mc->get_hotplug_handler) {
> - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> - }
> + } else {
> + hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
> }
> return hotplug_ctrl;
> }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 0891461..5aa536d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
> void qdev_init_nofail(DeviceState *dev);
> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> int required_for_version);
> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> void qdev_unplug(DeviceState *dev, Error **errp);
> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 8fd6df9..2891dde 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> return NULL;
> }
>
> + /* In case we don't have a bus, there must be a machine hotplug handler */
> + if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
current machine hotplug handler serves both cold and hot-plug so in reality it's
just 'plug' handler.
Is there a point -device/device_add devices on board that doesn't have 'hotplug'
handler that would wire device up properly?
Do we have such devices?
> + error_setg(errp, "Device '%s' can not be hotplugged on this machine",
> + driver);
> + object_unparent(OBJECT(dev));
> + object_unref(OBJECT(dev));
> + return NULL;
> + }
> +
> dev->opts = opts;
> object_property_set_bool(OBJECT(dev), true, "realized", &err);
> if (err != NULL) {
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-04 11:36 ` Igor Mammedov
@ 2017-10-04 19:29 ` Thomas Huth
2017-10-04 21:21 ` Eduardo Habkost
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2017-10-04 19:29 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Eduardo Habkost, Paolo Bonzini, Markus Armbruster,
Dr. David Alan Gilbert, Peter Maydell
On 04.10.2017 13:36, Igor Mammedov wrote:
> On Tue, 3 Oct 2017 18:46:02 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
>> 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. This could be
>> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
>> or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
>> controller when they are suitable for device_add.
>> 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,
>> there is no appropriate check available. 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.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> This is the follow-up patch from my earlier try "hw/core/qdev: Do not
>> allow hot-plugging without hotplug controller" ... AFAICS the function
>> qdev_device_add() is now the right spot to do the check.
>>
>> hw/core/qdev.c | 28 ++++++++++++++++++++--------
>> include/hw/qdev-core.h | 1 +
>> qdev-monitor.c | 9 +++++++++
>> 3 files changed, 30 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 606ab53..a953ec9 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>> dev->alias_required_for_version = required_for_version;
>> }
>>
>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
>> +{
>> + MachineState *machine;
>> + MachineClass *mc;
>> + Object *m_obj = qdev_get_machine();
>> +
>> + if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
>> + machine = MACHINE(m_obj);
>> + mc = MACHINE_GET_CLASS(machine);
>> + if (mc->get_hotplug_handler) {
>> + return mc->get_hotplug_handler(machine, dev);
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>> {
>> - HotplugHandler *hotplug_ctrl = NULL;
>> + HotplugHandler *hotplug_ctrl;
>>
>> if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>> hotplug_ctrl = dev->parent_bus->hotplug_handler;
>> - } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
>> - MachineState *machine = MACHINE(qdev_get_machine());
>> - MachineClass *mc = MACHINE_GET_CLASS(machine);
>> -
>> - if (mc->get_hotplug_handler) {
>> - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
>> - }
>> + } else {
>> + hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
>> }
>> return hotplug_ctrl;
>> }
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 0891461..5aa536d 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
>> void qdev_init_nofail(DeviceState *dev);
>> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>> int required_for_version);
>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
>> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>> void qdev_unplug(DeviceState *dev, Error **errp);
>> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 8fd6df9..2891dde 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>> return NULL;
>> }
>>
>> + /* In case we don't have a bus, there must be a machine hotplug handler */
>> + if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
> current machine hotplug handler serves both cold and hot-plug so in reality it's
> just 'plug' handler.
>
> Is there a point -device/device_add devices on board that doesn't have 'hotplug'
> handler that would wire device up properly?
Sorry, I did not get that question ... do you mean whether there's any
code that uses qdev_device_add() to add a device without hotplug
controller? I don't think so. It's currently only used by
qmp_device_add() for the QMP/HMP command, in vl.c for -device and in the
USB code for xen-usb host device. So this function currently really only
makes sense for devices that have a hotplug controller.
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-04 19:29 ` Thomas Huth
@ 2017-10-04 21:21 ` Eduardo Habkost
2017-10-05 5:56 ` Thomas Huth
0 siblings, 1 reply; 10+ messages in thread
From: Eduardo Habkost @ 2017-10-04 21:21 UTC (permalink / raw)
To: Thomas Huth
Cc: Igor Mammedov, qemu-devel, Paolo Bonzini, Markus Armbruster,
Dr. David Alan Gilbert, Peter Maydell
On Wed, Oct 04, 2017 at 09:29:54PM +0200, Thomas Huth wrote:
> On 04.10.2017 13:36, Igor Mammedov wrote:
> > On Tue, 3 Oct 2017 18:46:02 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >
> >> 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. This could be
> >> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
> >> or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
> >> controller when they are suitable for device_add.
> >> 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,
> >> there is no appropriate check available. 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.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >> This is the follow-up patch from my earlier try "hw/core/qdev: Do not
> >> allow hot-plugging without hotplug controller" ... AFAICS the function
> >> qdev_device_add() is now the right spot to do the check.
> >>
> >> hw/core/qdev.c | 28 ++++++++++++++++++++--------
> >> include/hw/qdev-core.h | 1 +
> >> qdev-monitor.c | 9 +++++++++
> >> 3 files changed, 30 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index 606ab53..a953ec9 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> >> dev->alias_required_for_version = required_for_version;
> >> }
> >>
> >> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
> >> +{
> >> + MachineState *machine;
> >> + MachineClass *mc;
> >> + Object *m_obj = qdev_get_machine();
> >> +
> >> + if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> >> + machine = MACHINE(m_obj);
> >> + mc = MACHINE_GET_CLASS(machine);
> >> + if (mc->get_hotplug_handler) {
> >> + return mc->get_hotplug_handler(machine, dev);
> >> + }
> >> + }
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> >> {
> >> - HotplugHandler *hotplug_ctrl = NULL;
> >> + HotplugHandler *hotplug_ctrl;
> >>
> >> if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >> hotplug_ctrl = dev->parent_bus->hotplug_handler;
> >> - } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> >> - MachineState *machine = MACHINE(qdev_get_machine());
> >> - MachineClass *mc = MACHINE_GET_CLASS(machine);
> >> -
> >> - if (mc->get_hotplug_handler) {
> >> - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> >> - }
> >> + } else {
> >> + hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
> >> }
> >> return hotplug_ctrl;
> >> }
> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> >> index 0891461..5aa536d 100644
> >> --- a/include/hw/qdev-core.h
> >> +++ b/include/hw/qdev-core.h
> >> @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
> >> void qdev_init_nofail(DeviceState *dev);
> >> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> >> int required_for_version);
> >> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> >> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> >> void qdev_unplug(DeviceState *dev, Error **errp);
> >> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> >> diff --git a/qdev-monitor.c b/qdev-monitor.c
> >> index 8fd6df9..2891dde 100644
> >> --- a/qdev-monitor.c
> >> +++ b/qdev-monitor.c
> >> @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> >> return NULL;
> >> }
> >>
> >> + /* In case we don't have a bus, there must be a machine hotplug handler */
> >> + if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
> > current machine hotplug handler serves both cold and hot-plug so in reality it's
> > just 'plug' handler.
> >
> > Is there a point -device/device_add devices on board that doesn't have 'hotplug'
> > handler that would wire device up properly?
>
> Sorry, I did not get that question ... do you mean whether there's any
> code that uses qdev_device_add() to add a device without hotplug
> controller? I don't think so. It's currently only used by
> qmp_device_add() for the QMP/HMP command, in vl.c for -device and in the
> USB code for xen-usb host device. So this function currently really only
> makes sense for devices that have a hotplug controller.
I assume you are talking only about hotpluggable devices. With
-device, qdev_device_add() is also used for devices that don't
have a hotplug controller, but this is supposed to be true only
for non-hotpluggable devices.
--
Eduardo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-04 21:21 ` Eduardo Habkost
@ 2017-10-05 5:56 ` Thomas Huth
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Huth @ 2017-10-05 5:56 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Igor Mammedov, qemu-devel, Paolo Bonzini, Markus Armbruster,
Dr. David Alan Gilbert, Peter Maydell
On 04.10.2017 23:21, Eduardo Habkost wrote:
> On Wed, Oct 04, 2017 at 09:29:54PM +0200, Thomas Huth wrote:
>> On 04.10.2017 13:36, Igor Mammedov wrote:
>>> On Tue, 3 Oct 2017 18:46:02 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> 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. This could be
>>>> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
>>>> or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
>>>> controller when they are suitable for device_add.
>>>> 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,
>>>> there is no appropriate check available. 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.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> This is the follow-up patch from my earlier try "hw/core/qdev: Do not
>>>> allow hot-plugging without hotplug controller" ... AFAICS the function
>>>> qdev_device_add() is now the right spot to do the check.
>>>>
>>>> hw/core/qdev.c | 28 ++++++++++++++++++++--------
>>>> include/hw/qdev-core.h | 1 +
>>>> qdev-monitor.c | 9 +++++++++
>>>> 3 files changed, 30 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>> index 606ab53..a953ec9 100644
>>>> --- a/hw/core/qdev.c
>>>> +++ b/hw/core/qdev.c
>>>> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>>> dev->alias_required_for_version = required_for_version;
>>>> }
>>>>
>>>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
>>>> +{
>>>> + MachineState *machine;
>>>> + MachineClass *mc;
>>>> + Object *m_obj = qdev_get_machine();
>>>> +
>>>> + if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
>>>> + machine = MACHINE(m_obj);
>>>> + mc = MACHINE_GET_CLASS(machine);
>>>> + if (mc->get_hotplug_handler) {
>>>> + return mc->get_hotplug_handler(machine, dev);
>>>> + }
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>>>> {
>>>> - HotplugHandler *hotplug_ctrl = NULL;
>>>> + HotplugHandler *hotplug_ctrl;
>>>>
>>>> if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>>>> hotplug_ctrl = dev->parent_bus->hotplug_handler;
>>>> - } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
>>>> - MachineState *machine = MACHINE(qdev_get_machine());
>>>> - MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>> -
>>>> - if (mc->get_hotplug_handler) {
>>>> - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
>>>> - }
>>>> + } else {
>>>> + hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
>>>> }
>>>> return hotplug_ctrl;
>>>> }
>>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>>> index 0891461..5aa536d 100644
>>>> --- a/include/hw/qdev-core.h
>>>> +++ b/include/hw/qdev-core.h
>>>> @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
>>>> void qdev_init_nofail(DeviceState *dev);
>>>> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>>> int required_for_version);
>>>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
>>>> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>>>> void qdev_unplug(DeviceState *dev, Error **errp);
>>>> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>>> index 8fd6df9..2891dde 100644
>>>> --- a/qdev-monitor.c
>>>> +++ b/qdev-monitor.c
>>>> @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>>> return NULL;
>>>> }
>>>>
>>>> + /* In case we don't have a bus, there must be a machine hotplug handler */
>>>> + if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
>>> current machine hotplug handler serves both cold and hot-plug so in reality it's
>>> just 'plug' handler.
>>>
>>> Is there a point -device/device_add devices on board that doesn't have 'hotplug'
>>> handler that would wire device up properly?
>>
>> Sorry, I did not get that question ... do you mean whether there's any
>> code that uses qdev_device_add() to add a device without hotplug
>> controller? I don't think so. It's currently only used by
>> qmp_device_add() for the QMP/HMP command, in vl.c for -device and in the
>> USB code for xen-usb host device. So this function currently really only
>> makes sense for devices that have a hotplug controller.
>
> I assume you are talking only about hotpluggable devices. With
> -device, qdev_device_add() is also used for devices that don't
> have a hotplug controller, but this is supposed to be true only
> for non-hotpluggable devices.
Right, the cold-pluggable devices should be fine because of the
"qdev_hotplug" check, forgot to mention that, sorry.
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-03 18:49 ` Eduardo Habkost
@ 2017-10-05 8:36 ` Igor Mammedov
2017-10-05 9:00 ` Thomas Huth
0 siblings, 1 reply; 10+ messages in thread
From: Igor Mammedov @ 2017-10-05 8:36 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Thomas Huth, Paolo Bonzini, qemu-devel, Dr. David Alan Gilbert,
Markus Armbruster
On Tue, 3 Oct 2017 15:49:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 03, 2017 at 06:46:02PM +0200, Thomas Huth wrote:
> > 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. This could be
> > provoked for a couple of devices in the past (see commit 4c93950659487c7ad
> > or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
> > controller when they are suitable for device_add.
> > 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,
> > there is no appropriate check available. 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.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> > This is the follow-up patch from my earlier try "hw/core/qdev: Do not
> > allow hot-plugging without hotplug controller" ... AFAICS the function
> > qdev_device_add() is now the right spot to do the check.
> >
> > hw/core/qdev.c | 28 ++++++++++++++++++++--------
> > include/hw/qdev-core.h | 1 +
> > qdev-monitor.c | 9 +++++++++
> > 3 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 606ab53..a953ec9 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> > dev->alias_required_for_version = required_for_version;
> > }
> >
> > +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
> > +{
> > + MachineState *machine;
> > + MachineClass *mc;
> > + Object *m_obj = qdev_get_machine();
> > +
> > + if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> > + machine = MACHINE(m_obj);
> > + mc = MACHINE_GET_CLASS(machine);
> > + if (mc->get_hotplug_handler) {
> > + return mc->get_hotplug_handler(machine, dev);
> > + }
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> > {
> > - HotplugHandler *hotplug_ctrl = NULL;
> > + HotplugHandler *hotplug_ctrl;
> >
> > if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> > hotplug_ctrl = dev->parent_bus->hotplug_handler;
> > - } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> > - MachineState *machine = MACHINE(qdev_get_machine());
> > - MachineClass *mc = MACHINE_GET_CLASS(machine);
> > -
> > - if (mc->get_hotplug_handler) {
> > - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> > - }
> > + } else {
> > + hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
> > }
> > return hotplug_ctrl;
> > }
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 0891461..5aa536d 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
> > void qdev_init_nofail(DeviceState *dev);
> > void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> > int required_for_version);
> > +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> > HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> > void qdev_unplug(DeviceState *dev, Error **errp);
> > void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 8fd6df9..2891dde 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> > return NULL;
> > }
> >
> > + /* In case we don't have a bus, there must be a machine hotplug handler */
> > + if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
> > + error_setg(errp, "Device '%s' can not be hotplugged on this machine",
> > + driver);
> > + object_unparent(OBJECT(dev));
>
> Isn't it better to check qdev_get_machine_hotplug_handler()
> earlier (before the qdev_set_parent_bus() and qdev_set_id()
> lines), so object_unparent() isn't necessary?
>
> (We probably don't need to call object_unparent() here, already,
> because bus is NULL. But moving the check before the "if (bus)
> qdev_set_parent_bus()" statement would make this more obvious).
it might be bus or bus-less device, so making check before
qdev_set_parent_bus() should be simpler.
> I would prefer to eventually make
> MachineClass::get_hotplug_handler() get a typename or
> DeviceClass* argument instead of DeviceState*, so we don't even
> create the device object. But I don't think it's a requirement
> for this bug fix.
choice of hotplug handler might theoretically depend on plugged
device instance (over-engineered? as far as I recall none does it so far)
>
>
> > + object_unref(OBJECT(dev));
> > + return NULL;
> > + }
wrt error exit path, I'd rework error path in qdev_device_add() in separate patch first
to look like it is in device_set_realized() and then just jump to appropriate label
from here.
> > +
> > dev->opts = opts;
> > object_property_set_bool(OBJECT(dev), true, "realized", &err);
> > if (err != NULL) {
> > --
> > 1.8.3.1
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-05 8:36 ` Igor Mammedov
@ 2017-10-05 9:00 ` Thomas Huth
2017-10-05 9:12 ` Igor Mammedov
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2017-10-05 9:00 UTC (permalink / raw)
To: Igor Mammedov, Eduardo Habkost
Cc: Paolo Bonzini, qemu-devel, Markus Armbruster, Dr. David Alan Gilbert
On 05.10.2017 10:36, Igor Mammedov wrote:
> On Tue, 3 Oct 2017 15:49:46 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Tue, Oct 03, 2017 at 06:46:02PM +0200, Thomas Huth wrote:
>>> 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. This could be
>>> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
>>> or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
>>> controller when they are suitable for device_add.
>>> 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,
>>> there is no appropriate check available. 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.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> This is the follow-up patch from my earlier try "hw/core/qdev: Do not
>>> allow hot-plugging without hotplug controller" ... AFAICS the function
>>> qdev_device_add() is now the right spot to do the check.
>>>
>>> hw/core/qdev.c | 28 ++++++++++++++++++++--------
>>> include/hw/qdev-core.h | 1 +
>>> qdev-monitor.c | 9 +++++++++
>>> 3 files changed, 30 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 606ab53..a953ec9 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>> dev->alias_required_for_version = required_for_version;
>>> }
>>>
>>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
>>> +{
>>> + MachineState *machine;
>>> + MachineClass *mc;
>>> + Object *m_obj = qdev_get_machine();
>>> +
>>> + if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
>>> + machine = MACHINE(m_obj);
>>> + mc = MACHINE_GET_CLASS(machine);
>>> + if (mc->get_hotplug_handler) {
>>> + return mc->get_hotplug_handler(machine, dev);
>>> + }
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>>> {
>>> - HotplugHandler *hotplug_ctrl = NULL;
>>> + HotplugHandler *hotplug_ctrl;
>>>
>>> if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>>> hotplug_ctrl = dev->parent_bus->hotplug_handler;
>>> - } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
>>> - MachineState *machine = MACHINE(qdev_get_machine());
>>> - MachineClass *mc = MACHINE_GET_CLASS(machine);
>>> -
>>> - if (mc->get_hotplug_handler) {
>>> - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
>>> - }
>>> + } else {
>>> + hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
>>> }
>>> return hotplug_ctrl;
>>> }
>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>> index 0891461..5aa536d 100644
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
>>> void qdev_init_nofail(DeviceState *dev);
>>> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>> int required_for_version);
>>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
>>> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>>> void qdev_unplug(DeviceState *dev, Error **errp);
>>> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 8fd6df9..2891dde 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>> return NULL;
>>> }
>>>
>>> + /* In case we don't have a bus, there must be a machine hotplug handler */
>>> + if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
>>> + error_setg(errp, "Device '%s' can not be hotplugged on this machine",
>>> + driver);
>>> + object_unparent(OBJECT(dev));
>>
>> Isn't it better to check qdev_get_machine_hotplug_handler()
>> earlier (before the qdev_set_parent_bus() and qdev_set_id()
>> lines), so object_unparent() isn't necessary?
>>
>> (We probably don't need to call object_unparent() here, already,
>> because bus is NULL. But moving the check before the "if (bus)
>> qdev_set_parent_bus()" statement would make this more obvious).
> it might be bus or bus-less device, so making check before
> qdev_set_parent_bus() should be simpler.
The check for devices that have a bus is already done earlier in this
function ("if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus))") ...
but yes, I'll move the new check for bus-less devices a little bit
earlier so that the unparent is not necessary anymore.
>> I would prefer to eventually make
>> MachineClass::get_hotplug_handler() get a typename or
>> DeviceClass* argument instead of DeviceState*, so we don't even
>> create the device object. But I don't think it's a requirement
>> for this bug fix.
> choice of hotplug handler might theoretically depend on plugged
> device instance (over-engineered? as far as I recall none does it so far)
Yes, currently we might be able to do it with the typename only ... but
that seems to be a rather big rework right now, and we might indeed need
a real device instance later again, so I'd prefer to rather not do that
rework right now.
>>> + object_unref(OBJECT(dev));
>>> + return NULL;
>>> + }
> wrt error exit path, I'd rework error path in qdev_device_add() in separate patch first
> to look like it is in device_set_realized() and then just jump to appropriate label
> from here.
Ok, I can have a try whether that looks better.
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-05 9:00 ` Thomas Huth
@ 2017-10-05 9:12 ` Igor Mammedov
2017-10-05 9:15 ` Igor Mammedov
0 siblings, 1 reply; 10+ messages in thread
From: Igor Mammedov @ 2017-10-05 9:12 UTC (permalink / raw)
To: Thomas Huth
Cc: Eduardo Habkost, Paolo Bonzini, qemu-devel,
Dr. David Alan Gilbert, Markus Armbruster
On Thu, 5 Oct 2017 11:00:29 +0200
Thomas Huth <thuth@redhat.com> wrote:
> On 05.10.2017 10:36, Igor Mammedov wrote:
> > On Tue, 3 Oct 2017 15:49:46 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> >> On Tue, Oct 03, 2017 at 06:46:02PM +0200, Thomas Huth wrote:
> >>> 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. This could be
> >>> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
> >>> or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
> >>> controller when they are suitable for device_add.
> >>> 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,
> >>> there is no appropriate check available. 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.
> >>>
> >>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>> ---
> >>> This is the follow-up patch from my earlier try "hw/core/qdev: Do not
> >>> allow hot-plugging without hotplug controller" ... AFAICS the function
> >>> qdev_device_add() is now the right spot to do the check.
> >>>
> >>> hw/core/qdev.c | 28 ++++++++++++++++++++--------
> >>> include/hw/qdev-core.h | 1 +
> >>> qdev-monitor.c | 9 +++++++++
> >>> 3 files changed, 30 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>> index 606ab53..a953ec9 100644
> >>> --- a/hw/core/qdev.c
> >>> +++ b/hw/core/qdev.c
> >>> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> >>> dev->alias_required_for_version = required_for_version;
> >>> }
> >>>
> >>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
> >>> +{
> >>> + MachineState *machine;
> >>> + MachineClass *mc;
> >>> + Object *m_obj = qdev_get_machine();
> >>> +
> >>> + if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> >>> + machine = MACHINE(m_obj);
> >>> + mc = MACHINE_GET_CLASS(machine);
> >>> + if (mc->get_hotplug_handler) {
> >>> + return mc->get_hotplug_handler(machine, dev);
> >>> + }
> >>> + }
> >>> +
> >>> + return NULL;
> >>> +}
> >>> +
> >>> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> >>> {
> >>> - HotplugHandler *hotplug_ctrl = NULL;
> >>> + HotplugHandler *hotplug_ctrl;
> >>>
> >>> if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >>> hotplug_ctrl = dev->parent_bus->hotplug_handler;
> >>> - } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> >>> - MachineState *machine = MACHINE(qdev_get_machine());
> >>> - MachineClass *mc = MACHINE_GET_CLASS(machine);
> >>> -
> >>> - if (mc->get_hotplug_handler) {
> >>> - hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> >>> - }
> >>> + } else {
> >>> + hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
> >>> }
> >>> return hotplug_ctrl;
> >>> }
> >>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> >>> index 0891461..5aa536d 100644
> >>> --- a/include/hw/qdev-core.h
> >>> +++ b/include/hw/qdev-core.h
> >>> @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
> >>> void qdev_init_nofail(DeviceState *dev);
> >>> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> >>> int required_for_version);
> >>> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> >>> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> >>> void qdev_unplug(DeviceState *dev, Error **errp);
> >>> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> >>> diff --git a/qdev-monitor.c b/qdev-monitor.c
> >>> index 8fd6df9..2891dde 100644
> >>> --- a/qdev-monitor.c
> >>> +++ b/qdev-monitor.c
> >>> @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> >>> return NULL;
> >>> }
> >>>
> >>> + /* In case we don't have a bus, there must be a machine hotplug handler */
> >>> + if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
> >>> + error_setg(errp, "Device '%s' can not be hotplugged on this machine",
> >>> + driver);
> >>> + object_unparent(OBJECT(dev));
> >>
> >> Isn't it better to check qdev_get_machine_hotplug_handler()
> >> earlier (before the qdev_set_parent_bus() and qdev_set_id()
> >> lines), so object_unparent() isn't necessary?
> >>
> >> (We probably don't need to call object_unparent() here, already,
> >> because bus is NULL. But moving the check before the "if (bus)
> >> qdev_set_parent_bus()" statement would make this more obvious).
> > it might be bus or bus-less device, so making check before
> > qdev_set_parent_bus() should be simpler.
>
> The check for devices that have a bus is already done earlier in this
> function ("if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus))") ...
> but yes, I'll move the new check for bus-less devices a little bit
> earlier so that the unparent is not necessary anymore.
>
> >> I would prefer to eventually make
> >> MachineClass::get_hotplug_handler() get a typename or
> >> DeviceClass* argument instead of DeviceState*, so we don't even
> >> create the device object. But I don't think it's a requirement
> >> for this bug fix.
> > choice of hotplug handler might theoretically depend on plugged
> > device instance (over-engineered? as far as I recall none does it so far)
>
> Yes, currently we might be able to do it with the typename only ... but
> that seems to be a rather big rework right now, and we might indeed need
> a real device instance later again, so I'd prefer to rather not do that
> rework right now.
it was just remark why it uses 'dev' and a call for an action.
>
> >>> + object_unref(OBJECT(dev));
> >>> + return NULL;
> >>> + }
> > wrt error exit path, I'd rework error path in qdev_device_add() in separate patch first
> > to look like it is in device_set_realized() and then just jump to appropriate label
> > from here.
>
> Ok, I can have a try whether that looks better.
the function already has couple error exit point that
duplicate cleanup steps, so it probably would read better
with cleanup
>
> Thomas
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device
2017-10-05 9:12 ` Igor Mammedov
@ 2017-10-05 9:15 ` Igor Mammedov
0 siblings, 0 replies; 10+ messages in thread
From: Igor Mammedov @ 2017-10-05 9:15 UTC (permalink / raw)
To: Thomas Huth
Cc: Paolo Bonzini, Markus Armbruster, Eduardo Habkost,
Dr. David Alan Gilbert, qemu-devel
On Thu, 5 Oct 2017 11:12:41 +0200
Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 5 Oct 2017 11:00:29 +0200
> Thomas Huth <thuth@redhat.com> wrote:
[...]
> > Yes, currently we might be able to do it with the typename only ... but
> > that seems to be a rather big rework right now, and we might indeed need
> > a real device instance later again, so I'd prefer to rather not do that
> > rework right now.
> it was just remark why it uses 'dev' and a call for an action.
forgot the most important word
s/a call/NOT a call/
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-10-05 9:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-03 16:46 [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device Thomas Huth
2017-10-03 18:49 ` Eduardo Habkost
2017-10-05 8:36 ` Igor Mammedov
2017-10-05 9:00 ` Thomas Huth
2017-10-05 9:12 ` Igor Mammedov
2017-10-05 9:15 ` Igor Mammedov
2017-10-04 11:36 ` Igor Mammedov
2017-10-04 19:29 ` Thomas Huth
2017-10-04 21:21 ` Eduardo Habkost
2017-10-05 5:56 ` Thomas Huth
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.