RE: [PATCH 1/1] net/mana: add vlan tagging support

[Wei Hu <weh@microsoft.com>]

> -----Original Message-----
> From: Long Li <longli@microsoft.com>
> Sent: Saturday, February 10, 2024 2:48 AM
> To: Wei Hu <weh@microsoft.com>; ferruh.yigit@amd.com;
> andrew.rybchenko@oktetlabs.ru; Thomas Monjalon
> <thomas@monjalon.net>; Alan Elder <alan.elder@microsoft.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 1/1] net/mana: add vlan tagging support
> 
> > +		if (oob->rx_vlan_tag_present) {
> > +			mbuf->ol_flags |=
> > +				RTE_MBUF_F_RX_VLAN |
> > RTE_MBUF_F_RX_VLAN_STRIPPED;
> > +			mbuf->vlan_tci = oob->rx_vlan_id;
> > +		}
> > +
> 
> Netvsc has the following code for dealing with vlan on RX mbufs (in hn_rxtx.c):
>                 /* NDIS always strips tag, put it back if necessary */
>                 if (!hv->vlan_strip && rte_vlan_insert(&m)) {
> 
> It seems we should do the same?

Not sure if we want to do the same. Two reasons. 

1. Searching the netvsc source, I don't see a place that it set hv->vlan_strip to false. It means
!hv->vlan_string is always false, and rte_vlan_insert(&m) never run. 

2. Usually vlan_strip can be set to true or false if the hardware supports this feature. In the mana
case, the hardware strips off the vlan tag anyway. There is no way to tell the mana hardware to 
keep the tag. Adding the tag back by software not only slows things down, but it also complicates the 
code and test. Not sure if there is any real application needs it. 

I am open to add it.  But in my opinion, we don't need it. Let me know what you think.
> 
> >  		pkts[pkt_received++] = mbuf;
> >  		rxq->stats.packets++;
> >  		rxq->stats.bytes += mbuf->data_len; diff --git
> > a/drivers/net/mana/tx.c b/drivers/net/mana/tx.c index
> > 58c4a1d976..f075fcb0f5 100644
> > --- a/drivers/net/mana/tx.c
> > +++ b/drivers/net/mana/tx.c
> > @@ -180,6 +180,15 @@ get_vsq_frame_num(uint32_t vsq)
> >  	return v.vsq_frame;
> >  }
> >
> > +#define VLAN_PRIO_MASK		0xe000 /* Priority Code Point */
> > +#define VLAN_PRIO_SHIFT		13
> > +#define VLAN_CFI_MASK		0x1000 /* Canonical Format Indicator
> > / Drop Eligible Indicator */
> > +#define VLAN_VID_MASK		0x0fff /* VLAN Identifier */
> > +
> > +#define mana_mbuf_vlan_tag_get_id(m)	((m)->vlan_tci &
> > VLAN_VID_MASK)
> > +#define mana_mbuf_vlan_tag_get_cfi(m)	(!!((m)->vlan_tci &
> > VLAN_CFI_MASK))
> > +#define mana_mbuf_vlan_tag_get_prio(m)	(((m)->vlan_tci &
> > VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT)
> > +
> 
> Those definitions look like those in @Alan Elder's patch for netvsc. Can we
> consolidate some of those definitions into a common place?
> 
> Maybe in "lib/net/rte_ether.h"?
> 

Ok. Will add it to rte_ether.h.

Thanks,
Wei


> Thanks,
> 
> Long