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 <[email protected]> > > > > Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx") > > > > Signed-off-by: Ciara Loftus <[email protected]> > > --- > > 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 > >
