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 <[email protected]> > > --- > > .../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
