perf intel-pt: Fixes

PATCH Applied Adrian Hunter 2 participants 9 messages Dec 10, 2021 perf events

Share & tools

Adrian Hunter

author reviewer

2021-12-10 16:23 UTC #0 lore raw

cover perf intel-pt: Fixes

Here are a few fairly small Intel PT fixes. Backports would be needed to apply before v5.15, so the stable tags reflect that.

Adrian Hunter (7):
perf intel-pt: Fix some PGE usage
perf intel-pt: Fix sync state
perf intel-pt: Fix intel_pt_fup_event() assumptions perf intel-pt: Fix overflow state
perf intel-pt: Fix next 'err' value, walking trace perf intel-pt: Fix missing 'instruction' events with 'q' option perf intel-pt: Fix error timestamp

 .../perf/util/intel-pt-decoder/intel-pt-decoder.c  | 85 ++++++++++++++--------
 tools/perf/util/intel-pt.c                         |  1 +
 2 files changed, 56 insertions(+), 30 deletions(-)

Regards
Adrian

Adrian Hunter

author reviewer

2021-12-10 16:23 UTC #1 lore raw

PATCH 1/7 perf intel-pt: Fix some PGE usage

Packet generation enable (PGE) refers to whether control flow (COFI) packets are being produced. PGE may be false even when branch-tracing is enabled, due to being out-of-context, or outside a filter address range. Fix some missing PGE usage.

Fixes 839598176b0554 ("perf intel-pt: Allow decoding with branch tracing disabled")

Fixes 7c1b16ba0e26e6 ("perf intel-pt: Add support for decoding FUP/TIP only")

Cc [email protected] # v5.15+

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/intel-pt-decoder/intel-pt-decoder.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

tools/perf/util/intel-pt-decoder/intel-pt-decoder.c +4 -3

