Re: [PATCH 1/7] vmxnet3: add support for VLAN filtering

[Yong Wang <yongwang-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>]

On 12/16/14, 9:13 PM, "Stephen Hemminger" <stephen@networkplumber.org>
wrote:

>From: Stephen Hemminger <shemming@brocade.com>
>
>VMXNET3 supports configuring filter table in host.
>
>Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>---
> lib/librte_pmd_vmxnet3/vmxnet3_ethdev.c | 107
>+++++++++++++++++++++++++++++---
> lib/librte_pmd_vmxnet3/vmxnet3_ethdev.h |   2 +-
> lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c   |  25 --------
> 3 files changed, 99 insertions(+), 35 deletions(-)
>
>diff --git a/lib/librte_pmd_vmxnet3/vmxnet3_ethdev.c
>b/lib/librte_pmd_vmxnet3/vmxnet3_ethdev.c
>index ef0af16..30d0659 100644
>--- a/lib/librte_pmd_vmxnet3/vmxnet3_ethdev.c
>+++ b/lib/librte_pmd_vmxnet3/vmxnet3_ethdev.c
>@@ -87,6 +87,12 @@ static void vmxnet3_dev_stats_get(struct rte_eth_dev
>*dev,
> 				struct rte_eth_stats *stats);
> static void vmxnet3_dev_info_get(struct rte_eth_dev *dev,
> 				struct rte_eth_dev_info *dev_info);
>+static int vmxnet3_dev_vlan_filter_set(struct rte_eth_dev *dev,
>+				       uint16_t vid, int on);
>+static void vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int
>mask);
>+static void vmxnet3_dev_vlan_offload_set_clear(struct rte_eth_dev *dev,
>+						int mask, int clear);
>+
> #if PROCESS_SYS_EVENTS == 1
> static void vmxnet3_process_events(struct vmxnet3_hw *);
> #endif
>@@ -113,6 +119,8 @@ static struct eth_dev_ops vmxnet3_eth_dev_ops = {
> 	.link_update          = vmxnet3_dev_link_update,
> 	.stats_get            = vmxnet3_dev_stats_get,
> 	.dev_infos_get        = vmxnet3_dev_info_get,
>+	.vlan_filter_set      = vmxnet3_dev_vlan_filter_set,
>+	.vlan_offload_set     = vmxnet3_dev_vlan_offload_set,
> 	.rx_queue_setup       = vmxnet3_dev_rx_queue_setup,
> 	.rx_queue_release     = vmxnet3_dev_rx_queue_release,
> 	.tx_queue_setup       = vmxnet3_dev_tx_queue_setup,
>@@ -371,7 +379,7 @@ vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
> 	Vmxnet3_DSDevRead *devRead = &shared->devRead;
> 	uint32_t *mac_ptr;
> 	uint32_t val, i;
>-	int ret;
>+	int ret, mask;
> 
> 	shared->magic = VMXNET3_REV1_MAGIC;
> 	devRead->misc.driverInfo.version = VMXNET3_DRIVER_VERSION_NUM;
>@@ -442,9 +450,6 @@ vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
> 	if (dev->data->dev_conf.rxmode.hw_ip_checksum)
> 		devRead->misc.uptFeatures |= VMXNET3_F_RXCSUM;
> 
>-	if (dev->data->dev_conf.rxmode.hw_vlan_strip)
>-		devRead->misc.uptFeatures |= VMXNET3_F_RXVLAN;
>-
> 	if (port_conf.rxmode.mq_mode == ETH_MQ_RX_RSS) {
> 		ret = vmxnet3_rss_configure(dev);
> 		if (ret != VMXNET3_SUCCESS)
>@@ -456,11 +461,14 @@ vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
> 		devRead->rssConfDesc.confPA  = hw->rss_confPA;
> 	}
> 
>-	if (dev->data->dev_conf.rxmode.hw_vlan_filter) {
>-		ret = vmxnet3_vlan_configure(dev);
>-		if (ret != VMXNET3_SUCCESS)
>-			return ret;
>-	}
>+	mask = 0;
>+	if (dev->data->dev_conf.rxmode.hw_vlan_strip)
>+		mask |= ETH_VLAN_STRIP_MASK;
>+
>+	if (dev->data->dev_conf.rxmode.hw_vlan_filter)
>+		mask |= ETH_VLAN_FILTER_MASK;
>+
>+	vmxnet3_dev_vlan_offload_set_clear(dev, mask, 1);
> 
> 	PMD_INIT_LOG(DEBUG,
> 		     "Writing MAC Address : %02x:%02x:%02x:%02x:%02x:%02x",
>@@ -690,13 +698,23 @@ vmxnet3_dev_set_rxmode(struct vmxnet3_hw *hw,
>uint32_t feature, int set) {
> 	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_UPDATE_RX_MODE);
> }
> 
>+static void
>+vmxnet3_dev_update_filters(struct vmxnet3_hw *hw)
>+{
>+	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
>+			       VMXNET3_CMD_UPDATE_VLAN_FILTERS);
>+}

This function is not really necessary given what it does.  If you prefer
to keep it, please rename it to vmxnet3_dev_update_vlan_filters to be more
precise.

>+
> /* Promiscuous supported only if Vmxnet3_DriverShared is initialized in
>adapter */
> static void
> vmxnet3_dev_promiscuous_enable(struct rte_eth_dev *dev)
> {
> 	struct vmxnet3_hw *hw = dev->data->dev_private;
>+	uint32_t *vf_table = hw->shared->devRead.rxFilterConf.vfTable;
> 
>+	memset(vf_table, 0, VMXNET3_VFT_SIZE * sizeof(*vf_table));
> 	vmxnet3_dev_set_rxmode(hw, VMXNET3_RXM_PROMISC, 1);
>+	vmxnet3_dev_update_filters(hw);
> }
> 
> /* Promiscuous supported only if Vmxnet3_DriverShared is initialized in
>adapter */
>@@ -704,8 +722,11 @@ static void
> vmxnet3_dev_promiscuous_disable(struct rte_eth_dev *dev)
> {
> 	struct vmxnet3_hw *hw = dev->data->dev_private;
>+	uint32_t *vf_table = hw->shared->devRead.rxFilterConf.vfTable;
> 
>+	memset(vf_table, 0, VMXNET3_VFT_SIZE * sizeof(*vf_table));

Rather than clearing all vlan filters, we should restore the saved filters
in shadow_vfta before promiscuous mode is enabled.  This is also needed
when the device is stopped/restarted.

> 	vmxnet3_dev_set_rxmode(hw, VMXNET3_RXM_PROMISC, 0);
>+	vmxnet3_dev_update_filters(hw);
> }
> 
> /* Allmulticast supported only if Vmxnet3_DriverShared is initialized in
>adapter */
>@@ -726,6 +747,74 @@ vmxnet3_dev_allmulticast_disable(struct rte_eth_dev
>*dev)
> 	vmxnet3_dev_set_rxmode(hw, VMXNET3_RXM_ALL_MULTI, 0);
> }
> 
>+/* Enable/disable filter on vlan */
>+static int
>+vmxnet3_dev_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vid, int
>on)
>+{
>+	struct vmxnet3_hw *hw = dev->data->dev_private;
>+	struct Vmxnet3_RxFilterConf *rxConf = &hw->shared->devRead.rxFilterConf;
>+	uint32_t *vf_table = hw->shared->devRead.rxFilterConf.vfTable;

Since you already defined rxConf, vf_table can be defined as:
uint32_t *vf_table = rxConf->vfTable;

>+
>+	/* save state for restore */
>+	if (on)
>+		VMXNET3_SET_VFTABLE_ENTRY(hw->shadow_vfta, vid);
>+	else
>+		VMXNET3_CLEAR_VFTABLE_ENTRY(hw->shadow_vfta, vid);
>+
>+	/* don't change active filter if in promiscious mode */
>+	if (rxConf->rxMode & VMXNET3_RXM_PROMISC)
>+		return 0;
>+
>+	/* set in hardware */
>+	if (on)
>+		VMXNET3_SET_VFTABLE_ENTRY(vf_table, vid);
>+	else
>+		VMXNET3_CLEAR_VFTABLE_ENTRY(vf_table, vid);
>+
>+	vmxnet3_dev_update_filters(hw);
>+	return 0;
>+}
>+
>+
>+static void
>+vmxnet3_dev_vlan_offload_set_clear(struct rte_eth_dev *dev,
>+						int mask, int clear)

Remove on tab here for better indention.

