[PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[David Edmondson]

To help when debugging failures in the field, if instruction emulation
fails, report the VM exit reason to userspace in order that it can be
recorded.

I'm unsure whether sgx_handle_emulation_failure() needs to be adapted
to use the emulation_failure part of the exit union in struct kvm_run
- advice welcomed.

v2:
- Improve patch comments (dmatlack)
- Intel should provide the full exit reason (dmatlack)
- Pass a boolean rather than flags (dmatlack)
- Use the helper in kvm_task_switch() and kvm_handle_memory_failure()
  (dmatlack)
- Describe the exit_reason field of the emulation_failure structure
  (dmatlack)

David Edmondson (2):
  KVM: x86: Add kvm_x86_ops.get_exit_reason
  KVM: x86: On emulation failure, convey the exit reason to userspace

 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  3 +++
 arch/x86/kvm/svm/svm.c             |  6 ++++++
 arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
 arch/x86/kvm/x86.c                 | 22 +++++++++++++---------
 include/uapi/linux/kvm.h           |  7 +++++++
 6 files changed, 37 insertions(+), 13 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] KVM: x86: Add kvm_x86_ops.get_exit_reason

[David Edmondson]

Add a per-implementation static call which returns the cause of the
most recent VM exit.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 arch/x86/include/asm/kvm-x86-ops.h | 1 +
 arch/x86/include/asm/kvm_host.h    | 1 +
 arch/x86/kvm/svm/svm.c             | 6 ++++++
 arch/x86/kvm/vmx/vmx.c             | 6 ++++++
 4 files changed, 14 insertions(+)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index a12a4987154e..afb0917497c1 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -85,6 +85,7 @@ KVM_X86_OP_NULL(sync_pir_to_irr)
 KVM_X86_OP(set_tss_addr)
 KVM_X86_OP(set_identity_map_addr)
 KVM_X86_OP(get_mt_mask)
+KVM_X86_OP(get_exit_reason)
 KVM_X86_OP(load_mmu_pgd)
 KVM_X86_OP_NULL(has_wbinvd_exit)
 KVM_X86_OP(get_l2_tsc_offset)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 974cbfb1eefe..0ee580c68839 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1365,6 +1365,7 @@ struct kvm_x86_ops {
 	int (*set_tss_addr)(struct kvm *kvm, unsigned int addr);
 	int (*set_identity_map_addr)(struct kvm *kvm, u64 ident_addr);
 	u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
+	u64 (*get_exit_reason)(struct kvm_vcpu *vcpu);
 
 	void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
 			     int root_level);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8834822c00cd..2180729ddcb0 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3988,6 +3988,11 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	return 0;
 }
 
+static u64 svm_get_exit_reason(struct kvm_vcpu *vcpu)
+{
+	return to_svm(vcpu)->vmcb->control.exit_code;
+}
+
 static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -4552,6 +4557,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.set_tss_addr = svm_set_tss_addr,
 	.set_identity_map_addr = svm_set_identity_map_addr,
 	.get_mt_mask = svm_get_mt_mask,
+	.get_exit_reason = svm_get_exit_reason,
 
 	.get_exit_info = svm_get_exit_info,
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 927a552393b9..d9a4d6cf6406 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6997,6 +6997,11 @@ static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	return (cache << VMX_EPT_MT_EPTE_SHIFT) | ipat;
 }
 
