> -----Original Message-----
> From: Yang, Qiming <qiming.y...@intel.com>
> Sent: Friday, September 2, 2022 11:29 AM
> To: Liu, KevinX <kevinx....@intel.com>; dev@dpdk.org
> Cc: Zhang, Yuying <yuying.zh...@intel.com>; Xing, Beilei
> <beilei.x...@intel.com>; Yang, SteveX <stevex.y...@intel.com>; Liu, KevinX
> <kevinx....@intel.com>
> Subject: RE: [PATCH] net/i40e: fix incorrect VLAN stripping for QinQ
>
>
>
> > -----Original Message-----
> > From: Kevin Liu <kevinx....@intel.com>
> > Sent: Thursday, September 1, 2022 6:06 PM
> > To: dev@dpdk.org
> > Cc: Zhang, Yuying <yuying.zh...@intel.com>; Xing, Beilei
> > <beilei.x...@intel.com>; Yang, SteveX <stevex.y...@intel.com>; Liu,
> > KevinX <kevinx....@intel.com>
> > Subject: [PATCH] net/i40e: fix incorrect VLAN stripping for QinQ
> >
> > QinQ enable, when enable strip function, it is wrong to strip inner
> > VLAN of double VLAN package. The correct action is outer VLAN is
> > stripped. So, need to configure 'outer_vlan_flags' to update vsi.
> >
>
> This commit message don't explain why we need to change the inner vlan strip
> to outer vlan.
> We should align with kernel driver's new behavior.
> And from my point of view, we have no need to add too many new functions for
> this fix.
> Can you work out another simple design?
Thanks for your suggestion. I'll rework it.
>
> > When enable QinQ strip function, need to set 'port_vlan_flags' to
> > configure inner VLAN strip.
> >
> > Signed-off-by: Kevin Liu <kevinx....@intel.com>
> > ---
> > doc/guides/nics/i40e.rst | 2 -
> > drivers/net/i40e/i40e_ethdev.c | 170 +++++++++++++++++++++++++++++++--
> > drivers/net/i40e/i40e_ethdev.h | 4 +
> > 3 files changed, 164 insertions(+), 12 deletions(-)
> >
> > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst index
> > 15b796e67a..ffcb2a2220 100644
> > --- a/doc/guides/nics/i40e.rst
> > +++ b/doc/guides/nics/i40e.rst
> > @@ -982,8 +982,6 @@ Vlan related Features miss when FW >= 8.4 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.
> > -#. 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 27cfda6ff8..0c3009ebfa 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -52,6 +52,8 @@
> > #define I40E_VSI_TSR_QINQ_STRIP 0x4010
> > #define I40E_VSI_TSR(_i) (0x00050800 + ((_i) * 4))
> >
> > +#define I40E_OVLAN_EMOD_SHIFT(x) ((x) <<
> > I40E_AQ_VSI_OVLAN_EMOD_SHIFT)
> > +
> > /* Maximun number of capability elements */
> > #define I40E_MAX_CAP_ELE_NUM 128
> >
> > @@ -4011,10 +4013,15 @@ i40e_vlan_offload_set(struct rte_eth_dev *dev,
> > int mask)
> >
> > if (mask & RTE_ETH_VLAN_STRIP_MASK) {
> > /* Enable or disable VLAN stripping */
> > - if (rxmode->offloads & RTE_ETH_RX_OFFLOAD_VLAN_STRIP)
> > - i40e_vsi_config_vlan_stripping(vsi, TRUE);
> > - else
> > + if (rxmode->offloads & RTE_ETH_RX_OFFLOAD_VLAN_STRIP)
> > {
> > + if (rxmode->offloads &
> > RTE_ETH_RX_OFFLOAD_VLAN_EXTEND)
> > + i40e_vsi_config_vlan_stripping_v1(vsi, TRUE);
> > + else
> > + i40e_vsi_config_vlan_stripping(vsi, TRUE);
> > + } else {
> > i40e_vsi_config_vlan_stripping(vsi, FALSE);
> > + i40e_vsi_config_vlan_stripping_v1(vsi, FALSE);
> > + }
> > }
> >
> > if (mask & RTE_ETH_VLAN_EXTEND_MASK) { @@ -4068,6 +4075,10
> @@
> > i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask)
> > if (rxmode->offloads &
> > RTE_ETH_RX_OFFLOAD_VLAN_EXTEND) {
> > if (pf->fw8_3gt) {
> > i40e_vsi_config_qinq(vsi, TRUE);
> > + if (rxmode->offloads &
> > RTE_ETH_RX_OFFLOAD_VLAN_STRIP) {
> > + i40e_vsi_config_vlan_stripping(vsi,
> > FALSE);
> > + i40e_vsi_config_vlan_stripping_v1(vsi,
> > TRUE);
> > + }
> > } else {
> > i40e_vsi_config_double_vlan(vsi, TRUE);
> > /* Set global registers with default ethertype.
> > */ @@ -4077,10 +4088,15 @@ i40e_vlan_offload_set(struct rte_eth_dev
> > *dev, int mask)
> >
> > RTE_ETHER_TYPE_VLAN);
> > }
> > } else {
> > - if (pf->fw8_3gt)
> > + if (pf->fw8_3gt) {
> > i40e_vsi_config_qinq(vsi, FALSE);
> > - else
> > + if (rxmode->offloads &
> > RTE_ETH_RX_OFFLOAD_VLAN_STRIP) {
> > + i40e_vsi_config_vlan_stripping_v1(vsi,
> > FALSE);
> > + i40e_vsi_config_vlan_stripping(vsi,
> > TRUE);
> > + }
> > + } else {
> > i40e_vsi_config_double_vlan(vsi, FALSE);
> > + }
> > }
> > /*restore mac/vlan filters of all ports*/
> > for (j = 0; j < port_num; j++) {
> > @@ -4096,10 +4112,17 @@ i40e_vlan_offload_set(struct rte_eth_dev *dev,
> > int mask)
> >
> > if (mask & RTE_ETH_QINQ_STRIP_MASK) {
> > /* Enable or disable outer VLAN stripping */
> > - if (rxmode->offloads & RTE_ETH_RX_OFFLOAD_QINQ_STRIP)
> > - i40e_vsi_config_outer_vlan_stripping(vsi, TRUE);
> > - else
> > - i40e_vsi_config_outer_vlan_stripping(vsi, FALSE);
> > + if (rxmode->offloads & RTE_ETH_RX_OFFLOAD_QINQ_STRIP)
> > {
> > + if (pf->fw8_3gt)
> > + i40e_vsi_config_inner_vlan_stripping(vsi,
> > TRUE);
> > + else
> > + i40e_vsi_config_outer_vlan_stripping(vsi,
> > TRUE);
> > + } else {
> > + if (pf->fw8_3gt)
> > + i40e_vsi_config_inner_vlan_stripping(vsi,
> > FALSE);
> > + else
> > + i40e_vsi_config_outer_vlan_stripping(vsi,
> > FALSE);
> > + }
> > }
> >
> > return 0;
> > @@ -5231,6 +5254,7 @@ int
> > i40e_vsi_vlan_pvid_set(struct i40e_vsi *vsi,
> > struct i40e_vsi_vlan_pvid_info *info) {
> > + struct i40e_pf *pf = I40E_VSI_TO_PF(vsi);
> > struct i40e_hw *hw;
> > struct i40e_vsi_context ctxt;
> > uint8_t vlan_flags = 0;
> > @@ -5241,6 +5265,9 @@ i40e_vsi_vlan_pvid_set(struct i40e_vsi *vsi,
> > return I40E_ERR_PARAM;
> > }
> >
> > + if (pf->fw8_3gt)
> > + return i40e_vsi_vlan_ovid_set(vsi, info);
> > +
> > if (info->on) {
> > vsi->info.pvid = info->config.pvid;
> > /**
> > @@ -5880,8 +5907,16 @@ i40e_vsi_setup(struct i40e_pf *pf,
> > memset(&ctxt, 0, sizeof(ctxt));
> > vsi->info.valid_sections |=
> > rte_cpu_to_le_16(I40E_AQ_VSI_PROP_VLAN_VALID);
> > - vsi->info.port_vlan_flags = I40E_AQ_VSI_PVLAN_MODE_ALL
> > |
> > + if (pf->fw8_3gt) {
> > + vsi->info.port_vlan_flags =
> > I40E_AQ_VSI_PVLAN_MODE_ALL |
> > +
> > I40E_AQ_VSI_PVLAN_EMOD_NOTHING;
> > + vsi->info.outer_vlan_flags =
> > I40E_AQ_VSI_OVLAN_MODE_ALL |
> > +
> > I40E_OVLAN_EMOD_SHIFT(I40E_AQ_VSI_OVLAN_EMOD_SHOW_ALL)
> > |
> > +
> > I40E_OVLAN_EMOD_SHIFT(I40E_AQ_VSI_OVLAN_CTRL_ENA);
> > + } else {
> > + vsi->info.port_vlan_flags =
> > I40E_AQ_VSI_PVLAN_MODE_ALL |
> >
> > I40E_AQ_VSI_PVLAN_EMOD_STR_BOTH;
> > + }
> > rte_memcpy(&ctxt.info, &vsi->info,
> > sizeof(struct i40e_aqc_vsi_properties_data));
> > ret = i40e_vsi_config_tc_queue_mapping(vsi, &ctxt.info, @@
> > -6175,6 +6210,121 @@ i40e_vsi_config_vlan_stripping(struct i40e_vsi
> > *vsi, bool on)
> > return ret;
> > }
> >
> > +int i40e_vsi_config_vlan_stripping_v1(struct i40e_vsi *vsi, bool on)
> > +{
>
> Why need to add a new function? Not fix it in exist one?
>
> > + struct i40e_hw *hw = I40E_VSI_TO_HW(vsi);
> > + struct i40e_vsi_context ctxt;
> > + uint8_t vlan_flags;
> > + int ret = I40E_SUCCESS;
> > +
> > + /* Check if it has been already on or off */
> > + if (vsi->info.valid_sections &
> > + rte_cpu_to_le_16(I40E_AQ_VSI_PROP_VLAN_VALID)) {
> > + if (on) {
> > + if ((vsi->info.outer_vlan_flags &
> > + I40E_AQ_VSI_OVLAN_EMOD_MASK)
> > == 0)
> > + return 0; /* already on */
> > + } else {
> > + if ((vsi->info.outer_vlan_flags &
> > + I40E_AQ_VSI_OVLAN_EMOD_MASK) ==
> > + I40E_AQ_VSI_OVLAN_EMOD_MASK)
> > + return 0; /* already off */
> > + }
> > + }
> > +
> > + if (on)
> > + vlan_flags = I40E_AQ_VSI_OVLAN_MODE_ALL |
> > +
> > I40E_OVLAN_EMOD_SHIFT(I40E_AQ_VSI_OVLAN_EMOD_SHOW_ALL)
> > |
> > +
> > I40E_OVLAN_EMOD_SHIFT(I40E_AQ_VSI_OVLAN_CTRL_ENA);
> > + else
> > + vlan_flags = I40E_AQ_VSI_OVLAN_MODE_ALL |
> > +
> > I40E_OVLAN_EMOD_SHIFT(I40E_AQ_VSI_OVLAN_EMOD_NOTHING)
> > |
> > +
> > I40E_OVLAN_EMOD_SHIFT(I40E_AQ_VSI_OVLAN_CTRL_ENA);
> > +
> > + vsi->info.valid_sections =
> > + rte_cpu_to_le_16(I40E_AQ_VSI_PROP_VLAN_VALID);
> > + vsi->info.outer_vlan_flags = vlan_flags;
> > + ctxt.seid = vsi->seid;
> > + rte_memcpy(&ctxt.info, &vsi->info, sizeof(vsi->info));
> > + ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
> > + if (ret)
> > + PMD_DRV_LOG(INFO, "Update VSI failed to %s outer vlan
> > stripping",
> > + on ? "enable" : "disable");
> > +
> > + return ret;
> > +}
> > +
> > +int i40e_vsi_config_inner_vlan_stripping(struct i40e_vsi *vsi, bool
> > +on) {
> > + struct i40e_hw *hw = I40E_VSI_TO_HW(vsi);
> > + struct i40e_vsi_context ctxt;
> > + uint8_t vlan_flags;
> > + int ret = I40E_SUCCESS;
> > +
> > + if (on)
> > + vlan_flags = I40E_AQ_VSI_PVLAN_EMOD_STR_BOTH;
> > + else
> > + vlan_flags = I40E_AQ_VSI_PVLAN_EMOD_NOTHING;
> > + vsi->info.valid_sections =
> > + rte_cpu_to_le_16(I40E_AQ_VSI_PROP_VLAN_VALID);
> > + vsi->info.port_vlan_flags &= ~(I40E_AQ_VSI_PVLAN_EMOD_MASK);
> > + vsi->info.port_vlan_flags |= vlan_flags;
> > + ctxt.seid = vsi->seid;
> > + rte_memcpy(&ctxt.info, &vsi->info, sizeof(vsi->info));
> > + ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
> > + if (ret)
> > + PMD_DRV_LOG(INFO, "Update VSI failed to %s inner
> > stripping",
> > + on ? "enable" : "disable");
> > +
> > + return ret;
> > +}
> > +
> > +int
> > +i40e_vsi_vlan_ovid_set(struct i40e_vsi *vsi,
> > + struct i40e_vsi_vlan_pvid_info *info) {
> > + struct i40e_hw *hw;
> > + struct i40e_vsi_context ctxt;
> > + uint8_t vlan_flags = 0;
> > + int ret;
> > +
> > + if (vsi == NULL || info == NULL) {
> > + PMD_DRV_LOG(ERR, "invalid parameters");
> > + return I40E_ERR_PARAM;
> > + }
> > +
> > + if (info->on) {
> > + vsi->info.outer_vlan = info->config.pvid;
> > + vlan_flags = I40E_AQ_VSI_OVLAN_MODE_UNTAGGED |
> > + I40E_AQ_VSI_OVLAN_INSERT_PVID |
> > +
> > I40E_OVLAN_EMOD_SHIFT(I40E_AQ_VSI_OVLAN_EMOD_HIDE_ALL)
> > |
> > +
> > I40E_OVLAN_EMOD_SHIFT(I40E_AQ_VSI_OVLAN_CTRL_ENA);
> > + } else {
> > + vsi->info.outer_vlan = 0;
> > + if (info->config.reject.tagged == 0)
> > + vlan_flags |= I40E_AQ_VSI_OVLAN_MODE_TAGGED;
> > +
> > + if (info->config.reject.untagged == 0)
> > + vlan_flags |=
> > I40E_AQ_VSI_OVLAN_MODE_UNTAGGED;
> > + }
> > + vsi->info.outer_vlan_flags |= vlan_flags;
> > + vsi->info.outer_vlan_flags &= ~(I40E_AQ_VSI_OVLAN_INSERT_PVID |
> > + I40E_AQ_VSI_OVLAN_MODE_MASK);
> > + vsi->info.valid_sections =
> > + rte_cpu_to_le_16(I40E_AQ_VSI_PROP_VLAN_VALID);
> > + memset(&ctxt, 0, sizeof(ctxt));
> > + rte_memcpy(&ctxt.info, &vsi->info, sizeof(vsi->info));
> > + ctxt.seid = vsi->seid;
> > +
> > + hw = I40E_VSI_TO_HW(vsi);
> > + ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
> > + if (ret != I40E_SUCCESS)
> > + PMD_DRV_LOG(ERR, "Failed to update VSI params");
> > +
> > + return ret;
> > +}
> > +
> > +
> > static int
> > i40e_dev_init_vlan(struct rte_eth_dev *dev) { diff --git
> > a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> > index
> > fe943a45ff..2f513fce52 100644
> > --- a/drivers/net/i40e/i40e_ethdev.h
> > +++ b/drivers/net/i40e/i40e_ethdev.h
> > @@ -1316,6 +1316,10 @@ void i40e_vsi_disable_queues_intr(struct
> > i40e_vsi *vsi); int i40e_vsi_vlan_pvid_set(struct i40e_vsi *vsi,
> > struct i40e_vsi_vlan_pvid_info *info); int
> > i40e_vsi_config_vlan_stripping(struct i40e_vsi *vsi, bool on);
> > +int i40e_vsi_config_vlan_stripping_v1(struct i40e_vsi *vsi, bool on);
> > +int i40e_vsi_config_inner_vlan_stripping(struct i40e_vsi *vsi, bool
> > +on); int i40e_vsi_vlan_ovid_set(struct i40e_vsi *vsi,
> > + struct i40e_vsi_vlan_pvid_info *info);
> > int i40e_vsi_config_vlan_filter(struct i40e_vsi *vsi, bool on);
> > uint64_t i40e_config_hena(const struct i40e_adapter *adapter, uint64_t
> > flags); uint64_t i40e_parse_hena(const struct i40e_adapter *adapter,
> > uint64_t flags);
> > --
> > 2.34.1
