Re: [Qemu-devel] [PULL 22/25] target/i386: kvm: Add nested migration blocker only when kernel lacks required capabilities

[Liran Alon]

> On 21 Jun 2019, at 14:30, Paolo Bonzini <address@hidden> wrote:
> 
> From: Liran Alon <address@hidden>
> 
> Previous commits have added support for migration of nested virtualization
> workloads. This was done by utilising two new KVM capabilities:
> KVM_CAP_NESTED_STATE and KVM_CAP_EXCEPTION_PAYLOAD. Both which are
> required in order to correctly migrate such workloads.
> 
> Therefore, change code to add a migration blocker for vCPUs exposed with
> Intel VMX or AMD SVM in case one of these kernel capabilities is
> missing.
> 
> Signed-off-by: Liran Alon <address@hidden>
> Reviewed-by: Maran Wilson <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> target/i386/kvm.c     | 9 +++++++--
> target/i386/machine.c | 2 +-
> 2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index c931e9d..e4b4f57 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1640,9 +1640,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
>                                   !!(c->ecx & CPUID_EXT_SMX);
>     }
> 
> -    if (cpu_has_nested_virt(env) && !nested_virt_mig_blocker) {
> +    if (cpu_has_vmx(env) && !nested_virt_mig_blocker &&

The change here from cpu_has_nested_virt(env) to cpu_has_vmx(env) is not clear.
We should explicitly explain that it’s because we still wish to preserve 
backwards-compatability
to migrating AMD vCPU exposed with SVM.

In addition, commit ("target/i386: kvm: Block migration for vCPUs exposed with 
nested virtualization")
doesn’t make sense in case we still want to allow migrating AMD vCPU exposed 
with SVM.

Since QEMU commit 75d373ef9729 ("target-i386: Disable SVM by default in KVM 
mode"),
machine-types since v2.2 don’t expose AMD SVM by default.
Therefore, my personal opinion on this is that it’s fine to block migration in 
this case.

> +        ((kvm_max_nested_state_length() <= 0) || !has_exception_payload)) {
>         error_setg(&nested_virt_mig_blocker,
> -                   "Nested virtualization does not support live migration 
> yet");
> +                   "Kernel do not provide required capabilities for “

As Maran have noted, we should change this “do not” to “does not”.
Sorry for my bad English grammer. :)

> +                   "nested virtualization migration. "
> +                   "(CAP_NESTED_STATE=%d, CAP_EXCEPTION_PAYLOAD=%d)",
> +                   kvm_max_nested_state_length() > 0,
> +                   has_exception_payload);
>         r = migrate_add_blocker(nested_virt_mig_blocker, &local_err);
>         if (local_err) {
>             error_report_err(local_err);
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index fc49e5a..851b249 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -233,7 +233,7 @@ static int cpu_pre_save(void *opaque)
> 
> #ifdef CONFIG_KVM
>     /* Verify we have nested virtualization state from kernel if required */
> -    if (cpu_has_nested_virt(env) && !env->nested_state) {
> +    if (kvm_enabled() && cpu_has_vmx(env) && !env->nested_state) {

Good catch regarding adding kvm_enabled() here.
But I think this should have been added to commit ("target/i386: kvm: Add 
support for save and restore nested state”).

-Liran

>         error_report("Guest enabled nested virtualization but kernel "
>                 "does not support saving of nested state");
>         return -EINVAL;
> -- 
> 1.8.3.1
> 
>