| Message ID | 20210412130938.68178-6-david.edmondson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Make the cause of instruction emulation available to user-space | expand |
+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.
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.
> > > 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.
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 --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;