[PATCH] rtnetlink: make SR-IOV VF interface symmetric

[PATCH] rtnetlink: make SR-IOV VF interface symmetric

[Chris Wright]

Now we have a set of nested attributes:

  IFLA_VFINFO_LIST (NESTED)
    IFLA_VF_INFO (NESTED)
      IFLA_VF_MAC
      IFLA_VF_VLAN
      IFLA_VF_TX_RATE

This allows a single set to operate on multiple attributes if desired.
Among other things, it means a dump can be replayed to set state.

The current interface has yet to be released, so this seems like
something to consider for 2.6.34.

Signed-off-by: Chris Wright <chrisw@sous-sol.org
---

Stephen, I'll send the iproute2 update as well

 include/linux/if_link.h |   23 ++++++-
 net/core/rtnetlink.c    |  159 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 129 insertions(+), 53 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index c9bf92c..d94963b 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -79,10 +79,7 @@ enum {
 	IFLA_NET_NS_PID,
 	IFLA_IFALIAS,
 	IFLA_NUM_VF,		/* Number of VFs if device is SR-IOV PF */
-	IFLA_VF_MAC,		/* Hardware queue specific attributes */
-	IFLA_VF_VLAN,
-	IFLA_VF_TX_RATE,	/* TX Bandwidth Allocation */
-	IFLA_VFINFO,
+	IFLA_VFINFO_LIST,
 	__IFLA_MAX
 };
 
@@ -203,6 +200,24 @@ enum macvlan_mode {
 
 /* SR-IOV virtual function managment section */
 
+enum {
+	IFLA_VF_INFO_UNSPEC,
+	IFLA_VF_INFO,
+	__IFLA_VF_INFO_MAX,
+};
+
+#define IFLA_VF_INFO_MAX (__IFLA_VF_INFO_MAX - 1)
+
+enum {
+	IFLA_VF_UNSPEC,
+	IFLA_VF_MAC,		/* Hardware queue specific attributes */
+	IFLA_VF_VLAN,
+	IFLA_VF_TX_RATE,	/* TX Bandwidth Allocation */
+	__IFLA_VF_MAX,
+};
+
+#define IFLA_VF_MAX (__IFLA_VF_MAX - 1)
+
 struct ifla_vf_mac {
 	__u32 vf;
 	__u8 mac[32]; /* MAX_ADDR_LEN */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index fe776c9..074afcd 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -602,12 +602,19 @@ static void copy_rtnl_link_stats(struct rtnl_link_stats *a,
 	a->tx_compressed = b->tx_compressed;
 };
 
+/* All VF info */
 static inline int rtnl_vfinfo_size(const struct net_device *dev)
 {
-	if (dev->dev.parent && dev_is_pci(dev->dev.parent))
-		return dev_num_vf(dev->dev.parent) *
-			sizeof(struct ifla_vf_info);
-	else
+	if (dev->dev.parent && dev_is_pci(dev->dev.parent)) {
+		
+		int num_vfs = dev_num_vf(dev->dev.parent);
+		size_t size = nlmsg_total_size(sizeof(struct nlattr));
+		size += nlmsg_total_size(num_vfs * sizeof(struct nlattr));
+		size += num_vfs * (sizeof(struct ifla_vf_mac) +
+				  sizeof(struct ifla_vf_vlan) +
+				  sizeof(struct ifla_vf_tx_rate));
+		return size;
+	} else
 		return 0;
 }
 
@@ -629,7 +636,7 @@ static inline size_t if_nlmsg_size(const struct net_device *dev)
 	       + nla_total_size(1) /* IFLA_OPERSTATE */
 	       + nla_total_size(1) /* IFLA_LINKMODE */
 	       + nla_total_size(4) /* IFLA_NUM_VF */
-	       + nla_total_size(rtnl_vfinfo_size(dev)) /* IFLA_VFINFO */
+	       + rtnl_vfinfo_size(dev) /* IFLA_VFINFO_LIST */
 	       + rtnl_link_get_size(dev); /* IFLA_LINKINFO */
 }
 
@@ -700,14 +707,37 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 
 	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) {
 		int i;
-		struct ifla_vf_info ivi;
 
-		NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));
-		for (i = 0; i < dev_num_vf(dev->dev.parent); i++) {
+		struct nlattr *vfinfo, *vf;
+		int num_vfs = dev_num_vf(dev->dev.parent);
+
+		NLA_PUT_U32(skb, IFLA_NUM_VF, num_vfs);
+		vfinfo = nla_nest_start(skb, IFLA_VFINFO_LIST);
+		if (!vfinfo)
+			goto nla_put_failure;
+		for (i = 0; i < num_vfs; i++) {
+			struct ifla_vf_info ivi;
+			struct ifla_vf_mac vf_mac;
+			struct ifla_vf_vlan vf_vlan;
+			struct ifla_vf_tx_rate vf_tx_rate;
 			if (dev->netdev_ops->ndo_get_vf_config(dev, i, &ivi))
 				break;
-			NLA_PUT(skb, IFLA_VFINFO, sizeof(ivi), &ivi);
+			vf_mac.vf = vf_vlan.vf = vf_tx_rate.vf = ivi.vf;
+			memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
+			vf_vlan.vlan = ivi.vlan;
+			vf_vlan.qos = ivi.qos;
+			vf_tx_rate.rate = ivi.tx_rate;
+			vf = nla_nest_start(skb, IFLA_VF_INFO);
+			if (!vf) {
+				nla_nest_cancel(skb, vfinfo);
+				goto nla_put_failure;
+			}
+			NLA_PUT(skb, IFLA_VF_MAC, sizeof(vf_mac), &vf_mac);
+			NLA_PUT(skb, IFLA_VF_VLAN, sizeof(vf_vlan), &vf_vlan);
+			NLA_PUT(skb, IFLA_VF_TX_RATE, sizeof(vf_tx_rate), &vf_tx_rate);
+			nla_nest_end(skb, vf);
 		}
+		nla_nest_end(skb, vfinfo);
 	}
 	if (dev->rtnl_link_ops) {
 		if (rtnl_link_fill(skb, dev) < 0)
@@ -769,12 +799,7 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_LINKINFO]		= { .type = NLA_NESTED },
 	[IFLA_NET_NS_PID]	= { .type = NLA_U32 },
 	[IFLA_IFALIAS]	        = { .type = NLA_STRING, .len = IFALIASZ-1 },
-	[IFLA_VF_MAC]		= { .type = NLA_BINARY,
-				    .len = sizeof(struct ifla_vf_mac) },
-	[IFLA_VF_VLAN]		= { .type = NLA_BINARY,
-				    .len = sizeof(struct ifla_vf_vlan) },
-	[IFLA_VF_TX_RATE]	= { .type = NLA_BINARY,
-				    .len = sizeof(struct ifla_vf_tx_rate) },
+	[IFLA_VFINFO_LIST]	= {. type = NLA_NESTED },
 };
 EXPORT_SYMBOL(ifla_policy);
 
@@ -783,6 +808,19 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
 	[IFLA_INFO_DATA]	= { .type = NLA_NESTED },
 };
 
+static const struct nla_policy ifla_vfinfo_policy[IFLA_VF_INFO_MAX+1] = {
+	[IFLA_VF_INFO]		= { .type = NLA_NESTED },
+};
+
+static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
+	[IFLA_VF_MAC]		= { .type = NLA_BINARY,
+				    .len = sizeof(struct ifla_vf_mac) },
+	[IFLA_VF_VLAN]		= { .type = NLA_BINARY,
+				    .len = sizeof(struct ifla_vf_vlan) },
+	[IFLA_VF_TX_RATE]	= { .type = NLA_BINARY,
+				    .len = sizeof(struct ifla_vf_tx_rate) },
+};
+
 struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
 {
 	struct net *net;
@@ -812,6 +850,52 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
 	return 0;
 }
 
