[Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Thomas Huth]

qdev_unplug() bails out with an assertion if the user tries to device_del
a hot-plugged device that does not have a hotplug controller. Unfortunately,
our devices are all marked with hotpluggable = true by default (see the
device_class_init() function in qdev.c), so it currently can happen that
the user runs into this situation and QEMU gets terminated unexpectedly:

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

Hotplugging devices without a hotplug controller does not make much sense,
so we should disallow this during the device_add process already!

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/core/qdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 606ab53..d9ccce6 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             if (local_err != NULL) {
                 goto fail;
             }
+        } else if (dev->hotplugged) {
+            /* Hot-plugged device without hotplug controller? No way! */
+            error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG,
+                       object_get_typename(obj));
+            goto fail;
         }
 
         if (dc->realize) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Igor Mammedov]

On Thu,  7 Sep 2017 11:22:42 +0200
Thomas Huth <thuth@redhat.com> wrote:

> qdev_unplug() bails out with an assertion if the user tries to device_del
> a hot-plugged device that does not have a hotplug controller. Unfortunately,
> our devices are all marked with hotpluggable = true by default (see the
> device_class_init() function in qdev.c), so it currently can happen that
> the user runs into this situation and QEMU gets terminated unexpectedly:
> 
> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
> QEMU 2.10.50 monitor - type 'help' for more information
> (qemu) device_add aux-to-i2c-bridge,id=x
> (qemu) device_del x
> **
> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
> Hotplugging devices without a hotplug controller does not make much sense,
> so we should disallow this during the device_add process already!
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/core/qdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 606ab53..d9ccce6 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              if (local_err != NULL) {
>                  goto fail;
>              }
> +        } else if (dev->hotplugged) {
> +            /* Hot-plugged device without hotplug controller? No way! */
> +            error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG,
> +                       object_get_typename(obj));
> +            goto fail;
>          }
>  
>          if (dc->realize) {

maybe it should be other way around, i.e, fix device so that following would work

  device_set_realized()
    if (dev->hotplugged && !dc->hotpluggable) {                                  
        error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));      
        return;                                                                  
    }

instead of leaving device broken, like in yours
 84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Thomas Huth]

On 11.09.2017 14:53, Igor Mammedov wrote:
> On Thu,  7 Sep 2017 11:22:42 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> qdev_unplug() bails out with an assertion if the user tries to device_del
>> a hot-plugged device that does not have a hotplug controller. Unfortunately,
>> our devices are all marked with hotpluggable = true by default (see the
>> device_class_init() function in qdev.c), so it currently can happen that
>> the user runs into this situation and QEMU gets terminated unexpectedly:
>>
>> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
>> QEMU 2.10.50 monitor - type 'help' for more information
>> (qemu) device_add aux-to-i2c-bridge,id=x
>> (qemu) device_del x
>> **
>> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
>> Aborted (core dumped)
>> Hotplugging devices without a hotplug controller does not make much sense,
>> so we should disallow this during the device_add process already!
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/core/qdev.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 606ab53..d9ccce6 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>              if (local_err != NULL) {
>>                  goto fail;
>>              }
>> +        } else if (dev->hotplugged) {
>> +            /* Hot-plugged device without hotplug controller? No way! */
>> +            error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG,
>> +                       object_get_typename(obj));
>> +            goto fail;
>>          }
>>  
>>          if (dc->realize) {
> 
> maybe it should be other way around, i.e, fix device so that following would work
> 
>   device_set_realized()
>     if (dev->hotplugged && !dc->hotpluggable) {                                  
>         error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));      
>         return;                                                                  
>     }
> 
> instead of leaving device broken, like in yours
>  84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable

No, that apparently does not work right for new devices since people
keep forgetting to set hotpluggable = false there. Both, Paolo and Peter
suggested that we should not allow hot-plugging if there's no hot plug
controller - it indeed does not make sense, so we should not allow it.

 Thomas

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Igor Mammedov]

