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

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

[David Edmondson]

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

The changes are on top of Aaron's patches from
https://lore.kernel.org/r/20210510144834.658457-1-aaronlewis@google.com
which are in the KVM queue, but not yet upstream.

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    |  1 +
 arch/x86/kvm/svm/svm.c             |  6 ++++++
 arch/x86/kvm/vmx/vmx.c             |  6 ++++++
 arch/x86/kvm/x86.c                 | 23 +++++++++++++++++------
 include/uapi/linux/kvm.h           |  2 ++
 6 files changed, 33 insertions(+), 6 deletions(-)

-- 
2.30.2

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

[David Edmondson]

For later use.

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 616b9679ddcc..408c854b4ac9 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4009,6 +4009,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);
@@ -4573,6 +4578,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..a19b006c287a 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.basic;
+}
+
 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 2/2] KVM: x86: On emulation failure, convey the exit reason to userspace

[David Edmondson]

To aid in debugging.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8166ad113fb2..48ef0dc68faf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7455,7 +7455,7 @@ 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)
+static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu, uint64_t flags)
 {
 	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
 	u32 insn_size = ctxt->fetch.end - ctxt->fetch.data;
@@ -7466,7 +7466,8 @@ 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 &&
+	    (flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES)) {
 		run->emulation_failure.ndata = 3;
 		run->emulation_failure.flags |=
 			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
@@ -7476,6 +7477,14 @@ static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
 		memcpy(run->emulation_failure.insn_bytes,
 		       ctxt->fetch.data, insn_size);
 	}
+
+	if (flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON) {
+		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);
+	}
 }
 
 static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
@@ -7492,16 +7501,18 @@ 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);
+		prepare_emulation_failure_exit(
+			vcpu,
+			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES |
+			KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON);
 		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;
+		prepare_emulation_failure_exit(
+			vcpu, KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON);
 		return 0;
 	}
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 68c9e6d8bbda..3e4126652a67 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,7 @@ struct kvm_run {
 			__u64 flags;
 			__u8  insn_size;
 			__u8  insn_bytes[15];
+			__u64 exit_reason;
 		} emulation_failure;
 		/* KVM_EXIT_OSI */
 		struct {
-- 
2.30.2

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

[David Matlack]

On Mon, Jun 28, 2021 at 06:31:50PM +0100, David Edmondson wrote:
> To aid in debugging failures in the field, when instruction emulation

What do you mean by a "debugging failure"?

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

> 
> The changes are on top of Aaron's patches from
> https://lore.kernel.org/r/20210510144834.658457-1-aaronlewis@google.com
> which are in the KVM queue, but not yet upstream.
> 
> 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    |  1 +
>  arch/x86/kvm/svm/svm.c             |  6 ++++++
>  arch/x86/kvm/vmx/vmx.c             |  6 ++++++
>  arch/x86/kvm/x86.c                 | 23 +++++++++++++++++------
>  include/uapi/linux/kvm.h           |  2 ++
>  6 files changed, 33 insertions(+), 6 deletions(-)
> 
> -- 
> 2.30.2
> 

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

[David Matlack]

On Mon, Jun 28, 2021 at 06:31:51PM +0100, David Edmondson wrote:
> For later use.

Please add more context to the commit message.

> 
> 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 616b9679ddcc..408c854b4ac9 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4009,6 +4009,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);
> @@ -4573,6 +4578,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..a19b006c287a 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.basic;

Why not the full exit reason?

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

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

[David Matlack]

On Mon, Jun 28, 2021 at 06:31:52PM +0100, David Edmondson wrote:
> To aid in debugging.

Please add more context to the commit message.

> 
> Suggested-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
>  arch/x86/kvm/x86.c       | 23 +++++++++++++++++------
>  include/uapi/linux/kvm.h |  2 ++
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8166ad113fb2..48ef0dc68faf 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7455,7 +7455,7 @@ 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)
> +static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu, uint64_t flags)
>  {
>  	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>  	u32 insn_size = ctxt->fetch.end - ctxt->fetch.data;
> @@ -7466,7 +7466,8 @@ 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 &&
> +	    (flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES)) {
>  		run->emulation_failure.ndata = 3;
>  		run->emulation_failure.flags |=
>  			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
> @@ -7476,6 +7477,14 @@ static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
>  		memcpy(run->emulation_failure.insn_bytes,
>  		       ctxt->fetch.data, insn_size);
>  	}
> +
> +	if (flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON) {

This flag is always passed so this check if superfluous. Perhaps change
`int flags` to `bool instruction_bytes` and have it control only whether
the instruction bytes are populated.

> +		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);
> +	}
>  }
>  
>  static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
> @@ -7492,16 +7501,18 @@ 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);
> +		prepare_emulation_failure_exit(
> +			vcpu,
> +			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES |
> +			KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON);
>  		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;
> +		prepare_emulation_failure_exit(
> +			vcpu, KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON);

