Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

From: Peter Zijlstra
Date: Tue Feb 14 2017 - 14:49:16 EST


On Tue, Feb 14, 2017 at 07:38:07PM +0100, Peter Zijlstra wrote:

> Right, so I question the whole 'lets not schedule PT when VMX' premise,
> it leads to inconsistencies all over. How about we treat it like
> ->add() succeeded and VMX simply results in no output.
>
> Esp. when you then emit 'fake' data into/from a vmlaunch/vmresume
> instruction.

That is, what about something like the below? (completely untested for
obvious raisins).

That should schedule PT like normal, and where VMXON will auto-clear
TraceEn for us, we make VMXOFF set it again.

---
arch/x86/events/intel/pt.c | 38 ++++++++++++++++++++++++++------------
arch/x86/events/intel/pt.h | 1 +
2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 5900471ee508..a42fa1bef761 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -411,6 +411,7 @@ static u64 pt_config_filters(struct perf_event *event)

static void pt_config(struct perf_event *event)
{
+ struct pt *pt = this_cpu_ptr(&pt_ctx);
u64 reg;

if (!event->hw.itrace_started) {
@@ -429,11 +430,14 @@ static void pt_config(struct perf_event *event)
reg |= (event->attr.config & PT_CONFIG_MASK);

event->hw.config = reg;
- wrmsrl(MSR_IA32_RTIT_CTL, reg);
+
+ if (!pt->vmx_on)
+ wrmsrl(MSR_IA32_RTIT_CTL, reg);
}

static void pt_config_stop(struct perf_event *event)
{
+ struct pt *pt = this_cpu_ptr(&pt_ctx);
u64 ctl = READ_ONCE(event->hw.config);

/* may be already stopped by a PMI */
@@ -441,7 +445,9 @@ static void pt_config_stop(struct perf_event *event)
return;

ctl &= ~RTIT_CTL_TRACEEN;
- wrmsrl(MSR_IA32_RTIT_CTL, ctl);
+
+ if (!pt->vmx_on)
+ wrmsrl(MSR_IA32_RTIT_CTL, ctl);

WRITE_ONCE(event->hw.config, ctl);

@@ -1174,10 +1180,12 @@ void intel_pt_interrupt(void)
/*
* If VMX is on and PT does not support it, don't touch anything.
*/
- if (READ_ONCE(pt->vmx_on))
+ if (READ_ONCE(pt->vmx_on)) {
+ WRITE_ONCE(pt->vmx_pmi_pending, 1);
return;
+ }

- if (!event)
+ if (WARN_ON(!event)) /* should be set if handle_nmi */
return;

pt_config_stop(event);
@@ -1236,13 +1244,22 @@ void intel_pt_handle_vmx(int on)
*/
local_irq_save(flags);
WRITE_ONCE(pt->vmx_on, on);
+ if (on)
+ goto done;

- if (on) {
- /* prevent pt_config_stop() from writing RTIT_CTL */
- event = pt->handle.event;
- if (event)
- event->hw.config = 0;
+ /* OTOH, if we just did VMXOFF, we need to set TraceEn again */
+ event = pt->handle.event;
+ if (!event)
+ goto done;
+
+ if (pt->vmx_pmi_pending) {
+ intel_pt_interrupt();
+ pt->vmx_pmi_pending = 0;
+ } else {
+ wrmsrl(MSR_IA32_RTIT_CTL, event->hw.config);
}
+
+done:
local_irq_restore(flags);
}
EXPORT_SYMBOL_GPL(intel_pt_handle_vmx);
@@ -1257,9 +1274,6 @@ static void pt_event_start(struct perf_event *event, int mode)
struct pt *pt = this_cpu_ptr(&pt_ctx);
struct pt_buffer *buf;

- if (READ_ONCE(pt->vmx_on))
- return;
-
buf = perf_aux_output_begin(&pt->handle, event);
if (!buf)
goto fail_stop;
diff --git a/arch/x86/events/intel/pt.h b/arch/x86/events/intel/pt.h
index 53473c21b554..98ed385e187b 100644
--- a/arch/x86/events/intel/pt.h
+++ b/arch/x86/events/intel/pt.h
@@ -187,6 +187,7 @@ struct pt {
struct pt_filters filters;
int handle_nmi;
int vmx_on;
+ int vmx_pmi_pending;
};

#endif /* __INTEL_PT_H__ */