+static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
+{
+	int rem, err = -EINVAL;
+	struct nlattr *vf;
+	const struct net_device_ops *ops = dev->netdev_ops;
+	
+	nla_for_each_nested(vf, attr, rem) {
+		switch (nla_type(vf)) {
+		case IFLA_VF_MAC: {
+			struct ifla_vf_mac *ivm;
+			ivm = nla_data(vf);
+			err = -EOPNOTSUPP;
+			if (ops->ndo_set_vf_mac)
+				err = ops->ndo_set_vf_mac(dev, ivm->vf,
+							  ivm->mac);
+			break;
+		}
+		case IFLA_VF_VLAN: {
+			struct ifla_vf_vlan *ivv;
+			ivv = nla_data(vf);
+			err = -EOPNOTSUPP;
+			if (ops->ndo_set_vf_vlan)
+				err = ops->ndo_set_vf_vlan(dev, ivv->vf,
+							   ivv->vlan,
+							   ivv->qos);
+			break;
+		}
+		case IFLA_VF_TX_RATE: {
+			struct ifla_vf_tx_rate *ivt;
+			ivt = nla_data(vf);
+			err = -EOPNOTSUPP;
+			if (ops->ndo_set_vf_tx_rate)
+				err = ops->ndo_set_vf_tx_rate(dev, ivt->vf,
+							      ivt->rate);
+			break;
+		}
+		default:
+			err = -EINVAL;
+			break;
+		}
+		if (err)
+			break;
+	}
+	return err;
+}
+
 static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		      struct nlattr **tb, char *ifname, int modified)
 {
@@ -942,40 +1026,17 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		write_unlock_bh(&dev_base_lock);
 	}
 
