Re: [PATCH v2 06/14] perf arm-spe: Refactor packet header parsing

[Leo Yan <leo.yan@linaro.org>]

On Thu, Oct 08, 2020 at 08:49:11PM +0100, André Przywara wrote:
> 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!

Welcome :)

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

You are right, here I introduced confusion for MASK3 for bits [7:5];
will change to use MASK2 for the mask of the extended header type.

> > +		/* 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?

Exactly, will apply this in next spin.

Very appreicate for your detailed reviewing.

Thanks,
Leo