Re: [PATCH 1/5] KVM: VMX: Fix handling of #MC that occurs during VM-Entry

From: Paolo Bonzini <pbonzini@redhat.com>
To: "Sean Christopherson" <sean.j.christopherson@intel.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Joerg Roedel" <joro@8bytes.org>
Cc: kvm@vger.kernel.org, Jim Mattson <jmattson@google.com>
Subject: Re: [PATCH 1/5] KVM: VMX: Fix handling of #MC that occurs during VM-Entry
Date: Thu, 6 Jun 2019 14:57:30 +0200	[thread overview]
Message-ID: <c368af05-7027-5d1b-52f0-105d50df1d83@redhat.com> (raw)
In-Reply-To: <20190420055059.16816-2-sean.j.christopherson@intel.com>

On 20/04/19 07:50, Sean Christopherson wrote:
> A previous fix to prevent KVM from consuming stale VMCS state after a
> failed VM-Entry inadvertantly blocked KVM's handling of machine checks
> that occur during VM-Entry.
> 
> Per Intel's SDM, a #MC during VM-Entry is handled in one of three ways,
> depending on when the #MC is recognoized.  As it pertains to this bug
> fix, the third case explicitly states EXIT_REASON_MCE_DURING_VMENTRY
> is handled like any other VM-Exit during VM-Entry, i.e. sets bit 31 to
> indicate the VM-Entry failed.
> 
> If a machine-check event occurs during a VM entry, one of the following occurs:
>  - The machine-check event is handled as if it occurred before the VM entry:
>         ...
>  - The machine-check event is handled after VM entry completes:
>         ...
>  - A VM-entry failure occurs as described in Section 26.7. The basic
>    exit reason is 41, for "VM-entry failure due to machine-check event".
> 
> Explicitly handle EXIT_REASON_MCE_DURING_VMENTRY as a one-off case in
> vmx_vcpu_run() instead of binning it into vmx_complete_atomic_exit().
> Doing so allows vmx_vcpu_run() to handle VMX_EXIT_REASONS_FAILED_VMENTRY
> in a sane fashion and also simplifies vmx_complete_atomic_exit() since
> VMCS.VM_EXIT_INTR_INFO is guaranteed to be fresh.
> 
> Fixes: b060ca3b2e9e7 ("kvm: vmx: Handle VMLAUNCH/VMRESUME failure properly")
> Cc: Jim Mattson <jmattson@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index d8f101b58ab8..79ce9c7062f9 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6103,28 +6103,21 @@ static void vmx_apicv_post_state_restore(struct kvm_vcpu *vcpu)
>  
>  static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
>  {
> -	u32 exit_intr_info = 0;
> -	u16 basic_exit_reason = (u16)vmx->exit_reason;
> -
> -	if (!(basic_exit_reason == EXIT_REASON_MCE_DURING_VMENTRY
> -	      || basic_exit_reason == EXIT_REASON_EXCEPTION_NMI))
> +	if (vmx->exit_reason != EXIT_REASON_EXCEPTION_NMI)
>  		return;
>  
> -	if (!(vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
> -		exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
> -	vmx->exit_intr_info = exit_intr_info;
> +	vmx->exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
>  
>  	/* if exit due to PF check for async PF */
> -	if (is_page_fault(exit_intr_info))
> +	if (is_page_fault(vmx->exit_intr_info))
>  		vmx->vcpu.arch.apf.host_apf_reason = kvm_read_and_reset_pf_reason();
>  
>  	/* Handle machine checks before interrupts are enabled */
> -	if (basic_exit_reason == EXIT_REASON_MCE_DURING_VMENTRY ||
> -	    is_machine_check(exit_intr_info))
> +	if (is_machine_check(vmx->exit_intr_info))
>  		kvm_machine_check();
>  
>  	/* We need to handle NMIs before interrupts are enabled */
> -	if (is_nmi(exit_intr_info)) {
> +	if (is_nmi(vmx->exit_intr_info)) {
>  		kvm_before_interrupt(&vmx->vcpu);
>  		asm("int $2");
>  		kvm_after_interrupt(&vmx->vcpu);

This is indeed cleaner in addition to fixing the bug.  I'm also applying this

-------------- 8< --------------
Subject: [PATCH] kvm: nVMX: small cleanup in handle_exception
From: Paolo Bonzini <pbonzini@redhat.com>

The reason for skipping handling of NMI and #MC in handle_exception is
the same, namely they are handled earlier by vmx_complete_atomic_exit.
Calling the machine check handler (which just returns 1) is misleading,
don't do it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1b3ca0582a0c..da6c829bad9f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4455,11 +4455,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
 	vect_info = vmx->idt_vectoring_info;
 	intr_info = vmx->exit_intr_info;
 
-	if (is_machine_check(intr_info))
-		return handle_machine_check(vcpu);
-
-	if (is_nmi(intr_info))
-		return 1;  /* already handled by vmx_vcpu_run() */
+	if (is_machine_check(intr_info) || is_nmi(intr_info))
+		return 1;  /* already handled by vmx_complete_atomic_exit */
 
 	if (is_invalid_opcode(intr_info))
 		return handle_ud(vcpu);

