[PATCH rdma-next 0/6] RDMA/mana_ib: Implement RNIC CQs

[PATCH rdma-next 0/6] RDMA/mana_ib: Implement RNIC CQs

[Konstantin Taranov]

From: Konstantin Taranov <kotaranov@microsoft.com>

This patch series implements creation and destruction of CQs
which can be used with RC QPs.

Patches with RC QPs will be sent in the next patch series.

To create a CQ for RNIC, mana_ib requires creation of EQs
within mana_ib device. An EQ of mana ethernet cannot be used.

To make the implementation of create_cq cleaner, this series
also introduces minor changes to mana_cq structure (cqe->buf_size)
and adds a helper to remove CQ callbacks.

Mana ethernet and mana_ib CQs are different entities which are
created in different isolation zones (ethernet vs rnic).
As a result, RNIC cannot use ethenet CQs and ethernet cannot
use RNIC CQs.
That is why, we use existing udata request for creation of
ethernet CQs. If the request has extra fields, then we create
an RNIC CQ. The kernel-level CQs will be RNIC CQs (in future
patches).

To preserve backward and forward compatibility with RDMA-CORE,
we will make the following changes to mana provider in RDMA-CORE:

The rdma-core will request RNIC CQs by default, with the proposed
request format.
If the mana has installed an allocator with manadv_set_context_attr,
then the rdma-core undestands that this is a DPDK use-case and
requests an ethernet CQ, using old short request format.

Konstantin Taranov (6):
  RDMA/mana_ib: create EQs for RNIC CQs
  RDMA/mana_ib: create and destroy RNIC cqs
  RDMA/mana_ib: replace duplicate cqe with buf_size
  RDMA/mana_ib: introduce a helper to remove cq callbacks
  RDMA/mana_ib: boundary check before installing cq callbacks
  RDMA/mana_ib: implement uapi for creation of rnic cq

 drivers/infiniband/hw/mana/cq.c      | 77 ++++++++++++++++++++----
 drivers/infiniband/hw/mana/main.c    | 88 +++++++++++++++++++++++++++-
 drivers/infiniband/hw/mana/mana_ib.h | 36 +++++++++++-
 drivers/infiniband/hw/mana/qp.c      | 30 ++--------
 include/uapi/rdma/mana-abi.h         |  7 +++
 5 files changed, 200 insertions(+), 38 deletions(-)

-- 
2.43.0

[PATCH rdma-next 1/6] RDMA/mana_ib: create EQs for RNIC CQs

[Konstantin Taranov]

From: Konstantin Taranov <kotaranov@microsoft.com>

Create EQs within mana_ib device. Such EQs are required
for creation of RNIC CQs.

Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
---
 drivers/infiniband/hw/mana/main.c    | 34 ++++++++++++++++++++++++++--
 drivers/infiniband/hw/mana/mana_ib.h |  1 +
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/mana/main.c b/drivers/infiniband/hw/mana/main.c
index f540147..546d059 100644
--- a/drivers/infiniband/hw/mana/main.c
+++ b/drivers/infiniband/hw/mana/main.c
@@ -658,7 +658,7 @@ int mana_ib_create_eqs(struct mana_ib_dev *mdev)
 {
 	struct gdma_context *gc = mdev_to_gc(mdev);
 	struct gdma_queue_spec spec = {};
-	int err;
+	int err, i;
 
 	spec.type = GDMA_EQ;
 	spec.monitor_avl_buf = false;
@@ -672,12 +672,42 @@ int mana_ib_create_eqs(struct mana_ib_dev *mdev)
 	if (err)
 		return err;
 
+	mdev->eqs = kcalloc(mdev->ib_dev.num_comp_vectors, sizeof(struct gdma_queue *),
+			    GFP_KERNEL);
+	if (!mdev->eqs) {
+		err = -ENOMEM;
+		goto destroy_fatal_eq;
+	}
+
+	for (i = 0; i < mdev->ib_dev.num_comp_vectors; i++) {
+		spec.eq.msix_index = (i + 1) % gc->num_msix_usable;
+		err = mana_gd_create_mana_eq(mdev->gdma_dev, &spec, &mdev->eqs[i]);
+		if (err)
+			goto destroy_eqs;
+	}
+
 	return 0;
+
+destroy_eqs:
+	while (i-- > 0)
+		mana_gd_destroy_queue(gc, mdev->eqs[i]);
+	kfree(mdev->eqs);
+destroy_fatal_eq:
+	mana_gd_destroy_queue(gc, mdev->fatal_err_eq);
+	return err;
 }
 
 void mana_ib_destroy_eqs(struct mana_ib_dev *mdev)
 {
-	mana_gd_destroy_queue(mdev_to_gc(mdev), mdev->fatal_err_eq);
+	struct gdma_context *gc = mdev_to_gc(mdev);
+	int i;
+
+	mana_gd_destroy_queue(gc, mdev->fatal_err_eq);
+
+	for (i = 0; i < mdev->ib_dev.num_comp_vectors; i++)
+		mana_gd_destroy_queue(gc, mdev->eqs[i]);
+
+	kfree(mdev->eqs);
 }
 
 int mana_ib_gd_create_rnic_adapter(struct mana_ib_dev *mdev)
diff --git a/drivers/infiniband/hw/mana/mana_ib.h b/drivers/infiniband/hw/mana/mana_ib.h
index 4c1240d..bfcf6df 100644
--- a/drivers/infiniband/hw/mana/mana_ib.h
+++ b/drivers/infiniband/hw/mana/mana_ib.h
@@ -56,6 +56,7 @@ struct mana_ib_dev {
 	struct gdma_dev *gdma_dev;
 	mana_handle_t adapter_handle;
 	struct gdma_queue *fatal_err_eq;
+	struct gdma_queue **eqs;
 	struct mana_ib_adapter_caps adapter_caps;
 };
 
-- 
2.43.0

RE: [PATCH rdma-next 1/6] RDMA/mana_ib: create EQs for RNIC CQs

[Long Li]

