Re: [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.kuehl...@amd.com>
Sent: Friday, August 8, 2025 3:34 PM
To: Russell, Kent <kent.russ...@amd.com>; amd-gfx@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.russ...@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,