-	if (tb[IFLA_VF_MAC]) {
-		struct ifla_vf_mac *ivm;
-		ivm = nla_data(tb[IFLA_VF_MAC]);
-		err = -EOPNOTSUPP;
-		if (ops->ndo_set_vf_mac)
-			err = ops->ndo_set_vf_mac(dev, ivm->vf, ivm->mac);
-		if (err < 0)
-			goto errout;
-		modified = 1;
-	}
-
-	if (tb[IFLA_VF_VLAN]) {
-		struct ifla_vf_vlan *ivv;
-		ivv = nla_data(tb[IFLA_VF_VLAN]);
-		err = -EOPNOTSUPP;
-		if (ops->ndo_set_vf_vlan)
-			err = ops->ndo_set_vf_vlan(dev, ivv->vf,
-						   ivv->vlan,
-						   ivv->qos);
-		if (err < 0)
-			goto errout;
-		modified = 1;
-	}
-	err = 0;
-
-	if (tb[IFLA_VF_TX_RATE]) {
-		struct ifla_vf_tx_rate *ivt;
-		ivt = nla_data(tb[IFLA_VF_TX_RATE]);
-		err = -EOPNOTSUPP;
-		if (ops->ndo_set_vf_tx_rate)
-			err = ops->ndo_set_vf_tx_rate(dev, ivt->vf, ivt->rate);
-		if (err < 0)
-			goto errout;
-		modified = 1;
+	if (tb[IFLA_VFINFO_LIST]) {
+		struct nlattr *attr;
+		int rem;
+		nla_for_each_nested(attr, tb[IFLA_VFINFO_LIST], rem) {
+			if (nla_type(attr) != IFLA_VF_INFO)
+				goto errout;
+			err = do_setvfinfo(dev, attr);
+			if (err < 0)
+				goto errout;
+			modified = 1;
+		}
 	}
 	err = 0;
 
-- 
1.6.5.2

Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric

[Arnd Bergmann]

On Saturday 15 May 2010 05:14:16 Chris Wright wrote:
> Now we have a set of nested attributes:
> 
>   IFLA_VFINFO_LIST (NESTED)
>     IFLA_VF_INFO (NESTED)
>       IFLA_VF_MAC
>       IFLA_VF_VLAN
>       IFLA_VF_TX_RATE
> 
> This allows a single set to operate on multiple attributes if desired.
> Among other things, it means a dump can be replayed to set state.
> 
> The current interface has yet to be released, so this seems like
> something to consider for 2.6.34.
> 
> Signed-off-by: Chris Wright <chrisw@sous-sol.org

Very nice! This would be the minimum change to make the ABI conform
to the general rules, so it would be really good to have that.

Acked-by: Arnd Bergmann <arnd@arndb.de>

It does make the interface a bit strange (less than before), since the
new IFLA_VF_INFO now contains three nested attributes that each contain their
own vf number field, and we don't require that they are identical or that
each of the nested attributes inside VF_INFO appears only once.

How about a second patch that splits out an IFLA_VF_NUMBER attribute
and makes do_setvfinfo use nla_parse_nested instead of nla_for_each_nested
in order to tighten the rules on this some more?

	Arnd

Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric

[David Miller]

From: Chris Wright <chrisw@sous-sol.org>
Date: Fri, 14 May 2010 20:14:16 -0700

> Now we have a set of nested attributes:
> 
>   IFLA_VFINFO_LIST (NESTED)
>     IFLA_VF_INFO (NESTED)
>       IFLA_VF_MAC
>       IFLA_VF_VLAN
>       IFLA_VF_TX_RATE
> 
> This allows a single set to operate on multiple attributes if desired.
> Among other things, it means a dump can be replayed to set state.
> 
> The current interface has yet to be released, so this seems like
> something to consider for 2.6.34.
> 
> Signed-off-by: Chris Wright <chrisw@sous-sol.org

Agreed, applied to net-2.6, thanks Chris!

Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric

[Stephen Hemminger]

On Fri, 14 May 2010 20:14:16 -0700
Chris Wright <chrisw@sous-sol.org> wrote:

> Now we have a set of nested attributes:
> 
>   IFLA_VFINFO_LIST (NESTED)
>     IFLA_VF_INFO (NESTED)
>       IFLA_VF_MAC
>       IFLA_VF_VLAN
>       IFLA_VF_TX_RATE
> 
> This allows a single set to operate on multiple attributes if desired.
> Among other things, it means a dump can be replayed to set state.
> 
> The current interface has yet to be released, so this seems like
> something to consider for 2.6.34.
> 
> Signed-off-by: Chris Wright <chrisw@sous-sol.org
> ---

iproute2 update please?

Also I would really like documentation on this.


-- 

RE: [PATCH] rtnetlink: make SR-IOV VF interface symmetric

[Williams, Mitch A]

>-----Original Message-----
>From: Stephen Hemminger [mailto:shemminger@vyatta.com]
>Sent: Monday, May 17, 2010 9:00 AM
>To: Chris Wright
>Cc: davem@davemloft.net; kaber@trash.net; Williams, Mitch A;
>arnd@arndb.de; scofeldm@cisco.com; netdev@vger.kernel.org
>Subject: Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric
>
>On Fri, 14 May 2010 20:14:16 -0700
>Chris Wright <chrisw@sous-sol.org> wrote:
>
>> Now we have a set of nested attributes:
>>
>>   IFLA_VFINFO_LIST (NESTED)
>>     IFLA_VF_INFO (NESTED)
>>       IFLA_VF_MAC
>>       IFLA_VF_VLAN
>>       IFLA_VF_TX_RATE
>>
>> This allows a single set to operate on multiple attributes if desired.
>> Among other things, it means a dump can be replayed to set state.
>>
>> The current interface has yet to be released, so this seems like
>> something to consider for 2.6.34.
>>
>> Signed-off-by: Chris Wright <chrisw@sous-sol.org
>> ---
>
>iproute2 update please?
>
>Also I would really like documentation on this.
>
>
>--

Chris, have you got the iproute2 parts working, or do I need to pick it up?

Thanks again for your work on this.

-Mitch

Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric

[Chris Wright]

* Williams, Mitch A (mitch.a.williams@intel.com) wrote:
> >-----Original Message-----
> >From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> >Sent: Monday, May 17, 2010 9:00 AM
> >To: Chris Wright
> >Cc: davem@davemloft.net; kaber@trash.net; Williams, Mitch A;
> >arnd@arndb.de; scofeldm@cisco.com; netdev@vger.kernel.org
> >Subject: Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric
> >
> >On Fri, 14 May 2010 20:14:16 -0700
> >Chris Wright <chrisw@sous-sol.org> wrote:
> >
> >> Now we have a set of nested attributes:
> >>
> >>   IFLA_VFINFO_LIST (NESTED)
> >>     IFLA_VF_INFO (NESTED)
> >>       IFLA_VF_MAC
> >>       IFLA_VF_VLAN
> >>       IFLA_VF_TX_RATE
> >>
> >> This allows a single set to operate on multiple attributes if desired.
> >> Among other things, it means a dump can be replayed to set state.
> >>
> >> The current interface has yet to be released, so this seems like
> >> something to consider for 2.6.34.
> >>
> >> Signed-off-by: Chris Wright <chrisw@sous-sol.org
> >> ---
> >
> >iproute2 update please?
> >
> >Also I would really like documentation on this.

And a pony? ;-)

