[PATCH v6 00/21] perf arm-spe: Refactor decoding & dumping flow

[PATCH v6 00/21] perf arm-spe: Refactor decoding & dumping flow

[Leo Yan]

This is patch set v6 for refactoring Arm SPE trace decoding and dumping.
It follows Andre's comment to directly bail out arm_spe_pkt_snprintf()
if any error occurred.

This patch set is cleanly applied on the top of perf/core branch
with commit 7cf726a59435 ("Merge tag 'linux-kselftest-kunit-5.10-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest").

Have tested this patch set on Hisilicon D06 platform with commands
"perf report -D" and "perf script", compared the decoding results
between with this patch set and without this patch set, didn't find
issue with "diff" tool.

Changes from v5:
- Directly bail out arm_spe_pkt_snprintf() if any error occurred
  (Andre).

Changes from v4:
- Implemented a cumulative error for arm_spe_pkt_snprintf() and changed
  to condense code for printing strings (Dave);
- Changed to check payload bits [55:52] for parse kernel address
  (Andre).

Changes from v3:
- Refined arm_spe_payload_len() and removed macro SPE_HEADER_SZ()
  (Andre);
- Refined packet header index macros (Andre);
- Added patch "perf arm_spe: Fixup top byte for data virtual address" to
  fixup the data virtual address for 64KB pages and refined comments for
  the fixup (Andre);
- Added Andre's review tag (using "b4 am" command);
- Changed the macros to SPE_PKT_IS_XXX() format to check operation types
  (Andre);


Andre Przywara (1):
  perf arm_spe: Decode memory tagging properties

Leo Yan (19):
  perf arm-spe: Include bitops.h for BIT() macro
  perf arm-spe: Fix a typo in comment
  perf arm-spe: Refactor payload size calculation
  perf arm-spe: Refactor arm_spe_get_events()
  perf arm-spe: Fix packet length handling
  perf arm-spe: Refactor printing string to buffer
  perf arm-spe: Refactor packet header parsing
  perf arm-spe: Add new function arm_spe_pkt_desc_addr()
  perf arm-spe: Refactor address packet handling
  perf arm_spe: Fixup top byte for data virtual address
  perf arm-spe: Refactor context packet handling
  perf arm-spe: Add new function arm_spe_pkt_desc_counter()
  perf arm-spe: Refactor counter packet handling
  perf arm-spe: Add new function arm_spe_pkt_desc_event()
  perf arm-spe: Refactor event type handling
  perf arm-spe: Remove size condition checking for events
  perf arm-spe: Add new function arm_spe_pkt_desc_op_type()
  perf arm-spe: Refactor operation packet handling
  perf arm-spe: Add more sub classes for operation packet

Wei Li (1):
  perf arm-spe: Add support for ARMv8.3-SPE

 .../util/arm-spe-decoder/arm-spe-decoder.c    |  59 +-
 .../util/arm-spe-decoder/arm-spe-decoder.h    |  17 -
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 568 ++++++++++--------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 122 +++-
 4 files changed, 445 insertions(+), 321 deletions(-)

-- 
2.17.1

[PATCH v6 01/21] perf arm-spe: Include bitops.h for BIT() macro

[Leo Yan]

Include header linux/bitops.h, directly use its BIT() macro and remove
the self defined macros.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-decoder.c     | 5 +----
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 3 +--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
index 93e063f22be5..cc18a1e8c212 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
@@ -12,6 +12,7 @@
 #include <string.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <linux/bitops.h>
 #include <linux/compiler.h>
 #include <linux/zalloc.h>
 
@@ -21,10 +22,6 @@
 
 #include "arm-spe-decoder.h"
 
