[v3] app/testpmd: support ddp dump command for ice

Message ID 20220518065806.1005694-1-stevex.yang@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Andrew Rybchenko
Headers
Series [v3] app/testpmd: support ddp dump command for ice |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues
ci/github-robot: build fail github build: failed
ci/iol-testing fail build patch failure

Commit Message

Steve Yang May 18, 2022, 6:58 a.m. UTC
  Dump DDP runtime configure into a binary(package) file from ice PF port.

Add command line:
    ddp dump <port_id> <config_path>

Parameters:
    <port_id>       the PF Port ID
    <config_path>   dumped runtime configure file, if not a absolute path,
                    it will be dumped to testpmd running directory.

For example:
testpmd> ddp dump 0 current.pkg

If you want to dump ice VF DDP runtime configure, you need bind other
unused PF port of the NIC first, and then dump the PF's runtime configure
as target output.

Signed-off-by: Steve Yang <stevex.yang@intel.com>

---
v3: change git commit log
---
 app/test-pmd/cmdline.c   | 76 ++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/meson.build |  3 ++
 2 files changed, 79 insertions(+)
  

Comments

Stephen Hemminger May 18, 2022, 3:24 p.m. UTC | #1
On Wed, 18 May 2022 06:58:06 +0000
Steve Yang <stevex.yang@intel.com> wrote:

> +#define ICE_BUFF_SIZE	0x000c9000
Having magic size hard coded in testpmd is bad idea.
If there is driver specific size it should be exposed by API

> +	size = ICE_BUFF_SIZE;
> +	buff = (uint8_t *)malloc(ICE_BUFF_SIZE);

Cast of void * is not necessary in C (only C++)
  
Andrew Rybchenko May 19, 2022, 8:14 a.m. UTC | #2
On 5/18/22 18:24, Stephen Hemminger wrote:
> On Wed, 18 May 2022 06:58:06 +0000
> Steve Yang <stevex.yang@intel.com> wrote:
> 
>> +#define ICE_BUFF_SIZE	0x000c9000
> Having magic size hard coded in testpmd is bad idea.
> If there is driver specific size it should be exposed by API
> 
>> +	size = ICE_BUFF_SIZE;
>> +	buff = (uint8_t *)malloc(ICE_BUFF_SIZE);
> 
> Cast of void * is not necessary in C (only C++)

Also the patch breaks build as reported by CI [1]:

[1] 
https://patches.dpdk.org/project/dpdk/patch/20220518065806.1005694-1-stevex.yang@intel.com/
  
David Marchand May 19, 2022, 12:12 p.m. UTC | #3
On Wed, May 18, 2022 at 9:07 AM Steve Yang <stevex.yang@intel.com> wrote:
>
> Dump DDP runtime configure into a binary(package) file from ice PF port.
>
> Add command line:
>     ddp dump <port_id> <config_path>
>
> Parameters:
>     <port_id>       the PF Port ID
>     <config_path>   dumped runtime configure file, if not a absolute path,
>                     it will be dumped to testpmd running directory.
>
> For example:
> testpmd> ddp dump 0 current.pkg
>
> If you want to dump ice VF DDP runtime configure, you need bind other
> unused PF port of the NIC first, and then dump the PF's runtime configure
> as target output.
>
> Signed-off-by: Steve Yang <stevex.yang@intel.com>

I proposed a way to move such code in drivers, please have a look
at/give feedback to:
https://patchwork.dpdk.org/project/dpdk/list/?series=23000&state=*
  
Qi Zhang May 20, 2022, 3:07 a.m. UTC | #4
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, May 19, 2022 8:13 PM
> To: Yang, SteveX <stevex.yang@intel.com>
> Cc: dev <dev@dpdk.org>; Zhang, Yuying <yuying.zhang@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Ferruh Yigit <ferruh.yigit@xilinx.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Thomas Monjalon
> <thomas@monjalon.net>
> Subject: Re: [PATCH v3] app/testpmd: support ddp dump command for ice
> 
> On Wed, May 18, 2022 at 9:07 AM Steve Yang <stevex.yang@intel.com> wrote:
> >
> > Dump DDP runtime configure into a binary(package) file from ice PF port.
> >
> > Add command line:
> >     ddp dump <port_id> <config_path>
> >
> > Parameters:
> >     <port_id>       the PF Port ID
> >     <config_path>   dumped runtime configure file, if not a absolute path,
> >                     it will be dumped to testpmd running directory.
> >
> > For example:
> > testpmd> ddp dump 0 current.pkg
> >
> > If you want to dump ice VF DDP runtime configure, you need bind other
> > unused PF port of the NIC first, and then dump the PF's runtime
> > configure as target output.
> >
> > Signed-off-by: Steve Yang <stevex.yang@intel.com>
> 
> I proposed a way to move such code in drivers, please have a look at/give
> feedback to:
> https://patchwork.dpdk.org/project/dpdk/list/?series=23000&state=*

