diff mbox series

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

Message ID 20240209085211.2643148-1-weh@microsoft.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [1/1] net/mana: add vlan tagging support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Wei Hu Feb. 9, 2024, 8:52 a.m. UTC 
  For tx path, use LONG_PACKET_FORMAT if vlan tag is present. For rx,
extract vlan id from oob, put into mbuf and set the vlan flags in
mbuf.

Also add myself to the maintainers list for vmbus, mana and netvsc.

Signed-off-by: Wei Hu <weh@microsoft.com>
---
 MAINTAINERS             |  3 +++
 drivers/net/mana/mana.h |  2 ++
 drivers/net/mana/rx.c   |  6 ++++++
 drivers/net/mana/tx.c   | 30 +++++++++++++++++++++++++++---
 4 files changed, 38 insertions(+), 3 deletions(-)

Comments

Ferruh Yigit Feb. 9, 2024, 4:06 p.m. UTC | #1 
On 2/9/2024 8:52 AM, Wei Hu wrote:
> For tx path, use LONG_PACKET_FORMAT if vlan tag is present. For rx,
> extract vlan id from oob, put into mbuf and set the vlan flags in
> mbuf.
> 
> Also add myself to the maintainers list for vmbus, mana and netvsc.
> 
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
>  MAINTAINERS             |  3 +++
>  drivers/net/mana/mana.h |  2 ++
>  drivers/net/mana/rx.c   |  6 ++++++
>  drivers/net/mana/tx.c   | 30 +++++++++++++++++++++++++++---
>  4 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5fb3a73f84..9983d013a6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -608,6 +608,7 @@ F: app/test/test_vdev.c
>  
>  VMBUS bus driver
>  M: Long Li <longli@microsoft.com>
> +M: Wei Hu <weh@microsoft.com>
>  F: drivers/bus/vmbus/
>  
>  
> @@ -867,6 +868,7 @@ F: doc/guides/nics/features/mlx5.ini
>  
>  Microsoft mana
>  M: Long Li <longli@microsoft.com>
> +M: Wei Hu <weh@microsoft.com>
>  F: drivers/net/mana/
>  F: doc/guides/nics/mana.rst
>  F: doc/guides/nics/features/mana.ini
> @@ -878,6 +880,7 @@ F: doc/guides/nics/vdev_netvsc.rst
>  
>  Microsoft Hyper-V netvsc
>  M: Long Li <longli@microsoft.com>
> +M: Wei Hu <weh@microsoft.com>
>  F: drivers/net/netvsc/
>  F: doc/guides/nics/netvsc.rst
>  F: doc/guides/nics/features/netvsc.ini
>

Hi Wei,

Can you please separate the maintainer file update to its own patch?
Long Li Feb. 9, 2024, 6:48 p.m. UTC | #2 
> +		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?

>  		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"?

Thanks,

Long
Wei Hu Feb. 20, 2024, 4:29 a.m. UTC | #3 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Saturday, February 10, 2024 12:06 AM
> To: Wei Hu <weh@microsoft.com>; andrew.rybchenko@oktetlabs.ru; Thomas
> Monjalon <thomas@monjalon.net>; Long Li <longli@microsoft.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 1/1] net/mana: add vlan tagging support
> 
> On 2/9/2024 8:52 AM, Wei Hu wrote:
> > For tx path, use LONG_PACKET_FORMAT if vlan tag is present. For rx,
> > extract vlan id from oob, put into mbuf and set the vlan flags in
> > mbuf.
> >
> > Also add myself to the maintainers list for vmbus, mana and netvsc.
> >
> > Signed-off-by: Wei Hu <weh@microsoft.com>
> > ---
> >  MAINTAINERS             |  3 +++
> >  drivers/net/mana/mana.h |  2 ++
> >  drivers/net/mana/rx.c   |  6 ++++++
> >  drivers/net/mana/tx.c   | 30 +++++++++++++++++++++++++++---
> >  4 files changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index 5fb3a73f84..9983d013a6
> > 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -608,6 +608,7 @@ F: app/test/test_vdev.c
> >
> >  VMBUS bus driver
> >  M: Long Li <longli@microsoft.com>
> > +M: Wei Hu <weh@microsoft.com>
> >  F: drivers/bus/vmbus/
> >
> >
> > @@ -867,6 +868,7 @@ F: doc/guides/nics/features/mlx5.ini
> >
> >  Microsoft mana
> >  M: Long Li <longli@microsoft.com>
> > +M: Wei Hu <weh@microsoft.com>
> >  F: drivers/net/mana/
> >  F: doc/guides/nics/mana.rst
> >  F: doc/guides/nics/features/mana.ini
> > @@ -878,6 +880,7 @@ F: doc/guides/nics/vdev_netvsc.rst
> >
> >  Microsoft Hyper-V netvsc
> >  M: Long Li <longli@microsoft.com>
> > +M: Wei Hu <weh@microsoft.com>
> >  F: drivers/net/netvsc/
> >  F: doc/guides/nics/netvsc.rst
> >  F: doc/guides/nics/features/netvsc.ini
> >
> 
> Hi Wei,
> 
> Can you please separate the maintainer file update to its own patch?

Sure. I will remove the maintainer file update and send it out later separately.

