Re: [Qemu-devel] [PATCH 1/2] target-i386: cpu: convert plus/minus properties to global properties

[Eduardo Habkost]

On Mon, Aug 21, 2017 at 10:32:41AM +0200, Igor Mammedov wrote:
> On Fri, 18 Aug 2017 14:40:29 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Thu, Aug 17, 2017 at 04:07:56PM +0200, Igor Mammedov wrote:
> > > Since
> > >  (commit d4a606b3 i386: Don't override -cpu options on -cpu host/max)
> > > it became possible to delete hack where it was necessary to
> > > postpone applying plus/minus features to realize time
> > > after max_features were applied to keep legacy +-feat
> > > override semantics.
> > > 
> > > With above commit it's possible to convert +-feat to a set
> > > of GlobalProperty items and keep +-feat override semantics,
> > > these properties should be added to global list at the end
> > > to override properties that were set with feat=on|off syntax.
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>  
> > [...]
> > > +/* Parse "+feature,-feature,feature=foo" CPU feature string */
> > >  static void x86_cpu_parse_featurestr(const char *typename, char 
> > > *features,
> > >                                       Error **errp)
> > >  {
> > > +    /* Compatibily hack to maintain legacy +-feat semantic,
> > > +     * where +-feat overwrites any feature set by
> > > +     * feat=on|feat even if the later is parsed after +-feat
> > > +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> > > +     */
> > > +    GList *l, *plus_features = NULL, *minus_features = NULL;  
> > 
> > The warning about ambiguous CPU options exists since 2.8, I think
> > this is a good opportunity to get rid of the "[+-]feat overrides
> > feat=on|off" rule and simplify the parsing code.  Do you want to
> > do this in the same patch?
> I'd prefer not to do it in this patch/series, as it's not related.
> 
> WE can do cleanups later on top.

This is not a problem in this patch, but it is a problem when you
create a generic cpu_legacy_parse_featurestr() function in
another series.  We should remove that useless feature from the
function before making it generic.  It has the additional benefit
of making the resulting patch and code easier to review.

-- 
Eduardo