This looks like a good idea, we will review and thanks for the heads-up

> 
> 
> --
> David Marchand
  
Qi Zhang May 21, 2022, 1:19 a.m. UTC | #5
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Friday, May 20, 2022 11:07 AM
> To: David Marchand <david.marchand@redhat.com>; Yang, SteveX
> <stevex.yang@intel.com>
> Cc: dev <dev@dpdk.org>; Zhang, Yuying <Yuying.Zhang@intel.com>; Ferruh
> Yigit <ferruh.yigit@xilinx.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Thomas Monjalon <thomas@monjalon.net>
> Subject: RE: [PATCH v3] app/testpmd: support ddp dump command for ice
> 
> 
> 
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Thursday, May 19, 2022 8:13 PM
> > To: Yang, SteveX <stevex.yang@intel.com>
> > Cc: dev <dev@dpdk.org>; Zhang, Yuying <yuying.zhang@intel.com>; Zhang,
> > Qi Z <qi.z.zhang@intel.com>; Ferruh Yigit <ferruh.yigit@xilinx.com>;
> > Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Thomas Monjalon
> > <thomas@monjalon.net>
> > Subject: Re: [PATCH v3] app/testpmd: support ddp dump command for ice
> >
> > On Wed, May 18, 2022 at 9:07 AM Steve Yang <stevex.yang@intel.com>
> wrote:
> > >
> > > Dump DDP runtime configure into a binary(package) file from ice PF port.
> > >
> > > Add command line:
> > >     ddp dump <port_id> <config_path>
> > >
> > > Parameters:
> > >     <port_id>       the PF Port ID
> > >     <config_path>   dumped runtime configure file, if not a absolute path,
> > >                     it will be dumped to testpmd running directory.
> > >
> > > For example:
> > > testpmd> ddp dump 0 current.pkg
> > >
> > > If you want to dump ice VF DDP runtime configure, you need bind
> > > other unused PF port of the NIC first, and then dump the PF's
> > > runtime configure as target output.
> > >
> > > Signed-off-by: Steve Yang <stevex.yang@intel.com>
> >
> > I proposed a way to move such code in drivers, please have a look
> > at/give feedback to:
> > https://patchwork.dpdk.org/project/dpdk/list/?series=23000&state=*
> 
> This looks like a good idea, we will review and thanks for the heads-up
> 

David
	Are these RFC patches target to DPDK 22.07?
	If not, is below proposal acceptable?
	1. merge this patch in DPDK 22.07.
	2. meanwhile we will also contribute the ice refactor patch based on your RFC, so it can be  captured in next release.
Thanks
Qi

> >
> >
> > --
> > David Marchand
  
David Marchand May 23, 2022, 7:14 a.m. UTC | #6
On Sat, May 21, 2022 at 3:20 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > > Dump DDP runtime configure into a binary(package) file from ice PF port.
> > > >
> > > > Add command line:
> > > >     ddp dump <port_id> <config_path>
> > > >
> > > > Parameters:
> > > >     <port_id>       the PF Port ID
> > > >     <config_path>   dumped runtime configure file, if not a absolute path,
> > > >                     it will be dumped to testpmd running directory.
> > > >
> > > > For example:
> > > > testpmd> ddp dump 0 current.pkg
> > > >
> > > > If you want to dump ice VF DDP runtime configure, you need bind
> > > > other unused PF port of the NIC first, and then dump the PF's
> > > > runtime configure as target output.
> > > >
> > > > Signed-off-by: Steve Yang <stevex.yang@intel.com>
> > >
> > > I proposed a way to move such code in drivers, please have a look
> > > at/give feedback to:
> > > https://patchwork.dpdk.org/project/dpdk/list/?series=23000&state=*
> >
> > This looks like a good idea, we will review and thanks for the heads-up
> >
>
> David
>         Are these RFC patches target to DPDK 22.07?
>         If not, is below proposal acceptable?
>         1. merge this patch in DPDK 22.07.
>         2. meanwhile we will also contribute the ice refactor patch based on your RFC, so it can be  captured in next release.

My intention is to get them in 22.07, if other maintainers are happy with it.
I just posted a non-RFC series.
  

Patch

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6ffea8e21a..6282981ee9 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -60,6 +60,9 @@ 
 #ifdef RTE_NET_I40E
 #include <rte_pmd_i40e.h>
 #endif
