From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9EhD-00033d-TK for qemu-devel@nongnu.org; Sat, 04 Jun 2016 12:45:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b9EhB-0005x8-NF for qemu-devel@nongnu.org; Sat, 04 Jun 2016 12:45:10 -0400 Date: Sat, 4 Jun 2016 18:44:59 +0200 From: Igor Mammedov Message-ID: <20160604184459.4749f961@nial.brq.redhat.com> In-Reply-To: <20160603192622.GX19055@thinpad.lan.raisama.net> References: <1464799050-11002-1-git-send-email-imammedo@redhat.com> <1464799050-11002-2-git-send-email-imammedo@redhat.com> <20160601174309.GA13503@thinpad.lan.raisama.net> <20160602115930.6945e5d6@igors-macbook-pro.local> <20160602143826.GD19055@thinpad.lan.raisama.net> <20160602185655.20d7c7e0@nial.brq.redhat.com> <20160602173427.GI19055@thinpad.lan.raisama.net> <20160603121318.7e4b625d@nial.brq.redhat.com> <20160603192622.GX19055@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: peter.maydell@linaro.org, mark.cave-ayland@ilande.co.uk, qemu-devel@nongnu.org, blauwirbel@gmail.com, qemu-arm@nongnu.org, pbonzini@redhat.com, rth@twiddle.net On Fri, 3 Jun 2016 16:26:22 -0300 Eduardo Habkost wrote: > On Fri, Jun 03, 2016 at 12:13:18PM +0200, Igor Mammedov wrote: > > On Thu, 2 Jun 2016 14:34:27 -0300 > > Eduardo Habkost 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 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 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? Sure, thanks!