[PATCH v3 rdma-next 0/2] RDMA/qedr: Add EDPM kernel-user flags for feature compatibility

[PATCH v3 rdma-next 0/2] RDMA/qedr: Add EDPM kernel-user flags for feature compatibility

[Michal Kalderon]

The two patches in this series are related to the EDPM feature.
EDPM is a hw feature for improving latency under certain conditions.

Related rdma-core pull request #786
https://github.com/linux-rdma/rdma-core/pull/786

Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>

Changes from v2
Remove mis-use of !! when checking if a flag is set.

Changes from v1
Add explicit padding to struct qedr_alloc_ucontext_resp in qedr-abi.h

Michal Kalderon (2):
  RDMA/qedr: Add EDPM mode type for user-fw compatibility
  RDMA/qedr: Add EDPM max size to alloc ucontext response

 drivers/infiniband/hw/qedr/qedr.h  |  1 +
 drivers/infiniband/hw/qedr/verbs.c | 20 ++++++++++++++------
 include/uapi/rdma/qedr-abi.h       | 10 +++++++---
 3 files changed, 22 insertions(+), 9 deletions(-)

-- 
2.14.5

[PATCH v3 rdma-next 1/2] RDMA/qedr: Add EDPM mode type for user-fw compatibility

[Michal Kalderon]

In older FW versions the completion flag was treated as the ack flag
in edpm messages.
commit ff937b916eb6 ("qed: Add EDPM mode type for user-fw compatibility")
exposed the FW option of setting which mode the QP is in by
adding a flag to the qedr <-> qed API.

This patch adds the qedr <-> libqedr interface so that the libqedr
can set the flag appropriately and qedr can pass it down to FW.
Flag is added for backward compatibility with libqedr.

For older libs, this flag didn't exist and therefore set to zero.

Fixes: ac1b36e55a51 ("qedr: Add support for user context verbs")
Signed-off-by: Yuval Bason <yuval.bason@marvell.com>
Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
---
 drivers/infiniband/hw/qedr/qedr.h  |  1 +
 drivers/infiniband/hw/qedr/verbs.c | 11 ++++++++---
 include/uapi/rdma/qedr-abi.h       |  4 ++--
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/qedr/qedr.h b/drivers/infiniband/hw/qedr/qedr.h
index fdf90ecb2699..13d5eafb553f 100644
--- a/drivers/infiniband/hw/qedr/qedr.h
+++ b/drivers/infiniband/hw/qedr/qedr.h
@@ -235,6 +235,7 @@ struct qedr_ucontext {
 	u32 dpi_size;
 	u16 dpi;
 	bool db_rec;
+	u8 edpm_mode;
 };
 
 union db_prod32 {
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index 9b9e80266367..fbb0c66c7f2c 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -275,7 +275,8 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
 			DP_ERR(dev, "Problem copying data from user space\n");
 			return -EFAULT;
 		}
-
+		ctx->edpm_mode = !!(ureq.context_flags &
+				    QEDR_ALLOC_UCTX_EDPM_MODE);
 		ctx->db_rec = !!(ureq.context_flags & QEDR_ALLOC_UCTX_DB_REC);
 	}
 
@@ -316,7 +317,8 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
 		uresp.dpm_flags = QEDR_DPM_TYPE_IWARP_LEGACY;
 	else
 		uresp.dpm_flags = QEDR_DPM_TYPE_ROCE_ENHANCED |
-				  QEDR_DPM_TYPE_ROCE_LEGACY;
+				  QEDR_DPM_TYPE_ROCE_LEGACY |
+				  QEDR_DPM_TYPE_ROCE_EDPM_MODE;
 
 	uresp.dpm_flags |= QEDR_DPM_SIZES_SET;
 	uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE;
