Re: [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model

["Andreas Färber" <afaerber@suse.de>]

Am 09.05.2012 21:01, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

$subject is right, my bad.
(Although I'm missing the target-i386: prefix. ;))

Re "for 1.1-rc1", fwiw this was broken in rc0 already, but then again
there was no tarball of that.

Does this fix some particular guest-visible behavior? Commit message
doesn't mention.

More comments inline:

> ---
>  target-i386/cpu.c |   35 ++++++++++++++++++-----------------
>  1 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 65d9af6..5d11e7b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1153,6 +1153,22 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
>      }
>  }
>  
> +static void mce_init(X86CPU *cpu)
> +{
> +    CPUX86State *cenv = &cpu->env;
> +    unsigned int bank;
> +
> +    if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> +        && (cenv->cpuid_features & (CPUID_MCE | CPUID_MCA)) ==
> +            (CPUID_MCE | CPUID_MCA)) {
> +        cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> +        cenv->mcg_ctl = ~(uint64_t)0;
> +        for (bank = 0; bank < MCE_BANKS_DEF; bank++) {
> +            cenv->mce_banks[bank * 4] = ~(uint64_t)0;
> +        }
> +    }
> +}
> +
>  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
>      CPUX86State *env = &cpu->env;
> @@ -1204,6 +1220,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          error_free(error);
>          return -1;
>      }
> +
> +    mce_init(cpu);

This would be okay as a short-term rc1 fix.

But long-term this doesn't fix the problem either, since we'd run into
the same problem again when the CPU gets manipulated via QMP after the
initialization.

A more correct fix would thus be to move mce_init() back into
cpu_x86_init(), but that's in a different file iirc.

My preferred solution would be to introduce an x86-local
x86_cpu_realize() function in cpu.c calling mce_init() - mce_init()
won't need to move then - and to call that from cpu_x86_init().
I'll send a patch once I'm through fixing my cpu_xxx_init() changes.

Regards,
Andreas

>      return 0;
>  }
>  
> @@ -1706,22 +1724,6 @@ static void x86_cpu_reset(CPUState *s)
>      cpu_watchpoint_remove_all(env, BP_CPU);
>  }
>  
> -static void mce_init(X86CPU *cpu)
> -{
> -    CPUX86State *cenv = &cpu->env;
> -    unsigned int bank;
> -
> -    if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> -        && (cenv->cpuid_features & (CPUID_MCE | CPUID_MCA)) ==
> -            (CPUID_MCE | CPUID_MCA)) {
> -        cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> -        cenv->mcg_ctl = ~(uint64_t)0;
> -        for (bank = 0; bank < MCE_BANKS_DEF; bank++) {
> -            cenv->mce_banks[bank * 4] = ~(uint64_t)0;
> -        }
> -    }
> -}
> -
>  static void x86_cpu_initfn(Object *obj)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> @@ -1755,7 +1757,6 @@ static void x86_cpu_initfn(Object *obj)
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
>  
>      env->cpuid_apic_id = env->cpu_index;
> -    mce_init(cpu);
>  }
>  
>  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg