* [PATCH 0/2] Fixes for iavf VLAN insertion offload
@ 2025-10-31 15:22 Ciara Loftus
2025-10-31 15:22 ` [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion Ciara Loftus
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Ciara Loftus @ 2025-10-31 15:22 UTC (permalink / raw)
To: dev; +Cc: Ciara Loftus
The first patch fixes double VLAN offload in the AVX-512 path.
The second fixes single VLAN insertion offload across all paths.
Ciara Loftus (2):
net/iavf: fix AVX-512 double VLAN (QinQ) insertion
net/iavf: fix single VLAN insertion positioning
drivers/net/intel/iavf/iavf_rxtx.c | 50 +++++---------
drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 68 ++++++++++---------
drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 15 ++--
3 files changed, 60 insertions(+), 73 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion 2025-10-31 15:22 [PATCH 0/2] Fixes for iavf VLAN insertion offload Ciara Loftus @ 2025-10-31 15:22 ` Ciara Loftus 2025-11-04 17:13 ` Bruce Richardson 2025-10-31 15:22 ` [PATCH 2/2] net/iavf: fix single VLAN insertion positioning Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus 2 siblings, 1 reply; 15+ messages in thread From: Ciara Loftus @ 2025-10-31 15:22 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus 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. Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") Signed-off-by: Ciara Loftus <ciara.loftus@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; @@ -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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion 2025-10-31 15:22 ` [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion Ciara Loftus @ 2025-11-04 17:13 ` Bruce Richardson 2025-11-04 17:16 ` Bruce Richardson 0 siblings, 1 reply; 15+ messages in thread From: Bruce Richardson @ 2025-11-04 17:13 UTC (permalink / raw) To: Ciara Loftus; +Cc: dev 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@intel.com> > Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") > > Signed-off-by: Ciara Loftus <ciara.loftus@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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion 2025-11-04 17:13 ` Bruce Richardson @ 2025-11-04 17:16 ` Bruce Richardson 0 siblings, 0 replies; 15+ messages in thread From: Bruce Richardson @ 2025-11-04 17:16 UTC (permalink / raw) To: Ciara Loftus; +Cc: dev 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@intel.com> > > > > Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") > > > > Signed-off-by: Ciara Loftus <ciara.loftus@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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] net/iavf: fix single VLAN insertion positioning 2025-10-31 15:22 [PATCH 0/2] Fixes for iavf VLAN insertion offload Ciara Loftus 2025-10-31 15:22 ` [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion Ciara Loftus @ 2025-10-31 15:22 ` Ciara Loftus 2025-11-04 17:21 ` Bruce Richardson 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus 2 siblings, 1 reply; 15+ messages in thread From: Ciara Loftus @ 2025-10-31 15:22 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus 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(-) 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; -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] net/iavf: fix single VLAN insertion positioning 2025-10-31 15:22 ` [PATCH 2/2] net/iavf: fix single VLAN insertion positioning Ciara Loftus @ 2025-11-04 17:21 ` Bruce Richardson 2025-11-04 17:28 ` Bruce Richardson 0 siblings, 1 reply; 15+ messages in thread From: Bruce Richardson @ 2025-11-04 17:21 UTC (permalink / raw) To: Ciara Loftus; +Cc: dev 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] net/iavf: fix single VLAN insertion positioning 2025-11-04 17:21 ` Bruce Richardson @ 2025-11-04 17:28 ` Bruce Richardson 0 siblings, 0 replies; 15+ messages in thread From: Bruce Richardson @ 2025-11-04 17:28 UTC (permalink / raw) To: Ciara Loftus; +Cc: dev 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 0/6] Fixes for iavf VLAN insertion offload 2025-10-31 15:22 [PATCH 0/2] Fixes for iavf VLAN insertion offload Ciara Loftus 2025-10-31 15:22 ` [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion Ciara Loftus 2025-10-31 15:22 ` [PATCH 2/2] net/iavf: fix single VLAN insertion positioning Ciara Loftus @ 2025-11-05 15:26 ` Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 1/6] net/iavf: fix compilation with VLAN insertion disabled Ciara Loftus ` (5 more replies) 2 siblings, 6 replies; 15+ messages in thread From: Ciara Loftus @ 2025-11-05 15:26 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus A series of fixes for VLAN insertion offload in the iavf PMD. v2: * Split AVX512 QinQ fixes into two separate patches (5 and 6) * Remove duplication in AVX-512 VLAN insert logic * Ensure correct behaviour when IAVF_TX_VLAN_QINQ_OFFLOAD is set/unset * Consistent checking of mbuf offload flags before vlan location flags * Reorder, put fix for existing AVX-512 bulk logic before introducing single tx logic for QinQ Ciara Loftus (6): net/iavf: fix compilation with VLAN insertion disabled net/iavf: remove duplication in AVX-512 VLAN insert logic net/iavf: ensure correct conditions for AVX-512 VLAN offload net/iavf: fix single VLAN insertion positioning net/iavf: fix QinQ insertion for single packet Tx net/iavf: fix QinQ insertion with mbuf flags VLAN and QINQ drivers/net/intel/iavf/iavf_rxtx.c | 53 +++++------- drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 83 ++++++++----------- drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 21 +++-- 3 files changed, 67 insertions(+), 90 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/6] net/iavf: fix compilation with VLAN insertion disabled 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus @ 2025-11-05 15:26 ` Ciara Loftus 2025-11-05 19:31 ` Stephen Hemminger 2025-11-05 15:26 ` [PATCH v2 2/6] net/iavf: remove duplication in AVX-512 VLAN insert logic Ciara Loftus ` (4 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Ciara Loftus @ 2025-11-05 15:26 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus If IAVF_TX_VLAN_QINQ_OFFLOAD is undefined there will be an unused variable warning. Fix this. Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> --- drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h index f513777663..5ac96f239b 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h @@ -164,7 +164,12 @@ iavf_tx_vec_dev_check_default(struct rte_eth_dev *dev) static __rte_always_inline void iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt, - uint64_t *txd_hi, uint8_t vlan_flag) + uint64_t *txd_hi, +#ifdef IAVF_TX_VLAN_QINQ_OFFLOAD + uint8_t vlan_flag) +#else + __rte_unused uint8_t vlan_flag) +#endif { #if defined(IAVF_TX_CSUM_OFFLOAD) || defined(IAVF_TX_VLAN_QINQ_OFFLOAD) uint64_t ol_flags = tx_pkt->ol_flags; -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/6] net/iavf: fix compilation with VLAN insertion disabled 2025-11-05 15:26 ` [PATCH v2 1/6] net/iavf: fix compilation with VLAN insertion disabled Ciara Loftus @ 2025-11-05 19:31 ` Stephen Hemminger 0 siblings, 0 replies; 15+ messages in thread From: Stephen Hemminger @ 2025-11-05 19:31 UTC (permalink / raw) To: Ciara Loftus; +Cc: dev On Wed, 5 Nov 2025 15:26:37 +0000 Ciara Loftus <ciara.loftus@intel.com> wrote: > static __rte_always_inline void > iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt, > - uint64_t *txd_hi, uint8_t vlan_flag) > + uint64_t *txd_hi, > +#ifdef IAVF_TX_VLAN_QINQ_OFFLOAD > + uint8_t vlan_flag) > +#else > + __rte_unused uint8_t vlan_flag) > +#endif This is not the best way to do this. Adding #ifdef there can confuse static parsers etc. You can use RTE_SET_USED() macro instead. The ifdef could be rearranged for clarity as well. diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h index f513777663..dbe7d2b2da 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h @@ -166,15 +166,14 @@ static __rte_always_inline void iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt, uint64_t *txd_hi, uint8_t vlan_flag) { + uint32_t td_cmd = 0; #if defined(IAVF_TX_CSUM_OFFLOAD) || defined(IAVF_TX_VLAN_QINQ_OFFLOAD) uint64_t ol_flags = tx_pkt->ol_flags; #endif - uint32_t td_cmd = 0; + #ifdef IAVF_TX_CSUM_OFFLOAD uint32_t td_offset = 0; -#endif -#ifdef IAVF_TX_CSUM_OFFLOAD /* Set MACLEN */ if (ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK) td_offset |= (tx_pkt->outer_l2_len >> 1) @@ -240,6 +239,8 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt, *txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer << IAVF_TXD_QW1_L2TAG1_SHIFT); } +#else + RTE_SET_USED(vlan_flag); #endif *txd_hi |= ((uint64_t)td_cmd) << IAVF_TXD_QW1_CMD_SHIFT; ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/6] net/iavf: remove duplication in AVX-512 VLAN insert logic 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 1/6] net/iavf: fix compilation with VLAN insertion disabled Ciara Loftus @ 2025-11-05 15:26 ` Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 3/6] net/iavf: ensure correct conditions for AVX-512 VLAN offload Ciara Loftus ` (3 subsequent siblings) 5 siblings, 0 replies; 15+ messages in thread From: Ciara Loftus @ 2025-11-05 15:26 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus, stable The part of the transmit descriptor that holds the L2TAG1 VLAN tag was potentially being written to twice on the AVX-512 context path. Fix this by removing one of the writes. Fixes: 4f8259df563a ("net/iavf: enable Tx outer checksum offload on AVX512") Cc: stable@dpdk.org Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> --- drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 66 ++++++------------- 1 file changed, 20 insertions(+), 46 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..28d83ca3ed 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c @@ -2127,63 +2127,37 @@ 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_VLAN && + 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; } if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) { + uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? + (uint64_t)pkt[0]->vlan_tci_outer : + (uint64_t)pkt[0]->vlan_tci; 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; - } + low_ctx_qw1 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; } 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) { - 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 (pkt[0]->ol_flags & RTE_MBUF_F_TX_VLAN && + 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; } if (pkt[0]->ol_flags & RTE_MBUF_F_TX_QINQ) { + uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? + (uint64_t)pkt[0]->vlan_tci_outer : + (uint64_t)pkt[0]->vlan_tci; 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; - } + low_ctx_qw0 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; } if (IAVF_CHECK_TX_LLDP(pkt[0])) hi_ctx_qw0 |= IAVF_TX_CTX_DESC_SWTCH_UPLINK -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/6] net/iavf: ensure correct conditions for AVX-512 VLAN offload 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 1/6] net/iavf: fix compilation with VLAN insertion disabled Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 2/6] net/iavf: remove duplication in AVX-512 VLAN insert logic Ciara Loftus @ 2025-11-05 15:26 ` Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 4/6] net/iavf: fix single VLAN insertion positioning Ciara Loftus ` (2 subsequent siblings) 5 siblings, 0 replies; 15+ messages in thread From: Ciara Loftus @ 2025-11-05 15:26 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus Commit 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") re-enabled VLAN insertion on the AVX-512 path after it was disabled in commit efe1b63775e8 ("net/iavf: fix VLAN insertion in vector path"). The initial implementation introduced in commit 4f8259df563a ("net/iavf: enable Tx outer checksum offload on AVX512") was inconsistent in that if the vlan tag was to be placed in the L2TAG1 field, the offload was only performed if IAVF_TX_VLAN_QINQ_OFFLOAD was defined and if the path was an offload path. However if the vlan tag was to be placed in the L2TAG2 field (requiring a context descriptor), the insert was performed unconditionally. When VLAN insertion was re-enabled, these inconsistencies remained. This commit fixes these and ensures the following two conditions are met before VLAN insert is offloaded on the AVX-512 path: 1. IAVF_TX_VLAN_QINQ_OFFLOAD is defined (defined by default) 2. The path is an "offload" path Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> --- drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 79 +++++++++++-------- 1 file changed, 46 insertions(+), 33 deletions(-) diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c index 28d83ca3ed..7eb7e47390 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c @@ -2074,14 +2074,17 @@ ctx_vtx1(volatile struct iavf_tx_desc *txdp, struct rte_mbuf *pkt, uint64_t high_ctx_qw = IAVF_TX_DESC_DTYPE_CONTEXT; uint64_t low_ctx_qw = 0; - 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) || - (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; + if (offload) { + iavf_fill_ctx_desc_tunneling_avx512(&low_ctx_qw, pkt); +#ifdef IAVF_TX_VLAN_QINQ_OFFLOAD + 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; } +#endif } if (IAVF_CHECK_TX_LLDP(pkt)) high_ctx_qw |= IAVF_TX_CTX_DESC_SWTCH_UPLINK @@ -2127,38 +2130,48 @@ 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 && - 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; - } - if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) { - uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? - (uint64_t)pkt[0]->vlan_tci_outer : - (uint64_t)pkt[0]->vlan_tci; - hi_ctx_qw1 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT; - low_ctx_qw1 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; +#ifdef IAVF_TX_VLAN_QINQ_OFFLOAD + if (offload) { + if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN && + 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; + } + if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) { + uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? + (uint64_t)pkt[1]->vlan_tci_outer : + (uint64_t)pkt[1]->vlan_tci; + hi_ctx_qw1 |= IAVF_TX_CTX_DESC_IL2TAG2 << + IAVF_TXD_CTX_QW1_CMD_SHIFT; + low_ctx_qw1 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; + } } +#endif 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 && - 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; - } - if (pkt[0]->ol_flags & RTE_MBUF_F_TX_QINQ) { - uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? - (uint64_t)pkt[0]->vlan_tci_outer : - (uint64_t)pkt[0]->vlan_tci; - hi_ctx_qw0 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT; - low_ctx_qw0 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; +#ifdef IAVF_TX_VLAN_QINQ_OFFLOAD + if (offload) { + if (pkt[0]->ol_flags & RTE_MBUF_F_TX_VLAN && + 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; + } + if (pkt[0]->ol_flags & RTE_MBUF_F_TX_QINQ) { + uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? + (uint64_t)pkt[0]->vlan_tci_outer : + (uint64_t)pkt[0]->vlan_tci; + hi_ctx_qw0 |= IAVF_TX_CTX_DESC_IL2TAG2 << + IAVF_TXD_CTX_QW1_CMD_SHIFT; + low_ctx_qw0 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; + } } +#endif if (IAVF_CHECK_TX_LLDP(pkt[0])) hi_ctx_qw0 |= IAVF_TX_CTX_DESC_SWTCH_UPLINK << IAVF_TXD_CTX_QW1_CMD_SHIFT; -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/6] net/iavf: fix single VLAN insertion positioning 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus ` (2 preceding siblings ...) 2025-11-05 15:26 ` [PATCH v2 3/6] net/iavf: ensure correct conditions for AVX-512 VLAN offload Ciara Loftus @ 2025-11-05 15:26 ` Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 5/6] net/iavf: fix QinQ insertion for single packet Tx Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 6/6] net/iavf: fix QinQ insertion with mbuf flags VLAN and QINQ Ciara Loftus 5 siblings, 0 replies; 15+ messages in thread From: Ciara Loftus @ 2025-11-05 15:26 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus 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 | 53 +++++++------------ drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 8 +-- drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 5 +- 3 files changed, 26 insertions(+), 40 deletions(-) diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c index a3ef13c791..ea49059f83 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,12 @@ 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); - if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) - desc->l2tag2 = m->vlan_tci; - + /* vlan_flag specifies VLAN tag location for VLAN, and outer tag location for QinQ. */ 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; + desc->l2tag2 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? m->vlan_tci_outer : + m->vlan_tci; + else if (m->ol_flags & RTE_MBUF_F_TX_VLAN && vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) + desc->l2tag2 = m->vlan_tci; } @@ -2660,11 +2645,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 7eb7e47390..6182988612 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c @@ -2141,8 +2141,8 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp, } if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) { uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? - (uint64_t)pkt[1]->vlan_tci_outer : - (uint64_t)pkt[1]->vlan_tci; + (uint64_t)pkt[1]->vlan_tci : + (uint64_t)pkt[1]->vlan_tci_outer; hi_ctx_qw1 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT; low_ctx_qw1 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; @@ -2164,8 +2164,8 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp, } if (pkt[0]->ol_flags & RTE_MBUF_F_TX_QINQ) { uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? - (uint64_t)pkt[0]->vlan_tci_outer : - (uint64_t)pkt[0]->vlan_tci; + (uint64_t)pkt[0]->vlan_tci : + (uint64_t)pkt[0]->vlan_tci_outer; hi_ctx_qw0 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT; low_ctx_qw0 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h index 5ac96f239b..36150f0f65 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h @@ -238,11 +238,12 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt, 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); } #endif -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 5/6] net/iavf: fix QinQ insertion for single packet Tx 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus ` (3 preceding siblings ...) 2025-11-05 15:26 ` [PATCH v2 4/6] net/iavf: fix single VLAN insertion positioning Ciara Loftus @ 2025-11-05 15:26 ` Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 6/6] net/iavf: fix QinQ insertion with mbuf flags VLAN and QINQ Ciara Loftus 5 siblings, 0 replies; 15+ messages in thread From: Ciara Loftus @ 2025-11-05 15:26 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus QinQ insertion was enabled in the bulk transmit function but not the single packet transmit function. Implement it in the single packet function. Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> --- drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c index 6182988612..cd067a0199 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c @@ -2077,12 +2077,16 @@ ctx_vtx1(volatile struct iavf_tx_desc *txdp, struct rte_mbuf *pkt, if (offload) { iavf_fill_ctx_desc_tunneling_avx512(&low_ctx_qw, pkt); #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD - 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; + 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; } #endif } -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 6/6] net/iavf: fix QinQ insertion with mbuf flags VLAN and QINQ 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus ` (4 preceding siblings ...) 2025-11-05 15:26 ` [PATCH v2 5/6] net/iavf: fix QinQ insertion for single packet Tx Ciara Loftus @ 2025-11-05 15:26 ` Ciara Loftus 5 siblings, 0 replies; 15+ messages in thread From: Ciara Loftus @ 2025-11-05 15:26 UTC (permalink / raw) To: dev; +Cc: Ciara Loftus Fix an issue that would arise in the event an mbuf had both the VLAN and QINQ insertion 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. Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> --- drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 26 +++++++++---------- drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 9 +++---- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c index cd067a0199..7c0907b7cf 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c @@ -2136,13 +2136,6 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp, #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD if (offload) { - if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN && - 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; - } if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) { uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? (uint64_t)pkt[1]->vlan_tci : @@ -2150,6 +2143,12 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp, hi_ctx_qw1 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT; low_ctx_qw1 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; + } else if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN && + 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; } } #endif @@ -2159,13 +2158,6 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp, #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD if (offload) { - if (pkt[0]->ol_flags & RTE_MBUF_F_TX_VLAN && - 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; - } if (pkt[0]->ol_flags & RTE_MBUF_F_TX_QINQ) { uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? (uint64_t)pkt[0]->vlan_tci : @@ -2173,6 +2165,12 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp, hi_ctx_qw0 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT; low_ctx_qw0 |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM; + } else if (pkt[0]->ol_flags & RTE_MBUF_F_TX_VLAN && + 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; } } #endif diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h index 36150f0f65..d98fb9fe29 100644 --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h @@ -230,12 +230,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; /* vlan_flag specifies outer tag location for QinQ. */ @@ -245,6 +239,9 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt, else *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; + *txd_hi |= ((uint64_t)tx_pkt->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT); } #endif -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-11-05 19:31 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-10-31 15:22 [PATCH 0/2] Fixes for iavf VLAN insertion offload Ciara Loftus 2025-10-31 15:22 ` [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion Ciara Loftus 2025-11-04 17:13 ` Bruce Richardson 2025-11-04 17:16 ` Bruce Richardson 2025-10-31 15:22 ` [PATCH 2/2] net/iavf: fix single VLAN insertion positioning Ciara Loftus 2025-11-04 17:21 ` Bruce Richardson 2025-11-04 17:28 ` Bruce Richardson 2025-11-05 15:26 ` [PATCH v2 0/6] Fixes for iavf VLAN insertion offload Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 1/6] net/iavf: fix compilation with VLAN insertion disabled Ciara Loftus 2025-11-05 19:31 ` Stephen Hemminger 2025-11-05 15:26 ` [PATCH v2 2/6] net/iavf: remove duplication in AVX-512 VLAN insert logic Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 3/6] net/iavf: ensure correct conditions for AVX-512 VLAN offload Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 4/6] net/iavf: fix single VLAN insertion positioning Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 5/6] net/iavf: fix QinQ insertion for single packet Tx Ciara Loftus 2025-11-05 15:26 ` [PATCH v2 6/6] net/iavf: fix QinQ insertion with mbuf flags VLAN and QINQ Ciara Loftus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).