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

[Sean Christopherson <seanjc@google.com>]

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
--