Thanks,
Wei
Wei Hu Feb. 20, 2024, 5:48 a.m. UTC | #4 
> -----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
Long Li Feb. 20, 2024, 6:59 p.m. UTC | #5 
> 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.

It's set here:
drivers/net/netvsc/hn_ethdev.c: hv->vlan_strip = !!(rxmode->offloads & RTE_ETH_RX_OFFLOAD_VLAN_STRIP);

> 
> 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.

If MANA always strips the tag, you need to add the tag back if needed.

> 
> 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.

@Alan has sent a patch for some helper marcos for parsing vlan_tci, please use those helper marcos in
https://git.dpdk.org/next/dpdk-next-net/commit/?id=f9c66ea9b5bfd15f314f6d101f156a312121e7e1

Thanks,

Long
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 5fb3a73f84..9983d013a6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -608,6 +608,7 @@  F: app/test/test_vdev.c
 
 VMBUS bus driver
 M: Long Li <longli@microsoft.com>
+M: Wei Hu <weh@microsoft.com>
 F: drivers/bus/vmbus/
 
 
@@ -867,6 +868,7 @@  F: doc/guides/nics/features/mlx5.ini
 
 Microsoft mana
 M: Long Li <longli@microsoft.com>
+M: Wei Hu <weh@microsoft.com>
 F: drivers/net/mana/
 F: doc/guides/nics/mana.rst
 F: doc/guides/nics/features/mana.ini
@@ -878,6 +880,7 @@  F: doc/guides/nics/vdev_netvsc.rst
 
 Microsoft Hyper-V netvsc
 M: Long Li <longli@microsoft.com>
+M: Wei Hu <weh@microsoft.com>
 F: drivers/net/netvsc/
 F: doc/guides/nics/netvsc.rst
 F: doc/guides/nics/features/netvsc.ini
diff --git a/drivers/net/mana/mana.h b/drivers/net/mana/mana.h
index 4c56e6f746..2b65a19878 100644
--- a/drivers/net/mana/mana.h
+++ b/drivers/net/mana/mana.h
@@ -21,10 +21,12 @@  struct mana_shared_data {
 #define MANA_MAX_MAC_ADDR 1
 
 #define MANA_DEV_RX_OFFLOAD_SUPPORT ( \
+		RTE_ETH_RX_OFFLOAD_VLAN_STRIP | \
 		RTE_ETH_RX_OFFLOAD_CHECKSUM | \
 		RTE_ETH_RX_OFFLOAD_RSS_HASH)
 
 #define MANA_DEV_TX_OFFLOAD_SUPPORT ( \
+		RTE_ETH_TX_OFFLOAD_VLAN_INSERT | \
 		RTE_ETH_TX_OFFLOAD_MULTI_SEGS | \
 		RTE_ETH_TX_OFFLOAD_IPV4_CKSUM | \
 		RTE_ETH_TX_OFFLOAD_TCP_CKSUM | \
diff --git a/drivers/net/mana/rx.c b/drivers/net/mana/rx.c
index acad5e26cd..506f073708 100644
--- a/drivers/net/mana/rx.c
+++ b/drivers/net/mana/rx.c
@@ -517,6 +517,12 @@  mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
 			mbuf->hash.rss = oob->packet_info[pkt_idx].packet_hash;
 		}
 
+		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;
+		}
+
 		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)
+
 uint16_t
 mana_tx_burst(void *dpdk_txq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 {
@@ -254,7 +263,18 @@  mana_tx_burst(void *dpdk_txq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		}
 
 		/* Fill in the oob */
-		tx_oob.short_oob.packet_format = SHORT_PACKET_FORMAT;
+		if (m_pkt->ol_flags & RTE_MBUF_F_TX_VLAN) {
+			tx_oob.short_oob.packet_format = LONG_PACKET_FORMAT;
+			tx_oob.long_oob.inject_vlan_prior_tag = 1;
+			tx_oob.long_oob.priority_code_point =
+				mana_mbuf_vlan_tag_get_prio(m_pkt);
+			tx_oob.long_oob.drop_eligible_indicator =
+				mana_mbuf_vlan_tag_get_cfi(m_pkt);
+			tx_oob.long_oob.vlan_identifier =
+				mana_mbuf_vlan_tag_get_id(m_pkt);
+		} else {
+			tx_oob.short_oob.packet_format = SHORT_PACKET_FORMAT;
+		}
 		tx_oob.short_oob.tx_is_outer_ipv4 =
 			m_pkt->ol_flags & RTE_MBUF_F_TX_IPV4 ? 1 : 0;
 		tx_oob.short_oob.tx_is_outer_ipv6 =
@@ -409,8 +429,12 @@  mana_tx_burst(void *dpdk_txq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 
 		work_req.sgl = sgl.gdma_sgl;
 		work_req.num_sgl_elements = m_pkt->nb_segs;
-		work_req.inline_oob_size_in_bytes =
-			sizeof(struct transmit_short_oob_v2);
+		if (tx_oob.short_oob.packet_format == SHORT_PACKET_FORMAT)
+			work_req.inline_oob_size_in_bytes =
+				sizeof(struct transmit_short_oob_v2);
+		else
+			work_req.inline_oob_size_in_bytes =
+				sizeof(struct transmit_oob_v2);
 		work_req.inline_oob_data = &tx_oob;
 		work_req.flags = 0;
 		work_req.client_data_unit = NOT_USING_CLIENT_DATA_UNIT;