On Tue, Mar 31, 2026 at 09:57:12PM -0700, Matthew Brost wrote:
> On Tue, Mar 31, 2026 at 10:26:35AM -0700, Lizhi Hou wrote:
> > From: Max Zhen <[email protected]>
> >
> > Update the amdxdna user-pointer (ubuf) BO path to support creating buffer
> > objects from read-only user mappings.
> >
> > Detect read-only VMAs by checking VMA permissions across all user virtual
> > address ranges associated with the BO. When all entries are read-only, pin
> > user pages without FOLL_WRITE and export the resulting dmabuf as read-only
> > (O_RDONLY).
> >
> > This allows userptr BOs backed by read-only mappings to be safely imported
> > and used without requiring write access, which was previously rejected due
> > to unconditional FOLL_WRITE usage.
> >
> > Signed-off-by: Max Zhen <[email protected]>
> > Signed-off-by: Lizhi Hou <[email protected]>
> > ---
> > drivers/accel/amdxdna/amdxdna_ubuf.c | 29 ++++++++++++++++++++++++++--
> > 1 file changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c
> > b/drivers/accel/amdxdna/amdxdna_ubuf.c
> > index 4c0647057759..3769210c55cc 100644
> > --- a/drivers/accel/amdxdna/amdxdna_ubuf.c
> > +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
> > @@ -125,6 +125,26 @@ static const struct dma_buf_ops
> > amdxdna_ubuf_dmabuf_ops = {
> > .vunmap = amdxdna_ubuf_vunmap,
> > };
> >
> > +static int readonly_va_entry(struct amdxdna_drm_va_entry *va_ent)
> > +{
> > + struct mm_struct *mm = current->mm;
> > + struct vm_area_struct *vma;
> > + int ret;
> > +
> > + mmap_read_lock(mm);
> > +
> > + vma = find_vma(mm, va_ent->vaddr);
> > + if (!vma ||
> > + vma->vm_start > va_ent->vaddr ||
> > + vma->vm_end - va_ent->vaddr < va_ent->len)
> > + ret = -ENOENT;
> > + else
> > + ret = vma->vm_flags & VM_WRITE ? 0 : 1;
> > +
> > + mmap_read_unlock(mm);
>
>
> This looks highly questionable. Drivers should be reaching into the core
s/should/shouldn't
Matt
> MM to create primitives.
>
> I also glanced at the userptr implementation here — it’s quite
> questionable as well, especially regarding whether notifier locking /
> hmm_range_fault interaction is needed on the driver side.
>
> I’m fairly certain that, with a bit of thought and some extensions to
> DRM GPUSVM, amdxdna could build userptr on that layer (Xe does this
> wihtout SVM). That would isolate core MM interactions to the common DRM
> layer, which I believe the core MM folks would appreciate.
>
> The biggest issue I see is that get_pages() in GPUSVM also performs a
> DMA map, which amdxdna doesn’t appear to need. That should be easy
> enough to split out. But amdxdna does need locking semantics, notifiers,
> etc., which GPUSVM already provides.
>
> I’d rather see GPUSVM expanded for the amdxdna use case so future
> drivers can use it as well.
>
> Happy to work with you on this.
>
> Matt
>
> > + return ret;
> > +}
> > +
> > struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
> > u32 num_entries, void __user *va_entries)
> > {
> > @@ -134,6 +154,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
> > struct amdxdna_ubuf_priv *ubuf;
> > u32 npages, start = 0;
> > struct dma_buf *dbuf;
> > + bool readonly = true;
> > int i, ret;
> > DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> >
> > @@ -172,6 +193,10 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device
> > *dev,
> > ret = -EINVAL;
> > goto free_ent;
> > }
> > +
> > + /* Pin pages as writable as long as not all entries are
> > read-only. */
> > + if (readonly && readonly_va_entry(&va_ent[i]) != 1)
> > + readonly = false;
> > }
> >
> > ubuf->nr_pages = exp_info.size >> PAGE_SHIFT;
> > @@ -194,7 +219,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
> > npages = va_ent[i].len >> PAGE_SHIFT;
> >
> > ret = pin_user_pages_fast(va_ent[i].vaddr, npages,
> > - FOLL_WRITE | FOLL_LONGTERM,
> > + (readonly ? 0 : FOLL_WRITE) |
> > FOLL_LONGTERM,
> > &ubuf->pages[start]);
> > if (ret >= 0) {
> > start += ret;
> > @@ -211,7 +236,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
> >
> > exp_info.ops = &amdxdna_ubuf_dmabuf_ops;
> > exp_info.priv = ubuf;
> > - exp_info.flags = O_RDWR | O_CLOEXEC;
> > + exp_info.flags = (readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC;
> >
> > dbuf = dma_buf_export(&exp_info);
> > if (IS_ERR(dbuf)) {
> > --
> > 2.34.1
> >
