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

[Paolo Bonzini]

On 21/06/19 14:39, Liran Alon wrote:
>> 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.

I totally missed that commit.  My bad.

Actually, now that I think about it SVM *will* have some state while
running in guest mode, namely:

- the NPT page table root

- the L1 CR4.PAE, EFER.LMA and EFER.NXE bits, which determine the format
of the NPT12 page tables

These are covered by the existing vmstate_svm_npt subsection.

On the other hand, the lack of something like VMXON/VMCS12 state means
that AMD already sorta works unless you're migrating while in guest
mode.  For Intel, just execute VMXON before migration, and starting any
VM after migration is doomed.

So, overall I prefer not to block migration.

>> +        ((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.

Caught by "make check", not by me!

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

This commit is where bisection broke. :)

Paolo