|SUCCESS| [v3] net/iavf: add support for QinQ strip

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162954

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-09

I'll review this DPDK patch for the iavf driver that adds QinQ strip support.

commit <not provided - patch file format>
Author: Anurag Mandal <anurag.mandal at intel.com>

net/iavf: add support for QinQ strip

This patch adds support for QinQ stripping with both Outer VLAN TPIDs
0x88a8 (802.1ad) and 0x8100 (802.1Q). The implementation enables QinQ
strip functionality through VLAN extend configuration and proper TPID
settings.

> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index f8008d0fda..35d82b7cc0 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -292,6 +292,7 @@ struct iavf_info {
>  	bool in_reset_recovery;
>  
>  	uint32_t ptp_caps;
> +	uint16_t tpid;  /* VLAN tag identifier */

Does the comment accurately describe this field? The TPID is specifically
the Tag Protocol Identifier used for outer VLAN tagging, not a generic
"VLAN tag identifier". A more precise comment might be "Outer VLAN TPID"
or "QinQ outer TPID".

>  	rte_spinlock_t phc_time_aq_lock;
>  };
> 
> @@ -456,6 +457,7 @@ int iavf_configure_rss_key(struct iavf_adapter *adapter);
>  int iavf_configure_queues(struct iavf_adapter *adapter, uint16_t num_queue_pairs);
>  int iavf_get_supported_rxdid(struct iavf_adapter *adapter);
>  int iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable);
> +int iavf_config_outer_vlan_strip_v2(struct iavf_adapter *adapter, bool enable);
>  int iavf_config_vlan_insert_v2(struct iavf_adapter *adapter, bool enable);
>  int iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid,
>  			 bool add);
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 3126d9b644..f3094fffa9 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c

[ ... ]

> @@ -717,6 +720,7 @@ iavf_dev_configure(struct rte_eth_dev *dev)
>  	}
>  
>  	iavf_dev_init_vlan(dev);
> +	vf->tpid = RTE_ETHER_TYPE_VLAN; /* VLAN TPID set to 0x8100 by default */

Does this initialization properly handle reconfiguration scenarios? If
iavf_dev_configure() is called multiple times (e.g., during port restart),
does resetting tpid to the default value potentially conflict with a
previously configured outer TPID via iavf_vlan_tpid_set()?

[ ... ]

> @@ -1365,6 +1369,32 @@ iavf_dev_del_mac_addr(struct rte_eth_dev *dev, uint32_t index)
>  	vf->mac_num--;
>  }
>  
> +static int
> +iavf_vlan_tpid_set(struct rte_eth_dev *dev, enum rte_vlan_type vlan_type, uint16_t tpid)
> +{
> +	struct iavf_adapter *adapter =
> +		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> +	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
> +	int qinq = dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
> +
> +	if ((vlan_type != RTE_ETH_VLAN_TYPE_INNER &&
> +	    vlan_type != RTE_ETH_VLAN_TYPE_OUTER) ||
> +	    (!qinq && vlan_type == RTE_ETH_VLAN_TYPE_INNER)) {
> +		PMD_DRV_LOG(ERR, "Unsupported vlan type.");
> +		return -EINVAL;
> +	}
> +
> +	/* This API only fills internal iavf_info structure
> +	 * and does not send any signal to hardware
> +	 */
> +	if (qinq && vlan_type == RTE_ETH_VLAN_TYPE_OUTER)
> +		vf->tpid = rte_cpu_to_le_16(tpid); /* Outer VLAN can be 0x88a8 or 0x8100 */

Does this function need TPID value validation? The code stores the tpid
without checking whether the provided value is valid (0x88a8 or 0x8100).
Can invalid TPID values cause issues in iavf_config_outer_vlan_strip_v2()
or iavf_add_del_vlan_v2()?

Also, does the byte order conversion make sense here? The tpid field is
uint16_t and later compared directly with RTE_ETHER_TYPE_QINQ and
RTE_ETHER_TYPE_VLAN which are in CPU byte order. Should this be stored in
CPU byte order instead?

> +	else
> +		vf->tpid = RTE_ETHER_TYPE_VLAN; /* Inner VLAN 0x8100 */
> +	return 0;
> +}

[ ... ]