-#ifndef BIT
-#define BIT(n)		(1UL << (n))
-#endif
-
 static u64 arm_spe_calc_ip(int index, u64 payload)
 {
 	u8 *addr = (u8 *)&payload;
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index b94001b756c7..46ddb53a6457 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -8,11 +8,10 @@
 #include <string.h>
 #include <endian.h>
 #include <byteswap.h>
+#include <linux/bitops.h>
 
 #include "arm-spe-pkt-decoder.h"
 
-#define BIT(n)		(1ULL << (n))
-
 #define NS_FLAG		BIT(63)
 #define EL_FLAG		(BIT(62) | BIT(61))
 
-- 
2.17.1

[PATCH v6 02/21] perf arm-spe: Fix a typo in comment

[Leo Yan]

Fix a typo: s/iff/if.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 46ddb53a6457..7c7b5eb09fba 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -142,7 +142,7 @@ static int arm_spe_get_events(const unsigned char *buf, size_t len,
 
 	/* we use index to identify Events with a less number of
 	 * comparisons in arm_spe_pkt_desc(): E.g., the LLC-ACCESS,
-	 * LLC-REFILL, and REMOTE-ACCESS events are identified iff
+	 * LLC-REFILL, and REMOTE-ACCESS events are identified if
 	 * index > 1.
 	 */
 	packet->index = ret - 1;
-- 
2.17.1

[PATCH v6 03/21] perf arm-spe: Refactor payload size calculation

[Leo Yan]

This patch defines macro to extract "sz" field from header, and renames
the function payloadlen() to arm_spe_payload_len().

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../util/arm-spe-decoder/arm-spe-pkt-decoder.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 7c7b5eb09fba..06b3eec4494e 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -69,22 +69,22 @@ const char *arm_spe_pkt_name(enum arm_spe_pkt_type type)
 	return arm_spe_packet_name[type];
 }
 
-/* return ARM SPE payload size from its encoding,
- * which is in bits 5:4 of the byte.
- * 00 : byte
- * 01 : halfword (2)
- * 10 : word (4)
- * 11 : doubleword (8)
+/*
+ * Extracts the field "sz" from header bits and converts to bytes:
+ *   00 : byte (1)
+ *   01 : halfword (2)
+ *   10 : word (4)
+ *   11 : doubleword (8)
  */
-static int payloadlen(unsigned char byte)
+static unsigned int arm_spe_payload_len(unsigned char hdr)
 {
-	return 1 << ((byte & 0x30) >> 4);
+	return 1U << ((hdr & GENMASK_ULL(5, 4)) >> 4);
 }
 
 static int arm_spe_get_payload(const unsigned char *buf, size_t len,
 			       struct arm_spe_pkt *packet)
 {
-	size_t payload_len = payloadlen(buf[0]);
+	size_t payload_len = arm_spe_payload_len(buf[0]);
 
 	if (len < 1 + payload_len)
 		return ARM_SPE_NEED_MORE_BYTES;
-- 
2.17.1

[PATCH v6 04/21] perf arm-spe: Refactor arm_spe_get_events()

[Leo Yan]

In function arm_spe_get_events(), the event packet's 'index' is assigned
as payload length, but the flow is not directive: it firstly gets the
packet length from the return value of arm_spe_get_payload(), the value
includes header length (1) and payload length:

  int ret = arm_spe_get_payload(buf, len, packet);

and then reduces header length from packet length, so finally get the
payload length:

  packet->index = ret - 1;

To simplify the code, this patch directly assigns payload length to
event packet's index; and at the end it calls arm_spe_get_payload() to
return the payload value.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 06b3eec4494e..f1b4cb008837 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -136,8 +136,6 @@ static int arm_spe_get_timestamp(const unsigned char *buf, size_t len,
 static int arm_spe_get_events(const unsigned char *buf, size_t len,
 			      struct arm_spe_pkt *packet)
 {
-	int ret = arm_spe_get_payload(buf, len, packet);
-
 	packet->type = ARM_SPE_EVENTS;
 
 	/* we use index to identify Events with a less number of
@@ -145,9 +143,9 @@ static int arm_spe_get_events(const unsigned char *buf, size_t len,
 	 * LLC-REFILL, and REMOTE-ACCESS events are identified if
 	 * index > 1.
 	 */
-	packet->index = ret - 1;
+	packet->index = arm_spe_payload_len(buf[0]);
 
-	return ret;
+	return arm_spe_get_payload(buf, len, packet);
 }
 
 static int arm_spe_get_data_source(const unsigned char *buf, size_t len,
-- 
2.17.1

[PATCH v6 05/21] perf arm-spe: Fix packet length handling

[Leo Yan]

When processing address packet and counter packet, if the packet
contains extended header, it misses to account the extra one byte for
header length calculation, thus returns the wrong packet length.

To correct the packet length calculation, one possible fixing is simply
to plus extra 1 for extended header, but will spread some duplicate code
in the flows for processing address packet and counter packet.
Alternatively, we can refine the function arm_spe_get_payload() to not
only support short header and allow it to support extended header, and
rely on it for the packet length calculation.

So this patch refactors function arm_spe_get_payload() with a new
argument 'ext_hdr' for support extended header; the packet processing
flows can invoke this function to unify the packet length calculation.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 34 +++++++------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index f1b4cb008837..04fd7fd7c15f 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -82,14 +82,15 @@ static unsigned int arm_spe_payload_len(unsigned char hdr)
 }
 
 static int arm_spe_get_payload(const unsigned char *buf, size_t len,
+			       unsigned char ext_hdr,
 			       struct arm_spe_pkt *packet)
 {
-	size_t payload_len = arm_spe_payload_len(buf[0]);
+	size_t payload_len = arm_spe_payload_len(buf[ext_hdr]);
 
-	if (len < 1 + payload_len)
+	if (len < 1 + ext_hdr + payload_len)
 		return ARM_SPE_NEED_MORE_BYTES;
 
-	buf++;
+	buf += 1 + ext_hdr;
 
 	switch (payload_len) {
 	case 1: packet->payload = *(uint8_t *)buf; break;
@@ -99,7 +100,7 @@ static int arm_spe_get_payload(const unsigned char *buf, size_t len,
 	default: return ARM_SPE_BAD_PACKET;
 	}
 
-	return 1 + payload_len;
+	return 1 + ext_hdr + payload_len;
 }
 
 static int arm_spe_get_pad(struct arm_spe_pkt *packet)
@@ -130,7 +131,7 @@ static int arm_spe_get_timestamp(const unsigned char *buf, size_t len,
 				 struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_TIMESTAMP;
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_events(const unsigned char *buf, size_t len,
@@ -145,14 +146,14 @@ static int arm_spe_get_events(const unsigned char *buf, size_t len,
 	 */
 	packet->index = arm_spe_payload_len(buf[0]);
 
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_data_source(const unsigned char *buf, size_t len,
 				   struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_DATA_SOURCE;
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_context(const unsigned char *buf, size_t len,
@@ -160,8 +161,7 @@ static int arm_spe_get_context(const unsigned char *buf, size_t len,
 {
 	packet->type = ARM_SPE_CONTEXT;
 	packet->index = buf[0] & 0x3;
-
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_op_type(const unsigned char *buf, size_t len,
@@ -169,41 +169,31 @@ static int arm_spe_get_op_type(const unsigned char *buf, size_t len,
 {
 	packet->type = ARM_SPE_OP_TYPE;
 	packet->index = buf[0] & 0x3;
-	return arm_spe_get_payload(buf, len, packet);
+	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
 static int arm_spe_get_counter(const unsigned char *buf, size_t len,
 			       const unsigned char ext_hdr, struct arm_spe_pkt *packet)
 {
-	if (len < 2)
-		return ARM_SPE_NEED_MORE_BYTES;
-
 	packet->type = ARM_SPE_COUNTER;
 	if (ext_hdr)
 		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
 	else
 		packet->index = buf[0] & 0x7;
 
-	packet->payload = le16_to_cpu(*(uint16_t *)(buf + 1));
-
-	return 1 + ext_hdr + 2;
+	return arm_spe_get_payload(buf, len, ext_hdr, packet);
 }
 
 static int arm_spe_get_addr(const unsigned char *buf, size_t len,
 			    const unsigned char ext_hdr, struct arm_spe_pkt *packet)
 {
-	if (len < 8)
-		return ARM_SPE_NEED_MORE_BYTES;
-
 	packet->type = ARM_SPE_ADDRESS;
 	if (ext_hdr)
 		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
 	else
 		packet->index = buf[0] & 0x7;
 
-	memcpy_le64(&packet->payload, buf + 1, 8);
-
-	return 1 + ext_hdr + 8;
+	return arm_spe_get_payload(buf, len, ext_hdr, packet);
 }
 
 static int arm_spe_do_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

[PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[Leo Yan]

When outputs strings to the decoding buffer with function snprintf(),
SPE decoder needs to detects if any error returns from snprintf() and if
so needs to directly bail out.  If snprintf() returns success, it needs
to update buffer pointer and reduce the buffer length so can continue to
output the next string into the consequent memory space.

This complex logics are spreading in the function arm_spe_pkt_desc() so
there has many duplicate codes for handling error detecting, increment
buffer pointer and decrement buffer size.

To avoid the duplicate code, this patch introduces a new helper function
arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
it's used by the caller arm_spe_pkt_desc(); if printing buffer is called
for multiple times in a flow, the error is a cumulative value and simply
returns its final value.

This patch also moves the variable 'blen' as the function's local
variable, this allows to remove the unnecessary braces and improve the
readability.

Suggested-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 267 ++++++++----------
 1 file changed, 117 insertions(+), 150 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 04fd7fd7c15f..1ecaf9805b79 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -9,6 +9,7 @@
 #include <endian.h>
 #include <byteswap.h>
 #include <linux/bitops.h>
+#include <stdarg.h>
 
 #include "arm-spe-pkt-decoder.h"
 
@@ -258,192 +259,158 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
 	return ret;
 }
 
+static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
+				const char *fmt, ...)
+{
+	va_list ap;
+	int ret;
+
+	/* Bail out if any error occurred */
+	if (err && *err)
+		return *err;
+
+	va_start(ap, fmt);
+	ret = vsnprintf(*buf_p, *blen, fmt, ap);
+	va_end(ap);
+
+	if (ret < 0) {
+		if (err && !*err)
+			*err = ret;
+	} else {
+		*buf_p += ret;
+		*blen -= ret;
+	}
+
+	return ret;
+}
+
 int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		     size_t buf_len)
 {
-	int ret, ns, el, idx = packet->index;
+	int ns, el, idx = packet->index;
 	unsigned long long payload = packet->payload;
 	const char *name = arm_spe_pkt_name(packet->type);
+	size_t blen = buf_len;
+	int err = 0;
 
 	switch (packet->type) {
 	case ARM_SPE_BAD:
 	case ARM_SPE_PAD:
 	case ARM_SPE_END:
-		return snprintf(buf, buf_len, "%s", name);
-	case ARM_SPE_EVENTS: {
-		size_t blen = buf_len;
-
-		ret = 0;
-		ret = snprintf(buf, buf_len, "EV");
-		buf += ret;
-		blen -= ret;
-		if (payload & 0x1) {
-			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
-			buf += ret;
-			blen -= ret;
-		}
-		if (payload & 0x2) {
-			ret = snprintf(buf, buf_len, " RETIRED");
-			buf += ret;
-			blen -= ret;
-		}
-		if (payload & 0x4) {
-			ret = snprintf(buf, buf_len, " L1D-ACCESS");
-			buf += ret;
-			blen -= ret;
-		}
-		if (payload & 0x8) {
-			ret = snprintf(buf, buf_len, " L1D-REFILL");
-			buf += ret;
-			blen -= ret;
-		}
-		if (payload & 0x10) {
-			ret = snprintf(buf, buf_len, " TLB-ACCESS");
-			buf += ret;
-			blen -= ret;
-		}
-		if (payload & 0x20) {
-			ret = snprintf(buf, buf_len, " TLB-REFILL");
-			buf += ret;
-			blen -= ret;
-		}
-		if (payload & 0x40) {
-			ret = snprintf(buf, buf_len, " NOT-TAKEN");
-			buf += ret;
-			blen -= ret;
-		}
-		if (payload & 0x80) {
-			ret = snprintf(buf, buf_len, " MISPRED");
-			buf += ret;
-			blen -= ret;
-		}
+		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
+	case ARM_SPE_EVENTS:
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
+
+		if (payload & 0x1)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
+		if (payload & 0x2)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
+		if (payload & 0x4)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
+		if (payload & 0x8)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
+		if (payload & 0x10)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
+		if (payload & 0x20)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
+		if (payload & 0x40)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
+		if (payload & 0x80)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
 		if (idx > 1) {
-			if (payload & 0x100) {
-				ret = snprintf(buf, buf_len, " LLC-ACCESS");
-				buf += ret;
-				blen -= ret;
-			}
-			if (payload & 0x200) {
-				ret = snprintf(buf, buf_len, " LLC-REFILL");
-				buf += ret;
-				blen -= ret;
-			}
-			if (payload & 0x400) {
-				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
-				buf += ret;
-				blen -= ret;
-			}
+			if (payload & 0x100)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
+			if (payload & 0x200)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
+			if (payload & 0x400)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
 		}
-		if (ret < 0)
-			return ret;
-		blen -= ret;
-		return buf_len - blen;
-	}
+		return err ?: (int)(buf_len - blen);
+
 	case ARM_SPE_OP_TYPE:
 		switch (idx) {
-		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
-					"COND-SELECT" : "INSN-OTHER");
-		case 1:	{
-			size_t blen = buf_len;
+		case 0:
+			return arm_spe_pkt_snprintf(&err, &buf, &blen,
+					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
+		case 1:
+			arm_spe_pkt_snprintf(&err, &buf, &blen,
+					     payload & 0x1 ? "ST" : "LD");
 
-			if (payload & 0x1)
-				ret = snprintf(buf, buf_len, "ST");
-			else
-				ret = snprintf(buf, buf_len, "LD");
-			buf += ret;
-			blen -= ret;
 			if (payload & 0x2) {
-				if (payload & 0x4) {
-					ret = snprintf(buf, buf_len, " AT");
-					buf += ret;
-					blen -= ret;
-				}
-				if (payload & 0x8) {
-					ret = snprintf(buf, buf_len, " EXCL");
-					buf += ret;
-					blen -= ret;
-				}
-				if (payload & 0x10) {
-					ret = snprintf(buf, buf_len, " AR");
-					buf += ret;
-					blen -= ret;
-				}
+				if (payload & 0x4)
+					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
+				if (payload & 0x8)
+					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
+				if (payload & 0x10)
+					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
 			} else if (payload & 0x4) {
-				ret = snprintf(buf, buf_len, " SIMD-FP");
-				buf += ret;
-				blen -= ret;
-			}
-			if (ret < 0)
-				return ret;
-			blen -= ret;
-			return buf_len - blen;
-		}
-		case 2:	{
-			size_t blen = buf_len;
-
-			ret = snprintf(buf, buf_len, "B");
-			buf += ret;
-			blen -= ret;
-			if (payload & 0x1) {
-				ret = snprintf(buf, buf_len, " COND");
-				buf += ret;
-				blen -= ret;
-			}
-			if (payload & 0x2) {
-				ret = snprintf(buf, buf_len, " IND");
-				buf += ret;
-				blen -= ret;
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
 			}
-			if (ret < 0)
-				return ret;
-			blen -= ret;
-			return buf_len - blen;
-			}
-		default: return 0;
+
+			return err ?: (int)(buf_len - blen);
+
+		case 2:
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
+
+			if (payload & 0x1)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
+			if (payload & 0x2)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
+
+			return err ?: (int)(buf_len - blen);
+
+		default:
+			return 0;
 		}
 	case ARM_SPE_DATA_SOURCE:
 	case ARM_SPE_TIMESTAMP:
-		return snprintf(buf, buf_len, "%s %lld", name, payload);
+		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
 	case ARM_SPE_ADDRESS:
 		switch (idx) {
 		case 0:
 		case 1: ns = !!(packet->payload & NS_FLAG);
 			el = (packet->payload & EL_FLAG) >> 61;
 			payload &= ~(0xffULL << 56);
-			return snprintf(buf, buf_len, "%s 0x%llx el%d ns=%d",
+			return arm_spe_pkt_snprintf(&err, &buf, &blen,
+					"%s 0x%llx el%d ns=%d",
 				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
-		case 2:	return snprintf(buf, buf_len, "VA 0x%llx", payload);
+		case 2:
+			return arm_spe_pkt_snprintf(&err, &buf, &blen,
+						    "VA 0x%llx", payload);
 		case 3:	ns = !!(packet->payload & NS_FLAG);
 			payload &= ~(0xffULL << 56);
-			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
-					payload, ns);
-		default: return 0;
+			return arm_spe_pkt_snprintf(&err, &buf, &blen,
+						    "PA 0x%llx ns=%d", payload, ns);
+		default:
+			return 0;
 		}
 	case ARM_SPE_CONTEXT:
-		return snprintf(buf, buf_len, "%s 0x%lx el%d", name,
-				(unsigned long)payload, idx + 1);
-	case ARM_SPE_COUNTER: {
-		size_t blen = buf_len;
-
-		ret = snprintf(buf, buf_len, "%s %d ", name,
-			       (unsigned short)payload);
-		buf += ret;
-		blen -= ret;
+		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
+					    name, (unsigned long)payload, idx + 1);
+	case ARM_SPE_COUNTER:
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
+				     (unsigned short)payload);
+
 		switch (idx) {
-		case 0:	ret = snprintf(buf, buf_len, "TOT"); break;
-		case 1:	ret = snprintf(buf, buf_len, "ISSUE"); break;
-		case 2:	ret = snprintf(buf, buf_len, "XLAT"); break;
-		default: ret = 0;
+		case 0:
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
+			break;
+		case 1:
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
+			break;
+		case 2:
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
+			break;
+		default:
+			break;
 		}
-		if (ret < 0)
-			return ret;
-		blen -= ret;
-		return buf_len - blen;
-	}
+
+		return err ?: (int)(buf_len - blen);
+
 	default:
 		break;
 	}
 
-	return snprintf(buf, buf_len, "%s 0x%llx (%d)",
-			name, payload, packet->index);
+	return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%llx (%d)",
+				    name, payload, packet->index);
 }
-- 
2.17.1

Re: [PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[André Przywara]

On 30/10/2020 02:57, Leo Yan wrote:
> When outputs strings to the decoding buffer with function snprintf(),
> SPE decoder needs to detects if any error returns from snprintf() and if
> so needs to directly bail out.  If snprintf() returns success, it needs
> to update buffer pointer and reduce the buffer length so can continue to
> output the next string into the consequent memory space.
> 
> This complex logics are spreading in the function arm_spe_pkt_desc() so
> there has many duplicate codes for handling error detecting, increment
> buffer pointer and decrement buffer size.
> 
> To avoid the duplicate code, this patch introduces a new helper function
> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> it's used by the caller arm_spe_pkt_desc(); if printing buffer is called
> for multiple times in a flow, the error is a cumulative value and simply
> returns its final value.
> 
> This patch also moves the variable 'blen' as the function's local
> variable, this allows to remove the unnecessary braces and improve the
> readability.
> 
> Suggested-by: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Wow, that's a bit tricky to review in its current incarnation, but it
looks alright to me.
I have some optimisation idea below, but that's a nit and the patch as
is seems correct to me (checked twice, the second time in an editor,
matching the lines), so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

...

> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 267 ++++++++----------
>  1 file changed, 117 insertions(+), 150 deletions(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index 04fd7fd7c15f..1ecaf9805b79 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -9,6 +9,7 @@
>  #include <endian.h>
>  #include <byteswap.h>
>  #include <linux/bitops.h>
> +#include <stdarg.h>
>  
>  #include "arm-spe-pkt-decoder.h"
>  
> @@ -258,192 +259,158 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
>  	return ret;
>  }
>  
> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> +				const char *fmt, ...)
> +{
> +	va_list ap;
> +	int ret;
> +
> +	/* Bail out if any error occurred */
> +	if (err && *err)
> +		return *err;
> +
> +	va_start(ap, fmt);
> +	ret = vsnprintf(*buf_p, *blen, fmt, ap);
> +	va_end(ap);
> +
> +	if (ret < 0) {
> +		if (err && !*err)
> +			*err = ret;
> +	} else {
> +		*buf_p += ret;
> +		*blen -= ret;
> +	}
> +
> +	return ret;
> +}
> +
>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  		     size_t buf_len)
>  {
> -	int ret, ns, el, idx = packet->index;
> +	int ns, el, idx = packet->index;
>  	unsigned long long payload = packet->payload;
>  	const char *name = arm_spe_pkt_name(packet->type);
> +	size_t blen = buf_len;
> +	int err = 0;
>  
>  	switch (packet->type) {
>  	case ARM_SPE_BAD:
>  	case ARM_SPE_PAD:
>  	case ARM_SPE_END:
> -		return snprintf(buf, buf_len, "%s", name);
> -	case ARM_SPE_EVENTS: {
> -		size_t blen = buf_len;
> -
> -		ret = 0;
> -		ret = snprintf(buf, buf_len, "EV");
> -		buf += ret;
> -		blen -= ret;
> -		if (payload & 0x1) {
> -			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x2) {
> -			ret = snprintf(buf, buf_len, " RETIRED");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x4) {
> -			ret = snprintf(buf, buf_len, " L1D-ACCESS");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x8) {
> -			ret = snprintf(buf, buf_len, " L1D-REFILL");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x10) {
> -			ret = snprintf(buf, buf_len, " TLB-ACCESS");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x20) {
> -			ret = snprintf(buf, buf_len, " TLB-REFILL");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x40) {
> -			ret = snprintf(buf, buf_len, " NOT-TAKEN");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x80) {
> -			ret = snprintf(buf, buf_len, " MISPRED");
> -			buf += ret;
> -			blen -= ret;
> -		}
> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);

Nothing critical, but in case you need to respin this series for
whatever reason, you might want to use NULL for the first argument,
since you return here and there is little point in setting err. Same for
other cases where you return directly from an arm_spe_pkt_snprintf() call.
But it doesn't hurt, so you could leave it as well, and it's more robust
in case someone extends the code later.

> +	case ARM_SPE_EVENTS:
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
> +
> +		if (payload & 0x1)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
> +		if (payload & 0x2)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
> +		if (payload & 0x4)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
> +		if (payload & 0x8)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
> +		if (payload & 0x10)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
> +		if (payload & 0x20)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
> +		if (payload & 0x40)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
> +		if (payload & 0x80)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
>  		if (idx > 1) {
> -			if (payload & 0x100) {
> -				ret = snprintf(buf, buf_len, " LLC-ACCESS");
> -				buf += ret;
> -				blen -= ret;
> -			}
> -			if (payload & 0x200) {
> -				ret = snprintf(buf, buf_len, " LLC-REFILL");
> -				buf += ret;
> -				blen -= ret;
> -			}
> -			if (payload & 0x400) {
> -				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
> -				buf += ret;
> -				blen -= ret;
> -			}
> +			if (payload & 0x100)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
> +			if (payload & 0x200)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
> +			if (payload & 0x400)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
>  		}
> -		if (ret < 0)
> -			return ret;
> -		blen -= ret;
> -		return buf_len - blen;
> -	}
> +		return err ?: (int)(buf_len - blen);
> +
>  	case ARM_SPE_OP_TYPE:
>  		switch (idx) {
> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> -					"COND-SELECT" : "INSN-OTHER");
> -		case 1:	{
> -			size_t blen = buf_len;
> +		case 0:
> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");

same here (and below, you get the idea ...)

Cheers,
Andre


> +		case 1:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					     payload & 0x1 ? "ST" : "LD");
>  
> -			if (payload & 0x1)
> -				ret = snprintf(buf, buf_len, "ST");
> -			else
> -				ret = snprintf(buf, buf_len, "LD");
> -			buf += ret;
> -			blen -= ret;
>  			if (payload & 0x2) {
> -				if (payload & 0x4) {
> -					ret = snprintf(buf, buf_len, " AT");
> -					buf += ret;
> -					blen -= ret;
> -				}
> -				if (payload & 0x8) {
> -					ret = snprintf(buf, buf_len, " EXCL");
> -					buf += ret;
> -					blen -= ret;
> -				}
> -				if (payload & 0x10) {
> -					ret = snprintf(buf, buf_len, " AR");
> -					buf += ret;
> -					blen -= ret;
> -				}
> +				if (payload & 0x4)
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
> +				if (payload & 0x8)
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
> +				if (payload & 0x10)
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
>  			} else if (payload & 0x4) {
> -				ret = snprintf(buf, buf_len, " SIMD-FP");
> -				buf += ret;
> -				blen -= ret;
> -			}
> -			if (ret < 0)
> -				return ret;
> -			blen -= ret;
> -			return buf_len - blen;
> -		}
> -		case 2:	{
> -			size_t blen = buf_len;
> -
> -			ret = snprintf(buf, buf_len, "B");
> -			buf += ret;
> -			blen -= ret;
> -			if (payload & 0x1) {
> -				ret = snprintf(buf, buf_len, " COND");
> -				buf += ret;
> -				blen -= ret;
> -			}
> -			if (payload & 0x2) {
> -				ret = snprintf(buf, buf_len, " IND");
> -				buf += ret;
> -				blen -= ret;
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
>  			}
> -			if (ret < 0)
> -				return ret;
> -			blen -= ret;
> -			return buf_len - blen;
> -			}
> -		default: return 0;
> +
> +			return err ?: (int)(buf_len - blen);
> +
> +		case 2:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
> +
> +			if (payload & 0x1)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
> +			if (payload & 0x2)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
> +
> +			return err ?: (int)(buf_len - blen);
> +
> +		default:
> +			return 0;
>  		}
>  	case ARM_SPE_DATA_SOURCE:
>  	case ARM_SPE_TIMESTAMP:
> -		return snprintf(buf, buf_len, "%s %lld", name, payload);
> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
>  	case ARM_SPE_ADDRESS:
>  		switch (idx) {
>  		case 0:
>  		case 1: ns = !!(packet->payload & NS_FLAG);
>  			el = (packet->payload & EL_FLAG) >> 61;
>  			payload &= ~(0xffULL << 56);
> -			return snprintf(buf, buf_len, "%s 0x%llx el%d ns=%d",
> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					"%s 0x%llx el%d ns=%d",
>  				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
> -		case 2:	return snprintf(buf, buf_len, "VA 0x%llx", payload);
> +		case 2:
> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +						    "VA 0x%llx", payload);
>  		case 3:	ns = !!(packet->payload & NS_FLAG);
>  			payload &= ~(0xffULL << 56);
> -			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
> -					payload, ns);
> -		default: return 0;
> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +						    "PA 0x%llx ns=%d", payload, ns);
> +		default:
> +			return 0;
>  		}
>  	case ARM_SPE_CONTEXT:
> -		return snprintf(buf, buf_len, "%s 0x%lx el%d", name,
> -				(unsigned long)payload, idx + 1);
> -	case ARM_SPE_COUNTER: {
> -		size_t blen = buf_len;
> -
> -		ret = snprintf(buf, buf_len, "%s %d ", name,
> -			       (unsigned short)payload);
> -		buf += ret;
> -		blen -= ret;
> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
> +					    name, (unsigned long)payload, idx + 1);
> +	case ARM_SPE_COUNTER:
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
> +				     (unsigned short)payload);
> +
>  		switch (idx) {
> -		case 0:	ret = snprintf(buf, buf_len, "TOT"); break;
> -		case 1:	ret = snprintf(buf, buf_len, "ISSUE"); break;
> -		case 2:	ret = snprintf(buf, buf_len, "XLAT"); break;
> -		default: ret = 0;
> +		case 0:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
> +			break;
> +		case 1:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
> +			break;
> +		case 2:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
> +			break;
> +		default:
> +			break;
>  		}
> -		if (ret < 0)
> -			return ret;
> -		blen -= ret;
> -		return buf_len - blen;
> -	}
> +
> +		return err ?: (int)(buf_len - blen);
> +
>  	default:
>  		break;
>  	}
>  
> -	return snprintf(buf, buf_len, "%s 0x%llx (%d)",
> -			name, payload, packet->index);
> +	return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%llx (%d)",
> +				    name, payload, packet->index);
>  }
> 

Re: [PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[Leo Yan]

On Mon, Nov 02, 2020 at 03:50:01PM +0000, André Przywara wrote:
> On 30/10/2020 02:57, Leo Yan wrote:
> > When outputs strings to the decoding buffer with function snprintf(),
> > SPE decoder needs to detects if any error returns from snprintf() and if
> > so needs to directly bail out.  If snprintf() returns success, it needs
> > to update buffer pointer and reduce the buffer length so can continue to
> > output the next string into the consequent memory space.
> > 
> > This complex logics are spreading in the function arm_spe_pkt_desc() so
> > there has many duplicate codes for handling error detecting, increment
> > buffer pointer and decrement buffer size.
> > 
> > To avoid the duplicate code, this patch introduces a new helper function
> > arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> > it's used by the caller arm_spe_pkt_desc(); if printing buffer is called
> > for multiple times in a flow, the error is a cumulative value and simply
> > returns its final value.
> > 
> > This patch also moves the variable 'blen' as the function's local
> > variable, this allows to remove the unnecessary braces and improve the
> > readability.
> > 
> > Suggested-by: Dave Martin <Dave.Martin@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> 
> Wow, that's a bit tricky to review in its current incarnation, but it
> looks alright to me.

Yes, I had the same feeling when I saw the generated patch :)

> I have some optimisation idea below, but that's a nit and the patch as
> is seems correct to me (checked twice, the second time in an editor,
> matching the lines), so:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> 
> ...
> 
> > ---
> >  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 267 ++++++++----------
> >  1 file changed, 117 insertions(+), 150 deletions(-)
> > 
> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > index 04fd7fd7c15f..1ecaf9805b79 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > @@ -9,6 +9,7 @@
> >  #include <endian.h>
> >  #include <byteswap.h>
> >  #include <linux/bitops.h>
> > +#include <stdarg.h>
> >  
> >  #include "arm-spe-pkt-decoder.h"
> >  
> > @@ -258,192 +259,158 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
> >  	return ret;
> >  }
> >  
> > +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> > +				const char *fmt, ...)
> > +{
> > +	va_list ap;
> > +	int ret;
> > +
> > +	/* Bail out if any error occurred */
> > +	if (err && *err)
> > +		return *err;
> > +
> > +	va_start(ap, fmt);
> > +	ret = vsnprintf(*buf_p, *blen, fmt, ap);
> > +	va_end(ap);
> > +
> > +	if (ret < 0) {
> > +		if (err && !*err)
> > +			*err = ret;
> > +	} else {
> > +		*buf_p += ret;
> > +		*blen -= ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >  		     size_t buf_len)
> >  {
> > -	int ret, ns, el, idx = packet->index;
> > +	int ns, el, idx = packet->index;
> >  	unsigned long long payload = packet->payload;
> >  	const char *name = arm_spe_pkt_name(packet->type);
> > +	size_t blen = buf_len;
> > +	int err = 0;
> >  
> >  	switch (packet->type) {
> >  	case ARM_SPE_BAD:
> >  	case ARM_SPE_PAD:
> >  	case ARM_SPE_END:
> > -		return snprintf(buf, buf_len, "%s", name);
> > -	case ARM_SPE_EVENTS: {
> > -		size_t blen = buf_len;
> > -
> > -		ret = 0;
> > -		ret = snprintf(buf, buf_len, "EV");
> > -		buf += ret;
> > -		blen -= ret;
> > -		if (payload & 0x1) {
> > -			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x2) {
> > -			ret = snprintf(buf, buf_len, " RETIRED");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x4) {
> > -			ret = snprintf(buf, buf_len, " L1D-ACCESS");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x8) {
> > -			ret = snprintf(buf, buf_len, " L1D-REFILL");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x10) {
> > -			ret = snprintf(buf, buf_len, " TLB-ACCESS");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x20) {
> > -			ret = snprintf(buf, buf_len, " TLB-REFILL");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x40) {
> > -			ret = snprintf(buf, buf_len, " NOT-TAKEN");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x80) {
> > -			ret = snprintf(buf, buf_len, " MISPRED");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
> 
> Nothing critical, but in case you need to respin this series for
> whatever reason, you might want to use NULL for the first argument,
> since you return here and there is little point in setting err. Same for
> other cases where you return directly from an arm_spe_pkt_snprintf() call.
> But it doesn't hurt, so you could leave it as well, and it's more robust
> in case someone extends the code later.

Makes sense for me.  Eventually, we also can remove the local variable
"err", so will do it.  Thanks a lot!

> > +	case ARM_SPE_EVENTS:
> > +		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
> > +
> > +		if (payload & 0x1)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
> > +		if (payload & 0x2)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
> > +		if (payload & 0x4)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
> > +		if (payload & 0x8)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
> > +		if (payload & 0x10)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
> > +		if (payload & 0x20)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
> > +		if (payload & 0x40)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
> > +		if (payload & 0x80)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
> >  		if (idx > 1) {
> > -			if (payload & 0x100) {
> > -				ret = snprintf(buf, buf_len, " LLC-ACCESS");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > -			if (payload & 0x200) {
> > -				ret = snprintf(buf, buf_len, " LLC-REFILL");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > -			if (payload & 0x400) {
> > -				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > +			if (payload & 0x100)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
> > +			if (payload & 0x200)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
> > +			if (payload & 0x400)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
> >  		}
> > -		if (ret < 0)
> > -			return ret;
> > -		blen -= ret;
> > -		return buf_len - blen;
> > -	}
> > +		return err ?: (int)(buf_len - blen);
> > +
> >  	case ARM_SPE_OP_TYPE:
> >  		switch (idx) {
> > -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > -					"COND-SELECT" : "INSN-OTHER");
> > -		case 1:	{
> > -			size_t blen = buf_len;
> > +		case 0:
> > +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
> 
> same here (and below, you get the idea ...)
> 
> Cheers,
> Andre
> 
> 
> > +		case 1:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +					     payload & 0x1 ? "ST" : "LD");
> >  
> > -			if (payload & 0x1)
> > -				ret = snprintf(buf, buf_len, "ST");
> > -			else
> > -				ret = snprintf(buf, buf_len, "LD");
> > -			buf += ret;
> > -			blen -= ret;
> >  			if (payload & 0x2) {
> > -				if (payload & 0x4) {
> > -					ret = snprintf(buf, buf_len, " AT");
> > -					buf += ret;
> > -					blen -= ret;
> > -				}
> > -				if (payload & 0x8) {
> > -					ret = snprintf(buf, buf_len, " EXCL");
> > -					buf += ret;
> > -					blen -= ret;
> > -				}
> > -				if (payload & 0x10) {
> > -					ret = snprintf(buf, buf_len, " AR");
> > -					buf += ret;
> > -					blen -= ret;
> > -				}
> > +				if (payload & 0x4)
> > +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
> > +				if (payload & 0x8)
> > +					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
> > +				if (payload & 0x10)
> > +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
> >  			} else if (payload & 0x4) {
> > -				ret = snprintf(buf, buf_len, " SIMD-FP");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > -			if (ret < 0)
> > -				return ret;
> > -			blen -= ret;
> > -			return buf_len - blen;
> > -		}
> > -		case 2:	{
> > -			size_t blen = buf_len;
> > -
> > -			ret = snprintf(buf, buf_len, "B");
> > -			buf += ret;
> > -			blen -= ret;
> > -			if (payload & 0x1) {
> > -				ret = snprintf(buf, buf_len, " COND");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > -			if (payload & 0x2) {
> > -				ret = snprintf(buf, buf_len, " IND");
> > -				buf += ret;
> > -				blen -= ret;
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
> >  			}
> > -			if (ret < 0)
> > -				return ret;
> > -			blen -= ret;
> > -			return buf_len - blen;
> > -			}
> > -		default: return 0;
> > +
> > +			return err ?: (int)(buf_len - blen);
> > +
> > +		case 2:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
> > +
> > +			if (payload & 0x1)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
> > +			if (payload & 0x2)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
> > +
> > +			return err ?: (int)(buf_len - blen);
> > +
> > +		default:
> > +			return 0;
> >  		}
> >  	case ARM_SPE_DATA_SOURCE:
> >  	case ARM_SPE_TIMESTAMP:
> > -		return snprintf(buf, buf_len, "%s %lld", name, payload);
> > +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
> >  	case ARM_SPE_ADDRESS:
> >  		switch (idx) {
> >  		case 0:
> >  		case 1: ns = !!(packet->payload & NS_FLAG);
> >  			el = (packet->payload & EL_FLAG) >> 61;
> >  			payload &= ~(0xffULL << 56);
> > -			return snprintf(buf, buf_len, "%s 0x%llx el%d ns=%d",
> > +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +					"%s 0x%llx el%d ns=%d",
> >  				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
> > -		case 2:	return snprintf(buf, buf_len, "VA 0x%llx", payload);
> > +		case 2:
> > +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +						    "VA 0x%llx", payload);
> >  		case 3:	ns = !!(packet->payload & NS_FLAG);
> >  			payload &= ~(0xffULL << 56);
> > -			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
> > -					payload, ns);
> > -		default: return 0;
> > +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +						    "PA 0x%llx ns=%d", payload, ns);
> > +		default:
> > +			return 0;
> >  		}
> >  	case ARM_SPE_CONTEXT:
> > -		return snprintf(buf, buf_len, "%s 0x%lx el%d", name,
> > -				(unsigned long)payload, idx + 1);
> > -	case ARM_SPE_COUNTER: {
> > -		size_t blen = buf_len;
> > -
> > -		ret = snprintf(buf, buf_len, "%s %d ", name,
> > -			       (unsigned short)payload);
> > -		buf += ret;
> > -		blen -= ret;
> > +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
> > +					    name, (unsigned long)payload, idx + 1);
> > +	case ARM_SPE_COUNTER:
> > +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
> > +				     (unsigned short)payload);
> > +
> >  		switch (idx) {
> > -		case 0:	ret = snprintf(buf, buf_len, "TOT"); break;
> > -		case 1:	ret = snprintf(buf, buf_len, "ISSUE"); break;
> > -		case 2:	ret = snprintf(buf, buf_len, "XLAT"); break;
> > -		default: ret = 0;
> > +		case 0:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
> > +			break;
> > +		case 1:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
> > +			break;
> > +		case 2:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
> > +			break;
> > +		default:
> > +			break;
> >  		}
> > -		if (ret < 0)
> > -			return ret;
> > -		blen -= ret;
> > -		return buf_len - blen;
> > -	}
> > +
> > +		return err ?: (int)(buf_len - blen);
> > +
> >  	default:
> >  		break;
> >  	}
> >  
> > -	return snprintf(buf, buf_len, "%s 0x%llx (%d)",
> > -			name, payload, packet->index);
> > +	return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%llx (%d)",
> > +				    name, payload, packet->index);
> >  }
> > 
> 

