[PATCH v3] net/netvsc: fix parsing of VLAN metadata

[PATCH v3] net/netvsc: fix parsing of VLAN metadata

[Alan Elder]

The previous code incorrectly parsed the VLAN ID and priority.
If the 16-bits of VLAN ID and priority/CFI on the wire was
0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
were macros defined to handle this conversion but they were not
used.

This fix takes an approach similar to the Linux netvsc driver and
defines an explicit structure to use for parsing.

Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
Cc: sthemmin@microsoft.com
Cc: stable@dpdk.org

Signed-off-by: Alan Elder <alan.elder@microsoft.com>
---
 .mailmap                     |  1 +
 drivers/net/netvsc/hn_rxtx.c | 23 +++++++++++++++++------
 drivers/net/netvsc/ndis.h    | 23 +++++++++++++----------
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/.mailmap b/.mailmap
index a0756974e2..eca02318d6 100644
--- a/.mailmap
+++ b/.mailmap
@@ -33,6 +33,7 @@ Alain Leon <xerebz@gmail.com>
 Alan Brady <alan.brady@intel.com>
 Alan Carew <alan.carew@intel.com>
 Alan Dewar <alan.dewar@att.com> <adewar@brocade.com>
+Alan Elder <alan.elder@microsoft.com>
 Alan Liu <zaoxingliu@gmail.com>
 Alan Winkowski <walan@marvell.com>
 Alejandro Lucero <alejandro.lucero@netronome.com>
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index e4f5015aa3..e3b9899f1e 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -42,8 +42,13 @@
 #define HN_TXD_CACHE_SIZE	32 /* per cpu tx_descriptor pool cache */
 #define HN_RXQ_EVENT_DEFAULT	2048
 
