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

[Bruce Richardson]

On Tue, Nov 04, 2025 at 05:13:36PM +0000, Bruce Richardson wrote:
> 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);

Very minor nit - this line doesn't need to be wrapped, it fits in 100
chars.

> >  	}
> >  #endif
> >  
> > -- 
> > 2.34.1
> >