On Mon, 11 Sep 2017 16:31:39 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 11.09.2017 14:53, Igor Mammedov wrote:
> > On Thu,  7 Sep 2017 11:22:42 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> qdev_unplug() bails out with an assertion if the user tries to device_del
> >> a hot-plugged device that does not have a hotplug controller. Unfortunately,
> >> our devices are all marked with hotpluggable = true by default (see the
> >> device_class_init() function in qdev.c), so it currently can happen that
> >> the user runs into this situation and QEMU gets terminated unexpectedly:
> >>
> >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
> >> QEMU 2.10.50 monitor - type 'help' for more information
> >> (qemu) device_add aux-to-i2c-bridge,id=x
> >> (qemu) device_del x
> >> **
> >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> >> Aborted (core dumped)
> >> Hotplugging devices without a hotplug controller does not make much sense,
> >> so we should disallow this during the device_add process already!
> >>
> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  hw/core/qdev.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index 606ab53..d9ccce6 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >>              if (local_err != NULL) {
> >>                  goto fail;
> >>              }
> >> +        } else if (dev->hotplugged) {
> >> +            /* Hot-plugged device without hotplug controller? No way! */
> >> +            error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG,
> >> +                       object_get_typename(obj));
> >> +            goto fail;
> >>          }
> >>  
> >>          if (dc->realize) {  
> > 
> > maybe it should be other way around, i.e, fix device so that following would work
> > 
> >   device_set_realized()
> >     if (dev->hotplugged && !dc->hotpluggable) {                                  
> >         error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));      
> >         return;                                                                  
> >     }
> > 
> > instead of leaving device broken, like in yours
> >  84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable  
> 
> No, that apparently does not work right for new devices since people
> keep forgetting to set hotpluggable = false there. Both, Paolo and Peter
> suggested that we should not allow hot-plugging if there's no hot plug
> controller - it indeed does not make sense, so we should not allow it.
historically all devices were hotpluggble and conversion to hotplug
controller didn't fix it which os fine as far as user did not attempt
unreasonable things. However it should be fixedfor code to work correctly.

I'd suggest to flip default
 dc->hotpluggable = false;
and set it to true explicitly for devices that support hotplug,
it obviously harder to do than this patch as it requires audit
of all devices, but it looks more correct than fixing symptoms of
incorrectly set dc->hotpluggable property.

>  Thomas

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Eduardo Habkost]

On Mon, Sep 11, 2017 at 05:06:00PM +0200, Igor Mammedov wrote:
> On Mon, 11 Sep 2017 16:31:39 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 11.09.2017 14:53, Igor Mammedov wrote:
> > > On Thu,  7 Sep 2017 11:22:42 +0200
> > > Thomas Huth <thuth@redhat.com> wrote:
> > >   
> > >> qdev_unplug() bails out with an assertion if the user tries to device_del
> > >> a hot-plugged device that does not have a hotplug controller. Unfortunately,
> > >> our devices are all marked with hotpluggable = true by default (see the
> > >> device_class_init() function in qdev.c), so it currently can happen that
> > >> the user runs into this situation and QEMU gets terminated unexpectedly:
> > >>
> > >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
> > >> QEMU 2.10.50 monitor - type 'help' for more information
> > >> (qemu) device_add aux-to-i2c-bridge,id=x
> > >> (qemu) device_del x
> > >> **
> > >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> > >> Aborted (core dumped)
> > >> Hotplugging devices without a hotplug controller does not make much sense,
> > >> so we should disallow this during the device_add process already!
> > >>
> > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > >> ---
> > >>  hw/core/qdev.c | 5 +++++
> > >>  1 file changed, 5 insertions(+)
> > >>
> > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > >> index 606ab53..d9ccce6 100644
> > >> --- a/hw/core/qdev.c
> > >> +++ b/hw/core/qdev.c
> > >> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> > >>              if (local_err != NULL) {
> > >>                  goto fail;
> > >>              }
> > >> +        } else if (dev->hotplugged) {
> > >> +            /* Hot-plugged device without hotplug controller? No way! */
> > >> +            error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG,
> > >> +                       object_get_typename(obj));
> > >> +            goto fail;
> > >>          }
> > >>  
> > >>          if (dc->realize) {  
> > > 
> > > maybe it should be other way around, i.e, fix device so that following would work
> > > 
> > >   device_set_realized()
> > >     if (dev->hotplugged && !dc->hotpluggable) {                                  
> > >         error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));      
> > >         return;                                                                  
> > >     }
> > > 
> > > instead of leaving device broken, like in yours
> > >  84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable  
> > 
> > No, that apparently does not work right for new devices since people
> > keep forgetting to set hotpluggable = false there. Both, Paolo and Peter
> > suggested that we should not allow hot-plugging if there's no hot plug
> > controller - it indeed does not make sense, so we should not allow it.
> historically all devices were hotpluggble and conversion to hotplug
> controller didn't fix it which os fine as far as user did not attempt
> unreasonable things. However it should be fixedfor code to work correctly.
> 
> I'd suggest to flip default
>  dc->hotpluggable = false;
> and set it to true explicitly for devices that support hotplug,
> it obviously harder to do than this patch as it requires audit
> of all devices, but it looks more correct than fixing symptoms of
> incorrectly set dc->hotpluggable property.

