[PATCH 0/2] perf/x86/intel/pt: Misc updates

[PATCH 0/2] perf/x86/intel/pt: Misc updates

[Alexander Shishkin]

Hi Peter,

Here I have two small updates. Branch tracing can now be disabled
so that we have more bandwidth for other things that PT may log,
such as power events. The latter, along with the PTWRITE, didn't
get their own format strings before, so adding them now.

Alexander Shishkin (2):
  perf/x86/intel/pt: Add format strings for PTWRITE and power event
    tracing
  perf/x86/intel/pt: Allow disabling branch tracing

 arch/x86/events/intel/pt.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 arch/x86/events/intel/pt.h |  1 +
 2 files changed, 42 insertions(+), 1 deletion(-)

-- 
2.11.0

[PATCH 1/2] perf/x86/intel/pt: Add format strings for PTWRITE and power event tracing

[Alexander Shishkin]

Commit 8ee83b2ab3 ("perf/x86/intel/pt: Add support for PTWRITE and power
event tracing") forgot to add format strings to the PT driver. So one
could enable these features by setting corresponding bits in the event
config, but not by their mnemonic names.

This patch adds the format strings.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Fixes: 8ee83b2ab3 ("perf/x86/intel/pt: Add support for PTWRITE...")
---
 arch/x86/events/intel/pt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 54dd585771..ece5fb06db 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -99,18 +99,24 @@ static struct attribute_group pt_cap_group = {
 };
 
 PMU_FORMAT_ATTR(cyc,		"config:1"	);
+PMU_FORMAT_ATTR(pwr_evt,	"config:4"	);
+PMU_FORMAT_ATTR(fup_on_ptw,	"config:5"	);
 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(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_cyc.attr,
+	&format_attr_pwr_evt.attr,
+	&format_attr_fup_on_ptw.attr,
 	&format_attr_mtc.attr,
 	&format_attr_tsc.attr,
 	&format_attr_noretcomp.attr,
+	&format_attr_ptw.attr,
 	&format_attr_mtc_period.attr,
 	&format_attr_cyc_thresh.attr,
 	&format_attr_psb_period.attr,
-- 
2.11.0

[PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

[Alexander Shishkin]

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.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 arch/x86/events/intel/pt.c | 36 +++++++++++++++++++++++++++++++++++-
 arch/x86/events/intel/pt.h |  1 +
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index ece5fb06db..57478c2d0f 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"
@@ -105,6 +106,7 @@ 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(no_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"	);
@@ -117,6 +119,7 @@ static struct attribute *pt_formats_attr[] = {
 	&format_attr_tsc.attr,
 	&format_attr_noretcomp.attr,
 	&format_attr_ptw.attr,
+	&format_attr_no_branch.attr,
 	&format_attr_mtc_period.attr,
 	&format_attr_cyc_thresh.attr,
 	&format_attr_psb_period.attr,
@@ -197,6 +200,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
@@ -265,6 +281,7 @@ static int __init pt_pmu_hw_init(void)
 
 #define PT_CONFIG_MASK (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 +349,10 @@ static bool pt_event_valid(struct perf_event *event)
 			return false;
 	}
 
+	/* trying to unset BRANCH_EN where it is not supported */
+	if (config & RTIT_CTL_BRANCH_EN && pt_pmu.branch_en_always_on)
+		return false;
+
 	return true;
 }
 
@@ -419,7 +440,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. So, to keep compatibility, setting
+	 * BRANCH_EN bit in the event config (no_branch=1) will have the
+	 * reverse effect and *not* set BRANCH_EN in the hardware
+	 * configuration.
+	 */
+	if (!(event->attr.config & RTIT_CTL_BRANCH_EN))
+		reg |= RTIT_CTL_BRANCH_EN;
+	else
+		event->attr.config &= ~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..93a3633e4c 100644
--- a/arch/x86/events/intel/pt.h
+++ b/arch/x86/events/intel/pt.h
@@ -110,6 +110,7 @@ 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

Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

[Ingo Molnar]

* Alexander Shishkin <alexander.shishkin@linux.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: */

>  
>  	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. So, to keep compatibility, setting
> +	 * BRANCH_EN bit in the event config (no_branch=1) will have the
> +	 * reverse effect and *not* set BRANCH_EN in the hardware
> +	 * configuration.
> +	 */
> +	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...

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. )

Also, rename "RTIT_CTL_BRANCH_EN" to "RTIT_CTL_PT_EN" (but without changing the 
ABI), to more clearly express what that bit realy does.

I.e. we'd have a hierarchy of flags:

	- the old RTIT_CTL_BRANCH_EN bit (now RTIT_CTL_PT_EN) enables all of PT, 
	  with all features

	- individual feature disabling bits, which default to 0 (i.e. the feature 
	  is enabled) in the attr structure control the finegrained 
	  enabling/disabling of PT features. Currently there are two bits: 
	  pt_feature_branch_disable and pt_feature_branch_enable. More are added 
	  in the future if PT grows even more features.

or so?

Thanks,

	Ingo

[tip:perf/core] perf/x86/intel/pt: Add format strings for PTWRITE and power event tracing

[tip-bot for Alexander Shishkin]

Commit-ID:  5443624bedd0d23e112d5f2a919435182875bce9
Gitweb:     http://git.kernel.org/tip/5443624bedd0d23e112d5f2a919435182875bce9
Author:     Alexander Shishkin <alexander.shishkin@linux.intel.com>
AuthorDate: Fri, 27 Jan 2017 17:16:43 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 1 Feb 2017 11:09:54 +0100

perf/x86/intel/pt: Add format strings for PTWRITE and power event tracing

Commit:

  8ee83b2ab3 ("perf/x86/intel/pt: Add support for PTWRITE and power event tracing")

forgot to add format strings to the PT driver. So one could enable these features
by setting corresponding bits in the event config, but not by their mnemonic names.

This patch adds the format strings.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: vince@deater.net
Fixes: 8ee83b2ab3 ("perf/x86/intel/pt: Add support for PTWRITE...")
Link: http://lkml.kernel.org/r/20170127151644.8585-2-alexander.shishkin@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/events/intel/pt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 1c1b9fe..5900471 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -99,18 +99,24 @@ static struct attribute_group pt_cap_group = {
 };
 
 PMU_FORMAT_ATTR(cyc,		"config:1"	);
+PMU_FORMAT_ATTR(pwr_evt,	"config:4"	);
+PMU_FORMAT_ATTR(fup_on_ptw,	"config:5"	);
 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(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_cyc.attr,
+	&format_attr_pwr_evt.attr,
+	&format_attr_fup_on_ptw.attr,
 	&format_attr_mtc.attr,
 	&format_attr_tsc.attr,
 	&format_attr_noretcomp.attr,
+	&format_attr_ptw.attr,
 	&format_attr_mtc_period.attr,
 	&format_attr_cyc_thresh.attr,
 	&format_attr_psb_period.attr,

Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

[Alexander Shishkin]

Ingo Molnar <mingo@kernel.org> writes:

> * Alexander Shishkin <alexander.shishkin@linux.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@linux.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@linux.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

Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

[Alexander Shishkin]

Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:

> @@ -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),

Thinking some more: we don't actually need this as the format string
already serves this purpose.

Regards,
--
Alex

[PATCH] perf/x86/intel/pt: Allow disabling branch tracing

[Alexander Shishkin]

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.

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

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 9e92a4097d..d92a60ef08 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"
@@ -98,6 +99,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 +107,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 +121,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 +202,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 +281,20 @@ 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().
+ */
+#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 +362,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 +476,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..93a3633e4c 100644
--- a/arch/x86/events/intel/pt.h
+++ b/arch/x86/events/intel/pt.h
@@ -110,6 +110,7 @@ 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

Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

[Andi Kleen]

Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:

> 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.

cpu/passthrough=1,branchen=1/ seems far uglier/more complicanted to me
than the original cpu/nobranch=1/

Just think how you would explain it to the user in the manpage.

-Andi

Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

[Alexander Shishkin]

Andi Kleen <andi@firstfloor.org> writes:

> Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:
>
>> 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.
>
> cpu/passthrough=1,branchen=1/ seems far uglier/more complicanted to me
> than the original cpu/nobranch=1/

It's /passthrough=1,branch=0/ or simply /passthrough=1/.

Regards,
--
Alex

Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

[Andi Kleen]

On Mon, Feb 06, 2017 at 06:05:29PM +0200, Alexander Shishkin wrote:
> Andi Kleen <andi@firstfloor.org> writes:
> 
> > Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:
> >
> >> 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.
> >
> > cpu/passthrough=1,branchen=1/ seems far uglier/more complicanted to me
> > than the original cpu/nobranch=1/
> 
> It's /passthrough=1,branch=0/ or simply /passthrough=1/.

Ok, but still you have to list exactly to which flags passthrough
applies to, and it will only ever be branchen.

So basically you turned nobranch=1 into two more difficult to
explain flags without any future advantage.

That is why nobranch=1 is better. It is far easier to explain
and logical to the user.

-Andi

[tip:perf/core] perf/x86/intel/pt: Allow the disabling of branch tracing

[tip-bot for Alexander Shishkin]

Commit-ID:  d35869ba348d3f1ff3e6d8214fe0f674bb0e404e
Gitweb:     http://git.kernel.org/tip/d35869ba348d3f1ff3e6d8214fe0f674bb0e404e
Author:     Alexander Shishkin <alexander.shishkin@linux.intel.com>
AuthorDate: Mon, 6 Feb 2017 16:41:40 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 30 Mar 2017 09:53:49 +0200

perf/x86/intel/pt: Allow the disabling of 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.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20170206144140.14402-1-alexander.shishkin@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/events/intel/pt.c | 74 ++++++++++++++++++++++++++++++++++++++++++++--
 arch/x86/events/intel/pt.h |  1 +
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 354e9ff..ae8324d 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"
@@ -98,6 +99,7 @@ static struct attribute_group pt_cap_group = {
 	.name	= "caps",
 };
 
+PMU_FORMAT_ATTR(pt,		"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 +107,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_pt.attr,
 	&format_attr_cyc.attr,
 	&format_attr_pwr_evt.attr,
 	&format_attr_fup_on_ptw.attr,
@@ -117,6 +121,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 +202,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 +281,20 @@ fail:
 #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().
+ */
+#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 +362,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;
 }
 
@@ -420,7 +477,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 b528e8f..0eb41d0 100644
--- a/arch/x86/events/intel/pt.h
+++ b/arch/x86/events/intel/pt.h
@@ -110,6 +110,7 @@ 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;

Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

[Adrian Hunter]

On 06/02/17 19:19, Andi Kleen wrote:
> On Mon, Feb 06, 2017 at 06:05:29PM +0200, Alexander Shishkin wrote:
>> Andi Kleen <andi@firstfloor.org> writes:
>>
>>> Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:
>>>
>>>> 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.
>>>
>>> cpu/passthrough=1,branchen=1/ seems far uglier/more complicanted to me
>>> than the original cpu/nobranch=1/
>>
>> It's /passthrough=1,branch=0/ or simply /passthrough=1/.
> 
> Ok, but still you have to list exactly to which flags passthrough
> applies to, and it will only ever be branchen.

It doesn't have to be that bad.

For Intel PT, perf tools already provides default config which the user must
override.  So, when 'passthrough' is supported, the default config would
include 'passthrough=1,branch=1'.  So then the user would only have to put
'branch=0' when they want no branches.