+#ifdef RTE_NET_ICE
+#include <rte_pmd_ice.h>
+#endif
 #ifdef RTE_NET_BNXT
 #include <rte_pmd_bnxt.h>
 #endif
@@ -654,6 +657,9 @@  static void cmd_help_long_parsed(void *parsed_result,
 			"set link-down port (port_id)\n"
 			"	Set link down for a port.\n\n"
 
+			"ddp dump (port_id) (config_path)\n"
+			"    Dump a runtime configure on a port\n\n"
+
 			"ddp add (port_id) (profile_path[,backup_profile_path])\n"
 			"    Load a profile package on a port\n\n"
 
@@ -14471,6 +14477,75 @@  cmdline_parse_inst_t cmd_strict_link_prio = {
 	},
 };
 
+/* Dump device ddp package, only for ice PF */
+struct cmd_ddp_dump_result {
+	cmdline_fixed_string_t ddp;
+	cmdline_fixed_string_t add;
+	portid_t port_id;
+	char filepath[];
+};
+
+cmdline_parse_token_string_t cmd_ddp_dump_ddp =
+	TOKEN_STRING_INITIALIZER(struct cmd_ddp_dump_result, ddp, "ddp");
+cmdline_parse_token_string_t cmd_ddp_dump_dump =
+	TOKEN_STRING_INITIALIZER(struct cmd_ddp_dump_result, add, "dump");
+cmdline_parse_token_num_t cmd_ddp_dump_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_ddp_dump_result, port_id, RTE_UINT16);
+cmdline_parse_token_string_t cmd_ddp_dump_filepath =
+	TOKEN_STRING_INITIALIZER(struct cmd_ddp_dump_result, filepath, NULL);
+
+static void
+cmd_ddp_dump_parsed(
+	void *parsed_result,
+	__rte_unused struct cmdline *cl,
+	__rte_unused void *data)
+{
+	struct cmd_ddp_dump_result *res = parsed_result;
+	uint8_t *buff;
+	uint32_t size;
+	int ret = -ENOTSUP;
+
+#define ICE_BUFF_SIZE	0x000c9000
+	size = ICE_BUFF_SIZE;
+	buff = (uint8_t *)malloc(ICE_BUFF_SIZE);
+	if (buff) {
+#ifdef RTE_NET_ICE
+		ret = rte_pmd_ice_dump_package(res->port_id, &buff, &size);
+#endif
+		switch (ret) {
+		case 0:
+			save_file(res->filepath, buff, size);
+			break;
+		case -EINVAL:
+			fprintf(stderr, "Invalid buffer size\n");
+			break;
+		case -ENOTSUP:
+			fprintf(stderr,
+				"Device doesn't support "
+				"dump DDP runtime configure.\n");
+			break;
+		default:
+			fprintf(stderr,
+				"Failed to dump DDP runtime configure,"
+				" error: (%s)\n", strerror(-ret));
+		}
+	}
+	free(buff);
+}
+
+cmdline_parse_inst_t cmd_ddp_dump = {
+	.f = cmd_ddp_dump_parsed,
+	.data = NULL,
+	.help_str = "ddp dump <port_id> <config_path>",
+	.tokens = {
+		(void *)&cmd_ddp_dump_ddp,
+		(void *)&cmd_ddp_dump_dump,
+		(void *)&cmd_ddp_dump_port_id,
+		(void *)&cmd_ddp_dump_filepath,
+		NULL,
+	},
+};
+
 /* Load dynamic device personalization*/
 struct cmd_ddp_add_result {
 	cmdline_fixed_string_t ddp;
@@ -18025,6 +18100,7 @@  cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_ddp_del,
 	(cmdline_parse_inst_t *)&cmd_ddp_get_list,
 	(cmdline_parse_inst_t *)&cmd_ddp_get_info,
+	(cmdline_parse_inst_t *)&cmd_ddp_dump,
 	(cmdline_parse_inst_t *)&cmd_cfg_input_set,
 	(cmdline_parse_inst_t *)&cmd_clear_input_set,
 	(cmdline_parse_inst_t *)&cmd_show_vf_stats,
diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build
index 43130c8856..569e039bf7 100644
--- a/app/test-pmd/meson.build
+++ b/app/test-pmd/meson.build
@@ -67,6 +67,9 @@  endif
 if dpdk_conf.has('RTE_NET_I40E')
     deps += 'net_i40e'
 endif
+if dpdk_conf.has('RTE_NET_ICE')
+    deps += 'net_ice'
+endif
 if dpdk_conf.has('RTE_NET_IXGBE')
     deps += 'net_ixgbe'
 endif