[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git-commits-head
Subject:    KVM: VMX: Make VMREAD error path play nice with noinstr
From:       Linux Kernel Mailing List <linux-kernel () vger ! kernel ! org>
Date:       2023-07-30 19:43:57
Message-ID: git-mailbomb-linux-master-c20d403fd04c20959b7e6669868372f433947e5f () kernel ! org
[Download RAW message or body]

Commit:     c20d403fd04c20959b7e6669868372f433947e5f
Parent:     5e1fe4a21c0c2a69419d97d62d3213e8f843920d
Refname:    refs/heads/master
Web:        https://git.kernel.org/torvalds/c/c20d403fd04c20959b7e6669868372f433947e5f
Author:     Sean Christopherson <seanjc@google.com>
AuthorDate: Fri Jul 21 16:56:36 2023 -0700
Committer:  Paolo Bonzini <pbonzini@redhat.com>
CommitDate: Sat Jul 29 11:05:26 2023 -0400

    KVM: VMX: Make VMREAD error path play nice with noinstr
    
    Mark vmread_error_trampoline() as noinstr, and add a second trampoline
    for the CONFIG_CC_HAS_ASM_GOTO_OUTPUT=n case to enable instrumentation
    when handling VM-Fail on VMREAD.  VMREAD is used in various noinstr
    flows, e.g. immediately after VM-Exit, and objtool rightly complains that
    the call to the error trampoline leaves a no-instrumentation section
    without annotating that it's safe to do so.
    
      vmlinux.o: warning: objtool: vmx_vcpu_enter_exit+0xc9:
      call to vmread_error_trampoline() leaves .noinstr.text section
    
    Note, strictly speaking, enabling instrumentation in the VM-Fail path
    isn't exactly safe, but if VMREAD fails the kernel/system is likely hosed
    anyways, and logging that there is a fatal error is more important than
    *maybe* encountering slightly unsafe instrumentation.
    
    Reported-by: Su Hui <suhui@nfschina.com>
    Signed-off-by: Sean Christopherson <seanjc@google.com>
    Message-Id: <20230721235637.2345403-2-seanjc@google.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/vmenter.S |  8 ++++----
 arch/x86/kvm/vmx/vmx.c     | 18 ++++++++++++++----
 arch/x86/kvm/vmx/vmx_ops.h |  9 ++++++++-
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index 07e927d4d099c..be275a0410a89 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -303,10 +303,8 @@ SYM_FUNC_START(vmx_do_nmi_irqoff)
 	VMX_DO_EVENT_IRQOFF call asm_exc_nmi_kvm_vmx
 SYM_FUNC_END(vmx_do_nmi_irqoff)
 
-
-.section .text, "ax"
-
 #ifndef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+
 /**
  * vmread_error_trampoline - Trampoline from inline asm to vmread_error()
  * @field:	VMCS field encoding that failed
@@ -335,7 +333,7 @@ SYM_FUNC_START(vmread_error_trampoline)
 	mov 3*WORD_SIZE(%_ASM_BP), %_ASM_ARG2
 	mov 2*WORD_SIZE(%_ASM_BP), %_ASM_ARG1
 
-	call vmread_error
+	call vmread_error_trampoline2
 
 	/* Zero out @fault, which will be popped into the result register. */
 	_ASM_MOV $0, 3*WORD_SIZE(%_ASM_BP)
@@ -357,6 +355,8 @@ SYM_FUNC_START(vmread_error_trampoline)
 SYM_FUNC_END(vmread_error_trampoline)
 #endif
 
+.section .text, "ax"
+
 SYM_FUNC_START(vmx_do_interrupt_irqoff)
 	VMX_DO_EVENT_IRQOFF CALL_NOSPEC _ASM_ARG1
 SYM_FUNC_END(vmx_do_interrupt_irqoff)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 3c5a13f054fce..0dd4612b0ca21 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -441,13 +441,23 @@ do {					\
 	pr_warn_ratelimited(fmt);	\
 } while (0)
 
-void vmread_error(unsigned long field, bool fault)
+noinline void vmread_error(unsigned long field)
 {
-	if (fault)
+	vmx_insn_failed("vmread failed: field=%lx\n", field);
+}
+
+#ifndef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+noinstr void vmread_error_trampoline2(unsigned long field, bool fault)
+{
+	if (fault) {
 		kvm_spurious_fault();
-	else
-		vmx_insn_failed("vmread failed: field=%lx\n", field);
+	} else {
+		instrumentation_begin();
+		vmread_error(field);
+		instrumentation_end();
+	}
 }
+#endif
 
 noinline void vmwrite_error(unsigned long field, unsigned long value)
 {
diff --git a/arch/x86/kvm/vmx/vmx_ops.h b/arch/x86/kvm/vmx/vmx_ops.h
index ce47dc265f89a..5fa74779a37a4 100644
--- a/arch/x86/kvm/vmx/vmx_ops.h
+++ b/arch/x86/kvm/vmx/vmx_ops.h
@@ -10,7 +10,7 @@
 #include "vmcs.h"
 #include "../x86.h"
 
-void vmread_error(unsigned long field, bool fault);
+void vmread_error(unsigned long field);
 void vmwrite_error(unsigned long field, unsigned long value);
 void vmclear_error(struct vmcs *vmcs, u64 phys_addr);
 void vmptrld_error(struct vmcs *vmcs, u64 phys_addr);
@@ -31,6 +31,13 @@ void invept_error(unsigned long ext, u64 eptp, gpa_t gpa);
  * void vmread_error_trampoline(unsigned long field, bool fault);
  */
 extern unsigned long vmread_error_trampoline;
+
+/*
+ * The second VMREAD error trampoline, called from the assembly trampoline,
+ * exists primarily to enable instrumentation for the VM-Fail path.
+ */
+void vmread_error_trampoline2(unsigned long field, bool fault);
+
 #endif
 
 static __always_inline void vmcs_check16(unsigned long field)
[prev in list] [next in list] [prev in thread] [next in thread]