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

[Markus Armbruster <armbru@redhat.com>]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> 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.

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.

2. assert(dc->bus_type || !dc->hotpluggable) in a suitable spot.

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.