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

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

[Daniel Henrique Barboza]

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);
 }
 
 void spapr_vscsi_create(SpaprVioBus *bus)
-- 
2.26.2

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

On Thu, Aug 20, 2020 at 04:06:35PM -0300, 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>

That was much easier than I feared.  Nice!

Applied to ppc-for-5.2.

> ---
>  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);
>  }
>  
>  void spapr_vscsi_create(SpaprVioBus *bus)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Philippe Mathieu-Daudé]

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()?

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

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

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

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

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

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

[Markus Armbruster]

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

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

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?

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3919 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]