Message-ID: <87lgtpambe.fsf@ashishki-desk.ger.corp.intel.com>
Date:   Wed, 01 Feb 2017 18:49:09 +0200
From:   Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        vince@...ter.net, eranian@...gle.com,
        Arnaldo Carvalho de Melo <acme@...radead.org>,
        Borislav Petkov <bp@...e.de>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

Ingo Molnar <mingo@...nel.org> writes:

> * Alexander Shishkin <alexander.shishkin@...ux.intel.com> wrote:
>
>> Now that Intel PT supports more types of trace content than just branch
>> tracing, it may be useful to allow the user to disable branch tracing
>> when it is not needed.
>> 
>> The special case is BDW, where not setting BranchEn is not supported.
>> 
>> This patch adds 'no_branch' event format string to PT events, which
>> will disable setting BranchEn bit in the hardware trace configuration.
>
>> +	/* trying to unset BRANCH_EN where it is not supported */
>
> Please capitalize comments consistently and use the typical tense. This one should 
> be something like:
>
> 	/* Try to unset BRANCH_EN where it is not supported: */

Will do.

>> +	if (!(event->attr.config & RTIT_CTL_BRANCH_EN))
>> +		reg |= RTIT_CTL_BRANCH_EN;
>> +	else
>> +		event->attr.config &= ~RTIT_CTL_BRANCH_EN;
>
>
> So I really hate this ABI hack - it's these unclean approaches how ABIs degrade 
> over time, by death of a thousand cuts...

Agreed.

> Any reason why we couldn't add a separate pt_feature_branch_disable and 
> pt_feature_trace_disable bits and interpret them in a straightforward way, or 
> something like that?
>
> ( This means two more bits, but that's our punishment for not anticipating 
>   extensions to the hardware feature. )

Most features (all but BranchEn) are at the moment set-to-enable. It is
only this unfortunate default of BranchEn=1 that is a pain. Another
relatively painless thing we can do is add a 'passthrough' bit, which
will allow the user to (not) set BranchEn bit when passthrough=1 and
behave like it did before, when passthrough=0. Like below:

>From d28174b582aba285baa1ab9fea73b554e8937f89 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Date: Thu, 19 Jan 2017 10:00:52 +0200
Subject: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

Now that Intel PT supports more types of trace content than just branch
tracing, it may be useful to allow the user to disable branch tracing
when it is not needed.

The special case is BDW, where not setting BranchEn is not supported.

This is slightly trickier than necessary, because up to this moment
the driver has been setting BranchEn automatically and the userspace
assumes as much. Instead of reversing the semantics of BranchEn, we
introduce a 'passthrough' bit, which will forego the default and allow
the user to set BranchEn to their heart's content. A new synthetic
capability is also introduced to advertise the availability of this
'passthrough' bit.

Signed-off-by: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
---
 arch/x86/events/intel/pt.c | 84 ++++++++++++++++++++++++++++++++++++++++++++--
 arch/x86/events/intel/pt.h |  2 ++
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index ece5fb06db..a31b570577 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -28,6 +28,7 @@
 #include <asm/insn.h>
 #include <asm/io.h>
 #include <asm/intel_pt.h>
+#include <asm/intel-family.h>
 
 #include "../perf_event.h"
 #include "pt.h"
@@ -50,6 +51,10 @@ static struct pt_pmu pt_pmu;
 #define PT_CAP(_n, _l, _r, _m)						\
 	[PT_CAP_ ## _n] = { .name = __stringify(_n), .leaf = _l,	\
 			    .reg = _r, .mask = _m }
