[PATCH 00/10] KVM: VMX: Unionize vcpu_vmx.exit_reason

[PATCH 00/10] KVM: VMX: Unionize vcpu_vmx.exit_reason

[Sean Christopherson]

Convert the exit_reason field in struct vcpu_vmx from a vanilla u32 to a
union, (ab)using the union to provide access to the basic exit reason and
flags.

There are three motivating factors for making exit_reason a union:

  - Help avoid bugs where a basic exit reason is compared against the full
    exit reason, e.g. there have been two bugs where MCE_DURING_VMENTRY
    was incorrectly compared against the full exit reason.

  - Clarify the intent of related flows, e.g. exit_reason is used for both
    "basic exit reason" and "full exit reason", and it's not always clear
    which of the two is intended without a fair bit of digging.

  - Prepare for future Intel features, e.g. SGX, that add new exit flags
    that are less restricted than FAILED_VMENTRY, i.e. can be set on what
    is otherwise a standard VM-Exit.

Sean Christopherson (10):
  KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit()
  KVM: nVMX: Drop a superfluous WARN on reflecting EXTERNAL_INTERRUPT
  KVM: nVMX: Pull exit_reason from vcpu_vmx in
    nested_vmx_exit_reflected()
  KVM: VMX: Convert local exit_reason to u16 in
    nested_vmx_exit_reflected()
  KVM: VMX: Convert local exit_reason to u16 in vmx_handle_exit()
  KVM: nVMX: Convert local exit_reason to u16 in
    ...enter_non_root_mode()
  KVM: nVMX: Cast exit_reason to u16 to check for nested
    EXTERNAL_INTERRUPT
  KVM: nVMX: Rename exit_reason to vm_exit_reason for nested VM-Exit
  KVM: VMX: Cache vmx->exit_reason in local u16 in
    vmx_handle_exit_irqoff()
  KVM: VMX: Convert vcpu_vmx.exit_reason to a union

 arch/x86/kvm/vmx/nested.c | 49 +++++++++++++++++++++++----------------
 arch/x86/kvm/vmx/nested.h | 28 ++++++++++++----------
 arch/x86/kvm/vmx/vmx.c    | 39 +++++++++++++++++--------------
 arch/x86/kvm/vmx/vmx.h    | 25 +++++++++++++++++++-
 4 files changed, 90 insertions(+), 51 deletions(-)

-- 
2.24.1

[PATCH 01/10] KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit()

[Sean Christopherson]

Move the call to nested_vmx_exit_reflected() from vmx_handle_exit() into
nested_vmx_reflect_vmexit() and change the semantics of the return value
for nested_vmx_reflect_vmexit() to indicate whether or not the exit was
reflected into L1.  nested_vmx_exit_reflected() and
nested_vmx_reflect_vmexit() are intrinsically tied together, calling one
without simultaneously calling the other makes little sense.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.h | 16 +++++++++++-----
 arch/x86/kvm/vmx/vmx.c    |  4 ++--
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 21d36652f213..6bc379cf4755 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -72,12 +72,16 @@ static inline bool nested_ept_ad_enabled(struct kvm_vcpu *vcpu)
 }
 
 /*
- * Reflect a VM Exit into L1.
+ * Conditionally reflect a VM-Exit into L1.  Returns %true if the VM-Exit was
+ * reflected into L1.
  */
-static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
-					    u32 exit_reason)
+static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
+					     u32 exit_reason)
 {
-	u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+	u32 exit_intr_info;
+
+	if (!nested_vmx_exit_reflected(vcpu, exit_reason))
+		return false;
 
 	/*
 	 * At this point, the exit interruption info in exit_intr_info
@@ -85,6 +89,8 @@ static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
 	 * we need to query the in-kernel LAPIC.
 	 */
 	WARN_ON(exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT);
+
+	exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
 	if ((exit_intr_info &
 	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==
 	    (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) {
@@ -96,7 +102,7 @@ static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
 
 	nested_vmx_vmexit(vcpu, exit_reason, exit_intr_info,
 			  vmcs_readl(EXIT_QUALIFICATION));
-	return 1;
+	return true;
 }
 
 /*
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 57742ddfd854..c1caac7e8f57 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5863,8 +5863,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_reflected(vcpu, exit_reason))
-		return nested_vmx_reflect_vmexit(vcpu, exit_reason);
+	if (is_guest_mode(vcpu) && nested_vmx_reflect_vmexit(vcpu, exit_reason))
+		return 1;
 
 	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
 		dump_vmcs();
-- 
2.24.1

[PATCH 02/10] KVM: nVMX: Drop a superfluous WARN on reflecting EXTERNAL_INTERRUPT

[Sean Christopherson]

Drop the WARN in nested_vmx_reflect_vmexit() that fires if KVM attempts
to reflect an external interrupt.  nested_vmx_exit_reflected() is now
called from nested_vmx_reflect_vmexit() and unconditionally returns
false for EXTERNAL_INTERRUPT, i.e. barring a compiler or major CPU bug,
the WARN will never fire.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 6bc379cf4755..8f5ff3e259c9 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -84,12 +84,11 @@ static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
 		return false;
 
 	/*
-	 * 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.
+	 * vmcs.VM_EXIT_INTR_INFO is only valid for EXCEPTION_NMI exits.  For
+	 * EXTERNAL_INTERRUPT, the value for vmcs12->vm_exit_intr_info would
+	 * need to be synthesized by querying the in-kernel LAPIC, but external
+	 * interrupts are never reflected to L1 so it's a non-issue.
 	 */
-	WARN_ON(exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT);
-
 	exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
 	if ((exit_intr_info &
 	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==
-- 
2.24.1

[PATCH 03/10] KVM: nVMX: Pull exit_reason from vcpu_vmx in nested_vmx_exit_reflected()

[Sean Christopherson]

Grab the exit reason from the vcpu struct in nested_vmx_exit_reflected()
instead of having the exit reason explicitly passed from the caller.
This fixes a discrepancy between VM-Fail and VM-Exit handling, as the
VM-Fail case is already handled by checking vcpu_vmx, e.g. the exit
reason previously passed on the stack is bogus if vmx->fail is set.

Not taking the exit reason on the stack also avoids having to document
that nested_vmx_exit_reflected() requires the full exit reason, as
opposed to just the basic exit reason, which is not at all obvious since
the only usage of the full exit reason is for tracing.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 3 ++-
 arch/x86/kvm/vmx/nested.h | 9 ++++-----
 arch/x86/kvm/vmx/vmx.c    | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 79c7764c77b1..cb05bcbbfc4e 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5518,11 +5518,12 @@ static bool nested_vmx_exit_handled_vmcs_access(struct kvm_vcpu *vcpu,
  * should handle it ourselves in L0 (and then continue L2). Only call this
  * when in is_guest_mode (L2).
  */
-bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
+bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu)
 {
 	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;
 
 	if (vmx->nested.nested_run_pending)
 		return false;
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 8f5ff3e259c9..569cb828b6ca 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -24,7 +24,7 @@ void nested_vmx_set_vmcs_shadowing_bitmap(void);
 void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu);
 enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
 						     bool from_vmentry);
-bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason);
+bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu);
 void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 		       u32 exit_intr_info, unsigned long exit_qualification);
 void nested_sync_vmcs12_to_shadow(struct kvm_vcpu *vcpu);
@@ -75,12 +75,11 @@ static inline bool nested_ept_ad_enabled(struct kvm_vcpu *vcpu)
  * Conditionally reflect a VM-Exit into L1.  Returns %true if the VM-Exit was
  * reflected into L1.
  */
-static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
-					     u32 exit_reason)
+static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
 {
 	u32 exit_intr_info;
 
-	if (!nested_vmx_exit_reflected(vcpu, exit_reason))
+	if (!nested_vmx_exit_reflected(vcpu))
 		return false;
 
 	/*
@@ -99,7 +98,7 @@ static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
 			vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
 	}
 
-	nested_vmx_vmexit(vcpu, exit_reason, exit_intr_info,
+	nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, exit_intr_info,
 			  vmcs_readl(EXIT_QUALIFICATION));
 	return true;
 }
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c1caac7e8f57..c7715c880ea7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5863,7 +5863,7 @@ 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_reflect_vmexit(vcpu, exit_reason))
+	if (is_guest_mode(vcpu) && nested_vmx_reflect_vmexit(vcpu))
 		return 1;
 
 	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
-- 
2.24.1

[PATCH 04/10] KVM: VMX: Convert local exit_reason to u16 in nested_vmx_exit_reflected()

[Sean Christopherson]

Store only the basic exit reason in the local "exit_reason" variable in
nested_vmx_exit_reflected().  Except for tracing, all references to
exit_reason are expecting to encounter only the basic exit reason.

Opportunistically align the params to nested_vmx_exit_handled_msr().

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index cb05bcbbfc4e..1848ca0116c0 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5374,7 +5374,7 @@ static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
  * MSR bitmap. This may be the case even when L0 doesn't use MSR bitmaps.
  */
 static bool nested_vmx_exit_handled_msr(struct kvm_vcpu *vcpu,
-	struct vmcs12 *vmcs12, u32 exit_reason)
+					struct vmcs12 *vmcs12, u16 exit_reason)
 {
 	u32 msr_index = kvm_rcx_read(vcpu);
 	gpa_t bitmap;
@@ -5523,7 +5523,7 @@ bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu)
 	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;
