On Thu, 2 Jun 2016 14:34:27 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote:
> > On Thu, 2 Jun 2016 11:38:26 -0300
> > Eduardo Habkost <ehabk...@redhat.com> wrote:
> >
> > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote:
> > > > On Wed, 1 Jun 2016 14:43:09 -0300
> > > > Eduardo Habkost <ehabk...@redhat.com> wrote:
> > > > [...]
> > > >
> > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > index 3fbc6f3..6159a7f 100644
> > > > > > --- a/target-i386/cpu.c
> > > > > > +++ b/target-i386/cpu.c
> > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > +/* Features to be added */
> > > > >
> > > > > Please add something like "Features to be added. Will be
> > > > > replaced by global variables in the future".
> > > > >
> > > > > > +static FeatureWordArray plus_features = { 0 };
> > > > > > +/* Features to be removed */
> > > > > > +static FeatureWordArray minus_features = { 0 };
> > > > > > +
> > > > >
> > > > > I see that this hack is replaced by the following patches,
> > > > > but is there an easy way to remove the CPUState argument from
> > > > > x86_cpu_parse_featurestr() before we introduce these static
> > > > > variables? (No problem if there's no way to do that, as long
> > > > > as the static variables are explicitly documented as a
> > > > > temporary hack)
> > > > It's hack to keep legacy +- semantic (i.e. it overrides
> > > > feat1=x,feat2) local to x86 that probably would stay here
> > > > forever. I should add comment that explains why +- can't be
> > > > replaced with normal properties.
> > >
> > > Oh, I assumed it would be temporary. In that case, I would like
> > > to avoid adding the static variables if possible.
> > >
> > > >
> > > > I don't plan to replace plus/minus_features with anything nor to
> > > > make this variables a global ones to spread +- x86/sparc legacy
> > > > format everywhere.
> > >
> > > Can't the +/- semantics be emulated by simply registering
> > > plus_features/minus_features after the other global properties
> > > are registered inside x86_cpu_parse_featurestr()?
> > it could be done, at the first glance it will take 2 extra parsing
> > passes
> >
> > 1: copy featurestr, parse feat=x,feat
> > 2: copy featurestr, parse +feat
> > 3: copy featurestr, parse -feat
>
> Why? Can't we just replace plus_features and minus_features with
> two string lists (or a QDict), and make the corresponding
> object_property_parse()/qdev_prop_register_global() calls after
> the main parsing loop?
>
> (Didn't you do that in your old "target-i386: set [+-]feature
> using static properties" patch?)
ok, I'll do that.
>
> >
> > but that probably will complicate way to disable +-feat handling in
> > future, with current static vars it's just a matter of specifying
> > compat-prop for X86CPU driver in appropriate machine type.
>
> I see. But I don't see why we need the extra machine-type cruft.
>
> To me, the whole point of removing the old syntax is to make the
> code simpler. If we have to add even more code/complexity just to
> add a machine-type restriction (but keeping the old code there),
> I don't see the point.
>
> I believe we should either: 1) remove it completely after people
> have time to update their scripts/libvirt; or 2) just keep it
> working on all machine-types, but print a warning every time
> people use the old syntax.
>
> (I am not sure if we should do (1) without giving users a long
> time to adapt, so I suggest we do (2) by now)
>
>
> > So I'd leave it as is unless you insist on doing it like you
> > suggested above.
> > > >
> > > > What I would do though before enabling -device/device_add for
> > > > X86CPU is to disable +- handling for new machine types so that
> > > > CPUs would follow generic property semantic of device used
> > > > everywhere else.
> > >
> > > We can't do that unless we give libvirt (and users that have
> > > their own scripts) time to adapt to the new syntax
> > leaving it enabled will lead to mixed semantics in combination
> > with device_add that will be even more confusing to users
> > if users will use both:
> > for example: -cpu cpu,-featx and -device cpu,featx=on
> > That's why I'm suggesting to make a clean break in new machine
> > type with error saying to replace legacy +-feat with canonical one.
> > For old machine types nothing would break as it would still use
> > legacy syntax and legacy cpu-add, with device_add disabled.
>
> Just warn users very clearly, if they use the old syntax. If they
> insist in using it, they shouldn't blame us if they get confused.
>
> >
> > We probably can fix libvirt in sync with this QEMU release
> > if it still uses +- syntax.
>
> I would wait for at least 1 or 2 libvirt releases before removing
> support.
>
> But as I said above: if we are not deleting any code (and are
> adding extra code instead), I don't see the point of forcibly
> disabling it. We can just leave it there and print a warning.
ok, lets go with warning, saying"
"[+-]feature syntax is obsoleted and will be removed in future,
it's recommended to use canonical property syntax 'feature=value'"
>
> >
> > > (and we warn users that newer QEMU versions will require newer
> > > libvirt).
> > yep we should do it in release notes.
