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 <address@hidden> wrote:

> On 11.09.2017 14:53, Igor Mammedov wrote:
> > On Thu,  7 Sep 2017 11:22:42 +0200
> > Thomas Huth <address@hidden> 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 <address@hidden>
> >> Signed-off-by: Thomas Huth <address@hidden>
> >> ---
> >>  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