> -----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?
> 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
