Re: [PATCH v3 03/20] perf arm-spe: Refactor payload size calculation

["André Przywara" <andre.przywara@arm.com>]

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

Hi Leo,

> 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>
> ---
>  .../util/arm-spe-decoder/arm-spe-pkt-decoder.c | 18 +++++++++---------
>  .../util/arm-spe-decoder/arm-spe-pkt-decoder.h |  3 +++
>  2 files changed, 12 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..4294c133a465 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 1 << SPE_HEADER_SZ(hdr);

I know, I know, I asked for this, but now looking again at it - and
after having seen the whole series:
This is now really trivial, and there are just two users? And
SPE_HEADER_SZ() is only used in here?

So either you just stuff the "1U << .." into the callers of
arm_spe_payload_len(), or indeed put all of this into one macro (as you
had originally).

Apologies for this forth and back, but I didn't realise how this is
really used eventually, and I just saw the transition from function to
macro.

But please use 1U << .., signed shifts are treacherous.

>  }
>  
>  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;
> 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..e9ea8e3ead5d 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
> @@ -9,6 +9,7 @@
>  
>  #include <stddef.h>
>  #include <stdint.h>
> +#include <linux/bits.h>
>  
>  #define ARM_SPE_PKT_DESC_MAX		256
>  
> @@ -36,6 +37,8 @@ struct arm_spe_pkt {
>  	uint64_t		payload;
>  };
>  
> +#define SPE_HEADER_SZ(val)			((val & GENMASK_ULL(5, 4)) >> 4)

If you should keep this, please put parentheses around "val".

Cheers,
Andre

> +
>  #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)
>