diff mbox series

[2/2] net/iavf: fix single VLAN insertion positioning

Message ID 20251031152250.2441980-3-ciara.loftus@intel.com (mailing list archive)
State Superseded
Delegated to: Bruce Richardson
Headers
Series Fixes for iavf VLAN insertion offload |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Functional success Functional Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-sample-apps-testing warning Testing issues
ci/github-robot: build success github build: passed
ci/github-robot-post success github post: success
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-amd64-testing warning Testing issues
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS

Commit Message

Ciara Loftus Oct. 31, 2025, 3:22 p.m. UTC 
Commit fdc37964c2bf ("net/iavf: support QinQ insertion offload in scalar
Tx") broke single VLAN insertion offload in cases where the v2 offload
capability and both inner and outer insertion were supported because it
caused inner VLAN tags to be inserted instead of outer.

When an iavf tx queue is being set up, if v2 offload capability is
supported, the driver queries the insertion capabilities and takes note
of where VLAN tags should be placed in the transmit and/or context
descriptors for insertion offload. In the offending commit, when both
inner and outer insertion was reported as supported, the flag
"vlan_flag" was changed to hold the location for inner VLAN tags.
However this caused inner VLAN tags to be inserted in the case of single
VLAN offload which is incorrect behaviour for this use case.

To fix this, revert the "vlan_flag" back to holding the location for
outer VLAN tags and update the datapath code accordingly.

Fixes: fdc37964c2bf ("net/iavf: support QinQ insertion offload in scalar Tx")

Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 drivers/net/intel/iavf/iavf_rxtx.c            | 50 +++++++------------
 drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 24 ++++-----
 drivers/net/intel/iavf/iavf_rxtx_vec_common.h |  5 +-
 3 files changed, 32 insertions(+), 47 deletions(-)

Comments

Bruce Richardson Nov. 4, 2025, 5:21 p.m. UTC | #1 
On Fri, Oct 31, 2025 at 03:22:50PM +0000, Ciara Loftus wrote:
> Commit fdc37964c2bf ("net/iavf: support QinQ insertion offload in scalar
> Tx") broke single VLAN insertion offload in cases where the v2 offload
> capability and both inner and outer insertion were supported because it
> caused inner VLAN tags to be inserted instead of outer.
> 
> When an iavf tx queue is being set up, if v2 offload capability is
> supported, the driver queries the insertion capabilities and takes note
> of where VLAN tags should be placed in the transmit and/or context
> descriptors for insertion offload. In the offending commit, when both
> inner and outer insertion was reported as supported, the flag
> "vlan_flag" was changed to hold the location for inner VLAN tags.
> However this caused inner VLAN tags to be inserted in the case of single
> VLAN offload which is incorrect behaviour for this use case.
> 
> To fix this, revert the "vlan_flag" back to holding the location for
> outer VLAN tags and update the datapath code accordingly.
> 
> Fixes: fdc37964c2bf ("net/iavf: support QinQ insertion offload in scalar Tx")
> 
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>

One suggestion inline below.

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


> ---
>  drivers/net/intel/iavf/iavf_rxtx.c            | 50 +++++++------------
>  drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 24 ++++-----
>  drivers/net/intel/iavf/iavf_rxtx_vec_common.h |  5 +-
>  3 files changed, 32 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c
> index a3ef13c791..66f718424a 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx.c
> @@ -799,32 +799,17 @@ iavf_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  			&adapter->vf.vlan_v2_caps.offloads.insertion_support;
>  		uint32_t insertion_cap;
>  
> -		if (insertion_support->outer == VIRTCHNL_VLAN_UNSUPPORTED ||
> -				insertion_support->inner == VIRTCHNL_VLAN_UNSUPPORTED) {
> -			/* Only one insertion is supported. */
> -			if (insertion_support->outer)
> -				insertion_cap = insertion_support->outer;
> -			else
> -				insertion_cap = insertion_support->inner;
> -
> -			if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> -				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> -				PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG1");
> -			} else if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2) {
> -				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> -				PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG2");
> -			}
> -		} else {
> -			 /* Both outer and inner insertion supported. */
> -			if (insertion_support->inner & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> -				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> -				PMD_INIT_LOG(DEBUG, "Inner VLAN insertion_cap: L2TAG1");
> -				PMD_INIT_LOG(DEBUG, "Outer VLAN insertion_cap: L2TAG2");
> -			} else {
> -				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> -				PMD_INIT_LOG(DEBUG, "Inner VLAN insertion_cap: L2TAG2");
> -				PMD_INIT_LOG(DEBUG, "Outer VLAN insertion_cap: L2TAG1");
> -			}
> +		if (insertion_support->outer)
> +			insertion_cap = insertion_support->outer;
> +		else
> +			insertion_cap = insertion_support->inner;
> +
> +		if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> +			txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> +			PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG1");
> +		} else if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2) {
> +			txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> +			PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG2");
>  		}
>  	} else {
>  		txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> @@ -2600,12 +2585,11 @@ iavf_fill_context_desc(volatile struct iavf_tx_context_desc *desc,
>  	desc_qws->qw0 = rte_cpu_to_le_64(desc_qws->qw0);
>  	desc_qws->qw1 = rte_cpu_to_le_64(desc_qws->qw1);
>  
> +	/* vlan_flag specifies VLAN tag location for VLAN, and outer tag location for QinQ. */
>  	if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)
> +		desc->l2tag2 = m->ol_flags & RTE_MBUF_F_TX_QINQ ? m->vlan_tci_outer : m->vlan_tci;
> +	else if (m->ol_flags & RTE_MBUF_F_TX_QINQ)
>  		desc->l2tag2 = m->vlan_tci;

Minor issue, but the expression of the logic here is different to how its
worked in the previous patch code. There we check the QINQ or VLAN tag
first, and then have a condition based on the L2TAG* flag. Here we check
the L2TAG? first then switch on the QINQ flag. Can you maybe rework to have
all conditional checks for this consistent?

> -
> -	if (m->ol_flags & RTE_MBUF_F_TX_QINQ)
> -		desc->l2tag2 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? m->vlan_tci :
> -										m->vlan_tci_outer;
>  }
>  
>  
> @@ -2660,11 +2644,11 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
>  		l2tag1 |= m->vlan_tci;
>  	}
>  
> -	/* Descriptor based QinQ insertion */
> +	/* Descriptor based QinQ insertion. vlan_flag specifies outer tag location. */
>  	if (m->ol_flags & RTE_MBUF_F_TX_QINQ) {
>  		command |= (uint64_t)IAVF_TX_DESC_CMD_IL2TAG1;
> -		l2tag1 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1 ? m->vlan_tci :
> -									m->vlan_tci_outer;
> +		l2tag1 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1 ? m->vlan_tci_outer :
> +									m->vlan_tci;
>  	}
>  
>  	if ((m->ol_flags &
> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> index c800ae29e1..6f150cb1c1 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> @@ -2136,17 +2136,17 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
>  		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) {
> -				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
> -				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> -							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> -				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
> -							IAVF_TXD_QW1_L2TAG1_SHIFT;
> -			} else {
>  				/* Outer tag at L2TAG2, inner tag at L2TAG1. */
>  				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
>  							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
>  				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
>  							IAVF_TXD_QW1_L2TAG1_SHIFT;
> +			} else {
> +				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
> +				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> +							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> +				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
> +							IAVF_TXD_QW1_L2TAG1_SHIFT;
>  			}
>  		} else if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN) {
>  			if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> @@ -2166,17 +2166,17 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
>  		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) {
> -				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
> -				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> -							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> -				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
> -							IAVF_TXD_QW1_L2TAG1_SHIFT;
> -			} else {
>  				/* Outer tag at L2TAG2, inner tag at L2TAG1. */
>  				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
>  							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
>  				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
>  							IAVF_TXD_QW1_L2TAG1_SHIFT;
> +			} else {
> +				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
> +				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> +							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> +				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
> +							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) {
> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> index bf8faf3632..86523a7d2b 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> @@ -227,11 +227,12 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt,
>  #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD
>  	if (ol_flags & RTE_MBUF_F_TX_QINQ) {
>  		td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
> +		/* vlan_flag specifies outer tag location for QinQ. */
>  		if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1)
> -			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> +			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
>  					IAVF_TXD_QW1_L2TAG1_SHIFT);
>  		else
> -			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
> +			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
>  					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;
> -- 
> 2.34.1
>
Bruce Richardson Nov. 4, 2025, 5:28 p.m. UTC | #2 
On Tue, Nov 04, 2025 at 05:21:10PM +0000, Bruce Richardson wrote:
> On Fri, Oct 31, 2025 at 03:22:50PM +0000, Ciara Loftus wrote:
> > Commit fdc37964c2bf ("net/iavf: support QinQ insertion offload in scalar
> > Tx") broke single VLAN insertion offload in cases where the v2 offload
> > capability and both inner and outer insertion were supported because it
> > caused inner VLAN tags to be inserted instead of outer.
> > 
> > When an iavf tx queue is being set up, if v2 offload capability is
> > supported, the driver queries the insertion capabilities and takes note
> > of where VLAN tags should be placed in the transmit and/or context
> > descriptors for insertion offload. In the offending commit, when both
> > inner and outer insertion was reported as supported, the flag
> > "vlan_flag" was changed to hold the location for inner VLAN tags.
> > However this caused inner VLAN tags to be inserted in the case of single
> > VLAN offload which is incorrect behaviour for this use case.
> > 
> > To fix this, revert the "vlan_flag" back to holding the location for
> > outer VLAN tags and update the datapath code accordingly.
> > 
> > Fixes: fdc37964c2bf ("net/iavf: support QinQ insertion offload in scalar Tx")
> > 
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> 
> One suggestion inline below.
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> 
> > ---
> >  drivers/net/intel/iavf/iavf_rxtx.c            | 50 +++++++------------
> >  drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 24 ++++-----
> >  drivers/net/intel/iavf/iavf_rxtx_vec_common.h |  5 +-
> >  3 files changed, 32 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c
> > index a3ef13c791..66f718424a 100644
> > --- a/drivers/net/intel/iavf/iavf_rxtx.c
> > +++ b/drivers/net/intel/iavf/iavf_rxtx.c
> > @@ -799,32 +799,17 @@ iavf_dev_tx_queue_setup(struct rte_eth_dev *dev,
> >  			&adapter->vf.vlan_v2_caps.offloads.insertion_support;
> >  		uint32_t insertion_cap;
> >  
> > -		if (insertion_support->outer == VIRTCHNL_VLAN_UNSUPPORTED ||
> > -				insertion_support->inner == VIRTCHNL_VLAN_UNSUPPORTED) {
> > -			/* Only one insertion is supported. */
> > -			if (insertion_support->outer)
> > -				insertion_cap = insertion_support->outer;
> > -			else
> > -				insertion_cap = insertion_support->inner;
> > -
> > -			if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> > -				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > -				PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG1");
> > -			} else if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2) {
> > -				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> > -				PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG2");
> > -			}
> > -		} else {
> > -			 /* Both outer and inner insertion supported. */
> > -			if (insertion_support->inner & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> > -				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > -				PMD_INIT_LOG(DEBUG, "Inner VLAN insertion_cap: L2TAG1");
> > -				PMD_INIT_LOG(DEBUG, "Outer VLAN insertion_cap: L2TAG2");
> > -			} else {
> > -				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> > -				PMD_INIT_LOG(DEBUG, "Inner VLAN insertion_cap: L2TAG2");
> > -				PMD_INIT_LOG(DEBUG, "Outer VLAN insertion_cap: L2TAG1");
> > -			}
> > +		if (insertion_support->outer)
> > +			insertion_cap = insertion_support->outer;
> > +		else
> > +			insertion_cap = insertion_support->inner;
> > +
> > +		if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> > +			txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > +			PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG1");
> > +		} else if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2) {
> > +			txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> > +			PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG2");
> >  		}
> >  	} else {
> >  		txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > @@ -2600,12 +2585,11 @@ iavf_fill_context_desc(volatile struct iavf_tx_context_desc *desc,
> >  	desc_qws->qw0 = rte_cpu_to_le_64(desc_qws->qw0);
> >  	desc_qws->qw1 = rte_cpu_to_le_64(desc_qws->qw1);
> >  
> > +	/* vlan_flag specifies VLAN tag location for VLAN, and outer tag location for QinQ. */
> >  	if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)
> > +		desc->l2tag2 = m->ol_flags & RTE_MBUF_F_TX_QINQ ? m->vlan_tci_outer : m->vlan_tci;
> > +	else if (m->ol_flags & RTE_MBUF_F_TX_QINQ)
> >  		desc->l2tag2 = m->vlan_tci;
> 
> Minor issue, but the expression of the logic here is different to how its
> worked in the previous patch code. There we check the QINQ or VLAN tag
> first, and then have a condition based on the L2TAG* flag. Here we check
> the L2TAG? first then switch on the QINQ flag. Can you maybe rework to have
> all conditional checks for this consistent?
> 
> > -
> > -	if (m->ol_flags & RTE_MBUF_F_TX_QINQ)
> > -		desc->l2tag2 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? m->vlan_tci :
> > -										m->vlan_tci_outer;
> >  }
> >  
> >  
> > @@ -2660,11 +2644,11 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
> >  		l2tag1 |= m->vlan_tci;
> >  	}
> >  
> > -	/* Descriptor based QinQ insertion */
> > +	/* Descriptor based QinQ insertion. vlan_flag specifies outer tag location. */
> >  	if (m->ol_flags & RTE_MBUF_F_TX_QINQ) {
> >  		command |= (uint64_t)IAVF_TX_DESC_CMD_IL2TAG1;
> > -		l2tag1 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1 ? m->vlan_tci :
> > -									m->vlan_tci_outer;
> > +		l2tag1 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1 ? m->vlan_tci_outer :
> > +									m->vlan_tci;
> >  	}
> >  
> >  	if ((m->ol_flags &
> > diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> > index c800ae29e1..6f150cb1c1 100644
> > --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> > +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> > @@ -2136,17 +2136,17 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
> >  		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) {
> > -				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
> > -				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> > -							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > -				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
> > -							IAVF_TXD_QW1_L2TAG1_SHIFT;
> > -			} else {
> >  				/* Outer tag at L2TAG2, inner tag at L2TAG1. */
> >  				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
> >  							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> >  				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> >  							IAVF_TXD_QW1_L2TAG1_SHIFT;
> > +			} else {
> > +				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
> > +				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> > +							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > +				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
> > +							IAVF_TXD_QW1_L2TAG1_SHIFT;
> >  			}
> >  		} else if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> >  			if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> > @@ -2166,17 +2166,17 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
> >  		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) {
> > -				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
> > -				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> > -							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > -				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
> > -							IAVF_TXD_QW1_L2TAG1_SHIFT;
> > -			} else {
> >  				/* Outer tag at L2TAG2, inner tag at L2TAG1. */
> >  				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
> >  							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> >  				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> >  							IAVF_TXD_QW1_L2TAG1_SHIFT;
> > +			} else {
> > +				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
> > +				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> > +							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > +				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
> > +							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) {

On first review, I missed the fact that you were also changing the logic in
the AVX512 code paths too. Since I don't see the update to the ctx_vtx1
function I assume that the previous patch updated that correctly the first
time, and that this patch corrects the ctx_vtx function only. Is that
correct?

In that case, I'd suggest that this patch should come first (or else be
split and put this AVX-512 bit first), so that we correct the ctx_vtx bulk
before adding the new and correct ctx_vtx1 implementation. As it now
stands, after patch 1 we have the two different ctx_vtx functions in the AVX512
path behaving in reversed fashion, which is rather strange.

/Bruce

> > diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> > index bf8faf3632..86523a7d2b 100644
> > --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> > +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> > @@ -227,11 +227,12 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt,
> >  #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD
> >  	if (ol_flags & RTE_MBUF_F_TX_QINQ) {
> >  		td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
> > +		/* vlan_flag specifies outer tag location for QinQ. */
> >  		if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1)
> > -			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> > +			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
> >  					IAVF_TXD_QW1_L2TAG1_SHIFT);
> >  		else
> > -			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
> > +			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> >  					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;
> > -- 
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c
index a3ef13c791..66f718424a 100644
--- a/drivers/net/intel/iavf/iavf_rxtx.c
+++ b/drivers/net/intel/iavf/iavf_rxtx.c
@@ -799,32 +799,17 @@  iavf_dev_tx_queue_setup(struct rte_eth_dev *dev,
 			&adapter->vf.vlan_v2_caps.offloads.insertion_support;
 		uint32_t insertion_cap;
 
-		if (insertion_support->outer == VIRTCHNL_VLAN_UNSUPPORTED ||
-				insertion_support->inner == VIRTCHNL_VLAN_UNSUPPORTED) {
-			/* Only one insertion is supported. */
-			if (insertion_support->outer)
-				insertion_cap = insertion_support->outer;
-			else
-				insertion_cap = insertion_support->inner;
-
-			if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
-				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
-				PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG1");
-			} else if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2) {
-				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
-				PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG2");
-			}
-		} else {
-			 /* Both outer and inner insertion supported. */
-			if (insertion_support->inner & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
-				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
-				PMD_INIT_LOG(DEBUG, "Inner VLAN insertion_cap: L2TAG1");
-				PMD_INIT_LOG(DEBUG, "Outer VLAN insertion_cap: L2TAG2");
-			} else {
-				txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
-				PMD_INIT_LOG(DEBUG, "Inner VLAN insertion_cap: L2TAG2");
-				PMD_INIT_LOG(DEBUG, "Outer VLAN insertion_cap: L2TAG1");
-			}
+		if (insertion_support->outer)
+			insertion_cap = insertion_support->outer;
+		else
+			insertion_cap = insertion_support->inner;
+
+		if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
+			txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
+			PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG1");
+		} else if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2) {
+			txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
+			PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG2");
 		}
 	} else {
 		txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
@@ -2600,12 +2585,11 @@  iavf_fill_context_desc(volatile struct iavf_tx_context_desc *desc,
 	desc_qws->qw0 = rte_cpu_to_le_64(desc_qws->qw0);
 	desc_qws->qw1 = rte_cpu_to_le_64(desc_qws->qw1);
 
+	/* vlan_flag specifies VLAN tag location for VLAN, and outer tag location for QinQ. */
 	if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)
+		desc->l2tag2 = m->ol_flags & RTE_MBUF_F_TX_QINQ ? m->vlan_tci_outer : m->vlan_tci;
+	else if (m->ol_flags & RTE_MBUF_F_TX_QINQ)
 		desc->l2tag2 = m->vlan_tci;
-
-	if (m->ol_flags & RTE_MBUF_F_TX_QINQ)
-		desc->l2tag2 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? m->vlan_tci :
-										m->vlan_tci_outer;
 }
 
 
