Re: [Qemu-devel] [PATCH] qdev: Make "hotplugged" property read-only

[Eduardo Habkost]

On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote:
> On Thu, 29 Dec 2016 20:38:15 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > The "hotplugged" property is user visible, but it was never meant
> > to be set by the user. There are probably multiple ways to break
> > or crash device code by overriding the property. One example:
> > 
> >   $ qemu-system-x86_64 -cpu qemu64,hotplugged=true
> >   Segmentation fault (core dumped)
> > 
> > The DeviceState::hotplugged struct field is set directly by
> > device_initfn(), there's no need to provide a setter for the
> > property.
> this property is meant to be used for individual devices on target side
> of migration.

I didn't know that. Is this documented somewhere? Is it actually
used by any existing software?

> Doing above is a rather big hammer with behavioral change of migrated
> instance.

If the property is really supposed to be set directly by users,
then I agree. But I would like to understand how exactly it is
supposed to work, so we can fix those issues properly.

Do you have any existing example where setting "hotplugged=true"
on the command-line is necessary and where it really works?


> 
> So I'd  fix crash caused by assumption that hotplugged CPU
> guarantied to have rtc&fw_cfg available.
> I'll post a patch with the fix.

Most of the cases where I see DeviceState::hotplugged being used
are related to generation of hotplug events[1] or completing
steps that are normally done by machine init code[2][3], and I am
not sure this should be the case when we're just migrating
hotplugged devices. Are all those cases broken, and they should
be checking the qdev_hotplug global variable instead?

Examples:

[1] acpi_cpu_plug_cb(), acpi_pcihp_device_plug_cb(),
    nvdimm_acpi_plug_cb();
[2] cpu_common_realizefn() calls cpu_synchronize_post_init() and
    cpu_resume() if dev->hotplug is set.
[3] pc_cpu_plug()

> 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> >  hw/core/qdev.c | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 57834423b9..f5989c41cb 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -1013,13 +1013,6 @@ static bool device_get_hotplugged(Object *obj, Error 
> > **err)
> >      return dev->hotplugged;
> >  }
> >  
> > -static void device_set_hotplugged(Object *obj, bool value, Error **err)
> > -{
> > -    DeviceState *dev = DEVICE(obj);
> > -
> > -    dev->hotplugged = value;
> > -}
> > -
> >  static void device_initfn(Object *obj)
> >  {
> >      DeviceState *dev = DEVICE(obj);
> > @@ -1039,7 +1032,7 @@ static void device_initfn(Object *obj)
> >      object_property_add_bool(obj, "hotpluggable",
> >                               device_get_hotpluggable, NULL, NULL);
> >      object_property_add_bool(obj, "hotplugged",
> > -                             device_get_hotplugged, device_set_hotplugged,
> > +                             device_get_hotplugged, NULL,
> >                               &error_abort);
> >  
> >      class = object_get_class(OBJECT(dev));
> 

-- 
Eduardo