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

[André Przywara]

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 <leo.yan@xxxxxxxxxx>
> ---
>  .../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,
>