[PATCH] Add support for configuring Infiniband GUIDs

[PATCH] Add support for configuring Infiniband GUIDs

[Eli Cohen]

Add two NLA's that allow configuration of Infiniband node or port GUIDs
by referencing the IPoIB net device set over then physical function. The
format to be used is as follows:

ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70
ip link set dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78

Issue: 702759
Change-Id: I5ffb54d6de7bfa8650bf5818f484279914991d6e
Signed-off-by: Eli Cohen <eli@mellanox.com>
---
 ip/iplink.c           | 40 ++++++++++++++++++++++++++++++++++++++++
 man/man8/ip-link.8.in | 12 +++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index d2e586b6d133..3f885defdfeb 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -237,6 +237,30 @@ struct iplink_req {
 	char			buf[1024];
 };
 
+static int extract_guid(__u64 *guid, char *arg)
+{
+	__u64 ret;
+	int g[8];
+	int err;
+
+	err = sscanf(arg, "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
+		     g, g + 1, g + 2, g + 3, g + 4, g + 5, g + 6, g + 7);
+	if (err != 8)
+		return -1;
+
+	ret = ((__u64)(g[0]) << 56) |
+	      ((__u64)(g[1]) << 48) |
+	      ((__u64)(g[2]) << 40) |
+	      ((__u64)(g[3]) << 32) |
+	      ((__u64)(g[4]) << 24) |
+	      ((__u64)(g[5]) << 16) |
+	      ((__u64)(g[6]) << 8) |
+	      ((__u64)(g[7]));
+	*guid = ret;
+
+	return 0;
+}
+
 static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 			   struct iplink_req *req, int dev_index)
 {
@@ -383,6 +407,22 @@ static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 				invarg("Invalid \"state\" value\n", *argv);
 			ivl.vf = vf;
 			addattr_l(&req->n, sizeof(*req), IFLA_VF_LINK_STATE, &ivl, sizeof(ivl));
+		} else if (matches(*argv, "node_guid") == 0) {
+			struct ifla_vf_guid ivg;
+
+			NEXT_ARG();
+			ivg.vf = vf;
+			if (extract_guid(&ivg.guid, *argv))
+				return -1;
+			addattr_l(&req->n, sizeof(*req), IFLA_VF_IB_NODE_GUID, &ivg, sizeof(ivg));
+		} else if (matches(*argv, "port_guid") == 0) {
+			struct ifla_vf_guid ivg;
+
+			NEXT_ARG();
+			ivg.vf = vf;
+			if (extract_guid(&ivg.guid, *argv))
+				return -1;
+			addattr_l(&req->n, sizeof(*req), IFLA_VF_IB_PORT_GUID, &ivg, sizeof(ivg));
 		} else {
 			/* rewind arg */
 			PREV_ARG();
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 805511423ef2..e143a5ec8a9a 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -143,7 +143,11 @@ ip-link \- network device configuration
 .br
 .RB "[ " state " { " auto " | " enable " | " disable " } ]"
 .br
-.RB "[ " trust " { " on " | " off " } ] ]"
+.RB "[ " trust " { " on " | " off " } ]"
+.br
+.RB "[ " node_guid " eui64 ]"
+.br
+.RB "[ " port_guid " eui64 ] ]"
 .br
 .in -9
 .RB "[ " master
@@ -1033,6 +1037,12 @@ sent by the VF.
 .BI trust " on|off"
 - trust the specified VF user. This enables that VF user can set a specific feature
 which may impact security and/or performance. (e.g. VF multicast promiscuous mode)
+.sp
+.BI node_guid " eui64"
+- configure node GUID for the VF.
+.sp
+.BI port_guid " eui64"
+- configure port GUID for the VF.
 .in -8
 
 .TP
-- 
2.8.1

Re: [PATCH] Add support for configuring Infiniband GUIDs

[Sergei Shtylyov]

Hello.

On 05/06/2016 06:43 PM, Eli Cohen wrote:

> Add two NLA's that allow configuration of Infiniband node or port GUIDs
> by referencing the IPoIB net device set over then physical function. The
> format to be used is as follows:
>
> ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70
> ip link set dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78
>
> Issue: 702759
> Change-Id: I5ffb54d6de7bfa8650bf5818f484279914991d6e
> Signed-off-by: Eli Cohen <eli@mellanox.com>
> ---
>   ip/iplink.c           | 40 ++++++++++++++++++++++++++++++++++++++++
>   man/man8/ip-link.8.in | 12 +++++++++++-
>   2 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/ip/iplink.c b/ip/iplink.c
> index d2e586b6d133..3f885defdfeb 100644
> --- a/ip/iplink.c
> +++ b/ip/iplink.c
> @@ -237,6 +237,30 @@ struct iplink_req {
>   	char			buf[1024];
>   };
>
> +static int extract_guid(__u64 *guid, char *arg)
> +{
> +	__u64 ret;
> +	int g[8];
> +	int err;
> +
> +	err = sscanf(arg, "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
> +		     g, g + 1, g + 2, g + 3, g + 4, g + 5, g + 6, g + 7);
> +	if (err != 8)

    Strange name for a variable, if sscanf() returns # of fields read... In 
fact, you don't even need this variable.

> +		return -1;
> +
> +	ret = ((__u64)(g[0]) << 56) |
> +	      ((__u64)(g[1]) << 48) |
> +	      ((__u64)(g[2]) << 40) |
> +	      ((__u64)(g[3]) << 32) |
> +	      ((__u64)(g[4]) << 24) |
> +	      ((__u64)(g[5]) << 16) |
> +	      ((__u64)(g[6]) << 8) |
> +	      ((__u64)(g[7]));
> +	*guid = ret;
> +
> +	return 0;
> +}
> +
>   static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
>   			   struct iplink_req *req, int dev_index)
>   {
[...]

MBR, Sergei

RE: [PATCH] Add support for configuring Infiniband GUIDs

[Eli Cohen]

Hi Sergei,
I am not following on your comment. Are you referring to the variable 'g'? It's just a short name to make the code more readable. I also don't follow your comment on the fact that sscanf returns the number of arguments read. Please explain.

Eli

-----Original Message-----
From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] 
Sent: Friday, May 06, 2016 12:05 PM
To: Eli Cohen <eli@mellanox.com>; shemminger@osdl.org
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH] Add support for configuring Infiniband GUIDs

Hello.

On 05/06/2016 06:43 PM, Eli Cohen wrote:

> Add two NLA's that allow configuration of Infiniband node or port 
> GUIDs by referencing the IPoIB net device set over then physical 
> function. The format to be used is as follows:
>
> ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70 ip link set 
> dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78
>
> Issue: 702759
> Change-Id: I5ffb54d6de7bfa8650bf5818f484279914991d6e
> Signed-off-by: Eli Cohen <eli@mellanox.com>
> ---
>   ip/iplink.c           | 40 ++++++++++++++++++++++++++++++++++++++++
>   man/man8/ip-link.8.in | 12 +++++++++++-
>   2 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/ip/iplink.c b/ip/iplink.c index 
> d2e586b6d133..3f885defdfeb 100644
> --- a/ip/iplink.c
> +++ b/ip/iplink.c
> @@ -237,6 +237,30 @@ struct iplink_req {
>   	char			buf[1024];
>   };
>
> +static int extract_guid(__u64 *guid, char *arg) {
> +	__u64 ret;
> +	int g[8];
> +	int err;
> +
> +	err = sscanf(arg, "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
> +		     g, g + 1, g + 2, g + 3, g + 4, g + 5, g + 6, g + 7);
> +	if (err != 8)

    Strange name for a variable, if sscanf() returns # of fields read... In fact, you don't even need this variable.

> +		return -1;
> +
> +	ret = ((__u64)(g[0]) << 56) |
> +	      ((__u64)(g[1]) << 48) |
> +	      ((__u64)(g[2]) << 40) |
> +	      ((__u64)(g[3]) << 32) |
> +	      ((__u64)(g[4]) << 24) |
> +	      ((__u64)(g[5]) << 16) |
> +	      ((__u64)(g[6]) << 8) |
> +	      ((__u64)(g[7]));
> +	*guid = ret;
> +
> +	return 0;
> +}
> +
>   static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
>   			   struct iplink_req *req, int dev_index)
>   {
[...]

MBR, Sergei

Re: [PATCH] Add support for configuring Infiniband GUIDs

[Stephen Hemminger]

On Fri,  6 May 2016 10:43:25 -0500
Eli Cohen <eli@mellanox.com> wrote:

> Add two NLA's that allow configuration of Infiniband node or port GUIDs
> by referencing the IPoIB net device set over then physical function. The
> format to be used is as follows:
> 
> ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70
> ip link set dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78
> 
> Issue: 702759
> Change-Id: I5ffb54d6de7bfa8650bf5818f484279914991d6e
> Signed-off-by: Eli Cohen <eli@mellanox.com>

I am not that familiar with Infiniband, but the documentation seems
to use a non-colon form:
 # ip link set dev ib0 vf 0 node_guid 0002c90300216e70

Seems like ip should follow the lead of ibstat and friends.

RE: [PATCH] Add support for configuring Infiniband GUIDs

[Eli Cohen]

Hi Stephen,

ibstat is usually used with MLNX_OFED distributions and I don't think it is a part of libibverbs.
Moreover, in a previous patch I sent https://patchwork.ozlabs.org/patch/596492/ I documented in the change log the same format I mention in this patch. So if I change the format here we will have the changelog in the kernel tree invalid.

How do you think this should be addressed?

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@networkplumber.org] 
Sent: Friday, May 06, 2016 1:54 PM
To: Eli Cohen <eli@mellanox.com>
Cc: shemminger@osdl.org; netdev@vger.kernel.org
Subject: Re: [PATCH] Add support for configuring Infiniband GUIDs

