| Message ID | 20200218232953.5724-7-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Allow userspace to disable the emulator | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Explicitly pass the emulation context to the emulate tracepoint in > preparation of dynamically allocation the emulation context. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/trace.h | 22 +++++++++++----------- > arch/x86/kvm/x86.c | 13 ++++++++----- > 2 files changed, 19 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h > index f194dd058470..5605000ca5f6 100644 > --- a/arch/x86/kvm/trace.h > +++ b/arch/x86/kvm/trace.h > @@ -731,8 +731,9 @@ TRACE_EVENT(kvm_skinit, > }) > > TRACE_EVENT(kvm_emulate_insn, > - TP_PROTO(struct kvm_vcpu *vcpu, __u8 failed), > - TP_ARGS(vcpu, failed), > + TP_PROTO(struct kvm_vcpu *vcpu, struct x86_emulate_ctxt *ctxt, > + __u8 failed), > + TP_ARGS(vcpu, ctxt, failed), > > TP_STRUCT__entry( > __field( __u64, rip ) > @@ -745,13 +746,10 @@ TRACE_EVENT(kvm_emulate_insn, > > TP_fast_assign( > __entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS); This seems the only usage of 'vcpu' parameter now; I checked and even after switching to dynamic emulation context allocation we still set ctxt->vcpu in alloc_emulate_ctxt(), can we get rid of 'vcpu' parameter here then (and use ctxt->vcpu instead)? > - __entry->len = vcpu->arch.emulate_ctxt.fetch.ptr > - - vcpu->arch.emulate_ctxt.fetch.data; > - __entry->rip = vcpu->arch.emulate_ctxt._eip - __entry->len; > - memcpy(__entry->insn, > - vcpu->arch.emulate_ctxt.fetch.data, > - 15); > - __entry->flags = kei_decode_mode(vcpu->arch.emulate_ctxt.mode); > + __entry->len = ctxt->fetch.ptr - ctxt->fetch.data; > + __entry->rip = ctxt->_eip - __entry->len; > + memcpy(__entry->insn, ctxt->fetch.data, 15); > + __entry->flags = kei_decode_mode(ctxt->mode); > __entry->failed = failed; > ), > > @@ -764,8 +762,10 @@ TRACE_EVENT(kvm_emulate_insn, > ) > ); > > -#define trace_kvm_emulate_insn_start(vcpu) trace_kvm_emulate_insn(vcpu, 0) > -#define trace_kvm_emulate_insn_failed(vcpu) trace_kvm_emulate_insn(vcpu, 1) > +#define trace_kvm_emulate_insn_start(vcpu, ctxt) \ > + trace_kvm_emulate_insn(vcpu, ctxt, 0) > +#define trace_kvm_emulate_insn_failed(vcpu, ctxt) \ > + trace_kvm_emulate_insn(vcpu, ctxt, 1) > > TRACE_EVENT( > vcpu_match_mmio, > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 79d1113ad6e7..69d3dd64d90c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6460,10 +6460,13 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip) > } > EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt); > > -static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type) > +static int handle_emulation_failure(struct x86_emulate_ctxt *ctxt, > + int emulation_type) > { > + struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); > + > ++vcpu->stat.insn_emulation_fail; > - trace_kvm_emulate_insn_failed(vcpu); > + trace_kvm_emulate_insn_failed(vcpu, ctxt); > > if (emulation_type & EMULTYPE_VMWARE_GP) { > kvm_queue_exception_e(vcpu, GP_VECTOR, 0); > @@ -6788,7 +6791,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > > r = x86_decode_insn(ctxt, insn, insn_len); > > - trace_kvm_emulate_insn_start(vcpu); > + trace_kvm_emulate_insn_start(vcpu, ctxt); > ++vcpu->stat.insn_emulation; > if (r != EMULATION_OK) { > if ((emulation_type & EMULTYPE_TRAP_UD) || > @@ -6810,7 +6813,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > inject_emulated_exception(ctxt); > return 1; > } > - return handle_emulation_failure(vcpu, emulation_type); > + return handle_emulation_failure(ctxt, emulation_type); > } > } > > @@ -6856,7 +6859,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > emulation_type)) > return 1; > > - return handle_emulation_failure(vcpu, emulation_type); > + return handle_emulation_failure(ctxt, emulation_type); > } > > if (ctxt->have_exception) {
On Wed, Feb 26, 2020 at 06:11:25PM +0100, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > > Explicitly pass the emulation context to the emulate tracepoint in > > preparation of dynamically allocation the emulation context. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kvm/trace.h | 22 +++++++++++----------- > > arch/x86/kvm/x86.c | 13 ++++++++----- > > 2 files changed, 19 insertions(+), 16 deletions(-) > > > > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h > > index f194dd058470..5605000ca5f6 100644 > > --- a/arch/x86/kvm/trace.h > > +++ b/arch/x86/kvm/trace.h > > @@ -731,8 +731,9 @@ TRACE_EVENT(kvm_skinit, > > }) > > > > TRACE_EVENT(kvm_emulate_insn, > > - TP_PROTO(struct kvm_vcpu *vcpu, __u8 failed), > > - TP_ARGS(vcpu, failed), > > + TP_PROTO(struct kvm_vcpu *vcpu, struct x86_emulate_ctxt *ctxt, > > + __u8 failed), > > + TP_ARGS(vcpu, ctxt, failed), > > > > TP_STRUCT__entry( > > __field( __u64, rip ) > > @@ -745,13 +746,10 @@ TRACE_EVENT(kvm_emulate_insn, > > > > TP_fast_assign( > > __entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS); > > This seems the only usage of 'vcpu' parameter now; I checked and even > after switching to dynamic emulation context allocation we still set > ctxt->vcpu in alloc_emulate_ctxt(), can we get rid of 'vcpu' parameter > here then (and use ctxt->vcpu instead)? Hmm, ya, not sure what I was thinking here. > > - __entry->len = vcpu->arch.emulate_ctxt.fetch.ptr > > - - vcpu->arch.emulate_ctxt.fetch.data; > > - __entry->rip = vcpu->arch.emulate_ctxt._eip - __entry->len; > > - memcpy(__entry->insn, > > - vcpu->arch.emulate_ctxt.fetch.data, > > - 15); > > - __entry->flags = kei_decode_mode(vcpu->arch.emulate_ctxt.mode); > > + __entry->len = ctxt->fetch.ptr - ctxt->fetch.data; > > + __entry->rip = ctxt->_eip - __entry->len; > > + memcpy(__entry->insn, ctxt->fetch.data, 15); > > + __entry->flags = kei_decode_mode(ctxt->mode); > > __entry->failed = failed; > > ), > > > > @@ -764,8 +762,10 @@ TRACE_EVENT(kvm_emulate_insn, > > ) > > ); > > > > -#define trace_kvm_emulate_insn_start(vcpu) trace_kvm_emulate_insn(vcpu, 0) > > -#define trace_kvm_emulate_insn_failed(vcpu) trace_kvm_emulate_insn(vcpu, 1) > > +#define trace_kvm_emulate_insn_start(vcpu, ctxt) \ > > + trace_kvm_emulate_insn(vcpu, ctxt, 0) > > +#define trace_kvm_emulate_insn_failed(vcpu, ctxt) \ > > + trace_kvm_emulate_insn(vcpu, ctxt, 1) > > > > TRACE_EVENT( > > vcpu_match_mmio,
On 03/03/20 17:48, Sean Christopherson wrote: >>> TP_fast_assign( >>> __entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS); >> This seems the only usage of 'vcpu' parameter now; I checked and even >> after switching to dynamic emulation context allocation we still set >> ctxt->vcpu in alloc_emulate_ctxt(), can we get rid of 'vcpu' parameter >> here then (and use ctxt->vcpu instead)? > Hmm, ya, not sure what I was thinking here. > As long as we have one use of vcpu, I'd rather skip this patch and adjust patch 8 to use "->". Even the other "explicitly take context" parts are kinda debatable since you still have to do emul_to_vcpu. Throwing a handful of - struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt; + struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt; into patch 8 is not bad at all and limits the churn. Paolo
On Tue, Mar 03, 2020 at 06:29:30PM +0100, Paolo Bonzini wrote: > On 03/03/20 17:48, Sean Christopherson wrote: > >>> TP_fast_assign( > >>> __entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS); > >> This seems the only usage of 'vcpu' parameter now; I checked and even > >> after switching to dynamic emulation context allocation we still set > >> ctxt->vcpu in alloc_emulate_ctxt(), can we get rid of 'vcpu' parameter > >> here then (and use ctxt->vcpu instead)? > > Hmm, ya, not sure what I was thinking here. > > > > As long as we have one use of vcpu, I'd rather skip this patch and > adjust patch 8 to use "->". Even the other "explicitly take context" > parts are kinda debatable since you still have to do emul_to_vcpu. > Throwing a handful of > > - struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt; > + struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt; > > into patch 8 is not bad at all and limits the churn. Hmm, I'd prefer to explicitly pass @ctxt, even for the tracepoint. I get that it's technically unnecessary churn, but explicitly passing @ctxt means that every funcition that grabs arch.emulate_ctxt (all three of 'em) checks for a NULL ctxt. That makes it trivial to visually audit that there's no risk of a bad pointer dereference, and IMO having @ctxt in the prototype is helpful to see "oh, this helper is called from within the emulator".
On 03/03/20 18:42, Sean Christopherson wrote: >> As long as we have one use of vcpu, I'd rather skip this patch and >> adjust patch 8 to use "->". Even the other "explicitly take context" >> parts are kinda debatable since you still have to do emul_to_vcpu. >> Throwing a handful of >> >> - struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt; >> + struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt; >> >> into patch 8 is not bad at all and limits the churn. > Hmm, I'd prefer to explicitly pass @ctxt, even for the tracepoint. I get > that it's technically unnecessary churn, but explicitly passing @ctxt means > that every funcition that grabs arch.emulate_ctxt (all three of 'em) checks > for a NULL ctxt. That makes it trivial to visually audit that there's no > risk of a bad pointer dereference, and IMO having @ctxt in the prototype > is helpful to see "oh, this helper is called from within the emulator". > That's a good rationale, but I believe this refactoring belongs more in the "disable emulator" part than this one. Paolo
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h index f194dd058470..5605000ca5f6 100644 --- a/arch/x86/kvm/trace.h +++ b/arch/x86/kvm/trace.h @@ -731,8 +731,9 @@ TRACE_EVENT(kvm_skinit, }) TRACE_EVENT(kvm_emulate_insn, - TP_PROTO(struct kvm_vcpu *vcpu, __u8 failed), - TP_ARGS(vcpu, failed), + TP_PROTO(struct kvm_vcpu *vcpu, struct x86_emulate_ctxt *ctxt, + __u8 failed), + TP_ARGS(vcpu, ctxt, failed), TP_STRUCT__entry( __field( __u64, rip ) @@ -745,13 +746,10 @@ TRACE_EVENT(kvm_emulate_insn, TP_fast_assign( __entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS); - __entry->len = vcpu->arch.emulate_ctxt.fetch.ptr - - vcpu->arch.emulate_ctxt.fetch.data; - __entry->rip = vcpu->arch.emulate_ctxt._eip - __entry->len; - memcpy(__entry->insn, - vcpu->arch.emulate_ctxt.fetch.data, - 15); - __entry->flags = kei_decode_mode(vcpu->arch.emulate_ctxt.mode); + __entry->len = ctxt->fetch.ptr - ctxt->fetch.data; + __entry->rip = ctxt->_eip - __entry->len; + memcpy(__entry->insn, ctxt->fetch.data, 15); + __entry->flags = kei_decode_mode(ctxt->mode); __entry->failed = failed; ), @@ -764,8 +762,10 @@ TRACE_EVENT(kvm_emulate_insn, ) ); -#define trace_kvm_emulate_insn_start(vcpu) trace_kvm_emulate_insn(vcpu, 0) -#define trace_kvm_emulate_insn_failed(vcpu) trace_kvm_emulate_insn(vcpu, 1) +#define trace_kvm_emulate_insn_start(vcpu, ctxt) \ + trace_kvm_emulate_insn(vcpu, ctxt, 0) +#define trace_kvm_emulate_insn_failed(vcpu, ctxt) \ + trace_kvm_emulate_insn(vcpu, ctxt, 1) TRACE_EVENT( vcpu_match_mmio, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 79d1113ad6e7..69d3dd64d90c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6460,10 +6460,13 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip) } EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt); -static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type) +static int handle_emulation_failure(struct x86_emulate_ctxt *ctxt, + int emulation_type) { + struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt); + ++vcpu->stat.insn_emulation_fail; - trace_kvm_emulate_insn_failed(vcpu); + trace_kvm_emulate_insn_failed(vcpu, ctxt); if (emulation_type & EMULTYPE_VMWARE_GP) { kvm_queue_exception_e(vcpu, GP_VECTOR, 0); @@ -6788,7 +6791,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, r = x86_decode_insn(ctxt, insn, insn_len); - trace_kvm_emulate_insn_start(vcpu); + trace_kvm_emulate_insn_start(vcpu, ctxt); ++vcpu->stat.insn_emulation; if (r != EMULATION_OK) { if ((emulation_type & EMULTYPE_TRAP_UD) || @@ -6810,7 +6813,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, inject_emulated_exception(ctxt); return 1; } - return handle_emulation_failure(vcpu, emulation_type); + return handle_emulation_failure(ctxt, emulation_type); } } @@ -6856,7 +6859,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, emulation_type)) return 1; - return handle_emulation_failure(vcpu, emulation_type); + return handle_emulation_failure(ctxt, emulation_type); } if (ctxt->have_exception) {
Explicitly pass the emulation context to the emulate tracepoint in preparation of dynamically allocation the emulation context. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/trace.h | 22 +++++++++++----------- arch/x86/kvm/x86.c | 13 ++++++++----- 2 files changed, 19 insertions(+), 16 deletions(-)