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

[Eduardo Habkost]

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.


> > 
> > 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.

> 
> > 
> > > ---
> > >  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
> > > 
> > [...]
> > 
> 
> 

-- 
Eduardo