+	u16 exit_reason;
 
 	if (vmx->nested.nested_run_pending)
 		return false;
@@ -5548,13 +5548,15 @@ bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu)
 	 */
 	nested_mark_vmcs12_pages_dirty(vcpu);
 
-	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), exit_reason,
+	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), vmx->exit_reason,
 				vmcs_readl(EXIT_QUALIFICATION),
 				vmx->idt_vectoring_info,
 				intr_info,
 				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
 				KVM_ISA_VMX);
 
+	exit_reason = vmx->exit_reason;
+
 	switch (exit_reason) {
 	case EXIT_REASON_EXCEPTION_NMI:
 		if (is_nmi(intr_info))
-- 
2.24.1

[PATCH 05/10] KVM: VMX: Convert local exit_reason to u16 in vmx_handle_exit()

[Sean Christopherson]

Convert the local "exit_reason" in vmx_handle_exit() from a u32 to a u16
as most references expect to encounter only the basic exit reason.  Use
vmx->exit_reason directly for paths that expect the full exit reason,
i.e. to avoid having to figure out a decent name for a second local
variable.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c7715c880ea7..d43e1d28bb58 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5844,10 +5844,10 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 	enum exit_fastpath_completion exit_fastpath)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	u32 exit_reason = vmx->exit_reason;
 	u32 vectoring_info = vmx->idt_vectoring_info;
+	u16 exit_reason;
 
-	trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
+	trace_kvm_exit(vmx->exit_reason, vcpu, KVM_ISA_VMX);
 
 	/*
 	 * Flush logged GPAs PML buffer, this will make dirty_bitmap more
@@ -5866,11 +5866,11 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 	if (is_guest_mode(vcpu) && nested_vmx_reflect_vmexit(vcpu))
 		return 1;
 
-	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
+	if (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
 		dump_vmcs();
 		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
 		vcpu->run->fail_entry.hardware_entry_failure_reason
-			= exit_reason;
+			= vmx->exit_reason;
 		return 0;
 	}
 
@@ -5882,6 +5882,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 		return 0;
 	}
 
+	exit_reason = vmx->exit_reason;
+
 	/*
 	 * Note:
 	 * Do not try to fix EXIT_REASON_EPT_MISCONFIG if it caused by
@@ -5898,7 +5900,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
 		vcpu->run->internal.ndata = 3;
 		vcpu->run->internal.data[0] = vectoring_info;
-		vcpu->run->internal.data[1] = exit_reason;
+		vcpu->run->internal.data[1] = vmx->exit_reason;
 		vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
 		if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
 			vcpu->run->internal.ndata++;
@@ -5957,13 +5959,14 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 	return kvm_vmx_exit_handlers[exit_reason](vcpu);
 
 unexpected_vmexit:
-	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
+	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n",
+		    vmx->exit_reason);
 	dump_vmcs();
 	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 	vcpu->run->internal.suberror =
 			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
 	vcpu->run->internal.ndata = 1;
-	vcpu->run->internal.data[0] = exit_reason;
+	vcpu->run->internal.data[0] = vmx->exit_reason;
 	return 0;
 }
 
-- 
2.24.1

[PATCH 06/10] KVM: nVMX: Convert local exit_reason to u16 in ...enter_non_root_mode()

[Sean Christopherson]

Use a u16 for nested_vmx_enter_non_root_mode()'s local "exit_reason" to
make it clear the intermediate code is only responsible for setting the
basic exit reason, e.g. FAILED_VMENTRY is unconditionally OR'd in when
emulating a failed VM-Entry.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 1848ca0116c0..8fbbe2152ab7 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3182,7 +3182,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 	bool evaluate_pending_interrupts;
-	u32 exit_reason = EXIT_REASON_INVALID_STATE;
+	u16 exit_reason = EXIT_REASON_INVALID_STATE;
 	u32 exit_qual;
 
 	evaluate_pending_interrupts = exec_controls_get(vmx) &
@@ -3308,7 +3308,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
 		return NVMX_VMENTRY_VMEXIT;
 
 	load_vmcs12_host_state(vcpu, vmcs12);
-	vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
+	vmcs12->vm_exit_reason = VMX_EXIT_REASONS_FAILED_VMENTRY | exit_reason;
 	vmcs12->exit_qualification = exit_qual;
 	if (enable_shadow_vmcs || vmx->nested.hv_evmcs)
 		vmx->nested.need_vmcs12_to_shadow_sync = true;
-- 
2.24.1

[PATCH 07/10] KVM: nVMX: Cast exit_reason to u16 to check for nested EXTERNAL_INTERRUPT

[Sean Christopherson]

Explicitly check only the basic exit reason when emulating an external
interrupt VM-Exit in nested_vmx_vmexit().  Checking the full exit reason
doesn't currently cause problems, but only because the only exit reason
modifier support by KVM is FAILED_VMENTRY, which is mutually exclusive
with EXTERNAL_INTERRUPT.  Future modifiers, e.g. ENCLAVE_MODE, will
coexist with EXTERNAL_INTERRUPT.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 8fbbe2152ab7..86b12a2918c5 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4337,7 +4337,7 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 
 	if (likely(!vmx->fail)) {
-		if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
+		if ((u16)exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
 		    nested_exit_intr_ack_set(vcpu)) {
 			int irq = kvm_cpu_get_interrupt(vcpu);
 			WARN_ON(irq < 0);
-- 
2.24.1

[PATCH 08/10] KVM: nVMX: Rename exit_reason to vm_exit_reason for nested VM-Exit

[Sean Christopherson]

Use "vm_exit_reason" when passing around the full exit reason for nested
VM-Exits to make it clear that it's not just the basic exit reason.  The
basic exit reason (bits 15:0 of vmcs.VM_EXIT_REASON) is colloquially
referred to as simply "exit reason", e.g. vmx_handle_vmexit() tracks the
basic exit reason in a local variable named "exit_reason".

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 29 +++++++++++++++--------------
 arch/x86/kvm/vmx/nested.h |  2 +-
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 86b12a2918c5..c775feca3eb0 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -328,19 +328,19 @@ static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
 {
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	u32 exit_reason;
+	u32 vm_exit_reason;
 	unsigned long exit_qualification = vcpu->arch.exit_qualification;
 
 	if (vmx->nested.pml_full) {
-		exit_reason = EXIT_REASON_PML_FULL;
+		vm_exit_reason = EXIT_REASON_PML_FULL;
 		vmx->nested.pml_full = false;
 		exit_qualification &= INTR_INFO_UNBLOCK_NMI;
 	} else if (fault->error_code & PFERR_RSVD_MASK)
-		exit_reason = EXIT_REASON_EPT_MISCONFIG;
+		vm_exit_reason = EXIT_REASON_EPT_MISCONFIG;
 	else
-		exit_reason = EXIT_REASON_EPT_VIOLATION;
+		vm_exit_reason = EXIT_REASON_EPT_VIOLATION;
 
-	nested_vmx_vmexit(vcpu, exit_reason, 0, exit_qualification);
+	nested_vmx_vmexit(vcpu, vm_exit_reason, 0, exit_qualification);
 	vmcs12->guest_physical_address = fault->address;
 }
 
@@ -3919,11 +3919,11 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
  * which already writes to vmcs12 directly.
  */
 static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
-			   u32 exit_reason, u32 exit_intr_info,
+			   u32 vm_exit_reason, u32 exit_intr_info,
 			   unsigned long exit_qualification)
 {
 	/* update exit information fields: */
-	vmcs12->vm_exit_reason = exit_reason;
+	vmcs12->vm_exit_reason = vm_exit_reason;
 	vmcs12->exit_qualification = exit_qualification;
 	vmcs12->vm_exit_intr_info = exit_intr_info;
 
@@ -4252,7 +4252,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
  * and modify vmcs12 to make it see what it would expect to see there if
  * L2 was its real guest. Must only be called when in L2 (is_guest_mode())
  */
-void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
+void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
 		       u32 exit_intr_info, unsigned long exit_qualification)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -4272,9 +4272,9 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	if (likely(!vmx->fail)) {
 		sync_vmcs02_to_vmcs12(vcpu, vmcs12);
 
-		if (exit_reason != -1)
-			prepare_vmcs12(vcpu, vmcs12, exit_reason, exit_intr_info,
-				       exit_qualification);
+		if (vm_exit_reason != -1)
+			prepare_vmcs12(vcpu, vmcs12, vm_exit_reason,
+				       exit_intr_info, exit_qualification);
 
 		/*
 		 * Must happen outside of sync_vmcs02_to_vmcs12() as it will
@@ -4330,14 +4330,15 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	 */
 	kvm_make_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu);
 
