From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35820) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8PPq-0003xT-6q for qemu-devel@nongnu.org; Thu, 02 Jun 2016 05:59:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8PPo-0000Lu-4C for qemu-devel@nongnu.org; Thu, 02 Jun 2016 05:59:49 -0400 Date: Thu, 2 Jun 2016 11:59:30 +0200 From: Igor Mammedov Message-ID: <20160602115930.6945e5d6@igors-macbook-pro.local> In-Reply-To: <20160601174309.GA13503@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> 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: qemu-devel@nongnu.org, peter.maydell@linaro.org, pbonzini@redhat.com, rth@twiddle.net, blauwirbel@gmail.com, mark.cave-ayland@ilande.co.uk, qemu-arm@nongnu.org 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. 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. What I would do though before enabling -device/device_add for X86CPU is to disable +- handling for new machine types so that CPUs would follow generic property semantic of device used everywhere else. > > > /* Parse "+feature,-feature,feature=foo" CPU feature string > > */ > > static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > @@ -1939,13 +1944,7 @@ static void > > x86_cpu_parse_featurestr(CPUState *cs, char *features, { > > X86CPU *cpu = X86_CPU(cs); > > char *featurestr; /* Single 'key=value" string being parsed */ > > - FeatureWord w; > > - /* Features to be added */ > > - FeatureWordArray plus_features = { 0 }; > > - /* Features to be removed */ > > - FeatureWordArray minus_features = { 0 }; > > uint32_t numvalue; > > - CPUX86State *env = &cpu->env; > > Error *local_err = NULL; > > > > featurestr = features ? strtok(features, ",") : NULL; > > @@ -2019,18 +2018,6 @@ static void > > x86_cpu_parse_featurestr(CPUState *cs, char *features, } > > featurestr = strtok(NULL, ","); > > } > > - > > - if (cpu->host_features) { > > - for (w = 0; w < FEATURE_WORDS; w++) { > > - env->features[w] = > > - x86_cpu_get_supported_feature_word(w, > > cpu->migratable); > > - } > > - } > > - > > - for (w = 0; w < FEATURE_WORDS; w++) { > > - env->features[w] |= plus_features[w]; > > - env->features[w] &= ~minus_features[w]; > > - } > > } > > > > /* Print all cpuid feature names in featureset > > @@ -2912,12 +2899,25 @@ static void x86_cpu_realizefn(DeviceState > > *dev, Error **errp) CPUX86State *env = &cpu->env; > > Error *local_err = NULL; > > static bool ht_warned; > > + FeatureWord w; > > > > if (cpu->apic_id < 0) { > > error_setg(errp, "apic-id property was not initialized > > properly"); return; > > } > > > > + if (cpu->host_features) { > > + for (w = 0; w < FEATURE_WORDS; w++) { > > + env->features[w] = > > + x86_cpu_get_supported_feature_word(w, > > cpu->migratable); > > + } > > + } > > + > > + for (w = 0; w < FEATURE_WORDS; w++) { > > + cpu->env.features[w] |= plus_features[w]; > > + cpu->env.features[w] &= ~minus_features[w]; > > + } > > + > > if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) { > > env->cpuid_level = 7; > > } > > -- > > 1.8.3.1 > > >