Re: [PATCH v3 4/4] drm/xe/tests: Add xe_migrate_access_memory subtest

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The byte-wise verification logic in `verify_access_memory_read` assumes a little-endian memory layout, causing the test to fail on big-endian architectures.
- [Low] The fallback condition `if (!unaligned_offset)` is dead code and will never evaluate to true.

Pre-existing issues:
- [High] Infinite recursion in `xe_migrate_access_memory` when the allocated bounce buffer is not cacheline aligned.
--

commit bc41ecbf5d587d033724f3736f3ff4ebc8156d3a
Author: Jan Maslak <jan.maslak@intel.com>
Subject: drm/xe/tests: Add xe_migrate_access_memory subtest

This commit adds a new subtest to xe_migrate to verify vram to sram migration.
It specifically tests scenarios where the destination address, migration size,
or source BO offset are not 4-byte aligned.

> diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c
> index f10d9513747b4..9e18fcd51e26c 100644
> --- a/drivers/gpu/drm/xe/tests/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
[ ... ]
> +static bool verify_access_memory_read(struct kunit *test, u8 *buf,
> +				      u32 buf_start, u32 bo_start,
> +				      u32 size, const char *scenario)
> +{
> +	u32 j;
> +
> +	for (j = 0; j < size; j++) {
> +		u32 bo_byte_offset = bo_start + j;
> +		u32 byte_in_cell = bo_byte_offset % sizeof(u32);
> +		u32 cell_idx = bo_byte_offset / sizeof(u32);
> +		u32 cell_value = cell_idx;
> +		u8 expected_byte = (cell_value >> (byte_in_cell * 8)) & 0xFF;

[Severity: Medium]
Will this test fail spuriously on big-endian architectures? 

The bit-shift logic here in verify_access_memory_read() explicitly expects 
bytes to be laid out in little-endian order, but the array is primed in the
host's native byte order later in access_memory_test_run_tile().

[ ... ]
> +static void access_memory_test_unaligned_bo_offset(struct kunit *test,
> +						    struct xe_migrate *m,
> +						    struct xe_bo *bo, u8 *buf,
> +						    u32 unaligned_offset)
> +{
> +	u32 partial_size = ACCESS_MEM_BO_SIZE - unaligned_offset;
> +	int ret;
> +
> +	memset(buf, 0, ACCESS_MEM_BO_SIZE);
> +
> +	kunit_info(test, "Partial read: bo_offset=0x%x bytes=0x%x\n",
> +		   unaligned_offset, partial_size);
> +
> +	ret = xe_migrate_access_memory(m, bo, unaligned_offset, buf,
> +				       partial_size, false);

[Severity: High]
This is a pre-existing issue, but could these unaligned tests trigger an
infinite recursion loop in xe_migrate_access_memory()? 

When handling an unaligned vram access on a platform without
has_mem_copy_instr, xe_migrate_access_memory() allocates a bounce buffer:

    bounce = kmalloc(XE_CACHELINE_BYTES, GFP_KERNEL);

If debugging options like Kasan or Slub redzoning are enabled, kmalloc is not
guaranteed to return a cacheline-aligned pointer. The unaligned bounce buffer
pointer is then passed to the recursive call where it fails the alignment
check again, leading to another allocation and recursing infinitely.

[ ... ]
> +static void access_memory_test_run_tile(struct xe_device *xe,
> +					struct xe_tile *tile,
> +					struct kunit *test,
> +					bool use_random_offset)
> +{
> +	unsigned int bo_flags = XE_BO_FLAG_VRAM_IF_DGFX(tile);
> +	struct xe_migrate *m = tile->migrate;
> +	u32 j, unaligned_offset;
> +	struct xe_bo *bo;
> +	u8 *buf;
> +	int ret;
> +
> +	if (use_random_offset) {
> +		unaligned_offset = get_random_u32() % (ACCESS_MEM_BO_SIZE / 2);
> +		/* Ensure the offset is not 4-byte aligned */
> +		if (IS_ALIGNED(unaligned_offset, 4))
> +			unaligned_offset |= 2;
> +		if (!unaligned_offset)

[Severity: Low]
Is this fallback condition dead code? 

The value of unaligned_offset can never be 0 at this point. Since 0 is 4-byte
aligned, the preceding bitwise OR is guaranteed to execute and change the
value to 2, rendering this subsequent check permanently false.

> +			unaligned_offset = 3;
> +	} else {
> +		unaligned_offset = ACCESS_MEM_UNALIGNED_OFFSET;
> +	}
[ ... ]
> +	/* Prime the BO */
> +	for (j = 0; j < ACCESS_MEM_NUM_CELLS; j++)
> +		((u32 *)buf)[j] = j;

[Severity: Medium]
Does this code create a mismatch on big-endian architectures? 

The array is primed in the host's native byte order here in
access_memory_test_run_tile(), creating a mismatch when verified against the
little-endian assumption in verify_access_memory_read().

[ ... ]

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