-	if ((exit_reason != -1) && (enable_shadow_vmcs || vmx->nested.hv_evmcs))
+	if ((vm_exit_reason != -1) &&
+	    (enable_shadow_vmcs || vmx->nested.hv_evmcs))
 		vmx->nested.need_vmcs12_to_shadow_sync = true;
 
 	/* in case we halted in L2 */
 	vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 
 	if (likely(!vmx->fail)) {
-		if ((u16)exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
+		if ((u16)vm_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
 		    nested_exit_intr_ack_set(vcpu)) {
 			int irq = kvm_cpu_get_interrupt(vcpu);
 			WARN_ON(irq < 0);
@@ -4345,7 +4346,7 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 				INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
 		}
 
-		if (exit_reason != -1)
+		if (vm_exit_reason != -1)
 			trace_kvm_nested_vmexit_inject(vmcs12->vm_exit_reason,
 						       vmcs12->exit_qualification,
 						       vmcs12->idt_vectoring_info_field,
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 569cb828b6ca..04584bcbcc8d 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -25,7 +25,7 @@ void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu);
 enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
 						     bool from_vmentry);
 bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu);
-void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
+void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
 		       u32 exit_intr_info, unsigned long exit_qualification);
 void nested_sync_vmcs12_to_shadow(struct kvm_vcpu *vcpu);
 int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
-- 
2.24.1

[PATCH 09/10] KVM: VMX: Cache vmx->exit_reason in local u16 in vmx_handle_exit_irqoff()

[Sean Christopherson]

Use a u16 to hold the exit reason in vmx_handle_exit_irqoff(), as the
checks for INTR/NMI/WRMSR expect to encounter only the basic exit reason
in vmx->exit_reason.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d43e1d28bb58..910a7cadeaf7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6287,16 +6287,16 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu)
 STACK_FRAME_NON_STANDARD(handle_external_interrupt_irqoff);
 
 static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
-	enum exit_fastpath_completion *exit_fastpath)
+				   enum exit_fastpath_completion *exit_fastpath)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	u16 exit_reason = vmx->exit_reason;
 
-	if (vmx->exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
+	if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
 		handle_external_interrupt_irqoff(vcpu);
-	else if (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI)
+	else if (exit_reason == EXIT_REASON_EXCEPTION_NMI)
 		handle_exception_nmi_irqoff(vmx);
-	else if (!is_guest_mode(vcpu) &&
-		vmx->exit_reason == EXIT_REASON_MSR_WRITE)
+	else if (!is_guest_mode(vcpu) && exit_reason == EXIT_REASON_MSR_WRITE)
 		*exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
 }
 
-- 
2.24.1

[PATCH 10/10] KVM: VMX: Convert vcpu_vmx.exit_reason to a union

[Sean Christopherson]

Convert vcpu_vmx.exit_reason from a u32 to a union (of size u32).  The
full VM_EXIT_REASON field is comprised of a 16-bit basic exit reason in
bits 15:0, and single-bit modifiers in bits 31:16.

Historically, KVM has only had to worry about handling the "failed
VM-Entry" modifier, which could only be set in very specific flows and
required dedicated handling.  I.e. manually stripping the FAILED_VMENTRY
bit was a somewhat viable approach.  But even with only a single bit to
worry about, KVM has had several bugs related to comparing a basic exit
reason against the full exit reason stored in vcpu_vmx.

Upcoming Intel features, e.g. SGX, will add new modifier bits that can
be set on more or less any VM-Exit, as opposed to the significantly more
restricted FAILED_VMENTRY, i.e. correctly handling everything in one-off
flows isn't scalable.  Tracking exit reason in a union forces code to
explicitly choose between consuming the full exit reason and the basic
exit reason, and is a convenient way to document and access the
modifiers.

No functional change intended.

Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 11 ++++++++---
 arch/x86/kvm/vmx/nested.h |  2 +-
 arch/x86/kvm/vmx/vmx.c    | 24 ++++++++++++------------
 arch/x86/kvm/vmx/vmx.h    | 25 ++++++++++++++++++++++++-
 4 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index c775feca3eb0..0c7cea35dd33 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5307,7 +5307,12 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
 	return kvm_skip_emulated_instruction(vcpu);
 
 fail:
-	nested_vmx_vmexit(vcpu, vmx->exit_reason,
+	/*
+	 * This is effectively a reflected VM-Exit, as opposed to a synthesized
+	 * nested VM-Exit.  Pass the original exit reason, i.e. don't hardcode
+	 * EXIT_REASON_VMFUNC as the exit reason.
+	 */
+	nested_vmx_vmexit(vcpu, vmx->exit_reason.full,
 			  vmcs_read32(VM_EXIT_INTR_INFO),
 			  vmcs_readl(EXIT_QUALIFICATION));
 	return 1;
@@ -5549,14 +5554,14 @@ bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu)
 	 */
 	nested_mark_vmcs12_pages_dirty(vcpu);
 
-	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), vmx->exit_reason,
+	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), vmx->exit_reason.full,
 				vmcs_readl(EXIT_QUALIFICATION),
 				vmx->idt_vectoring_info,
 				intr_info,
 				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
 				KVM_ISA_VMX);
 
-	exit_reason = vmx->exit_reason;
+	exit_reason = vmx->exit_reason.basic;
 
 	switch (exit_reason) {
 	case EXIT_REASON_EXCEPTION_NMI:
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 04584bcbcc8d..07ce09f88977 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -98,7 +98,7 @@ static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
 			vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
 	}
 
-	nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, exit_intr_info,
+	nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason.full, exit_intr_info,
 			  vmcs_readl(EXIT_QUALIFICATION));
 	return true;
 }
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 910a7cadeaf7..521b99f63608 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1588,7 +1588,7 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	 * i.e. we end up advancing IP with some random value.
 	 */
 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
-	    to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
+	    to_vmx(vcpu)->exit_reason.basic != EXIT_REASON_EPT_MISCONFIG) {
 		rip = kvm_rip_read(vcpu);
 		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 		kvm_rip_write(vcpu, rip);
@@ -5847,7 +5847,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 	u32 vectoring_info = vmx->idt_vectoring_info;
 	u16 exit_reason;
 
-	trace_kvm_exit(vmx->exit_reason, vcpu, KVM_ISA_VMX);
+	trace_kvm_exit(vmx->exit_reason.full, vcpu, KVM_ISA_VMX);
 
 	/*
 	 * Flush logged GPAs PML buffer, this will make dirty_bitmap more
@@ -5866,11 +5866,11 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 	if (is_guest_mode(vcpu) && nested_vmx_reflect_vmexit(vcpu))
 		return 1;
 
-	if (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
+	if (vmx->exit_reason.failed_vmentry) {
 		dump_vmcs();
 		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
 		vcpu->run->fail_entry.hardware_entry_failure_reason
-			= vmx->exit_reason;
+			= vmx->exit_reason.full;
 		return 0;
 	}
 
@@ -5882,7 +5882,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 		return 0;
 	}
 
-	exit_reason = vmx->exit_reason;
+	exit_reason = vmx->exit_reason.basic;
 
 	/*
 	 * Note:
@@ -5900,7 +5900,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
 		vcpu->run->internal.ndata = 3;
 		vcpu->run->internal.data[0] = vectoring_info;
-		vcpu->run->internal.data[1] = vmx->exit_reason;
+		vcpu->run->internal.data[1] = vmx->exit_reason.full;
 		vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
 		if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
 			vcpu->run->internal.ndata++;
@@ -5960,13 +5960,13 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 
 unexpected_vmexit:
 	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n",
-		    vmx->exit_reason);
+		    vmx->exit_reason.full);
 	dump_vmcs();
 	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 	vcpu->run->internal.suberror =
 			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
 	vcpu->run->internal.ndata = 1;
-	vcpu->run->internal.data[0] = vmx->exit_reason;
+	vcpu->run->internal.data[0] = vmx->exit_reason.full;
 	return 0;
 }
 
@@ -6290,7 +6290,7 @@ static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
 				   enum exit_fastpath_completion *exit_fastpath)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	u16 exit_reason = vmx->exit_reason;
+	u16 exit_reason = vmx->exit_reason.basic;
 
 	if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
 		handle_external_interrupt_irqoff(vcpu);
@@ -6672,11 +6672,11 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	vmx->nested.nested_run_pending = 0;
 	vmx->idt_vectoring_info = 0;
 
-	vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
-	if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
+	vmx->exit_reason.full = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
+	if (vmx->exit_reason.basic == EXIT_REASON_MCE_DURING_VMENTRY)
 		kvm_machine_check();
 
-	if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
+	if (vmx->fail || vmx->exit_reason.failed_vmentry)
 		return;
 
 	vmx->loaded_vmcs->launched = 1;
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index e64da06c7009..2d9a005d11ab 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -93,6 +93,29 @@ struct pt_desc {
 	struct pt_ctx guest;
 };
 
+union vmx_exit_reason {
+	struct {
+		u32	basic			: 16;
+		u32	reserved16		: 1;
+		u32	reserved17		: 1;
+		u32	reserved18		: 1;
+		u32	reserved19		: 1;
+		u32	reserved20		: 1;
+		u32	reserved21		: 1;
+		u32	reserved22		: 1;
+		u32	reserved23		: 1;
+		u32	reserved24		: 1;
+		u32	reserved25		: 1;
+		u32	reserved26		: 1;
+		u32	enclave_mode		: 1;
+		u32	smi_pending_mtf		: 1;
+		u32	smi_from_vmx_root	: 1;
+		u32	reserved30		: 1;
+		u32	failed_vmentry		: 1;
+	};
+	u32 full;
+};
+
 /*
  * The nested_vmx structure is part of vcpu_vmx, and holds information we need
  * for correct emulation of VMX (i.e., nested VMX) on this vcpu.
@@ -263,7 +286,7 @@ struct vcpu_vmx {
 	int vpid;
 	bool emulation_required;
 
-	u32 exit_reason;
+	union vmx_exit_reason exit_reason;
 
 	/* Posted interrupt descriptor */
 	struct pi_desc pi_desc;
-- 
2.24.1

Re: [PATCH 01/10] KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit()

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Move the call to nested_vmx_exit_reflected() from vmx_handle_exit() into
> nested_vmx_reflect_vmexit() and change the semantics of the return value
> for nested_vmx_reflect_vmexit() to indicate whether or not the exit was
> reflected into L1.  nested_vmx_exit_reflected() and
> nested_vmx_reflect_vmexit() are intrinsically tied together, calling one
> without simultaneously calling the other makes little sense.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.h | 16 +++++++++++-----
>  arch/x86/kvm/vmx/vmx.c    |  4 ++--
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index 21d36652f213..6bc379cf4755 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -72,12 +72,16 @@ static inline bool nested_ept_ad_enabled(struct kvm_vcpu *vcpu)
>  }
>  
>  /*
> - * Reflect a VM Exit into L1.
> + * Conditionally reflect a VM-Exit into L1.  Returns %true if the VM-Exit was
> + * reflected into L1.
>   */
> -static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
> -					    u32 exit_reason)
> +static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
> +					     u32 exit_reason)
>  {
> -	u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
> +	u32 exit_intr_info;
> +
> +	if (!nested_vmx_exit_reflected(vcpu, exit_reason))
> +		return false;

(unrelated to your patch)

It's probably just me but 'nested_vmx_exit_reflected()' name always
makes me thinkg 'the vmexit WAS [already] reflected' and not 'the vmexit
NEEDS to be reflected'. 'nested_vmx_exit_needs_reflecting()' maybe?

>  
>  	/*
>  	 * At this point, the exit interruption info in exit_intr_info
> @@ -85,6 +89,8 @@ static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
>  	 * we need to query the in-kernel LAPIC.
>  	 */
>  	WARN_ON(exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT);
> +
> +	exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
>  	if ((exit_intr_info &
>  	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==
>  	    (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) {
> @@ -96,7 +102,7 @@ static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
>  
>  	nested_vmx_vmexit(vcpu, exit_reason, exit_intr_info,
>  			  vmcs_readl(EXIT_QUALIFICATION));
> -	return 1;
> +	return true;
>  }
>  
>  /*
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 57742ddfd854..c1caac7e8f57 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5863,8 +5863,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_reflected(vcpu, exit_reason))
> -		return nested_vmx_reflect_vmexit(vcpu, exit_reason);
> +	if (is_guest_mode(vcpu) && nested_vmx_reflect_vmexit(vcpu, exit_reason))
> +		return 1;
>  
>  	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
>  		dump_vmcs();

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 02/10] KVM: nVMX: Drop a superfluous WARN on reflecting EXTERNAL_INTERRUPT

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Drop the WARN in nested_vmx_reflect_vmexit() that fires if KVM attempts
> to reflect an external interrupt.  nested_vmx_exit_reflected() is now
> called from nested_vmx_reflect_vmexit() and unconditionally returns
> false for EXTERNAL_INTERRUPT, i.e. barring a compiler or major CPU bug,
> the WARN will never fire.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index 6bc379cf4755..8f5ff3e259c9 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -84,12 +84,11 @@ static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
>  		return false;
>  
>  	/*
> -	 * 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.
> +	 * vmcs.VM_EXIT_INTR_INFO is only valid for EXCEPTION_NMI exits.  For
> +	 * EXTERNAL_INTERRUPT, the value for vmcs12->vm_exit_intr_info would
> +	 * need to be synthesized by querying the in-kernel LAPIC, but external
> +	 * interrupts are never reflected to L1 so it's a non-issue.
>  	 */
> -	WARN_ON(exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT);
> -
>  	exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
>  	if ((exit_intr_info &
>  	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 03/10] KVM: nVMX: Pull exit_reason from vcpu_vmx in nested_vmx_exit_reflected()

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Grab the exit reason from the vcpu struct in nested_vmx_exit_reflected()
> instead of having the exit reason explicitly passed from the caller.
> This fixes a discrepancy between VM-Fail and VM-Exit handling, as the
> VM-Fail case is already handled by checking vcpu_vmx, e.g. the exit
> reason previously passed on the stack is bogus if vmx->fail is set.

It's 0xdead, not bogus :-)

>
> Not taking the exit reason on the stack also avoids having to document
> that nested_vmx_exit_reflected() requires the full exit reason, as
> opposed to just the basic exit reason, which is not at all obvious since
> the only usage of the full exit reason is for tracing.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 3 ++-
>  arch/x86/kvm/vmx/nested.h | 9 ++++-----
>  arch/x86/kvm/vmx/vmx.c    | 2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 79c7764c77b1..cb05bcbbfc4e 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5518,11 +5518,12 @@ static bool nested_vmx_exit_handled_vmcs_access(struct kvm_vcpu *vcpu,
>   * should handle it ourselves in L0 (and then continue L2). Only call this
>   * when in is_guest_mode (L2).
>   */
> -bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
> +bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu)
>  {
>  	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;
>  
>  	if (vmx->nested.nested_run_pending)
>  		return false;
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index 8f5ff3e259c9..569cb828b6ca 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -24,7 +24,7 @@ void nested_vmx_set_vmcs_shadowing_bitmap(void);
>  void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu);
>  enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
>  						     bool from_vmentry);
> -bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason);
> +bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu);
>  void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>  		       u32 exit_intr_info, unsigned long exit_qualification);
>  void nested_sync_vmcs12_to_shadow(struct kvm_vcpu *vcpu);
> @@ -75,12 +75,11 @@ static inline bool nested_ept_ad_enabled(struct kvm_vcpu *vcpu)
>   * Conditionally reflect a VM-Exit into L1.  Returns %true if the VM-Exit was
>   * reflected into L1.
>   */
> -static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
> -					     u32 exit_reason)
> +static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
>  {
>  	u32 exit_intr_info;
>  
> -	if (!nested_vmx_exit_reflected(vcpu, exit_reason))
> +	if (!nested_vmx_exit_reflected(vcpu))
>  		return false;
>  
>  	/*
> @@ -99,7 +98,7 @@ static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
>  			vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
>  	}
>  
> -	nested_vmx_vmexit(vcpu, exit_reason, exit_intr_info,
> +	nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, exit_intr_info,
>  			  vmcs_readl(EXIT_QUALIFICATION));
>  	return true;
>  }
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index c1caac7e8f57..c7715c880ea7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5863,7 +5863,7 @@ 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_reflect_vmexit(vcpu, exit_reason))
> +	if (is_guest_mode(vcpu) && nested_vmx_reflect_vmexit(vcpu))
>  		return 1;
>  
>  	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 04/10] KVM: VMX: Convert local exit_reason to u16 in nested_vmx_exit_reflected()

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Store only the basic exit reason in the local "exit_reason" variable in
> nested_vmx_exit_reflected().  Except for tracing, all references to
> exit_reason are expecting to encounter only the basic exit reason.
>
> Opportunistically align the params to nested_vmx_exit_handled_msr().
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index cb05bcbbfc4e..1848ca0116c0 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5374,7 +5374,7 @@ static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
>   * MSR bitmap. This may be the case even when L0 doesn't use MSR bitmaps.
>   */
>  static bool nested_vmx_exit_handled_msr(struct kvm_vcpu *vcpu,
> -	struct vmcs12 *vmcs12, u32 exit_reason)
> +					struct vmcs12 *vmcs12, u16 exit_reason)
>  {
>  	u32 msr_index = kvm_rcx_read(vcpu);
>  	gpa_t bitmap;
> @@ -5523,7 +5523,7 @@ bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu)
>  	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;
> +	u16 exit_reason;
>  
>  	if (vmx->nested.nested_run_pending)
>  		return false;
> @@ -5548,13 +5548,15 @@ bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu)
>  	 */
>  	nested_mark_vmcs12_pages_dirty(vcpu);
>  
> -	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), exit_reason,
> +	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), vmx->exit_reason,
>  				vmcs_readl(EXIT_QUALIFICATION),
>  				vmx->idt_vectoring_info,
>  				intr_info,
>  				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
>  				KVM_ISA_VMX);
>  
> +	exit_reason = vmx->exit_reason;
> +
>  	switch (exit_reason) {
>  	case EXIT_REASON_EXCEPTION_NMI:
>  		if (is_nmi(intr_info))

If the patch is looked at by itself (and not as part of the series) one
may ask to add a comment explaining that we do the trunctation
deliberately but with all patches of the series it is superfluous.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 05/10] KVM: VMX: Convert local exit_reason to u16 in vmx_handle_exit()

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Convert the local "exit_reason" in vmx_handle_exit() from a u32 to a u16
> as most references expect to encounter only the basic exit reason.  Use
> vmx->exit_reason directly for paths that expect the full exit reason,
> i.e. to avoid having to figure out a decent name for a second local
> variable.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index c7715c880ea7..d43e1d28bb58 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5844,10 +5844,10 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  	enum exit_fastpath_completion exit_fastpath)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> -	u32 exit_reason = vmx->exit_reason;
>  	u32 vectoring_info = vmx->idt_vectoring_info;
> +	u16 exit_reason;
>  
> -	trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
> +	trace_kvm_exit(vmx->exit_reason, vcpu, KVM_ISA_VMX);
>  
>  	/*
>  	 * Flush logged GPAs PML buffer, this will make dirty_bitmap more
> @@ -5866,11 +5866,11 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  	if (is_guest_mode(vcpu) && nested_vmx_reflect_vmexit(vcpu))
>  		return 1;
>  
> -	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
> +	if (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
>  		dump_vmcs();
>  		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>  		vcpu->run->fail_entry.hardware_entry_failure_reason
> -			= exit_reason;
> +			= vmx->exit_reason;
>  		return 0;
>  	}
>  
> @@ -5882,6 +5882,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  		return 0;
>  	}
>  
> +	exit_reason = vmx->exit_reason;
> +
>  	/*
>  	 * Note:
>  	 * Do not try to fix EXIT_REASON_EPT_MISCONFIG if it caused by
> @@ -5898,7 +5900,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
>  		vcpu->run->internal.ndata = 3;
>  		vcpu->run->internal.data[0] = vectoring_info;
> -		vcpu->run->internal.data[1] = exit_reason;
> +		vcpu->run->internal.data[1] = vmx->exit_reason;
>  		vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
>  		if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>  			vcpu->run->internal.ndata++;
> @@ -5957,13 +5959,14 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  	return kvm_vmx_exit_handlers[exit_reason](vcpu);
>  
>  unexpected_vmexit:
> -	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
> +	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n",
> +		    vmx->exit_reason);
>  	dump_vmcs();
>  	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  	vcpu->run->internal.suberror =
>  			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
>  	vcpu->run->internal.ndata = 1;
> -	vcpu->run->internal.data[0] = exit_reason;
> +	vcpu->run->internal.data[0] = vmx->exit_reason;
>  	return 0;
>  }

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 06/10] KVM: nVMX: Convert local exit_reason to u16 in ...enter_non_root_mode()

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Use a u16 for nested_vmx_enter_non_root_mode()'s local "exit_reason" to
> make it clear the intermediate code is only responsible for setting the
> basic exit reason, e.g. FAILED_VMENTRY is unconditionally OR'd in when
> emulating a failed VM-Entry.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 1848ca0116c0..8fbbe2152ab7 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3182,7 +3182,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
>  	bool evaluate_pending_interrupts;
> -	u32 exit_reason = EXIT_REASON_INVALID_STATE;
> +	u16 exit_reason = EXIT_REASON_INVALID_STATE;
>  	u32 exit_qual;
>  
>  	evaluate_pending_interrupts = exec_controls_get(vmx) &
> @@ -3308,7 +3308,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
>  		return NVMX_VMENTRY_VMEXIT;
>  
>  	load_vmcs12_host_state(vcpu, vmcs12);
> -	vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
> +	vmcs12->vm_exit_reason = VMX_EXIT_REASONS_FAILED_VMENTRY | exit_reason;

My personal preference would be to do
 (u32)exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY 
instead but maybe I'm just not in love with implicit type convertion in C.

>  	vmcs12->exit_qualification = exit_qual;
>  	if (enable_shadow_vmcs || vmx->nested.hv_evmcs)
>  		vmx->nested.need_vmcs12_to_shadow_sync = true;

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 07/10] KVM: nVMX: Cast exit_reason to u16 to check for nested EXTERNAL_INTERRUPT

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Explicitly check only the basic exit reason when emulating an external
> interrupt VM-Exit in nested_vmx_vmexit().  Checking the full exit reason
> doesn't currently cause problems, but only because the only exit reason
> modifier support by KVM is FAILED_VMENTRY, which is mutually exclusive
> with EXTERNAL_INTERRUPT.  Future modifiers, e.g. ENCLAVE_MODE, will
> coexist with EXTERNAL_INTERRUPT.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 8fbbe2152ab7..86b12a2918c5 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4337,7 +4337,7 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>  	vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>  
>  	if (likely(!vmx->fail)) {
> -		if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> +		if ((u16)exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
>  		    nested_exit_intr_ack_set(vcpu)) {
>  			int irq = kvm_cpu_get_interrupt(vcpu);
>  			WARN_ON(irq < 0);

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

RE: [PATCH 06/10] KVM: nVMX: Convert local exit_reason to u16 in ...enter_non_root_mode()

[David Laight]

From: Vitaly Kuznetsov
> Sent: 13 March 2020 13:56
...
> >  	load_vmcs12_host_state(vcpu, vmcs12);
> > -	vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
> > +	vmcs12->vm_exit_reason = VMX_EXIT_REASONS_FAILED_VMENTRY | exit_reason;
> 
> My personal preference would be to do
>  (u32)exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY
> instead but maybe I'm just not in love with implicit type convertion in C.

I look at the cast and wonder what is going on :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 08/10] KVM: nVMX: Rename exit_reason to vm_exit_reason for nested VM-Exit

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Use "vm_exit_reason" when passing around the full exit reason for nested
> VM-Exits to make it clear that it's not just the basic exit reason.  The
> basic exit reason (bits 15:0 of vmcs.VM_EXIT_REASON) is colloquially
> referred to as simply "exit reason", e.g. vmx_handle_vmexit() tracks the
> basic exit reason in a local variable named "exit_reason".
>

Would it make sense to stop using 'exit_reason' without a prefix (full,
basic,...) completely?

> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 29 +++++++++++++++--------------
>  arch/x86/kvm/vmx/nested.h |  2 +-
>  2 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 86b12a2918c5..c775feca3eb0 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -328,19 +328,19 @@ static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
>  {
>  	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> -	u32 exit_reason;
> +	u32 vm_exit_reason;
>  	unsigned long exit_qualification = vcpu->arch.exit_qualification;
>  
>  	if (vmx->nested.pml_full) {
> -		exit_reason = EXIT_REASON_PML_FULL;
> +		vm_exit_reason = EXIT_REASON_PML_FULL;
>  		vmx->nested.pml_full = false;
>  		exit_qualification &= INTR_INFO_UNBLOCK_NMI;
>  	} else if (fault->error_code & PFERR_RSVD_MASK)
> -		exit_reason = EXIT_REASON_EPT_MISCONFIG;
> +		vm_exit_reason = EXIT_REASON_EPT_MISCONFIG;
>  	else
> -		exit_reason = EXIT_REASON_EPT_VIOLATION;
> +		vm_exit_reason = EXIT_REASON_EPT_VIOLATION;
>  
> -	nested_vmx_vmexit(vcpu, exit_reason, 0, exit_qualification);
> +	nested_vmx_vmexit(vcpu, vm_exit_reason, 0, exit_qualification);
>  	vmcs12->guest_physical_address = fault->address;
>  }
>  
> @@ -3919,11 +3919,11 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>   * which already writes to vmcs12 directly.
>   */
>  static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> -			   u32 exit_reason, u32 exit_intr_info,
> +			   u32 vm_exit_reason, u32 exit_intr_info,
>  			   unsigned long exit_qualification)
>  {
>  	/* update exit information fields: */
> -	vmcs12->vm_exit_reason = exit_reason;
> +	vmcs12->vm_exit_reason = vm_exit_reason;
>  	vmcs12->exit_qualification = exit_qualification;
>  	vmcs12->vm_exit_intr_info = exit_intr_info;
>  
> @@ -4252,7 +4252,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
>   * and modify vmcs12 to make it see what it would expect to see there if
>   * L2 was its real guest. Must only be called when in L2 (is_guest_mode())
>   */
> -void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
> +void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
>  		       u32 exit_intr_info, unsigned long exit_qualification)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -4272,9 +4272,9 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>  	if (likely(!vmx->fail)) {
>  		sync_vmcs02_to_vmcs12(vcpu, vmcs12);
>  
> -		if (exit_reason != -1)
> -			prepare_vmcs12(vcpu, vmcs12, exit_reason, exit_intr_info,
> -				       exit_qualification);
> +		if (vm_exit_reason != -1)
> +			prepare_vmcs12(vcpu, vmcs12, vm_exit_reason,
> +				       exit_intr_info, exit_qualification);
>  
>  		/*
>  		 * Must happen outside of sync_vmcs02_to_vmcs12() as it will
> @@ -4330,14 +4330,15 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>  	 */
>  	kvm_make_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu);
>  
> -	if ((exit_reason != -1) && (enable_shadow_vmcs || vmx->nested.hv_evmcs))
> +	if ((vm_exit_reason != -1) &&
> +	    (enable_shadow_vmcs || vmx->nested.hv_evmcs))
>  		vmx->nested.need_vmcs12_to_shadow_sync = true;
>  
>  	/* in case we halted in L2 */
>  	vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>  
>  	if (likely(!vmx->fail)) {
> -		if ((u16)exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> +		if ((u16)vm_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
>  		    nested_exit_intr_ack_set(vcpu)) {
>  			int irq = kvm_cpu_get_interrupt(vcpu);
>  			WARN_ON(irq < 0);
> @@ -4345,7 +4346,7 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>  				INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
>  		}
>  
> -		if (exit_reason != -1)
> +		if (vm_exit_reason != -1)
>  			trace_kvm_nested_vmexit_inject(vmcs12->vm_exit_reason,
>  						       vmcs12->exit_qualification,
>  						       vmcs12->idt_vectoring_info_field,
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index 569cb828b6ca..04584bcbcc8d 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -25,7 +25,7 @@ void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu);
>  enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
>  						     bool from_vmentry);
>  bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu);
> -void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
> +void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
>  		       u32 exit_intr_info, unsigned long exit_qualification);
>  void nested_sync_vmcs12_to_shadow(struct kvm_vcpu *vcpu);
>  int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);

