Re: [Qemu-devel] [ [PATCH]] nVMX: Initialize IA32_FEATURE_CONTROL MSR in reset and migration

[Eduardo Habkost]

On Tue, Jul 16, 2013 at 03:01:58PM +0300, Gleb Natapov wrote:
> On Tue, Jul 16, 2013 at 07:56:25PM +0800, Arthur Chunqi Li wrote:
> > On Tue, Jul 16, 2013 at 7:42 PM, Gleb Natapov <address@hidden> wrote:
> > > On Sun, Jul 07, 2013 at 11:13:37PM +0800, Arthur Chunqi Li wrote:
> > >> The recent KVM patch adds IA32_FEATURE_CONTROL support. QEMU needs
> > >> to clear this MSR when reset vCPU and keep the value of it when
> > >> migration. This patch add this feature.
> > >>
> > > So what happens if we migrate from qemu that does not have this patch
> > > to qemu that does? Since msr_ia32_feature_control will not be migrated
> > > it will not be set on the destination so destination will not be able to
> > > use nested vmx. Since nested vmx is experimental it may be to early for
> > > us to care about it though, and nested vmx does not work with migration
> > > anyway.
> > In my test, if migration doesn't care about msr_ia32_feature_control,
> > the value will be set to 0 in the destination VM and this may cause
> > some logical confusions, but the VMX running on it may not aware of
> > this (if migration nested vmx is supported in the future) because once
> > VMX initialized, it will not check this msr any more in normal cases.
> > 
> With vmm_exclusive=0 kvm does vmxon/vmxoff while running. But lest not
> worry about nested kvm migration for now. There are much harder problems
> to overcome before it will work.
> 
> > This is also a complex problem since we don't know how many states
> > like this msr need to be migrated related to nested virt. If there're
> > a lot of states need migrating, it is better to reconstruct the
> > relevant codes. But now this patch is enough.
> > 
> > Besides, though migration is not supported in nested vmx, we should
> > keep the machine state consistent during migration. So this patch is
> > also meaningful.

I'm assuming that even "qemu-1.6 -machine pc-1.5" is not expected to
allow migration to a qemu-1.5 binary. Is that OK for everybody, or
should we support backwards migration?

Other than that, the patch looks good to me. If migrating from a version
that doesn't have the patch, we are just going to get the same behavior
we had before.