>+{
>+	struct vmxnet3_hw *hw = dev->data->dev_private;
>+	uint32_t *vf_table = hw->shared->devRead.rxFilterConf.vfTable;
>+	Vmxnet3_DSDevRead *devRead = &hw->shared->devRead;

devRead can be defined before vf_table and vf_table as
devRead->rxFilterConf.vfTable.

>+
>+	if (mask & ETH_VLAN_STRIP_MASK)
>+		devRead->misc.uptFeatures |= UPT1_F_RXVLAN;
>+	else
>+		devRead->misc.uptFeatures &= ~UPT1_F_RXVLAN;

There is a missing REG write after making changes to uptFeatures from
the .vlan_offload_set callback path (i.e, when “clear" is 0).

VMXNET3_WRITE_BAR1_REG(sc, VMXNET3_REG_CMD, VMXNET3_CMD_UPDATE_FEATURE);

>+
>+	if (mask & ETH_VLAN_FILTER_MASK) {
>+		if (clear) {
>+			memset(hw->shadow_vfta, 0,
>+					VMXNET3_VFT_SIZE * sizeof(uint32_t));

Remove one tab here for better indention.

Instead of uint32_t, change it to sizeof(*hw->shadow_vfta) to be
consistent with the memset of vf_table.

 
>+			/* allow untagged pkts */
>+			VMXNET3_SET_VFTABLE_ENTRY(hw->shadow_vfta, 0);
>+		}
>+		memcpy(vf_table, hw->shadow_vfta,
>+				VMXNET3_VFT_SIZE * sizeof(*vf_table));
>+	} else {
>+		/* allow any pkts -- no filtering */
>+		if (clear)
>+			memset(hw->shadow_vfta, 0xff,
>+				VMXNET3_VFT_SIZE * sizeof(uint32_t));
>+		memset(vf_table, 0xff, VMXNET3_VFT_SIZE * sizeof(uint32_t));

Same here.

>+	}
>+
>+	vmxnet3_dev_update_filters(hw);
>+}
>+
>+static void
>+vmxnet3_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>+{
>+	vmxnet3_dev_vlan_offload_set_clear(dev, mask, 0);
>+}
>+
> #if PROCESS_SYS_EVENTS == 1
> static void
> vmxnet3_process_events(struct vmxnet3_hw *hw)
>diff --git a/lib/librte_pmd_vmxnet3/vmxnet3_ethdev.h
>b/lib/librte_pmd_vmxnet3/vmxnet3_ethdev.h
>index 0941cfc..2c180ad 100644
>--- a/lib/librte_pmd_vmxnet3/vmxnet3_ethdev.h
>+++ b/lib/librte_pmd_vmxnet3/vmxnet3_ethdev.h
>@@ -115,6 +115,7 @@ struct vmxnet3_hw {
> 	VMXNET3_RSSConf		 *rss_conf;
> 	uint64_t			 rss_confPA;
> 	vmxnet3_mf_table_t   *mf_table;
>+	uint32_t	      shadow_vfta[VMXNET3_VFT_SIZE];
> };
> 
> #define VMXNET3_GET_ADDR_LO(reg)   ((uint32_t)(reg))
>@@ -167,7 +168,6 @@ int  vmxnet3_dev_tx_queue_setup(struct rte_eth_dev
>*dev, uint16_t tx_queue_id,
> int vmxnet3_dev_rxtx_init(struct rte_eth_dev *dev);
> 
> int vmxnet3_rss_configure(struct rte_eth_dev *dev);
>-int vmxnet3_vlan_configure(struct rte_eth_dev *dev);
> 
> uint16_t vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> 		uint16_t nb_pkts);
>diff --git a/lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c
>b/lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c
>index 8425f32..9871f16 100644
>--- a/lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c
>+++ b/lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c
>@@ -1051,28 +1051,3 @@ vmxnet3_rss_configure(struct rte_eth_dev *dev)
> 
> 	return VMXNET3_SUCCESS;
> }
>-
>-/*
>- * Configure VLAN Filter feature
>- */
>-int
>-vmxnet3_vlan_configure(struct rte_eth_dev *dev)
>-{
>-	uint8_t i;
>-	struct vmxnet3_hw *hw = dev->data->dev_private;
>-	uint32_t *vf_table = hw->shared->devRead.rxFilterConf.vfTable;
>-
>-	PMD_INIT_FUNC_TRACE();
>-
>-	/* Verify if this tag is already set */
>-	for (i = 0; i < VMXNET3_VFT_SIZE; i++) {
>-		/* Filter all vlan tags out by default */
>-		vf_table[i] = 0;
>-		/* To-Do: Provide another routine in dev_ops for user config */
>-
>-		PMD_INIT_LOG(DEBUG, "Registering VLAN portid: %"PRIu8" tag %u",
>-					dev->data->port_id, vf_table[i]);
>-	}
>-
>-	return VMXNET3_SUCCESS;
>-}
>-- 
>2.1.3