Re: [PATCH v4 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:
- [High] xe: infinite recursion in xe_migrate_access_memory() due to unaligned kmalloc
- [Medium] drm/xe: test failure on big-endian architectures due to little-endian assumption
- [Low] drm/xe/tests: dead code in random offset generation
--

commit 123f5e095a34197f112fb5cb560c6b8a8cf51802
Author: Jan Maslak <jan.maslak@intel.com>

drm/xe/tests: Add xe_migrate_access_memory subtest

This commit adds a new xe_migrate subtest to verify VRAM to SRAM migration.
It explicitly tests migration when the destination address, migration size,
or source BO offset is not 4-byte aligned.

> diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c
> --- 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 falsely fail on big-endian architectures? 

Buffer generation in access_memory_test_run_tile() primes the test buffer
using native host endianness:

    ((u32 *)buf)[j] = j;

However, the bit shift here fundamentally assumes a little-endian memory
layout where the least significant byte is stored at the lowest address. On
a big-endian CPU, the most significant byte sits at the lowest address,
resulting in a mismatch. Should the code use explicit cpu_to_le32 and
le32_to_cpu macros to remain architecture-agnostic?

[ ... ]

> +static void access_memory_test_run_tile(struct xe_device *xe,
> +					struct xe_tile *tile,
> +					struct kunit *test,
> +					bool use_random_offset)
> +{
> +	struct drm_exec *exec = XE_VALIDATION_OPT_OUT;
> +	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;
> +	long timeout;
> +	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)
> +			unaligned_offset = 3;

[Severity: Low]
Is the zero-check fallback logic unreachable? 

Any initial zero value from get_random_u32() is transformed to 2 by the
preceding alignment enforcement, so unaligned_offset cannot be 0 at this
point. This appears to be unnecessary dead code.

[ ... ]

> +	access_memory_test_unaligned_bo_offset(test, m, bo, buf, unaligned_offset);

[Severity: High]
Could this unaligned testing trigger an infinite recursion and stack overflow
in xe_migrate_access_memory() when memory debugging is enabled? 

When xe->info.has_mem_copy_instr is false, xe_migrate_access_memory()
handles unaligned offsets by allocating a bounce buffer and then recursively
calling itself:

drivers/gpu/drm/xe/xe_migrate.c:xe_migrate_access_memory() {
    ...
    bounce = kmalloc(XE_CACHELINE_BYTES, GFP_KERNEL);
    ...
    err = xe_migrate_access_memory(m, bo,
                                   offset & ~XE_CACHELINE_MASK,
                                   bounce,
                                   XE_CACHELINE_BYTES, 0);
    ...
}

Under configurations like CONFIG_SLUB_DEBUG or CONFIG_KASAN, kmalloc()
for 64 bytes does not guarantee 64-byte alignment due to redzoning and
padding. If the returned bounce buffer is not cacheline aligned, the
recursive call's page_offset will be unaligned, causing the alignment check
to evaluate to true again and leading to unbounded recursion.

> +	access_memory_test_unaligned_dst(test, m, bo, buf, unaligned_offset);
> +	access_memory_test_unaligned_dst_aligned_size(test, m, bo, buf, unaligned_offset);

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