> @@ -1453,6 +1483,8 @@ iavf_dev_vlan_offload_set_v2(struct rte_eth_dev *dev, int mask)
>  	struct iavf_adapter *adapter =
>  		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
>  	bool enable;
> +	int qinq = dev->data->dev_conf.rxmode.offloads &
> +		RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
>  	int err;
>  
>  	if (mask & RTE_ETH_VLAN_FILTER_MASK) {

[ ... ]

> @@ -1472,6 +1504,20 @@ iavf_dev_vlan_offload_set_v2(struct rte_eth_dev *dev, int mask)
>  			return -EIO;
>  	}
>  
> +	if (mask & RTE_ETH_QINQ_STRIP_MASK) {
> +		if (!qinq) {
> +			PMD_DRV_LOG(ERR, "VLAN-extend disabled");
> +			return -ENOSYS;
> +		}
> +		enable = !!(rxmode->offloads & RTE_ETH_RX_OFFLOAD_QINQ_STRIP);
> +		err = iavf_config_outer_vlan_strip_v2(adapter, enable);
> +		/* If not support, the stripping is already disabled by PF */
> +		if (err == -ENOTSUP && !enable)
> +			err = 0;
> +		if (err)
> +			return -EIO;
> +	}
> +
>  	return 0;
>  }

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c b/drivers/net/intel/iavf/iavf_vchnl.c
> index 23d115298c..bae5d54553 100644
> --- a/drivers/net/intel/iavf/iavf_vchnl.c
> +++ b/drivers/net/intel/iavf/iavf_vchnl.c
> @@ -829,6 +829,50 @@ iavf_get_supported_rxdid(struct iavf_adapter *adapter)
>  	return 0;
>  }
>  
> +int
> +iavf_config_outer_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
> +{
> +	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> +	struct virtchnl_vlan_supported_caps *stripping_caps;
> +	struct virtchnl_vlan_setting vlan_strip;
> +	uint8_t msg_buf[IAVF_AQ_BUF_SZ] = {0};
> +	struct iavf_cmd_info args;
> +	uint32_t *ethertype;
> +	int ret;
> +
> +	memset(&vlan_strip, 0, sizeof(vlan_strip));
> +	stripping_caps = &vf->vlan_v2_caps.offloads.stripping_support;
> +	if ((stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_88A8) &&
> +	    (stripping_caps->outer & VIRTCHNL_VLAN_TOGGLE) &&
> +	    vf->tpid == RTE_ETHER_TYPE_QINQ) {
> +		ethertype = &vlan_strip.outer_ethertype_setting;
> +		*ethertype = VIRTCHNL_VLAN_ETHERTYPE_88A8;
> +	} else if ((stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_8100) &&
> +		   (stripping_caps->outer & VIRTCHNL_VLAN_TOGGLE) &&
> +		   vf->tpid == RTE_ETHER_TYPE_VLAN) {
> +		ethertype = &vlan_strip.outer_ethertype_setting;
> +		*ethertype = VIRTCHNL_VLAN_ETHERTYPE_8100;
> +	} else {
> +		return -ENOTSUP;
> +	}

Does the comparison vf->tpid == RTE_ETHER_TYPE_QINQ work correctly given
the byte order handling in iavf_vlan_tpid_set()? The tpid field is stored
using rte_cpu_to_le_16(tpid) but compared here against CPU byte order
constants.

> +
> +	vlan_strip.vport_id = vf->vsi_res->vsi_id;
> +
> +	args.ops = enable ? VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2 :
> +			    VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2;
> +	args.in_args = (uint8_t *)&vlan_strip;
> +	args.in_args_size = sizeof(vlan_strip);
> +	args.out_buffer = msg_buf;
> +	args.out_size = IAVF_AQ_BUF_SZ;
> +	ret = iavf_execute_vf_cmd_safe(adapter, &args);
> +	if (ret)
> +		PMD_DRV_LOG(ERR, "fail to execute command %s",
> +			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2" :
> +				     "VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2");
> +
> +	return ret;
> +}
> +
>  int
>  iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
>  {
> @@ -838,14 +882,20 @@ iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
>  	uint8_t msg_buf[IAVF_AQ_BUF_SZ] = {0};
>  	struct iavf_cmd_info args;
>  	uint32_t *ethertype;
> +	int qinq = adapter->dev_data->dev_conf.rxmode.offloads &
> +		   RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
>  	int ret;
>  
>  	stripping_caps = &vf->vlan_v2_caps.offloads.stripping_support;
>  
> -	if ((stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_8100) &&
> +	/* When VLAN extend is disabled, Single VLAN mode which is Outer VLAN
> +	 * When VLAN extend is enabled, QinQ mode, this API works only on
> +	 * Inner VLAN strip which is always 0x8100.
> +	 */
> +	if (!qinq && (stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_8100) &&
>  	    (stripping_caps->outer & VIRTCHNL_VLAN_TOGGLE))
>  		ethertype = &vlan_strip.outer_ethertype_setting;
> -	else if ((stripping_caps->inner & VIRTCHNL_VLAN_ETHERTYPE_8100) &&
> +	else if (qinq && (stripping_caps->inner & VIRTCHNL_VLAN_ETHERTYPE_8100) &&
>  		 (stripping_caps->inner & VIRTCHNL_VLAN_TOGGLE))
>  		ethertype = &vlan_strip.inner_ethertype_setting;
>  	else
> @@ -921,6 +971,8 @@ iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
>  	struct virtchnl_vlan *vlan_setting;
>  	struct iavf_cmd_info args;
>  	uint32_t filtering_caps;
> +	int qinq = adapter->dev_data->dev_conf.rxmode.offloads &
> +		   RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
>  	int err;
>  
>  	supported_caps = &vf->vlan_v2_caps.filtering.filtering_support;
> @@ -938,7 +990,10 @@ iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
>  	memset(&vlan_filter, 0, sizeof(vlan_filter));
>  	vlan_filter.vport_id = vf->vsi_res->vsi_id;
>  	vlan_filter.num_elements = 1;
> -	vlan_setting->tpid = RTE_ETHER_TYPE_VLAN;
> +	if (qinq && vf->tpid ==