From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-DB5-obe.outbound.protection.outlook.com (mail-eopbgr40059.outbound.protection.outlook.com [40.107.4.59]) by dpdk.org (Postfix) with ESMTP id 063A31B131 for ; Mon, 15 Apr 2019 11:11:56 +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=UwVIxpgC1A0Bm6dKeFPHVr4wBzPr5k4CaIa6qNCcUiw=; b=CCrmDk3cA6XUffTEjNv8LNzviDeedWKfquOTMORSS6IPZlekSJVz3VkHHEHSgq2LbXjPKuX0oxTTC5GhliXr+8TwyIxZLm9idyzeS6hYzXR+U7x3kMS+KJ3AFfkamU49SvBALN+vkuF8Ja3XULqgzbFka1ZWQ+xIbxAm1KgK0eQ= Received: from AM4PR05MB3265.eurprd05.prod.outlook.com (10.171.188.154) by AM4PR05MB3236.eurprd05.prod.outlook.com (10.171.186.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1792.19; Mon, 15 Apr 2019 09:11:54 +0000 Received: from AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::c5f8:16a9:a4a6:a135]) by AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::c5f8:16a9:a4a6:a135%4]) with mapi id 15.20.1792.018; Mon, 15 Apr 2019 09:11:54 +0000 From: Slava Ovsiienko To: Shahaf Shuler , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor Thread-Index: AQHU8Uc376sguZWIQkK3K+qoHDWrZKY7SMKAgAGlKvA= Date: Mon, 15 Apr 2019 09:11:53 +0000 Message-ID: References: <1555084107-24692-1-git-send-email-viacheslavo@mellanox.com> In-Reply-To: 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=viacheslavo@mellanox.com; x-originating-ip: [95.164.10.10] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 1db3e064-d10f-425b-34f0-08d6c182672f x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(5600140)(711020)(4605104)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7193020); SRVR:AM4PR05MB3236; x-ms-traffictypediagnostic: AM4PR05MB3236: x-microsoft-antispam-prvs: x-forefront-prvs: 000800954F x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(376002)(396003)(366004)(346002)(136003)(13464003)(189003)(199004)(102836004)(30864003)(6436002)(99286004)(106356001)(14454004)(53546011)(105586002)(6506007)(76176011)(2501003)(97736004)(25786009)(66066001)(71190400001)(71200400001)(446003)(110136005)(316002)(26005)(6246003)(486006)(305945005)(256004)(86362001)(476003)(14444005)(5024004)(11346002)(7736002)(53936002)(81156014)(81166006)(9686003)(229853002)(7696005)(55016002)(52536014)(74316002)(8676002)(68736007)(6116002)(5660300002)(8936002)(186003)(3846002)(33656002)(478600001)(2906002); DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR05MB3236; H:AM4PR05MB3265.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A: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: 2IWZzD0EwsJTTZng/il/v69FOresVvgQk3dU5AQb2bukTn47PcwJsaLP5QcStYNe8gSRNUkdNqXH3NU34cqzDAxh+INML7xEpdNyfYnIszEnu9BV+AuXMpke+ofl3ITwcgpkpaKPqefWvZa/ldak2Ino2L4iD060qm+XeFvfjfMFCSwcvHHHGd5XONgRqXwPUwfBttcIgSN5fcVv29kKuMQErZUzAbm7bK6uP5uuE2/ORlqhmebtP4+8rWgvcLiynfolOAhEGwNtStd1Lu1z55SY9SbaiXLPSVnVS+sxKtYRezQatKYOyWIxVgY/LNXapJmPm6WZgDkgsbTLcwTOsKWPv28zOaWaA3RMUrFlwDN4jJwuYX4cvF+itAetDzzBmjg3pf+5HCrFrAfAPfhkeow3vKeUFA991HTOZoTgGik= 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: 1db3e064-d10f-425b-34f0-08d6c182672f X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Apr 2019 09:11:53.8798 (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: AM4PR05MB3236 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Apr 2019 09:11:56 -0000 Hi, Shahaf > -----Original Message----- > From: Shahaf Shuler > Sent: Sunday, April 14, 2019 10:43 > To: Slava Ovsiienko ; dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF > representor >=20 > Hi Slava, >=20 > Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko: > > Subject: [dpdk-dev] [PATCH 1/1] 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 extends port recognizing capability on the base of physical > > port name. The following naming formats are supported: > > > > - 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. > uplink representor is p%d > representors are (including PF representor) pf%dvf%d. the vf index for th= e PF > representor is 65535. OK, I'll try to update the commit message to make it more clear. >=20 > > 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. > > >=20 > Please also reference the kernel commit which introduce the name change. OK. >=20 > > Signed-off-by: Viacheslav Ovsiienko > > --- > > 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(-) > > > > 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" > > > > +/* 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); >=20 >=20 > Why we need to split the logic of the master/representor detection betwee= n > the mlx5_translate_port_name and the caller function? We have two stages: - gathering the port attributes (name, vf_num, switchdev_id, presence of de= vice directory, etc.) in callbacks - analyzing the parameters on gathering completion. We need all the paramet= ers to make a reliable master/representor recognition. Translate routine is called on gathering stage and just recognizes the name= format, extracts useful values (pf/vf num) and stores the results in compa= ct form. It is easier than storing the entire port name.=20 >=20 > The way I envision it is mlx5_tranlate_port_name receives the > phys_port_name and maybe more metadata and return the port This "metadata" may be not gathered yet at the moment of port name processi= ng. > classification (master/representor) and the representor/pf number. > 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) Actually this patch is also some kind of preparation for coming subfuctions= . If we have set of mutual exclusive formats the switch/case seems to be the = most native way to treat these ones. I'd prefer to keep switch/case. Comments ar= e clear and it is easy to understand where legacy/new format is treated. And it is easy to extend for coming subfunctions. I think we should isolate master/representor recognition logic into separat= e routine, "translate" routine is for parameter gathering stage, "recognize" = for analyzing. >=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; > > > > /* > > * 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 > > index > > 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; > > > > @@ -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"); >=20 > Same comment as above. Would like to avoid this switch case outside of th= e > translate function . Don't you mind if we provide separated analyzing routine ? >=20 > > } > > } > > assert(!(info.master && info.representor)); > > -- > > 1.8.3.1 With best regards, Slava From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 5BC13A00E6 for ; Mon, 15 Apr 2019 11:11:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E6A441B144; Mon, 15 Apr 2019 11:11:57 +0200 (CEST) Received: from EUR03-DB5-obe.outbound.protection.outlook.com (mail-eopbgr40059.outbound.protection.outlook.com [40.107.4.59]) by dpdk.org (Postfix) with ESMTP id 063A31B131 for ; Mon, 15 Apr 2019 11:11:56 +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=UwVIxpgC1A0Bm6dKeFPHVr4wBzPr5k4CaIa6qNCcUiw=; b=CCrmDk3cA6XUffTEjNv8LNzviDeedWKfquOTMORSS6IPZlekSJVz3VkHHEHSgq2LbXjPKuX0oxTTC5GhliXr+8TwyIxZLm9idyzeS6hYzXR+U7x3kMS+KJ3AFfkamU49SvBALN+vkuF8Ja3XULqgzbFka1ZWQ+xIbxAm1KgK0eQ= Received: from AM4PR05MB3265.eurprd05.prod.outlook.com (10.171.188.154) by AM4PR05MB3236.eurprd05.prod.outlook.com (10.171.186.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1792.19; Mon, 15 Apr 2019 09:11:54 +0000 Received: from AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::c5f8:16a9:a4a6:a135]) by AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::c5f8:16a9:a4a6:a135%4]) with mapi id 15.20.1792.018; Mon, 15 Apr 2019 09:11:54 +0000 From: Slava Ovsiienko To: Shahaf Shuler , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor Thread-Index: AQHU8Uc376sguZWIQkK3K+qoHDWrZKY7SMKAgAGlKvA= Date: Mon, 15 Apr 2019 09:11:53 +0000 Message-ID: References: <1555084107-24692-1-git-send-email-viacheslavo@mellanox.com> In-Reply-To: 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=viacheslavo@mellanox.com; x-originating-ip: [95.164.10.10] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 1db3e064-d10f-425b-34f0-08d6c182672f x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(5600140)(711020)(4605104)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7193020); SRVR:AM4PR05MB3236; x-ms-traffictypediagnostic: AM4PR05MB3236: x-microsoft-antispam-prvs: x-forefront-prvs: 000800954F x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(376002)(396003)(366004)(346002)(136003)(13464003)(189003)(199004)(102836004)(30864003)(6436002)(99286004)(106356001)(14454004)(53546011)(105586002)(6506007)(76176011)(2501003)(97736004)(25786009)(66066001)(71190400001)(71200400001)(446003)(110136005)(316002)(26005)(6246003)(486006)(305945005)(256004)(86362001)(476003)(14444005)(5024004)(11346002)(7736002)(53936002)(81156014)(81166006)(9686003)(229853002)(7696005)(55016002)(52536014)(74316002)(8676002)(68736007)(6116002)(5660300002)(8936002)(186003)(3846002)(33656002)(478600001)(2906002); DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR05MB3236; H:AM4PR05MB3265.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A: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: 2IWZzD0EwsJTTZng/il/v69FOresVvgQk3dU5AQb2bukTn47PcwJsaLP5QcStYNe8gSRNUkdNqXH3NU34cqzDAxh+INML7xEpdNyfYnIszEnu9BV+AuXMpke+ofl3ITwcgpkpaKPqefWvZa/ldak2Ino2L4iD060qm+XeFvfjfMFCSwcvHHHGd5XONgRqXwPUwfBttcIgSN5fcVv29kKuMQErZUzAbm7bK6uP5uuE2/ORlqhmebtP4+8rWgvcLiynfolOAhEGwNtStd1Lu1z55SY9SbaiXLPSVnVS+sxKtYRezQatKYOyWIxVgY/LNXapJmPm6WZgDkgsbTLcwTOsKWPv28zOaWaA3RMUrFlwDN4jJwuYX4cvF+itAetDzzBmjg3pf+5HCrFrAfAPfhkeow3vKeUFA991HTOZoTgGik= 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: 1db3e064-d10f-425b-34f0-08d6c182672f X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Apr 2019 09:11:53.8798 (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: AM4PR05MB3236 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190415091153.CYCpm_nGOR6top7yWvKuL0BRDrLRel-2llenL4v1UUY@z> Hi, Shahaf > -----Original Message----- > From: Shahaf Shuler > Sent: Sunday, April 14, 2019 10:43 > To: Slava Ovsiienko ; dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF > representor >=20 > Hi Slava, >=20 > Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko: > > Subject: [dpdk-dev] [PATCH 1/1] 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 extends port recognizing capability on the base of physical > > port name. The following naming formats are supported: > > > > - 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. > uplink representor is p%d > representors are (including PF representor) pf%dvf%d. the vf index for th= e PF > representor is 65535. OK, I'll try to update the commit message to make it more clear. >=20 > > 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. > > >=20 > Please also reference the kernel commit which introduce the name change. OK. >=20 > > Signed-off-by: Viacheslav Ovsiienko > > --- > > 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(-) > > > > 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" > > > > +/* 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); >=20 >=20 > Why we need to split the logic of the master/representor detection betwee= n > the mlx5_translate_port_name and the caller function? We have two stages: - gathering the port attributes (name, vf_num, switchdev_id, presence of de= vice directory, etc.) in callbacks - analyzing the parameters on gathering completion. We need all the paramet= ers to make a reliable master/representor recognition. Translate routine is called on gathering stage and just recognizes the name= format, extracts useful values (pf/vf num) and stores the results in compa= ct form. It is easier than storing the entire port name.=20 >=20 > The way I envision it is mlx5_tranlate_port_name receives the > phys_port_name and maybe more metadata and return the port This "metadata" may be not gathered yet at the moment of port name processi= ng. > classification (master/representor) and the representor/pf number. > 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) Actually this patch is also some kind of preparation for coming subfuctions= . If we have set of mutual exclusive formats the switch/case seems to be the = most native way to treat these ones. I'd prefer to keep switch/case. Comments ar= e clear and it is easy to understand where legacy/new format is treated. And it is easy to extend for coming subfunctions. I think we should isolate master/representor recognition logic into separat= e routine, "translate" routine is for parameter gathering stage, "recognize" = for analyzing. >=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; > > > > /* > > * 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 > > index > > 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; > > > > @@ -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"); >=20 > Same comment as above. Would like to avoid this switch case outside of th= e > translate function . Don't you mind if we provide separated analyzing routine ? >=20 > > } > > } > > assert(!(info.master && info.representor)); > > -- > > 1.8.3.1 With best regards, Slava