Re: [PATCH v2 03/11] drm/xe/cper: Add CPER structures and trace event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] CPER structures use native integer types and C bitfields for a standardized external binary format, causing ABI corruption on non-little-endian architectures.
- [Medium] Tracepoint uses `%pU` to print `guid_t` fields, incorrectly formatting them as big-endian.
--

commit ad510a6c4b7008ac7d33a168936259e5f8400da7
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/cper: Add CPER structures and trace event

Define packed data structures and Intel-specific GUID macros needed
to build Intel GPU CPER (Common Platform Error Record) non-standard
records.

> diff --git a/drivers/gpu/drm/xe/xe_cper_types.h b/drivers/gpu/drm/xe/xe_cper_types.h
> new file mode 100644
> index 0000000000000..82167ea4eb161
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cper_types.h
[ ... ]
> +struct xe_cper_sec_intel_err_hdr {
> +	/** @error_class: Error classification (type, component, location, cause) */
> +	union {
> +		struct {
> +			/** @error_class.error_type: RAS error severity */
> +			u8  error_type;
> +			/** @error_class.error_component: IP block that raised the error */
> +			u8  error_component;
> +			/** @error_class.tile: Tile number */
> +			u8  tile;
> +			/** @error_class.instance: Instance within the tile */
> +			u32 instance;
> +			/** @error_class.cause: Error cause code */
> +			u32 cause;
> +			/** @error_class.reserved: Reserved, must be zero */
> +			u8  reserved;
> +		} error_class;
> +		/** @class: Raw byte view of the error class */
> +		u8 class[12];
> +	};
> +	/** @first_timestamp: Timestamp of the first occurrence of this error class */
> +	u64 first_timestamp;

[Severity: Medium]
Can this native u64 type break the CPER binary layout on big-endian 
architectures?

The CPER standard dictates a strict binary layout and requires multi-byte fields
to be little-endian. Using architecture-dependent native integer types like u64
causes the struct's binary representation to vary based on host endianness,
which can produce corrupted CPER records on big-endian architectures.

Should this use explicitly sized and endian-specific types like __le64?

> +	/** @sig_id: Aggregated error class SIG ID; set to U32_MAX if unknown */
> +	u32 sig_id;
> +	/** @error_count: Number of times this error has been observed */
> +	u32 error_count;
> +	/** @valid_bits: Bitmask indicating which header fields are populated */
> +	union {
> +		struct {
> +			/** @valid_bits.location: @error_class field is valid */
> +			u16 location		: 1;

[Severity: Medium]
Will this compiler-dependent bitfield cause ABI corruption?

Since bitfield layout is compiler- and architecture-dependent, it breaks the
exact bit-level binary layout required for the CPER standard. 

Should this be handled with standard bitwise operations instead?

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_trace_cper.h b/drivers/gpu/drm/xe/xe_trace_cper.h
> new file mode 100644
> index 0000000000000..6d2dbf5048884
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_trace_cper.h
[ ... ]
> +	TP_printk("dev=%s severity=%d platform_id=%pU fru_id=%pU "

[Severity: Medium]
Does this tracepoint format guid_t fields incorrectly?

The printk format specifier %pU defaults to %pUb, which formats UUIDs in
big-endian byte order (RFC 4122). However, guid_t represents little-endian
UUIDs.

Using %pU instead of %pUl causes the first three components of the GUID to be
incorrectly byte-swapped in the trace output, presenting a corrupted GUID to
userspace consumers.

> +		"intel_err_hdr_raw=%s cper_len=%u cper_raw=%s",
> +		__get_str(dev), __entry->sev,
> +		__entry->platform_id, __entry->fru_id,
> +		__print_hex(__entry->ihdr_raw,
> +		    sizeof(struct xe_cper_sec_intel_err_hdr)),
> +		__entry->cper_len,
> +		__print_hex(__get_dynamic_array(cper),
> +		    __entry->cper_len))
> +);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825175916.1103841-13-badal.nilawar@intel.com?part=3