[net-next-2.6 PATCH v3 1/5] pci: Add SR-IOV convenience functions and macros

[net-next-2.6 PATCH v3 1/5] pci: Add SR-IOV convenience functions and macros

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

Add and export pci_num_vf to allow other subsystems to determine how many
virtual function devices are associated with an SR-IOV physical function
device.
Add macros dev_is_pci, dev_is_ps, and dev_num_vf to make it easier for
non-PCI specific code to determine SR-IOV capabilities.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/pci/iov.c   |   15 +++++++++++++++
 include/linux/pci.h |   11 +++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index b2a448e..3e5ab2b 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -706,6 +706,21 @@ irqreturn_t pci_sriov_migration(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(pci_sriov_migration);
 
+/**
+ * pci_num_vf - return number of VFs associated with a PF device_release_driver
+ * @dev: the PCI device
+ *
+ * Returns number of VFs, or 0 if SR-IOV is not enabled.
+ */
+int pci_num_vf(struct pci_dev *dev)
+{
+	if (!dev || !dev->is_physfn)
+		return 0;
+	else
+		return dev->sriov->nr_virtfn;
+}
+EXPORT_SYMBOL_GPL(pci_num_vf);
+
 static int ats_alloc_one(struct pci_dev *dev, int ps)
 {
 	int pos;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 174e539..59a98e2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -612,6 +612,9 @@ extern void pci_remove_bus_device(struct pci_dev *dev);
 extern void pci_stop_bus_device(struct pci_dev *dev);
 void pci_setup_cardbus(struct pci_bus *bus);
 extern void pci_sort_breadthfirst(void);
+#define dev_is_pci(d) ((d)->bus == &pci_bus_type)
+#define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false))
+#define dev_num_vf(d) ((dev_is_pci(d) ? pci_num_vf(to_pci_dev(d)) : 0))
 
 /* Generic PCI functions exported to card drivers */
 
@@ -1129,6 +1132,9 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
 						unsigned int devfn)
 { return NULL; }
 
+#define dev_is_pci(d) (false)
+#define dev_is_pf(d) (false)
+#define dev_num_vf(d) (0)
 #endif /* CONFIG_PCI */
 
 /* Include architecture-dependent settings and functions */
@@ -1286,6 +1292,7 @@ void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar);
 extern int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
 extern void pci_disable_sriov(struct pci_dev *dev);
 extern irqreturn_t pci_sriov_migration(struct pci_dev *dev);
