Re: [PATCH v4 14/35] net/intel: support configurable VLAN tag insertion on Tx

Burakov, Anatoly
2026-02-11T09:29:42Z

On 2/10/2026 7:20 PM, Bruce Richardson wrote:
> On Tue, Feb 10, 2026 at 02:21:49PM +0100, Burakov, Anatoly wrote:
>> On 2/9/2026 5:45 PM, Bruce Richardson wrote:
>>> Make the VLAN tag insertion logic configurable in the common code, as to
>>> where inner/outer tags get placed.
>>>
>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>
>> Hi Bruce,
>> I might be missing something obvious, but...
>>
>>> - /* Descriptor based VLAN insertion */
>>> - if (ol_flags & (RTE_MBUF_F_TX_VLAN | RTE_MBUF_F_TX_QINQ)) {
>>> + /* Descriptor based VLAN/QinQ insertion */
>>> + /* for single vlan offload, only insert in data desc with VLAN_IN_L2TAG1 is set
>>> + * for qinq offload, we always put inner tag in L2Tag1
>>> + */
>>> + if (((ol_flags & RTE_MBUF_F_TX_VLAN) && l2tag1_field == CI_VLAN_IN_L2TAG1) ||
>>> + (ol_flags & RTE_MBUF_F_TX_QINQ)) {
>>> td_cmd |= CI_TX_DESC_CMD_IL2TAG1;
>>> td_tag = tx_pkt->vlan_tci;
>>> }
>>
>> I can see that we insert VLAN tag on TX_VLAN and VLAN_IN_TAG1. But then...
>>
>>
>>> @@ -1004,8 +1004,7 @@ get_context_desc(uint64_t ol_flags, const struct rte_mbuf *tx_pkt,
>>> /* TX context descriptor based double VLAN insert */
>>> if (ol_flags & RTE_MBUF_F_TX_QINQ) {
>>> cd_l2tag2 = tx_pkt->vlan_tci_outer;
>>> - cd_type_cmd_tso_mss |=
>>> - ((uint64_t)I40E_TX_CTX_DESC_IL2TAG2 << I40E_TXD_CTX_QW1_CMD_SHIFT);
>>> + cd_type_cmd_tso_mss |= (I40E_TX_CTX_DESC_IL2TAG2 << I40E_TXD_CTX_QW1_CMD_SHIFT);
>>
>> this logic is only triggered for QinQ. Meaning, there's nowhere we insert
>> VLAN tag on TX_VLAN and VLAN_IN_TAG2?
>>
> For VLAN_IN_L2TAG2 the vlan tag goes in the context descriptor, and
> creating the context descriptor is the responsibility of each individual
> driver. The two drivers using this common path, i40e and ice both put the
> tag in the TAG1 slot, so don't have a path to add it to the TAG2 slot. We
> make it configurable in this patch so that in the next patches we can add
> in support for the iavf driver - which does support putting the VLAN tag in
> the Tag2 slot, in it's context descriptor function.
>
> /Bruce

Perhaps we need to document that more explicitly then, and double-check
that all driver paths actually do this in all required cases (i.e. not
just for QinQ but also for regular VLAN insertion case).

--
Thanks,
Anatoly