Re: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor

[Dekel Peled <dekelp@mellanox.com>]

PSB minor comments.

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Viacheslav Ovsiienko
> Sent: Tuesday, April 16, 2019 5:10 PM
> To: dev@dpdk.org
> Cc: Shahaf Shuler <shahafs@mellanox.com>
> Subject: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor
> 
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of ARM.
> The traffic sent by the external host to the NIC via PF will be seem by ARM on
> this PF representor.
> 
> This patch refactors port recognizing capability on the base of physical port
> name. We have two groups of name formats. Legacy name formats are
> supported by kernels before ver 5.0 (being more precise - before the patch
> [1]) or before Mellanox OFED 4.6, and new naming formats added by the
> patch [1].
> 
> Legacy naming formats are supported:
> 
>   - missing physical port name (no sysfs/netlink key) at all,
>     master is assumed
> 
>   - decimal digits (for example "12"), representor is assumed,
>     the value is the index of attached VF
> 
> New naming formats are supported:
> 
>   - "p" followed by decimal digits, for example "p2", master
>     is assumed
> 
>   - "pf" followed by PF index concatenated with "vf" followed by
>     VF index, for example "pf0vf1", representor is assumed.
>     If index of VF is "-1" it is a special case  of host PF
>     representor, this representor must be indexed in devargs
>     as 65535, for example representor=[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and for host PF.
> 
>     Note: do not specify representor=[0-65535], it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
> 
> Applications should distinguish representors and master devices exclusively
> by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on switch
> port_id (mlx5 PMD deduces ones from representor_id) values returned by
> dev_infos_get() API.
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> .spinics.net%2Flists%2Fnetdev%2Fmsg547007.html&amp;data=02%7C01%7C
> dekelp%40mellanox.com%7C1bb595beee23451f97ec08d6c275566b%7Ca6529
> 71c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636910206560409734&amp;sd
> ata=hdO9GhbFdoVt4pQND5ROXhTeDBhVhrNA1HyQbchsgvg%3D&amp;rese
> rved=0
>     Linux-tree: c12ecc23 (Or Gerlitz 2018-04-25 17:32 +0300)
>     "net/mlx5e: Move to use common phys port names for vport
> representors"
> 

I suggest to use this link instead:
https://github.com/torvalds/linux/commit/c12ecc2

> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> ---
> v2: - clarified commit log message
>     - separated port attributes analyzing routines
>     - port name format related routines are gathered together
> 
> v1:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> es.dpdk.org%2Fpatch%2F52725%2F&amp;data=02%7C01%7Cdekelp%40mell
> anox.com%7C1bb595beee23451f97ec08d6c275566b%7Ca652971c7d2e4d9ba6
> a4d149256f461b%7C0%7C0%7C636910206560409734&amp;sdata=pB9vIdQ4s
> VzLcKNL0flg8SnDP%2F%2BtisSJVCbj1RkkPwg%3D&amp;reserved=0
> 
>  drivers/net/mlx5/mlx5.h        |  18 ++++-
>  drivers/net/mlx5/mlx5_ethdev.c | 163
> ++++++++++++++++++++++++++++++++++-------
>  drivers/net/mlx5/mlx5_nl.c     |  20 +----
>  3 files changed, 158 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 14c7f3c..2069fad 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -80,11 +80,21 @@ struct mlx5_mp_param {
>  /** Key string for IPC. */
>  #define MLX5_MP_NAME "net_mlx5_mp"
> 
> +/* Recognized Infiniband device physical port name types. */ enum
> +mlx5_phys_port_name_type {
> +	MLX5_PHYS_PORT_NAME_TYPE_NOTSET = 0, /* Not set. */
> +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN, /* Unrecognized. */ };
> +
>  /** Switch information returned by mlx5_nl_switch_info(). */  struct
> mlx5_switch_info {
>  	uint32_t master:1; /**< Master device. */
>  	uint32_t representor:1; /**< Representor device. */
> -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> */
> +	enum mlx5_phys_port_name_type name_type; /** < Port name
> type. */
> +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
>  	int32_t port_name; /**< Representor port name. */
>  	uint64_t switch_id; /**< Switch identifier. */  }; @@ -404,7 +414,11
> @@ unsigned int mlx5_dev_to_port_id(const struct rte_device *dev,
>  				 unsigned int port_list_n);
>  int mlx5_sysfs_switch_info(unsigned int ifindex,
>  			   struct mlx5_switch_info *info);
> -bool mlx5_translate_port_name(const char *port_name_in,
> +void mlx5_sysfs_check_switch_info(bool device_dir,
> +				  struct mlx5_switch_info *switch_info); void
> +mlx5_nl_check_switch_info(bool nun_vf_set,
> +			       struct mlx5_switch_info *switch_info); void
> +mlx5_translate_port_name(const char *port_name_in,
>  			      struct mlx5_switch_info *port_info_out);
> 
>  /* mlx5_mac.c */
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c index 3992918..9c24462 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  	struct mlx5_switch_info data = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	DIR *dir;
> -	bool port_name_set = false;
>  	bool port_switch_id_set = false;
>  	bool device_dir = false;
>  	char c;
> @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev,
> char *fw_ver, size_t fw_size)
>  		ret = fscanf(file, "%s", port_name);
>  		fclose(file);
>  		if (ret == 1)
> -			port_name_set =
> mlx5_translate_port_name(port_name,
> -								 &data);
> +			mlx5_translate_port_name(port_name, &data);
>  	}
>  	file = fopen(phys_switch_id, "rb");
>  	if (file == NULL) {
> @@ -1440,8 +1438,10 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  		closedir(dir);
>  		device_dir = true;
>  	}
> -	data.master = port_switch_id_set && (!port_name_set ||
> device_dir);
> -	data.representor = port_switch_id_set && port_name_set &&
> !device_dir;
> +	if (port_switch_id_set) {
> +		/* We have some E-Switch configuration. */
> +		mlx5_sysfs_check_switch_info(device_dir, &data);
> +	}
>  	*info = data;
>  	assert(!(data.master && data.representor));
>  	if (data.master && data.representor) { @@ -1454,41 +1454,154 @@
> int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> fw_size)  }
> 
>  /**
> + * Analyze gathered port parameters via Netlink to recognize master
> + * and representor devices for E-Switch configuration.
> + *
> + * @param[in] num_vf_set
> + *   flag of presence of number of VFs port attribute.
> + * @param[inout] switch_info
> + *   Port information, including port name as a number and port name
> + *   type if recognized
> + *
> + * @return
> + *   master and representor flags are set in switch_info according to
> + *   recognized parameters (if any).
> + */
> +void
> +mlx5_nl_check_switch_info(bool num_vf_set,
> +			  struct mlx5_switch_info *switch_info) {
> +	switch (switch_info->name_type) {
> +	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +		/*
> +		 * Name is not recognized, assume the master,
> +		 * check the number of VFs key presence.
> +		 */
> +		switch_info->master = num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
> +		/*
> +		 * Name is not set, this assumes the legacy naming
> +		 * schema for master, just check if there is a
> +		 * number of VFs key.
> +		 */
> +		switch_info->master = num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +		/* New uplink naming schema recognized. */
> +		switch_info->master = 1;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
> +		/* Legacy representors naming schema. */
> +		switch_info->representor = !num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +		/* New representors naming schema. */
> +		switch_info->representor = 1;
> +		break;

Suggest to add default for clarity.

> +	}
> +}
> +
> +/**
> + * Analyze gathered port parameters via sysfs to recognize master
> + * and representor devices for E-Switch configuration.
> + *
> + * @param[in] device_dir
> + *   flag of presence of "device" directory under port device key.
> + * @param[inout] switch_info
> + *   Port information, including port name as a number and port name
> + *   type if recognized
> + *
> + * @return
> + *   master and representor flags are set in switch_info according to
> + *   recognized parameters (if any).
> + */
> +void
> +mlx5_sysfs_check_switch_info(bool device_dir,
> +			     struct mlx5_switch_info *switch_info) {
> +	switch (switch_info->name_type) {
> +	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +		/*
> +		 * Name is not recognized, assume the master,
> +		 * check the device directory presence.
> +		 */
> +		switch_info->master = device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
> +		/*
> +		 * Name is not set, this assumes the legacy naming
> +		 * schema for master, just check if there is
> +		 * a device directory.
> +		 */
> +		switch_info->master = device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +		/* New uplink naming schema recognized. */
> +		switch_info->master = 1;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
> +		/* Legacy representors naming schema. */
> +		switch_info->representor = !device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +		/* New representors naming schema. */
> +		switch_info->representor = 1;
> +		break;

Suggest to add default for clarity.

> +	}
> +}
> +
> +/**
>   * Extract port name, as a number, from sysfs or netlink information.
>   *
>   * @param[in] port_name_in
>   *   String representing the port name.
>   * @param[out] port_info_out
> - *   Port information, including port name as a number.
> + *   Port information, including port name as a number and port name
> + *   type if recognized
>   *
>   * @return
> - *   true on success, false otherwise.
> + *   port_name field set according to recognized name format.
>   */
> -bool
> +void
>  mlx5_translate_port_name(const char *port_name_in,
>  			 struct mlx5_switch_info *port_info_out)  {
>  	char pf_c1, pf_c2, vf_c1, vf_c2;
>  	char *end;
> -	int32_t pf_num;
> -	bool port_name_set = false;
> +	int sc_items;
> 
>  	/*
>  	 * Check for port-name as a string of the form pf0vf0
> -	 * (support kernel ver >= 5.0)
> +	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
>  	 */
> -	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d",
> &pf_c1, &pf_c2,
> -				&pf_num, &vf_c1, &vf_c2,
> -				&port_info_out->port_name) == 6);
> -	if (port_name_set) {
> -		port_info_out->port_name_new = 1;
> -	} else {
> -		/* Check for port-name as a number (support kernel ver <
> 5.0 */
> -		errno = 0;
> -		port_info_out->port_name = strtol(port_name_in, &end, 0);
> -		if (!errno &&
> -		    (size_t)(end - port_name_in) == strlen(port_name_in))
> -			port_name_set = true;
> +	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
> +			  &pf_c1, &pf_c2, &port_info_out->pf_num,
> +			  &vf_c1, &vf_c2, &port_info_out->port_name);
> +	if (sc_items == 6 &&
> +	    pf_c1 == 'p' && pf_c2 == 'f' &&
> +	    vf_c1 == 'v' && vf_c2 == 'f') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> +		return;
> +	}
> +	/*
> +	 * Check for port-name as a string of the form p0
> +	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
> +	 */
> +	sc_items = sscanf(port_name_in, "%c%d",
> +			  &pf_c1, &port_info_out->port_name);
> +	if (sc_items == 2 && pf_c1 == 'p') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		return;
> +	}
> +	/* Check for port-name as a number (support kernel ver < 5.0 */
> +	errno = 0;
> +	port_info_out->port_name = strtol(port_name_in, &end, 0);
> +	if (!errno &&
> +	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> +		return;
>  	}
> -	return port_name_set;
> +	port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> +	return;
>  }
> diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c index
> fd9226b..0ff9667 100644
> --- a/drivers/net/mlx5/mlx5_nl.c
> +++ b/drivers/net/mlx5/mlx5_nl.c
> @@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
>  	struct mlx5_switch_info info = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -	bool port_name_set = false;
>  	bool switch_id_set = false;
>  	bool num_vf_set = false;
> 
> @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
>  			num_vf_set = true;
>  			break;
>  		case IFLA_PHYS_PORT_NAME:
> -			port_name_set =
> -				mlx5_translate_port_name((char *)payload,
> -							 &info);
> +			mlx5_translate_port_name((char *)payload, &info);
>  			break;
>  		case IFLA_PHYS_SWITCH_ID:
>  			info.switch_id = 0;
> @@ -926,17 +923,8 @@ struct mlx5_nl_ifindex_data {
>  		off += RTA_ALIGN(ra->rta_len);
>  	}
>  	if (switch_id_set) {
> -		if (info.port_name_new) {
> -			/* New representors naming schema. */
> -			if (port_name_set) {
> -				info.master = (info.port_name == -1);
> -				info.representor = (info.port_name != -1);
> -			}
> -		} else {
> -			/* Legacy representors naming schema. */
> -			info.master = (!port_name_set || num_vf_set);
> -			info.representor = port_name_set && !num_vf_set;
> -		}
> +		/* We have some E-Switch configuration. */
> +		mlx5_nl_check_switch_info(num_vf_set, &info);
>  	}
>  	assert(!(info.master && info.representor));
>  	memcpy(arg, &info, sizeof(info));
> --
> 1.8.3.1