[PATCH 0/4] KVM: nVMX: Fix some failed VM-entry issues

[PATCH 0/4] KVM: nVMX: Fix some failed VM-entry issues

[Jim Mattson]

According to the Intel SDM, volume 3, section 26.7: VM-Entry Failures
During or After Loading Guest State,

  Although this process resembles that of a VM exit, many steps taken
  during a VM exit do not occur for these VM-entry failures:

  o Most VM-exit information fields are not updated (see step 1
    above).
  o The valid bit in the VM-entry interruption-information field is
    not cleared.
  o The guest-state area is not modified.
  o No MSRs are saved into the VM-exit MSR-store area.

"Step 1 above" indicates that information about the VM-entry failure
is only recorded in the exit reason and exit qualification fields.
*All other VM-exit information fields are unmodified.*

Moreover, the pseudo-code for VMLAUNCH/VMRESUME in section 30.3
indicates that the launch state of the VMCS is only set to "launched"
when the VM-entry succeeds.

The current nested_vmx_vmexit code does not sufficiently distinguish
VM-entry failure from a normal VM-exit, and therefore gets most of
these things wrong.

Jim Mattson (4):
  KVM: nVMX: Sequester all vmcs12 guest-state updates
  KVM: nVMX: Introduce update_vmcs12_vm_entry_controls
  KVM: nVMX: Introduce record_extra_vmcs12_exit_information
  KVM: nVMX: Don't set vmcs12 to "launched" when VMLAUNCH fails

 arch/x86/kvm/vmx.c | 112 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 69 insertions(+), 43 deletions(-)

-- 
2.13.0.219.gdb65acc882-goog

[PATCH 1/4] KVM: nVMX: Sequester all vmcs12 guest-state updates

[Jim Mattson]

Rename sync_vmcs12 to save_vmcs12_guest_state and hoist out any code
that doesn't just update guest-state fields in vmcs12. Guest-state
fields are not updated on failed VM-entry, so move the call site into
the appropriate conditional block.

Fixes: 4704d0befb072 ("KVM: nVMX: Exiting from L2 to L1")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ae66ebc7fb82..a01dd8bd712c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10627,11 +10627,10 @@ static u32 vmx_get_preemption_timer_value(struct kvm_vcpu *vcpu)
 
 /*
  * Update the guest state fields of vmcs12 to reflect changes that
- * occurred while L2 was running. (The "IA-32e mode guest" bit of the
- * VM-entry controls is also updated, since this is really a guest
- * state bit.)
+ * occurred while L2 was running.
  */
-static void sync_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
+static void save_vmcs12_guest_state(struct kvm_vcpu *vcpu,
+				    struct vmcs12 *vmcs12)
 {
 	vmcs12->guest_cr0 = vmcs12_guest_cr0(vcpu, vmcs12);
 	vmcs12->guest_cr4 = vmcs12_guest_cr4(vcpu, vmcs12);
@@ -10686,13 +10685,9 @@ static void sync_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	else
 		vmcs12->guest_activity_state = GUEST_ACTIVITY_ACTIVE;
 
-	if (nested_cpu_has_preemption_timer(vmcs12)) {
-		if (vmcs12->vm_exit_controls &
-		    VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)
-			vmcs12->vmx_preemption_timer_value =
-				vmx_get_preemption_timer_value(vcpu);
-		hrtimer_cancel(&to_vmx(vcpu)->nested.preemption_timer);
-	}
+	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)
+		vmcs12->vmx_preemption_timer_value =
+			vmx_get_preemption_timer_value(vcpu);
 
 	/*
 	 * In some cases (usually, nested EPT), L2 is allowed to change its
@@ -10710,15 +10705,9 @@ static void sync_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 		vmcs12->guest_pdptr3 = vmcs_read64(GUEST_PDPTR3);
 	}
 
-	vmcs12->guest_linear_address = vmcs_readl(GUEST_LINEAR_ADDRESS);
-
 	if (nested_cpu_has_vid(vmcs12))
 		vmcs12->guest_intr_status = vmcs_read16(GUEST_INTR_STATUS);
 
-	vmcs12->vm_entry_controls =
-		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
-		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
-
 	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS) {
 		kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7);
 		vmcs12->guest_ia32_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
@@ -10752,9 +10741,6 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 			   u32 exit_reason, u32 exit_intr_info,
 			   unsigned long exit_qualification)
 {
-	/* update guest state fields: */
-	sync_vmcs12(vcpu, vmcs12);
-
 	/* update exit information fields: */
 
 	vmcs12->vm_exit_reason = exit_reason;
