Re: [Qemu-devel] [PATCH v3 2/2] target-i386: add migration support for Intel LMCE

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

On 06/07/16 17:18, Eduardo Habkost wrote:
> On Fri, Jun 03, 2016 at 02:09:44PM +0800, Haozhong Zhang wrote:
> > LMCE is disabled by default, but a cpu option 'lmce=on/off' is provided
> > to enable/disable it. Migration is only allowed between VCPUs with the
> > same lmce option.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Boris Petkov <bp@suse.de>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Andi Kleen <andi.kleen@intel.com>
> > Cc: Ashok Raj <ashok.raj@intel.com>
> > ---
> >  include/hw/i386/pc.h  |  7 ++++++-
> >  target-i386/cpu.c     |  1 +
> >  target-i386/cpu.h     |  5 +++++
> >  target-i386/machine.c | 24 ++++++++++++++++++++++++
> >  4 files changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index ca23609..058eef9 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -357,7 +357,12 @@ int e820_get_num_entries(void);
> >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >  
> >  #define PC_COMPAT_2_6 \
> > -    HW_COMPAT_2_6
> > +    HW_COMPAT_2_6 \
> > +    {\
> > +        .driver   = TYPE_X86_CPU,\
> > +        .property = "lmce",\
> > +        .value    = "off",\
> > +    },
> 
> You don't need this if lmce is disabled by default.
>

Oh yes, I'll remove in the next version.

> >  
> >  #define PC_COMPAT_2_5 \
> >      PC_COMPAT_2_6 \
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 9b4dbab..c69cc17 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3232,6 +3232,7 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> >      DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
> > +    DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
> 
> Maybe this belong to patch 1/2?
>

I think it's better to not allow users to enable LMCE until we fix the
migration in patch 2, so I didn't put it in patch 1.

> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >  
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 2d411ba..b512fd6 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1182,6 +1182,11 @@ struct X86CPU {
> >       */
> >      bool enable_pmu;
> >  
> > +    /* Enable LMCE support which is set via cpu option 'lmce=on/off'. LMCE is
> > +     * disabled by default to avoid breaking the migration between QEMU with
> > +     * different LMCE support. Only migrating between QEMU with the same LMCE
> > +     * support is allowed.
> > +     */
> >      bool enable_lmce;
> >  
> >      /* in order to simplify APIC support, we leave this pointer to the
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index cb9adf2..b55d376 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -347,6 +347,11 @@ static int cpu_post_load(void *opaque, int version_id)
> >          return -EINVAL;
> >      }
> >  
> > +    if (!cpu->enable_lmce && (env->mcg_cap & MCG_LMCE_P)) {
> > +        error_report("LMCE not enabled");
> > +        return -EINVAL;
> > +    }
> 
> Nice. But the error message could be clearer, to indicate that it
> is about command-line configuration not being the same on both
> sides. What about something like:
>   config mismatch: VCPU has LMCE is enabled, but "lmce" option is disabled
>

Yes, yours is much clearer. I'll change in the next version.

Thanks,
Haozhong

> > +
> >      /*
> >       * Real mode guest segments register DPL should be zero.
> >       * Older KVM version were setting it wrongly.
> > @@ -896,6 +901,24 @@ static const VMStateDescription vmstate_tsc_khz = {
> >      }
> >  };
> >  
> > +static bool mcg_ext_ctl_needed(void *opaque)
> > +{
> > +    X86CPU *cpu = opaque;
> > +    CPUX86State *env = &cpu->env;
> > +    return cpu->enable_lmce && env->mcg_ext_ctl;
> > +}
> > +
> > +static const VMStateDescription vmstate_mcg_ext_ctl = {
> > +    .name = "cpu/mcg_ext_ctl",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = mcg_ext_ctl_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT64(env.mcg_ext_ctl, X86CPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  VMStateDescription vmstate_x86_cpu = {
> >      .name = "cpu",
> >      .version_id = 12,
> > @@ -1022,6 +1045,7 @@ VMStateDescription vmstate_x86_cpu = {
> >  #ifdef TARGET_X86_64
> >          &vmstate_pkru,
> >  #endif
> > +        &vmstate_mcg_ext_ctl,
> >          NULL
> >      }
> >  };
> > -- 
> > 2.8.3
> > 
> 
> -- 
> Eduardo