[PATCH net-next 0/2] cxgb4: Adds support for VF mgmt ndo's

[PATCH net-next 0/2] cxgb4: Adds support for VF mgmt ndo's

[Hariprasad Shenai]

Hi,

This patch series adds support for ndo_set_vf_vlan and ndo_get_vf_config
for cxgb4 driver.

This patch series has been created against net-next tree.

We have included all the maintainers of respective drivers. Kindly review
the change and let us know in case of any review comments.

Thanks

Hariprasad Shenai (2):
  cxgb4/cxgb4vf: Add support for ndo_set_vf_vlan
  cxgb4: Add support for ndo_get_vf_config

 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h         |   11 +++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    |   96 +++++++++++++++++++-
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c         |   27 ++++++-
 drivers/net/ethernet/chelsio/cxgb4vf/adapter.h     |    1 +
 .../net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c    |    2 +
 drivers/net/ethernet/chelsio/cxgb4vf/sge.c         |   15 +++-
 drivers/net/ethernet/chelsio/cxgb4vf/t4vf_common.h |    1 +
 drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c     |   28 ++++++
 8 files changed, 176 insertions(+), 5 deletions(-)

-- 
1.7.3

[PATCH net-next 1/2] cxgb4/cxgb4vf: Add support for ndo_set_vf_vlan

[Hariprasad Shenai]

Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h         |    1 +
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    |   21 +++++++++++++++
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c         |   25 +++++++++++++++++
 drivers/net/ethernet/chelsio/cxgb4vf/adapter.h     |    1 +
 .../net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c    |    2 +
 drivers/net/ethernet/chelsio/cxgb4vf/sge.c         |   15 ++++++++--
 drivers/net/ethernet/chelsio/cxgb4vf/t4vf_common.h |    1 +
 drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c     |   28 ++++++++++++++++++++
 8 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index 3f7b33a..b20d345 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -1627,6 +1627,7 @@ void t4_idma_monitor(struct adapter *adapter,
 		     int hz, int ticks);
 int t4_set_vf_mac_acl(struct adapter *adapter, unsigned int vf,
 		      unsigned int naddr, u8 *addr);
+int t4_set_vf_vlan_acl(struct adapter *adapter, unsigned int vf, u16 vlan);
 void uld_mem_free(struct adapter *adap);
 int uld_mem_alloc(struct adapter *adap);
 void free_rspq_fl(struct adapter *adap, struct sge_rspq *rq, struct sge_fl *fl);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 44019bd..330e18a 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -3111,6 +3111,26 @@ static int cxgb_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
 		 "Setting MAC %pM on VF %d\n", mac, vf);
 	return t4_set_vf_mac_acl(adap, vf + 1, 1, mac);
 }
+
+static int cxgb_set_vf_vlan(struct net_device *dev, int vf, u16 vlan, u8 qos)
+{
+	struct port_info *pi = netdev_priv(dev);
+	struct adapter *adap = pi->adapter;
+
+	if (vlan > 4095 || qos > 7) {
+		dev_err(pi->adapter->pdev_dev,
+			"Illegal vlan value %u qos %u\n", vlan, qos);
+		return -EINVAL;
+	}
+
+	dev_info(pi->adapter->pdev_dev,
+		 "Setting Vlan %u to VF [%d]\n", vlan, vf);
+	dev_info(pi->adapter->pdev_dev,
+		 "The VF [%d] interface needs to brought down and up, "
+		 "if VF is already up and running, for VST to work\n", vf);
+	vlan |= qos << VLAN_PRIO_SHIFT;
+	return t4_set_vf_vlan_acl(adap, vf + 1, vlan);
+}
 #endif
 
 static int cxgb_set_mac_addr(struct net_device *dev, void *p)
@@ -3259,6 +3279,7 @@ static const struct net_device_ops cxgb4_netdev_ops = {
 static const struct net_device_ops cxgb4_mgmt_netdev_ops = {
 	.ndo_open             = dummy_open,
 	.ndo_set_vf_mac       = cxgb_set_vf_mac,
+	.ndo_set_vf_vlan      = cxgb_set_vf_vlan,
 };
 #endif
 
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index de451ee..210979c 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -8334,3 +8334,28 @@ int t4_sched_params(struct adapter *adapter, int type, int level, int mode,
 	return t4_wr_mbox_meat(adapter, adapter->mbox, &cmd, sizeof(cmd),
 			       NULL, 1);
 }
+
+/**
+ *	t4_set_vf_vlan_acl - Set MAC address for the specified VF
+ *	@adapter: The adapter
+ *	@vf: one of the VFs instantiated by the specified PF
+ *	@vlan: the vlan id
+ *	@qos: QOS
+ */
+int t4_set_vf_vlan_acl(struct adapter *adapter, unsigned int vf, u16 vlan)
+{
+	struct fw_acl_vlan_cmd cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+
+	cmd.op_to_vfn = htonl(FW_CMD_OP_V(FW_ACL_VLAN_CMD) |
+			      FW_CMD_REQUEST_F |
+			      FW_CMD_WRITE_F |
+			      FW_ACL_MAC_CMD_PFN_V(adapter->pf) |
+			      FW_ACL_MAC_CMD_VFN_V(vf));
+	cmd.en_to_len16 = htonl((unsigned int)FW_LEN16(cmd));
+	cmd.nvlan = 1;
+	cmd.vlanid[0] = vlan;
+
+	return t4_wr_mbox(adapter, adapter->mbox, &cmd, sizeof(cmd), &cmd);
+}
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/adapter.h b/drivers/net/ethernet/chelsio/cxgb4vf/adapter.h
index 109bc63..ed1037e 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/adapter.h
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/adapter.h
@@ -92,6 +92,7 @@ struct sge_rspq;
  */
 struct port_info {
 	struct adapter *adapter;	/* our adapter */
+	u32 vlan_id;			/* vlan id for VST */
 	u16 viid;			/* virtual interface ID */
 	s16 xact_addr_filt;		/* index of our MAC address filter */
 	u16 rss_size;			/* size of VI's RSS table slice */
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
index f2951bf..d6324a6 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
@@ -795,6 +795,8 @@ static int cxgb4vf_open(struct net_device *dev)
 	if (err)
 		goto err_unwind;
 
+	pi->vlan_id = t4vf_get_vf_vlan_acl(adapter);
+
 	netif_tx_start_all_queues(dev);
 	set_bit(pi->port_id, &adapter->open_device_map);
 	return 0;
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/sge.c b/drivers/net/ethernet/chelsio/cxgb4vf/sge.c
index c8fd4f8..9a9963b 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/sge.c
@@ -1202,6 +1202,10 @@ int t4vf_eth_xmit(struct sk_buff *skb, struct net_device *dev)
 	BUG_ON(qidx >= pi->nqsets);
 	txq = &adapter->sge.ethtxq[pi->first_qset + qidx];
 
+	if (pi->vlan_id && !skb_vlan_tag_present(skb))
+		__vlan_hwaccel_put_tag(skb, cpu_to_be16(ETH_P_8021Q),
+				       pi->vlan_id);
+
 	/*
 	 * Take this opportunity to reclaim any TX Descriptors whose DMA
 	 * transfers have completed.
@@ -1570,6 +1574,7 @@ static void do_gro(struct sge_eth_rxq *rxq, const struct pkt_gl *gl,
 {
 	struct adapter *adapter = rxq->rspq.adapter;
 	struct sge *s = &adapter->sge;
+	struct port_info *pi;
 	int ret;
 	struct sk_buff *skb;
 
@@ -1586,8 +1591,9 @@ static void do_gro(struct sge_eth_rxq *rxq, const struct pkt_gl *gl,
 	skb->truesize += skb->data_len;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 	skb_record_rx_queue(skb, rxq->rspq.idx);
+	pi = netdev_priv(skb->dev);
 
-	if (pkt->vlan_ex) {
+	if (pkt->vlan_ex && !pi->vlan_id) {
 		__vlan_hwaccel_put_tag(skb, cpu_to_be16(ETH_P_8021Q),
 					be16_to_cpu(pkt->vlan));
 		rxq->stats.vlan_ex++;
@@ -1620,6 +1626,7 @@ int t4vf_ethrx_handler(struct sge_rspq *rspq, const __be64 *rsp,
 	struct sge_eth_rxq *rxq = container_of(rspq, struct sge_eth_rxq, rspq);
 	struct adapter *adapter = rspq->adapter;
 	struct sge *s = &adapter->sge;
+	struct port_info *pi;
 
 	/*
 	 * If this is a good TCP packet and we have Generic Receive Offload
@@ -1644,6 +1651,7 @@ int t4vf_ethrx_handler(struct sge_rspq *rspq, const __be64 *rsp,
 	__skb_pull(skb, s->pktshift);
 	skb->protocol = eth_type_trans(skb, rspq->netdev);
 	skb_record_rx_queue(skb, rspq->idx);
+	pi = netdev_priv(skb->dev);
 	rxq->stats.pkts++;
 
 	if (csum_ok && !pkt->err_vec &&
@@ -1659,9 +1667,10 @@ int t4vf_ethrx_handler(struct sge_rspq *rspq, const __be64 *rsp,
 	} else
 		skb_checksum_none_assert(skb);
 
-	if (pkt->vlan_ex) {
+	if (pkt->vlan_ex && !pi->vlan_id) {
 		rxq->stats.vlan_ex++;
-		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), be16_to_cpu(pkt->vlan));
+		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
+				       be16_to_cpu(pkt->vlan));
 	}
 
 	netif_receive_skb(skb);
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_common.h b/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_common.h
index 8067424..fa06344 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_common.h
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_common.h
@@ -384,5 +384,6 @@ int t4vf_handle_fw_rpl(struct adapter *, const __be64 *);
 int t4vf_prep_adapter(struct adapter *);
 int t4vf_get_vf_mac_acl(struct adapter *adapter, unsigned int pf,
 			unsigned int *naddr, u8 *addr);
+int t4vf_get_vf_vlan_acl(struct adapter *adapter);
 
 #endif /* __T4VF_COMMON_H__ */
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c b/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
index 879f4c5..b46f6f3 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
@@ -1858,3 +1858,31 @@ int t4vf_get_vf_mac_acl(struct adapter *adapter, unsigned int pf,
 
 	return ret;
 }
+
+/**
+ *	t4vf_get_vf_vlan_acl - Get the VLAN ID to be set to
+ *                             the VI of this VF.
+ *	@adapter: The adapter
+ *
+ *	Find the VLAN ID to be set to the VF's VI. The requested VLAN ID
+ *	is from the host OS via callback in the PF driver.
+ */
+int t4vf_get_vf_vlan_acl(struct adapter *adapter)
+{
+	struct fw_acl_vlan_cmd cmd;
+	int vlan = 0;
+	int ret = 0;
+
+	cmd.op_to_vfn = htonl(FW_CMD_OP_V(FW_ACL_VLAN_CMD) |
+			      FW_CMD_REQUEST_F | FW_CMD_READ_F);
+
+	/* Note: Do not enable the ACL */
+	cmd.en_to_len16 = htonl((unsigned int)FW_LEN16(cmd));
+
+	ret = t4vf_wr_mbox(adapter, &cmd, sizeof(cmd), &cmd);
+
+	if (!ret)
+		vlan = cmd.vlanid[0];
+
+	return vlan;
+}
-- 
1.7.3

[PATCH net-next 2/2] cxgb4: Add support for ndo_get_vf_config

[Hariprasad Shenai]

Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h      |   10 +++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c |   77 ++++++++++++++++++++++-
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c      |    2 +-
 3 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index b20d345..2781658 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -788,6 +788,13 @@ struct uld_msix_info {
 	char desc[IFNAMSIZ + 10];
 };
 
+struct vf_info {
+	unsigned char vf_mac_addr[ETH_ALEN];
+	bool pf_set_mac;
+	u16 pf_vlan; /* When set, guest VLAN config not allowed. */
+	u16 pf_qos;
+};
+
 struct adapter {
 	void __iomem *regs;
 	void __iomem *bar2;
@@ -821,6 +828,9 @@ struct adapter {
 	struct net_device *port[MAX_NPORTS];
 	u8 chan_map[NCHAN];                   /* channel -> port map */
 
+	struct vf_info *vfinfo;
+	u8 num_vfs;
+
 	u32 filter_mode;
 	unsigned int l2t_start;
 	unsigned int l2t_end;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 330e18a..9baab2c 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -3094,10 +3094,43 @@ static int dummy_open(struct net_device *dev)
 	return 0;
 }
 
+static void fill_vf_station_mac_addr(struct adapter *adap)
+{
+	unsigned int i;
+	u8 hw_addr[ETH_ALEN], macaddr[ETH_ALEN];
+	int err;
+	u8 *na;
+	u16 a, b;
+
+	err = t4_get_raw_vpd_params(adap, &adap->params.vpd);
+	if (!err) {
+		na = adap->params.vpd.na;
+		for (i = 0; i < ETH_ALEN; i++)
+			hw_addr[i] = (hex2val(na[2 * i + 0]) * 16 +
+				      hex2val(na[2 * i + 1]));
+		a = (hw_addr[0] << 8) | hw_addr[1];
+		b = (hw_addr[1] << 8) | hw_addr[2];
+		a ^= b;
+		a |= 0x0200;    /* locally assigned Ethernet MAC address */
+		a &= ~0x0100;   /* not a multicast Ethernet MAC address */
+		macaddr[0] = a >> 8;
+		macaddr[1] = a & 0xff;
+
+		for (i = 2; i < 5; i++)
+			macaddr[i] = hw_addr[i + 1];
+
+		for (i = 0; i < adap->num_vfs; i++) {
+			macaddr[5] = adap->pf * 16 + i;
+			ether_addr_copy(adap->vfinfo[i].vf_mac_addr, macaddr);
+		}
+	}
+}
+
 static int cxgb_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
 {
 	struct port_info *pi = netdev_priv(dev);
 	struct adapter *adap = pi->adapter;
+	int ret;
 
 	/* verify MAC addr is valid */
 	if (!is_valid_ether_addr(mac)) {
@@ -3109,14 +3142,22 @@ static int cxgb_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
 
 	dev_info(pi->adapter->pdev_dev,
 		 "Setting MAC %pM on VF %d\n", mac, vf);
-	return t4_set_vf_mac_acl(adap, vf + 1, 1, mac);
+	ret = t4_set_vf_mac_acl(adap, vf + 1, 1, mac);
+	if (!ret)
+		ether_addr_copy(adap->vfinfo[vf].vf_mac_addr, mac);
+	return ret;
 }
 
 static int cxgb_set_vf_vlan(struct net_device *dev, int vf, u16 vlan, u8 qos)
 {
 	struct port_info *pi = netdev_priv(dev);
 	struct adapter *adap = pi->adapter;
+	int ret;
 
+	if (vf >= adap->num_vfs) {
+		dev_err(pi->adapter->pdev_dev, "Invalid VF %d\n", vf);
+		return -EINVAL;
+	}
 	if (vlan > 4095 || qos > 7) {
 		dev_err(pi->adapter->pdev_dev,
 			"Illegal vlan value %u qos %u\n", vlan, qos);
@@ -3129,7 +3170,27 @@ static int cxgb_set_vf_vlan(struct net_device *dev, int vf, u16 vlan, u8 qos)
 		 "The VF [%d] interface needs to brought down and up, "
 		 "if VF is already up and running, for VST to work\n", vf);
 	vlan |= qos << VLAN_PRIO_SHIFT;
-	return t4_set_vf_vlan_acl(adap, vf + 1, vlan);
+	ret = t4_set_vf_vlan_acl(adap, vf + 1, vlan);
+	if (!ret) {
+		adap->vfinfo[vf].pf_vlan = vlan & VLAN_VID_MASK;
+		adap->vfinfo[vf].pf_qos = qos;
+	}
+	return ret;
+}
+
+static int cxgb_get_vf_config(struct net_device *dev,
+			      int vf, struct ifla_vf_info *ivi)
+{
+	struct port_info *pi = netdev_priv(dev);
+	struct adapter *adap = pi->adapter;
+
+	if (vf >= adap->num_vfs)
+		return -EINVAL;
+	ivi->vf = vf;
+	ether_addr_copy(ivi->mac, adap->vfinfo[vf].vf_mac_addr);
+	ivi->vlan = adap->vfinfo[vf].pf_vlan;
+	ivi->qos = adap->vfinfo[vf].pf_qos;
+	return 0;
 }
 #endif
 
@@ -3280,6 +3341,7 @@ static const struct net_device_ops cxgb4_mgmt_netdev_ops = {
 	.ndo_open             = dummy_open,
 	.ndo_set_vf_mac       = cxgb_set_vf_mac,
 	.ndo_set_vf_vlan      = cxgb_set_vf_vlan,
+	.ndo_get_vf_config    = cxgb_get_vf_config,
 };
 #endif
 
@@ -5137,6 +5199,10 @@ static int cxgb4_iov_configure(struct pci_dev *pdev, int num_vfs)
 			unregister_netdev(adap->port[0]);
 			adap->port[0] = NULL;
 		}
+		/* free VF resources */
+		kfree(adap->vfinfo);
+		adap->vfinfo = NULL;
+		adap->num_vfs = 0;
 		return num_vfs;
 	}
 
@@ -5145,10 +5211,16 @@ static int cxgb4_iov_configure(struct pci_dev *pdev, int num_vfs)
 		if (err)
 			return err;
 
+		adap->num_vfs = num_vfs;
 		err = config_mgmt_dev(pdev);
 		if (err)
 			return err;
 	}
+
+	adap->vfinfo = kcalloc(adap->num_vfs,
+			       sizeof(struct vf_info), GFP_KERNEL);
+	if (adap->vfinfo)
+		fill_vf_station_mac_addr(adap);
 	return num_vfs;
 }
 #endif