Re: [PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[Leo Yan]

Hi Andre, Dave,

On Mon, Nov 02, 2020 at 03:50:01PM +0000, André Przywara wrote:

[...]

> >  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >  		     size_t buf_len)
> >  {
> > -	int ret, ns, el, idx = packet->index;
> > +	int ns, el, idx = packet->index;
> >  	unsigned long long payload = packet->payload;
> >  	const char *name = arm_spe_pkt_name(packet->type);
> > +	size_t blen = buf_len;
> > +	int err = 0;
> >  
> >  	switch (packet->type) {
> >  	case ARM_SPE_BAD:
> >  	case ARM_SPE_PAD:
> >  	case ARM_SPE_END:
> > -		return snprintf(buf, buf_len, "%s", name);
> > -	case ARM_SPE_EVENTS: {
> > -		size_t blen = buf_len;
> > -
> > -		ret = 0;
> > -		ret = snprintf(buf, buf_len, "EV");
> > -		buf += ret;
> > -		blen -= ret;
> > -		if (payload & 0x1) {
> > -			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x2) {
> > -			ret = snprintf(buf, buf_len, " RETIRED");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x4) {
> > -			ret = snprintf(buf, buf_len, " L1D-ACCESS");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x8) {
> > -			ret = snprintf(buf, buf_len, " L1D-REFILL");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x10) {
> > -			ret = snprintf(buf, buf_len, " TLB-ACCESS");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x20) {
> > -			ret = snprintf(buf, buf_len, " TLB-REFILL");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x40) {
> > -			ret = snprintf(buf, buf_len, " NOT-TAKEN");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x80) {
> > -			ret = snprintf(buf, buf_len, " MISPRED");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
> 
> Nothing critical, but in case you need to respin this series for
> whatever reason, you might want to use NULL for the first argument,
> since you return here and there is little point in setting err. Same for
> other cases where you return directly from an arm_spe_pkt_snprintf() call.
> But it doesn't hurt, so you could leave it as well, and it's more robust
> in case someone extends the code later.

Just remind, in the new patch set v7, I don't change to use NULL for
the first argument and still pass '&err', the main reason is to
consolidate with return value, which is heavily dependent on the 'err'
parameter of arm_spe_pkt_snprintf(); the brief form is as follow:

    switch (type) {
    ...
    case ARM_SPE_END:
        arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
        break;
    ...
    default:
        break;
    }

    handle the 'err';

    return err;

Please see the new introduced patch [1] so it can give more idea for
the changes.

Thanks,
Leo

[1] https://lore.kernel.org/patchwork/patch/1333881/

Re: [PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[Dave Martin]

On Fri, Oct 30, 2020 at 02:57:09AM +0000, Leo Yan wrote:
> When outputs strings to the decoding buffer with function snprintf(),
> SPE decoder needs to detects if any error returns from snprintf() and if
> so needs to directly bail out.  If snprintf() returns success, it needs
> to update buffer pointer and reduce the buffer length so can continue to
> output the next string into the consequent memory space.
> 
> This complex logics are spreading in the function arm_spe_pkt_desc() so
> there has many duplicate codes for handling error detecting, increment
> buffer pointer and decrement buffer size.
> 
> To avoid the duplicate code, this patch introduces a new helper function
> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> it's used by the caller arm_spe_pkt_desc(); if printing buffer is called
> for multiple times in a flow, the error is a cumulative value and simply
> returns its final value.
> 
> This patch also moves the variable 'blen' as the function's local
> variable, this allows to remove the unnecessary braces and improve the
> readability.
> 
> Suggested-by: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

This looks like a good refacroting now, but as pointed out by Andre this
patch is now rather hard to review, since it combines the refactoring
with other changes.

If reposting this series, it would be good if this could be split into a
first patch that introduces arm_spe_pkt_snprintf() and just updates each
snprintf() call site to use it, but without moving other code around or
optimising anything, followed by one or more patches that clean up and
simplify arm_spe_pkt_desc().

If the series is otherwise mature though, then this rework may be
overkill.

> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 267 ++++++++----------
>  1 file changed, 117 insertions(+), 150 deletions(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index 04fd7fd7c15f..1ecaf9805b79 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -9,6 +9,7 @@
>  #include <endian.h>
>  #include <byteswap.h>
>  #include <linux/bitops.h>
> +#include <stdarg.h>
>  
>  #include "arm-spe-pkt-decoder.h"
>  
> @@ -258,192 +259,158 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
>  	return ret;
>  }
>  
> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> +				const char *fmt, ...)
> +{
> +	va_list ap;
> +	int ret;
> +
> +	/* Bail out if any error occurred */
> +	if (err && *err)
> +		return *err;
> +
> +	va_start(ap, fmt);
> +	ret = vsnprintf(*buf_p, *blen, fmt, ap);
> +	va_end(ap);
> +
> +	if (ret < 0) {
> +		if (err && !*err)
> +			*err = ret;

What happens on buffer overrun (i.e., ret >= *blen)?

It looks to me like we'll advance buf_p too far, blen will wrap around,
and the string at *buf_p won't be null terminated.  Because the return
value is still >= 0, this condition will be returned up the stack as
"success".

Perhaps this can never happen given the actual buffer sizes and strings
being printed, but it feels a bit unsafe.


It may be better to clamp the adjustments to *buf_p and *blen to
*blen - 1 in this case, and explicitly set (*buf_p)[*blen - 1] to '\0'.
We _may_ want indicate failure in the return from arm_spe_pkt_desc() in
this situation, but I don't know enough about how this code is called to
enable me to judge that.

(Note, this issue is not introduced by this patch, but this refactoring
makes it easier to address it in a single place -- so it may now be
worth doing so.)

> +	} else {
> +		*buf_p += ret;
> +		*blen -= ret;
> +	}
> +
> +	return ret;
> +}
> +
>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  		     size_t buf_len)
>  {
> -	int ret, ns, el, idx = packet->index;
> +	int ns, el, idx = packet->index;
>  	unsigned long long payload = packet->payload;
>  	const char *name = arm_spe_pkt_name(packet->type);
> +	size_t blen = buf_len;
> +	int err = 0;
>  
>  	switch (packet->type) {
>  	case ARM_SPE_BAD:
>  	case ARM_SPE_PAD:
>  	case ARM_SPE_END:
> -		return snprintf(buf, buf_len, "%s", name);
> -	case ARM_SPE_EVENTS: {
> -		size_t blen = buf_len;
> -
> -		ret = 0;
> -		ret = snprintf(buf, buf_len, "EV");
> -		buf += ret;
> -		blen -= ret;
> -		if (payload & 0x1) {
> -			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x2) {
> -			ret = snprintf(buf, buf_len, " RETIRED");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x4) {
> -			ret = snprintf(buf, buf_len, " L1D-ACCESS");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x8) {
> -			ret = snprintf(buf, buf_len, " L1D-REFILL");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x10) {
> -			ret = snprintf(buf, buf_len, " TLB-ACCESS");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x20) {
> -			ret = snprintf(buf, buf_len, " TLB-REFILL");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x40) {
> -			ret = snprintf(buf, buf_len, " NOT-TAKEN");
> -			buf += ret;
> -			blen -= ret;
> -		}
> -		if (payload & 0x80) {
> -			ret = snprintf(buf, buf_len, " MISPRED");
> -			buf += ret;
> -			blen -= ret;
> -		}
> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
> +	case ARM_SPE_EVENTS:
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
> +
> +		if (payload & 0x1)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
> +		if (payload & 0x2)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
> +		if (payload & 0x4)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
> +		if (payload & 0x8)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
> +		if (payload & 0x10)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
> +		if (payload & 0x20)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
> +		if (payload & 0x40)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
> +		if (payload & 0x80)
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
>  		if (idx > 1) {
> -			if (payload & 0x100) {
> -				ret = snprintf(buf, buf_len, " LLC-ACCESS");
> -				buf += ret;
> -				blen -= ret;
> -			}
> -			if (payload & 0x200) {
> -				ret = snprintf(buf, buf_len, " LLC-REFILL");
> -				buf += ret;
> -				blen -= ret;
> -			}
> -			if (payload & 0x400) {
> -				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
> -				buf += ret;
> -				blen -= ret;
> -			}
> +			if (payload & 0x100)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
> +			if (payload & 0x200)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
> +			if (payload & 0x400)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
>  		}
> -		if (ret < 0)
> -			return ret;
> -		blen -= ret;
> -		return buf_len - blen;

It looks like we now fall off the bottom of the switch() here.  It's
preferable to add a break, both to document the intended code flow, and
to avoid accidents if another case is added later.

> -	}
> +		return err ?: (int)(buf_len - blen);

Nit: unexplained type cast.  Does the result definitely fit into an int?
If not, why doesn't it matter?


Also:

If the actual return value is important for the caller to determine the
number of bytes appended, then it would be better to compute it in one
place.  Otherwise, it would be better to squash all success returns to 0,
rather than spend effort computing a value that may be misleading or
wrong anyway.

In its current form, it's tricky to see whether this patch derives the
return value consistently for all cases.  In particular, if some code
patch does one or more arm_spe_pkt_snprintf(), followed by a
return arm_spe_pkt_snprintf(); then the return value (which only takes
the last arm_spe_pkt_desc() into account) would be wrong.  It would be
preferable if the return value for the success case were always
computed from buf_len and blen, to avoid this risk.  (I'm not saying
this bug exists, just that it's hard to see from the patch that it
doesn't exist.)

> +
>  	case ARM_SPE_OP_TYPE:
>  		switch (idx) {
> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> -					"COND-SELECT" : "INSN-OTHER");
> -		case 1:	{
> -			size_t blen = buf_len;
> +		case 0:
> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
> +		case 1:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					     payload & 0x1 ? "ST" : "LD");
>  
> -			if (payload & 0x1)
> -				ret = snprintf(buf, buf_len, "ST");
> -			else
> -				ret = snprintf(buf, buf_len, "LD");
> -			buf += ret;
> -			blen -= ret;
>  			if (payload & 0x2) {
> -				if (payload & 0x4) {
> -					ret = snprintf(buf, buf_len, " AT");
> -					buf += ret;
> -					blen -= ret;
> -				}
> -				if (payload & 0x8) {
> -					ret = snprintf(buf, buf_len, " EXCL");
> -					buf += ret;
> -					blen -= ret;
> -				}
> -				if (payload & 0x10) {
> -					ret = snprintf(buf, buf_len, " AR");
> -					buf += ret;
> -					blen -= ret;
> -				}
> +				if (payload & 0x4)
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
> +				if (payload & 0x8)
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
> +				if (payload & 0x10)
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
>  			} else if (payload & 0x4) {
> -				ret = snprintf(buf, buf_len, " SIMD-FP");
> -				buf += ret;
> -				blen -= ret;
> -			}
> -			if (ret < 0)
> -				return ret;
> -			blen -= ret;
> -			return buf_len - blen;
> -		}
> -		case 2:	{
> -			size_t blen = buf_len;
> -
> -			ret = snprintf(buf, buf_len, "B");
> -			buf += ret;
> -			blen -= ret;
> -			if (payload & 0x1) {
> -				ret = snprintf(buf, buf_len, " COND");
> -				buf += ret;
> -				blen -= ret;
> -			}
> -			if (payload & 0x2) {
> -				ret = snprintf(buf, buf_len, " IND");
> -				buf += ret;
> -				blen -= ret;
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
>  			}
> -			if (ret < 0)
> -				return ret;
> -			blen -= ret;
> -			return buf_len - blen;
> -			}
> -		default: return 0;
> +
> +			return err ?: (int)(buf_len - blen);
> +
> +		case 2:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
> +
> +			if (payload & 0x1)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
> +			if (payload & 0x2)
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
> +
> +			return err ?: (int)(buf_len - blen);
> +
> +		default:
> +			return 0;
>  		}
>  	case ARM_SPE_DATA_SOURCE:
>  	case ARM_SPE_TIMESTAMP:
> -		return snprintf(buf, buf_len, "%s %lld", name, payload);
> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
>  	case ARM_SPE_ADDRESS:
>  		switch (idx) {
>  		case 0:
>  		case 1: ns = !!(packet->payload & NS_FLAG);
>  			el = (packet->payload & EL_FLAG) >> 61;
>  			payload &= ~(0xffULL << 56);
> -			return snprintf(buf, buf_len, "%s 0x%llx el%d ns=%d",
> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					"%s 0x%llx el%d ns=%d",
>  				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
> -		case 2:	return snprintf(buf, buf_len, "VA 0x%llx", payload);
> +		case 2:
> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +						    "VA 0x%llx", payload);
>  		case 3:	ns = !!(packet->payload & NS_FLAG);
>  			payload &= ~(0xffULL << 56);
> -			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
> -					payload, ns);
> -		default: return 0;
> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +						    "PA 0x%llx ns=%d", payload, ns);
> +		default:
> +			return 0;
>  		}
>  	case ARM_SPE_CONTEXT:
> -		return snprintf(buf, buf_len, "%s 0x%lx el%d", name,
> -				(unsigned long)payload, idx + 1);
> -	case ARM_SPE_COUNTER: {
> -		size_t blen = buf_len;
> -
> -		ret = snprintf(buf, buf_len, "%s %d ", name,
> -			       (unsigned short)payload);
> -		buf += ret;
> -		blen -= ret;
> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
> +					    name, (unsigned long)payload, idx + 1);
> +	case ARM_SPE_COUNTER:
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
> +				     (unsigned short)payload);
> +
>  		switch (idx) {
> -		case 0:	ret = snprintf(buf, buf_len, "TOT"); break;
> -		case 1:	ret = snprintf(buf, buf_len, "ISSUE"); break;
> -		case 2:	ret = snprintf(buf, buf_len, "XLAT"); break;
> -		default: ret = 0;
> +		case 0:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
> +			break;
> +		case 1:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
> +			break;
> +		case 2:
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
> +			break;
> +		default:
> +			break;
>  		}
> -		if (ret < 0)
> -			return ret;
> -		blen -= ret;
> -		return buf_len - blen;
> -	}
> +
> +		return err ?: (int)(buf_len - blen);
> +
>  	default:
>  		break;
>  	}
>  
> -	return snprintf(buf, buf_len, "%s 0x%llx (%d)",
> -			name, payload, packet->index);
> +	return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%llx (%d)",
> +				    name, payload, packet->index);

Otherwise, the patch looks generally OK, but I'd like to see an answer
on my points above.

Cheers
---Dave

Re: [PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[Leo Yan]

Hi Dave,

On Mon, Nov 02, 2020 at 05:06:53PM +0000, Dave Martin wrote:
> On Fri, Oct 30, 2020 at 02:57:09AM +0000, Leo Yan wrote:
> > When outputs strings to the decoding buffer with function snprintf(),
> > SPE decoder needs to detects if any error returns from snprintf() and if
> > so needs to directly bail out.  If snprintf() returns success, it needs
> > to update buffer pointer and reduce the buffer length so can continue to
> > output the next string into the consequent memory space.
> > 
> > This complex logics are spreading in the function arm_spe_pkt_desc() so
> > there has many duplicate codes for handling error detecting, increment
> > buffer pointer and decrement buffer size.
> > 
> > To avoid the duplicate code, this patch introduces a new helper function
> > arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> > it's used by the caller arm_spe_pkt_desc(); if printing buffer is called
> > for multiple times in a flow, the error is a cumulative value and simply
> > returns its final value.
> > 
> > This patch also moves the variable 'blen' as the function's local
> > variable, this allows to remove the unnecessary braces and improve the
> > readability.
> > 
> > Suggested-by: Dave Martin <Dave.Martin@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> 
> This looks like a good refacroting now, but as pointed out by Andre this
> patch is now rather hard to review, since it combines the refactoring
> with other changes.
> 
> If reposting this series, it would be good if this could be split into a
> first patch that introduces arm_spe_pkt_snprintf() and just updates each
> snprintf() call site to use it, but without moving other code around or
> optimising anything, followed by one or more patches that clean up and
> simplify arm_spe_pkt_desc().

I will respin the patch set and follow this approach.

> If the series is otherwise mature though, then this rework may be
> overkill.
> 
> > ---
> >  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 267 ++++++++----------
> >  1 file changed, 117 insertions(+), 150 deletions(-)
> > 
> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > index 04fd7fd7c15f..1ecaf9805b79 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > @@ -9,6 +9,7 @@
> >  #include <endian.h>
> >  #include <byteswap.h>
> >  #include <linux/bitops.h>
> > +#include <stdarg.h>
> >  
> >  #include "arm-spe-pkt-decoder.h"
> >  
> > @@ -258,192 +259,158 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
> >  	return ret;
> >  }
> >  
> > +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> > +				const char *fmt, ...)
> > +{
> > +	va_list ap;
> > +	int ret;
> > +
> > +	/* Bail out if any error occurred */
> > +	if (err && *err)
> > +		return *err;
> > +
> > +	va_start(ap, fmt);
> > +	ret = vsnprintf(*buf_p, *blen, fmt, ap);
> > +	va_end(ap);
> > +
> > +	if (ret < 0) {
> > +		if (err && !*err)
> > +			*err = ret;
> 
> What happens on buffer overrun (i.e., ret >= *blen)?
> 
> It looks to me like we'll advance buf_p too far, blen will wrap around,
> and the string at *buf_p won't be null terminated.  Because the return
> value is still >= 0, this condition will be returned up the stack as
> "success".

Thanks for pointint out this.  I never note for the potential issue
caused by returned value (ret >= *blen);  checked again for the
manual, it says:

"The functions snprintf() and vsnprintf() do not write more than size
bytes (including the terminating null byte ('\0')).  If the output was
truncated due to this limit, then the return value is the number of
characters (excluding  the  terminating  null  byte) which would have
been written to the final string if enough space had been available.
Thus, a return value of size or more means that the output was
truncated."

> Perhaps this can never happen given the actual buffer sizes and strings
> being printed, but it feels a bit unsafe.
> 
> 
> It may be better to clamp the adjustments to *buf_p and *blen to
> *blen - 1 in this case, and explicitly set (*buf_p)[*blen - 1] to '\0'.
> We _may_ want indicate failure in the return from arm_spe_pkt_desc() in
> this situation, but I don't know enough about how this code is called to
> enable me to judge that.

The caller arm_spe_dump() will print out the string if the return
value > 0 [1]; so I think it can simply return the string length which
has been written to the buffer (with the clamped value).  The function
can be refined as below:

static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
				const char *fmt, ...)
{
	va_list ap;
	int ret;

	/* Bail out if any error occurred */
	if (err && *err)
		return *err;

	va_start(ap, fmt);
	ret = vsnprintf(*buf_p, *blen - 1, fmt, ap);
	va_end(ap);

	if (ret < 0) {
		if (err && !*err)
			*err = ret;
	} else {

                /*
                 * A return value of (*blen - 1) or more means that the
                 * output was truncated and the buffer is overrun.
                 */
                if (ret >= (*blen - 1)) {
                        (*buf_p)[*blen - 1] = '\0';

                        /*
                         * Set *err to -1 to avoid overflow if tries to
                         * fill this buffer sequentially.
                         */
		        if (err && !*err)
		        	*err = -1;

                        /* Clamp the size of characters printed */
                        ret = min(ret, *blen);
                }

		*buf_p += ret;
		*blen -= ret;
	}

	return ret;
}

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/arm-spe.c#n116

> (Note, this issue is not introduced by this patch, but this refactoring
> makes it easier to address it in a single place -- so it may now be
> worth doing so.)

Agree.

> > +	} else {
> > +		*buf_p += ret;
> > +		*blen -= ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >  		     size_t buf_len)
> >  {
> > -	int ret, ns, el, idx = packet->index;
> > +	int ns, el, idx = packet->index;
> >  	unsigned long long payload = packet->payload;
> >  	const char *name = arm_spe_pkt_name(packet->type);
> > +	size_t blen = buf_len;
> > +	int err = 0;
> >  
> >  	switch (packet->type) {
> >  	case ARM_SPE_BAD:
> >  	case ARM_SPE_PAD:
> >  	case ARM_SPE_END:
> > -		return snprintf(buf, buf_len, "%s", name);
> > -	case ARM_SPE_EVENTS: {
> > -		size_t blen = buf_len;
> > -
> > -		ret = 0;
> > -		ret = snprintf(buf, buf_len, "EV");
> > -		buf += ret;
> > -		blen -= ret;
> > -		if (payload & 0x1) {
> > -			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x2) {
> > -			ret = snprintf(buf, buf_len, " RETIRED");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x4) {
> > -			ret = snprintf(buf, buf_len, " L1D-ACCESS");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x8) {
> > -			ret = snprintf(buf, buf_len, " L1D-REFILL");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x10) {
> > -			ret = snprintf(buf, buf_len, " TLB-ACCESS");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x20) {
> > -			ret = snprintf(buf, buf_len, " TLB-REFILL");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x40) {
> > -			ret = snprintf(buf, buf_len, " NOT-TAKEN");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > -		if (payload & 0x80) {
> > -			ret = snprintf(buf, buf_len, " MISPRED");
> > -			buf += ret;
> > -			blen -= ret;
> > -		}
> > +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
> > +	case ARM_SPE_EVENTS:
> > +		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
> > +
> > +		if (payload & 0x1)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
> > +		if (payload & 0x2)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
> > +		if (payload & 0x4)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
> > +		if (payload & 0x8)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
> > +		if (payload & 0x10)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
> > +		if (payload & 0x20)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
> > +		if (payload & 0x40)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
> > +		if (payload & 0x80)
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
> >  		if (idx > 1) {
> > -			if (payload & 0x100) {
> > -				ret = snprintf(buf, buf_len, " LLC-ACCESS");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > -			if (payload & 0x200) {
> > -				ret = snprintf(buf, buf_len, " LLC-REFILL");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > -			if (payload & 0x400) {
> > -				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > +			if (payload & 0x100)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
> > +			if (payload & 0x200)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
> > +			if (payload & 0x400)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
> >  		}
> > -		if (ret < 0)
> > -			return ret;
> > -		blen -= ret;
> > -		return buf_len - blen;
> 
> It looks like we now fall off the bottom of the switch() here.  It's
> preferable to add a break, both to document the intended code flow, and
> to avoid accidents if another case is added later.

Will do this.

> > -	}
> > +		return err ?: (int)(buf_len - blen);
> 
> Nit: unexplained type cast.  Does the result definitely fit into an int?
> If not, why doesn't it matter?

The case is to dismiss the compiler warning.

GCC doesn't compliant for the code:

  return buf_len - blen;
  
But GCC compliants type mismatch after change to code:

  return err ?: buf_len - blen;

> Also:
> 
> If the actual return value is important for the caller to determine the
> number of bytes appended, then it would be better to compute it in one
> place.  Otherwise, it would be better to squash all success returns to 0,
> rather than spend effort computing a value that may be misleading or
> wrong anyway.
> 
> In its current form, it's tricky to see whether this patch derives the
> return value consistently for all cases.  In particular, if some code
> patch does one or more arm_spe_pkt_snprintf(), followed by a
> return arm_spe_pkt_snprintf(); then the return value (which only takes
> the last arm_spe_pkt_desc() into account) would be wrong.  It would be
> preferable if the return value for the success case were always
> computed from buf_len and blen, to avoid this risk.  (I'm not saying
> this bug exists, just that it's hard to see from the patch that it
> doesn't exist.)

I think I can go back to return value "buf_len - blen" and don't
check "err" anymore.

If the string buffer is truncated due to size limitation, we can still
return "buf_len - blen" so can allow caller to dump string as
possible.

If arm_spe_pkt_snprintf() returns -1 at the first call, "buf_len" is
equal to "blen", thus return value of "buf_len - blen" will be zero.
For this case, the caller will get to know there have no valid string
and will skip to print anything.

Does this make sense?

> > +
> >  	case ARM_SPE_OP_TYPE:
> >  		switch (idx) {
> > -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > -					"COND-SELECT" : "INSN-OTHER");
> > -		case 1:	{
> > -			size_t blen = buf_len;
> > +		case 0:
> > +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
> > +		case 1:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +					     payload & 0x1 ? "ST" : "LD");
> >  
> > -			if (payload & 0x1)
> > -				ret = snprintf(buf, buf_len, "ST");
> > -			else
> > -				ret = snprintf(buf, buf_len, "LD");
> > -			buf += ret;
> > -			blen -= ret;
> >  			if (payload & 0x2) {
> > -				if (payload & 0x4) {
> > -					ret = snprintf(buf, buf_len, " AT");
> > -					buf += ret;
> > -					blen -= ret;
> > -				}
> > -				if (payload & 0x8) {
> > -					ret = snprintf(buf, buf_len, " EXCL");
> > -					buf += ret;
> > -					blen -= ret;
> > -				}
> > -				if (payload & 0x10) {
> > -					ret = snprintf(buf, buf_len, " AR");
> > -					buf += ret;
> > -					blen -= ret;
> > -				}
> > +				if (payload & 0x4)
> > +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
> > +				if (payload & 0x8)
> > +					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
> > +				if (payload & 0x10)
> > +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
> >  			} else if (payload & 0x4) {
> > -				ret = snprintf(buf, buf_len, " SIMD-FP");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > -			if (ret < 0)
> > -				return ret;
> > -			blen -= ret;
> > -			return buf_len - blen;
> > -		}
> > -		case 2:	{
> > -			size_t blen = buf_len;
> > -
> > -			ret = snprintf(buf, buf_len, "B");
> > -			buf += ret;
> > -			blen -= ret;
> > -			if (payload & 0x1) {
> > -				ret = snprintf(buf, buf_len, " COND");
> > -				buf += ret;
> > -				blen -= ret;
> > -			}
> > -			if (payload & 0x2) {
> > -				ret = snprintf(buf, buf_len, " IND");
> > -				buf += ret;
> > -				blen -= ret;
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
> >  			}
> > -			if (ret < 0)
> > -				return ret;
> > -			blen -= ret;
> > -			return buf_len - blen;
> > -			}
> > -		default: return 0;
> > +
> > +			return err ?: (int)(buf_len - blen);
> > +
> > +		case 2:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
> > +
> > +			if (payload & 0x1)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
> > +			if (payload & 0x2)
> > +				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
> > +
> > +			return err ?: (int)(buf_len - blen);
> > +
> > +		default:
> > +			return 0;
> >  		}
> >  	case ARM_SPE_DATA_SOURCE:
> >  	case ARM_SPE_TIMESTAMP:
> > -		return snprintf(buf, buf_len, "%s %lld", name, payload);
> > +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
> >  	case ARM_SPE_ADDRESS:
> >  		switch (idx) {
> >  		case 0:
> >  		case 1: ns = !!(packet->payload & NS_FLAG);
> >  			el = (packet->payload & EL_FLAG) >> 61;
> >  			payload &= ~(0xffULL << 56);
> > -			return snprintf(buf, buf_len, "%s 0x%llx el%d ns=%d",
> > +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +					"%s 0x%llx el%d ns=%d",
> >  				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
> > -		case 2:	return snprintf(buf, buf_len, "VA 0x%llx", payload);
> > +		case 2:
> > +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +						    "VA 0x%llx", payload);
> >  		case 3:	ns = !!(packet->payload & NS_FLAG);
> >  			payload &= ~(0xffULL << 56);
> > -			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
> > -					payload, ns);
> > -		default: return 0;
> > +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> > +						    "PA 0x%llx ns=%d", payload, ns);
> > +		default:
> > +			return 0;
> >  		}
> >  	case ARM_SPE_CONTEXT:
> > -		return snprintf(buf, buf_len, "%s 0x%lx el%d", name,
> > -				(unsigned long)payload, idx + 1);
> > -	case ARM_SPE_COUNTER: {
> > -		size_t blen = buf_len;
> > -
> > -		ret = snprintf(buf, buf_len, "%s %d ", name,
> > -			       (unsigned short)payload);
> > -		buf += ret;
> > -		blen -= ret;
> > +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
> > +					    name, (unsigned long)payload, idx + 1);
> > +	case ARM_SPE_COUNTER:
> > +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
> > +				     (unsigned short)payload);
> > +
> >  		switch (idx) {
> > -		case 0:	ret = snprintf(buf, buf_len, "TOT"); break;
> > -		case 1:	ret = snprintf(buf, buf_len, "ISSUE"); break;
> > -		case 2:	ret = snprintf(buf, buf_len, "XLAT"); break;
> > -		default: ret = 0;
> > +		case 0:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
> > +			break;
> > +		case 1:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
> > +			break;
> > +		case 2:
> > +			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
> > +			break;
> > +		default:
> > +			break;
> >  		}
> > -		if (ret < 0)
> > -			return ret;
> > -		blen -= ret;
> > -		return buf_len - blen;
> > -	}
> > +
> > +		return err ?: (int)(buf_len - blen);
> > +
> >  	default:
> >  		break;
> >  	}
> >  
> > -	return snprintf(buf, buf_len, "%s 0x%llx (%d)",
> > -			name, payload, packet->index);
> > +	return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%llx (%d)",
> > +				    name, payload, packet->index);
> 
> Otherwise, the patch looks generally OK, but I'd like to see an answer
> on my points above.

