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?)
>
> 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.
>
> > (and we warn users that newer QEMU versions will require newer libvirt).
> yep we should do it in release notes.
--
Eduardo
