qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH] qdev: Make "hotplugged" property read-only
Date: Tue, 3 Jan 2017 12:22:23 -0200
User-agent: Mutt/1.7.1 (2016-10-04)

On Tue, Jan 03, 2017 at 02:02:52PM +0100, Igor Mammedov wrote:
> On Fri, 30 Dec 2016 16:23:08 -0200
> Eduardo Habkost <address@hidden> 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 <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?
> 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 <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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]