@@ -10770,7 +10756,14 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	vmcs12->vm_exit_instruction_len = vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 	vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
 
+	vmcs12->guest_linear_address = vmcs_readl(GUEST_LINEAR_ADDRESS);
+
+	vmcs12->vm_entry_controls =
+		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
+		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
+
 	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) {
+		save_vmcs12_guest_state(vcpu, vmcs12);
 		/* vm_entry_intr_info_field is cleared on exit. Emulate this
 		 * instead of reading the real value. */
 		vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;
@@ -10944,6 +10937,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	/* trying to cancel vmlaunch/vmresume is a bug */
 	WARN_ON_ONCE(vmx->nested.nested_run_pending);
 
+	if (nested_cpu_has_preemption_timer(vmcs12))
+		hrtimer_cancel(&to_vmx(vcpu)->nested.preemption_timer);
+
 	leave_guest_mode(vcpu);
 	prepare_vmcs12(vcpu, vmcs12, exit_reason, exit_intr_info,
 		       exit_qualification);
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 2/4] KVM: nVMX: Introduce update_vmcs12_vm_entry_controls

[Jim Mattson]

VM-entry controls are updated on a VM-exit, but not on a failed
VM-entry. Collect these updates in a new function, and call that
function from the appropriate conditional block.

Fixes: 4704d0befb072 ("KVM: nVMX: Exiting from L2 to L1")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a01dd8bd712c..5059c6b45914 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10727,6 +10727,28 @@ static void save_vmcs12_guest_state(struct kvm_vcpu *vcpu,
 }
 
 /*
+ * Update the VM-entry control fields in vmcs12 after an emulated
+ * VM-exit (but not after a failed VM-entry).
+ */
+static void update_vmcs12_vm_entry_controls(struct kvm_vcpu *vcpu,
+					    struct vmcs12 *vmcs12)
+{
+	/*
+	 * The valid bit (bit 31) is cleared in the "VM-entry
+	 * interruption-information field" on VM-exit.
+	 */
+	vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;
+
+	/*
+	 * The value of IA32_EFER.LMA is stored in the "IA-32e mode guest"
+	 * VM-entry control.
+	 */
+	vmcs12->vm_entry_controls =
+		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
+		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
+}
+
+/*
  * prepare_vmcs12 is part of what we need to do when the nested L2 guest exits
  * and we want to prepare to run its L1 parent. L1 keeps a vmcs for L2 (vmcs12),
  * and this function updates it to reflect the changes to the guest state while
@@ -10758,16 +10780,9 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 
 	vmcs12->guest_linear_address = vmcs_readl(GUEST_LINEAR_ADDRESS);
 
-	vmcs12->vm_entry_controls =
-		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
-		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
-
 	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) {
+		update_vmcs12_vm_entry_controls(vcpu, vmcs12);
 		save_vmcs12_guest_state(vcpu, vmcs12);
-		/* vm_entry_intr_info_field is cleared on exit. Emulate this
-		 * instead of reading the real value. */
-		vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;
-
 		/*
 		 * Transfer the event that L0 or L1 may wanted to inject into
 		 * L2 to IDT_VECTORING_INFO_FIELD.
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 3/4] KVM: nVMX: Introduce record_extra_vmcs12_exit_information

[Jim Mattson]

Only the exit reason and exit qualification fields are written on a
failed VM-entry. Sequester the other exit-information field updates in
a separate function, and call it from the appropriate conditional
block.

Fixes: 4704d0befb072 ("KVM: nVMX: Exiting from L2 to L1")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 54 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5059c6b45914..4cddd6fab1c5 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10531,6 +10531,7 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu,
 	u32 idt_vectoring;
 	unsigned int nr;
 
+	vmcs12->idt_vectoring_info_field = 0;
 	if (vcpu->arch.exception.pending && vcpu->arch.exception.reinject) {
 		nr = vcpu->arch.exception.nr;
 		idt_vectoring = nr | VECTORING_INFO_VALID_MASK;
@@ -10749,6 +10750,34 @@ static void update_vmcs12_vm_entry_controls(struct kvm_vcpu *vcpu,
 }
 
 /*
+ * Record information about the nature of the VM-exit in the VM-exit
+ * information fields of vmcs12. Note that this function assumes that
+ * all of the extra VM-exit information aside from the "VM-exit
+ * interruption information" is live in vmcs02.
+ */
+static void record_extra_vmcs12_exit_information(struct kvm_vcpu *vcpu,
+						 struct vmcs12 *vmcs12,
+						 u32 exit_intr_info)
+{
+	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->vm_exit_instruction_len = vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
+	vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
+	vmcs12->guest_linear_address = vmcs_readl(GUEST_LINEAR_ADDRESS);
+	/*
+	 * Transfer the event that L0 or L1 may want to inject into L2
+	 * to "IDT-vectoring information" and associated fields. Note
+	 * that this function may overwrite "VM-exit instruction
+	 * length."
+	 */
+	vmcs12_save_pending_event(vcpu, vmcs12);
+}
+
+/*
  * prepare_vmcs12 is part of what we need to do when the nested L2 guest exits
  * and we want to prepare to run its L1 parent. L1 keeps a vmcs for L2 (vmcs12),
  * and this function updates it to reflect the changes to the guest state while
@@ -10763,31 +10792,18 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 			   u32 exit_reason, u32 exit_intr_info,
 			   unsigned long exit_qualification)
 {
-	/* update exit information fields: */
-
+	/*
+	 * The exit reason and exit qualification are saved for failed
+	 * VM-entry as well as VM-exit.
+	 */
 	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);
