Re: [PATCH v2 4/4] drm/xe/kunit: Add tests for PF-mem regions

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The hardcoded mock pointer `SZ_4G` truncates to `NULL` on 32-bit architectures, breaking KUnit tests.
- [Low] The out-of-bounds offset test case is shadowed by a page alignment check.
--

--- Patch [4]: [PATCH v2 4/4] drm/xe/kunit: Add tests for PF-mem regions ---
commit def1e04af372e808abcf812a9ee0ac48e736b86c
Author: Piotr Piórkowski <[email protected]>

drm/xe/kunit: Add tests for PF-mem regions

Add KUnit tests for PF-mem VRAM regions to provide coverage for region
metadata, bounds checking, and CPU visibility constraints.

> diff --git a/drivers/gpu/drm/xe/tests/xe_vram.c b/drivers/gpu/drm/xe/tests/xe_vram.c
> new file mode 100644
> index 0000000000000..36a49fbef2206
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/tests/xe_vram.c
> @@ -0,0 +1,115 @@

[ ... ]

> +static struct xe_vram_region pfmem_parent(struct xe_device *xe, u64 io_size)
> +{
> +	struct xe_vram_region parent = {
> +		.xe = xe,
> +		.id = 0,
> +		.io_start = SZ_1G,
> +		.io_size = io_size,
> +		.dpa_base = SZ_2G,
> +		.mapping = (__force void __iomem *)SZ_4G,

[Severity: Medium]
Will this cast truncate to NULL on 32-bit architectures?

If this code is built on 32-bit architectures (for example, via COMPILE_TEST),
SZ_4G (0x100000000ULL) won't fit in a 32-bit pointer and truncates to NULL.

When xe_vram_region_init_pfmem() later delegates to vram_region_init(), it
checks if the mapping is NULL and fails with -ENOMEM instead of 0.

Could this cause the KUnit tests to unexpectedly fail on 32-bit builds?

> +		.usable_size = SZ_64M,
> +	};
> +
> +	return parent;
> +}

[ ... ]

> +static void pfmem_invalid_range(struct kunit *test)
> +{
> +	struct xe_device *xe = test->priv;
> +	struct xe_vram_region parent = pfmem_parent(xe, SZ_64M);
> +	struct xe_vram_region *vram;
> +
> +	vram = xe_vram_region_alloc_pfmem(xe, 0);
> +	KUNIT_ASSERT_NOT_NULL(test, vram);
> +	KUNIT_EXPECT_EQ(test, xe_vram_region_init_pfmem(vram, &parent, 0, 0), -EINVAL);
> +	KUNIT_EXPECT_EQ(test, xe_vram_region_init_pfmem(vram, &parent, SZ_64M + 1, SZ_4M), -EINVAL);

[Severity: Low]
Does this actually test the bounds check?

Since SZ_64M + 1 is not page-aligned, xe_vram_region_init_pfmem() will
evaluate the alignment check first and short-circuit to return -EINVAL.

It seems the offset > parent->usable_size bounds check would remain
completely untested here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4