I agree we should do this.  If we have any device-type that is
not hotpluggable on any machine because no machine will return a
hotplug controller for it, we shouldn't report it as hotpluggable
through QMP and HMP.

But this patch also seems to be required, for cases where not all
machine-types accept hotplug of a given device type.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Igor Mammedov]

On Tue, 12 Sep 2017 15:05:56 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Sep 11, 2017 at 05:06:00PM +0200, Igor Mammedov wrote:
> > On Mon, 11 Sep 2017 16:31:39 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> > > On 11.09.2017 14:53, Igor Mammedov wrote:  
> > > > On Thu,  7 Sep 2017 11:22:42 +0200
> > > > Thomas Huth <thuth@redhat.com> wrote:
> > > >     
> > > >> qdev_unplug() bails out with an assertion if the user tries to device_del
> > > >> a hot-plugged device that does not have a hotplug controller. Unfortunately,
> > > >> our devices are all marked with hotpluggable = true by default (see the
> > > >> device_class_init() function in qdev.c), so it currently can happen that
> > > >> the user runs into this situation and QEMU gets terminated unexpectedly:
> > > >>
> > > >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
> > > >> QEMU 2.10.50 monitor - type 'help' for more information
> > > >> (qemu) device_add aux-to-i2c-bridge,id=x
> > > >> (qemu) device_del x
> > > >> **
> > > >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> > > >> Aborted (core dumped)
> > > >> Hotplugging devices without a hotplug controller does not make much sense,
> > > >> so we should disallow this during the device_add process already!
> > > >>
> > > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > > >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > >> ---
> > > >>  hw/core/qdev.c | 5 +++++
> > > >>  1 file changed, 5 insertions(+)
> > > >>
> > > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > >> index 606ab53..d9ccce6 100644
> > > >> --- a/hw/core/qdev.c
> > > >> +++ b/hw/core/qdev.c
> > > >> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> > > >>              if (local_err != NULL) {
> > > >>                  goto fail;
> > > >>              }
> > > >> +        } else if (dev->hotplugged) {
> > > >> +            /* Hot-plugged device without hotplug controller? No way! */
> > > >> +            error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG,
> > > >> +                       object_get_typename(obj));
> > > >> +            goto fail;
> > > >>          }
> > > >>  
> > > >>          if (dc->realize) {    
> > > > 
> > > > maybe it should be other way around, i.e, fix device so that following would work
> > > > 
> > > >   device_set_realized()
> > > >     if (dev->hotplugged && !dc->hotpluggable) {                                  
> > > >         error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));      
> > > >         return;                                                                  
> > > >     }
> > > > 
> > > > instead of leaving device broken, like in yours
> > > >  84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable    
> > > 
> > > No, that apparently does not work right for new devices since people
> > > keep forgetting to set hotpluggable = false there. Both, Paolo and Peter
> > > suggested that we should not allow hot-plugging if there's no hot plug
> > > controller - it indeed does not make sense, so we should not allow it.  
> > historically all devices were hotpluggble and conversion to hotplug
> > controller didn't fix it which os fine as far as user did not attempt
> > unreasonable things. However it should be fixedfor code to work correctly.
> > 
> > I'd suggest to flip default
> >  dc->hotpluggable = false;
> > and set it to true explicitly for devices that support hotplug,
> > it obviously harder to do than this patch as it requires audit
> > of all devices, but it looks more correct than fixing symptoms of
> > incorrectly set dc->hotpluggable property.  
> 
> I agree we should do this.  If we have any device-type that is
> not hotpluggable on any machine because no machine will return a
> hotplug controller for it, we shouldn't report it as hotpluggable
> through QMP and HMP.
> 
> But this patch also seems to be required, for cases where not all
> machine-types accept hotplug of a given device type.
Agreed,

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Peter Maydell]

