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 18:02, Paolo Bonzini <address@hidden> wrote:
> 
> 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.

True.

> 
> So, overall I prefer not to block migration.

I’m not sure I agree.
It is quite likely that vCPU is currently in guest-mode while you are migrating…
A good hypervisor tries to maximise CPU time to be in guest-mode rather than 
host-mode. :)

I prefer to block migration and once we formally complete the implementation of 
SVM nested state,
we can modify QEMU code such that migration of vCPU exposed with SVM will work 
in case
nested-state states that guest is in host-mode.

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

Ah nice to know :) Thanks for the tip.

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