+extern int pci_num_vf(struct pci_dev *dev);
 #else
 static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
 {
@@ -1298,6 +1305,10 @@ static inline irqreturn_t pci_sriov_migration(struct pci_dev *dev)
 {
 	return IRQ_NONE;
 }
+static inline int pci_num_vf(struct pci_dev *dev)
+{
+	return 0;
+}
 #endif
 
 #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)

[net-next-2.6 PATCH v3 2/5] if_link: Add SR-IOV configuration methods

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

Add SR-IOV VF management methods to IFLA_LINKINFO. This allows userspace to
use rtnetlink to configure VF network devices.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 include/linux/if_link.h |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 6674791..c9bf92c 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -78,6 +78,11 @@ enum {
 #define IFLA_LINKINFO IFLA_LINKINFO
 	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_MAX
 };
 
@@ -196,4 +201,29 @@ enum macvlan_mode {
 	MACVLAN_MODE_BRIDGE  = 4, /* talk to bridge ports directly */
 };
 
+/* SR-IOV virtual function managment section */
+
+struct ifla_vf_mac {
+	__u32 vf;
+	__u8 mac[32]; /* MAX_ADDR_LEN */
+};
+
+struct ifla_vf_vlan {
+	__u32 vf;
+	__u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
+	__u32 qos;
+};
+
+struct ifla_vf_tx_rate {
+	__u32 vf;
+	__u32 rate; /* Max TX bandwidth in Mbps, 0 disables throttling */
+};
+
+struct ifla_vf_info {
+	__u32 vf;
+	__u8 mac[32];
+	__u32 vlan;
+	__u32 qos;
+	__u32 tx_rate;
+};
 #endif /* _LINUX_IF_LINK_H */

Re: [net-next-2.6 PATCH v3 2/5] if_link: Add SR-IOV configuration methods

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 10 Feb 2010 03:43:24 -0800

> From: Williams, Mitch A <mitch.a.williams@intel.com>
> 
> Add SR-IOV VF management methods to IFLA_LINKINFO. This allows userspace to
> use rtnetlink to configure VF network devices.
> 
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied to net-next-2.6

[net-next-2.6 PATCH v3 3/5] net: Add netdev ops for SR-IOV configuration

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

Add netdev ops for configuring SR-IOV VF devices through the PF driver.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 include/linux/netdevice.h |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e535700..49ef6d8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -28,6 +28,7 @@
 #include <linux/if.h>
 #include <linux/if_ether.h>
 #include <linux/if_packet.h>
+#include <linux/if_link.h>
 
 #ifdef __KERNEL__
 #include <linux/timer.h>
@@ -621,6 +622,13 @@ struct netdev_queue {
  *	this function is called when a VLAN id is unregistered.
  *
  * void (*ndo_poll_controller)(struct net_device *dev);
+ *
+ *	SR-IOV management functions.
+ * int (*ndo_set_vf_mac)(struct net_device *dev, int vf, u8* mac);
+ * int (*ndo_set_vf_vlan)(struct net_device *dev, int vf, u16 vlan, u8 qos);
+ * int (*ndo_set_vf_tx_rate)(struct net_device *dev, int vf, int rate);
+ * int (*ndo_get_vf_config)(struct net_device *dev,
+ *			    int vf, struct ifla_vf_info *ivf);
  */
 #define HAVE_NET_DEVICE_OPS
 struct net_device_ops {
@@ -660,6 +668,15 @@ struct net_device_ops {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	void                    (*ndo_poll_controller)(struct net_device *dev);
 #endif
+	int			(*ndo_set_vf_mac)(struct net_device *dev,
+						  int queue, u8 *mac);
+	int			(*ndo_set_vf_vlan)(struct net_device *dev,
+						   int queue, u16 vlan, u8 qos);
+	int			(*ndo_set_vf_tx_rate)(struct net_device *dev,
+						      int vf, int rate);
+	int			(*ndo_get_vf_config)(struct net_device *dev,
+						     int vf,
+						     struct ifla_vf_info *ivf);
 #if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
 	int			(*ndo_fcoe_enable)(struct net_device *dev);
 	int			(*ndo_fcoe_disable)(struct net_device *dev);

Re: [net-next-2.6 PATCH v3 3/5] net: Add netdev ops for SR-IOV configuration

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 10 Feb 2010 03:43:46 -0800

> From: Williams, Mitch A <mitch.a.williams@intel.com>
> 
> Add netdev ops for configuring SR-IOV VF devices through the PF driver.
> 
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied to net-next-2.6

[net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

Add code to allow rtnetlink clients to query and set VF information through
the PF driver.
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/core/rtnetlink.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 62f3878..42da96a 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -35,6 +35,7 @@
 #include <linux/security.h>
 #include <linux/mutex.h>
 #include <linux/if_addr.h>
+#include <linux/pci.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -580,6 +581,15 @@ static void copy_rtnl_link_stats(struct rtnl_link_stats *a,
 	a->tx_compressed = b->tx_compressed;
 };
 
+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
+		return 0;
+}
+
 static inline size_t if_nlmsg_size(const struct net_device *dev)
 {
 	return NLMSG_ALIGN(sizeof(struct ifinfomsg))
@@ -597,6 +607,8 @@ static inline size_t if_nlmsg_size(const struct net_device *dev)
 	       + nla_total_size(4) /* IFLA_MASTER */
 	       + 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_link_get_size(dev); /* IFLA_LINKINFO */
 }
 
@@ -665,6 +677,17 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	stats = dev_get_stats(dev);
 	copy_rtnl_link_stats(nla_data(attr), stats);
 
+	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++) {
+			if (dev->netdev_ops->ndo_get_vf_config(dev, i, &ivi))
+				break;
+			NLA_PUT(skb, IFLA_VFINFO, sizeof(ivi), &ivi);
+		}
+	}
 	if (dev->rtnl_link_ops) {
 		if (rtnl_link_fill(skb, dev) < 0)
 			goto nla_put_failure;
@@ -725,6 +748,12 @@ 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) },
 };
 EXPORT_SYMBOL(ifla_policy);
 
