From: "André Przywara" <andre.przywara@arm.com>
To: Leo Yan <leo.yan@linaro.org>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
Wei Li <liwei391@huawei.com>, James Clark <james.clark@arm.com>,
Dave Martin <Dave.Martin@arm.com>,
linux-kernel@vger.kernel.org, Al Grant <Al.Grant@arm.com>
Subject: Re: [PATCH v2 06/14] perf arm-spe: Refactor packet header parsing
Date: Thu, 8 Oct 2020 20:49:11 +0100 [thread overview]
Message-ID: <d3d0bf64-0285-1aef-3ec1-74f0d28257a6@arm.com> (raw)
In-Reply-To: <20200929133917.9224-7-leo.yan@linaro.org>
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.yan@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)
>
>
next prev parent reply other threads:[~2020-10-08 19:50 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-29 13:39 [PATCH v2 00/14] perf arm-spe: Refactor decoding & dumping flow Leo Yan
2020-09-29 13:39 ` [PATCH v2 01/14] perf arm-spe: Include bitops.h for BIT() macro Leo Yan
2020-10-08 13:44 ` André Przywara
2020-09-29 13:39 ` [PATCH v2 02/14] perf arm-spe: Fix a typo in comment Leo Yan
2020-10-08 13:44 ` André Przywara
2020-09-29 13:39 ` [PATCH v2 03/14] perf arm-spe: Refactor payload length calculation Leo Yan
2020-10-08 13:44 ` André Przywara
2020-10-12 0:21 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 04/14] perf arm-spe: Fix packet length handling Leo Yan
2020-10-08 13:45 ` André Przywara
2020-09-29 13:39 ` [PATCH v2 05/14] perf arm-spe: Refactor printing string to buffer Leo Yan
2020-10-08 13:46 ` André Przywara
2020-10-12 0:29 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 06/14] perf arm-spe: Refactor packet header parsing Leo Yan
2020-10-08 19:49 ` André Przywara [this message]
2020-10-12 1:00 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 07/14] perf arm-spe: Refactor address packet handling Leo Yan
2020-10-19 9:01 ` André Przywara
2020-10-19 10:41 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 08/14] perf arm-spe: Refactor context " Leo Yan
2020-10-20 21:53 ` André Przywara
2020-09-29 13:39 ` [PATCH v2 09/14] perf arm-spe: Refactor counter " Leo Yan
2020-10-20 21:53 ` André Przywara
2020-10-21 3:52 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 10/14] perf arm-spe: Refactor event type handling Leo Yan
2020-10-20 21:54 ` André Przywara
2020-10-21 4:54 ` Leo Yan
2020-10-21 9:20 ` André Przywara
2020-10-21 10:13 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 11/14] perf arm-spe: Refactor operation packet handling Leo Yan
2020-09-29 13:39 ` [PATCH v2 12/14] perf arm-spe: Add more sub classes for operation packet Leo Yan
2020-10-20 21:54 ` André Przywara
2020-10-21 5:16 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 13/14] perf arm_spe: Decode memory tagging properties Leo Yan
2020-09-29 13:39 ` [PATCH v2 14/14] perf arm-spe: Add support for ARMv8.3-SPE Leo Yan
2020-10-20 21:54 ` André Przywara
2020-10-21 5:10 ` Leo Yan
2020-10-21 9:26 ` André Przywara
2020-10-21 10:17 ` Leo Yan
2020-10-21 14:53 ` André Przywara
2020-10-22 0:44 ` Leo Yan
2020-10-13 14:53 ` [PATCH v2 00/14] perf arm-spe: Refactor decoding & dumping flow Arnaldo Carvalho de Melo
2020-10-13 15:19 ` Leo Yan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d3d0bf64-0285-1aef-3ec1-74f0d28257a6@arm.com \
--to=andre.przywara@arm.com \
--cc=Al.Grant@arm.com \
--cc=Dave.Martin@arm.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=james.clark@arm.com \
--cc=jolsa@redhat.com \
--cc=leo.yan@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=liwei391@huawei.com \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).