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

[Russell, Kent]

[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?

>
>
> > +                           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,