[PATCH V1] accel/amdxdna: wait uninterruptibly for jobs in the HMM invalidate callback

[PATCH V1] accel/amdxdna: wait uninterruptibly for jobs in the HMM invalidate callback

[Lizhi Hou]

From: Matthew Brost <matthew.brost@intel.com>

aie2_hmm_invalidate() waits for the jobs using a BO before the CPU pages
behind it are released, but does so interruptibly. When the task
triggering the invalidation has a signal pending the wait returns
-ERESTARTSYS immediately, the notifier completes and the pages can be
freed or migrated while the device is still accessing them through the
PASID.

An MMU notifier cannot fail a blockable invalidation, so wait
uninterruptibly. With MAX_SCHEDULE_TIMEOUT an uninterruptible wait
cannot time out either, so drop the now dead error handling.

Fixes: aac243092b70 ("accel/amdxdna: Add command execution")
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/aie2_ctx.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index 9c6b7353cbe9..84ab8a55b7ab 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -1286,16 +1286,12 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job,
 void aie2_hmm_invalidate(struct amdxdna_gem_obj *abo,
 			 unsigned long cur_seq)
 {
-	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
-	struct drm_gem_object *gobj = to_gobj(abo);
-	long ret;
-
-	ret = dma_resv_wait_timeout(gobj->resv, DMA_RESV_USAGE_BOOKKEEP,
-				    true, MAX_SCHEDULE_TIMEOUT);
-	if (!ret)
-		XDNA_ERR(xdna, "Failed to wait for bo, ret %ld", ret);
-	else if (ret == -ERESTARTSYS)
-		XDNA_DBG(xdna, "Wait for bo interrupted by signal");
+	/*
+	 * The pages must not be released while the device may still access
+	 * them, so this wait must be neither interruptible nor time limited.
+	 */
+	dma_resv_wait_timeout(to_gobj(abo)->resv, DMA_RESV_USAGE_BOOKKEEP,
+			      false, MAX_SCHEDULE_TIMEOUT);
 }
 
 int aie2_hwctx_heap_expand(struct amdxdna_hwctx *hwctx,
-- 
2.34.1

Re: [PATCH V1] accel/amdxdna: wait uninterruptibly for jobs in the HMM invalidate callback

[Mario Limonciello]

On 10/15/26 13:32, Lizhi Hou wrote:
> From: Matthew Brost <matthew.brost@intel.com>
> 
> aie2_hmm_invalidate() waits for the jobs using a BO before the CPU pages
> behind it are released, but does so interruptibly. When the task
> triggering the invalidation has a signal pending the wait returns
> -ERESTARTSYS immediately, the notifier completes and the pages can be
> freed or migrated while the device is still accessing them through the
> PASID.
> 
> An MMU notifier cannot fail a blockable invalidation, so wait
> uninterruptibly. With MAX_SCHEDULE_TIMEOUT an uninterruptible wait
> cannot time out either, so drop the now dead error handling.
> 
> Fixes: aac243092b70 ("accel/amdxdna: Add command execution")
> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by is not a valid tag.  It should be:

Assisted-by: LLM

> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> ---
>   drivers/accel/amdxdna/aie2_ctx.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index 9c6b7353cbe9..84ab8a55b7ab 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -1286,16 +1286,12 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job,
>   void aie2_hmm_invalidate(struct amdxdna_gem_obj *abo,
>   			 unsigned long cur_seq)
>   {
> -	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
> -	struct drm_gem_object *gobj = to_gobj(abo);
> -	long ret;
> -
> -	ret = dma_resv_wait_timeout(gobj->resv, DMA_RESV_USAGE_BOOKKEEP,
> -				    true, MAX_SCHEDULE_TIMEOUT);
> -	if (!ret)
> -		XDNA_ERR(xdna, "Failed to wait for bo, ret %ld", ret);
> -	else if (ret == -ERESTARTSYS)
> -		XDNA_DBG(xdna, "Wait for bo interrupted by signal");
> +	/*
> +	 * The pages must not be released while the device may still access
> +	 * them, so this wait must be neither interruptible nor time limited.
> +	 */
> +	dma_resv_wait_timeout(to_gobj(abo)->resv, DMA_RESV_USAGE_BOOKKEEP,
> +			      false, MAX_SCHEDULE_TIMEOUT);
>   }
>   
>   int aie2_hwctx_heap_expand(struct amdxdna_hwctx *hwctx,

Re: [PATCH V1] accel/amdxdna: wait uninterruptibly for jobs in the HMM invalidate callback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: circular locking deadlock between MMU notifier and IOMMU page fault
--

commit b2f7ee3cb752f6d69d4b4ef2181868d9befe8e3c
Author: Matthew Brost <matthew.brost@intel.com>

accel/amdxdna: wait uninterruptibly for jobs in the HMM invalidate callback

This commit changes the BO wait in aie2_hmm_invalidate() to be
uninterruptible and not time-limited, since MMU notifiers cannot fail a
blockable invalidation.

> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index d927c8c9d557a..9d38703501963 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -1280,16 +1280,12 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job,
>  void aie2_hmm_invalidate(struct amdxdna_gem_obj *abo,
>  			 unsigned long cur_seq)
>  {
> -	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
> -	struct drm_gem_object *gobj = to_gobj(abo);
> -	long ret;
> -
> -	ret = dma_resv_wait_timeout(gobj->resv, DMA_RESV_USAGE_BOOKKEEP,
> -				    true, MAX_SCHEDULE_TIMEOUT);
> -	if (!ret)
> -		XDNA_ERR(xdna, "Failed to wait for bo, ret %ld", ret);
> -	else if (ret == -ERESTARTSYS)
> -		XDNA_DBG(xdna, "Wait for bo interrupted by signal");
> +	/*
> +	 * The pages must not be released while the device may still access
> +	 * them, so this wait must be neither interruptible nor time limited.
> +	 */
> +	dma_resv_wait_timeout(to_gobj(abo)->resv, DMA_RESV_USAGE_BOOKKEEP,
> +			      false, MAX_SCHEDULE_TIMEOUT);

[Severity: High]
Does this introduce a circular locking deadlock with the IOMMU page fault
handler?

Since aie2_hmm_invalidate() is called from MMU interval notifiers (for
example, during munmap), it executes with the mmap_lock held for writing. By
performing an uninterruptible wait here for the BO's fences to signal, we
block waiting on the GPU job.

If that GPU job triggers an IOMMU page fault (IOPF), iommu_sva_handle_iopf()
will attempt to acquire mmap_read_lock() to handle the fault:

drivers/iommu/iommu-sva.c:iommu_sva_handle_iopf() {
    ...
    mmap_read_lock(mm);
    ...
    ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
    ...
}

This creates a circular dependency:
1. The MMU notifier holds mmap_lock and waits for the GPU job.
2. The GPU job stalls waiting for the IOPF handler.
3. The IOPF handler waits for the mmap_lock.

Additionally, if the Timeout Detection and Recovery (TDR) is disabled
(which is supported via the tdr_timeout_ms module parameter), this deadlock
is permanent and freezes the process. Even with TDR enabled, resolving a
page fault by timing out and resetting the hardware is a functional regression
and a severe performance penalty.

>  }
>  
>  int aie2_hwctx_heap_expand(struct amdxdna_hwctx *hwctx,

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015183248.3414119-1-lizhi.hou@amd.com?part=1