On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote:
> On Thu, 29 Dec 2016 20:38:15 -0200
> Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@redhat.com>
> > ---
> > 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