Docs in what form?

> Chris, have you got the iproute2 parts working, or do I need to pick it up?
> 
> Thanks again for your work on this.

I've got those bits, just was waiting until patches merged.
Will send them out this afternoon.

thanks,
-chris

Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric

[Chris Wright]

* Arnd Bergmann (arnd@arndb.de) wrote:
> On Saturday 15 May 2010 05:14:16 Chris Wright wrote:
> > Now we have a set of nested attributes:
> > 
> >   IFLA_VFINFO_LIST (NESTED)
> >     IFLA_VF_INFO (NESTED)
> >       IFLA_VF_MAC
> >       IFLA_VF_VLAN
> >       IFLA_VF_TX_RATE
> > 
> > This allows a single set to operate on multiple attributes if desired.
> > Among other things, it means a dump can be replayed to set state.
> > 
> > The current interface has yet to be released, so this seems like
> > something to consider for 2.6.34.
> > 
> > Signed-off-by: Chris Wright <chrisw@sous-sol.org
> 
> Very nice! This would be the minimum change to make the ABI conform
> to the general rules, so it would be really good to have that.
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> It does make the interface a bit strange (less than before), since the
> new IFLA_VF_INFO now contains three nested attributes that each contain their
> own vf number field, and we don't require that they are identical or that
> each of the nested attributes inside VF_INFO appears only once.
> 
> How about a second patch that splits out an IFLA_VF_NUMBER attribute
> and makes do_setvfinfo use nla_parse_nested instead of nla_for_each_nested
> in order to tighten the rules on this some more?

Yes, that's a great idea Arnd.  I'll tighten that up.

thanks,
-chris

Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric

[Stephen Hemminger]

On Mon, 17 May 2010 09:07:32 -0700
Chris Wright <chrisw@sous-sol.org> wrote:

> * Williams, Mitch A (mitch.a.williams@intel.com) wrote:
> > >-----Original Message-----
> > >From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> > >Sent: Monday, May 17, 2010 9:00 AM
> > >To: Chris Wright
> > >Cc: davem@davemloft.net; kaber@trash.net; Williams, Mitch A;
> > >arnd@arndb.de; scofeldm@cisco.com; netdev@vger.kernel.org
> > >Subject: Re: [PATCH] rtnetlink: make SR-IOV VF interface symmetric
> > >
> > >On Fri, 14 May 2010 20:14:16 -0700
> > >Chris Wright <chrisw@sous-sol.org> wrote:
> > >
> > >> Now we have a set of nested attributes:
> > >>
> > >>   IFLA_VFINFO_LIST (NESTED)
> > >>     IFLA_VF_INFO (NESTED)
> > >>       IFLA_VF_MAC
> > >>       IFLA_VF_VLAN
> > >>       IFLA_VF_TX_RATE
> > >>
> > >> This allows a single set to operate on multiple attributes if desired.
> > >> Among other things, it means a dump can be replayed to set state.
> > >>
> > >> The current interface has yet to be released, so this seems like
> > >> something to consider for 2.6.34.
> > >>
> > >> Signed-off-by: Chris Wright <chrisw@sous-sol.org
> > >> ---
> > >
> > >iproute2 update please?
> > >
> > >Also I would really like documentation on this.
> 
> And a pony? ;-)
> 
> Docs in what form?
> 

The man page for ip.8 already has some pieces from other version.
Just fix them.  I just pushed up a couple of changes.

--