Re: [RFC v2 11/21] drm/xe/svm: Handle svm vma for acc_ctr trigger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The early graceful bailout for non-migratable ranges on access counter triggers is unreachable on discrete GPUs, causing an incorrect `-EACCES` error return.
- [Medium] Valid page fault statistics are incorrectly incremented for access counter triggers, skewing hardware telemetry.
--

commit 67501dc98da68ff714f7eec618d994bd502ef87c
Author: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Subject: drm/xe/svm: Handle svm vma for acc_ctr trigger

This commit generalizes page fault handling into a range setup function to
support hardware access-counter triggers. It aims to skip page-fault-specific
bookkeeping and forcefully migrate triggered ranges to device memory if
possible.

> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c
[ ... ]
> @@ -1314,7 +1316,8 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
>  	if (IS_ERR(range))
>  		return PTR_ERR(range);
>  
> -	xe_svm_range_fault_count_stats_incr(gt, range);
> +	if (!acc_ctr_trigger)
> +		xe_svm_range_fault_count_stats_incr(gt, range);

[Severity: Medium]
While this successfully skips the primary page fault stat increment, does it
leave the valid page fault statistics skewed for access counter triggers?

If the range is already valid, __xe_svm_range_setup() evaluates
xe_svm_range_is_valid() and unconditionally increments the valid fault stat:

        if (xe_svm_range_is_valid(range, tile, ctx.devmem_only, dpagemap)) {
                xe_svm_range_valid_fault_count_stats_incr(gt, range);
                range_debug(range, acc_ctr_trigger ? ...
                goto out;
        }

Should xe_svm_range_valid_fault_count_stats_incr() also be guarded by
!acc_ctr_trigger to prevent falsely inflating hardware telemetry during normal
operation?

>  
>  	mutex_lock(&range->lock);
>  
> @@ -1336,6 +1339,10 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
>  
>  	range_debug(range, "PAGE FAULT");
>  
> +	if (acc_ctr_trigger && !range_flags.migrate_devmem) {
> +		goto out;
> +	}
> +

[Severity: Medium]
Will this newly added graceful bailout ever be reached on discrete GPUs?

Because ctx.devmem_only is initialized to (need_vram || acc_ctr_trigger) &&
devmem_possible, it evaluates to true during an access counter trigger on a
discrete GPU.

Earlier in __xe_svm_range_setup(), there is a pre-existing check before
reaching this code block:

        if (ctx.devmem_only && !range_flags.migrate_devmem) {
                err = -EACCES;
                goto err_out;
        }

Since both ctx.devmem_only and !range_flags.migrate_devmem are true for
non-migratable ranges during an access counter trigger, the function appears
to immediately return -EACCES. Does this bypass the intended graceful exit
added here, creating dead code and returning an incorrect error instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909124457.1610890-23-himal.prasad.ghimiray@intel.com?part=11