diff mbox series

[5/6] KVM: SVM: pass a proper reason in kvm_emulate_instruction()

Message ID 20210412130938.68178-6-david.edmondson@oracle.com (mailing list archive)
State New
Headers show
Series KVM: x86: Make the cause of instruction emulation available to user-space | expand

Commit Message

David Edmondson April 12, 2021, 1:09 p.m. UTC
From: Joao Martins <joao.m.martins@oracle.com>

Declare various causes of emulation and use them as appropriate.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 arch/x86/include/asm/kvm_host.h |  6 ++++++
 arch/x86/kvm/svm/avic.c         |  3 ++-
 arch/x86/kvm/svm/svm.c          | 26 +++++++++++++++-----------
 3 files changed, 23 insertions(+), 12 deletions(-)

Comments

Sean Christopherson April 12, 2021, 4:04 p.m. UTC | #1
+Aaron

On Mon, Apr 12, 2021, David Edmondson wrote:
> From: Joao Martins <joao.m.martins@oracle.com>
> 
> Declare various causes of emulation and use them as appropriate.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  6 ++++++
>  arch/x86/kvm/svm/avic.c         |  3 ++-
>  arch/x86/kvm/svm/svm.c          | 26 +++++++++++++++-----------
>  3 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 79e9ca756742..e1284680cbdc 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1535,6 +1535,12 @@ enum {
>  	EMULREASON_IO_COMPLETE,
>  	EMULREASON_UD,
>  	EMULREASON_PF,
> +	EMULREASON_SVM_NOASSIST,
> +	EMULREASON_SVM_RSM,
> +	EMULREASON_SVM_RDPMC,
> +	EMULREASON_SVM_CR,
> +	EMULREASON_SVM_DR,
> +	EMULREASON_SVM_AVIC_UNACCEL,

Passing these to userspace arguably makes them ABI, i.e. they need to go into
uapi/kvm.h somewhere.  That said, I don't like passing arbitrary values for what
is effectively the VM-Exit reason.  Why not simply pass the exit reason, assuming
we do indeed want to dump this info to userspace?

What is the intended end usage of this information?  Actual emulation?  Debug?
Logging?

Depending on what you're trying to do with the info, maybe there's a better
option.  E.g. Aaron is working on a series that includes passing pass the code
stream (instruction bytes) to userspace on emulation failure, though I'm not
sure if he's planning on providing the VM-Exit reason.
David Edmondson April 13, 2021, 10:53 a.m. UTC | #2
On Monday, 2021-04-12 at 16:04:02 GMT, Sean Christopherson wrote:

> +Aaron
>
> On Mon, Apr 12, 2021, David Edmondson wrote:
>> From: Joao Martins <joao.m.martins@oracle.com>
>> 
>> Declare various causes of emulation and use them as appropriate.
>> 
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h |  6 ++++++
>>  arch/x86/kvm/svm/avic.c         |  3 ++-
>>  arch/x86/kvm/svm/svm.c          | 26 +++++++++++++++-----------
>>  3 files changed, 23 insertions(+), 12 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 79e9ca756742..e1284680cbdc 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1535,6 +1535,12 @@ enum {
>>  	EMULREASON_IO_COMPLETE,
>>  	EMULREASON_UD,
>>  	EMULREASON_PF,
>> +	EMULREASON_SVM_NOASSIST,
>> +	EMULREASON_SVM_RSM,
>> +	EMULREASON_SVM_RDPMC,
>> +	EMULREASON_SVM_CR,
>> +	EMULREASON_SVM_DR,
>> +	EMULREASON_SVM_AVIC_UNACCEL,
>
> Passing these to userspace arguably makes them ABI, i.e. they need to go into
> uapi/kvm.h somewhere.  That said, I don't like passing arbitrary values for what
> is effectively the VM-Exit reason.  Why not simply pass the exit reason, assuming
> we do indeed want to dump this info to userspace?

That would suffice, yes.

> What is the intended end usage of this information?  Actual emulation?  Debug?
> Logging?

Debug (which implies logging, given that I want this to happen on
systems that are in service).

> Depending on what you're trying to do with the info, maybe there's a better
> option.  E.g. Aaron is working on a series that includes passing pass the code
> stream (instruction bytes) to userspace on emulation failure, though I'm not
> sure if he's planning on providing the VM-Exit reason.

Having the instruction stream will be good.

Aaron: do you have anything to share now? In what time frame do you
think you might submit patches?

I'm happy to re-work this to make the exit reason available, if that's
the appropriate direction.

dme.
Aaron Lewis April 13, 2021, 6:45 p.m. UTC | #3
>
> > Depending on what you're trying to do with the info, maybe there's a better
> > option.  E.g. Aaron is working on a series that includes passing pass the code
> > stream (instruction bytes) to userspace on emulation failure, though I'm not
> > sure if he's planning on providing the VM-Exit reason.
>
> Having the instruction stream will be good.
>
> Aaron: do you have anything to share now? In what time frame do you
> think you might submit patches?

I should be able to have something out later this week.  There is no
exit reason as Sean indicated, so if that's important it will have to
be reworked afterwards.  For struct internal in kvm_run I use data[0]
for flags to indicate what's contained in the rest of it, I use
data[1] as the instruction size, and I use data[2,3] to store the
instruction bytes.  Hope that helps.

>
> I'm happy to re-work this to make the exit reason available, if that's
> the appropriate direction.
>
> dme.
> --
> And you're standing here beside me, I love the passing of time.
David Edmondson April 14, 2021, 12:25 p.m. UTC | #4
On Tuesday, 2021-04-13 at 11:45:52 -07, Aaron Lewis wrote:

>>
>> > Depending on what you're trying to do with the info, maybe there's a better
>> > option.  E.g. Aaron is working on a series that includes passing pass the code
>> > stream (instruction bytes) to userspace on emulation failure, though I'm not
>> > sure if he's planning on providing the VM-Exit reason.
>>
>> Having the instruction stream will be good.
>>
>> Aaron: do you have anything to share now? In what time frame do you
>> think you might submit patches?
>
> I should be able to have something out later this week.  There is no
> exit reason as Sean indicated, so if that's important it will have to
> be reworked afterwards.  For struct internal in kvm_run I use data[0]
> for flags to indicate what's contained in the rest of it, I use
> data[1] as the instruction size, and I use data[2,3] to store the
> instruction bytes.  Hope that helps.

Thanks. I'll hang on to look at the patches before doing anything else.

dme.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 79e9ca756742..e1284680cbdc 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1535,6 +1535,12 @@  enum {
 	EMULREASON_IO_COMPLETE,
 	EMULREASON_UD,
 	EMULREASON_PF,
+	EMULREASON_SVM_NOASSIST,
+	EMULREASON_SVM_RSM,
+	EMULREASON_SVM_RDPMC,
+	EMULREASON_SVM_CR,
+	EMULREASON_SVM_DR,
+	EMULREASON_SVM_AVIC_UNACCEL,
 };
 
 int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type,
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 31a17fa6a37c..faa5d4db7ccc 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -558,7 +558,8 @@  int avic_unaccelerated_access_interception(struct vcpu_svm *svm)
 		ret = avic_unaccel_trap_write(svm);
 	} else {
 		/* Handling Fault */
-		ret = kvm_emulate_instruction(&svm->vcpu, 0, 0);
+		ret = kvm_emulate_instruction(&svm->vcpu, 0,
+					      EMULREASON_SVM_AVIC_UNACCEL);
 	}
 
 	return ret;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index bba3b72390a8..2646aa2eae22 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -344,7 +344,8 @@  static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	}
 
 	if (!svm->next_rip) {
-		if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP, 0))
+		if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP,
+					     EMULREASON_SKIP))
 			return 0;
 	} else {
 		kvm_rip_write(vcpu, svm->next_rip);
@@ -2077,7 +2078,8 @@  static int io_interception(struct vcpu_svm *svm)
 		if (sev_es_guest(vcpu->kvm))
 			return sev_es_string_io(svm, size, port, in);
 		else
-			return kvm_emulate_instruction(vcpu, 0, 0);
+			return kvm_emulate_instruction(vcpu, 0,
+						       EMULREASON_IO);
 	}
 
 	svm->next_rip = svm->vmcb->control.exit_info_2;
