From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <shahafs@mellanox.com>
Received: from EUR01-DB5-obe.outbound.protection.outlook.com
 (mail-eopbgr150042.outbound.protection.outlook.com [40.107.15.42])
 by dpdk.org (Postfix) with ESMTP id CED4C29AC
 for <dev@dpdk.org>; Sun, 14 Apr 2019 09:42:51 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com;
 s=selector1;
 h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;
 bh=bkoS4IjHDuepPqYQHgfsIDwHVIm6TOHyPPUhMHqCFrs=;
 b=ybhTYjfw840AQGFBvJJvJGV+6C937YDABJzp57orlFh8Ab3gS6xz+ZVkDWU3/5E/8XkmcypAk+d9vkWb9qB8GLJnC0avhe+P+z+godA+TqZ5F7W1XcatBRPHDRKaIM4wXdMgTWJ86wZhMWUyUUTL7sH4FKpy18zAD9q+yYiZP00=
Received: from AM0PR0502MB3795.eurprd05.prod.outlook.com (52.133.45.150) by
 AM0PR0502MB3988.eurprd05.prod.outlook.com (52.133.34.142) with Microsoft SMTP
 Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
 15.20.1792.17; Sun, 14 Apr 2019 07:42:50 +0000
Received: from AM0PR0502MB3795.eurprd05.prod.outlook.com
 ([fe80::4192:b468:41e1:c323]) by AM0PR0502MB3795.eurprd05.prod.outlook.com
 ([fe80::4192:b468:41e1:c323%4]) with mapi id 15.20.1792.018; Sun, 14 Apr 2019
 07:42:50 +0000
From: Shahaf Shuler <shahafs@mellanox.com>
To: Slava Ovsiienko <viacheslavo@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>
Thread-Topic: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
Thread-Index: AQHU8Uc1OLN+yNXlCUWNTkvjYSC346Y7QNuw
Date: Sun, 14 Apr 2019 07:42:49 +0000
Message-ID: <AM0PR0502MB379565BC25BE9EBCEEF4DE7EC32A0@AM0PR0502MB3795.eurprd05.prod.outlook.com>
References: <1555084107-24692-1-git-send-email-viacheslavo@mellanox.com>
In-Reply-To: <1555084107-24692-1-git-send-email-viacheslavo@mellanox.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
authentication-results: spf=none (sender IP is )
 smtp.mailfrom=shahafs@mellanox.com; 
x-originating-ip: [31.154.10.105]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 986186e5-de56-4f2c-95d6-08d6c0accb86
x-ms-office365-filtering-ht: Tenant
x-microsoft-antispam: BCL:0; PCL:0;
 RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600139)(711020)(4605104)(4618075)(2017052603328)(7193020);
 SRVR:AM0PR0502MB3988; 
x-ms-traffictypediagnostic: AM0PR0502MB3988:
x-microsoft-antispam-prvs: <AM0PR0502MB39887CD5B7EFF28866DBA77AC32A0@AM0PR0502MB3988.eurprd05.prod.outlook.com>
x-forefront-prvs: 00073DB75F
x-forefront-antispam-report: SFV:NSPM;
 SFS:(10009020)(396003)(346002)(366004)(39850400004)(376002)(136003)(199004)(189003)(14454004)(105586002)(102836004)(7736002)(52536014)(81166006)(186003)(6506007)(81156014)(6246003)(66066001)(478600001)(2501003)(229853002)(74316002)(86362001)(106356001)(68736007)(256004)(33656002)(110136005)(2906002)(8936002)(99286004)(71200400001)(71190400001)(5660300002)(76176011)(6116002)(3846002)(5024004)(305945005)(6436002)(7696005)(316002)(9686003)(55016002)(26005)(8676002)(30864003)(97736004)(476003)(53936002)(486006)(11346002)(446003)(25786009);
 DIR:OUT; SFP:1101; SCL:1; SRVR:AM0PR0502MB3988;
 H:AM0PR0502MB3795.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en;
 PTR:InfoNoRecords; A:1; MX:1; 
