[PATCH 1/2] perf, x86: Disallow setting undefined bits for PEBS events

[PATCH 1/2] perf, x86: Disallow setting undefined bits for PEBS events

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The SDM forbids setting various event qualifiers with PEBS
events. The magic cycles:pp event uses it, but it has caused
problems in the past. We continue allowing it for cycles:pp,
but forbid it for all other events to follow the SDM.

SDM Vol 3 18.8.4:
"PEBS events are only valid when the following fields of
IA32_PERFEVTSELx are all zero: AnyThread, Edge, Invert, Cmask."

One visible change from this is that the cycles:pp event
can now only be set with "cycles:pp", but not with
raw form. If that was a problem we can allow it again.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event.h       |  2 +-
 arch/x86/kernel/cpu/perf_event_intel.c | 26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 1cca3d8..cf1eda1 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -461,7 +461,7 @@ struct x86_pmu {
 	int		pebs_record_size;
 	void		(*drain_pebs)(struct pt_regs *regs);
 	struct event_constraint *pebs_constraints;
-	void		(*pebs_aliases)(struct perf_event *event);
+	void		(*pebs_aliases)(struct perf_event *event, bool *changed);
 	int 		max_pebs_events;
 
 	/*
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index bf0a64c..f42562e 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1691,7 +1691,7 @@ static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
 	intel_put_shared_regs_event_constraints(cpuc, event);
 }
 
-static void intel_pebs_aliases_core2(struct perf_event *event)
+static void intel_pebs_aliases_core2(struct perf_event *event, bool *changed)
 {
 	if ((event->hw.config & X86_RAW_EVENT_MASK) == 0x003c) {
 		/*
@@ -1716,10 +1716,11 @@ static void intel_pebs_aliases_core2(struct perf_event *event)
 
 		alt_config |= (event->hw.config & ~X86_RAW_EVENT_MASK);
 		event->hw.config = alt_config;
+		*changed = true;
 	}
 }
 
-static void intel_pebs_aliases_snb(struct perf_event *event)
+static void intel_pebs_aliases_snb(struct perf_event *event, bool *changed)
 {
 	if ((event->hw.config & X86_RAW_EVENT_MASK) == 0x003c) {
 		/*
@@ -1744,18 +1745,26 @@ static void intel_pebs_aliases_snb(struct perf_event *event)
 
 		alt_config |= (event->hw.config & ~X86_RAW_EVENT_MASK);
 		event->hw.config = alt_config;
+		*changed = true;
 	}
 }
 
+#define ARCH_PERFMON_NOT_WITH_PEBS 	\
+	(ARCH_PERFMON_EVENTSEL_ANY | 	\
+	 ARCH_PERFMON_EVENTSEL_CMASK | 	\
+	 ARCH_PERFMON_EVENTSEL_EDGE | 	\
+	 ARCH_PERFMON_EVENTSEL_INV)
+
 static int intel_pmu_hw_config(struct perf_event *event)
 {
+	bool changed = false;
 	int ret = x86_pmu_hw_config(event);
 
 	if (ret)
 		return ret;
 
 	if (event->attr.precise_ip && x86_pmu.pebs_aliases)
-		x86_pmu.pebs_aliases(event);
+		x86_pmu.pebs_aliases(event, &changed);
 
 	if (intel_pmu_needs_lbr_smpl(event)) {
 		ret = intel_pmu_setup_lbr_filter(event);
@@ -1766,6 +1775,17 @@ static int intel_pmu_hw_config(struct perf_event *event)
 	if (event->attr.type != PERF_TYPE_RAW)
 		return 0;
 
+	/* 
+	 * SDM Vol 3 18.8.4: 
+	 * "PEBS events are only valid when the following fields of 
+	 * IA32_PERFEVTSELx are all zero: AnyThread, Edge, Invert, Cmask.
+	 *
+	 * We only make an exception for the magic pebs aliases.
+	 */
+	if (event->attr.precise_ip && !changed &&
+		(event->attr.config & ARCH_PERFMON_NOT_WITH_PEBS))
+		return -EINVAL;
+
 	if (!(event->attr.config & ARCH_PERFMON_EVENTSEL_ANY))
 		return 0;
 
-- 
1.8.5.3

[PATCH 2/2] perf, x86: Don't mark DataLA addresses as store

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Haswell supports reporting the data address for a range
of events, including UOPS_RETIRED.ALL and some load
events. Currently these addresses were always marked
as stores, which is wrong, as they could be loads too.
Change it to NA instead.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event_intel_ds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
index 77bdb09..7a319cf 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
@@ -113,7 +113,7 @@ static u64 precise_store_data_hsw(u64 status)
 	union perf_mem_data_src dse;
 
 	dse.val = 0;
-	dse.mem_op = PERF_MEM_OP_STORE;
+	dse.mem_op = PERF_MEM_OP_NA;
 	dse.mem_lvl = PERF_MEM_LVL_NA;
 	if (status & 1)
 		dse.mem_lvl = PERF_MEM_LVL_L1;
-- 
1.8.5.3

Re: [PATCH 1/2] perf, x86: Disallow setting undefined bits for PEBS events

[Peter Zijlstra]

On Thu, Mar 13, 2014 at 02:22:12PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> The SDM forbids setting various event qualifiers with PEBS
> events. The magic cycles:pp event uses it, but it has caused
> problems in the past. We continue allowing it for cycles:pp,
> but forbid it for all other events to follow the SDM.

Last time this came up I asked for what kind of problems it caused.

No answer then I think, and certainly no answer now.

Re: [PATCH 1/2] perf, x86: Disallow setting undefined bits for PEBS events

[Andi Kleen]

On Fri, Mar 14, 2014 at 09:13:38AM +0100, Peter Zijlstra wrote:
> On Thu, Mar 13, 2014 at 02:22:12PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > The SDM forbids setting various event qualifiers with PEBS
> > events. The magic cycles:pp event uses it, but it has caused
> > problems in the past. We continue allowing it for cycles:pp,
> > but forbid it for all other events to follow the SDM.
> 
> Last time this came up I asked for what kind of problems it caused.

The original PEBS lockup on Sandy Bridge was caused by these bits.
The person who architected the hardware strongly advised to not
use them with PEBS events. 

Besides we do what the SDM tells us to do. Do you disagree
with that?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 1/2] perf, x86: Disallow setting undefined bits for PEBS events