Reviewded-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 09/10] KVM: VMX: Cache vmx->exit_reason in local u16 in vmx_handle_exit_irqoff()

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Use a u16 to hold the exit reason in vmx_handle_exit_irqoff(), as the
> checks for INTR/NMI/WRMSR expect to encounter only the basic exit reason
> in vmx->exit_reason.
>

True Sean would also add:

"No functional change intended."

"Opportunistically align the params to handle_external_interrupt_irqoff()."

> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index d43e1d28bb58..910a7cadeaf7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6287,16 +6287,16 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu)
>  STACK_FRAME_NON_STANDARD(handle_external_interrupt_irqoff);
>  
>  static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
> -	enum exit_fastpath_completion *exit_fastpath)
> +				   enum exit_fastpath_completion *exit_fastpath)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	u16 exit_reason = vmx->exit_reason;
>  
> -	if (vmx->exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
> +	if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>  		handle_external_interrupt_irqoff(vcpu);
> -	else if (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI)
> +	else if (exit_reason == EXIT_REASON_EXCEPTION_NMI)
>  		handle_exception_nmi_irqoff(vmx);
> -	else if (!is_guest_mode(vcpu) &&
> -		vmx->exit_reason == EXIT_REASON_MSR_WRITE)
> +	else if (!is_guest_mode(vcpu) && exit_reason == EXIT_REASON_MSR_WRITE)
>  		*exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
>  }

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 10/10] KVM: VMX: Convert vcpu_vmx.exit_reason to a union

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Convert vcpu_vmx.exit_reason from a u32 to a union (of size u32).  The
> full VM_EXIT_REASON field is comprised of a 16-bit basic exit reason in
> bits 15:0, and single-bit modifiers in bits 31:16.
>
> Historically, KVM has only had to worry about handling the "failed
> VM-Entry" modifier, which could only be set in very specific flows and
> required dedicated handling.  I.e. manually stripping the FAILED_VMENTRY
> bit was a somewhat viable approach.  But even with only a single bit to
> worry about, KVM has had several bugs related to comparing a basic exit
> reason against the full exit reason stored in vcpu_vmx.
>
> Upcoming Intel features, e.g. SGX, will add new modifier bits that can
> be set on more or less any VM-Exit, as opposed to the significantly more
> restricted FAILED_VMENTRY, i.e. correctly handling everything in one-off
> flows isn't scalable.  Tracking exit reason in a union forces code to
> explicitly choose between consuming the full exit reason and the basic
> exit reason, and is a convenient way to document and access the
> modifiers.
>
> No functional change intended.
>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 11 ++++++++---
>  arch/x86/kvm/vmx/nested.h |  2 +-
>  arch/x86/kvm/vmx/vmx.c    | 24 ++++++++++++------------
>  arch/x86/kvm/vmx/vmx.h    | 25 ++++++++++++++++++++++++-
>  4 files changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index c775feca3eb0..0c7cea35dd33 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5307,7 +5307,12 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
>  	return kvm_skip_emulated_instruction(vcpu);
>  
>  fail:
> -	nested_vmx_vmexit(vcpu, vmx->exit_reason,
> +	/*
> +	 * This is effectively a reflected VM-Exit, as opposed to a synthesized
> +	 * nested VM-Exit.  Pass the original exit reason, i.e. don't hardcode
> +	 * EXIT_REASON_VMFUNC as the exit reason.
> +	 */
> +	nested_vmx_vmexit(vcpu, vmx->exit_reason.full,
>  			  vmcs_read32(VM_EXIT_INTR_INFO),
>  			  vmcs_readl(EXIT_QUALIFICATION));
>  	return 1;
> @@ -5549,14 +5554,14 @@ bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu)
>  	 */
>  	nested_mark_vmcs12_pages_dirty(vcpu);
>  
> -	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), vmx->exit_reason,
> +	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), vmx->exit_reason.full,
>  				vmcs_readl(EXIT_QUALIFICATION),
>  				vmx->idt_vectoring_info,
>  				intr_info,
>  				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
>  				KVM_ISA_VMX);
>  
> -	exit_reason = vmx->exit_reason;
> +	exit_reason = vmx->exit_reason.basic;
>  
>  	switch (exit_reason) {
>  	case EXIT_REASON_EXCEPTION_NMI:
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index 04584bcbcc8d..07ce09f88977 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -98,7 +98,7 @@ static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
>  			vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
>  	}
>  
> -	nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, exit_intr_info,
> +	nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason.full, exit_intr_info,
>  			  vmcs_readl(EXIT_QUALIFICATION));
>  	return true;
>  }
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 910a7cadeaf7..521b99f63608 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1588,7 +1588,7 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	 * i.e. we end up advancing IP with some random value.
>  	 */
>  	if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
> -	    to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
> +	    to_vmx(vcpu)->exit_reason.basic != EXIT_REASON_EPT_MISCONFIG) {
>  		rip = kvm_rip_read(vcpu);
>  		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
>  		kvm_rip_write(vcpu, rip);
> @@ -5847,7 +5847,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  	u32 vectoring_info = vmx->idt_vectoring_info;
>  	u16 exit_reason;
>  
> -	trace_kvm_exit(vmx->exit_reason, vcpu, KVM_ISA_VMX);
> +	trace_kvm_exit(vmx->exit_reason.full, vcpu, KVM_ISA_VMX);
>  
>  	/*
>  	 * Flush logged GPAs PML buffer, this will make dirty_bitmap more
> @@ -5866,11 +5866,11 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  	if (is_guest_mode(vcpu) && nested_vmx_reflect_vmexit(vcpu))
>  		return 1;
>  
> -	if (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
> +	if (vmx->exit_reason.failed_vmentry) {
>  		dump_vmcs();
>  		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>  		vcpu->run->fail_entry.hardware_entry_failure_reason
> -			= vmx->exit_reason;
> +			= vmx->exit_reason.full;
>  		return 0;
>  	}
>  
> @@ -5882,7 +5882,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  		return 0;
>  	}
>  
> -	exit_reason = vmx->exit_reason;
> +	exit_reason = vmx->exit_reason.basic;
>  
>  	/*
>  	 * Note:
> @@ -5900,7 +5900,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
>  		vcpu->run->internal.ndata = 3;
>  		vcpu->run->internal.data[0] = vectoring_info;
> -		vcpu->run->internal.data[1] = vmx->exit_reason;
> +		vcpu->run->internal.data[1] = vmx->exit_reason.full;
>  		vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
>  		if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>  			vcpu->run->internal.ndata++;
> @@ -5960,13 +5960,13 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  
>  unexpected_vmexit:
>  	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n",
> -		    vmx->exit_reason);
> +		    vmx->exit_reason.full);
>  	dump_vmcs();
>  	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  	vcpu->run->internal.suberror =
>  			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
>  	vcpu->run->internal.ndata = 1;
> -	vcpu->run->internal.data[0] = vmx->exit_reason;
> +	vcpu->run->internal.data[0] = vmx->exit_reason.full;
>  	return 0;
>  }
>  
> @@ -6290,7 +6290,7 @@ static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
>  				   enum exit_fastpath_completion *exit_fastpath)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> -	u16 exit_reason = vmx->exit_reason;
> +	u16 exit_reason = vmx->exit_reason.basic;
>  
>  	if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>  		handle_external_interrupt_irqoff(vcpu);
> @@ -6672,11 +6672,11 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  	vmx->nested.nested_run_pending = 0;
>  	vmx->idt_vectoring_info = 0;
>  
> -	vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> -	if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
> +	vmx->exit_reason.full = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> +	if (vmx->exit_reason.basic == EXIT_REASON_MCE_DURING_VMENTRY)
>  		kvm_machine_check();
>  
> -	if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
> +	if (vmx->fail || vmx->exit_reason.failed_vmentry)
>  		return;
>  
>  	vmx->loaded_vmcs->launched = 1;
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index e64da06c7009..2d9a005d11ab 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -93,6 +93,29 @@ struct pt_desc {
>  	struct pt_ctx guest;
>  };
>  
> +union vmx_exit_reason {
> +	struct {
> +		u32	basic			: 16;
> +		u32	reserved16		: 1;
> +		u32	reserved17		: 1;
> +		u32	reserved18		: 1;
> +		u32	reserved19		: 1;
> +		u32	reserved20		: 1;
> +		u32	reserved21		: 1;
> +		u32	reserved22		: 1;
> +		u32	reserved23		: 1;
> +		u32	reserved24		: 1;
> +		u32	reserved25		: 1;
> +		u32	reserved26		: 1;
> +		u32	enclave_mode		: 1;
> +		u32	smi_pending_mtf		: 1;
> +		u32	smi_from_vmx_root	: 1;
> +		u32	reserved30		: 1;
> +		u32	failed_vmentry		: 1;

Just wondering, is there any particular benefit in using 'u32' instead
of 'u16' here?

> +	};
> +	u32 full;
> +};
> +
>  /*
>   * The nested_vmx structure is part of vcpu_vmx, and holds information we need
>   * for correct emulation of VMX (i.e., nested VMX) on this vcpu.
> @@ -263,7 +286,7 @@ struct vcpu_vmx {
>  	int vpid;
>  	bool emulation_required;
>  
> -	u32 exit_reason;
> +	union vmx_exit_reason exit_reason;
>  
>  	/* Posted interrupt descriptor */
>  	struct pi_desc pi_desc;

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH 08/10] KVM: nVMX: Rename exit_reason to vm_exit_reason for nested VM-Exit

