On 29/09/2020 14:39, Leo Yan wrote:
Hi Leo,
> 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'.
Yeah, that's so much better, thank you!
I checked all the bits and comparisons against the ARM ARM.
Two minor things below ...
>
> Signed-off-by: Leo Yan <leo....@linaro.org>
> ---
> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 92 +++++++++----------
> .../arm-spe-decoder/arm-spe-pkt-decoder.h | 21 +++++
> 2 files changed, 62 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 96b717a19163..e738bd04f209 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
> @@ -198,46 +176,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_OPERATION)
> + return arm_spe_get_op_type(buf, len, packet);
> +
> + if ((hdr & SPE_HEADER0_MASK3) == SPE_HEADER0_EXTENDED) {
Is there any reason you are using MASK3 here, and not MASK2? The ARM ARM
seems to suggest that bits [7:2] make up the mask for the extended
header type, as the actual subtype is handled in the next byte.
> + /* 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_MASK4) == SPE_HEADER0_ADDRESS)
> + return arm_spe_get_addr(buf, len, ext_hdr, packet);
> +
> + if ((hdr & SPE_HEADER0_MASK4) == 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 f2d0af39a58c..a30fe3c5ab67 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
> @@ -37,6 +37,27 @@ 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_OPERATION 0x48
Just a nit, but should the name be ..._OP_TYPE instead?
Cheers,
Andre
> +/* Mask for extended format */
> +#define SPE_HEADER0_MASK3 GENMASK_ULL(7, 5)
> +#define SPE_HEADER0_EXTENDED 0x20
> +/* Mask for address & counter */
> +#define SPE_HEADER0_MASK4 GENMASK_ULL(7, 3)
> +#define SPE_HEADER0_ADDRESS 0xb0
> +#define SPE_HEADER0_COUNTER 0x98
> +#define SPE_HEADER1_ALIGNMENT 0x0
> +
> #define SPE_HEADER_SZ_SHIFT (4)
> #define SPE_HEADER_SZ_MASK GENMASK_ULL(5, 4)
>
>
