| Message ID | 1497790871-23945-2-git-send-email-Ram.Amrani@cavium.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@cavium.com> > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@cavium.com> > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 {
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@cavium.com> --- 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(+)