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

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

On 2/27/26 3:11 PM, Gustavo Sousa wrote:
> Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:
> 
>> From: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>>
>> The XE2 (and PVC) HW has a limitation that the pagefault due to invalid
>> access will halt the corresponding EUs. To solve this problem, enable
>> EU pagefault handling functionality, which allows to unhalt pagefaulted
>> eu threads and to EU debugger to get inform about the eu attentions state
>> of EU threads during execution.
>>
>> If a pagefault occurs, send the DRM_XE_EUDEBUG_EVENT_PAGEFAULT event
>> after handling the pagefault.
>>
>> The pagefault handling is a mechanism that allows a stalled EU thread to
>> enter SIP mode by installing a temporal null page to the page table entry
>> where the pagefault happened.
>>
>> A brief description of the page fault handling mechanism flow between KMD
>> and the eu thread is as follows
>>
>> (1) eu thread accesses unallocated address
>> (2) pagefault happens and eu thread stalls
>> (3) XE kmd set an force eu thread exception to allow the running eu thread
>>      to enter SIP mode (kmd set ForceException / ForceExternalHalt bit of
>>      TD_CTL register)
>>      Not stalled (none-pagefaulted) eu threads enter SIP mode
>> (4) XE kmd installs temporal null page to the pagetable entry of the
>>      address where pagefault happened.
>> (5) XE kmd replies pagefault successful message to GUC
>> (6) stalled eu thread resumes as per pagefault condition has resolved
>> (7) resumed eu thread enters SIP mode due to force exception set by (3)
>> (8) adapted to consumer/produced pagefaults
>>
>> As designed this feature to only work when eudbug is enabled, it should
>> have no impact to regular recoverable pagefault code path.
>>
>> v2: - pf->q holds the vm ref so drop it (Mika)
>>      - streamline uapi (Mika)
>>      - cleanup the pagefault through producer if (Mika)
>>
>> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_pagefault.c   |  8 +++++++
>>   drivers/gpu/drm/xe/xe_pagefault.c       | 31 ++++++++++++++++++++++++-
>>   drivers/gpu/drm/xe/xe_pagefault_types.h |  9 +++++++
>>   3 files changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pagefault.c b/drivers/gpu/drm/xe/xe_guc_pagefault.c
>> index d48f6ed103bb..6adf3bf73b1c 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pagefault.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pagefault.c
>> @@ -8,6 +8,7 @@
>>   #include "xe_guc_ct.h"
>>   #include "xe_guc_pagefault.h"
>>   #include "xe_pagefault.h"
>> +#include "xe_eudebug_pagefault.h"
>>   
>>   static void guc_ack_fault(struct xe_pagefault *pf, int err)
>>   {
>> @@ -37,8 +38,15 @@ static void guc_ack_fault(struct xe_pagefault *pf, int err)
>>   	xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
>>   }
>>   
>> +static void guc_cleanup_fault(struct xe_pagefault *pf, int err)
>> +{
>> +	xe_eudebug_pagefault_service(pf);
>> +	xe_eudebug_pagefault_destroy(pf, 0);
>> +}
>> +
>>   static const struct xe_pagefault_ops guc_pagefault_ops = {
>>   	.ack_fault = guc_ack_fault,
>> +	.cleanup_fault = guc_cleanup_fault,
>>   };
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
>> index 72f589fd2b64..9dcd854e99f9 100644
>> --- a/drivers/gpu/drm/xe/xe_pagefault.c
>> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
>> @@ -10,6 +10,7 @@
>>   
>>   #include "xe_bo.h"
>>   #include "xe_device.h"
>> +#include "xe_eudebug_pagefault.h"
>>   #include "xe_gt_printk.h"
>>   #include "xe_gt_types.h"
>>   #include "xe_gt_stats.h"
>> @@ -171,6 +172,8 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
>>   	if (IS_ERR(vm))
>>   		return PTR_ERR(vm);
>>   
>> +	xe_eudebug_pagefault_create(vm, pf);
> 
> This might not get called because of the early returns above. And
> looking at xe_guc_pagefault_handler(), we don't seem to zero initialize
> the "pf" variable there.  Couldn't this cause a risk of
> xe_eudebug_pagefault_destroy() being called with pf->consumer.epf
> containing garbage?
> 
Yes, you're right. Thank you for the good point.
I'll also apply this fix to the version that stores epf in 
pf->producer.private.

G.G.

> --
> Gustavo Sousa
> 
>> +
>>   	/*
>>   	 * TODO: Change to read lock? Using write lock for simplicity.
>>   	 */
>> @@ -184,9 +187,28 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
>>   	vma = xe_vm_find_vma_by_addr(vm, pf->consumer.page_addr);
>>   	if (!vma) {
>>   		err = -EINVAL;
>> -		goto unlock_vm;
>> +		vma = xe_eudebug_create_vma(vm, pf);
>> +		if (IS_ERR(vma)) {
>> +			err = PTR_ERR(vma);
>> +			vma = NULL;
>> +		}
>>   	}
>>   
>> +	if (vma) {
>> +		/*
>> +		 * When creating an instance of eudebug_pagefault, there was
>> +		 * no vma containing the ppgtt address where the pagefault occurred,
>> +		 * but when reacquiring vm->lock, there is.
>> +		 * During not aquiring the vm->lock from this context,
>> +		 * but vma corresponding to the address where the pagefault occurred
>> +		 * in another context has allocated.
>> +		 */
>> +		err = 0;
>> +	}
>> +
>> +	if (err)
>> +		goto unlock_vm;
>> +
>>   	atomic = xe_pagefault_access_is_atomic(pf->consumer.access_type);
>>   
>>   	if (xe_vma_is_cpu_addr_mirror(vma))
>> @@ -198,6 +220,10 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
>>   unlock_vm:
>>   	if (!err)
>>   		vm->usm.last_fault_vma = vma;
>> +
>> +	if (err)
>> +		xe_eudebug_pagefault_destroy(pf, err);
>> +
>>   	up_write(&vm->lock);
>>   	xe_vm_put(vm);
>>   
>> @@ -268,6 +294,9 @@ static void xe_pagefault_queue_work(struct work_struct *w)
>>   
>>   		pf.producer.ops->ack_fault(&pf, err);
>>   
>> +		if (pf.producer.ops->cleanup_fault)
>> +			pf.producer.ops->cleanup_fault(&pf, err);
>> +
>>   		if (time_after(jiffies, threshold)) {
>>   			queue_work(gt_to_xe(pf.gt)->usm.pf_wq, w);
>>   			break;
>> diff --git a/drivers/gpu/drm/xe/xe_pagefault_types.h b/drivers/gpu/drm/xe/xe_pagefault_types.h
>> index 2bee858da597..9d2d29d35a4b 100644
>> --- a/drivers/gpu/drm/xe/xe_pagefault_types.h
>> +++ b/drivers/gpu/drm/xe/xe_pagefault_types.h
>> @@ -43,6 +43,15 @@ struct xe_pagefault_ops {
>>   	 * sends the result to the HW/FW interface.
>>   	 */
>>   	void (*ack_fault)(struct xe_pagefault *pf, int err);
>> +
>> +	/**
>> +	 * @cleanup_fault: Cleanup for producer, if any
>> +	 * @pf: Page fault
>> +	 * @err: Error state of fault
>> +	 *
>> +	 * Page fault producer received cleanup request from consumer
>> +	 */
>> +	void (*cleanup_fault)(struct xe_pagefault *pf, int err);
>>   };
>>   
>>   /**
>> -- 
>> 2.43.0