+#define HN_VLAN_PRIO_MASK	0xe000 /* Priority Code Point */
+#define HN_VLAN_PRIO_SHIFT	13
+#define HN_VLAN_CFI_MASK	0x1000 /* Canonical Format Indicator / Drop Eligible Indicator */
+#define HN_VLAN_VID_MASK	0x0fff /* VLAN Identifier */
+
 struct hn_rxinfo {
-	uint32_t	vlan_info;
+	struct ndis_pkt_vlan_info vlan_info;
 	uint32_t	csum_info;
 	uint32_t	hash_info;
 	uint32_t	hash_value;
@@ -477,7 +482,7 @@ hn_rndis_rxinfo(const void *info_data, unsigned int info_dlen,
 		case NDIS_PKTINFO_TYPE_VLAN:
 			if (unlikely(dlen < NDIS_VLAN_INFO_SIZE))
 				return -EINVAL;
-			info->vlan_info = *((const uint32_t *)data);
+			info->vlan_info = *((const struct ndis_pkt_vlan_info *)data);
 			mask |= HN_RXINFO_VLAN;
 			break;
 
@@ -611,8 +616,10 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 					   RTE_PTYPE_L3_MASK |
 					   RTE_PTYPE_L4_MASK);
 
-	if (info->vlan_info != HN_NDIS_VLAN_INFO_INVALID) {
-		m->vlan_tci = info->vlan_info;
+	if (info->vlan_info.value != HN_NDIS_VLAN_INFO_INVALID) {
+		m->vlan_tci = info->vlan_info.vlanid |
+				(info->vlan_info.pri << HN_VLAN_PRIO_SHIFT) |
+				(info->vlan_info.cfi ? HN_VLAN_CFI_MASK : 0);
 		m->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED | RTE_MBUF_F_RX_VLAN;
 
 		/* NDIS always strips tag, put it back if necessary */
@@ -669,7 +676,7 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
 	unsigned int pktinfo_off, pktinfo_len;
 	const struct rndis_packet_msg *pkt = data;
 	struct hn_rxinfo info = {
-		.vlan_info = HN_NDIS_VLAN_INFO_INVALID,
+		.vlan_info.value = HN_NDIS_VLAN_INFO_INVALID,
 		.csum_info = HN_NDIS_RXCSUM_INFO_INVALID,
 		.hash_info = HN_NDIS_HASH_INFO_INVALID,
 	};
@@ -1332,7 +1339,11 @@ static void hn_encap(struct rndis_packet_msg *pkt,
 	if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
 		pi_data = hn_rndis_pktinfo_append(pkt, NDIS_VLAN_INFO_SIZE,
 						  NDIS_PKTINFO_TYPE_VLAN);
-		*pi_data = m->vlan_tci;
+		struct ndis_pkt_vlan_info *vlan = (struct ndis_pkt_vlan_info *)pi_data;
+		vlan->value = 0;
+		vlan->vlanid = (m->vlan_tci & HN_VLAN_VID_MASK);
+		vlan->cfi = (!!(m->vlan_tci & HN_VLAN_CFI_MASK));
+		vlan->pri = ((m->vlan_tci & HN_VLAN_PRIO_MASK) >> HN_VLAN_PRIO_SHIFT);
 	}
 
 	if (m->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
diff --git a/drivers/net/netvsc/ndis.h b/drivers/net/netvsc/ndis.h
index d97a397a86..a1e587c738 100644
--- a/drivers/net/netvsc/ndis.h
+++ b/drivers/net/netvsc/ndis.h
@@ -316,16 +316,19 @@ struct ndis_offload {
  */
 
 /* VLAN */
-#define	NDIS_VLAN_INFO_SIZE		sizeof(uint32_t)
-#define	NDIS_VLAN_INFO_PRI_MASK		0x0007
-#define	NDIS_VLAN_INFO_CFI_MASK		0x0008
-#define	NDIS_VLAN_INFO_ID_MASK		0xfff0
-#define	NDIS_VLAN_INFO_MAKE(id, pri, cfi)	\
-	(((pri) & NDIS_VLAN_INFO_PRI_MASK) |	\
-	 (((cfi) & 0x1) << 3) | (((id) & 0xfff) << 4))
-#define	NDIS_VLAN_INFO_ID(inf)		(((inf) & NDIS_VLAN_INFO_ID_MASK) >> 4)
-#define	NDIS_VLAN_INFO_CFI(inf)		(((inf) & NDIS_VLAN_INFO_CFI_MASK) >> 3)
-#define	NDIS_VLAN_INFO_PRI(inf)		((inf) & NDIS_VLAN_INFO_PRI_MASK)
+struct ndis_pkt_vlan_info {
+	union {
+		struct {
+			uint32_t pri:3; /* User Priority */
+			uint32_t cfi:1; /* Canonical Format ID / DEI */
+			uint32_t vlanid:12; /* VLAN ID */
+			uint32_t reserved:16;
+		};
+		uint32_t value;
+	};
+};
+
+#define	NDIS_VLAN_INFO_SIZE		sizeof(struct ndis_pkt_vlan_info)
 
 /* Reception checksum */
 #define	NDIS_RXCSUM_INFO_SIZE		sizeof(uint32_t)
-- 
2.25.1

RE: [PATCH v3] net/netvsc: fix parsing of VLAN metadata

[Long Li]

> -----Original Message-----
> From: Alan Elder <alan.elder@microsoft.com>
> Sent: Thursday, February 8, 2024 7:09 AM
> To: Long Li <longli@microsoft.com>
> Cc: dev@dpdk.org
> Subject: [PATCH v3] net/netvsc: fix parsing of VLAN metadata
> 
> The previous code incorrectly parsed the VLAN ID and priority.
> If the 16-bits of VLAN ID and priority/CFI on the wire was 0123456789ABCDEF
> the code parsed it as 456789ABCDEF3012.  There were macros defined to handle
> this conversion but they were not used.
> 
> This fix takes an approach similar to the Linux netvsc driver and defines an
> explicit structure to use for parsing.
> 
> Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
> Cc: sthemmin@microsoft.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alan Elder <alan.elder@microsoft.com>
> ---
>  .mailmap                     |  1 +
>  drivers/net/netvsc/hn_rxtx.c | 23 +++++++++++++++++------
>  drivers/net/netvsc/ndis.h    | 23 +++++++++++++----------
>  3 files changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/.mailmap b/.mailmap
> index a0756974e2..eca02318d6 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -33,6 +33,7 @@ Alain Leon <xerebz@gmail.com>  Alan Brady
> <alan.brady@intel.com>  Alan Carew <alan.carew@intel.com>  Alan Dewar
> <alan.dewar@att.com> <adewar@brocade.com>
> +Alan Elder <alan.elder@microsoft.com>
>  Alan Liu <zaoxingliu@gmail.com>
>  Alan Winkowski <walan@marvell.com>
>  Alejandro Lucero <alejandro.lucero@netronome.com> diff --git
> a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c index
> e4f5015aa3..e3b9899f1e 100644
> --- a/drivers/net/netvsc/hn_rxtx.c
> +++ b/drivers/net/netvsc/hn_rxtx.c
> @@ -42,8 +42,13 @@
>  #define HN_TXD_CACHE_SIZE	32 /* per cpu tx_descriptor pool cache */
>  #define HN_RXQ_EVENT_DEFAULT	2048
> 
> +#define HN_VLAN_PRIO_MASK	0xe000 /* Priority Code Point */
> +#define HN_VLAN_PRIO_SHIFT	13
> +#define HN_VLAN_CFI_MASK	0x1000 /* Canonical Format Indicator / Drop
> Eligible Indicator */
> +#define HN_VLAN_VID_MASK	0x0fff /* VLAN Identifier */
> +
>  struct hn_rxinfo {
> -	uint32_t	vlan_info;
> +	struct ndis_pkt_vlan_info vlan_info;
>  	uint32_t	csum_info;
>  	uint32_t	hash_info;
>  	uint32_t	hash_value;
> @@ -477,7 +482,7 @@ hn_rndis_rxinfo(const void *info_data, unsigned int
> info_dlen,
>  		case NDIS_PKTINFO_TYPE_VLAN:
>  			if (unlikely(dlen < NDIS_VLAN_INFO_SIZE))
>  				return -EINVAL;
> -			info->vlan_info = *((const uint32_t *)data);
> +			info->vlan_info = *((const struct ndis_pkt_vlan_info
> *)data);
>  			mask |= HN_RXINFO_VLAN;
>  			break;
> 
> @@ -611,8 +616,10 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct
> hn_rx_bufinfo *rxb,
>  					   RTE_PTYPE_L3_MASK |
>  					   RTE_PTYPE_L4_MASK);
> 
> -	if (info->vlan_info != HN_NDIS_VLAN_INFO_INVALID) {
> -		m->vlan_tci = info->vlan_info;
> +	if (info->vlan_info.value != HN_NDIS_VLAN_INFO_INVALID) {
> +		m->vlan_tci = info->vlan_info.vlanid |
> +				(info->vlan_info.pri << HN_VLAN_PRIO_SHIFT) |
> +				(info->vlan_info.cfi ? HN_VLAN_CFI_MASK : 0);
>  		m->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED |
> RTE_MBUF_F_RX_VLAN;
> 
>  		/* NDIS always strips tag, put it back if necessary */ @@ -669,7
> +676,7 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
>  	unsigned int pktinfo_off, pktinfo_len;
>  	const struct rndis_packet_msg *pkt = data;
>  	struct hn_rxinfo info = {
> -		.vlan_info = HN_NDIS_VLAN_INFO_INVALID,
> +		.vlan_info.value = HN_NDIS_VLAN_INFO_INVALID,
>  		.csum_info = HN_NDIS_RXCSUM_INFO_INVALID,
>  		.hash_info = HN_NDIS_HASH_INFO_INVALID,
>  	};
> @@ -1332,7 +1339,11 @@ static void hn_encap(struct rndis_packet_msg *pkt,
>  	if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
>  		pi_data = hn_rndis_pktinfo_append(pkt, NDIS_VLAN_INFO_SIZE,
>  						  NDIS_PKTINFO_TYPE_VLAN);
> -		*pi_data = m->vlan_tci;
> +		struct ndis_pkt_vlan_info *vlan = (struct ndis_pkt_vlan_info
> *)pi_data;
> +		vlan->value = 0;
> +		vlan->vlanid = (m->vlan_tci & HN_VLAN_VID_MASK);
> +		vlan->cfi = (!!(m->vlan_tci & HN_VLAN_CFI_MASK));
> +		vlan->pri = ((m->vlan_tci & HN_VLAN_PRIO_MASK) >>
> +HN_VLAN_PRIO_SHIFT);

Thanks Alan.

Can you remove the extra "()" as suggested by Stephen? The rest of the patch looks good.

Please include maintainers of of dpdk-next-net tree: (from "MAINTAINERS")
Next-net Tree
M: Ferruh Yigit <ferruh.yigit@amd.com>
M: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>


Long

[PATCH v4] net/netvsc: fix parsing of VLAN metadata

[Alan Elder]

The previous code incorrectly parsed the VLAN ID and priority.
If the 16-bits of VLAN ID and priority/CFI on the wire was
0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
were macros defined to handle this conversion but they were not
used.

Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
Cc: sthemmin@microsoft.com
Cc: stable@dpdk.org

Signed-off-by: Alan Elder <alan.elder@microsoft.com>
---
v4:
* Make consistent with FreeBSD code

 .mailmap                     |  1 +
 drivers/net/netvsc/hn_rxtx.c | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/.mailmap b/.mailmap
index a0756974e2..eca02318d6 100644
--- a/.mailmap
+++ b/.mailmap
@@ -33,6 +33,7 @@ Alain Leon <xerebz@gmail.com>
 Alan Brady <alan.brady@intel.com>
 Alan Carew <alan.carew@intel.com>
 Alan Dewar <alan.dewar@att.com> <adewar@brocade.com>
+Alan Elder <alan.elder@microsoft.com>
 Alan Liu <zaoxingliu@gmail.com>
 Alan Winkowski <walan@marvell.com>
 Alejandro Lucero <alejandro.lucero@netronome.com>
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index e4f5015aa3..1cbb8df03f 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -42,6 +42,19 @@
 #define HN_TXD_CACHE_SIZE	32 /* per cpu tx_descriptor pool cache */
 #define HN_RXQ_EVENT_DEFAULT	2048
 
+#define HN_VLAN_CFI_SHIFT	12
+#define HN_VLAN_PRI_SHIFT	13
+#define HN_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
+#define HN_VLAN_CFI_MASK	0x1000 /* Canonical Format Indicator / Drop Eligible Indicator */
+#define HN_VLAN_VID_MASK	0x0fff /* VLAN Identifier */
+
+#define HN_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & HN_VLAN_VID_MASK)
+#define HN_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & HN_VLAN_PRI_MASK) >> HN_VLAN_PRI_SHIFT)
+#define HN_VLAN_TCI_CFI(vlan_tci)	(((vlan_tci) & HN_VLAN_CFI_MASK) >> HN_VLAN_CFI_SHIFT)
+#define HN_VLAN_TCI_MAKE(id, pri, cfi)	((id) |				\
+					 ((pri) << HN_VLAN_PRI_SHIFT) |	\
+					 ((cfi) << HN_VLAN_CFI_SHIFT))
+
 struct hn_rxinfo {
 	uint32_t	vlan_info;
 	uint32_t	csum_info;
@@ -612,7 +625,9 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 					   RTE_PTYPE_L4_MASK);
 
 	if (info->vlan_info != HN_NDIS_VLAN_INFO_INVALID) {
-		m->vlan_tci = info->vlan_info;
+		m->vlan_tci = HN_VLAN_TCI_MAKE(NDIS_VLAN_INFO_ID(info->vlan_info),
+					       NDIS_VLAN_INFO_PRI(info->vlan_info),
+					       NDIS_VLAN_INFO_CFI(info->vlan_info));
 		m->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED | RTE_MBUF_F_RX_VLAN;
 
 		/* NDIS always strips tag, put it back if necessary */
@@ -1332,7 +1347,9 @@ static void hn_encap(struct rndis_packet_msg *pkt,
 	if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
 		pi_data = hn_rndis_pktinfo_append(pkt, NDIS_VLAN_INFO_SIZE,
 						  NDIS_PKTINFO_TYPE_VLAN);
-		*pi_data = m->vlan_tci;
+		*pi_data = NDIS_VLAN_INFO_MAKE(HN_VLAN_TCI_ID(m->vlan_tci),
+					       HN_VLAN_TCI_PRI(m->vlan_tci),
+					       HN_VLAN_TCI_CFI(m->vlan_tci));
 	}
 
 	if (m->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
-- 
2.25.1

RE: [PATCH v4] net/netvsc: fix parsing of VLAN metadata

[Long Li]

> +#define HN_VLAN_CFI_SHIFT	12
> +#define HN_VLAN_PRI_SHIFT	13
> +#define HN_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
> +#define HN_VLAN_CFI_MASK	0x1000 /* Canonical Format Indicator / Drop
> Eligible Indicator */
> +#define HN_VLAN_VID_MASK	0x0fff /* VLAN Identifier */
> +
> +#define HN_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & HN_VLAN_VID_MASK)
> +#define HN_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & HN_VLAN_PRI_MASK) >>
> HN_VLAN_PRI_SHIFT)
> +#define HN_VLAN_TCI_CFI(vlan_tci)	(((vlan_tci) & HN_VLAN_CFI_MASK) >>
> HN_VLAN_CFI_SHIFT)
> +#define HN_VLAN_TCI_MAKE(id, pri, cfi)	((id) |
> 	\
> +					 ((pri) << HN_VLAN_PRI_SHIFT) |	\
> +					 ((cfi) << HN_VLAN_CFI_SHIFT))
> +

The patch looks good.

It seems HN_VLAN_TCI_ID, HN_VLAN_TCI_PRI, HN_VLAN_TCI_CFI and HN_VLAN_TCI_MAKE could be useful to other drivers. (at least to MANA)

Ferruh, do you think we should define those common functions in ./lib/net/rte_ether.h?

Thanks

Long

Re: [PATCH v4] net/netvsc: fix parsing of VLAN metadata

[Ferruh Yigit]

On 2/14/2024 10:17 PM, Long Li wrote:
>> +#define HN_VLAN_CFI_SHIFT	12
>> +#define HN_VLAN_PRI_SHIFT	13
>> +#define HN_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
>> +#define HN_VLAN_CFI_MASK	0x1000 /* Canonical Format Indicator / Drop
>> Eligible Indicator */
>> +#define HN_VLAN_VID_MASK	0x0fff /* VLAN Identifier */
>> +
>> +#define HN_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & HN_VLAN_VID_MASK)
>> +#define HN_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & HN_VLAN_PRI_MASK) >>
>> HN_VLAN_PRI_SHIFT)
>> +#define HN_VLAN_TCI_CFI(vlan_tci)	(((vlan_tci) & HN_VLAN_CFI_MASK) >>
>> HN_VLAN_CFI_SHIFT)
>> +#define HN_VLAN_TCI_MAKE(id, pri, cfi)	((id) |
>> 	\
>> +					 ((pri) << HN_VLAN_PRI_SHIFT) |	\
>> +					 ((cfi) << HN_VLAN_CFI_SHIFT))
>> +
> 
> The patch looks good.
> 
> It seems HN_VLAN_TCI_ID, HN_VLAN_TCI_PRI, HN_VLAN_TCI_CFI and HN_VLAN_TCI_MAKE could be useful to other drivers. (at least to MANA)
> 
> Ferruh, do you think we should define those common functions in ./lib/net/rte_ether.h?
> 

Hi Long,

That is good idea indeed, so others can benefit from them. Thanks.

@Alan, can you please move above macros to './lib/net/rte_ether.h', with
RTE_VLAN_ prefix? And use them from net library in the driver.

Btw, CFI seems renamed to DEI (Drop Eligible Indicator), perhaps can be
good to go with that acronym.

[PATCH v5] net/netvsc: fix parsing of VLAN metadata

[Alan Elder]

The previous code incorrectly parsed the VLAN ID and priority.
If the 16-bits of VLAN ID and priority/CFI on the wire was
0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
were macros defined to handle this conversion but they were not
used.

Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
Cc: sthemmin@microsoft.com
Cc: stable@dpdk.org

Signed-off-by: Alan Elder <alan.elder@microsoft.com>
---
v5:
* Move the VLAN parsing macros to rte_ether.h

v4:
* Make consistent with FreeBSD code

---
 .mailmap                     |  1 +
 drivers/net/netvsc/hn_rxtx.c |  8 ++++++--
 lib/net/rte_ether.h          | 16 ++++++++++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/.mailmap b/.mailmap
index a0756974e2..eca02318d6 100644
--- a/.mailmap
+++ b/.mailmap
@@ -33,6 +33,7 @@ Alain Leon <xerebz@gmail.com>
 Alan Brady <alan.brady@intel.com>
 Alan Carew <alan.carew@intel.com>
 Alan Dewar <alan.dewar@att.com> <adewar@brocade.com>
+Alan Elder <alan.elder@microsoft.com>
 Alan Liu <zaoxingliu@gmail.com>
 Alan Winkowski <walan@marvell.com>
 Alejandro Lucero <alejandro.lucero@netronome.com>
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index e4f5015aa3..9bf1ec5509 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -612,7 +612,9 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 					   RTE_PTYPE_L4_MASK);
 
 	if (info->vlan_info != HN_NDIS_VLAN_INFO_INVALID) {
-		m->vlan_tci = info->vlan_info;
+		m->vlan_tci = RTE_VLAN_TCI_MAKE(NDIS_VLAN_INFO_ID(info->vlan_info),
+						NDIS_VLAN_INFO_PRI(info->vlan_info),
+						NDIS_VLAN_INFO_CFI(info->vlan_info));
 		m->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED | RTE_MBUF_F_RX_VLAN;
 
 		/* NDIS always strips tag, put it back if necessary */
@@ -1332,7 +1334,9 @@ static void hn_encap(struct rndis_packet_msg *pkt,
 	if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
 		pi_data = hn_rndis_pktinfo_append(pkt, NDIS_VLAN_INFO_SIZE,
 						  NDIS_PKTINFO_TYPE_VLAN);
-		*pi_data = m->vlan_tci;
+		*pi_data = NDIS_VLAN_INFO_MAKE(RTE_VLAN_TCI_ID(m->vlan_tci),
+					       RTE_VLAN_TCI_PRI(m->vlan_tci),
+					       RTE_VLAN_TCI_DEI(m->vlan_tci));
 	}
 
 	if (m->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index ce073ea818..b27f38b3a9 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -46,6 +46,22 @@ extern "C" {
 
 #define RTE_ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
 
+/* VLAN header fields */
+#define RTE_VLAN_DEI_SHIFT	12
+#define RTE_VLAN_PRI_SHIFT	13
+#define RTE_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
+#define RTE_VLAN_DEI_MASK	0x1000 /* Drop Eligible Indicator */
+#define RTE_VLAN_ID_MASK	0x0fff /* VLAN Identifier */
+
+#define RTE_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & RTE_VLAN_ID_MASK)
+#define RTE_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & RTE_VLAN_PRI_MASK) >>	\
+					 RTE_VLAN_PRI_SHIFT)
+#define RTE_VLAN_TCI_DEI(vlan_tci)	(((vlan_tci) & RTE_VLAN_DEI_MASK) >>	\
+					 RTE_VLAN_DEI_SHIFT)
+#define RTE_VLAN_TCI_MAKE(id, pri, dei)	((id) |					\
+					 ((pri) << RTE_VLAN_PRI_SHIFT) |	\
+					 ((dei) << RTE_VLAN_DEI_SHIFT))
+
 /**
  * Ethernet address:
  * A universally administered address is uniquely assigned to a device by its
-- 
2.25.1

Re: [PATCH v5] net/netvsc: fix parsing of VLAN metadata

[Stephen Hemminger]

On Thu, 15 Feb 2024 18:12:35 +0000
Alan Elder <alan.elder@microsoft.com> wrote:

> +/* VLAN header fields */
> +#define RTE_VLAN_DEI_SHIFT	12
> +#define RTE_VLAN_PRI_SHIFT	13
> +#define RTE_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
> +#define RTE_VLAN_DEI_MASK	0x1000 /* Drop Eligible Indicator */
> +#define RTE_VLAN_ID_MASK	0x0fff /* VLAN Identifier */
> +
> +#define RTE_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & RTE_VLAN_ID_MASK)
> +#define RTE_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & RTE_VLAN_PRI_MASK) >>	\
> +					 RTE_VLAN_PRI_SHIFT)
> +#define RTE_VLAN_TCI_DEI(vlan_tci)	(((vlan_tci) & RTE_VLAN_DEI_MASK) >>	\
> +					 RTE_VLAN_DEI_SHIFT)
> +#define RTE_VLAN_TCI_MAKE(id, pri, dei)	((id) |					\
> +					 ((pri) << RTE_VLAN_PRI_SHIFT) |	\
> +					 ((dei) << RTE_VLAN_DEI_SHIFT))
> +

