Re: [PATCH v3] [CI-ONLY][DONOT-REVIEW] Access counter squash

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Access counter events for SVM ranges that cannot be migrated to VRAM will cause an infinite AC interrupt storm because the driver fails to set the No-Count (NC) bit to silence the hardware.
- [High] Incorrect virtual address calculation in xe_guc_access_counter_page_va discards the sub-granule index, incorrectly shifting the entire access counter window.
- [High] XE_VMA_MIGRATE_ON_ACC flag suppresses mandatory migrations for device atomics and ignores explicit madvise preferred locations.
- [High] Returning -ENOSPC for a full advisory access counter queue triggers a fatal GPU reset.
- [High] Use-after-free of pagefault_wq due to teardown asymmetry between workqueue destruction and GuC CT disablement.
- [High] Access counter triggers on non-migratable SVM ranges fail with -EACCES instead of being gracefully skipped, due to unreachable code.
- [High] Overlapping bitmask definitions for ACC_GRANULARITY and ACC_VIRTUAL_ADDR_RANGE_LO cause virtual address corruption and incorrect granularity values.
--

commit d8aa5e34df6118bc5e5f7ead213c84de95b73180
Author: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Access counter squash

This commit implements the Xe access counter mechanism for tracking memory
access frequency and migrating hot regions to VRAM. It sets up the access
counter queue, adds the GuC to host notification handler, and modifies the
SVM page fault handler to service access counter triggers.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -997,6 +998,10 @@ int xe_device_probe(struct xe_device *xe)
>  	err = xe_pagefault_init(xe);
>  	if (err)
>  		return err;
> +
> +	err = xe_access_counter_init(xe);
> +	if (err)
> +		return err;

[Severity: High]
Will this initialization order lead to a use-after-free during teardown?