Thanks a lot for reviewing!

Leo

Re: [PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[André Przywara]

On 03/11/2020 06:40, Leo Yan wrote:

Hi Dave, Leo,

> On Mon, Nov 02, 2020 at 05:06:53PM +0000, Dave Martin wrote:
>> On Fri, Oct 30, 2020 at 02:57:09AM +0000, Leo Yan wrote:
>>> When outputs strings to the decoding buffer with function snprintf(),
>>> SPE decoder needs to detects if any error returns from snprintf() and if
>>> so needs to directly bail out.  If snprintf() returns success, it needs
>>> to update buffer pointer and reduce the buffer length so can continue to
>>> output the next string into the consequent memory space.
>>>
>>> This complex logics are spreading in the function arm_spe_pkt_desc() so
>>> there has many duplicate codes for handling error detecting, increment
>>> buffer pointer and decrement buffer size.
>>>
>>> To avoid the duplicate code, this patch introduces a new helper function
>>> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
>>> it's used by the caller arm_spe_pkt_desc(); if printing buffer is called
>>> for multiple times in a flow, the error is a cumulative value and simply
>>> returns its final value.
>>>
>>> This patch also moves the variable 'blen' as the function's local
>>> variable, this allows to remove the unnecessary braces and improve the
>>> readability.
>>>
>>> Suggested-by: Dave Martin <Dave.Martin@arm.com>
>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>
>> This looks like a good refacroting now, but as pointed out by Andre this
>> patch is now rather hard to review, since it combines the refactoring
>> with other changes.
>>
>> If reposting this series, it would be good if this could be split into a
>> first patch that introduces arm_spe_pkt_snprintf() and just updates each
>> snprintf() call site to use it, but without moving other code around or
>> optimising anything, followed by one or more patches that clean up and
>> simplify arm_spe_pkt_desc().
> 
> I will respin the patch set and follow this approach.

Well, I am afraid this is not easily possible.

Dave: this patch is basically following the pattern turning this:
===============
	if (condition) {
		ret = snprintf(buf, buf_len, "foo");
		buf += ret;
		blen -= ret;
	}
	...
	if (ret < 0)
		return ret;
	blen -= ret;
	return buf_len - blen;
===============
into this:
---------------
	if (condition)
		arm_spe_pkt_snprintf(&err, &buf, &blen, "foo");
	...
	return err ?: (int)(buf_len - blen);
---------------

And "diff" is getting really ahead of itself here and tries to be super
clever, which leads to this hard to read patch.

But I don't think there is anything we can really do here, this is
already the minimal version. Leo adds the optimisations only later on,
in other patches.

Cheers,
Andre


> 
>> If the series is otherwise mature though, then this rework may be
>> overkill.
>>
>>> ---
>>>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 267 ++++++++----------
>>>  1 file changed, 117 insertions(+), 150 deletions(-)
>>>
>>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>>> index 04fd7fd7c15f..1ecaf9805b79 100644
>>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>>> @@ -9,6 +9,7 @@
>>>  #include <endian.h>
>>>  #include <byteswap.h>
>>>  #include <linux/bitops.h>
>>> +#include <stdarg.h>
>>>  
>>>  #include "arm-spe-pkt-decoder.h"
>>>  
>>> @@ -258,192 +259,158 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
>>>  	return ret;
>>>  }
>>>  
>>> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
>>> +				const char *fmt, ...)
>>> +{
>>> +	va_list ap;
>>> +	int ret;
>>> +
>>> +	/* Bail out if any error occurred */
>>> +	if (err && *err)
>>> +		return *err;
>>> +
>>> +	va_start(ap, fmt);
>>> +	ret = vsnprintf(*buf_p, *blen, fmt, ap);
>>> +	va_end(ap);
>>> +
>>> +	if (ret < 0) {
>>> +		if (err && !*err)
>>> +			*err = ret;
>>
>> What happens on buffer overrun (i.e., ret >= *blen)?
>>
>> It looks to me like we'll advance buf_p too far, blen will wrap around,
>> and the string at *buf_p won't be null terminated.  Because the return
>> value is still >= 0, this condition will be returned up the stack as
>> "success".
> 
> Thanks for pointint out this.  I never note for the potential issue
> caused by returned value (ret >= *blen);  checked again for the
> manual, it says:
> 
> "The functions snprintf() and vsnprintf() do not write more than size
> bytes (including the terminating null byte ('\0')).  If the output was
> truncated due to this limit, then the return value is the number of
> characters (excluding  the  terminating  null  byte) which would have
> been written to the final string if enough space had been available.
> Thus, a return value of size or more means that the output was
> truncated."
> 
>> Perhaps this can never happen given the actual buffer sizes and strings
>> being printed, but it feels a bit unsafe.
>>
>>
>> It may be better to clamp the adjustments to *buf_p and *blen to
>> *blen - 1 in this case, and explicitly set (*buf_p)[*blen - 1] to '\0'.
>> We _may_ want indicate failure in the return from arm_spe_pkt_desc() in
>> this situation, but I don't know enough about how this code is called to
>> enable me to judge that.
> 
> The caller arm_spe_dump() will print out the string if the return
> value > 0 [1]; so I think it can simply return the string length which
> has been written to the buffer (with the clamped value).  The function
> can be refined as below:
> 
> static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> 				const char *fmt, ...)
> {
> 	va_list ap;
> 	int ret;
> 
> 	/* Bail out if any error occurred */
> 	if (err && *err)
> 		return *err;
> 
> 	va_start(ap, fmt);
> 	ret = vsnprintf(*buf_p, *blen - 1, fmt, ap);
> 	va_end(ap);
> 
> 	if (ret < 0) {
> 		if (err && !*err)
> 			*err = ret;
> 	} else {
> 
>                 /*
>                  * A return value of (*blen - 1) or more means that the
>                  * output was truncated and the buffer is overrun.
>                  */
>                 if (ret >= (*blen - 1)) {
>                         (*buf_p)[*blen - 1] = '\0';
> 
>                         /*
>                          * Set *err to -1 to avoid overflow if tries to
>                          * fill this buffer sequentially.
>                          */
> 		        if (err && !*err)
> 		        	*err = -1;
> 
>                         /* Clamp the size of characters printed */
>                         ret = min(ret, *blen);
>                 }
> 
> 		*buf_p += ret;
> 		*blen -= ret;
> 	}
> 
> 	return ret;
> }
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/arm-spe.c#n116
> 
>> (Note, this issue is not introduced by this patch, but this refactoring
>> makes it easier to address it in a single place -- so it may now be
>> worth doing so.)
> 
> Agree.
> 
>>> +	} else {
>>> +		*buf_p += ret;
>>> +		*blen -= ret;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>>  		     size_t buf_len)
>>>  {
>>> -	int ret, ns, el, idx = packet->index;
>>> +	int ns, el, idx = packet->index;
>>>  	unsigned long long payload = packet->payload;
>>>  	const char *name = arm_spe_pkt_name(packet->type);
>>> +	size_t blen = buf_len;
>>> +	int err = 0;
>>>  
>>>  	switch (packet->type) {
>>>  	case ARM_SPE_BAD:
>>>  	case ARM_SPE_PAD:
>>>  	case ARM_SPE_END:
>>> -		return snprintf(buf, buf_len, "%s", name);
>>> -	case ARM_SPE_EVENTS: {
>>> -		size_t blen = buf_len;
>>> -
>>> -		ret = 0;
>>> -		ret = snprintf(buf, buf_len, "EV");
>>> -		buf += ret;
>>> -		blen -= ret;
>>> -		if (payload & 0x1) {
>>> -			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -		}
>>> -		if (payload & 0x2) {
>>> -			ret = snprintf(buf, buf_len, " RETIRED");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -		}
>>> -		if (payload & 0x4) {
>>> -			ret = snprintf(buf, buf_len, " L1D-ACCESS");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -		}
>>> -		if (payload & 0x8) {
>>> -			ret = snprintf(buf, buf_len, " L1D-REFILL");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -		}
>>> -		if (payload & 0x10) {
>>> -			ret = snprintf(buf, buf_len, " TLB-ACCESS");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -		}
>>> -		if (payload & 0x20) {
>>> -			ret = snprintf(buf, buf_len, " TLB-REFILL");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -		}
>>> -		if (payload & 0x40) {
>>> -			ret = snprintf(buf, buf_len, " NOT-TAKEN");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -		}
>>> -		if (payload & 0x80) {
>>> -			ret = snprintf(buf, buf_len, " MISPRED");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -		}
>>> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
>>> +	case ARM_SPE_EVENTS:
>>> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
>>> +
>>> +		if (payload & 0x1)
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
>>> +		if (payload & 0x2)
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
>>> +		if (payload & 0x4)
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
>>> +		if (payload & 0x8)
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
>>> +		if (payload & 0x10)
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
>>> +		if (payload & 0x20)
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
>>> +		if (payload & 0x40)
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
>>> +		if (payload & 0x80)
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
>>>  		if (idx > 1) {
>>> -			if (payload & 0x100) {
>>> -				ret = snprintf(buf, buf_len, " LLC-ACCESS");
>>> -				buf += ret;
>>> -				blen -= ret;
>>> -			}
>>> -			if (payload & 0x200) {
>>> -				ret = snprintf(buf, buf_len, " LLC-REFILL");
>>> -				buf += ret;
>>> -				blen -= ret;
>>> -			}
>>> -			if (payload & 0x400) {
>>> -				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
>>> -				buf += ret;
>>> -				blen -= ret;
>>> -			}
>>> +			if (payload & 0x100)
>>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
>>> +			if (payload & 0x200)
>>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
>>> +			if (payload & 0x400)
>>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
>>>  		}
>>> -		if (ret < 0)
>>> -			return ret;
>>> -		blen -= ret;
>>> -		return buf_len - blen;
>>
>> It looks like we now fall off the bottom of the switch() here.  It's
>> preferable to add a break, both to document the intended code flow, and
>> to avoid accidents if another case is added later.
> 
> Will do this.
> 
>>> -	}
>>> +		return err ?: (int)(buf_len - blen);
>>
>> Nit: unexplained type cast.  Does the result definitely fit into an int?
>> If not, why doesn't it matter?
> 
> The case is to dismiss the compiler warning.
> 
> GCC doesn't compliant for the code:
> 
>   return buf_len - blen;
>   
> But GCC compliants type mismatch after change to code:
> 
>   return err ?: buf_len - blen;
> 
>> Also:
>>
>> If the actual return value is important for the caller to determine the
>> number of bytes appended, then it would be better to compute it in one
>> place.  Otherwise, it would be better to squash all success returns to 0,
>> rather than spend effort computing a value that may be misleading or
>> wrong anyway.
>>
>> In its current form, it's tricky to see whether this patch derives the
>> return value consistently for all cases.  In particular, if some code
>> patch does one or more arm_spe_pkt_snprintf(), followed by a
>> return arm_spe_pkt_snprintf(); then the return value (which only takes
>> the last arm_spe_pkt_desc() into account) would be wrong.  It would be
>> preferable if the return value for the success case were always
>> computed from buf_len and blen, to avoid this risk.  (I'm not saying
>> this bug exists, just that it's hard to see from the patch that it
>> doesn't exist.)
> 
> I think I can go back to return value "buf_len - blen" and don't
> check "err" anymore.
> 
> If the string buffer is truncated due to size limitation, we can still
> return "buf_len - blen" so can allow caller to dump string as
> possible.
> 
> If arm_spe_pkt_snprintf() returns -1 at the first call, "buf_len" is
> equal to "blen", thus return value of "buf_len - blen" will be zero.
> For this case, the caller will get to know there have no valid string
> and will skip to print anything.
> 
> Does this make sense?
> 
>>> +
>>>  	case ARM_SPE_OP_TYPE:
>>>  		switch (idx) {
>>> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>>> -					"COND-SELECT" : "INSN-OTHER");
>>> -		case 1:	{
>>> -			size_t blen = buf_len;
>>> +		case 0:
>>> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
>>> +					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
>>> +		case 1:
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen,
>>> +					     payload & 0x1 ? "ST" : "LD");
>>>  
>>> -			if (payload & 0x1)
>>> -				ret = snprintf(buf, buf_len, "ST");
>>> -			else
>>> -				ret = snprintf(buf, buf_len, "LD");
>>> -			buf += ret;
>>> -			blen -= ret;
>>>  			if (payload & 0x2) {
>>> -				if (payload & 0x4) {
>>> -					ret = snprintf(buf, buf_len, " AT");
>>> -					buf += ret;
>>> -					blen -= ret;
>>> -				}
>>> -				if (payload & 0x8) {
>>> -					ret = snprintf(buf, buf_len, " EXCL");
>>> -					buf += ret;
>>> -					blen -= ret;
>>> -				}
>>> -				if (payload & 0x10) {
>>> -					ret = snprintf(buf, buf_len, " AR");
>>> -					buf += ret;
>>> -					blen -= ret;
>>> -				}
>>> +				if (payload & 0x4)
>>> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
>>> +				if (payload & 0x8)
>>> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
>>> +				if (payload & 0x10)
>>> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
>>>  			} else if (payload & 0x4) {
>>> -				ret = snprintf(buf, buf_len, " SIMD-FP");
>>> -				buf += ret;
>>> -				blen -= ret;
>>> -			}
>>> -			if (ret < 0)
>>> -				return ret;
>>> -			blen -= ret;
>>> -			return buf_len - blen;
>>> -		}
>>> -		case 2:	{
>>> -			size_t blen = buf_len;
>>> -
>>> -			ret = snprintf(buf, buf_len, "B");
>>> -			buf += ret;
>>> -			blen -= ret;
>>> -			if (payload & 0x1) {
>>> -				ret = snprintf(buf, buf_len, " COND");
>>> -				buf += ret;
>>> -				blen -= ret;
>>> -			}
>>> -			if (payload & 0x2) {
>>> -				ret = snprintf(buf, buf_len, " IND");
>>> -				buf += ret;
>>> -				blen -= ret;
>>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
>>>  			}
>>> -			if (ret < 0)
>>> -				return ret;
>>> -			blen -= ret;
>>> -			return buf_len - blen;
>>> -			}
>>> -		default: return 0;
>>> +
>>> +			return err ?: (int)(buf_len - blen);
>>> +
>>> +		case 2:
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
>>> +
>>> +			if (payload & 0x1)
>>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
>>> +			if (payload & 0x2)
>>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
>>> +
>>> +			return err ?: (int)(buf_len - blen);
>>> +
>>> +		default:
>>> +			return 0;
>>>  		}
>>>  	case ARM_SPE_DATA_SOURCE:
>>>  	case ARM_SPE_TIMESTAMP:
>>> -		return snprintf(buf, buf_len, "%s %lld", name, payload);
>>> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
>>>  	case ARM_SPE_ADDRESS:
>>>  		switch (idx) {
>>>  		case 0:
>>>  		case 1: ns = !!(packet->payload & NS_FLAG);
>>>  			el = (packet->payload & EL_FLAG) >> 61;
>>>  			payload &= ~(0xffULL << 56);
>>> -			return snprintf(buf, buf_len, "%s 0x%llx el%d ns=%d",
>>> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
>>> +					"%s 0x%llx el%d ns=%d",
>>>  				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
>>> -		case 2:	return snprintf(buf, buf_len, "VA 0x%llx", payload);
>>> +		case 2:
>>> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
>>> +						    "VA 0x%llx", payload);
>>>  		case 3:	ns = !!(packet->payload & NS_FLAG);
>>>  			payload &= ~(0xffULL << 56);
>>> -			return snprintf(buf, buf_len, "PA 0x%llx ns=%d",
>>> -					payload, ns);
>>> -		default: return 0;
>>> +			return arm_spe_pkt_snprintf(&err, &buf, &blen,
>>> +						    "PA 0x%llx ns=%d", payload, ns);
>>> +		default:
>>> +			return 0;
>>>  		}
>>>  	case ARM_SPE_CONTEXT:
>>> -		return snprintf(buf, buf_len, "%s 0x%lx el%d", name,
>>> -				(unsigned long)payload, idx + 1);
>>> -	case ARM_SPE_COUNTER: {
>>> -		size_t blen = buf_len;
>>> -
>>> -		ret = snprintf(buf, buf_len, "%s %d ", name,
>>> -			       (unsigned short)payload);
>>> -		buf += ret;
>>> -		blen -= ret;
>>> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
>>> +					    name, (unsigned long)payload, idx + 1);
>>> +	case ARM_SPE_COUNTER:
>>> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
>>> +				     (unsigned short)payload);
>>> +
>>>  		switch (idx) {
>>> -		case 0:	ret = snprintf(buf, buf_len, "TOT"); break;
>>> -		case 1:	ret = snprintf(buf, buf_len, "ISSUE"); break;
>>> -		case 2:	ret = snprintf(buf, buf_len, "XLAT"); break;
>>> -		default: ret = 0;
>>> +		case 0:
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
>>> +			break;
>>> +		case 1:
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
>>> +			break;
>>> +		case 2:
>>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
>>> +			break;
>>> +		default:
>>> +			break;
>>>  		}
>>> -		if (ret < 0)
>>> -			return ret;
>>> -		blen -= ret;
>>> -		return buf_len - blen;
>>> -	}
>>> +
>>> +		return err ?: (int)(buf_len - blen);
>>> +
>>>  	default:
>>>  		break;
>>>  	}
>>>  
>>> -	return snprintf(buf, buf_len, "%s 0x%llx (%d)",
>>> -			name, payload, packet->index);
>>> +	return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%llx (%d)",
>>> +				    name, payload, packet->index);
>>
>> Otherwise, the patch looks generally OK, but I'd like to see an answer
>> on my points above.
> 
> Thanks a lot for reviewing!
> 
> Leo
> 