With current DPDK standard max line length 100, the lines may not need to be broken any more.

[PATCH v6] net/netvsc: fix parsing of VLAN metadata

[Alan Elder]

The previous code incorrectly parsed the VLAN ID and priority.
If the 16-bits of VLAN ID and priority/CFI on the wire was
0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
were macros defined to handle this conversion but they were not
used.

Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
Cc: sthemmin@microsoft.com
Cc: stable@dpdk.org

Signed-off-by: Alan Elder <alan.elder@microsoft.com>
---
V6:
* Line length can be 100 - un-split lines

v5:
* Move the VLAN parsing macros to rte_ether.h

v4:
* Make consistent with FreeBSD code

---
 .mailmap                     |  1 +
 drivers/net/netvsc/hn_rxtx.c |  8 ++++++--
 lib/net/rte_ether.h          | 14 ++++++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/.mailmap b/.mailmap
index de339562f4..08fce0c472 100644
--- a/.mailmap
+++ b/.mailmap
@@ -33,6 +33,7 @@ Alain Leon <xerebz@gmail.com>
 Alan Brady <alan.brady@intel.com>
 Alan Carew <alan.carew@intel.com>
 Alan Dewar <alan.dewar@att.com> <adewar@brocade.com>
+Alan Elder <alan.elder@microsoft.com>
 Alan Liu <zaoxingliu@gmail.com>
 Alan Winkowski <walan@marvell.com>
 Alejandro Lucero <alejandro.lucero@netronome.com>
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index e4f5015aa3..9bf1ec5509 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -612,7 +612,9 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 					   RTE_PTYPE_L4_MASK);
 
 	if (info->vlan_info != HN_NDIS_VLAN_INFO_INVALID) {
-		m->vlan_tci = info->vlan_info;
+		m->vlan_tci = RTE_VLAN_TCI_MAKE(NDIS_VLAN_INFO_ID(info->vlan_info),
+						NDIS_VLAN_INFO_PRI(info->vlan_info),
+						NDIS_VLAN_INFO_CFI(info->vlan_info));
 		m->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED | RTE_MBUF_F_RX_VLAN;
 
 		/* NDIS always strips tag, put it back if necessary */
@@ -1332,7 +1334,9 @@ static void hn_encap(struct rndis_packet_msg *pkt,
 	if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
 		pi_data = hn_rndis_pktinfo_append(pkt, NDIS_VLAN_INFO_SIZE,
 						  NDIS_PKTINFO_TYPE_VLAN);
-		*pi_data = m->vlan_tci;
+		*pi_data = NDIS_VLAN_INFO_MAKE(RTE_VLAN_TCI_ID(m->vlan_tci),
+					       RTE_VLAN_TCI_PRI(m->vlan_tci),
+					       RTE_VLAN_TCI_DEI(m->vlan_tci));
 	}
 
 	if (m->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index ce073ea818..75285bdd12 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -46,6 +46,20 @@ extern "C" {
 
 #define RTE_ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
 
+/* VLAN header fields */
+#define RTE_VLAN_DEI_SHIFT	12
+#define RTE_VLAN_PRI_SHIFT	13
+#define RTE_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
+#define RTE_VLAN_DEI_MASK	0x1000 /* Drop Eligible Indicator */
+#define RTE_VLAN_ID_MASK	0x0fff /* VLAN Identifier */
+
+#define RTE_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & RTE_VLAN_ID_MASK)
+#define RTE_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & RTE_VLAN_PRI_MASK) >> RTE_VLAN_PRI_SHIFT)
+#define RTE_VLAN_TCI_DEI(vlan_tci)	(((vlan_tci) & RTE_VLAN_DEI_MASK) >> RTE_VLAN_DEI_SHIFT)
+#define RTE_VLAN_TCI_MAKE(id, pri, dei)	((id) |					\
+					 ((pri) << RTE_VLAN_PRI_SHIFT) |	\
+					 ((dei) << RTE_VLAN_DEI_SHIFT))
+
 /**
  * Ethernet address:
  * A universally administered address is uniquely assigned to a device by its
-- 
2.25.1

Re: [PATCH v6] net/netvsc: fix parsing of VLAN metadata

[Ferruh Yigit]

On 2/16/2024 9:43 AM, Alan Elder wrote:
> The previous code incorrectly parsed the VLAN ID and priority.
> If the 16-bits of VLAN ID and priority/CFI on the wire was
> 0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
> were macros defined to handle this conversion but they were not
> used.
> 
> Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
> Cc: sthemmin@microsoft.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alan Elder <alan.elder@microsoft.com>
>

I missed v6 but put some comment on v5, briefly can you please split the
patch for lib/net and driver?

[PATCH v7 0/2] fix parsing of VLAN metadata

[Alan Elder]

The previous netvsc code incorrectly parsed the VLAN ID and priority.
If the 16-bits of VLAN ID and priority/CFI on the wire was
0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  This patch
fixes netvsc parsing code and adds common macros for extracting and
setting parts of the VLAN tag.

Alan Elder (2):
  lib/net: fix parsing of VLAN metadata
  net/netvsc: fix parsing of VLAN metadata

---
v7:
* Split patches for lib and driver

v6:
* Line length can be 100 - un-split lines

v5:
* Move the VLAN parsing macros to rte_ether.h

v4:
* Make consistent with FreeBSD code

---
.mailmap                     |  1 +
 drivers/net/netvsc/hn_rxtx.c |  8 ++++++--
 lib/net/rte_ether.h          | 14 ++++++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v7 1/2] lib/net: fix parsing of VLAN metadata

[Alan Elder]

Add common macros for extracting parts of VLAN tag.

Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
Cc: sthemmin@microsoft.com
Cc: stable@dpdk.org

Signed-off-by: Alan Elder <alan.elder@microsoft.com>
---
v7:
* Split patches for lib and driver

v6:
* Line length can be 100 - un-split lines

v5:
* Move the VLAN parsing macros to rte_ether.h

v4:
* Make consistent with FreeBSD code

---
 .mailmap            |  1 +
 lib/net/rte_ether.h | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/.mailmap b/.mailmap
index de339562f4..08fce0c472 100644
--- a/.mailmap
+++ b/.mailmap
@@ -33,6 +33,7 @@ Alain Leon <xerebz@gmail.com>
 Alan Brady <alan.brady@intel.com>
 Alan Carew <alan.carew@intel.com>
 Alan Dewar <alan.dewar@att.com> <adewar@brocade.com>
+Alan Elder <alan.elder@microsoft.com>
 Alan Liu <zaoxingliu@gmail.com>
 Alan Winkowski <walan@marvell.com>
 Alejandro Lucero <alejandro.lucero@netronome.com>
diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index ce073ea818..75285bdd12 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -46,6 +46,20 @@ extern "C" {
 
 #define RTE_ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
 
+/* VLAN header fields */
+#define RTE_VLAN_DEI_SHIFT	12
+#define RTE_VLAN_PRI_SHIFT	13
+#define RTE_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
+#define RTE_VLAN_DEI_MASK	0x1000 /* Drop Eligible Indicator */
+#define RTE_VLAN_ID_MASK	0x0fff /* VLAN Identifier */
+
+#define RTE_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & RTE_VLAN_ID_MASK)
+#define RTE_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & RTE_VLAN_PRI_MASK) >> RTE_VLAN_PRI_SHIFT)
+#define RTE_VLAN_TCI_DEI(vlan_tci)	(((vlan_tci) & RTE_VLAN_DEI_MASK) >> RTE_VLAN_DEI_SHIFT)
+#define RTE_VLAN_TCI_MAKE(id, pri, dei)	((id) |					\
+					 ((pri) << RTE_VLAN_PRI_SHIFT) |	\
+					 ((dei) << RTE_VLAN_DEI_SHIFT))
+
 /**
  * Ethernet address:
  * A universally administered address is uniquely assigned to a device by its
-- 
2.25.1

Re: [PATCH v7 1/2] lib/net: fix parsing of VLAN metadata

[Ferruh Yigit]

On 2/19/2024 9:31 AM, Alan Elder wrote:
> Add common macros for extracting parts of VLAN tag.
> 
> Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alan Elder <alan.elder@microsoft.com>
>

Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>

Re: [PATCH v7 1/2] lib/net: fix parsing of VLAN metadata

[Ferruh Yigit]

On 2/19/2024 11:12 AM, Ferruh Yigit wrote:
> On 2/19/2024 9:31 AM, Alan Elder wrote:
>> Add common macros for extracting parts of VLAN tag.
>>
>> Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Alan Elder <alan.elder@microsoft.com>
>>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>
>

Series applied to dpdk-next-net/main, thanks.


Updated patch title as below while merging:
net: add helper macros for VLAN metadata parsing


Also kept fixes and stable tag, although patch itself is not fix, to
request backporting the patch and highlight the reason of the request.

[PATCH v7 2/2] net/netvsc: fix parsing of VLAN metadata

[Alan Elder]

The previous code incorrectly parsed the VLAN ID and priority.
If the 16-bits of VLAN ID and priority/CFI on the wire was
0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
were macros defined to handle this conversion but they were not
used.

Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
Cc: sthemmin@microsoft.com
Cc: stable@dpdk.org

Signed-off-by: Alan Elder <alan.elder@microsoft.com>
---
v7:
* Split into two patches, one for lib and one for driver

v6:
* Line length can be 100 - un-split lines

v5:
* Move the VLAN parsing macros to rte_ether.h

v4:
* Make consistent with FreeBSD code

---
 drivers/net/netvsc/hn_rxtx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index e4f5015aa3..9bf1ec5509 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -612,7 +612,9 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 					   RTE_PTYPE_L4_MASK);
 
 	if (info->vlan_info != HN_NDIS_VLAN_INFO_INVALID) {
-		m->vlan_tci = info->vlan_info;
+		m->vlan_tci = RTE_VLAN_TCI_MAKE(NDIS_VLAN_INFO_ID(info->vlan_info),
+						NDIS_VLAN_INFO_PRI(info->vlan_info),
+						NDIS_VLAN_INFO_CFI(info->vlan_info));
 		m->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED | RTE_MBUF_F_RX_VLAN;
 
 		/* NDIS always strips tag, put it back if necessary */
@@ -1332,7 +1334,9 @@ static void hn_encap(struct rndis_packet_msg *pkt,
 	if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
 		pi_data = hn_rndis_pktinfo_append(pkt, NDIS_VLAN_INFO_SIZE,
 						  NDIS_PKTINFO_TYPE_VLAN);
-		*pi_data = m->vlan_tci;
+		*pi_data = NDIS_VLAN_INFO_MAKE(RTE_VLAN_TCI_ID(m->vlan_tci),
+					       RTE_VLAN_TCI_PRI(m->vlan_tci),
+					       RTE_VLAN_TCI_DEI(m->vlan_tci));
 	}
 
 	if (m->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
-- 
2.25.1

Re: [PATCH v7 2/2] net/netvsc: fix parsing of VLAN metadata

[Ferruh Yigit]

On 2/19/2024 9:31 AM, Alan Elder wrote:
> The previous code incorrectly parsed the VLAN ID and priority.
> If the 16-bits of VLAN ID and priority/CFI on the wire was
> 0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
> were macros defined to handle this conversion but they were not
> used.
> 
> Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alan Elder <alan.elder@microsoft.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

RE: [EXTERNAL] Re: [PATCH v6] net/netvsc: fix parsing of VLAN metadata

[Alan Elder]

On 2/16/2024 11:40 AM, Ferruh Yigit:
> I missed v6 but put some comment on v5, briefly can you please split the patch for lib/net and driver?

I've tried to split the patch - hopefully got the formatting right - please let me know if not (it's my first time submitting a patch and I don't have all the tooling set up)

Re: [PATCH v5] net/netvsc: fix parsing of VLAN metadata

[Ferruh Yigit]

On 2/15/2024 6:12 PM, Alan Elder wrote:
> The previous code incorrectly parsed the VLAN ID and priority.
> If the 16-bits of VLAN ID and priority/CFI on the wire was
> 0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
> were macros defined to handle this conversion but they were not
> used.
> 
> Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
> Cc: sthemmin@microsoft.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alan Elder <alan.elder@microsoft.com>
>

Thanks Alan, overall looks good to me.

Can you please make separate the lib/net patch?
As first patch lib/net updates, second updates the net/vetvsc.

> ---
> v5:
> * Move the VLAN parsing macros to rte_ether.h
> 
> v4:
> * Make consistent with FreeBSD code
> 
> ---
>  .mailmap                     |  1 +
>  drivers/net/netvsc/hn_rxtx.c |  8 ++++++--
>  lib/net/rte_ether.h          | 16 ++++++++++++++++
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/.mailmap b/.mailmap
> index a0756974e2..eca02318d6 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -33,6 +33,7 @@ Alain Leon <xerebz@gmail.com>
>  Alan Brady <alan.brady@intel.com>
>  Alan Carew <alan.carew@intel.com>
>  Alan Dewar <alan.dewar@att.com> <adewar@brocade.com>
> +Alan Elder <alan.elder@microsoft.com>
>  Alan Liu <zaoxingliu@gmail.com>
>  Alan Winkowski <walan@marvell.com>
>  Alejandro Lucero <alejandro.lucero@netronome.com>
> diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
> index e4f5015aa3..9bf1ec5509 100644
> --- a/drivers/net/netvsc/hn_rxtx.c
> +++ b/drivers/net/netvsc/hn_rxtx.c
> @@ -612,7 +612,9 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
>  					   RTE_PTYPE_L4_MASK);
>  
>  	if (info->vlan_info != HN_NDIS_VLAN_INFO_INVALID) {
> -		m->vlan_tci = info->vlan_info;
> +		m->vlan_tci = RTE_VLAN_TCI_MAKE(NDIS_VLAN_INFO_ID(info->vlan_info),
> +						NDIS_VLAN_INFO_PRI(info->vlan_info),
> +						NDIS_VLAN_INFO_CFI(info->vlan_info));
>

Is there a good reason why format is not same as spec format?