> Subject: [PATCH rdma-next 1/6] RDMA/mana_ib: create EQs for RNIC CQs
> 
> From: Konstantin Taranov <kotaranov@microsoft.com>
> 
> Create EQs within mana_ib device. Such EQs are required for creation of RNIC
> CQs.
> 
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>

Reviewed-by: Long Li <longli@microsoft.com>

[PATCH rdma-next 2/6] RDMA/mana_ib: create and destroy RNIC cqs

[Konstantin Taranov]

From: Konstantin Taranov <kotaranov@microsoft.com>

Implement RNIC requests for creation and destruction of RNIC CQs.

Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
---
 drivers/infiniband/hw/mana/main.c    | 54 ++++++++++++++++++++++++++++
 drivers/infiniband/hw/mana/mana_ib.h | 32 +++++++++++++++++
 2 files changed, 86 insertions(+)

diff --git a/drivers/infiniband/hw/mana/main.c b/drivers/infiniband/hw/mana/main.c
index 546d059..2a41135 100644
--- a/drivers/infiniband/hw/mana/main.c
+++ b/drivers/infiniband/hw/mana/main.c
@@ -834,3 +834,57 @@ int mana_ib_gd_config_mac(struct mana_ib_dev *mdev, enum mana_ib_addr_op op, u8
 
 	return 0;
 }
