From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 84924C433DF for ; Thu, 8 Oct 2020 19:50:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2544722202 for ; Thu, 8 Oct 2020 19:50:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729257AbgJHTuG (ORCPT ); Thu, 8 Oct 2020 15:50:06 -0400 Received: from foss.arm.com ([217.140.110.172]:47110 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729220AbgJHTuF (ORCPT ); Thu, 8 Oct 2020 15:50:05 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E9FCA1063; Thu, 8 Oct 2020 12:50:03 -0700 (PDT) Received: from [192.168.2.22] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 073FC3F66B; Thu, 8 Oct 2020 12:50:01 -0700 (PDT) Subject: Re: [PATCH v2 06/14] perf arm-spe: Refactor packet header parsing To: Leo Yan , Arnaldo Carvalho de Melo , Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Wei Li , James Clark , Dave Martin , linux-kernel@vger.kernel.org, Al Grant References: <20200929133917.9224-1-leo.yan@linaro.org> <20200929133917.9224-7-leo.yan@linaro.org> From: =?UTF-8?Q?Andr=c3=a9_Przywara?= Organization: ARM Ltd. Message-ID: Date: Thu, 8 Oct 2020 20:49:11 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20200929133917.9224-7-leo.yan@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 > --- > .../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) > >