+static u64 vmx_get_exit_reason(struct kvm_vcpu *vcpu)
+{
+	return to_vmx(vcpu)->exit_reason.full;
+}
+
 static void vmcs_set_secondary_exec_control(struct vcpu_vmx *vmx)
 {
 	/*
@@ -7613,6 +7618,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.set_tss_addr = vmx_set_tss_addr,
 	.set_identity_map_addr = vmx_set_identity_map_addr,
 	.get_mt_mask = vmx_get_mt_mask,
+	.get_exit_reason = vmx_get_exit_reason,
 
 	.get_exit_info = vmx_get_exit_info,
 
-- 
2.30.2

[PATCH v2 2/2] KVM: x86: On emulation failure, convey the exit reason to userspace

[David Edmondson]

Should instruction emulation fail, include the VM exit reason in the
emulation_failure data passed to userspace, in order that the VMM can
report it as a debugging aid when describing the failure.

Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/vmx/vmx.c          |  5 +----
 arch/x86/kvm/x86.c              | 22 +++++++++++++---------
 include/uapi/linux/kvm.h        |  7 +++++++
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0ee580c68839..2e411e26e40e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1630,6 +1630,8 @@ extern u64 kvm_mce_cap_supported;
 int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
 int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
 					void *insn, int insn_len);
+void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu,
+					bool instruction_bytes);
 
 void kvm_enable_efer_bits(u64);
 bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d9a4d6cf6406..4fb240204c2c 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5367,10 +5367,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 
 		if (vmx->emulation_required && !vmx->rmode.vm86_active &&
 		    vcpu->arch.exception.pending) {
-			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-			vcpu->run->internal.suberror =
-						KVM_INTERNAL_ERROR_EMULATION;
-			vcpu->run->internal.ndata = 0;
+			kvm_prepare_emulation_failure_exit(vcpu, false);
 			return 0;
 		}
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 17468d983fbd..bf30b445b65d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7452,7 +7452,8 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
 }
 EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
 
-static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
+void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu,
+					bool instruction_bytes)
 {
 	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
 	u32 insn_size = ctxt->fetch.end - ctxt->fetch.data;
@@ -7463,7 +7464,7 @@ static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
 	run->emulation_failure.ndata = 0;
 	run->emulation_failure.flags = 0;
 
-	if (insn_size) {
+	if (insn_size && instruction_bytes) {
 		run->emulation_failure.ndata = 3;
 		run->emulation_failure.flags |=
 			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
@@ -7473,7 +7474,14 @@ static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
 		memcpy(run->emulation_failure.insn_bytes,
 		       ctxt->fetch.data, insn_size);
 	}
+
+	run->emulation_failure.ndata = 4;
+	run->emulation_failure.flags |=
+		KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON;
+	run->emulation_failure.exit_reason =
+		static_call(kvm_x86_get_exit_reason)(vcpu);
 }
+EXPORT_SYMBOL_GPL(kvm_prepare_emulation_failure_exit);
 
 static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
 {
@@ -7489,16 +7497,14 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
 
 	if (kvm->arch.exit_on_emulation_error ||
 	    (emulation_type & EMULTYPE_SKIP)) {
-		prepare_emulation_failure_exit(vcpu);
+		kvm_prepare_emulation_failure_exit(vcpu, true);
 		return 0;
 	}
 
 	kvm_queue_exception(vcpu, UD_VECTOR);
 
 	if (!is_guest_mode(vcpu) && static_call(kvm_x86_get_cpl)(vcpu) == 0) {
-		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-		vcpu->run->internal.ndata = 0;
+		kvm_prepare_emulation_failure_exit(vcpu, false);
 		return 0;
 	}
 
@@ -12092,9 +12098,7 @@ int kvm_handle_memory_failure(struct kvm_vcpu *vcpu, int r,
 	 * doesn't seem to be a real use-case behind such requests, just return
 	 * KVM_EXIT_INTERNAL_ERROR for now.
 	 */
-	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-	vcpu->run->internal.ndata = 0;
+	kvm_prepare_emulation_failure_exit(vcpu, false);
 
 	return 0;
 }
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index d9e4aabcb31a..863195371272 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -282,6 +282,7 @@ struct kvm_xen_exit {
 
 /* Flags that describe what fields in emulation_failure hold valid data. */
 #define KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES (1ULL << 0)
+#define KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON       (1ULL << 1)
 
 /* for KVM_RUN, returned by mmap(vcpu_fd, offset=0) */
 struct kvm_run {
@@ -404,6 +405,12 @@ struct kvm_run {
 			__u64 flags;
 			__u8  insn_size;
 			__u8  insn_bytes[15];
+			/*
+			 * The "exit reason" extracted from the
+			 * VMCS/VMCB that was the cause of attempted
+			 * emulation.
+			 */
+			__u64 exit_reason;
 		} emulation_failure;
 		/* KVM_EXIT_OSI */
 		struct {
-- 
2.30.2

Re: [PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[David Matlack]

On Tue, Jul 06, 2021 at 11:12:05AM +0100, David Edmondson wrote:
> To help when debugging failures in the field, if instruction emulation
> fails, report the VM exit reason to userspace in order that it can be
> recorded.

What is the benefit of seeing the VM-exit reason that led to an
emulation failure?

> 
> I'm unsure whether sgx_handle_emulation_failure() needs to be adapted
> to use the emulation_failure part of the exit union in struct kvm_run
> - advice welcomed.
> 
> v2:
> - Improve patch comments (dmatlack)
> - Intel should provide the full exit reason (dmatlack)
> - Pass a boolean rather than flags (dmatlack)
> - Use the helper in kvm_task_switch() and kvm_handle_memory_failure()
>   (dmatlack)
> - Describe the exit_reason field of the emulation_failure structure
>   (dmatlack)
> 
> David Edmondson (2):
>   KVM: x86: Add kvm_x86_ops.get_exit_reason
>   KVM: x86: On emulation failure, convey the exit reason to userspace
> 
>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
>  arch/x86/include/asm/kvm_host.h    |  3 +++
>  arch/x86/kvm/svm/svm.c             |  6 ++++++
>  arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
>  arch/x86/kvm/x86.c                 | 22 +++++++++++++---------
>  include/uapi/linux/kvm.h           |  7 +++++++
>  6 files changed, 37 insertions(+), 13 deletions(-)
> 
> -- 
> 2.30.2
> 

Re: [PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[David Matlack]

On Tue, Jul 06, 2021 at 11:12:05AM +0100, David Edmondson wrote:
> To help when debugging failures in the field, if instruction emulation
> fails, report the VM exit reason to userspace in order that it can be
> recorded.
> 
> I'm unsure whether sgx_handle_emulation_failure() needs to be adapted
> to use the emulation_failure part of the exit union in struct kvm_run
> - advice welcomed.
> 
> v2:
> - Improve patch comments (dmatlack)
> - Intel should provide the full exit reason (dmatlack)

I just asked if Intel should provide the full exit reason, I do not have
an opinion either way. It really comes down to your usecase for wanting
the exit reason. Would the full exit reason be useful or do you just
need the basic exit number?

> - Pass a boolean rather than flags (dmatlack)
> - Use the helper in kvm_task_switch() and kvm_handle_memory_failure()
>   (dmatlack)
> - Describe the exit_reason field of the emulation_failure structure
>   (dmatlack)
> 
> David Edmondson (2):
>   KVM: x86: Add kvm_x86_ops.get_exit_reason
>   KVM: x86: On emulation failure, convey the exit reason to userspace
> 
>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
>  arch/x86/include/asm/kvm_host.h    |  3 +++
>  arch/x86/kvm/svm/svm.c             |  6 ++++++
>  arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
>  arch/x86/kvm/x86.c                 | 22 +++++++++++++---------
>  include/uapi/linux/kvm.h           |  7 +++++++
>  6 files changed, 37 insertions(+), 13 deletions(-)
> 
> -- 
> 2.30.2
> 

Re: [PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[David Edmondson]

Apologies if you see two of these - I had some email problems earlier.

On Wednesday, 2021-07-07 at 23:20:04 UTC, David Matlack wrote:

> On Tue, Jul 06, 2021 at 11:12:05AM +0100, David Edmondson wrote:
>> To help when debugging failures in the field, if instruction emulation
>> fails, report the VM exit reason to userspace in order that it can be
>> recorded.
>
> What is the benefit of seeing the VM-exit reason that led to an
> emulation failure?

I can't cite an example of where this has definitively led in a
direction that helped solve a problem, but we do sometimes see emulation
failures reported in situations where we are not able to reproduce the
failures on demand and the existing information provided at the time of
failure is either insufficient or suspect.

Given that, I'm left casting about for data that can be made available
to assist in postmortem analysis of the failures.

>> I'm unsure whether sgx_handle_emulation_failure() needs to be adapted
>> to use the emulation_failure part of the exit union in struct kvm_run
>> - advice welcomed.
>> 
>> v2:
>> - Improve patch comments (dmatlack)
>> - Intel should provide the full exit reason (dmatlack)
>> - Pass a boolean rather than flags (dmatlack)
>> - Use the helper in kvm_task_switch() and kvm_handle_memory_failure()
>>   (dmatlack)
>> - Describe the exit_reason field of the emulation_failure structure
>>   (dmatlack)
>> 
>> David Edmondson (2):
>>   KVM: x86: Add kvm_x86_ops.get_exit_reason
>>   KVM: x86: On emulation failure, convey the exit reason to userspace
>> 
>>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
>>  arch/x86/include/asm/kvm_host.h    |  3 +++
>>  arch/x86/kvm/svm/svm.c             |  6 ++++++
>>  arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
>>  arch/x86/kvm/x86.c                 | 22 +++++++++++++---------
>>  include/uapi/linux/kvm.h           |  7 +++++++
>>  6 files changed, 37 insertions(+), 13 deletions(-)
>> 
>> -- 
>> 2.30.2
>> 

dme.
-- 
It's gettin', it's gettin', it's gettin' kinda hectic.

Re: [PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[David Edmondson]

On Wednesday, 2021-07-07 at 23:22:44 UTC, David Matlack wrote:

> On Tue, Jul 06, 2021 at 11:12:05AM +0100, David Edmondson wrote:
>> To help when debugging failures in the field, if instruction emulation
>> fails, report the VM exit reason to userspace in order that it can be
>> recorded.
>> 
>> I'm unsure whether sgx_handle_emulation_failure() needs to be adapted
>> to use the emulation_failure part of the exit union in struct kvm_run
>> - advice welcomed.
>> 
>> v2:
>> - Improve patch comments (dmatlack)
>> - Intel should provide the full exit reason (dmatlack)
>
> I just asked if Intel should provide the full exit reason, I do not have
> an opinion either way. It really comes down to your usecase for wanting
> the exit reason. Would the full exit reason be useful or do you just
> need the basic exit number?

Given that this is intended as a debug aid, having the full exit reason
makes sense.

>> - Pass a boolean rather than flags (dmatlack)
>> - Use the helper in kvm_task_switch() and kvm_handle_memory_failure()
>>   (dmatlack)
>> - Describe the exit_reason field of the emulation_failure structure
>>   (dmatlack)
>> 
>> David Edmondson (2):
>>   KVM: x86: Add kvm_x86_ops.get_exit_reason
>>   KVM: x86: On emulation failure, convey the exit reason to userspace
>> 
>>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
>>  arch/x86/include/asm/kvm_host.h    |  3 +++
>>  arch/x86/kvm/svm/svm.c             |  6 ++++++
>>  arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
>>  arch/x86/kvm/x86.c                 | 22 +++++++++++++---------
>>  include/uapi/linux/kvm.h           |  7 +++++++
>>  6 files changed, 37 insertions(+), 13 deletions(-)
>> 
>> -- 
>> 2.30.2
>> 

dme.
-- 
You know it's not the twilight zone.

Re: [PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[David Matlack]

On Thu, Jul 08, 2021 at 03:17:40PM +0100, David Edmondson wrote:
> Apologies if you see two of these - I had some email problems earlier.

I only got one! :)

> 
> On Wednesday, 2021-07-07 at 23:20:04 UTC, David Matlack wrote:
> 
> > On Tue, Jul 06, 2021 at 11:12:05AM +0100, David Edmondson wrote:
> >> To help when debugging failures in the field, if instruction emulation
> >> fails, report the VM exit reason to userspace in order that it can be
> >> recorded.
> >
> > What is the benefit of seeing the VM-exit reason that led to an
> > emulation failure?
> 
> I can't cite an example of where this has definitively led in a
> direction that helped solve a problem, but we do sometimes see emulation
> failures reported in situations where we are not able to reproduce the
> failures on demand and the existing information provided at the time of
> failure is either insufficient or suspect.
> 
> Given that, I'm left casting about for data that can be made available
> to assist in postmortem analysis of the failures.

Understood, thanks for the context. My only concern would be that
userspace APIs are difficult to change once they exist. If it turns
out knowing the exit reason does not help with debugging emulation
failures we'd still be stuck with exporting it on every emulation
failure.

My intuition is that the instruction bytes (which are now available with
Aaron's patch) and the guest register state (which is queryable through
other ioctls) should be sufficient to set up a reproduction of the
emulation failure in a kvm-unit-test and the exit reason should not
really matter. I'm curious if that's not the case?

I'm really not opposed to exporting the exit reason if it is useful, I'm
just not sure it will help.

> 
> >> I'm unsure whether sgx_handle_emulation_failure() needs to be adapted
> >> to use the emulation_failure part of the exit union in struct kvm_run
> >> - advice welcomed.
> >> 
> >> v2:
> >> - Improve patch comments (dmatlack)
> >> - Intel should provide the full exit reason (dmatlack)
> >> - Pass a boolean rather than flags (dmatlack)
> >> - Use the helper in kvm_task_switch() and kvm_handle_memory_failure()
> >>   (dmatlack)
> >> - Describe the exit_reason field of the emulation_failure structure
> >>   (dmatlack)
> >> 
> >> David Edmondson (2):
> >>   KVM: x86: Add kvm_x86_ops.get_exit_reason
> >>   KVM: x86: On emulation failure, convey the exit reason to userspace
> >> 
> >>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
> >>  arch/x86/include/asm/kvm_host.h    |  3 +++
> >>  arch/x86/kvm/svm/svm.c             |  6 ++++++
> >>  arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
> >>  arch/x86/kvm/x86.c                 | 22 +++++++++++++---------
> >>  include/uapi/linux/kvm.h           |  7 +++++++
> >>  6 files changed, 37 insertions(+), 13 deletions(-)
> >> 
> >> -- 
> >> 2.30.2
> >> 
> 
> dme.
> -- 
> It's gettin', it's gettin', it's gettin' kinda hectic.

Re: [PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[David Edmondson]

On Thursday, 2021-07-08 at 18:38:18 UTC, David Matlack wrote:

> On Thu, Jul 08, 2021 at 03:17:40PM +0100, David Edmondson wrote:
>> Apologies if you see two of these - I had some email problems earlier.
>
> I only got one! :)

Phew!

>> On Wednesday, 2021-07-07 at 23:20:04 UTC, David Matlack wrote:
>> 
>> > On Tue, Jul 06, 2021 at 11:12:05AM +0100, David Edmondson wrote:
>> >> To help when debugging failures in the field, if instruction emulation
>> >> fails, report the VM exit reason to userspace in order that it can be
>> >> recorded.
>> >
>> > What is the benefit of seeing the VM-exit reason that led to an
>> > emulation failure?
>> 
>> I can't cite an example of where this has definitively led in a
>> direction that helped solve a problem, but we do sometimes see emulation
>> failures reported in situations where we are not able to reproduce the
>> failures on demand and the existing information provided at the time of
>> failure is either insufficient or suspect.
>> 
>> Given that, I'm left casting about for data that can be made available
>> to assist in postmortem analysis of the failures.
>
> Understood, thanks for the context. My only concern would be that
> userspace APIs are difficult to change once they exist.

Agreed.

> If it turns out knowing the exit reason does not help with debugging
> emulation failures we'd still be stuck with exporting it on every
> emulation failure.

We could stop setting the flag and never export it, but this would waste
space in the structure and be odd, without doubt.

> My intuition is that the instruction bytes (which are now available with
> Aaron's patch) and the guest register state (which is queryable through
> other ioctls) should be sufficient to set up a reproduction of the
> emulation failure in a kvm-unit-test and the exit reason should not
> really matter. I'm curious if that's not the case?

The instruction bytes around the reported EIP are all zeroes - the
register dump looks suspect, and doesn't correspond with the reported
behaviour of the VM at the time of the failure.

It's possible that Aaron's changes will help, indeed, given that they
report state from within the instruction emulator itself. So far I don't
have a sufficiently reproducible case to be able to see if that is the
case.

> I'm really not opposed to exporting the exit reason if it is useful, I'm
> just not sure it will help.

In the emulation failure case we are not in something I would consider a
fast path, and the overhead of acquiring and reporting the exit reason
is low.

Do you anticipate a case where it would be inappropriate or expensive to
report the reason?

>> 
>> >> I'm unsure whether sgx_handle_emulation_failure() needs to be adapted
>> >> to use the emulation_failure part of the exit union in struct kvm_run
>> >> - advice welcomed.
>> >> 
>> >> v2:
>> >> - Improve patch comments (dmatlack)
>> >> - Intel should provide the full exit reason (dmatlack)
>> >> - Pass a boolean rather than flags (dmatlack)
>> >> - Use the helper in kvm_task_switch() and kvm_handle_memory_failure()
>> >>   (dmatlack)
>> >> - Describe the exit_reason field of the emulation_failure structure
>> >>   (dmatlack)
>> >> 
>> >> David Edmondson (2):
>> >>   KVM: x86: Add kvm_x86_ops.get_exit_reason
>> >>   KVM: x86: On emulation failure, convey the exit reason to userspace
>> >> 
>> >>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
>> >>  arch/x86/include/asm/kvm_host.h    |  3 +++
>> >>  arch/x86/kvm/svm/svm.c             |  6 ++++++
>> >>  arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
>> >>  arch/x86/kvm/x86.c                 | 22 +++++++++++++---------
>> >>  include/uapi/linux/kvm.h           |  7 +++++++
>> >>  6 files changed, 37 insertions(+), 13 deletions(-)
>> >> 
>> >> -- 
>> >> 2.30.2
>> >> 
>> 
>> dme.
>> -- 
>> It's gettin', it's gettin', it's gettin' kinda hectic.

dme.
-- 
Please forgive me if I act a little strange, for I know not what I do.

Re: [PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[David Matlack]

On Thu, Jul 08, 2021 at 09:13:38PM +0100, David Edmondson wrote:
> On Thursday, 2021-07-08 at 18:38:18 UTC, David Matlack wrote:
> 
> > On Thu, Jul 08, 2021 at 03:17:40PM +0100, David Edmondson wrote:
> >> Apologies if you see two of these - I had some email problems earlier.
> >
> > I only got one! :)
> 
> Phew!
> 
> >> On Wednesday, 2021-07-07 at 23:20:04 UTC, David Matlack wrote:
> >> 
> >> > On Tue, Jul 06, 2021 at 11:12:05AM +0100, David Edmondson wrote:
> >> >> To help when debugging failures in the field, if instruction emulation
> >> >> fails, report the VM exit reason to userspace in order that it can be
> >> >> recorded.
> >> >
> >> > What is the benefit of seeing the VM-exit reason that led to an
> >> > emulation failure?
> >> 
> >> I can't cite an example of where this has definitively led in a
> >> direction that helped solve a problem, but we do sometimes see emulation
> >> failures reported in situations where we are not able to reproduce the
> >> failures on demand and the existing information provided at the time of
> >> failure is either insufficient or suspect.
> >> 
> >> Given that, I'm left casting about for data that can be made available
> >> to assist in postmortem analysis of the failures.
> >
> > Understood, thanks for the context. My only concern would be that
> > userspace APIs are difficult to change once they exist.
> 
> Agreed.
> 
> > If it turns out knowing the exit reason does not help with debugging
> > emulation failures we'd still be stuck with exporting it on every
> > emulation failure.
> 
> We could stop setting the flag and never export it, but this would waste
> space in the structure and be odd, without doubt.
> 
> > My intuition is that the instruction bytes (which are now available with
> > Aaron's patch) and the guest register state (which is queryable through
> > other ioctls) should be sufficient to set up a reproduction of the
> > emulation failure in a kvm-unit-test and the exit reason should not
> > really matter. I'm curious if that's not the case?
> 
> The instruction bytes around the reported EIP are all zeroes - the
> register dump looks suspect, and doesn't correspond with the reported
> behaviour of the VM at the time of the failure.

Interesting... Nothing comes to mind but others on this list might have
a suggestion of where to look next.

> 
> It's possible that Aaron's changes will help, indeed, given that they
> report state from within the instruction emulator itself. So far I don't
> have a sufficiently reproducible case to be able to see if that is the
> case.
> 
> > I'm really not opposed to exporting the exit reason if it is useful, I'm
> > just not sure it will help.
> 
> In the emulation failure case we are not in something I would consider a
> fast path, and the overhead of acquiring and reporting the exit reason
> is low.

Agreed. I'm not worried about performance, only code complexity and
bloat in the userspace API. But as you suggested above we could always
stop setting the flag and remove the code that populates the exit reason
if it turns out to not be useful. The field in kvm_run is the only thing
that could be hard to remove in the future.

> 
> Do you anticipate a case where it would be inappropriate or expensive to
> report the reason?
> 
> >> 
> >> >> I'm unsure whether sgx_handle_emulation_failure() needs to be adapted
> >> >> to use the emulation_failure part of the exit union in struct kvm_run
> >> >> - advice welcomed.
> >> >> 
> >> >> v2:
> >> >> - Improve patch comments (dmatlack)
> >> >> - Intel should provide the full exit reason (dmatlack)
> >> >> - Pass a boolean rather than flags (dmatlack)
> >> >> - Use the helper in kvm_task_switch() and kvm_handle_memory_failure()
> >> >>   (dmatlack)
> >> >> - Describe the exit_reason field of the emulation_failure structure
> >> >>   (dmatlack)
> >> >> 
> >> >> David Edmondson (2):
> >> >>   KVM: x86: Add kvm_x86_ops.get_exit_reason
> >> >>   KVM: x86: On emulation failure, convey the exit reason to userspace
> >> >> 
> >> >>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
> >> >>  arch/x86/include/asm/kvm_host.h    |  3 +++
> >> >>  arch/x86/kvm/svm/svm.c             |  6 ++++++
> >> >>  arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
> >> >>  arch/x86/kvm/x86.c                 | 22 +++++++++++++---------
> >> >>  include/uapi/linux/kvm.h           |  7 +++++++
> >> >>  6 files changed, 37 insertions(+), 13 deletions(-)
> >> >> 
> >> >> -- 
> >> >> 2.30.2
> >> >> 
> >> 
> >> dme.
> >> -- 
> >> It's gettin', it's gettin', it's gettin' kinda hectic.
> 
> dme.
> -- 
> Please forgive me if I act a little strange, for I know not what I do.

Re: [PATCH v2 2/2] KVM: x86: On emulation failure, convey the exit reason to userspace

[Sean Christopherson]

On Tue, Jul 06, 2021, David Edmondson wrote:
> Should instruction emulation fail, include the VM exit reason in the
> emulation_failure data passed to userspace, in order that the VMM can
> report it as a debugging aid when describing the failure.

...

> @@ -7473,7 +7474,14 @@ static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
>  		memcpy(run->emulation_failure.insn_bytes,
>  		       ctxt->fetch.data, insn_size);
>  	}
> +
> +	run->emulation_failure.ndata = 4;
> +	run->emulation_failure.flags |=
> +		KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON;
> +	run->emulation_failure.exit_reason =
> +		static_call(kvm_x86_get_exit_reason)(vcpu);
>  }

...

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index d9e4aabcb31a..863195371272 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -282,6 +282,7 @@ struct kvm_xen_exit {
>  
>  /* Flags that describe what fields in emulation_failure hold valid data. */
>  #define KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES (1ULL << 0)
> +#define KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON       (1ULL << 1)
>  
>  /* for KVM_RUN, returned by mmap(vcpu_fd, offset=0) */
>  struct kvm_run {
> @@ -404,6 +405,12 @@ struct kvm_run {
>  			__u64 flags;
>  			__u8  insn_size;
>  			__u8  insn_bytes[15];
> +			/*
> +			 * The "exit reason" extracted from the
> +			 * VMCS/VMCB that was the cause of attempted
> +			 * emulation.
> +			 */
> +			__u64 exit_reason;

Rather than providing just the exit reason and adding another kvm_x86_ops hook,
I would prefer to extend kvm_x86_get_exit_info() to also provide the exit reason
and use that.  E.g. on VMX, all exceptions funnel through a single exit reason.
Dumping exit_info_{1,2} and error_code in addition to intr_info might not be all
that useful, but I can't see in harm either, and more info is generally a good
thing.

The only other user of kvm_x86_get_exit_info() is for tracepoints, those could
be modified to not pass in the exit reason.

>  		} emulation_failure;
>  		/* KVM_EXIT_OSI */
>  		struct {
> -- 
> 2.30.2
> 

Re: [PATCH v2 0/2] kvm: x86: Convey the exit reason to user-space on emulation failure

[Sean Christopherson]

On Thu, Jul 08, 2021, David Matlack wrote:
> On Thu, Jul 08, 2021 at 09:13:38PM +0100, David Edmondson wrote:
> > On Thursday, 2021-07-08 at 18:38:18 UTC, David Matlack wrote:
> > > On Thu, Jul 08, 2021 at 03:17:40PM +0100, David Edmondson wrote:
> > >> I can't cite an example of where this has definitively led in a
> > >> direction that helped solve a problem, but we do sometimes see emulation
> > >> failures reported in situations where we are not able to reproduce the
> > >> failures on demand and the existing information provided at the time of
> > >> failure is either insufficient or suspect.
> > >> 
> > >> Given that, I'm left casting about for data that can be made available
> > >> to assist in postmortem analysis of the failures.
> > >
> > > Understood, thanks for the context. My only concern would be that
> > > userspace APIs are difficult to change once they exist.
> > 
> > Agreed.
> > 
> > > If it turns out knowing the exit reason does not help with debugging
> > > emulation failures we'd still be stuck with exporting it on every
> > > emulation failure.

I can think of multiple cases where knowing why KVM emulated in the first place
would be helpful, e.g. a failure on EPT misconfig (MMIO) exit could be a simple
"drat, KVM doesn't handle SSE instructions", whereas a failure on a descriptor
table exit (for UMIP emulation) would be a completely different mess.

Re: [PATCH v2 2/2] KVM: x86: On emulation failure, convey the exit reason to userspace

[David Edmondson]

On Friday, 2021-07-09 at 15:47:08 UTC, Sean Christopherson wrote:

> On Tue, Jul 06, 2021, David Edmondson wrote:
>> Should instruction emulation fail, include the VM exit reason in the
>> emulation_failure data passed to userspace, in order that the VMM can
>> report it as a debugging aid when describing the failure.
>
> ...
>
>> @@ -7473,7 +7474,14 @@ static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
>>  		memcpy(run->emulation_failure.insn_bytes,
>>  		       ctxt->fetch.data, insn_size);
>>  	}
>> +
>> +	run->emulation_failure.ndata = 4;
>> +	run->emulation_failure.flags |=
>> +		KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON;
>> +	run->emulation_failure.exit_reason =
>> +		static_call(kvm_x86_get_exit_reason)(vcpu);
>>  }
>
> ...
>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index d9e4aabcb31a..863195371272 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -282,6 +282,7 @@ struct kvm_xen_exit {
>>  
>>  /* Flags that describe what fields in emulation_failure hold valid data. */
>>  #define KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES (1ULL << 0)
>> +#define KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON       (1ULL << 1)
>>  
>>  /* for KVM_RUN, returned by mmap(vcpu_fd, offset=0) */
>>  struct kvm_run {
>> @@ -404,6 +405,12 @@ struct kvm_run {
>>  			__u64 flags;
>>  			__u8  insn_size;
>>  			__u8  insn_bytes[15];
>> +			/*
>> +			 * The "exit reason" extracted from the
>> +			 * VMCS/VMCB that was the cause of attempted
>> +			 * emulation.
>> +			 */
>> +			__u64 exit_reason;
>
> Rather than providing just the exit reason and adding another kvm_x86_ops hook,
> I would prefer to extend kvm_x86_get_exit_info() to also provide the exit reason
> and use that.  E.g. on VMX, all exceptions funnel through a single exit reason.
> Dumping exit_info_{1,2} and error_code in addition to intr_info might not be all
> that useful, but I can't see in harm either, and more info is generally a good
> thing.
>
> The only other user of kvm_x86_get_exit_info() is for tracepoints, those could
> be modified to not pass in the exit reason.

Okay.

>>  		} emulation_failure;
>>  		/* KVM_EXIT_OSI */
>>  		struct {
>> -- 
>> 2.30.2
>> 

dme.
-- 
What did you learn today? I learnt nothing.