[net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile (was iovnl)

[net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile (was iovnl)

[Scott Feldman]

From: Scott Feldman <scofeldm@cisco.com>

(This is take #2 on the iovnl patches posted earlier based on feedback from
Chris Wright, Arnd Bergmann, and others.  Thanks guys!)

Add new netdev ops ndo_set_vf_port_profile to allow setting of port-profile
on VF, along the lines of existing nds_set_vf_* ops.  Extends RTM_SETLINK
with new sub cmd called IFLA_VF_PORT_PROFILE (added to end on cmd list).  The
port-profile cmd arguments are (as seen from iproute2 cmdline):

       ip link set DEVICE [ { up | down } ]
                          ...
                          [ vf NUM [ mac LLADDR ]
                                   [ vlan VLANID [ qos VLAN-QOS ] ]
                                   [ rate TXRATE ] ] 
                                   [ port_profile [ PORT-PROFILE
                                           [ mac LLADDR ]
                                           [ host_uuid HOST_UUID ]
                                           [ client_uuid CLIENT_UUID ]
                                           [ client_name CLIENT_NAME ] ] ] ]


I took some liberties and s/SR-IOV/IOV in the code comments around the
ndo_set_vf_* cmds as they can apply to both SR-IOV and non-SR-IOV adapters,
as long as there is a PF:VF parent:child relationship.

A port-profile is used to configure/enable the network port backing the VF, not
to configure the host-facing side of the VF.

Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Roopa Prabhu<roprabhu@cisco.com>
---
 include/linux/if_link.h   |   15 +++++++++++++--
 include/linux/netdevice.h |   11 ++++++++++-
 net/core/rtnetlink.c      |   20 ++++++++++++++++++++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index cfd420b..2c5cc65 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -110,12 +110,13 @@ 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_NUM_VF,		/* Number of VFs if device is IOV PF */
 	IFLA_VF_MAC,		/* Hardware queue specific attributes */
 	IFLA_VF_VLAN,
 	IFLA_VF_TX_RATE,	/* TX Bandwidth Allocation */
 	IFLA_VFINFO,
 	IFLA_STATS64,
+	IFLA_VF_PORT_PROFILE,
 	__IFLA_MAX
 };
 
@@ -234,7 +235,7 @@ enum macvlan_mode {
 	MACVLAN_MODE_BRIDGE  = 4, /* talk to bridge ports directly */
 };
 
-/* SR-IOV virtual function managment section */
+/* IOV virtual function managment section */
 
 struct ifla_vf_mac {
 	__u32 vf;
@@ -259,4 +260,14 @@ struct ifla_vf_info {
 	__u32 qos;
 	__u32 tx_rate;
 };
+
+struct ifla_vf_port_profile {
+	__u32 vf;
+	__u8 port_profile[64];
+	__u8 mac[32];
+	__u8 host_uuid[64]; /* e.g. "CEEFD3B1-9E11-11DE-BDFD-000BAB01C0FB" */
+	__u8 client_uuid[64];
+	__u8 client_name[64]; /* e.g. "vm0-eth1" */
+};
+
 #endif /* _LINUX_IF_LINK_H */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3c5ed5f..26dd4cb 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -690,10 +690,13 @@ struct netdev_rx_queue {
  *
  * void (*ndo_poll_controller)(struct net_device *dev);
  *
- *	SR-IOV management functions.
+ *	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_set_vf_port_profile)(struct net_device *dev, int vf,
+ *				  u8 *port_profile, u8 *mac, u8 *host_uuid,
+ *				  u8 *client_uuid, u8 *client_name);
  * int (*ndo_get_vf_config)(struct net_device *dev,
  *			    int vf, struct ifla_vf_info *ivf);
  */
@@ -741,6 +744,12 @@ struct net_device_ops {
 						   int queue, u16 vlan, u8 qos);
 	int			(*ndo_set_vf_tx_rate)(struct net_device *dev,
 						      int vf, int rate);
+	int			(*ndo_set_vf_port_profile)(
+					struct net_device *dev, int vf,
+					u8 *port_profile, u8 *mac,
+					u8 *host_uuid,
+					u8 *client_uuid,
+					u8 *client_name);
 	int			(*ndo_get_vf_config)(struct net_device *dev,
 						     int vf,
 						     struct ifla_vf_info *ivf);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 78c8598..7268e8e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -824,6 +824,8 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 				    .len = sizeof(struct ifla_vf_vlan) },
 	[IFLA_VF_TX_RATE]	= { .type = NLA_BINARY,
 				    .len = sizeof(struct ifla_vf_tx_rate) },
+	[IFLA_VF_PORT_PROFILE]	= { .type = NLA_BINARY,
+				    .len = sizeof(struct ifla_vf_port_profile)},
 };
 EXPORT_SYMBOL(ifla_policy);
 
@@ -1028,6 +1030,24 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 	}
 	err = 0;
 
