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

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 06/16/16 16:37, Eduardo Habkost wrote:
> On Thu, Jun 16, 2016 at 02:06:19PM +0800, Haozhong Zhang wrote:
> > From: Ashok Raj <ashok.raj@intel.com>
> > 
> > This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> > are injected to only one VCPU rather than broadcast to all VCPUs. As KVM
> > reports LMCE support on Intel platforms, this features is only available
> > on Intel platforms.
> > 
> > LMCE is disabled by default and can be enabled/disabled by cpu option
> > 'lmce=on/off'.
> > 
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > [Haozhong: Enable LMCE only on Intel platforms
> >            Disable LMCE by default and add a cpu option 'lmce'
> > 	   Disable LMCE if missing KVM support
> > 	   Remove MCG_LMCE_P from MCE_CAP_DEF
> > 	   Minor code style changes]
>
> You are mixing tabs and spaces above.
>

Oops, I missed to take care tabs in the commit message. will fix

> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  target-i386/cpu.c | 23 +++++++++++++++++++++++
> >  target-i386/cpu.h | 12 ++++++++++++
> >  target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> >  3 files changed, 66 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 895a386..bd35db2 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2777,15 +2777,37 @@ static void x86_cpu_machine_reset_cb(void *opaque)
> >  }
> >  #endif
> >  
> > +static bool lmce_supported(void)
> > +{
> > +    uint64_t mce_cap;
> > +
> > +    if (!kvm_enabled() ||
> > +        kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
> > +        return false;
> > +    }
> > +
> > +    return !!(mce_cap & MCG_LMCE_P);
> > +}
> > +
> >  static void mce_init(X86CPU *cpu)
> >  {
> >      CPUX86State *cenv = &cpu->env;
> >      unsigned int bank;
> > +    Error *local_err = NULL;
> >  
> >      if (((cenv->cpuid_version >> 8) & 0xf) >= 6
> >          && (cenv->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
> >              (CPUID_MCE | CPUID_MCA)) {
> >          cenv->mcg_cap = MCE_CAP_DEF | MCE_BANKS_DEF;
> > +
> > +        if (cpu->enable_lmce) {
> > +            if (!lmce_supported()) {
> > +                error_setg(&local_err, "KVM unavailable or LMCE not supported");
> > +                error_propagate(&error_abort, local_err);
> > +            }
> > +            cenv->mcg_cap |= MCG_LMCE_P;
> > +        }
> > +
> 
> This duplicates the existing check in kvm_arch_init_vcpu(). The
> difference is that the existing code is KVM-specific and doesn't
> stop initialization when capabilities are missing. We can unify
> them into a single mcg_cap-checking function as a follow-up.
>

If I reuse the existing MCE capability check in kvm_arch_init_vcpu(),
is it reasonable to make change to stop initialization if missing
capabilities? Or should we stop only for missing newly added capabilities
(e.g. LMCE) in order to keep backwards compatibility?

Thanks,
Haozhong