+
+int mana_ib_gd_create_cq(struct mana_ib_dev *mdev, struct mana_ib_cq *cq, u32 doorbell)
+{
+	struct gdma_context *gc = mdev_to_gc(mdev);
+	struct mana_rnic_create_cq_resp resp = {};
+	struct mana_rnic_create_cq_req req = {};
+	int err;
+
+	mana_gd_init_req_hdr(&req.hdr, MANA_IB_CREATE_CQ, sizeof(req), sizeof(resp));
+	req.hdr.dev_id = gc->mana_ib.dev_id;
+	req.adapter = mdev->adapter_handle;
+	req.gdma_region = cq->queue.gdma_region;
+	req.eq_id = mdev->eqs[cq->comp_vector]->id;
+	req.doorbell_page = doorbell;
+
+	err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
+
+	if (err) {
+		ibdev_err(&mdev->ib_dev, "Failed to create cq err %d", err);
+		return err;
+	}
+
+	cq->queue.id  = resp.cq_id;
+	cq->cq_handle = resp.cq_handle;
+	/* The GDMA region is now owned by the CQ handle */
+	cq->queue.gdma_region = GDMA_INVALID_DMA_REGION;
+
+	return 0;
+}
+
+int mana_ib_gd_destroy_cq(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
+{
+	struct gdma_context *gc = mdev_to_gc(mdev);
+	struct mana_rnic_destroy_cq_resp resp = {};
+	struct mana_rnic_destroy_cq_req req = {};
+	int err;
+
+	if (cq->cq_handle == INVALID_MANA_HANDLE)
+		return 0;
+
+	mana_gd_init_req_hdr(&req.hdr, MANA_IB_DESTROY_CQ, sizeof(req), sizeof(resp));
+	req.hdr.dev_id = gc->mana_ib.dev_id;
+	req.adapter = mdev->adapter_handle;
+	req.cq_handle = cq->cq_handle;
+
+	err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
+
+	if (err) {
+		ibdev_err(&mdev->ib_dev, "Failed to destroy cq err %d", err);
+		return err;
+	}
+
+	return 0;
+}
diff --git a/drivers/infiniband/hw/mana/mana_ib.h b/drivers/infiniband/hw/mana/mana_ib.h
index bfcf6df..9162f29 100644
--- a/drivers/infiniband/hw/mana/mana_ib.h
+++ b/drivers/infiniband/hw/mana/mana_ib.h
@@ -92,6 +92,7 @@ struct mana_ib_cq {
 	struct mana_ib_queue queue;
 	int cqe;
 	u32 comp_vector;
+	mana_handle_t  cq_handle;
 };
 
 struct mana_ib_qp {
@@ -119,6 +120,8 @@ enum mana_ib_command_code {
 	MANA_IB_DESTROY_ADAPTER = 0x30003,
 	MANA_IB_CONFIG_IP_ADDR	= 0x30004,
 	MANA_IB_CONFIG_MAC_ADDR	= 0x30005,
+	MANA_IB_CREATE_CQ       = 0x30008,
+	MANA_IB_DESTROY_CQ      = 0x30009,
 };
 
 struct mana_ib_query_adapter_caps_req {
@@ -202,6 +205,31 @@ struct mana_rnic_config_mac_addr_resp {
 	struct gdma_resp_hdr hdr;
 }; /* HW Data */
 
+struct mana_rnic_create_cq_req {
+	struct gdma_req_hdr hdr;
+	mana_handle_t adapter;
+	u64 gdma_region;
+	u32 eq_id;
+	u32 doorbell_page;
+}; /* HW Data */
+
+struct mana_rnic_create_cq_resp {
+	struct gdma_resp_hdr hdr;
+	mana_handle_t cq_handle;
+	u32 cq_id;
+	u32 reserved;
+}; /* HW Data */
+
+struct mana_rnic_destroy_cq_req {
+	struct gdma_req_hdr hdr;
+	mana_handle_t adapter;
+	mana_handle_t cq_handle;
+}; /* HW Data */
+
+struct mana_rnic_destroy_cq_resp {
+	struct gdma_resp_hdr hdr;
+}; /* HW Data */
+
 static inline struct gdma_context *mdev_to_gc(struct mana_ib_dev *mdev)
 {
 	return mdev->gdma_dev->gdma_context;
@@ -321,4 +349,8 @@ int mana_ib_gd_add_gid(const struct ib_gid_attr *attr, void **context);
 int mana_ib_gd_del_gid(const struct ib_gid_attr *attr, void **context);
 
 int mana_ib_gd_config_mac(struct mana_ib_dev *mdev, enum mana_ib_addr_op op, u8 *mac);
+
+int mana_ib_gd_create_cq(struct mana_ib_dev *mdev, struct mana_ib_cq *cq, u32 doorbell);
+
+int mana_ib_gd_destroy_cq(struct mana_ib_dev *mdev, struct mana_ib_cq *cq);
 #endif
-- 
2.43.0

RE: [PATCH rdma-next 2/6] RDMA/mana_ib: create and destroy RNIC cqs

[Long Li]

> Subject: [PATCH rdma-next 2/6] RDMA/mana_ib: create and destroy RNIC cqs
> 
> From: Konstantin Taranov <kotaranov@microsoft.com>
> 
> Implement RNIC requests for creation and destruction of RNIC CQs.
> 
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>

Reviewed-by: Long Li <longli@microsoft.com>

[PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe with buf_size

[Konstantin Taranov]

From: Konstantin Taranov <kotaranov@microsoft.com>

Replace cqe with buf_size in struct mana_ib_cq.
The cqe field is already present in struct ib_cq and can be accessed there.
The buf_size field is required for mana RNIC CQs.

Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
---
 drivers/infiniband/hw/mana/cq.c      | 4 ++--
 drivers/infiniband/hw/mana/mana_ib.h | 2 +-
 drivers/infiniband/hw/mana/qp.c      | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index dc931b9..0467ee8 100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -33,8 +33,8 @@ int mana_ib_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 		return -EINVAL;
 	}
 
-	cq->cqe = attr->cqe;
-	err = mana_ib_create_queue(mdev, ucmd.buf_addr, cq->cqe * COMP_ENTRY_SIZE, &cq->queue);
+	cq->buf_size = attr->cqe * COMP_ENTRY_SIZE;
+	err = mana_ib_create_queue(mdev, ucmd.buf_addr, cq->buf_size, &cq->queue);
 	if (err) {
 		ibdev_dbg(ibdev, "Failed to create queue for create cq, %d\n", err);
 		return err;
diff --git a/drivers/infiniband/hw/mana/mana_ib.h b/drivers/infiniband/hw/mana/mana_ib.h
index 9162f29..9c07021 100644
--- a/drivers/infiniband/hw/mana/mana_ib.h
+++ b/drivers/infiniband/hw/mana/mana_ib.h
@@ -90,7 +90,7 @@ struct mana_ib_mr {
 struct mana_ib_cq {
 	struct ib_cq ibcq;
 	struct mana_ib_queue queue;
-	int cqe;
+	u32 buf_size;
 	u32 comp_vector;
 	mana_handle_t  cq_handle;
 };
diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
index 280e85a..c4fb8b4 100644
--- a/drivers/infiniband/hw/mana/qp.c
+++ b/drivers/infiniband/hw/mana/qp.c
@@ -196,7 +196,7 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
 		wq_spec.queue_size = wq->wq_buf_size;
 
 		cq_spec.gdma_region = cq->queue.gdma_region;
-		cq_spec.queue_size = cq->cqe * COMP_ENTRY_SIZE;
+		cq_spec.queue_size = cq->buf_size;
 		cq_spec.modr_ctx_id = 0;
 		eq = &mpc->ac->eqs[cq->comp_vector];
 		cq_spec.attached_eq = eq->eq->id;
@@ -355,7 +355,7 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd *ibpd,
 	wq_spec.queue_size = ucmd.sq_buf_size;
 
 	cq_spec.gdma_region = send_cq->queue.gdma_region;
-	cq_spec.queue_size = send_cq->cqe * COMP_ENTRY_SIZE;
+	cq_spec.queue_size = send_cq->buf_size;
 	cq_spec.modr_ctx_id = 0;
 	eq_vec = send_cq->comp_vector;
 	eq = &mpc->ac->eqs[eq_vec];
-- 
2.43.0

RE: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe with buf_size

[Long Li]

> Subject: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe with
> buf_size

I don't understand this commit message on "duplicate" cqe. I couldn't find a duplicate of it in the existing code.

RE: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe with buf_size

[Konstantin Taranov]

> From: Long Li <longli@microsoft.com>
> Sent: Wednesday, 24 April 2024 01:35
> To: Konstantin Taranov <kotaranov@linux.microsoft.com>; Konstantin
> Taranov <kotaranov@microsoft.com>; sharmaajay@microsoft.com;
> jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe
> with buf_size
> 
> > Subject: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe
> > with buf_size
> 
> I don't understand this commit message on "duplicate" cqe. I couldn't find a
> duplicate of it in the existing code.

If we need cqe, we could use it at cq->ibcq.cqe. The patch does not assign it as
it is not used, but if you want I can add "cq->ibcq.cqe = attr->cqe;" in v2.

- Konstantin

RE: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe with buf_size

[Long Li]

> Subject: RE: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe with
> buf_size
> 
> > From: Long Li <longli@microsoft.com>
> > Sent: Wednesday, 24 April 2024 01:35
> > To: Konstantin Taranov <kotaranov@linux.microsoft.com>; Konstantin
> > Taranov <kotaranov@microsoft.com>; sharmaajay@microsoft.com;
> > jgg@ziepe.ca; leon@kernel.org
> > Cc: linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe
> > with buf_size
> >
> > > Subject: [PATCH rdma-next 3/6] RDMA/mana_ib: replace duplicate cqe
> > > with buf_size
> >
> > I don't understand this commit message on "duplicate" cqe. I couldn't
> > find a duplicate of it in the existing code.
> 
> If we need cqe, we could use it at cq->ibcq.cqe. The patch does not assign it as it
> is not used, but if you want I can add "cq->ibcq.cqe = attr->cqe;" in v2.
> 
> - Konstantin

I see. We don't need buf_size because it can be computed from cq->ibcq.cqe?

The commit message is confusing enough to make people think cqe is a duplicate of buf_size.

Long 

[PATCH rdma-next 4/6] RDMA/mana_ib: introduce a helper to remove cq callbacks

[Konstantin Taranov]

From: Konstantin Taranov <kotaranov@microsoft.com>

Intoduce the mana_ib_remove_cq_cb helper to remove cq callbacks.
The helper removes code duplicates.

Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
---
 drivers/infiniband/hw/mana/cq.c      | 19 ++++++++++++-------
 drivers/infiniband/hw/mana/mana_ib.h |  1 +
 drivers/infiniband/hw/mana/qp.c      | 26 ++++----------------------
 3 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index 0467ee8..6c3bb8c 100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -48,16 +48,10 @@ int mana_ib_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
 	struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
 	struct ib_device *ibdev = ibcq->device;
 	struct mana_ib_dev *mdev;
-	struct gdma_context *gc;
 
 	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
-	gc = mdev_to_gc(mdev);
-
-	if (cq->queue.id != INVALID_QUEUE_ID) {
-		kfree(gc->cq_table[cq->queue.id]);
-		gc->cq_table[cq->queue.id] = NULL;
-	}
 
+	mana_ib_remove_cq_cb(mdev, cq);
 	mana_ib_destroy_queue(mdev, &cq->queue);
 
 	return 0;
@@ -89,3 +83,14 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 	gc->cq_table[cq->queue.id] = gdma_cq;
 	return 0;
 }
+
+void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
+{
+	struct gdma_context *gc = mdev_to_gc(mdev);
+
+	if (cq->queue.id >= gc->max_num_cqs)
+		return;
+
+	kfree(gc->cq_table[cq->queue.id]);
+	gc->cq_table[cq->queue.id] = NULL;
+}
diff --git a/drivers/infiniband/hw/mana/mana_ib.h b/drivers/infiniband/hw/mana/mana_ib.h
index 9c07021..6c19f4f 100644
--- a/drivers/infiniband/hw/mana/mana_ib.h
+++ b/drivers/infiniband/hw/mana/mana_ib.h
@@ -255,6 +255,7 @@ static inline void copy_in_reverse(u8 *dst, const u8 *src, u32 size)
 }
 
 int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq);
+void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq);
 
 int mana_ib_create_zero_offset_dma_region(struct mana_ib_dev *dev, struct ib_umem *umem,
 					  mana_handle_t *gdma_region);
diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
index c4fb8b4..169b286 100644
--- a/drivers/infiniband/hw/mana/qp.c
+++ b/drivers/infiniband/hw/mana/qp.c
@@ -95,11 +95,9 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
 	struct mana_ib_qp *qp = container_of(ibqp, struct mana_ib_qp, ibqp);
 	struct mana_ib_dev *mdev =
 		container_of(pd->device, struct mana_ib_dev, ib_dev);
-	struct gdma_context *gc = mdev_to_gc(mdev);
 	struct ib_rwq_ind_table *ind_tbl = attr->rwq_ind_tbl;
 	struct mana_ib_create_qp_rss_resp resp = {};
 	struct mana_ib_create_qp_rss ucmd = {};
-	struct gdma_queue **gdma_cq_allocated;
 	mana_handle_t *mana_ind_table;
 	struct mana_port_context *mpc;
 	unsigned int ind_tbl_size;
@@ -173,13 +171,6 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
 		goto fail;
 	}
 
-	gdma_cq_allocated = kcalloc(ind_tbl_size, sizeof(*gdma_cq_allocated),
-				    GFP_KERNEL);
-	if (!gdma_cq_allocated) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-
 	qp->port = port;
 
 	for (i = 0; i < ind_tbl_size; i++) {
@@ -229,8 +220,6 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
 		ret = mana_ib_install_cq_cb(mdev, cq);
 		if (ret)
 			goto fail;
-
-		gdma_cq_allocated[i] = gc->cq_table[cq->queue.id];
 	}
 	resp.num_entries = i;
 
@@ -250,7 +239,6 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
 		goto fail;
 	}
 
