RE: [PATCH 1/3] net/iavf: support qinq insertion offload for scalar path

["Loftus, Ciara" <ciara.loftus@intel.com>]

> 
> On Thu, Jun 19, 2025 at 01:36:56PM +0000, Ciara Loftus wrote:
> > Enable Tx QINQ offload if the VF reports support for inserting both an
> > outer and inner VLAN tag. The VF capabilities report the locations for
> > placing each of the tags - either L2TAG1 in the tx descriptor or L2TAG2
> > in the context descriptor. Use this information to configure the
> > descriptors correctly.
> > This offload was previously incorrectly reported as always supported in
> > the device configuration, so this is corrected.
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > ---
> >  doc/guides/nics/features/iavf.ini    |  1 +
> >  drivers/net/intel/iavf/iavf_ethdev.c |  8 +++-
> >  drivers/net/intel/iavf/iavf_rxtx.c   | 55 +++++++++++++++++++++-------
> >  3 files changed, 48 insertions(+), 16 deletions(-)
> >
> > diff --git a/doc/guides/nics/features/iavf.ini
> b/doc/guides/nics/features/iavf.ini
> > index ce9860e963..61c4742197 100644
> > --- a/doc/guides/nics/features/iavf.ini
> > +++ b/doc/guides/nics/features/iavf.ini
> > @@ -29,6 +29,7 @@ Traffic manager      = Y
> >  Inline crypto        = Y
> >  CRC offload          = Y
> >  VLAN offload         = P
> > +QinQ offload         = P
> >  L3 checksum offload  = Y
> >  L4 checksum offload  = Y
> >  Timestamp offload    = Y
> > diff --git a/drivers/net/intel/iavf/iavf_ethdev.c
> b/drivers/net/intel/iavf/iavf_ethdev.c
> > index b3dacbef84..d058b87d54 100644
> > --- a/drivers/net/intel/iavf/iavf_ethdev.c
> > +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> > @@ -622,7 +622,7 @@ iavf_dev_vlan_insert_set(struct rte_eth_dev *dev)
> >  		return 0;
> >
> >  	enable = !!(dev->data->dev_conf.txmode.offloads &
> > -		    RTE_ETH_TX_OFFLOAD_VLAN_INSERT);
> > +		    (RTE_ETH_TX_OFFLOAD_VLAN_INSERT |
> RTE_ETH_TX_OFFLOAD_QINQ_INSERT));
> >  	iavf_config_vlan_insert_v2(adapter, enable);
> >
> >  	return 0;
> > @@ -1158,7 +1158,6 @@ iavf_dev_info_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
> >
> >  	dev_info->tx_offload_capa =
> >  		RTE_ETH_TX_OFFLOAD_VLAN_INSERT |
> > -		RTE_ETH_TX_OFFLOAD_QINQ_INSERT |
> >  		RTE_ETH_TX_OFFLOAD_IPV4_CKSUM |
> >  		RTE_ETH_TX_OFFLOAD_UDP_CKSUM |
> >  		RTE_ETH_TX_OFFLOAD_TCP_CKSUM |
> > @@ -1182,6 +1181,11 @@ iavf_dev_info_get(struct rte_eth_dev *dev,
> struct rte_eth_dev_info *dev_info)
> >  	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP)
> >  		dev_info->rx_offload_capa |=
> RTE_ETH_RX_OFFLOAD_TIMESTAMP;
> >
> > +	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_VLAN_V2 &&
> > +			vf->vlan_v2_caps.offloads.insertion_support.inner
> &&
> > +			vf->vlan_v2_caps.offloads.insertion_support.outer)
> > +		dev_info->tx_offload_capa |=
> RTE_ETH_TX_OFFLOAD_QINQ_INSERT;
> > +
> >  	if (iavf_ipsec_crypto_supported(adapter)) {
> >  		dev_info->rx_offload_capa |=
> RTE_ETH_RX_OFFLOAD_SECURITY;
> >  		dev_info->tx_offload_capa |=
> RTE_ETH_TX_OFFLOAD_SECURITY;
> > diff --git a/drivers/net/intel/iavf/iavf_rxtx.c
> b/drivers/net/intel/iavf/iavf_rxtx.c
> > index 5411eb6897..1ce9de0699 100644
> > --- a/drivers/net/intel/iavf/iavf_rxtx.c
> > +++ b/drivers/net/intel/iavf/iavf_rxtx.c
> > @@ -797,17 +797,32 @@ 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)
> > -			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");
> > +		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");
> 
> Is this scenario possible - we only support insertion of a single vlan tag,
> and it's to go in the context descriptor. In this case, do we not need to
> add in the VLAN offload flag to the list of offloads needing a context
> descriptor below?

This scenario is possible. It's the case I hit when I test with a VF from my Fortville card.
This code block is the same as what's in the existing code, it's wrapped around a new if condition checking if only one insertion capability is supported (inner or outer). The new code is in the else {}.
No update is needed to iavf_calc_context_desc because what you are describing is already in place.

> 
> > +			}
> > +		} 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");
> > +			}
> >  		}
> >  	} else {
> >  		txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > @@ -2391,7 +2406,7 @@ iavf_calc_context_desc(struct rte_mbuf *mb,
> uint8_t vlan_flag)
> >  	uint64_t flags = mb->ol_flags;
> >  	if (flags & (RTE_MBUF_F_TX_TCP_SEG | RTE_MBUF_F_TX_UDP_SEG |
> >  	    RTE_MBUF_F_TX_TUNNEL_MASK |
> RTE_MBUF_F_TX_OUTER_IP_CKSUM |
> > -	    RTE_MBUF_F_TX_OUTER_UDP_CKSUM))
> > +	    RTE_MBUF_F_TX_OUTER_UDP_CKSUM | RTE_MBUF_F_TX_QINQ))
> >  		return 1;
> 
> In this check, do we need to take into account possible case for only a
> single vlan offload, and requiring an L2TAG2 field for it?

This already exists. Here's what's already there:

	if (flags & RTE_MBUF_F_TX_VLAN &&
	    vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)
		return 1;

Thanks,
Ciara

> 
> /Bruce