Re: [PATCH v2 1/5] drm/gpusvm: split MM state flags out of drm_gpusvm_pages_flags

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Compilation error due to missing `pages` member in `struct xe_svm_range`.

Pre-existing issues:
- [High] Critical error paths swallow error codes by jumping to `out`, causing an infinite GPU page fault livelock.
--

commit c7d50cf1df054885b8dfd4565b263894bfd104b3
Author: Honglei Huang <honghuan@amd.com>
Subject: drm/gpusvm: split MM state flags out of drm_gpusvm_pages_flags

This commit splits the MM state flags out of drm_gpusvm_pages_flags and moves
them to a new drm_gpusvm_range_flags structure. It keeps the mapping state
flags in drm_gpusvm_pages_flags and updates usage in the amd and xe drivers.

Link: https://gitlab.freedesktop.org/mbrost/xe-kernel-driver-svn-perf-6-15-2025/-/commit/623f6a50c037d9e44f6c9fbe6859a0ba7ad50177

> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> index e1651e70c8f01..05578e187d204 100644
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c
> @@ -134,7 +134,8 @@ xe_svm_garbage_collector_add_range(struct xe_vm *vm, struct xe_svm_range *range,
>  
>  	range_debug(range, "GARBAGE COLLECTOR ADD");
>  
> -	drm_gpusvm_range_set_unmapped(&range->base, mmu_range);
> +	drm_gpusvm_range_set_unmapped(&range->base, &range->pages, 1,
> +				      mmu_range);

[Severity: Low]
Does this cause a compilation error due to pages not being a direct member
of struct xe_svm_range? It looks like the pages struct is located inside the
embedded base field (range->base.pages).

While this might be fixed in a later patch in the series, should each commit
compile successfully on its own to preserve bisection?

[ ... ]

> @@ -1248,7 +1249,7 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
>  
>  	xe_svm_range_fault_count_stats_incr(gt, range);
>  
> -	if (ctx.devmem_only && !range->base.pages.flags.migrate_devmem) {
> +	if (ctx.devmem_only && !range->base.flags.migrate_devmem) {
>  		err = -EACCES;
>  		goto out;
>  	}

[Severity: High]
This is a pre-existing issue, but can this error path swallow the error code by
jumping to the out label?

Looking at __xe_svm_handle_pagefault(), the out label appears to
unconditionally return 0:

out:
	xe_svm_range_fault_us_stats_incr(gt, range, start);
	return 0;

If we set err = -EACCES and jump to out, or if xe_svm_range_get_pages()
fails and jumps to out later in the function, does returning 0 cause the
caller to believe the page fault was handled successfully?

Since the pages weren't mapped, wouldn't the GPU immediately retry the
instruction and fault again, potentially causing an infinite livelock? Should
these error paths jump to err_out instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617103602.1491609-1-honglei1.huang@amd.com?part=1