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

[Daniel Henrique Barboza]

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)