[Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[Thomas Huth]

QEMU currently aborts if the user tries to do something like this:

$ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
QEMU 2.9.93 monitor - type 'help' for more information
(qemu) device_add aux-to-i2c-bridge,id=x
(qemu) device_del x
**
ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
Aborted (core dumped)

Looks like the device is not hot-pluggable, so let's mark it
accordingly.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/misc/auxbus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 8a90ddd..2c62515 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -222,9 +222,17 @@ static inline I2CBus *aux_bridge_get_i2c_bus(AUXTOI2CState *bridge)
     return bridge->i2c_bus;
 }
 
+static void aux_bridge_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->hotpluggable = false;
+}
+
 static const TypeInfo aux_to_i2c_type_info = {
     .name = TYPE_AUXTOI2C,
     .parent = TYPE_DEVICE,
+    .class_init = aux_bridge_class_init,
     .instance_size = sizeof(AUXTOI2CState),
     .instance_init = aux_bridge_init
 };
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[Peter Maydell]

On 22 August 2017 at 16:06, Thomas Huth <thuth@redhat.com> wrote:
> QEMU currently aborts if the user tries to do something like this:
>
> $ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
> QEMU 2.9.93 monitor - type 'help' for more information
> (qemu) device_add aux-to-i2c-bridge,id=x
> (qemu) device_del x
> **
> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
>
> Looks like the device is not hot-pluggable, so let's mark it
> accordingly.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/misc/auxbus.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
> index 8a90ddd..2c62515 100644
> --- a/hw/misc/auxbus.c
> +++ b/hw/misc/auxbus.c
> @@ -222,9 +222,17 @@ static inline I2CBus *aux_bridge_get_i2c_bus(AUXTOI2CState *bridge)
>      return bridge->i2c_bus;
>  }
>
> +static void aux_bridge_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->hotpluggable = false;
> +}

Why is our default "hotpluggable" rather than "not hotpluggable" ?
We must have way more non-hotpluggable devices than hotpluggable
ones, and it takes active effort to make a hotpluggable device
model, so it seems like it would be much less bug-prone to
require hotpluggable devices to set dc->hotpluggable true
rather than all the non-hotpluggable ones to set it false...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[Thomas Huth]

On 22.08.2017 17:12, Peter Maydell wrote:
> On 22 August 2017 at 16:06, Thomas Huth <thuth@redhat.com> wrote:
>> QEMU currently aborts if the user tries to do something like this:
>>
>> $ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
>> QEMU 2.9.93 monitor - type 'help' for more information
>> (qemu) device_add aux-to-i2c-bridge,id=x
>> (qemu) device_del x
>> **
>> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
>> Aborted (core dumped)
>>
>> Looks like the device is not hot-pluggable, so let's mark it
>> accordingly.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/misc/auxbus.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
>> index 8a90ddd..2c62515 100644
>> --- a/hw/misc/auxbus.c
>> +++ b/hw/misc/auxbus.c
>> @@ -222,9 +222,17 @@ static inline I2CBus *aux_bridge_get_i2c_bus(AUXTOI2CState *bridge)
>>      return bridge->i2c_bus;
>>  }
>>
>> +static void aux_bridge_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->hotpluggable = false;
>> +}
> 
> Why is our default "hotpluggable" rather than "not hotpluggable" ?
> We must have way more non-hotpluggable devices than hotpluggable
> ones, and it takes active effort to make a hotpluggable device
> model, so it seems like it would be much less bug-prone to
> require hotpluggable devices to set dc->hotpluggable true
> rather than all the non-hotpluggable ones to set it false...

I think most devices are already non-hotpluggable automatically because
they sit on a bus that is not hot-pluggable (e.g. sysbus devices). The
problematic ones are the devices with .parent = TYPE_DEVICE. And for
these, I think it is quite hard to say whether they should be
hot-pluggable by default or not?
Anyway, according to my tests (I'm currently working on a test that
automatically does device_add + device_del for all devices, as you might
have guessed already), there are not that many devices that cause
problems here, so I guess marking some few with hotpluggable = false
should be OK?

 Thomas