[Sean Christopherson]

On Fri, Mar 13, 2020 at 03:01:55PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > Use "vm_exit_reason" when passing around the full exit reason for nested
> > VM-Exits to make it clear that it's not just the basic exit reason.  The
> > basic exit reason (bits 15:0 of vmcs.VM_EXIT_REASON) is colloquially
> > referred to as simply "exit reason", e.g. vmx_handle_vmexit() tracks the
> > basic exit reason in a local variable named "exit_reason".
> >
> 
> Would it make sense to stop using 'exit_reason' without a prefix (full,
> basic,...) completely?

I'd prefer to keep using exit_reason as a local variable.  Either that or
grab the whole union locally and use "exit_reason.basic".  

IMO, referring to the basic exit reason as simply "exit reason" is so
pervasive that it's reasonable to use exit_reason as a local variable.

Re: [PATCH 10/10] KVM: VMX: Convert vcpu_vmx.exit_reason to a union

[Sean Christopherson]

On Fri, Mar 13, 2020 at 03:18:09PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index e64da06c7009..2d9a005d11ab 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -93,6 +93,29 @@ struct pt_desc {
> >  	struct pt_ctx guest;
> >  };
> >  
> > +union vmx_exit_reason {
> > +	struct {
> > +		u32	basic			: 16;
> > +		u32	reserved16		: 1;
> > +		u32	reserved17		: 1;
> > +		u32	reserved18		: 1;
> > +		u32	reserved19		: 1;
> > +		u32	reserved20		: 1;
> > +		u32	reserved21		: 1;
> > +		u32	reserved22		: 1;
> > +		u32	reserved23		: 1;
> > +		u32	reserved24		: 1;
> > +		u32	reserved25		: 1;
> > +		u32	reserved26		: 1;
> > +		u32	enclave_mode		: 1;
> > +		u32	smi_pending_mtf		: 1;
> > +		u32	smi_from_vmx_root	: 1;
> > +		u32	reserved30		: 1;
> > +		u32	failed_vmentry		: 1;
> 
> Just wondering, is there any particular benefit in using 'u32' instead
> of 'u16' here?

Not that I know of.  Paranoia that the compiler will do something weird?

> > +	};
> > +	u32 full;
> > +};
> > +
> >  /*
> >   * The nested_vmx structure is part of vcpu_vmx, and holds information we need
> >   * for correct emulation of VMX (i.e., nested VMX) on this vcpu.
> > @@ -263,7 +286,7 @@ struct vcpu_vmx {
> >  	int vpid;
> >  	bool emulation_required;
> >  
> > -	u32 exit_reason;
> > +	union vmx_exit_reason exit_reason;
> >  
> >  	/* Posted interrupt descriptor */
> >  	struct pi_desc pi_desc;
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> -- 
> Vitaly
> 

