[PATCH] KVM: VMX: unify VMX instruction error reporting

[PATCH] KVM: VMX: unify VMX instruction error reporting

[Jim Mattson]

From: David Matlack <dmatlack@google.com>

Include the value of the "VM-instruction error" field from the current
VMCS (if any) in the error message whenever a VMX instruction (other
than VMREAD) fails. Previously, this field was only reported for
VMWRITE errors.

(Omit the "VM-instruction error" field for VMREAD to avoid potentially
infinite recursion.)

Signed-off-by: David Matlack <dmatlack@google.com>
[Rebased and refactored code; reworded commit message.]
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d58b763df855..c7d2d60fd35b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -392,24 +392,26 @@ noinline void vmwrite_error(unsigned long field, unsigned long value)
 
 noinline void vmclear_error(struct vmcs *vmcs, u64 phys_addr)
 {
-	vmx_insn_failed("kvm: vmclear failed: %p/%llx\n", vmcs, phys_addr);
+	vmx_insn_failed("kvm: vmclear failed: %p/%llx err=%d\n",
+			vmcs, phys_addr, vmcs_read32(VM_INSTRUCTION_ERROR));
 }
 
 noinline void vmptrld_error(struct vmcs *vmcs, u64 phys_addr)
 {
-	vmx_insn_failed("kvm: vmptrld failed: %p/%llx\n", vmcs, phys_addr);
+	vmx_insn_failed("kvm: vmptrld failed: %p/%llx err=%d\n",
+			vmcs, phys_addr, vmcs_read32(VM_INSTRUCTION_ERROR));
 }
 
 noinline void invvpid_error(unsigned long ext, u16 vpid, gva_t gva)
 {
-	vmx_insn_failed("kvm: invvpid failed: ext=0x%lx vpid=%u gva=0x%lx\n",
-			ext, vpid, gva);
+	vmx_insn_failed("kvm: invvpid failed: ext=0x%lx vpid=%u gva=0x%lx err=%d\n",
+			ext, vpid, gva, vmcs_read32(VM_INSTRUCTION_ERROR));
 }
 
 noinline void invept_error(unsigned long ext, u64 eptp, gpa_t gpa)
 {
-	vmx_insn_failed("kvm: invept failed: ext=0x%lx eptp=%llx gpa=0x%llx\n",
-			ext, eptp, gpa);
+	vmx_insn_failed("kvm: invept failed: ext=0x%lx eptp=%llx gpa=0x%llx err=%d\n",
+			ext, eptp, gpa, vmcs_read32(VM_INSTRUCTION_ERROR));
 }
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
-- 
2.36.0.512.ge40c2bad7a-goog

Re: [PATCH] KVM: VMX: unify VMX instruction error reporting

[Sean Christopherson]

On Wed, May 04, 2022, Jim Mattson wrote:
> From: David Matlack <dmatlack@google.com>
> 
> Include the value of the "VM-instruction error" field from the current
> VMCS (if any) in the error message whenever a VMX instruction (other
> than VMREAD) fails. Previously, this field was only reported for
> VMWRITE errors.

Eh, this is pointless for INVVPID and INVEPT, they both have a single error
reason.  VMCLEAR is slightly less pointless, but printing the PA will most likely
make the error reason superfluous.  Ditto for VMPTRLD.

I'm not strictly opposed to printing the error info, but it's not a clear cut win
versus the added risk of blowing up the VMREAD (which is a tiny risk, but non-zero).
And, if we want to do this for everything, then we might as well do it for VMREAD
too.

To reduce the risk of blowing up the host and play nice with VMREAD, what about
adding a dedicated "safe" helper to read VM_INSTRUCTION_ERROR for the error
handlers?  Then modify this patch to go on top...

Compile tested only (I can fully test later today).

--
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 5 May 2022 08:19:58 -0700
Subject: [PATCH] KVM: VMX: Add a dedicated "safe" helper for
 VMREAD(VM_INSTRUCTION_ERROR)

Add a fully "safe" helper to do VMREAD(VM_INSTRUCTION_ERROR) so that VMX
instruction error handlers, e.g. for VMPTRLD, VMREAD itself, etc..., can
get and print the extra error information without the risk of triggering
kvm_spurious_fault() or an infinite loop (in the VMREAD case).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 26ec9b814651..14ac7b8b0723 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -373,6 +373,26 @@ static u32 vmx_segment_access_rights(struct kvm_segment *var);

 void vmx_vmexit(void);

+static u32 vmread_vm_instruction_error_safe(void)
+{
+	unsigned long value;
+
+	asm volatile("1: vmread %1, %0\n\t"
+		     "ja 3f\n\t"
+
+		     /* VMREAD failed or faulted, clear the return value. */
+		     "2:\n\t"
+		     "xorl %k0, %k0\n\t"
+		     "3:\n\t"
+
+		     _ASM_EXTABLE(1b, 2b)
+
+		     : "=&r"(value)
+		     : "r"((unsigned long)VM_INSTRUCTION_ERROR)
+		     : "cc");
+	return value;
+}
+
 #define vmx_insn_failed(fmt...)		\
 do {					\
 	WARN_ONCE(1, fmt);		\
@@ -390,7 +410,7 @@ asmlinkage void vmread_error(unsigned long field, bool fault)
 noinline void vmwrite_error(unsigned long field, unsigned long value)
 {
 	vmx_insn_failed("kvm: vmwrite failed: field=%lx val=%lx err=%d\n",
-			field, value, vmcs_read32(VM_INSTRUCTION_ERROR));
+			field, value, vmread_vm_instruction_error_safe());
 }

 noinline void vmclear_error(struct vmcs *vmcs, u64 phys_addr)

base-commit: 68973d7fff6d23ae4d05708db429c56e50d377e5
--