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

[Igor Mammedov <imammedo@redhat.com>]

On Fri,  7 Oct 2016 17:29:01 -0300
Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com>
> ---
> 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 <pbonzini@redhat.com>
> 
> 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
 
[...]
>      if (x86_cpu_filter_features(cpu) &&
>          (cpu->check_cpuid || cpu->enforce_cpuid)) {
>          x86_cpu_report_filtered_features(cpu);