@@ -5642,6 +5714,7 @@ static void remove_one(struct pci_dev *pdev)
 		if (adapter->port[0])
 			unregister_netdev(adapter->port[0]);
 		iounmap(adapter->regs);
+		kfree(adapter->vfinfo);
 		kfree(adapter);
 		pci_disable_sriov(pdev);
 		pci_release_regions(pdev);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index 210979c..f215aef 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -2729,7 +2729,7 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
 
 out:
 	vfree(vpd);
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 /**
-- 
1.7.3

RE: [PATCH net-next 1/2] cxgb4/cxgb4vf: Add support for ndo_set_vf_vlan

[Yuval Mintz]

> @@ -1202,6 +1202,10 @@ int t4vf_eth_xmit(struct sk_buff *skb, struct
> net_device *dev)
>  	BUG_ON(qidx >= pi->nqsets);
>  	txq = &adapter->sge.ethtxq[pi->first_qset + qidx];
> 
> +	if (pi->vlan_id && !skb_vlan_tag_present(skb))
> +		__vlan_hwaccel_put_tag(skb, cpu_to_be16(ETH_P_8021Q),
> +				       pi->vlan_id);
> +

So it's a purely SW implementation of the feature on the VF side?
Does the HW enforces the configuration in any way on the VF?

Also, looks like an already tagged packet would be processed with
the original vlan-id [instead of the one of PF has provided].
Is that intentional?

RE: [PATCH net-next 2/2] cxgb4: Add support for ndo_get_vf_config

[Yuval Mintz]

> +static void fill_vf_station_mac_addr(struct adapter *adap)
> +{
> +	unsigned int i;
> +	u8 hw_addr[ETH_ALEN], macaddr[ETH_ALEN];
> +	int err;
> +	u8 *na;
> +	u16 a, b;
> +
> +	err = t4_get_raw_vpd_params(adap, &adap->params.vpd);
> +	if (!err) {
> +		na = adap->params.vpd.na;
> +		for (i = 0; i < ETH_ALEN; i++)
> +			hw_addr[i] = (hex2val(na[2 * i + 0]) * 16 +
> +				      hex2val(na[2 * i + 1]));
> +		a = (hw_addr[0] << 8) | hw_addr[1];
> +		b = (hw_addr[1] << 8) | hw_addr[2];
> +		a ^= b;
> +		a |= 0x0200;    /* locally assigned Ethernet MAC address */
> +		a &= ~0x0100;   /* not a multicast Ethernet MAC address */
> +		macaddr[0] = a >> 8;
> +		macaddr[1] = a & 0xff;
> +
> +		for (i = 2; i < 5; i++)
> +			macaddr[i] = hw_addr[i + 1];
> +
> +		for (i = 0; i < adap->num_vfs; i++) {
> +			macaddr[5] = adap->pf * 16 + i;
> +			ether_addr_copy(adap->vfinfo[i].vf_mac_addr,
> macaddr);
> +		}
> +	}
> +}

