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

[Paolo Bonzini]

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.

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

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

Paolo