Re: [Qemu-devel] [PATCH v4 1/3] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/16/16 11:50, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 08:06, Haozhong Zhang wrote:
> > +            if (!lmce_supported()) {
> > +                error_setg(&local_err, "KVM unavailable or LMCE not 
> > supported");
> > +                error_propagate(&error_abort, local_err);
> > +            }
> 
> Aborts should never be triggered by user input.  The error instead
> should propagate from mce_init to its caller with a new errp argument
> (i.e. error_setg(errp, "KVM unavailable or LMCE not supported")).
> 
> x86_cpu_realizefn can pass &local_err and check the outcome through
> local_err != NULL.  See the existing call to x86_cpu_apic_create, right
> above the call to mce_init.
>

Ah yes, I'll pass that local_err into mce_init() in the next version.

> > @@ -878,7 +891,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >      c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
> >      if (c) {
> >          has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
> > -                                  !!(c->ecx & CPUID_EXT_SMX);
> > +                                  !!(c->ecx & CPUID_EXT_SMX) ||
> > +                                  !!(env->mcg_cap & MCG_LMCE_P);
> 
> This part is wrong; env->mcg_cap is independent from CPUID[1].ECX.
>

Along with your next comment, I'll set it in the next if.

> > +    }
> > +
> > +    if (has_msr_feature_control && (env->mcg_cap & MCG_LMCE_P)) {
> 
> Don't test has_msr_feature_control here, instead set it to true inside
> the "if".
> 
> > +        has_msr_mcg_ext_ctl = true;
> >      }
> >  
> >      c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> 
> Which silicon has LMCE?  We may want to enable the property for some CPU
> models.  Apart from this, the patch is pretty much okay.
>

Skylake-EX

Thanks,
Haozhong