[PATCH 0/2] KVM: nVMX: fixes to nested virt interrupt injection

[PATCH 0/2] KVM: nVMX: fixes to nested virt interrupt injection

[Paolo Bonzini]

With these two patches, KVM does not blindly pass the exit interruption
info and exit qualification from the vmcs02 and vmcs12 when injecting
an exception.  There were two spots where this was done, namely
nested_vmx_check_exception and vmx_inject_page_fault_nested.

Patch 1 avoids writing the vmcs02's VM_EXIT_INTR_ERROR_CODE field,
which as Jim noticed isn't possible on pre-Haswell machines, but
otherwise has no semantic effect.

Patch 2 is the actual bugfix.

Paolo Bonzini (2):
  KVM: nVMX: do not fill vm_exit_intr_error_code in prepare_vmcs12
  KVM: nVMX: fixes to nested virt interrupt injection

 arch/x86/kvm/svm.c |  10 ++++
 arch/x86/kvm/vmx.c | 133 +++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 103 insertions(+), 40 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] KVM: nVMX: do not fill vm_exit_intr_error_code in prepare_vmcs12

[Paolo Bonzini]

Do this in the caller of nested_vmx_vmexit instead.

nested_vmx_check_exception was doing a vmwrite to the vmcs02's
VM_EXIT_INTR_ERROR_CODE field, so that prepare_vmcs12 would move
the field to vmcs12->vm_exit_intr_error_code.  However that isn't
possible on pre-Haswell machines.  Moving the vmcs12 write to the
callers fixes it.

Reported-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx.c | 52 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 47c47f09ee90..18a9a1bb3991 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2442,7 +2442,7 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu)
 		return 0;
 
 	if (vcpu->arch.exception.nested_apf) {
-		vmcs_write32(VM_EXIT_INTR_ERROR_CODE, vcpu->arch.exception.error_code);
+		vmcs12->vm_exit_intr_error_code = vcpu->arch.exception.error_code;
 		nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,
 			PF_VECTOR | INTR_TYPE_HARD_EXCEPTION |
 			INTR_INFO_DELIVER_CODE_MASK | INTR_INFO_VALID_MASK,
@@ -2450,6 +2450,7 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu)
 		return 1;
 	}
 
+	vmcs12->vm_exit_intr_error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
 	nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,
 			  vmcs_read32(VM_EXIT_INTR_INFO),
 			  vmcs_readl(EXIT_QUALIFICATION));
@@ -2667,7 +2668,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
 	 * reason is that if one of these bits is necessary, it will appear
 	 * in vmcs01 and prepare_vmcs02, when it bitwise-or's the control
 	 * fields of vmcs01 and vmcs02, will turn these bits off - and
-	 * nested_vmx_exit_handled() will not pass related exits to L1.
+	 * nested_vmx_exit_reflected() will not pass related exits to L1.
 	 * These rules have exceptions below.
 	 */
 
@@ -8019,12 +8020,11 @@ static bool nested_vmx_exit_handled_cr(struct kvm_vcpu *vcpu,
  * should handle it ourselves in L0 (and then continue L2). Only call this
  * when in is_guest_mode (L2).
  */
-static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
+static bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
 {
 	u32 intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
-	u32 exit_reason = vmx->exit_reason;
 
 	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), exit_reason,
 				vmcs_readl(EXIT_QUALIFICATION),
@@ -8173,6 +8173,29 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 	}
 }
 