That's some... magical magic? :-)
But I couldn't see anywhere in the patch where this MAC is propagated
to the VF, only to the new NDO(). Care to explain how does the VF learn it?
	

Re: [PATCH net-next 2/2] cxgb4: Add support for ndo_get_vf_config

[Hariprasad Shenai]

On Wednesday, August 08/24/16, 2016 at 07:15:49 +0000, Yuval Mintz wrote:
> > +static void fill_vf_station_mac_addr(struct adapter *adap)
> > +{
> > +	unsigned int i;
> > +	u8 hw_addr[ETH_ALEN], macaddr[ETH_ALEN];
> > +	int err;
> > +	u8 *na;
> > +	u16 a, b;
> > +
> > +	err = t4_get_raw_vpd_params(adap, &adap->params.vpd);
> > +	if (!err) {
> > +		na = adap->params.vpd.na;
> > +		for (i = 0; i < ETH_ALEN; i++)
> > +			hw_addr[i] = (hex2val(na[2 * i + 0]) * 16 +
> > +				      hex2val(na[2 * i + 1]));
> > +		a = (hw_addr[0] << 8) | hw_addr[1];
> > +		b = (hw_addr[1] << 8) | hw_addr[2];
> > +		a ^= b;
> > +		a |= 0x0200;    /* locally assigned Ethernet MAC address */
> > +		a &= ~0x0100;   /* not a multicast Ethernet MAC address */
> > +		macaddr[0] = a >> 8;
> > +		macaddr[1] = a & 0xff;
> > +
> > +		for (i = 2; i < 5; i++)
> > +			macaddr[i] = hw_addr[i + 1];
> > +
> > +		for (i = 0; i < adap->num_vfs; i++) {
> > +			macaddr[5] = adap->pf * 16 + i;
> > +			ether_addr_copy(adap->vfinfo[i].vf_mac_addr,
> > macaddr);
> > +		}
> > +	}
> > +}
> 
> That's some... magical magic? :-)
> But I couldn't see anywhere in the patch where this MAC is propagated
> to the VF, only to the new NDO(). Care to explain how does the VF learn it?
> 	
The same logic is used by the firmware to generate the MAC address for the VF.
If MAC address isn't provided through IFLA_VF_MAC. This function is only used
to populate the stationary MAC's for the VF, when user hasn't provided one.

