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 A1D7B43B51; Tue, 20 Feb 2024 09:43:17 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8F06640691; Tue, 20 Feb 2024 09:43:17 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 067AF4067C for ; Tue, 20 Feb 2024 09:43:15 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.88.194]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4TfCYN3MYCzvVpF; Tue, 20 Feb 2024 16:41:12 +0800 (CST) Received: from kwepemd100004.china.huawei.com (unknown [7.221.188.31]) by mail.maildlp.com (Postfix) with ESMTPS id AE3941400FD; Tue, 20 Feb 2024 16:43:13 +0800 (CST) Received: from [10.67.121.175] (10.67.121.175) by kwepemd100004.china.huawei.com (7.221.188.31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.2.1258.28; Tue, 20 Feb 2024 16:43:12 +0800 Message-ID: <5e5e7f10-0227-3b5a-1e82-fdb92b5573b1@huawei.com> Date: Tue, 20 Feb 2024 16:43:12 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH v2 1/7] ethdev: support report register names and filter To: Ferruh Yigit , CC: , , , References: <20231214015650.3738578-1-haijie1@huawei.com> <20240205105151.275591-1-haijie1@huawei.com> <20240205105151.275591-2-haijie1@huawei.com> <7884b943-376e-4c04-9b9f-ebf04515899c@amd.com> From: Jie Hai In-Reply-To: <7884b943-376e-4c04-9b9f-ebf04515899c@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.175] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemd100004.china.huawei.com (7.221.188.31) 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 Hi, Ferruh, Thanks for your review. On 2024/2/8 1:00, Ferruh Yigit wrote: > On 2/5/2024 10:51 AM, Jie Hai wrote: >> This patch adds "filter" and "names" fields to "rte_dev_reg_info" >> structure. Names of registers in data fields can be reported and >> the registers can be filtered by their names. >> >> For compatibility, the original API rte_eth_dev_get_reg_info() >> does not use the name and filter fields. The new API >> rte_eth_dev_get_reg_info_ext() is added to support reporting >> names and filtering by names. If the drivers does not report >> the names, set them to "offset_XXX". >> >> Signed-off-by: Jie Hai >> --- >> doc/guides/rel_notes/release_24_03.rst | 8 ++++++ >> lib/ethdev/rte_dev_info.h | 11 ++++++++ >> lib/ethdev/rte_ethdev.c | 36 ++++++++++++++++++++++++++ >> lib/ethdev/rte_ethdev.h | 22 ++++++++++++++++ >> 4 files changed, 77 insertions(+) >> >> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst >> index 84d3144215c6..5d402341223a 100644 >> --- a/doc/guides/rel_notes/release_24_03.rst >> +++ b/doc/guides/rel_notes/release_24_03.rst >> @@ -75,6 +75,11 @@ New Features >> * Added support for Atomic Rules' TK242 packet-capture family of devices >> with PCI IDs: ``0x1024, 0x1025, 0x1026``. >> >> +* **Added support for dumping regiters with names and filter.** >> > > s/regiters/registers/ Will correct in v3. > >> + >> + * Added new API functions ``rte_eth_dev_get_reg_info_ext()`` to and filter >> + * the registers by their names and get the information of registers(names, >> + * values and other attributes). >> > > '*' makes a bullet, but above seems one sentences, if so please only > keep the first '*'. Will correct in v3. > >> Removed Items >> ------------- >> @@ -124,6 +129,9 @@ ABI Changes >> >> * No ABI change that would break compatibility with 23.11. >> >> +* ethdev: Added ``filter`` and ``names`` fields to ``rte_dev_reg_info`` >> + structure for reporting names of regiters and filtering them by names. >> + >> > > This will break the ABI. > > Think about a case, an application compiled with an old version of DPDK, > later same application started to use this version without re-compile, > application will send old version of 'struct rte_dev_reg_info', but new > version of DPDK will try to access or update new fields of the 'struct > rte_dev_reg_info' > Actually, we use a local variable "struct rte_dev_reg_info reg_info" in 'rte_eth_dev_get_reg_info()' to pass to the driver, and the new fields are set to zero. The old drivers do not visit the new fields. We make constrains that if filter is NULL, do not filter and get info of all registers. For an old version APP and new version ethdev, driver will not visit the new fields. so it does not break the ABI. > One option is: > - to add a new 'struct rte_dev_reg_info_ext', > - 'rte_eth_dev_get_reg_info()' still uses old 'struct rte_dev_reg_info' > - 'get_reg()' dev_ops will use this new 'struct rte_dev_reg_info_ext' > - Add deprecation notice to update 'rte_eth_dev_get_reg_info()' to use > new struct in next LTS release > > >> Known Issues >> ------------ >> diff --git a/lib/ethdev/rte_dev_info.h b/lib/ethdev/rte_dev_info.h >> index 67cf0ae52668..2f4541bd46c8 100644 >> --- a/lib/ethdev/rte_dev_info.h >> +++ b/lib/ethdev/rte_dev_info.h >> @@ -11,6 +11,11 @@ extern "C" { >> >> #include >> >> +#define RTE_ETH_REG_NAME_SIZE 128 >> +struct rte_eth_reg_name { >> + char name[RTE_ETH_REG_NAME_SIZE]; >> +}; >> + >> /* >> * Placeholder for accessing device registers >> */ >> @@ -20,6 +25,12 @@ struct rte_dev_reg_info { >> uint32_t length; /**< Number of registers to fetch */ >> uint32_t width; /**< Size of device register */ >> uint32_t version; /**< Device version */ >> + /** >> + * Filter for target subset of registers. >> + * This field could affects register selection for data/length/names. >> + */ >> + char *filter; >> + struct rte_eth_reg_name *names; /**< Registers name saver */ >> }; >> >> /* >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index f1c658f49e80..3e0294e49092 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -6388,8 +6388,39 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock) >> >> int >> rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info) >> +{ >> + struct rte_dev_reg_info reg_info; >> + int ret; >> + >> + if (info == NULL) { >> + RTE_ETHDEV_LOG_LINE(ERR, >> + "Cannot get ethdev port %u register info to NULL", >> + port_id); >> + return -EINVAL; >> + } >> + >> + reg_info.length = info->length; >> + reg_info.data = info->data; >> + reg_info.names = NULL; >> + reg_info.filter = NULL; >> + >> + ret = rte_eth_dev_get_reg_info_ext(port_id, ®_info); >> + if (ret != 0) >> + return ret; >> + >> + info->length = reg_info.length; >> + info->width = reg_info.width; >> + info->version = reg_info.version; >> + info->offset = reg_info.offset; >> + >> + return 0; >> +} >> + >> +int >> +rte_eth_dev_get_reg_info_ext(uint16_t port_id, struct rte_dev_reg_info *info) >> { >> struct rte_eth_dev *dev; >> + uint32_t i; >> int ret; >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> @@ -6408,6 +6439,11 @@ rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info) >> >> rte_ethdev_trace_get_reg_info(port_id, info, ret); >> >> + /* Report the default names if drivers not report. */ >> + if (info->names != NULL && strlen(info->names[0].name) == 0) >> + for (i = 0; i < info->length; i++) >> + sprintf(info->names[i].name, "offset_%x", >> + info->offset + i * info->width); >> > > Better to use 'snprintf()' > Will correct in v3. >> return ret; >> } >> >> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h >> index 2687c23fa6fb..3abc2ad3f865 100644 >> --- a/lib/ethdev/rte_ethdev.h >> +++ b/lib/ethdev/rte_ethdev.h >> @@ -5053,6 +5053,28 @@ __rte_experimental >> int rte_eth_get_monitor_addr(uint16_t port_id, uint16_t queue_id, >> struct rte_power_monitor_cond *pmc); >> >> +/** >> + * Retrieve the filtered device registers (values and names) and >> + * register attributes (number of registers and register size) >> + * >> + * @param port_id >> + * The port identifier of the Ethernet device. >> + * @param info >> + * Pointer to rte_dev_reg_info structure to fill in. If info->data is >> + * NULL, the function fills in the width and length fields. If non-NULL, >> + * the values of registers whose name contains the filter string are put >> + * into the buffer pointed at by the data field. Do the same for the names >> + * of registers if info->names is not NULL. >> > > May be good to mention if info->names is not NULL, but driver doesn't > support names, ehtdev fills the names automatically. > > As both 'rte_eth_dev_get_reg_info()' & 'rte_eth_dev_get_reg_info_ext()' > use same dev_ops ('get_reg()'), it is possible that driver doesn't > support filtering, so if the user provides info->filter but driver > doesn't support it, I think API should return error, what do you think? > > And can you please make it clear above, if filtering is provided with > info->data = NULL, are the returned width and length fields for filtered > number of registers or all registers? > > Will correct in v3. >> + * @return >> + * - (0) if successful. >> + * - (-ENOTSUP) if hardware doesn't support. >> + * - (-EINVAL) if bad parameter. >> + * - (-ENODEV) if *port_id* invalid. >> + * - (-EIO) if device is removed. >> + * - others depends on the specific operations implementation. >> + */ >> +int rte_eth_dev_get_reg_info_ext(uint16_t port_id, struct rte_dev_reg_info *info); >> + >> > > Can you please make the new API as experimental. That is the requirement > for new APIs. > > Also need to add the API to version.map Will correct in v3. > > >> /** >> * Retrieve device registers and register attributes (number of registers and >> * register size) > > . Best regards, Jie Hai