| Message ID | 1547928284-2915-3-git-send-email-luwei.kang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Inject a PMI for KVM Guest when ToPA buffer is filled | expand |
On 19/01/19 21:04, Luwei Kang wrote: > static struct pt_pmu pt_pmu; > > @@ -1260,6 +1262,14 @@ void intel_pt_interrupt(void) > struct pt_buffer *buf; > struct perf_event *event = pt->handle.event; > > + if (pt->vcpu) { > + /* Inject PMI to Guest */ > + kvm_make_request(KVM_REQ_PMI, pt->vcpu); > + __set_bit(MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI_BIT, > + (unsigned long *)&pt->vcpu->arch.pmu.global_status); > + return; > + } > + There is no need to touch struct pt and to know details of KVM in arch/x86/events. Please add a function pointer void (*kvm_handle_pt_interrupt)(void); to some header, and in handle_pmi_common do if (unlikely(kvm_handle_intel_pt_interrupt)) kvm_handle_intel_pt_interrupt(); else intel_pt_interrupt(); The function pointer can be assigned in kvm_before_interrupt/kvm_after_interrupt just like you do now. This should be a simpler patch too. Paolo
On Wed, Jan 30, 2019 at 06:02:27PM +0100, Paolo Bonzini wrote: > On 19/01/19 21:04, Luwei Kang wrote: > > static struct pt_pmu pt_pmu; > > > > @@ -1260,6 +1262,14 @@ void intel_pt_interrupt(void) > > struct pt_buffer *buf; > > struct perf_event *event = pt->handle.event; > > > > + if (pt->vcpu) { > > + /* Inject PMI to Guest */ > > + kvm_make_request(KVM_REQ_PMI, pt->vcpu); > > + __set_bit(MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI_BIT, > > + (unsigned long *)&pt->vcpu->arch.pmu.global_status); > > + return; > > + } > > + > > There is no need to touch struct pt and to know details of KVM in > arch/x86/events. Please add a function pointer > > void (*kvm_handle_pt_interrupt)(void); > > to some header, and in handle_pmi_common do > > if (unlikely(kvm_handle_intel_pt_interrupt)) > kvm_handle_intel_pt_interrupt(); > else > intel_pt_interrupt(); > > The function pointer can be assigned in > kvm_before_interrupt/kvm_after_interrupt just like you do now. > > This should be a simpler patch too. I know we do this in other places too; but it really is a very bad pattern. Exported function pointers are a fscking disaster waiting to happen. There is nothing that limits access to kvm.o, any random module can try and poke at it. How about we extend perf_guest_info_callback with an arch section and add: diff --git a/kernel/events/core.c b/kernel/events/core.c index 5aeb4c74fb99..76ce804e72c1 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -5835,6 +5835,9 @@ struct perf_guest_info_callbacks *perf_guest_cbs; int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs) { + if (WARN_ON_ONCE(perf_guest_cbs)) + return -EBUSY; + perf_guest_cbs = cbs; return 0; } @@ -5842,6 +5845,9 @@ EXPORT_SYMBOL_GPL(perf_register_guest_info_callbacks); int perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs) { + if (WARN_ON_ONCE(perf_guest_cbs != cbs)) + return -EINVAL; + perf_guest_cbs = NULL; return 0; }
On 06/02/19 17:34, Peter Zijlstra wrote: > > How about we extend perf_guest_info_callback with an arch section and > add: > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 5aeb4c74fb99..76ce804e72c1 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -5835,6 +5835,9 @@ struct perf_guest_info_callbacks *perf_guest_cbs; > > int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs) > { > + if (WARN_ON_ONCE(perf_guest_cbs)) > + return -EBUSY; > + > perf_guest_cbs = cbs; > return 0; > } > @@ -5842,6 +5845,9 @@ EXPORT_SYMBOL_GPL(perf_register_guest_info_callbacks); > > int perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs) > { > + if (WARN_ON_ONCE(perf_guest_cbs != cbs)) > + return -EINVAL; > + > perf_guest_cbs = NULL; > return 0; > } Makes total sense. Paolo
diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c index 9494ca6..09375bd 100644 --- a/arch/x86/events/intel/pt.c +++ b/arch/x86/events/intel/pt.c @@ -23,6 +23,7 @@ #include <linux/types.h> #include <linux/slab.h> #include <linux/device.h> +#include <linux/kvm_host.h> #include <asm/perf_event.h> #include <asm/insn.h> @@ -33,7 +34,8 @@ #include "../perf_event.h" #include "pt.h" -static DEFINE_PER_CPU(struct pt, pt_ctx); +DEFINE_PER_CPU(struct pt, pt_ctx); +EXPORT_PER_CPU_SYMBOL_GPL(pt_ctx); static struct pt_pmu pt_pmu; @@ -1260,6 +1262,14 @@ void intel_pt_interrupt(void) struct pt_buffer *buf; struct perf_event *event = pt->handle.event; + if (pt->vcpu) { + /* Inject PMI to Guest */ + kvm_make_request(KVM_REQ_PMI, pt->vcpu); + __set_bit(MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI_BIT, + (unsigned long *)&pt->vcpu->arch.pmu.global_status); + return; + } + /* * There may be a dangling PT bit in the interrupt status register * after PT has been disabled by pt_event_stop(). Make sure we don't diff --git a/arch/x86/include/asm/intel_pt.h b/arch/x86/include/asm/intel_pt.h index ee960fb..32da2e9 100644 --- a/arch/x86/include/asm/intel_pt.h +++ b/arch/x86/include/asm/intel_pt.h @@ -62,6 +62,7 @@ struct pt { struct pt_filters filters; int handle_nmi; int vmx_on; + struct kvm_vcpu *vcpu; }; #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_INTEL) diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 8e40c24..ae01fb0 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -775,6 +775,10 @@ #define MSR_CORE_PERF_GLOBAL_CTRL 0x0000038f #define MSR_CORE_PERF_GLOBAL_OVF_CTRL 0x00000390 +/* PERF_GLOBAL_OVF_CTL bits */ +#define MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI_BIT 55 +#define MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI (1ULL << MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI_BIT) + /* Geode defined MSRs */ #define MSR_GEODE_BUSCONT_CONF0 0x00001900 diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index 224cd0a..a9ee498 100644 --- a/arch/x86/kvm/x86.h +++ b/arch/x86/kvm/x86.h @@ -4,6 +4,7 @@ #include <linux/kvm_host.h> #include <asm/pvclock.h> +#include <asm/intel_pt.h> #include "kvm_cache_regs.h" #define KVM_DEFAULT_PLE_GAP 128 @@ -331,15 +332,20 @@ static inline bool kvm_pause_in_guest(struct kvm *kvm) } DECLARE_PER_CPU(struct kvm_vcpu *, current_vcpu); +DECLARE_PER_CPU(struct pt, pt_ctx); static inline void kvm_before_interrupt(struct kvm_vcpu *vcpu) { __this_cpu_write(current_vcpu, vcpu); + if (kvm_x86_ops->pt_supported()) + this_cpu_ptr(&pt_ctx)->vcpu = vcpu; } static inline void kvm_after_interrupt(struct kvm_vcpu *vcpu) { __this_cpu_write(current_vcpu, NULL); + if (kvm_x86_ops->pt_supported()) + this_cpu_ptr(&pt_ctx)->vcpu = NULL; } #endif
Inject a PMI for KVM guest when Intel PT working in Host-Guest mode and Guest ToPA entry memory buffer was completely filled. The definition of ‘kvm_make_request’ and ‘KVM_REQ_PMI’ depend on "linux/kvm_host.h" header. Signed-off-by: Luwei Kang <luwei.kang@intel.com> --- arch/x86/events/intel/pt.c | 12 +++++++++++- arch/x86/include/asm/intel_pt.h | 1 + arch/x86/include/asm/msr-index.h | 4 ++++ arch/x86/kvm/x86.h | 6 ++++++ 4 files changed, 22 insertions(+), 1 deletion(-)