+static bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason)
+{
+	u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+
+	/*
+	 * At this point, the exit interruption info in exit_intr_info
+	 * is only valid for EXCEPTION_NMI exits.  For EXTERNAL_INTERRUPT
+	 * we need to query the in-kernel LAPIC.
+	 */
+	WARN_ON(exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT);
+	if ((exit_intr_info &
+	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==
+	    (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) {
+		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+		vmcs12->vm_exit_intr_error_code =
+			vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
+	}
+
+	nested_vmx_vmexit(vcpu, exit_reason, exit_intr_info,
+			  vmcs_readl(EXIT_QUALIFICATION));
+	return 1;
+}
+
 static void vmx_get_exit_info(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2)
 {
 	*info1 = vmcs_readl(EXIT_QUALIFICATION);
@@ -8419,12 +8442,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
 	if (vmx->emulation_required)
 		return handle_invalid_guest_state(vcpu);
 
-	if (is_guest_mode(vcpu) && nested_vmx_exit_handled(vcpu)) {
-		nested_vmx_vmexit(vcpu, exit_reason,
-				  vmcs_read32(VM_EXIT_INTR_INFO),
-				  vmcs_readl(EXIT_QUALIFICATION));
-		return 1;
-	}
+	if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason))
+		return nested_vmx_reflect_vmexit(vcpu, exit_reason);
 
 	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
 		dump_vmcs();
@@ -9524,12 +9543,14 @@ static void vmx_inject_page_fault_nested(struct kvm_vcpu *vcpu,
 
 	WARN_ON(!is_guest_mode(vcpu));
 
-	if (nested_vmx_is_page_fault_vmexit(vmcs12, fault->error_code))
+	if (nested_vmx_is_page_fault_vmexit(vmcs12, fault->error_code)) {
+		vmcs12->vm_exit_intr_error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
 		nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason,
 				  vmcs_read32(VM_EXIT_INTR_INFO),
 				  vmcs_readl(EXIT_QUALIFICATION));
-	else
+	} else {
 		kvm_inject_page_fault(vcpu, fault);
+	}
 }
 
 static inline bool nested_vmx_merge_msr_bitmap(struct kvm_vcpu *vcpu,
@@ -10863,13 +10884,8 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 
 	vmcs12->vm_exit_reason = exit_reason;
 	vmcs12->exit_qualification = exit_qualification;
-
 	vmcs12->vm_exit_intr_info = exit_intr_info;
-	if ((vmcs12->vm_exit_intr_info &
-	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==
-	    (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK))
-		vmcs12->vm_exit_intr_error_code =
-			vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
+
 	vmcs12->idt_vectoring_info_field = 0;
 	vmcs12->vm_exit_instruction_len = vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 	vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-- 
1.8.3.1

[PATCH 2/2] KVM: nVMX: fixes to nested virt interrupt injection

[Paolo Bonzini]

There are three issues in nested_vmx_check_exception:

1) it is not taking PFEC_MATCH/PFEC_MASK into account, as reported
by Wanpeng Li;

2) it should rebuild the interruption info and exit qualification fields
from scratch, as reported by Jim Mattson, because the values from the
L2->L0 vmexit may be invalid (e.g. if an emulated instruction causes
a page fault, the EPT misconfig's exit qualification is incorrect).
This applies to vmx_inject_page_fault_nested as well.

3) CR2 and DR6 should not be written for exception intercept vmexits
(CR2 only for AMD).

This patch fixes the first two and adds a comment about the last,
outlining the fix.

Cc: Jim Mattson <jmattson@google.com>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm.c | 10 +++++++
 arch/x86/kvm/vmx.c | 87 ++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 4d8141e533c3..1107626938cc 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2430,6 +2430,16 @@ static int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 	svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + nr;
 	svm->vmcb->control.exit_code_hi = 0;
 	svm->vmcb->control.exit_info_1 = error_code;
+
+	/*
+	 * FIXME: we should not write CR2 when L1 intercepts an L2 #PF exception.
+	 * The fix is to add the ancillary datum (CR2 or DR6) to structs
+	 * kvm_queued_exception and kvm_vcpu_events, so that CR2 and DR6 can be
+	 * written only when inject_pending_event runs (DR6 would written here
+	 * too).  This should be conditional on a new capability---if the
+	 * capability is disabled, kvm_multiple_exception would write the
+	 * ancillary information to CR2 or DR6, for backwards ABI-compatibility.
+	 */
 	if (svm->vcpu.arch.exception.nested_apf)
 		svm->vmcb->control.exit_info_2 = svm->vcpu.arch.apf.nested_apf_token;
 	else
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 18a9a1bb3991..273734379ac8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -927,6 +927,10 @@ static void vmx_get_segment(struct kvm_vcpu *vcpu,
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx);
 static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx);
 static int alloc_identity_pagetable(struct kvm *kvm);
+static bool vmx_get_nmi_mask(struct kvm_vcpu *vcpu);
+static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked);
+static bool nested_vmx_is_page_fault_vmexit(struct vmcs12 *vmcs12,
+					    u16 error_code);
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
@@ -2428,6 +2432,30 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	vmx_set_interrupt_shadow(vcpu, 0);
 }
 
