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

[Igor Mammedov]

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.