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

[Daniel Henrique Barboza]

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


Thanks,


DHB




> >   }
> >   
> >   void spapr_vscsi_create(SpaprVioBus *bus)