Re: [PATCH 06/10] KVM: nVMX: Convert local exit_reason to u16 in ...enter_non_root_mode()

[Sean Christopherson]

On Fri, Mar 13, 2020 at 02:55:38PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > Use a u16 for nested_vmx_enter_non_root_mode()'s local "exit_reason" to
> > make it clear the intermediate code is only responsible for setting the
> > basic exit reason, e.g. FAILED_VMENTRY is unconditionally OR'd in when
> > emulating a failed VM-Entry.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 1848ca0116c0..8fbbe2152ab7 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -3182,7 +3182,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
> >  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> >  	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> >  	bool evaluate_pending_interrupts;
> > -	u32 exit_reason = EXIT_REASON_INVALID_STATE;
> > +	u16 exit_reason = EXIT_REASON_INVALID_STATE;
> >  	u32 exit_qual;
> >  
> >  	evaluate_pending_interrupts = exec_controls_get(vmx) &
> > @@ -3308,7 +3308,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
> >  		return NVMX_VMENTRY_VMEXIT;
> >  
> >  	load_vmcs12_host_state(vcpu, vmcs12);
> > -	vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
> > +	vmcs12->vm_exit_reason = VMX_EXIT_REASONS_FAILED_VMENTRY | exit_reason;
> 
> My personal preference would be to do
>  (u32)exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY 
> instead but maybe I'm just not in love with implicit type convertion in C.

Either way works for me.  Paolo?

Re: [PATCH 01/10] KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit()

[Sean Christopherson]

