qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH RFC 1/8] target-i386: cpu: move features logic tha


From: Igor Mammedov
Subject: Re: [Qemu-arm] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time
Date: Thu, 2 Jun 2016 20:23:37 +0200

On Thu, 2 Jun 2016 14:34:27 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden> 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.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]