@@ -898,6 +927,44 @@ 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]);
+		write_lock_bh(&dev_base_lock);
+		if (ops->ndo_set_vf_mac)
+			err = ops->ndo_set_vf_mac(dev, ivm->vf, ivm->mac);
+		write_unlock_bh(&dev_base_lock);
+		if (err < 0)
+			goto errout;
+		modified = 1;
+	}
+
+	if (tb[IFLA_VF_VLAN]) {
+		struct ifla_vf_vlan *ivv;
+		ivv = nla_data(tb[IFLA_VF_VLAN]);
+		write_lock_bh(&dev_base_lock);
+		if (ops->ndo_set_vf_vlan)
+			err = ops->ndo_set_vf_vlan(dev, ivv->vf,
+						   (u16)ivv->vlan,
+						   (u8)ivv->qos);
+		write_unlock_bh(&dev_base_lock);
+		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]);
+		write_lock_bh(&dev_base_lock);
+		if (ops->ndo_set_vf_tx_rate)
+			err = ops->ndo_set_vf_tx_rate(dev, ivt->vf, ivt->rate);
+		write_unlock_bh(&dev_base_lock);
+		if (err < 0)
+			goto errout;
+		modified = 1;
+	}
 	err = 0;
 
 errout:

Re: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[Patrick McHardy]

Jeff Kirsher wrote:
> @@ -665,6 +677,17 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  	stats = dev_get_stats(dev);
>  	copy_rtnl_link_stats(nla_data(attr), stats);
>  
> +	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++) {
> +			if (dev->netdev_ops->ndo_get_vf_config(dev, i, &ivi))
> +				break;
> +			NLA_PUT(skb, IFLA_VFINFO, sizeof(ivi), &ivi);

We usually encapsulate lists of the same attribute type in another
top-level attribute. Check out the IFLA_VLAN_*_QOS attributes for
an example.

The interface should also be symetrical, IOW you should dump the
same attributes used in the userspace->kernel direction instead
of a combined "info" attribute.

> +		}
> +	}

> @@ -898,6 +927,44 @@ 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]);
> +		write_lock_bh(&dev_base_lock);

It dev_base_lock really correct here? This is running under the RTNL, so
changes to the device list can't happen.

> +		if (ops->ndo_set_vf_mac)
> +			err = ops->ndo_set_vf_mac(dev, ivm->vf, ivm->mac);

Shouldn't this indicate an error if the attributes aren't supported?

> +		write_unlock_bh(&dev_base_lock);
> +		if (err < 0)
> +			goto errout;
> +		modified = 1;
> +	}
> +
> +	if (tb[IFLA_VF_VLAN]) {
> +		struct ifla_vf_vlan *ivv;
> +		ivv = nla_data(tb[IFLA_VF_VLAN]);
> +		write_lock_bh(&dev_base_lock);
> +		if (ops->ndo_set_vf_vlan)
> +			err = ops->ndo_set_vf_vlan(dev, ivv->vf,
> +						   (u16)ivv->vlan,
> +						   (u8)ivv->qos);

The casts aren't necessary. But why does struct ifla_vf_vlan use u32
for the vlan in the first place?

> +		write_unlock_bh(&dev_base_lock);
> +		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]);
> +		write_lock_bh(&dev_base_lock);
> +		if (ops->ndo_set_vf_tx_rate)
> +			err = ops->ndo_set_vf_tx_rate(dev, ivt->vf, ivt->rate);
> +		write_unlock_bh(&dev_base_lock);
> +		if (err < 0)
> +			goto errout;
> +		modified = 1;
> +	}
>  	err = 0;
>  
>  errout:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[Williams, Mitch A]

>From: Patrick McHardy [mailto:kaber@trash.net]
[snip]
>
>We usually encapsulate lists of the same attribute type in another
>top-level attribute. Check out the IFLA_VLAN_*_QOS attributes for
>an example.
>
>The interface should also be symetrical, IOW you should dump the
>same attributes used in the userspace->kernel direction instead
>of a combined "info" attribute.

Sheesh, Patrick, where were you three months ago when I first
posted this stuff? It would have helped a lot if I heard from
you back then. We've had at least five internal review cycles
here and nobody caught this, mostly because nobody understands
it.

That being said, I'll take another look at the NLA_NESTED stuff
and see what I can figure out. Do you know of any place (outside
of the code) where this is documented?  It's particularly
difficult to follow this code.

I see your point about symmetrical interfaces, but I'm not sure
it's the best thing here. We want the user to be able to set these
attributes independently, without blowing away any other settings.
If we put all three settings together into one data structure,
the code flow will end up being much more complicated.

I'd prefer to leave the data structures as they are, and switch
to using nested attributes for the status reporting part, i.e.
what happens when you type 'ip link show'. Would this work for you?

>
>It dev_base_lock really correct here? This is running under the RTNL, so
>changes to the device list can't happen.
>

Good catch - I'll pull out the lock.


>> +		if (ops->ndo_set_vf_mac)
>> +			err = ops->ndo_set_vf_mac(dev, ivm->vf, ivm->mac);
>
>Shouldn't this indicate an error if the attributes aren't supported?

Yes. I'll fix this.

>The casts aren't necessary. But why does struct ifla_vf_vlan use u32
>for the vlan in the first place?

I used u32 for all of the values because that's what everything else
used. All the stuff in iproute2 seems to like u32 size as well.

The casts aren't necessary for the compiler, but I put them in for
readability purposes - to make it obvious. I can remove them if 
they're objectionable.

Thanks for your review, Patrick.

-Mitch

Re: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[Patrick McHardy]

Williams, Mitch A wrote:
>> From: Patrick McHardy [mailto:kaber@trash.net]
> [snip]
>> We usually encapsulate lists of the same attribute type in another
>> top-level attribute. Check out the IFLA_VLAN_*_QOS attributes for
>> an example.
>>
>> The interface should also be symetrical, IOW you should dump the
>> same attributes used in the userspace->kernel direction instead
>> of a combined "info" attribute.
> 
> Sheesh, Patrick, where were you three months ago when I first
> posted this stuff? It would have helped a lot if I heard from
> you back then. We've had at least five internal review cycles
> here and nobody caught this, mostly because nobody understands
> it.

Well .. sorry :)

> That being said, I'll take another look at the NLA_NESTED stuff
> and see what I can figure out. Do you know of any place (outside
> of the code) where this is documented?  It's particularly
> difficult to follow this code.

No, but its quite simple. For dumping attributes, you create
a top-level nested attribute, then add all inner attributes
and "end" the top-level attribute, which fills in the entire
length. In terms of the kernel netlink helpers:

	nest = nla_nest_start(skb, IFLA_VF);
	if (nest == NULL)
		goto nla_put_failure;

	for (...) {
		<get info>
		NLA_PUT(skb, IFLA_VF_INFO, &info);
	}

	nla_nest_end(skb, nest);

For parsing withing the kernel, you use nla_for_each_nested()
to iterate over the encapsulated attributes and (unless the
inner attributes are nested themselves) nla_data() to get
at the payload:

	nla_for_each_nested(attr, nla[IFLA_VF_INFO], rem) {
		if (nla_type(attr) != MY_TYPE)
			goto err_inval;
		info = nla_data(attr);
		<do something with info>
	}

You can also put nested attributes into the list, in that case
you'd use nla_parse_nested(attr, ...) instead of nla_data().

> I see your point about symmetrical interfaces, but I'm not sure
> it's the best thing here. We want the user to be able to set these
> attributes independently, without blowing away any other settings.
> If we put all three settings together into one data structure,
> the code flow will end up being much more complicated.

Using a symetrical interface doesn't necessarily prevent incremental
configuration. Please see below.

> I'd prefer to leave the data structures as they are, and switch
> to using nested attributes for the status reporting part, i.e.
> what happens when you type 'ip link show'. Would this work for you?

The way rtnetlink is currently built the only difference between
configuration requests from userspace and notifications from the
kernel is the NLM_F_REQUEST bit. This is a useful property because
you can re-create objects in the kernel simply be replaying a
notification as request. Its also necessary to be able to use
the same parsing functions for notifications and error messages
in userspace (error messages have the original request appended).

What should be relatively easy to do is to use lists of (combined
or non-combined) attributes in both directions. In the userspace->
kernel direction all you need to change is to encapsulate your
new attributes and walk through the list. In the kernel->userspace
direction you can use the combined struct within the kernel if
thats more useful and translate it into individual attributes when
filling the netlink message.

Re: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[Patrick McHardy]

Williams, Mitch A wrote:
>> From: Patrick McHardy [mailto:kaber@trash.net]
> [snip]
>> We usually encapsulate lists of the same attribute type in another
>> top-level attribute. Check out the IFLA_VLAN_*_QOS attributes for
>> an example.
>>
>> The interface should also be symetrical, IOW you should dump the
>> same attributes used in the userspace->kernel direction instead
>> of a combined "info" attribute.
> 
> Sheesh, Patrick, where were you three months ago when I first
> posted this stuff? It would have helped a lot if I heard from
> you back then. We've had at least five internal review cycles
> here and nobody caught this, mostly because nobody understands
> it.
> 
> That being said, I'll take another look at the NLA_NESTED stuff
> and see what I can figure out. Do you know of any place (outside
> of the code) where this is documented?  It's particularly
> difficult to follow this code.
> 
> I see your point about symmetrical interfaces, but I'm not sure
> it's the best thing here. We want the user to be able to set these
> attributes independently, without blowing away any other settings.
> If we put all three settings together into one data structure,
> the code flow will end up being much more complicated.
> 
> I'd prefer to leave the data structures as they are, and switch
> to using nested attributes for the status reporting part, i.e.
> what happens when you type 'ip link show'. Would this work for you?
> 
>> It dev_base_lock really correct here? This is running under the RTNL, so
>> changes to the device list can't happen.
>>
> 
> Good catch - I'll pull out the lock.
> 
> 
>>> +		if (ops->ndo_set_vf_mac)
>>> +			err = ops->ndo_set_vf_mac(dev, ivm->vf, ivm->mac);
>> Shouldn't this indicate an error if the attributes aren't supported?
> 
> Yes. I'll fix this.
> 
>> The casts aren't necessary. But why does struct ifla_vf_vlan use u32
>> for the vlan in the first place?
> 
> I used u32 for all of the values because that's what everything else
> used. All the stuff in iproute2 seems to like u32 size as well.
> 
> The casts aren't necessary for the compiler, but I put them in for
> readability purposes - to make it obvious. I can remove them if 
> they're objectionable.

I just noticed the patch went in without any of these changes.
Are you going to fix this up?

RE: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[Williams, Mitch A]

>-----Original Message-----
>From: Patrick McHardy [mailto:kaber@trash.net]

[snip]

>
>I just noticed the patch went in without any of these changes.
>Are you going to fix this up?

I've got a patch in Jeff's queue that takes care of the low-hanging
fruit - return codes and excessive locking. You should see this go
up pretty quickly.
 
I'm planning to look into the data structure changes today. 

-Mitch

Re: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[Patrick McHardy]

Williams, Mitch A wrote:
>> -----Original Message-----
>> From: Patrick McHardy [mailto:kaber@trash.net]
> 
> [snip]
> 
>> I just noticed the patch went in without any of these changes.
>> Are you going to fix this up?
> 
> I've got a patch in Jeff's queue that takes care of the low-hanging
> fruit - return codes and excessive locking. You should see this go
> up pretty quickly.
>  
> I'm planning to look into the data structure changes today. 

Thanks for the information.

Re: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 10 Feb 2010 03:44:05 -0800

> From: Williams, Mitch A <mitch.a.williams@intel.com>
> 
> Add code to allow rtnetlink clients to query and set VF information through
> the PF driver.
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied to net-next-2.6

[net-next-2.6 PATCH v3 5/5] igb: support for VF configuration tools

[Jeff Kirsher]

From: Williams, Mitch A <mitch.a.williams@intel.com>

Add support to the igb driver for VF configuration mechanisms through the
PF interface.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/e1000_82575.h |    3 +
 drivers/net/igb/e1000_regs.h  |    1 
 drivers/net/igb/igb.h         |    3 +
 drivers/net/igb/igb_main.c    |  130 ++++++++++++++++++++++++++++++++++++++---
 4 files changed, 126 insertions(+), 11 deletions(-)

diff --git a/drivers/net/igb/e1000_82575.h b/drivers/net/igb/e1000_82575.h
index d51c992..bb53083 100644
--- a/drivers/net/igb/e1000_82575.h
+++ b/drivers/net/igb/e1000_82575.h
@@ -219,6 +219,9 @@ struct e1000_adv_tx_context_desc {
 #define E1000_VLVF_LVLAN          0x00100000
 #define E1000_VLVF_VLANID_ENABLE  0x80000000
 
+#define E1000_VMVIR_VLANA_DEFAULT      0x40000000 /* Always use default VLAN */
+#define E1000_VMVIR_VLANA_NEVER        0x80000000 /* Never insert VLAN tag */
+
 #define E1000_IOVCTL 0x05BBC
 #define E1000_IOVCTL_REUSE_VFQ 0x00000001
 
diff --git a/drivers/net/igb/e1000_regs.h b/drivers/net/igb/e1000_regs.h
index dd4e6ff..abb7333 100644
--- a/drivers/net/igb/e1000_regs.h
+++ b/drivers/net/igb/e1000_regs.h
@@ -310,6 +310,7 @@
 #define E1000_VMOLR(_n)        (0x05AD0 + (4 * (_n)))
 #define E1000_VLVF(_n)         (0x05D00 + (4 * (_n))) /* VLAN Virtual Machine
                                                        * Filter - RW */
+#define E1000_VMVIR(_n)        (0x03700 + (4 * (_n)))
 
 #define wr32(reg, value) (writel(value, hw->hw_addr + reg))
 #define rd32(reg) (readl(hw->hw_addr + reg))
diff --git a/drivers/net/igb/igb.h b/drivers/net/igb/igb.h
index b1c1eb8..83ea117 100644
--- a/drivers/net/igb/igb.h
+++ b/drivers/net/igb/igb.h
@@ -75,11 +75,14 @@ struct vf_data_storage {
 	u16 vlans_enabled;
 	u32 flags;
 	unsigned long last_nack;
+	u16 pf_vlan; /* When set, guest VLAN config not allowed. */
+	u16 pf_qos;
 };
 
 #define IGB_VF_FLAG_CTS            0x00000001 /* VF is clear to send data */
 #define IGB_VF_FLAG_UNI_PROMISC    0x00000002 /* VF has unicast promisc */
 #define IGB_VF_FLAG_MULTI_PROMISC  0x00000004 /* VF has multicast promisc */
+#define IGB_VF_FLAG_PF_SET_MAC     0x00000008 /* PF has set MAC address */
 
 /* RX descriptor control thresholds.
  * PTHRESH - MAC will consider prefetch if it has fewer than this number of
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 83cd0d7..879362d 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -133,6 +133,12 @@ static void igb_msg_task(struct igb_adapter *);
 static void igb_vmm_control(struct igb_adapter *);
 static int igb_set_vf_mac(struct igb_adapter *, int, unsigned char *);
 static void igb_restore_vf_multicasts(struct igb_adapter *adapter);
+static int igb_ndo_set_vf_mac(struct net_device *netdev, int vf, u8 *mac);
+static int igb_ndo_set_vf_vlan(struct net_device *netdev,
+			       int vf, u16 vlan, u8 qos);
+static int igb_ndo_set_vf_bw(struct net_device *netdev, int vf, int tx_rate);
+static int igb_ndo_get_vf_config(struct net_device *netdev, int vf,
+				 struct ifla_vf_info *ivi);
 
 #ifdef CONFIG_PM
 static int igb_suspend(struct pci_dev *, pm_message_t);
@@ -1352,6 +1358,10 @@ static const struct net_device_ops igb_netdev_ops = {
 	.ndo_vlan_rx_register	= igb_vlan_rx_register,
 	.ndo_vlan_rx_add_vid	= igb_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= igb_vlan_rx_kill_vid,
+	.ndo_set_vf_mac		= igb_ndo_set_vf_mac,
+	.ndo_set_vf_vlan	= igb_ndo_set_vf_vlan,
+	.ndo_set_vf_tx_rate	= igb_ndo_set_vf_bw,
+	.ndo_get_vf_config	= igb_ndo_get_vf_config,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= igb_netpoll,
 #endif
@@ -2479,7 +2489,8 @@ static void igb_rlpml_set(struct igb_adapter *adapter)
 	wr32(E1000_RLPML, max_frame_size);
 }
 
-static inline void igb_set_vmolr(struct igb_adapter *adapter, int vfn)
+static inline void igb_set_vmolr(struct igb_adapter *adapter,
+				 int vfn, bool aupe)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	u32 vmolr;
@@ -2492,8 +2503,11 @@ static inline void igb_set_vmolr(struct igb_adapter *adapter, int vfn)
 		return;
 
 	vmolr = rd32(E1000_VMOLR(vfn));
-	vmolr |= E1000_VMOLR_AUPE |        /* Accept untagged packets */
-	         E1000_VMOLR_STRVLAN;      /* Strip vlan tags */
+	vmolr |= E1000_VMOLR_STRVLAN;      /* Strip vlan tags */
+	if (aupe)
+		vmolr |= E1000_VMOLR_AUPE;        /* Accept untagged packets */
+	else
+		vmolr &= ~(E1000_VMOLR_AUPE); /* Tagged packets ONLY */
 
 	/* clear all bits that might not be set */
 	vmolr &= ~(E1000_VMOLR_BAM | E1000_VMOLR_RSSE);
@@ -2564,7 +2578,7 @@ void igb_configure_rx_ring(struct igb_adapter *adapter,
 	wr32(E1000_SRRCTL(reg_idx), srrctl);
 
 	/* set filtering for VMDQ pools */
-	igb_set_vmolr(adapter, reg_idx & 0x7);
+	igb_set_vmolr(adapter, reg_idx & 0x7, true);
 
 	/* enable receive descriptor fetching */
 	rxdctl = rd32(E1000_RXDCTL(reg_idx));
@@ -4490,10 +4504,57 @@ static s32 igb_vlvf_set(struct igb_adapter *adapter, u32 vid, bool add, u32 vf)
 				reg |= size;
 				wr32(E1000_VMOLR(vf), reg);
 			}
-			return 0;
 		}
 	}
-	return -1;
+	return 0;
+}
+
+static void igb_set_vmvir(struct igb_adapter *adapter, u32 vid, u32 vf)
+{
+	struct e1000_hw *hw = &adapter->hw;
+
+	if (vid)
+		wr32(E1000_VMVIR(vf), (vid | E1000_VMVIR_VLANA_DEFAULT));
+	else
+		wr32(E1000_VMVIR(vf), 0);
+}
+
+static int igb_ndo_set_vf_vlan(struct net_device *netdev,
+			       int vf, u16 vlan, u8 qos)
+{
+	int err = 0;
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	if ((vf >= adapter->vfs_allocated_count) || (vlan > 4095) || (qos > 7))
+		return -EINVAL;
+	if (vlan || qos) {
+		err = igb_vlvf_set(adapter, vlan, !!vlan, vf);
+		if (err)
+			goto out;
+		igb_set_vmvir(adapter, vlan | (qos << VLAN_PRIO_SHIFT), vf);
+		igb_set_vmolr(adapter, vf, !vlan);
+		adapter->vf_data[vf].pf_vlan = vlan;
+		adapter->vf_data[vf].pf_qos = qos;
+		dev_info(&adapter->pdev->dev,
+			 "Setting VLAN %d, QOS 0x%x on VF %d\n", vlan, qos, vf);
+		if (test_bit(__IGB_DOWN, &adapter->state)) {
+			dev_warn(&adapter->pdev->dev,
+				 "The VF VLAN has been set,"
+				 " but the PF device is not up.\n");
+			dev_warn(&adapter->pdev->dev,
+				 "Bring the PF device up before"
+				 " attempting to use the VF device.\n");
+		}
+	} else {
+		igb_vlvf_set(adapter, adapter->vf_data[vf].pf_vlan,
+				   false, vf);
+		igb_set_vmvir(adapter, vlan, vf);
+		igb_set_vmolr(adapter, vf, true);
+		adapter->vf_data[vf].pf_vlan = 0;
+		adapter->vf_data[vf].pf_qos = 0;
+       }
+out:
+       return err;
 }
 
 static int igb_set_vf_vlan(struct igb_adapter *adapter, u32 *msgbuf, u32 vf)