-Hari

Re: [PATCH net-next 1/2] cxgb4/cxgb4vf: Add support for ndo_set_vf_vlan

[Hariprasad Shenai]

On Wednesday, August 08/24/16, 2016 at 07:08:14 +0000, Yuval Mintz wrote:
> > @@ -1202,6 +1202,10 @@ int t4vf_eth_xmit(struct sk_buff *skb, struct
> > net_device *dev)
> >  	BUG_ON(qidx >= pi->nqsets);
> >  	txq = &adapter->sge.ethtxq[pi->first_qset + qidx];
> > 
> > +	if (pi->vlan_id && !skb_vlan_tag_present(skb))
> > +		__vlan_hwaccel_put_tag(skb, cpu_to_be16(ETH_P_8021Q),
> > +				       pi->vlan_id);
> > +
> 
> So it's a purely SW implementation of the feature on the VF side?
> Does the HW enforces the configuration in any way on the VF?
Basically the PF driver passes the VLAN ID it got through ndo_set_vf_vlan
to the VF driver. And then the VF driver reads it and requests hardware to
tag it.

> 
> Also, looks like an already tagged packet would be processed with
> the original vlan-id [instead of the one of PF has provided].
> Is that intentional?
No, this isn't intentional. I thought VST and VGT cannot co-exist.
What should be the behavior? 

