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

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

Applied to drm-misc-next for a quick fix.

And I will converge it to gpusvm after aie4 changes.


Thanks,

Lizhi

On 10/13/26 09:36, Lizhi Hou wrote:
>
> 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 {