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 <danielhb...@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) >>> >> >
