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

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

On 2023/12/20 16:53, Markus Armbruster wrote:
> 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.

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