Re: perf_fuzzer crash on pentium 4

[Cyrill Gorcunov]

On Wed, May 07, 2014 at 10:00:50PM -0400, Don Zickus wrote:
> 
> I think my commit 13beacee817d27a40ffc6f065ea0042685611dd5 explains this
> corruption.  Though I have to admit I haven't looked through the problem
> very closely yet.
> 
> IOW my lazy fix in that commit doesn't cover fuzzers and the real problem
> in p4_pmu_schedule_events. :-)

Don, Vince, could you please give the patch a run? I've only compile tested
it obviously since I've no real p4 hw. And the patch itself is a bit ugly
but should bring the light if we're still having problems in events
scheduling.
---
 arch/x86/kernel/cpu/perf_event_p4.c |   61 +++++++++++++++---------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event_p4.c
+++ linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
@@ -1063,23 +1063,23 @@ static int p4_pmu_handle_irq(struct pt_r
  * swap thread specific fields according to a thread
  * we are going to run on
  */
-static void p4_pmu_swap_config_ts(struct hw_perf_event *hwc, int cpu)
+static u64 p4_pmu_swap_config_ts(u64 config, int cpu)
 {
 	u32 escr, cccr;
 
 	/*
 	 * we either lucky and continue on same cpu or no HT support
 	 */
-	if (!p4_should_swap_ts(hwc->config, cpu))
-		return;
+	if (!p4_should_swap_ts(config, cpu))
+		return config;
 
 	/*
 	 * the event is migrated from an another logical
 	 * cpu, so we need to swap thread specific flags
 	 */
 
-	escr = p4_config_unpack_escr(hwc->config);
-	cccr = p4_config_unpack_cccr(hwc->config);
+	escr = p4_config_unpack_escr(config);
+	cccr = p4_config_unpack_cccr(config);
 
 	if (p4_ht_thread(cpu)) {
 		cccr &= ~P4_CCCR_OVF_PMI_T0;
@@ -1092,9 +1092,9 @@ static void p4_pmu_swap_config_ts(struct
 			escr &= ~P4_ESCR_T0_USR;
 			escr |= P4_ESCR_T1_USR;
 		}
-		hwc->config  = p4_config_pack_escr(escr);
-		hwc->config |= p4_config_pack_cccr(cccr);
-		hwc->config |= P4_CONFIG_HT;
+		config  = p4_config_pack_escr(escr);
+		config |= p4_config_pack_cccr(cccr);
+		config |= P4_CONFIG_HT;
 	} else {
 		cccr &= ~P4_CCCR_OVF_PMI_T1;
 		cccr |= P4_CCCR_OVF_PMI_T0;
@@ -1106,10 +1106,12 @@ static void p4_pmu_swap_config_ts(struct
 			escr &= ~P4_ESCR_T1_USR;
 			escr |= P4_ESCR_T0_USR;
 		}
-		hwc->config  = p4_config_pack_escr(escr);
-		hwc->config |= p4_config_pack_cccr(cccr);
-		hwc->config &= ~P4_CONFIG_HT;
+		config  = p4_config_pack_escr(escr);
+		config |= p4_config_pack_cccr(cccr);
+		config &= ~P4_CONFIG_HT;
 	}
+
+	return config;
 }
 
 /*
@@ -1208,6 +1210,7 @@ static int p4_pmu_schedule_events(struct
 	unsigned long used_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
 	unsigned long escr_mask[BITS_TO_LONGS(P4_ESCR_MSR_TABLE_SIZE)];
 	int cpu = smp_processor_id();
+	u64 config[X86_PMC_IDX_MAX];
 	struct hw_perf_event *hwc;
 	struct p4_event_bind *bind;
 	unsigned int i, thread, num;
@@ -1233,12 +1236,13 @@ again:
 		if (pass > 2)
 			goto done;
 
-		bind = p4_config_get_bind(hwc->config);
+		config[i] = hwc->config;
+		bind = p4_config_get_bind(config[i]);
 		escr_idx = p4_get_escr_idx(bind->escr_msr[thread]);
 		if (unlikely(escr_idx == -1))
 			goto done;
 
-		if (hwc->idx != -1 && !p4_should_swap_ts(hwc->config, cpu)) {
+		if (hwc->idx != -1 && !p4_should_swap_ts(config[i], cpu)) {
 			cntr_idx = hwc->idx;
 			if (assign)
 				assign[i] = hwc->idx;
@@ -1250,32 +1254,15 @@ again:
 			/*
 			 * Check whether an event alias is still available.
 			 */
-			config_alias = p4_get_alias_event(hwc->config);
+			config_alias = p4_get_alias_event(config[i]);
 			if (!config_alias)
 				goto done;
-			hwc->config = config_alias;
+			config[i] = config_alias;
 			pass++;
 			goto again;
 		}
-		/*
-		 * Perf does test runs to see if a whole group can be assigned
-		 * together succesfully.  There can be multiple rounds of this.
-		 * Unfortunately, p4_pmu_swap_config_ts touches the hwc->config
-		 * bits, such that the next round of group assignments will
-		 * cause the above p4_should_swap_ts to pass instead of fail.
-		 * This leads to counters exclusive to thread0 being used by
-		 * thread1.
-		 *
-		 * Solve this with a cheap hack, reset the idx back to -1 to
-		 * force a new lookup (p4_next_cntr) to get the right counter
-		 * for the right thread.
-		 *
-		 * This probably doesn't comply with the general spirit of how
-		 * perf wants to work, but P4 is special. :-(
-		 */
-		if (p4_should_swap_ts(hwc->config, cpu))
-			hwc->idx = -1;
-		p4_pmu_swap_config_ts(hwc, cpu);
+
+		config[i] = p4_pmu_swap_config_ts(config[i], cpu);
 		if (assign)
 			assign[i] = cntr_idx;
 reserve:
@@ -1284,6 +1271,12 @@ reserve:
 	}
 
 done:
+	if (num == 0) {
+		for (i = 0; i < n; i++, num--) {
+			hwc = &cpuc->event_list[i]->hw;
+			hwc->config = config[i];
+		}
+	}
 	return num ? -EINVAL : 0;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/