net/ice: support for more flexible loading of DDP package

Message ID 20240828035335.1069153-1-zhichaox.zeng@intel.com (mailing list archive)
State Superseded
Delegated to: Bruce Richardson
Headers
Series net/ice: support for more flexible loading of DDP package |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build success github build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Zeng, ZhichaoX Aug. 28, 2024, 3:53 a.m. UTC
The "Dynamic Device Personalization" package is loaded at initialization
time by the driver, but the specific package file loaded depends upon
what package file is found first by searching through a hard-coded list
of firmware paths.

To enable greater control over the package loading, this commit two ways
to support custom DDP packages:
1. Add device option to choose a specific DDP package file to load.
   For example:
   -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
2. Read firmware search path from
   "/sys/module/firmware_class/parameters/path" like the kernel behavior.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
---
 doc/guides/nics/ice.rst      | 12 +++++++
 drivers/net/ice/ice_ethdev.c | 61 ++++++++++++++++++++++++++++++++++++
 drivers/net/ice/ice_ethdev.h |  2 ++
 3 files changed, 75 insertions(+)
  

Comments

Bruce Richardson Aug. 28, 2024, 7:55 a.m. UTC | #1
On Wed, Aug 28, 2024 at 11:53:35AM +0800, Zhichao Zeng wrote:
> The "Dynamic Device Personalization" package is loaded at initialization
> time by the driver, but the specific package file loaded depends upon
> what package file is found first by searching through a hard-coded list
> of firmware paths.
> 
> To enable greater control over the package loading, this commit two ways
> to support custom DDP packages:
> 1. Add device option to choose a specific DDP package file to load.
>    For example:
>    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
> 2. Read firmware search path from
>    "/sys/module/firmware_class/parameters/path" like the kernel behavior.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

Hi Zhichao,

since there are two different methods being supported for picking a DDP
package this patch would be better split into two, one for each method
added.

The support for #1 above is already on-list as a standalone patch[1], so you
really only need to do a new patch for #2 above. However, I'm ok for you to
take my patch and include it in a 2-patch set for this if you prefer, since
both patches will be related to choosing a DDP file. I'll leave it up to
you whether v2 is a single patch for the search path, or a 2-patch set
including [1].

Regards,
/Bruce

[1] https://patches.dpdk.org/project/dpdk/patch/20240812152815.1132697-5-bruce.richardson@intel.com/

