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

[Sean Christopherson <sean.j.christopherson@intel.com>]

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
>