Re: [dpdk-dev] [PATCH v1] net/ice: support device-specific DDP package loading

["Stillwell Jr, Paul M" <paul.m.stillwell.jr@intel.com>]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ting Xu
> Sent: Thursday, August 29, 2019 6:18 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v1] net/ice: support device-specific DDP package
> loading
> 
> This patch adds the feature that supports loading DDP package according to
> the device serial number. Prior to loading the default DDP package (ice.pkg),
> the driver will check for the presence of a device-specific DDP package with
> the name containing 64-bit PCIe Device Serial Number (ice-
> xxxxxxxxxxxxxxxx.pkg) during initialization. Users can use "lspci -vs" to get
> the device serial number.
> The pkg search path are /lib/firmware/intel/ice/ddp/ and
> /lib/firmware/updates/intel/ice/ddp/. If the package exists, the driver will
> download it to the device instead of the default one. The loaded package
> type (OS default and COMMS) will be stored in ice_adapter-
> >active_pkg_type. The package version is stored in ice_hw->active_pkg_ver.
> 
> Signed-off-by: Ting Xu <ting.xu@intel.com>
> ---
>  drivers/net/ice/ice_ethdev.c | 132
> ++++++++++++++++++++++++++++++++++-
>  drivers/net/ice/ice_ethdev.h |   8 +++
>  2 files changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 686d6f00f..92eafdd1f 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -28,7 +28,15 @@ static const char * const ice_valid_args[] = {  };
> 
>  #define ICE_DFLT_OUTER_TAG_TYPE ICE_AQ_VSI_OUTER_TAG_VLAN_9100
> +
> +/* DDP package search path */
>  #define ICE_DFLT_PKG_FILE "/lib/firmware/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_OS_DEFAULT_PKG_NAME		"ICE OS Default
> Package"
> +#define ICE_COMMS_PKG_NAME			"ICE COMMS
> Package"
> +#define ICE_MAX_PKG_FILENAME_SIZE   256
> 
>  int ice_logtype_init;
>  int ice_logtype_driver;
> @@ -1265,15 +1273,134 @@ ice_pf_setup(struct ice_pf *pf)
>  	return 0;
>  }
> 
> +/* PCIe configuration space setting */
> +#define PCI_CFG_SPACE_SIZE          256
> +#define PCI_CFG_SPACE_EXP_SIZE      4096
> +#define PCI_EXT_CAP_ID(header)      (int)((header) & 0x0000ffff)
> +#define PCI_EXT_CAP_NEXT(header)    (((header) >> 20) & 0xffc)
> +#define PCI_EXT_CAP_ID_DSN          0x03
> +
> +static int
> +ice_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap) {
> +	uint32_t header;
> +	int ttl;
> +	int pos = PCI_CFG_SPACE_SIZE;
> +
> +	/* minimum 8 bytes per capability */
> +	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> +
> +	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> +		PMD_INIT_LOG(ERR, "ice error reading extended
> capabilities\n");
> +		return -1;
> +	}
> +
> +	/*
> +	 * If we have no capabilities, this is indicated by cap ID,
> +	 * cap version and next pointer all being 0.
> +	 */
> +	if (header == 0)
> +		return 0;
> +
> +	while (ttl-- > 0) {
> +		if (PCI_EXT_CAP_ID(header) == cap)
> +			return pos;
> +
> +		pos = PCI_EXT_CAP_NEXT(header);
> +
> +		if (pos < PCI_CFG_SPACE_SIZE)
> +			break;
> +
> +		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> +			PMD_INIT_LOG(ERR, "ice error reading extended
> capabilities\n");
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/* Extract device serial number from PCIe Configuration Space and
> + * determine the pkg file path according to the DSN.
> + */
> +static int
> +ice_pkg_file_search_path(struct rte_pci_device *pci_dev, char
> +*pkg_file) {
> +	int pos;
> +	char opt_ddp_filename[ICE_MAX_PKG_FILENAME_SIZE];
> +	uint32_t dword;
> +	uint32_t dsn_low, dsn_high;
> +
> +	pos = ice_pci_find_next_ext_capability(pci_dev,
> PCI_EXT_CAP_ID_DSN);
> +
> +	if (pos) {
> +		rte_pci_read_config(pci_dev, &dword, 4, pos + 4);
> +		dsn_low = dword;
> +		rte_pci_read_config(pci_dev, &dword, 4, pos + 8);
> +		dsn_high = dword;
> +		snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
> +			 "ice-%08x%08x.pkg", dsn_high, dsn_low);
> +	} else {
> +		PMD_INIT_LOG(INFO, "Failed to read device serial
> number\n");
> +		strncpy(pkg_file, ICE_DFLT_PKG_FILE,
> ICE_MAX_PKG_FILENAME_SIZE);
> +
> +		return 0;
> +	}
> +
> +	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
> +		ICE_MAX_PKG_FILENAME_SIZE);
> +	if (!access(strncat(pkg_file, opt_ddp_filename,
> +		ICE_MAX_PKG_FILENAME_SIZE), 0))
> +		return 0;
> +
> +	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
> +		ICE_MAX_PKG_FILENAME_SIZE);
> +	if (!access(strncat(pkg_file, opt_ddp_filename,
> +		ICE_MAX_PKG_FILENAME_SIZE), 0))
> +		return 0;
> +

The above search order is not correct. You should search the updates path first and then the default path. That is the order that the kernel interfaces search them.

> +	strncpy(pkg_file, ICE_DFLT_PKG_FILE,
> ICE_MAX_PKG_FILENAME_SIZE);
> +
> +	return 0;
> +}
> +
> +static enum ice_pkg_type
> +ice_get_pkg_type(struct ice_hw *hw)
> +{
> +	enum ice_pkg_type package_type;
> +
> +	/* store the activated package type (OS default or Comms) */
> +	if (!strncmp((char *)hw->active_pkg_name,
> ICE_OS_DEFAULT_PKG_NAME,
> +		ICE_PKG_NAME_SIZE))
> +		package_type = ICE_PKG_TYPE_OS_DEFAULT;
> +	else if (!strncmp((char *)hw->active_pkg_name,
> ICE_COMMS_PKG_NAME,
> +		ICE_PKG_NAME_SIZE))
> +		package_type = ICE_PKG_TYPE_COMMS;
> +	else
> +		package_type = ICE_PKG_TYPE_UNKNOWN;
> +
> +	PMD_INIT_LOG(INFO, "Activated package is: %d.%d.%d.%d, %s\n",

Activated => Active

> +		hw->active_pkg_ver.major, hw->active_pkg_ver.minor,
> +		hw->active_pkg_ver.update, hw->active_pkg_ver.draft,
> +		hw->active_pkg_name);
> +
> +	return package_type;
> +}
> +
>  static int ice_load_pkg(struct rte_eth_dev *dev)  {
>  	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> -	const char *pkg_file = ICE_DFLT_PKG_FILE;
> +	char pkg_file[ICE_MAX_PKG_FILENAME_SIZE];
>  	int err;
>  	uint8_t *buf;
>  	int buf_len;
>  	FILE *file;
>  	struct stat fstat;
> +	struct rte_pci_device *pci_dev = RTE_DEV_TO_PCI(dev->device);
> +	struct ice_adapter *ad =
> +		ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +
> +	ice_pkg_file_search_path(pci_dev, pkg_file);
> 
>  	file = fopen(pkg_file, "rb");
>  	if (!file)  {
> @@ -1313,6 +1440,9 @@ static int ice_load_pkg(struct rte_eth_dev *dev)
>  		PMD_INIT_LOG(ERR, "ice_copy_and_init_hw failed: %d\n",
> err);
>  		goto fail_exit;
>  	}
> +
> +	ad->active_pkg_type = ice_get_pkg_type(hw);
> +

How does active_pkg_type get used? What is the purpose?

>  	err = ice_init_hw_tbls(hw);
>  	if (err) {
>  		PMD_INIT_LOG(ERR, "ice_init_hw_tbls failed: %d\n", err);
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> index 5063960a8..d1d07641d 100644
> --- a/drivers/net/ice/ice_ethdev.h
> +++ b/drivers/net/ice/ice_ethdev.h
> @@ -124,6 +124,13 @@
>  #define ICE_ETH_OVERHEAD \
>  	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN +
> ICE_VLAN_TAG_SIZE * 2)
> 
> +/* DDP package type */
> +enum ice_pkg_type {
> +	ICE_PKG_TYPE_UNKNOWN,
> +	ICE_PKG_TYPE_OS_DEFAULT,
> +	ICE_PKG_TYPE_COMMS,
> +};
> +
>  struct ice_adapter;
> 
>  /**
> @@ -296,6 +303,7 @@ struct ice_adapter {
>  	uint32_t ptype_tbl[ICE_MAX_PKT_TYPE] __rte_cache_min_aligned;
>  	bool is_safe_mode;
>  	struct ice_devargs devargs;
> +	enum ice_pkg_type active_pkg_type; /* loaded ddp package type */
>  };
> 
>  struct ice_vsi_vlan_pvid_info {
> --
> 2.17.1