[Peter Zijlstra]

On Fri, Mar 14, 2014 at 06:54:39AM -0700, Andi Kleen wrote:
> On Fri, Mar 14, 2014 at 09:13:38AM +0100, Peter Zijlstra wrote:
> > On Thu, Mar 13, 2014 at 02:22:12PM -0700, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > The SDM forbids setting various event qualifiers with PEBS
> > > events. The magic cycles:pp event uses it, but it has caused
> > > problems in the past. We continue allowing it for cycles:pp,
> > > but forbid it for all other events to follow the SDM.
> > 
> > Last time this came up I asked for what kind of problems it caused.
> 
> The original PEBS lockup on Sandy Bridge was caused by these bits.

I reread that thread (non-public sorry) and no; it was a clear hardware
fail there that got INST_RETIRED.ANY_P (0x00c0) retracted as PEBS
capable. It wasn't the extra bits, it would take your machine down just
fine without them.

And the work around suggested by Intel (the one currently in
intel_pebs_alias_snb) also didn't fail because of those extra bits
AFAIK. The thread suggests something else -- but again, that's all
non-public :-(

The very fact that we have Intel sanctioned events with those bits in
says they're not universally bad.

> The person who architected the hardware strongly advised to not
> use them with PEBS events.

That's a non statement; it doesn't contain anything useful, such
statements must be qualified with why he advises against them.

If its purely because the results are not guaranteed; we don't care. We
allow programming all kinds of crap into normal counters too; you get to
keep whatever nonsense the CPU counts.

> Besides we do what the SDM tells us to do. Do you disagree
> with that?

We don't, that thing is plenty wrong.

So stop trying to play the non-information game and state something
concrete already. Then again, frankly I don't trust you anymore so
pretty much anything you say will be doubted.