Re: [Qemu-arm] [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov]

On Fri, 3 Jun 2016 16:26:22 -0300
Eduardo Habkost <address@hidden> wrote:

> On Fri, Jun 03, 2016 at 12:13:18PM +0200, Igor Mammedov wrote:
> > 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?)  
> > It doesn't look like it will work due to broken 4d1b279b0 as
> > plus_features/minus_features are applied after:
> > 
> >     if (cpu->host_features) {                                               
> >      
> >         for (w = 0; w < FEATURE_WORDS; w++) {                               
> >      
> >             env->features[w] =                                              
> >      
> >                 x86_cpu_get_supported_feature_word(w, cpu->migratable);     
> >      
> >         }                                                                   
> >      
> >     }
> > 
> > and with above moving to realize(), +-feats would be overwritten by it.
> > Lets temporary use static variables as in this patch so not to delay
> > series on not related fixes. And deal with it when 4d1b279b0 is fixed.
> > 
> > 1 way to deal with it is to wait several releases till users fix their
> > +-feats CLIs and then just drop it.  
> 
> We can fix that after getting rid the host_features hack (and
> also fix the "-cpu host,foo=off,foo=on" bug we already have).
> 
> In that case, I think we can live with the static variables
> temporarily in the meantime. Can you add a comment above the
> static variable declarations saying that they can't be replaced
> by globals yet because of the host_features hack?
Sure, thanks!