Since xe_pagefault_init() occurs before xe_access_counter_init(), its
corresponding teardown will destroy pagefault_wq before the GuC CT is
disabled. If an access counter notification arrives during this window,
the handler will attempt to queue work on the destroyed pagefault_wq.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_access_counter.c b/drivers/gpu/drm/xe/xe_access_counter.c
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_access_counter.c
[ ... ]
> +int xe_access_counter_handler(struct xe_device *xe, struct xe_access_counter *ac)
> +{
> +	struct xe_usm_queue *ac_queue = &xe->usm.ac_queue;
> +	int idx;
> +	unsigned long flags;
> +	bool full;
> +
> +	spin_lock_irqsave(&ac_queue->lock, flags);
> +	idx = xe_access_counter_work_index(xe);
> +	full = xe_usm_queue_full(ac_queue);
> +	if (!full) {
> +		xe_usm_queue_push(ac_queue, ac);
> +		queue_work(xe->usm.pagefault_wq, &xe->usm.ac_workers[idx].work);

[Severity: High]
Could this dereference the freed pagefault_wq during device removal?

As noted regarding the initialization order, a pending access counter
notification received while the driver unbinds might reach this point
after pagefault_wq has already been destroyed.

> +	} else {
> +		drm_warn(&xe->drm,
> +			 "AccessCounter Queue full, shouldn't be possible\n");
> +	}
> +	spin_unlock_irqrestore(&ac_queue->lock, flags);
> +
> +	return full ? -ENOSPC : 0;

[Severity: High]
Does returning -ENOSPC for a full advisory queue trigger a fatal GPU reset?

This error propagates back to process_g2h_msg(), which interprets a
non-zero return as a fatal error and calls CT_DEAD. If a heavy workload
temporarily fills the queue, it seems this will wedge the device instead
of gracefully dropping the hint.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_guc_access_counter.c b/drivers/gpu/drm/xe/xe_guc_access_counter.c
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_access_counter.c
> @@ -0,0 +1,74 @@
[ ... ]
> +static u64 xe_guc_access_counter_page_va(struct xe_device *xe, u64 addr, u8 gran)
> +{
> +	u64 gran_size = xe_access_counter_granularity_to_size(gran);
> +	u64 sub_gran_size = gran_size / 32;
> +	u64 region_base = ALIGN_DOWN(addr, gran_size);
> +	u64 offset_in_subchunk = addr & (sub_gran_size - 1);
> +	u64 page_va = region_base + offset_in_subchunk;

[Severity: High]
Does this math inadvertently drop the active sub-chunk index?

By masking the address with (sub_gran_size - 1), it strips out the
sub-granule offset. This will incorrectly shift the entire search
window backwards in the virtual address space.

> +
> +	xe_dbg(xe, "gran_size = %llx, addr = = %llx, region_base = %llx, page_va=%llx\n",
> +	       gran_size, addr, region_base, page_va);
> +	return page_va;
> +}
[ ... ]
> +int xe_guc_access_counter_handler(struct xe_guc *guc, u32 *msg, u32 len)
> +{
[ ... ]
> +	ac.consumer.counter_type = FIELD_GET(ACC_TYPE, msg[0]);
> +	if (ac.consumer.counter_type > XE_ACCESS_COUNTER_TYPE_NOTIFY)
> +		return -EINVAL;
> +
> +	addr = ((u64)(msg[3] & ACC_VIRTUAL_ADDR_RANGE_HI) << 32) |
> +		(msg[2] & ACC_VIRTUAL_ADDR_RANGE_LO);

[Severity: High]
Does extracting the address with ACC_VIRTUAL_ADDR_RANGE_LO unintentionally
mix the granularity bits into the address?

Because the ACC_VIRTUAL_ADDR_RANGE_LO and ACC_GRANULARITY masks overlap,
this operation corrupts the virtual address by injecting the granularity
value into it.

> +	ac.consumer.page_va = xe_guc_access_counter_page_va(xe, addr, ac.consumer.granularity);
> +	ac.gran_end = ac.consumer.page_va +
> +		      xe_access_counter_granularity_to_size(ac.consumer.granularity);

[Severity: High]
Will this boundary calculation use the corrupted start address from
xe_guc_access_counter_page_va() and shift the search window out of alignment?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
[ ... ]
> @@ -1753,6 +1754,9 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
[ ... ]
>  	if (ret) {
>  		xe_gt_err(gt, "G2H action %#04x failed (%pe) len %u msg %*ph\n",
>  			  action, ERR_PTR(ret), hxg_len, (int)sizeof(u32) * hxg_len, hxg);
>  		CT_DEAD(ct, NULL, PROCESS_FAILED);
>  	}

[Severity: High]
When xe_guc_access_counter_handler() returns -ENOSPC due to a full queue,
does this block inadvertently trigger a fatal GPU reset?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_guc_fwif.h b/drivers/gpu/drm/xe/xe_guc_fwif.h
> --- a/drivers/gpu/drm/xe/xe_guc_fwif.h
> +++ b/drivers/gpu/drm/xe/xe_guc_fwif.h
[ ... ]
> +	u32 dw2;
> +#define ACC_VIRTUAL_ADDR_RANGE_LO	GENMASK(31, 12)
> +#define ACC_GRANULARITY	GENMASK(22, 20)

[Severity: High]
Do these bitmasks overlap?

Both masks cover bits 20-22, which causes the address reconstruction in
xe_guc_access_counter_handler() to mix granularity data into the
virtual address.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
[ ... ]
> @@ -862,6 +862,27 @@ xe_pt_stage_bind(struct xe_tile *tile, struct xe_vma *vma,
[ ... ]
> +	if (range) {
> +		/*
> +		 * Always set NC (no-count) on VRAM PTEs for SVM ranges so
> +		 * this GPU does not generate access counter events for memory
> +		 * it is already responsible for.  Today default_vram_pte
> +		 * covers only local VRAM; once UAL lands, remote-device VRAM
> +		 * pages will also flow through default_vram_pte — NC remains
> +		 * the right setting there too, as the owning device handles
> +		 * AC for its own local memory.  Update this comment when UAL
> +		 * support is added and adjust if the semantics change.
> +		 */
> +		xe_walk.default_vram_pte |= XE_PPGTT_PTE_NC;
> +		if (!xe_svm_range_allowed_in_devmem(range))
> +			xe_walk.default_system_pte |= XE_PPGTT_PTE_NC;

[Severity: High]
Could this logic omit the No-Count (NC) bit for un-migratable SVM ranges?

If a sub-64K range triggers an event on a platform that does not support
4K migration, xe_svm_range_allowed_in_devmem() remains true, so the NC
bit is not set here. The migration itself is skipped in
__xe_svm_range_setup(), leaving the hardware free to repeatedly trigger
access counter events for this same range, creating an interrupt storm.

[ ... ]

> 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
[ ... ]
> @@ -1369,13 +1369,32 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
[ ... ]
>  	/* READ_ONCE pairs with WRITE_ONCE in drm_gpusvm_range_set_unmapped() */
>  	range_flags.__flags = READ_ONCE(range->base.flags.__flags);
>  	if (ctx.devmem_only && !range_flags.migrate_devmem) {
>  		err = -EACCES;
>  		goto err_out;
>  	}

[Severity: High]
Does this early error check prevent access counters from gracefully skipping
non-migratable ranges?

Because an access counter trigger sets ctx.devmem_only = true, encountering
a non-migratable range here returns -EACCES. This aborts the worker for the
entire granularity window and prevents the graceful skip at the goto out
below, silently ignoring any other hot VMAs in the same window.

>  
>  	if (xe_svm_range_is_valid(range, tile, ctx.devmem_only, dpagemap)) {
>  		xe_svm_range_valid_fault_count_stats_incr(gt, range);
[ ... ]
> +	/*
> +	 * AC-triggered setup: if the range is already in device memory a
> +	 * rebind is all that's needed.  Otherwise proceed to the
> +	 * migration path — with no explicit madvise preferred location, the
> +	 * default first-touch-then-migrate-on-hot policy applies and we
> +	 * should attempt VRAM placement for this hot range.
> +	 */
> +	if (acc_ctr_trigger && !range_flags.migrate_devmem)
> +		goto out;

[Severity: High]
Is this intended graceful exit actually dead code?

As mentioned above, the earlier check for !range_flags.migrate_devmem
already aborts the function with -EACCES before reaching this point.

>  
> +	/*
> +	 * With MIGRATE_ON_ACCESS_COUNTER, defer first placement to access
> +	 * counters: skip the eager migrate on a normal page fault, but still
> +	 * migrate when this setup was itself triggered by an AC event.
> +	 */
>  	if (--migrate_try_count >= 0 &&
> +	    (acc_ctr_trigger || !(vma->gpuva.flags & XE_VMA_MIGRATE_ON_ACC)) &&
>  	    xe_svm_range_needs_migrate_to_vram(range, vma, dpagemap)) {

[Severity: High]
Does checking XE_VMA_MIGRATE_ON_ACC here suppress mandatory migrations?

If userspace passes DRM_XE_VM_BIND_FLAG_MIGRATE_ON_ACCESS_COUNTER but then
issues a device atomic operation (requiring VRAM) or sets an explicit
madvise preferred location, this check short-circuits. Skipping eager
migration in these cases violates UAPI guarantees for atomics and madvise.

Additionally, when an access counter event triggers for a range that is
un-migratable (e.g. sub-64K size), xe_svm_range_needs_migrate_to_vram()
returns false. This skips migration but takes no action to prevent further
access counter triggers for this range, contributing to the interrupt storm
issue mentioned earlier in xe_pt_stage_bind().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910111204.1708465-2-himal.prasad.ghimiray@intel.com?part=1