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

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

[Eduardo Habkost]

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.

Signed-off-by: Eduardo Habkost <ehabkost@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));
-- 
2.11.0.259.g40922b1

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

[Igor Mammedov]

On Thu, 29 Dec 2016 20:38:15 -0200
Eduardo Habkost <ehabkost@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.
Doing above is a rather big hammer with behavioral change of migrated
instance.

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.

> Signed-off-by: Eduardo Habkost <ehabkost@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));

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 <ehabkost@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 <ehabkost@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

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

[Igor Mammedov]

On Fri, 30 Dec 2016 16:23:08 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote:
> > On Thu, 29 Dec 2016 20:38:15 -0200
> > Eduardo Habkost <ehabkost@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?
not that I know of. But users should be fixed if they are not using it.

> > 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?
skipping hotplugged for x86 CPUs and pc-dimms on target side shouldn't
affect functionality as current ACPI code doesn't depends on it
and it's safe to drop hotplug events for already plugged devices
as machine init/done code would do the rest of initialization
in this case.

However if hotplugged property is not set on target then
mgmt would loose information that device has been hotplugged
when it would query qemu. For example:
info memory-devices 
Memory device [dimm]: ""
  addr: 0x100000000
  slot: 1
  node: 0
  size: 1073741824
  memdev: /objects/mem2
  hotplugged: false
  hotpluggable: true  <=== would become false


Also beside of simplistic use of hotplugged by x86 cpus/pc-dimm,
'git grep hotplugged' shows that it's used by
pci, spapr and other devices and I'm not sure that
changing hotplugged from true to false is safe thing to do.


> 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 <ehabkost@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));  
> >   
> 

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

[Eduardo Habkost]

On Tue, Jan 03, 2017 at 02:02:52PM +0100, Igor Mammedov wrote:
> On Fri, 30 Dec 2016 16:23:08 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote:
> > > On Thu, 29 Dec 2016 20:38:15 -0200
> > > Eduardo Habkost <ehabkost@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?
> not that I know of. But users should be fixed if they are not using it.

I see. The problem is that the mechanism is undocumented,
untested, and seems very likely to trigger bugs in device code.

> 
> > > 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?
> skipping hotplugged for x86 CPUs and pc-dimms on target side shouldn't
> affect functionality as current ACPI code doesn't depends on it
> and it's safe to drop hotplug events for already plugged devices
> as machine init/done code would do the rest of initialization
> in this case.
> 
> However if hotplugged property is not set on target then
> mgmt would loose information that device has been hotplugged
> when it would query qemu. For example:
> info memory-devices 
> Memory device [dimm]: ""
>   addr: 0x100000000
>   slot: 1
>   node: 0
>   size: 1073741824
>   memdev: /objects/mem2
>   hotplugged: false
>   hotpluggable: true  <=== would become false
> 
> 
> Also beside of simplistic use of hotplugged by x86 cpus/pc-dimm,
> 'git grep hotplugged' shows that it's used by
> pci, spapr and other devices and I'm not sure that
> changing hotplugged from true to false is safe thing to do.

Yes. But for the same reason I am not sure that setting
hotplugged=on on command-line devices is a safe thing to do.

So, we have two problems we want to avoid:
A) Not changing hotplugged from true to false on migration;
B) Not breaking devices when using
   "-device ...,hotplugged=true" on the command-line.

Problem (A) already exists, as far as I can see, but we never got
any bug reports related to it.

The fix for (A) you propose is to set "hotplugged=true" on the
command-line. This triggered (B) on the CPU code, but your patch
fixed it.

The fix I proposed for (B) (this patch) prevents your fix for (A)
from being implemented. This is a problem.

Your patch to fix (B) in the CPU code looks good, but I am
worried about other possible instances of (B).

So, I propose we do this:

1) Drop this patch, by now;
2) Document properly what management software is supposed
   to do to fix (A), and test if the proposed solution doesn't
   break anything (I think we will find other device code to
   crash or misbehave if using "-device ...,hotplugged=true");
3) If we don't do (2) in the next QEMU release, I will resubmit
   this patch to remove the unused/undocumented/untested feature.

> 
> 
> > 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 <ehabkost@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

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

[Igor Mammedov]

On Tue, 3 Jan 2017 12:22:23 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jan 03, 2017 at 02:02:52PM +0100, Igor Mammedov wrote:
> > On Fri, 30 Dec 2016 16:23:08 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote:  
> > > > On Thu, 29 Dec 2016 20:38:15 -0200
> > > > Eduardo Habkost <ehabkost@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?  
> > not that I know of. But users should be fixed if they are not using it.  
> 
> I see. The problem is that the mechanism is undocumented,
> untested, and seems very likely to trigger bugs in device code.
> 
> >   
> > > > 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?  
> > skipping hotplugged for x86 CPUs and pc-dimms on target side shouldn't
> > affect functionality as current ACPI code doesn't depends on it
> > and it's safe to drop hotplug events for already plugged devices
> > as machine init/done code would do the rest of initialization
> > in this case.
> > 
> > However if hotplugged property is not set on target then
> > mgmt would loose information that device has been hotplugged
> > when it would query qemu. For example:
> > info memory-devices 
> > Memory device [dimm]: ""
> >   addr: 0x100000000
> >   slot: 1
> >   node: 0
> >   size: 1073741824
> >   memdev: /objects/mem2
> >   hotplugged: false
> >   hotpluggable: true  <=== would become false
> > 
> > 
> > Also beside of simplistic use of hotplugged by x86 cpus/pc-dimm,
> > 'git grep hotplugged' shows that it's used by
> > pci, spapr and other devices and I'm not sure that
> > changing hotplugged from true to false is safe thing to do.  
> 
> Yes. But for the same reason I am not sure that setting
> hotplugged=on on command-line devices is a safe thing to do.
> 
> So, we have two problems we want to avoid:
> A) Not changing hotplugged from true to false on migration;
> B) Not breaking devices when using
>    "-device ...,hotplugged=true" on the command-line.
> 
> Problem (A) already exists, as far as I can see, but we never got
> any bug reports related to it.
> 
> The fix for (A) you propose is to set "hotplugged=true" on the
> command-line. This triggered (B) on the CPU code, but your patch
> fixed it.
> 
> The fix I proposed for (B) (this patch) prevents your fix for (A)
> from being implemented. This is a problem.
> 
> Your patch to fix (B) in the CPU code looks good, but I am
> worried about other possible instances of (B).
> 
> So, I propose we do this:
> 
> 1) Drop this patch, by now;
agreed

