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
