Re: [PATCH] net/ice: support for more flexible loading of DDP package

[Bruce Richardson <bruce.richardson@intel.com>]

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
>