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

[Eduardo Habkost]

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?

-- 
Eduardo