Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
> Shortlog says nVMX, code says nSVM :-)
>
My brain is tainted, you know :-) Also, saying *VMX* always helps to get
your review so I may use the trick again :-)
> On Wed, May 27, 2020 at 10:29:21AM +0200, Vitaly Kuznetsov wrote:
>> L2 guest hang is observed after 'exit_required' was dropped and nSVM
>> switched to check_nested_events() completely. The hang is a busy loop when
>> e.g. KVM is emulating an instruction (e.g. L2 is accessing MMIO space and
>> we drop to userspace). After nested_svm_vmexit() and when L1 is doing VMRUN
>> nested guest's RIP is not advanced so KVM goes into emulating the same
>> instruction which cased nested_svm_vmexit() and the loop continues.
>
> s/cased/caused?
>
Yep.
>> nested_svm_vmexit() is not new, however, with check_nested_events() we're
>> now calling it later than before. In case by that time KVM has modified
>> register state we may pick stale values from VMCS when trying to save
>
> s/VMCS/VMCB
>
Yep.
>> nested guest state to nested VMCB.
>>
>> VMX code handles this case correctly: sync_vmcs02_to_vmcs12() called from
>> nested_vmx_vmexit() does 'vmcs12->guest_rip = kvm_rip_read(vcpu)' and this
>> ensures KVM-made modifications are preserved. Do the same for nVMX.
>
> s/nVMX/nSVM
>
Yep.
I'll do v2 to avoid the confusion.
>>
>> Generally, nested_vmx_vmexit()/nested_svm_vmexit() need to pick up all
>> nested guest state modifications done by KVM after vmexit. It would be
>> great to find a way to express this in a way which would not require to
>> manually track these changes, e.g. nested_{vmcb,vmcs}_get_field().
>>
>> Co-debugged-with: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>> ---
>> - To certain extent we're fixing currently-pending 'KVM: SVM: immediately
>> inject INTR vmexit' commit but I'm not certain about that. We had so many
>> problems with nested events before switching to check_nested_events() that
>> what worked before could just be treated as a miracle. Miracles tend to
>> appear and disappear all of a sudden.
>> ---
>> arch/x86/kvm/svm/nested.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 0f02521550b9..6b1049148c1b 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -537,9 +537,9 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>> nested_vmcb->save.cr2 = vmcb->save.cr2;
>> nested_vmcb->save.cr4 = svm->vcpu.arch.cr4;
>> nested_vmcb->save.rflags = kvm_get_rflags(&svm->vcpu);
>> - nested_vmcb->save.rip = vmcb->save.rip;
>> - nested_vmcb->save.rsp = vmcb->save.rsp;
>> - nested_vmcb->save.rax = vmcb->save.rax;
>> + nested_vmcb->save.rip = kvm_rip_read(&svm->vcpu);
>> + nested_vmcb->save.rsp = kvm_rsp_read(&svm->vcpu);
>> + nested_vmcb->save.rax = kvm_rax_read(&svm->vcpu);
>> nested_vmcb->save.dr7 = vmcb->save.dr7;
>> nested_vmcb->save.dr6 = svm->vcpu.arch.dr6;
>> nested_vmcb->save.cpl = vmcb->save.cpl;
>> --
>> 2.25.4
>>
>
--
Vitaly