-	kfree(gdma_cq_allocated);
 	kfree(mana_ind_table);
 
 	return 0;
@@ -262,13 +250,10 @@ fail:
 		wq = container_of(ibwq, struct mana_ib_wq, ibwq);
 		cq = container_of(ibcq, struct mana_ib_cq, ibcq);
 
-		gc->cq_table[cq->queue.id] = NULL;
-		kfree(gdma_cq_allocated[i]);
-
+		mana_ib_remove_cq_cb(mdev, cq);
 		mana_destroy_wq_obj(mpc, GDMA_RQ, wq->rx_object);
 	}
 
-	kfree(gdma_cq_allocated);
 	kfree(mana_ind_table);
 
 	return ret;
@@ -287,10 +272,8 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd *ibpd,
 	struct mana_ib_ucontext *mana_ucontext =
 		rdma_udata_to_drv_context(udata, struct mana_ib_ucontext,
 					  ibucontext);
-	struct gdma_context *gc = mdev_to_gc(mdev);
 	struct mana_ib_create_qp_resp resp = {};
 	struct mana_ib_create_qp ucmd = {};
-	struct gdma_queue *gdma_cq = NULL;
 	struct mana_obj_spec wq_spec = {};
 	struct mana_obj_spec cq_spec = {};
 	struct mana_port_context *mpc;
@@ -395,14 +378,13 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd *ibpd,
 		ibdev_dbg(&mdev->ib_dev,
 			  "Failed copy udata for create qp-raw, %d\n",
 			  err);
-		goto err_release_gdma_cq;
+		goto err_remove_cq_cb;
 	}
 
 	return 0;
 
-err_release_gdma_cq:
-	kfree(gdma_cq);
-	gc->cq_table[send_cq->queue.id] = NULL;
+err_remove_cq_cb:
+	mana_ib_remove_cq_cb(mdev, send_cq);
 
 err_destroy_wq_obj:
 	mana_destroy_wq_obj(mpc, GDMA_SQ, qp->qp_handle);
-- 
2.43.0

RE: [PATCH rdma-next 4/6] RDMA/mana_ib: introduce a helper to remove cq callbacks

[Long Li]

