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

[Igor Mammedov]

On Thu, 23 Jun 2016 13:04:53 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > ---
> > >  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 <address@hidden>
> 
> 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];
> > 
>