Re: [PATCH v10 27/27] drm/xe/eudebug: Enable EU pagefault handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Temporary NULL VMAs for subsequent eudebug pagefaults are destroyed immediately after ACKing the fault, creating a race condition that leads to livelock.
- [High] Successful SVM pagefaults bypass private tagging, which permanently stalls the finalization of the active eudebug session.
- [High] Pagefault cache hits bypass eudebug tagging, permanently hanging the debugger session.
- [High] `xe_vm_svm_vma_subtract()` is called with unaligned address boundaries, corrupting the VM layout.
--

commit bd171058301961cced31067feb0527d9f0b38c87
Author: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>

drm/xe/eudebug: Enable EU pagefault handling

This commit enables EU pagefault handling for XE2/PVC hardware, allowing
the driver to unhalt pagefaulted EU threads via SIP mode and a temporary
null page.

> diff --git a/drivers/gpu/drm/xe/xe_guc_pagefault.c b/drivers/gpu/drm/xe/xe_guc_pagefault.c
> --- a/drivers/gpu/drm/xe/xe_guc_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pagefault.c
> @@ -59,13 +60,14 @@ static void guc_ack_fault(struct xe_pagefault *pf, int err)
>  			      write_only);
>  }
>  
> -static void guc_ack_fault_end(void *private)
> +static void guc_ack_fault_end(void *private, int err)
>  {
> -	struct xe_guc *guc = private;
> +	struct xe_guc *guc = xe_eudebug_pagefault_get_private(private);
>  
>  	if ((guc->pagefault_ack_counter & (XE_GUC_PAGEFAULT_FLUSH_PERIOD - 1)) != 1)
>  		xe_guc_ct_send_flush(&guc->ct);
>  	xe_guc_ct_unlock(&guc->ct);
> +	xe_eudebug_pagefault_end(private, err);

[Severity: High]
Does this create a race condition that leads to a livelock? 

When this calls xe_eudebug_pagefault_end(), temporary null VMAs for
subsequent pagefaults are destroyed immediately (destroy_pf = 1) after
ACKing the fault and signaling the hardware to resume. 

Since there is no synchronization to guarantee the EU thread has re-executed 
the faulting instruction before the KMD unbinds the VMA, if the VMA is
unbound before the EU thread accesses the address, won't the thread
pagefault again on the same instruction and loop infinitely?

>  }
>  
> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
[ ... ]
> @@ -227,8 +228,56 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
>  
>  	vma = xe_vm_find_vma_by_addr(vm, pf->consumer.page_addr);
>  	if (!vma) {
[ ... ]
> +	} else {
> +		/*
> +		 * For non-SVM case:
> +		 * Eudebug with active pagefault always needs to be attached
> +		 * to pagefault since it waits for all pagefaults with matching
> +		 * asid to be resolved.
> +		 */
> +		if (!xe_vma_is_cpu_addr_mirror(vma))
> +			xe_eudebug_pagefault_set_private(pf, vm);

[Severity: High]
Does skipping the private tagging for SVM (cpu_addr_mirror) VMAs cause the 
eudebug session to hang permanently? 

If an active eudebug session exists for the same ASID and its finalization 
was postponed because this SVM fault was still in the queue, 
xe_eudebug_pagefault_end() will see no eudebug tag and return immediately 
without triggering eudebug_pagefault_finalize(). This seems like it would 
leave the EU threads permanently stuck in SIP mode.

> +	}
>  
>  	if (xe_vma_read_only(vma) &&
> @@ -239,11 +288,52 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
>  
>  	atomic = xe_pagefault_access_is_atomic(pf->consumer.access_type);
>  
> -	if (xe_vma_is_cpu_addr_mirror(vma))
> +	if (xe_vma_is_cpu_addr_mirror(vma)) {
>  		err = xe_svm_handle_pagefault(vm, vma, pf, gt,
>  					      pf->consumer.page_addr, atomic);
> -	else
> +
> +#if IS_ENABLED(CONFIG_DRM_XE_EUDEBUG)
> +		/*
> +		 * If err is -ENOENT, it means that the cpu-address-space-mirrored
> +		 * xe vma exists, but there is no mm vma allocated in
> +		 * the CPU address space. This indicates that no memory has been
> +		 * allocated in the CPU address space.
> +		 */
> +		if (err == -ENOENT &&
> +		    !xe_vm_is_closed_or_banned(vm) &&
> +		    xe_eudebug_pagefault_creatable(gt, vm)) {
> +			u32 page_size = vm->flags & XE_VM_FLAG_64K ? SZ_64K : SZ_4K;
[ ... ]
> +			vma = xe_vm_find_vma_by_addr(vm, pf->consumer.page_addr);
> +			if (vma && xe_vma_is_cpu_addr_mirror(vma))
> +				vma = xe_vm_svm_vma_subtract(vm, vma,
> +							     pf->consumer.page_addr,
> +							     pf->consumer.page_addr + page_size);

[Severity: High]
Is it safe to pass pf->consumer.page_addr directly as the subtraction
boundaries? 

The hardware fault address is not guaranteed to be aligned to page_size. 
Passing unaligned boundaries to xe_vm_svm_vma_subtract() will create
unaligned head/tail VMAs via xe_vma_create(). 

Inserting unaligned VMAs into the interval tree and page tables violates
core memory management invariants and could lead to page table corruption or
kernel panics.

> +			else
> +				vma = ERR_PTR(-EINVAL);
[ ... ]
> @@ -598,6 +688,12 @@ static void xe_pagefault_queue_work(struct work_struct *w)
>  		}
>  
>  ack_fault:
> +		/*
> +		 * set private after xe_pagefault_service() since eudebug could swap
> +		 * the pf->producer.private field. Also needed when cache was hit.
> +		 */
> +		private = pf->producer.private;

[Severity: High]
Does a pagefault cache hit permanently hang the eudebug session? 

When a pagefault matches the cache, the code jumps directly to ack_fault, 
skipping xe_pagefault_service(). Since the service routine is bypassed, 
xe_eudebug_pagefault_set_private() is never called, leaving the fault 
untagged. 

Similar to the SVM bypass above, if the session postponed finalization for 
this queued fault, it will never trigger eudebug_pagefault_finalize() upon 
completion.

> +
>  		xe_assert(xe, pf->consumer.alloc_state ==
>  			  XE_PAGEFAULT_ALLOC_STATE_ACTIVE);
>  		xe_assert(xe, pf == pf_work->cache.pf);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903145952.848051-1-mika.kuoppala@linux.intel.com?part=27