RE: [PATCH] net/i40e: fix single VLAN cannot work normal

["Liu, KevinX" <kevinx.liu@intel.com>]

> -----Original Message-----
> From: Zhang, Yuying <yuying.zhang@intel.com>
> Sent: Tuesday, September 6, 2022 10:16 AM
> To: Liu, KevinX <kevinx.liu@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Yang, SteveX <stevex.yang@intel.com>
> Subject: RE: [PATCH] net/i40e: fix single VLAN cannot work normal
> 
> Hi,
> 
> > -----Original Message-----
> > From: Liu, KevinX <kevinx.liu@intel.com>
> > Sent: Friday, August 19, 2022 12:04 AM
> > To: dev@dpdk.org
> > Cc: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Yang, SteveX <stevex.yang@intel.com>; Liu,
> > KevinX <kevinx.liu@intel.com>
> > Subject: [PATCH] net/i40e: fix single VLAN cannot work normal
> >
> > After disable QinQ, single VLAN can not work normal.
> > The reason is that QinQ is not disabled correctly.
> >
> > Before configuring QinQ, need to back up and clean MAC/VLAN filters of
> > all ports. After configuring QinQ, restore MAC/VLAN filters of all
> > ports. When disable QinQ, need to set valid_flags to 0x0008 and set first_tag
> to 0x88a8.
> >
> > Signed-off-by: Kevin Liu <kevinx.liu@intel.com>
> > ---
> >  doc/guides/nics/i40e.rst       |   1 -
> >  drivers/net/i40e/i40e_ethdev.c | 159
> > +++++++++++++++++++++++----------
> >  2 files changed, 111 insertions(+), 49 deletions(-)
> >
> > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst index
> > abb99406b3..15b796e67a 100644
> > --- a/doc/guides/nics/i40e.rst
> > +++ b/doc/guides/nics/i40e.rst
> > @@ -983,7 +983,6 @@ If FW version >= 8.4, there'll be some Vlan related
> issues:
> >
> >  #. TCI input set for QinQ  is invalid.
> >  #. Fail to configure TPID for QinQ.
> > -#. Need to enable QinQ before enabling Vlan filter.
> >  #. Fail to strip outer Vlan.
> >
> >  Example of getting best performance with l3fwd example diff --git
> > a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> > index
> > 67d79de08d..27cfda6ff8 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -325,6 +325,7 @@ static int i40e_veb_release(struct i40e_veb *veb);
> > static struct i40e_veb *i40e_veb_setup(struct i40e_pf *pf,
> >  						struct i40e_vsi *vsi);
> >  static int i40e_vsi_config_double_vlan(struct i40e_vsi *vsi, int on);
> > +static int i40e_vsi_config_qinq(struct i40e_vsi *vsi, int on);
> >  static inline int i40e_find_all_mac_for_vlan(struct i40e_vsi *vsi,
> >  					     struct i40e_macvlan_filter *mv_f,
> >  					     int num,
> > @@ -3909,7 +3910,6 @@ i40e_vlan_tpid_set(struct rte_eth_dev *dev,
> >  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> > >dev_private);
> >  	int qinq = dev->data->dev_conf.rxmode.offloads &
> >  		   RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
> > -	u16 sw_flags = 0, valid_flags = 0;
> >  	int ret = 0;
> >
> >  	if ((vlan_type != RTE_ETH_VLAN_TYPE_INNER && @@ -3928,10
> > +3928,6 @@ i40e_vlan_tpid_set(struct rte_eth_dev *dev,
> >  	/* 802.1ad frames ability is added in NVM API 1.7*/
> >  	if (hw->flags & I40E_HW_FLAG_802_1AD_CAPABLE) {
> >  		if (qinq) {
> > -			if (pf->fw8_3gt) {
> > -				sw_flags =
> > I40E_AQ_SET_SWITCH_CFG_OUTER_VLAN;
> > -				valid_flags =
> > I40E_AQ_SET_SWITCH_CFG_OUTER_VLAN;
> > -			}
> >  			if (vlan_type == RTE_ETH_VLAN_TYPE_OUTER)
> >  				hw->first_tag = rte_cpu_to_le_16(tpid);
> >  			else if (vlan_type == RTE_ETH_VLAN_TYPE_INNER) @@
> > -3940,8 +3936,8 @@ i40e_vlan_tpid_set(struct rte_eth_dev *dev,
> >  			if (vlan_type == RTE_ETH_VLAN_TYPE_OUTER)
> >  				hw->second_tag = rte_cpu_to_le_16(tpid);
> >  		}
> > -		ret = i40e_aq_set_switch_config(hw, sw_flags,
> > -						valid_flags, 0, NULL);
> > +		ret = i40e_aq_set_switch_config(hw, 0,
> > +						0, 0, NULL);
> >  		if (ret != I40E_SUCCESS) {
> >  			PMD_DRV_LOG(ERR,
> >  				    "Set switch config failed aq_err: %d", @@ -
> > 3993,11 +3989,15 @@ static int  i40e_vlan_offload_set(struct
> > rte_eth_dev *dev, int mask)  {
> >  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> > >dev_private);
> > +	struct i40e_mac_filter_info *vmac_filter[RTE_MAX_ETHPORTS];
> > +	struct i40e_vsi *vvsi[RTE_MAX_ETHPORTS];
> >  	struct i40e_mac_filter_info *mac_filter;
> >  	struct i40e_vsi *vsi = pf->main_vsi;
> >  	struct rte_eth_rxmode *rxmode;
> > +	int vnum[RTE_MAX_ETHPORTS];
> >  	struct i40e_mac_filter *f;
> > -	int i, num;
> > +	int port_num = 0;
> > +	int i, num, j;
> >  	void *temp;
> >  	int ret;
> >
> > @@ -4018,50 +4018,80 @@ i40e_vlan_offload_set(struct rte_eth_dev *dev,
> > int
> > mask)
> >  	}
> >
> >  	if (mask & RTE_ETH_VLAN_EXTEND_MASK) {
> > -		i = 0;
> > -		num = vsi->mac_num;
> > -		mac_filter = rte_zmalloc("mac_filter_info_data",
> > -				 num * sizeof(*mac_filter), 0);
> > -		if (mac_filter == NULL) {
> > -			PMD_DRV_LOG(ERR, "failed to allocate memory");
> > -			return I40E_ERR_NO_MEMORY;
> > -		}
> > -
> > -		/*
> > -		 * Outer VLAN processing is supported after firmware v8.4,
> > kernel driver
> > -		 * also change the default behavior to support this feature. To
> > align with
> > -		 * kernel driver, set switch config in 'i40e_vlan_tpie_set' to
> > support for
> > -		 * outer VLAN processing. But it is forbidden for firmware to
> > change the
> > -		 * Inner/Outer VLAN configuration while there are MAC/VLAN
> > filters in the
> > -		 * switch table. Therefore, we need to clear the MAC table
> > before setting
> > -		 * config, and then restore the MAC table after setting. This
> > feature is
> > -		 * recommended to be used in firmware v8.6.
> > -		 */
> > -		/* Remove all existing mac */
> > -		RTE_TAILQ_FOREACH_SAFE(f, &vsi->mac_list, next, temp) {
> > -			mac_filter[i] = f->mac_info;
> > -			ret = i40e_vsi_delete_mac(vsi, &f-
> > >mac_info.mac_addr);
> > -			if (ret)
> > -				PMD_DRV_LOG(ERR, "i40e vsi delete mac fail.");
> > -			i++;
> > +		/*back up and clean mac/vlan filters of all ports*/
> > +		for (j = 0; j < RTE_MAX_ETHPORTS; j++) {
> > +			/*
> > +			 * It is impossible to confirm whether the port is pf
> > +			 * only through the state field, so it is also necessary
> > +			 * to verify the intr_handle field.
> > +			 */
> > +			if (rte_eth_devices[j].state != RTE_ETH_DEV_UNUSED
> > &&
> > +
> > rte_eth_devices[j].intr_handle) {
> > +				struct rte_eth_dev *tmp_dev =
> > &rte_eth_devices[j];
> > +				struct i40e_pf *tmp_pf =
> > +					I40E_DEV_PRIVATE_TO_PF(tmp_dev-
> > >data->dev_private);
> > +				struct i40e_vsi *tmp_vsi = tmp_pf->main_vsi;
> > +				i = 0;
> > +				num = tmp_vsi->mac_num;
> > +				mac_filter =
> > rte_zmalloc("mac_filter_info_data",
> > +						num * sizeof(*mac_filter), 0);
> > +				if (mac_filter == NULL) {
> > +					PMD_DRV_LOG(ERR, "failed to
> > allocate memory");
> > +					return I40E_ERR_NO_MEMORY;
> > +				}
> > +				/*
> > +				 * Outer VLAN processing is supported after
> > firmware v8.4,
> > +				 * kernel driver also change the default
> > behavior to support
> > +				 * this feature. To align with kernel driver, set
> > switch
> > +				 * config in 'i40e_vlan_tpid_set' to support for
> > outer VLAN
> > +				 * processing. But it is forbidden for firmware
> > to change the
> > +				 * Inner/Outer VLAN configuration while there
> > are MAC/VLAN
> > +				 * filters in the switch table. Therefore, we
> > need to clean
> > +				 * MAC/VLAN filters of all ports before setting
> > config, and
> > +				 * then restore the MAC table after setting. This
> > feature is
> > +				 * recommended to be used in firmware v8.6.
> > +				 */
> > +				/* Remove all existing mac */
> > +				RTE_TAILQ_FOREACH_SAFE(f, &tmp_vsi-
> > >mac_list, next, temp) {
> > +					mac_filter[i] = f->mac_info;
> > +					ret = i40e_vsi_delete_mac(tmp_vsi, &f-
> > >mac_info.mac_addr);
> > +					if (ret)
> > +						PMD_DRV_LOG(ERR, "i40e vsi
> > delete mac fail.");
> > +					i++;
> > +				}
> > +				vmac_filter[j] = mac_filter;
> > +				vvsi[j] = tmp_vsi;
> > +				vnum[j] = num;
> > +				port_num++;
> > +			}
> >  		}
> >  		if (rxmode->offloads & RTE_ETH_RX_OFFLOAD_VLAN_EXTEND)
> {
> > -			i40e_vsi_config_double_vlan(vsi, TRUE);
> > -			/* Set global registers with default ethertype. */
> > -			i40e_vlan_tpid_set(dev, RTE_ETH_VLAN_TYPE_OUTER,
> > -					   RTE_ETHER_TYPE_VLAN);
> > -			i40e_vlan_tpid_set(dev, RTE_ETH_VLAN_TYPE_INNER,
> > -					   RTE_ETHER_TYPE_VLAN);
> > +			if (pf->fw8_3gt) {
> > +				i40e_vsi_config_qinq(vsi, TRUE);
> > +			} else {
> > +				i40e_vsi_config_double_vlan(vsi, TRUE);
> > +				/* Set global registers with default ethertype.
> > */
> > +				i40e_vlan_tpid_set(dev,
> > RTE_ETH_VLAN_TYPE_OUTER,
> > +
> > 	RTE_ETHER_TYPE_VLAN);
> > +				i40e_vlan_tpid_set(dev,
> > RTE_ETH_VLAN_TYPE_INNER,
> > +
> > 	RTE_ETHER_TYPE_VLAN);
> > +			}
> >  		} else {
> > -			i40e_vsi_config_double_vlan(vsi, FALSE);
> > -		}
> > -		/* Restore all mac */
> > -		for (i = 0; i < num; i++) {
> > -			ret = i40e_vsi_add_mac(vsi, &mac_filter[i]);
> > -			if (ret)
> > -				PMD_DRV_LOG(ERR, "i40e vsi add mac fail.");
> > +			if (pf->fw8_3gt)
> > +				i40e_vsi_config_qinq(vsi, FALSE);
> > +			else
> > +				i40e_vsi_config_double_vlan(vsi, FALSE);
> > +		}
> > +		/*restore mac/vlan filters of all ports*/
> > +		for (j = 0; j < port_num; j++) {
> 
> The index here is incorrect.

Thanks, I will send v2.
> 
> > +			mac_filter = vmac_filter[j];
> > +			for (i = 0; i < vnum[j]; i++) {
> > +				ret = i40e_vsi_add_mac(vvsi[j], &mac_filter[i]);
> > +				if (ret)
> > +					PMD_DRV_LOG(ERR, "i40e vsi add mac
> > fail.");
> > +			}
> > +			rte_free(mac_filter);
> >  		}
> > -		rte_free(mac_filter);
> >  	}
> >
> >  	if (mask & RTE_ETH_QINQ_STRIP_MASK) { @@ -6181,6 +6211,39 @@
> > i40e_vsi_config_double_vlan(struct i40e_vsi *vsi, int
> > on)
> >  	return i40e_aq_set_port_parameters(hw, vsi->seid, 0, 1, on, NULL);
> > }
> >
> > +static int
> > +i40e_vsi_config_qinq(struct i40e_vsi *vsi, int on) {
> > +	struct i40e_hw *hw = I40E_VSI_TO_HW(vsi);
> > +	u16 sw_flags = 0, valid_flags = 0;
> > +	int ret = 0;
> > +
> > +	ret = i40e_aq_set_port_parameters(hw, vsi->seid, 0, 1, on, NULL);
> > +	if (ret != I40E_SUCCESS) {
> > +		PMD_DRV_LOG(ERR, "Failed to set port params");
> > +		return -1;
> > +	}
> > +
> > +	if (on) {
> > +		sw_flags = I40E_AQ_SET_SWITCH_CFG_OUTER_VLAN;
> > +		valid_flags = I40E_AQ_SET_SWITCH_CFG_OUTER_VLAN;
> > +		hw->first_tag = rte_cpu_to_le_16(RTE_ETHER_TYPE_VLAN);
> > +	} else {
> > +		valid_flags = I40E_AQ_SET_SWITCH_CFG_OUTER_VLAN;
> > +		hw->first_tag = rte_cpu_to_le_16(RTE_ETHER_TYPE_QINQ);
> > +	}
> > +
> > +	ret = i40e_aq_set_switch_config(hw, sw_flags, valid_flags, 0, NULL);
> > +	if (ret) {
> > +		PMD_DRV_LOG(ERR,
> > +				    "Set switch config failed aq_err: %d",
> > +				    hw->aq.asq_last_status);
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Qinq is double vlan, please use i40e_vsi_config_double_vlan() function for bug
> fixing.

Ok, I will fix it.
> 
> > +
> >  static int
> >  i40e_update_flow_control(struct i40e_hw *hw)  {
> > --
> > 2.34.1