2020-10-22 17:51:13

by Leo Yan

[permalink] [raw]
Subject: [PATCH v3 17/20] perf arm-spe: Refactor operation packet handling

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 <[email protected]>
---
.../arm-spe-decoder/arm-spe-pkt-decoder.c | 34 +++++++++++--------
.../arm-spe-decoder/arm-spe-pkt-decoder.h | 26 ++++++++++++++
2 files changed, 45 insertions(+), 15 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 19d05d9734ab..59b538563d31 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);
}

@@ -339,37 +339,39 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
char *buf, size_t buf_len)
{
- int ret, idx = packet->index;
+ int ret, class = packet->index;
unsigned long long payload = packet->payload;
size_t blen = buf_len;

- switch (idx) {
- case 0:
+ switch (class) {
+ case SPE_OP_PKT_HDR_CLASS_OTHER:
return arm_spe_pkt_snprintf(&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:
ret = arm_spe_pkt_snprintf(&buf, &blen,
- payload & 0x1 ? "ST" : "LD");
+ payload & SPE_OP_PKT_ST ? "ST" : "LD");
if (ret < 0)
return ret;

- if (payload & 0x2) {
- if (payload & 0x4) {
+ if (SPE_OP_PKT_LDST_SUBCLASS_ATOMIC_GET(payload) ==
+ SPE_OP_PKT_LDST_SUBCLASS_ATOMIC) {
+ if (payload & SPE_OP_PKT_AT) {
ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
if (ret < 0)
return ret;
}
- if (payload & 0x8) {
+ if (payload & SPE_OP_PKT_EXCL) {
ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
if (ret < 0)
return ret;
}
- if (payload & 0x10) {
+ if (payload & SPE_OP_PKT_AR) {
ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
if (ret < 0)
return ret;
}
- } else if (payload & 0x4) {
+ } else if (SPE_OP_PKT_LDST_SUBCLASS_GET(payload) ==
+ SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP) {
ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
if (ret < 0)
return ret;
@@ -377,17 +379,19 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,

return buf_len - blen;

- case 2:
+ case SPE_OP_PKT_HDR_CLASS_BR_ERET:
ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
if (ret < 0)
return ret;

- if (payload & 0x1) {
+ if (payload & SPE_OP_PKT_COND) {
ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
if (ret < 0)
return ret;
}
- if (payload & 0x2) {
+
+ if (SPE_OP_PKT_BRANCH_SUBCLASS_GET(payload) ==
+ SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT) {
ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
if (ret < 0)
return ret;
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 12c344454cf1..31dbb8c0fde3 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
@@ -110,6 +110,32 @@ 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_LDST_SUBCLASS_ATOMIC_GET(v) ((v) & (GENMASK_ULL(7, 5) | GENMASK_ULL(1, 1)))
+#define SPE_OP_PKT_LDST_SUBCLASS_ATOMIC 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_BRANCH_SUBCLASS_GET(v) ((v) & GENMASK_ULL(7, 1))
+#define SPE_OP_PKT_BRANCH_SUBCLASS_DIRECT 0x0
+#define SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT 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


2020-10-27 03:33:26

by Andre Przywara

[permalink] [raw]
Subject: Re: [PATCH v3 17/20] perf arm-spe: Refactor operation packet handling

On 22/10/2020 15:58, Leo Yan wrote:

Hi,

> 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 <[email protected]>
> ---
> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 34 +++++++++++--------
> .../arm-spe-decoder/arm-spe-pkt-decoder.h | 26 ++++++++++++++
> 2 files changed, 45 insertions(+), 15 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 19d05d9734ab..59b538563d31 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);
> }
>
> @@ -339,37 +339,39 @@ static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
> static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
> char *buf, size_t buf_len)
> {
> - int ret, idx = packet->index;
> + int ret, class = packet->index;
> unsigned long long payload = packet->payload;
> size_t blen = buf_len;
>
> - switch (idx) {
> - case 0:
> + switch (class) {
> + case SPE_OP_PKT_HDR_CLASS_OTHER:
> return arm_spe_pkt_snprintf(&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:
> ret = arm_spe_pkt_snprintf(&buf, &blen,
> - payload & 0x1 ? "ST" : "LD");
> + payload & SPE_OP_PKT_ST ? "ST" : "LD");
> if (ret < 0)
> return ret;
>
> - if (payload & 0x2) {
> - if (payload & 0x4) {
> + if (SPE_OP_PKT_LDST_SUBCLASS_ATOMIC_GET(payload) ==
> + SPE_OP_PKT_LDST_SUBCLASS_ATOMIC) {

This looks somewhat hard to read, and those symbols are only used once?
So what about combining this down in the header so that you can use:
if (SPE_OP_PKT_IS_LDST_ATOMIC(payload)) {

> + if (payload & SPE_OP_PKT_AT) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " AT");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x8) {
> + if (payload & SPE_OP_PKT_EXCL) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCL");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x10) {
> + if (payload & SPE_OP_PKT_AR) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " AR");
> if (ret < 0)
> return ret;
> }
> - } else if (payload & 0x4) {
> + } else if (SPE_OP_PKT_LDST_SUBCLASS_GET(payload) ==
> + SPE_OP_PKT_LDST_SUBCLASS_SIMD_FP) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " SIMD-FP");
> if (ret < 0)
> return ret;
> @@ -377,17 +379,19 @@ static int arm_spe_pkt_desc_op_type(const struct arm_spe_pkt *packet,
>
> return buf_len - blen;
>
> - case 2:
> + case SPE_OP_PKT_HDR_CLASS_BR_ERET:
> ret = arm_spe_pkt_snprintf(&buf, &blen, "B");
> if (ret < 0)
> return ret;
>
> - if (payload & 0x1) {
> + if (payload & SPE_OP_PKT_COND) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " COND");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x2) {
> +
> + if (SPE_OP_PKT_BRANCH_SUBCLASS_GET(payload) ==
> + SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT) {

Same here, it's the only user of both symbols, so maybe:
if (SPE_OP_PKT_IS_INDIRECT_BRANCH(payload)) {

Cheers,
Andre

> ret = arm_spe_pkt_snprintf(&buf, &blen, " IND");
> if (ret < 0)
> return ret;
> 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 12c344454cf1..31dbb8c0fde3 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
> @@ -110,6 +110,32 @@ 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_LDST_SUBCLASS_ATOMIC_GET(v) ((v) & (GENMASK_ULL(7, 5) | GENMASK_ULL(1, 1)))
> +#define SPE_OP_PKT_LDST_SUBCLASS_ATOMIC 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_BRANCH_SUBCLASS_GET(v) ((v) & GENMASK_ULL(7, 1))
> +#define SPE_OP_PKT_BRANCH_SUBCLASS_DIRECT 0x0
> +#define SPE_OP_PKT_BRANCH_SUBCLASS_INDIRECT 0x2
> +
> const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
>
> int arm_spe_get_packet(const unsigned char *buf, size_t len,
>