Re: [Qemu-devel] [PATCH 03/10] target-i386: move hyperv_* static globals to CPUState

[Igor Mammedov]

On Tue, 26 Feb 2013 14:21:22 -0300
Eduardo Habkost <address@hidden> wrote:

> On Tue, Feb 26, 2013 at 06:10:25PM +0100, Igor Mammedov wrote:
> > On Tue, 26 Feb 2013 14:06:05 -0300
> > Eduardo Habkost <address@hidden> wrote:
> > 
> > > On Tue, Feb 26, 2013 at 05:39:02PM +0100, Igor Mammedov wrote:
> > > [...]
> > > > > > > > > * I don't expect hv-* to appear on a machine-type compat_props
> > > > > > > > > array in the near feature.
> > > > > > > > > * I don't expect people to need to set per-CPU hv-* properties
> > > > > > > > > on device_add for CPU hotplug.
> > > > > > > > > 
> > > > > > > > > So we could keep them as special cases on parse_featurestr(),
> > > > > > > > > and convert them to per-CPU properties only after we have the
> > > > > > > > > subclasses and CPU hotplug working.
> > > > > > > > it won't be a consistent interface, where user who has 
> > > > > > > > "-cpu XXX,+foo1,+hv_spinlocks,+foo2" on cmd-line
> > > > > > > > would have to use "device_add XXX,foo1=true,foo2=true" manually
> > > > > > > > excluding options from device_add, i.e. it propagates special
> > > > > > > > casing to users as well. And when hv_ are moved to per-CPU
> > > > > > > > fields, it might break users since they will still exclude some
> > > > > > > > options.
> > > > > > > 
> > > > > > > Won't -cpu/parse_featurestr() simply set global properties? In 
> > > > > > > this
> > > > > > > case, the common case would be to call "device_add XXX" with no
> > > > > > > extra options at all, so there's no option to be excluded and no
> > > > > > > special case to care about.
> > > > > > That is if global properties will made to 1.5  which I highly doubt
> > > > > > taking in account how fast patches are reviewed and accepted.
> > > > > > Otherwise release would be broken. 
> > > > > 
> > > > > IMO it _has_ to make 1.5 and is a requirement to make device_add 
> > > > > usable
> > > > > for CPU hotplug. Otherwise we would have to change the behavior of 
> > > > > -cpu
> > > > > + device_add in an incompatible way.
> > > > if all -cpu features are converted to static properties, we do not have 
> > > > to
> > > > have global properties working. In absence of 'global properties', user
> > > > will have to use the same properties at device_add that was specified at
> > > > -cpu. And introduction of global properties won't regress it, it will
> > > > just allow to use device_add without features specified on -cpu
> > > 
> > > Strictly, we do not have to, but changing -cpu to set global properties
> > > only later would change the behavior of "-cpu XXX,foo=1,bar=2" followed
> > > by "device_add XXX" in an incompatible way. So if our long-term plan is
> > Could you explain how ^^^ it will be incompatible, pls?
> 
> Suppose that "foo" defaults to 0, and we run: "-cpu XXX,foo=1", followed
> by "device_add XXX".
> 
> Without globals/defaults set by -cpu, the above will create a new CPU
> with foo=0.
> 
> With globals/defaults set by -cpu, the above will create a new CPU with
> foo=1.
> 
> If I recall correctly, we agreed that the latter is the behavior we
> wanted (because it is simpler for users, matches the fact that "-cpu"
> already affects multiple CPU devices [it already affects all the CPUs
> created on startup], and is the most common use-case [creating CPUs that
> look basically the same]).
Yes, that is the goal. I wouldn't say incompatible if user will start
QEMU with "-cpu XXX,foo=1" and then use "device_add XXX,foo=1". That's a
strict minimum that would work for hot-plug. Plain "device_add XXX" is an
invalid in this case since it won't produce the same CPU.
So later on top of "-cpu XXX,foo=1" + "device_add XXX,foo=1" we add up
conversion of -cpu to global properties it shouldn't break anything, only add
new option to create the same CPU usin "device_add XXX", users will still be
able to use "device_add XXX,foo=1" if desired.

I hope that -cpu => global properties will make it in 1.5, but it is not
must have show-stopper for hot-plug if it misses it.

> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor