Re: [PATCH v3 07/20] perf arm-spe: Refactor packet header parsing

From: André Przywara
Date: Fri Oct 23 2020 - 13:10:32 EST

Next message: André Przywara: "Re: [PATCH v3 08/20] perf arm-spe: Add new function arm_spe_pkt_desc_addr()"
Previous message: André Przywara: "Re: [PATCH v3 04/20] perf arm-spe: Refactor arm_spe_get_events()"
In reply to: Leo Yan: "[PATCH v3 07/20] perf arm-spe: Refactor packet header parsing"
Next in thread: Leo Yan: "[PATCH v3 06/20] perf arm-spe: Refactor printing string to buffer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Hi,

> 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@xxxxxxxxxx>

Compared against the previous version, which I had checked already
against the manual. Thanks for the fixes!

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

Thanks,
Andre

> ---
> .../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 c7b6dc016f11..6f2329990729 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 e9ea8e3ead5d..68552ff8a8f7 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,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_HEADER_SZ(val) ((val & GENMASK_ULL(5, 4)) >> 4)
>
> #define SPE_ADDR_PKT_HDR_INDEX_INS (0x0)
>

Next message: André Przywara: "Re: [PATCH v3 08/20] perf arm-spe: Add new function arm_spe_pkt_desc_addr()"
Previous message: André Przywara: "Re: [PATCH v3 04/20] perf arm-spe: Refactor arm_spe_get_events()"
In reply to: Leo Yan: "[PATCH v3 07/20] perf arm-spe: Refactor packet header parsing"
Next in thread: Leo Yan: "[PATCH v3 06/20] perf arm-spe: Refactor printing string to buffer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]