* [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model @ 2012-05-09 19:01 Igor Mammedov 2012-05-09 19:08 ` Anthony Liguori 2012-05-09 19:26 ` Andreas Färber 0 siblings, 2 replies; 5+ messages in thread From: Igor Mammedov @ 2012-05-09 19:01 UTC (permalink / raw) To: qemu-devel; +Cc: mdroth, aliguori, afaerber, ehabkost 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model 2012-05-09 19:01 [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model Igor Mammedov @ 2012-05-09 19:08 ` Anthony Liguori 2012-05-09 19:26 ` Andreas Färber 1 sibling, 0 replies; 5+ messages in thread From: Anthony Liguori @ 2012-05-09 19:08 UTC (permalink / raw) To: Igor Mammedov; +Cc: mdroth, qemu-devel, ehabkost, afaerber 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) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model 2012-05-09 19:01 [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model Igor Mammedov 2012-05-09 19:08 ` Anthony Liguori @ 2012-05-09 19:26 ` Andreas Färber 2012-05-09 19:49 ` Igor Mammedov 1 sibling, 1 reply; 5+ messages in thread From: Andreas Färber @ 2012-05-09 19:26 UTC (permalink / raw) To: Igor Mammedov; +Cc: mdroth, aliguori, qemu-devel, ehabkost 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model 2012-05-09 19:26 ` Andreas Färber @ 2012-05-09 19:49 ` Igor Mammedov 2012-05-09 22:31 ` Andreas Färber 0 siblings, 1 reply; 5+ messages in thread From: Igor Mammedov @ 2012-05-09 19:49 UTC (permalink / raw) To: Andreas Färber; +Cc: mdroth, aliguori, qemu-devel, ehabkost ----- 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model 2012-05-09 19:49 ` Igor Mammedov @ 2012-05-09 22:31 ` Andreas Färber 0 siblings, 0 replies; 5+ messages in thread From: Andreas Färber @ 2012-05-09 22:31 UTC (permalink / raw) To: Igor Mammedov; +Cc: mdroth, aliguori, qemu-devel, ehabkost 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-09 22:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-09 19:01 [Qemu-devel] [PATCH for 1.1-rc1] mce_init should be called after parsing cpu_model Igor Mammedov 2012-05-09 19:08 ` Anthony Liguori 2012-05-09 19:26 ` Andreas Färber 2012-05-09 19:49 ` Igor Mammedov 2012-05-09 22:31 ` Andreas Färber
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).