-
-	vmcs12->guest_linear_address = vmcs_readl(GUEST_LINEAR_ADDRESS);
-
 	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) {
 		update_vmcs12_vm_entry_controls(vcpu, vmcs12);
 		save_vmcs12_guest_state(vcpu, vmcs12);
-		/*
-		 * Transfer the event that L0 or L1 may wanted to inject into
-		 * L2 to IDT_VECTORING_INFO_FIELD.
-		 */
-		vmcs12_save_pending_event(vcpu, vmcs12);
+		record_extra_vmcs12_exit_information(vcpu, vmcs12,
+						     exit_intr_info);
 	}
 
 	/*
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 4/4] KVM: nVMX: Don't set vmcs12 to "launched" when VMLAUNCH fails

[Jim Mattson]

The VMCS launch state is not set to "launched" unless the VMLAUNCH
actually succeeds. VMLAUNCH failure includes VM-entry failure during
or after loading guest state (i.e. VM-exits with bit 31 set in the
exit reason).

Note that this change does not address the related problem that a
failure to launch/resume vmcs02 (i.e. vmx->fail) is not handled
correctly.

Fixes: cd232ad02f002 ("KVM: nVMX: Implement VMLAUNCH and VMRESUME")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4cddd6fab1c5..1d308c5b89fd 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10396,8 +10396,6 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
 		return 1;
 	}
 
-	vmcs12->launch_state = 1;
-
 	/*
 	 * Note no nested_vmx_succeed or nested_vmx_fail here. At this point
 	 * we are no longer running L1, and VMLAUNCH/VMRESUME has not yet
@@ -10800,6 +10798,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	vmcs12->exit_qualification = exit_qualification;
 
 	if (!(vmcs12->vm_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) {
+		vmcs12->launch_state = 1;
 		update_vmcs12_vm_entry_controls(vcpu, vmcs12);
 		save_vmcs12_guest_state(vcpu, vmcs12);
 		record_extra_vmcs12_exit_information(vcpu, vmcs12,
-- 
2.13.0.219.gdb65acc882-goog