> ---
>  doc/guides/nics/ice.rst      | 12 +++++++
>  drivers/net/ice/ice_ethdev.c | 61 ++++++++++++++++++++++++++++++++++++
>  drivers/net/ice/ice_ethdev.h |  2 ++
>  3 files changed, 75 insertions(+)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..0484fafbc1 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -108,6 +108,18 @@ Runtime Configuration
>  
>      -a 80:00.0,default-mac-disable=1
>  
> +- ``DDP Package File``
> +
> +  Rather than have the driver search for the DDP package to load,
> +  or to override what package is used,
> +  the ``ddp_pkg_file`` option can be used to provide the path to a specific package file.
> +  For example::
> +
> +    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
> +
> +  There is also support for customizing the firmware search path, will read the search path
> +  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
> +
>  - ``Protocol extraction for per queue``
>  
>    Configure the RX queues to do protocol extraction into mbuf for protocol
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 304f959b7e..bd78c14000 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -36,6 +36,7 @@
>  #define ICE_ONE_PPS_OUT_ARG       "pps_out"
>  #define ICE_RX_LOW_LATENCY_ARG    "rx_low_latency"
>  #define ICE_MBUF_CHECK_ARG       "mbuf_check"
> +#define ICE_DDP_FILENAME          "ddp_pkg_file"
>  
>  #define ICE_CYCLECOUNTER_MASK  0xffffffffffffffffULL
>  
> @@ -52,6 +53,7 @@ static const char * const ice_valid_args[] = {
>  	ICE_RX_LOW_LATENCY_ARG,
>  	ICE_DEFAULT_MAC_DISABLE,
>  	ICE_MBUF_CHECK_ARG,
> +	ICE_DDP_FILENAME,
>  	NULL
>  };
>  
> @@ -692,6 +694,18 @@ handle_field_name_arg(__rte_unused const char *key, const char *value,
>  	return 0;
>  }
>  
> +static int
> +handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *name_args)
> +{
> +	const char **filename = name_args;
> +	if (strlen(value) >= ICE_MAX_PKG_FILENAME_SIZE) {
> +		PMD_DRV_LOG(ERR, "The DDP package filename is too long : '%s'", value);
> +		return -1;
> +	}
> +	*filename = strdup(value);
> +	return 0;
> +}
> +
>  static void
>  ice_check_proto_xtr_support(struct ice_hw *hw)
>  {
> @@ -1873,6 +1887,22 @@ ice_load_pkg_type(struct ice_hw *hw)
>  	return package_type;
>  }
>  
> +static int ice_read_customized_path(char *pkg_file)
> +{
> +	char buf[ICE_MAX_PKG_FILENAME_SIZE];
> +	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
> +	if (fp == NULL) {
> +		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");
> +		return -EIO;
> +	}
> +	if (fscanf(fp, "%s\n", buf) > 0)
> +		strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);
> +	else
> +		return -EIO;
> +
> +	return 0;
> +}
> +
>  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  {
>  	struct ice_hw *hw = &adapter->hw;
> @@ -1882,12 +1912,28 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  	size_t bufsz;
>  	int err;
>  
> +	if (adapter->devargs.ddp_filename != NULL) {
> +		strlcpy(pkg_file, adapter->devargs.ddp_filename, sizeof(pkg_file));
> +		if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0) {
> +			goto load_fw;
> +		} else {
> +			PMD_INIT_LOG(ERR, "Cannot load DDP file: %s\n", pkg_file);
> +			return -1;
> +		}
> +	}
> +
>  	if (!use_dsn)
>  		goto no_dsn;
>  
>  	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
>  	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
>  		"ice-%016" PRIx64 ".pkg", dsn);
> +
> +	ice_read_customized_path(pkg_file);
> +	strcat(pkg_file, opt_ddp_filename);
> +	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +		goto load_fw;
> +
>  	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
>  		ICE_MAX_PKG_FILENAME_SIZE);
>  	strcat(pkg_file, opt_ddp_filename);
> @@ -1901,6 +1947,10 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  		goto load_fw;
>  
>  no_dsn:
> +	ice_read_customized_path(pkg_file);
> +	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +		goto load_fw;
> +
>  	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
>  	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
>  		goto load_fw;
> @@ -2217,6 +2267,14 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
>  	ret = rte_kvargs_process(kvlist, ICE_RX_LOW_LATENCY_ARG,
>  				 &parse_bool, &ad->devargs.rx_low_latency);
>  
> +	if (ret)
> +		goto bail;
> +
> +	ret = rte_kvargs_process(kvlist, ICE_DDP_FILENAME,
> +				 &handle_ddp_filename_arg, &ad->devargs.ddp_filename);
> +	if (ret)
> +		goto bail;
> +
>  bail:
>  	rte_kvargs_free(kvlist);
>  	return ret;
> @@ -2689,6 +2747,8 @@ ice_dev_close(struct rte_eth_dev *dev)
>  	ice_free_hw_tbls(hw);
>  	rte_free(hw->port_info);
>  	hw->port_info = NULL;
> +	free((void *)(uintptr_t)ad->devargs.ddp_filename);
> +	ad->devargs.ddp_filename = NULL;
>  	ice_shutdown_all_ctrlq(hw, true);
>  	rte_free(pf->proto_xtr);
>  	pf->proto_xtr = NULL;
> @@ -6981,6 +7041,7 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
>  			      ICE_PROTO_XTR_ARG "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp|ip_offset>"
>  			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
>  			      ICE_DEFAULT_MAC_DISABLE "=<0|1>"
> +				  ICE_DDP_FILENAME "=</path/to/file>"
>  			      ICE_RX_LOW_LATENCY_ARG "=<0|1>");
>  
>  RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> index 3ea9f37dc8..2781362d04 100644
> --- a/drivers/net/ice/ice_ethdev.h
> +++ b/drivers/net/ice/ice_ethdev.h
> @@ -51,6 +51,7 @@
>  #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
>  #define ICE_PKG_FILE_SEARCH_PATH_DEFAULT "/lib/firmware/intel/ice/ddp/"
>  #define ICE_PKG_FILE_SEARCH_PATH_UPDATES "/lib/firmware/updates/intel/ice/ddp/"
> +#define ICE_PKG_FILE_CUSTOMIZED_PATH "/sys/module/firmware_class/parameters/path"
>  #define ICE_MAX_PKG_FILENAME_SIZE   256
>  
>  #define MAX_ACL_NORMAL_ENTRIES    256
> @@ -568,6 +569,7 @@ struct ice_devargs {
>  	/* Name of the field. */
>  	char xtr_field_name[RTE_MBUF_DYN_NAMESIZE];
>  	uint64_t mbuf_check;
> +	const char *ddp_filename;
>  };
>  
>  /**
> -- 
> 2.34.1
>
  
Zeng, ZhichaoX Aug. 28, 2024, 8:53 a.m. UTC | #2
> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Wednesday, August 28, 2024 3:55 PM
> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/ice: support for more flexible loading of DDP package
> 
> On Wed, Aug 28, 2024 at 11:53:35AM +0800, Zhichao Zeng wrote:
> > The "Dynamic Device Personalization" package is loaded at
> > initialization time by the driver, but the specific package file
> > loaded depends upon what package file is found first by searching
> > through a hard-coded list of firmware paths.
> >
> > To enable greater control over the package loading, this commit two
> > ways to support custom DDP packages:
> > 1. Add device option to choose a specific DDP package file to load.
> >    For example:
> >    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
> > 2. Read firmware search path from
> >    "/sys/module/firmware_class/parameters/path" like the kernel behavior.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> Hi Zhichao,
> 
> since there are two different methods being supported for picking a DDP
> package this patch would be better split into two, one for each method added.
> 
> The support for #1 above is already on-list as a standalone patch[1], so you
> really only need to do a new patch for #2 above. However, I'm ok for you to
> take my patch and include it in a 2-patch set for this if you prefer, since both
> patches will be related to choosing a DDP file. I'll leave it up to you whether v2
> is a single patch for the search path, or a 2-patch set including [1].
> 
> Regards,
> /Bruce
> 
> [1]
> https://patches.dpdk.org/project/dpdk/patch/20240812152815.1132697-
> 5-bruce.richardson@intel.com/
> 
Hi Bruce,

Thanks for your comments, sorry I didn't check the patchwork and didn't notice that #1 had been submitted, I'll rework the patch for #2 separately, thanks.

Regards
Zhichao
> > ---
> >  doc/guides/nics/ice.rst      | 12 +++++++
> >  drivers/net/ice/ice_ethdev.c | 61
> > ++++++++++++++++++++++++++++++++++++
> >  drivers/net/ice/ice_ethdev.h |  2 ++
> >  3 files changed, 75 insertions(+)
> >
<Snip>
  

Patch

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index ae975d19ad..0484fafbc1 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -108,6 +108,18 @@  Runtime Configuration
 
     -a 80:00.0,default-mac-disable=1
 
+- ``DDP Package File``
+
+  Rather than have the driver search for the DDP package to load,
+  or to override what package is used,
+  the ``ddp_pkg_file`` option can be used to provide the path to a specific package file.
+  For example::
+
+    -a 80:00.0,ddp_pkg_file=/path/to/ice-version.pkg
+
+  There is also support for customizing the firmware search path, will read the search path
+  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
+
 - ``Protocol extraction for per queue``
 
   Configure the RX queues to do protocol extraction into mbuf for protocol
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 304f959b7e..bd78c14000 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -36,6 +36,7 @@ 
 #define ICE_ONE_PPS_OUT_ARG       "pps_out"
 #define ICE_RX_LOW_LATENCY_ARG    "rx_low_latency"
 #define ICE_MBUF_CHECK_ARG       "mbuf_check"
+#define ICE_DDP_FILENAME          "ddp_pkg_file"
 
 #define ICE_CYCLECOUNTER_MASK  0xffffffffffffffffULL
 
@@ -52,6 +53,7 @@  static const char * const ice_valid_args[] = {
 	ICE_RX_LOW_LATENCY_ARG,
 	ICE_DEFAULT_MAC_DISABLE,
 	ICE_MBUF_CHECK_ARG,
+	ICE_DDP_FILENAME,
 	NULL
 };
 
@@ -692,6 +694,18 @@  handle_field_name_arg(__rte_unused const char *key, const char *value,
 	return 0;
 }
 
+static int
+handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *name_args)
+{
+	const char **filename = name_args;
+	if (strlen(value) >= ICE_MAX_PKG_FILENAME_SIZE) {
+		PMD_DRV_LOG(ERR, "The DDP package filename is too long : '%s'", value);
+		return -1;
+	}
+	*filename = strdup(value);
+	return 0;
+}
+
 static void
 ice_check_proto_xtr_support(struct ice_hw *hw)
 {
@@ -1873,6 +1887,22 @@  ice_load_pkg_type(struct ice_hw *hw)
 	return package_type;
 }
 
+static int ice_read_customized_path(char *pkg_file)
+{
+	char buf[ICE_MAX_PKG_FILENAME_SIZE];
+	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
+	if (fp == NULL) {
+		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");
+		return -EIO;
+	}
+	if (fscanf(fp, "%s\n", buf) > 0)
+		strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);
+	else
+		return -EIO;
+
+	return 0;
+}
+
 int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 {
 	struct ice_hw *hw = &adapter->hw;
@@ -1882,12 +1912,28 @@  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 	size_t bufsz;
 	int err;
 
+	if (adapter->devargs.ddp_filename != NULL) {
+		strlcpy(pkg_file, adapter->devargs.ddp_filename, sizeof(pkg_file));
+		if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0) {
+			goto load_fw;
+		} else {
+			PMD_INIT_LOG(ERR, "Cannot load DDP file: %s\n", pkg_file);
+			return -1;
+		}
+	}
+
 	if (!use_dsn)
 		goto no_dsn;
 
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
+
+	ice_read_customized_path(pkg_file);
+	strcat(pkg_file, opt_ddp_filename);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
 	strcat(pkg_file, opt_ddp_filename);
@@ -1901,6 +1947,10 @@  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		goto load_fw;
 
 no_dsn:
+	ice_read_customized_path(pkg_file);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
+		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
 	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
@@ -2217,6 +2267,14 @@  static int ice_parse_devargs(struct rte_eth_dev *dev)
 	ret = rte_kvargs_process(kvlist, ICE_RX_LOW_LATENCY_ARG,
 				 &parse_bool, &ad->devargs.rx_low_latency);
 
+	if (ret)
+		goto bail;
+
+	ret = rte_kvargs_process(kvlist, ICE_DDP_FILENAME,
+				 &handle_ddp_filename_arg, &ad->devargs.ddp_filename);
+	if (ret)
+		goto bail;
+
 bail:
 	rte_kvargs_free(kvlist);
 	return ret;
@@ -2689,6 +2747,8 @@  ice_dev_close(struct rte_eth_dev *dev)
 	ice_free_hw_tbls(hw);
 	rte_free(hw->port_info);
 	hw->port_info = NULL;
+	free((void *)(uintptr_t)ad->devargs.ddp_filename);
+	ad->devargs.ddp_filename = NULL;
 	ice_shutdown_all_ctrlq(hw, true);
 	rte_free(pf->proto_xtr);
 	pf->proto_xtr = NULL;
@@ -6981,6 +7041,7 @@  RTE_PMD_REGISTER_PARAM_STRING(net_ice,
 			      ICE_PROTO_XTR_ARG "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp|ip_offset>"
 			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
 			      ICE_DEFAULT_MAC_DISABLE "=<0|1>"
+				  ICE_DDP_FILENAME "=</path/to/file>"
 			      ICE_RX_LOW_LATENCY_ARG "=<0|1>");
 
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
index 3ea9f37dc8..2781362d04 100644
--- a/drivers/net/ice/ice_ethdev.h
+++ b/drivers/net/ice/ice_ethdev.h
@@ -51,6 +51,7 @@ 
 #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
 #define ICE_PKG_FILE_SEARCH_PATH_DEFAULT "/lib/firmware/intel/ice/ddp/"
 #define ICE_PKG_FILE_SEARCH_PATH_UPDATES "/lib/firmware/updates/intel/ice/ddp/"
+#define ICE_PKG_FILE_CUSTOMIZED_PATH "/sys/module/firmware_class/parameters/path"
 #define ICE_MAX_PKG_FILENAME_SIZE   256
 
 #define MAX_ACL_NORMAL_ENTRIES    256
@@ -568,6 +569,7 @@  struct ice_devargs {
 	/* Name of the field. */
 	char xtr_field_name[RTE_MBUF_DYN_NAMESIZE];
 	uint64_t mbuf_check;
+	const char *ddp_filename;
 };
 
 /**