[Qemu-devel] [PATCH] fixup! target-i386: x86_cpu_load_features() function

[Eduardo Habkost]

On Mon, Oct 10, 2016 at 02:25:32PM +0200, Igor Mammedov wrote:
> On Fri,  7 Oct 2016 17:29:01 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > When probing for CPU model information, we need to reuse the code
> > that initializes CPUID fields, but not the remaining side-effects
> > of x86_cpu_realizefn(). Move that code to a separate function
> > that can be reused later.
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Changes v5 -> v6:
> > * Move x86_cpu_filter_features() outside x86_cpu_load_features(),
> >   as the CPU model querying API won't run
> >   x86_cpu_filter_features() on most cases
> 
> > 
> > Changes v4 -> v5:
> > * Fix typo on x86_cpu_load_features() comment
> >   Reported-by: Paolo Bonzini <address@hidden>
> > 
> > Changes series v3 -> v4:
> > * New patch added to series
> > ---
> >  target-i386/cpu.c | 67 
> > +++++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 43 insertions(+), 24 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 1e8127b..23cc19b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2993,34 +2993,13 @@ static void x86_cpu_enable_xsave_components(X86CPU 
> > *cpu)
> >      env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
> >  }
> >  
> > -#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && 
> > \
> > -                           (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && 
> > \
> > -                           (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> > -#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> > -                         (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> > -                         (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> > -static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > +/* Load CPUID data based on configured features */
> > +static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
> >  {
> > -    CPUState *cs = CPU(dev);
> > -    X86CPU *cpu = X86_CPU(dev);
> > -    X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> >      CPUX86State *env = &cpu->env;
> > -    Error *local_err = NULL;
> > -    static bool ht_warned;
> >      FeatureWord w;
> >      GList *l;
> > -
> > -    if (xcc->kvm_required && !kvm_enabled()) {
> > -        char *name = x86_cpu_class_get_model_name(xcc);
> > -        error_setg(&local_err, "CPU model '%s' requires KVM", name);
> > -        g_free(name);
> > -        goto out;
> > -    }
> > -
> > -    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > -        error_setg(errp, "apic-id property was not initialized properly");
> > -        return;
> > -    }
> > +    Error *local_err = NULL;
> >  
> >      /*TODO: cpu->host_features incorrectly overwrites features
> >       * set using "feat=on|off". Once we fix this, we can convert
> > @@ -3086,6 +3065,46 @@ static void x86_cpu_realizefn(DeviceState *dev, 
> > Error **errp)
> >          env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
> >      }
> >  
> > +out:
> > +    if (local_err != NULL) {
> > +        error_propagate(errp, local_err);
> > +    }
> > +}
> > +
> > +#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && 
> > \
> > +                           (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && 
> > \
> > +                           (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> > +#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> > +                         (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> > +                         (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> > +static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > +{
> > +    CPUState *cs = CPU(dev);
> > +    X86CPU *cpu = X86_CPU(dev);
> > +    X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> > +    CPUX86State *env = &cpu->env;
> > +    Error *local_err = NULL;
> > +    static bool ht_warned;
> > +
> > +    if (xcc->kvm_required && !kvm_enabled()) {
> > +        char *name = x86_cpu_class_get_model_name(xcc);
> > +        error_setg(&local_err, "CPU model '%s' requires KVM", name);
> > +        g_free(name);
> > +        goto out;
> > +    }
> > +
> > +    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > +        error_setg(errp, "apic-id property was not initialized properly");
> > +        return;
> > +    }
> > +
> > +    x86_cpu_load_features(cpu, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    x86_cpu_filter_features(cpu);
> that makes 2 invocations of ^^ inside realize,
> see followup line vvvv

Oops, leftover from v5. Thanks for spotting that! Fixup below.

Signed-off-by: Eduardo Habkost <address@hidden>
---
 target-i386/cpu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 23cc19b..4294746 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3103,8 +3103,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
         goto out;
     }
 
-    x86_cpu_filter_features(cpu);
-
     if (x86_cpu_filter_features(cpu) &&
         (cpu->check_cpuid || cpu->enforce_cpuid)) {
         x86_cpu_report_filtered_features(cpu);
-- 
2.7.4

-- 
Eduardo