On Fri, Mar 13, 2020 at 01:12:33PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > Move the call to nested_vmx_exit_reflected() from vmx_handle_exit() into
> > nested_vmx_reflect_vmexit() and change the semantics of the return value
> > for nested_vmx_reflect_vmexit() to indicate whether or not the exit was
> > reflected into L1.  nested_vmx_exit_reflected() and
> > nested_vmx_reflect_vmexit() are intrinsically tied together, calling one
> > without simultaneously calling the other makes little sense.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/vmx/nested.h | 16 +++++++++++-----
> >  arch/x86/kvm/vmx/vmx.c    |  4 ++--
> >  2 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> > index 21d36652f213..6bc379cf4755 100644
> > --- a/arch/x86/kvm/vmx/nested.h
> > +++ b/arch/x86/kvm/vmx/nested.h
> > @@ -72,12 +72,16 @@ static inline bool nested_ept_ad_enabled(struct kvm_vcpu *vcpu)
> >  }
> >  
> >  /*
> > - * Reflect a VM Exit into L1.
> > + * Conditionally reflect a VM-Exit into L1.  Returns %true if the VM-Exit was
> > + * reflected into L1.
> >   */
> > -static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
> > -					    u32 exit_reason)
> > +static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
> > +					     u32 exit_reason)
> >  {
> > -	u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
> > +	u32 exit_intr_info;
> > +
> > +	if (!nested_vmx_exit_reflected(vcpu, exit_reason))
> > +		return false;
> 
> (unrelated to your patch)
> 
> It's probably just me but 'nested_vmx_exit_reflected()' name always
> makes me thinkg 'the vmexit WAS [already] reflected' and not 'the vmexit
> NEEDS to be reflected'. 'nested_vmx_exit_needs_reflecting()' maybe?

Not just you.  It'd be nice if the name some how reflected (ha) that the
logic is mostly based on whether or not L1 expects the exit, with a few
exceptions.  E.g. something like

	if (!l1_expects_vmexit(...) && !is_system_vmexit(...))
		return false;

The downside of that is the logic is split, which is probably a net loss?

Re: [PATCH 01/10] KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit()

[Sean Christopherson]

On Mon, Mar 16, 2020 at 10:33:27PM -0700, Sean Christopherson wrote:
> On Fri, Mar 13, 2020 at 01:12:33PM +0100, Vitaly Kuznetsov wrote:
> > Sean Christopherson <sean.j.christopherson@intel.com> writes:
> > 
> > > -static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
> > > -					    u32 exit_reason)
> > > +static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
> > > +					     u32 exit_reason)
> > >  {
> > > -	u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
> > > +	u32 exit_intr_info;
> > > +
> > > +	if (!nested_vmx_exit_reflected(vcpu, exit_reason))
> > > +		return false;
> > 
> > (unrelated to your patch)
> > 
> > It's probably just me but 'nested_vmx_exit_reflected()' name always
> > makes me thinkg 'the vmexit WAS [already] reflected' and not 'the vmexit
> > NEEDS to be reflected'. 'nested_vmx_exit_needs_reflecting()' maybe?
> 
> Not just you.  It'd be nice if the name some how reflected (ha) that the
> logic is mostly based on whether or not L1 expects the exit, with a few
> exceptions.  E.g. something like
> 
> 	if (!l1_expects_vmexit(...) && !is_system_vmexit(...))
> 		return false;

Doh, the system VM-Exit logic is backwards, it should be

	if (!l1_expects_vmexit(...) || is_system_vmexit(...))
		return false;
> 
> The downside of that is the logic is split, which is probably a net loss?

Re: [PATCH 01/10] KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit()

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> On Mon, Mar 16, 2020 at 10:33:27PM -0700, Sean Christopherson wrote:
>> On Fri, Mar 13, 2020 at 01:12:33PM +0100, Vitaly Kuznetsov wrote:
>> > Sean Christopherson <sean.j.christopherson@intel.com> writes:
>> > 
>> > > -static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
>> > > -					    u32 exit_reason)
>> > > +static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
>> > > +					     u32 exit_reason)
>> > >  {
>> > > -	u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
>> > > +	u32 exit_intr_info;
>> > > +
>> > > +	if (!nested_vmx_exit_reflected(vcpu, exit_reason))
>> > > +		return false;
>> > 
>> > (unrelated to your patch)
>> > 
>> > It's probably just me but 'nested_vmx_exit_reflected()' name always
>> > makes me thinkg 'the vmexit WAS [already] reflected' and not 'the vmexit
>> > NEEDS to be reflected'. 'nested_vmx_exit_needs_reflecting()' maybe?
>> 
>> Not just you.  It'd be nice if the name some how reflected (ha) that the
>> logic is mostly based on whether or not L1 expects the exit, with a few
>> exceptions.  E.g. something like
>> 
>> 	if (!l1_expects_vmexit(...) && !is_system_vmexit(...))
>> 		return false;
>
> Doh, the system VM-Exit logic is backwards, it should be
>
> 	if (!l1_expects_vmexit(...) || is_system_vmexit(...))
> 		return false;
>> 
>> The downside of that is the logic is split, which is probably a net loss?
>

Yea,

(just thinking out loud below)

the problem with the split is that we'll have to handle the same exit
reason twice, e.g. EXIT_REASON_EXCEPTION_NMI (is_nmi() check goes to
is_system_vmexit() and vmcs12->exception_bitmap check goes to
l1_expects_vmexit()). Also, we have two 'special' cases: vmx->fail and
nested_run_pending. While the former belongs to to l1_expects_vmexit(),
the later doesn't belong to either (but we can move it to
nested_vmx_reflect_vmexit() I believe).

On the other hand, I'm a great fan of splitting checkers ('pure'
functions) from actors (functions with 'side-effects') and
nested_vmx_exit_reflected() while looking like a checker does a lot of
'acting': nested_mark_vmcs12_pages_dirty(), trace printk.

-- 
Vitaly

Re: [PATCH 01/10] KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit()

[Paolo Bonzini]

On 17/03/20 18:00, Vitaly Kuznetsov wrote:
> 
> On the other hand, I'm a great fan of splitting checkers ('pure'
> functions) from actors (functions with 'side-effects') and
> nested_vmx_exit_reflected() while looking like a checker does a lot of
> 'acting': nested_mark_vmcs12_pages_dirty(), trace printk.

Good idea (trace_printk is not a big deal, but
nested_mark_vmcs12_pages_dirty should be done outside).  I'll send a
patch, just to show that I can still write KVM code. :)

Paolo

Re: [PATCH 06/10] KVM: nVMX: Convert local exit_reason to u16 in ...enter_non_root_mode()

[Paolo Bonzini]

On 17/03/20 06:29, Sean Christopherson wrote:
>>>  
>>>  	load_vmcs12_host_state(vcpu, vmcs12);
>>> -	vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
>>> +	vmcs12->vm_exit_reason = VMX_EXIT_REASONS_FAILED_VMENTRY | exit_reason;
>> My personal preference would be to do
>>  (u32)exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY 
>> instead but maybe I'm just not in love with implicit type convertion in C.
> Either way works for me.  Paolo?
> 

Flip a coin? :)  I think your version is fine.

Paolo

Re: [PATCH 09/10] KVM: VMX: Cache vmx->exit_reason in local u16 in vmx_handle_exit_irqoff()

[Paolo Bonzini]

On 13/03/20 15:09, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
>> Use a u16 to hold the exit reason in vmx_handle_exit_irqoff(), as the
>> checks for INTR/NMI/WRMSR expect to encounter only the basic exit reason
>> in vmx->exit_reason.
>>
> True Sean would also add:
> 
> "No functional change intended."
> 
> "Opportunistically align the params to handle_external_interrupt_irqoff()."

Ahah that's perhaps a bit too much, but "no functional change intended"
makes sense.

Paolo

Re: [PATCH 10/10] KVM: VMX: Convert vcpu_vmx.exit_reason to a union

[Paolo Bonzini]

On 17/03/20 06:28, Sean Christopherson wrote:
>>>  
>>> +union vmx_exit_reason {
>>> +	struct {
>>> +		u32	basic			: 16;
>>> +		u32	reserved16		: 1;
>>> +		u32	reserved17		: 1;
>>> +		u32	reserved18		: 1;
>>> +		u32	reserved19		: 1;
>>> +		u32	reserved20		: 1;
>>> +		u32	reserved21		: 1;
>>> +		u32	reserved22		: 1;
>>> +		u32	reserved23		: 1;
>>> +		u32	reserved24		: 1;
>>> +		u32	reserved25		: 1;
>>> +		u32	reserved26		: 1;
>>> +		u32	enclave_mode		: 1;
>>> +		u32	smi_pending_mtf		: 1;
>>> +		u32	smi_from_vmx_root	: 1;
>>> +		u32	reserved30		: 1;
>>> +		u32	failed_vmentry		: 1;
>> Just wondering, is there any particular benefit in using 'u32' instead
>> of 'u16' here?
> Not that I know of.  Paranoia that the compiler will do something weird?

Since we have an "u32 full" it makes sense to have u32 here too, it
documents that we're matching an u32 field in the other side of the union.

Paolo

Re: [PATCH 01/10] KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit()

[Sean Christopherson]

On Tue, Mar 17, 2020 at 06:38:20PM +0100, Paolo Bonzini wrote:
> On 17/03/20 18:00, Vitaly Kuznetsov wrote:
> > 
> > On the other hand, I'm a great fan of splitting checkers ('pure'
> > functions) from actors (functions with 'side-effects') and
> > nested_vmx_exit_reflected() while looking like a checker does a lot of
> > 'acting': nested_mark_vmcs12_pages_dirty(), trace printk.
> 
> Good idea (trace_printk is not a big deal, but
> nested_mark_vmcs12_pages_dirty should be done outside).  I'll send a
> patch, just to show that I can still write KVM code. :)

LOL, don't jinx yourself.