On Fri,  6 May 2016 10:43:25 -0500
Eli Cohen <eli@mellanox.com> wrote:

> Add two NLA's that allow configuration of Infiniband node or port 
> GUIDs by referencing the IPoIB net device set over then physical 
> function. The format to be used is as follows:
> 
> ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70 ip link set 
> dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78
> 
> Issue: 702759
> Change-Id: I5ffb54d6de7bfa8650bf5818f484279914991d6e
> Signed-off-by: Eli Cohen <eli@mellanox.com>

I am not that familiar with Infiniband, but the documentation seems to use a non-colon form:
 # ip link set dev ib0 vf 0 node_guid 0002c90300216e70

Seems like ip should follow the lead of ibstat and friends.

Re: [PATCH] Add support for configuring Infiniband GUIDs

[Eli Cohen]

Hi Stephen,

can you please comment on this or are you going to apply this?

On Mon, May 09, 2016 at 03:52:02PM +0000, Eli Cohen wrote:
> Hi Stephen,
> 
> ibstat is usually used with MLNX_OFED distributions and I don't think it is a part of libibverbs.
> Moreover, in a previous patch I sent https://patchwork.ozlabs.org/patch/596492/ I documented in the change log the same format I mention in this patch. So if I change the format here we will have the changelog in the kernel tree invalid.
> 
> How do you think this should be addressed?
> 

Re: [PATCH] Add support for configuring Infiniband GUIDs

[Eli Cohen]

Hi Stephen,

can you please comment on this or can we move forward with submitting
this patch?

Thanks,
Eli

On Fri, May 13, 2016 at 10:52:25PM +0300, Eli Cohen wrote:
> Hi Stephen,
> 
> can you please comment on this or are you going to apply this?
> 
> On Mon, May 09, 2016 at 03:52:02PM +0000, Eli Cohen wrote:
> > Hi Stephen,
> > 
> > ibstat is usually used with MLNX_OFED distributions and I don't think it is a part of libibverbs.
> > Moreover, in a previous patch I sent https://patchwork.ozlabs.org/patch/596492/ I documented in the change log the same format I mention in this patch. So if I change the format here we will have the changelog in the kernel tree invalid.
> > 
> > How do you think this should be addressed?
> > 

Re: [PATCH] Add support for configuring Infiniband GUIDs

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Fri, May 06, 2016 at 10:43:25AM -0500, Eli Cohen wrote:
> Add two NLA's that allow configuration of Infiniband node or port GUIDs
> by referencing the IPoIB net device set over then physical function. The
> format to be used is as follows:
> 
> ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70
> ip link set dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78
> 
> Issue: 702759
> Change-Id: I5ffb54d6de7bfa8650bf5818f484279914991d6e

These two lines are not needed to be part of commit message.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] Add support for configuring Infiniband GUIDs

[Eli Cohen]

Add two NLA's that allow configuration of Infiniband node or port GUIDs
by referencing the IPoIB net device set over then physical function. The
format to be used is as follows:

ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70
ip link set dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78

Signed-off-by: Eli Cohen <eli@mellanox.com>
---