Thanks,
Hariprasad Shenai
 

RE: [PATCH net-next 1/2] cxgb4/cxgb4vf: Add support for ndo_set_vf_vlan

[Yuval Mintz]

> > > @@ -1202,6 +1202,10 @@ int t4vf_eth_xmit(struct sk_buff *skb, struct
> > > net_device *dev)
> > >  	BUG_ON(qidx >= pi->nqsets);
> > >  	txq = &adapter->sge.ethtxq[pi->first_qset + qidx];
> > >
> > > +	if (pi->vlan_id && !skb_vlan_tag_present(skb))
> > > +		__vlan_hwaccel_put_tag(skb, cpu_to_be16(ETH_P_8021Q),
> > > +				       pi->vlan_id);
> > > +
> >
> > So it's a purely SW implementation of the feature on the VF side?
> > Does the HW enforces the configuration in any way on the VF?
> Basically the PF driver passes the VLAN ID it got through ndo_set_vf_vlan to the
> VF driver. And then the VF driver reads it and requests hardware to tag it.

Problem with SW implementations is mainly that they have no effect over
Malicious VFs
 I.e., if the purpose here is to add the VF to some vlan-tagged subnet
Whereas the user is oblivious to it, a malicious user can easily modify
the driver to ignore this restriction and get access to the entire network.

