From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48801) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bG7vV-0005tH-Bk for qemu-devel@nongnu.org; Thu, 23 Jun 2016 12:56:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bG7vR-0007WY-AU for qemu-devel@nongnu.org; Thu, 23 Jun 2016 12:56:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60566) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bG7vR-0007WI-1B for qemu-devel@nongnu.org; Thu, 23 Jun 2016 12:56:21 -0400 Date: Thu, 23 Jun 2016 18:56:12 +0200 From: Igor Mammedov Message-ID: <20160623185612.19229ab0@igors-macbook-pro.local> In-Reply-To: <20160623160453.GC2048@thinpad.lan.raisama.net> References: <1466453564-7572-1-git-send-email-ehabkost@redhat.com> <1466453564-7572-3-git-send-email-ehabkost@redhat.com> <20160623165928.74a5f46f@igors-macbook-pro.local> <20160623160453.GC2048@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] target-i386: Introduce x86_cpu_load_host_data() function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: libvir-list@redhat.com, dahi@linux.vnet.ibm.com, Jiri Denemark , qemu-devel@nongnu.org On Thu, 23 Jun 2016 13:04:53 -0300 Eduardo Habkost wrote: > On Thu, Jun 23, 2016 at 04:59:28PM +0200, Igor Mammedov wrote: > > On Mon, 20 Jun 2016 17:12:43 -0300 > > Eduardo Habkost wrote: > > > > > The code that loads host-specific information inside > > > x86_cpu_realizefn() will be reused by the implementation of > > > query-host-cpu, so move it to a separate function. > > > > > > Signed-off-by: Eduardo Habkost > > > --- > > > target-i386/cpu.c | 23 ++++++++++++++++------- > > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index aadd0b9..3d3635d 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -1491,6 +1491,20 @@ void x86_cpu_change_kvm_default(const char > > > *prop, const char *value) static uint32_t > > > x86_cpu_get_supported_feature_word(FeatureWord w, bool > > > migratable_only); > > > +/* Load host-dependent CPU information, when applicable */ > > > +static void x86_cpu_load_host_data(X86CPU *cpu) > > > +{ > > > + CPUX86State *env = &cpu->env; > > > + FeatureWord w; > > > + > > > + if (cpu->host_features) { > > > + for (w = 0; w < FEATURE_WORDS; w++) { > > > + env->features[w] = > > > + x86_cpu_get_supported_feature_word(w, > > > cpu->migratable); > > > + } > > > + } > > > +} > > > + > > > #ifdef CONFIG_KVM > > > > > > static int cpu_x86_fill_model_id(char *str) > > > @@ -3012,18 +3026,13 @@ static void x86_cpu_realizefn(DeviceState > > > *dev, Error **errp) return; > > > } > > > > > > + x86_cpu_load_host_data(cpu); > > this function should be below TODO comment as it applies to moved > > code. > > It was on purpose. The comment is actually about the > plus_features/minus_features code, that is the hack we want to > remove after cpu->host_features is fixed. > > Placing the comment before the x86_cpu_load_host_data() call > wouldn't make sense, as the host_features code is now hidden > inside the function. > > > > > with this fixed > > Reviewed-by: Igor Mammedov > > Considering the above explanation, do you prefer that I keep the > patch as-is, or move the comment inside x86_cpu_load_host_data()? I prefer comment inside call as it is related to bug introduced by moving env->features[w] = x86_cpu_get_supported_feature_word(w, cpu->migratable); into x86_cpu_parse_featurestr() for initfn(). plus_features/minus_features code in realize are side affect of above otherwise they could be converted at x86_cpu_parse_featurestr() time. > > (I will not move it before the x86_cpu_load_host_data() call) > > > > > > > + > > > /*TODO: cpu->host_features incorrectly overwrites features > > > * set using "feat=on|off". Once we fix this, we can convert > > > * plus_features & minus_features to global properties > > > * inside x86_cpu_parse_featurestr() too. > > > */ > > > - 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]; > > >