Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 06/14] perf arm-spe: Refactor packet header parsing
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>
References: <20200929133917.9224-1-leo.yan@linaro.org>
 <20200929133917.9224-7-leo.yan@linaro.org>
From:   =?UTF-8?Q?Andr=c3=a9_Przywara?= <andre.przywara@arm.com>
Organization: ARM Ltd.
Message-ID: <d3d0bf64-0285-1aef-3ec1-74f0d28257a6@arm.com>
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

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