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" <address@hidden>
> To: "Igor Mammedov" <address@hidden>
> Cc: address@hidden, address@hidden, address@hidden, address@hidden
> 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 <address@hidden>
>
> $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
>