> Subject: [PATCH rdma-next 4/6] RDMA/mana_ib: introduce a helper to
> remove cq callbacks
> 
> From: Konstantin Taranov <kotaranov@microsoft.com>
> 
> Intoduce the mana_ib_remove_cq_cb helper to remove cq callbacks.
> The helper removes code duplicates.
> 
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> ---
>  drivers/infiniband/hw/mana/cq.c      | 19 ++++++++++++-------
>  drivers/infiniband/hw/mana/mana_ib.h |  1 +
>  drivers/infiniband/hw/mana/qp.c      | 26 ++++----------------------
>  3 files changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mana/cq.c
> b/drivers/infiniband/hw/mana/cq.c index 0467ee8..6c3bb8c 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -48,16 +48,10 @@ int mana_ib_destroy_cq(struct ib_cq *ibcq, struct
> ib_udata *udata)
>  	struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
>  	struct ib_device *ibdev = ibcq->device;
>  	struct mana_ib_dev *mdev;
> -	struct gdma_context *gc;
> 
>  	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> -	gc = mdev_to_gc(mdev);
> -
> -	if (cq->queue.id != INVALID_QUEUE_ID) {
> -		kfree(gc->cq_table[cq->queue.id]);
> -		gc->cq_table[cq->queue.id] = NULL;
> -	}
> 
> +	mana_ib_remove_cq_cb(mdev, cq);
>  	mana_ib_destroy_queue(mdev, &cq->queue);
> 
>  	return 0;
> @@ -89,3 +83,14 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev,
> struct mana_ib_cq *cq)
>  	gc->cq_table[cq->queue.id] = gdma_cq;
>  	return 0;
>  }
> +
> +void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct
> mana_ib_cq
> +*cq) {
> +	struct gdma_context *gc = mdev_to_gc(mdev);
> +
> +	if (cq->queue.id >= gc->max_num_cqs)
> +		return;
> +
> +	kfree(gc->cq_table[cq->queue.id]);
> +	gc->cq_table[cq->queue.id] = NULL;

Why the check for (cq->queue.id != INVALID_QUEUE_ID) is removed?

> +}
> diff --git a/drivers/infiniband/hw/mana/mana_ib.h
> b/drivers/infiniband/hw/mana/mana_ib.h
> index 9c07021..6c19f4f 100644
> --- a/drivers/infiniband/hw/mana/mana_ib.h
> +++ b/drivers/infiniband/hw/mana/mana_ib.h
> @@ -255,6 +255,7 @@ static inline void copy_in_reverse(u8 *dst, const u8
> *src, u32 size)  }
> 
>  int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq
> *cq);
> +void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct
> mana_ib_cq
> +*cq);
> 
>  int mana_ib_create_zero_offset_dma_region(struct mana_ib_dev *dev,
> struct ib_umem *umem,
>  					  mana_handle_t *gdma_region);
> diff --git a/drivers/infiniband/hw/mana/qp.c
> b/drivers/infiniband/hw/mana/qp.c index c4fb8b4..169b286 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
> @@ -95,11 +95,9 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp,
> struct ib_pd *pd,
>  	struct mana_ib_qp *qp = container_of(ibqp, struct mana_ib_qp,
> ibqp);
>  	struct mana_ib_dev *mdev =
>  		container_of(pd->device, struct mana_ib_dev, ib_dev);
> -	struct gdma_context *gc = mdev_to_gc(mdev);
>  	struct ib_rwq_ind_table *ind_tbl = attr->rwq_ind_tbl;
>  	struct mana_ib_create_qp_rss_resp resp = {};
>  	struct mana_ib_create_qp_rss ucmd = {};
> -	struct gdma_queue **gdma_cq_allocated;
>  	mana_handle_t *mana_ind_table;
>  	struct mana_port_context *mpc;
>  	unsigned int ind_tbl_size;
> @@ -173,13 +171,6 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp,
> struct ib_pd *pd,
>  		goto fail;
>  	}
> 
> -	gdma_cq_allocated = kcalloc(ind_tbl_size,
> sizeof(*gdma_cq_allocated),
> -				    GFP_KERNEL);
> -	if (!gdma_cq_allocated) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -

Why the allocation for CQs is removed? This is not related to this patch.

RE: [PATCH rdma-next 4/6] RDMA/mana_ib: introduce a helper to remove cq callbacks

[Konstantin Taranov]

> > +void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct
> > mana_ib_cq
> > +*cq) {
> > +	struct gdma_context *gc = mdev_to_gc(mdev);
> > +
> > +	if (cq->queue.id >= gc->max_num_cqs)
> > +		return;
> > +
> > +	kfree(gc->cq_table[cq->queue.id]);
> > +	gc->cq_table[cq->queue.id] = NULL;
> 
> Why the check for (cq->queue.id != INVALID_QUEUE_ID) is removed?

As max_num_cqs is always less than INVALID_QUEUE_ID, it is included in the "if".
I can add " || cq->queue.id == INVALID_QUEUE_ID " to the condition if you want.

> > @@ -173,13 +171,6 @@ static int mana_ib_create_qp_rss(struct ib_qp
> > *ibqp, struct ib_pd *pd,
> >  		goto fail;
> >  	}
> >
> > -	gdma_cq_allocated = kcalloc(ind_tbl_size,
> > sizeof(*gdma_cq_allocated),
> > -				    GFP_KERNEL);
> > -	if (!gdma_cq_allocated) {
> > -		ret = -ENOMEM;
> > -		goto fail;
> > -	}
> > -
> 
> Why the allocation for CQs is removed? This is not related to this patch.

It becomes the dead code if I add the helper. You allocated gdma_cq_allocated to
temporary store gdma_queue to be able to deallocate them. The introduced helper
frees pointers to gdma_queue from kfree(gc->cq_table[cq->queue.id]), making
gdma_cq_allocated unused.

RE: [PATCH rdma-next 4/6] RDMA/mana_ib: introduce a helper to remove cq callbacks

[Long Li]

> > > +void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct
> > > mana_ib_cq
> > > +*cq) {
> > > +	struct gdma_context *gc = mdev_to_gc(mdev);
> > > +
> > > +	if (cq->queue.id >= gc->max_num_cqs)
> > > +		return;
> > > +
> > > +	kfree(gc->cq_table[cq->queue.id]);
> > > +	gc->cq_table[cq->queue.id] = NULL;
> >
> > Why the check for (cq->queue.id != INVALID_QUEUE_ID) is removed?
> 
> As max_num_cqs is always less than INVALID_QUEUE_ID, it is included in the "if".
> I can add " || cq->queue.id == INVALID_QUEUE_ID " to the condition if you want.

Okay, can you add a comment before if (cq->queue.id >= gc->max_num_cqs) saying it also works with INVALID_QUEUE_ID?

[PATCH rdma-next 5/6] RDMA/mana_ib: boundary check before installing cq callbacks

[Konstantin Taranov]

From: Konstantin Taranov <kotaranov@microsoft.com>

Add a boundary check inside mana_ib_install_cq_cb to prevent index overflow.

Fixes: 2a31c5a7e0d8 ("RDMA/mana_ib: Introduce mana_ib_install_cq_cb helper function")
Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
---
 drivers/infiniband/hw/mana/cq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index 6c3bb8c..8323085 100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -70,6 +70,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 	struct gdma_context *gc = mdev_to_gc(mdev);
 	struct gdma_queue *gdma_cq;
 
+	if (cq->queue.id >= gc->max_num_cqs)
+		return -EINVAL;
 	/* Create CQ table entry */
 	WARN_ON(gc->cq_table[cq->queue.id]);
 	gdma_cq = kzalloc(sizeof(*gdma_cq), GFP_KERNEL);
-- 
2.43.0

RE: [PATCH rdma-next 5/6] RDMA/mana_ib: boundary check before installing cq callbacks

[Long Li]

> Subject: [PATCH rdma-next 5/6] RDMA/mana_ib: boundary check before
> installing cq callbacks
> 
> From: Konstantin Taranov <kotaranov@microsoft.com>
> 
> Add a boundary check inside mana_ib_install_cq_cb to prevent index
> overflow.

How is this condition possible that we are getting an out of bound queue id from SOC?

> 
> Fixes: 2a31c5a7e0d8 ("RDMA/mana_ib: Introduce mana_ib_install_cq_cb
> helper function")
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> ---
>  drivers/infiniband/hw/mana/cq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/mana/cq.c
> b/drivers/infiniband/hw/mana/cq.c index 6c3bb8c..8323085 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -70,6 +70,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev,
> struct mana_ib_cq *cq)
>  	struct gdma_context *gc = mdev_to_gc(mdev);
>  	struct gdma_queue *gdma_cq;
> 
> +	if (cq->queue.id >= gc->max_num_cqs)
> +		return -EINVAL;
>  	/* Create CQ table entry */
>  	WARN_ON(gc->cq_table[cq->queue.id]);
>  	gdma_cq = kzalloc(sizeof(*gdma_cq), GFP_KERNEL);
> --
> 2.43.0

RE: [PATCH rdma-next 5/6] RDMA/mana_ib: boundary check before installing cq callbacks

[Konstantin Taranov]

> > Add a boundary check inside mana_ib_install_cq_cb to prevent index
> > overflow.
> 
> How is this condition possible that we are getting an out of bound queue id
> from SOC?
> 

Yes, it should not happen as the HW says the upper limit on CQ_ID,
but I think it is safer to have it to dodge bugs/faulty HW.
Better safe than sorry.
You can see the same check all over the mana.ko module.


> >
> > Fixes: 2a31c5a7e0d8 ("RDMA/mana_ib: Introduce mana_ib_install_cq_cb
> > helper function")
> > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > ---
> >  drivers/infiniband/hw/mana/cq.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/infiniband/hw/mana/cq.c
> > b/drivers/infiniband/hw/mana/cq.c index 6c3bb8c..8323085 100644
> > --- a/drivers/infiniband/hw/mana/cq.c
> > +++ b/drivers/infiniband/hw/mana/cq.c
> > @@ -70,6 +70,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev
> *mdev,
> > struct mana_ib_cq *cq)
> >  	struct gdma_context *gc = mdev_to_gc(mdev);
> >  	struct gdma_queue *gdma_cq;
> >
> > +	if (cq->queue.id >= gc->max_num_cqs)
> > +		return -EINVAL;
> >  	/* Create CQ table entry */
> >  	WARN_ON(gc->cq_table[cq->queue.id]);
> >  	gdma_cq = kzalloc(sizeof(*gdma_cq), GFP_KERNEL);
> > --
> > 2.43.0

RE: [PATCH rdma-next 5/6] RDMA/mana_ib: boundary check before installing cq callbacks

[Long Li]

> Subject: [PATCH rdma-next 5/6] RDMA/mana_ib: boundary check before
> installing cq callbacks
> 
> From: Konstantin Taranov <kotaranov@microsoft.com>
> 
> Add a boundary check inside mana_ib_install_cq_cb to prevent index overflow.
> 
> Fixes: 2a31c5a7e0d8 ("RDMA/mana_ib: Introduce mana_ib_install_cq_cb helper
> function")
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>

Reviewed-by: Long Li <longli@microsoft.com>

> ---
>  drivers/infiniband/hw/mana/cq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index 6c3bb8c..8323085 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -70,6 +70,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev,
> struct mana_ib_cq *cq)
>  	struct gdma_context *gc = mdev_to_gc(mdev);
>  	struct gdma_queue *gdma_cq;
> 
> +	if (cq->queue.id >= gc->max_num_cqs)
> +		return -EINVAL;
>  	/* Create CQ table entry */
>  	WARN_ON(gc->cq_table[cq->queue.id]);
>  	gdma_cq = kzalloc(sizeof(*gdma_cq), GFP_KERNEL);
> --
> 2.43.0

[PATCH rdma-next 6/6] RDMA/mana_ib: implement uapi for creation of rnic cq

[Konstantin Taranov]

From: Konstantin Taranov <kotaranov@microsoft.com>

Enable users to create RNIC CQs.
With the previous request size, an ethernet CQ is created.
Use the cq_buf_size from the user to create an RNIC CQ and return its ID.

Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
---
 drivers/infiniband/hw/mana/cq.c | 56 ++++++++++++++++++++++++++++++---
 include/uapi/rdma/mana-abi.h    |  7 +++++
 2 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index 8323085..a62bda7 100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -9,17 +9,25 @@ int mana_ib_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 		      struct ib_udata *udata)
 {
 	struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
+	struct mana_ib_create_cq_resp resp = {};
+	struct mana_ib_ucontext *mana_ucontext;
 	struct ib_device *ibdev = ibcq->device;
 	struct mana_ib_create_cq ucmd = {};
 	struct mana_ib_dev *mdev;
+	bool is_rnic_cq = true;
+	u32 doorbell;
 	int err;
 
 	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
 
-	if (udata->inlen < sizeof(ucmd))
+	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
+	cq->cq_handle = INVALID_MANA_HANDLE;
+
+	if (udata->inlen < offsetof(struct mana_ib_create_cq, cq_buf_size))
 		return -EINVAL;
 
-	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
+	if (udata->inlen == offsetof(struct mana_ib_create_cq, cq_buf_size))
+		is_rnic_cq = false;
 
 	err = ib_copy_from_udata(&ucmd, udata, min(sizeof(ucmd), udata->inlen));
 	if (err) {
@@ -28,19 +36,53 @@ int mana_ib_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 		return err;
 	}
 
-	if (attr->cqe > mdev->adapter_caps.max_qp_wr) {
+	if (!is_rnic_cq && attr->cqe > mdev->adapter_caps.max_qp_wr) {
 		ibdev_dbg(ibdev, "CQE %d exceeding limit\n", attr->cqe);
 		return -EINVAL;
 	}
 
-	cq->buf_size = attr->cqe * COMP_ENTRY_SIZE;
+	cq->buf_size = is_rnic_cq ? ucmd.cq_buf_size : attr->cqe * COMP_ENTRY_SIZE;
 	err = mana_ib_create_queue(mdev, ucmd.buf_addr, cq->buf_size, &cq->queue);
 	if (err) {
 		ibdev_dbg(ibdev, "Failed to create queue for create cq, %d\n", err);
 		return err;
 	}
 
+	mana_ucontext = rdma_udata_to_drv_context(udata, struct mana_ib_ucontext,
+						  ibucontext);
+	doorbell = mana_ucontext->doorbell;
+
+	if (is_rnic_cq) {
+		err = mana_ib_gd_create_cq(mdev, cq, doorbell);
+		if (err) {
+			ibdev_dbg(ibdev, "Failed to create RNIC cq, %d\n", err);
+			goto err_destroy_queue;
+		}
+
+		err = mana_ib_install_cq_cb(mdev, cq);
+		if (err) {
+			ibdev_dbg(ibdev, "Failed to install cq callback, %d\n", err);
+			goto err_destroy_rnic_cq;
+		}
+	}
+
+	resp.cqid = cq->queue.id;
+	err = ib_copy_to_udata(udata, &resp, min(sizeof(resp), udata->outlen));
+	if (err) {
+		ibdev_dbg(&mdev->ib_dev, "Failed to copy to udata, %d\n", err);
+		goto err_remove_cq_cb;
+	}
+
 	return 0;
+
+err_remove_cq_cb:
+	mana_ib_remove_cq_cb(mdev, cq);
+err_destroy_rnic_cq:
+	mana_ib_gd_destroy_cq(mdev, cq);
+err_destroy_queue:
+	mana_ib_destroy_queue(mdev, &cq->queue);
+
+	return err;
 }
 
 int mana_ib_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
@@ -52,6 +94,12 @@ int mana_ib_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
 	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
 
 	mana_ib_remove_cq_cb(mdev, cq);
+
+	/* Ignore return code as there is not much we can do about it.
+	 * The error message is printed inside.
+	 */
+	mana_ib_gd_destroy_cq(mdev, cq);
+
 	mana_ib_destroy_queue(mdev, &cq->queue);
 
 	return 0;
diff --git a/include/uapi/rdma/mana-abi.h b/include/uapi/rdma/mana-abi.h
index 5fcb31b..8fc9d32 100644
--- a/include/uapi/rdma/mana-abi.h
+++ b/include/uapi/rdma/mana-abi.h
@@ -18,6 +18,13 @@
 
 struct mana_ib_create_cq {
 	__aligned_u64 buf_addr;
+	__u32 cq_buf_size;
+	__u32 reserved;
+};
+
+struct mana_ib_create_cq_resp {
+	__u32 cqid;
+	__u32 reserved;
 };
 
 struct mana_ib_create_qp {
-- 
2.43.0

RE: [PATCH rdma-next 6/6] RDMA/mana_ib: implement uapi for creation of rnic cq

[Long Li]

> Subject: [PATCH rdma-next 6/6] RDMA/mana_ib: implement uapi for creation
> of rnic cq
> 
> From: Konstantin Taranov <kotaranov@microsoft.com>
> 
> Enable users to create RNIC CQs.
> With the previous request size, an ethernet CQ is created.
> Use the cq_buf_size from the user to create an RNIC CQ and return its ID.
> 
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> ---
>  drivers/infiniband/hw/mana/cq.c | 56 ++++++++++++++++++++++++++++++---
>  include/uapi/rdma/mana-abi.h    |  7 +++++
>  2 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mana/cq.c
> b/drivers/infiniband/hw/mana/cq.c index 8323085..a62bda7 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -9,17 +9,25 @@ int mana_ib_create_cq(struct ib_cq *ibcq, const struct
> ib_cq_init_attr *attr,
>  		      struct ib_udata *udata)
>  {
>  	struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
> +	struct mana_ib_create_cq_resp resp = {};
> +	struct mana_ib_ucontext *mana_ucontext;
>  	struct ib_device *ibdev = ibcq->device;
>  	struct mana_ib_create_cq ucmd = {};
>  	struct mana_ib_dev *mdev;
> +	bool is_rnic_cq = true;
> +	u32 doorbell;
>  	int err;
> 
>  	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> 
> -	if (udata->inlen < sizeof(ucmd))
> +	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
> +	cq->cq_handle = INVALID_MANA_HANDLE;
> +
> +	if (udata->inlen < offsetof(struct mana_ib_create_cq, cq_buf_size))
>  		return -EINVAL;
> 
> -	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
> +	if (udata->inlen == offsetof(struct mana_ib_create_cq, cq_buf_size))
> +		is_rnic_cq = false;

I think it's okay with checking on offset in uapi message to decide if this is a newer/updated RNIC uverb.

But increasing MANA_IB_UVERBS_ABI_VERSION may make the code simpler. I have a feeling that you may need to increase it anyway, because a new uapi message "mana_ib_create_cq_resp" is introduced.

Jason or Leon may have a better idea on this.

Re: [PATCH rdma-next 6/6] RDMA/mana_ib: implement uapi for creation of rnic cq

[Leon Romanovsky]

On Tue, Apr 23, 2024 at 11:57:53PM +0000, Long Li wrote:
> > Subject: [PATCH rdma-next 6/6] RDMA/mana_ib: implement uapi for creation
> > of rnic cq
> > 
> > From: Konstantin Taranov <kotaranov@microsoft.com>
> > 
> > Enable users to create RNIC CQs.
> > With the previous request size, an ethernet CQ is created.
> > Use the cq_buf_size from the user to create an RNIC CQ and return its ID.
> > 
> > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > ---
> >  drivers/infiniband/hw/mana/cq.c | 56 ++++++++++++++++++++++++++++++---
> >  include/uapi/rdma/mana-abi.h    |  7 +++++
> >  2 files changed, 59 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/infiniband/hw/mana/cq.c
> > b/drivers/infiniband/hw/mana/cq.c index 8323085..a62bda7 100644
> > --- a/drivers/infiniband/hw/mana/cq.c
> > +++ b/drivers/infiniband/hw/mana/cq.c
> > @@ -9,17 +9,25 @@ int mana_ib_create_cq(struct ib_cq *ibcq, const struct
> > ib_cq_init_attr *attr,
> >  		      struct ib_udata *udata)
> >  {
> >  	struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
> > +	struct mana_ib_create_cq_resp resp = {};
> > +	struct mana_ib_ucontext *mana_ucontext;
> >  	struct ib_device *ibdev = ibcq->device;
> >  	struct mana_ib_create_cq ucmd = {};
> >  	struct mana_ib_dev *mdev;
> > +	bool is_rnic_cq = true;
> > +	u32 doorbell;
> >  	int err;
> > 
> >  	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> > 
> > -	if (udata->inlen < sizeof(ucmd))
> > +	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
> > +	cq->cq_handle = INVALID_MANA_HANDLE;
> > +
> > +	if (udata->inlen < offsetof(struct mana_ib_create_cq, cq_buf_size))
> >  		return -EINVAL;
> > 
> > -	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
> > +	if (udata->inlen == offsetof(struct mana_ib_create_cq, cq_buf_size))
> > +		is_rnic_cq = false;
> 
> I think it's okay with checking on offset in uapi message to decide if this is a newer/updated RNIC uverb.
> 
> But increasing MANA_IB_UVERBS_ABI_VERSION may make the code simpler. I have a feeling that you may need to increase it anyway, because a new uapi message "mana_ib_create_cq_resp" is introduced.
> 
> Jason or Leon may have a better idea on this.

You should really try to avoid changing MANA_IB_UVERBS_ABI_VERSION as it
usually means that backward compatibility will be broken after such change.

Thanks

Re: [PATCH rdma-next 6/6] RDMA/mana_ib: implement uapi for creation of rnic cq

[Jason Gunthorpe]

On Tue, Apr 30, 2024 at 03:37:15PM +0300, Leon Romanovsky wrote:
> On Tue, Apr 23, 2024 at 11:57:53PM +0000, Long Li wrote:
> > > Subject: [PATCH rdma-next 6/6] RDMA/mana_ib: implement uapi for creation
> > > of rnic cq
> > > 
> > > From: Konstantin Taranov <kotaranov@microsoft.com>
> > > 
> > > Enable users to create RNIC CQs.
> > > With the previous request size, an ethernet CQ is created.
> > > Use the cq_buf_size from the user to create an RNIC CQ and return its ID.
> > > 
> > > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > > ---
> > >  drivers/infiniband/hw/mana/cq.c | 56 ++++++++++++++++++++++++++++++---
> > >  include/uapi/rdma/mana-abi.h    |  7 +++++
> > >  2 files changed, 59 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/hw/mana/cq.c
> > > b/drivers/infiniband/hw/mana/cq.c index 8323085..a62bda7 100644
> > > --- a/drivers/infiniband/hw/mana/cq.c
> > > +++ b/drivers/infiniband/hw/mana/cq.c
> > > @@ -9,17 +9,25 @@ int mana_ib_create_cq(struct ib_cq *ibcq, const struct
> > > ib_cq_init_attr *attr,
> > >  		      struct ib_udata *udata)
> > >  {
> > >  	struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
> > > +	struct mana_ib_create_cq_resp resp = {};
> > > +	struct mana_ib_ucontext *mana_ucontext;
> > >  	struct ib_device *ibdev = ibcq->device;
> > >  	struct mana_ib_create_cq ucmd = {};
> > >  	struct mana_ib_dev *mdev;
> > > +	bool is_rnic_cq = true;
> > > +	u32 doorbell;
> > >  	int err;
> > > 
> > >  	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> > > 
> > > -	if (udata->inlen < sizeof(ucmd))
> > > +	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
> > > +	cq->cq_handle = INVALID_MANA_HANDLE;
> > > +
> > > +	if (udata->inlen < offsetof(struct mana_ib_create_cq, cq_buf_size))
> > >  		return -EINVAL;
> > > 
> > > -	cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
> > > +	if (udata->inlen == offsetof(struct mana_ib_create_cq, cq_buf_size))
> > > +		is_rnic_cq = false;
> > 
> > I think it's okay with checking on offset in uapi message to decide if this is a newer/updated RNIC uverb.

Yes, this is the expected method

> You should really try to avoid changing MANA_IB_UVERBS_ABI_VERSION as it
> usually means that backward compatibility will be broken after such change.

Yes, please don't do that.

Jason