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

[Igor Mammedov]

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

> On Tue, Feb 26, 2013 at 05:12:21PM +0100, Igor Mammedov wrote:
> > On Tue, 26 Feb 2013 13:01:12 -0300
> > Eduardo Habkost <address@hidden> wrote:
> > 
> > > On Tue, Feb 26, 2013 at 04:42:17PM +0100, Igor Mammedov wrote:
> > > > On Tue, 26 Feb 2013 11:50:23 -0300
> > > > Eduardo Habkost <address@hidden> wrote:
> > > > 
> > > > > On Mon, Feb 25, 2013 at 02:03:02AM +0100, Igor Mammedov wrote:
> > > > > > - since hyperv_* helper functions are used only in
> > > > > > target-i386/kvm.c move them there as static helpers
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > > Requestd-By: Eduardo Habkost <address@hidden>
> > > > > 
> > > > > I wonder if we could do this later, to make review and testing
> > > > > easier. We could simply keep the hv_* handling inside
> > > > > parse_featurestr() by now. I don't think this would block any of
> > > > > the CPU hotplug or CPU model probing/compatibility/reliability work
> > > > > we're doing. I mean:
> > > > device_add from CPU hot-add POV would be usable once we have all
> > > > features accepted on -cpu converted to properties + sub-classes.
> > > 
> > > If we keep hv_* as special cases inside parse_featurestr() (the way they
> > > already are), CPU hotplug should still work perfectly. I don't see why
> > > it would block it.
> > > 
> > > 
> > > > 
> > > > > 
> > > > > * 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

> 
> 
> > 
> > > 
> > > 
> > > > > 
> > > > > Considering that -cpu options will be translated to global
> > > > > properties, it will be trivial to keep compatibility with existing
> > > > > behavior of "-cpu hv_*=..." once we change them from static
> > > > > variables to per-CPU fields.
> > > > Global properties would just allow not to specify foo1,foo2 on
> > > > device_add from hot-plug POV.
> > > > 
> > > > If this and following patch are to complex we could fallback to
> > > > alternative from v6 for hv_* features, which produce the same external
> > > > property interface just with different internal approach.
> > > 
> > > No, v6 exposes completely different (and unexpected) semantics. It
> > > exposes a per-CPU property that affects all CPU objects when it gets
> > > changed.
> > Do you know any guest that will work with mixed CPUs that have and doesn't
> > have hv_* set at the same time?
> 
> I don't know and I don't care. But in either case you are introducing a
> per-CPU QOM property for it. If it is a per-CPU QOM property, it just
> makes sense that it will affect only the CPU object being changed.
> 
> What I am proposing is to make it a per-CPU QOM property only after we
> make it really per-CPU, so we don't introduce weird externally-visible
> property semantics that are likely to change in the near future.
Well this patch and next do it per-CPU as you asked, so no weird property
semantics is introduced and no special-casing of anything is required.

> 
> 
> > 
> > > > 
> > > > > 
> > > > > > ---
> > > > > >  target-i386/Makefile.objs |    2 +-
> > > > > >  target-i386/cpu.c         |   16 +++++++---
> > > > > >  target-i386/cpu.h         |    7 +++++
> > > > > >  target-i386/hyperv.c      |   64
> > > > > > --------------------------------------------- target-i386/hyperv.h
> > > > > > |   45 ------------------------------- target-i386/kvm.c         |
> > > > > > 36 ++++++++++++++++++------- 6 files changed, 45 insertions(+),
> > > > > > 125 deletions(-) delete mode 100644 target-i386/hyperv.c
> > > > > >  delete mode 100644 target-i386/hyperv.h
> > > > > > 
> > > > > [...]
> > > > > 
> > > > 
> > > > 
> > > 
> > 
>