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

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