Re: [PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[Dave Martin]

On Tue, Nov 03, 2020 at 10:13:49AM +0000, André Przywara wrote:
> On 03/11/2020 06:40, Leo Yan wrote:
> 
> Hi Dave, Leo,
> 
> > On Mon, Nov 02, 2020 at 05:06:53PM +0000, Dave Martin wrote:
> >> On Fri, Oct 30, 2020 at 02:57:09AM +0000, Leo Yan wrote:
> >>> When outputs strings to the decoding buffer with function snprintf(),
> >>> SPE decoder needs to detects if any error returns from snprintf() and if
> >>> so needs to directly bail out.  If snprintf() returns success, it needs
> >>> to update buffer pointer and reduce the buffer length so can continue to
> >>> output the next string into the consequent memory space.
> >>>
> >>> This complex logics are spreading in the function arm_spe_pkt_desc() so
> >>> there has many duplicate codes for handling error detecting, increment
> >>> buffer pointer and decrement buffer size.
> >>>
> >>> To avoid the duplicate code, this patch introduces a new helper function
> >>> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> >>> it's used by the caller arm_spe_pkt_desc(); if printing buffer is called
> >>> for multiple times in a flow, the error is a cumulative value and simply
> >>> returns its final value.
> >>>
> >>> This patch also moves the variable 'blen' as the function's local
> >>> variable, this allows to remove the unnecessary braces and improve the
> >>> readability.
> >>>
> >>> Suggested-by: Dave Martin <Dave.Martin@arm.com>
> >>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >>
> >> This looks like a good refacroting now, but as pointed out by Andre this
> >> patch is now rather hard to review, since it combines the refactoring
> >> with other changes.
> >>
> >> If reposting this series, it would be good if this could be split into a
> >> first patch that introduces arm_spe_pkt_snprintf() and just updates each
> >> snprintf() call site to use it, but without moving other code around or
> >> optimising anything, followed by one or more patches that clean up and
> >> simplify arm_spe_pkt_desc().
> > 
> > I will respin the patch set and follow this approach.
> 
> Well, I am afraid this is not easily possible.
> 
> Dave: this patch is basically following the pattern turning this:
> ===============
> 	if (condition) {
> 		ret = snprintf(buf, buf_len, "foo");
> 		buf += ret;
> 		blen -= ret;
> 	}
> 	...
> 	if (ret < 0)
> 		return ret;
> 	blen -= ret;
> 	return buf_len - blen;
> ===============
> into this:
> ---------------
> 	if (condition)
> 		arm_spe_pkt_snprintf(&err, &buf, &blen, "foo");
> 	...
> 	return err ?: (int)(buf_len - blen);
> ---------------
> 
> And "diff" is getting really ahead of itself here and tries to be super
> clever, which leads to this hard to read patch.
> 
> But I don't think there is anything we can really do here, this is
> already the minimal version. Leo adds the optimisations only later on,
> in other patches.

OK, this is only nice-to-have, if feasible -- so I'd say Leo should not
bother to split this patch up unless it is easy to do and actually does
make the diff more intelligible!


> 
> Cheers,
> Andre
> 
> 
> > 
> >> If the series is otherwise mature though, then this rework may be
> >> overkill.
> >>
> >>> ---
> >>>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 267 ++++++++----------
> >>>  1 file changed, 117 insertions(+), 150 deletions(-)
> >>>
> >>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >>> index 04fd7fd7c15f..1ecaf9805b79 100644
> >>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >>> @@ -9,6 +9,7 @@
> >>>  #include <endian.h>
> >>>  #include <byteswap.h>
> >>>  #include <linux/bitops.h>
> >>> +#include <stdarg.h>
> >>>  
> >>>  #include "arm-spe-pkt-decoder.h"
> >>>  
> >>> @@ -258,192 +259,158 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
> >>>  	return ret;
> >>>  }
> >>>  
> >>> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> >>> +				const char *fmt, ...)
> >>> +{
> >>> +	va_list ap;
> >>> +	int ret;
> >>> +
> >>> +	/* Bail out if any error occurred */
> >>> +	if (err && *err)
> >>> +		return *err;
> >>> +
> >>> +	va_start(ap, fmt);
> >>> +	ret = vsnprintf(*buf_p, *blen, fmt, ap);
> >>> +	va_end(ap);
> >>> +
> >>> +	if (ret < 0) {
> >>> +		if (err && !*err)
> >>> +			*err = ret;
> >>
> >> What happens on buffer overrun (i.e., ret >= *blen)?
> >>
> >> It looks to me like we'll advance buf_p too far, blen will wrap around,
> >> and the string at *buf_p won't be null terminated.  Because the return
> >> value is still >= 0, this condition will be returned up the stack as
> >> "success".
> > 
> > Thanks for pointint out this.  I never note for the potential issue
> > caused by returned value (ret >= *blen);  checked again for the
> > manual, it says:
> > 
> > "The functions snprintf() and vsnprintf() do not write more than size
> > bytes (including the terminating null byte ('\0')).  If the output was
> > truncated due to this limit, then the return value is the number of
> > characters (excluding  the  terminating  null  byte) which would have
> > been written to the final string if enough space had been available.
> > Thus, a return value of size or more means that the output was
> > truncated."
> > 
> >> Perhaps this can never happen given the actual buffer sizes and strings
> >> being printed, but it feels a bit unsafe.
> >>
> >>
> >> It may be better to clamp the adjustments to *buf_p and *blen to
> >> *blen - 1 in this case, and explicitly set (*buf_p)[*blen - 1] to '\0'.
> >> We _may_ want indicate failure in the return from arm_spe_pkt_desc() in
> >> this situation, but I don't know enough about how this code is called to
> >> enable me to judge that.
> > 
> > The caller arm_spe_dump() will print out the string if the return
> > value > 0 [1]; so I think it can simply return the string length which
> > has been written to the buffer (with the clamped value).  The function
> > can be refined as below:
> > 
> > static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> > 				const char *fmt, ...)
> > {
> > 	va_list ap;
> > 	int ret;
> > 
> > 	/* Bail out if any error occurred */
> > 	if (err && *err)
> > 		return *err;
> > 
> > 	va_start(ap, fmt);
> > 	ret = vsnprintf(*buf_p, *blen - 1, fmt, ap);

I think this should still be *blen.  Passing *blen - 1 here will limit
the number of chars of generated text (excluding the '\0') to *blen - 2,
which does not seem to be what is intended.

> > 	va_end(ap);
> > 
> > 	if (ret < 0) {
> > 		if (err && !*err)
> > 			*err = ret;
> > 	} else {
> > 
> >                 /*
> >                  * A return value of (*blen - 1) or more means that the
> >                  * output was truncated and the buffer is overrun.
> >                  */
> >                 if (ret >= (*blen - 1)) {
> >                         (*buf_p)[*blen - 1] = '\0';
> > 
> >                         /*
> >                          * Set *err to -1 to avoid overflow if tries to
> >                          * fill this buffer sequentially.
> >                          */
> > 		        if (err && !*err)
> > 		        	*err = -1;

Makes sense, though the important thing here is to make sure that the
caller knows the output was truncated.  

> > 
> >                         /* Clamp the size of characters printed */
> >                         ret = min(ret, *blen);

I'm not sure it matters, but it seems more logical for this to be
*blen - 1.  Otherwise, the return value counts the '\0' that we just
added as one of the chars "printed".  That seems a bit odd.

Alternatively, if nobody cares about the actual number returned, it may
make sense to stick to snprintf() semantics -- i.e., just return the
value that snprintf returned.

> >                 }
> > 
> > 		*buf_p += ret;
> > 		*blen -= ret;
> > 	}
> > 
> > 	return ret;
> > }
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/arm-spe.c#n116
> > 
> >> (Note, this issue is not introduced by this patch, but this refactoring
> >> makes it easier to address it in a single place -- so it may now be
> >> worth doing so.)
> > 
> > Agree.
> > 
> >>> +	} else {
> >>> +		*buf_p += ret;
> >>> +		*blen -= ret;
> >>> +	}
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >>>  		     size_t buf_len)
> >>>  {
> >>> -	int ret, ns, el, idx = packet->index;
> >>> +	int ns, el, idx = packet->index;
> >>>  	unsigned long long payload = packet->payload;
> >>>  	const char *name = arm_spe_pkt_name(packet->type);
> >>> +	size_t blen = buf_len;
> >>> +	int err = 0;
> >>>  
> >>>  	switch (packet->type) {
> >>>  	case ARM_SPE_BAD:
> >>>  	case ARM_SPE_PAD:
> >>>  	case ARM_SPE_END:
> >>> -		return snprintf(buf, buf_len, "%s", name);
> >>> -	case ARM_SPE_EVENTS: {
> >>> -		size_t blen = buf_len;
> >>> -
> >>> -		ret = 0;
> >>> -		ret = snprintf(buf, buf_len, "EV");
> >>> -		buf += ret;
> >>> -		blen -= ret;
> >>> -		if (payload & 0x1) {
> >>> -			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> >>> -			buf += ret;
> >>> -			blen -= ret;
> >>> -		}
> >>> -		if (payload & 0x2) {
> >>> -			ret = snprintf(buf, buf_len, " RETIRED");
> >>> -			buf += ret;
> >>> -			blen -= ret;
> >>> -		}
> >>> -		if (payload & 0x4) {
> >>> -			ret = snprintf(buf, buf_len, " L1D-ACCESS");
> >>> -			buf += ret;
> >>> -			blen -= ret;
> >>> -		}
> >>> -		if (payload & 0x8) {
> >>> -			ret = snprintf(buf, buf_len, " L1D-REFILL");
> >>> -			buf += ret;
> >>> -			blen -= ret;
> >>> -		}
> >>> -		if (payload & 0x10) {
> >>> -			ret = snprintf(buf, buf_len, " TLB-ACCESS");
> >>> -			buf += ret;
> >>> -			blen -= ret;
> >>> -		}
> >>> -		if (payload & 0x20) {
> >>> -			ret = snprintf(buf, buf_len, " TLB-REFILL");
> >>> -			buf += ret;
> >>> -			blen -= ret;
> >>> -		}
> >>> -		if (payload & 0x40) {
> >>> -			ret = snprintf(buf, buf_len, " NOT-TAKEN");
> >>> -			buf += ret;
> >>> -			blen -= ret;
> >>> -		}
> >>> -		if (payload & 0x80) {
> >>> -			ret = snprintf(buf, buf_len, " MISPRED");
> >>> -			buf += ret;
> >>> -			blen -= ret;
> >>> -		}
> >>> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
> >>> +	case ARM_SPE_EVENTS:
> >>> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
> >>> +
> >>> +		if (payload & 0x1)
> >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
> >>> +		if (payload & 0x2)
> >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
> >>> +		if (payload & 0x4)
> >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
> >>> +		if (payload & 0x8)
> >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
> >>> +		if (payload & 0x10)
> >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
> >>> +		if (payload & 0x20)
> >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
> >>> +		if (payload & 0x40)
> >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
> >>> +		if (payload & 0x80)
> >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
> >>>  		if (idx > 1) {
> >>> -			if (payload & 0x100) {
> >>> -				ret = snprintf(buf, buf_len, " LLC-ACCESS");
> >>> -				buf += ret;
> >>> -				blen -= ret;
> >>> -			}
> >>> -			if (payload & 0x200) {
> >>> -				ret = snprintf(buf, buf_len, " LLC-REFILL");
> >>> -				buf += ret;
> >>> -				blen -= ret;
> >>> -			}
> >>> -			if (payload & 0x400) {
> >>> -				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
> >>> -				buf += ret;
> >>> -				blen -= ret;
> >>> -			}
> >>> +			if (payload & 0x100)
> >>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
> >>> +			if (payload & 0x200)
> >>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
> >>> +			if (payload & 0x400)
> >>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
> >>>  		}
> >>> -		if (ret < 0)
> >>> -			return ret;
> >>> -		blen -= ret;
> >>> -		return buf_len - blen;
> >>
> >> It looks like we now fall off the bottom of the switch() here.  It's
> >> preferable to add a break, both to document the intended code flow, and
> >> to avoid accidents if another case is added later.
> > 
> > Will do this.
> > 
> >>> -	}
> >>> +		return err ?: (int)(buf_len - blen);
> >>
> >> Nit: unexplained type cast.  Does the result definitely fit into an int?
> >> If not, why doesn't it matter?
> > 
> > The case is to dismiss the compiler warning.
> > 
> > GCC doesn't compliant for the code:
> > 
> >   return buf_len - blen;
> >   
> > But GCC compliants type mismatch after change to code:
> > 
> >   return err ?: buf_len - blen;

So, does buf_len - blen fit in an int?

> > 
> >> Also:
> >>
> >> If the actual return value is important for the caller to determine the
> >> number of bytes appended, then it would be better to compute it in one
> >> place.  Otherwise, it would be better to squash all success returns to 0,
> >> rather than spend effort computing a value that may be misleading or
> >> wrong anyway.
> >>
> >> In its current form, it's tricky to see whether this patch derives the
> >> return value consistently for all cases.  In particular, if some code
> >> patch does one or more arm_spe_pkt_snprintf(), followed by a
> >> return arm_spe_pkt_snprintf(); then the return value (which only takes
> >> the last arm_spe_pkt_desc() into account) would be wrong.  It would be
> >> preferable if the return value for the success case were always
> >> computed from buf_len and blen, to avoid this risk.  (I'm not saying
> >> this bug exists, just that it's hard to see from the patch that it
> >> doesn't exist.)
> > 
> > I think I can go back to return value "buf_len - blen" and don't
> > check "err" anymore.

Probably, now that blen is adjusted in one place -- at least for success
returns.

We still need to indicate failure though, so we may need to check err
in some places.

One option is to have snprintf() semantics for the return value of this
whole function too, so that the caller can detect truncation (if wanted)
by comparing the return value against buf_len.  We'd need to make sure
all callers are updated appropriately if going down that route.

> > 
> > If the string buffer is truncated due to size limitation, we can still
> > return "buf_len - blen" so can allow caller to dump string as
> > possible.

This really depends on what the caller is trying to do.  In the case of
truncation, will we send unparseable junk to userspace?  Will userspace
be able to detect that this has happened and recover gracefully?

> > If arm_spe_pkt_snprintf() returns -1 at the first call, "buf_len" is
> > equal to "blen", thus return value of "buf_len - blen" will be zero.
> > For this case, the caller will get to know there have no valid string
> > and will skip to print anything.
> > 
> > Does this make sense?

Probably, though it's easy for callers to forget about such conventions
and assume that 0 means "success".

[...]

Cheers
---Dave

Re: [PATCH v6 06/21] perf arm-spe: Refactor printing string to buffer

[Leo Yan]

On Tue, Nov 03, 2020 at 12:13:04PM +0000, Dave Martin wrote:
> On Tue, Nov 03, 2020 at 10:13:49AM +0000, André Przywara wrote:
> > On 03/11/2020 06:40, Leo Yan wrote:
> > 
> > Hi Dave, Leo,
> > 
> > > On Mon, Nov 02, 2020 at 05:06:53PM +0000, Dave Martin wrote:
> > >> On Fri, Oct 30, 2020 at 02:57:09AM +0000, Leo Yan wrote:
> > >>> When outputs strings to the decoding buffer with function snprintf(),
> > >>> SPE decoder needs to detects if any error returns from snprintf() and if
> > >>> so needs to directly bail out.  If snprintf() returns success, it needs
> > >>> to update buffer pointer and reduce the buffer length so can continue to
> > >>> output the next string into the consequent memory space.
> > >>>
> > >>> This complex logics are spreading in the function arm_spe_pkt_desc() so
> > >>> there has many duplicate codes for handling error detecting, increment
> > >>> buffer pointer and decrement buffer size.
> > >>>
> > >>> To avoid the duplicate code, this patch introduces a new helper function
> > >>> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> > >>> it's used by the caller arm_spe_pkt_desc(); if printing buffer is called
> > >>> for multiple times in a flow, the error is a cumulative value and simply
> > >>> returns its final value.
> > >>>
> > >>> This patch also moves the variable 'blen' as the function's local
> > >>> variable, this allows to remove the unnecessary braces and improve the
> > >>> readability.
> > >>>
> > >>> Suggested-by: Dave Martin <Dave.Martin@arm.com>
> > >>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > >>
> > >> This looks like a good refacroting now, but as pointed out by Andre this
> > >> patch is now rather hard to review, since it combines the refactoring
> > >> with other changes.
> > >>
> > >> If reposting this series, it would be good if this could be split into a
> > >> first patch that introduces arm_spe_pkt_snprintf() and just updates each
> > >> snprintf() call site to use it, but without moving other code around or
> > >> optimising anything, followed by one or more patches that clean up and
> > >> simplify arm_spe_pkt_desc().
> > > 
> > > I will respin the patch set and follow this approach.
> > 
> > Well, I am afraid this is not easily possible.
> > 
> > Dave: this patch is basically following the pattern turning this:
> > ===============
> > 	if (condition) {
> > 		ret = snprintf(buf, buf_len, "foo");
> > 		buf += ret;
> > 		blen -= ret;
> > 	}
> > 	...
> > 	if (ret < 0)
> > 		return ret;
> > 	blen -= ret;
> > 	return buf_len - blen;
> > ===============
> > into this:
> > ---------------
> > 	if (condition)
> > 		arm_spe_pkt_snprintf(&err, &buf, &blen, "foo");
> > 	...
> > 	return err ?: (int)(buf_len - blen);
> > ---------------
> > 
> > And "diff" is getting really ahead of itself here and tries to be super
> > clever, which leads to this hard to read patch.
> > 
> > But I don't think there is anything we can really do here, this is
> > already the minimal version. Leo adds the optimisations only later on,
> > in other patches.

Thanks for explaination, Andre.

> OK, this is only nice-to-have, if feasible -- so I'd say Leo should not
> bother to split this patch up unless it is easy to do and actually does
> make the diff more intelligible!

Anyway, I will give a try, if it needs much time or it's difficult,
I will keep current form.

> > 
> > Cheers,
> > Andre
> > 
> > 
> > > 
> > >> If the series is otherwise mature though, then this rework may be
> > >> overkill.
> > >>
> > >>> ---
> > >>>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 267 ++++++++----------
> > >>>  1 file changed, 117 insertions(+), 150 deletions(-)
> > >>>
> > >>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >>> index 04fd7fd7c15f..1ecaf9805b79 100644
> > >>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >>> @@ -9,6 +9,7 @@
> > >>>  #include <endian.h>
> > >>>  #include <byteswap.h>
> > >>>  #include <linux/bitops.h>
> > >>> +#include <stdarg.h>
> > >>>  
> > >>>  #include "arm-spe-pkt-decoder.h"
> > >>>  
> > >>> @@ -258,192 +259,158 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len,
> > >>>  	return ret;
> > >>>  }
> > >>>  
> > >>> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> > >>> +				const char *fmt, ...)
> > >>> +{
> > >>> +	va_list ap;
> > >>> +	int ret;
> > >>> +
> > >>> +	/* Bail out if any error occurred */
> > >>> +	if (err && *err)
> > >>> +		return *err;
> > >>> +
> > >>> +	va_start(ap, fmt);
> > >>> +	ret = vsnprintf(*buf_p, *blen, fmt, ap);
> > >>> +	va_end(ap);
> > >>> +
> > >>> +	if (ret < 0) {
> > >>> +		if (err && !*err)
> > >>> +			*err = ret;
> > >>
> > >> What happens on buffer overrun (i.e., ret >= *blen)?
> > >>
> > >> It looks to me like we'll advance buf_p too far, blen will wrap around,
> > >> and the string at *buf_p won't be null terminated.  Because the return
> > >> value is still >= 0, this condition will be returned up the stack as
> > >> "success".
> > > 
> > > Thanks for pointint out this.  I never note for the potential issue
> > > caused by returned value (ret >= *blen);  checked again for the
> > > manual, it says:
> > > 
> > > "The functions snprintf() and vsnprintf() do not write more than size
> > > bytes (including the terminating null byte ('\0')).  If the output was
> > > truncated due to this limit, then the return value is the number of
> > > characters (excluding  the  terminating  null  byte) which would have
> > > been written to the final string if enough space had been available.
> > > Thus, a return value of size or more means that the output was
> > > truncated."
> > > 
> > >> Perhaps this can never happen given the actual buffer sizes and strings
> > >> being printed, but it feels a bit unsafe.
> > >>
> > >>
> > >> It may be better to clamp the adjustments to *buf_p and *blen to
> > >> *blen - 1 in this case, and explicitly set (*buf_p)[*blen - 1] to '\0'.
> > >> We _may_ want indicate failure in the return from arm_spe_pkt_desc() in
> > >> this situation, but I don't know enough about how this code is called to
> > >> enable me to judge that.
> > > 
> > > The caller arm_spe_dump() will print out the string if the return
> > > value > 0 [1]; so I think it can simply return the string length which
> > > has been written to the buffer (with the clamped value).  The function
> > > can be refined as below:
> > > 
> > > static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> > > 				const char *fmt, ...)
> > > {
> > > 	va_list ap;
> > > 	int ret;
> > > 
> > > 	/* Bail out if any error occurred */
> > > 	if (err && *err)
> > > 		return *err;
> > > 
> > > 	va_start(ap, fmt);
> > > 	ret = vsnprintf(*buf_p, *blen - 1, fmt, ap);
> 
> I think this should still be *blen.  Passing *blen - 1 here will limit
> the number of chars of generated text (excluding the '\0') to *blen - 2,
> which does not seem to be what is intended.

Agree, will use *blen.

> > > 	va_end(ap);
> > > 
> > > 	if (ret < 0) {
> > > 		if (err && !*err)
> > > 			*err = ret;
> > > 	} else {
> > > 
> > >                 /*
> > >                  * A return value of (*blen - 1) or more means that the
> > >                  * output was truncated and the buffer is overrun.
> > >                  */
> > >                 if (ret >= (*blen - 1)) {
> > >                         (*buf_p)[*blen - 1] = '\0';
> > > 
> > >                         /*
> > >                          * Set *err to -1 to avoid overflow if tries to
> > >                          * fill this buffer sequentially.
> > >                          */
> > > 		        if (err && !*err)
> > > 		        	*err = -1;
> 
> Makes sense, though the important thing here is to make sure that the
> caller knows the output was truncated.  

For explicitly indicate the output was truncated, here can set
"*err = ret".

> > > 
> > >                         /* Clamp the size of characters printed */
> > >                         ret = min(ret, *blen);
> 
> I'm not sure it matters, but it seems more logical for this to be
> *blen - 1.  Otherwise, the return value counts the '\0' that we just
> added as one of the chars "printed".  That seems a bit odd.
> 
> Alternatively, if nobody cares about the actual number returned, it may
> make sense to stick to snprintf() semantics -- i.e., just return the
> value that snprintf returned.

Good point.  If connect with the below sentence "*blen -= ret", it
uses "ret" to decrease the string size which has been outputted to the
buffer, this is why here clamp the "ret" value.

For stciking the return value to snprintf() semantics and use more
explicit way to handle the case for "ret >= (*blen - 1)", the function
can be refined as:

static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
				const char *fmt, ...)
{
	va_list ap;
	int ret;

	/* Bail out if any error occurred */
	if (err && *err)
		return *err;

	va_start(ap, fmt);
	ret = vsnprintf(*buf_p, *blen, fmt, ap);
	va_end(ap);

	if (ret < 0) {
		if (err && !*err)
			*err = ret;

	/*
	 * A return value of (*blen - 1) or more means that the
	 * output was truncated and the buffer is overrun.
	 */
	} else if (ret >= (*blen - 1)) {
		(*buf_p)[*blen - 1] = '\0';

		/*
		 * Set *err to -1 to avoid overflow if tries to
		 * fill this buffer sequentially.
		 */
		if (err && !*err)
			*err = ret;

		*buf_p += (*blen - 1);
		*blen -= (*blen - 1);
	} else {
		*buf_p += ret;
		*blen -= ret;
	}

	return ret;
}

> > >                 }
> > > 
> > > 		*buf_p += ret;
> > > 		*blen -= ret;
> > > 	}
> > > 
> > > 	return ret;
> > > }
> > > 
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/arm-spe.c#n116
> > > 
> > >> (Note, this issue is not introduced by this patch, but this refactoring
> > >> makes it easier to address it in a single place -- so it may now be
> > >> worth doing so.)
> > > 
> > > Agree.
> > > 
> > >>> +	} else {
> > >>> +		*buf_p += ret;
> > >>> +		*blen -= ret;
> > >>> +	}
> > >>> +
> > >>> +	return ret;
> > >>> +}
> > >>> +
> > >>>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > >>>  		     size_t buf_len)
> > >>>  {
> > >>> -	int ret, ns, el, idx = packet->index;
> > >>> +	int ns, el, idx = packet->index;
> > >>>  	unsigned long long payload = packet->payload;
> > >>>  	const char *name = arm_spe_pkt_name(packet->type);
> > >>> +	size_t blen = buf_len;
> > >>> +	int err = 0;
> > >>>  
> > >>>  	switch (packet->type) {
> > >>>  	case ARM_SPE_BAD:
> > >>>  	case ARM_SPE_PAD:
> > >>>  	case ARM_SPE_END:
> > >>> -		return snprintf(buf, buf_len, "%s", name);
> > >>> -	case ARM_SPE_EVENTS: {
> > >>> -		size_t blen = buf_len;
> > >>> -
> > >>> -		ret = 0;
> > >>> -		ret = snprintf(buf, buf_len, "EV");
> > >>> -		buf += ret;
> > >>> -		blen -= ret;
> > >>> -		if (payload & 0x1) {
> > >>> -			ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> > >>> -			buf += ret;
> > >>> -			blen -= ret;
> > >>> -		}
> > >>> -		if (payload & 0x2) {
> > >>> -			ret = snprintf(buf, buf_len, " RETIRED");
> > >>> -			buf += ret;
> > >>> -			blen -= ret;
> > >>> -		}
> > >>> -		if (payload & 0x4) {
> > >>> -			ret = snprintf(buf, buf_len, " L1D-ACCESS");
> > >>> -			buf += ret;
> > >>> -			blen -= ret;
> > >>> -		}
> > >>> -		if (payload & 0x8) {
> > >>> -			ret = snprintf(buf, buf_len, " L1D-REFILL");
> > >>> -			buf += ret;
> > >>> -			blen -= ret;
> > >>> -		}
> > >>> -		if (payload & 0x10) {
> > >>> -			ret = snprintf(buf, buf_len, " TLB-ACCESS");
> > >>> -			buf += ret;
> > >>> -			blen -= ret;
> > >>> -		}
> > >>> -		if (payload & 0x20) {
> > >>> -			ret = snprintf(buf, buf_len, " TLB-REFILL");
> > >>> -			buf += ret;
> > >>> -			blen -= ret;
> > >>> -		}
> > >>> -		if (payload & 0x40) {
> > >>> -			ret = snprintf(buf, buf_len, " NOT-TAKEN");
> > >>> -			buf += ret;
> > >>> -			blen -= ret;
> > >>> -		}
> > >>> -		if (payload & 0x80) {
> > >>> -			ret = snprintf(buf, buf_len, " MISPRED");
> > >>> -			buf += ret;
> > >>> -			blen -= ret;
> > >>> -		}
> > >>> +		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
> > >>> +	case ARM_SPE_EVENTS:
> > >>> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
> > >>> +
> > >>> +		if (payload & 0x1)
> > >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
> > >>> +		if (payload & 0x2)
> > >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
> > >>> +		if (payload & 0x4)
> > >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
> > >>> +		if (payload & 0x8)
> > >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
> > >>> +		if (payload & 0x10)
> > >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
> > >>> +		if (payload & 0x20)
> > >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
> > >>> +		if (payload & 0x40)
> > >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
> > >>> +		if (payload & 0x80)
> > >>> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
> > >>>  		if (idx > 1) {
> > >>> -			if (payload & 0x100) {
> > >>> -				ret = snprintf(buf, buf_len, " LLC-ACCESS");
> > >>> -				buf += ret;
> > >>> -				blen -= ret;
> > >>> -			}
> > >>> -			if (payload & 0x200) {
> > >>> -				ret = snprintf(buf, buf_len, " LLC-REFILL");
> > >>> -				buf += ret;
> > >>> -				blen -= ret;
> > >>> -			}
> > >>> -			if (payload & 0x400) {
> > >>> -				ret = snprintf(buf, buf_len, " REMOTE-ACCESS");
> > >>> -				buf += ret;
> > >>> -				blen -= ret;
> > >>> -			}
> > >>> +			if (payload & 0x100)
> > >>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
> > >>> +			if (payload & 0x200)
> > >>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
> > >>> +			if (payload & 0x400)
> > >>> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
> > >>>  		}
> > >>> -		if (ret < 0)
> > >>> -			return ret;
> > >>> -		blen -= ret;
> > >>> -		return buf_len - blen;
> > >>
> > >> It looks like we now fall off the bottom of the switch() here.  It's
> > >> preferable to add a break, both to document the intended code flow, and
> > >> to avoid accidents if another case is added later.
> > > 
> > > Will do this.
> > > 
> > >>> -	}
> > >>> +		return err ?: (int)(buf_len - blen);
> > >>
> > >> Nit: unexplained type cast.  Does the result definitely fit into an int?
> > >> If not, why doesn't it matter?
> > > 
> > > The case is to dismiss the compiler warning.
> > > 
> > > GCC doesn't compliant for the code:
> > > 
> > >   return buf_len - blen;
> > >   
> > > But GCC compliants type mismatch after change to code:
> > > 
> > >   return err ?: buf_len - blen;
> 
> So, does buf_len - blen fit in an int?
> 
> > > 
> > >> Also:
> > >>
> > >> If the actual return value is important for the caller to determine the
> > >> number of bytes appended, then it would be better to compute it in one
> > >> place.  Otherwise, it would be better to squash all success returns to 0,
> > >> rather than spend effort computing a value that may be misleading or
> > >> wrong anyway.
> > >>
> > >> In its current form, it's tricky to see whether this patch derives the
> > >> return value consistently for all cases.  In particular, if some code
> > >> patch does one or more arm_spe_pkt_snprintf(), followed by a
> > >> return arm_spe_pkt_snprintf(); then the return value (which only takes
> > >> the last arm_spe_pkt_desc() into account) would be wrong.  It would be
> > >> preferable if the return value for the success case were always
> > >> computed from buf_len and blen, to avoid this risk.  (I'm not saying
> > >> this bug exists, just that it's hard to see from the patch that it
> > >> doesn't exist.)
> > > 
> > > I think I can go back to return value "buf_len - blen" and don't
> > > check "err" anymore.
> 
> Probably, now that blen is adjusted in one place -- at least for success
> returns.
> 
> We still need to indicate failure though, so we may need to check err
> in some places.
> 
> One option is to have snprintf() semantics for the return value of this
> whole function too, so that the caller can detect truncation (if wanted)
> by comparing the return value against buf_len.  We'd need to make sure
> all callers are updated appropriately if going down that route.
> 
> > > 
> > > If the string buffer is truncated due to size limitation, we can still
> > > return "buf_len - blen" so can allow caller to dump string as
> > > possible.
> 
> This really depends on what the caller is trying to do.  In the case of
> truncation, will we send unparseable junk to userspace?  Will userspace
> be able to detect that this has happened and recover gracefully?

I understand your point: if the string buffer is truncated, it's
pointless to output the string with incomplete info.

> > > If arm_spe_pkt_snprintf() returns -1 at the first call, "buf_len" is
> > > equal to "blen", thus return value of "buf_len - blen" will be zero.
> > > For this case, the caller will get to know there have no valid string
> > > and will skip to print anything.
> > > 
> > > Does this make sense?
> 
> Probably, though it's easy for callers to forget about such conventions
> and assume that 0 means "success".

Okay, let me digist your suggestion :)  So the code can be refined with
below fashion:

    /*
     * arm_spe_pkt_snprintf()'s return value has the same
     * semantics with snprintf();
     *
     * *err = 0: no errors;
     * *err != 0: error occurs.
     */
    int arm_spe_pkt_snprintf(int *err, ...)
    {
        va_list ap;
        int ret;

        /* Bail out if any error occurred */
        if (err && *err)
            return *err;

        va_start(ap, fmt);
        ret = vsnprintf(*buf_p, *blen, fmt, ap);
        va_end(ap);

        if (any failure)  {
            if (err && *err)
                *err = ret;
	}

        ...

	return ret;
    }

    int arm_spe_pkt_desc()
    {
	int err = 0;

        switch (packet->type) {
	case ARM_SPE_END:
            arm_spe_pkt_snprintf(&err, ...);
	    break;
	case ARM_SPE_EVENTS:
            arm_spe_pkt_snprintf(&err, ...);
	    break;
	...
	default:
	    err = -1;
	    break;
	}

	return err;
    }

    void arm_spe_dump()
    {
        int ret;

	...

    	ret = arm_spe_pkt_desc(&packet, desc,
                               ARM_SPE_PKT_DESC_MAX););
	if (!ret)
            color_fprintf(stdout, color, " %s\n", desc);

        ...
    }

Thanks a lot for suggestions,
Leo

[PATCH v6 07/21] perf arm-spe: Refactor packet header parsing

[Leo Yan]

The packet header parsing uses the hard coded values and it uses nested
if-else statements.

To improve the readability, this patch refactors the macros for packet
header format so it removes the hard coded values.  Furthermore, based
on the new mask macros it reduces the nested if-else statements and
changes to use the flat conditions checking, this is directive and can
easily map to the descriptions in ARMv8-a architecture reference manual
(ARM DDI 0487E.a), chapter 'D10.1.5 Statistical Profiling Extension
protocol packet headers'.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 92 +++++++++----------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 20 ++++
 2 files changed, 61 insertions(+), 51 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 1ecaf9805b79..30d6c987c287 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -16,28 +16,6 @@
 #define NS_FLAG		BIT(63)
 #define EL_FLAG		(BIT(62) | BIT(61))
 
-#define SPE_HEADER0_PAD			0x0
-#define SPE_HEADER0_END			0x1
-#define SPE_HEADER0_ADDRESS		0x30 /* address packet (short) */
-#define SPE_HEADER0_ADDRESS_MASK	0x38
-#define SPE_HEADER0_COUNTER		0x18 /* counter packet (short) */
-#define SPE_HEADER0_COUNTER_MASK	0x38
-#define SPE_HEADER0_TIMESTAMP		0x71
-#define SPE_HEADER0_TIMESTAMP		0x71
-#define SPE_HEADER0_EVENTS		0x2
-#define SPE_HEADER0_EVENTS_MASK		0xf
-#define SPE_HEADER0_SOURCE		0x3
-#define SPE_HEADER0_SOURCE_MASK		0xf
-#define SPE_HEADER0_CONTEXT		0x24
-#define SPE_HEADER0_CONTEXT_MASK	0x3c
-#define SPE_HEADER0_OP_TYPE		0x8
-#define SPE_HEADER0_OP_TYPE_MASK	0x3c
-#define SPE_HEADER1_ALIGNMENT		0x0
-#define SPE_HEADER1_ADDRESS		0xb0 /* address packet (extended) */
-#define SPE_HEADER1_ADDRESS_MASK	0xf8
-#define SPE_HEADER1_COUNTER		0x98 /* counter packet (extended) */
-#define SPE_HEADER1_COUNTER_MASK	0xf8
-
 #if __BYTE_ORDER == __BIG_ENDIAN
 #define le16_to_cpu bswap_16
 #define le32_to_cpu bswap_32
@@ -200,46 +178,58 @@ static int arm_spe_get_addr(const unsigned char *buf, size_t len,
 static int arm_spe_do_get_packet(const unsigned char *buf, size_t len,
 				 struct arm_spe_pkt *packet)
 {
-	unsigned int byte;
+	unsigned int hdr;
+	unsigned char ext_hdr = 0;
 
 	memset(packet, 0, sizeof(struct arm_spe_pkt));
 
 	if (!len)
 		return ARM_SPE_NEED_MORE_BYTES;
 
-	byte = buf[0];
-	if (byte == SPE_HEADER0_PAD)
+	hdr = buf[0];
+
+	if (hdr == SPE_HEADER0_PAD)
 		return arm_spe_get_pad(packet);
-	else if (byte == SPE_HEADER0_END) /* no timestamp at end of record */
+
+	if (hdr == SPE_HEADER0_END) /* no timestamp at end of record */
 		return arm_spe_get_end(packet);
-	else if (byte & 0xc0 /* 0y11xxxxxx */) {
-		if (byte & 0x80) {
-			if ((byte & SPE_HEADER0_ADDRESS_MASK) == SPE_HEADER0_ADDRESS)
-				return arm_spe_get_addr(buf, len, 0, packet);
-			if ((byte & SPE_HEADER0_COUNTER_MASK) == SPE_HEADER0_COUNTER)
-				return arm_spe_get_counter(buf, len, 0, packet);
-		} else
-			if (byte == SPE_HEADER0_TIMESTAMP)
-				return arm_spe_get_timestamp(buf, len, packet);
-			else if ((byte & SPE_HEADER0_EVENTS_MASK) == SPE_HEADER0_EVENTS)
-				return arm_spe_get_events(buf, len, packet);
-			else if ((byte & SPE_HEADER0_SOURCE_MASK) == SPE_HEADER0_SOURCE)
-				return arm_spe_get_data_source(buf, len, packet);
-			else if ((byte & SPE_HEADER0_CONTEXT_MASK) == SPE_HEADER0_CONTEXT)
-				return arm_spe_get_context(buf, len, packet);
-			else if ((byte & SPE_HEADER0_OP_TYPE_MASK) == SPE_HEADER0_OP_TYPE)
-				return arm_spe_get_op_type(buf, len, packet);
-	} else if ((byte & 0xe0) == 0x20 /* 0y001xxxxx */) {
-		/* 16-bit header */
-		byte = buf[1];
-		if (byte == SPE_HEADER1_ALIGNMENT)
+
+	if (hdr == SPE_HEADER0_TIMESTAMP)
+		return arm_spe_get_timestamp(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK1) == SPE_HEADER0_EVENTS)
+		return arm_spe_get_events(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK1) == SPE_HEADER0_SOURCE)
+		return arm_spe_get_data_source(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_CONTEXT)
+		return arm_spe_get_context(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_OP_TYPE)
+		return arm_spe_get_op_type(buf, len, packet);
+
+	if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_EXTENDED) {
+		/* 16-bit extended format header */
+		ext_hdr = 1;
+
+		hdr = buf[1];
+		if (hdr == SPE_HEADER1_ALIGNMENT)
 			return arm_spe_get_alignment(buf, len, packet);
-		else if ((byte & SPE_HEADER1_ADDRESS_MASK) == SPE_HEADER1_ADDRESS)
-			return arm_spe_get_addr(buf, len, 1, packet);
-		else if ((byte & SPE_HEADER1_COUNTER_MASK) == SPE_HEADER1_COUNTER)
-			return arm_spe_get_counter(buf, len, 1, packet);
 	}
 
+	/*
+	 * The short format header's byte 0 or the extended format header's
+	 * byte 1 has been assigned to 'hdr', which uses the same encoding for
+	 * address packet and counter packet, so don't need to distinguish if
+	 * it's short format or extended format and handle in once.
+	 */
+	if ((hdr & SPE_HEADER0_MASK3) == SPE_HEADER0_ADDRESS)
+		return arm_spe_get_addr(buf, len, ext_hdr, packet);
+
+	if ((hdr & SPE_HEADER0_MASK3) == SPE_HEADER0_COUNTER)
+		return arm_spe_get_counter(buf, len, ext_hdr, packet);
+
 	return ARM_SPE_BAD_PACKET;
 }
 
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
index 4c870521b8eb..129f43405eb1 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
@@ -36,6 +36,26 @@ struct arm_spe_pkt {
 	uint64_t		payload;
 };
 