> 2) Document properly what management software is supposed
>    to do to fix (A), and test if the proposed solution doesn't
Do you have in mind where to document it?
I'll add to loop libvirt folks.

>    break anything (I think we will find other device code to
>    crash or misbehave if using "-device ...,hotplugged=true");
As for tests, I'm not sure I'll be able to cover all usecases,
but I can take care of x86 cpus and pc/nv-dimms.

-device path  is unlikely to trigger bugs as it's executed rather late
when machine init code is completed and basically tested by real device
hotplug path.
Your example of x86 cpu crash is unexpected usage of
  -global foo.hotplugged=on
which is applied early in machine init time to all devices of given type
which looks wrong to me.
I'd propose to prevent using -global for essentially per-instance property.

> 3) If we don't do (2) in the next QEMU release, I will resubmit
>    this patch to remove the unused/undocumented/untested feature.
Applying this patch looks wrong to me regardless of (2) for already
mentioned reasons.
We should at least document property usage and ask libvirt to use it
if it's not using it yet.

> 
> > 
> >   
> > > 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 <ehabkost@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));    
> > > >     
> > >   
> >   
> 

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

[Paolo Bonzini]

On 03/01/2017 15:22, Eduardo Habkost wrote:
>> I didn't know that. Is this documented somewhere?
>> Is it actually used by any existing software?
> not that I know of. But users should be fixed if they are not using it.
> 
> I see. The problem is that the mechanism is undocumented,
> untested, and seems very likely to trigger bugs in device code.

I agree.  Why can't hotplugged be migrated?

Paolo

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

[Igor Mammedov]

On Tue, 3 Jan 2017 17:10:15 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 03/01/2017 15:22, Eduardo Habkost wrote:
> >> I didn't know that. Is this documented somewhere?
> >> Is it actually used by any existing software?
> > not that I know of. But users should be fixed if they are not using it.
> > 
> > I see. The problem is that the mechanism is undocumented,
> > untested, and seems very likely to trigger bugs in device code.
> 
> I agree.  Why can't hotplugged be migrated?
It's probably not migrated because of it's not runtime/guest modified
state so we don't have to migrate it as it's know in advance.

For now it should set manually on CLI (-device) with the rest of
hotplugged device properties.

> Paolo

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

[Eduardo Habkost]

(CCing libvir-list and Laine)

On Tue, Jan 03, 2017 at 05:53:27PM +0100, Igor Mammedov wrote:
> On Tue, 3 Jan 2017 17:10:15 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > 
> > 
> > On 03/01/2017 15:22, Eduardo Habkost wrote:
> > >> I didn't know that. Is this documented somewhere?
> > >> Is it actually used by any existing software?
> > > not that I know of. But users should be fixed if they are not using it.
> > > 
> > > I see. The problem is that the mechanism is undocumented,
> > > untested, and seems very likely to trigger bugs in device code.
> > 
> > I agree.  Why can't hotplugged be migrated?
> It's probably not migrated because of it's not runtime/guest modified
> state so we don't have to migrate it as it's know in advance.
> 
> For now it should set manually on CLI (-device) with the rest of
> hotplugged device properties.

As this recommendation has the potential to trigger hidden bugs
(and known to trigger a bug in QEMU <= 2.8), I would like it to
be properly documented, and the documentation/recommendations
reviewed following the usual patch review process.

While we don't do that, setting "hotplugged=true" on the
command-line is an unused, undocumented, untested (and
unsupported?) feature.

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Jan 03, 2017 at 03:06:02PM -0200, Eduardo Habkost wrote:
> (CCing libvir-list and Laine)
> 
> On Tue, Jan 03, 2017 at 05:53:27PM +0100, Igor Mammedov wrote:
> > On Tue, 3 Jan 2017 17:10:15 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On 03/01/2017 15:22, Eduardo Habkost wrote:
> > > >> I didn't know that. Is this documented somewhere?
> > > >> Is it actually used by any existing software?
> > > > not that I know of. But users should be fixed if they are not using it.
> > > > 
> > > > I see. The problem is that the mechanism is undocumented,
> > > > untested, and seems very likely to trigger bugs in device code.
> > > 
> > > I agree.  Why can't hotplugged be migrated?
> > It's probably not migrated because of it's not runtime/guest modified
> > state so we don't have to migrate it as it's know in advance.
> > 
> > For now it should set manually on CLI (-device) with the rest of
> > hotplugged device properties.
> 
> As this recommendation has the potential to trigger hidden bugs
> (and known to trigger a bug in QEMU <= 2.8), I would like it to
> be properly documented, and the documentation/recommendations
> reviewed following the usual patch review process.
> 
> While we don't do that, setting "hotplugged=true" on the
> command-line is an unused, undocumented, untested (and
> unsupported?) feature.

As the ability to set hotplugged=true is still unused and
undocumented, I will resubmit this patch to disable the feature
in QEMU 2.9.

-- 
Eduardo