@@ -2263,7 +2265,8 @@  static int gp_interception(struct vcpu_svm *svm)
 		 */
 		if (!is_guest_mode(vcpu))
 			return kvm_emulate_instruction(vcpu,
-				EMULTYPE_VMWARE_GP | EMULTYPE_NO_DECODE, 0);
+				EMULTYPE_VMWARE_GP | EMULTYPE_NO_DECODE,
+				EMULREASON_GP);
 	} else
 		return emulate_svm_instr(vcpu, opcode);
 
@@ -2459,20 +2462,21 @@  static int invd_interception(struct vcpu_svm *svm)
 static int invlpg_interception(struct vcpu_svm *svm)
 {
 	if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
-		return kvm_emulate_instruction(&svm->vcpu, 0, 0);
+		return kvm_emulate_instruction(&svm->vcpu, 0, EMULREASON_SVM_NOASSIST);
 
 	kvm_mmu_invlpg(&svm->vcpu, svm->vmcb->control.exit_info_1);
 	return kvm_skip_emulated_instruction(&svm->vcpu);
 }
 
-static int emulate_on_interception(struct vcpu_svm *svm)
+static int emulate_on_interception(struct vcpu_svm *svm, int emulation_reason)
 {
-	return kvm_emulate_instruction(&svm->vcpu, 0, 0);
+	return kvm_emulate_instruction(&svm->vcpu, 0, emulation_reason);
 }
 
 static int rsm_interception(struct vcpu_svm *svm)
 {
-	return kvm_emulate_instruction_from_buffer(&svm->vcpu, rsm_ins_bytes, 2, 0);
+	return kvm_emulate_instruction_from_buffer(&svm->vcpu, rsm_ins_bytes, 2,
+						   EMULREASON_SVM_RSM);
 }
 
 static int rdpmc_interception(struct vcpu_svm *svm)