+/* Short header (HEADER0) and extended header (HEADER1) */
+#define SPE_HEADER0_PAD				0x0
+#define SPE_HEADER0_END				0x1
+#define SPE_HEADER0_TIMESTAMP			0x71
+/* Mask for event & data source */
+#define SPE_HEADER0_MASK1			(GENMASK_ULL(7, 6) | GENMASK_ULL(3, 0))
+#define SPE_HEADER0_EVENTS			0x42
+#define SPE_HEADER0_SOURCE			0x43
+/* Mask for context & operation */
+#define SPE_HEADER0_MASK2			GENMASK_ULL(7, 2)
+#define SPE_HEADER0_CONTEXT			0x64
+#define SPE_HEADER0_OP_TYPE			0x48
+/* Mask for extended format */
+#define SPE_HEADER0_EXTENDED			0x20
+/* Mask for address & counter */
+#define SPE_HEADER0_MASK3			GENMASK_ULL(7, 3)
+#define SPE_HEADER0_ADDRESS			0xb0
+#define SPE_HEADER0_COUNTER			0x98
+#define SPE_HEADER1_ALIGNMENT			0x0
+
 #define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
 #define SPE_ADDR_PKT_HDR_INDEX_BRANCH		(0x1)
 #define SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT	(0x2)
-- 
2.17.1

[PATCH v6 08/21] perf arm-spe: Add new function arm_spe_pkt_desc_addr()

[Leo Yan]

This patch moves out the address parsing code from arm_spe_pkt_desc()
and uses the new introduced function arm_spe_pkt_desc_addr() to process
address packet.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 50 ++++++++++++-------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 30d6c987c287..ae68cd788e33 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -274,10 +274,39 @@ static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
 	return ret;
 }
 
