* [PATCH V2] accel/amdxdna: Drop dma-buf wrapping for ubuf
@ 2026-09-21 21:14 Lizhi Hou
2026-09-21 21:32 ` sashiko-bot
0 siblings, 1 reply; 2+ messages in thread
From: Lizhi Hou @ 2026-09-21 21:14 UTC (permalink / raw)
To: ogabbay, quic_jhugo, dri-devel, mario.limonciello,
karol.wachowski, max.zhen, Christian.Koenig, simona
Cc: Lizhi Hou, linux-kernel, linaro-mm-sig, sonal.santan
A ubuf buffer is a range of existing userspace memory passed in through
CREATE_BO. The driver currently wraps those pages in a dma-buf, then
imports that dma-buf as a GEM object. The extra layer does not add a
sharing path: ubuf is not exportable, and the pages never belonged to
the driver in the first place.
Make ubuf a DRM GEM private object and tracks the user VA with HMM
instead of a long-term pin.
- Create the BO directly from the VA table without going through dma-buf.
Export stays unsupported.
- Drop pin_user_pages(FOLL_LONGTERM). Create registers an MMU interval
notifier over the user range and marks it invalid. Command submit
faults the range in if it is still invalid. That only works with
PASID, which is the only mode where ubuf is supported.
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
V2:
Fix sashiko comment: remove holding process's virtual address space.
drivers/accel/amdxdna/amdxdna_gem.c | 85 ++++++-----
drivers/accel/amdxdna/amdxdna_gem.h | 12 ++
drivers/accel/amdxdna/amdxdna_ubuf.c | 207 ++++++++++-----------------
drivers/accel/amdxdna/amdxdna_ubuf.h | 8 +-
4 files changed, 138 insertions(+), 174 deletions(-)
diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index f4832337ec31..a3d9a54c6563 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -164,8 +164,7 @@ void amdxdna_gem_heap_free(struct amdxdna_client *client, struct amdxdna_gem_obj
mutex_unlock(&client->mm_lock);
}
-static void
-amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
+void amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
{
mutex_destroy(&abo->lock);
kfree(abo);
@@ -190,7 +189,9 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
if (!abo->mem.kva) {
ret = drm_gem_vmap(to_gobj(abo), &map);
- if (ret)
+ if (ret == -EOPNOTSUPP)
+ XDNA_DBG(xdna, "Vmap bo is not supported");
+ else if (ret)
XDNA_ERR(xdna, "Vmap bo failed, ret %d", ret);
else
abo->mem.kva = map.vaddr;
@@ -291,7 +292,7 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
up_write(&xdna->notifier_lock);
}
-static void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force)
+void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force)
{
struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
struct amdxdna_umap *mapp, *tmp;
@@ -335,12 +336,11 @@ static void amdxdna_hmm_unreg_work(struct work_struct *work)
amdxdna_hmm_unreg_umaps(abo, false);
}
-static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
- struct vm_area_struct *vma)
+int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, struct vm_area_struct *vma,
+ size_t offset, size_t len)
{
struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
- unsigned long len = vma->vm_end - vma->vm_start;
- unsigned long addr = vma->vm_start;
+ unsigned long addr = vma->vm_start + offset;
struct amdxdna_umap *mapp;
unsigned long nr_pages;
int ret;
@@ -359,7 +359,7 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
down_read(&xdna->notifier_lock);
list_for_each_entry(mapp, &abo->mem.umap_list, node) {
- if (compare_range(mapp, current->mm, addr, addr + len)) {
+ if (compare_range(mapp, vma->vm_mm, addr, addr + len)) {
up_read(&xdna->notifier_lock);
return 0;
}
@@ -371,15 +371,15 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
return -ENOMEM;
nr_pages = (PAGE_ALIGN(addr + len) - (addr & PAGE_MASK)) >> PAGE_SHIFT;
- mapp->range.hmm_pfns = kvzalloc_objs(*mapp->range.hmm_pfns, nr_pages);
+ mapp->range.hmm_pfns = kvzalloc_objs(*mapp->range.hmm_pfns, nr_pages, GFP_KERNEL_ACCOUNT);
if (!mapp->range.hmm_pfns) {
ret = -ENOMEM;
goto free_map;
}
mapp->range.notifier = &mapp->notifier;
- mapp->range.start = vma->vm_start;
- mapp->range.end = vma->vm_end;
+ mapp->range.start = addr;
+ mapp->range.end = addr + len;
/*
* Access permissions are fixed at mmap() time. Changing them later
* with mprotect() is not supported: the range keeps requesting the
@@ -393,7 +393,7 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
kref_init(&mapp->refcnt);
ret = mmu_interval_notifier_insert_locked(&mapp->notifier,
- current->mm,
+ vma->vm_mm,
addr,
len,
&amdxdna_hmm_ops);
@@ -417,7 +417,7 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
return ret;
}
-static struct amdxdna_gem_obj *
+struct amdxdna_gem_obj *
amdxdna_gem_create_obj(struct drm_device *dev, size_t size)
{
struct amdxdna_gem_obj *abo;
@@ -462,8 +462,9 @@ static void amdxdna_gem_dev_obj_free(struct drm_gem_object *gobj)
amdxdna_gem_destroy_obj(abo);
}
-static void amdxdna_mark_mapp_invalid(struct amdxdna_gem_obj *abo,
- struct vm_area_struct *vma)
+void amdxdna_mark_mapp_invalid(struct amdxdna_gem_obj *abo,
+ struct mm_struct *mm,
+ unsigned long start, unsigned long end)
{
struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
struct amdxdna_umap *mapp;
@@ -471,7 +472,7 @@ static void amdxdna_mark_mapp_invalid(struct amdxdna_gem_obj *abo,
down_write(&xdna->notifier_lock);
abo->mem.map_invalid = true;
list_for_each_entry(mapp, &abo->mem.umap_list, node) {
- if (compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) {
+ if (compare_range(mapp, mm, start, end)) {
mapp->invalid = true;
break;
}
@@ -502,7 +503,7 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo,
return ret;
}
- amdxdna_mark_mapp_invalid(abo, vma);
+ amdxdna_mark_mapp_invalid(abo, vma->vm_mm, vma->vm_start, vma->vm_end);
/* Drop the reference drm_gem_mmap_obj() acquired.*/
drm_gem_object_put(to_gobj(abo));
@@ -539,7 +540,7 @@ static int amdxdna_gem_obj_mmap(struct drm_gem_object *gobj,
drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
vma->vm_start, gobj->size);
- ret = amdxdna_hmm_register(abo, vma);
+ ret = amdxdna_hmm_register(abo, vma, 0, vma->vm_end - vma->vm_start);
if (ret)
return ret;
@@ -682,7 +683,7 @@ static struct dma_buf *amdxdna_gem_prime_export(struct drm_gem_object *gobj, int
struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
- if (abo->private_buffer)
+ if (is_private_bo(abo))
return ERR_PTR(-EOPNOTSUPP);
if (abo->dma_buf) {
@@ -766,7 +767,7 @@ static void amdxdna_gem_obj_free(struct drm_gem_object *gobj)
drm_gem_shmem_free(&abo->base);
}
-static int amdxdna_gem_obj_open(struct drm_gem_object *gobj, struct drm_file *filp)
+int amdxdna_gem_obj_open(struct drm_gem_object *gobj, struct drm_file *filp)
{
struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
@@ -805,7 +806,7 @@ static int amdxdna_gem_obj_open(struct drm_gem_object *gobj, struct drm_file *fi
return 0;
}
-static void amdxdna_gem_obj_close(struct drm_gem_object *gobj, struct drm_file *filp)
+void amdxdna_gem_obj_close(struct drm_gem_object *gobj, struct drm_file *filp)
{
struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
struct amdxdna_client *client = NULL;
@@ -991,40 +992,46 @@ amdxdna_gem_create_shmem_object(struct drm_device *dev, struct amdxdna_drm_creat
}
static struct amdxdna_gem_obj *
-amdxdna_gem_create_ubuf_object(struct drm_device *dev, struct amdxdna_drm_create_bo *args)
+amdxdna_gem_create_ubuf_object(struct drm_device *dev,
+ struct amdxdna_drm_create_bo *args,
+ struct drm_file *filp)
{
+ struct amdxdna_client *client = filp->driver_priv;
struct amdxdna_dev *xdna = to_xdna_dev(dev);
struct amdxdna_drm_va_tbl va_tbl;
struct amdxdna_gem_obj *abo;
struct drm_gem_object *gobj;
struct dma_buf *dma_buf;
+ if (args->type == AMDXDNA_BO_DEV_HEAP || args->type == AMDXDNA_BO_CMD)
+ return ERR_PTR(-EOPNOTSUPP);
+
if (copy_from_user(&va_tbl, u64_to_user_ptr(args->vaddr), sizeof(va_tbl))) {
XDNA_DBG(xdna, "Access va table failed");
return ERR_PTR(-EINVAL);
}
if (va_tbl.num_entries) {
- dma_buf = amdxdna_get_ubuf(dev, va_tbl.num_entries,
- u64_to_user_ptr(args->vaddr + sizeof(va_tbl)));
+ abo = amdxdna_alloc_ubuf_bo(client, va_tbl.num_entries,
+ u64_to_user_ptr(args->vaddr + sizeof(va_tbl)));
+ if (IS_ERR(abo))
+ return abo;
} else {
dma_buf = dma_buf_get(va_tbl.dmabuf_fd);
- }
+ if (IS_ERR(dma_buf))
+ return ERR_CAST(dma_buf);
- if (IS_ERR(dma_buf))
- return ERR_CAST(dma_buf);
+ gobj = amdxdna_gem_prime_import(dev, dma_buf);
+ if (IS_ERR(gobj)) {
+ dma_buf_put(dma_buf);
+ return ERR_CAST(gobj);
+ }
- gobj = amdxdna_gem_prime_import(dev, dma_buf);
- if (IS_ERR(gobj)) {
dma_buf_put(dma_buf);
- return ERR_CAST(gobj);
+ abo = to_xdna_obj(gobj);
}
- dma_buf_put(dma_buf);
-
- abo = to_xdna_obj(gobj);
abo->private_buffer = true;
-
return abo;
}
@@ -1112,7 +1119,7 @@ amdxdna_drm_create_share_bo(struct drm_device *dev,
struct amdxdna_gem_obj *abo;
if (args->vaddr)
- abo = amdxdna_gem_create_ubuf_object(dev, args);
+ abo = amdxdna_gem_create_ubuf_object(dev, args, filp);
else if (amdxdna_use_carveout(to_xdna_dev(dev)))
abo = amdxdna_gem_create_cbuf_object(dev, args);
else
@@ -1293,7 +1300,7 @@ static int amdxdna_bo_pin(struct amdxdna_gem_obj *abo)
struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
int ret;
- if (is_import_bo(abo))
+ if (is_import_bo(abo) || is_private_bo(abo))
return 0;
ret = drm_gem_shmem_pin(&abo->base);
@@ -1306,7 +1313,7 @@ static void amdxdna_bo_unpin(struct amdxdna_gem_obj *abo)
{
struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
- if (is_import_bo(abo))
+ if (is_import_bo(abo) || is_private_bo(abo))
return;
drm_gem_shmem_unpin(&abo->base);
@@ -1406,7 +1413,7 @@ int amdxdna_drm_get_bo_info_ioctl(struct drm_device *dev, void *data, struct drm
args->vaddr = amdxdna_gem_uva(abo);
args->xdna_addr = amdxdna_gem_dev_addr(abo);
- if (abo->type != AMDXDNA_BO_DEV)
+ if (abo->type != AMDXDNA_BO_DEV && !is_private_bo(abo))
args->map_offset = drm_vma_node_offset_addr(&gobj->vma_node);
else
args->map_offset = AMDXDNA_INVALID_ADDR;
diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
index 9b4aa21a37c9..815348a3c429 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.h
+++ b/drivers/accel/amdxdna/amdxdna_gem.h
@@ -60,6 +60,7 @@ struct amdxdna_gem_obj {
#define to_gobj(obj) (&(obj)->base.base)
#define is_import_bo(obj) ((obj)->attach)
+#define is_private_bo(obj) ((obj)->private_buffer)
static inline struct amdxdna_gem_obj *to_xdna_obj(struct drm_gem_object *gobj)
{
@@ -112,10 +113,21 @@ amdxdna_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf);
struct amdxdna_gem_obj *
amdxdna_drm_create_dev_bo(struct drm_device *dev,
struct amdxdna_drm_create_bo *args, struct drm_file *filp);
+struct amdxdna_gem_obj *
+amdxdna_gem_create_obj(struct drm_device *dev, size_t size);
+void amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo);
+void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force);
int amdxdna_gem_pin_nolock(struct amdxdna_gem_obj *abo);
int amdxdna_gem_pin(struct amdxdna_gem_obj *abo);
void amdxdna_gem_unpin(struct amdxdna_gem_obj *abo);
+int amdxdna_gem_obj_open(struct drm_gem_object *gobj, struct drm_file *filp);
+void amdxdna_gem_obj_close(struct drm_gem_object *gobj, struct drm_file *filp);
+int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, struct vm_area_struct *vma,
+ size_t offset, size_t len);
+void amdxdna_mark_mapp_invalid(struct amdxdna_gem_obj *abo,
+ struct mm_struct *mm,
+ unsigned long start, unsigned long end);
int amdxdna_drm_create_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
int amdxdna_drm_get_bo_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
index 0e0cd69cd1fb..5c291786d981 100644
--- a/drivers/accel/amdxdna/amdxdna_ubuf.c
+++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
@@ -6,182 +6,127 @@
#include <drm/amdxdna_accel.h>
#include <drm/drm_device.h>
#include <drm/drm_print.h>
-#include <linux/dma-buf.h>
#include <linux/overflow.h>
#include <linux/pagemap.h>
#include <linux/vmalloc.h>
+#include "amdxdna_gem.h"
#include "amdxdna_pci_drv.h"
#include "amdxdna_ubuf.h"
-struct amdxdna_ubuf_priv {
- struct page **pages;
- u64 nr_pages;
- struct mm_struct *mm;
-};
-
-static struct sg_table *amdxdna_ubuf_map(struct dma_buf_attachment *attach,
- enum dma_data_direction direction)
+static int amdxdna_ubuf_hmm_register(struct amdxdna_client *client,
+ struct amdxdna_gem_obj *abo,
+ struct amdxdna_drm_va_entry *va_ent)
{
- struct amdxdna_ubuf_priv *ubuf = attach->dmabuf->priv;
- struct sg_table *sg;
+ struct vm_area_struct *vma;
int ret;
- sg = kzalloc_obj(*sg);
- if (!sg)
- return ERR_PTR(-ENOMEM);
-
- ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->nr_pages, 0,
- ubuf->nr_pages << PAGE_SHIFT, GFP_KERNEL);
- if (ret)
- goto err_free_sg;
+ mmap_write_lock(client->mm);
+ vma = find_vma(client->mm, va_ent->vaddr);
+ if (!vma || vma->vm_start > va_ent->vaddr ||
+ vma->vm_end - va_ent->vaddr < va_ent->len) {
+ ret = -EINVAL;
+ goto unlock;
+ }
- ret = dma_map_sgtable(attach->dev, sg, direction, 0);
- if (ret)
- goto err_free_table;
+ ret = amdxdna_hmm_register(abo, vma, va_ent->vaddr - vma->vm_start, va_ent->len);
- return sg;
+unlock:
+ mmap_write_unlock(client->mm);
-err_free_table:
- sg_free_table(sg);
-err_free_sg:
- kfree(sg);
- return ERR_PTR(ret);
+ return ret;
}
-static void amdxdna_ubuf_unmap(struct dma_buf_attachment *attach,
- struct sg_table *sg,
- enum dma_data_direction direction)
+static void amdxdna_gem_ubuf_obj_free(struct drm_gem_object *gobj)
{
- dma_unmap_sgtable(attach->dev, sg, direction, 0);
- sg_free_table(sg);
- kfree(sg);
+ struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
+
+ amdxdna_hmm_unreg_umaps(abo, true);
+ cancel_work_sync(&abo->hmm_unreg_work);
+
+ drm_gem_object_release(gobj);
+ amdxdna_gem_destroy_obj(abo);
}
-static void amdxdna_ubuf_release(struct dma_buf *dbuf)
+static struct dma_buf *amdxdna_gem_ubuf_obj_export(struct drm_gem_object *gobj, int flags)
{
- struct amdxdna_ubuf_priv *ubuf = dbuf->priv;
-
- unpin_user_pages(ubuf->pages, ubuf->nr_pages);
- kvfree(ubuf->pages);
- atomic64_sub(ubuf->nr_pages, &ubuf->mm->pinned_vm);
- mmdrop(ubuf->mm);
- kfree(ubuf);
+ return ERR_PTR(-EOPNOTSUPP);
}
-static const struct dma_buf_ops amdxdna_ubuf_dmabuf_ops = {
- .map_dma_buf = amdxdna_ubuf_map,
- .unmap_dma_buf = amdxdna_ubuf_unmap,
- .release = amdxdna_ubuf_release,
+static const struct drm_gem_object_funcs amdxdna_gem_ubuf_obj_funcs = {
+ .free = amdxdna_gem_ubuf_obj_free,
+ .open = amdxdna_gem_obj_open,
+ .close = amdxdna_gem_obj_close,
+ .export = amdxdna_gem_ubuf_obj_export,
};
-struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
- u32 num_entries, void __user *va_entries)
+struct amdxdna_gem_obj *amdxdna_alloc_ubuf_bo(struct amdxdna_client *client,
+ u32 num_entries, void __user *va_entries)
{
- struct amdxdna_dev *xdna = to_xdna_dev(dev);
- unsigned long lock_limit, new_pinned;
+ struct amdxdna_dev *xdna = client->xdna;
struct amdxdna_drm_va_entry *va_ent;
- struct amdxdna_ubuf_priv *ubuf;
- u32 npages, start = 0;
- struct dma_buf *dbuf;
- int i, ret;
- DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+ struct amdxdna_gem_obj *abo;
+ size_t bufsize;
+ long ret;
- if (!can_do_mlock())
- return ERR_PTR(-EPERM);
+ if (!amdxdna_pasid_on(client))
+ return ERR_PTR(-EOPNOTSUPP);
- ubuf = kzalloc_obj(*ubuf);
- if (!ubuf)
- return ERR_PTR(-ENOMEM);
+ /*
+ * There is not any valid case to use more than 1 entry.
+ * Hardcode maximum entries to 1.
+ */
+ if (num_entries > 1)
+ return ERR_PTR(-EINVAL);
- ubuf->mm = current->mm;
- mmgrab(ubuf->mm);
+ if (current->mm != client->mm)
+ return ERR_PTR(-EINVAL);
- va_ent = kvzalloc_objs(*va_ent, num_entries);
- if (!va_ent) {
- ret = -ENOMEM;
- goto free_ubuf;
- }
+ va_ent = kvzalloc_obj(*va_ent);
+ if (!va_ent)
+ return ERR_PTR(-ENOMEM);
- if (copy_from_user(va_ent, va_entries, sizeof(*va_ent) * num_entries)) {
+ if (copy_from_user(va_ent, va_entries, sizeof(*va_ent))) {
XDNA_DBG(xdna, "Access va entries failed");
ret = -EINVAL;
goto free_ent;
}
- for (i = 0, exp_info.size = 0; i < num_entries; i++) {
- if (!IS_ALIGNED(va_ent[i].vaddr, PAGE_SIZE) ||
- !IS_ALIGNED(va_ent[i].len, PAGE_SIZE)) {
- XDNA_ERR(xdna, "Invalid address or len %llx, %llx",
- va_ent[i].vaddr, va_ent[i].len);
- ret = -EINVAL;
- goto free_ent;
- }
-
- if (check_add_overflow(exp_info.size, va_ent[i].len, &exp_info.size)) {
- ret = -EINVAL;
- goto free_ent;
- }
- }
-
- ubuf->nr_pages = exp_info.size >> PAGE_SHIFT;
- lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
- new_pinned = atomic64_add_return(ubuf->nr_pages, &ubuf->mm->pinned_vm);
- if (new_pinned > lock_limit && !capable(CAP_IPC_LOCK)) {
- XDNA_DBG(xdna, "New pin %ld, limit %ld, cap %d",
- new_pinned, lock_limit, capable(CAP_IPC_LOCK));
- ret = -ENOMEM;
- goto sub_pin_cnt;
+ if (!IS_ALIGNED(va_ent->vaddr, PAGE_SIZE) ||
+ !IS_ALIGNED(va_ent->len, PAGE_SIZE) ||
+ !va_ent->len ||
+ check_add_overflow(va_ent->vaddr, va_ent->len, &bufsize)) {
+ XDNA_DBG(xdna, "Invalid address or len %llx, %llx",
+ va_ent->vaddr, va_ent->len);
+ ret = -EINVAL;
+ goto free_ent;
}
- ubuf->pages = kvmalloc_objs(*ubuf->pages, ubuf->nr_pages);
- if (!ubuf->pages) {
- ret = -ENOMEM;
- goto sub_pin_cnt;
+ bufsize = va_ent->len;
+ abo = amdxdna_gem_create_obj(&xdna->ddev, bufsize);
+ if (IS_ERR(abo)) {
+ ret = PTR_ERR(abo);
+ goto free_ent;
}
- for (i = 0; i < num_entries; i++) {
- npages = va_ent[i].len >> PAGE_SHIFT;
-
- ret = pin_user_pages_fast(va_ent[i].vaddr, npages,
- FOLL_WRITE | FOLL_LONGTERM,
- &ubuf->pages[start]);
- if (ret >= 0) {
- start += ret;
- if (ret != npages) {
- XDNA_ERR(xdna, "Partially pinned pages %d/%u", ret, npages);
- ret = -ENOMEM;
- goto destroy_pages;
- }
- } else {
- XDNA_ERR(xdna, "Failed to pin pages ret %d", ret);
- goto destroy_pages;
- }
- }
+ abo->type = AMDXDNA_BO_SHARE;
+ abo->mem.uva = va_ent->vaddr;
+ to_gobj(abo)->funcs = &amdxdna_gem_ubuf_obj_funcs;
+ drm_gem_private_object_init(&xdna->ddev, to_gobj(abo), bufsize);
- exp_info.ops = &amdxdna_ubuf_dmabuf_ops;
- exp_info.priv = ubuf;
- exp_info.flags = O_RDWR | O_CLOEXEC;
+ ret = amdxdna_ubuf_hmm_register(client, abo, va_ent);
+ if (ret)
+ goto put_obj;
- dbuf = dma_buf_export(&exp_info);
- if (IS_ERR(dbuf)) {
- ret = PTR_ERR(dbuf);
- goto destroy_pages;
- }
+ amdxdna_mark_mapp_invalid(abo, client->mm, va_ent->vaddr,
+ va_ent->vaddr + va_ent->len);
kvfree(va_ent);
- return dbuf;
+ return abo;
-destroy_pages:
- if (start)
- unpin_user_pages(ubuf->pages, start);
- kvfree(ubuf->pages);
-sub_pin_cnt:
- atomic64_sub(ubuf->nr_pages, &ubuf->mm->pinned_vm);
+put_obj:
+ drm_gem_object_put(to_gobj(abo));
free_ent:
kvfree(va_ent);
-free_ubuf:
- mmdrop(ubuf->mm);
- kfree(ubuf);
return ERR_PTR(ret);
}
diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.h b/drivers/accel/amdxdna/amdxdna_ubuf.h
index 8900a6dc4371..f6335603da34 100644
--- a/drivers/accel/amdxdna/amdxdna_ubuf.h
+++ b/drivers/accel/amdxdna/amdxdna_ubuf.h
@@ -5,10 +5,10 @@
#ifndef _AMDXDNA_UBUF_H_
#define _AMDXDNA_UBUF_H_
-#include <drm/drm_device.h>
-#include <linux/dma-buf.h>
+#include "amdxdna_gem.h"
+#include "amdxdna_pci_drv.h"
-struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
- u32 num_entries, void __user *va_entries);
+struct amdxdna_gem_obj *amdxdna_alloc_ubuf_bo(struct amdxdna_client *client,
+ u32 num_entries, void __user *va_entries);
#endif /* _AMDXDNA_UBUF_H_ */
--
2.34.1
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH V2] accel/amdxdna: Drop dma-buf wrapping for ubuf
2026-09-21 21:14 [PATCH V2] accel/amdxdna: Drop dma-buf wrapping for ubuf Lizhi Hou
@ 2026-09-21 21:32 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-09-21 21:32 UTC (permalink / raw)
To: Lizhi Hou; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] amdxdna: UBUF BOs trigger kernel WARN_ON and crash when opened by non-PASID client
- [Medium] amdxdna: CREATE_BO ubuf rejects valid multi-VMA user buffers
- [Medium] amdxdna: unprotected assignment to shared abo->mem.uva causes data race
--
commit 1efe7ea3fe610d7e9d53d3b1fead24d8137d077d
Author: Lizhi Hou <lizhi.hou@amd.com>
Subject: accel/amdxdna: Drop dma-buf wrapping for ubuf
This commit drops dma-buf wrapping for ubuf, directly importing userspace
memory as a DRM GEM private object and tracking the user VA with HMM. It
removes the FOLL_LONGTERM pin and relies on an MMU interval notifier to
fault the range in on command submission.
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index f4832337ec31d..a3d9a54c65634 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
[ ... ]
> @@ -335,12 +336,11 @@ static void amdxdna_hmm_unreg_work(struct work_struct *work)
> amdxdna_hmm_unreg_umaps(abo, false);
> }
>
> -static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
> - struct vm_area_struct *vma)
> +int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, struct vm_area_struct *vma,
> + size_t offset, size_t len)
> {
> struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
> - unsigned long len = vma->vm_end - vma->vm_start;
> - unsigned long addr = vma->vm_start;
> + unsigned long addr = vma->vm_start + offset;
[Severity: Medium]
If a process creates a BO, forks, and both parent and child processes
concurrently mmap the inherited file descriptor, they can concurrently reach
the early return path in this function where abo->mem.uva is set to addr.
Does this unprotected assignment to abo->mem.uva cause a data race,
potentially leading to corrupted addresses being returned via GET_BO_INFO?
> struct amdxdna_umap *mapp;
> unsigned long nr_pages;
> int ret;
[ ... ]
> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 0e0cd69cd1fbf..5c291786d981b 100644
> --- a/drivers/accel/amdxdna/amdxdna_ubuf.c
> +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
[ ... ]
> +static int amdxdna_ubuf_hmm_register(struct amdxdna_client *client,
> + struct amdxdna_gem_obj *abo,
> + struct amdxdna_drm_va_entry *va_ent)
> {
> - struct amdxdna_ubuf_priv *ubuf = attach->dmabuf->priv;
> - struct sg_table *sg;
> + struct vm_area_struct *vma;
> int ret;
>
> - sg = kzalloc_obj(*sg);
> - if (!sg)
> - return ERR_PTR(-ENOMEM);
> -
> - ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->nr_pages, 0,
> - ubuf->nr_pages << PAGE_SHIFT, GFP_KERNEL);
> - if (ret)
> - goto err_free_sg;
> + mmap_write_lock(client->mm);
> + vma = find_vma(client->mm, va_ent->vaddr);
> + if (!vma || vma->vm_start > va_ent->vaddr ||
> + vma->vm_end - va_ent->vaddr < va_ent->len) {
> + ret = -EINVAL;
> + goto unlock;
> + }
[Severity: Medium]
If a user calls CREATE_BO with a ubuf that spans multiple VMAs (such as
adjacent mmaps or split VMAs due to mprotect), find_vma() will return the
first VMA.
Does this check against vma->vm_end fail the entire request and incorrectly
return -EINVAL for a valid multi-VMA userspace buffer?
> - ret = dma_map_sgtable(attach->dev, sg, direction, 0);
> - if (ret)
> - goto err_free_table;
> + ret = amdxdna_hmm_register(abo, vma, va_ent->vaddr - vma->vm_start, va_ent->len);
[ ... ]
> +struct amdxdna_gem_obj *amdxdna_alloc_ubuf_bo(struct amdxdna_client *client,
> + u32 num_entries, void __user *va_entries)
> {
[ ... ]
> + bufsize = va_ent->len;
> + abo = amdxdna_gem_create_obj(&xdna->ddev, bufsize);
> + if (IS_ERR(abo)) {
> + ret = PTR_ERR(abo);
> + goto free_ent;
> }
>
> - for (i = 0; i < num_entries; i++) {
> - npages = va_ent[i].len >> PAGE_SHIFT;
> -
> - ret = pin_user_pages_fast(va_ent[i].vaddr, npages,
> - FOLL_WRITE | FOLL_LONGTERM,
> - &ubuf->pages[start]);
> - if (ret >= 0) {
> - start += ret;
> - if (ret != npages) {
> - XDNA_ERR(xdna, "Partially pinned pages %d/%u", ret, npages);
> - ret = -ENOMEM;
> - goto destroy_pages;
> - }
> - } else {
> - XDNA_ERR(xdna, "Failed to pin pages ret %d", ret);
> - goto destroy_pages;
> - }
> - }
> + abo->type = AMDXDNA_BO_SHARE;
[Severity: High]
If a PASID client creates a UBUF BO and exports it via GEM_FLINK, and a
non-PASID client then calls GEM_OPEN on it, the open path calls
amdxdna_dma_map_bo(). Since the type is set to AMDXDNA_BO_SHARE here, the
mapping routine invokes drm_gem_shmem_get_pages_sgt() for this object.
Can this dereference an uninitialized shmem->base.filp and trigger a kernel
WARN_ON and crash, given that UBUF BOs lack shmem initialization?
> + abo->mem.uva = va_ent->vaddr;
> + to_gobj(abo)->funcs = &amdxdna_gem_ubuf_obj_funcs;
> + drm_gem_private_object_init(&xdna->ddev, to_gobj(abo), bufsize);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260921211448.39832-1-lizhi.hou@amd.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-09-21 21:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-21 21:14 [PATCH V2] accel/amdxdna: Drop dma-buf wrapping for ubuf Lizhi Hou
2026-09-21 21:32 ` sashiko-bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.