Should kvm_task_switch and kvm_handle_memory_failure also be updated
like this?

>  		return 0;
>  	}
>  
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 68c9e6d8bbda..3e4126652a67 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,7 @@ struct kvm_run {
>  			__u64 flags;
>  			__u8  insn_size;
>  			__u8  insn_bytes[15];
> +			__u64 exit_reason;

Please document what this field contains, especially since its contents
depend on AMD versus Intel.

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

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

[David Matlack]

On Wed, Jun 30, 2021 at 9:33 AM David Matlack <dmatlack@google.com> wrote:
>
> On Mon, Jun 28, 2021 at 06:31:50PM +0100, David Edmondson wrote:
> > To aid in debugging failures in the field, when instruction emulation
>
> What do you mean by a "debugging failure"?

Oh! Sorry I misread this as "*debugging failures*" rather than
"debugging *failures*". I know what you mean here :-).

>
> > 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?
>
> >
> > The changes are on top of Aaron's patches from
> > https://lore.kernel.org/r/20210510144834.658457-1-aaronlewis@google.com
> > which are in the KVM queue, but not yet upstream.
> >
> > 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    |  1 +
> >  arch/x86/kvm/svm/svm.c             |  6 ++++++
> >  arch/x86/kvm/vmx/vmx.c             |  6 ++++++
> >  arch/x86/kvm/x86.c                 | 23 +++++++++++++++++------
> >  include/uapi/linux/kvm.h           |  2 ++
> >  6 files changed, 33 insertions(+), 6 deletions(-)
> >
> > --
> > 2.30.2
> >

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

[David Edmondson]

On Wednesday, 2021-06-30 at 16:48:42 UTC, David Matlack wrote:

> On Mon, Jun 28, 2021 at 06:31:52PM +0100, David Edmondson wrote:
>> To aid in debugging.
>
> Please add more context to the commit message.

Okay.

>> 
>> Suggested-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>  arch/x86/kvm/x86.c       | 23 +++++++++++++++++------
>>  include/uapi/linux/kvm.h |  2 ++
>>  2 files changed, 19 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 8166ad113fb2..48ef0dc68faf 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -7455,7 +7455,7 @@ 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)
>> +static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu, uint64_t flags)
>>  {
>>  	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>>  	u32 insn_size = ctxt->fetch.end - ctxt->fetch.data;
>> @@ -7466,7 +7466,8 @@ 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 &&
>> +	    (flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES)) {
>>  		run->emulation_failure.ndata = 3;
>>  		run->emulation_failure.flags |=
>>  			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
>> @@ -7476,6 +7477,14 @@ static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
>>  		memcpy(run->emulation_failure.insn_bytes,
>>  		       ctxt->fetch.data, insn_size);
>>  	}
>> +
>> +	if (flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON) {
>
> This flag is always passed so this check if superfluous. Perhaps change
> `int flags` to `bool instruction_bytes` and have it control only whether
> the instruction bytes are populated.

Okay.

>> +		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);
>> +	}
>>  }
>>  
>>  static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>> @@ -7492,16 +7501,18 @@ 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);
>> +		prepare_emulation_failure_exit(
>> +			vcpu,
>> +			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES |
>> +			KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON);
>>  		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;
>> +		prepare_emulation_failure_exit(
>> +			vcpu, KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON);
>
> Should kvm_task_switch and kvm_handle_memory_failure also be updated
> like this?

Will do in v2.

sgx_handle_emulation_failure() seems like an existing user of
KVM_INTERNAL_ERROR_EMULATION that doesn't follow the new protocol (use
the emulation_failure part of the union).

Sean: If I add another flag for this case, what is the existing
user-level consumer?

