Re: [PATCH 2/2] perf hisi-ptt: Fix PTT trace TLP header parsing

[Sizhe Liu <liusizhe5@huawei.com>]

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