+	if (tb[IFLA_VF_PORT_PROFILE]) {
+		struct ifla_vf_port_profile *ivp;
+		ivp = nla_data(tb[IFLA_VF_PORT_PROFILE]);
+		err = -EOPNOTSUPP;
+		if (ops->ndo_set_vf_port_profile)
+			ivp->port_profile[sizeof(ivp->port_profile)-1] = 0;
+			ivp->host_uuid[sizeof(ivp->host_uuid)-1] = 0;
+			ivp->client_uuid[sizeof(ivp->client_uuid)-1] = 0;
+			ivp->client_name[sizeof(ivp->client_name)-1] = 0;
+			err = ops->ndo_set_vf_port_profile(dev, ivp->vf,
+				ivp->port_profile, ivp->mac, ivp->host_uuid,
+				ivp->client_uuid, ivp->client_name);
+		if (err < 0)
+			goto errout;
+		modified = 1;
+	}
+	err = 0;
+
 errout:
 	if (err < 0 && modified && net_ratelimit())
 		printk(KERN_WARNING "A link change request failed with "

[net-next-2.6 PATCH 2/2] add enic ndo_vf_set_port_profile op support for dynamic vnics

[Scott Feldman]

From: Scott Feldman <scofeldm@cisco.com>

Add enic ndo_vf_set_port_profile op to support setting port-profile for
dynamic vnics.  Enic dynamic vnics are just like normal enic eth vnics except
dynamic vnics require an extra configuration step to assign a port-profile
identifier to the interface before the interface is useable. Once assigned,
link comes up on the interface and is ready for I/O.  The port-profile is
used to configure the network port assigned to the interface.  The network
port configuration includes VLAN membership, QoS policies, and port security
settings typical of a data center network.

Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Roopa Prabhu<roprabhu@cisco.com>
---
 drivers/net/enic/Makefile    |    2 -
 drivers/net/enic/enic.h      |    2 -
 drivers/net/enic/enic_main.c |  144 +++++++++++++++++++++++++++++++++++-------
 drivers/net/enic/vnic_dev.c  |   50 +++++++++++++++
 drivers/net/enic/vnic_dev.h  |    3 +
 drivers/net/enic/vnic_vic.c  |   73 +++++++++++++++++++++
 drivers/net/enic/vnic_vic.h  |   59 +++++++++++++++++
 7 files changed, 306 insertions(+), 27 deletions(-)

diff --git a/drivers/net/enic/Makefile b/drivers/net/enic/Makefile
index 391c3bc..e7b6c31 100644
--- a/drivers/net/enic/Makefile
+++ b/drivers/net/enic/Makefile
@@ -1,5 +1,5 @@
 obj-$(CONFIG_ENIC) := enic.o
 
 enic-y := enic_main.o vnic_cq.o vnic_intr.o vnic_wq.o \
-	enic_res.o vnic_dev.o vnic_rq.o
+	enic_res.o vnic_dev.o vnic_rq.o vnic_vic.o
 
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 5fa56f1..0956ca9 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -34,7 +34,7 @@
 
 #define DRV_NAME		"enic"
 #define DRV_DESCRIPTION		"Cisco VIC Ethernet NIC Driver"
-#define DRV_VERSION		"1.3.1.1"
+#define DRV_VERSION		"1.3.1.1-iov"
 #define DRV_COPYRIGHT		"Copyright 2008-2009 Cisco Systems, Inc"
 #define PFX			DRV_NAME ": "
 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 1232887..9a0305e 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -29,6 +29,7 @@
 #include <linux/etherdevice.h>
 #include <linux/if_ether.h>
 #include <linux/if_vlan.h>
+#include <linux/if_link.h>
 #include <linux/ethtool.h>
 #include <linux/in.h>
 #include <linux/ip.h>
@@ -40,6 +41,7 @@
 #include "vnic_dev.h"
 #include "vnic_intr.h"
 #include "vnic_stats.h"
+#include "vnic_vic.h"
 #include "enic_res.h"
 #include "enic.h"
 
@@ -49,10 +51,12 @@
 #define ENIC_DESC_MAX_SPLITS		(MAX_TSO / WQ_ENET_MAX_DESC_LEN + 1)
 
 #define PCI_DEVICE_ID_CISCO_VIC_ENET         0x0043  /* ethernet vnic */
+#define PCI_DEVICE_ID_CISCO_VIC_ENET_DYN     0x0044  /* enet dynamic vnic */
 
 /* Supported devices */
 static DEFINE_PCI_DEVICE_TABLE(enic_id_table) = {
 	{ PCI_VDEVICE(CISCO, PCI_DEVICE_ID_CISCO_VIC_ENET) },
+	{ PCI_VDEVICE(CISCO, PCI_DEVICE_ID_CISCO_VIC_ENET_DYN) },
 	{ 0, }	/* end of table */
 };
 
@@ -113,6 +117,11 @@ static const struct enic_stat enic_rx_stats[] = {
 static const unsigned int enic_n_tx_stats = ARRAY_SIZE(enic_tx_stats);
 static const unsigned int enic_n_rx_stats = ARRAY_SIZE(enic_rx_stats);
 
+static int enic_is_dynamic(struct enic *enic)
+{
+	return enic->pdev->device == PCI_DEVICE_ID_CISCO_VIC_ENET_DYN;
+}
+
 static int enic_get_settings(struct net_device *netdev,
 	struct ethtool_cmd *ecmd)
 {
@@ -810,14 +819,24 @@ static void enic_reset_mcaddrs(struct enic *enic)
 
 static int enic_set_mac_addr(struct net_device *netdev, char *addr)
 {
-	if (!is_valid_ether_addr(addr))
-		return -EADDRNOTAVAIL;
+	struct enic *enic = netdev_priv(netdev);
 
-	memcpy(netdev->dev_addr, addr, netdev->addr_len);
+	if (enic_is_dynamic(enic)) {
+		random_ether_addr(netdev->dev_addr);
+	} else {
+		if (!is_valid_ether_addr(addr))
+			return -EADDRNOTAVAIL;
+		memcpy(netdev->dev_addr, addr, netdev->addr_len);
+	}
 
 	return 0;
 }
 
+static int enic_set_mac_address(struct net_device *netdev, void *p)
+{
+	return -EOPNOTSUPP;
+}
+
 /* netif_tx_lock held, BHs disabled */
 static void enic_set_multicast_list(struct net_device *netdev)
 {
@@ -922,6 +941,76 @@ static void enic_tx_timeout(struct net_device *netdev)
 	schedule_work(&enic->reset);
 }
 
+static int enic_vnic_dev_deinit(struct enic *enic)
+{
+	int err;
+
+	spin_lock(&enic->devcmd_lock);
+	err = vnic_dev_deinit(enic->vdev);
+	spin_unlock(&enic->devcmd_lock);
+	return err;
+}
+
+static int enic_dev_init_prov(struct enic *enic, struct vic_provinfo *vp)
+{
+	int err;
+
+	spin_lock(&enic->devcmd_lock);
+	err = vnic_dev_init_prov(enic->vdev, (u8 *)vp, vic_provinfo_size(vp));
+	spin_unlock(&enic->devcmd_lock);
+	return err;
+}
+
+static int enic_provinfo_add_tlv_str(struct vic_provinfo *vp, u16 type,
+	char *str)
+{
+	return str ? vic_provinfo_add_tlv(vp, type, strlen(str) + 1, str) : 0;
+}
+
+static int enic_set_vf_port_profile(struct net_device *netdev, int vf,
+	u8 *port_profile, u8 *mac, u8 *host_uuid, u8 *client_uuid,
+	u8 *client_name)
+{
+	struct enic *enic = netdev_priv(netdev);
+	struct vic_provinfo *vp;
+	u8 oui[3] = VIC_PROVINFO_CISCO_OUI;
+	int err;
+
+	if (!enic_is_dynamic(enic))
+		return -EOPNOTSUPP;
+
+	if (strlen(port_profile) == 0)
+		return enic_vnic_dev_deinit(enic);
+
+	vp = vic_provinfo_alloc(GFP_KERNEL, oui, VIC_PROVINFO_LINUX_TYPE);
+	if (!vp)
+		return -ENOMEM;
+
+	enic_provinfo_add_tlv_str(vp, VIC_LINUX_PROV_TLV_PORT_PROFILE_NAME_STR,
+		port_profile);
+	vic_provinfo_add_tlv(vp, VIC_LINUX_PROV_TLV_CLIENT_MAC_ADDR,
+		ETH_ALEN, mac);
+	enic_provinfo_add_tlv_str(vp, VIC_LINUX_PROV_TLV_HOST_UUID_STR,
+		host_uuid);
+	enic_provinfo_add_tlv_str(vp, VIC_LINUX_PROV_TLV_CLIENT_UUID_STR,
+		client_uuid);
+	enic_provinfo_add_tlv_str(vp, VIC_LINUX_PROV_TLV_CLIENT_NAME_STR,
+		client_name);
+
+	err = enic_vnic_dev_deinit(enic);
+	if (err)
+		goto err_out;
+
+	err = enic_dev_init_prov(enic, vp);
+
+err_out:
+	vic_provinfo_free(vp);
+
+	enic_set_multicast_list(netdev);
+
+	return err;
+}
+
 static void enic_free_rq_buf(struct vnic_rq *rq, struct vnic_rq_buf *buf)
 {
 	struct enic *enic = vnic_dev_priv(rq->vdev);
@@ -1440,10 +1529,12 @@ static int enic_open(struct net_device *netdev)
 	for (i = 0; i < enic->rq_count; i++)
 		vnic_rq_enable(&enic->rq[i]);
 
-	spin_lock(&enic->devcmd_lock);
-	enic_add_station_addr(enic);
-	spin_unlock(&enic->devcmd_lock);
-	enic_set_multicast_list(netdev);
+	if (!enic_is_dynamic(enic)) {
+		spin_lock(&enic->devcmd_lock);
+		enic_add_station_addr(enic);
+		spin_unlock(&enic->devcmd_lock);
+		enic_set_multicast_list(netdev);
+	}
 
 	netif_wake_queue(netdev);
 	napi_enable(&enic->napi);
@@ -1775,20 +1866,21 @@ static void enic_clear_intr_mode(struct enic *enic)
 }
 
 static const struct net_device_ops enic_netdev_ops = {
-	.ndo_open		= enic_open,
-	.ndo_stop		= enic_stop,
-	.ndo_start_xmit		= enic_hard_start_xmit,
-	.ndo_get_stats		= enic_get_stats,
-	.ndo_validate_addr	= eth_validate_addr,
-	.ndo_set_mac_address 	= eth_mac_addr,
-	.ndo_set_multicast_list	= enic_set_multicast_list,
-	.ndo_change_mtu		= enic_change_mtu,
-	.ndo_vlan_rx_register	= enic_vlan_rx_register,
-	.ndo_vlan_rx_add_vid	= enic_vlan_rx_add_vid,
-	.ndo_vlan_rx_kill_vid	= enic_vlan_rx_kill_vid,
-	.ndo_tx_timeout		= enic_tx_timeout,
+	.ndo_open			= enic_open,
+	.ndo_stop			= enic_stop,
+	.ndo_start_xmit			= enic_hard_start_xmit,
+	.ndo_get_stats			= enic_get_stats,
+	.ndo_validate_addr		= eth_validate_addr,
+	.ndo_set_multicast_list		= enic_set_multicast_list,
+	.ndo_set_mac_address		= enic_set_mac_address,
+	.ndo_change_mtu			= enic_change_mtu,
+	.ndo_vlan_rx_register		= enic_vlan_rx_register,
+	.ndo_vlan_rx_add_vid		= enic_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid		= enic_vlan_rx_kill_vid,
+	.ndo_tx_timeout			= enic_tx_timeout,
+	.ndo_set_vf_port_profile	= enic_set_vf_port_profile,
 #ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= enic_poll_controller,
+	.ndo_poll_controller		= enic_poll_controller,
 #endif
 };
 
@@ -2010,11 +2102,13 @@ static int __devinit enic_probe(struct pci_dev *pdev,
 
 	netif_carrier_off(netdev);
 
-	err = vnic_dev_init(enic->vdev, 0);
-	if (err) {
-		printk(KERN_ERR PFX
-			"vNIC dev init failed, aborting.\n");
-		goto err_out_dev_close;
+	if (!enic_is_dynamic(enic)) {
+		err = vnic_dev_init(enic->vdev, 0);
+		if (err) {
+			printk(KERN_ERR PFX
+				"vNIC dev init failed, aborting.\n");
+			goto err_out_dev_close;
+		}
 	}
 
 	err = enic_dev_init(enic);
diff --git a/drivers/net/enic/vnic_dev.c b/drivers/net/enic/vnic_dev.c
index d43a9d4..e351b0f 100644
--- a/drivers/net/enic/vnic_dev.c
+++ b/drivers/net/enic/vnic_dev.c
@@ -682,6 +682,56 @@ int vnic_dev_init(struct vnic_dev *vdev, int arg)
 	return r;
 }
 
+int vnic_dev_init_done(struct vnic_dev *vdev, int *done, int *err)
+{
+	u64 a0 = 0, a1 = 0;
+	int wait = 1000;
+	int ret;
+
+	*done = 0;
+
+	ret = vnic_dev_cmd(vdev, CMD_INIT_STATUS, &a0, &a1, wait);
+	if (ret)
+		return ret;
+
+	*done = (a0 == 0);
+
+	*err = (a0 == 0) ? a1 : 0;
+
+	return 0;
+}
+
+int vnic_dev_init_prov(struct vnic_dev *vdev, u8 *buf, u32 len)
+{
+	u64 a0, a1 = len;
+	int wait = 1000;
+	u64 prov_pa;
+	void *prov_buf;
+	int ret;
+
+	prov_buf = pci_alloc_consistent(vdev->pdev, len, &prov_pa);
+	if (!prov_buf)
+		return -ENOMEM;
+
+	memcpy(prov_buf, buf, len);
+
+	a0 = prov_pa;
+
+	ret = vnic_dev_cmd(vdev, CMD_INIT_PROV_INFO, &a0, &a1, wait);
+
+	pci_free_consistent(vdev->pdev, len, prov_buf, prov_pa);
+
+	return ret;
+}
+
+int vnic_dev_deinit(struct vnic_dev *vdev)
+{
+	u64 a0 = 0, a1 = 0;
+	int wait = 1000;
+
+	return vnic_dev_cmd(vdev, CMD_DEINIT, &a0, &a1, wait);
+}
+
 int vnic_dev_link_status(struct vnic_dev *vdev)
 {
 	if (vdev->linkstatus)
diff --git a/drivers/net/enic/vnic_dev.h b/drivers/net/enic/vnic_dev.h
index f5be640..27f5a5a 100644
--- a/drivers/net/enic/vnic_dev.h
+++ b/drivers/net/enic/vnic_dev.h
@@ -124,6 +124,9 @@ int vnic_dev_disable(struct vnic_dev *vdev);
 int vnic_dev_open(struct vnic_dev *vdev, int arg);
 int vnic_dev_open_done(struct vnic_dev *vdev, int *done);
 int vnic_dev_init(struct vnic_dev *vdev, int arg);
+int vnic_dev_init_done(struct vnic_dev *vdev, int *done, int *err);
+int vnic_dev_init_prov(struct vnic_dev *vdev, u8 *buf, u32 len);
+int vnic_dev_deinit(struct vnic_dev *vdev);
 int vnic_dev_soft_reset(struct vnic_dev *vdev, int arg);
 int vnic_dev_soft_reset_done(struct vnic_dev *vdev, int *done);
 void vnic_dev_set_intr_mode(struct vnic_dev *vdev,
diff --git a/drivers/net/enic/vnic_vic.c b/drivers/net/enic/vnic_vic.c
new file mode 100644
index 0000000..d769772
--- /dev/null
+++ b/drivers/net/enic/vnic_vic.c
@@ -0,0 +1,73 @@
+/*
+ * Copyright 2010 Cisco Systems, Inc.  All rights reserved.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+
+#include "vnic_vic.h"
+
+struct vic_provinfo *vic_provinfo_alloc(gfp_t flags, u8 *oui, u8 type)
+{
+	struct vic_provinfo *vp = kzalloc(VIC_PROVINFO_MAX_DATA, flags);
+
+	if (!vp || !oui)
+		return NULL;
+
+	memcpy(vp->oui, oui, sizeof(vp->oui));
+	vp->type = type;
+	vp->length = htonl(sizeof(vp->num_tlvs));
+
+	return vp;
+}
+
+void vic_provinfo_free(struct vic_provinfo *vp)
+{
+	kfree(vp);
+}
+
+int vic_provinfo_add_tlv(struct vic_provinfo *vp, u16 type, u16 length,
+	void *value)
+{
+	struct vic_provinfo_tlv *tlv;
+
+	if (!vp || !value)
+		return -EINVAL;
+
+	if (ntohl(vp->length) + sizeof(*tlv) + length >
+		VIC_PROVINFO_MAX_TLV_DATA)
+		return -ENOMEM;
+
+	tlv = (struct vic_provinfo_tlv *)((u8 *)vp->tlv +
+		ntohl(vp->length) - sizeof(vp->num_tlvs));
+
+	tlv->type = htons(type);
+	tlv->length = htons(length);
+	memcpy(tlv->value, value, length);
+
+	vp->num_tlvs = htonl(ntohl(vp->num_tlvs) + 1);
+	vp->length = htonl(ntohl(vp->length) + sizeof(*tlv) + length);
+
+	return 0;
+}
+
+size_t vic_provinfo_size(struct vic_provinfo *vp)
+{
+	return vp ?  ntohl(vp->length) + sizeof(*vp) - sizeof(vp->num_tlvs) : 0;
+}
diff --git a/drivers/net/enic/vnic_vic.h b/drivers/net/enic/vnic_vic.h
new file mode 100644
index 0000000..085c2a2
--- /dev/null
+++ b/drivers/net/enic/vnic_vic.h
@@ -0,0 +1,59 @@
+/*
+ * Copyright 2010 Cisco Systems, Inc.  All rights reserved.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#ifndef _VNIC_VIC_H_
+#define _VNIC_VIC_H_
+
+/* Note: All integer fields in NETWORK byte order */
+
+/* Note: String field lengths include null char */
+
+#define VIC_PROVINFO_CISCO_OUI		{ 0x00, 0x00, 0x0c }
+#define VIC_PROVINFO_LINUX_TYPE		0x2
+
+enum vic_linux_prov_tlv_type {
+	VIC_LINUX_PROV_TLV_PORT_PROFILE_NAME_STR = 0,
+	VIC_LINUX_PROV_TLV_CLIENT_MAC_ADDR = 1,			/* u8[6] */
+	VIC_LINUX_PROV_TLV_CLIENT_NAME_STR = 2,
+	VIC_LINUX_PROV_TLV_HOST_UUID_STR = 8,
+	VIC_LINUX_PROV_TLV_CLIENT_UUID_STR = 9,
+};
+
+struct vic_provinfo {
+	u8 oui[3];		/* OUI of data provider */
+	u8 type;		/* provider-specific type */
+	u32 length;		/* length of data below */
+	u32 num_tlvs;		/* number of tlvs */
+	struct vic_provinfo_tlv {
+		u16 type;
+		u16 length;
+		u8 value[0];
+	} tlv[0];
+} __attribute__ ((packed));
+
+#define VIC_PROVINFO_MAX_DATA		1385
+#define VIC_PROVINFO_MAX_TLV_DATA (VIC_PROVINFO_MAX_DATA - \
+	sizeof(struct vic_provinfo))
+
+struct vic_provinfo *vic_provinfo_alloc(gfp_t flags, u8 *oui, u8 type);
+void vic_provinfo_free(struct vic_provinfo *vp);
+int vic_provinfo_add_tlv(struct vic_provinfo *vp, u16 type, u16 length,
+	void *value);
+size_t vic_provinfo_size(struct vic_provinfo *vp);
+
+#endif	/* _VNIC_VIC_H_ */

Re: [net-next-2.6 PATCH 2/2] add enic ndo_vf_set_port_profile op support for dynamic vnics

[Chris Wright]

* Scott Feldman (scofeldm@cisco.com) wrote:
> -#define DRV_VERSION		"1.3.1.1"
> +#define DRV_VERSION		"1.3.1.1-iov"

not a version bump?

> @@ -810,14 +819,24 @@ static void enic_reset_mcaddrs(struct enic *enic)
>  
>  static int enic_set_mac_addr(struct net_device *netdev, char *addr)
>  {
> -	if (!is_valid_ether_addr(addr))
> -		return -EADDRNOTAVAIL;
> +	struct enic *enic = netdev_priv(netdev);
>  
> -	memcpy(netdev->dev_addr, addr, netdev->addr_len);
> +	if (enic_is_dynamic(enic)) {
> +		random_ether_addr(netdev->dev_addr);

Would it make more sense to just ignore this?  Then the default (not
port profile configured yet) mac is still a useful identifier.

> +	} else {
> +		if (!is_valid_ether_addr(addr))
> +			return -EADDRNOTAVAIL;
> +		memcpy(netdev->dev_addr, addr, netdev->addr_len);
> +	}
>  
>  	return 0;
>  }
>  
> +static int enic_set_mac_address(struct net_device *netdev, void *p)
> +{
> +	return -EOPNOTSUPP;
> +}
> +

Ever?  Even on non-dynamic enic?  Oh, I see, this was just a lie before ;-)

> +static int enic_set_vf_port_profile(struct net_device *netdev, int vf,
> +	u8 *port_profile, u8 *mac, u8 *host_uuid, u8 *client_uuid,
> +	u8 *client_name)
> +{
> +	struct enic *enic = netdev_priv(netdev);
> +	struct vic_provinfo *vp;
> +	u8 oui[3] = VIC_PROVINFO_CISCO_OUI;
> +	int err;
> +
> +	if (!enic_is_dynamic(enic))
> +		return -EOPNOTSUPP;

Do you want to validate vf (like require it to be 0) or something?
How should userspace know to talk directly to the VF (dynamic enic) in
this device, but a PF + VF_index for another device?

> +	if (strlen(port_profile) == 0)
> +		return enic_vnic_dev_deinit(enic);
> +
> +	vp = vic_provinfo_alloc(GFP_KERNEL, oui, VIC_PROVINFO_LINUX_TYPE);
> +	if (!vp)
> +		return -ENOMEM;
> +
> +	enic_provinfo_add_tlv_str(vp, VIC_LINUX_PROV_TLV_PORT_PROFILE_NAME_STR,
> +		port_profile);
> +	vic_provinfo_add_tlv(vp, VIC_LINUX_PROV_TLV_CLIENT_MAC_ADDR,
> +		ETH_ALEN, mac);
> +	enic_provinfo_add_tlv_str(vp, VIC_LINUX_PROV_TLV_HOST_UUID_STR,
> +		host_uuid);
> +	enic_provinfo_add_tlv_str(vp, VIC_LINUX_PROV_TLV_CLIENT_UUID_STR,
> +		client_uuid);
> +	enic_provinfo_add_tlv_str(vp, VIC_LINUX_PROV_TLV_CLIENT_NAME_STR,
> +		client_name);
> +
> +	err = enic_vnic_dev_deinit(enic);
> +	if (err)
> +		goto err_out;
> +
> +	err = enic_dev_init_prov(enic, vp);
> +
> +err_out:
> +	vic_provinfo_free(vp);
> +
> +	enic_set_multicast_list(netdev);

Should that happen in error case (and is the locking correct)?

Re: [net-next-2.6 PATCH 2/2] add enic ndo_vf_set_port_profile op support for dynamic vnics

[Scott Feldman]

On 4/23/10 7:21 PM, "Chris Wright" <chrisw@redhat.com> wrote:

> * Scott Feldman (scofeldm@cisco.com) wrote:
>> -#define DRV_VERSION  "1.3.1.1"
>> +#define DRV_VERSION  "1.3.1.1-iov"
> 
> not a version bump?

Anything ver diff will work to tell this patch from what's upstream already.
 
>> @@ -810,14 +819,24 @@ static void enic_reset_mcaddrs(struct enic *enic)
>>  
>>  static int enic_set_mac_addr(struct net_device *netdev, char *addr)
>>  {
>> - if (!is_valid_ether_addr(addr))
>> -  return -EADDRNOTAVAIL;
>> + struct enic *enic = netdev_priv(netdev);
>>  
>> - memcpy(netdev->dev_addr, addr, netdev->addr_len);
>> + if (enic_is_dynamic(enic)) {
>> +  random_ether_addr(netdev->dev_addr);
> 
> Would it make more sense to just ignore this?  Then the default (not
> port profile configured yet) mac is still a useful identifier.

Dynamic enics start out with all zero mac addr, so this assigns a random one
to the interface when created.  After I sent out this patch, I realized if
mac addr arg wasn't included in port-profile by user (it's optional), then I
should use this random netdev->dev_addr for the port-profile mac addr.  Next
patch.

>> +static int enic_set_mac_address(struct net_device *netdev, void *p)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
> 
> Ever?  Even on non-dynamic enic?  Oh, I see, this was just a lie before ;-)

Static enics get mac addr from mgmt tool; and dynamic enics get mac addr
during port-profile assignment.  In either case, there is no need for the
user to change the interface mac addr, so I'm adding this explicit block.

It's weird, without setting any value to ndo_set_mac_address, you can change
the mac addr on the interface when the interface is DOWN but not when it's
UP.  Not sure why that is.  In any case, adding this ndo_set_mac_address
callback blocks all attempts to change mac addr on interface.
 
>> +static int enic_set_vf_port_profile(struct net_device *netdev, int vf,
>> + u8 *port_profile, u8 *mac, u8 *host_uuid, u8 *client_uuid,
>> + u8 *client_name)
>> +{
>> + struct enic *enic = netdev_priv(netdev);
>> + struct vic_provinfo *vp;
>> + u8 oui[3] = VIC_PROVINFO_CISCO_OUI;
>> + int err;
>> +
>> + if (!enic_is_dynamic(enic))
>> +  return -EOPNOTSUPP;
> 
> Do you want to validate vf (like require it to be 0) or something?

Yes, I should add a check for vf == 0.

> How should userspace know to talk directly to the VF (dynamic enic) in
> this device, but a PF + VF_index for another device?

Can we use IFLA_NUM_VF?  In this enic case since PF==VF (for now), we'll
need to return IFLA_NUM_VF=1.  Let me see what we can do here.

>> + err = enic_vnic_dev_deinit(enic);
>> + if (err)
>> +  goto err_out;
>> +
>> + err = enic_dev_init_prov(enic, vp);
>> +
>> +err_out:
>> + vic_provinfo_free(vp);
>> +
>> + enic_set_multicast_list(netdev);
> 
> Should that happen in error case (and is the locking correct)?

Locking is correct.  I change this to do this only on the non-err patch.

-scott

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile (was iovnl)

[Chris Wright]

* Scott Feldman (scofeldm@cisco.com) wrote:
> From: Scott Feldman <scofeldm@cisco.com>
> 
> (This is take #2 on the iovnl patches posted earlier based on feedback from
> Chris Wright, Arnd Bergmann, and others.  Thanks guys!)
> 
> Add new netdev ops ndo_set_vf_port_profile to allow setting of port-profile
> on VF, along the lines of existing nds_set_vf_* ops.  Extends RTM_SETLINK
> with new sub cmd called IFLA_VF_PORT_PROFILE (added to end on cmd list).  The
> port-profile cmd arguments are (as seen from iproute2 cmdline):
> 
>        ip link set DEVICE [ { up | down } ]
>                           ...
>                           [ vf NUM [ mac LLADDR ]
>                                    [ vlan VLANID [ qos VLAN-QOS ] ]
>                                    [ rate TXRATE ] ] 
>                                    [ port_profile [ PORT-PROFILE
>                                            [ mac LLADDR ]
>                                            [ host_uuid HOST_UUID ]
>                                            [ client_uuid CLIENT_UUID ]
>                                            [ client_name CLIENT_NAME ] ] ] ]
> 
> 
> I took some liberties and s/SR-IOV/IOV in the code comments around the
> ndo_set_vf_* cmds as they can apply to both SR-IOV and non-SR-IOV adapters,
> as long as there is a PF:VF parent:child relationship.

For enic case, which do you expect to use for net_dev and VF index?  Would
this be VF + index== 0 (meaning the degenerate case you described last
time where PF==VF)?

> A port-profile is used to configure/enable the network port backing the VF, not
> to configure the host-facing side of the VF.

How shall we do the lldpad case?

> Signed-off-by: Scott Feldman <scofeldm@cisco.com>
> Signed-off-by: Roopa Prabhu<roprabhu@cisco.com>
> ---
>  include/linux/if_link.h   |   15 +++++++++++++--
>  include/linux/netdevice.h |   11 ++++++++++-
>  net/core/rtnetlink.c      |   20 ++++++++++++++++++++
>  3 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/if_link.h b/include/linux/if_link.h
> index cfd420b..2c5cc65 100644
> --- a/include/linux/if_link.h
> +++ b/include/linux/if_link.h
> @@ -110,12 +110,13 @@ 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_NUM_VF,		/* Number of VFs if device is IOV PF */
>  	IFLA_VF_MAC,		/* Hardware queue specific attributes */
>  	IFLA_VF_VLAN,
>  	IFLA_VF_TX_RATE,	/* TX Bandwidth Allocation */
>  	IFLA_VFINFO,
>  	IFLA_STATS64,
> +	IFLA_VF_PORT_PROFILE,
>  	__IFLA_MAX
>  };
>  
> @@ -234,7 +235,7 @@ enum macvlan_mode {
>  	MACVLAN_MODE_BRIDGE  = 4, /* talk to bridge ports directly */
>  };
>  
> -/* SR-IOV virtual function managment section */
> +/* IOV virtual function managment section */
>  
>  struct ifla_vf_mac {
>  	__u32 vf;
> @@ -259,4 +260,14 @@ struct ifla_vf_info {
>  	__u32 qos;
>  	__u32 tx_rate;
>  };
> +
> +struct ifla_vf_port_profile {
> +	__u32 vf;
> +	__u8 port_profile[64];
> +	__u8 mac[32];
> +	__u8 host_uuid[64]; /* e.g. "CEEFD3B1-9E11-11DE-BDFD-000BAB01C0FB" */
> +	__u8 client_uuid[64];
> +	__u8 client_name[64]; /* e.g. "vm0-eth1" */
> +};
> +
>  #endif /* _LINUX_IF_LINK_H */
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 3c5ed5f..26dd4cb 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -690,10 +690,13 @@ struct netdev_rx_queue {
>   *
>   * void (*ndo_poll_controller)(struct net_device *dev);
>   *
> - *	SR-IOV management functions.
> + *	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_set_vf_port_profile)(struct net_device *dev, int vf,
> + *				  u8 *port_profile, u8 *mac, u8 *host_uuid,
> + *				  u8 *client_uuid, u8 *client_name);
>   * int (*ndo_get_vf_config)(struct net_device *dev,
>   *			    int vf, struct ifla_vf_info *ivf);
>   */
> @@ -741,6 +744,12 @@ struct net_device_ops {
>  						   int queue, u16 vlan, u8 qos);
>  	int			(*ndo_set_vf_tx_rate)(struct net_device *dev,
>  						      int vf, int rate);
> +	int			(*ndo_set_vf_port_profile)(
> +					struct net_device *dev, int vf,
> +					u8 *port_profile, u8 *mac,
> +					u8 *host_uuid,
> +					u8 *client_uuid,
> +					u8 *client_name);
>  	int			(*ndo_get_vf_config)(struct net_device *dev,
>  						     int vf,
>  						     struct ifla_vf_info *ivf);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 78c8598..7268e8e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -824,6 +824,8 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = {
>  				    .len = sizeof(struct ifla_vf_vlan) },
>  	[IFLA_VF_TX_RATE]	= { .type = NLA_BINARY,
>  				    .len = sizeof(struct ifla_vf_tx_rate) },
> +	[IFLA_VF_PORT_PROFILE]	= { .type = NLA_BINARY,
> +				    .len = sizeof(struct ifla_vf_port_profile)},
>  };
>  EXPORT_SYMBOL(ifla_policy);
>  
> @@ -1028,6 +1030,24 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
>  	}
>  	err = 0;
>  
> +	if (tb[IFLA_VF_PORT_PROFILE]) {
> +		struct ifla_vf_port_profile *ivp;
> +		ivp = nla_data(tb[IFLA_VF_PORT_PROFILE]);
> +		err = -EOPNOTSUPP;
> +		if (ops->ndo_set_vf_port_profile)
> +			ivp->port_profile[sizeof(ivp->port_profile)-1] = 0;
> +			ivp->host_uuid[sizeof(ivp->host_uuid)-1] = 0;
> +			ivp->client_uuid[sizeof(ivp->client_uuid)-1] = 0;
> +			ivp->client_name[sizeof(ivp->client_name)-1] = 0;

Seems a little unusual to modify the buffer, add a kernel internal structure
that can be passed to ndo callback (where buffer lens can be knonw)?

> +			err = ops->ndo_set_vf_port_profile(dev, ivp->vf,
> +				ivp->port_profile, ivp->mac, ivp->host_uuid,
> +				ivp->client_uuid, ivp->client_name);
> +		if (err < 0)
> +			goto errout;
> +		modified = 1;
> +	}
> +	err = 0;
> +
>  errout:
>  	if (err < 0 && modified && net_ratelimit())
>  		printk(KERN_WARNING "A link change request failed with "
> 

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile (was iovnl)

[Scott Feldman]

On 4/23/10 7:22 PM, "Chris Wright" <chrisw@redhat.com> wrote:

>> I took some liberties and s/SR-IOV/IOV in the code comments around the
>> ndo_set_vf_* cmds as they can apply to both SR-IOV and non-SR-IOV adapters,
>> as long as there is a PF:VF parent:child relationship.
> 
> For enic case, which do you expect to use for net_dev and VF index?  Would
> this be VF + index== 0 (meaning the degenerate case you described last
> time where PF==VF)?

Yes, for this enic PF==VF, but that's a short term situation.  It's a small
matter of programming (in firmware) to turn enic into the more general case.
But I want to focus on getting port-profile support in first, with the
current enic+firmware.

>> A port-profile is used to configure/enable the network port backing the VF,
>> not
>> to configure the host-facing side of the VF.
> 
> How shall we do the lldpad case?

Same as before with iovnl.  The sender of RTM_SETLINK msg (say libvirt)
needs to send with mcast group RTMGRP_LINK and listener (say lldpad) needs
to listen on that mcast group.  This way, both kernel and user-space get the
msg.

>> + if (tb[IFLA_VF_PORT_PROFILE]) {
>> +  struct ifla_vf_port_profile *ivp;
>> +  ivp = nla_data(tb[IFLA_VF_PORT_PROFILE]);
>> +  err = -EOPNOTSUPP;
>> +  if (ops->ndo_set_vf_port_profile)
>> +   ivp->port_profile[sizeof(ivp->port_profile)-1] = 0;
>> +   ivp->host_uuid[sizeof(ivp->host_uuid)-1] = 0;
>> +   ivp->client_uuid[sizeof(ivp->client_uuid)-1] = 0;
>> +   ivp->client_name[sizeof(ivp->client_name)-1] = 0;
> 
> Seems a little unusual to modify the buffer, add a kernel internal structure
> that can be passed to ndo callback (where buffer lens can be knonw)?

Ok, let me see what can be done here.

-scott

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[David Miller]

From: Scott Feldman <scofeldm@cisco.com>
Date: Fri, 23 Apr 2010 17:35:41 -0700

> +
> +struct ifla_vf_port_profile {
> +	__u32 vf;
> +	__u8 port_profile[64];
> +	__u8 mac[32];
> +	__u8 host_uuid[64]; /* e.g. "CEEFD3B1-9E11-11DE-BDFD-000BAB01C0FB" */
> +	__u8 client_uuid[64];
> +	__u8 client_name[64]; /* e.g. "vm0-eth1" */
> +};
> +

Please define some macros to represent the string sizes.

;
>  	int			(*ndo_set_vf_tx_rate)(struct net_device *dev,
>  						      int vf, int rate);
> +	int			(*ndo_set_vf_port_profile)(
> +					struct net_device *dev, int vf,
> +					u8 *port_profile, u8 *mac,
> +					u8 *host_uuid,
> +					u8 *client_uuid,
> +					u8 *client_name);
>  	int			(*ndo_get_vf_config)(struct net_device *dev,

Just pass the "struct ifla_vf_port_profile *" instead of tons of
arguments.

Also, I think it's reasonable to fetch the current profile in use, so
GETLINK ought to report these things.  To make it generic we can
maintain the settings given to us in software, hung off of the netdev
struct, and simply report that during GETLINK.

In fact, in general, anything that can be "set" should always be
available from a "get".

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Scott Feldman]

On 4/24/10 12:19 AM, "David Miller" <davem@davemloft.net> wrote:

>> int   (*ndo_set_vf_tx_rate)(struct net_device *dev,
>>      int vf, int rate);
>> + int   (*ndo_set_vf_port_profile)(
>> +     struct net_device *dev, int vf,
>> +     u8 *port_profile, u8 *mac,
>> +     u8 *host_uuid,
>> +     u8 *client_uuid,
>> +     u8 *client_name);
>> int   (*ndo_get_vf_config)(struct net_device *dev,
> 
> Just pass the "struct ifla_vf_port_profile *" instead of tons of
> arguments.

Ok
 
> Also, I think it's reasonable to fetch the current profile in use, so
> GETLINK ought to report these things.  To make it generic we can
> maintain the settings given to us in software, hung off of the netdev
> struct, and simply report that during GETLINK.

We'd need an array of struct ifla_vf_port_profile hanging off of netdev, one
element for each VF.  That seems like a lot of mem hanging off of netdev,
and we'd have to define a MAX_VF to size the array.  How about a
ndo_get_vf_port_profile() that the netdev fills in, and the netdev keeps the
data in it's private area?  That's how ndo_get_vf_config() is working.
 
-scott

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Scott Feldman]

On 4/26/10 12:27 PM, "Scott Feldman" <scofeldm@cisco.com> wrote:
> On 4/24/10 12:19 AM, "David Miller" <davem@davemloft.net> wrote:
>> Also, I think it's reasonable to fetch the current profile in use, so
>> GETLINK ought to report these things.  To make it generic we can
>> maintain the settings given to us in software, hung off of the netdev
>> struct, and simply report that during GETLINK.
> 
> We'd need an array of struct ifla_vf_port_profile hanging off of netdev, one
> element for each VF.  That seems like a lot of mem hanging off of netdev,
> and we'd have to define a MAX_VF to size the array.  How about a
> ndo_get_vf_port_profile() that the netdev fills in, and the netdev keeps the
> data in it's private area?  That's how ndo_get_vf_config() is working.

Hmmm....even that isn't so nice because the port-profile info for all VFs is
going to get stuffed into the RTM_GETLINK skb, and I assume there are limits
on the skb return size.

Here's a proposal:

Currently we have RTM_GETLINK for

    ip link show [ DEVICE ]

This dumps everything for the DEVICE including info for each VF.  Let's
modify RTM_GETLINK to look like this:

    ip link show [ DEVICE [ vf NUM] ]

If you don't give the optional vf NUM you get base dump on DEVICE.  If you
give vf NUM, you get the VF-specific dump.  This scales much better with the
number of VFs.  

(Number of VFs can easily be > 128 in some designs).

Comments?

-scott

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[David Miller]

From: Scott Feldman <scofeldm@cisco.com>
Date: Mon, 26 Apr 2010 12:57:06 -0700

> Hmmm....even that isn't so nice because the port-profile info for all VFs is
> going to get stuffed into the RTM_GETLINK skb, and I assume there are limits
> on the skb return size.

It's probably better to use .dump() for this, which allows to return
the result in multiple SKBs.

RE: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Rose, Gregory V]

>-----Original Message-----
>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
>On Behalf Of Scott Feldman
>Sent: Monday, April 26, 2010 12:57 PM
>To: Scott Feldman; David Miller
>Cc: netdev@vger.kernel.org; chrisw@redhat.com; arnd@arndb.de
>Subject: Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile
>
>On 4/26/10 12:27 PM, "Scott Feldman" <scofeldm@cisco.com> wrote:
>> We'd need an array of struct ifla_vf_port_profile hanging off of
>netdev, one
>> element for each VF.  That seems like a lot of mem hanging off of
>netdev,
>> and we'd have to define a MAX_VF to size the array.  How about a
>> ndo_get_vf_port_profile() that the netdev fills in, and the netdev
>keeps the
>> data in it's private area?  That's how ndo_get_vf_config() is working.
>
>Hmmm....even that isn't so nice because the port-profile info for all
>VFs is
>going to get stuffed into the RTM_GETLINK skb, and I assume there are
>limits
>on the skb return size.
>
>Here's a proposal:
>
>Currently we have RTM_GETLINK for
>
>    ip link show [ DEVICE ]
>
>This dumps everything for the DEVICE including info for each VF.  Let's
>modify RTM_GETLINK to look like this:
>
>    ip link show [ DEVICE [ vf NUM] ]
>
>If you don't give the optional vf NUM you get base dump on DEVICE.  If
>you
>give vf NUM, you get the VF-specific dump.  This scales much better with
>the
>number of VFs.
>
>(Number of VFs can easily be > 128 in some designs).
>
>Comments?

It seems to me that this:

ip link show [ DEVICE ]

should at least return the number of VFs so
that you can make sure the subsequent usage of this:

ip link show [ DEVICE [ vf NUM ] ]

is still in range with the [ vf NUM ] parameter.  Otherwise you wouldn't know
what the range of NUM is.

Other than that I can see why you would want to limit the size of the dump
when using 'ip link show [ DEVICE ]'.

- Greg

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Scott Feldman]

On 4/26/10 3:38 PM, "Rose, Gregory V" <gregory.v.rose@intel.com> wrote:

>> On 4/26/10 12:27 PM, "Scott Feldman" <scofeldm@cisco.com> wrote:
>> Here's a proposal:
>> 
>> Currently we have RTM_GETLINK for
>> 
>>    ip link show [ DEVICE ]
>> 
>> This dumps everything for the DEVICE including info for each VF.  Let's
>> modify RTM_GETLINK to look like this:
>> 
>>    ip link show [ DEVICE [ vf NUM] ]
>> 
>> If you don't give the optional vf NUM you get base dump on DEVICE.  If
>> you
>> give vf NUM, you get the VF-specific dump.  This scales much better with
>> the
>> number of VFs.
>> 
>> (Number of VFs can easily be > 128 in some designs).
>> 
>> Comments?
> 
> It seems to me that this:
> 
> ip link show [ DEVICE ]
> 
> should at least return the number of VFs so
> that you can make sure the subsequent usage of this:

Yes, I believe that's there today:

    NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));

The number of VFs is returned in RTM_GETLINK.  But, it's only returned if:

    if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent)

For my proposal, I'll need to return IFLA_NUM_VF unconditionally so callers
can get num VFs.
 
> ip link show [ DEVICE [ vf NUM ] ]
> 
> is still in range with the [ vf NUM ] parameter.  Otherwise you wouldn't know
> what the range of NUM is.
> 
> Other than that I can see why you would want to limit the size of the dump
> when using 'ip link show [ DEVICE ]'.
> 
> - Greg
> 

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Scott Feldman]

On 4/26/10 4:21 PM, "Scott Feldman" <scofeldm@cisco.com> wrote:

>> It seems to me that this:
>> 
>> ip link show [ DEVICE ]
>> 
>> should at least return the number of VFs so
>> that you can make sure the subsequent usage of this:
> 
> Yes, I believe that's there today:
> 
>     NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));
> 
> The number of VFs is returned in RTM_GETLINK.  But, it's only returned if:
> 
>     if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent)
> 
> For my proposal, I'll need to return IFLA_NUM_VF unconditionally so callers
> can get num VFs.

Hmmm...seems IFLA_NUM_VF assumes a PCI device supporting SR-IOV when it uses
dev_num_vf().  I think a better option would have been to query the device
for the number of VFs, without assuming SR-IOV or even PCI.

I see a ndo_get_num_vf() coming...

-scott

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Chris Wright]

* Scott Feldman (scofeldm@cisco.com) wrote:
> Hmmm...seems IFLA_NUM_VF assumes a PCI device supporting SR-IOV when it uses
> dev_num_vf().  I think a better option would have been to query the device
> for the number of VFs, without assuming SR-IOV or even PCI.
> 
> I see a ndo_get_num_vf() coming...

That was discussed when the patch originally merged.  The proposal to make
get/set symmetric.  Right now it's an unusual mix of sets + one
collective get.

thanks,
-chris

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Arnd Bergmann]

On Tuesday 27 April 2010, Scott Feldman wrote:
> > Yes, I believe that's there today:
> > 
> >     NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));
> > 
> > The number of VFs is returned in RTM_GETLINK.  But, it's only returned if:
> > 
> >     if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent)
> > 
> > For my proposal, I'll need to return IFLA_NUM_VF unconditionally so callers
> > can get num VFs.
> 
> Hmmm...seems IFLA_NUM_VF assumes a PCI device supporting SR-IOV when it uses
> dev_num_vf().  I think a better option would have been to query the device
> for the number of VFs, without assuming SR-IOV or even PCI.
> 
> I see a ndo_get_num_vf() coming...

Shouldn't the number of registered port profiles be totally independent of
the number of virtual functions?