>>  		return 0;
>>  	}
>>  
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 68c9e6d8bbda..3e4126652a67 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,7 @@ struct kvm_run {
>>  			__u64 flags;
>>  			__u8  insn_size;
>>  			__u8  insn_bytes[15];
>> +			__u64 exit_reason;
>
> Please document what this field contains, especially since its contents
> depend on AMD versus Intel.

Okay.

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

dme.
-- 
Welcome to Conditioning.

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

[Sean Christopherson]

On Fri, Jul 02, 2021, David Edmondson wrote:
> On Wednesday, 2021-06-30 at 16:48:42 UTC, David Matlack wrote:
> 
> > On Mon, Jun 28, 2021 at 06:31:52PM +0100, David Edmondson wrote:
> >>  	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;
> >> +		prepare_emulation_failure_exit(
> >> +			vcpu, KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON);
> >
> > Should kvm_task_switch and kvm_handle_memory_failure also be updated
> > like this?
> 
> Will do in v2.
> 
> sgx_handle_emulation_failure() seems like an existing user of
> KVM_INTERNAL_ERROR_EMULATION that doesn't follow the new protocol (use
> the emulation_failure part of the union).
> 
> Sean: If I add another flag for this case, what is the existing
> user-level consumer?

Doh, the SGX case should have been updated as part of commit c88339d88b0a ("kvm:
x86: Allow userspace to handle emulation errors").  The easiest fix for SGX would
be to zero out 'flags', bump ndata, and shift the existing field usage.  That
would resolve the existing problem of the address being misinterpreted as flags,
and would play nice _if_ additional flags are added.  I'll send a patch for that.

Regarding the consumer, there is no existing consumer per se.  SGX is simply
dumping the bad address that prevented emulation (the only SGX emulation failure
scenarios are bad/missing memslots/vmas).  The SGX case is very similar to
nested VMX instruction emulation, where failure is either due to a bad userspace
configuration (bad/missing memslot) or a busted L1 kernel (SGX instruction data
operand points at emulated MMIO).  A bad userspace configuration is almost always
going to be fatal, and I highly doubt any userspace VMM will bother emulating
SGX+MMIO.  In other words, the info dumped by SGX is purely for debug.

Which brings me back to adding another flag when dumping the exit reason.  Unless
there is a concrete use case for programmatically taking action in reponse to
failed emulation, e.g. attemping emulation in userspace using insn_bytes+insn_size,
I think we should not add a flag and instead dump info for debug/triage purposes
without committing to an ABI.  I.e. define the ABI such that KVM can dump
arbitrary info in the unused portions of data[].

Not having a true ABI will be a bit gross, but digging into these types of
failures is going to be painful no matter what; having to deduce the format of
the data is unlikely to shift the needle much.  And the code should be
straightforward, especially for userspace, e.g. dump all of data[] if emulation
in userspace failed.

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

[David Edmondson]

On Friday, 2021-07-09 at 21:58:12 GMT, Sean Christopherson wrote:

> On Fri, Jul 02, 2021, David Edmondson wrote:
>> On Wednesday, 2021-06-30 at 16:48:42 UTC, David Matlack wrote:
>> 
>> > On Mon, Jun 28, 2021 at 06:31:52PM +0100, David Edmondson wrote:
>> >>  	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;
>> >> +		prepare_emulation_failure_exit(
>> >> +			vcpu, KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_REASON);
>> >
>> > Should kvm_task_switch and kvm_handle_memory_failure also be updated
>> > like this?
>> 
>> Will do in v2.
>> 
>> sgx_handle_emulation_failure() seems like an existing user of
>> KVM_INTERNAL_ERROR_EMULATION that doesn't follow the new protocol (use
>> the emulation_failure part of the union).
>> 
>> Sean: If I add another flag for this case, what is the existing
>> user-level consumer?
>
> Doh, the SGX case should have been updated as part of commit c88339d88b0a ("kvm:
> x86: Allow userspace to handle emulation errors").  The easiest fix for SGX would
> be to zero out 'flags', bump ndata, and shift the existing field usage.  That
> would resolve the existing problem of the address being misinterpreted as flags,
> and would play nice _if_ additional flags are added.  I'll send a patch for that.
>
> [...]
>
> Which brings me back to adding another flag when dumping the exit reason.  Unless
> there is a concrete use case for programmatically taking action in reponse to
> failed emulation, e.g. attemping emulation in userspace using insn_bytes+insn_size,
> I think we should not add a flag and instead dump info for debug/triage purposes
> without committing to an ABI.  I.e. define the ABI such that KVM can dump
> arbitrary info in the unused portions of data[].

https://lore.kernel.org/r/20210729133931.1129696-1-david.edmondson@oracle.com
includes both of these suggestions.