On 7 September 2017 at 10:22, Thomas Huth <thuth@redhat.com> wrote:
> qdev_unplug() bails out with an assertion if the user tries to device_del
> a hot-plugged device that does not have a hotplug controller. Unfortunately,
> our devices are all marked with hotpluggable = true by default (see the
> device_class_init() function in qdev.c), so it currently can happen that
> the user runs into this situation and QEMU gets terminated unexpectedly:

Why are our devices hotpluggable by default? Hotpluggable
is the unusual case which requires the device author to
take special case (notably writing a function to undo anything
done in realize so we don't leak stuff on unplug), so it
should be the case that requires the device author to
mark the device as hotplugged specifically.

We should just change the default (and effectively whitelist
devices which do hotplug successfully), or we'll be continually
playing whack-a-mole with devices or classes of devices or
machines that accidentally forgot to disable hotplugging.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Eduardo Habkost]

On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote:
> qdev_unplug() bails out with an assertion if the user tries to device_del
> a hot-plugged device that does not have a hotplug controller. Unfortunately,
> our devices are all marked with hotpluggable = true by default (see the
> device_class_init() function in qdev.c), so it currently can happen that
> the user runs into this situation and QEMU gets terminated unexpectedly:
> 
> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
> QEMU 2.10.50 monitor - type 'help' for more information
> (qemu) device_add aux-to-i2c-bridge,id=x
> (qemu) device_del x
> **
> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
> 
> Hotplugging devices without a hotplug controller does not make much sense,
> so we should disallow this during the device_add process already!
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

I'm queueing this on machine-next.  We still want it even if we
apply the patch that changes TYPE_DEVICE to hotpluggable=false by
default, right?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Thomas Huth]

On 21.09.2017 19:56, Eduardo Habkost wrote:
> On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote:
>> qdev_unplug() bails out with an assertion if the user tries to device_del
>> a hot-plugged device that does not have a hotplug controller. Unfortunately,
>> our devices are all marked with hotpluggable = true by default (see the
>> device_class_init() function in qdev.c), so it currently can happen that
>> the user runs into this situation and QEMU gets terminated unexpectedly:
>>
>> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
>> QEMU 2.10.50 monitor - type 'help' for more information
>> (qemu) device_add aux-to-i2c-bridge,id=x
>> (qemu) device_del x
>> **
>> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
>> Aborted (core dumped)
>>
>> Hotplugging devices without a hotplug controller does not make much sense,
>> so we should disallow this during the device_add process already!
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> I'm queueing this on machine-next.  We still want it even if we
> apply the patch that changes TYPE_DEVICE to hotpluggable=false by
> default, right?

As far as I can see, yes. There might be machine types where e.g. the
PCI bus comes without a hotplug controller, so in that case we'd need this.

 Thomas

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Thomas Huth]

On 21.09.2017 19:56, Eduardo Habkost wrote:
> On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote:
>> qdev_unplug() bails out with an assertion if the user tries to device_del
>> a hot-plugged device that does not have a hotplug controller. Unfortunately,
>> our devices are all marked with hotpluggable = true by default (see the
>> device_class_init() function in qdev.c), so it currently can happen that
>> the user runs into this situation and QEMU gets terminated unexpectedly:
>>
>> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
>> QEMU 2.10.50 monitor - type 'help' for more information
>> (qemu) device_add aux-to-i2c-bridge,id=x
>> (qemu) device_del x
>> **
>> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
>> Aborted (core dumped)
>>
>> Hotplugging devices without a hotplug controller does not make much sense,
>> so we should disallow this during the device_add process already!
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> I'm queueing this on machine-next.  We still want it even if we
> apply the patch that changes TYPE_DEVICE to hotpluggable=false by
> default, right?

OK, just to be sure: Please de-queue it again. As you already mentioned
in another mail, there are situations where this does not work (e.g.
when one hot-pluggable device wants to instantiate another hot-pluggable
device internally).