received-spf: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam-message-info: JxTep06aBvwJEoHemhzaq6kD69KisOu2BmuByljlqwJpIwxAdkuqwG9FJCMdmz+DJKDY9OyLOufPo7HWU35M9qvqs8ooryEdGipiqQRDMrxdMOSFBEjAZuP8Plt93qsmnCM3/YGl//3KJlAdqCk9nO2e3JhDsYWtzpP4qmnju44bYPXKpAG1hn5pdorngKEQhegmE+Ip7iT876svkeBX0U4t/M82AKpLH0d0yF/JW9guIQP95+zPNgVpZ0NrrzA3UMUa0OnXeoHaPy8wURG3PafgNcdaW+XIV8cOZYIGf43yezDO/p27sb5KEUqLJ+40WNug0aGVbo8Yi8LhuWlrXZeVnwaYzJ1DsVrH4YIfm0wN5rRhKiUxyYscf3TU0TH9AC6MJfYw6DgL2yOJ3s5w3vEgMQxbKf0HHZ2OE9KSvig=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Mellanox.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 986186e5-de56-4f2c-95d6-08d6c0accb86
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Apr 2019 07:42:50.0296 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR0502MB3988
Subject: Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Sun, 14 Apr 2019 07:42:52 -0000

Hi Slava,

Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
>=20
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of A=
RM.
> The traffic sent by the external host to the NIC via PF will be seem by A=
RM on
> this PF representor.
>=20
> This patch extends port recognizing capability on the base of physical po=
rt
> name. The following naming formats are supported:
>=20
>   - missing physical port name (no sysfs/netlink key) at all,
>     this is old style (before kernel 5.0) format, master assumed
>   - 1 (decimal digits) - old style (before kernel 5.0) format,
>     exists only for representors, master does not have physical
>     port name at all (see above)
>   - p0 ("p" followed by decimal digits), new style (kernel version
>     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
>     for uplink representor, plays the role of master
>   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
>     followed by VF index),  new style (kernel version  is 5.0
>     or higher, Mellanox OFED 4.6 or higher) name format for
>     VF representor. 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=3D[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and host PF.
>     Note: do not specify representor=3D[0-65535] it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
>=20

The above is a bit complex to understand and in fact we have 2 modes:
1. legacy - phys_port_name are numbers. Master doesn't have phys_port_name
2. modern - phys_port_name are strings.=20
uplink representor is p%d
representors are (including PF representor) pf%dvf%d. the vf index for the =
PF representor is 65535.=20

> Applications should distinguish representors and master devices exclusive=
ly
> 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.
>=20

Please also reference the kernel commit which introduce the name change.=20

> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.h        | 11 ++++++-
>  drivers/net/mlx5/mlx5_ethdev.c | 68 +++++++++++++++++++++++++++----
> -----------
>  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
>  3 files changed, 82 insertions(+), 39 deletions(-)
>=20
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 8eb1019..81c02ce 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -80,11 +80,20 @@ struct mlx5_mp_param {
>  /** Key string for IPC. */
>  #define MLX5_MP_NAME "net_mlx5_mp"
>=20
> +/* Recognized Infiniband device physical port name types. */ enum
> +mlx5_phys_port_name_type {
> +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN =3D 0, /* Unrecognized.
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >=3D 5.0 */
> +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >=3D 5.0
> */ };
> +
>  /** 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. */  }; diff --git
> a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c index
> 3992918..371989f 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 =3D {
>  		.master =3D 0,
>  		.representor =3D 0,
> -		.port_name_new =3D 0,
> +		.name_type =3D MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
>  		.port_name =3D 0,
>  		.switch_id =3D 0,
>  	};
>  	DIR *dir;
> -	bool port_name_set =3D false;
>  	bool port_switch_id_set =3D false;
>  	bool device_dir =3D 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 =3D fscanf(file, "%s", port_name);
>  		fclose(file);
>  		if (ret =3D=3D 1)
> -			port_name_set =3D
> mlx5_translate_port_name(port_name,
> -								 &data);
> +			mlx5_translate_port_name(port_name, &data);
>  	}
>  	file =3D fopen(phys_switch_id, "rb");
>  	if (file =3D=3D NULL) {
> @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  		closedir(dir);
>  		device_dir =3D true;
>  	}
> -	data.master =3D port_switch_id_set && (!port_name_set ||
> device_dir);
> -	data.representor =3D port_switch_id_set && port_name_set &&
> !device_dir;
> +	if (port_switch_id_set) {
> +		data.master =3D
> +			device_dir ||
> +			data.name_type =3D=3D
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> +			data.name_type =3D=3D
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		data.representor =3D !device_dir &&
> +			(data.name_type =3D=3D
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> +			 data.name_type =3D=3D
> MLX5_PHYS_PORT_NAME_TYPE_PFVF);


Why we need to split the logic of the master/representor detection between =
the mlx5_translate_port_name and the caller function?

The way I envision it is mlx5_tranlate_port_name receives the phys_port_nam=
e and maybe more metadata and return the port classification (master/repres=
entor) and the representor/pf number.=20
No need for data.master =3D some_logic(translate_port_name_info).=20

Inside the translate function I would expect to have 2 smaller function:
1. to handle the modern format (strings)
2. to handle the legacy format (integers)=20

> +	}
>  	*info =3D data;
>  	assert(!(data.master && data.representor));
>  	if (data.master && data.representor) { @@ -1459,10 +1464,11 @@ int
> mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> fw_size)
>   * @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.
> + *   true on success (if name format recognized), false otherwise.
>   */
>  bool
>  mlx5_translate_port_name(const char *port_name_in, @@ -1470,25
> +1476,39 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char
> *fw_ver, size_t fw_size)  {
>  	char pf_c1, pf_c2, vf_c1, vf_c2;
>  	char *end;
> -	int32_t pf_num;
> -	bool port_name_set =3D false;
> +	int sc_items;
>=20
>  	/*
>  	 * Check for port-name as a string of the form pf0vf0
> -	 * (support kernel ver >=3D 5.0)
> +	 * (support kernel ver >=3D 5.0 or OFED ver >=3D 4.6).
>  	 */
> -	port_name_set =3D	(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) =3D=3D 6);
> -	if (port_name_set) {
> -		port_info_out->port_name_new =3D 1;
> -	} else {
> -		/* Check for port-name as a number (support kernel ver <
> 5.0 */
> -		errno =3D 0;
> -		port_info_out->port_name =3D strtol(port_name_in, &end, 0);
> -		if (!errno &&
> -		    (size_t)(end - port_name_in) =3D=3D strlen(port_name_in))
> -			port_name_set =3D true;
> +	sc_items =3D 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 =3D=3D 6 &&
> +	    pf_c1 =3D=3D 'p' && pf_c2 =3D=3D 'f' &&
> +	    vf_c1 =3D=3D 'v' && vf_c2 =3D=3D 'f') {
> +		port_info_out->name_type =3D
> MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> +		return true;
> +	}
> +	/*
> +	 * Check for port-name as a string of the form p0
> +	 * (support kernel ver >=3D 5.0, or OFED ver >=3D 4.6).
> +	 */
> +	sc_items =3D sscanf(port_name_in, "%c%d",
> +			  &pf_c1, &port_info_out->port_name);
> +	if (sc_items =3D=3D 2 && pf_c1 =3D=3D 'p') {
> +		port_info_out->name_type =3D
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		return true;
> +	}
> +	/* Check for port-name as a number (support kernel ver < 5.0 */
> +	errno =3D 0;
> +	port_info_out->port_name =3D strtol(port_name_in, &end, 0);
> +	if (!errno &&
> +	    (size_t)(end - port_name_in) =3D=3D strlen(port_name_in)) {
> +		port_info_out->name_type =3D
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> +		return  true;
>  	}
> -	return port_name_set;
> +	port_info_out->name_type =3D
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> +	return false;
>  }
> diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c inde=
x
> fd9226b..669de76 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 =3D {
>  		.master =3D 0,
>  		.representor =3D 0,
> -		.port_name_new =3D 0,
> +		.name_type =3D MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
>  		.port_name =3D 0,
>  		.switch_id =3D 0,
>  	};
>  	size_t off =3D NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -	bool port_name_set =3D false;
>  	bool switch_id_set =3D false;
>  	bool num_vf_set =3D false;
>=20
> @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
>  			num_vf_set =3D true;
>  			break;
>  		case IFLA_PHYS_PORT_NAME:
> -			port_name_set =3D
> -				mlx5_translate_port_name((char *)payload,
> -							 &info);
> +			mlx5_translate_port_name((char *)payload, &info);
>  			break;
>  		case IFLA_PHYS_SWITCH_ID:
>  			info.switch_id =3D 0;
> @@ -926,16 +923,33 @@ struct mlx5_nl_ifindex_data {
>  		off +=3D RTA_ALIGN(ra->rta_len);
>  	}
>  	if (switch_id_set) {
> -		if (info.port_name_new) {
> -			/* New representors naming schema. */
> -			if (port_name_set) {
> -				info.master =3D (info.port_name =3D=3D -1);
> -				info.representor =3D (info.port_name !=3D -1);
> -			}
> -		} else {
> +		/* We have some E-Switch configuration. */
> +		switch (info.name_type) {
> +		case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +			/*
> +			 * Name is not recognized or not set,
> +			 * it can not be representor, check
> +			 * VF number to see if it is a master.
> +			 */
> +			info.master =3D num_vf_set;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
>  			/* Legacy representors naming schema. */
> -			info.master =3D (!port_name_set || num_vf_set);
> -			info.representor =3D port_name_set && !num_vf_set;
> +			info.representor =3D !num_vf_set;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +			/* New uplink naming schema. */
> +			info.master =3D 1;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +			/* New representors naming schema. */
> +			info.representor =3D 1;
> +			break;
> +		}
> +		if (!info.master && !info.representor) {
> +			DRV_LOG(INFO,
> +				"unable to recognize master/representors"
> +				" on the device in switch domain");

Same comment as above. Would like to avoid this switch case outside of the =
translate function .

>  		}
>  	}
>  	assert(!(info.master && info.representor));
> --
> 1.8.3.1

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by dpdk.space (Postfix) with ESMTP id 21DE2A00E6
	for <public@inbox.dpdk.org>; Sun, 14 Apr 2019 09:42:55 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id DF521325F;
	Sun, 14 Apr 2019 09:42:52 +0200 (CEST)
Received: from EUR01-DB5-obe.outbound.protection.outlook.com
 (mail-eopbgr150042.outbound.protection.outlook.com [40.107.15.42])
 by dpdk.org (Postfix) with ESMTP id CED4C29AC
 for <dev@dpdk.org>; Sun, 14 Apr 2019 09:42:51 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com;
 s=selector1;
 h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;
 bh=bkoS4IjHDuepPqYQHgfsIDwHVIm6TOHyPPUhMHqCFrs=;
 b=ybhTYjfw840AQGFBvJJvJGV+6C937YDABJzp57orlFh8Ab3gS6xz+ZVkDWU3/5E/8XkmcypAk+d9vkWb9qB8GLJnC0avhe+P+z+godA+TqZ5F7W1XcatBRPHDRKaIM4wXdMgTWJ86wZhMWUyUUTL7sH4FKpy18zAD9q+yYiZP00=
Received: from AM0PR0502MB3795.eurprd05.prod.outlook.com (52.133.45.150) by
 AM0PR0502MB3988.eurprd05.prod.outlook.com (52.133.34.142) with Microsoft SMTP
 Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
 15.20.1792.17; Sun, 14 Apr 2019 07:42:50 +0000
Received: from AM0PR0502MB3795.eurprd05.prod.outlook.com
 ([fe80::4192:b468:41e1:c323]) by AM0PR0502MB3795.eurprd05.prod.outlook.com
 ([fe80::4192:b468:41e1:c323%4]) with mapi id 15.20.1792.018; Sun, 14 Apr 2019
 07:42:50 +0000
From: Shahaf Shuler <shahafs@mellanox.com>
To: Slava Ovsiienko <viacheslavo@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>
Thread-Topic: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
Thread-Index: AQHU8Uc1OLN+yNXlCUWNTkvjYSC346Y7QNuw
Date: Sun, 14 Apr 2019 07:42:49 +0000
Message-ID:
 <AM0PR0502MB379565BC25BE9EBCEEF4DE7EC32A0@AM0PR0502MB3795.eurprd05.prod.outlook.com>
References: <1555084107-24692-1-git-send-email-viacheslavo@mellanox.com>
In-Reply-To: <1555084107-24692-1-git-send-email-viacheslavo@mellanox.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
authentication-results: spf=none (sender IP is )
 smtp.mailfrom=shahafs@mellanox.com; 
x-originating-ip: [31.154.10.105]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 986186e5-de56-4f2c-95d6-08d6c0accb86
x-ms-office365-filtering-ht: Tenant
x-microsoft-antispam: BCL:0; PCL:0;
 RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600139)(711020)(4605104)(4618075)(2017052603328)(7193020);
 SRVR:AM0PR0502MB3988; 