+static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
+				 char *buf, size_t buf_len)
+{
+	int ns, el, idx = packet->index;
+	u64 payload = packet->payload;
+	int err = 0;
+
+	switch (idx) {
+	case 0:
+	case 1:
+		ns = !!(packet->payload & NS_FLAG);
+		el = (packet->payload & EL_FLAG) >> 61;
+		payload &= ~(0xffULL << 56);
+		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
+				"%s 0x%llx el%d ns=%d",
+				(idx == 1) ? "TGT" : "PC", payload, el, ns);
+	case 2:
+		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
+					    "VA 0x%llx", payload);
+	case 3:
+		ns = !!(packet->payload & NS_FLAG);
+		payload &= ~(0xffULL << 56);
+		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
+					    "PA 0x%llx ns=%d", payload, ns);
+	default:
+		return 0;
+	}
+}
+
 int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		     size_t buf_len)
 {
-	int ns, el, idx = packet->index;
+	int idx = packet->index;
 	unsigned long long payload = packet->payload;
 	const char *name = arm_spe_pkt_name(packet->type);
 	size_t blen = buf_len;
@@ -356,24 +385,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	case ARM_SPE_TIMESTAMP:
 		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
 	case ARM_SPE_ADDRESS:
-		switch (idx) {
-		case 0:
-		case 1: ns = !!(packet->payload & NS_FLAG);
-			el = (packet->payload & EL_FLAG) >> 61;
-			payload &= ~(0xffULL << 56);
-			return arm_spe_pkt_snprintf(&err, &buf, &blen,
-					"%s 0x%llx el%d ns=%d",
-				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
-		case 2:
-			return arm_spe_pkt_snprintf(&err, &buf, &blen,
-						    "VA 0x%llx", payload);
-		case 3:	ns = !!(packet->payload & NS_FLAG);
-			payload &= ~(0xffULL << 56);
-			return arm_spe_pkt_snprintf(&err, &buf, &blen,
-						    "PA 0x%llx ns=%d", payload, ns);
-		default:
-			return 0;
-		}
+		return arm_spe_pkt_desc_addr(packet, buf, buf_len);
 	case ARM_SPE_CONTEXT:
 		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
 					    name, (unsigned long)payload, idx + 1);
-- 
2.17.1

[PATCH v6 09/21] perf arm-spe: Refactor address packet handling

[Leo Yan]

This patch is to refactor address packet handling, it defines macros for
address packet's header and payload, these macros are used by decoder
and the dump flow.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../util/arm-spe-decoder/arm-spe-decoder.c    | 29 ++++++++-------
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 26 +++++++-------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 35 ++++++++++++-------
 3 files changed, 48 insertions(+), 42 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
index cc18a1e8c212..776b3e6628bb 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
@@ -24,36 +24,35 @@
 
 static u64 arm_spe_calc_ip(int index, u64 payload)
 {
-	u8 *addr = (u8 *)&payload;
-	int ns, el;
+	u64 ns, el;
 
 	/* Instruction virtual address or Branch target address */
 	if (index == SPE_ADDR_PKT_HDR_INDEX_INS ||
 	    index == SPE_ADDR_PKT_HDR_INDEX_BRANCH) {
-		ns = addr[7] & SPE_ADDR_PKT_NS;
-		el = (addr[7] & SPE_ADDR_PKT_EL_MASK) >> SPE_ADDR_PKT_EL_OFFSET;
+		ns = SPE_ADDR_PKT_GET_NS(payload);
+		el = SPE_ADDR_PKT_GET_EL(payload);
+
+		/* Clean highest byte */
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 
 		/* Fill highest byte for EL1 or EL2 (VHE) mode */
 		if (ns && (el == SPE_ADDR_PKT_EL1 || el == SPE_ADDR_PKT_EL2))
-			addr[7] = 0xff;
-		/* Clean highest byte for other cases */
-		else
-			addr[7] = 0x0;
+			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
 
 	/* Data access virtual address */
 	} else if (index == SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT) {
 
+		/* Clean tags */
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
+
 		/* Fill highest byte if bits [48..55] is 0xff */
-		if (addr[6] == 0xff)
-			addr[7] = 0xff;
-		/* Otherwise, cleanup tags */
-		else
-			addr[7] = 0x0;
+		if (SPE_ADDR_PKT_ADDR_GET_BYTE_6(payload) == 0xffULL)
+			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
 
 	/* Data access physical address */
 	} else if (index == SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS) {
-		/* Cleanup byte 7 */
-		addr[7] = 0x0;
+		/* Clean highest byte */
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 	} else {
 		pr_err("unsupported address packet index: 0x%x\n", index);
 	}
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index ae68cd788e33..e507cd66df01 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -13,9 +13,6 @@
 
 #include "arm-spe-pkt-decoder.h"
 
-#define NS_FLAG		BIT(63)
-#define EL_FLAG		(BIT(62) | BIT(61))
-
 #if __BYTE_ORDER == __BIG_ENDIAN
 #define le16_to_cpu bswap_16
 #define le32_to_cpu bswap_32
@@ -167,10 +164,11 @@ static int arm_spe_get_addr(const unsigned char *buf, size_t len,
 			    const unsigned char ext_hdr, struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_ADDRESS;
+
 	if (ext_hdr)
-		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
+		packet->index = SPE_HDR_EXTENDED_INDEX(buf[0], buf[1]);
 	else
-		packet->index = buf[0] & 0x7;
+		packet->index = SPE_HDR_SHORT_INDEX(buf[0]);
 
 	return arm_spe_get_payload(buf, len, ext_hdr, packet);
 }
@@ -282,20 +280,20 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 	int err = 0;
 
 	switch (idx) {
-	case 0:
-	case 1:
-		ns = !!(packet->payload & NS_FLAG);
-		el = (packet->payload & EL_FLAG) >> 61;
-		payload &= ~(0xffULL << 56);
+	case SPE_ADDR_PKT_HDR_INDEX_INS:
+	case SPE_ADDR_PKT_HDR_INDEX_BRANCH:
+		ns = !!SPE_ADDR_PKT_GET_NS(payload);
+		el = SPE_ADDR_PKT_GET_EL(payload);
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
 				"%s 0x%llx el%d ns=%d",
 				(idx == 1) ? "TGT" : "PC", payload, el, ns);
-	case 2:
+	case SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT:
 		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
 					    "VA 0x%llx", payload);
-	case 3:
-		ns = !!(packet->payload & NS_FLAG);
-		payload &= ~(0xffULL << 56);
+	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
+		ns = !!SPE_ADDR_PKT_GET_NS(payload);
+		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
 					    "PA 0x%llx ns=%d", payload, ns);
 	default:
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
index 129f43405eb1..f97d6840be3a 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
@@ -56,19 +56,28 @@ struct arm_spe_pkt {
 #define SPE_HEADER0_COUNTER			0x98
 #define SPE_HEADER1_ALIGNMENT			0x0
 
-#define SPE_ADDR_PKT_HDR_INDEX_INS		(0x0)
-#define SPE_ADDR_PKT_HDR_INDEX_BRANCH		(0x1)
-#define SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT	(0x2)
-#define SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS	(0x3)
-
-#define SPE_ADDR_PKT_NS				BIT(7)
-#define SPE_ADDR_PKT_CH				BIT(6)
-#define SPE_ADDR_PKT_EL_OFFSET			(5)
-#define SPE_ADDR_PKT_EL_MASK			(0x3 << SPE_ADDR_PKT_EL_OFFSET)
-#define SPE_ADDR_PKT_EL0			(0)
-#define SPE_ADDR_PKT_EL1			(1)
-#define SPE_ADDR_PKT_EL2			(2)
-#define SPE_ADDR_PKT_EL3			(3)
+#define SPE_HDR_SHORT_INDEX(h)			((h) & GENMASK_ULL(2, 0))
+#define SPE_HDR_EXTENDED_INDEX(h0, h1)		(((h0) & GENMASK_ULL(1, 0)) << 3 | \
+						 SPE_HDR_SHORT_INDEX(h1))
+
+/* Address packet header */
+#define SPE_ADDR_PKT_HDR_INDEX_INS		0x0
+#define SPE_ADDR_PKT_HDR_INDEX_BRANCH		0x1
+#define SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT	0x2
+#define SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS	0x3
+
+/* Address packet payload */
+#define SPE_ADDR_PKT_ADDR_BYTE7_SHIFT		56
+#define SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(v)	((v) & GENMASK_ULL(55, 0))
+#define SPE_ADDR_PKT_ADDR_GET_BYTE_6(v)		(((v) & GENMASK_ULL(55, 48)) >> 48)
+
+#define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT_ULL(63)) >> 63)
+#define SPE_ADDR_PKT_GET_EL(v)			(((v) & GENMASK_ULL(62, 61)) >> 61)
+
+#define SPE_ADDR_PKT_EL0			0
+#define SPE_ADDR_PKT_EL1			1
+#define SPE_ADDR_PKT_EL2			2
+#define SPE_ADDR_PKT_EL3			3
 
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
-- 
2.17.1

[PATCH v6 10/21] perf arm_spe: Fixup top byte for data virtual address

[Leo Yan]

To establish a valid address from the address packet payload and finally
the address value can be used for parsing data symbol in DSO, current
code uses 0xff to replace the tag in the top byte of data virtual
address.

So far the code only fixups top byte for the memory layouts with 4KB
pages, it misses to support memory layouts with 64KB pages.

This patch adds the conditions for checking bits [55:52] are 0xf, if
detects the pattern it will fill 0xff into the top byte of the address,
also adds comment to explain the fixing up.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../util/arm-spe-decoder/arm-spe-decoder.c    | 20 ++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
index 776b3e6628bb..cac2ef79c025 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
@@ -24,7 +24,7 @@
 
 static u64 arm_spe_calc_ip(int index, u64 payload)
 {
-	u64 ns, el;
+	u64 ns, el, val;
 
 	/* Instruction virtual address or Branch target address */
 	if (index == SPE_ADDR_PKT_HDR_INDEX_INS ||
@@ -45,8 +45,22 @@ static u64 arm_spe_calc_ip(int index, u64 payload)
 		/* Clean tags */
 		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 
-		/* Fill highest byte if bits [48..55] is 0xff */
-		if (SPE_ADDR_PKT_ADDR_GET_BYTE_6(payload) == 0xffULL)
+		/*
+		 * Armv8 ARM (ARM DDI 0487F.c), chapter "D10.2.1 Address packet"
+		 * defines the data virtual address payload format, the top byte
+		 * (bits [63:56]) is assigned as top-byte tag; so we only can
+		 * retrieve address value from bits [55:0].
+		 *
+		 * According to Documentation/arm64/memory.rst, if detects the
+		 * specific pattern in bits [55:52] of payload which falls in
+		 * the kernel space, should fixup the top byte and this allows
+		 * perf tool to parse DSO symbol for data address correctly.
+		 *
+		 * For this reason, if detects the bits [55:52] is 0xf, will
+		 * fill 0xff into the top byte.
+		 */
+		val = SPE_ADDR_PKT_ADDR_GET_BYTE_6(payload);
+		if ((val & 0xf0ULL) == 0xf0ULL)
 			payload |= 0xffULL << SPE_ADDR_PKT_ADDR_BYTE7_SHIFT;
 
 	/* Data access physical address */
-- 
2.17.1

[PATCH v6 11/21] perf arm-spe: Refactor context packet handling

[Leo Yan]

Minor refactoring to use macro for index mask.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 2 +-
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index e507cd66df01..e4ab43ba80bd 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -136,7 +136,7 @@ static int arm_spe_get_context(const unsigned char *buf, size_t len,
 			       struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_CONTEXT;
-	packet->index = buf[0] & 0x3;
+	packet->index = SPE_CTX_PKT_HDR_INDEX(buf[0]);
 	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
index f97d6840be3a..9bc876bffd35 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
@@ -79,6 +79,9 @@ struct arm_spe_pkt {
 #define SPE_ADDR_PKT_EL2			2
 #define SPE_ADDR_PKT_EL3			3
 
+/* Context packet header */
+#define SPE_CTX_PKT_HDR_INDEX(h)		((h) & GENMASK_ULL(1, 0))
+
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
 int arm_spe_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

[PATCH v6 12/21] perf arm-spe: Add new function arm_spe_pkt_desc_counter()

[Leo Yan]

This patch moves out the counter packet parsing code from
arm_spe_pkt_desc() to the new function arm_spe_pkt_desc_counter().

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 +++++++++++--------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index e4ab43ba80bd..403eb15c13d7 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -301,6 +301,34 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 	}
 }
 
+static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
+				    char *buf, size_t buf_len)
+{
+	u64 payload = packet->payload;
+	const char *name = arm_spe_pkt_name(packet->type);
+	size_t blen = buf_len;
+	int err = 0;
+
+	arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
+			     (unsigned short)payload);
+
+	switch (packet->index) {
+	case 0:
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
+		break;
+	case 1:
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
+		break;
+	case 2:
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
+		break;
+	default:
+		break;
+	}
+
+	return err ?: (int)(buf_len - blen);
+}
+
 int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		     size_t buf_len)
 {
@@ -388,25 +416,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
 					    name, (unsigned long)payload, idx + 1);
 	case ARM_SPE_COUNTER:
-		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
-				     (unsigned short)payload);
-
-		switch (idx) {
-		case 0:
-			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
-			break;
-		case 1:
-			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
-			break;
-		case 2:
-			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
-			break;
-		default:
-			break;
-		}
-
-		return err ?: (int)(buf_len - blen);
-
+		return arm_spe_pkt_desc_counter(packet, buf, buf_len);
 	default:
 		break;
 	}
-- 
2.17.1

[PATCH v6 13/21] perf arm-spe: Refactor counter packet handling

[Leo Yan]

This patch defines macros for counter packet header, and uses macros to
replace hard code values in functions arm_spe_get_counter() and
arm_spe_pkt_desc().

In the function arm_spe_get_counter(), adds a new line for more
readable.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 11 ++++++-----
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h |  5 +++++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 403eb15c13d7..d867d7024480 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -152,10 +152,11 @@ static int arm_spe_get_counter(const unsigned char *buf, size_t len,
 			       const unsigned char ext_hdr, struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_COUNTER;
+
 	if (ext_hdr)
-		packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7);
+		packet->index = SPE_HDR_EXTENDED_INDEX(buf[0], buf[1]);
 	else
-		packet->index = buf[0] & 0x7;
+		packet->index = SPE_HDR_SHORT_INDEX(buf[0]);
 
 	return arm_spe_get_payload(buf, len, ext_hdr, packet);
 }
