Re: [PATCH v4 3/4] drm/xe/xe_migrate: Optimize unaligned access_memory copies

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] xe_migrate: Integer overflow in max_bytes calculation with 64K pages
- [Low] drm/xe: unused variable 'pitch' in xe_migrate_access_memory()
--

commit 1cfd0248d15da5743e2e644789c02a8f2508bbb8
Author: Jan Maslak <jan.maslak@intel.com>
Date:   Wed Oct 14 13:46:33 2026 +0000

    drm/xe/xe_migrate: Optimize unaligned access_memory copies

This commit modifies xe_migrate_access_memory() to optimize memory copies
by splitting transfers into an alignment phase, a matrix copy phase, and a
tail phase. It calculates the largest supported alignment pitch and
restricts copy lengths accordingly.

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 4b995c93fcea9..5609f29b28fea 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
[ ... ]
> @@ -2540,15 +2620,66 @@ int xe_migrate_access_memory(struct xe_migrate *m, struct xe_bo *bo,
>  		else
>  			current_bytes = min_t(int, bytes_left, cursor.size);
>  
> -		pitch = xe_migrate_copy_pitch(xe, current_bytes,
> -					      write ? vram_addr :
> -					      (unsigned long)buf & ~PAGE_MASK);
> -		if (xe->info.has_mem_copy_instr)
> -			current_bytes = min_t(int, current_bytes, U16_MAX * pitch);
> -		else
> -			current_bytes = min_t(int, current_bytes,
> -					      round_down(S16_MAX * pitch,
> -							 XE_CACHELINE_BYTES));
> +		{
> +			/*
> +			 * Optimized copy strategy to minimize number of jobs:
> +			 * 1. Linear copy to align dst to best reachable alignment
> +			 * 2. Matrix copy for bulk data copy
> +			 * 3. Single linear copy for remaining bytes
> +			 *
> +			 * The phase logic controls transfer chunking; emit_copy() selects
> +			 * MEM_COPY_CMD or XY_FAST_COPY_BLT based on hardware support.
> +			 */
> +			u64 dst_addr = write ? vram_addr :
> +					       (unsigned long)buf & ~PAGE_MASK;
> +			u32 max_linear = xe->info.has_mem_copy_instr ? U16_MAX :
> +					 round_down(S16_MAX, XE_CACHELINE_BYTES);
> +			u32 best_align = xe_migrate_best_alignment(dst_addr, max_linear);
> +			u32 bytes_to_align = best_align ?
> +					     xe_migrate_bytes_to_align(dst_addr, best_align) : 0;
> +
> +			if (bytes_to_align > 0 && bytes_to_align <= (u32)current_bytes) {
> +				/*
> +				 * Phase 1: Linear copy to reach best alignment.
> +				 * Copy just enough bytes to align dst_addr.
> +				 */
> +				pitch = 1;

[Severity: Low]
Is the pitch variable used anywhere after being assigned?

It looks like it is assigned multiple times across these branches but is never
read or passed to xe_migrate_vram() later in the loop. Could this unused
variable trigger compiler warnings with CONFIG_WERROR?

> +				current_bytes = min_t(int, bytes_to_align, max_linear);
> +			} else if (IS_ALIGNED(dst_addr, 4)) {
> +				/*
> +				 * Phase 2: Matrix copy for aligned bulk data.
> +				 * Use the best pitch that dst alignment supports,
> +				 * then round down len to that pitch.
> +				 */
> +				u32 dst_pitch = xe_migrate_dst_pitch(dst_addr);
> +				u32 aligned_len = xe_migrate_aligned_copy_len(current_bytes,
> +								      dst_pitch);
> +
> +				if (aligned_len >= dst_pitch) {
> +					u32 max_bytes = xe->info.has_mem_copy_instr ?
> +							U16_MAX * dst_pitch :
> +							round_down(S16_MAX * dst_pitch,
> +								   XE_CACHELINE_BYTES);
> +
> +					pitch = dst_pitch;
> +					current_bytes = min_t(int, aligned_len, max_bytes);

[Severity: High]
Can max_bytes overflow and become negative here when cast by min_t()?

On architectures supporting 64K pages (where PAGE_SIZE is 65536), max_bytes
would be evaluated as U16_MAX * dst_pitch (65535 * 65536 = 4294901760, or
0xFFFE0000).

Because min_t(int, ...) forces a cast to a signed 32-bit int, 0xFFFE0000
evaluates to -131072. Since -131072 is less than aligned_len, current_bytes
would become negative. This negative value would then be passed to
xe_migrate_vram() where it is implicitly cast to an unsigned long, wrapping
around to an enormous positive value and triggering out-of-bounds page table
indexing.

> +				} else {
> +					/*
> +					 * Phase 3: Linear copy for remainder.
> +					 * Not enough for matrix copy - do single linear copy.
> +					 */
> +					pitch = 1;
> +					current_bytes = min_t(int, current_bytes, max_linear);
> +				}
> +			} else {
> +				/*
> +				 * Phase 3: Linear copy for remainder.
> +				 * dst not 4-aligned and can't reach alignment - linear copy.
> +				 */
> +				pitch = 1;
> +				current_bytes = min_t(int, current_bytes, max_linear);
> +			}
> +		}
>  
>  		__fence = xe_migrate_vram(m, current_bytes,
>  					  (unsigned long)buf & ~PAGE_MASK,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014134634.582314-1-jan.maslak@intel.com?part=3