Re: [PATCH 5/5] drm/xe/xe_ras: Track offlined pages by firmware to avoid duplicates

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Using PAGE_SHIFT instead of a GPU-specific page shift causes multiple 4KB GPU pages to map to the same XArray index on systems with larger CPU pages.
--

--- Patch [5]: [PATCH 5/5] drm/xe/xe_ras: Track offlined pages by firmware to avoid duplicates ---
commit a03c3279356ffe1e7132d6c4d017e5f44ae3cc0b
Author: Riana Tauro <riana.tauro@intel.com>

drm/xe/xe_ras: Track offlined pages by firmware to avoid duplicates

This commit adds an XArray to track pages that have been offlined by the
firmware to prevent issuing redundant page offline requests for the same
memory address.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 5303d1a4e485a..f0dac06f050b9 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c

[ ... ]

> @@ -244,6 +248,10 @@ static int send_page_offline_cmd(struct xe_device *xe, u64 page_address,
>  		return ret;
>  	}
>  
> +	if (action == XE_RAS_PAGE_ACTION_OFFLINE)
> +		xa_store(&ras->offlined_pages, page_address >> PAGE_SHIFT,
> +			 XE_RAS_PAGE_OFFLINED, GFP_KERNEL);
> +
>  	return ret;
>  }

[Severity: High]
Does using PAGE_SHIFT here cause aliasing on systems with larger CPU page
sizes?

If the CPU page size is larger than the GPU page size (for example, 64KB on
ARM64 vs 4KB XE_PAGE_SIZE), shifting by PAGE_SHIFT will truncate the lower
bits of the GPU physical address. This would cause multiple distinct 4KB GPU
pages to map to the exact same XArray index. Should this use a GPU-specific
page shift instead?

> @@ -287,7 +295,11 @@ static int handle_page_offline(struct xe_device *xe, u64 page_address, bool send
>  			   "Physical page address belongs to critical BO: 0x%llx\n", page_address);
>  		return ret;
>  	case -EEXIST:
> -		action = XE_RAS_PAGE_ACTION_OFFLINE;
> +		if (xa_load(&xe->ras.offlined_pages, page_address >> PAGE_SHIFT))
> +			action = XE_RAS_PAGE_ACTION_DECLINE;
> +		else
> +			action = XE_RAS_PAGE_ACTION_OFFLINE;
> +

[Severity: High]
If aliasing occurs due to PAGE_SHIFT on 64KB CPU page architectures, could
this xa_load() falsely return a match?

If a subsequent ECC error occurs in a different 4KB GPU page but within the
same 64KB CPU page block, it appears this would return a false match. The
driver would then incorrectly decline the firmware's request to offline the
second page, allowing the system to continue using poisoned memory.

[ ... ]

> @@ -409,8 +421,11 @@ static void get_offlined_list(struct xe_device *xe)
>  			return;
>  		}
>  
> -		for (i = 0; i < response.pages_returned && i < XE_RAS_NUM_PAGES; i++)
> +		for (i = 0; i < response.pages_returned && i < XE_RAS_NUM_PAGES; i++) {
>  			handle_page_offline(xe, response.page_addresses[i], false);
> +			xa_store(&ras->offlined_pages, response.page_addresses[i] >> PAGE_SHIFT,
> +				 XE_RAS_PAGE_OFFLINED, GFP_KERNEL);
> +		}

[Severity: High]
Similarly, syncing the initial list of offlined pages from firmware uses
PAGE_SHIFT here, which might suffer from the same address truncation issue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825063615.3697317-7-riana.tauro@intel.com?part=5