Re: [PATCH 1/1] spapr_vscsi: do not allow device hotplug

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 8/21/20 11:12 AM, Philippe Mathieu-Daudé wrote:
> On 8/21/20 4:08 PM, Daniel Henrique Barboza wrote:
>>
>>
>> On 8/21/20 8:08 AM, Philippe Mathieu-Daudé wrote:
>>> Cc'ing Markus
>>>
>>> On 8/20/20 9:06 PM, Daniel Henrique Barboza wrote:
>>>> We do not implement hotplug in the vscsi bus, but we forgot to
>>>> tell qdev about it. The result is that users are able to hotplug
>>>> devices in the vscsi bus, the devices appear in qdev, but they
>>>> aren't usable by the guest OS unless the user reboots it first.
>>>>
>>>> Setting qbus hotplug_handler to NULL will tell qdev-monitor, via
>>>> qbus_is_hotpluggable(), that we do not support hotplug operations
>>>> in spapr_vscsi.
>>>>
>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1862059
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>>    hw/scsi/spapr_vscsi.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
>>>> index d17dc03c73..57f0a1336f 100644
>>>> --- a/hw/scsi/spapr_vscsi.c
>>>> +++ b/hw/scsi/spapr_vscsi.c
>>>> @@ -1219,6 +1219,9 @@ static void spapr_vscsi_realize(SpaprVioDevice
>>>> *dev, Error **errp)
>>>>          scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
>>>>                     &vscsi_scsi_info, NULL);
>>>> +
>>>> +    /* ibmvscsi SCSI bus does not allow hotplug. */
>>>> +    qbus_set_hotplug_handler(BUS(&s->bus), NULL);
>>>
>>> Can't this be a problem later in DeviceClass::unrealize()?
>>
>> Not as far as I've tested. A call to qbus_set_hotplug_handler(bus,NULL)
>> after setting it to NULL isn't breaking anything either (just tested).
>>
>> I verified before sending the patch that setting hotplug_handler to
>> NULL is done in some unrealize() calls in buses, but not on devices.
>> And I'm not sure which instance would cause an unrealize() in the
>> device to fail if the hotplug_handler of the bus is NULL. As far as
>> I'm concerned this shouldn't be happening in our case here, where we're
>> not dealing with hotplug devices in the bus at all.
>>
>> Which potential problems are you referring to?
> 
> I was wondering if qdev_unrealize() runs similar checks than
> qdev_realize().

I have no idea. I would need to check/study the code to answer that.

> 
>>
>>>
>>> I was expecting something like, overwriting the parent bus type:
>>>
>>> -- >8 --
>>> @@ -1271,6 +1271,7 @@ static void spapr_vscsi_class_init(ObjectClass
>>> *klass, void *data)
>>>        DeviceClass *dc = DEVICE_CLASS(klass);
>>>        SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_CLASS(klass);
>>>
>>> +    k->bus_type = NULL; /* ibmvscsi SCSI bus does not allow hotplug. */
>>>        k->realize = spapr_vscsi_realize;
>>>        k->reset = spapr_vscsi_reset;
>>>        k->devnode = spapr_vscsi_devnode;
>>> ---
>>
>> spapr_vscsi is not a bus, is an interface. Setting NULL to bus_type in
>> spapr_vio
>> breaks guest init:
>>
>>
>> qemu-system-ppc64: /home/danielhb/qemu/hw/core/qdev.c:102:
>> qdev_set_parent_bus: Assertion `dc->bus_type &&
>> object_dynamic_cast(OBJECT(bus), dc->bus_type)' failed.
>> Aborted
>>
>>
>> I'm not so sure this would be better than what I'm doing either.
>> qdev_device_add()
>> calls qbus_is_hotpluggable() to see if the chosen bus allows hotplug. This
>> function verifies if bus->hotplug_handler is NULL. What I'm doing is simply
>> setting hotplug_handler to NULL in the SCSI bus instance that belongs to
>> spapr_vscsi. As far as I understand this is a valid use of the qdev API - I
>> should be able to set hotplug_handler to NULL if I don't want devices being
>> hotplugged in the bus I'm instantiating. Either that, or
>> qbus_is_hotpluggable()
>> must check for something else that I can safely turn off.
> 
> I'm not saying I know the correct way to do that, this is why I added
> Markus in the loop :) I'm wondering from an API point of view what is
> the best way to achieve what you want. I'm glad this works this way.


It's alright to question hehehe far too often we're remembered that 'works this
way' is a looong way from 'this is the right way'. Let's see if Markus agrees
with this approach or if there is a better way of doing it. I'll happily
change for a safer approach that will not hit me in the back in the future.


Thanks,


DHB

> 
>>
>>
>> Thanks,
>>
>>
>> DHB
>>
>>
>>
>>
>>>
>>>>    }
>>>>      void spapr_vscsi_create(SpaprVioBus *bus)
>>>>
>>>
>>
>