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

[Philippe Mathieu-Daudé]

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

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