Re: [PATCH rdma-next v2 01/15] RDMA/core: Introduce generic buffer descriptor infrastructure for umem

Jiri Pirko
2026-04-13T08:32:21Z

Sun, Apr 12, 2026 at 02:33:22PM +0200, mrgolin@amazon.com wrote:
>On Sat, Apr 11, 2026 at 04:49:01PM +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> Add a unified mechanism for userspace to pass memory buffers to any
>> uverbs command via a single UVERBS_ATTR_BUFFERS attribute. Each
>> buffer is described by struct ib_uverbs_buffer_desc with a type
>> discriminator supporting dma-buf and user VA backed memory, extensible
>> for future buffer types.
>>
>> The ib_umem_list API enables any uverbs command to accept multiple
>> buffers indexed by per-command slot enums, without requiring new UAPI
>> attributes for each buffer. A consumption check ensures userspace and
>> driver agree on which buffers are used.
>>
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> drivers/infiniband/core/umem.c | 248 ++++++++++++++++++++++++
>> include/rdma/ib_umem.h | 54 ++++++
>> include/rdma/uverbs_ioctl.h | 14 ++
>> include/uapi/rdma/ib_user_ioctl_cmds.h | 1 +
>> include/uapi/rdma/ib_user_ioctl_verbs.h | 27 +++
>> 5 files changed, 344 insertions(+)
>>
>> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
>> index 786fa1aa8e55..f5b03e903b9d 100644
>> --- a/drivers/infiniband/core/umem.c
>> +++ b/drivers/infiniband/core/umem.c
>> @@ -37,6 +37,7 @@
>> #include <linux/dma-mapping.h>
>> #include <linux/sched/signal.h>
>> #include <linux/sched/mm.h>
>> +#include <linux/err.h>
>> #include <linux/export.h>
>> #include <linux/slab.h>
>> #include <linux/pagemap.h>
>> @@ -332,3 +333,250 @@ int ib_umem_copy_from(void *dst, struct ib_umem *umem, size_t offset,
>> return 0;
>> }
>> EXPORT_SYMBOL(ib_umem_copy_from);
>> +
>> +struct ib_umem_list {
>> + unsigned int count; /* Total slots in the list. */
>> + unsigned long provided; /* Bitmask of slots provided by the user. */
>> + unsigned long loaded; /* Bitmask of slots loaded by the driver. */
>> + struct ib_umem *umems[] __counted_by(count);
>> +};
>> +
>> +/**
>> + * ib_umem_list_create - Create a umem list from UVERBS_ATTR_BUFFERS
>> + * @device: IB device
>> + * @attrs: uverbs attribute bundle
>> + * @slot_max: highest buffer slot index (count = slot_max + 1)
>> + *
>> + * Return: umem list, or ERR_PTR on failure.
>> + */
>> +struct ib_umem_list *ib_umem_list_create(struct ib_device *device,
>> + const struct uverbs_attr_bundle *attrs,
>> + unsigned int slot_max)
>> +{
>> + const struct ib_uverbs_buffer_desc *descs;
>> + struct ib_umem_dmabuf *umem_dmabuf;
>> + struct ib_umem_list *list;
>> + struct ib_umem *umem;
>> + unsigned int count;
>> + int num_descs;
>> + int err;
>> + int i;
>> +
>> + if (WARN_ON_ONCE(slot_max >= BITS_PER_LONG))
>> + return ERR_PTR(-EINVAL);
>> + count = slot_max + 1;
>> +
>> + num_descs = uverbs_attr_ptr_get_array_size(
>> + (struct uverbs_attr_bundle *)attrs, UVERBS_ATTR_BUFFERS,
>> + sizeof(*descs));
>> + if (num_descs == -ENOENT) {
>> + num_descs = 0;
>> + descs = NULL;
>> + } else if (num_descs < 0) {
>> + return ERR_PTR(num_descs);
>> + } else if (num_descs > count) {
>> + return ERR_PTR(-EINVAL);
>> + } else {
>> + descs = uverbs_attr_get_alloced_ptr(attrs, UVERBS_ATTR_BUFFERS);
>> + if (IS_ERR(descs))
>> + return ERR_CAST(descs);
>> + }
>> +
>> + list = kzalloc(struct_size(list, umems, count), GFP_KERNEL);
>> + if (!list)
>> + return ERR_PTR(-ENOMEM);
>> + list->count = count;
>> +
>> + for (i = 0; i < num_descs; i++) {
>
>While I like the idea of standardizing the way we pass buffer
>information to the kernel, the list thing looks like over generalization
>to me, especially after Leon's refactoring of CQ creation. Maybe we can
>add buffer as a new attribute type that can be used for multiple
>parameters in a command, and have a helper with the code below that
>takes an attribute id and returns a umem object, letting each handler
>store it. This would also make it easier for drivers to pass their
>private buffers using this infrastructure.

Currently we have set of attrs (4) to pass CQ umem. I tried to make this
very smooth for all possible uverbs, passing single attr of array of
structs describing a buffer. Uverb attr api knows how to work with
arrays, all clicks.

Drivers can easily pass their specific buffers over this list too. I
didn't implement it as there was no need, but the idea is to have index>X
for driver specific indexes.

What's the benefit of passing per-uverb attrs with a struct? Perhaps I'm
missing something.

>
>Michael
>
>> + unsigned int idx = descs[i].index;
>> +
>> + if (descs[i].reserved) {
>> + err = -EINVAL;
>> + goto err_release;
>> + }
>> + if (idx >= count || (list->provided & BIT(idx))) {
>> + err = -EINVAL;
>> + goto err_release;
>> + }
>> +
>> + switch (descs[i].type) {
>> + case IB_UVERBS_BUFFER_TYPE_DMABUF:
>> + umem_dmabuf = ib_umem_dmabuf_get_pinned(
>> + device, descs[i].addr, descs[i].length,
>> + descs[i].fd, IB_ACCESS_LOCAL_WRITE);
>> + if (IS_ERR(umem_dmabuf)) {
>> + err = PTR_ERR(umem_dmabuf);
>> + goto err_release;
>> + }
>> + list->umems[idx] = &umem_dmabuf->umem;
>> + break;
>> + case IB_UVERBS_BUFFER_TYPE_VA:
>> + umem = ib_umem_get(device, descs[i].addr,
>> + descs[i].length, IB_ACCESS_LOCAL_WRITE);
>> + if (IS_ERR(umem)) {
>> + err = PTR_ERR(umem);
>> + goto err_release;
>> + }
>> + list->umems[idx] = umem;
>> + break;
>> + default:
>> + err = -EINVAL;
>> + goto err_release;
>> + }
>> + list->provided |= BIT(idx);
>> + }
>> +
>> + return list;
>> +
>> +err_release:
>> + ib_umem_list_release(list);
>> + return ERR_PTR(err);
>> +}
>> +EXPORT_SYMBOL(ib_umem_list_create);
>> +
>> +/**
>> + * ib_umem_list_release - Release all umems in the list and free it
>> + * @list: umem list
>> + */
>> +void ib_umem_list_release(struct ib_umem_list *list)
>> +{
>> + int i;
>> +
>> + if (!list)
>> + return;
>> + for (i = 0; i < list->count; i++)
>> + ib_umem_release(list->umems[i]);
>> + kfree(list);
>> +}
>> +EXPORT_SYMBOL(ib_umem_list_release);
>> +
>> +/**
>> + * ib_umem_list_check_consumed - Verify all provided umems were loaded
>> + * @list: umem list
>> + *
>> + * Return: 0 if all provided slots were loaded, -EINVAL otherwise.
>> + */
>> +int ib_umem_list_check_consumed(const struct ib_umem_list *list)
>> +{
>> + return (list->provided & ~list->loaded) == 0 ? 0 : -EINVAL;
>> +}
>> +EXPORT_SYMBOL(ib_umem_list_check_consumed);
>> +
>> +/**
>> + * ib_umem_list_insert - Insert a umem into the list at a given index
>> + * @list: umem list
>> + * @index: per-command buffer slot index
>> + * @umem: umem pointer to store
>> + *
>> + * Stores @umem at @index (replacing any existing). For use from create_cq
>> + * when the buffer comes from legacy ATTRs rather than the buffer list.
>> + */
>> +void ib_umem_list_insert(struct ib_umem_list *list, unsigned int index,
>> + struct ib_umem *umem)
>> +{
>> + ib_umem_list_replace(list, index, umem);
>> + if (umem)
>> + list->provided |= BIT(index);
>> +}
>> +EXPORT_SYMBOL(ib_umem_list_insert);
>> +
>> +/**
>> + * ib_umem_list_load - Load a umem from the list by index
>> + * @list: umem list (may be NULL)
>> + * @index: per-command buffer slot index
>> + * @size: minimum required umem length
>> + *
>> + * Return: umem pointer, or NULL if the slot is empty or
>> + * the slot is out of bounds, or ERR_PTR(-EINVAL) if the umem is too small.
>> + */
>> +struct ib_umem *ib_umem_list_load(struct ib_umem_list *list,
>> + unsigned int index, size_t size)
>> +{
>> + struct ib_umem *umem;
>> +
>> + if (!list || index >= list->count)
>> + return NULL;
>> + umem = list->umems[index];
>> + if (!umem)
>> + return NULL;
>> + if (umem->length < size)
>> + return ERR_PTR(-EINVAL);
>> + list->loaded |= BIT(index);
>> + return umem;
>> +}
>> +EXPORT_SYMBOL(ib_umem_list_load);
>> +
>> +/**
>> + * ib_umem_list_load_or_get - Umem from list or pin user memory
>> + * @list: umem list (may be NULL)
>> + * @index: per-command buffer slot index
>> + * @device: IB device for ib_umem_get when the list slot is empty
>> + * @addr: user virtual address for ib_umem_get
>> + * @size: length for ib_umem_get
>> + * @access: access flags for ib_umem_get
>> + *
>> + * If @list has a umem at @index, returns it like ib_umem_list_load() (and
>> + * marks the slot loaded). Otherwise calls ib_umem_get() with the given
>> + * @access flags and on success stores the result at @index when
>> + * @list is non-NULL.
>> + *
>> + * Return: valid umem pointer, or ERR_PTR.
>> + */
>> +struct ib_umem *ib_umem_list_load_or_get(struct ib_umem_list *list,
>> + unsigned int index,
>> + struct ib_device *device,
>> + unsigned long addr, size_t size,
>> + int access)
>> +{
>> + struct ib_umem *umem;
>> +
>> + umem = ib_umem_list_load(list, index, size);
>> + if (IS_ERR(umem) || umem)
>> + return umem;
>> + umem = ib_umem_get(device, addr, size, access);
>> + if (IS_ERR(umem))
>> + return umem;
>> + if (list && index < list->count)
>> + list->umems[index] = umem;
>> + return umem;
>> +}
>> +EXPORT_SYMBOL(ib_umem_list_load_or_get);
>> +
>> +/**
>> + * ib_umem_list_replace - Replace umem at index, releasing the previous one
>> + * @list: umem list (may be NULL)
>> + * @index: per-command buffer slot index
>> + * @umem: new umem pointer (may be NULL to clear the slot)
>> + *
>> + * Stores @umem at @index. If a different umem was already stored there, it is
>> + * released. Used for CQ resize and similar.
>> + */
>> +void ib_umem_list_replace(struct ib_umem_list *list, unsigned int index,
>> + struct ib_umem *umem)
>> +{
>> + struct ib_umem *old;
>> +
>> + if (!list || index >= list->count)
>> + return;
>> + old = list->umems[index];
>> + list->umems[index] = umem;
>> + if (old && old != umem)
>> + ib_umem_release(old);
>> +}
>> +EXPORT_SYMBOL(ib_umem_list_replace);
>> +
>> +/**
>> + * ib_umem_release_non_listed - Release a umem that is not stored in the list
>> + * @list: umem list
>> + * @index: per-command buffer slot index
>> + * @umem: umem pointer to release
>> + *
>> + * Releases @umem if it is not stored in @list.
>> + */
>> +void ib_umem_release_non_listed(struct ib_umem_list *list, unsigned int index,
>> + struct ib_umem *umem)
>> +{
>> + if (!list || index >= list->count || list->umems[index] != umem)
>> + ib_umem_release(umem);
>> +}
>> +EXPORT_SYMBOL(ib_umem_release_non_listed);
>> diff --git a/include/rdma/ib_umem.h b/include/rdma/ib_umem.h
>> index 2ad52cc1d52b..924acb8d08c3 100644
>> --- a/include/rdma/ib_umem.h
>> +++ b/include/rdma/ib_umem.h
>> @@ -11,6 +11,7 @@
>>
>> struct ib_device;
>> struct dma_buf_attach_ops;
>> +struct uverbs_attr_bundle;
>>
>> struct ib_umem {
>> struct ib_device *ibdev;
>> @@ -80,6 +81,36 @@ struct ib_umem *ib_umem_get(struct ib_device *device, unsigned long addr,
>> void ib_umem_release(struct ib_umem *umem);
>> int ib_umem_copy_from(void *dst, struct ib_umem *umem, size_t offset,
>> size_t length);
>> +
>> +/**
>> + * struct ib_umem_list - collection of pre-mapped umems
>> + *
>> + * Created from the UVERBS_ATTR_BUFFERS attribute. Each entry is indexed
>> + * by a per-command buffer slot enum (e.g., IB_UMEM_CQ_BUF for CQ CREATE).
>> + * Drivers use ib_umem_list_load() to retrieve a specific umem by index.
>> + */
>> +struct ib_umem_list;
>> +
>> +struct ib_umem_list *ib_umem_list_create(struct ib_device *device,
>> + const struct uverbs_attr_bundle *attrs,
>> + unsigned int slot_max);
>> +void ib_umem_list_release(struct ib_umem_list *list);
>> +int ib_umem_list_check_consumed(const struct ib_umem_list *list);
>> +void ib_umem_list_insert(struct ib_umem_list *list, unsigned int index,
>> + struct ib_umem *umem);
>> +
>> +struct ib_umem *ib_umem_list_load(struct ib_umem_list *list,
>> + unsigned int index, size_t size);
>> +struct ib_umem *ib_umem_list_load_or_get(struct ib_umem_list *list,
>> + unsigned int index,
>> + struct ib_device *device,
>> + unsigned long addr, size_t size,
>> + int access);
>> +void ib_umem_list_replace(struct ib_umem_list *list, unsigned int index,
>> + struct ib_umem *umem);
>> +void ib_umem_release_non_listed(struct ib_umem_list *list, unsigned int index,
>> + struct ib_umem *umem);
>> +
>> unsigned long ib_umem_find_best_pgsz(struct ib_umem *umem,
>> unsigned long pgsz_bitmap,
>> unsigned long virt);
>> @@ -230,5 +261,28 @@ static inline void ib_umem_dmabuf_revoke_lock(struct ib_umem_dmabuf *umem_dmabuf
>> static inline void ib_umem_dmabuf_revoke_unlock(struct ib_umem_dmabuf *umem_dmabuf) {}
>> static inline void ib_umem_dmabuf_revoke(struct ib_umem_dmabuf *umem_dmabuf) {}
>>
>> +struct ib_umem_list;
>> +
>> +static inline void ib_umem_list_release(struct ib_umem_list *list) { }
>> +static inline struct ib_umem *ib_umem_list_load(struct ib_umem_list *list,
>> + unsigned int index,
>> + size_t size)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +static inline struct ib_umem *
>> +ib_umem_list_load_or_get(struct ib_umem_list *list, unsigned int index,
>> + struct ib_device *device, unsigned long addr,
>> + size_t size, int access)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +static inline void ib_umem_list_replace(struct ib_umem_list *list,
>> + unsigned int index,
>> + struct ib_umem *umem) { }
>> +static inline void ib_umem_release_non_listed(struct ib_umem_list *list,
>> + unsigned int index,
>> + struct ib_umem *umem) { }
>> +
>> #endif /* CONFIG_INFINIBAND_USER_MEM */
>> #endif /* IB_UMEM_H */
>> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
>> index e2af17da3e32..05bcab27a87d 100644
>> --- a/include/rdma/uverbs_ioctl.h
>> +++ b/include/rdma/uverbs_ioctl.h
>> @@ -590,6 +590,20 @@ struct uapi_definition {
>> UA_OPTIONAL, \
>> .is_udata = 1)
>>
>> +/*
>> + * Optional array of struct ib_uverbs_buffer_desc describing memory regions
>> + * backed by dma-buf or user virtual address. Can be added to any method
>> + * that needs external buffer support.
>> + * Each entry carries an index field selecting the per-command buffer slot.
>> + * Use ib_umem_list_create() to map them and ib_umem_list_load() to access.
>> + */
>> +#define UVERBS_ATTR_BUFFERS() \
>> + UVERBS_ATTR_PTR_IN(UVERBS_ATTR_BUFFERS, \
>> + UVERBS_ATTR_MIN_SIZE( \
>> + sizeof(struct ib_uverbs_buffer_desc)), \
>> + UA_OPTIONAL, \
>> + UA_ALLOC_AND_COPY)
>> +
>> /* =================================================
>> * Parsing infrastructure
>> * =================================================
>> diff --git a/include/uapi/rdma/ib_user_ioctl_cmds.h b/include/uapi/rdma/ib_user_ioctl_cmds.h
>> index 72041c1b0ea5..10aa6568abf1 100644
>> --- a/include/uapi/rdma/ib_user_ioctl_cmds.h
>> +++ b/include/uapi/rdma/ib_user_ioctl_cmds.h
>> @@ -64,6 +64,7 @@ enum {
>> UVERBS_ATTR_UHW_IN = UVERBS_ID_DRIVER_NS,
>> UVERBS_ATTR_UHW_OUT,
>> UVERBS_ID_DRIVER_NS_WITH_UHW,
>> + UVERBS_ATTR_BUFFERS,
>> };
>>
>> enum uverbs_methods_device {
>> diff --git a/include/uapi/rdma/ib_user_ioctl_verbs.h b/include/uapi/rdma/ib_user_ioctl_verbs.h
>> index 90c5cd8e7753..41ed9f75b4de 100644
>> --- a/include/uapi/rdma/ib_user_ioctl_verbs.h
>> +++ b/include/uapi/rdma/ib_user_ioctl_verbs.h
>> @@ -273,4 +273,31 @@ struct ib_uverbs_gid_entry {
>> __u32 netdev_ifindex; /* It is 0 if there is no netdev associated with it */
>> };
>>
>> +enum ib_uverbs_buffer_type {
>> + IB_UVERBS_BUFFER_TYPE_DMABUF,
>> + IB_UVERBS_BUFFER_TYPE_VA,
>> +};
>> +
>> +/*
>> + * Describes a single buffer backed by dma-buf or user virtual address.
>> + * Passed as an array via UVERBS_ATTR_BUFFERS. Each uverb command that
>> + * accepts this attribute defines its own per-command buffer slot enum.
>> + * The index field selects the buffer slot this descriptor maps to.
>> + *
>> + * @fd: dma-buf file descriptor (valid for IB_UVERBS_BUFFER_TYPE_DMABUF)
>> + * @type: buffer type from enum ib_uverbs_buffer_type
>> + * @index: per-command buffer slot index
>> + * @reserved: must be zero
>> + * @addr: offset within dma-buf, or user virtual address for VA
>> + * @length: buffer length in bytes
>> + */
>> +struct ib_uverbs_buffer_desc {
>> + __s32 fd;
>> + __u32 type;
>> + __u32 index;
>> + __u32 reserved;
>> + __aligned_u64 addr;
>> + __aligned_u64 length;
>> +};
>> +
>> #endif
>> --
>> 2.53.0
>>