Re: [PATCH 1/5] Add CPER logging support for CRI-Do not review

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe/ras: unchecked firmware operation_status in get_info_queue_data()
- [Medium] drm/xe/ras: missing return on invalid first chunk in xe_ras_drain_info_queue_raw()
--

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

Add CPER logging support for CRI-Do not review

This commit adds UEFI CPER error record generation and tracepoint logging
for Intel Xe RAS hardware errors and counter queries, including draining
extended error info queues from system controller firmware and exposing a
cper_on_query sysfs toggle.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 9c15ce8f0b6a8..afd41b7e9e86c 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c

[ ... ]

> @@ -1016,6 +1086,123 @@ int xe_ras_set_threshold(struct xe_device *xe, u8 severity, u8 component, u32 th
>  	return 0;
>  }
>  
> +static int get_info_queue_data(struct xe_device *xe,
> +			       struct xe_ras_get_info_queue_request *req,
> +			       struct xe_ras_get_info_queue_response *out)
> +{
> +	struct xe_ras_get_info_queue_response response = {0};
> +	struct xe_sysctrl_mailbox_command command = {0};
> +	const struct xe_ras_info_queue_header *hdr;
> +	size_t rlen;
> +	int ret;
> +
> +	xe_sysctrl_create_command(&command, XE_SYSCTRL_GROUP_GFSP, XE_SYSCTRL_CMD_GET_INFO_QUEUE,
> +				  req, sizeof(*req), &response, sizeof(response));
> +
> +	ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
> +	if (ret) {
> +		xe_err(xe, "sysctrl: failed to get info queue data %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (rlen != sizeof(response)) {
> +		xe_err(xe, "sysctrl: unexpected get info queue data response length %zu (expected %zu)\n",
> +		       rlen, sizeof(response));
> +		return -EIO;
> +	}
> +
> +	hdr = &response.queue_response.header;
> +	xe_dbg(xe, "[RAS]: info queue data: status=%u chunk_size=%u flags=0x%x\n",
> +	       response.operation_status, hdr->chunk_size, hdr->flags);
> +
> +	*out = response;
> +	return 0;
> +}

[Severity: Medium]
In drivers/gpu/drm/xe/xe_ras.c, get_info_queue_data() logs
response.operation_status (defined in struct xe_ras_get_info_queue_response
in drivers/gpu/drm/xe/xe_ras_types.h) via xe_dbg(), but never validates it
with ras_status_to_errno() before copying response to *out and returning 0.

Other command handlers in drivers/gpu/drm/xe/xe_ras.c that receive a status
field in their response structure (send_page_offline_cmd(),
xe_ras_clear_counter(), xe_ras_set_threshold(), and gpu_health_store())
convert the status via ras_status_to_errno() and return a negative error
code when it is non-zero.

When draining multi-chunk info queues via:

  xe_cper_emit_counter()
    -> xe_prepare_cper_error_info()
      -> xe_ras_drain_info_queue_raw()
        -> get_info_queue_data()

if firmware returns a non-zero error code in response.operation_status while
the mailbox transport succeeds, should get_info_queue_data() check
response.operation_status with ras_status_to_errno() so callers do not
parse a failed or unpopulated info-queue response chunk?

> +
> +static bool chunk_is_valid(const struct xe_ras_info_queue_header *hdr,
> +			   u32 expected_offset, u32 dest_size)
> +{
> +	u32 end;
> +
> +	return hdr->chunk_offset == expected_offset &&
> +	       hdr->chunk_size && hdr->chunk_size <= XE_RAS_INFO_QUEUE_MAX_CHUNK_SIZE &&
> +	       !check_add_overflow(hdr->chunk_offset, hdr->chunk_size, &end) &&
> +	       end <= XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE && end <= dest_size;
> +}
> +
> +/**
> + * xe_ras_drain_info_queue_raw() - Drain the full RAS info queue into a flat buffer
> + * @xe: xe device
> + * @counter_resp: counter response carrying the first embedded chunk and the
> + *                counter identifier used as the source context for subsequent
> + *                GET_INFO_QUEUE fetches
> + * @dest_buf: destination buffer supplied by the caller
> + * @dest_size: size of @dest_buf in bytes; also caps the total amount of data
> + *             assembled from the info queue
> + *
> + * Copies the first chunk already embedded in @counter_resp, then loops
> + * issuing GET_INFO_QUEUE to fetch any remaining chunks until the queue
> + * signals no more data or a transport/bounds error is encountered.  On
> + * transport or bounds errors the function stops and returns whatever has
> + * been assembled so far.
> + *
> + * Returns: number of valid bytes written into @dest_buf.  Zero if @dest_buf is
> + *          NULL or @dest_size is 0.
> + */
> +u32 xe_ras_drain_info_queue_raw(struct xe_device *xe,
> +				const struct xe_ras_get_counter_response *counter_resp,
> +				u8 *dest_buf, u32 dest_size)
> +{
> +	const struct xe_ras_info_queue_header *first_hdr = &counter_resp->first.header;
> +	struct xe_ras_get_info_queue_request req = {0};
> +	struct xe_ras_get_info_queue_response resp;
> +	u32 flags = first_hdr->flags;
> +	u32 offset = 0;
> +
> +	if (!dest_buf || !dest_size)
> +		return 0;
> +
> +	if (first_hdr->total_size > dest_size)
> +		xe_warn(xe, "[RAS]: info queue size %u exceeds %u, truncating\n",
> +			first_hdr->total_size, dest_size);
> +
> +	/* Copy first chunk already embedded in the counter response */
> +	if (chunk_is_valid(first_hdr, offset, dest_size)) {
> +		memcpy(dest_buf, counter_resp->first.data, first_hdr->chunk_size);
> +		offset = first_hdr->chunk_size;
> +	}

[Severity: Medium]
In drivers/gpu/drm/xe/xe_ras.c, when xe_ras_drain_info_queue_raw() is called
from xe_cper_emit_counter() -> xe_prepare_cper_error_info() with
counter_resp->has_info_queue set, what happens if the first embedded chunk
in counter_resp->first fails chunk_is_valid(first_hdr, offset, dest_size)
(for example, when first_hdr->chunk_offset is non-zero, first_hdr->chunk_size
is 0, or first_hdr->chunk_size exceeds dest_size)?

Here xe_ras_drain_info_queue_raw() skips copying the first chunk without
logging a warning or returning 0, leaving offset at 0 while keeping flags
initialized from the invalid first_hdr->flags. If the more-data flag bit is
set in flags, execution falls through into the while loop below and issues a
follow-up info queue fetch starting at offset 0.

Should a bounds or header validation failure on the first chunk log a
warning and return 0 immediately, matching the !chunk_is_valid() handling
inside the while loop below?

> +
> +	req.source_command = XE_SYSCTRL_CMD_GET_COUNTER;
> +	req.source_context = counter_resp->counter;
> +	req.queue_request.session_id = counter_resp->counter;
> +
> +	/* Fetch any remaining chunks */
> +	while ((flags & XE_RAS_INFO_QUEUE_FLAG_MORE_DATA) && offset < dest_size) {
> +		const struct xe_ras_info_queue_header *hdr = &resp.queue_response.header;
> +
> +		req.queue_request.offset = offset;
> +		req.queue_request.size = min(XE_RAS_INFO_QUEUE_MAX_CHUNK_SIZE, dest_size - offset);
> +
> +		if (get_info_queue_data(xe, &req, &resp)) {
> +			xe_err(xe, "[RAS]: info queue drain aborted: fetch at offset=%u failed\n",
> +			       offset);
> +			return offset;
> +		}
> +
> +		if (!chunk_is_valid(hdr, offset, dest_size)) {
> +			xe_warn(xe, "[RAS]: invalid info queue chunk (offset=%u size=%u, expected offset=%u)\n",
> +				hdr->chunk_offset, hdr->chunk_size, offset);
> +			return offset;
> +		}
> +
> +		memcpy(dest_buf + offset, resp.queue_response.data, hdr->chunk_size);
> +		offset += hdr->chunk_size;
> +		flags = hdr->flags;
> +	}
> +
> +	return min(offset, first_hdr->total_size);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021162508.265700-7-mallesh.koujalagi@intel.com?part=1