| Message ID | 1520257985-197864-3-git-send-email-liuyixian@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Leon Romanovsky |
| Headers | show |
On Mon, Mar 05, 2018 at 09:53:05PM +0800, Yixian Liu wrote: > diff --git a/providers/hns/hns_roce_u_abi.h b/providers/hns/hns_roce_u_abi.h > index ec145bb..c4f36ec 100644 > +++ b/providers/hns/hns_roce_u_abi.h > @@ -57,6 +57,7 @@ struct hns_roce_create_cq_resp { > struct ib_uverbs_create_cq_resp ibv_resp; > __u32 cqn; > __u32 reserved; > + __u64 cap_flags; > }; This struct is wrong.. the kernel does this: if (context) { if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && (udata->outlen == sizeof(resp))) { hr_cq->db_en = 1; resp.cqn = hr_cq->cqn; resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB; ret = ib_copy_to_udata(udata, &resp, sizeof(resp)); } else ret = ib_copy_to_udata(udata, &hr_cq->cqn, sizeof(u64)); So 'cqn' should be a u64, it is a mistake in the userspace to have used a u32 here. You could correct it, but it would need to be used as u32 consistently. Instead I recommend changing the above struct (in user and kernel) to simply: struct hns_roce_ib_create_cq_resp { __u64 cqn; /* Only 32 bits used, 64 for compat */ __u64 cap_flags; }; And change the kernel to simply do: if (context) { resp.cqn = hr_cq->cqn; if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && (udata->outlen == sizeof(resp))) { hr_cq->db_en = 1; resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB; } ret = ib_copy_to_udata(udata, &resp, sizeof(resp)); ib_copy_to_udata() will do the right thing. Please send a patch for this immediately before this gets into a released kernel.. Also all these tests: if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && (udata->outlen == sizeof(resp))) Should probably be: if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && (udata->outlen >= sizeof(resp))) Arguably a flag should have been added to the request for this new behavior though, as is will limit what you can do in future. 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
On 2018/3/15 4:30, Jason Gunthorpe wrote: > On Mon, Mar 05, 2018 at 09:53:05PM +0800, Yixian Liu wrote: >> diff --git a/providers/hns/hns_roce_u_abi.h b/providers/hns/hns_roce_u_abi.h >> index ec145bb..c4f36ec 100644 >> +++ b/providers/hns/hns_roce_u_abi.h >> @@ -57,6 +57,7 @@ struct hns_roce_create_cq_resp { >> struct ib_uverbs_create_cq_resp ibv_resp; >> __u32 cqn; >> __u32 reserved; >> + __u64 cap_flags; >> }; > > This struct is wrong.. the kernel does this: > > if (context) { > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && > (udata->outlen == sizeof(resp))) { > hr_cq->db_en = 1; > resp.cqn = hr_cq->cqn; > resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB; > ret = ib_copy_to_udata(udata, &resp, sizeof(resp)); > } else > ret = ib_copy_to_udata(udata, &hr_cq->cqn, sizeof(u64)); > > So 'cqn' should be a u64, it is a mistake in the userspace to have > used a u32 here. > > You could correct it, but it would need to be used as u32 > consistently. > > Instead I recommend changing the above struct (in user and kernel) to > simply: > > struct hns_roce_ib_create_cq_resp { > __u64 cqn; /* Only 32 bits used, 64 for compat */ > __u64 cap_flags; > }; > > And change the kernel to simply do: > > if (context) { > resp.cqn = hr_cq->cqn; > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && > (udata->outlen == sizeof(resp))) { > hr_cq->db_en = 1; > resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB; > } > ret = ib_copy_to_udata(udata, &resp, sizeof(resp)); > > ib_copy_to_udata() will do the right thing. > > Please send a patch for this immediately before this gets into a > released kernel.. > > Also all these tests: > > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && > (udata->outlen == sizeof(resp))) > > Should probably be: > > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && > (udata->outlen >= sizeof(resp))) > > Arguably a flag should have been added to the request for this new > behavior though, as is will limit what you can do in future. > > Jason Thank you very much! I will send the patch sets for kernel and userspace today. -- 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, Mar 15, 2018 at 11:02:35AM +0800, Liuyixian (Eason) wrote: > > > On 2018/3/15 4:30, Jason Gunthorpe wrote: > > On Mon, Mar 05, 2018 at 09:53:05PM +0800, Yixian Liu wrote: > >> diff --git a/providers/hns/hns_roce_u_abi.h b/providers/hns/hns_roce_u_abi.h > >> index ec145bb..c4f36ec 100644 > >> +++ b/providers/hns/hns_roce_u_abi.h > >> @@ -57,6 +57,7 @@ struct hns_roce_create_cq_resp { > >> struct ib_uverbs_create_cq_resp ibv_resp; > >> __u32 cqn; > >> __u32 reserved; > >> + __u64 cap_flags; > >> }; > > > > This struct is wrong.. the kernel does this: > > > > if (context) { > > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && > > (udata->outlen == sizeof(resp))) { > > hr_cq->db_en = 1; > > resp.cqn = hr_cq->cqn; > > resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB; > > ret = ib_copy_to_udata(udata, &resp, sizeof(resp)); > > } else > > ret = ib_copy_to_udata(udata, &hr_cq->cqn, sizeof(u64)); > > > > So 'cqn' should be a u64, it is a mistake in the userspace to have > > used a u32 here. > > > > You could correct it, but it would need to be used as u32 > > consistently. > > > > Instead I recommend changing the above struct (in user and kernel) to > > simply: > > > > struct hns_roce_ib_create_cq_resp { > > __u64 cqn; /* Only 32 bits used, 64 for compat */ > > __u64 cap_flags; > > }; > > > > And change the kernel to simply do: > > > > if (context) { > > resp.cqn = hr_cq->cqn; > > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && > > (udata->outlen == sizeof(resp))) { > > hr_cq->db_en = 1; > > resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB; > > } > > ret = ib_copy_to_udata(udata, &resp, sizeof(resp)); > > > > ib_copy_to_udata() will do the right thing. > > > > Please send a patch for this immediately before this gets into a > > released kernel.. > > > > Also all these tests: > > > > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && > > (udata->outlen == sizeof(resp))) > > > > Should probably be: > > > > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && > > (udata->outlen >= sizeof(resp))) > > > > Arguably a flag should have been added to the request for this new > > behavior though, as is will limit what you can do in future. > > > > Jason > > Thank you very much! I will send the patch sets for kernel and userspace today. Also make sure resp is zeroed in the kernel... 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
On 2018/3/15 11:30, Jason Gunthorpe wrote: > On Thu, Mar 15, 2018 at 11:02:35AM +0800, Liuyixian (Eason) wrote: >> >> >> On 2018/3/15 4:30, Jason Gunthorpe wrote: >>> On Mon, Mar 05, 2018 at 09:53:05PM +0800, Yixian Liu wrote: >>>> diff --git a/providers/hns/hns_roce_u_abi.h b/providers/hns/hns_roce_u_abi.h >>>> index ec145bb..c4f36ec 100644 >>>> +++ b/providers/hns/hns_roce_u_abi.h >>>> @@ -57,6 +57,7 @@ struct hns_roce_create_cq_resp { >>>> struct ib_uverbs_create_cq_resp ibv_resp; >>>> __u32 cqn; >>>> __u32 reserved; >>>> + __u64 cap_flags; >>>> }; >>> >>> This struct is wrong.. the kernel does this: >>> >>> if (context) { >>> if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && >>> (udata->outlen == sizeof(resp))) { >>> hr_cq->db_en = 1; >>> resp.cqn = hr_cq->cqn; >>> resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB; >>> ret = ib_copy_to_udata(udata, &resp, sizeof(resp)); >>> } else >>> ret = ib_copy_to_udata(udata, &hr_cq->cqn, sizeof(u64)); >>> >>> So 'cqn' should be a u64, it is a mistake in the userspace to have >>> used a u32 here. >>> >>> You could correct it, but it would need to be used as u32 >>> consistently. >>> >>> Instead I recommend changing the above struct (in user and kernel) to >>> simply: >>> >>> struct hns_roce_ib_create_cq_resp { >>> __u64 cqn; /* Only 32 bits used, 64 for compat */ >>> __u64 cap_flags; >>> }; >>> >>> And change the kernel to simply do: >>> >>> if (context) { >>> resp.cqn = hr_cq->cqn; >>> if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && >>> (udata->outlen == sizeof(resp))) { >>> hr_cq->db_en = 1; >>> resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB; >>> } >>> ret = ib_copy_to_udata(udata, &resp, sizeof(resp)); >>> >>> ib_copy_to_udata() will do the right thing. >>> >>> Please send a patch for this immediately before this gets into a >>> released kernel.. >>> >>> Also all these tests: >>> >>> if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && >>> (udata->outlen == sizeof(resp))) >>> >>> Should probably be: >>> >>> if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && >>> (udata->outlen >= sizeof(resp))) >>> >>> Arguably a flag should have been added to the request for this new >>> behavior though, as is will limit what you can do in future. >>> >>> Jason >> >> Thank you very much! I will send the patch sets for kernel and userspace today. > > Also make sure resp is zeroed in the kernel... > > Jason Okay, I will. -- 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/providers/hns/hns_roce_u.h b/providers/hns/hns_roce_u.h index 4404c85..781b36b 100644 --- a/providers/hns/hns_roce_u.h +++ b/providers/hns/hns_roce_u.h @@ -150,6 +150,7 @@ struct hns_roce_cq { unsigned int *set_ci_db; unsigned int *arm_db; int arm_sn; + unsigned long flags; }; struct hns_roce_srq { diff --git a/providers/hns/hns_roce_u_abi.h b/providers/hns/hns_roce_u_abi.h index ec145bb..c4f36ec 100644 --- a/providers/hns/hns_roce_u_abi.h +++ b/providers/hns/hns_roce_u_abi.h @@ -57,6 +57,7 @@ struct hns_roce_create_cq_resp { struct ib_uverbs_create_cq_resp ibv_resp; __u32 cqn; __u32 reserved; + __u64 cap_flags; }; struct hns_roce_create_qp { diff --git a/providers/hns/hns_roce_u_hw_v2.c b/providers/hns/hns_roce_u_hw_v2.c index b145b17..0889d0b 100644 --- a/providers/hns/hns_roce_u_hw_v2.c +++ b/providers/hns/hns_roce_u_hw_v2.c @@ -476,7 +476,11 @@ static int hns_roce_u_v2_poll_cq(struct ibv_cq *ibvcq, int ne, if (npolled) { mmio_ordered_writes_hack(); - hns_roce_v2_update_cq_cons_index(ctx, cq); + if (cq->flags & HNS_ROCE_SUPPORT_CQ_RECORD_DB) + *cq->set_ci_db = (unsigned int)(cq->cons_index & + ((cq->cq_depth << 1) - 1)); + else + hns_roce_v2_update_cq_cons_index(ctx, cq); } pthread_spin_unlock(&cq->lock); diff --git a/providers/hns/hns_roce_u_hw_v2.h b/providers/hns/hns_roce_u_hw_v2.h index 15ac0ca..84a7726 100644 --- a/providers/hns/hns_roce_u_hw_v2.h +++ b/providers/hns/hns_roce_u_hw_v2.h @@ -44,6 +44,10 @@ enum { HNS_ROCE_SUPPORT_RQ_RECORD_DB = 1 << 0, }; +enum { + HNS_ROCE_SUPPORT_CQ_RECORD_DB = 1 << 0, +}; + /* V2 REG DEFINITION */ #define ROCEE_VF_DB_CFG0_OFFSET 0x0230 diff --git a/providers/hns/hns_roce_u_verbs.c b/providers/hns/hns_roce_u_verbs.c index 62097a1..24dbbce 100644 --- a/providers/hns/hns_roce_u_verbs.c +++ b/providers/hns/hns_roce_u_verbs.c @@ -263,8 +263,8 @@ struct ibv_cq *hns_roce_u_create_cq(struct ibv_context *context, int cqe, struct ibv_comp_channel *channel, int comp_vector) { - struct hns_roce_create_cq cmd; - struct hns_roce_create_cq_resp resp; + struct hns_roce_create_cq cmd = {}; + struct hns_roce_create_cq_resp resp = {}; struct hns_roce_cq *cq; int ret; @@ -290,6 +290,15 @@ struct ibv_cq *hns_roce_u_create_cq(struct ibv_context *context, int cqe, cmd.buf_addr = (uintptr_t) cq->buf.buf; + if (to_hr_dev(context->device)->hw_version != HNS_ROCE_HW_VER1) { + cq->set_ci_db = hns_roce_alloc_db(to_hr_ctx(context), + HNS_ROCE_CQ_TYPE_DB); + if (!cq->set_ci_db) + goto err_buf; + + cmd.db_addr = (uintptr_t) cq->set_ci_db; + } + ret = ibv_cmd_create_cq(context, cqe, channel, comp_vector, &cq->ibv_cq, &cmd.ibv_cmd, sizeof(cmd), &resp.ibv_resp, sizeof(resp)); @@ -298,12 +307,10 @@ struct ibv_cq *hns_roce_u_create_cq(struct ibv_context *context, int cqe, cq->cqn = resp.cqn; cq->cq_depth = cqe; + cq->flags = resp.cap_flags; if (to_hr_dev(context->device)->hw_version == HNS_ROCE_HW_VER1) cq->set_ci_db = to_hr_ctx(context)->cq_tptr_base + cq->cqn * 2; - else - cq->set_ci_db = to_hr_ctx(context)->uar + - ROCEE_VF_DB_CFG0_OFFSET; cq->arm_db = cq->set_ci_db; cq->arm_sn = 1; @@ -313,6 +320,11 @@ struct ibv_cq *hns_roce_u_create_cq(struct ibv_context *context, int cqe, return &cq->ibv_cq; err_db: + if (to_hr_dev(context->device)->hw_version != HNS_ROCE_HW_VER1) + hns_roce_free_db(to_hr_ctx(context), cq->set_ci_db, + HNS_ROCE_CQ_TYPE_DB); + +err_buf: hns_roce_free_buf(&cq->buf); err: @@ -334,6 +346,9 @@ int hns_roce_u_destroy_cq(struct ibv_cq *cq) if (ret) return ret; + if (to_hr_dev(cq->context->device)->hw_version != HNS_ROCE_HW_VER1) + hns_roce_free_db(to_hr_ctx(cq->context), + to_hr_cq(cq)->set_ci_db, HNS_ROCE_CQ_TYPE_DB); hns_roce_free_buf(&to_hr_cq(cq)->buf); free(to_hr_cq(cq));