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

[Peter Zijlstra]

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__ */