| Message ID | 20180426072251.14108-2-bharat@chelsio.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
On Thu, Apr 26, 2018 at 12:52:50PM +0530, Potnuri Bharat Teja wrote: > Adds iw_cxgb4 functionality to support RDMA_WRITE_WITH_IMMEDATE opcode. > > Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > drivers/infiniband/hw/cxgb4/cq.c | 23 +++++++++++++++++++--- > drivers/infiniband/hw/cxgb4/qp.c | 32 +++++++++++++++++++++++-------- > drivers/infiniband/hw/cxgb4/t4.h | 6 ++++++ > drivers/infiniband/hw/cxgb4/t4fw_ri_api.h | 8 +++++--- > include/uapi/rdma/cxgb4-abi.h | 3 ++- > 5 files changed, 57 insertions(+), 15 deletions(-) > > diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c > index cc95a9c270ae..1ccef76e2ae5 100644 > +++ b/drivers/infiniband/hw/cxgb4/cq.c > @@ -791,15 +791,32 @@ static int c4iw_poll_cq_one(struct c4iw_cq *chp, struct ib_wc *wc) > wc->byte_len = CQE_LEN(&cqe); > else > wc->byte_len = 0; > - wc->opcode = IB_WC_RECV; > - if (CQE_OPCODE(&cqe) == FW_RI_SEND_WITH_INV || > - CQE_OPCODE(&cqe) == FW_RI_SEND_WITH_SE_INV) { > + > + switch (CQE_OPCODE(&cqe)) { > + case FW_RI_SEND: > + wc->opcode = IB_WC_RECV; > + break; > + case FW_RI_SEND_WITH_INV: > + case FW_RI_SEND_WITH_SE_INV: > + wc->opcode = IB_WC_RECV; > wc->ex.invalidate_rkey = CQE_WRID_STAG(&cqe); > wc->wc_flags |= IB_WC_WITH_INVALIDATE; > c4iw_invalidate_mr(qhp->rhp, wc->ex.invalidate_rkey); > + break; > + case FW_RI_WRITE_IMMEDIATE: > + wc->opcode = IB_WC_RECV_RDMA_WITH_IMM; > + wc->ex.imm_data = CQE_IMM_DATA(&cqe); wc->ex.imm_data is a be32 While the struct here is: > + struct { > + __be32 mo; > + __be32 msn; > + __u64 imm_data; > + } imm_data_rcqe; Is that right? This seems like a great way to make an endianess oopsie, ie it probably doesn't work on BE. The struct should define imm_data as a be32 at the right offset. > enum { > - C4IW_QPF_ONCHIP = (1 << 0) > + C4IW_QPF_ONCHIP = (1 << 0), > + C4IW_QPF_WRITE_W_IMM = (1 << 1) Add the trailing , and don't neededlessly add white space (eg do not column align) 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 Thursday, May 05/17/18, 2018 at 00:37:37 +0530, Jason Gunthorpe wrote: > On Thu, Apr 26, 2018 at 12:52:50PM +0530, Potnuri Bharat Teja wrote: > > Adds iw_cxgb4 functionality to support RDMA_WRITE_WITH_IMMEDATE opcode. > > > > Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com> > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > drivers/infiniband/hw/cxgb4/cq.c | 23 +++++++++++++++++++--- > > drivers/infiniband/hw/cxgb4/qp.c | 32 +++++++++++++++++++++++-------- > > drivers/infiniband/hw/cxgb4/t4.h | 6 ++++++ > > drivers/infiniband/hw/cxgb4/t4fw_ri_api.h | 8 +++++--- > > include/uapi/rdma/cxgb4-abi.h | 3 ++- > > 5 files changed, 57 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c > > index cc95a9c270ae..1ccef76e2ae5 100644 > > +++ b/drivers/infiniband/hw/cxgb4/cq.c > > @@ -791,15 +791,32 @@ static int c4iw_poll_cq_one(struct c4iw_cq *chp, struct ib_wc *wc) > > wc->byte_len = CQE_LEN(&cqe); > > else > > wc->byte_len = 0; > > - wc->opcode = IB_WC_RECV; > > - if (CQE_OPCODE(&cqe) == FW_RI_SEND_WITH_INV || > > - CQE_OPCODE(&cqe) == FW_RI_SEND_WITH_SE_INV) { > > + > > + switch (CQE_OPCODE(&cqe)) { > > + case FW_RI_SEND: > > + wc->opcode = IB_WC_RECV; > > + break; > > + case FW_RI_SEND_WITH_INV: > > + case FW_RI_SEND_WITH_SE_INV: > > + wc->opcode = IB_WC_RECV; > > wc->ex.invalidate_rkey = CQE_WRID_STAG(&cqe); > > wc->wc_flags |= IB_WC_WITH_INVALIDATE; > > c4iw_invalidate_mr(qhp->rhp, wc->ex.invalidate_rkey); > > + break; > > + case FW_RI_WRITE_IMMEDIATE: > > + wc->opcode = IB_WC_RECV_RDMA_WITH_IMM; > > + wc->ex.imm_data = CQE_IMM_DATA(&cqe); > > wc->ex.imm_data is a be32 > > While the struct here is: > > > + struct { > > + __be32 mo; > > + __be32 msn; > > + __u64 imm_data; > > + } imm_data_rcqe; > > Is that right? > > This seems like a great way to make an endianess oopsie, ie it > probably doesn't work on BE. > > The struct should define imm_data as a be32 at the right offset. > Agreed. I beleive, immd_data field is not supposed to be BE. It is opaque to the driver and iwarp protocol, perhaps its made BE to help interop between BE and LE machines. Moreover iWARP protocol defines immediate data to be 64b. So that is why the WR has it as a __u64. Another concern here is, RDMA stack has it defined as __be32, I am not sure if it is an IB limitation. One possible way to fix this would be to change the __u64 to __be64 as iWARP spec recommends it to be 64. and the force cast it from ib WR to iw_cxgb4 WR. like this: enum fw_ri_mpa_attrs { @@ -546,7 +548,7 @@ struct fw_ri_rdma_write_wr { __u16 wrid; __u8 r1[3]; __u8 len16; - __be64 r2; + __be64 immd_data; ---- + if (wr->opcode == IB_WR_RDMA_WRITE_WITH_IMM) + wqe->write.immd_data = (__force __be64)wr->ex.imm_data; + else ---- + case FW_RI_WRITE_IMMEDIATE: + wc->opcode = IB_WC_RECV_RDMA_WITH_IMM; + wc->ex.imm_data = (__force __be32)CQE_IMM_DATA(&cqe); + wc->wc_flags |= IB_WC_WITH_IMM; > > enum { > > - C4IW_QPF_ONCHIP = (1 << 0) > > + C4IW_QPF_ONCHIP = (1 << 0), > > + C4IW_QPF_WRITE_W_IMM = (1 << 1) > > Add the trailing , and don't neededlessly add white space (eg do not > column align) I'll fix it. Thanks for the Review! Bharat > > 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 Thu, May 17, 2018 at 06:52:57PM +0530, Potnuri Bharat Teja wrote: > > > + case FW_RI_WRITE_IMMEDIATE: > > > + wc->opcode = IB_WC_RECV_RDMA_WITH_IMM; > > > + wc->ex.imm_data = CQE_IMM_DATA(&cqe); > > > > wc->ex.imm_data is a be32 > > > > While the struct here is: > > > > > + struct { > > > + __be32 mo; > > > + __be32 msn; > > > + __u64 imm_data; > > > + } imm_data_rcqe; > > > > Is that right? > > > > This seems like a great way to make an endianess oopsie, ie it > > probably doesn't work on BE. > > > > The struct should define imm_data as a be32 at the right offset. > > > Agreed. > I beleive, immd_data field is not supposed to be BE. It is opaque to the > driver and iwarp protocol, perhaps its made BE to help interop > between BE It is made BE just to make sparse more useful - really the rule is 'the value is not in cpu-endian' and standard convention is to use it as a BE value, technically LE would be OK as well. > and LE machines. Moreover iWARP protocol defines immediate data to be 64b. > So that is why the WR has it as a __u64. Ah, that causes troubles then :) > Another concern here is, RDMA stack has it defined as __be32, I am not sure if > it is an IB limitation. It is. You need to define how interop works, exactly which 32 bits on the wire of the 64 bit imm_data will be represented by the 32 bit IB WR/WC? > One possible way to fix this would be to change the __u64 to __be64 as iWARP > spec recommends it to be 64. and the force cast it from ib WR to iw_cxgb4 WR. No, that isn't good enough, it still will get the wrong bytes during truncation to 32 bits. You need to define it as union iwarp_imm_data { struct { u32 reserved; be32 ib_imm_data32; }; be64 imm_data64; } And probably in some common header with a nice comment since all iwarp drivers will need this. Assuming you decide that the last 4 bytes of the imm_data should be used as the 32 bits for IB (this is consistent with a BE representation) 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/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c index cc95a9c270ae..1ccef76e2ae5 100644 --- a/drivers/infiniband/hw/cxgb4/cq.c +++ b/drivers/infiniband/hw/cxgb4/cq.c @@ -791,15 +791,32 @@ static int c4iw_poll_cq_one(struct c4iw_cq *chp, struct ib_wc *wc) wc->byte_len = CQE_LEN(&cqe); else wc->byte_len = 0; - wc->opcode = IB_WC_RECV; - if (CQE_OPCODE(&cqe) == FW_RI_SEND_WITH_INV || - CQE_OPCODE(&cqe) == FW_RI_SEND_WITH_SE_INV) { + + switch (CQE_OPCODE(&cqe)) { + case FW_RI_SEND: + wc->opcode = IB_WC_RECV; + break; + case FW_RI_SEND_WITH_INV: + case FW_RI_SEND_WITH_SE_INV: + wc->opcode = IB_WC_RECV; wc->ex.invalidate_rkey = CQE_WRID_STAG(&cqe); wc->wc_flags |= IB_WC_WITH_INVALIDATE; c4iw_invalidate_mr(qhp->rhp, wc->ex.invalidate_rkey); + break; + case FW_RI_WRITE_IMMEDIATE: + wc->opcode = IB_WC_RECV_RDMA_WITH_IMM; + wc->ex.imm_data = CQE_IMM_DATA(&cqe); + wc->wc_flags |= IB_WC_WITH_IMM; + break; + default: + pr_err("Unexpected opcode %d in the CQE received for QPID=0x%0x\n", + CQE_OPCODE(&cqe), CQE_QPID(&cqe)); + ret = -EINVAL; + goto out; } } else { switch (CQE_OPCODE(&cqe)) { + case FW_RI_WRITE_IMMEDIATE: case FW_RI_RDMA_WRITE: wc->opcode = IB_WC_RDMA_WRITE; break; diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c index 42defb81b23d..dbfa450771fc 100644 --- a/drivers/infiniband/hw/cxgb4/qp.c +++ b/drivers/infiniband/hw/cxgb4/qp.c @@ -556,7 +556,10 @@ static int build_rdma_write(struct t4_sq *sq, union t4_wr *wqe, if (wr->num_sge > T4_MAX_SEND_SGE) return -EINVAL; - wqe->write.r2 = 0; + if (wr->opcode == IB_WR_RDMA_WRITE_WITH_IMM) + wqe->write.immd_data = wr->ex.imm_data; + else + wqe->write.immd_data = 0; wqe->write.stag_sink = cpu_to_be32(rdma_wr(wr)->rkey); wqe->write.to_sink = cpu_to_be64(rdma_wr(wr)->remote_addr); if (wr->num_sge) { @@ -847,6 +850,9 @@ static int ib_to_fw_opcode(int ib_opcode) case IB_WR_RDMA_WRITE: opcode = FW_RI_RDMA_WRITE; break; + case IB_WR_RDMA_WRITE_WITH_IMM: + opcode = FW_RI_WRITE_IMMEDIATE; + break; case IB_WR_RDMA_READ: case IB_WR_RDMA_READ_WITH_INV: opcode = FW_RI_READ_REQ; @@ -965,6 +971,7 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, enum fw_wr_opcodes fw_opcode = 0; enum fw_ri_wr_flags fw_flags; struct c4iw_qp *qhp; + struct c4iw_dev *rhp; union t4_wr *wqe = NULL; u32 num_wrs; struct t4_swsqe *swsqe; @@ -972,6 +979,7 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, u16 idx = 0; qhp = to_c4iw_qp(ibqp); + rhp = qhp->rhp; spin_lock_irqsave(&qhp->lock, flag); /* @@ -1016,6 +1024,13 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, swsqe->opcode = FW_RI_SEND_WITH_INV; err = build_rdma_send(&qhp->wq.sq, wqe, wr, &len16); break; + case IB_WR_RDMA_WRITE_WITH_IMM: + if (unlikely(!rhp->rdev.lldi.write_w_imm_support)) { + err = -EINVAL; + break; + } + fw_flags |= FW_RI_RDMA_WRITE_WITH_IMMEDIATE; + /*FALLTHROUGH*/ case IB_WR_RDMA_WRITE: fw_opcode = FW_RI_RDMA_WRITE_WR; swsqe->opcode = FW_RI_RDMA_WRITE; @@ -1026,8 +1041,7 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, fw_opcode = FW_RI_RDMA_READ_WR; swsqe->opcode = FW_RI_READ_REQ; if (wr->opcode == IB_WR_RDMA_READ_WITH_INV) { - c4iw_invalidate_mr(qhp->rhp, - wr->sg_list[0].lkey); + c4iw_invalidate_mr(rhp, wr->sg_list[0].lkey); fw_flags = FW_RI_RDMA_READ_INVALIDATE; } else { fw_flags = 0; @@ -1043,7 +1057,7 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, struct c4iw_mr *mhp = to_c4iw_mr(reg_wr(wr)->mr); swsqe->opcode = FW_RI_FAST_REGISTER; - if (qhp->rhp->rdev.lldi.fr_nsmr_tpte_wr_support && + if (rhp->rdev.lldi.fr_nsmr_tpte_wr_support && !mhp->attr.state && mhp->mpl_len <= 2) { fw_opcode = FW_RI_FR_NSMR_TPTE_WR; build_tpte_memreg(&wqe->fr_tpte, reg_wr(wr), @@ -1052,7 +1066,7 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, fw_opcode = FW_RI_FR_NSMR_WR; err = build_memreg(&qhp->wq.sq, wqe, reg_wr(wr), mhp, &len16, - qhp->rhp->rdev.lldi.ulptx_memwrite_dsgl); + rhp->rdev.lldi.ulptx_memwrite_dsgl); if (err) break; } @@ -1065,7 +1079,7 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, fw_opcode = FW_RI_INV_LSTAG_WR; swsqe->opcode = FW_RI_LOCAL_INV; err = build_inv_stag(wqe, wr, &len16); - c4iw_invalidate_mr(qhp->rhp, wr->ex.invalidate_rkey); + c4iw_invalidate_mr(rhp, wr->ex.invalidate_rkey); break; default: pr_warn("%s post of type=%d TBD!\n", __func__, @@ -1084,7 +1098,7 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, swsqe->wr_id = wr->wr_id; if (c4iw_wr_log) { swsqe->sge_ts = cxgb4_read_sge_timestamp( - qhp->rhp->rdev.lldi.ports[0]); + rhp->rdev.lldi.ports[0]); swsqe->host_time = ktime_get(); } @@ -1098,7 +1112,7 @@ int c4iw_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, t4_sq_produce(&qhp->wq, len16); idx += DIV_ROUND_UP(len16*16, T4_EQ_ENTRY_SIZE); } - if (!qhp->rhp->rdev.status_page->db_off) { + if (!rhp->rdev.status_page->db_off) { t4_ring_sq_db(&qhp->wq, idx, wqe); spin_unlock_irqrestore(&qhp->lock, flag); } else { @@ -2095,6 +2109,8 @@ struct ib_qp *c4iw_create_qp(struct ib_pd *pd, struct ib_qp_init_attr *attrs, uresp.flags = C4IW_QPF_ONCHIP; } else uresp.flags = 0; + if (rhp->rdev.lldi.write_w_imm_support) + uresp.flags |= C4IW_QPF_WRITE_W_IMM; uresp.qid_mask = rhp->rdev.qpmask; uresp.sqid = qhp->wq.sq.qid; uresp.sq_size = qhp->wq.sq.size; diff --git a/drivers/infiniband/hw/cxgb4/t4.h b/drivers/infiniband/hw/cxgb4/t4.h index e62fb7d164ee..5fff9a685ca8 100644 --- a/drivers/infiniband/hw/cxgb4/t4.h +++ b/drivers/infiniband/hw/cxgb4/t4.h @@ -190,6 +190,11 @@ struct t4_cqe { __be32 abs_rqe_idx; } srcqe; u64 drain_cookie; + struct { + __be32 mo; + __be32 msn; + __u64 imm_data; + } imm_data_rcqe; } u; __be64 reserved[4]; __be64 bits_type_ts; @@ -248,6 +253,7 @@ struct t4_cqe { #define CQE_WRID_STAG(x) (be32_to_cpu((x)->u.rcqe.stag)) #define CQE_WRID_MSN(x) (be32_to_cpu((x)->u.rcqe.msn)) #define CQE_ABS_RQE_IDX(x) (be32_to_cpu((x)->u.srcqe.abs_rqe_idx)) +#define CQE_IMM_DATA(x) ((x)->u.imm_data_rcqe.imm_data) /* used for SQ completion processing */ #define CQE_WRID_SQ_IDX(x) ((x)->u.scqe.cidx) diff --git a/drivers/infiniband/hw/cxgb4/t4fw_ri_api.h b/drivers/infiniband/hw/cxgb4/t4fw_ri_api.h index 0f4f86b004d6..48c187690921 100644 --- a/drivers/infiniband/hw/cxgb4/t4fw_ri_api.h +++ b/drivers/infiniband/hw/cxgb4/t4fw_ri_api.h @@ -50,7 +50,8 @@ enum fw_ri_wr_opcode { FW_RI_BYPASS = 0xd, FW_RI_RECEIVE = 0xe, - FW_RI_SGE_EC_CR_RETURN = 0xf + FW_RI_SGE_EC_CR_RETURN = 0xf, + FW_RI_WRITE_IMMEDIATE = FW_RI_RDMA_INIT }; enum fw_ri_wr_flags { @@ -59,7 +60,8 @@ enum fw_ri_wr_flags { FW_RI_SOLICITED_EVENT_FLAG = 0x04, FW_RI_READ_FENCE_FLAG = 0x08, FW_RI_LOCAL_FENCE_FLAG = 0x10, - FW_RI_RDMA_READ_INVALIDATE = 0x20 + FW_RI_RDMA_READ_INVALIDATE = 0x20, + FW_RI_RDMA_WRITE_WITH_IMMEDIATE = 0x40 }; enum fw_ri_mpa_attrs { @@ -546,7 +548,7 @@ struct fw_ri_rdma_write_wr { __u16 wrid; __u8 r1[3]; __u8 len16; - __be64 r2; + __u64 immd_data; __be32 plen; __be32 stag_sink; __be64 to_sink; diff --git a/include/uapi/rdma/cxgb4-abi.h b/include/uapi/rdma/cxgb4-abi.h index 7b1aa3f3fae3..6ec7ceadbd88 100644 --- a/include/uapi/rdma/cxgb4-abi.h +++ b/include/uapi/rdma/cxgb4-abi.h @@ -65,7 +65,8 @@ struct c4iw_create_cq_resp { }; enum { - C4IW_QPF_ONCHIP = (1 << 0) + C4IW_QPF_ONCHIP = (1 << 0), + C4IW_QPF_WRITE_W_IMM = (1 << 1) }; struct c4iw_create_qp_resp {