[PATCH for-next 0/2] RDMA/qedr: Add feature indications to user-space

[PATCH for-next 0/2] RDMA/qedr: Add feature indications to user-space

[Ram Amrani]

The first patch passes user space indication whether EDPM is supported
or not. EDPM stands for Enhanced Direct Packet Mode for low latency.

The second patch passes the indication of whether WIDs are supported
and how many, per user application. WIDs are per CPU areas in the BAR
that are used for parallel simultaneous doorbells.

Ram Amrani (2):
  RDMA/qedr: notify user application if EDPM is supported
  RDMA/qedr: notify user application number of supported WIDs

 drivers/infiniband/hw/qedr/main.c  | 1 +
 drivers/infiniband/hw/qedr/qedr.h  | 2 ++
 drivers/infiniband/hw/qedr/verbs.c | 9 +++++++++
 include/uapi/rdma/qedr-abi.h       | 3 +++
 4 files changed, 15 insertions(+)

-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for-next 1/2] RDMA/qedr: Notify user application if DPM is supported

[Ram Amrani]

Direct Packet Mode support may be disabled, e.g, due to limited BAR
resources. Notifying the user application prevents wasting cycles
on attempting to send these kind of packets.

Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/qedr/main.c  | 1 +
 drivers/infiniband/hw/qedr/qedr.h  | 2 ++
 drivers/infiniband/hw/qedr/verbs.c | 6 ++++++
 include/uapi/rdma/qedr-abi.h       | 1 +
 4 files changed, 10 insertions(+)

diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
index 6a72095..d228718 100644
--- a/drivers/infiniband/hw/qedr/main.c
+++ b/drivers/infiniband/hw/qedr/main.c
@@ -778,6 +778,7 @@ static struct qedr_dev *qedr_add(struct qed_dev *cdev, struct pci_dev *pdev,
 	if (rc)
 		goto init_err;
 
+	dev->user_dpm_enabled = dev_info.user_dpm_enabled;
 	dev->num_hwfns = dev_info.common.num_hwfns;
 	dev->rdma_ctx = dev->ops->rdma_get_rdma_ctx(cdev);
 
diff --git a/drivers/infiniband/hw/qedr/qedr.h b/drivers/infiniband/hw/qedr/qedr.h
index aa08c76..42af9b6 100644
--- a/drivers/infiniband/hw/qedr/qedr.h
+++ b/drivers/infiniband/hw/qedr/qedr.h
@@ -158,6 +158,8 @@ struct qedr_dev {
 	struct qedr_qp		*gsi_qp;
 
 	unsigned long enet_state;
+
+	u8 user_dpm_enabled;
 };
 
 #define QEDR_MAX_SQ_PBL			(0x8000)
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index 17685cf..6a0acfa 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -335,6 +335,9 @@ static bool qedr_search_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
 	return found;
 }
 