@@ -1750,7 +1752,7 @@ static int qedr_create_user_qp(struct qedr_dev *dev,
 	struct qed_rdma_create_qp_out_params out_params;
 	struct qedr_pd *pd = get_qedr_pd(ibpd);
 	struct qedr_create_qp_uresp uresp;
-	struct qedr_ucontext *ctx = NULL;
+	struct qedr_ucontext *ctx = pd ? pd->uctx : NULL;
 	struct qedr_create_qp_ureq ureq;
 	int alloc_and_init = rdma_protocol_roce(&dev->ibdev, 1);
 	int rc = -EINVAL;
@@ -1788,6 +1790,9 @@ static int qedr_create_user_qp(struct qedr_dev *dev,
 		in_params.rq_pbl_ptr = qp->urq.pbl_tbl->pa;
 	}
 
+	if (ctx)
+		SET_FIELD(in_params.flags, QED_ROCE_EDPM_MODE, ctx->edpm_mode);
+
 	qp->qed_qp = dev->ops->rdma_create_qp(dev->rdma_ctx,
 					      &in_params, &out_params);
 
diff --git a/include/uapi/rdma/qedr-abi.h b/include/uapi/rdma/qedr-abi.h
index a0b83c9d4498..b261c9fca07b 100644
--- a/include/uapi/rdma/qedr-abi.h
+++ b/include/uapi/rdma/qedr-abi.h
@@ -39,7 +39,7 @@
 
 /* user kernel communication data structures. */
 enum qedr_alloc_ucontext_flags {
-	QEDR_ALLOC_UCTX_RESERVED	= 1 << 0,
+	QEDR_ALLOC_UCTX_EDPM_MODE	= 1 << 0,
 	QEDR_ALLOC_UCTX_DB_REC		= 1 << 1
 };
 
@@ -56,7 +56,7 @@ enum qedr_rdma_dpm_type {
 	QEDR_DPM_TYPE_ROCE_ENHANCED	= 1 << 0,
 	QEDR_DPM_TYPE_ROCE_LEGACY	= 1 << 1,
 	QEDR_DPM_TYPE_IWARP_LEGACY	= 1 << 2,
-	QEDR_DPM_TYPE_RESERVED		= 1 << 3,
+	QEDR_DPM_TYPE_ROCE_EDPM_MODE	= 1 << 3,
 	QEDR_DPM_SIZES_SET		= 1 << 4,
 };
 
-- 
2.14.5

[PATCH v3 rdma-next 2/2] RDMA/qedr: Add EDPM max size to alloc ucontext response

[Michal Kalderon]

User space should receive the maximum edpm size from kernel
driver, similar to other edpm/ldpm related limits.
Add an additional parameter to the alloc_ucontext_resp
structure for the edpm maximum size.

In addition, pass an indication from user-space to kernel
(and not just kernel to user) that the DPM sizes are supported.

This is for supporting backward-forward compatibility between driver and
lib for everything related to DPM transaction and limit sizes.

This should have been part of commit mentioned in Fixes tag.
Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for dpm
enabled mode")
Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
---
 drivers/infiniband/hw/qedr/verbs.c | 9 ++++++---
 include/uapi/rdma/qedr-abi.h       | 6 +++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index fbb0c66c7f2c..cfe4cd637f1c 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
 				  QEDR_DPM_TYPE_ROCE_LEGACY |
 				  QEDR_DPM_TYPE_ROCE_EDPM_MODE;
 
-	uresp.dpm_flags |= QEDR_DPM_SIZES_SET;
-	uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE;
-	uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE;
+	if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) {
+		uresp.dpm_flags |= QEDR_DPM_SIZES_SET;
+		uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE;
+		uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE;
+		uresp.edpm_limit_size = QEDR_EDPM_MAX_SIZE;
+	}
 
 	uresp.wids_enabled = 1;
 	uresp.wid_count = oparams.wid_count;