@@ -4506,15 +4567,21 @@ static int igb_set_vf_vlan(struct igb_adapter *adapter, u32 *msgbuf, u32 vf)
 
 static inline void igb_vf_reset(struct igb_adapter *adapter, u32 vf)
 {
-	/* clear all flags */
-	adapter->vf_data[vf].flags = 0;
+	/* clear flags */
+	adapter->vf_data[vf].flags &= ~(IGB_VF_FLAG_PF_SET_MAC);
 	adapter->vf_data[vf].last_nack = jiffies;
 
 	/* reset offloads to defaults */
-	igb_set_vmolr(adapter, vf);
+	igb_set_vmolr(adapter, vf, true);
 
 	/* reset vlans for device */
 	igb_clear_vf_vfta(adapter, vf);
+	if (adapter->vf_data[vf].pf_vlan)
+		igb_ndo_set_vf_vlan(adapter->netdev, vf,
+				    adapter->vf_data[vf].pf_vlan,
+				    adapter->vf_data[vf].pf_qos);
+	else
+		igb_clear_vf_vfta(adapter, vf);
 
 	/* reset multicast table array for vf */
 	adapter->vf_data[vf].num_vf_mc_hashes = 0;
@@ -4528,7 +4595,8 @@ static void igb_vf_reset_event(struct igb_adapter *adapter, u32 vf)
 	unsigned char *vf_mac = adapter->vf_data[vf].vf_mac_addresses;
 
 	/* generate a new mac address as we were hotplug removed/added */
-	random_ether_addr(vf_mac);
+	if (!(adapter->vf_data[vf].flags & IGB_VF_FLAG_PF_SET_MAC))
+		random_ether_addr(vf_mac);
 
 	/* process remaining reset events */
 	igb_vf_reset(adapter, vf);
@@ -4641,7 +4709,10 @@ static void igb_rcv_msg_from_vf(struct igb_adapter *adapter, u32 vf)
 		retval = igb_set_vf_rlpml(adapter, msgbuf[1], vf);
 		break;
 	case E1000_VF_SET_VLAN:
-		retval = igb_set_vf_vlan(adapter, msgbuf, vf);
+		if (adapter->vf_data[vf].pf_vlan)
+			retval = -1;
+		else
+			retval = igb_set_vf_vlan(adapter, msgbuf, vf);
 		break;
 	default:
 		dev_err(&pdev->dev, "Unhandled Msg %08x\n", msgbuf[0]);
@@ -6003,6 +6074,43 @@ static int igb_set_vf_mac(struct igb_adapter *adapter,
 	return 0;
 }
 
+static int igb_ndo_set_vf_mac(struct net_device *netdev, int vf, u8 *mac)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	if (!is_valid_ether_addr(mac) || (vf >= adapter->vfs_allocated_count))
+		return -EINVAL;
+	adapter->vf_data[vf].flags |= IGB_VF_FLAG_PF_SET_MAC;
+	dev_info(&adapter->pdev->dev, "setting MAC %pM on VF %d\n", mac, vf);
+	dev_info(&adapter->pdev->dev, "Reload the VF driver to make this"
+				      " change effective.");
+	if (test_bit(__IGB_DOWN, &adapter->state)) {
+		dev_warn(&adapter->pdev->dev, "The VF MAC address has been set,"
+			 " but the PF device is not up.\n");
+		dev_warn(&adapter->pdev->dev, "Bring the PF device up before"
+			 " attempting to use the VF device.\n");
+	}
+	return igb_set_vf_mac(adapter, vf, mac);
+}
+
+static int igb_ndo_set_vf_bw(struct net_device *netdev, int vf, int tx_rate)
+{
+	return -EOPNOTSUPP;
+}
+
+static int igb_ndo_get_vf_config(struct net_device *netdev,
+				 int vf, struct ifla_vf_info *ivi)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	if (vf >= adapter->vfs_allocated_count)
+		return -EINVAL;
+	ivi->vf = vf;
+	memcpy(&ivi->mac, adapter->vf_data[vf].vf_mac_addresses, ETH_ALEN);
+	ivi->tx_rate = 0;
+	ivi->vlan = adapter->vf_data[vf].pf_vlan;
+	ivi->qos = adapter->vf_data[vf].pf_qos;
+	return 0;
+}
+
 static void igb_vmm_control(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;

Re: [net-next-2.6 PATCH v3 5/5] igb: support for VF configuration tools

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 10 Feb 2010 03:44:24 -0800

> From: Williams, Mitch A <mitch.a.williams@intel.com>
> 
> Add support to the igb driver for VF configuration mechanisms through the
> PF interface.
> 
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied to net-next-2.6

Re: [net-next-2.6 PATCH v3 1/5] pci: Add SR-IOV convenience functions and macros

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 10 Feb 2010 03:43:04 -0800

> From: Williams, Mitch A <mitch.a.williams@intel.com>
> 
> Add and export pci_num_vf to allow other subsystems to determine how many
> virtual function devices are associated with an SR-IOV physical function
> device.
> Add macros dev_is_pci, dev_is_ps, and dev_num_vf to make it easier for
> non-PCI specific code to determine SR-IOV capabilities.
> 
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied to net-next-2.6