Re: [libvirt] [Qemu-devel] [PATCH 2/3] target-i386: Introduce x86_cpu_load_host_data() function

On Thu, 23 Jun 2016 13:04:53 -0300 Eduardo Habkost <ehabkost(a)redhat.com&gt; 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 <ehabkost(a)redhat.com&gt; 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 <ehabkost(a)redhat.com&gt; > > > --- > > > 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 <imammedo(a)redhat.com&gt; > > 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.