Re: [PATCH v2 6/6] 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] Incorrect bit shift used to calculate XArray index for device physical addresses.
--

commit cb65d57e28d4e4a8f1d6464c1a918d7392d5e460
Author: Riana Tauro <riana.tauro@intel.com>

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

This commit introduces tracking for pages successfully offlined by the
firmware using an XArray. This prevents the memory scrubber from issuing
redundant page offline requests for the same address.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -244,11 +248,16 @@ static int send_page_offline_cmd(struct xe_device *xe, u64 page_address,
>  		return ret;
>  	}
>  
> +	if (action == XE_RAS_PAGE_ACTION_OFFLINE)
> +		xa_store(&state->offlined_pages, page_address >> PAGE_SHIFT,
> +			 XE_RAS_PAGE_OFFLINED, GFP_KERNEL);

[Severity: High]
Does this code use the correct page shift for device physical addresses?

PAGE_SHIFT is the host's page shift, which could be 64K depending on the
kernel build, whereas the device page size is likely 4K. If the host uses 64K
pages, shifting by PAGE_SHIFT would truncate the lower bits of the device
address, causing multiple distinct 4K device pages to hash to the exact same
XArray index.

> +
>  	return ret;
>  }
>  
>  static int handle_page_offline(struct xe_device *xe, u64 page_address, bool send_cmd)
>  {
> +	struct xe_ras_state *state = &xe->ras.state;
>  	enum xe_ras_page_action action;
>  	int ret = 0;
>  
[ ... ]
> @@ -288,7 +297,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(&state->offlined_pages, page_address >> PAGE_SHIFT))

[Severity: High]
Will this check cause false positives on non-4K kernel page builds?

If a hardware memory error occurs on a different 4K device page that falls
within the same 64K host page as a previously offlined page, this lookup
will find the existing entry. The driver might then mistakenly conclude this
specific device page is already offlined and skip sending the command to the
firmware.

> +			action = XE_RAS_PAGE_ACTION_DECLINE;
> +		else
> +			action = XE_RAS_PAGE_ACTION_OFFLINE;
> +
>  		xe_log_err(xe, DEVICE_MEMORY, ret,
>  			   "Double-bit ECC error detected at physical address 0x%llx, page already software offlined\n",
>  			   page_address);
[ ... ]
> @@ -410,8 +423,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(&state->offlined_pages, response.page_addresses[i] >> PAGE_SHIFT,
> +				 XE_RAS_PAGE_OFFLINED, GFP_KERNEL);

[Severity: High]
Could this initialization also suffer from the same device vs. host page shift
mismatch?

Using PAGE_SHIFT here would populate the cache with indices that may group
multiple device pages into a single host page index.

> +		}
>  
>  		if (!state->max_pages)
>  			state->max_pages = response.max_entries;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907094706.1407436-8-riana.tauro@intel.com?part=6