Re: [PATCH v2] qdev: Report an error for machine without HotplugHandler

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2023/12/18 23:02, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2023/12/11 15:51, Markus Armbruster wrote:
>>> Akihiko Odaki <akihiko.odaki@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.odaki@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-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.