Re: [PATCH 2/3] perf/x86/intel/pt: Inject PMI for KVM guest
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
Re: [PATCH 2/3] perf/x86/intel/pt: Inject PMI for KVM guest
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;
}
Re: [PATCH 2/3] perf/x86/intel/pt: Inject PMI for KVM guest
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
[PATCH 2/3] perf/x86/intel/pt: Inject PMI for KVM guest
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
---
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(-)
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
#include
#include
+#include
#include
#include
@@ -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 0x038f
#define MSR_CORE_PERF_GLOBAL_OVF_CTRL 0x0390
+/* 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_CONF00x1900
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
#include
+#include
#include "kvm_cache_regs.h"
#define KVM_DEFAULT_PLE_GAP128
@@ -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
--
1.8.3.1
