* [PATCH 0/2] perf hisi-ptt: Fix TLP header parsing and naming
@ 2026-07-29 6:02 Sizhe Liu
2026-07-29 6:02 ` [PATCH 1/2] perf hisi-ptt: Fix spelling and abbreviation errors Sizhe Liu
2026-07-29 6:02 ` [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing Sizhe Liu
0 siblings, 2 replies; 8+ messages in thread
From: Sizhe Liu @ 2026-07-29 6:02 UTC (permalink / raw)
To: rostedt, mhiramat, mathieu.desnoyers, corbet, skhan, bhelgaas,
yangyccccc, jic23, john.g.garry, will, james.clark, mike.leach,
leo.yan, peterz, mingo, acme, namhyung, mark.rutland,
alexander.shishkin, jolsa, irogers, adrian.hunter, wangyushan12
Cc: linux-kernel, linux-pci, linux-perf-users, linux-arm-kernel,
linux-doc, linuxarm, prime.zeng, wuyifan50, liusizhe5
This series fixes two issues in the hisi_ptt packet decoder that were
introduced by commit 5e91e57e6809 ("perf auxtrace arm64: Add support
for parsing HiSilicon PCIe Trace packet").
Patch 1 fixes spelling / abbreviation errors in symbol names and
macros.
Patch 2 fixes a bug:
The DW0 bit-field layout of the hisi_ptt_4dw union does not match
the actual little-endian memory ordering, so the extracted
Format/Type/T9/T8/TH/SO/Length/Time fields are wrong.
Meanwhile, make all DW hex values printed in big-endian byte order
for readability.
Sizhe Liu (2):
perf hisi-ptt: Fix spelling and abbreviation errors
perf hisi-ptt: Fix PTT trace TLP header parsing
Documentation/trace/hisi-ptt.rst | 28 ++++-----
.../hisi-ptt-decoder/hisi-ptt-pkt-decoder.c | 62 +++++++++----------
.../hisi-ptt-decoder/hisi-ptt-pkt-decoder.h | 14 ++++-
3 files changed, 58 insertions(+), 46 deletions(-)
--
2.33.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] perf hisi-ptt: Fix spelling and abbreviation errors 2026-07-29 6:02 [PATCH 0/2] perf hisi-ptt: Fix TLP header parsing and naming Sizhe Liu @ 2026-07-29 6:02 ` Sizhe Liu 2026-07-29 6:10 ` sashiko-bot 2026-07-29 10:17 ` James Clark 2026-07-29 6:02 ` [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing Sizhe Liu 1 sibling, 2 replies; 8+ messages in thread From: Sizhe Liu @ 2026-07-29 6:02 UTC (permalink / raw) To: rostedt, mhiramat, mathieu.desnoyers, corbet, skhan, bhelgaas, yangyccccc, jic23, john.g.garry, will, james.clark, mike.leach, leo.yan, peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, wangyushan12 Cc: linux-kernel, linux-pci, linux-perf-users, linux-arm-kernel, linux-doc, linuxarm, prime.zeng, wuyifan50, liusizhe5 Fix spelling and abbreviation errors in the PTT packet decoder: - HISI_PTT_FIELD_LENTH -> HISI_PTT_FIELD_LENGTH - hisi_ptt_8dw_kpt_desc -> hisi_ptt_8dw_pkt_desc - hisi_ptt_4dw_kpt_desc -> hisi_ptt_4dw_pkt_desc Cc: stable@vger.kernel.org Fixes: 5e91e57e6809 ("perf auxtrace arm64: Add support for parsing HiSilicon PCIe Trace packet") Signed-off-by: Sizhe Liu <liusizhe5@huawei.com> --- .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.c | 20 +++++++++---------- .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.h | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c index a17c423a526d..c48b2ce7c4a3 100644 --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c @@ -94,26 +94,26 @@ static void hisi_ptt_print_pkt(const unsigned char *buf, int pos, const char *de printf("."); color_fprintf(stdout, color, " %08x: ", pos); - for (i = 0; i < HISI_PTT_FIELD_LENTH; i++) + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) color_fprintf(stdout, color, "%02x ", buf[pos + i]); for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) color_fprintf(stdout, color, " "); color_fprintf(stdout, color, " %s\n", desc); } -static int hisi_ptt_8dw_kpt_desc(const unsigned char *buf, int pos) +static int hisi_ptt_8dw_pkt_desc(const unsigned char *buf, int pos) { int i; for (i = 0; i < HISI_PTT_8DW_TYPE_MAX; i++) { /* Do not show 8DW check field and reserved fields */ if (i == HISI_PTT_8DW_CHK_AND_RSV0 || i == HISI_PTT_8DW_RSV1) { - pos += HISI_PTT_FIELD_LENTH; + pos += HISI_PTT_FIELD_LENGTH; continue; } hisi_ptt_print_pkt(buf, pos, hisi_ptt_8dw_pkt_field_name[i]); - pos += HISI_PTT_FIELD_LENTH; + pos += HISI_PTT_FIELD_LENGTH; } return hisi_ptt_pkt_size[HISI_PTT_8DW_PKT]; @@ -128,7 +128,7 @@ static void hisi_ptt_4dw_print_dw0(const unsigned char *buf, int pos) dw0.value = *(uint32_t *)(buf + pos); printf("."); color_fprintf(stdout, color, " %08x: ", pos); - for (i = 0; i < HISI_PTT_FIELD_LENTH; i++) + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) color_fprintf(stdout, color, "%02x ", buf[pos + i]); for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) color_fprintf(stdout, color, " "); @@ -140,16 +140,16 @@ static void hisi_ptt_4dw_print_dw0(const unsigned char *buf, int pos) dw0.len, "Time", dw0.time); } -static int hisi_ptt_4dw_kpt_desc(const unsigned char *buf, int pos) +static int hisi_ptt_4dw_pkt_desc(const unsigned char *buf, int pos) { int i; hisi_ptt_4dw_print_dw0(buf, pos); - pos += HISI_PTT_FIELD_LENTH; + pos += HISI_PTT_FIELD_LENGTH; for (i = 0; i < HISI_PTT_4DW_TYPE_MAX; i++) { hisi_ptt_print_pkt(buf, pos, hisi_ptt_4dw_pkt_field_name[i]); - pos += HISI_PTT_FIELD_LENTH; + pos += HISI_PTT_FIELD_LENGTH; } return hisi_ptt_pkt_size[HISI_PTT_4DW_PKT]; @@ -158,7 +158,7 @@ static int hisi_ptt_4dw_kpt_desc(const unsigned char *buf, int pos) int hisi_ptt_pkt_desc(const unsigned char *buf, int pos, enum hisi_ptt_pkt_type type) { if (type == HISI_PTT_8DW_PKT) - return hisi_ptt_8dw_kpt_desc(buf, pos); + return hisi_ptt_8dw_pkt_desc(buf, pos); - return hisi_ptt_4dw_kpt_desc(buf, pos); + return hisi_ptt_4dw_pkt_desc(buf, pos); } diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h index e78f1b5bc836..6772b16b817b 100644 --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h @@ -13,7 +13,7 @@ #define HISI_PTT_8DW_CHECK_MASK GENMASK(31, 11) #define HISI_PTT_IS_8DW_PKT GENMASK(31, 11) #define HISI_PTT_MAX_SPACE_LEN 10 -#define HISI_PTT_FIELD_LENTH 4 +#define HISI_PTT_FIELD_LENGTH 4 enum hisi_ptt_pkt_type { HISI_PTT_4DW_PKT, -- 2.33.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf hisi-ptt: Fix spelling and abbreviation errors 2026-07-29 6:02 ` [PATCH 1/2] perf hisi-ptt: Fix spelling and abbreviation errors Sizhe Liu @ 2026-07-29 6:10 ` sashiko-bot 2026-07-29 10:17 ` James Clark 1 sibling, 0 replies; 8+ messages in thread From: sashiko-bot @ 2026-07-29 6:10 UTC (permalink / raw) To: Sizhe Liu; +Cc: linux-perf-users, linux-pci > Fix spelling and abbreviation errors in the PTT packet decoder: > - HISI_PTT_FIELD_LENTH -> HISI_PTT_FIELD_LENGTH > - hisi_ptt_8dw_kpt_desc -> hisi_ptt_8dw_pkt_desc > - hisi_ptt_4dw_kpt_desc -> hisi_ptt_4dw_pkt_desc > > Cc: stable@vger.kernel.org > Fixes: 5e91e57e6809 ("perf auxtrace arm64: Add support for parsing HiSilicon PCIe Trace packet") > Signed-off-by: Sizhe Liu <liusizhe5@huawei.com> Sashiko has reviewed this patch and found no issues. It looks great! -- Sashiko AI review · https://sashiko.dev/#/patchset/20260729060220.624250-1-liusizhe5@huawei.com?part=1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] perf hisi-ptt: Fix spelling and abbreviation errors 2026-07-29 6:02 ` [PATCH 1/2] perf hisi-ptt: Fix spelling and abbreviation errors Sizhe Liu 2026-07-29 6:10 ` sashiko-bot @ 2026-07-29 10:17 ` James Clark 1 sibling, 0 replies; 8+ messages in thread From: James Clark @ 2026-07-29 10:17 UTC (permalink / raw) To: Sizhe Liu Cc: linux-kernel, linux-pci, linux-perf-users, linux-arm-kernel, linux-doc, linuxarm, prime.zeng, wuyifan50, rostedt, mhiramat, mathieu.desnoyers, corbet, skhan, bhelgaas, yangyccccc, jic23, john.g.garry, will, mike.leach, leo.yan, peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, wangyushan12 On 29/07/2026 7:02 am, Sizhe Liu wrote: > Fix spelling and abbreviation errors in the PTT packet decoder: > - HISI_PTT_FIELD_LENTH -> HISI_PTT_FIELD_LENGTH > - hisi_ptt_8dw_kpt_desc -> hisi_ptt_8dw_pkt_desc > - hisi_ptt_4dw_kpt_desc -> hisi_ptt_4dw_pkt_desc > > Cc: stable@vger.kernel.org > Fixes: 5e91e57e6809 ("perf auxtrace arm64: Add support for parsing HiSilicon PCIe Trace packet") Probably doesn't need a fixes tag, but: Reviewed-by: James Clark <james.clark@linaro.org> > Signed-off-by: Sizhe Liu <liusizhe5@huawei.com> > --- > .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.c | 20 +++++++++---------- > .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.h | 2 +- > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c > index a17c423a526d..c48b2ce7c4a3 100644 > --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c > +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c > @@ -94,26 +94,26 @@ static void hisi_ptt_print_pkt(const unsigned char *buf, int pos, const char *de > > printf("."); > color_fprintf(stdout, color, " %08x: ", pos); > - for (i = 0; i < HISI_PTT_FIELD_LENTH; i++) > + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) > color_fprintf(stdout, color, "%02x ", buf[pos + i]); > for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) > color_fprintf(stdout, color, " "); > color_fprintf(stdout, color, " %s\n", desc); > } > > -static int hisi_ptt_8dw_kpt_desc(const unsigned char *buf, int pos) > +static int hisi_ptt_8dw_pkt_desc(const unsigned char *buf, int pos) > { > int i; > > for (i = 0; i < HISI_PTT_8DW_TYPE_MAX; i++) { > /* Do not show 8DW check field and reserved fields */ > if (i == HISI_PTT_8DW_CHK_AND_RSV0 || i == HISI_PTT_8DW_RSV1) { > - pos += HISI_PTT_FIELD_LENTH; > + pos += HISI_PTT_FIELD_LENGTH; > continue; > } > > hisi_ptt_print_pkt(buf, pos, hisi_ptt_8dw_pkt_field_name[i]); > - pos += HISI_PTT_FIELD_LENTH; > + pos += HISI_PTT_FIELD_LENGTH; > } > > return hisi_ptt_pkt_size[HISI_PTT_8DW_PKT]; > @@ -128,7 +128,7 @@ static void hisi_ptt_4dw_print_dw0(const unsigned char *buf, int pos) > dw0.value = *(uint32_t *)(buf + pos); > printf("."); > color_fprintf(stdout, color, " %08x: ", pos); > - for (i = 0; i < HISI_PTT_FIELD_LENTH; i++) > + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) > color_fprintf(stdout, color, "%02x ", buf[pos + i]); > for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) > color_fprintf(stdout, color, " "); > @@ -140,16 +140,16 @@ static void hisi_ptt_4dw_print_dw0(const unsigned char *buf, int pos) > dw0.len, "Time", dw0.time); > } > > -static int hisi_ptt_4dw_kpt_desc(const unsigned char *buf, int pos) > +static int hisi_ptt_4dw_pkt_desc(const unsigned char *buf, int pos) > { > int i; > > hisi_ptt_4dw_print_dw0(buf, pos); > - pos += HISI_PTT_FIELD_LENTH; > + pos += HISI_PTT_FIELD_LENGTH; > > for (i = 0; i < HISI_PTT_4DW_TYPE_MAX; i++) { > hisi_ptt_print_pkt(buf, pos, hisi_ptt_4dw_pkt_field_name[i]); > - pos += HISI_PTT_FIELD_LENTH; > + pos += HISI_PTT_FIELD_LENGTH; > } > > return hisi_ptt_pkt_size[HISI_PTT_4DW_PKT]; > @@ -158,7 +158,7 @@ static int hisi_ptt_4dw_kpt_desc(const unsigned char *buf, int pos) > int hisi_ptt_pkt_desc(const unsigned char *buf, int pos, enum hisi_ptt_pkt_type type) > { > if (type == HISI_PTT_8DW_PKT) > - return hisi_ptt_8dw_kpt_desc(buf, pos); > + return hisi_ptt_8dw_pkt_desc(buf, pos); > > - return hisi_ptt_4dw_kpt_desc(buf, pos); > + return hisi_ptt_4dw_pkt_desc(buf, pos); > } > diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h > index e78f1b5bc836..6772b16b817b 100644 > --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h > +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h > @@ -13,7 +13,7 @@ > #define HISI_PTT_8DW_CHECK_MASK GENMASK(31, 11) > #define HISI_PTT_IS_8DW_PKT GENMASK(31, 11) > #define HISI_PTT_MAX_SPACE_LEN 10 > -#define HISI_PTT_FIELD_LENTH 4 > +#define HISI_PTT_FIELD_LENGTH 4 > > enum hisi_ptt_pkt_type { > HISI_PTT_4DW_PKT, ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing 2026-07-29 6:02 [PATCH 0/2] perf hisi-ptt: Fix TLP header parsing and naming Sizhe Liu 2026-07-29 6:02 ` [PATCH 1/2] perf hisi-ptt: Fix spelling and abbreviation errors Sizhe Liu @ 2026-07-29 6:02 ` Sizhe Liu 2026-07-29 6:20 ` sashiko-bot 2026-07-29 10:16 ` James Clark 1 sibling, 2 replies; 8+ messages in thread From: Sizhe Liu @ 2026-07-29 6:02 UTC (permalink / raw) To: rostedt, mhiramat, mathieu.desnoyers, corbet, skhan, bhelgaas, yangyccccc, jic23, john.g.garry, will, james.clark, mike.leach, leo.yan, peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, wangyushan12 Cc: linux-kernel, linux-pci, linux-perf-users, linux-arm-kernel, linux-doc, linuxarm, prime.zeng, wuyifan50, liusizhe5 TLP Headers traced by HiSilicon PCIe tune and trace device (PTT) in 4DW format are shown in the document as below: bits [31:30] [ 29:25 ][24][23][22][21][ 20:11 ][ 10:0 ] |-----|---------|---|---|---|---|-------------|-------------| DW0 [ Fmt ][ Type ][T9][T8][TH][SO][ Length ][ Time ] DW1 [ Header DW1 ] DW2 [ Header DW2 ] DW3 [ Header DW3 ] Problem: The DW0 bit field layout of the hisi_ptt_4dw union does not match the actual bit ordering in little-endian memory, causing incorrect field decoding. Test on Kunpeng 930 SOC, generating data flow with `iperf` commands: - server side: iperf -s - client side: iperf -c $ip_addr -t 30 Trace the TLP headers with hisi_ptt on server side at the same time: perf record -e hisi_ptt12_0/type=4,filter=0x05101,direction=2,format=0/ \ --max-size 50M -o perf.data & The trace aims to capture completion TLPs, learn more in the document: https://docs.kernel.org/trace/hisi-ptt.html Decode perf.data with hisi_ptt decoder: perf report -D The hisi_ptt decoder produces the following result: [...perf headers and other information] . ... HISI PTT data: size 8388608 bytes . 00000000: 68 87 20 94 Format 3 Type 1a T9 0 T8 1 TH 1 SO 1 Length 10 Time 4a1 . 00000004: 40 00 00 00 Header DW1 . 00000008: 40 00 01 51 Header DW2 . 0000000c: 00 00 00 00 Header DW3 [...other hisi_ptt TLP headers] According to PCIe r5.0 2.2.1, the Fmt & Type of Cpl/CplD is supposed to be 8b'00001010' / 8b'01001010' However, the Format & Type decoder analyzing result is 8b'01111010'. It does not match field encodings of any TLP messages. Correct decoder result should be: [...perf headers and other information] . ... HISI PTT data: size 8388608 bytes . 00000000: 94 20 87 68 Format 2 Type a T9 0 T8 0 TH 0 SO 1 Length 10 Time 768 . 00000004: 00 00 00 40 Header DW1 . 00000008: 51 01 00 40 Header DW2 . 0000000c: 00 00 00 00 Header DW3 [...other hisi_ptt TLP headers] To solve the problem: 1. Drop the union and C bitfield struct, store the raw DW value in a plain uint32_t, and extract the fields with FIELD_GET() against GENMASK/BIT masks declared in the header so they can be reused by other translation units. The masks are portable across endianness and compilers. 2. Print all DW hex values in big-endian byte order for readability, matching the bit field layout shown in the 4DW format diagram. Cc: stable@vger.kernel.org Fixes: 5e91e57e6809 ("perf auxtrace arm64: Add support for parsing HiSilicon PCIe Trace packet") Signed-off-by: Sizhe Liu <liusizhe5@huawei.com> --- Documentation/trace/hisi-ptt.rst | 28 +++++------ .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.c | 46 +++++++++---------- .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.h | 12 +++++ 3 files changed, 49 insertions(+), 37 deletions(-) diff --git a/Documentation/trace/hisi-ptt.rst b/Documentation/trace/hisi-ptt.rst index 6eef28ebb0c7..f6a2655f99e5 100644 --- a/Documentation/trace/hisi-ptt.rst +++ b/Documentation/trace/hisi-ptt.rst @@ -285,20 +285,20 @@ according to the format described previously (take 8DW as an example): [...perf headers and other information] . ... HISI PTT data: size 4194304 bytes . 00000000: 00 00 00 00 Prefix - . 00000004: 01 00 00 60 Header DW0 - . 00000008: 0f 1e 00 01 Header DW1 - . 0000000c: 04 00 00 00 Header DW2 - . 00000010: 40 00 81 02 Header DW3 - . 00000014: 33 c0 04 00 Time + . 00000004: 60 00 00 01 Header DW0 + . 00000008: 01 00 1e 0f Header DW1 + . 0000000c: 00 00 00 04 Header DW2 + . 00000010: 02 81 00 40 Header DW3 + . 00000014: 00 04 c0 33 Time . 00000020: 00 00 00 00 Prefix - . 00000024: 01 00 00 60 Header DW0 - . 00000028: 0f 1e 00 01 Header DW1 - . 0000002c: 04 00 00 00 Header DW2 - . 00000030: 40 00 81 02 Header DW3 - . 00000034: 02 00 00 00 Time + . 00000024: 60 00 00 01 Header DW0 + . 00000028: 01 00 1e 0f Header DW1 + . 0000002c: 00 00 00 04 Header DW2 + . 00000030: 02 81 00 40 Header DW3 + . 00000034: 00 00 00 02 Time . 00000040: 00 00 00 00 Prefix - . 00000044: 01 00 00 60 Header DW0 - . 00000048: 0f 1e 00 01 Header DW1 - . 0000004c: 04 00 00 00 Header DW2 - . 00000050: 40 00 81 02 Header DW3 + . 00000044: 60 00 00 01 Header DW0 + . 00000048: 01 00 1e 0f Header DW1 + . 0000004c: 00 00 00 04 Header DW2 + . 00000050: 02 81 00 40 Header DW3 [...] diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c index c48b2ce7c4a3..c1e169561087 100644 --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c @@ -10,6 +10,7 @@ #include <endian.h> #include <byteswap.h> #include <linux/bitops.h> +#include <linux/kernel.h> #include <stdarg.h> #include "../color.h" @@ -73,29 +74,20 @@ static const char * const hisi_ptt_4dw_pkt_field_name[] = { [HISI_PTT_4DW_HEAD3] = "Header DW3", }; -union hisi_ptt_4dw { - struct { - uint32_t format : 2; - uint32_t type : 5; - uint32_t t9 : 1; - uint32_t t8 : 1; - uint32_t th : 1; - uint32_t so : 1; - uint32_t len : 10; - uint32_t time : 11; - }; - uint32_t value; -}; - static void hisi_ptt_print_pkt(const unsigned char *buf, int pos, const char *desc) { const char *color = PERF_COLOR_BLUE; + uint32_t value; + uint8_t byte; int i; + value = le32_to_cpu(*(__le32 *)(buf + pos)); printf("."); color_fprintf(stdout, color, " %08x: ", pos); - for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) - color_fprintf(stdout, color, "%02x ", buf[pos + i]); + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) { + byte = (value >> (24 - i * 8)) & 0xFF; + color_fprintf(stdout, color, "%02x ", byte); + } for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) color_fprintf(stdout, color, " "); color_fprintf(stdout, color, " %s\n", desc); @@ -122,22 +114,30 @@ static int hisi_ptt_8dw_pkt_desc(const unsigned char *buf, int pos) static void hisi_ptt_4dw_print_dw0(const unsigned char *buf, int pos) { const char *color = PERF_COLOR_BLUE; - union hisi_ptt_4dw dw0; + uint8_t byte; + uint32_t dw; int i; - dw0.value = *(uint32_t *)(buf + pos); + dw = le32_to_cpu(*(__le32 *)(buf + pos)); printf("."); color_fprintf(stdout, color, " %08x: ", pos); - for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) - color_fprintf(stdout, color, "%02x ", buf[pos + i]); + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) { + byte = (dw >> (24 - i * 8)) & 0xFF; + color_fprintf(stdout, color, "%02x ", byte); + } for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) color_fprintf(stdout, color, " "); color_fprintf(stdout, color, " %s %x %s %x %s %x %s %x %s %x %s %x %s %x %s %x\n", - "Format", dw0.format, "Type", dw0.type, "T9", dw0.t9, - "T8", dw0.t8, "TH", dw0.th, "SO", dw0.so, "Length", - dw0.len, "Time", dw0.time); + "Format", FIELD_GET(HISI_PTT_HEAD0_4DW_FORMAT, dw), + "Type", FIELD_GET(HISI_PTT_HEAD0_4DW_TYPE, dw), + "T9", FIELD_GET(HISI_PTT_HEAD0_4DW_T9, dw), + "T8", FIELD_GET(HISI_PTT_HEAD0_4DW_T8, dw), + "TH", FIELD_GET(HISI_PTT_HEAD0_4DW_TH, dw), + "SO", FIELD_GET(HISI_PTT_HEAD0_4DW_SO, dw), + "Length", FIELD_GET(HISI_PTT_HEAD0_4DW_LEN, dw), + "Time", FIELD_GET(HISI_PTT_HEAD0_4DW_TIME, dw)); } static int hisi_ptt_4dw_pkt_desc(const unsigned char *buf, int pos) diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h index 6772b16b817b..3b349fcc24ae 100644 --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h @@ -9,12 +9,24 @@ #include <stddef.h> #include <stdint.h> +#include <linux/bits.h> +#include <linux/bitfield.h> #define HISI_PTT_8DW_CHECK_MASK GENMASK(31, 11) #define HISI_PTT_IS_8DW_PKT GENMASK(31, 11) #define HISI_PTT_MAX_SPACE_LEN 10 #define HISI_PTT_FIELD_LENGTH 4 +/* Header DW0 fields for 4DW format */ +#define HISI_PTT_HEAD0_4DW_TIME GENMASK_U32(10, 0) +#define HISI_PTT_HEAD0_4DW_LEN GENMASK_U32(20, 11) +#define HISI_PTT_HEAD0_4DW_SO BIT_U32(21) +#define HISI_PTT_HEAD0_4DW_TH BIT_U32(22) +#define HISI_PTT_HEAD0_4DW_T8 BIT_U32(23) +#define HISI_PTT_HEAD0_4DW_T9 BIT_U32(24) +#define HISI_PTT_HEAD0_4DW_TYPE GENMASK_U32(29, 25) +#define HISI_PTT_HEAD0_4DW_FORMAT GENMASK_U32(31, 30) + enum hisi_ptt_pkt_type { HISI_PTT_4DW_PKT, HISI_PTT_8DW_PKT, -- 2.33.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing 2026-07-29 6:02 ` [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing Sizhe Liu @ 2026-07-29 6:20 ` sashiko-bot 2026-07-29 10:16 ` James Clark 1 sibling, 0 replies; 8+ messages in thread From: sashiko-bot @ 2026-07-29 6:20 UTC (permalink / raw) To: Sizhe Liu; +Cc: linux-perf-users, linux-pci Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: New issues: - [Medium] Incompatible pointer cast from `const unsigned char *` to `__le32 *` violates strict aliasing rules. Pre-existing issues: - [High] Incomplete cross-platform decoding fix: `hisi_ptt_check_packet_type` reads the packet header using a host-endian memory cast, breaking 8DW packet identification on Big-Endian hosts. - [High] Heap out-of-bounds read when processing auxtrace events with a size less than 4 bytes, due to missing bounds checking before packet type evaluation. - [High] Integer truncation of the 64-bit `event->auxtrace.size` leads to allocation failures and stream desynchronization. -- --- Patch [2]: [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing --- commit 637114d43d04dfaf1c2cef1c93751d90838d2eb7 Author: Sizhe Liu <liusizhe5@huawei.com> perf hisi-ptt: Fix PTT trace TLP header parsing The DW0 bit field layout of the hisi_ptt_4dw union does not match the actual bit ordering in little-endian memory. This commit fixes the decoding by replacing the bitfield union with FIELD_GET() operations against GENMASK/BIT masks and printing DW values in big-endian order. > diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c > index c48b2ce7c4a3d..c1e169561087c 100644 > --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c > +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c [ ... ] > @@ -73,29 +74,20 @@ static const char * const hisi_ptt_4dw_pkt_field_name[] = { [ ... ] > static void hisi_ptt_print_pkt(const unsigned char *buf, int pos, const char *desc) > { > const char *color = PERF_COLOR_BLUE; > + uint32_t value; > + uint8_t byte; > int i; > > + value = le32_to_cpu(*(__le32 *)(buf + pos)); [Severity: Medium] Does this incompatible pointer cast from const unsigned char * to __le32 * violate strict aliasing rules? Would it be safer to use get_unaligned_le32(buf + pos) instead? > printf("."); > color_fprintf(stdout, color, " %08x: ", pos); > - for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) > - color_fprintf(stdout, color, "%02x ", buf[pos + i]); > + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) { > + byte = (value >> (24 - i * 8)) & 0xFF; > + color_fprintf(stdout, color, "%02x ", byte); > + } [ ... ] > @@ -122,22 +114,30 @@ static int hisi_ptt_8dw_pkt_desc(const unsigned char *buf, int pos) > static void hisi_ptt_4dw_print_dw0(const unsigned char *buf, int pos) > { > const char *color = PERF_COLOR_BLUE; > - union hisi_ptt_4dw dw0; > + uint8_t byte; > + uint32_t dw; > int i; > > - dw0.value = *(uint32_t *)(buf + pos); > + dw = le32_to_cpu(*(__le32 *)(buf + pos)); [Severity: Medium] Does this incompatible pointer cast from const unsigned char * to __le32 * violate strict aliasing rules? Would it be safer to use get_unaligned_le32(buf + pos) instead? > printf("."); > color_fprintf(stdout, color, " %08x: ", pos); > - for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) > - color_fprintf(stdout, color, "%02x ", buf[pos + i]); > + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) { > + byte = (dw >> (24 - i * 8)) & 0xFF; > + color_fprintf(stdout, color, "%02x ", byte); > + } [ ... ] [Severity: High] This is a pre-existing issue, but does hisi_ptt_check_packet_type() break 8DW packet identification on big-endian hosts? tools/perf/util/hisi-ptt.c:hisi_ptt_check_packet_type() uint32_t head = *(uint32_t *)buf; if ((HISI_PTT_8DW_CHECK_MASK & head) == HISI_PTT_IS_8DW_PKT) return HISI_PTT_8DW_PKT; The function reads the first 32-bit word using a host-endian memory cast without correcting for host endianness. When analyzing a little-endian trace on a big-endian host, this would cause valid 8DW packets to fail the bitwise mask check and be misclassified as 4DW packets. [Severity: High] This is a pre-existing issue, but can this sequence lead to a heap out-of-bounds read when processing malformed auxtrace events? If the user provides an event with an auxtrace.size between 0 and 3 bytes: tools/perf/util/hisi-ptt.c:hisi_ptt_process_auxtrace_event() int size = event->auxtrace.size; void *data = malloc(size); ... err = readn(fd, data, size); ... if (dump_trace) hisi_ptt_dump_event(ptt, data, size); The buffer is passed to hisi_ptt_dump(), which unconditionally dereferences the first 4 bytes of the buffer inside hisi_ptt_check_packet_type() before validating if the buffer is large enough: tools/perf/util/hisi-ptt.c:hisi_ptt_dump() type = hisi_ptt_check_packet_type(buf); tools/perf/util/hisi-ptt.c:hisi_ptt_check_packet_type() uint32_t head = *(uint32_t *)buf; Could this cause a heap out-of-bounds read if the buffer is smaller than 4 bytes? [Severity: High] This is a pre-existing issue, but does integer truncation on event->auxtrace.size cause allocation failures or stream desynchronization? tools/perf/util/hisi-ptt.c:hisi_ptt_process_auxtrace_event() int size = event->auxtrace.size; void *data = malloc(size); ... err = readn(fd, data, size); The 64-bit user-controlled size is implicitly truncated to a 32-bit signed integer. For large valid traces (e.g., > 2GB), the truncated size becomes negative, causing malloc to fail. Furthermore, if a huge size is specified, it truncates to a small positive integer, causing an undersized read in readn(). Could this leave unread payload in the pipe and permanently desynchronize the stream? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260729060220.624250-1-liusizhe5@huawei.com?part=2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing 2026-07-29 6:02 ` [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing Sizhe Liu 2026-07-29 6:20 ` sashiko-bot @ 2026-07-29 10:16 ` James Clark 2026-07-29 12:06 ` Sizhe Liu 1 sibling, 1 reply; 8+ messages in thread From: James Clark @ 2026-07-29 10:16 UTC (permalink / raw) To: Sizhe Liu Cc: linux-kernel, linux-pci, linux-perf-users, linux-arm-kernel, linux-doc, linuxarm, prime.zeng, wuyifan50, rostedt, mhiramat, mathieu.desnoyers, corbet, skhan, bhelgaas, yangyccccc, jic23, john.g.garry, will, mike.leach, leo.yan, peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, wangyushan12 On 29/07/2026 7:02 am, Sizhe Liu wrote: > TLP Headers traced by HiSilicon PCIe tune and trace device (PTT) in > 4DW format are shown in the document as below: > bits [31:30] [ 29:25 ][24][23][22][21][ 20:11 ][ 10:0 ] > |-----|---------|---|---|---|---|-------------|-------------| > DW0 [ Fmt ][ Type ][T9][T8][TH][SO][ Length ][ Time ] > DW1 [ Header DW1 ] > DW2 [ Header DW2 ] > DW3 [ Header DW3 ] > > Problem: > The DW0 bit field layout of the hisi_ptt_4dw union does not match the > actual bit ordering in little-endian memory, causing incorrect field > decoding. > > Test on Kunpeng 930 SOC, generating data flow with `iperf` commands: > - server side: > iperf -s > - client side: > iperf -c $ip_addr -t 30 > > Trace the TLP headers with hisi_ptt on server side at the same time: > perf record -e hisi_ptt12_0/type=4,filter=0x05101,direction=2,format=0/ \ > --max-size 50M -o perf.data & > The trace aims to capture completion TLPs, learn more in the document: > https://docs.kernel.org/trace/hisi-ptt.html > > Decode perf.data with hisi_ptt decoder: > perf report -D > > The hisi_ptt decoder produces the following result: > [...perf headers and other information] > . ... HISI PTT data: size 8388608 bytes > . 00000000: 68 87 20 94 Format 3 Type 1a T9 0 T8 1 TH 1 SO 1 Length 10 Time 4a1 > . 00000004: 40 00 00 00 Header DW1 > . 00000008: 40 00 01 51 Header DW2 > . 0000000c: 00 00 00 00 Header DW3 > [...other hisi_ptt TLP headers] > > According to PCIe r5.0 2.2.1, the Fmt & Type of Cpl/CplD is supposed to be > 8b'00001010' / 8b'01001010' > However, the Format & Type decoder analyzing result is 8b'01111010'. > It does not match field encodings of any TLP messages. > > Correct decoder result should be: > [...perf headers and other information] > . ... HISI PTT data: size 8388608 bytes > . 00000000: 94 20 87 68 Format 2 Type a T9 0 T8 0 TH 0 SO 1 Length 10 Time 768 > . 00000004: 00 00 00 40 Header DW1 > . 00000008: 51 01 00 40 Header DW2 > . 0000000c: 00 00 00 00 Header DW3 > [...other hisi_ptt TLP headers] > > To solve the problem: > 1. Drop the union and C bitfield struct, store the raw DW value in > a plain uint32_t, and extract the fields with FIELD_GET() against > GENMASK/BIT masks declared in the header so they can be reused by > other translation units. The masks are portable across endianness and > compilers. > > 2. Print all DW hex values in big-endian byte order for readability, > matching the bit field layout shown in the 4DW format diagram. > > Cc: stable@vger.kernel.org > Fixes: 5e91e57e6809 ("perf auxtrace arm64: Add support for parsing HiSilicon PCIe Trace packet") > Signed-off-by: Sizhe Liu <liusizhe5@huawei.com> > --- > Documentation/trace/hisi-ptt.rst | 28 +++++------ > .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.c | 46 +++++++++---------- > .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.h | 12 +++++ > 3 files changed, 49 insertions(+), 37 deletions(-) > > diff --git a/Documentation/trace/hisi-ptt.rst b/Documentation/trace/hisi-ptt.rst > index 6eef28ebb0c7..f6a2655f99e5 100644 > --- a/Documentation/trace/hisi-ptt.rst > +++ b/Documentation/trace/hisi-ptt.rst > @@ -285,20 +285,20 @@ according to the format described previously (take 8DW as an example): > [...perf headers and other information] > . ... HISI PTT data: size 4194304 bytes > . 00000000: 00 00 00 00 Prefix > - . 00000004: 01 00 00 60 Header DW0 > - . 00000008: 0f 1e 00 01 Header DW1 > - . 0000000c: 04 00 00 00 Header DW2 > - . 00000010: 40 00 81 02 Header DW3 > - . 00000014: 33 c0 04 00 Time > + . 00000004: 60 00 00 01 Header DW0 > + . 00000008: 01 00 1e 0f Header DW1 > + . 0000000c: 00 00 00 04 Header DW2 > + . 00000010: 02 81 00 40 Header DW3 > + . 00000014: 00 04 c0 33 Time > . 00000020: 00 00 00 00 Prefix > - . 00000024: 01 00 00 60 Header DW0 > - . 00000028: 0f 1e 00 01 Header DW1 > - . 0000002c: 04 00 00 00 Header DW2 > - . 00000030: 40 00 81 02 Header DW3 > - . 00000034: 02 00 00 00 Time > + . 00000024: 60 00 00 01 Header DW0 > + . 00000028: 01 00 1e 0f Header DW1 > + . 0000002c: 00 00 00 04 Header DW2 > + . 00000030: 02 81 00 40 Header DW3 > + . 00000034: 00 00 00 02 Time > . 00000040: 00 00 00 00 Prefix > - . 00000044: 01 00 00 60 Header DW0 > - . 00000048: 0f 1e 00 01 Header DW1 > - . 0000004c: 04 00 00 00 Header DW2 > - . 00000050: 40 00 81 02 Header DW3 > + . 00000044: 60 00 00 01 Header DW0 > + . 00000048: 01 00 1e 0f Header DW1 > + . 0000004c: 00 00 00 04 Header DW2 > + . 00000050: 02 81 00 40 Header DW3 > [...] > diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c > index c48b2ce7c4a3..c1e169561087 100644 > --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c > +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c > @@ -10,6 +10,7 @@ > #include <endian.h> > #include <byteswap.h> > #include <linux/bitops.h> > +#include <linux/kernel.h> > #include <stdarg.h> > > #include "../color.h" > @@ -73,29 +74,20 @@ static const char * const hisi_ptt_4dw_pkt_field_name[] = { > [HISI_PTT_4DW_HEAD3] = "Header DW3", > }; > > -union hisi_ptt_4dw { > - struct { > - uint32_t format : 2; > - uint32_t type : 5; > - uint32_t t9 : 1; > - uint32_t t8 : 1; > - uint32_t th : 1; > - uint32_t so : 1; > - uint32_t len : 10; > - uint32_t time : 11; > - }; > - uint32_t value; > -}; > - > static void hisi_ptt_print_pkt(const unsigned char *buf, int pos, const char *desc) > { > const char *color = PERF_COLOR_BLUE; > + uint32_t value; > + uint8_t byte; > int i; > > + value = le32_to_cpu(*(__le32 *)(buf + pos)); > printf("."); > color_fprintf(stdout, color, " %08x: ", pos); > - for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) > - color_fprintf(stdout, color, "%02x ", buf[pos + i]); > + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) { > + byte = (value >> (24 - i * 8)) & 0xFF; > + color_fprintf(stdout, color, "%02x ", byte); > + } > for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) > color_fprintf(stdout, color, " "); > color_fprintf(stdout, color, " %s\n", desc); > @@ -122,22 +114,30 @@ static int hisi_ptt_8dw_pkt_desc(const unsigned char *buf, int pos) > static void hisi_ptt_4dw_print_dw0(const unsigned char *buf, int pos) > { > const char *color = PERF_COLOR_BLUE; > - union hisi_ptt_4dw dw0; > + uint8_t byte; > + uint32_t dw; > int i; > > - dw0.value = *(uint32_t *)(buf + pos); > + dw = le32_to_cpu(*(__le32 *)(buf + pos)); Sashiko and the comment in generic.h say to use get_unaligned_le32() for unaligned data. It looks like this always is aligned but it's not enforced by the function, so to avoid anyone wondering if it's correct I would just use get_unaligned_le32(). With that: Reviewed-by: James Clark <james.clark@linaro.org> Sashiko also says there is another endianess issue in hisi_ptt_check_packet_type(). Up to you whether you want to fix it or not: https://sashiko.dev/#/patchset/20260729060220.624250-1-liusizhe5%40huawei.com > printf("."); > color_fprintf(stdout, color, " %08x: ", pos); > - for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) > - color_fprintf(stdout, color, "%02x ", buf[pos + i]); > + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) { > + byte = (dw >> (24 - i * 8)) & 0xFF; > + color_fprintf(stdout, color, "%02x ", byte); > + } > for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) > color_fprintf(stdout, color, " "); > > color_fprintf(stdout, color, > " %s %x %s %x %s %x %s %x %s %x %s %x %s %x %s %x\n", > - "Format", dw0.format, "Type", dw0.type, "T9", dw0.t9, > - "T8", dw0.t8, "TH", dw0.th, "SO", dw0.so, "Length", > - dw0.len, "Time", dw0.time); > + "Format", FIELD_GET(HISI_PTT_HEAD0_4DW_FORMAT, dw), > + "Type", FIELD_GET(HISI_PTT_HEAD0_4DW_TYPE, dw), > + "T9", FIELD_GET(HISI_PTT_HEAD0_4DW_T9, dw), > + "T8", FIELD_GET(HISI_PTT_HEAD0_4DW_T8, dw), > + "TH", FIELD_GET(HISI_PTT_HEAD0_4DW_TH, dw), > + "SO", FIELD_GET(HISI_PTT_HEAD0_4DW_SO, dw), > + "Length", FIELD_GET(HISI_PTT_HEAD0_4DW_LEN, dw), > + "Time", FIELD_GET(HISI_PTT_HEAD0_4DW_TIME, dw)); > } > > static int hisi_ptt_4dw_pkt_desc(const unsigned char *buf, int pos) > diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h > index 6772b16b817b..3b349fcc24ae 100644 > --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h > +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h > @@ -9,12 +9,24 @@ > > #include <stddef.h> > #include <stdint.h> > +#include <linux/bits.h> > +#include <linux/bitfield.h> > > #define HISI_PTT_8DW_CHECK_MASK GENMASK(31, 11) > #define HISI_PTT_IS_8DW_PKT GENMASK(31, 11) > #define HISI_PTT_MAX_SPACE_LEN 10 > #define HISI_PTT_FIELD_LENGTH 4 > > +/* Header DW0 fields for 4DW format */ > +#define HISI_PTT_HEAD0_4DW_TIME GENMASK_U32(10, 0) > +#define HISI_PTT_HEAD0_4DW_LEN GENMASK_U32(20, 11) > +#define HISI_PTT_HEAD0_4DW_SO BIT_U32(21) > +#define HISI_PTT_HEAD0_4DW_TH BIT_U32(22) > +#define HISI_PTT_HEAD0_4DW_T8 BIT_U32(23) > +#define HISI_PTT_HEAD0_4DW_T9 BIT_U32(24) > +#define HISI_PTT_HEAD0_4DW_TYPE GENMASK_U32(29, 25) > +#define HISI_PTT_HEAD0_4DW_FORMAT GENMASK_U32(31, 30) > + > enum hisi_ptt_pkt_type { > HISI_PTT_4DW_PKT, > HISI_PTT_8DW_PKT, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing 2026-07-29 10:16 ` James Clark @ 2026-07-29 12:06 ` Sizhe Liu 0 siblings, 0 replies; 8+ messages in thread From: Sizhe Liu @ 2026-07-29 12:06 UTC (permalink / raw) To: James Clark Cc: linux-kernel, linux-pci, linux-perf-users, linux-arm-kernel, linux-doc, linuxarm, prime.zeng, wuyifan50, rostedt, mhiramat, mathieu.desnoyers, corbet, skhan, bhelgaas, yangyccccc, jic23, john.g.garry, will, mike.leach, leo.yan, peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter, wangyushan12 On 2026/7/29 18:16, James Clark wrote: > > > On 29/07/2026 7:02 am, Sizhe Liu wrote: >> TLP Headers traced by HiSilicon PCIe tune and trace device (PTT) in >> 4DW format are shown in the document as below: >> bits [31:30] [ 29:25 ][24][23][22][21][ 20:11 ][ 10:0 ] >> |-----|---------|---|---|---|---|-------------|-------------| >> DW0 [ Fmt ][ Type ][T9][T8][TH][SO][ Length ][ Time ] >> DW1 [ Header DW1 ] >> DW2 [ Header DW2 ] >> DW3 [ Header DW3 ] >> >> Problem: >> The DW0 bit field layout of the hisi_ptt_4dw union does not match the >> actual bit ordering in little-endian memory, causing incorrect field >> decoding. >> >> Test on Kunpeng 930 SOC, generating data flow with `iperf` commands: >> - server side: >> iperf -s >> - client side: >> iperf -c $ip_addr -t 30 >> >> Trace the TLP headers with hisi_ptt on server side at the same time: >> perf record -e >> hisi_ptt12_0/type=4,filter=0x05101,direction=2,format=0/ \ >> --max-size 50M -o perf.data & >> The trace aims to capture completion TLPs, learn more in the document: >> https://docs.kernel.org/trace/hisi-ptt.html >> >> Decode perf.data with hisi_ptt decoder: >> perf report -D >> >> The hisi_ptt decoder produces the following result: >> [...perf headers and other information] >> . ... HISI PTT data: size 8388608 bytes >> . 00000000: 68 87 20 94 Format 3 >> Type 1a T9 0 T8 1 TH 1 SO 1 Length 10 Time 4a1 >> . 00000004: 40 00 00 00 Header DW1 >> . 00000008: 40 00 01 51 Header DW2 >> . 0000000c: 00 00 00 00 Header DW3 >> [...other hisi_ptt TLP headers] >> >> According to PCIe r5.0 2.2.1, the Fmt & Type of Cpl/CplD is supposed >> to be >> 8b'00001010' / 8b'01001010' >> However, the Format & Type decoder analyzing result is 8b'01111010'. >> It does not match field encodings of any TLP messages. >> >> Correct decoder result should be: >> [...perf headers and other information] >> . ... HISI PTT data: size 8388608 bytes >> . 00000000: 94 20 87 68 Format 2 >> Type a T9 0 T8 0 TH 0 SO 1 Length 10 Time 768 >> . 00000004: 00 00 00 40 Header DW1 >> . 00000008: 51 01 00 40 Header DW2 >> . 0000000c: 00 00 00 00 Header DW3 >> [...other hisi_ptt TLP headers] >> >> To solve the problem: >> 1. Drop the union and C bitfield struct, store the raw DW value in >> a plain uint32_t, and extract the fields with FIELD_GET() against >> GENMASK/BIT masks declared in the header so they can be reused by >> other translation units. The masks are portable across endianness and >> compilers. >> >> 2. Print all DW hex values in big-endian byte order for readability, >> matching the bit field layout shown in the 4DW format diagram. >> >> Cc: stable@vger.kernel.org >> Fixes: 5e91e57e6809 ("perf auxtrace arm64: Add support for parsing >> HiSilicon PCIe Trace packet") >> Signed-off-by: Sizhe Liu <liusizhe5@huawei.com> >> --- >> Documentation/trace/hisi-ptt.rst | 28 +++++------ >> .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.c | 46 +++++++++---------- >> .../hisi-ptt-decoder/hisi-ptt-pkt-decoder.h | 12 +++++ >> 3 files changed, 49 insertions(+), 37 deletions(-) >> >> diff --git a/Documentation/trace/hisi-ptt.rst >> b/Documentation/trace/hisi-ptt.rst >> index 6eef28ebb0c7..f6a2655f99e5 100644 >> --- a/Documentation/trace/hisi-ptt.rst >> +++ b/Documentation/trace/hisi-ptt.rst >> @@ -285,20 +285,20 @@ according to the format described previously >> (take 8DW as an example): >> [...perf headers and other information] >> . ... HISI PTT data: size 4194304 bytes >> . 00000000: 00 00 00 00 Prefix >> - . 00000004: 01 00 00 60 Header DW0 >> - . 00000008: 0f 1e 00 01 Header DW1 >> - . 0000000c: 04 00 00 00 Header DW2 >> - . 00000010: 40 00 81 02 Header DW3 >> - . 00000014: 33 c0 04 00 Time >> + . 00000004: 60 00 00 01 Header DW0 >> + . 00000008: 01 00 1e 0f Header DW1 >> + . 0000000c: 00 00 00 04 Header DW2 >> + . 00000010: 02 81 00 40 Header DW3 >> + . 00000014: 00 04 c0 33 Time >> . 00000020: 00 00 00 00 Prefix >> - . 00000024: 01 00 00 60 Header DW0 >> - . 00000028: 0f 1e 00 01 Header DW1 >> - . 0000002c: 04 00 00 00 Header DW2 >> - . 00000030: 40 00 81 02 Header DW3 >> - . 00000034: 02 00 00 00 Time >> + . 00000024: 60 00 00 01 Header DW0 >> + . 00000028: 01 00 1e 0f Header DW1 >> + . 0000002c: 00 00 00 04 Header DW2 >> + . 00000030: 02 81 00 40 Header DW3 >> + . 00000034: 00 00 00 02 Time >> . 00000040: 00 00 00 00 Prefix >> - . 00000044: 01 00 00 60 Header DW0 >> - . 00000048: 0f 1e 00 01 Header DW1 >> - . 0000004c: 04 00 00 00 Header DW2 >> - . 00000050: 40 00 81 02 Header DW3 >> + . 00000044: 60 00 00 01 Header DW0 >> + . 00000048: 01 00 1e 0f Header DW1 >> + . 0000004c: 00 00 00 04 Header DW2 >> + . 00000050: 02 81 00 40 Header DW3 >> [...] >> diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c >> b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c >> index c48b2ce7c4a3..c1e169561087 100644 >> --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c >> +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.c >> @@ -10,6 +10,7 @@ >> #include <endian.h> >> #include <byteswap.h> >> #include <linux/bitops.h> >> +#include <linux/kernel.h> >> #include <stdarg.h> >> #include "../color.h" >> @@ -73,29 +74,20 @@ static const char * const >> hisi_ptt_4dw_pkt_field_name[] = { >> [HISI_PTT_4DW_HEAD3] = "Header DW3", >> }; >> -union hisi_ptt_4dw { >> - struct { >> - uint32_t format : 2; >> - uint32_t type : 5; >> - uint32_t t9 : 1; >> - uint32_t t8 : 1; >> - uint32_t th : 1; >> - uint32_t so : 1; >> - uint32_t len : 10; >> - uint32_t time : 11; >> - }; >> - uint32_t value; >> -}; >> - >> static void hisi_ptt_print_pkt(const unsigned char *buf, int pos, >> const char *desc) >> { >> const char *color = PERF_COLOR_BLUE; >> + uint32_t value; >> + uint8_t byte; >> int i; >> + value = le32_to_cpu(*(__le32 *)(buf + pos)); >> printf("."); >> color_fprintf(stdout, color, " %08x: ", pos); >> - for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) >> - color_fprintf(stdout, color, "%02x ", buf[pos + i]); >> + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) { >> + byte = (value >> (24 - i * 8)) & 0xFF; >> + color_fprintf(stdout, color, "%02x ", byte); >> + } >> for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) >> color_fprintf(stdout, color, " "); >> color_fprintf(stdout, color, " %s\n", desc); >> @@ -122,22 +114,30 @@ static int hisi_ptt_8dw_pkt_desc(const unsigned >> char *buf, int pos) >> static void hisi_ptt_4dw_print_dw0(const unsigned char *buf, int pos) >> { >> const char *color = PERF_COLOR_BLUE; >> - union hisi_ptt_4dw dw0; >> + uint8_t byte; >> + uint32_t dw; >> int i; >> - dw0.value = *(uint32_t *)(buf + pos); >> + dw = le32_to_cpu(*(__le32 *)(buf + pos)); > > Sashiko and the comment in generic.h say to use get_unaligned_le32() > for unaligned data. It looks like this always is aligned but it's not > enforced by the function, so to avoid anyone wondering if it's correct > I would just use get_unaligned_le32(). > > With that: > > Reviewed-by: James Clark <james.clark@linaro.org> > > Sashiko also says there is another endianess issue in > hisi_ptt_check_packet_type(). Up to you whether you want to fix it or > not: > https://sashiko.dev/#/patchset/20260729060220.624250-1-liusizhe5%40huawei.com > Thanks for the review and the Reviewed-by tag. I agree with using get_unaligned_le32(), to make it more robust. I'am updating it in the next version along with other endianess issuses. Regards, Sizhe >> printf("."); >> color_fprintf(stdout, color, " %08x: ", pos); >> - for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) >> - color_fprintf(stdout, color, "%02x ", buf[pos + i]); >> + for (i = 0; i < HISI_PTT_FIELD_LENGTH; i++) { >> + byte = (dw >> (24 - i * 8)) & 0xFF; >> + color_fprintf(stdout, color, "%02x ", byte); >> + } >> for (i = 0; i < HISI_PTT_MAX_SPACE_LEN; i++) >> color_fprintf(stdout, color, " "); >> color_fprintf(stdout, color, >> " %s %x %s %x %s %x %s %x %s %x %s %x %s %x %s %x\n", >> - "Format", dw0.format, "Type", dw0.type, "T9", dw0.t9, >> - "T8", dw0.t8, "TH", dw0.th, "SO", dw0.so, "Length", >> - dw0.len, "Time", dw0.time); >> + "Format", FIELD_GET(HISI_PTT_HEAD0_4DW_FORMAT, dw), >> + "Type", FIELD_GET(HISI_PTT_HEAD0_4DW_TYPE, dw), >> + "T9", FIELD_GET(HISI_PTT_HEAD0_4DW_T9, dw), >> + "T8", FIELD_GET(HISI_PTT_HEAD0_4DW_T8, dw), >> + "TH", FIELD_GET(HISI_PTT_HEAD0_4DW_TH, dw), >> + "SO", FIELD_GET(HISI_PTT_HEAD0_4DW_SO, dw), >> + "Length", FIELD_GET(HISI_PTT_HEAD0_4DW_LEN, dw), >> + "Time", FIELD_GET(HISI_PTT_HEAD0_4DW_TIME, dw)); >> } >> static int hisi_ptt_4dw_pkt_desc(const unsigned char *buf, int pos) >> diff --git a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h >> b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h >> index 6772b16b817b..3b349fcc24ae 100644 >> --- a/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h >> +++ b/tools/perf/util/hisi-ptt-decoder/hisi-ptt-pkt-decoder.h >> @@ -9,12 +9,24 @@ >> #include <stddef.h> >> #include <stdint.h> >> +#include <linux/bits.h> >> +#include <linux/bitfield.h> >> #define HISI_PTT_8DW_CHECK_MASK GENMASK(31, 11) >> #define HISI_PTT_IS_8DW_PKT GENMASK(31, 11) >> #define HISI_PTT_MAX_SPACE_LEN 10 >> #define HISI_PTT_FIELD_LENGTH 4 >> +/* Header DW0 fields for 4DW format */ >> +#define HISI_PTT_HEAD0_4DW_TIME GENMASK_U32(10, 0) >> +#define HISI_PTT_HEAD0_4DW_LEN GENMASK_U32(20, 11) >> +#define HISI_PTT_HEAD0_4DW_SO BIT_U32(21) >> +#define HISI_PTT_HEAD0_4DW_TH BIT_U32(22) >> +#define HISI_PTT_HEAD0_4DW_T8 BIT_U32(23) >> +#define HISI_PTT_HEAD0_4DW_T9 BIT_U32(24) >> +#define HISI_PTT_HEAD0_4DW_TYPE GENMASK_U32(29, 25) >> +#define HISI_PTT_HEAD0_4DW_FORMAT GENMASK_U32(31, 30) >> + >> enum hisi_ptt_pkt_type { >> HISI_PTT_4DW_PKT, >> HISI_PTT_8DW_PKT, > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-07-29 12:06 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-29 6:02 [PATCH 0/2] perf hisi-ptt: Fix TLP header parsing and naming Sizhe Liu 2026-07-29 6:02 ` [PATCH 1/2] perf hisi-ptt: Fix spelling and abbreviation errors Sizhe Liu 2026-07-29 6:10 ` sashiko-bot 2026-07-29 10:17 ` James Clark 2026-07-29 6:02 ` [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing Sizhe Liu 2026-07-29 6:20 ` sashiko-bot 2026-07-29 10:16 ` James Clark 2026-07-29 12:06 ` Sizhe Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox