Re: [PATCH for-next 2/2] RDMA/bnxt_re: Share a page to expose per CQ info with userspace

[Leon Romanovsky <leon@kernel.org>]

On Tue, Dec 12, 2023 at 07:33:31PM -0800, Selvin Xavier wrote:
> Gen P7 adapters needs to share a toggle bits information received
> in kernel driver with the user space. User space needs this
> info during the request notify call back to arm the CQ.
> 
> User space application can get this page using the
> UAPI routines. Library will mmap this page and get the
> toggle bits to be used in the next ARM Doorbell.
> 
> Uses a hash list to map the CQ structure from the CQ ID.
> CQ structure is retrieved from the hash list while the
> library calls the UAPI routine to get the toggle page
> mapping. Currently the full page is mapped per CQ. This
> can be optimized to enable multiple CQs from the same
> application share the same page and different offsets
> in the page.
> 
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
>  drivers/infiniband/hw/bnxt_re/bnxt_re.h   |  3 ++
>  drivers/infiniband/hw/bnxt_re/ib_verbs.c  | 59 +++++++++++++++++++++++++++----
>  drivers/infiniband/hw/bnxt_re/ib_verbs.h  |  2 ++
>  drivers/infiniband/hw/bnxt_re/main.c      | 10 +++++-
>  drivers/infiniband/hw/bnxt_re/qplib_res.h |  6 ++++
>  include/uapi/rdma/bnxt_re-abi.h           |  5 +++
>  6 files changed, 77 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/bnxt_re/bnxt_re.h b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
> index 9fd9849..9dca451 100644
> --- a/drivers/infiniband/hw/bnxt_re/bnxt_re.h
> +++ b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
> @@ -41,6 +41,7 @@
>  #define __BNXT_RE_H__
>  #include <rdma/uverbs_ioctl.h>
>  #include "hw_counters.h"
> +#include <linux/hashtable.h>
>  #define ROCE_DRV_MODULE_NAME		"bnxt_re"
>  
>  #define BNXT_RE_DESC	"Broadcom NetXtreme-C/E RoCE Driver"
> @@ -135,6 +136,7 @@ struct bnxt_re_pacing {
>  #define BNXT_RE_DB_FIFO_ROOM_SHIFT 15
>  #define BNXT_RE_GRC_FIFO_REG_BASE 0x2000
>  
> +#define MAX_CQ_HASH_BITS		(16)
>  struct bnxt_re_dev {
>  	struct ib_device		ibdev;
>  	struct list_head		list;
> @@ -189,6 +191,7 @@ struct bnxt_re_dev {
>  	struct bnxt_re_pacing pacing;
>  	struct work_struct dbq_fifo_check_work;
>  	struct delayed_work dbq_pacing_work;
> +	DECLARE_HASHTABLE(cq_hash, MAX_CQ_HASH_BITS);
>  };
>  
>  #define to_bnxt_re_dev(ptr, member)	\
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 76cea30..de3d404 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -50,6 +50,7 @@
>  #include <rdma/ib_mad.h>
>  #include <rdma/ib_cache.h>
>  #include <rdma/uverbs_ioctl.h>
> +#include <linux/hashtable.h>
>  
>  #include "bnxt_ulp.h"
>  
> @@ -2910,14 +2911,20 @@ int bnxt_re_post_recv(struct ib_qp *ib_qp, const struct ib_recv_wr *wr,
>  /* Completion Queues */
>  int bnxt_re_destroy_cq(struct ib_cq *ib_cq, struct ib_udata *udata)
>  {
> -	struct bnxt_re_cq *cq;
> +	struct bnxt_qplib_chip_ctx *cctx;
>  	struct bnxt_qplib_nq *nq;
>  	struct bnxt_re_dev *rdev;
> +	struct bnxt_re_cq *cq;
>  
>  	cq = container_of(ib_cq, struct bnxt_re_cq, ib_cq);
>  	rdev = cq->rdev;
>  	nq = cq->qplib_cq.nq;
> +	cctx = rdev->chip_ctx;
>  
> +	if (cctx->modes.toggle_bits & BNXT_QPLIB_CQ_TOGGLE_BIT) {
> +		free_page((unsigned long)cq->uctx_cq_page);
> +		hash_del(&cq->hash_entry);
> +	}
>  	bnxt_qplib_destroy_cq(&rdev->qplib_res, &cq->qplib_cq);
>  	ib_umem_release(cq->umem);
>  
> @@ -2935,10 +2942,11 @@ int bnxt_re_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>  	struct bnxt_re_ucontext *uctx =
>  		rdma_udata_to_drv_context(udata, struct bnxt_re_ucontext, ib_uctx);
>  	struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr;
> -	int rc, entries;
> -	int cqe = attr->cqe;
> +	struct bnxt_qplib_chip_ctx *cctx;
>  	struct bnxt_qplib_nq *nq = NULL;
>  	unsigned int nq_alloc_cnt;
> +	int rc = -1, entries;

Why -1 and not some -EXXX value?

> +	int cqe = attr->cqe;
>  	u32 active_cqs;
>  
>  	if (attr->flags)
> @@ -2951,6 +2959,7 @@ int bnxt_re_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>  	}
>  
>  	cq->rdev = rdev;
> +	cctx = rdev->chip_ctx;
>  	cq->qplib_cq.cq_handle = (u64)(unsigned long)(&cq->qplib_cq);
>  
>  	entries = bnxt_re_init_depth(cqe + 1, uctx);
> @@ -3012,8 +3021,16 @@ int bnxt_re_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>  	spin_lock_init(&cq->cq_lock);
>  
>  	if (udata) {
> -		struct bnxt_re_cq_resp resp;
> -
> +		struct bnxt_re_cq_resp resp = {};
> +
> +		if (cctx->modes.toggle_bits & BNXT_QPLIB_CQ_TOGGLE_BIT) {

This bit is set for all gen_p7 cards, but bnxt_re_cq_resp doesn't have
comp_mask field in old rdma-core and it will cause to ib_copy_to_udata()
fail, isn't it?

> +			hash_add(rdev->cq_hash, &cq->hash_entry, cq->qplib_cq.id);
> +			/* Allocate a page */
> +			cq->uctx_cq_page = (void *)get_zeroed_page(GFP_KERNEL);
> +			if (!cq->uctx_cq_page)
> +				goto c2fail;
> +			resp.comp_mask |= BNXT_RE_CQ_TOGGLE_PAGE_SUPPORT;
> +		}

Thanks