[PATCH for-next 0/2] RDMA: Support CQs with user memory

[PATCH for-next 0/2] RDMA: Support CQs with user memory

[Michael Margolin]

This series is adding a common way for creating CQs on top of
preallocated memory supplied by userspace. The memory buffer can be
provided as a virtual address or by dmabuf, in both cases a umem object
is created for driver's use.
EFA is the first to support this new interface, new drivers or existing
drivers that want/need to get the memory from userspace are expected to
use this flow.

It's follow-up of this discussion:
https://lore.kernel.org/all/c7d312ac-19a0-45e3-9f40-3e6f81500f83@amazon.com/

Thanks

Michael Margolin (2):
  RDMA/uverbs: Add a common way to create CQ with umem
  RDMA/efa: Add CQ with external memory support

 drivers/infiniband/core/device.c              |  3 +
 drivers/infiniband/core/uverbs_std_types_cq.c | 82 ++++++++++++++++++-
 drivers/infiniband/hw/efa/efa_main.c          |  1 +
 drivers/infiniband/hw/efa/efa_verbs.c         | 47 ++++++++---
 include/rdma/ib_verbs.h                       |  6 ++
 include/uapi/rdma/efa-abi.h                   |  3 +-
 include/uapi/rdma/ib_user_ioctl_cmds.h        |  4 +
 7 files changed, 129 insertions(+), 17 deletions(-)

-- 
2.47.1

[PATCH for-next 1/2] RDMA/uverbs: Add a common way to create CQ with umem

[Michael Margolin]

Add ioctl command attributes and a common handling for the option to
create CQs with memory buffers passed from userspace. When required
attributes are supplied, create umem for driver use and store it in CQ
context.
The extension enables creation of CQs on top of preallocated CPU
virtual or device memory buffers, by supplying VA or dmabuf fd, in a
common way. At first stage the command fails for any driver that didn't
explicitly state its support by setting a flag in the ops struct.

Signed-off-by: Michael Margolin <mrgolin@amazon.com>
---
 drivers/infiniband/core/device.c              |  3 +
 drivers/infiniband/core/uverbs_std_types_cq.c | 82 ++++++++++++++++++-
 include/rdma/ib_verbs.h                       |  6 ++
 include/uapi/rdma/ib_user_ioctl_cmds.h        |  4 +
 4 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 468ed6bd4722..8b0ce0ec15dd 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2667,6 +2667,9 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
 	dev_ops->uverbs_no_driver_id_binding |=
 		ops->uverbs_no_driver_id_binding;
 
+	dev_ops->uverbs_support_cq_with_umem |=
+		ops->uverbs_support_cq_with_umem;
+
 	SET_DEVICE_OP(dev_ops, add_gid);
 	SET_DEVICE_OP(dev_ops, add_sub_dev);
 	SET_DEVICE_OP(dev_ops, advise_mr);
diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c
index 432054f0a8a4..a8d567d2bdf9 100644
--- a/drivers/infiniband/core/uverbs_std_types_cq.c
+++ b/drivers/infiniband/core/uverbs_std_types_cq.c
@@ -64,13 +64,19 @@ static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(
 	struct ib_ucq_object *obj = container_of(
 		uverbs_attr_get_uobject(attrs, UVERBS_ATTR_CREATE_CQ_HANDLE),
 		typeof(*obj), uevent.uobject);
+	struct ib_uverbs_completion_event_file *ev_file = NULL;
 	struct ib_device *ib_dev = attrs->context->device;
-	int ret;
-	u64 user_handle;
+	struct ib_umem_dmabuf *umem_dmabuf;
 	struct ib_cq_init_attr attr = {};
-	struct ib_cq                   *cq;
-	struct ib_uverbs_completion_event_file    *ev_file = NULL;
 	struct ib_uobject *ev_file_uobj;
+	struct ib_umem *umem = NULL;
+	u64 buffer_length;
+	u64 buffer_offset;
+	struct ib_cq *cq;
+	u64 user_handle;
+	u64 buffer_va;
+	int buffer_fd;
+	int ret;
 
 	if (!ib_dev->ops.create_cq || !ib_dev->ops.destroy_cq)
 		return -EOPNOTSUPP;
@@ -112,9 +118,65 @@ static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(
 	INIT_LIST_HEAD(&obj->comp_list);
 	INIT_LIST_HEAD(&obj->uevent.event_list);
 
+	if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_VA)) {
+
+		ret = uverbs_copy_from(&buffer_va, attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_VA);
+		if (ret)
+			goto err_event_file;
+
+		ret = uverbs_copy_from(&buffer_length, attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_LENGTH);
+		if (ret)
+			goto err_event_file;
+
+		if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_FD) ||
+		    uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_OFFSET) ||
+		    !ib_dev->ops.uverbs_support_cq_with_umem) {
+			ret = -EINVAL;
+			goto err_event_file;
+		}
+
+		umem = ib_umem_get(ib_dev, buffer_va, buffer_length, IB_ACCESS_LOCAL_WRITE);
+		if (IS_ERR(umem)) {
+			ret = PTR_ERR(umem);
+			goto err_event_file;
+		}
+	} else if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_FD)) {
+
+		ret = uverbs_get_raw_fd(&buffer_fd, attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_FD);
+		if (ret)
+			goto err_event_file;
+
+		ret = uverbs_copy_from(&buffer_offset, attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_OFFSET);
+		if (ret)
+			goto err_event_file;
+
+		ret = uverbs_copy_from(&buffer_length, attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_LENGTH);
+		if (ret)
+			goto err_event_file;
+
+		if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_VA) ||
+		    !ib_dev->ops.uverbs_support_cq_with_umem) {
+			ret = -EINVAL;
+			goto err_event_file;
+		}
+
+		umem_dmabuf = ib_umem_dmabuf_get_pinned(ib_dev, buffer_offset, buffer_length,
+							buffer_fd, IB_ACCESS_LOCAL_WRITE);
+		if (IS_ERR(umem_dmabuf)) {
+			ret = PTR_ERR(umem_dmabuf);
+			goto err_event_file;
+		}
+		umem = &umem_dmabuf->umem;
+	} else if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_OFFSET) ||
+		   uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUFFER_LENGTH)) {
+		ret = -EINVAL;
+		goto err_event_file;
+	}
+
 	cq = rdma_zalloc_drv_obj(ib_dev, ib_cq);
 	if (!cq) {
 		ret = -ENOMEM;
+		ib_umem_release(umem);
 		goto err_event_file;
 	}
 