Re: [Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[Peter Maydell]

On 22 August 2017 at 16:26, Thomas Huth <thuth@redhat.com> wrote:
> On 22.08.2017 17:12, Peter Maydell wrote:
>> Why is our default "hotpluggable" rather than "not hotpluggable" ?
>> We must have way more non-hotpluggable devices than hotpluggable
>> ones, and it takes active effort to make a hotpluggable device
>> model, so it seems like it would be much less bug-prone to
>> require hotpluggable devices to set dc->hotpluggable true
>> rather than all the non-hotpluggable ones to set it false...
>
> I think most devices are already non-hotpluggable automatically because
> they sit on a bus that is not hot-pluggable (e.g. sysbus devices). The
> problematic ones are the devices with .parent = TYPE_DEVICE. And for
> these, I think it is quite hard to say whether they should be
> hot-pluggable by default or not?

To implement hotplug you need to write extra code (notably some
kind of unrealize method to undo whatever you did in realize),
at which point also setting the hotpluggable flag is trivial.

> Anyway, according to my tests (I'm currently working on a test that
> automatically does device_add + device_del for all devices, as you might
> have guessed already), there are not that many devices that cause
> problems here, so I guess marking some few with hotpluggable = false
> should be OK?

The problem is not the devices we have today but the ones we're
going to write tomorrow. Having hotpluggable default to false
"fails safe" -- the worst that happens is that somebody writing
a hotpluggable device finds in their testing that they need to
set the flag to make it work. Having it default to true fails
non-safely -- as you've found, people writing devices that
were never expected to be hotplugged don't try to test the
error case of attempting hotplug anyway, and in code review
the absence of a line of code is very hard to reliably spot,
so we get devices in tree that crash QEMU if you try to
hotplug them. If we make the default be not-hotpluggable
we fix this not just today but forever.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[Thomas Huth]

On 22.08.2017 17:30, Peter Maydell wrote:
> On 22 August 2017 at 16:26, Thomas Huth <thuth@redhat.com> wrote:
>> On 22.08.2017 17:12, Peter Maydell wrote:
>>> Why is our default "hotpluggable" rather than "not hotpluggable" ?
>>> We must have way more non-hotpluggable devices than hotpluggable
>>> ones, and it takes active effort to make a hotpluggable device
>>> model, so it seems like it would be much less bug-prone to
>>> require hotpluggable devices to set dc->hotpluggable true
>>> rather than all the non-hotpluggable ones to set it false...
>>
>> I think most devices are already non-hotpluggable automatically because
>> they sit on a bus that is not hot-pluggable (e.g. sysbus devices). The
>> problematic ones are the devices with .parent = TYPE_DEVICE. And for
>> these, I think it is quite hard to say whether they should be
>> hot-pluggable by default or not?
> 
> To implement hotplug you need to write extra code (notably some
> kind of unrealize method to undo whatever you did in realize),
> at which point also setting the hotpluggable flag is trivial.
> 
>> Anyway, according to my tests (I'm currently working on a test that
>> automatically does device_add + device_del for all devices, as you might
>> have guessed already), there are not that many devices that cause
>> problems here, so I guess marking some few with hotpluggable = false
>> should be OK?
> 
> The problem is not the devices we have today but the ones we're
> going to write tomorrow. Having hotpluggable default to false
> "fails safe" -- the worst that happens is that somebody writing
> a hotpluggable device finds in their testing that they need to
> set the flag to make it work. Having it default to true fails
> non-safely -- as you've found, people writing devices that
> were never expected to be hotplugged don't try to test the
> error case of attempting hotplug anyway, and in code review
> the absence of a line of code is very hard to reliably spot,
> so we get devices in tree that crash QEMU if you try to
> hotplug them. If we make the default be not-hotpluggable
> we fix this not just today but forever.

OK, you've got a point. I'll ponder about this a little bit and will try
to come up with a patch / some patches ...

 Thomas

Re: [Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[KONRAD Frederic]

Hi Thomas,

Looking to this seems there is a second issue:
The aux-to-i2c-bridge device should connect on a TYPE_AUX_BUS.

I don't think there isn't any on integratorcp..

Anyway the patch you sent fix this issue indirectly and as far as
I remember I wasn't able to make this I2C bridge an
TYPE_AUX_DEVICE as it's a special device and it is internal (only 
instantiated at the bus creation).

Thanks,
Fred

On 08/22/2017 05:06 PM, Thomas Huth wrote:
> QEMU currently aborts if the user tries to do something like this:
> 
> $ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
> QEMU 2.9.93 monitor - type 'help' for more information
> (qemu) device_add aux-to-i2c-bridge,id=x
> (qemu) device_del x
> **
> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
> 
> Looks like the device is not hot-pluggable, so let's mark it
> accordingly.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/misc/auxbus.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
> index 8a90ddd..2c62515 100644
> --- a/hw/misc/auxbus.c
> +++ b/hw/misc/auxbus.c
> @@ -222,9 +222,17 @@ static inline I2CBus *aux_bridge_get_i2c_bus(AUXTOI2CState *bridge)
>       return bridge->i2c_bus;
>   }
>   
> +static void aux_bridge_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->hotpluggable = false;
> +}
> +
>   static const TypeInfo aux_to_i2c_type_info = {
>       .name = TYPE_AUXTOI2C,
>       .parent = TYPE_DEVICE,
> +    .class_init = aux_bridge_class_init,
>       .instance_size = sizeof(AUXTOI2CState),
>       .instance_init = aux_bridge_init
>   };
> 

Re: [Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[Thomas Huth]

On 22.08.2017 20:15, KONRAD Frederic wrote:
> Hi Thomas,
> 
> Looking to this seems there is a second issue:
> The aux-to-i2c-bridge device should connect on a TYPE_AUX_BUS.
> 
> I don't think there isn't any on integratorcp..
> 
> Anyway the patch you sent fix this issue indirectly and as far as
> I remember I wasn't able to make this I2C bridge an
> TYPE_AUX_DEVICE as it's a special device and it is internal (only
> instantiated at the bus creation).

If it is intended to be completely internal only (i.e. also not for use
with the "-device" parameter), then it should even be marked with
"user_creatable = false"! ... if you got some spare minutes, could you
maybe sent a patch for that? I'm not familiar with that code, so I'm
always having trouble to give the reasoning for setting a
"user_creatable = false" somewhere...

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable

[KONRAD Frederic]

On 08/23/2017 07:22 AM, Thomas Huth wrote:
> On 22.08.2017 20:15, KONRAD Frederic wrote:
>> Hi Thomas,
>>
>> Looking to this seems there is a second issue:
>> The aux-to-i2c-bridge device should connect on a TYPE_AUX_BUS.
>>
>> I don't think there isn't any on integratorcp..
>>
>> Anyway the patch you sent fix this issue indirectly and as far as
>> I remember I wasn't able to make this I2C bridge an
>> TYPE_AUX_DEVICE as it's a special device and it is internal (only
>> instantiated at the bus creation).
> 
> If it is intended to be completely internal only (i.e. also not for use
> with the "-device" parameter), then it should even be marked with
> "user_creatable = false"! ... if you got some spare minutes, could you
> maybe sent a patch for that? I'm not familiar with that code, so I'm
> always having trouble to give the reasoning for setting a
> "user_creatable = false" somewhere...
> 
>   Thanks,
>    Thomas
> 

Ok will do!

Thanks,
Fred