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 <[email protected]>
>
> One suggestion inline below.
>
> Acked-by: Bruce Richardson <[email protected]>
>
>
> > ---
> > 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
> >