Changes from V1:
Removed internal issue number and gerrit ID

 ip/iplink.c           | 40 ++++++++++++++++++++++++++++++++++++++++
 man/man8/ip-link.8.in | 12 +++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index b1f8a37922f5..0d2d750f2887 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -267,6 +267,30 @@ static int nl_get_ll_addr_len(unsigned int dev_index)
 	return RTA_PAYLOAD(tb[IFLA_ADDRESS]);
 }
 
+static int extract_guid(__u64 *guid, char *arg)
+{
+	__u64 ret;
+	int g[8];
+	int err;
+
+	err = sscanf(arg, "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
+		     g, g + 1, g + 2, g + 3, g + 4, g + 5, g + 6, g + 7);
+	if (err != 8)
+		return -1;
+
+	ret = ((__u64)(g[0]) << 56) |
+	      ((__u64)(g[1]) << 48) |
+	      ((__u64)(g[2]) << 40) |
+	      ((__u64)(g[3]) << 32) |
+	      ((__u64)(g[4]) << 24) |
+	      ((__u64)(g[5]) << 16) |
+	      ((__u64)(g[6]) << 8) |
+	      ((__u64)(g[7]));
+	*guid = ret;
+
+	return 0;
+}
+
 static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 			   struct iplink_req *req, int dev_index)
 {
@@ -420,6 +444,22 @@ static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 				invarg("Invalid \"state\" value\n", *argv);
 			ivl.vf = vf;
 			addattr_l(&req->n, sizeof(*req), IFLA_VF_LINK_STATE, &ivl, sizeof(ivl));
+		} else if (matches(*argv, "node_guid") == 0) {
+			struct ifla_vf_guid ivg;
+
+			NEXT_ARG();
+			ivg.vf = vf;
+			if (extract_guid(&ivg.guid, *argv))
+				return -1;
+			addattr_l(&req->n, sizeof(*req), IFLA_VF_IB_NODE_GUID, &ivg, sizeof(ivg));
+		} else if (matches(*argv, "port_guid") == 0) {
+			struct ifla_vf_guid ivg;
+
+			NEXT_ARG();
+			ivg.vf = vf;
+			if (extract_guid(&ivg.guid, *argv))
+				return -1;
+			addattr_l(&req->n, sizeof(*req), IFLA_VF_IB_PORT_GUID, &ivg, sizeof(ivg));
 		} else {
 			/* rewind arg */
 			PREV_ARG();
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 375b4d081408..07f0a94a289a 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -146,7 +146,11 @@ ip-link \- network device configuration
 .br
 .RB "[ " state " { " auto " | " enable " | " disable " } ]"
 .br
-.RB "[ " trust " { " on " | " off " } ] ]"
+.RB "[ " trust " { " on " | " off " } ]"
+.br
+.RB "[ " node_guid " eui64 ]"
+.br
+.RB "[ " port_guid " eui64 ] ]"
 .br
 .in -9
 .RB "[ " master
@@ -1191,6 +1195,12 @@ sent by the VF.
 .BI trust " on|off"
 - trust the specified VF user. This enables that VF user can set a specific feature
 which may impact security and/or performance. (e.g. VF multicast promiscuous mode)
+.sp
+.BI node_guid " eui64"
+- configure node GUID for the VF.
+.sp
+.BI port_guid " eui64"
+- configure port GUID for the VF.
 .in -8
 
 .TP
-- 
2.8.1

Re: [PATCH] Add support for configuring Infiniband GUIDs

[Stephen Hemminger]

On Tue,  5 Jul 2016 10:53:38 -0500
Eli Cohen <eli@mellanox.com> wrote:

>  
> +static int extract_guid(__u64 *guid, char *arg)
> +{
> +	__u64 ret;
> +	int g[8];
> +	int err;
> +
> +	err = sscanf(arg, "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
> +		     g, g + 1, g + 2, g + 3, g + 4, g + 5, g + 6, g + 7);
> +	if (err != 8)
> +		return -1;
> +
> +	ret = ((__u64)(g[0]) << 56) |
> +	      ((__u64)(g[1]) << 48) |
> +	      ((__u64)(g[2]) << 40) |
> +	      ((__u64)(g[3]) << 32) |
> +	      ((__u64)(g[4]) << 24) |
> +	      ((__u64)(g[5]) << 16) |
> +	      ((__u64)(g[6]) << 8) |
> +	      ((__u64)(g[7]));
> +	*guid = ret;
> +
> +	return 0;
> +}

I would like several things changed here.
 1. put this in generic (ie lib/utils.c) so that other places
    can use it. And rename it match other arg parsing code (ie get_guid)
 2. need range checking for each piece the string, and each hex piece must be unsigned int
    suprised gcc format checks didn't bust you on this.  why not %hhx as format specifier
    
 3. arg should be const char *
 4. local variable err is really unnecessary
 5. local variable ret is unnecessary, you could just assign to *guid
 

RE: [PATCH] Add support for configuring Infiniband GUIDs

[David Laight]

From: Stephen Hemminger
> Sent: 07 July 2016 05:05
> On Tue,  5 Jul 2016 10:53:38 -0500
> Eli Cohen <eli@mellanox.com> wrote:
> 
> >
> > +static int extract_guid(__u64 *guid, char *arg)
> > +{
> > +	__u64 ret;
> > +	int g[8];
> > +	int err;
> > +
> > +	err = sscanf(arg, "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
> > +		     g, g + 1, g + 2, g + 3, g + 4, g + 5, g + 6, g + 7);
> > +	if (err != 8)
> > +		return -1;
> > +
> > +	ret = ((__u64)(g[0]) << 56) |
> > +	      ((__u64)(g[1]) << 48) |
> > +	      ((__u64)(g[2]) << 40) |
> > +	      ((__u64)(g[3]) << 32) |
> > +	      ((__u64)(g[4]) << 24) |
> > +	      ((__u64)(g[5]) << 16) |
> > +	      ((__u64)(g[6]) << 8) |
> > +	      ((__u64)(g[7]));
> > +	*guid = ret;
> > +
> > +	return 0;
> > +}
> 
> I would like several things changed here.
>  1. put this in generic (ie lib/utils.c) so that other places
>     can use it. And rename it match other arg parsing code (ie get_guid)
>  2. need range checking for each piece the string, and each hex piece must be unsigned int
>     suprised gcc format checks didn't bust you on this.  why not %hhx as format specifier
> 
>  3. arg should be const char *
>  4. local variable err is really unnecessary
>  5. local variable ret is unnecessary, you could just assign to *guid

I'd suggest not using sscanf, but using the kernel equivalent of strtoul()
(or even just looking for [0-9a-fA-F] directly).
sscanf() can bite you in all sorts of unexpected ways.

	David

RE: [PATCH] Add support for configuring Infiniband GUIDs

[Eli Cohen]

Hi,

I have just sent a new version of the patch which addresses all the comments I got.
Thanks for reviewing. Please let me know if you have any other comments.

Thanks,
Eli

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@networkplumber.org] 
Sent: Wednesday, July 06, 2016 11:05 PM
To: Eli Cohen <eli@mellanox.com>
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH] Add support for configuring Infiniband GUIDs