diff --git a/include/uapi/rdma/qedr-abi.h b/include/uapi/rdma/qedr-abi.h
index b261c9fca07b..bf7333b2b5d7 100644
--- a/include/uapi/rdma/qedr-abi.h
+++ b/include/uapi/rdma/qedr-abi.h
@@ -40,7 +40,8 @@
 /* user kernel communication data structures. */
 enum qedr_alloc_ucontext_flags {
 	QEDR_ALLOC_UCTX_EDPM_MODE	= 1 << 0,
-	QEDR_ALLOC_UCTX_DB_REC		= 1 << 1
+	QEDR_ALLOC_UCTX_DB_REC		= 1 << 1,
+	QEDR_SUPPORT_DPM_SIZES		= 1 << 2,
 };
 
 struct qedr_alloc_ucontext_req {
@@ -50,6 +51,7 @@ struct qedr_alloc_ucontext_req {
 
 #define QEDR_LDPM_MAX_SIZE	(8192)
 #define QEDR_EDPM_TRANS_SIZE	(64)
+#define QEDR_EDPM_MAX_SIZE	(ROCE_REQ_MAX_INLINE_DATA_SIZE)
 
 enum qedr_rdma_dpm_type {
 	QEDR_DPM_TYPE_NONE		= 0,
@@ -77,6 +79,8 @@ struct qedr_alloc_ucontext_resp {
 	__u16 ldpm_limit_size;
 	__u8 edpm_trans_size;
 	__u8 reserved;
+	__u16 edpm_limit_size;
+	__u8 padding[6];
 };
 
 struct qedr_alloc_pd_ureq {
-- 
2.14.5

Re: [PATCH v3 rdma-next 2/2] RDMA/qedr: Add EDPM max size to alloc ucontext response

[Jason Gunthorpe]

On Tue, Jul 07, 2020 at 09:31:00AM +0300, Michal Kalderon wrote:
> User space should receive the maximum edpm size from kernel
> driver, similar to other edpm/ldpm related limits.
> Add an additional parameter to the alloc_ucontext_resp
> structure for the edpm maximum size.
> 
> In addition, pass an indication from user-space to kernel
> (and not just kernel to user) that the DPM sizes are supported.
> 
> This is for supporting backward-forward compatibility between driver and
> lib for everything related to DPM transaction and limit sizes.
> 
> This should have been part of commit mentioned in Fixes tag.
> Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for dpm
> enabled mode")
> Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
> Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
>  drivers/infiniband/hw/qedr/verbs.c | 9 ++++++---
>  include/uapi/rdma/qedr-abi.h       | 6 +++++-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
> index fbb0c66c7f2c..cfe4cd637f1c 100644
> +++ b/drivers/infiniband/hw/qedr/verbs.c
> @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
>  				  QEDR_DPM_TYPE_ROCE_LEGACY |
>  				  QEDR_DPM_TYPE_ROCE_EDPM_MODE;
>  
> -	uresp.dpm_flags |= QEDR_DPM_SIZES_SET;
> -	uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE;
> -	uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE;
> +	if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) {

Why does this need an input flag just to set some outputs?

The usual truncate on not enough size should take care of it, right?

Jason

RE: [PATCH v3 rdma-next 2/2] RDMA/qedr: Add EDPM max size to alloc ucontext response

[Michal Kalderon]

> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Jason Gunthorpe
> 
> On Tue, Jul 07, 2020 at 09:31:00AM +0300, Michal Kalderon wrote:
> > User space should receive the maximum edpm size from kernel driver,
> > similar to other edpm/ldpm related limits.
> > Add an additional parameter to the alloc_ucontext_resp structure for
> > the edpm maximum size.
> >
> > In addition, pass an indication from user-space to kernel (and not
> > just kernel to user) that the DPM sizes are supported.
> >
> > This is for supporting backward-forward compatibility between driver
> > and lib for everything related to DPM transaction and limit sizes.
> >
> > This should have been part of commit mentioned in Fixes tag.
> > Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for dpm
> > enabled mode")
> > Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
> > Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
> > drivers/infiniband/hw/qedr/verbs.c | 9 ++++++---
> >  include/uapi/rdma/qedr-abi.h       | 6 +++++-
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/qedr/verbs.c
> > b/drivers/infiniband/hw/qedr/verbs.c
> > index fbb0c66c7f2c..cfe4cd637f1c 100644
> > +++ b/drivers/infiniband/hw/qedr/verbs.c
> > @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx,
> struct ib_udata *udata)
> >  				  QEDR_DPM_TYPE_ROCE_LEGACY |
> >  				  QEDR_DPM_TYPE_ROCE_EDPM_MODE;
> >
> > -	uresp.dpm_flags |= QEDR_DPM_SIZES_SET;
> > -	uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE;
> > -	uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE;
> > +	if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) {
> 
> Why does this need an input flag just to set some outputs?
> 
> The usual truncate on not enough size should take care of it, right?
At this point it just sets some output, but for future related changes around these sizes
there will also be fw related configurations, we will need to know whether the lib supports
accepting different sizes or not. This is for forward compatibility between libqedr and
driver. 

> 
> Jason

Re: [PATCH v3 rdma-next 2/2] RDMA/qedr: Add EDPM max size to alloc ucontext response

[Jason Gunthorpe]

On Thu, Jul 16, 2020 at 06:17:29PM +0000, Michal Kalderon wrote:
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Jason Gunthorpe
> > 
> > On Tue, Jul 07, 2020 at 09:31:00AM +0300, Michal Kalderon wrote:
> > > User space should receive the maximum edpm size from kernel driver,
> > > similar to other edpm/ldpm related limits.
> > > Add an additional parameter to the alloc_ucontext_resp structure for
> > > the edpm maximum size.
> > >
> > > In addition, pass an indication from user-space to kernel (and not
> > > just kernel to user) that the DPM sizes are supported.
> > >
> > > This is for supporting backward-forward compatibility between driver
> > > and lib for everything related to DPM transaction and limit sizes.
> > >
> > > This should have been part of commit mentioned in Fixes tag.
> > > Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for dpm
> > > enabled mode")
> > > Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
> > > Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
> > > drivers/infiniband/hw/qedr/verbs.c | 9 ++++++---
> > >  include/uapi/rdma/qedr-abi.h       | 6 +++++-
> > >  2 files changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/qedr/verbs.c
> > > b/drivers/infiniband/hw/qedr/verbs.c
> > > index fbb0c66c7f2c..cfe4cd637f1c 100644
> > > +++ b/drivers/infiniband/hw/qedr/verbs.c
> > > @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx,
> > struct ib_udata *udata)
> > >  				  QEDR_DPM_TYPE_ROCE_LEGACY |
> > >  				  QEDR_DPM_TYPE_ROCE_EDPM_MODE;
> > >
> > > -	uresp.dpm_flags |= QEDR_DPM_SIZES_SET;
> > > -	uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE;
> > > -	uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE;
> > > +	if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) {
> > 
> > Why does this need an input flag just to set some outputs?
> > 
> > The usual truncate on not enough size should take care of it, right?
> At this point it just sets some output, but for future related changes around these sizes
> there will also be fw related configurations, we will need to know whether the lib supports
> accepting different sizes or not. This is for forward compatibility between libqedr and
> driver. 

I would be happier to see this flag introduced when it actually had a
purpose, as I really don't like the pattern of conditionally filling
uresp, but OK. Please delete this if when you make use of it the flag properly.

Jason

RE: [EXT] Re: [PATCH v3 rdma-next 2/2] RDMA/qedr: Add EDPM max size to alloc ucontext response

[Michal Kalderon]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, July 16, 2020 9:58 PM
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, Jul 16, 2020 at 06:17:29PM +0000, Michal Kalderon wrote:
> > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > > owner@vger.kernel.org> On Behalf Of Jason Gunthorpe
> > >
> > > On Tue, Jul 07, 2020 at 09:31:00AM +0300, Michal Kalderon wrote:
> > > > User space should receive the maximum edpm size from kernel
> > > > driver, similar to other edpm/ldpm related limits.
> > > > Add an additional parameter to the alloc_ucontext_resp structure
> > > > for the edpm maximum size.
> > > >
> > > > In addition, pass an indication from user-space to kernel (and not
> > > > just kernel to user) that the DPM sizes are supported.
> > > >
> > > > This is for supporting backward-forward compatibility between
> > > > driver and lib for everything related to DPM transaction and limit sizes.
> > > >
> > > > This should have been part of commit mentioned in Fixes tag.
> > > > Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for
> > > > dpm enabled mode")
> > > > Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
> > > > Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
> > > > drivers/infiniband/hw/qedr/verbs.c | 9 ++++++---
> > > >  include/uapi/rdma/qedr-abi.h       | 6 +++++-
> > > >  2 files changed, 11 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/qedr/verbs.c
> > > > b/drivers/infiniband/hw/qedr/verbs.c
> > > > index fbb0c66c7f2c..cfe4cd637f1c 100644
> > > > +++ b/drivers/infiniband/hw/qedr/verbs.c
> > > > @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext
> > > > *uctx,
> > > struct ib_udata *udata)
> > > >  				  QEDR_DPM_TYPE_ROCE_LEGACY |
> > > >  				  QEDR_DPM_TYPE_ROCE_EDPM_MODE;
> > > >
> > > > -	uresp.dpm_flags |= QEDR_DPM_SIZES_SET;
> > > > -	uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE;
> > > > -	uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE;
> > > > +	if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) {
> > >
> > > Why does this need an input flag just to set some outputs?
> > >
> > > The usual truncate on not enough size should take care of it, right?
> > At this point it just sets some output, but for future related changes
> > around these sizes there will also be fw related configurations, we
> > will need to know whether the lib supports accepting different sizes
> > or not. This is for forward compatibility between libqedr and driver.
> 
> I would be happier to see this flag introduced when it actually had a purpose,
> as I really don't like the pattern of conditionally filling uresp, but OK. Please
> delete this if when you make use of it the flag properly.
Sure, thanks.

> 
> Jason

Re: [PATCH v3 rdma-next 0/2] RDMA/qedr: Add EDPM kernel-user flags for feature compatibility

[Jason Gunthorpe]

On Tue, Jul 07, 2020 at 09:30:58AM +0300, Michal Kalderon wrote:
> The two patches in this series are related to the EDPM feature.
> EDPM is a hw feature for improving latency under certain conditions.
> 
> Related rdma-core pull request #786
> https://github.com/linux-rdma/rdma-core/pull/786
> 
> Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
> 
> Changes from v2
> Remove mis-use of !! when checking if a flag is set.
> 
> Changes from v1
> Add explicit padding to struct qedr_alloc_ucontext_resp in qedr-abi.h
> 
> Michal Kalderon (2):
>   RDMA/qedr: Add EDPM mode type for user-fw compatibility
>   RDMA/qedr: Add EDPM max size to alloc ucontext response

applied to for-next, thanks

Jason