Re: [PATCH 09/10] perf hisi-ptt: Add field-level parsing for header DW2/DW3

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Host-endian-dependent C bitfields break cross-platform trace analysis on big-endian hosts.
--

commit b5a1a94b81406195d7c90b32fd8a96a3d26602c2
Author: Sizhe Liu <liusizhe5@huawei.com>

perf hisi-ptt: Add field-level parsing for header DW2/DW3

This commit adds detailed field parsing for TLP header DW2 and DW3 based on
the message type parsed from DW0. It aims to provide more structured
information for users analyzing PTT trace data.

> 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 59ab8ec3a03da..46f11d5719aca 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
> @@ -127,6 +142,45 @@ union hisi_ptt_field_data {
>  		uint32_t type : 5;
>  		uint32_t format : 3;
>  	} dw0_8dw;
> +	/*
> +	 * Header DW2 for MWr/Msg/MsgD/FetchAdd/Swap/CAS/IORd/IOWr TLPs.
> +	 * Affects both 4DW and 8DW format.
> +	 */
> +	struct {
> +		uint32_t header_dw2 : 16;
> +		uint32_t tag : 4;
> +		uint32_t t : 1;
> +		uint32_t tv : 1;
> +		uint32_t rsv : 1;
> +		uint32_t request_segment : 8;
> +		uint32_t reserved : 1;
> +	} dw2_mixed;

[Severity: Medium]
Does using C bitfields here create an issue for big-endian hosts?

While le32_to_cpu() correctly handles the byte order of the raw integer,
the layout of C bitfields is implementation-defined. On big-endian systems,
won't the compiler pack these fields starting from the most significant
bit instead of the least significant bit?

> +	/*
> +	 * Header DW3 for CfgRd0/CfgWr0/CfgRd1/CfgWr1 TLPs.
> +	 * Affects both 4DW and 8DW format.
> +	 */
[ ... ]
> @@ -211,6 +265,85 @@ static void hisi_ptt_print_head0(struct hisi_ptt_pkt_buf *pkt_buf)
[ ... ]
> +static void hisi_ptt_print_head2(struct hisi_ptt_pkt_buf *pkt_buf)
> +{
> +	const char *color = PERF_COLOR_BLUE;
> +	union hisi_ptt_field_data dw;
> +
> +	dw.value = le32_to_cpu(*(__le32 *)(pkt_buf->buf + pkt_buf->pos));
> +	hisi_ptt_print_raw_record(pkt_buf->pos, dw.value);
> +
> +	if (pkt_buf->pkt_msg_type == HISI_PTT_PKT_TYPE_MWR ||
> +	    pkt_buf->pkt_msg_type == HISI_PTT_PKT_TYPE_MSG ||
> +	    pkt_buf->pkt_msg_type == HISI_PTT_PKT_TYPE_ATOM ||
> +	    pkt_buf->pkt_msg_type == HISI_PTT_PKT_TYPE_IO)
> +		color_fprintf(stdout, color,
> +			      "  %s %x %s %x %s %x %s %x %s %x %s %x %s %x\n",
> +			      "Reserved", dw.dw2_mixed.reserved,
> +			      "Request Segment", dw.dw2_mixed.request_segment,

[Severity: Medium]
Will this result in extracting entirely incorrect bits when a big-endian
host processes an ARM perf.data trace file?

Since perf tools are expected to support cross-platform trace analysis,
could we use bitwise operations instead of C bitfields to ensure the fields
are reliably extracted regardless of the host architecture's endianness?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604075005.2219785-1-liusizhe5@huawei.com?part=9