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

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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);
     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)
-- 
1.7.7.6

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

[Anthony Liguori]

What bug is this fixing?

Regards,

Anthony Liguori

On 05/09/2012 02:01 PM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> ---
>   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);
>       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)

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

[Andreas Färber]

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

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

[Igor Mammedov]

----- Original Message -----
> From: "Andreas Färber" <afaerber@suse.de>
> To: "Igor Mammedov" <imammedo@redhat.com>
> Cc: qemu-devel@nongnu.org, ehabkost@redhat.com, mdroth@linux.vnet.ibm.com, aliguori@us.ibm.com
> Sent: Wednesday, May 9, 2012 9:26:41 PM
> Subject: Re: [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model
>
> 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. ;))
Do I need to repost with 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.

no yet, just code review.
But wouldn't like to debug it when it explodes or doesn't work in guest.

>
> 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.

Exactly, it is intended as short term fix for 1.1, just noticed that
mce_init was moved before cpuid_* are initialized.

>
> 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.

I have such patch but I've thought it won't be accepted to for 1.1.
(It is more about rewrite then putting mce_init in place)

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

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

[Andreas Färber]

Am 09.05.2012 21:49, schrieb Igor Mammedov:
> ----- Original Message -----
>> From: "Andreas Färber" <afaerber@suse.de>
>> Am 09.05.2012 21:01, schrieb Igor Mammedov:
>>> 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.
> 
> Exactly, it is intended as short term fix for 1.1, just noticed that
> mce_init was moved before cpuid_* are initialized.

No doubt should that be fixed for 1.1. Hearing that 1.1-rc1 is already
well under way and in testing, it seems we can give this some more
thought and review.

I've posted an alternative patch that we could merge for 1.1-rc2. As
previously suggested for target-sh4 (where it didn't matter), it avoids
a dependency on Paolo's series, which we will definitely not get during
Hard Freeze, while remaining compatible to it for future assignment in
class_init.

Andreas

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