@@ -123,6 +185,7 @@ static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(
 	cq->comp_handler  = ib_uverbs_comp_handler;
 	cq->event_handler = ib_uverbs_cq_event_handler;
 	cq->cq_context    = ev_file ? &ev_file->ev_queue : NULL;
+	cq->umem          = umem;
 	atomic_set(&cq->usecnt, 0);
 
 	rdma_restrack_new(&cq->res, RDMA_RESTRACK_CQ);
@@ -180,6 +243,17 @@ DECLARE_UVERBS_NAMED_METHOD(
 		       UVERBS_OBJECT_ASYNC_EVENT,
 		       UVERBS_ACCESS_READ,
 		       UA_OPTIONAL),
+	UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_BUFFER_VA,
+			   UVERBS_ATTR_TYPE(u64),
+			   UA_OPTIONAL),
+	UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_BUFFER_LENGTH,
+			   UVERBS_ATTR_TYPE(u64),
+			   UA_OPTIONAL),
+	UVERBS_ATTR_RAW_FD(UVERBS_ATTR_CREATE_CQ_BUFFER_FD,
+			   UA_OPTIONAL),
+	UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_BUFFER_OFFSET,
+			   UVERBS_ATTR_TYPE(u64),
+			   UA_OPTIONAL),
 	UVERBS_ATTR_UHW());
 
 static int UVERBS_HANDLER(UVERBS_METHOD_CQ_DESTROY)(
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 38f68d245fa6..c7b082a78381 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1625,6 +1625,7 @@ struct ib_cq {
 	};
 	struct workqueue_struct *comp_wq;
 	struct dim *dim;
+	struct ib_umem *umem;
 
 	/* updated only by trace points */
 	ktime_t timestamp;
@@ -2343,6 +2344,11 @@ struct ib_device_ops {
 	enum rdma_driver_id driver_id;
 	u32 uverbs_abi_ver;
 	unsigned int uverbs_no_driver_id_binding:1;
+	/*
+	 * Driver gets ownership over the umem and is responsible for releasing
+	 * it on CQ destroy or when it's no longer needed.
+	 */
+	unsigned int uverbs_support_cq_with_umem:1;
 
 	/*
 	 * NOTE: New drivers should not make use of device_group; instead new
diff --git a/include/uapi/rdma/ib_user_ioctl_cmds.h b/include/uapi/rdma/ib_user_ioctl_cmds.h
index ac7b162611ed..5f3e5bee51b2 100644
--- a/include/uapi/rdma/ib_user_ioctl_cmds.h
+++ b/include/uapi/rdma/ib_user_ioctl_cmds.h
@@ -105,6 +105,10 @@ enum uverbs_attrs_create_cq_cmd_attr_ids {
 	UVERBS_ATTR_CREATE_CQ_FLAGS,
 	UVERBS_ATTR_CREATE_CQ_RESP_CQE,
 	UVERBS_ATTR_CREATE_CQ_EVENT_FD,
+	UVERBS_ATTR_CREATE_CQ_BUFFER_VA,
+	UVERBS_ATTR_CREATE_CQ_BUFFER_LENGTH,
+	UVERBS_ATTR_CREATE_CQ_BUFFER_FD,
+	UVERBS_ATTR_CREATE_CQ_BUFFER_OFFSET,
 };
 
 enum uverbs_attrs_destroy_cq_cmd_attr_ids {
-- 
2.47.1

[PATCH for-next 2/2] RDMA/efa: Add CQ with external memory support

[Michael Margolin]

Add an option to create CQ using external memory instead of allocating
in the driver. The memory can be passed from userspace by dmabuf fd and
an offset or a VA. One of the possible usages is creating CQs that
reside in accelerator memory, allowing low latency asynchronous direct
polling from the accelerator device. Add a capability bit to reflect on
the feature support.

Reviewed-by: Daniel Kranzdorf <dkkranzd@amazon.com>
Reviewed-by: Yonatan Nachum <ynachum@amazon.com>
Signed-off-by: Michael Margolin <mrgolin@amazon.com>
---
 drivers/infiniband/hw/efa/efa_main.c  |  1 +
 drivers/infiniband/hw/efa/efa_verbs.c | 47 ++++++++++++++++++++-------
 include/uapi/rdma/efa-abi.h           |  3 +-
 3 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 4f03c0ec819f..b066e8387df0 100644
--- a/drivers/infiniband/hw/efa/efa_main.c
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -366,6 +366,7 @@ static const struct ib_device_ops efa_dev_ops = {
 	.owner = THIS_MODULE,
 	.driver_id = RDMA_DRIVER_EFA,
 	.uverbs_abi_ver = EFA_UVERBS_ABI_VERSION,
+	.uverbs_support_cq_with_umem = 1,
 
 	.alloc_hw_port_stats = efa_alloc_hw_port_stats,
 	.alloc_hw_device_stats = efa_alloc_hw_device_stats,
diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
index a8645a40730f..5988f89837bd 100644
--- a/drivers/infiniband/hw/efa/efa_verbs.c
+++ b/drivers/infiniband/hw/efa/efa_verbs.c
@@ -249,6 +249,7 @@ int efa_query_device(struct ib_device *ibdev,
 		resp.max_rdma_size = dev_attr->max_rdma_size;
 
 		resp.device_caps |= EFA_QUERY_DEVICE_CAPS_CQ_WITH_SGID;
+		resp.device_caps |= EFA_QUERY_DEVICE_CAPS_CQ_WITH_EXT_MEM;
 		if (EFA_DEV_CAP(dev, RDMA_READ))
 			resp.device_caps |= EFA_QUERY_DEVICE_CAPS_RDMA_READ;
 
@@ -1082,8 +1083,11 @@ int efa_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
 		xa_erase(&dev->cqs_xa, cq->cq_idx);
 		synchronize_irq(cq->eq->irq.irqn);
 	}
-	efa_free_mapped(dev, cq->cpu_addr, cq->dma_addr, cq->size,
-			DMA_FROM_DEVICE);
+
+	if (ibcq->umem)
+		ib_umem_release(ibcq->umem);
+	else
+		efa_free_mapped(dev, cq->cpu_addr, cq->dma_addr, cq->size, DMA_FROM_DEVICE);
 	return 0;
 }
 
@@ -1133,8 +1137,10 @@ int efa_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 	struct efa_com_create_cq_result result;
 	struct ib_device *ibdev = ibcq->device;
 	struct efa_dev *dev = to_edev(ibdev);
+	struct ib_umem *umem = ibcq->umem;
 	struct efa_ibv_create_cq cmd = {};
 	struct efa_cq *cq = to_ecq(ibcq);
+	struct scatterlist *umem_sgl;
 	int entries = attr->cqe;
 	bool set_src_addr;
 	int err;
@@ -1202,11 +1208,24 @@ int efa_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 
 	cq->ucontext = ucontext;
 	cq->size = PAGE_ALIGN(cmd.cq_entry_size * entries * cmd.num_sub_cqs);
-	cq->cpu_addr = efa_zalloc_mapped(dev, &cq->dma_addr, cq->size,
-					 DMA_FROM_DEVICE);
-	if (!cq->cpu_addr) {
-		err = -ENOMEM;
-		goto err_out;
+
+	if (umem) {
+		umem_sgl = ibcq->umem->sgt_append.sgt.sgl;
+		if (sg_dma_len(umem_sgl) < ib_umem_offset(umem) + cq->size) {
+			ibdev_dbg(&dev->ibdev, "Non contiguous CQ unsupported\n");
+			err = -EINVAL;
+			goto err_free_mem;
+		}
+
+		cq->cpu_addr = NULL;
+		cq->dma_addr = sg_dma_address(umem_sgl) + ib_umem_offset(umem);
+	} else {
+		cq->cpu_addr = efa_zalloc_mapped(dev, &cq->dma_addr, cq->size,
+						 DMA_FROM_DEVICE);
+		if (!cq->cpu_addr) {
+			err = -ENOMEM;
+			goto err_out;
+		}
 	}
 
 	params.uarn = cq->ucontext->uarn;
@@ -1223,7 +1242,7 @@ int efa_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 
 	err = efa_com_create_cq(&dev->edev, &params, &result);
 	if (err)
-		goto err_free_mapped;
+		goto err_free_mem;
 
 	resp.db_off = result.db_off;
 	resp.cq_idx = result.cq_idx;
@@ -1231,7 +1250,9 @@ int efa_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 	cq->ibcq.cqe = result.actual_depth;
 	WARN_ON_ONCE(entries != result.actual_depth);
 
-	err = cq_mmap_entries_setup(dev, cq, &resp, result.db_valid);
+	if (!umem)
+		err = cq_mmap_entries_setup(dev, cq, &resp, result.db_valid);
+
 	if (err) {
 		ibdev_dbg(ibdev, "Could not setup cq[%u] mmap entries\n",
 			  cq->cq_idx);
@@ -1269,9 +1290,11 @@ int efa_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 	efa_cq_user_mmap_entries_remove(cq);
 err_destroy_cq:
 	efa_destroy_cq_idx(dev, cq->cq_idx);
-err_free_mapped:
-	efa_free_mapped(dev, cq->cpu_addr, cq->dma_addr, cq->size,
-			DMA_FROM_DEVICE);
+err_free_mem:
+	if (umem)
+		ib_umem_release(umem);
+	else
+		efa_free_mapped(dev, cq->cpu_addr, cq->dma_addr, cq->size, DMA_FROM_DEVICE);
 
 err_out:
 	atomic64_inc(&dev->stats.create_cq_err);
diff --git a/include/uapi/rdma/efa-abi.h b/include/uapi/rdma/efa-abi.h
index 11b94b0b035b..98b71b9979f8 100644
--- a/include/uapi/rdma/efa-abi.h
+++ b/include/uapi/rdma/efa-abi.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
 /*
- * Copyright 2018-2024 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2025 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #ifndef EFA_ABI_USER_H
@@ -131,6 +131,7 @@ enum {
 	EFA_QUERY_DEVICE_CAPS_DATA_POLLING_128 = 1 << 4,
 	EFA_QUERY_DEVICE_CAPS_RDMA_WRITE = 1 << 5,
 	EFA_QUERY_DEVICE_CAPS_UNSOLICITED_WRITE_RECV = 1 << 6,
+	EFA_QUERY_DEVICE_CAPS_CQ_WITH_EXT_MEM = 1 << 7,
 };
 
 struct efa_ibv_ex_query_device_resp {
-- 
2.47.1

Re: [PATCH for-next 1/2] RDMA/uverbs: Add a common way to create CQ with umem

[Jason Gunthorpe]

On Tue, Jul 01, 2025 at 11:15:44PM +0000, Michael Margolin wrote:
> Add ioctl command attributes and a common handling for the option to
> create CQs with memory buffers passed from userspace. When required
> attributes are supplied, create umem for driver use and store it in CQ
> context.
> The extension enables creation of CQs on top of preallocated CPU
> virtual or device memory buffers, by supplying VA or dmabuf fd, in a
> common way. At first stage the command fails for any driver that didn't
> explicitly state its support by setting a flag in the ops struct.
> 
> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
> ---
>  drivers/infiniband/core/device.c              |  3 +
>  drivers/infiniband/core/uverbs_std_types_cq.c | 82 ++++++++++++++++++-
>  include/rdma/ib_verbs.h                       |  6 ++
>  include/uapi/rdma/ib_user_ioctl_cmds.h        |  4 +
>  4 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 468ed6bd4722..8b0ce0ec15dd 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2667,6 +2667,9 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
>  	dev_ops->uverbs_no_driver_id_binding |=
>  		ops->uverbs_no_driver_id_binding;
>  
> +	dev_ops->uverbs_support_cq_with_umem |=
> +		ops->uverbs_support_cq_with_umem;

This seems to have turned out quite nice!

I might just suggest a tweak to streamline this flow

Add:

+       int (*create_cq_umem)(struct ib_cq *cq,
+                             const struct ib_cq_init_attr *attr,
+                             struct ib_umem *umem,
+                             struct uverbs_attr_bundle *attrs);


Instead of the uverbs_support_cq_with_umem

Then the core code would check the two ops and if only umem is present
it will insist on the new attributes, if it is not present it will
refuse them otherwise it will call the correct op.

In the driver the create_cq would obtain the umem from the private
attrs and then call create_cq_umem() in the same way

So it becomes quite easy to just reorganize the drivers to support
this.

Jason

Re: [PATCH for-next 2/2] RDMA/efa: Add CQ with external memory support

[Jason Gunthorpe]

On Tue, Jul 01, 2025 at 11:15:45PM +0000, Michael Margolin wrote:
> +	if (umem) {
> +		umem_sgl = ibcq->umem->sgt_append.sgt.sgl;
> +		if (sg_dma_len(umem_sgl) < ib_umem_offset(umem) + cq->size) {
> +			ibdev_dbg(&dev->ibdev, "Non contiguous CQ unsupported\n");
> +			err = -EINVAL;
> +			goto err_free_mem;
> +		}

I'd rather see this call ib_umem_find_best_pgsz()

Maybe like:
  len = ib_umem_offset(umem) + cq->size
  pgsz = roundup_pow_of_two(len);
  ib_umem_find_best_pgoff(umem, pgsz, U64_MAX);
  rdma_umem_for_each_dma_block(umem, biter, pgsz) {
       dma = rdma_block_iter_dma_address(&biter) +  ib_umem_dma_offset(umem, pgsz);
       break;
  }

It turns out the scatterlists can be irregular in some cases so this
will handle that properly while the little test above cannot.

And maybe the above thing could be a little helper:

bool ib_umem_get_contiguous_dma(umem, &dma_addr)

Also I am trying hard to keep scatterlist APIS out of the drivers.

Jason

Re: [PATCH for-next 2/2] RDMA/efa: Add CQ with external memory support

[Margolin, Michael]

On 7/4/2025 9:28 PM, Jason Gunthorpe wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue, Jul 01, 2025 at 11:15:45PM +0000, Michael Margolin wrote:
>> +     if (umem) {
>> +             umem_sgl = ibcq->umem->sgt_append.sgt.sgl;
>> +             if (sg_dma_len(umem_sgl) < ib_umem_offset(umem) + cq->size) {
>> +                     ibdev_dbg(&dev->ibdev, "Non contiguous CQ unsupported\n");
>> +                     err = -EINVAL;
>> +                     goto err_free_mem;
>> +             }
> I'd rather see this call ib_umem_find_best_pgsz()
>
> Maybe like:
>    len = ib_umem_offset(umem) + cq->size
>    pgsz = roundup_pow_of_two(len);
>    ib_umem_find_best_pgoff(umem, pgsz, U64_MAX);
>    rdma_umem_for_each_dma_block(umem, biter, pgsz) {
>         dma = rdma_block_iter_dma_address(&biter) +  ib_umem_dma_offset(umem, pgsz);
>         break;
>    }
>
> It turns out the scatterlists can be irregular in some cases so this
> will handle that properly while the little test above cannot.
>
> And maybe the above thing could be a little helper:
>
> bool ib_umem_get_contiguous_dma(umem, &dma_addr)
>
> Also I am trying hard to keep scatterlist APIS out of the drivers.
>
> Jason

Some disadvantage of this approach is that if the provided memory 
is contiguous in its first "roundup_pow_of_two(len)" bytes but consists 
of additional smaller or unaligned sgl entries, we will refuse to use it.

I will try to compile some combination of the two approaches into a 
helper function.

Michael

Re: [PATCH for-next 2/2] RDMA/efa: Add CQ with external memory support

[Jason Gunthorpe]

On Mon, Jul 07, 2025 at 08:07:43PM +0300, Margolin, Michael wrote:
> 
> On 7/4/2025 9:28 PM, Jason Gunthorpe wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Tue, Jul 01, 2025 at 11:15:45PM +0000, Michael Margolin wrote:
> > > +     if (umem) {
> > > +             umem_sgl = ibcq->umem->sgt_append.sgt.sgl;
> > > +             if (sg_dma_len(umem_sgl) < ib_umem_offset(umem) + cq->size) {
> > > +                     ibdev_dbg(&dev->ibdev, "Non contiguous CQ unsupported\n");
> > > +                     err = -EINVAL;
> > > +                     goto err_free_mem;
> > > +             }
> > I'd rather see this call ib_umem_find_best_pgsz()
> > 
> > Maybe like:
> >    len = ib_umem_offset(umem) + cq->size
> >    pgsz = roundup_pow_of_two(len);
> >    ib_umem_find_best_pgoff(umem, pgsz, U64_MAX);
> >    rdma_umem_for_each_dma_block(umem, biter, pgsz) {
> >         dma = rdma_block_iter_dma_address(&biter) +  ib_umem_dma_offset(umem, pgsz);
> >         break;
> >    }
> > 
> > It turns out the scatterlists can be irregular in some cases so this
> > will handle that properly while the little test above cannot.
> > 
> > And maybe the above thing could be a little helper:
> > 
> > bool ib_umem_get_contiguous_dma(umem, &dma_addr)
> > 
> > Also I am trying hard to keep scatterlist APIS out of the drivers.
> > 
> > Jason
> 
> Some disadvantage of this approach is that if the provided memory
> is contiguous in its first "roundup_pow_of_two(len)" bytes but consists of
> additional smaller or unaligned sgl entries, we will refuse to use
> it.

I intended "len" to be large enough that there was no additional
portion that is not covered.

Jason

Re: [PATCH for-next 1/2] RDMA/uverbs: Add a common way to create CQ with umem

[Margolin, Michael]

On 7/4/2025 9:14 PM, Jason Gunthorpe wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue, Jul 01, 2025 at 11:15:44PM +0000, Michael Margolin wrote:
>> Add ioctl command attributes and a common handling for the option to
>> create CQs with memory buffers passed from userspace. When required
>> attributes are supplied, create umem for driver use and store it in CQ
>> context.
>> The extension enables creation of CQs on top of preallocated CPU
>> virtual or device memory buffers, by supplying VA or dmabuf fd, in a
>> common way. At first stage the command fails for any driver that didn't
>> explicitly state its support by setting a flag in the ops struct.
>>
>> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
>> ---
>>   drivers/infiniband/core/device.c              |  3 +
>>   drivers/infiniband/core/uverbs_std_types_cq.c | 82 ++++++++++++++++++-
>>   include/rdma/ib_verbs.h                       |  6 ++
>>   include/uapi/rdma/ib_user_ioctl_cmds.h        |  4 +
>>   4 files changed, 91 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
>> index 468ed6bd4722..8b0ce0ec15dd 100644
>> --- a/drivers/infiniband/core/device.c
>> +++ b/drivers/infiniband/core/device.c
>> @@ -2667,6 +2667,9 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
>>        dev_ops->uverbs_no_driver_id_binding |=
>>                ops->uverbs_no_driver_id_binding;
>>
>> +     dev_ops->uverbs_support_cq_with_umem |=
>> +             ops->uverbs_support_cq_with_umem;
> This seems to have turned out quite nice!
>
> I might just suggest a tweak to streamline this flow
>
> Add:
>
> +       int (*create_cq_umem)(struct ib_cq *cq,
> +                             const struct ib_cq_init_attr *attr,
> +                             struct ib_umem *umem,
> +                             struct uverbs_attr_bundle *attrs);
>
>
> Instead of the uverbs_support_cq_with_umem
>
> Then the core code would check the two ops and if only umem is present
> it will insist on the new attributes, if it is not present it will
> refuse them otherwise it will call the correct op.
>
> In the driver the create_cq would obtain the umem from the private
> attrs and then call create_cq_umem() in the same way
>
> So it becomes quite easy to just reorganize the drivers to support
> this.
>
> Jason

Generally I would say that adding a new op for an additional optional 
param isn't scalable but I implemented your suggestion and I do think it 
makes things more structured for drivers as well as clarifying umem 
ownership.

Sending v2 series.

Michael