x-ms-traffictypediagnostic: AM0PR0502MB3988:
x-microsoft-antispam-prvs: <AM0PR0502MB39887CD5B7EFF28866DBA77AC32A0@AM0PR0502MB3988.eurprd05.prod.outlook.com>
x-forefront-prvs: 00073DB75F
x-forefront-antispam-report: SFV:NSPM;
 SFS:(10009020)(396003)(346002)(366004)(39850400004)(376002)(136003)(199004)(189003)(14454004)(105586002)(102836004)(7736002)(52536014)(81166006)(186003)(6506007)(81156014)(6246003)(66066001)(478600001)(2501003)(229853002)(74316002)(86362001)(106356001)(68736007)(256004)(33656002)(110136005)(2906002)(8936002)(99286004)(71200400001)(71190400001)(5660300002)(76176011)(6116002)(3846002)(5024004)(305945005)(6436002)(7696005)(316002)(9686003)(55016002)(26005)(8676002)(30864003)(97736004)(476003)(53936002)(486006)(11346002)(446003)(25786009);
 DIR:OUT; SFP:1101; SCL:1; SRVR:AM0PR0502MB3988;
 H:AM0PR0502MB3795.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en;
 PTR:InfoNoRecords; A:1; MX:1; 
received-spf: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam-message-info: JxTep06aBvwJEoHemhzaq6kD69KisOu2BmuByljlqwJpIwxAdkuqwG9FJCMdmz+DJKDY9OyLOufPo7HWU35M9qvqs8ooryEdGipiqQRDMrxdMOSFBEjAZuP8Plt93qsmnCM3/YGl//3KJlAdqCk9nO2e3JhDsYWtzpP4qmnju44bYPXKpAG1hn5pdorngKEQhegmE+Ip7iT876svkeBX0U4t/M82AKpLH0d0yF/JW9guIQP95+zPNgVpZ0NrrzA3UMUa0OnXeoHaPy8wURG3PafgNcdaW+XIV8cOZYIGf43yezDO/p27sb5KEUqLJ+40WNug0aGVbo8Yi8LhuWlrXZeVnwaYzJ1DsVrH4YIfm0wN5rRhKiUxyYscf3TU0TH9AC6MJfYw6DgL2yOJ3s5w3vEgMQxbKf0HHZ2OE9KSvig=
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Mellanox.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 986186e5-de56-4f2c-95d6-08d6c0accb86
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Apr 2019 07:42:50.0296 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR0502MB3988
Subject: Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Message-ID: <20190414074249.2CcgOzHNFeMxIOSXosy2R9Zc4BsyBnZ71pBLK3cN-to@z>