But maybe we could add the test for the availability of a hot-plug
controller to qmp_device_add() instead? (I haven't had a closer look at
that yet, though).

 Thomas

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Eduardo Habkost]

On Tue, Sep 26, 2017 at 12:05:47PM +0200, Thomas Huth wrote:
> On 21.09.2017 19:56, Eduardo Habkost wrote:
> > On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote:
> >> qdev_unplug() bails out with an assertion if the user tries to device_del
> >> a hot-plugged device that does not have a hotplug controller. Unfortunately,
> >> our devices are all marked with hotpluggable = true by default (see the
> >> device_class_init() function in qdev.c), so it currently can happen that
> >> the user runs into this situation and QEMU gets terminated unexpectedly:
> >>
> >> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
> >> QEMU 2.10.50 monitor - type 'help' for more information
> >> (qemu) device_add aux-to-i2c-bridge,id=x
> >> (qemu) device_del x
> >> **
> >> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> >> Aborted (core dumped)
> >>
> >> Hotplugging devices without a hotplug controller does not make much sense,
> >> so we should disallow this during the device_add process already!
> >>
> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > I'm queueing this on machine-next.  We still want it even if we
> > apply the patch that changes TYPE_DEVICE to hotpluggable=false by
> > default, right?
> 
> OK, just to be sure: Please de-queue it again. As you already mentioned
> in another mail, there are situations where this does not work (e.g.
> when one hot-pluggable device wants to instantiate another hot-pluggable
> device internally).

I just dequeued it.

> 
> But maybe we could add the test for the availability of a hot-plug
> controller to qmp_device_add() instead? (I haven't had a closer look at
> that yet, though).

I'm unsure about this.  Now we know we have some devices that
work without a hotplug controller, so what's the reason
device_add must always require one?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Thomas Huth]

On 26.09.2017 15:41, Eduardo Habkost wrote:
> On Tue, Sep 26, 2017 at 12:05:47PM +0200, Thomas Huth wrote:
>> On 21.09.2017 19:56, Eduardo Habkost wrote:
>>> On Thu, Sep 07, 2017 at 11:22:42AM +0200, Thomas Huth wrote:
>>>> qdev_unplug() bails out with an assertion if the user tries to device_del
>>>> a hot-plugged device that does not have a hotplug controller. Unfortunately,
>>>> our devices are all marked with hotpluggable = true by default (see the
>>>> device_class_init() function in qdev.c), so it currently can happen that
>>>> the user runs into this situation and QEMU gets terminated unexpectedly:
>>>>
>>>> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
>>>> QEMU 2.10.50 monitor - type 'help' for more information
>>>> (qemu) device_add aux-to-i2c-bridge,id=x
>>>> (qemu) device_del x
>>>> **
>>>> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
>>>> Aborted (core dumped)
>>>>
>>>> Hotplugging devices without a hotplug controller does not make much sense,
>>>> so we should disallow this during the device_add process already!
>>>>
>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>> I'm queueing this on machine-next.  We still want it even if we
>>> apply the patch that changes TYPE_DEVICE to hotpluggable=false by
>>> default, right?
>>
>> OK, just to be sure: Please de-queue it again. As you already mentioned
>> in another mail, there are situations where this does not work (e.g.
>> when one hot-pluggable device wants to instantiate another hot-pluggable
>> device internally).
> 
> I just dequeued it.
> 
>>
>> But maybe we could add the test for the availability of a hot-plug
>> controller to qmp_device_add() instead? (I haven't had a closer look at
>> that yet, though).
> 
> I'm unsure about this.  Now we know we have some devices that
> work without a hotplug controller, so what's the reason
> device_add must always require one?

The problem is that you can also "device_del" these devices again. And
qdev_unplug() currently has this piece of code in it:

    /* hotpluggable device MUST have HotplugHandler, if it doesn't
     * then something is very wrong with it */
    g_assert(hotplug_ctrl);

So if you do a device_add followed by a device_del with one of these
devices, QEMU aborts and kills your guest.

One solution is maybe to remove the assert here. OTOH, considering that
we have the assert in qdev_monitor.c->qdev_unplug(), adding a check for
the availability of a hotplug_ctrl in qdev_monitor.c->qmp_device_add()
currently sounds like the better solution to this problem to me. Anybody
got another opinion here? If not, I'll have a try and send a patch...

 Thomas