On Tue,  5 Jul 2016 10:53:38 -0500
Eli Cohen <eli@mellanox.com> wrote:

>  
> +static int extract_guid(__u64 *guid, char *arg) {
> +	__u64 ret;
> +	int g[8];
> +	int err;
> +
> +	err = sscanf(arg, "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
> +		     g, g + 1, g + 2, g + 3, g + 4, g + 5, g + 6, g + 7);
> +	if (err != 8)
> +		return -1;
> +
> +	ret = ((__u64)(g[0]) << 56) |
> +	      ((__u64)(g[1]) << 48) |
> +	      ((__u64)(g[2]) << 40) |
> +	      ((__u64)(g[3]) << 32) |
> +	      ((__u64)(g[4]) << 24) |
> +	      ((__u64)(g[5]) << 16) |
> +	      ((__u64)(g[6]) << 8) |
> +	      ((__u64)(g[7]));
> +	*guid = ret;
> +
> +	return 0;
> +}

I would like several things changed here.
 1. put this in generic (ie lib/utils.c) so that other places
    can use it. And rename it match other arg parsing code (ie get_guid)  2. need range checking for each piece the string, and each hex piece must be unsigned int
    suprised gcc format checks didn't bust you on this.  why not %hhx as format specifier
    
 3. arg should be const char *
 4. local variable err is really unnecessary  5. local variable ret is unnecessary, you could just assign to *guid