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

[David Gibson]

On Tue, Aug 25, 2020 at 08:27:28AM +0200, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
> > 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()?
> 
> Can't say offhand.  Cc'ing QOM maintainers for actual expertise.
> 
> A quick grep coughs up virtio_serial_device_realize() /
> virtio_serial_device_unrealize(), which set and unset the hotplug
> handler of a bus their device provides.  Not the same as the code above,

Hm, it almost is, actually.  Only difference is that virtio serial is
setting the hotplug handler to something, rather than disabling it.

> just a clue that messing with a bus's hotplug handler in a realize
> method can work.
> 
> qbus_set_hotplug_handler() is entirely undocumented.  It sets a link.
> Might be intended for use by the device that provides the bus.

Heh.

> > 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;
> 
> k->bus_type does not exist.  Do you mean dc->bus_type?

Yeah, I'm pretty sure changing the bus type is going to be worse than
setting the hotplug handler.

> TYPE_VIO_SPAPR_VSCSI_DEVICE is a subtype of TYPE_VIO_SPAPR_DEVICE is a
> subtype of TYPE_DEVICE.
> 
> TYPE_DEVICE is bus-less, i.e. (concrete) instances do not plug into a
> bus.
> 
> TYPE_VIO_SPAPR_DEVICE has bus_type TYPE_SPAPR_VIO_BUS, i.e. (concrete)
> instances plug into a TYPE_SPAPR_VIO_BUS.  Both hot and cold plug.
> 
> Zapping TYPE_VIO_SPAPR_VSCSI_DEVICE's bus_type makes it bus-less.  I
> doubt that's okay.  Also, it's not just about hot plug.  What are you
> trying to accomplish?

I'm pretty sure all of the above is not relevant.  It's the
subordinate bus of TYPE_VIO_SPAPR_VSCSI_DEVICE on which we want to
disable hotplugging, not the parent bus.  We can't hotplug spapr-vscsi
controllers, but that's fine.  What we're trying to address here is
that the controller supports a TYPE_SCSI_BUS, onto which disks can
normally be hotplugged, but not in this case.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature