target/i386: kvm: add VMX and SVM migration blockers

[Qemu-devel] [PATCH] target/i386: kvm: add VMX and SVM migration blockers

Posted by Paolo Bonzini 6 years, 11 months ago

Nested VMX and SVM do not support live migration yet.  Add a blocker
until that is worked out.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/kvm.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index db1f4104b6..3b6fbd3f20 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -860,6 +860,8 @@ static int hyperv_init_vcpu(X86CPU *cpu)
 }
 
 static Error *invtsc_mig_blocker;
+static Error *vmx_mig_blocker;
+static Error *svm_mig_blocker;
 
 #define KVM_MAX_CPUID_ENTRIES  100
 
@@ -1250,6 +1252,29 @@ int kvm_arch_init_vcpu(CPUState *cs)
     if (c) {
         has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
                                   !!(c->ecx & CPUID_EXT_SMX);
+
+    }
+
+    if ((env->features[FEAT_1_ECX] & CPUID_EXT_VMX) && !vmx_mig_blocker) {
+        error_setg(&vmx_mig_blocker,
+                   "Nested VMX virtualization does not support live migration yet");
+        r = migrate_add_blocker(vmx_mig_blocker, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            error_free(vmx_mig_blocker);
+            return r;
+        }
+    }
+
+    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) && !svm_mig_blocker) {
+        error_setg(&svm_mig_blocker,
+                   "Nested SVM virtualization does not support live migration yet");
+        r = migrate_add_blocker(svm_mig_blocker, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            error_free(svm_mig_blocker);
+            return r;
+        }
     }
 
     if (env->mcg_cap & MCG_LMCE_P) {
-- 
2.19.1

Re: [Qemu-devel] [PATCH] target/i386: kvm: add VMX and SVM migration blockers

Posted by Dr. David Alan Gilbert 6 years, 11 months ago

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Nested VMX and SVM do not support live migration yet.  Add a blocker
> until that is worked out.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/kvm.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index db1f4104b6..3b6fbd3f20 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -860,6 +860,8 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>  }
>  
>  static Error *invtsc_mig_blocker;
> +static Error *vmx_mig_blocker;
> +static Error *svm_mig_blocker;
>  
>  #define KVM_MAX_CPUID_ENTRIES  100
>  
> @@ -1250,6 +1252,29 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      if (c) {
>          has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
>                                    !!(c->ecx & CPUID_EXT_SMX);
> +
> +    }
> +
> +    if ((env->features[FEAT_1_ECX] & CPUID_EXT_VMX) && !vmx_mig_blocker) {
> +        error_setg(&vmx_mig_blocker,
> +                   "Nested VMX virtualization does not support live migration yet");
> +        r = migrate_add_blocker(vmx_mig_blocker, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            error_free(vmx_mig_blocker);
> +            return r;
> +        }
> +    }
> +
> +    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) && !svm_mig_blocker) {
> +        error_setg(&svm_mig_blocker,
> +                   "Nested SVM virtualization does not support live migration yet");
> +        r = migrate_add_blocker(svm_mig_blocker, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            error_free(svm_mig_blocker);
> +            return r;
> +        }

I think that's OK from a migration point of view; my only worry is if
people have nesting enabled by default.  On AMD isn't it common to have
it enabled by default in KVM? Especially if using -cpu host ?

Dave

>      }
>  
>      if (env->mcg_cap & MCG_LMCE_P) {
> -- 
> 2.19.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] target/i386: kvm: add VMX and SVM migration blockers

Posted by Paolo Bonzini 6 years, 11 months ago

On 16/11/18 17:56, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> Nested VMX and SVM do not support live migration yet.  Add a blocker
>> until that is worked out.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  target/i386/kvm.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index db1f4104b6..3b6fbd3f20 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -860,6 +860,8 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>>  }
>>  
>>  static Error *invtsc_mig_blocker;
>> +static Error *vmx_mig_blocker;
>> +static Error *svm_mig_blocker;
>>  
>>  #define KVM_MAX_CPUID_ENTRIES  100
>>  
>> @@ -1250,6 +1252,29 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      if (c) {
>>          has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
>>                                    !!(c->ecx & CPUID_EXT_SMX);
>> +
>> +    }
>> +
>> +    if ((env->features[FEAT_1_ECX] & CPUID_EXT_VMX) && !vmx_mig_blocker) {
>> +        error_setg(&vmx_mig_blocker,
>> +                   "Nested VMX virtualization does not support live migration yet");
>> +        r = migrate_add_blocker(vmx_mig_blocker, &local_err);
>> +        if (local_err) {
>> +            error_report_err(local_err);
>> +            error_free(vmx_mig_blocker);
>> +            return r;
>> +        }
>> +    }
>> +
>> +    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) && !svm_mig_blocker) {
>> +        error_setg(&svm_mig_blocker,
>> +                   "Nested SVM virtualization does not support live migration yet");
>> +        r = migrate_add_blocker(svm_mig_blocker, &local_err);
>> +        if (local_err) {
>> +            error_report_err(local_err);
>> +            error_free(svm_mig_blocker);
>> +            return r;
>> +        }
> 
> I think that's OK from a migration point of view; my only worry is if
> people have nesting enabled by default.  On AMD isn't it common to have
> it enabled by default in KVM? Especially if using -cpu host ?

Hmm, yeah.  I'll send a v2 with only nested VMX.

Paolo

[Qemu-devel] [PATCH] migration: savevm: consult migration blockers

Posted by Paolo Bonzini 6 years, 11 months ago

There is really no difference between live migration and savevm, except
that savevm does not require bdrv_invalidate_cache to be implemented
by all disks.  However, it is unlikely that savevm is used with anything
except qcow2 disks, so the penalty is small and worth the improvement
in catching bad usage of savevm.

Only one place was taking care of savevm when adding a migration blocker,
and it can be removed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration/savevm.c | 4 ++++
 target/i386/kvm.c  | 3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index ef707b8c43..1c49776a91 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2455,6 +2455,10 @@ int save_snapshot(const char *name, Error **errp)
     struct tm tm;
     AioContext *aio_context;
 
+    if (migration_is_blocked(errp)) {
+        return false;
+    }
+
     if (!replay_can_snapshot()) {
         error_setg(errp, "Record/replay does not allow making snapshot "
                    "right now. Try once more later.");
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 3b6fbd3f20..d222b68fe4 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1284,7 +1284,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
     if (!env->user_tsc_khz) {
         if ((env->features[FEAT_8000_0007_EDX] & CPUID_APM_INVTSC) &&
             invtsc_mig_blocker == NULL) {
-            /* for migration */
             error_setg(&invtsc_mig_blocker,
                        "State blocked by non-migratable CPU device"
                        " (invtsc flag)");
@@ -1294,8 +1293,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
                 error_free(invtsc_mig_blocker);
                 return r;
             }
-            /* for savevm */
-            vmstate_x86_cpu.unmigratable = 1;
         }
     }
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH] migration: savevm: consult migration blockers

Posted by Dr. David Alan Gilbert 6 years, 11 months ago

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> There is really no difference between live migration and savevm, except
> that savevm does not require bdrv_invalidate_cache to be implemented
> by all disks.  However, it is unlikely that savevm is used with anything
> except qcow2 disks, so the penalty is small and worth the improvement
> in catching bad usage of savevm.
> 
> Only one place was taking care of savevm when adding a migration blocker,
> and it can be removed.

OK, I'm not sure if it was actually a split between savevm and migration
or just that the migration blockers were added later.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration/savevm.c | 4 ++++
>  target/i386/kvm.c  | 3 ---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index ef707b8c43..1c49776a91 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2455,6 +2455,10 @@ int save_snapshot(const char *name, Error **errp)
>      struct tm tm;
>      AioContext *aio_context;
>  
> +    if (migration_is_blocked(errp)) {
> +        return false;
> +    }
> +
>      if (!replay_can_snapshot()) {
>          error_setg(errp, "Record/replay does not allow making snapshot "
>                     "right now. Try once more later.");
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 3b6fbd3f20..d222b68fe4 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1284,7 +1284,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      if (!env->user_tsc_khz) {
>          if ((env->features[FEAT_8000_0007_EDX] & CPUID_APM_INVTSC) &&
>              invtsc_mig_blocker == NULL) {
> -            /* for migration */
>              error_setg(&invtsc_mig_blocker,
>                         "State blocked by non-migratable CPU device"
>                         " (invtsc flag)");
> @@ -1294,8 +1293,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>                  error_free(invtsc_mig_blocker);
>                  return r;
>              }
> -            /* for savevm */
> -            vmstate_x86_cpu.unmigratable = 1;

So that means vmstate_x86_cpu can be static now - but why does it live
in machine.c rather than cpu.c ?

Dave

>          }
>      }
>  
> -- 
> 2.19.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: savevm: consult migration blockers

Posted by Paolo Bonzini 6 years, 11 months ago

On 16/11/18 18:12, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> There is really no difference between live migration and savevm, except
>> that savevm does not require bdrv_invalidate_cache to be implemented
>> by all disks.  However, it is unlikely that savevm is used with anything
>> except qcow2 disks, so the penalty is small and worth the improvement
>> in catching bad usage of savevm.
>>
>> Only one place was taking care of savevm when adding a migration blocker,
>> and it can be removed.
> 
> OK, I'm not sure if it was actually a split between savevm and migration
> or just that the migration blockers were added later.

Just the latter, I think.

>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  migration/savevm.c | 4 ++++
>>  target/i386/kvm.c  | 3 ---
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index ef707b8c43..1c49776a91 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2455,6 +2455,10 @@ int save_snapshot(const char *name, Error **errp)
>>      struct tm tm;
>>      AioContext *aio_context;
>>  
>> +    if (migration_is_blocked(errp)) {
>> +        return false;
>> +    }
>> +
>>      if (!replay_can_snapshot()) {
>>          error_setg(errp, "Record/replay does not allow making snapshot "
>>                     "right now. Try once more later.");
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 3b6fbd3f20..d222b68fe4 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -1284,7 +1284,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      if (!env->user_tsc_khz) {
>>          if ((env->features[FEAT_8000_0007_EDX] & CPUID_APM_INVTSC) &&
>>              invtsc_mig_blocker == NULL) {
>> -            /* for migration */
>>              error_setg(&invtsc_mig_blocker,
>>                         "State blocked by non-migratable CPU device"
>>                         " (invtsc flag)");
>> @@ -1294,8 +1293,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>                  error_free(invtsc_mig_blocker);
>>                  return r;
>>              }
>> -            /* for savevm */
>> -            vmstate_x86_cpu.unmigratable = 1;
> 
> So that means vmstate_x86_cpu can be static now - but why does it live
> in machine.c rather than cpu.c ?

Generally all vmstate lives in machine.c.  Just historical reasons, it
was moved out of vl.c (!) many moons ago.

Paolo

Re: [Qemu-devel] [PATCH] migration: savevm: consult migration blockers

Posted by Wang, Wei W 6 years, 11 months ago

On Saturday, November 17, 2018 12:48 AM, Paolo Bonzini wrote:
> To: qemu-devel@nongnu.org
> Subject: [Qemu-devel] [PATCH] migration: savevm: consult migration
> blockers
> 
> There is really no difference between live migration and savevm, except that
> savevm does not require bdrv_invalidate_cache to be implemented by all
> disks.  However, it is unlikely that savevm is used with anything except qcow2
> disks, so the penalty is small and worth the improvement in catching bad
> usage of savevm.
> 
> Only one place was taking care of savevm when adding a migration blocker,
> and it can be removed.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration/savevm.c | 4 ++++
>  target/i386/kvm.c  | 3 ---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c index
> ef707b8c43..1c49776a91 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2455,6 +2455,10 @@ int save_snapshot(const char *name, Error
> **errp)
>      struct tm tm;
>      AioContext *aio_context;
> 
> +    if (migration_is_blocked(errp)) {
> +        return false;

Just curious why returning false here when the returning type is "int"

Best,
Wei