I think one of the problems with this ndo is that it's poorly documented
and thus open for various interpretations - so it's debatable what's important
and what's not. [If it is properly documented anywhere, please educate me]

> > Also, looks like an already tagged packet would be processed with the
> > original vlan-id [instead of the one of PF has provided].
> > Is that intentional?
> No, this isn't intentional. I thought VST and VGT cannot co-exist.
> What should be the behavior?

Are you preventing VGT configuration once VST is configured?
If not, what to prevent VM user from configuring vlan interfaces
on top of the VF, even if VST is configured?

Re: [PATCH net-next 1/2] cxgb4/cxgb4vf: Add support for ndo_set_vf_vlan

[Hariprasad Shenai]

On Wednesday, August 08/24/16, 2016 at 08:31:58 +0000, Yuval Mintz wrote:
> > > > @@ -1202,6 +1202,10 @@ int t4vf_eth_xmit(struct sk_buff *skb, struct
> > > > net_device *dev)
> > > >  	BUG_ON(qidx >= pi->nqsets);
> > > >  	txq = &adapter->sge.ethtxq[pi->first_qset + qidx];
> > > >
> > > > +	if (pi->vlan_id && !skb_vlan_tag_present(skb))
> > > > +		__vlan_hwaccel_put_tag(skb, cpu_to_be16(ETH_P_8021Q),
> > > > +				       pi->vlan_id);
> > > > +
> > >
> > > So it's a purely SW implementation of the feature on the VF side?
> > > Does the HW enforces the configuration in any way on the VF?
> > Basically the PF driver passes the VLAN ID it got through ndo_set_vf_vlan to the
> > VF driver. And then the VF driver reads it and requests hardware to tag it.
> 
> Problem with SW implementations is mainly that they have no effect over
> Malicious VFs
>  I.e., if the purpose here is to add the VF to some vlan-tagged subnet
> Whereas the user is oblivious to it, a malicious user can easily modify
> the driver to ignore this restriction and get access to the entire network.
> 
> I think one of the problems with this ndo is that it's poorly documented
> and thus open for various interpretations - so it's debatable what's important
> and what's not. [If it is properly documented anywhere, please educate me]
I agree with you. Even I coudn't find a proper documentation for the same.
I never thought about security issuses, (i.e., user modifying the VF driver
to gain access over the network) while implementing this.

> > > Also, looks like an already tagged packet would be processed with the
> > > original vlan-id [instead of the one of PF has provided].
> > > Is that intentional?
> > No, this isn't intentional. I thought VST and VGT cannot co-exist.
> > What should be the behavior?
> 
> Are you preventing VGT configuration once VST is configured?
> If not, what to prevent VM user from configuring vlan interfaces
> on top of the VF, even if VST is configured?
Again this misses documentation, what if VLAN interface is already configured in
VM before VST is configured. 
Before there were callbacks to add/remove vlan interface, now that is removed how
to achieve it?
    OR 
am I missing something?

RE: [PATCH net-next 1/2] cxgb4/cxgb4vf: Add support for ndo_set_vf_vlan

[Yuval Mintz]

> > Are you preventing VGT configuration once VST is configured?
> > If not, what to prevent VM user from configuring vlan interfaces on
> > top of the VF, even if VST is configured?
> Again this misses documentation, what if VLAN interface is already configured in
> VM before VST is configured.
> Before there were callbacks to add/remove vlan interface, now that is removed
> how to achieve it?
>     OR
> am I missing something?

I can only offer what our drivers [bnx2x, qed*] are doing - 
  -  VST is achieved by FW adding the vlan tag [unknowingly to VF] on egress traffic,
and silently stripping the vlan tag from the incoming traffic [so VF driver never
sees the tag].
  - Once VST is enabled, device [well, actually PF driver] would reject any further
requests for configuring VGT.
  - Once VST is configured, device would silently drop any vlan-tagged egress
Traffic sent  by VF, and would classify incoming traffic for that VF only if it's
tagged with the VGT vlan-id.
This has the effect of making existing vlan-interface over the VF completely
dysfunctional [as all transmissions from them would be dropped and they'll
never see any additional incoming traffic].

Re: [PATCH net-next 1/2] cxgb4/cxgb4vf: Add support for ndo_set_vf_vlan

[David Miller]

From: Hariprasad Shenai <hariprasad@chelsio.com>
Date: Wed, 24 Aug 2016 12:20:33 +0530

> +	dev_info(pi->adapter->pdev_dev,
> +		 "Setting Vlan %u to VF [%d]\n", vlan, vf);
> +	dev_info(pi->adapter->pdev_dev,
> +		 "The VF [%d] interface needs to brought down and up, "
> +		 "if VF is already up and running, for VST to work\n", vf);

Requiring the user to do something like this for the feature to work
correctly is not reasonable at all.

I want you to seriously consider rearchitecting this whole facility.