Re: [PATCH V1] accel/amdxdna: Serialize HMM range faults on one umap

[Lizhi Hou <lizhi.hou@amd.com>]

On 10/12/26 21:38, Matthew Brost wrote:
> On Mon, Oct 12, 2026 at 09:10:15PM -0700, Max Zhen wrote:
>>
>> On 10/12/2026 Mon 13:31, Lizhi Hou wrote:
>>> Two hardware contexts can submit jobs that use the same GEM object while
>>> a user mapping is invalid. aie2_cmd_submit() drops the reservation and
>>> notifier_lock before aie2_populate_range(), and that function takes a
>>> reference on the first invalid umap without clearing invalid until the
>>> fault returns. Both submissions then call
>>> hmm_range_fault_unlocked_timeout() on the same struct hmm_range. That
>>> structure is not safe for concurrent callers. The calls write
>>> notifier_seq and hmm_pfns[] at the same time, so mmu_interval_read_retry()
>>> can see the other call's sequence, miss an invalidation, and clear
>>> invalid. The job is then pushed while the pages may already be gone.
>>> Give each umap its own mutex and hold it from the fault through the retry
>>> check and the store that clears invalid. notifier_lock is dropped before
>>> this mutex is taken, and the fault drops mmap_lock before notifier_lock
>>> is taken again.
>>>
>>> Fixes: aac243092b70 ("accel/amdxdna: Add command execution")
>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>> Reviewed-by: Max Zhen <max.zhen@amd.com>
> I was browsing dri-devel and this hmm interaction annoyed me enough to
> ask my AI to fix this and converge properly on a DRM framework:
>
> https://gitlab.freedesktop.org/mbrost/xe-kernel-driver-svn-perf-6-15-2025/-/commits/local_dev/amd_hmm_cleanup
>
> Improper HMM or abuse affects the entire kernel, hence we should
> converge.
>
> Please consider the AI suggestions (last 3 patches) instead of open
> coding this.

Sure. Using gpusvm interfaces would be great. I will patch these on my 
system and see how it works.

In the meanwhile, for back porting, will it still worth to merge this 
patch?


Also, we are working on aie4 hardware support which also relies on hmm. 
Thus, aie2_populate_range() will be renamed to amdxdna_populate_range() 
and moved to amdxdna_gem.c.

https://lore.kernel.org/dri-devel/20261009225343.3079074-16-yidong.zhang@amd.com/

As a great enhancement, would it be ok to put your last patch on top of 
aie4 patches?


And this patch in aie4 series tried to fix the same issue been pointed 
out by your second patch.

https://lore.kernel.org/dri-devel/20261009225343.3079074-15-yidong.zhang@amd.com/

Does it look ok to you? ("Fixes" tag needs to be added).


Thanks a lot,

Lizhi

>
> Matt
>
>>> ---
>>>    drivers/accel/amdxdna/aie2_ctx.c    | 7 ++++++-
>>>    drivers/accel/amdxdna/amdxdna_gem.c | 2 ++
>>>    drivers/accel/amdxdna/amdxdna_gem.h | 1 +
>>>    3 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
>>> index d927c8c9d557..4db67a789c34 100644
>>> --- a/drivers/accel/amdxdna/aie2_ctx.c
>>> +++ b/drivers/accel/amdxdna/aie2_ctx.c
>>> @@ -1155,19 +1155,24 @@ static int aie2_populate_range(struct amdxdna_gem_obj *abo)
>>>    		return -EFAULT;
>>>    	}
>>> +	mutex_lock(&mapp->lock);
>>>    	ret = hmm_range_fault_unlocked_timeout(&mapp->range, timeout);
>>> -	if (ret)
>>> +	if (ret) {
>>> +		mutex_unlock(&mapp->lock);
>>>    		goto put_mm;
>>> +	}
>>>    	down_write(&xdna->notifier_lock);
>>>    	if (mmu_interval_read_retry(&mapp->notifier, mapp->range.notifier_seq)) {
>>>    		up_write(&xdna->notifier_lock);
>>> +		mutex_unlock(&mapp->lock);
>>>    		amdxdna_umap_put(mapp);
>>>    		mmput(mm);
>>>    		goto again;
>>>    	}
>>>    	mapp->invalid = false;
>>>    	up_write(&xdna->notifier_lock);
>>> +	mutex_unlock(&mapp->lock);
>>>    	amdxdna_umap_put(mapp);
>>>    	mmput(mm);
>>>    	goto again;
>>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
>>> index f14484bc31ec..37bfa5058095 100644
>>> --- a/drivers/accel/amdxdna/amdxdna_gem.c
>>> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
>>> @@ -318,6 +318,7 @@ static void amdxdna_umap_release(struct kref *ref)
>>>    	struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt);
>>>    	mmu_interval_notifier_remove(&mapp->notifier);
>>> +	mutex_destroy(&mapp->lock);
>>>    	kvfree(mapp->range.hmm_pfns);
>>>    	kfree(mapp);
>>>    }
>>> @@ -402,6 +403,7 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
>>>    		goto free_pfns;
>>>    	}
>>> +	mutex_init(&mapp->lock);
>>>    	down_write(&xdna->notifier_lock);
>>>    	if (list_empty(&abo->mem.umap_list))
>>>    		abo->mem.uva = addr;
>>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
>>> index 9b4aa21a37c9..456a537eb68c 100644
>>> --- a/drivers/accel/amdxdna/amdxdna_gem.h
>>> +++ b/drivers/accel/amdxdna/amdxdna_gem.h
>>> @@ -19,6 +19,7 @@ struct amdxdna_umap {
>>>    	struct kref			refcnt;
>>>    	bool				invalid;
>>>    	bool				unmapped;
>>> +	struct mutex			lock; /* protect range */
>>>    };
>>>    struct amdxdna_mem {