index 5f83937bf8f3..6f6f163161a9 100644
@@ -2678,6 +2678,7 @@ static int intel_pt_hop_trace(struct intel_pt_decoder *decoder, bool *no_tip, in
 		return HOP_IGNORE;
 
 	case INTEL_PT_TIP_PGD:
+		decoder->pge = false;
 		if (!decoder->packet.count) {
 			intel_pt_set_nr(decoder);
 			return HOP_IGNORE;

@@ -2707,7 +2708,7 @@ static int intel_pt_hop_trace(struct intel_pt_decoder *decoder, bool *no_tip, in
 		intel_pt_set_ip(decoder);
 		if (intel_pt_fup_event(decoder))
 			return HOP_RETURN;
-		if (!decoder->branch_enable)
+		if (!decoder->branch_enable || !decoder->pge)
 			*no_tip = true;
 		if (*no_tip) {
 			decoder->state.type = INTEL_PT_INSTRUCTION;

@@ -2897,7 +2898,7 @@ static bool intel_pt_psb_with_fup(struct intel_pt_decoder *decoder, int *err)
 {
 	struct intel_pt_psb_info data = { .fup = false };
 
-	if (!decoder->branch_enable || !decoder->pge)
+	if (!decoder->branch_enable)
 		return false;
 
 	intel_pt_pkt_lookahead(decoder, intel_pt_psb_lookahead_cb, &data);

@@ -2999,7 +3000,7 @@ static int intel_pt_walk_trace(struct intel_pt_decoder *decoder)
 				break;
 			}
 			intel_pt_set_last_ip(decoder);
-			if (!decoder->branch_enable) {
+			if (!decoder->branch_enable || !decoder->pge) {
 				decoder->ip = decoder->last_ip;
 				if (intel_pt_fup_event(decoder))
 					return 0;

Signature · 1 line

2.25.1

Adrian Hunter

author reviewer

2021-12-10 16:23 UTC #2 lore raw

PATCH 2/7 perf intel-pt: Fix sync state

In reply to Adrian Hunter

When syncing, it may be that branch packet generation is not enabled at that point, in which case there will not immediately be a control-flow packet, so some packets before a control flow packet turns up, get ignored. However, the decoder is in sync as soon as a PSB is found, so the state should be set accordingly.

Fixes f4aa081949e7b6 ("perf tools: Add Intel PT decoder")

Cc [email protected] # v5.15+

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/intel-pt-decoder/intel-pt-decoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/util/intel-pt-decoder/intel-pt-decoder.c +1 -1

index 6f6f163161a9..bddf98123dc3 100644
@@ -3608,7 +3608,7 @@ static int intel_pt_sync(struct intel_pt_decoder *decoder)
 	}
 
 	decoder->have_last_ip = true;
-	decoder->pkt_state = INTEL_PT_STATE_NO_IP;
+	decoder->pkt_state = INTEL_PT_STATE_IN_SYNC;
 
 	err = intel_pt_walk_psb(decoder);
 	if (err)

Signature · 1 line

2.25.1

Adrian Hunter

author reviewer

2021-12-10 16:23 UTC #3 lore raw

PATCH 3/7 perf intel-pt: Fix intel_pt_fup_event() assumptions

In reply to Adrian Hunter

intel_pt_fup_event() assumes it can overwrite the state type if there has been an FUP event, but this is an unnecessary and unexpected constraint on callers. Fix by touching only the state type flags that are affected by an FUP event.

Fixes a472e65fc490a ("perf intel-pt: Add decoder support for ptwrite and power event packets")

Cc [email protected] # v5.15+

Signed-off-by Adrian Hunter <[email protected]>

---
 .../util/intel-pt-decoder/intel-pt-decoder.c  | 32 ++++++++-----------
 1 file changed, 13 insertions(+), 19 deletions(-)

tools/perf/util/intel-pt-decoder/intel-pt-decoder.c +13 ~~-19~~

index bddf98123dc3..16fbbf07e367 100644
@@ -1205,61 +1205,55 @@ static int intel_pt_walk_insn(struct intel_pt_decoder *decoder,
 
 static bool intel_pt_fup_event(struct intel_pt_decoder *decoder)
 {
+	enum intel_pt_sample_type type = decoder->state.type;
 	bool ret = false;
 
+	decoder->state.type &= ~INTEL_PT_BRANCH;
+
 	if (decoder->set_fup_tx_flags) {
 		decoder->set_fup_tx_flags = false;
 		decoder->tx_flags = decoder->fup_tx_flags;
-		decoder->state.type = INTEL_PT_TRANSACTION;
+		decoder->state.type |= INTEL_PT_TRANSACTION;
 		if (decoder->fup_tx_flags & INTEL_PT_ABORT_TX)
 			decoder->state.type |= INTEL_PT_BRANCH;
-		decoder->state.from_ip = decoder->ip;
-		decoder->state.to_ip = 0;
 		decoder->state.flags = decoder->fup_tx_flags;
-		return true;
+		ret = true;
 	}
 	if (decoder->set_fup_ptw) {
 		decoder->set_fup_ptw = false;
-		decoder->state.type = INTEL_PT_PTW;
+		decoder->state.type |= INTEL_PT_PTW;
 		decoder->state.flags |= INTEL_PT_FUP_IP;
-		decoder->state.from_ip = decoder->ip;
-		decoder->state.to_ip = 0;
 		decoder->state.ptw_payload = decoder->fup_ptw_payload;
-		return true;
+		ret = true;
 	}
 	if (decoder->set_fup_mwait) {
 		decoder->set_fup_mwait = false;
-		decoder->state.type = INTEL_PT_MWAIT_OP;
-		decoder->state.from_ip = decoder->ip;
-		decoder->state.to_ip = 0;
+		decoder->state.type |= INTEL_PT_MWAIT_OP;
 		decoder->state.mwait_payload = decoder->fup_mwait_payload;
 		ret = true;
 	}
 	if (decoder->set_fup_pwre) {
 		decoder->set_fup_pwre = false;
 		decoder->state.type |= INTEL_PT_PWR_ENTRY;
-		decoder->state.type &= ~INTEL_PT_BRANCH;
-		decoder->state.from_ip = decoder->ip;
-		decoder->state.to_ip = 0;
 		decoder->state.pwre_payload = decoder->fup_pwre_payload;
 		ret = true;
 	}
 	if (decoder->set_fup_exstop) {
 		decoder->set_fup_exstop = false;
 		decoder->state.type |= INTEL_PT_EX_STOP;
-		decoder->state.type &= ~INTEL_PT_BRANCH;
 		decoder->state.flags |= INTEL_PT_FUP_IP;
-		decoder->state.from_ip = decoder->ip;
-		decoder->state.to_ip = 0;
 		ret = true;
 	}
 	if (decoder->set_fup_bep) {
 		decoder->set_fup_bep = false;
 		decoder->state.type |= INTEL_PT_BLK_ITEMS;
-		decoder->state.type &= ~INTEL_PT_BRANCH;
+		ret = true;
+	}
+	if (ret) {
 		decoder->state.from_ip = decoder->ip;
 		decoder->state.to_ip = 0;
-		ret = true;
+	} else {
+		decoder->state.type = type;
 	}
 	return ret;
 }

Signature · 1 line

2.25.1

Adrian Hunter

author reviewer

2021-12-10 16:23 UTC #4 lore raw

PATCH 4/7 perf intel-pt: Fix overflow state

In reply to Adrian Hunter

An overflow (OVF packet) is treated as an error because it represents a loss of trace data, but there is no loss of synchronization, so the packet state should be INTEL_PT_STATE_IN_SYNC not INTEL_PT_STATE_ERR_RESYNC. To support that, some additional variables must be reset, and the FUP packet that may follow OVF is treated as an FUP event.

Fixes f4aa081949e7b6 ("perf tools: Add Intel PT decoder")

Cc [email protected] # v5.15+

Signed-off-by Adrian Hunter <[email protected]>

---
 .../util/intel-pt-decoder/intel-pt-decoder.c  | 32 ++++++++++++++++---
 1 file changed, 28 insertions(+), 4 deletions(-)

tools/perf/util/intel-pt-decoder/intel-pt-decoder.c +28 -4

index 16fbbf07e367..845b0ca866a4 100644
@@ -1249,6 +1249,20 @@ static bool intel_pt_fup_event(struct intel_pt_decoder *decoder)
 		decoder->state.type |= INTEL_PT_BLK_ITEMS;
 		ret = true;
 	}
+	if (decoder->overflow) {
+		decoder->overflow = false;
+		if (!ret && !decoder->pge) {
+			if (decoder->hop) {
+				decoder->state.type = 0;
+				decoder->pkt_state = INTEL_PT_STATE_RESAMPLE;
+			}
+			decoder->pge = true;
+			decoder->state.type |= INTEL_PT_BRANCH | INTEL_PT_TRACE_BEGIN;
+			decoder->state.from_ip = 0;
+			decoder->state.to_ip = decoder->ip;
+			return true;
+		}
+	}
 	if (ret) {
 		decoder->state.from_ip = decoder->ip;
 		decoder->state.to_ip = 0;

@@ -1602,7 +1616,16 @@ static int intel_pt_overflow(struct intel_pt_decoder *decoder)
 	intel_pt_clear_tx_flags(decoder);
 	intel_pt_set_nr(decoder);
 	decoder->timestamp_insn_cnt = 0;
-	decoder->pkt_state = INTEL_PT_STATE_ERR_RESYNC;
+	decoder->pkt_state = INTEL_PT_STATE_IN_SYNC;
+	decoder->state.from_ip = decoder->ip;
+	decoder->ip = 0;
+	decoder->pge = false;
+	decoder->set_fup_tx_flags = false;
+	decoder->set_fup_ptw = false;
+	decoder->set_fup_mwait = false;
+	decoder->set_fup_pwre = false;
+	decoder->set_fup_exstop = false;
+	decoder->set_fup_bep = false;
 	decoder->overflow = true;
 	return -EOVERFLOW;
 }

@@ -2957,6 +2980,7 @@ static int intel_pt_walk_trace(struct intel_pt_decoder *decoder)
 
 		case INTEL_PT_TIP_PGE: {
 			decoder->pge = true;
+			decoder->overflow = false;
 			intel_pt_mtc_cyc_cnt_pge(decoder);
 			intel_pt_set_nr(decoder);
 			if (decoder->packet.count == 0) {

@@ -3462,10 +3486,10 @@ static int intel_pt_sync_ip(struct intel_pt_decoder *decoder)
 	decoder->set_fup_pwre = false;
 	decoder->set_fup_exstop = false;
 	decoder->set_fup_bep = false;
+	decoder->overflow = false;
 
 	if (!decoder->branch_enable) {
 		decoder->pkt_state = INTEL_PT_STATE_IN_SYNC;
-		decoder->overflow = false;
 		decoder->state.type = 0; /* Do not have a sample */
 		return 0;
 	}

@@ -3480,7 +3504,6 @@ static int intel_pt_sync_ip(struct intel_pt_decoder *decoder)
 		decoder->pkt_state = INTEL_PT_STATE_RESAMPLE;
 	else
 		decoder->pkt_state = INTEL_PT_STATE_IN_SYNC;
-	decoder->overflow = false;
 
 	decoder->state.from_ip = 0;
 	decoder->state.to_ip = decoder->ip;

@@ -3699,7 +3722,8 @@ const struct intel_pt_state *intel_pt_decode(struct intel_pt_decoder *decoder)
 
 	if (err) {
 		decoder->state.err = intel_pt_ext_err(err);
-		decoder->state.from_ip = decoder->ip;
+		if (err != -EOVERFLOW)
+			decoder->state.from_ip = decoder->ip;
 		intel_pt_update_sample_time(decoder);
 		decoder->sample_tot_cyc_cnt = decoder->tot_cyc_cnt;
 		intel_pt_set_nr(decoder);

Signature · 1 line

2.25.1

Adrian Hunter

author reviewer

2021-12-10 16:23 UTC #5 lore raw

PATCH 5/7 perf intel-pt: Fix next 'err' value, walking trace

In reply to Adrian Hunter

Code after label 'next:' in intel_pt_walk_trace() assumes 'err' is zero, but it may not be, if arrived at via a 'goto'. Ensure it is zero.

Fixes 7c1b16ba0e26e6 ("perf intel-pt: Add support for decoding FUP/TIP only")

Cc [email protected] # v5.15+

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/intel-pt-decoder/intel-pt-decoder.c | 1 +
 1 file changed, 1 insertion(+)

tools/perf/util/intel-pt-decoder/intel-pt-decoder.c +1 -0

index 845b0ca866a4..75b504aed7f4 100644
@@ -2942,6 +2942,7 @@ static int intel_pt_walk_trace(struct intel_pt_decoder *decoder)
 		if (err)
 			return err;
 next:
+		err = 0;
 		if (decoder->cyc_threshold) {
 			if (decoder->sample_cyc && last_packet_type != INTEL_PT_CYC)
 				decoder->sample_cyc = false;

Signature · 1 line

2.25.1

Adrian Hunter

author reviewer

2021-12-10 16:23 UTC #6 lore raw

PATCH 6/7 perf intel-pt: Fix missing 'instruction' events with 'q' option

In reply to Adrian Hunter

FUP packets contain IP information, which makes them also an 'instruction' event in 'hop' mode i.e. the itrace 'q' option. That wasn't happening, so restructure the logic so that FUP events are added along with appropriate 'instruction' and 'branch' events.

Fixes 7c1b16ba0e26e6 ("perf intel-pt: Add support for decoding FUP/TIP only")

Cc [email protected] # v5.15+

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/intel-pt-decoder/intel-pt-decoder.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

tools/perf/util/intel-pt-decoder/intel-pt-decoder.c +8 -3

index 75b504aed7f4..0e013c2d9eb4 100644
@@ -2683,6 +2683,8 @@ static int intel_pt_scan_for_psb(struct intel_pt_decoder *decoder);
 /* Hop mode: Ignore TNT, do not walk code, but get ip from FUPs and TIPs */
 static int intel_pt_hop_trace(struct intel_pt_decoder *decoder, bool *no_tip, int *err)
 {
+	*err = 0;
+
 	/* Leap from PSB to PSB, getting ip from FUP within PSB+ */
 	if (decoder->leap && !decoder->in_psb && decoder->packet.type != INTEL_PT_PSB) {
 		*err = intel_pt_scan_for_psb(decoder);

@@ -2723,18 +2725,21 @@ static int intel_pt_hop_trace(struct intel_pt_decoder *decoder, bool *no_tip, in
 		if (!decoder->packet.count)
 			return HOP_IGNORE;
 		intel_pt_set_ip(decoder);
-		if (intel_pt_fup_event(decoder))
-			return HOP_RETURN;
+		if (decoder->set_fup_mwait || decoder->set_fup_pwre)
+			*no_tip = true;
 		if (!decoder->branch_enable || !decoder->pge)
 			*no_tip = true;
 		if (*no_tip) {
 			decoder->state.type = INTEL_PT_INSTRUCTION;
 			decoder->state.from_ip = decoder->ip;
 			decoder->state.to_ip = 0;
+			intel_pt_fup_event(decoder);
 			return HOP_RETURN;
 		}
+		intel_pt_fup_event(decoder);
+		decoder->state.type |= INTEL_PT_INSTRUCTION | INTEL_PT_BRANCH;
 		*err = intel_pt_walk_fup_tip(decoder);
-		if (!*err)
+		if (!*err && decoder->state.to_ip)
 			decoder->pkt_state = INTEL_PT_STATE_RESAMPLE;
 		return HOP_RETURN;

Signature · 1 line

2.25.1

Adrian Hunter

author reviewer

2021-12-10 16:23 UTC #7 lore raw

PATCH 7/7 perf intel-pt: Fix error timestamp

In reply to Adrian Hunter

An error timestamp shows the last known timestamp for the queue, but this is not updated on the error path. Fix by setting it.

Fixes f4aa081949e7b6 ("perf tools: Add Intel PT decoder")

Cc [email protected] # v5.15+

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/intel-pt.c | 1 +
 1 file changed, 1 insertion(+)

tools/perf/util/intel-pt.c +1 -0

index 556a893508da..10c3187e4c5a 100644
@@ -2565,6 +2565,7 @@ static int intel_pt_run_decoder(struct intel_pt_queue *ptq, u64 *timestamp)
 				ptq->sync_switch = false;
 				intel_pt_next_tid(pt, ptq);
 			}
+			ptq->timestamp = state->est_timestamp;
 			if (pt->synth_opts.errors) {
 				err = intel_ptq_synth_error(ptq, state);
 				if (err)

Signature · 1 line

2.25.1

Arnaldo Carvalho de Melo

maintainer Applied

2021-12-10 18:40 UTC #8 lore raw

In reply to Adrian Hunter

Em Fri, Dec 10, 2021 at 06:22:56PM +0200, Adrian Hunter escreveu:

Quoted 5 lines

Hi
Here are a few fairly small Intel PT fixes. Backports would be needed to apply before v5.15, so the stable tags reflect that.

Thanks, applied.

- Arnaldo

Quoted 16 lines

Adrian Hunter (7):
perf intel-pt: Fix some PGE usage
perf intel-pt: Fix sync state
perf intel-pt: Fix intel_pt_fup_event() assumptions perf intel-pt: Fix overflow state
perf intel-pt: Fix next 'err' value, walking trace perf intel-pt: Fix missing 'instruction' events with 'q' option perf intel-pt: Fix error timestamp
 .../perf/util/intel-pt-decoder/intel-pt-decoder.c  | 85 ++++++++++++++--------
 tools/perf/util/intel-pt.c                         |  1 +
 2 files changed, 56 insertions(+), 30 deletions(-)
Regards
Adrian

Signature · 1 line

- Arnaldo