[PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion

[Bruce Richardson]

On Fri, Oct 31, 2025 at 03:22:49PM +0000, Ciara Loftus wrote:
> QinQ insertion was enabled in the bulk transmit function but not the
> single packet transmit function. Implement it in the single packet
> function.
> 
> Also, fix an issue that would arise in the event an mbuf had both the
> VLAN and QINQ offload flags set. In this case the L2TAG2 field would be
> written to twice and could cause the tag to be corrupted. Reorder the
> logic of populating the L2TAG2 field and ensure that the field is only
> written to once.
> 

While both these described changes fix the same commit, can you please
split this patch into two, because it fixes two separate issues with that
code.

Just one review comment inline below. You can keep my ack on any new
versions.

Acked-by: Bruce Richardson <bruce.richardson at intel.com>


> Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx")
> 
> Signed-off-by: Ciara Loftus <ciara.loftus at intel.com>
> ---
>  drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 50 ++++++++++---------
>  drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 10 ++--
>  2 files changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> index d40a858413..c800ae29e1 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> @@ -2077,7 +2077,13 @@ ctx_vtx1(volatile struct iavf_tx_desc *txdp, struct rte_mbuf *pkt,
>  	if (((pkt->ol_flags & RTE_MBUF_F_TX_VLAN) || offload)) {
>  		if (offload)
>  			iavf_fill_ctx_desc_tunneling_avx512(&low_ctx_qw, pkt);
> -		if ((pkt->ol_flags & RTE_MBUF_F_TX_VLAN) ||
> +		if (pkt->ol_flags & RTE_MBUF_F_TX_QINQ) {
> +			uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ?
> +				(uint64_t)pkt->vlan_tci_outer :
> +				(uint64_t)pkt->vlan_tci;
> +			high_ctx_qw |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> +			low_ctx_qw |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> +		} else if ((pkt->ol_flags & RTE_MBUF_F_TX_VLAN) &&
>  				(vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)) {
>  			high_ctx_qw |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
>  			low_ctx_qw |= (uint64_t)pkt->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;

We are missing handling here for the case of vlan_flag set and L2TAG2 flag
not set. However, that is handled by iavf_txd_enable_offload - but only if
IAVF_TX_VLAN_QINQ_OFFLOAD is set, which it is by default as far as I can see.
[Maybe a future patch we can remove that define???]. If it's not set, do we
need to handle the vlan case here? There also seems to be a little
duplication between the code here and in that function.

> @@ -2127,17 +2133,6 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
>  				((uint64_t)pkt[0]->data_len <<
>  					IAVF_TXD_QW1_TX_BUF_SZ_SHIFT);
>  
> -		if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> -			if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> -				hi_ctx_qw1 |=
> -					IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> -				low_ctx_qw1 |=
> -					(uint64_t)pkt[1]->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> -			} else {
> -				hi_data_qw1 |=
> -					(uint64_t)pkt[1]->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT;
> -			}
> -		}
>  		if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) {
>  			hi_ctx_qw1 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
>  			if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> @@ -2153,22 +2148,21 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
>  				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
>  							IAVF_TXD_QW1_L2TAG1_SHIFT;
>  			}
> -		}
> -		if (IAVF_CHECK_TX_LLDP(pkt[1]))
> -			hi_ctx_qw1 |= IAVF_TX_CTX_DESC_SWTCH_UPLINK
> -				<< IAVF_TXD_CTX_QW1_CMD_SHIFT;
> -
> -		if (pkt[0]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> +		} else if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN) {
>  			if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> -				hi_ctx_qw0 |=
> +				hi_ctx_qw1 |=
>  					IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> -				low_ctx_qw0 |=
> -					(uint64_t)pkt[0]->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> +				low_ctx_qw1 |=
> +					(uint64_t)pkt[1]->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
>  			} else {
> -				hi_data_qw0 |=
> -					(uint64_t)pkt[0]->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT;
> +				hi_data_qw1 |=
> +					(uint64_t)pkt[1]->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT;
>  			}
>  		}
> +		if (IAVF_CHECK_TX_LLDP(pkt[1]))
> +			hi_ctx_qw1 |= IAVF_TX_CTX_DESC_SWTCH_UPLINK
> +				<< IAVF_TXD_CTX_QW1_CMD_SHIFT;
> +
>  		if (pkt[0]->ol_flags & RTE_MBUF_F_TX_QINQ) {
>  			hi_ctx_qw0 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
>  			if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> @@ -2184,6 +2178,16 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
>  				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
>  							IAVF_TXD_QW1_L2TAG1_SHIFT;
>  			}
> +		} else if (pkt[0]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> +			if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> +				hi_ctx_qw0 |=
> +					IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> +				low_ctx_qw0 |=
> +					(uint64_t)pkt[0]->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> +			} else {
> +				hi_data_qw0 |=
> +					(uint64_t)pkt[0]->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT;
> +			}
>  		}
>  		if (IAVF_CHECK_TX_LLDP(pkt[0]))
>  			hi_ctx_qw0 |= IAVF_TX_CTX_DESC_SWTCH_UPLINK
> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> index f513777663..bf8faf3632 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> @@ -225,12 +225,6 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt,
>  #endif
>  
>  #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD
> -	if (ol_flags & RTE_MBUF_F_TX_VLAN && vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1) {
> -		td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
> -		*txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> -			    IAVF_TXD_QW1_L2TAG1_SHIFT);
> -	}
> -
>  	if (ol_flags & RTE_MBUF_F_TX_QINQ) {
>  		td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
>  		if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1)
> @@ -239,6 +233,10 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt,
>  		else
>  			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
>  					IAVF_TXD_QW1_L2TAG1_SHIFT);
> +	} else if (ol_flags & RTE_MBUF_F_TX_VLAN && vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1) {
> +		td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
> +		*txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> +			    IAVF_TXD_QW1_L2TAG1_SHIFT);
>  	}
>  #endif
>  
> -- 
> 2.34.1
>