On Tue, Oct 20, 2020 at 10:53:47PM +0100, André Przywara wrote: > On 29/09/2020 14:39, Leo Yan wrote: > > Hi, > > > This patch defines macros for counter packet header, and uses macro to > > replace hard code values for packet parsing. > > > > Signed-off-by: Leo Yan <leo....@linaro.org> > > --- > > .../util/arm-spe-decoder/arm-spe-pkt-decoder.c | 17 ++++++++++------- > > .../util/arm-spe-decoder/arm-spe-pkt-decoder.h | 9 +++++++++ > > 2 files changed, 19 insertions(+), 7 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 00a2cd1af422..ed0f4c74dfc5 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 > > @@ -150,10 +150,13 @@ static int arm_spe_get_counter(const unsigned char > > *buf, size_t len, > > const unsigned char ext_hdr, struct arm_spe_pkt > > *packet) > > { > > packet->type = ARM_SPE_COUNTER; > > - if (ext_hdr) > > - packet->index = ((buf[0] & 0x3) << 3) | (buf[1] & 0x7); > > - else > > - packet->index = buf[0] & 0x7; > > + if (ext_hdr) { > > + packet->index = (buf[1] & SPE_CNT_PKT_HDR_INDEX_MASK); > > + packet->index |= ((buf[0] & SPE_CNT_PKT_HDR_EXT_INDEX_MASK) > > + << SPE_CNT_PKT_HDR_EXT_INDEX_SHIFT); > > + } else { > > + packet->index = buf[0] & SPE_CNT_PKT_HDR_INDEX_MASK; > > That looks suspiciously similar to the extended header in the address > packet. Can you use the same name for that?
Checked for counter packet and address packet, they are using the same format for index. Will use the same name. > And, similar to the address packet, what about: > packet->index |= SPE_PKT_EXT_HEADER_INDEX(buf[0]); Will do. > (merging the mask and the shift in the macro definition) > > > + } > > > > return arm_spe_get_payload(buf, len, ext_hdr, packet); > > } > > @@ -431,17 +434,17 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt > > *packet, char *buf, > > return ret; > > > > switch (idx) { > > - case 0: > > + case SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT: > > ret = arm_spe_pkt_snprintf(&buf, &blen, "TOT"); > > if (ret < 0) > > return ret; > > break; > > - case 1: > > + case SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT: > > ret = arm_spe_pkt_snprintf(&buf, &blen, "ISSUE"); > > if (ret < 0) > > return ret; > > break; > > - case 2: > > + case SPE_CNT_PKT_HDR_INDEX_TRANS_LAT: > > ret = arm_spe_pkt_snprintf(&buf, &blen, "XLAT"); > > if (ret < 0) > > return ret; > > 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 62db4ff91832..18667a63f5ba 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 > > @@ -89,6 +89,15 @@ struct arm_spe_pkt { > > /* Context packet header */ > > #define SPE_CTX_PKT_HDR_INDEX_MASK GENMASK_ULL(1, 0) > > > > +/* Counter packet header */ > > +#define SPE_CNT_PKT_HDR_INDEX_MASK GENMASK_ULL(2, 0) > > +#define SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT (0x0) > > +#define SPE_CNT_PKT_HDR_INDEX_ISSUE_LAT (0x1) > > +#define SPE_CNT_PKT_HDR_INDEX_TRANS_LAT (0x2) > > I think the Linux kernel coding style does not mention parentheses just > around numbers, so just 0x2 would suffice, for instance. > See section 12) in Documentation/process/coding-style.rst Yeah, it gives example as "#define CONSTANT 0x4000"; will follow the coding style. Thanks for suggestions! Leo