+#define QEDR_LIB_UCXT_SUPPORT(field, udata, value)			      \
+	((offsetof(struct qedr_alloc_ucontext_resp, field) < udata->outlen) ? \
+	 (value) : 0)
 struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
 					struct ib_udata *udata)
 {
@@ -368,6 +371,9 @@ struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
 
 	memset(&uresp, 0, sizeof(uresp));
 
+	uresp.dpm_enabled = QEDR_LIB_UCXT_SUPPORT(dpm_enabled, udata,
+						  dev->user_dpm_enabled);
+
 	uresp.db_pa = ctx->dpi_phys_addr;
 	uresp.db_size = ctx->dpi_size;
 	uresp.max_send_wr = dev->attr.max_sqe;
diff --git a/include/uapi/rdma/qedr-abi.h b/include/uapi/rdma/qedr-abi.h
index 75c270d..2684004 100644
--- a/include/uapi/rdma/qedr-abi.h
+++ b/include/uapi/rdma/qedr-abi.h
@@ -49,6 +49,7 @@ struct qedr_alloc_ucontext_resp {
 	__u32 sges_per_recv_wr;
 	__u32 sges_per_srq_wr;
 	__u32 max_cqes;
+	__u8 dpm_enabled;
 };
 
 struct qedr_alloc_pd_ureq {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 1/2] RDMA/qedr: Notify user application if DPM is supported

[Jason Gunthorpe]

On Sun, Jun 18, 2017 at 04:01:10PM +0300, Ram Amrani wrote:
> Direct Packet Mode support may be disabled, e.g, due to limited BAR
> resources. Notifying the user application prevents wasting cycles
> on attempting to send these kind of packets.
> 
> Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>  drivers/infiniband/hw/qedr/main.c  | 1 +
>  drivers/infiniband/hw/qedr/qedr.h  | 2 ++
>  drivers/infiniband/hw/qedr/verbs.c | 6 ++++++
>  include/uapi/rdma/qedr-abi.h       | 1 +
>  4 files changed, 10 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
> index 6a72095..d228718 100644
> +++ b/drivers/infiniband/hw/qedr/main.c
> @@ -778,6 +778,7 @@ static struct qedr_dev *qedr_add(struct qed_dev *cdev, struct pci_dev *pdev,
>  	if (rc)
>  		goto init_err;
>  
> +	dev->user_dpm_enabled = dev_info.user_dpm_enabled;
>  	dev->num_hwfns = dev_info.common.num_hwfns;
>  	dev->rdma_ctx = dev->ops->rdma_get_rdma_ctx(cdev);
>  
> diff --git a/drivers/infiniband/hw/qedr/qedr.h b/drivers/infiniband/hw/qedr/qedr.h
> index aa08c76..42af9b6 100644
> +++ b/drivers/infiniband/hw/qedr/qedr.h
> @@ -158,6 +158,8 @@ struct qedr_dev {
>  	struct qedr_qp		*gsi_qp;
>  
>  	unsigned long enet_state;
> +
> +	u8 user_dpm_enabled;
>  };
>  
>  #define QEDR_MAX_SQ_PBL			(0x8000)
> diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
> index 17685cf..6a0acfa 100644
> +++ b/drivers/infiniband/hw/qedr/verbs.c
> @@ -335,6 +335,9 @@ static bool qedr_search_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
>  	return found;
>  }
>  
> +#define QEDR_LIB_UCXT_SUPPORT(field, udata, value)			      \
> +	((offsetof(struct qedr_alloc_ucontext_resp, field) < udata->outlen) ? \
> +	 (value) : 0)
>  struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
>  					struct ib_udata *udata)
>  {
> @@ -368,6 +371,9 @@ struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
>  
>  	memset(&uresp, 0, sizeof(uresp));
>  
> +	uresp.dpm_enabled = QEDR_LIB_UCXT_SUPPORT(dpm_enabled, udata,
> +						  dev->user_dpm_enabled);
> +
>  	uresp.db_pa = ctx->dpi_phys_addr;
>  	uresp.db_size = ctx->dpi_size;
>  	uresp.max_send_wr = dev->attr.max_sqe;
> diff --git a/include/uapi/rdma/qedr-abi.h b/include/uapi/rdma/qedr-abi.h
> index 75c270d..2684004 100644
> +++ b/include/uapi/rdma/qedr-abi.h
> @@ -49,6 +49,7 @@ struct qedr_alloc_ucontext_resp {
>  	__u32 sges_per_recv_wr;
>  	__u32 sges_per_srq_wr;
>  	__u32 max_cqes;
> +	__u8 dpm_enabled;
>  };


Um, how is uapi compatibility achieved here? I don't see any size
tests related to qedr_alloc_ucontext_resp:

struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
                                        struct ib_udata *udata)
{
        struct qedr_alloc_ucontext_resp uresp;
        rc = ib_copy_to_udata(udata, &uresp, sizeof(uresp));

Seems bad.

Same with the other patch.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH for-next 1/2] RDMA/qedr: Notify user application if DPM is supported

[Amrani, Ram]

> Um, how is uapi compatibility achieved here? I don't see any size
> tests related to qedr_alloc_ucontext_resp:
> 
> struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
>                                         struct ib_udata *udata)
> {
>         struct qedr_alloc_ucontext_resp uresp;
>         rc = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> 
> Seems bad.
> 
> Same with the other patch.
> 
> Jason

It does seem bad. Did you had in mind something like this:
         struct qedr_alloc_ucontext_resp uresp;
         size_t copy_size = min_t(size_t, sizeof(uresp), udata->outlen);

         rc = ib_copy_to_udata(udata, &uresp, copy_size);

If so, it makes sense to me to protect everybody's transactions.
I.e.:
	static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len)
	{
		size_t copy_size = min_t(size_t, sizeof(uresp), udata->outlen);
		return copy_to_user(udata->outbuf, src, copy_size) ? -EFAULT : 0;
	}

Likewise, a protection can be added for ib_copy_from_udata() too.

Let me know if I'm missing something. If not, I'll send a patch.

Thanks,
Ram

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 1/2] RDMA/qedr: Notify user application if DPM is supported

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]

