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

[Eduardo Habkost]

On Wed, Aug 23, 2017 at 05:54:12PM +0200, Igor Mammedov wrote:
> On Wed, 23 Aug 2017 11:24:27 -0300
> Eduardo Habkost <address@hidden> 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 <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.
> 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.

I don't think it should be a hard error, it will be just a
semantics change (that we're already warning about for 3
releases).  But I understand that you don't intend to introduce
this behavior change in x86 now.

However:

> 
> 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

You are not introducing behavior change on x86, that's right.
But you are introducing new behavior on sparc, and the new
behavior includes the ordering misfeature we have in x86.  Let's
keep the misfeature x86-only.


> (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).

I agree to implement x86 behavior change separately, but I insist
we don't introduce the ordering misfeature in sparc too.  Fixing
x86 shouldn't hold the series, I agree.  But making sparc parser
as bad as x86 is a blocker to me.

-- 
Eduardo