Any of the VFs could multiplex multiple guests using macvlan, which means you
need to register each guest separately, not each VF.

Anything that ties port profiles to VFs seems fundamentally flawed AFAICT,
at least when we want to extend this to adapters that don't do it in firmware.

	Arnd

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Anirban Chakraborty]

On Apr 27, 2010, at 5:35 AM, Arnd Bergmann wrote:

> On Tuesday 27 April 2010, Scott Feldman wrote:
>>> Yes, I believe that's there today:
>>> 
>>>    NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));
>>> 
>>> The number of VFs is returned in RTM_GETLINK.  But, it's only returned if:
>>> 
>>>    if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent)
>>> 
>>> For my proposal, I'll need to return IFLA_NUM_VF unconditionally so callers
>>> can get num VFs.
>> 
>> Hmmm...seems IFLA_NUM_VF assumes a PCI device supporting SR-IOV when it uses
>> dev_num_vf().  I think a better option would have been to query the device
>> for the number of VFs, without assuming SR-IOV or even PCI.
>> 
>> I see a ndo_get_num_vf() coming...
> 
> Shouldn't the number of registered port profiles be totally independent of
> the number of virtual functions?
> 
> Any of the VFs could multiplex multiple guests using macvlan, which means you
> need to register each guest separately, not each VF.
> 
> Anything that ties port profiles to VFs seems fundamentally flawed AFAICT,
> at least when we want to extend this to adapters that don't do it in firmware.

Correct me if I am wrong. Shouldn't the port profile be tied to the physical NICs which are essentially
PCI functions (be it PF or VF)? I'd think that a port profile would have configuration settings for all the
physical NICs (PF/VF) of a specific physical port of the adapter. I liked the idea of querying the device
for number of VFs as it will cover both SR-IOV and non SR-IOV PCI functions.

thanks,
-Anirban

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Arnd Bergmann]

On Tuesday 27 April 2010 19:33:04 Anirban Chakraborty wrote:
> On Apr 27, 2010, at 5:35 AM, Arnd Bergmann wrote:
> > Anything that ties port profiles to VFs seems fundamentally flawed AFAICT,
> > at least when we want to extend this to adapters that don't do it in firmware.
> 
> Correct me if I am wrong. Shouldn't the port profile be tied to the physical NICs which are essentially
> PCI functions (be it PF or VF)? I'd think that a port profile would have configuration settings for all the
> physical NICs (PF/VF) of a specific physical port of the adapter. I liked the idea of querying the device
> for number of VFs as it will cover both SR-IOV and non SR-IOV PCI functions.

Yes, the port profile association is tied to whoever owns the link to the switch.
That can be a regular NIC, an SR-IOV PF, an ethernet bonding device or an S-component
implementing provider S-VLANs on top of any of these.

Usually it will be the same as a physical link, but in case of bonding it is two
physical links while in case of S-VLAN, you have multiple instances that each
have their own set of port profile association. If S-VLAN is implemented by
the NIC, that may be a VF.

Querying a PF for the number of VFs attached to it is a useful thing, but this
is independent of port profiles. Consider this (artificially complex) setup:

- eth0 is the PF of an SR-IOV NIC
- eth1 is a regular single-channel NIC
- vf0 is a VF of eth0, used by a guest using PCI passthrough mode on S-VLAN 2
- vf1 is a VF of eth0 owned by the host on S-VLAN 3
- vf1.23 is a VLAN port for VLAN 23 in S-VLAN 3
- br0 is a bridge connected to vf1
- br23 is a bridge VLAN device for br0
- vf2 is a VF of eth0 owned by the host on S-VLAN 4
- eth1.5 is a software vlan device for S-VLAN 4
- bond0 combines eth1.5 and vf2
- bond0.24 is a VLAN port for VLAN24 on bond0
- tap0 is a guest connected to br0 in trunk mode
- tap1 is a guest connected to br23 in access mode
- macvtap0 is a VEPA mode guest on bond0
- macvtap1 is a private mode guest on bond0.24

This means you have a total of five guests running, on vf0, tap0, tap1,
macvtap0 and macvtap1. Querying the number of VFs on eth0 will return '2',
for vf0 and vf1. What you are interested in however is which guests are
associated. Querying every single interface in the system will tell you

eth0: one guest (vf0)
vf1: two guests (tap0 and tap1)
bond0: two guests (macvtap0 and macvtap1)

	Arnd

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[Scott Feldman]

On 4/27/10 5:35 AM, "Arnd Bergmann" <arnd@arndb.de> wrote:

> On Tuesday 27 April 2010, Scott Feldman wrote:
>>> Yes, I believe that's there today:
>>> 
>>>     NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));
>>> 
>>> The number of VFs is returned in RTM_GETLINK.  But, it's only returned if:
>>> 
>>>     if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent)
>>> 
>>> For my proposal, I'll need to return IFLA_NUM_VF unconditionally so callers
>>> can get num VFs.
>> 
>> Hmmm...seems IFLA_NUM_VF assumes a PCI device supporting SR-IOV when it uses
>> dev_num_vf().  I think a better option would have been to query the device
>> for the number of VFs, without assuming SR-IOV or even PCI.
>> 
>> I see a ndo_get_num_vf() coming...
> 
> Shouldn't the number of registered port profiles be totally independent of
> the number of virtual functions?
> 
> Any of the VFs could multiplex multiple guests using macvlan, which means you
> need to register each guest separately, not each VF.
> 
> Anything that ties port profiles to VFs seems fundamentally flawed AFAICT,
> at least when we want to extend this to adapters that don't do it in firmware.

Ya, I tend I agree.  Let's just make port-profile a setting of any netdev,
an eth, macvtap, eth.x, bond, etc.  That's probably what I should have done
in the first place.  Something like:

       ip link set DEVICE [ { up | down } ]
                          [ arp { on | off } ]
                            <...clip...>
                          [ alias NAME ]
                          [ vf NUM [ mac LLADDR ]
                                   [ vlan VLANID [ qos VLAN-QOS ] ]
                                   [ rate TXRATE ] ]
                          [ port_profile [ PORT-PROFILE
                                   [ mac LLADDR ]
                                   [ host_uuid HOST_UUID ]
                                   [ client_uuid CLIENT_UUID ]
                                   [ client_name CLIENT_NAME ] ] ] ]
       ip link show [ DEVICE ]

I think I was trying to be too accommodating for models with VFs, but it
doesn't matter like you point out.

This way, I can get the RTM_GETLINK to return the port-profile in use.

New patches coming soon...

-scott

Re: [net-next-2.6 PATCH 1/2] Add ndo_set_vf_port_profile

[David Miller]

From: Scott Feldman <scofeldm@cisco.com>
Date: Mon, 26 Apr 2010 12:27:51 -0700

> We'd need an array of struct ifla_vf_port_profile hanging off of netdev, one
> element for each VF.  That seems like a lot of mem hanging off of netdev,
> and we'd have to define a MAX_VF to size the array.  How about a
> ndo_get_vf_port_profile() that the netdev fills in, and the netdev keeps the
> data in it's private area?  That's how ndo_get_vf_config() is working.

Sure if the device can do it, but for situations where it can't we can
use a linked list and dynamic memory allocation.