Re: [PATCH v2] KVM: nVMX: Fix the NMI IDT-vectoring handling

[Paolo Bonzini]

On 22/09/2016 11:55, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> 
> Run kvm-unit-tests/eventinj.flat in L1:
> 
> Sending NMI to self
> After NMI to self
> FAIL: NMI
> 
> This test scenario is to test whether VMM can handle NMI IDT-vectoring info correctly.
> 
> At the beginning, L2 writes LAPIC to send a self NMI, the EPT page tables on both L1 
> and L0 are empty so:
> 
> - The L2 accesses memory can generate EPT violation which can be intercepted by L0.
> 
>   The EPT violation vmexit occurred during delivery of this NMI, and the NMI info is 
>   recorded in vmcs02's IDT-vectoring info.
> 
> - L0 walks L1's EPT12 and L0 sees the mapping is invalid, it injects the EPT violation into L1.
> 
>   The vmcs02's IDT-vectoring info is reflected to vmcs12's IDT-vectoring info since 
>   it is a nested vmexit. 
>   
> - L1 receives the EPT violation, then fixes its EPT12.
> - L1 executes VMRESUME to resume L2 which generates vmexit and causes L1 exits to L0.
> - L0 emulates VMRESUME which is called from L1, then return to L2.
> 
>   L0 merges the requirement of vmcs12's IDT-vectoring info and injects it to L2 through 
>   vmcs02.
> 
> - The L2 re-executes the fault instruction and cause EPT violation again.
> - Since the L1's EPT12 is valid, L0 can fix its EPT02
> - L0 resume L2
> 	
>   The EPT violation vmexit occurred during delivery of this NMI again, and the NMI info 
>   is recorded in vmcs02's IDT-vectoring info. L0 should inject the NMI through vmentry 
>   event injection since it is caused by EPT02's EPT violation.
> 
> However, vmx_inject_nmi() refuses to inject NMI from IDT-vectoring info if vCPU is in 
> guest mode, this patch fix it by permitting to inject NMI from IDT-vectoring if it is 
> the L0's responsibility to inject NMI from IDT-vectoring info to L2.
> 
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> Cc: Bandan Das <bsd@xxxxxxxxxx>
> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> ---
> v1 -> v2: 
>  * move the if (vmx->rmode.vm86_active) part out if (!is_guest_mode())

Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>

> 
>  arch/x86/kvm/vmx.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 813658d..5429a43 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5309,29 +5309,30 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  
> -	if (is_guest_mode(vcpu))
> -		return;
> +	if (!is_guest_mode(vcpu)) {
> +		if (!cpu_has_virtual_nmis()) {
> +			/*
> +			 * Tracking the NMI-blocked state in software is built upon
> +			 * finding the next open IRQ window. This, in turn, depends on
> +			 * well-behaving guests: They have to keep IRQs disabled at
> +			 * least as long as the NMI handler runs. Otherwise we may
> +			 * cause NMI nesting, maybe breaking the guest. But as this is
> +			 * highly unlikely, we can live with the residual risk.
> +			 */
> +			vmx->soft_vnmi_blocked = 1;
> +			vmx->vnmi_blocked_time = 0;
> +		}
>  
> -	if (!cpu_has_virtual_nmis()) {
> -		/*
> -		 * Tracking the NMI-blocked state in software is built upon
> -		 * finding the next open IRQ window. This, in turn, depends on
> -		 * well-behaving guests: They have to keep IRQs disabled at
> -		 * least as long as the NMI handler runs. Otherwise we may
> -		 * cause NMI nesting, maybe breaking the guest. But as this is
> -		 * highly unlikely, we can live with the residual risk.
> -		 */
> -		vmx->soft_vnmi_blocked = 1;
> -		vmx->vnmi_blocked_time = 0;
> +		++vcpu->stat.nmi_injections;
> +		vmx->nmi_known_unmasked = false;
>  	}
>  
> -	++vcpu->stat.nmi_injections;
> -	vmx->nmi_known_unmasked = false;
>  	if (vmx->rmode.vm86_active) {
>  		if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR, 0) != EMULATE_DONE)
>  			kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>  		return;
>  	}
> +
>  	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
>  			INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK | NMI_VECTOR);
>  }
>