Messages in this thread | | | Subject | Re: [PATCH v3 03/20] perf arm-spe: Refactor payload size calculation | From | André Przywara <> | Date | Fri, 23 Oct 2020 18:08:53 +0100 |
| |
On 22/10/2020 15:57, Leo Yan wrote:
Hi Leo,
> This patch defines macro to extract "sz" field from header, and renames > the function payloadlen() to arm_spe_payload_len(). > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > .../util/arm-spe-decoder/arm-spe-pkt-decoder.c | 18 +++++++++--------- > .../util/arm-spe-decoder/arm-spe-pkt-decoder.h | 3 +++ > 2 files changed, 12 insertions(+), 9 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 7c7b5eb09fba..4294c133a465 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 > @@ -69,22 +69,22 @@ const char *arm_spe_pkt_name(enum arm_spe_pkt_type type) > return arm_spe_packet_name[type]; > } > > -/* return ARM SPE payload size from its encoding, > - * which is in bits 5:4 of the byte. > - * 00 : byte > - * 01 : halfword (2) > - * 10 : word (4) > - * 11 : doubleword (8) > +/* > + * Extracts the field "sz" from header bits and converts to bytes: > + * 00 : byte (1) > + * 01 : halfword (2) > + * 10 : word (4) > + * 11 : doubleword (8) > */ > -static int payloadlen(unsigned char byte) > +static unsigned int arm_spe_payload_len(unsigned char hdr) > { > - return 1 << ((byte & 0x30) >> 4); > + return 1 << SPE_HEADER_SZ(hdr);
I know, I know, I asked for this, but now looking again at it - and after having seen the whole series: This is now really trivial, and there are just two users? And SPE_HEADER_SZ() is only used in here?
So either you just stuff the "1U << .." into the callers of arm_spe_payload_len(), or indeed put all of this into one macro (as you had originally).
Apologies for this forth and back, but I didn't realise how this is really used eventually, and I just saw the transition from function to macro.
But please use 1U << .., signed shifts are treacherous.
> } > > static int arm_spe_get_payload(const unsigned char *buf, size_t len, > struct arm_spe_pkt *packet) > { > - size_t payload_len = payloadlen(buf[0]); > + size_t payload_len = arm_spe_payload_len(buf[0]); > > if (len < 1 + payload_len) > return ARM_SPE_NEED_MORE_BYTES; > 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 4c870521b8eb..e9ea8e3ead5d 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 > @@ -9,6 +9,7 @@ > > #include <stddef.h> > #include <stdint.h> > +#include <linux/bits.h> > > #define ARM_SPE_PKT_DESC_MAX 256 > > @@ -36,6 +37,8 @@ struct arm_spe_pkt { > uint64_t payload; > }; > > +#define SPE_HEADER_SZ(val) ((val & GENMASK_ULL(5, 4)) >> 4)
If you should keep this, please put parentheses around "val".
Cheers, Andre
> + > #define SPE_ADDR_PKT_HDR_INDEX_INS (0x0) > #define SPE_ADDR_PKT_HDR_INDEX_BRANCH (0x1) > #define SPE_ADDR_PKT_HDR_INDEX_DATA_VIRT (0x2) >
| |