+/*
+ * Software-defined/driver capabilities
+ */
+#define PT_SW_CAP	0xffffffff
 
 static struct pt_cap_desc {
 	const char	*name;
@@ -72,12 +77,14 @@ static struct pt_cap_desc {
 	PT_CAP(mtc_periods,		1, CPUID_EAX, 0xffff0000),
 	PT_CAP(cycle_thresholds,	1, CPUID_EBX, 0xffff),
 	PT_CAP(psb_periods,		1, CPUID_EBX, 0xffff0000),
+	PT_CAP(passthrough,		PT_SW_CAP, 0, 1),
 };
 
 static u32 pt_cap_get(enum pt_capabilities cap)
 {
 	struct pt_cap_desc *cd = &pt_caps[cap];
-	u32 c = pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
+	u32 c = cd->leaf == PT_SW_CAP ? 1 :
+		pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
 	unsigned int shift = __ffs(cd->mask);
 
 	return (c & cd->mask) >> shift;
@@ -98,6 +105,7 @@ static struct attribute_group pt_cap_group = {
 	.name	= "caps",
 };
 
+PMU_FORMAT_ATTR(passthrough,	"config:0"	);
 PMU_FORMAT_ATTR(cyc,		"config:1"	);
 PMU_FORMAT_ATTR(pwr_evt,	"config:4"	);
 PMU_FORMAT_ATTR(fup_on_ptw,	"config:5"	);
@@ -105,11 +113,13 @@ PMU_FORMAT_ATTR(mtc,		"config:9"	);
 PMU_FORMAT_ATTR(tsc,		"config:10"	);
 PMU_FORMAT_ATTR(noretcomp,	"config:11"	);
 PMU_FORMAT_ATTR(ptw,		"config:12"	);
+PMU_FORMAT_ATTR(branch,		"config:13"	);
 PMU_FORMAT_ATTR(mtc_period,	"config:14-17"	);
 PMU_FORMAT_ATTR(cyc_thresh,	"config:19-22"	);
 PMU_FORMAT_ATTR(psb_period,	"config:24-27"	);
 
 static struct attribute *pt_formats_attr[] = {
+	&format_attr_passthrough.attr,
 	&format_attr_cyc.attr,
 	&format_attr_pwr_evt.attr,
 	&format_attr_fup_on_ptw.attr,
@@ -117,6 +127,7 @@ static struct attribute *pt_formats_attr[] = {
 	&format_attr_tsc.attr,
 	&format_attr_noretcomp.attr,
 	&format_attr_ptw.attr,
+	&format_attr_branch.attr,
 	&format_attr_mtc_period.attr,
 	&format_attr_cyc_thresh.attr,
 	&format_attr_psb_period.attr,
@@ -197,6 +208,19 @@ static int __init pt_pmu_hw_init(void)
 		pt_pmu.tsc_art_den = eax;
 	}
 
+	/* model-specific quirks */
+	switch (boot_cpu_data.x86_model) {
+	case INTEL_FAM6_BROADWELL_CORE:
+	case INTEL_FAM6_BROADWELL_XEON_D:
+	case INTEL_FAM6_BROADWELL_GT3E:
+	case INTEL_FAM6_BROADWELL_X:
+		/* not setting BRANCH_EN will #GP, erratum BDM106 */
+		pt_pmu.branch_en_always_on = true;
+		break;
+	default:
+		break;
+	}
+
 	if (boot_cpu_has(X86_FEATURE_VMX)) {
 		/*
 		 * Intel SDM, 36.5 "Tracing post-VMXON" says that
@@ -263,8 +287,22 @@ static int __init pt_pmu_hw_init(void)
 #define RTIT_CTL_PTW	(RTIT_CTL_PTW_EN	| \
 			 RTIT_CTL_FUP_ON_PTW)
 
-#define PT_CONFIG_MASK (RTIT_CTL_TSC_EN		| \
+/*
+ * Bit 0 (TraceEn) in the attr.config is meaningless as the
+ * corresponding bit in the RTIT_CTL can only be controlled
+ * by the driver; therefore, repurpose it to mean: pass
+ * through the bit that was previously assumed to be always
+ * on for PT, thereby allowing the user to *not* set it if
+ * they so wish. See also pt_event_valid() and pt_config().
+ * The synthetic "passthough" capability tells the userspace
+ * that this feature is available.
+ */
+#define RTIT_CTL_PASSTHROUGH RTIT_CTL_TRACEEN
+
+#define PT_CONFIG_MASK (RTIT_CTL_TRACEEN	| \
+			RTIT_CTL_TSC_EN		| \
 			RTIT_CTL_DISRETC	| \
+			RTIT_CTL_BRANCH_EN	| \
 			RTIT_CTL_CYC_PSB	| \
 			RTIT_CTL_MTC		| \
 			RTIT_CTL_PWR_EVT_EN	| \
@@ -332,6 +370,33 @@ static bool pt_event_valid(struct perf_event *event)
 			return false;
 	}
 
+	/*
+	 * Setting bit 0 (TraceEn in RTIT_CTL MSR) in the attr.config
+	 * clears the assomption that BranchEn must always be enabled,
+	 * as was the case with the first implementation of PT.
+	 * If this bit is not set, the legacy behavior is preserved
+	 * for compatibility with the older userspace.
+	 *
+	 * Re-using bit 0 for this purpose is fine because it is never
+	 * directly set by the user; previous attempts at setting it in
+	 * the attr.config resulted in -EINVAL.
+	 */
+	if (config & RTIT_CTL_PASSTHROUGH) {
+		/*
+		 * Disallow not setting BRANCH_EN where BRANCH_EN is
+		 * always required.
+		 */
+		if (pt_pmu.branch_en_always_on &&
+		    !(config & RTIT_CTL_BRANCH_EN))
+			return false;
+	} else {
+		/*
+		 * Disallow BRANCH_EN without the PASSTHROUGH.
+		 */
+		if (config & RTIT_CTL_BRANCH_EN)
+			return false;
+	}
+
 	return true;
 }
 
@@ -419,7 +484,20 @@ static void pt_config(struct perf_event *event)
 	}
 
 	reg = pt_config_filters(event);
-	reg |= RTIT_CTL_TOPA | RTIT_CTL_BRANCH_EN | RTIT_CTL_TRACEEN;
+	reg |= RTIT_CTL_TOPA | RTIT_CTL_TRACEEN;
+
+	/*
+	 * Previously, we had BRANCH_EN on by default, but now that PT has
+	 * grown features outside of branch tracing, it is useful to allow
+	 * the user to disable it. Setting bit 0 in the event's attr.config
+	 * allows BRANCH_EN to pass through instead of being always on. See
+	 * also the comment in pt_event_valid().
+	 */
+	if (event->attr.config & BIT(0)) {
+		reg |= event->attr.config & RTIT_CTL_BRANCH_EN;
+	} else {
+		reg |= RTIT_CTL_BRANCH_EN;
+	}
 
 	if (!event->attr.exclude_kernel)
 		reg |= RTIT_CTL_OS;
diff --git a/arch/x86/events/intel/pt.h b/arch/x86/events/intel/pt.h
index 53473c21b5..0f17367b7b 100644
--- a/arch/x86/events/intel/pt.h
+++ b/arch/x86/events/intel/pt.h
@@ -104,12 +104,14 @@ enum pt_capabilities {
 	PT_CAP_mtc_periods,
 	PT_CAP_cycle_thresholds,
 	PT_CAP_psb_periods,
+	PT_CAP_passthrough,
 };
 
 struct pt_pmu {
 	struct pmu		pmu;
 	u32			caps[PT_CPUID_REGS_NUM * PT_CPUID_LEAVES];
 	bool			vmx;
+	bool			branch_en_always_on;
 	unsigned long		max_nonturbo_ratio;
 	unsigned int		tsc_art_num;
 	unsigned int		tsc_art_den;
-- 
2.11.0

Powered by blists - more mailing lists