@@ -2660,11 +2644,11 @@  iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
 		l2tag1 |= m->vlan_tci;
 	}
 
-	/* Descriptor based QinQ insertion */
+	/* Descriptor based QinQ insertion. vlan_flag specifies outer tag location. */
 	if (m->ol_flags & RTE_MBUF_F_TX_QINQ) {
 		command |= (uint64_t)IAVF_TX_DESC_CMD_IL2TAG1;
-		l2tag1 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1 ? m->vlan_tci :
-									m->vlan_tci_outer;
+		l2tag1 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1 ? m->vlan_tci_outer :
+									m->vlan_tci;
 	}
 
 	if ((m->ol_flags &
diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
index c800ae29e1..6f150cb1c1 100644
--- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
+++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
@@ -2136,17 +2136,17 @@  ctx_vtx(volatile struct iavf_tx_desc *txdp,
 		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) {
-				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
-				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
-							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
-				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
-							IAVF_TXD_QW1_L2TAG1_SHIFT;
-			} else {
 				/* Outer tag at L2TAG2, inner tag at L2TAG1. */
 				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
 							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
 				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
 							IAVF_TXD_QW1_L2TAG1_SHIFT;
+			} else {
+				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
+				low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
+							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
+				hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
+							IAVF_TXD_QW1_L2TAG1_SHIFT;
 			}
 		} else if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN) {
 			if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
@@ -2166,17 +2166,17 @@  ctx_vtx(volatile struct iavf_tx_desc *txdp,
 		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) {
-				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
-				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
-							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
-				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
-							IAVF_TXD_QW1_L2TAG1_SHIFT;
-			} else {
 				/* Outer tag at L2TAG2, inner tag at L2TAG1. */
 				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
 							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
 				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
 							IAVF_TXD_QW1_L2TAG1_SHIFT;
+			} else {
+				/* Inner tag at L2TAG2, outer tag at L2TAG1. */
+				low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
+							IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
+				hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
+							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) {
diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
index bf8faf3632..86523a7d2b 100644
--- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
+++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
@@ -227,11 +227,12 @@  iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt,
 #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD
 	if (ol_flags & RTE_MBUF_F_TX_QINQ) {
 		td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
+		/* vlan_flag specifies outer tag location for QinQ. */
 		if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1)
-			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
+			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
 					IAVF_TXD_QW1_L2TAG1_SHIFT);
 		else
-			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
+			*txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
 					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;