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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 23 Aug 2017 11:24:27 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Aug 21, 2017 at 10:32:41AM +0200, Igor Mammedov wrote:
> > On Fri, 18 Aug 2017 14:40:29 -0300
> > Eduardo Habkost <ehabkost@redhat.com> 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 <imammedo@redhat.com>    
> > > [...]  
> > > > +/* 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.
Removing means replacing warning with hard error, so that setups
that happen to use this combination would fail instead of silently
changing behavior.
So it won't actually simplify function but will cause side-effects
that weren't intended by this series.

As is in this series, I'm being rather conservative,
it's just replacing code duplication that exists in SPARC with x86 impl.
without behavioral change
(and even though it's in public header it's not really public API
that's why it's called legacy_foo() - to try preventing new usage).

If we decide to make it hard error, it would be better to do it separately.
I can post a patch on top if you insist, that would do it
so we could discuss there if it's ok to break users in 2 releases
or not but it should not hold this series as it's totally
orthogonal matter. (it also would be easier to revert patch
if we apply it but later decide to keep old ways).