On Thu, Dec 06, 2018 at 04:53:53PM +0800, liyangyang (M) wrote:
> Hi, Leon:
>
> Thanks a lot for your reply.
>
> On 2018/12/5 22:43, Leon Romanovsky wrote:
> > On Wed, Dec 05, 2018 at 05:41:42PM +0800, Lijun Ou wrote:
> >> From: Yangyang Li <liyangyang20@xxxxxxxxxx>
> >>
> >> This patch adds SCC context allocation and initialization support
> >> for DCQCN in kernel space driver.
> >>
> >> Signed-off-by: Yangyang Li <liyangyang20@xxxxxxxxxx>
> >> ---
> >> drivers/infiniband/hw/hns/hns_roce_cmd.h | 4 ++++
> >> drivers/infiniband/hw/hns/hns_roce_device.h | 6 +++++
> >> drivers/infiniband/hw/hns/hns_roce_hem.c | 26 ++++++++++++++++++---
> >> drivers/infiniband/hw/hns/hns_roce_hem.h | 1 +
> >> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 35 ++++++++++++++++++++++++++++-
> >> drivers/infiniband/hw/hns/hns_roce_hw_v2.h | 23 +++++++++++++++++--
> >> drivers/infiniband/hw/hns/hns_roce_main.c | 18 +++++++++++++++
> >> drivers/infiniband/hw/hns/hns_roce_qp.c | 20 ++++++++++++++++-
> >> 8 files changed, 126 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.h b/drivers/infiniband/hw/hns/hns_roce_cmd.h
> >> index 927701d..5348282 100644
> >> --- a/drivers/infiniband/hw/hns/hns_roce_cmd.h
> >> +++ b/drivers/infiniband/hw/hns/hns_roce_cmd.h
> >> @@ -89,6 +89,10 @@ enum {
> >> HNS_ROCE_CMD_DESTROY_SRQC_BT1 = 0x39,
> >> HNS_ROCE_CMD_DESTROY_SRQC_BT2 = 0x3a,
> >>
> >> + /* SCC CTX BT commands */
> >> + HNS_ROCE_CMD_READ_SCC_CTX_BT0 = 0xa4,
> >> + HNS_ROCE_CMD_WRITE_SCC_CTX_BT0 = 0xa5,
> >> +
> >> /* EQC commands */
> >> HNS_ROCE_CMD_CREATE_AEQC = 0x80,
> >> HNS_ROCE_CMD_MODIFY_AEQC = 0x81,
> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
> >> index 779dd4c4..718b415 100644
> >> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> >> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> >> @@ -482,6 +482,7 @@ struct hns_roce_qp_table {
> >> struct hns_roce_hem_table qp_table;
> >> struct hns_roce_hem_table irrl_table;
> >> struct hns_roce_hem_table trrl_table;
> >> + struct hns_roce_hem_table scc_ctx_table;
> >> };
> >>
> >> struct hns_roce_cq_table {
> >> @@ -768,6 +769,7 @@ struct hns_roce_caps {
> >> int irrl_entry_sz;
> >> int trrl_entry_sz;
> >> int cqc_entry_sz;
> >> + int scc_ctx_entry_sz;
> >> int srqc_entry_sz;
> >> int idx_entry_sz;
> >> u32 pbl_ba_pg_sz;
> >> @@ -780,6 +782,7 @@ struct hns_roce_caps {
> >> u32 srqc_bt_num;
> >> u32 cqc_bt_num;
> >> u32 mpt_bt_num;
> >> + u32 scc_ctx_bt_num;
> >> u32 qpc_ba_pg_sz;
> >> u32 qpc_buf_pg_sz;
> >> u32 qpc_hop_num;
> >> @@ -795,6 +798,9 @@ struct hns_roce_caps {
> >> u32 mtt_ba_pg_sz;
> >> u32 mtt_buf_pg_sz;
> >> u32 mtt_hop_num;
> >> + u32 scc_ctx_ba_pg_sz;
> >> + u32 scc_ctx_buf_pg_sz;
> >> + u32 scc_ctx_hop_num;
> >> u32 cqe_ba_pg_sz;
> >> u32 cqe_buf_pg_sz;
> >> u32 cqe_hop_num;
> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.c b/drivers/infiniband/hw/hns/hns_roce_hem.c
> >> index 4cdbcaf..9e4a2ba 100644
> >> --- a/drivers/infiniband/hw/hns/hns_roce_hem.c
> >> +++ b/drivers/infiniband/hw/hns/hns_roce_hem.c
> >> @@ -45,6 +45,7 @@ bool hns_roce_check_whether_mhop(struct hns_roce_dev *hr_dev, u32 type)
> >> (hr_dev->caps.mpt_hop_num && type == HEM_TYPE_MTPT) ||
> >> (hr_dev->caps.cqc_hop_num && type == HEM_TYPE_CQC) ||
> >> (hr_dev->caps.srqc_hop_num && type == HEM_TYPE_SRQC) ||
> >> + (hr_dev->caps.scc_ctx_hop_num && type == HEM_TYPE_SCC_CTX) ||
> >> (hr_dev->caps.cqe_hop_num && type == HEM_TYPE_CQE) ||
> >> (hr_dev->caps.mtt_hop_num && type == HEM_TYPE_MTT) ||
> >> (hr_dev->caps.srqwqe_hop_num && type == HEM_TYPE_SRQWQE) ||
> >> @@ -125,6 +126,14 @@ int hns_roce_calc_hem_mhop(struct hns_roce_dev *hr_dev,
> >> mhop->ba_l0_num = hr_dev->caps.cqc_bt_num;
> >> mhop->hop_num = hr_dev->caps.cqc_hop_num;
> >> break;
> >> + case HEM_TYPE_SCC_CTX:
> >> + mhop->buf_chunk_size = 1 << (hr_dev->caps.scc_ctx_buf_pg_sz
> >> + + PAGE_SHIFT);
> >> + mhop->bt_chunk_size = 1 << (hr_dev->caps.scc_ctx_ba_pg_sz
> >> + + PAGE_SHIFT);
> >> + mhop->ba_l0_num = hr_dev->caps.scc_ctx_bt_num;
> >> + mhop->hop_num = hr_dev->caps.scc_ctx_hop_num;
> >> + break;
> >> case HEM_TYPE_SRQC:
> >> mhop->buf_chunk_size = 1 << (hr_dev->caps.srqc_buf_pg_sz
> >> + PAGE_SHIFT);
> >> @@ -175,7 +184,7 @@ int hns_roce_calc_hem_mhop(struct hns_roce_dev *hr_dev,
> >> return 0;
> >>
> >> /*
> >> - * QPC/MTPT/CQC/SRQC alloc hem for buffer pages.
> >> + * QPC/MTPT/CQC/SRQC/SCC_CTX alloc hem for buffer pages.
> >> * MTT/CQE alloc hem for bt pages.
> >> */
> >> bt_num = hns_roce_get_bt_num(table->type, mhop->hop_num);
> >> @@ -486,7 +495,7 @@ static int hns_roce_table_mhop_get(struct hns_roce_dev *hr_dev,
> >> }
> >>
> >> /*
> >> - * alloc buffer space chunk for QPC/MTPT/CQC/SRQC.
> >> + * alloc buffer space chunk for QPC/MTPT/CQC/SRQC/SCC_CTX.
> >> * alloc bt space chunk for MTT/CQE.
> >> */
> >> size = table->type < HEM_TYPE_MTT ? buf_chunk_size : bt_chunk_size;
> >> @@ -658,7 +667,7 @@ static void hns_roce_table_mhop_put(struct hns_roce_dev *hr_dev,
> >> }
> >>
> >> /*
> >> - * free buffer space chunk for QPC/MTPT/CQC/SRQC.
> >> + * free buffer space chunk for QPC/MTPT/CQC/SRQC/SCC_CTX.
> >> * free bt space chunk for MTT/CQE.
> >> */
> >> hns_roce_free_hem(hr_dev, table->hem[hem_idx]);
> >> @@ -904,6 +913,14 @@ int hns_roce_init_hem_table(struct hns_roce_dev *hr_dev,
> >> num_bt_l0 = hr_dev->caps.cqc_bt_num;
> >> hop_num = hr_dev->caps.cqc_hop_num;
> >> break;
> >> + case HEM_TYPE_SCC_CTX:
> >> + buf_chunk_size = 1 << (hr_dev->caps.scc_ctx_buf_pg_sz
> >> + + PAGE_SHIFT);
> >> + bt_chunk_size = 1 << (hr_dev->caps.scc_ctx_ba_pg_sz
> >> + + PAGE_SHIFT);
> >> + num_bt_l0 = hr_dev->caps.scc_ctx_bt_num;
> >> + hop_num = hr_dev->caps.scc_ctx_hop_num;
> >> + break;
> >> case HEM_TYPE_SRQC:
> >> buf_chunk_size = 1 << (hr_dev->caps.srqc_buf_pg_sz
> >> + PAGE_SHIFT);
> >> @@ -1081,6 +1098,9 @@ void hns_roce_cleanup_hem(struct hns_roce_dev *hr_dev)
> >> hns_roce_cleanup_hem_table(hr_dev,
> >> &hr_dev->srq_table.table);
> >> hns_roce_cleanup_hem_table(hr_dev, &hr_dev->cq_table.table);
> >> + if (hr_dev->caps.scc_ctx_entry_sz)
> >> + hns_roce_cleanup_hem_table(hr_dev,
> >> + &hr_dev->qp_table.scc_ctx_table);
> >> if (hr_dev->caps.trrl_entry_sz)
> >> hns_roce_cleanup_hem_table(hr_dev,
> >> &hr_dev->qp_table.trrl_table);
> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.h b/drivers/infiniband/hw/hns/hns_roce_hem.h
> >> index a650278..6e4fe97 100644
> >> --- a/drivers/infiniband/hw/hns/hns_roce_hem.h
> >> +++ b/drivers/infiniband/hw/hns/hns_roce_hem.h
> >> @@ -44,6 +44,7 @@ enum {
> >> HEM_TYPE_MTPT,
> >> HEM_TYPE_CQC,
> >> HEM_TYPE_SRQC,
> >> + HEM_TYPE_SCC_CTX,
> >>
> >> /* UNMAP HEM */
> >> HEM_TYPE_MTT,
> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> >> index 835b783..77cfd9b 100644
> >> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> >> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> >> @@ -1078,6 +1078,9 @@ static int hns_roce_query_pf_resource(struct hns_roce_dev *hr_dev)
> >> hr_dev->caps.sl_num = roce_get_field(req_b->qid_idx_sl_num,
> >> PF_RES_DATA_3_PF_SL_NUM_M,
> >> PF_RES_DATA_3_PF_SL_NUM_S);
> >> + hr_dev->caps.scc_ctx_bt_num = roce_get_field(req_b->scc_ctx_bt_idx_num,
> >> + PF_RES_DATA_4_PF_SCC_CTX_BT_NUM_M,
> >> + PF_RES_DATA_4_PF_SCC_CTX_BT_NUM_S);
> >>
> >> return 0;
> >> }
> >> @@ -1178,6 +1181,7 @@ static int hns_roce_v2_set_bt(struct hns_roce_dev *hr_dev)
> >> u8 qpc_hop_num = hr_dev->caps.qpc_hop_num;
> >> u8 cqc_hop_num = hr_dev->caps.cqc_hop_num;
> >> u8 mpt_hop_num = hr_dev->caps.mpt_hop_num;
> >> + u8 scc_ctx_hop_num = hr_dev->caps.scc_ctx_hop_num;
> >> struct hns_roce_cfg_bt_attr *req;
> >> struct hns_roce_cmq_desc desc;
> >>
> >> @@ -1225,6 +1229,20 @@ static int hns_roce_v2_set_bt(struct hns_roce_dev *hr_dev)
> >> CFG_BT_ATTR_DATA_3_VF_MPT_HOPNUM_S,
> >> mpt_hop_num == HNS_ROCE_HOP_NUM_0 ? 0 : mpt_hop_num);
> >>
> >> + roce_set_field(req->vf_scc_ctx_cfg,
> >> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_BA_PGSZ_M,
> >> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_BA_PGSZ_S,
> >> + hr_dev->caps.scc_ctx_ba_pg_sz + PG_SHIFT_OFFSET);
> >> + roce_set_field(req->vf_scc_ctx_cfg,
> >> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_BUF_PGSZ_M,
> >> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_BUF_PGSZ_S,
> >> + hr_dev->caps.scc_ctx_buf_pg_sz + PG_SHIFT_OFFSET);
> >> + roce_set_field(req->vf_scc_ctx_cfg,
> >> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_HOPNUM_M,
> >> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_HOPNUM_S,
> >> + scc_ctx_hop_num ==
> >> + HNS_ROCE_HOP_NUM_0 ? 0 : scc_ctx_hop_num);
> >> +
> >> return hns_roce_cmq_send(hr_dev, &desc, 1);
> >> }
> >>
> >> @@ -1372,9 +1390,14 @@ static int hns_roce_v2_profile(struct hns_roce_dev *hr_dev)
> >> caps->max_srq_wrs = HNS_ROCE_V2_MAX_SRQ_WR;
> >> caps->max_srq_sges = HNS_ROCE_V2_MAX_SRQ_SGE;
> >>
> >> - if (hr_dev->pci_dev->revision == 0x21)
> >> + if (hr_dev->pci_dev->revision == 0x21) {
> >> caps->flags |= HNS_ROCE_CAP_FLAG_ATOMIC |
> >> HNS_ROCE_CAP_FLAG_SRQ;
> >> + caps->scc_ctx_entry_sz = HNS_ROCE_V2_SCC_CTX_ENTRY_SZ;
> >> + caps->scc_ctx_ba_pg_sz = 0;
> >> + caps->scc_ctx_buf_pg_sz = 0;
> >> + caps->scc_ctx_hop_num = HNS_ROCE_SCC_CTX_HOP_NUM;
> >> + }
> >>
> >> ret = hns_roce_v2_set_bt(hr_dev);
> >> if (ret)
> >> @@ -2618,6 +2641,13 @@ static int hns_roce_v2_set_hem(struct hns_roce_dev *hr_dev,
> >> case HEM_TYPE_SRQC:
> >> op = HNS_ROCE_CMD_WRITE_SRQC_BT0;
> >> break;
> >> + case HEM_TYPE_SCC_CTX:
> >> + if (step_idx) {
> >> + /* No need to notify Hardware when step_idx is 1 or 2*/
> >> + return 0;
> >> + }
> >> + op = HNS_ROCE_CMD_WRITE_SCC_CTX_BT0;
> >> + break;
> >> default:
> >> dev_warn(dev, "Table %d not to be written by mailbox!\n",
> >> table->type);
> >> @@ -2677,6 +2707,9 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
> >> case HEM_TYPE_CQC:
> >> op = HNS_ROCE_CMD_DESTROY_CQC_BT0;
> >> break;
> >> + case HEM_TYPE_SCC_CTX:
> >> + /* there is no need to destroy scc ctx */
> >> + return 0;
> >
> > Combination of "break" and "return" constructions in the same
> > switch<->case is a great way to shot yourself into the head.
> >
> > Please don't mix them.
> Good suggestion.
> "return" will be replaced by "goto" in function hns_roce_v2_set_hem and
> function hns_roce_v2_clear_hem.
> The modified function is as follows, please help me to see if this is appropriate?
>
> @@ -2618,6 +2641,14 @@ static int hns_roce_v2_set_hem(struct hns_roce_dev *hr_dev,
> case HEM_TYPE_SRQC:
> op = HNS_ROCE_CMD_WRITE_SRQC_BT0;
> break;
> + case HEM_TYPE_SCC_CTX:
> + if (step_idx) {
> + /* No need to notify Hardware when step_idx is 1 or 2*/
> + ret = 0;
> + goto done;
I have the same feeling about "goto" as for the "return".
Let's do this, prior to executing switch():
if ( ... == HEM_TYPE_SCC_CTX && !step_idx)
return 0;
It will make switch<->case clean and very readable, without jumps in the middle.
> + }
> + op = HNS_ROCE_CMD_WRITE_SCC_CTX_BT0;
> + break;
> default:
> dev_warn(dev, "Table %d not to be written by mailbox!\n",
> table->type);
> @@ -2652,6 +2683,8 @@ static int hns_roce_v2_set_hem(struct hns_roce_dev *hr_dev,
> }
>
> hns_roce_free_cmd_mailbox(hr_dev, mailbox);
> +
> +done:
> return ret;
> }
> @@ -2661,8 +2694,8 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
> {
> struct device *dev = hr_dev->dev;
> struct hns_roce_cmd_mailbox *mailbox;
> - int ret = 0;
> u16 op = 0xff;
> + int ret;
>
> if (!hns_roce_check_whether_mhop(hr_dev, table->type))
> return 0;
> @@ -2677,6 +2710,10 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
> case HEM_TYPE_CQC:
> op = HNS_ROCE_CMD_DESTROY_CQC_BT0;
> break;
> + case HEM_TYPE_SCC_CTX:
> + /* no need to destroy scc ctx */
> + ret = 0;
> + goto done;
The same commend as above.
> case HEM_TYPE_SRQC:
> op = HNS_ROCE_CMD_DESTROY_SRQC_BT0;
> break;
> @@ -2696,6 +2733,8 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
> HNS_ROCE_CMD_TIMEOUT_MSECS);
>
> hns_roce_free_cmd_mailbox(hr_dev, mailbox);
> +
> +done:
> return ret;
> }
>
> Thanks
> >
> > Thanks
> >
>
Attachment:
signature.asc
Description: PGP signature