On Tue, Jun 20, 2017 at 08:34:24AM +0000, Amrani, Ram wrote:
> > Um, how is uapi compatibility achieved here? I don't see any size
> > tests related to qedr_alloc_ucontext_resp:
> >
> > struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
> >                                         struct ib_udata *udata)
> > {
> >         struct qedr_alloc_ucontext_resp uresp;
> >         rc = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> >
> > Seems bad.
> >
> > Same with the other patch.
> >
> > Jason
>
> It does seem bad. Did you had in mind something like this:
>          struct qedr_alloc_ucontext_resp uresp;
>          size_t copy_size = min_t(size_t, sizeof(uresp), udata->outlen);
>
>          rc = ib_copy_to_udata(udata, &uresp, copy_size);
>
> If so, it makes sense to me to protect everybody's transactions.
> I.e.:
> 	static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len)
> 	{
> 		size_t copy_size = min_t(size_t, sizeof(uresp), udata->outlen);
> 		return copy_to_user(udata->outbuf, src, copy_size) ? -EFAULT : 0;
> 	}
>
> Likewise, a protection can be added for ib_copy_from_udata() too.

mlx4 and mlx5 don't need such protection, because they calculates the
response length and ensure that no extra data is copied.

Thanks

>
> Let me know if I'm missing something. If not, I'll send a patch.
>
> Thanks,
> Ram
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-next 1/2] RDMA/qedr: Notify user application if DPM is supported

[Jason Gunthorpe]

On Tue, Jun 20, 2017 at 01:41:04PM +0300, Leon Romanovsky wrote:
> On Tue, Jun 20, 2017 at 08:34:24AM +0000, Amrani, Ram wrote:
> > > Um, how is uapi compatibility achieved here? I don't see any size
> > > tests related to qedr_alloc_ucontext_resp:
> > >
> > > struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
> > >                                         struct ib_udata *udata)
> > > {
> > >         struct qedr_alloc_ucontext_resp uresp;
> > >         rc = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> > >
> > > Seems bad.
> > >
> > > Same with the other patch.
> > >
> > > Jason
> >
> > It does seem bad. Did you had in mind something like this:
> >          struct qedr_alloc_ucontext_resp uresp;
> >          size_t copy_size = min_t(size_t, sizeof(uresp), udata->outlen);
> >
> >          rc = ib_copy_to_udata(udata, &uresp, copy_size);
> >
> > If so, it makes sense to me to protect everybody's transactions.
> > I.e.:
> > 	static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len)
> > 	{
> > 		size_t copy_size = min_t(size_t, sizeof(uresp), udata->outlen);
> > 		return copy_to_user(udata->outbuf, src, copy_size) ? -EFAULT : 0;
> > 	}
> >
> > Likewise, a protection can be added for ib_copy_from_udata() too.
> 
> mlx4 and mlx5 don't need such protection, because they calculates the
> response length and ensure that no extra data is copied.

so you need to to whatever mlx4/5 do, and if we have some code
duplication then maybe a new ib_copy_to_udate_ex function is sensible.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH for-next 1/2] RDMA/qedr: Notify user application if DPM is supported

[Amrani, Ram]

 
> so you need to to whatever mlx4/5 do, and if we have some code
> duplication then maybe a new ib_copy_to_udate_ex function is sensible.
> 

OK

Ram

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH for-next 1/2] RDMA/qedr: Notify user application if DPM is supported

[Amrani, Ram]

