[PATCH] drm/amdkfd: Handle lack of READ permissions in SVM mapping

[Felix Kuehling]

On 2025-08-08 15:47, Russell, Kent wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
>> -----Original Message-----
>> From: Kuehling, Felix <Felix.Kuehling at amd.com>
>> Sent: Friday, August 8, 2025 3:34 PM
>> To: Russell, Kent <Kent.Russell at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdkfd: Handle lack of READ permissions in SVM
>> mapping
>>
>> On 2025-08-05 10:57, Kent Russell wrote:
>>> HMM assumes that pages have READ permissions by default. Inside
>>> svm_range_validate_and_map, we add READ permissions then add WRITE
>>> permissions if the VMA isn't read-only. This will conflict with regions
>>> that only have PROT_WRITE or have PROT_NONE. When that happens,
>>> svm_range_restore_work will continue to retry, silently, giving the
>>> impression of a hang if pr_debug isn't enabled to show the retries..
>>>
>>> If pages don't have READ permissions, simply unmap them and continue. If
>>> they weren't mapped in the first place, this would be a no-op. Since x86
>>> doesn't support write-only, and PROT_NONE doesn't allow reads or writes
>>> anyways, this will allow the svm range validation to continue without
>>> getting stuck in a loop forever on mappings we can't use with HMM.
>>>
>>> Signed-off-by: Kent Russell <kent.russell at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index e23b5a0f31f2..449595aab433 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -1713,6 +1713,28 @@ static int svm_range_validate_and_map(struct
>> mm_struct *mm,
>>>                      next = min(vma->vm_end, end);
>>>                      npages = (next - addr) >> PAGE_SHIFT;
>>> +                   /* HMM requires at least READ permissions. If provided with
>> PROT_NONE,
>>> +                    * unmap the memory. If it's not already mapped, this is a no-
>> op
>>> +                    * If PROT_WRITE is provided without READ, warn first then
>> unmap
>>> +                    */
>>> +                   if (!(vma->vm_flags & VM_READ)) {
>>> +                           unsigned long e, s;
>>> +
>>> +                           svm_range_lock(prange);
>>> +                           if (vma->vm_flags & VM_WRITE)
>>> +                                   pr_debug("VM_WRITE without VM_READ is
>> not supported");
>>> +                           s = max(start, prange->start);
>>> +                           e = min(end, prange->last);
>>> +                           if (e >= s)
>>> +                                   r = svm_range_unmap_from_gpus(prange, s,
>> e,
>>> +
>> KFD_SVM_UNMAP_TRIGGER_UNMAP_FROM_CPU);
>>> +                           addr = next;
>> Maybe move this as the last statement before continue below.
> Do you mean just the addr=next line? IE Not worrying about setting addr while holding the prange lock?

Yes. Setting addr=next sets things up for the next loop iteration. 
Therefore it seems logical to see this just before the continue, rather 
than "hiding" it between locking and error handling.

Regards,
   Felix


>
>>> +                           svm_range_unlock(prange);
>>> +                           if (r)
>>> +                                   return r;
>> This will skip some cleanup, including svm_range_unreserve_bos and
>> kfree(ctx). I think you can just continue in any case. If r != 0 the
>> loop will terminate.
> Thanks, I missed the !r in the for loop conditions.
>
>   Kent
>
>> Regards,
>>     Felix
>>
>>
>>> +                           continue;
>>> +                   }
>>> +
>>>                      WRITE_ONCE(p->svms.faulting_task, current);
>>>                      r = amdgpu_hmm_range_get_pages(&prange->notifier, addr,
>> npages,
>>>                                                     readonly, owner, NULL,