+static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu,
+					       unsigned long exit_qual)
+{
+	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+	unsigned int nr = vcpu->arch.exception.nr;
+	u32 intr_info = nr | INTR_INFO_VALID_MASK;
+
+	if (vcpu->arch.exception.has_error_code) {
+		vmcs12->vm_exit_intr_error_code = vcpu->arch.exception.error_code;
+		intr_info |= INTR_INFO_DELIVER_CODE_MASK;
+	}
+
+	if (kvm_exception_is_soft(nr))
+		intr_info |= INTR_TYPE_SOFT_EXCEPTION;
+	else
+		intr_info |= INTR_TYPE_HARD_EXCEPTION;
+
+	if (!(vmcs12->idt_vectoring_info_field & VECTORING_INFO_VALID_MASK) &&
+	    vmx_get_nmi_mask(vcpu))
+		intr_info |= INTR_INFO_UNBLOCK_NMI;
+
+	nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI, intr_info, exit_qual);
+}
+
 /*
  * KVM wants to inject page-faults which it got to the guest. This function
  * checks whether in a nested guest, we need to inject them to L1 or L2.
@@ -2437,24 +2465,38 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu)
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 	unsigned int nr = vcpu->arch.exception.nr;
 
-	if (!((vmcs12->exception_bitmap & (1u << nr)) ||
-		(nr == PF_VECTOR && vcpu->arch.exception.nested_apf)))
-		return 0;
+	if (nr == PF_VECTOR) {
+		if (vcpu->arch.exception.nested_apf) {
+			nested_vmx_inject_exception_vmexit(vcpu,
+							   vcpu->arch.apf.nested_apf_token);
+			return 1;
+		}
+		/*
+		 * FIXME: we must not write CR2 when L1 intercepts an L2 #PF exception.
+		 * The fix is to add the ancillary datum (CR2 or DR6) to structs
+		 * kvm_queued_exception and kvm_vcpu_events, so that CR2 and DR6
+		 * can be written only when inject_pending_event runs.  This should be
+		 * conditional on a new capability---if the capability is disabled,
+		 * kvm_multiple_exception would write the ancillary information to
+		 * CR2 or DR6, for backwards ABI-compatibility.
+		 */
+		if (nested_vmx_is_page_fault_vmexit(vmcs12,
+						    vcpu->arch.exception.error_code)) {
+			nested_vmx_inject_exception_vmexit(vcpu, vcpu->arch.cr2);
+			return 1;
+		}
+	} else {
+		unsigned long exit_qual = 0;
+		if (nr == DB_VECTOR)
+			exit_qual = vcpu->arch.dr6;
 
-	if (vcpu->arch.exception.nested_apf) {
-		vmcs12->vm_exit_intr_error_code = vcpu->arch.exception.error_code;
-		nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,
-			PF_VECTOR | INTR_TYPE_HARD_EXCEPTION |
-			INTR_INFO_DELIVER_CODE_MASK | INTR_INFO_VALID_MASK,
-			vcpu->arch.apf.nested_apf_token);
-		return 1;
+		if (vmcs12->exception_bitmap & (1u << nr)) {
+			nested_vmx_inject_exception_vmexit(vcpu, exit_qual);
+			return 1;
+		}
 	}
 
-	vmcs12->vm_exit_intr_error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
-	nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,
-			  vmcs_read32(VM_EXIT_INTR_INFO),
-			  vmcs_readl(EXIT_QUALIFICATION));
-	return 1;
+	return 0;
 }
 
 static void vmx_queue_exception(struct kvm_vcpu *vcpu)
@@ -9544,10 +9586,11 @@ static void vmx_inject_page_fault_nested(struct kvm_vcpu *vcpu,
 	WARN_ON(!is_guest_mode(vcpu));
 
 	if (nested_vmx_is_page_fault_vmexit(vmcs12, fault->error_code)) {
-		vmcs12->vm_exit_intr_error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
-		nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason,
-				  vmcs_read32(VM_EXIT_INTR_INFO),
-				  vmcs_readl(EXIT_QUALIFICATION));
+		vmcs12->vm_exit_intr_error_code = fault->error_code;
+		nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,
+				  PF_VECTOR | INTR_TYPE_HARD_EXCEPTION |
+				  INTR_INFO_DELIVER_CODE_MASK | INTR_INFO_VALID_MASK,
+				  fault->address);
 	} else {
 		kvm_inject_page_fault(vcpu, fault);
 	}
@@ -10130,12 +10173,6 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	 * "or"ing of the EB of vmcs01 and vmcs12, because when enable_ept,
 	 * vmcs01's EB.PF is 0 so the "or" will take vmcs12's value, and when
 	 * !enable_ept, EB.PF is 1, so the "or" will always be 1.
-	 *
-	 * A problem with this approach (when !enable_ept) is that L1 may be
-	 * injected with more page faults than it asked for. This could have
-	 * caused problems, but in practice existing hypervisors don't care.
-	 * To fix this, we will need to emulate the PFEC checking (on the L1
-	 * page tables), using walk_addr(), when injecting PFs to L1.
 	 */
 	vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK,
 		enable_ept ? vmcs12->page_fault_error_code_mask : 0);
-- 
1.8.3.1

Re: [PATCH 0/2] KVM: nVMX: fixes to nested virt interrupt injection