> > 
> > Arthur
> > >
> > >> Signed-off-by: Arthur Chunqi Li <address@hidden>
> > >> ---
> > >>  target-i386/cpu.h     |    2 ++
> > >>  target-i386/kvm.c     |    4 ++++
> > >>  target-i386/machine.c |   22 ++++++++++++++++++++++
> > >>  3 files changed, 28 insertions(+)
> > >>
> > >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > >> index 62e3547..a418e17 100644
> > >> --- a/target-i386/cpu.h
> > >> +++ b/target-i386/cpu.h
> > >> @@ -301,6 +301,7 @@
> > >>  #define MSR_IA32_APICBASE_BSP           (1<<8)
> > >>  #define MSR_IA32_APICBASE_ENABLE        (1<<11)
> > >>  #define MSR_IA32_APICBASE_BASE          (0xfffff<<12)
> > >> +#define MSR_IA32_FEATURE_CONTROL        0x0000003a
> > >>  #define MSR_TSC_ADJUST                  0x0000003b
> > >>  #define MSR_IA32_TSCDEADLINE            0x6e0
> > >>
> > >> @@ -813,6 +814,7 @@ typedef struct CPUX86State {
> > >>
> > >>      uint64_t mcg_status;
> > >>      uint64_t msr_ia32_misc_enable;
> > >> +    uint64_t msr_ia32_feature_control;
> > >>
> > >>      /* exception/interrupt handling */
> > >>      int error_code;
> > >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > >> index 39f4fbb..3cb2161 100644
> > >> --- a/target-i386/kvm.c
> > >> +++ b/target-i386/kvm.c
> > >> @@ -1122,6 +1122,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > >>          if (hyperv_vapic_recommended()) {
> > >>              kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE, 
> > >> 0);
> > >>          }
> > >> +        kvm_msr_entry_set(&msrs[n++], MSR_IA32_FEATURE_CONTROL, 
> > >> env->msr_ia32_feature_control);
> > >>      }
> > >>      if (env->mcg_cap) {
> > >>          int i;
> > >> @@ -1346,6 +1347,7 @@ static int kvm_get_msrs(X86CPU *cpu)
> > >>      if (has_msr_misc_enable) {
> > >>          msrs[n++].index = MSR_IA32_MISC_ENABLE;
> > >>      }
> > >> +    msrs[n++].index = MSR_IA32_FEATURE_CONTROL;
> > >>
> > >>      if (!env->tsc_valid) {
> > >>          msrs[n++].index = MSR_IA32_TSC;
> > >> @@ -1444,6 +1446,8 @@ static int kvm_get_msrs(X86CPU *cpu)
> > >>          case MSR_IA32_MISC_ENABLE:
> > >>              env->msr_ia32_misc_enable = msrs[i].data;
> > >>              break;
> > >> +        case MSR_IA32_FEATURE_CONTROL:
> > >> +            env->msr_ia32_feature_control = msrs[i].data;
> > >>          default:
> > >>              if (msrs[i].index >= MSR_MC0_CTL &&
> > >>                  msrs[i].index < MSR_MC0_CTL + (env->mcg_cap & 0xff) * 
> > >> 4) {
> > >> diff --git a/target-i386/machine.c b/target-i386/machine.c
> > >> index 3659db9..94ca914 100644
> > >> --- a/target-i386/machine.c
> > >> +++ b/target-i386/machine.c
> > >> @@ -399,6 +399,14 @@ static bool misc_enable_needed(void *opaque)
> > >>      return env->msr_ia32_misc_enable != MSR_IA32_MISC_ENABLE_DEFAULT;
> > >>  }
> > >>
> > >> +static bool feature_control_needed(void *opaque)
> > >> +{
> > >> +    X86CPU *cpu = opaque;
> > >> +    CPUX86State *env = &cpu->env;
> > >> +
> > >> +    return env->msr_ia32_feature_control != 0;
> > >> +}
> > >> +
> > >>  static const VMStateDescription vmstate_msr_ia32_misc_enable = {
> > >>      .name = "cpu/msr_ia32_misc_enable",
> > >>      .version_id = 1,
> > >> @@ -410,6 +418,17 @@ static const VMStateDescription 
> > >> vmstate_msr_ia32_misc_enable = {
> > >>      }
> > >>  };
> > >>
> > >> +static const VMStateDescription vmstate_msr_ia32_feature_control = {
> > >> +    .name = "cpu/msr_ia32_feature_control",
> > >> +    .version_id = 1,
> > >> +    .minimum_version_id = 1,
> > >> +    .minimum_version_id_old = 1,
> > >> +    .fields      = (VMStateField []) {
> > >> +        VMSTATE_UINT64(env.msr_ia32_feature_control, X86CPU),
> > >> +        VMSTATE_END_OF_LIST()
> > >> +    }
> > >> +};
> > >> +
> > >>  const VMStateDescription vmstate_x86_cpu = {
> > >>      .name = "cpu",
> > >>      .version_id = 12,
> > >> @@ -535,6 +554,9 @@ const VMStateDescription vmstate_x86_cpu = {
> > >>          }, {
> > >>              .vmsd = &vmstate_msr_ia32_misc_enable,
> > >>              .needed = misc_enable_needed,
> > >> +        }, {
> > >> +            .vmsd = &vmstate_msr_ia32_feature_control,
> > >> +            .needed = feature_control_needed,
> > >>          } , {
> > >>              /* empty */
> > >>          }
> > >> --
> > >> 1.7.9.5
> > >
> > > --
> > >                         Gleb.
> > 
> > 
> > 
> > --
> > Arthur Chunqi Li
> > Department of Computer Science
> > School of EECS
> > Peking University
> > Beijing, China
> 
> --
>                       Gleb.
> 

-- 
Eduardo