Hi Slava,

Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
>=20
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of A=
RM.
> The traffic sent by the external host to the NIC via PF will be seem by A=
RM on
> this PF representor.
>=20
> This patch extends port recognizing capability on the base of physical po=
rt
> name. The following naming formats are supported:
>=20
>   - missing physical port name (no sysfs/netlink key) at all,
>     this is old style (before kernel 5.0) format, master assumed
>   - 1 (decimal digits) - old style (before kernel 5.0) format,
>     exists only for representors, master does not have physical
>     port name at all (see above)
>   - p0 ("p" followed by decimal digits), new style (kernel version
>     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
>     for uplink representor, plays the role of master
>   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
>     followed by VF index),  new style (kernel version  is 5.0
>     or higher, Mellanox OFED 4.6 or higher) name format for
>     VF representor. 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=3D[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and host PF.
>     Note: do not specify representor=3D[0-65535] it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
>=20

The above is a bit complex to understand and in fact we have 2 modes:
1. legacy - phys_port_name are numbers. Master doesn't have phys_port_name
2. modern - phys_port_name are strings.=20
uplink representor is p%d
representors are (including PF representor) pf%dvf%d. the vf index for the =
PF representor is 65535.=20

> Applications should distinguish representors and master devices exclusive=
ly
> 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.
>=20

Please also reference the kernel commit which introduce the name change.=20

> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.h        | 11 ++++++-
>  drivers/net/mlx5/mlx5_ethdev.c | 68 +++++++++++++++++++++++++++----
> -----------
>  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
>  3 files changed, 82 insertions(+), 39 deletions(-)
>=20
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 8eb1019..81c02ce 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -80,11 +80,20 @@ struct mlx5_mp_param {
>  /** Key string for IPC. */
>  #define MLX5_MP_NAME "net_mlx5_mp"
>=20
> +/* Recognized Infiniband device physical port name types. */ enum
> +mlx5_phys_port_name_type {
> +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN =3D 0, /* Unrecognized.
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >=3D 5.0 */
> +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >=3D 5.0
> */ };
> +
>  /** 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. */  }; diff --git
> a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c index
> 3992918..371989f 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 =3D {
>  		.master =3D 0,
>  		.representor =3D 0,
> -		.port_name_new =3D 0,
> +		.name_type =3D MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
>  		.port_name =3D 0,
>  		.switch_id =3D 0,
>  	};
>  	DIR *dir;
> -	bool port_name_set =3D false;
>  	bool port_switch_id_set =3D false;
>  	bool device_dir =3D 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 =3D fscanf(file, "%s", port_name);
>  		fclose(file);
>  		if (ret =3D=3D 1)
> -			port_name_set =3D
> mlx5_translate_port_name(port_name,
> -								 &data);
> +			mlx5_translate_port_name(port_name, &data);
>  	}
>  	file =3D fopen(phys_switch_id, "rb");
>  	if (file =3D=3D NULL) {
> @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  		closedir(dir);
>  		device_dir =3D true;
>  	}
> -	data.master =3D port_switch_id_set && (!port_name_set ||
> device_dir);
> -	data.representor =3D port_switch_id_set && port_name_set &&
> !device_dir;
> +	if (port_switch_id_set) {
> +		data.master =3D
> +			device_dir ||
> +			data.name_type =3D=3D
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> +			data.name_type =3D=3D
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		data.representor =3D !device_dir &&
> +			(data.name_type =3D=3D
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> +			 data.name_type =3D=3D
> MLX5_PHYS_PORT_NAME_TYPE_PFVF);


Why we need to split the logic of the master/representor detection between =
the mlx5_translate_port_name and the caller function?

The way I envision it is mlx5_tranlate_port_name receives the phys_port_nam=
e and maybe more metadata and return the port classification (master/repres=
entor) and the representor/pf number.=20
No need for data.master =3D some_logic(translate_port_name_info).=20

Inside the translate function I would expect to have 2 smaller function:
1. to handle the modern format (strings)
2. to handle the legacy format (integers)=20

> +	}
>  	*info =3D data;
>  	assert(!(data.master && data.representor));
>  	if (data.master && data.representor) { @@ -1459,10 +1464,11 @@ int
> mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> fw_size)
>   * @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.
> + *   true on success (if name format recognized), false otherwise.
>   */
>  bool
>  mlx5_translate_port_name(const char *port_name_in, @@ -1470,25
> +1476,39 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char
> *fw_ver, size_t fw_size)  {
>  	char pf_c1, pf_c2, vf_c1, vf_c2;
>  	char *end;
> -	int32_t pf_num;
> -	bool port_name_set =3D false;
> +	int sc_items;
>=20
>  	/*
>  	 * Check for port-name as a string of the form pf0vf0
> -	 * (support kernel ver >=3D 5.0)
> +	 * (support kernel ver >=3D 5.0 or OFED ver >=3D 4.6).
>  	 */
> -	port_name_set =3D	(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) =3D=3D 6);
> -	if (port_name_set) {
> -		port_info_out->port_name_new =3D 1;
> -	} else {
> -		/* Check for port-name as a number (support kernel ver <
> 5.0 */
> -		errno =3D 0;
> -		port_info_out->port_name =3D strtol(port_name_in, &end, 0);
> -		if (!errno &&
> -		    (size_t)(end - port_name_in) =3D=3D strlen(port_name_in))
> -			port_name_set =3D true;
> +	sc_items =3D 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 =3D=3D 6 &&
> +	    pf_c1 =3D=3D 'p' && pf_c2 =3D=3D 'f' &&
> +	    vf_c1 =3D=3D 'v' && vf_c2 =3D=3D 'f') {
> +		port_info_out->name_type =3D
> MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> +		return true;
> +	}
> +	/*
> +	 * Check for port-name as a string of the form p0
> +	 * (support kernel ver >=3D 5.0, or OFED ver >=3D 4.6).
> +	 */
> +	sc_items =3D sscanf(port_name_in, "%c%d",
> +			  &pf_c1, &port_info_out->port_name);
> +	if (sc_items =3D=3D 2 && pf_c1 =3D=3D 'p') {
> +		port_info_out->name_type =3D
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		return true;
> +	}
> +	/* Check for port-name as a number (support kernel ver < 5.0 */
> +	errno =3D 0;
> +	port_info_out->port_name =3D strtol(port_name_in, &end, 0);
> +	if (!errno &&
> +	    (size_t)(end - port_name_in) =3D=3D strlen(port_name_in)) {
> +		port_info_out->name_type =3D
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> +		return  true;
>  	}
> -	return port_name_set;
> +	port_info_out->name_type =3D
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> +	return false;
>  }
> diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c inde=
x
> fd9226b..669de76 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 =3D {
>  		.master =3D 0,
>  		.representor =3D 0,
> -		.port_name_new =3D 0,
> +		.name_type =3D MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
>  		.port_name =3D 0,
>  		.switch_id =3D 0,
>  	};
>  	size_t off =3D NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -	bool port_name_set =3D false;
>  	bool switch_id_set =3D false;
>  	bool num_vf_set =3D false;
>=20
> @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
>  			num_vf_set =3D true;
>  			break;
>  		case IFLA_PHYS_PORT_NAME:
> -			port_name_set =3D
> -				mlx5_translate_port_name((char *)payload,
> -							 &info);
> +			mlx5_translate_port_name((char *)payload, &info);
>  			break;
>  		case IFLA_PHYS_SWITCH_ID:
>  			info.switch_id =3D 0;
> @@ -926,16 +923,33 @@ struct mlx5_nl_ifindex_data {
>  		off +=3D RTA_ALIGN(ra->rta_len);
>  	}
>  	if (switch_id_set) {
> -		if (info.port_name_new) {
> -			/* New representors naming schema. */
> -			if (port_name_set) {
> -				info.master =3D (info.port_name =3D=3D -1);
> -				info.representor =3D (info.port_name !=3D -1);
> -			}
> -		} else {
> +		/* We have some E-Switch configuration. */
> +		switch (info.name_type) {
> +		case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +			/*
> +			 * Name is not recognized or not set,
> +			 * it can not be representor, check
> +			 * VF number to see if it is a master.
> +			 */
> +			info.master =3D num_vf_set;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
>  			/* Legacy representors naming schema. */
> -			info.master =3D (!port_name_set || num_vf_set);
> -			info.representor =3D port_name_set && !num_vf_set;
> +			info.representor =3D !num_vf_set;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +			/* New uplink naming schema. */
> +			info.master =3D 1;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +			/* New representors naming schema. */
> +			info.representor =3D 1;
> +			break;
> +		}
> +		if (!info.master && !info.representor) {
> +			DRV_LOG(INFO,
> +				"unable to recognize master/representors"
> +				" on the device in switch domain");

Same comment as above. Would like to avoid this switch case outside of the =
translate function .

>  		}
>  	}
>  	assert(!(info.master && info.representor));
> --
> 1.8.3.1