[PATCH] KVM: nVMX: Preserve registers modifications done before nested_svm_vmexit()

[PATCH] KVM: nVMX: Preserve registers modifications done before nested_svm_vmexit()

[Vitaly Kuznetsov]

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.

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

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@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
- 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

Re: [PATCH] KVM: nVMX: Preserve registers modifications done before nested_svm_vmexit()

[Sean Christopherson]

Shortlog says nVMX, code says nSVM :-)

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?

> 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

> 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

> 
> 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@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> - 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
> 

Re: [PATCH] KVM: nVMX: Preserve registers modifications done before nested_svm_vmexit()

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> 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@redhat.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> - 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

Re: [PATCH] KVM: nVMX: Preserve registers modifications done before nested_svm_vmexit()

[Sean Christopherson]

On Wed, May 27, 2020 at 10:58:14AM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> 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 :-)

Beetlejuice...Beetlejuice...Beetlejuice!