> On Sun, Jun 18, 2017 at 04:01:10PM +0300, Ram Amrani wrote:
> > Direct Packet Mode support may be disabled, e.g, due to limited BAR
> > resources. Notifying the user application prevents wasting cycles
> > on attempting to send these kind of packets.
> >
> > Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> >  drivers/infiniband/hw/qedr/main.c  | 1 +
> >  drivers/infiniband/hw/qedr/qedr.h  | 2 ++
> >  drivers/infiniband/hw/qedr/verbs.c | 6 ++++++
> >  include/uapi/rdma/qedr-abi.h       | 1 +
> >  4 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
> > index 6a72095..d228718 100644
> > +++ b/drivers/infiniband/hw/qedr/main.c
> > @@ -778,6 +778,7 @@ static struct qedr_dev *qedr_add(struct qed_dev *cdev, struct pci_dev *pdev,
> >  	if (rc)
> >  		goto init_err;
> >
> > +	dev->user_dpm_enabled = dev_info.user_dpm_enabled;
> >  	dev->num_hwfns = dev_info.common.num_hwfns;
> >  	dev->rdma_ctx = dev->ops->rdma_get_rdma_ctx(cdev);
> >
> > diff --git a/drivers/infiniband/hw/qedr/qedr.h b/drivers/infiniband/hw/qedr/qedr.h
> > index aa08c76..42af9b6 100644
> > +++ b/drivers/infiniband/hw/qedr/qedr.h
> > @@ -158,6 +158,8 @@ struct qedr_dev {
> >  	struct qedr_qp		*gsi_qp;
> >
> >  	unsigned long enet_state;
> > +
> > +	u8 user_dpm_enabled;
> >  };
> >
> >  #define QEDR_MAX_SQ_PBL			(0x8000)
> > diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
> > index 17685cf..6a0acfa 100644
> > +++ b/drivers/infiniband/hw/qedr/verbs.c
> > @@ -335,6 +335,9 @@ static bool qedr_search_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
> >  	return found;
> >  }
> >
> > +#define QEDR_LIB_UCXT_SUPPORT(field, udata, value)			      \
> > +	((offsetof(struct qedr_alloc_ucontext_resp, field) < udata->outlen) ? \
> > +	 (value) : 0)
> >  struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
> >  					struct ib_udata *udata)
> >  {
> > @@ -368,6 +371,9 @@ struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
> >
> >  	memset(&uresp, 0, sizeof(uresp));
> >
> > +	uresp.dpm_enabled = QEDR_LIB_UCXT_SUPPORT(dpm_enabled, udata,
> > +						  dev->user_dpm_enabled);
> > +
> >  	uresp.db_pa = ctx->dpi_phys_addr;
> >  	uresp.db_size = ctx->dpi_size;
> >  	uresp.max_send_wr = dev->attr.max_sqe;
> > diff --git a/include/uapi/rdma/qedr-abi.h b/include/uapi/rdma/qedr-abi.h
> > index 75c270d..2684004 100644
> > +++ b/include/uapi/rdma/qedr-abi.h
> > @@ -49,6 +49,7 @@ struct qedr_alloc_ucontext_resp {
> >  	__u32 sges_per_recv_wr;
> >  	__u32 sges_per_srq_wr;
> >  	__u32 max_cqes;
> > +	__u8 dpm_enabled;
> >  };
> 
> 
> Um, how is uapi compatibility achieved here? I don't see any size
> tests related to qedr_alloc_ucontext_resp:
> 
> struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
>                                         struct ib_udata *udata)
> {
>         struct qedr_alloc_ucontext_resp uresp;
>         rc = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> 
> Seems bad.
> 
> Same with the other patch.
> 
> Jason

By the way, if it wasn't clear, the aspect part of the compatibility is
achieved via the QEDR_LIB_UCXT_SUPPORT macro above.

Ram

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 1/2] RDMA/qedr: Notify user application if DPM is supported

[Jason Gunthorpe]

On Thu, Jun 22, 2017 at 11:27:43AM +0000, Amrani, Ram wrote:
> > > +#define QEDR_LIB_UCXT_SUPPORT(field, udata, value)			      \
> > > +	((offsetof(struct qedr_alloc_ucontext_resp, field) < udata->outlen) ? \
> > > +	 (value) : 0)

> By the way, if it wasn't clear, the aspect part of the compatibility is
> achieved via the QEDR_LIB_UCXT_SUPPORT macro above.

It doesn't make any sense to do that, who cares what the value is in
the resp structure, you can't copy it, so userspace will never see
it. Do the copy properly and this macro goes away.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for-next 2/2] RDMA/qedr: Notify user application number of supported WIDs

[Ram Amrani]

The number of supported WIDs, if they are supported at all, can be
limited due to, e.g., BAR resources. Notifying the user space
application the number of available WIDs allows it to utilize them
correctly.

Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/qedr/verbs.c | 3 +++
 include/uapi/rdma/qedr-abi.h       | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index 6a0acfa..b1bcee0 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -373,6 +373,9 @@ struct ib_ucontext *qedr_alloc_ucontext(struct ib_device *ibdev,
 
 	uresp.dpm_enabled = QEDR_LIB_UCXT_SUPPORT(dpm_enabled, udata,
 						  dev->user_dpm_enabled);
+	uresp.wids_enabled = QEDR_LIB_UCXT_SUPPORT(wids_enabled, udata, 1);
+	uresp.wid_count = QEDR_LIB_UCXT_SUPPORT(wid_count, udata,
+						oparams.wid_count);
 
 	uresp.db_pa = ctx->dpi_phys_addr;
 	uresp.db_size = ctx->dpi_size;
diff --git a/include/uapi/rdma/qedr-abi.h b/include/uapi/rdma/qedr-abi.h
index 2684004..54b6435 100644
--- a/include/uapi/rdma/qedr-abi.h
+++ b/include/uapi/rdma/qedr-abi.h
@@ -50,6 +50,8 @@ struct qedr_alloc_ucontext_resp {
 	__u32 sges_per_srq_wr;
 	__u32 max_cqes;
 	__u8 dpm_enabled;
+	__u8 wids_enabled;
+	__u16 wid_count;
 };
 
 struct qedr_alloc_pd_ureq {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html