@@ -313,13 +314,13 @@ static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
 			     (unsigned short)payload);
 
 	switch (packet->index) {
-	case 0:
+	case SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT:
 		arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
 		break;
-	case 1:
+	case SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT:
 		arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
 		break;
-	case 2:
+	case SPE_CNT_PKT_HDR_INDEX_TRANS_LAT:
 		arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
 		break;
 	default:
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
index 9bc876bffd35..7d8e34e35f05 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
@@ -82,6 +82,11 @@ struct arm_spe_pkt {
 /* Context packet header */
 #define SPE_CTX_PKT_HDR_INDEX(h)		((h) & GENMASK_ULL(1, 0))
 
+/* Counter packet header */
+#define SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT		0x0
+#define SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT		0x1
+#define SPE_CNT_PKT_HDR_INDEX_TRANS_LAT		0x2
+
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
 int arm_spe_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

[PATCH v6 14/21] perf arm-spe: Add new function arm_spe_pkt_desc_event()

[Leo Yan]

This patch moves out the event packet parsing from arm_spe_pkt_desc()
to the new function arm_spe_pkt_desc_event().

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 66 +++++++++++--------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index d867d7024480..04ae4b760840 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -273,6 +273,43 @@ static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
 	return ret;
 }
 
+static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
+				  char *buf, size_t buf_len)
+{
+	u64 payload = packet->payload;
+	size_t blen = buf_len;
+	int err = 0;
+
+	arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
+
+	if (payload & 0x1)
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
+	if (payload & 0x2)
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
+	if (payload & 0x4)
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
+	if (payload & 0x8)
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
+	if (payload & 0x10)
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
+	if (payload & 0x20)
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
+	if (payload & 0x40)
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
+	if (payload & 0x80)
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
+	if (packet->index > 1) {
+		if (payload & 0x100)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
+		if (payload & 0x200)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
+		if (payload & 0x400)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
+	}
+
+	return err ?: (int)(buf_len - blen);
+}
+
 static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 				 char *buf, size_t buf_len)
 {
@@ -345,34 +382,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	case ARM_SPE_END:
 		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
 	case ARM_SPE_EVENTS:
-		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
-
-		if (payload & 0x1)
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
-		if (payload & 0x2)
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
-		if (payload & 0x4)
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
-		if (payload & 0x8)
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
-		if (payload & 0x10)
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
-		if (payload & 0x20)
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
-		if (payload & 0x40)
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
-		if (payload & 0x80)
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
-		if (idx > 1) {
-			if (payload & 0x100)
-				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
-			if (payload & 0x200)
-				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
-			if (payload & 0x400)
-				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
-		}
-		return err ?: (int)(buf_len - blen);
-
+		return arm_spe_pkt_desc_event(packet, buf, buf_len);
 	case ARM_SPE_OP_TYPE:
 		switch (idx) {
 		case 0:
-- 
2.17.1

[PATCH v6 15/21] perf arm-spe: Refactor event type handling

[Leo Yan]

Move the enums of event types to arm-spe-pkt-decoder.h, thus function
arm_spe_pkt_desc() can them for bitmasks.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../util/arm-spe-decoder/arm-spe-decoder.h    | 17 --------------
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 22 +++++++++----------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 18 +++++++++++++++
 3 files changed, 29 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
index a5111a8d4360..24727b8ca7ff 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
@@ -13,23 +13,6 @@
 
 #include "arm-spe-pkt-decoder.h"
 
-enum arm_spe_events {
-	EV_EXCEPTION_GEN	= 0,
-	EV_RETIRED		= 1,
-	EV_L1D_ACCESS		= 2,
-	EV_L1D_REFILL		= 3,
-	EV_TLB_ACCESS		= 4,
-	EV_TLB_WALK		= 5,
-	EV_NOT_TAKEN		= 6,
-	EV_MISPRED		= 7,
-	EV_LLC_ACCESS		= 8,
-	EV_LLC_MISS		= 9,
-	EV_REMOTE_ACCESS	= 10,
-	EV_ALIGNMENT		= 11,
-	EV_PARTIAL_PREDICATE	= 17,
-	EV_EMPTY_PREDICATE	= 18,
-};
-
 enum arm_spe_sample_type {
 	ARM_SPE_L1D_ACCESS	= 1 << 0,
 	ARM_SPE_L1D_MISS	= 1 << 1,
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 04ae4b760840..0d57a72859c5 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -282,28 +282,28 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 
 	arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
 
-	if (payload & 0x1)
+	if (payload & BIT(EV_EXCEPTION_GEN))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
-	if (payload & 0x2)
+	if (payload & BIT(EV_RETIRED))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
-	if (payload & 0x4)
+	if (payload & BIT(EV_L1D_ACCESS))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
-	if (payload & 0x8)
+	if (payload & BIT(EV_L1D_REFILL))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
-	if (payload & 0x10)
+	if (payload & BIT(EV_TLB_ACCESS))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
-	if (payload & 0x20)
+	if (payload & BIT(EV_TLB_WALK))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
-	if (payload & 0x40)
+	if (payload & BIT(EV_NOT_TAKEN))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
-	if (payload & 0x80)
+	if (payload & BIT(EV_MISPRED))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
 	if (packet->index > 1) {
-		if (payload & 0x100)
+		if (payload & BIT(EV_LLC_ACCESS))
 			arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
-		if (payload & 0x200)
+		if (payload & BIT(EV_LLC_MISS))
 			arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
-		if (payload & 0x400)
+		if (payload & BIT(EV_REMOTE_ACCESS))
 			arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
 	}
 
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
index 7d8e34e35f05..42ed4e61ede2 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
@@ -87,6 +87,24 @@ struct arm_spe_pkt {
 #define SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT		0x1
 #define SPE_CNT_PKT_HDR_INDEX_TRANS_LAT		0x2
 
+/* Event packet payload */
+enum arm_spe_events {
+	EV_EXCEPTION_GEN	= 0,
+	EV_RETIRED		= 1,
+	EV_L1D_ACCESS		= 2,
+	EV_L1D_REFILL		= 3,
+	EV_TLB_ACCESS		= 4,
+	EV_TLB_WALK		= 5,
+	EV_NOT_TAKEN		= 6,
+	EV_MISPRED		= 7,
+	EV_LLC_ACCESS		= 8,
+	EV_LLC_MISS		= 9,
+	EV_REMOTE_ACCESS	= 10,
+	EV_ALIGNMENT		= 11,
+	EV_PARTIAL_PREDICATE	= 17,
+	EV_EMPTY_PREDICATE	= 18,
+};
+
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
 int arm_spe_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

[PATCH v6 16/21] perf arm-spe: Remove size condition checking for events

[Leo Yan]

In the Armv8 ARM (ARM DDI 0487F.c), chapter "D10.2.6 Events packet", it
describes the event bit is valid with specific payload requirement.  For
example, the Last Level cache access event, the bit is defined as:

  E[8], byte 1 bit [0], when SZ == 0b01 , when SZ == 0b10 ,
  		     or when SZ == 0b11

It requires the payload size is at least 2 bytes, when byte 1 (start
counting from 0) is valid, E[8] (bit 0 in byte 1) can be used for LLC
access event type.  For safety, the code checks the condition for
payload size firstly, if meet the requirement for payload size, then
continue to parse event type.

If review function arm_spe_get_payload(), it has used cast, so any bytes
beyond the valid size have been set to zeros.

For this reason, we don't need to check payload size anymore afterwards
when parse events, thus this patch removes payload size conditions.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/perf/util/arm-spe-decoder/arm-spe-decoder.c  |  9 +++------
 .../util/arm-spe-decoder/arm-spe-pkt-decoder.c     | 14 ++++++--------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
index cac2ef79c025..90d575cee1b9 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
@@ -192,16 +192,13 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
 			if (payload & BIT(EV_TLB_ACCESS))
 				decoder->record.type |= ARM_SPE_TLB_ACCESS;
 
-			if ((idx == 2 || idx == 4 || idx == 8) &&
-			    (payload & BIT(EV_LLC_MISS)))
+			if (payload & BIT(EV_LLC_MISS))
 				decoder->record.type |= ARM_SPE_LLC_MISS;
 
-			if ((idx == 2 || idx == 4 || idx == 8) &&
-			    (payload & BIT(EV_LLC_ACCESS)))
+			if (payload & BIT(EV_LLC_ACCESS))
 				decoder->record.type |= ARM_SPE_LLC_ACCESS;
 
-			if ((idx == 2 || idx == 4 || idx == 8) &&
-			    (payload & BIT(EV_REMOTE_ACCESS)))
+			if (payload & BIT(EV_REMOTE_ACCESS))
 				decoder->record.type |= ARM_SPE_REMOTE_ACCESS;
 
 			if (payload & BIT(EV_MISPRED))
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 0d57a72859c5..07934e1eedca 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -298,14 +298,12 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
 	if (payload & BIT(EV_MISPRED))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
-	if (packet->index > 1) {
-		if (payload & BIT(EV_LLC_ACCESS))
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
-		if (payload & BIT(EV_LLC_MISS))
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
-		if (payload & BIT(EV_REMOTE_ACCESS))
-			arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
-	}
+	if (payload & BIT(EV_LLC_ACCESS))
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
+	if (payload & BIT(EV_LLC_MISS))
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
+	if (payload & BIT(EV_REMOTE_ACCESS))
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
 
 	return err ?: (int)(buf_len - blen);
 }
-- 
2.17.1

[PATCH v6 17/21] perf arm-spe: Add new function arm_spe_pkt_desc_op_type()

[Leo Yan]

The operation type packet is complex and contains subclass; the parsing
flow causes deep indentation; for more readable, this patch introduces
a new function arm_spe_pkt_desc_op_type() which is used for operation
type parsing.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 78 +++++++++++--------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 07934e1eedca..1616a88db9ba 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -308,6 +308,49 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 	return err ?: (int)(buf_len - blen);
 }
 
+static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
+				    char *buf, size_t buf_len)
+{
+	u64 payload = packet->payload;
+	size_t blen = buf_len;
+	int err = 0;
+
+	switch (packet->index) {
+	case 0:
+		return arm_spe_pkt_snprintf(&err, &buf, &blen,
+				payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
+	case 1:
+		arm_spe_pkt_snprintf(&err, &buf, &blen,
+				     payload & 0x1 ? "ST" : "LD");
+
+		if (payload & 0x2) {
+			if (payload & 0x4)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
+			if (payload & 0x8)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
+			if (payload & 0x10)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
+		} else if (payload & 0x4) {
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
+		}
+
+		return err ?: (int)(buf_len - blen);
+
+	case 2:
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
+
+		if (payload & 0x1)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
+		if (payload & 0x2)
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
+
+		return err ?: (int)(buf_len - blen);
+
+	default:
+		return 0;
+	}
+}
+
 static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 				 char *buf, size_t buf_len)
 {
@@ -382,40 +425,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	case ARM_SPE_EVENTS:
 		return arm_spe_pkt_desc_event(packet, buf, buf_len);
 	case ARM_SPE_OP_TYPE:
-		switch (idx) {
-		case 0:
-			return arm_spe_pkt_snprintf(&err, &buf, &blen,
-					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
-		case 1:
-			arm_spe_pkt_snprintf(&err, &buf, &blen,
-					     payload & 0x1 ? "ST" : "LD");
-
-			if (payload & 0x2) {
-				if (payload & 0x4)
-					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
-				if (payload & 0x8)
-					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
-				if (payload & 0x10)
-					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
-			} else if (payload & 0x4) {
-				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
-			}
-
-			return err ?: (int)(buf_len - blen);
-
-		case 2:
-			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
-
-			if (payload & 0x1)
-				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
-			if (payload & 0x2)
-				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
-
-			return err ?: (int)(buf_len - blen);
-
-		default:
-			return 0;
-		}
+		return arm_spe_pkt_desc_op_type(packet, buf, buf_len);
 	case ARM_SPE_DATA_SOURCE:
 	case ARM_SPE_TIMESTAMP:
 		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
-- 
2.17.1

[PATCH v6 18/21] perf arm-spe: Refactor operation packet handling

[Leo Yan]

Defines macros for operation packet header and formats (support sub
classes for 'other', 'branch', 'load and store', etc).  Uses these
macros for operation packet decoding and dumping.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 26 ++++++++++---------
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 23 ++++++++++++++++
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 1616a88db9ba..0e39ba48ea07 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -144,7 +144,7 @@ static int arm_spe_get_op_type(const unsigned char *buf, size_t len,
 			       struct arm_spe_pkt *packet)
 {
 	packet->type = ARM_SPE_OP_TYPE;
-	packet->index = buf[0] & 0x3;
+	packet->index = SPE_OP_PKT_HDR_CLASS(buf[0]);
 	return arm_spe_get_payload(buf, len, 0, packet);
 }
 
@@ -316,32 +316,34 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 	int err = 0;
 
 	switch (packet->index) {
-	case 0:
+	case SPE_OP_PKT_HDR_CLASS_OTHER:
 		return arm_spe_pkt_snprintf(&err, &buf, &blen,
-				payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
-	case 1:
+			payload & SPE_OP_PKT_COND ? "COND-SELECT" : "INSN-OTHER");
+	case SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC:
 		arm_spe_pkt_snprintf(&err, &buf, &blen,
 				     payload & 0x1 ? "ST" : "LD");
 
-		if (payload & 0x2) {
-			if (payload & 0x4)
+		if (SPE_OP_PKT_IS_LDST_ATOMIC(payload)) {
+			if (payload & SPE_OP_PKT_AT)
 				arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
-			if (payload & 0x8)
+			if (payload & SPE_OP_PKT_EXCL)
 				arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
-			if (payload & 0x10)
+			if (payload & SPE_OP_PKT_AR)
 				arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
-		} else if (payload & 0x4) {
+		} else if (SPE_OP_PKT_LDST_SUBCLASS_GET(payload) ==
+					SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP) {
 			arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
 		}
 
 		return err ?: (int)(buf_len - blen);
 
-	case 2:
+	case SPE_OP_PKT_HDR_CLASS_BR_ERET:
 		arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
 
-		if (payload & 0x1)
+		if (payload & SPE_OP_PKT_COND)
 			arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
-		if (payload & 0x2)
+
+		if (SPE_OP_PKT_IS_INDIRECT_BRANCH(payload))
 			arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
 
 		return err ?: (int)(buf_len - blen);
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
index 42ed4e61ede2..7032fc141ad4 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
@@ -105,6 +105,29 @@ enum arm_spe_events {
 	EV_EMPTY_PREDICATE	= 18,
 };
 
+/* Operation packet header */
+#define SPE_OP_PKT_HDR_CLASS(h)			((h) & GENMASK_ULL(1, 0))
+#define SPE_OP_PKT_HDR_CLASS_OTHER		0x0
+#define SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC	0x1
+#define SPE_OP_PKT_HDR_CLASS_BR_ERET		0x2
+
+#define SPE_OP_PKT_COND				BIT(0)
+
+#define SPE_OP_PKT_LDST_SUBCLASS_GET(v)		((v) & GENMASK_ULL(7, 1))
+#define SPE_OP_PKT_LDST_SUBCLASS_GP_REG		0x0
+#define SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP	0x4
+#define SPE_OP_PKT_LDST_SUBCLASS_UNSPEC_REG	0x10
+#define SPE_OP_PKT_LDST_SUBCLASS_NV_SYSREG	0x30
+
+#define SPE_OP_PKT_IS_LDST_ATOMIC(v)		(((v) & (GENMASK_ULL(7, 5) | BIT(1))) == 0x2)
+
+#define SPE_OP_PKT_AR				BIT(4)
+#define SPE_OP_PKT_EXCL				BIT(3)
+#define SPE_OP_PKT_AT				BIT(2)
+#define SPE_OP_PKT_ST				BIT(0)
+
+#define SPE_OP_PKT_IS_INDIRECT_BRANCH(v)	(((v) & GENMASK_ULL(7, 1)) == 0x2)
+
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
 
 int arm_spe_get_packet(const unsigned char *buf, size_t len,
-- 
2.17.1

[PATCH v6 19/21] perf arm-spe: Add more sub classes for operation packet

[Leo Yan]

For the operation type packet payload with load/store class, it misses
to support these sub classes:

  - A load/store targeting the general-purpose registers;
  - A load/store targeting unspecified registers;
  - The ARMv8.4 nested virtualisation extension can redirect system
    register accesses to a memory page controlled by the hypervisor.
    The SPE profiling feature in newer implementations can tag those
    memory accesses accordingly.

Add the bit pattern describing load/store sub classes, so that the perf
tool can decode it properly.

Inspired by Andre Przywara, refined the commit log and code for more
clear description.

Co-developed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../util/arm-spe-decoder/arm-spe-pkt-decoder.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 0e39ba48ea07..3fca65e9cbbf 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -330,9 +330,23 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 				arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
 			if (payload & SPE_OP_PKT_AR)
 				arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
-		} else if (SPE_OP_PKT_LDST_SUBCLASS_GET(payload) ==
-					SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP) {
+		}
+
+		switch (SPE_OP_PKT_LDST_SUBCLASS_GET(payload)) {
+		case SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP:
 			arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
+			break;
+		case SPE_OP_PKT_LDST_SUBCLASS_GP_REG:
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " GP-REG");
+			break;
+		case SPE_OP_PKT_LDST_SUBCLASS_UNSPEC_REG:
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " UNSPEC-REG");
+			break;
+		case SPE_OP_PKT_LDST_SUBCLASS_NV_SYSREG:
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " NV-SYSREG");
+			break;
+		default:
+			break;
 		}
 
 		return err ?: (int)(buf_len - blen);
-- 
2.17.1

[PATCH v6 20/21] perf arm_spe: Decode memory tagging properties

[Leo Yan]

From: Andre Przywara <andre.przywara@arm.com>

When SPE records a physical address, it can additionally tag the event
with information from the Memory Tagging architecture extension.

Decode the two additional fields in the SPE event payload.

[leoy: Refined patch to use predefined macros]

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 +++++-
 tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 3fca65e9cbbf..9ec3057de86f 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -371,6 +371,7 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 				 char *buf, size_t buf_len)
 {
 	int ns, el, idx = packet->index;
+	int ch, pat;
 	u64 payload = packet->payload;
 	int err = 0;
 
@@ -388,9 +389,12 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
 					    "VA 0x%llx", payload);
 	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
 		ns = !!SPE_ADDR_PKT_GET_NS(payload);
+		ch = !!SPE_ADDR_PKT_GET_CH(payload);
+		pat = SPE_ADDR_PKT_GET_PAT(payload);
 		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
 		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
-					    "PA 0x%llx ns=%d", payload, ns);
+					    "PA 0x%llx ns=%d ch=%d, pat=%x",
+					    payload, ns, ch, pat);
 	default:
 		return 0;
 	}
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
index 7032fc141ad4..1ad14885c2a1 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
@@ -73,6 +73,8 @@ struct arm_spe_pkt {
 
 #define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT_ULL(63)) >> 63)
 #define SPE_ADDR_PKT_GET_EL(v)			(((v) & GENMASK_ULL(62, 61)) >> 61)
+#define SPE_ADDR_PKT_GET_CH(v)			(((v) & BIT_ULL(62)) >> 62)
+#define SPE_ADDR_PKT_GET_PAT(v)			(((v) & GENMASK_ULL(59, 56)) >> 56)
 
 #define SPE_ADDR_PKT_EL0			0
 #define SPE_ADDR_PKT_EL1			1
-- 
2.17.1

Re: [PATCH v6 20/21] perf arm_spe: Decode memory tagging properties

[Dave Martin]

On Fri, Oct 30, 2020 at 02:57:23AM +0000, Leo Yan wrote:
> From: Andre Przywara <andre.przywara@arm.com>
> 
> When SPE records a physical address, it can additionally tag the event
> with information from the Memory Tagging architecture extension.
> 
> Decode the two additional fields in the SPE event payload.
> 
> [leoy: Refined patch to use predefined macros]
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 +++++-
>  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index 3fca65e9cbbf..9ec3057de86f 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -371,6 +371,7 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  				 char *buf, size_t buf_len)
>  {
>  	int ns, el, idx = packet->index;
> +	int ch, pat;
>  	u64 payload = packet->payload;
>  	int err = 0;
>  
> @@ -388,9 +389,12 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
>  					    "VA 0x%llx", payload);
>  	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
>  		ns = !!SPE_ADDR_PKT_GET_NS(payload);
> +		ch = !!SPE_ADDR_PKT_GET_CH(payload);
> +		pat = SPE_ADDR_PKT_GET_PAT(payload);
>  		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
>  		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
> -					    "PA 0x%llx ns=%d", payload, ns);
> +					    "PA 0x%llx ns=%d ch=%d, pat=%x",

Nit: given that this data is all closely related, do we really want the
extra comma here?

(Note, I am not familiar with how this text is consumed, so if there are
other reasons why the comma is needed then that's probably fine.)

> +					    payload, ns, ch, pat);
>  	default:
>  		return 0;
>  	}
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> index 7032fc141ad4..1ad14885c2a1 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> @@ -73,6 +73,8 @@ struct arm_spe_pkt {
>  
>  #define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT_ULL(63)) >> 63)
>  #define SPE_ADDR_PKT_GET_EL(v)			(((v) & GENMASK_ULL(62, 61)) >> 61)
> +#define SPE_ADDR_PKT_GET_CH(v)			(((v) & BIT_ULL(62)) >> 62)
> +#define SPE_ADDR_PKT_GET_PAT(v)			(((v) & GENMASK_ULL(59, 56)) >> 56)

These seem to match the spec.

With or without addressing the nit above:

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

[...]

Cheers
---Dave

Re: [PATCH v6 20/21] perf arm_spe: Decode memory tagging properties

[Leo Yan]

On Mon, Nov 02, 2020 at 04:25:36PM +0000, Dave Martin wrote:
> On Fri, Oct 30, 2020 at 02:57:23AM +0000, Leo Yan wrote:
> > From: Andre Przywara <andre.przywara@arm.com>
> > 
> > When SPE records a physical address, it can additionally tag the event
> > with information from the Memory Tagging architecture extension.
> > 
> > Decode the two additional fields in the SPE event payload.
> > 
> > [leoy: Refined patch to use predefined macros]
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 +++++-
> >  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 2 ++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > index 3fca65e9cbbf..9ec3057de86f 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > @@ -371,6 +371,7 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
> >  				 char *buf, size_t buf_len)
> >  {
> >  	int ns, el, idx = packet->index;
> > +	int ch, pat;
> >  	u64 payload = packet->payload;
> >  	int err = 0;
> >  
> > @@ -388,9 +389,12 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
> >  					    "VA 0x%llx", payload);
> >  	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
> >  		ns = !!SPE_ADDR_PKT_GET_NS(payload);
> > +		ch = !!SPE_ADDR_PKT_GET_CH(payload);
> > +		pat = SPE_ADDR_PKT_GET_PAT(payload);
> >  		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
> >  		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
> > -					    "PA 0x%llx ns=%d", payload, ns);
> > +					    "PA 0x%llx ns=%d ch=%d, pat=%x",
> 
> Nit: given that this data is all closely related, do we really want the
> extra comma here?

No reason for adding comma.  Will remove it.

> (Note, I am not familiar with how this text is consumed, so if there are
> other reasons why the comma is needed then that's probably fine.)
> 
> > +					    payload, ns, ch, pat);
> >  	default:
> >  		return 0;
> >  	}
> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> > index 7032fc141ad4..1ad14885c2a1 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> > @@ -73,6 +73,8 @@ struct arm_spe_pkt {
> >  
> >  #define SPE_ADDR_PKT_GET_NS(v)			(((v) & BIT_ULL(63)) >> 63)
> >  #define SPE_ADDR_PKT_GET_EL(v)			(((v) & GENMASK_ULL(62, 61)) >> 61)
> > +#define SPE_ADDR_PKT_GET_CH(v)			(((v) & BIT_ULL(62)) >> 62)
> > +#define SPE_ADDR_PKT_GET_PAT(v)			(((v) & GENMASK_ULL(59, 56)) >> 56)
> 
> These seem to match the spec.
> 
> With or without addressing the nit above:
> 
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>

Thanks for reviewing.

> [...]
> 
> Cheers
> ---Dave

Re: [PATCH v6 20/21] perf arm_spe: Decode memory tagging properties

[Dave Martin]

On Tue, Nov 03, 2020 at 06:51:01AM +0000, Leo Yan wrote:
> On Mon, Nov 02, 2020 at 04:25:36PM +0000, Dave Martin wrote:
> > On Fri, Oct 30, 2020 at 02:57:23AM +0000, Leo Yan wrote:
> > > From: Andre Przywara <andre.przywara@arm.com>
> > > 
> > > When SPE records a physical address, it can additionally tag the event
> > > with information from the Memory Tagging architecture extension.
> > > 
> > > Decode the two additional fields in the SPE event payload.
> > > 
> > > [leoy: Refined patch to use predefined macros]
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c | 6 +++++-
> > >  tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h | 2 ++
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > index 3fca65e9cbbf..9ec3057de86f 100644
> > > --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > @@ -371,6 +371,7 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
> > >  				 char *buf, size_t buf_len)
> > >  {
> > >  	int ns, el, idx = packet->index;
> > > +	int ch, pat;
> > >  	u64 payload = packet->payload;
> > >  	int err = 0;
> > >  
> > > @@ -388,9 +389,12 @@ static int arm_spe_pkt_desc_addr(const struct arm_spe_pkt *packet,
> > >  					    "VA 0x%llx", payload);
> > >  	case SPE_ADDR_PKT_HDR_INDEX_DATA_PHYS:
> > >  		ns = !!SPE_ADDR_PKT_GET_NS(payload);
> > > +		ch = !!SPE_ADDR_PKT_GET_CH(payload);
> > > +		pat = SPE_ADDR_PKT_GET_PAT(payload);
> > >  		payload = SPE_ADDR_PKT_ADDR_GET_BYTES_0_6(payload);
> > >  		return arm_spe_pkt_snprintf(&err, &buf, &buf_len,
> > > -					    "PA 0x%llx ns=%d", payload, ns);
> > > +					    "PA 0x%llx ns=%d ch=%d, pat=%x",
> > 
> > Nit: given that this data is all closely related, do we really want the
> > extra comma here?
> 
> No reason for adding comma.  Will remove it.

OK, I'm happy for my Reviewed-by to stand.

[...]

Cheers
---Dave

[PATCH v6 21/21] perf arm-spe: Add support for ARMv8.3-SPE

[Leo Yan]

From: Wei Li <liwei391@huawei.com>

This patch is to support Armv8.3 extension for SPE, it adds alignment
field in the Events packet and it supports the Scalable Vector Extension
(SVE) for Operation packet and Events packet with two additions:

  - The vector length for SVE operations in the Operation Type packet;
  - The incomplete predicate and empty predicate fields in the Events
    packet.

Signed-off-by: Wei Li <liwei391@huawei.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 39 ++++++++++++++++++-
 .../arm-spe-decoder/arm-spe-pkt-decoder.h     | 16 ++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 9ec3057de86f..52c4990885ae 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -304,6 +304,12 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
 	if (payload & BIT(EV_REMOTE_ACCESS))
 		arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
+	if (payload & BIT(EV_ALIGNMENT))
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " ALIGNMENT");
+	if (payload & BIT(EV_PARTIAL_PREDICATE))
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " SVE-PARTIAL-PRED");
+	if (payload & BIT(EV_EMPTY_PREDICATE))
+		arm_spe_pkt_snprintf(&err, &buf, &blen, " SVE-EMPTY-PRED");
 
 	return err ?: (int)(buf_len - blen);
 }
@@ -317,8 +323,26 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 
 	switch (packet->index) {
 	case SPE_OP_PKT_HDR_CLASS_OTHER:
-		return arm_spe_pkt_snprintf(&err, &buf, &blen,
-			payload & SPE_OP_PKT_COND ? "COND-SELECT" : "INSN-OTHER");
+		if (SPE_OP_PKT_IS_OTHER_SVE_OP(payload)) {
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "SVE-OTHER");
+
+			/* SVE effective vector length */
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " EVLEN %d",
+					     SPE_OP_PKG_SVE_EVL(payload));
+
+			if (payload & SPE_OP_PKT_SVE_FP)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " FP");
+			if (payload & SPE_OP_PKT_SVE_PRED)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " PRED");
+		} else {
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "OTHER");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " %s",
+					     payload & SPE_OP_PKT_COND ?
+					     "COND-SELECT" : "INSN-OTHER");
+		}
+
+		return err ?: (int)(buf_len - blen);
+
 	case SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC:
 		arm_spe_pkt_snprintf(&err, &buf, &blen,
 				     payload & 0x1 ? "ST" : "LD");
@@ -349,6 +373,17 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
 			break;
 		}
 
+		if (SPE_OP_PKT_IS_LDST_SVE(payload)) {
+			/* SVE effective vector length */
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " EVLEN %d",
+					     SPE_OP_PKG_SVE_EVL(payload));
+
+			if (payload & SPE_OP_PKT_SVE_PRED)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " PRED");
+			if (payload & SPE_OP_PKT_SVE_SG)
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " SG");
+		}
+
 		return err ?: (int)(buf_len - blen);
 
 	case SPE_OP_PKT_HDR_CLASS_BR_ERET:
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
index 1ad14885c2a1..9b970e7bf1e2 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
@@ -113,6 +113,8 @@ enum arm_spe_events {
 #define SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC	0x1
 #define SPE_OP_PKT_HDR_CLASS_BR_ERET		0x2
 
+#define SPE_OP_PKT_IS_OTHER_SVE_OP(v)		(((v) & (BIT(7) | BIT(3) | BIT(0))) == 0x8)
+
 #define SPE_OP_PKT_COND				BIT(0)
 
 #define SPE_OP_PKT_LDST_SUBCLASS_GET(v)		((v) & GENMASK_ULL(7, 1))
@@ -128,6 +130,20 @@ enum arm_spe_events {
 #define SPE_OP_PKT_AT				BIT(2)
 #define SPE_OP_PKT_ST				BIT(0)
 
+#define SPE_OP_PKT_IS_LDST_SVE(v)		(((v) & (BIT(3) | BIT(1))) == 0x8)
+
+#define SPE_OP_PKT_SVE_SG			BIT(7)
+/*
+ * SVE effective vector length (EVL) is stored in byte 0 bits [6:4];
+ * the length is rounded up to a power of two and use 32 as one step,
+ * so EVL calculation is:
+ *
+ *   32 * (2 ^ bits [6:4]) = 32 << (bits [6:4])
+ */
+#define SPE_OP_PKG_SVE_EVL(v)			(32 << (((v) & GENMASK_ULL(6, 4)) >> 4))
+#define SPE_OP_PKT_SVE_PRED			BIT(2)
+#define SPE_OP_PKT_SVE_FP			BIT(1)
+
 #define SPE_OP_PKT_IS_INDIRECT_BRANCH(v)	(((v) & GENMASK_ULL(7, 1)) == 0x2)
 
 const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
-- 
2.17.1