[Mike Galbraith]

On Fri, 2017-07-28 at 09:03 +0200, Paolo Bonzini wrote:
> With these two patches, KVM does not blindly pass the exit interruption
> info and exit qualification from the vmcs02 and vmcs12 when injecting
> an exception.  There were two spots where this was done, namely
> nested_vmx_check_exception and vmx_inject_page_fault_nested.
> 
> Patch 1 avoids writing the vmcs02's VM_EXIT_INTR_ERROR_CODE field,
> which as Jim noticed isn't possible on pre-Haswell machines, but
> otherwise has no semantic effect.
> 
> Patch 2 is the actual bugfix.

I was looking into oodles of lapic_timer.hv_timer_in_use warnings in
RT, introduced by KVM: LAPIC: Fix lapic timer injection delay, when I
noticed this post.  Happily, I can report that they are history.

	-Mike

Re: [PATCH 0/2] KVM: nVMX: fixes to nested virt interrupt injection

[Wanpeng Li]

2017-07-28 16:24 GMT+08:00 Mike Galbraith <efault@gmx.de>:
> On Fri, 2017-07-28 at 09:03 +0200, Paolo Bonzini wrote:
>> With these two patches, KVM does not blindly pass the exit interruption
>> info and exit qualification from the vmcs02 and vmcs12 when injecting
>> an exception.  There were two spots where this was done, namely
>> nested_vmx_check_exception and vmx_inject_page_fault_nested.
>>
>> Patch 1 avoids writing the vmcs02's VM_EXIT_INTR_ERROR_CODE field,
>> which as Jim noticed isn't possible on pre-Haswell machines, but
>> otherwise has no semantic effect.
>>
>> Patch 2 is the actual bugfix.
>
> I was looking into oodles of lapic_timer.hv_timer_in_use warnings in
> RT, introduced by KVM: LAPIC: Fix lapic timer injection delay, when I
> noticed this post.  Happily, I can report that they are history.

Yeah, it is fixed in the kvm/queue.
https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=queue&id=1d518c6820daf4e00d29adfba980aee05f605f0f

Regards,
Wanpeng Li

Re: [PATCH 0/2] KVM: nVMX: fixes to nested virt interrupt injection

[Paolo Bonzini]

On 28/07/2017 10:24, Mike Galbraith wrote:
> On Fri, 2017-07-28 at 09:03 +0200, Paolo Bonzini wrote:
>> With these two patches, KVM does not blindly pass the exit interruption
>> info and exit qualification from the vmcs02 and vmcs12 when injecting
>> an exception.  There were two spots where this was done, namely
>> nested_vmx_check_exception and vmx_inject_page_fault_nested.
>>
>> Patch 1 avoids writing the vmcs02's VM_EXIT_INTR_ERROR_CODE field,
>> which as Jim noticed isn't possible on pre-Haswell machines, but
>> otherwise has no semantic effect.
>>
>> Patch 2 is the actual bugfix.
> 
> I was looking into oodles of lapic_timer.hv_timer_in_use warnings in
> RT, introduced by KVM: LAPIC: Fix lapic timer injection delay, when I
> noticed this post.  Happily, I can report that they are history.

This patch won't fix them.  They are a race that comes and goes---but
yes, it's fixed.

Paolo

Re: [PATCH 0/2] KVM: nVMX: fixes to nested virt interrupt injection

[Mike Galbraith]

On Fri, 2017-07-28 at 13:53 +0200, Paolo Bonzini wrote:
> On 28/07/2017 10:24, Mike Galbraith wrote:
> > On Fri, 2017-07-28 at 09:03 +0200, Paolo Bonzini wrote:
> >> With these two patches, KVM does not blindly pass the exit interruption
> >> info and exit qualification from the vmcs02 and vmcs12 when injecting
> >> an exception.  There were two spots where this was done, namely
> >> nested_vmx_check_exception and vmx_inject_page_fault_nested.
> >>
> >> Patch 1 avoids writing the vmcs02's VM_EXIT_INTR_ERROR_CODE field,
> >> which as Jim noticed isn't possible on pre-Haswell machines, but
> >> otherwise has no semantic effect.
> >>
> >> Patch 2 is the actual bugfix.
> > 
> > I was looking into oodles of lapic_timer.hv_timer_in_use warnings in
> > RT, introduced by KVM: LAPIC: Fix lapic timer injection delay, when I
> > noticed this post.  Happily, I can report that they are history.
> 
> This patch won't fix them.  They are a race that comes and goes---but
> yes, it's fixed.

Yes, I just discovered that they had merely gone into hiding.
(and retrieved the patch that really really makes them dead)