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

[Eduardo Habkost <ehabkost@redhat.com>]

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.

>  
>  #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?

>      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

> +
>      /*
>       * 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