@@ -2480,7 +2484,7 @@  static int rdpmc_interception(struct vcpu_svm *svm)
 	int err;
 
 	if (!nrips)
-		return emulate_on_interception(svm);
+		return emulate_on_interception(svm, EMULREASON_SVM_RDPMC);
 
 	err = kvm_rdpmc(&svm->vcpu);
 	return kvm_complete_insn_gp(&svm->vcpu, err);
@@ -2516,10 +2520,10 @@  static int cr_interception(struct vcpu_svm *svm)
 	int err;
 
 	if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
-		return emulate_on_interception(svm);
+		return emulate_on_interception(svm, EMULREASON_SVM_NOASSIST);
 
 	if (unlikely((svm->vmcb->control.exit_info_1 & CR_VALID) == 0))
-		return emulate_on_interception(svm);
+		return emulate_on_interception(svm, EMULREASON_SVM_CR);
 
 	reg = svm->vmcb->control.exit_info_1 & SVM_EXITINFO_REG_MASK;
 	if (svm->vmcb->control.exit_code == SVM_EXIT_CR0_SEL_WRITE)
@@ -2635,7 +2639,7 @@  static int dr_interception(struct vcpu_svm *svm)
 	}
 
 	if (!boot_cpu_has(X86_FEATURE_DECODEASSISTS))
-		return emulate_on_interception(svm);
+		return emulate_on_interception(svm, EMULREASON_SVM_NOASSIST);
 
 	reg = svm->vmcb->control.exit_info_1 & SVM_EXITINFO_REG_MASK;
 	dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0;