From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 81BB04598C; Sat, 14 Sep 2024 10:46:54 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 11D6F402A8; Sat, 14 Sep 2024 10:46:54 +0200 (CEST) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id 6538E4026A for ; Sat, 14 Sep 2024 10:46:51 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.162.254]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4X5Pry2jcYz1T7X4; Sat, 14 Sep 2024 16:45:38 +0800 (CST) Received: from dggpeml500024.china.huawei.com (unknown [7.185.36.10]) by mail.maildlp.com (Postfix) with ESMTPS id CA98218010A; Sat, 14 Sep 2024 16:46:48 +0800 (CST) Received: from [10.67.121.161] (10.67.121.161) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Sat, 14 Sep 2024 16:46:48 +0800 Message-ID: <75e7c8a6-ffda-4d43-8a01-75b62ca5b91b@huawei.com> Date: Sat, 14 Sep 2024 16:46:48 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v7 8/8] net/hns3: support filter registers by module names To: Jie Hai , , , , CC: References: <20231214015650.3738578-1-haijie1@huawei.com> <20240914071320.1982-1-haijie1@huawei.com> <20240914071320.1982-9-haijie1@huawei.com> Content-Language: en-US From: fengchengwen In-Reply-To: <20240914071320.1982-9-haijie1@huawei.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.161] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggpeml500024.china.huawei.com (7.185.36.10) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2024/9/14 15:13, Jie Hai wrote: > This patch support dumping registers which name contains the > `filter` string. The module names are in lower case and so is > the `filter`. Available module names are cmdq, common_pf, > common_vf, ring, tqp_intr, 32_bit_dfx, 64_bit_dfx, bios, igu_egu, > ssu, ppp, rpu, ncsi, rtc, rcb, etc. > > Signed-off-by: Jie Hai > --- > drivers/net/hns3/hns3_regs.c | 309 ++++++++++++++++++++--------------- > 1 file changed, 180 insertions(+), 129 deletions(-) > > diff --git a/drivers/net/hns3/hns3_regs.c b/drivers/net/hns3/hns3_regs.c > index 622d2e1c3d02..265d9b433653 100644 > --- a/drivers/net/hns3/hns3_regs.c > +++ b/drivers/net/hns3/hns3_regs.c > @@ -12,8 +12,6 @@ > > #define HNS3_64_BIT_REG_OUTPUT_SIZE (sizeof(uint64_t) / sizeof(uint32_t)) > > -static int hns3_get_dfx_reg_cnt(struct hns3_hw *hw, uint32_t *count); > - > struct hns3_dirt_reg_entry { > const char *name; > uint32_t addr; > @@ -795,33 +793,77 @@ enum hns3_reg_modules { > HNS3_64_BIT_DFX, > }; > > +#define HNS3_MODULE_MASK(x) RTE_BIT32(x) > +#define HNS3_VF_MODULES (HNS3_MODULE_MASK(HNS3_CMDQ) | HNS3_MODULE_MASK(HNS3_COMMON_VF) | \ > + HNS3_MODULE_MASK(HNS3_RING) | HNS3_MODULE_MASK(HNS3_TQP_INTR)) > +#define HNS3_VF_ONLY_MODULES HNS3_MODULE_MASK(HNS3_COMMON_VF) > + > struct hns3_reg_list { > const void *reg_list; > uint32_t entry_num; > + const char *module; > }; > > static struct hns3_reg_list hns3_reg_lists[] = { > - [HNS3_BIOS_COMMON] = { dfx_bios_common_reg_list, RTE_DIM(dfx_bios_common_reg_list)}, > - [HNS3_SSU_0] = { dfx_ssu_reg_0_list, RTE_DIM(dfx_ssu_reg_0_list)}, > - [HNS3_SSU_1] = { dfx_ssu_reg_1_list, RTE_DIM(dfx_ssu_reg_1_list)}, > - [HNS3_IGU_EGU] = { dfx_igu_egu_reg_list, RTE_DIM(dfx_igu_egu_reg_list)}, > - [HNS3_RPU_0] = { dfx_rpu_reg_0_list, RTE_DIM(dfx_rpu_reg_0_list)}, > - [HNS3_RPU_1] = { dfx_rpu_reg_1_list, RTE_DIM(dfx_rpu_reg_1_list)}, > - [HNS3_NCSI] = { dfx_ncsi_reg_list, RTE_DIM(dfx_ncsi_reg_list)}, > - [HNS3_RTC] = { dfx_rtc_reg_list, RTE_DIM(dfx_rtc_reg_list)}, > - [HNS3_PPP] = { dfx_ppp_reg_list, RTE_DIM(dfx_ppp_reg_list)}, > - [HNS3_RCB] = { dfx_rcb_reg_list, RTE_DIM(dfx_rcb_reg_list)}, > - [HNS3_TQP] = { dfx_tqp_reg_list, RTE_DIM(dfx_tqp_reg_list)}, > - [HNS3_SSU_2] = { dfx_ssu_reg_2_list, RTE_DIM(dfx_ssu_reg_2_list)}, > - > - [HNS3_CMDQ] = { cmdq_reg_list, RTE_DIM(cmdq_reg_list)}, > - [HNS3_COMMON_PF] = { common_reg_list, RTE_DIM(common_reg_list)}, > - [HNS3_COMMON_VF] = { common_vf_reg_list, RTE_DIM(common_vf_reg_list)}, > - [HNS3_RING] = { ring_reg_list, RTE_DIM(ring_reg_list)}, > - [HNS3_TQP_INTR] = { tqp_intr_reg_list, RTE_DIM(tqp_intr_reg_list)}, > - > - [HNS3_32_BIT_DFX] = { regs_32_bit_list, RTE_DIM(regs_32_bit_list)}, > - [HNS3_64_BIT_DFX] = { regs_64_bit_list, RTE_DIM(regs_64_bit_list)}, > + [HNS3_BIOS_COMMON] = { > + dfx_bios_common_reg_list, RTE_DIM(dfx_bios_common_reg_list), "bios" > + }, > + [HNS3_SSU_0] = { > + dfx_ssu_reg_0_list, RTE_DIM(dfx_ssu_reg_0_list), "ssu" > + }, > + [HNS3_SSU_1] = { > + dfx_ssu_reg_1_list, RTE_DIM(dfx_ssu_reg_1_list), "ssu" > + }, > + [HNS3_IGU_EGU] = { > + dfx_igu_egu_reg_list, RTE_DIM(dfx_igu_egu_reg_list), "igu_egu" > + }, > + [HNS3_RPU_0] = { > + dfx_rpu_reg_0_list, RTE_DIM(dfx_rpu_reg_0_list), "rpu" > + }, > + [HNS3_RPU_1] = { > + dfx_rpu_reg_1_list, RTE_DIM(dfx_rpu_reg_1_list), "rpu" > + }, > + [HNS3_NCSI] = { > + dfx_ncsi_reg_list, RTE_DIM(dfx_ncsi_reg_list), "ncsi" > + }, > + [HNS3_RTC] = { > + dfx_rtc_reg_list, RTE_DIM(dfx_rtc_reg_list), "rtc" > + }, > + [HNS3_PPP] = { > + dfx_ppp_reg_list, RTE_DIM(dfx_ppp_reg_list), "ppp" > + }, > + [HNS3_RCB] = { > + dfx_rcb_reg_list, RTE_DIM(dfx_rcb_reg_list), "rcb" > + }, > + [HNS3_TQP] = { > + dfx_tqp_reg_list, RTE_DIM(dfx_tqp_reg_list), "tqp" > + }, > + [HNS3_SSU_2] = { > + dfx_ssu_reg_2_list, RTE_DIM(dfx_ssu_reg_2_list), "ssu" > + }, > + > + [HNS3_CMDQ] = { > + cmdq_reg_list, RTE_DIM(cmdq_reg_list), "cmdq" > + }, > + [HNS3_COMMON_PF] = { > + common_reg_list, RTE_DIM(common_reg_list), "common_pf" > + }, > + [HNS3_COMMON_VF] = { > + common_vf_reg_list, RTE_DIM(common_vf_reg_list), "common_vf" > + }, > + [HNS3_RING] = { > + ring_reg_list, RTE_DIM(ring_reg_list), "ring" > + }, > + [HNS3_TQP_INTR] = { > + tqp_intr_reg_list, RTE_DIM(tqp_intr_reg_list), "tqp_intr" > + }, > + > + [HNS3_32_BIT_DFX] = { > + regs_32_bit_list, RTE_DIM(regs_32_bit_list), "32_bit_dfx" > + }, > + [HNS3_64_BIT_DFX] = { > + regs_64_bit_list, RTE_DIM(regs_64_bit_list), "64_bit_dfx" > + }, > }; > > static const uint32_t hns3_dfx_reg_opcode_list[] = { > @@ -865,21 +907,52 @@ hns3_get_regs_num(struct hns3_hw *hw, uint32_t *regs_num_32_bit, > return 0; > } > > -static int > -hns3_get_32_64_regs_cnt(struct hns3_hw *hw, uint32_t *count) > +static bool > +hns3_check_module_match(const char *module, const char *filter) > { > - uint32_t regs_num_32_bit, regs_num_64_bit; > - int ret; > + if (filter != NULL && strcmp(filter, module) != 0) > + return false; > > - ret = hns3_get_regs_num(hw, ®s_num_32_bit, ®s_num_64_bit); > - if (ret) { > - hns3_err(hw, "fail to get the number of registers, " > - "ret = %d.", ret); > - return ret; > + return true; > +} > + > +#define HNS3_MAX_MODULES_LEN 512 > +static uint32_t > +hns3_check_module_names(const char *filter, char *names) > +{ > + uint32_t ret = 0, pos = 0; > + size_t i; > + > + for (i = 0; i < RTE_DIM(hns3_reg_lists); i++) { > + if (hns3_check_module_match(hns3_reg_lists[i].module, filter)) > + ret |= HNS3_MODULE_MASK(i); > + if (HNS3_MAX_MODULES_LEN - pos <= strlen(hns3_reg_lists[i].module)) > + return -ENOMEM; I can't understand it, could you explain it. > + snprintf(&names[pos], HNS3_MAX_MODULES_LEN - pos, " %s", hns3_reg_lists[i].module); Suggest add a new function which format all filter's name, e.g. hns3_format_reg_module_name()? > + pos += strlen(hns3_reg_lists[i].module) + 1; > } > + return ret; > +} > > - *count += regs_num_32_bit + regs_num_64_bit * HNS3_64_BIT_REG_OUTPUT_SIZE; > - return 0; > +static uint32_t > +hns3_check_filter(struct hns3_hw *hw, const char *filter) > +{ > + struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw); > + char names[HNS3_MAX_MODULES_LEN + 1] = {0}; > + uint32_t modules = 0; > + > + if (filter == NULL) > + return (1 << RTE_DIM(hns3_reg_lists)) - 1; The returned bitmask should remove the difference of PF and VF. > + > + modules = hns3_check_module_names(filter, names); > + if (hns->is_vf) > + modules &= HNS3_VF_MODULES; > + else > + modules &= ~HNS3_VF_ONLY_MODULES; > + if (modules == 0) > + hns3_err(hw, "mismatched module name! Available names are:\n%s.", names); Except the error should this, how about add the modules info in driver's rst? > + > + return modules; > } > > static int > @@ -917,73 +990,25 @@ hns3_get_dfx_reg_bd_num(struct hns3_hw *hw, uint32_t *bd_num_list, > return 0; > } > > -static int > -hns3_get_dfx_reg_cnt(struct hns3_hw *hw, uint32_t *count) > -{ > - int opcode_num = RTE_DIM(hns3_dfx_reg_opcode_list); > - uint32_t bd_num_list[opcode_num]; > - uint32_t reg_num; > - int ret; > - int i; > - > - ret = hns3_get_dfx_reg_bd_num(hw, bd_num_list, opcode_num); > - if (ret) > - return ret; > - > - for (i = 0; i < opcode_num; i++) { > - reg_num = bd_num_list[i] * HNS3_CMD_DESC_DATA_NUM; > - if (reg_num != hns3_reg_lists[i].entry_num) { > - hns3_err(hw, "Query register number differ from the list for module!"); > - return -EINVAL; > - } > - *count += reg_num; > - } > - > - return 0; > -} > - > -static int > -hns3_get_firmware_reg_cnt(struct hns3_hw *hw, uint32_t *count) > -{ > - int ret; > - > - ret = hns3_get_32_64_regs_cnt(hw, count); > - if (ret < 0) > - return ret; > - > - return hns3_get_dfx_reg_cnt(hw, count); > -} > - > -static int > -hns3_get_regs_length(struct hns3_hw *hw, uint32_t *length) > +static uint32_t > +hns3_get_regs_length(struct hns3_hw *hw, uint32_t modules) > { > - struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw); > - uint32_t dfx_reg_cnt = 0; > - uint32_t common_cnt; > - uint32_t len; > - int ret; > - > - if (hns->is_vf) > - common_cnt = RTE_DIM(common_vf_reg_list); > - else > - common_cnt = RTE_DIM(common_reg_list); > + uint32_t reg_num = 0, length = 0; > + uint32_t i; > > - len = RTE_DIM(cmdq_reg_list) + common_cnt + > - RTE_DIM(ring_reg_list) * hw->tqps_num + > - RTE_DIM(tqp_intr_reg_list) * hw->intr_tqps_num; > + for (i = 0; i < RTE_DIM(hns3_reg_lists); i++) { > + if ((BIT(i) & modules) == 0) Please use RTE_BIT32, so that it consistent with "#define HNS3_MODULE_MASK(x) RTE_BIT32(x)" > + continue; > + reg_num = hns3_reg_lists[i].entry_num; > + if (i == HNS3_RING) > + reg_num *= hw->tqps_num; > + else if (i == HNS3_TQP_INTR) > + reg_num *= hw->intr_tqps_num; > > - if (!hns->is_vf) { > - ret = hns3_get_firmware_reg_cnt(hw, &dfx_reg_cnt); > - if (ret) { > - hns3_err(hw, "fail to get the number of dfx registers, " > - "ret = %d.", ret); > - return ret; > - } > - len += dfx_reg_cnt; > + length += reg_num; > } > > - *length = len; > - return 0; > + return length; > } > > static void > @@ -1124,12 +1149,15 @@ hns3_get_64_bit_regs(struct hns3_hw *hw, uint32_t regs_num, struct rte_dev_reg_i > > static void > hns3_direct_access_regs_help(struct hns3_hw *hw, struct rte_dev_reg_info *regs, > - enum hns3_reg_modules idx) > + uint32_t modules, enum hns3_reg_modules idx) > { > const struct hns3_dirt_reg_entry *reg_list; > uint32_t *data = regs->data; > size_t reg_num, i, cnt; > > + if ((modules & HNS3_MODULE_MASK(idx)) == 0) > + return; > + > data += regs->length; > reg_num = hns3_reg_lists[idx].entry_num; > reg_list = hns3_reg_lists[idx].reg_list; > @@ -1157,14 +1185,14 @@ hns3_get_module_tqp_reg_offset(enum hns3_reg_modules idx, uint16_t queue_id) > > static void > hns3_direct_access_tqp_regs_help(struct hns3_hw *hw, struct rte_dev_reg_info *regs, > - enum hns3_reg_modules idx) > + uint32_t modules, enum hns3_reg_modules idx) > { > const struct hns3_dirt_reg_entry *reg_list; > uint16_t tqp_num, reg_offset; > uint32_t *data = regs->data; > uint32_t reg_num, i, j; > > - if (idx != HNS3_RING && idx != HNS3_TQP_INTR) > + if ((modules & HNS3_MODULE_MASK(idx)) == 0) > return; > > tqp_num = (idx == HNS3_RING) ? hw->tqps_num : hw->intr_tqps_num; > @@ -1184,18 +1212,13 @@ hns3_direct_access_tqp_regs_help(struct hns3_hw *hw, struct rte_dev_reg_info *re > } > > static void > -hns3_direct_access_regs(struct hns3_hw *hw, struct rte_dev_reg_info *regs) > +hns3_direct_access_regs(struct hns3_hw *hw, struct rte_dev_reg_info *regs, uint32_t modules) > { > - struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw); > - > - if (hns->is_vf) > - hns3_direct_access_regs_help(hw, regs, HNS3_COMMON_VF); > - else > - hns3_direct_access_regs_help(hw, regs, HNS3_COMMON_PF); > - > - hns3_direct_access_regs_help(hw, regs, HNS3_CMDQ); > - hns3_direct_access_tqp_regs_help(hw, regs, HNS3_RING); > - hns3_direct_access_tqp_regs_help(hw, regs, HNS3_TQP_INTR); > + hns3_direct_access_regs_help(hw, regs, modules, HNS3_COMMON_VF); > + hns3_direct_access_regs_help(hw, regs, modules, HNS3_COMMON_PF); > + hns3_direct_access_regs_help(hw, regs, modules, HNS3_CMDQ); > + hns3_direct_access_tqp_regs_help(hw, regs, modules, HNS3_RING); > + hns3_direct_access_tqp_regs_help(hw, regs, modules, HNS3_TQP_INTR); > } > > static int > @@ -1239,7 +1262,7 @@ hns3_dfx_reg_fetch_data(struct hns3_cmd_desc *desc, int bd_num, uint32_t *reg) > } > > static int > -hns3_get_dfx_regs(struct hns3_hw *hw, struct rte_dev_reg_info *regs) > +hns3_get_dfx_regs(struct hns3_hw *hw, struct rte_dev_reg_info *regs, uint32_t modules) > { > int opcode_num = RTE_DIM(hns3_dfx_reg_opcode_list); > uint32_t max_bd_num, bd_num, opcode, regs_num; > @@ -1264,6 +1287,8 @@ hns3_get_dfx_regs(struct hns3_hw *hw, struct rte_dev_reg_info *regs) > for (i = 0; i < opcode_num; i++) { > opcode = hns3_dfx_reg_opcode_list[i]; > bd_num = bd_num_list[i]; > + if ((modules & HNS3_MODULE_MASK(i)) == 0) > + continue; > if (bd_num == 0) > continue; > ret = hns3_dfx_reg_cmd_send(hw, cmd_descs, bd_num, opcode); > @@ -1272,6 +1297,11 @@ hns3_get_dfx_regs(struct hns3_hw *hw, struct rte_dev_reg_info *regs) > > data += regs->length; > regs_num = hns3_dfx_reg_fetch_data(cmd_descs, bd_num, data); > + if (regs_num != hns3_reg_lists[i].entry_num) { > + hns3_err(hw, "Query register number differ from the list for module %s!", > + hns3_reg_lists[i].module); > + return -EINVAL; > + } > hns3_fill_dfx_regs_name(hw, regs, hns3_reg_lists[i].reg_list, regs_num); > regs->length += regs_num; > } > @@ -1281,14 +1311,14 @@ hns3_get_dfx_regs(struct hns3_hw *hw, struct rte_dev_reg_info *regs) > } > > static int > -hns3_get_regs_from_firmware(struct hns3_hw *hw, struct rte_dev_reg_info *regs) > +hns3_get_32_b4_bit_regs(struct hns3_hw *hw, struct rte_dev_reg_info *regs, uint32_t modules) > { > - struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw); > uint32_t regs_num_32_bit; > uint32_t regs_num_64_bit; > int ret; > > - if (hns->is_vf) > + if ((modules & HNS3_MODULE_MASK(HNS3_32_BIT_DFX)) == 0 && > + (modules & HNS3_MODULE_MASK(HNS3_32_BIT_DFX)) == 0) > return 0; > > ret = hns3_get_regs_num(hw, ®s_num_32_bit, ®s_num_64_bit); > @@ -1297,19 +1327,39 @@ hns3_get_regs_from_firmware(struct hns3_hw *hw, struct rte_dev_reg_info *regs) > return ret; > } > > - ret = hns3_get_32_bit_regs(hw, regs_num_32_bit, regs); > - if (ret) { > - hns3_err(hw, "Get 32 bit register failed, ret = %d", ret); > - return ret; > + if ((modules & HNS3_MODULE_MASK(HNS3_32_BIT_DFX)) != 0) { > + ret = hns3_get_32_bit_regs(hw, regs_num_32_bit, regs); > + if (ret) { > + hns3_err(hw, "Get 32 bit register failed, ret = %d", ret); > + return ret; > + } > } > > - ret = hns3_get_64_bit_regs(hw, regs_num_64_bit, regs); > - if (ret) { > - hns3_err(hw, "Get 64 bit register failed, ret = %d", ret); > - return ret; > + if ((modules & HNS3_MODULE_MASK(HNS3_32_BIT_DFX)) != 0) { > + ret = hns3_get_64_bit_regs(hw, regs_num_64_bit, regs); > + if (ret) { > + hns3_err(hw, "Get 64 bit register failed, ret = %d", ret); > + return ret; > + } > } > > - return hns3_get_dfx_regs(hw, regs); > + return 0; > +} > + > +static int > +hns3_get_regs_from_firmware(struct hns3_hw *hw, struct rte_dev_reg_info *regs, uint32_t modules) > +{ > + struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw); > + int ret; > + > + if (hns->is_vf) > + return 0; > + > + ret = hns3_get_32_b4_bit_regs(hw, regs, modules); > + if (ret != 0) > + return ret; > + > + return hns3_get_dfx_regs(hw, regs, modules); > } > > int > @@ -1317,13 +1367,14 @@ hns3_get_regs(struct rte_eth_dev *eth_dev, struct rte_dev_reg_info *regs) > { > struct hns3_adapter *hns = eth_dev->data->dev_private; > struct hns3_hw *hw = &hns->hw; > + uint32_t modules = 0; No need to initilize zero. > uint32_t length; > - int ret; > > - ret = hns3_get_regs_length(hw, &length); > - if (ret) > - return ret; > + modules = hns3_check_filter(hw, regs->filter); How about rename hns3_check_filter with hns3_parse_modules_by_filter()? > + if (modules == 0) > + return -EINVAL; > > + length = hns3_get_regs_length(hw, modules); > if (regs->data == NULL) { > regs->length = length; > regs->width = sizeof(uint32_t); > @@ -1339,8 +1390,8 @@ hns3_get_regs(struct rte_eth_dev *eth_dev, struct rte_dev_reg_info *regs) > regs->length = 0; > > /* fetching per-PF registers values from PF PCIe register space */ > - hns3_direct_access_regs(hw, regs); > + hns3_direct_access_regs(hw, regs, modules); > > /* fetching PF common registers values from firmware */ > - return hns3_get_regs_from_firmware(hw, regs); > + return hns3_get_regs_from_firmware(hw, regs, modules); > }