* [PATCH] net/netvsc: fix parsing of VLAN metadata
@ 2024-02-08 14:12 Alan Elder
2024-02-08 14:42 ` [PATCH v2] " Alan Elder
0 siblings, 1 reply; 5+ messages in thread
From: Alan Elder @ 2024-02-08 14:12 UTC (permalink / raw)
To: Long Li; +Cc: dev
[-- Attachment #1: Type: text/plain, Size: 5704 bytes --]
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<mailto:sthemmin@microsoft.com>
Cc: stable@dpdk.org<mailto:stable@dpdk.org>
Signed-off-by: Alan Elder alan.elder@microsoft.com<mailto: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 864d33ee46..0fde0fa509 100644
--- a/.mailmap
+++ b/.mailmap
@@ -32,6 +32,7 @@ Akihiko Odaki akihiko.odaki@daynix.com<mailto:akihiko.odaki@daynix.com>
Alain Leon xerebz@gmail.com<mailto:xerebz@gmail.com>
Alan Carew alan.carew@intel.com<mailto:alan.carew@intel.com>
Alan Dewar alan.dewar@att.com<mailto:alan.dewar@att.com> adewar@brocade.com<mailto:adewar@brocade.com>
+Alan Elder alan.elder@microsoft.com<mailto:alan.elder@microsoft.com>
Alan Liu zaoxingliu@gmail.com<mailto:zaoxingliu@gmail.com>
Alan Winkowski walan@marvell.com<mailto:walan@marvell.com>
Alejandro Lucero alejandro.lucero@netronome.com<mailto:alejandro.lucero@netronome.com>
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index bc6f60c64a..949780abd5 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
[-- Attachment #2: Type: text/html, Size: 21376 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] net/netvsc: fix parsing of VLAN metadata
2024-02-08 14:12 [PATCH] net/netvsc: fix parsing of VLAN metadata Alan Elder
@ 2024-02-08 14:42 ` Alan Elder
2024-02-08 17:54 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Alan Elder @ 2024-02-08 14:42 UTC (permalink / raw)
To: Long Li; +Cc: dev
[-- Attachment #1: Type: text/plain, Size: 6960 bytes --]
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
[-- Attachment #2: Type: text/html, Size: 24040 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] net/netvsc: fix parsing of VLAN metadata
2024-02-08 14:42 ` [PATCH v2] " Alan Elder
@ 2024-02-08 17:54 ` Stephen Hemminger
2024-02-09 1:13 ` Long Li
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2024-02-08 17:54 UTC (permalink / raw)
To: Alan Elder; +Cc: Long Li, dev
On Thu, 8 Feb 2024 14:42:44 +0000
Alan Elder <alan.elder@microsoft.com> wrote:
> + 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);
Lots of extra parenthesis here, please remove them.
> +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;
> + };
> +};
Order of union elements is byte order sensitive, granted Hyper-V/Azure doesn't do big-endian.
Most of this driver came from FreeBSD. Did you look there to make sure naming is the
same. I.e would be good to be able to have same code in both places as much as possible.
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v2] net/netvsc: fix parsing of VLAN metadata
2024-02-08 17:54 ` Stephen Hemminger
@ 2024-02-09 1:13 ` Long Li
2024-02-09 15:40 ` Alan Elder
0 siblings, 1 reply; 5+ messages in thread
From: Long Li @ 2024-02-09 1:13 UTC (permalink / raw)
To: stephen, Alan Elder; +Cc: dev
> > +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;
> > + };
> > +};
>
> Order of union elements is byte order sensitive, granted Hyper-V/Azure doesn't do
> big-endian.
Hyper-V/Azure uses the same endian for host and guest. They are all in little endian on X86 and ARM. If it uses big endian, this bug will not show up.
>
> Most of this driver came from FreeBSD. Did you look there to make sure naming is
> the same. I.e would be good to be able to have same code in both places as much
> as possible.
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v2] net/netvsc: fix parsing of VLAN metadata
2024-02-09 1:13 ` Long Li
@ 2024-02-09 15:40 ` Alan Elder
0 siblings, 0 replies; 5+ messages in thread
From: Alan Elder @ 2024-02-09 15:40 UTC (permalink / raw)
To: Long Li, stephen; +Cc: dev
-----Original Message-----
From: Long Li <longli@microsoft.com>
Sent: Friday, February 9, 2024 1:14 AM
To: stephen <stephen@networkplumber.org>; Alan Elder <alan.elder@microsoft.com>
Cc: dev@dpdk.org
Subject: RE: [PATCH v2] net/netvsc: fix parsing of VLAN metadata
>
> Most of this driver came from FreeBSD. Did you look there to make sure
> naming is the same. I.e would be good to be able to have same code in
> both places as much as possible.
I hadn't looked, but have now - the FreeBSD code does not suffer from the same issue as it uses a series of MACROs to convert between formats.
Agree it seems sensible to keep the same code structure, so I will update the patch to follow the same style as FreeBSD code.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-09 15:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-08 14:12 [PATCH] net/netvsc: fix parsing of VLAN metadata Alan Elder
2024-02-08 14:42 ` [PATCH v2] " Alan Elder
2024-02-08 17:54 ` Stephen Hemminger
2024-02-09 1:13 ` Long Li
2024-02-09 15:40 ` Alan Elder
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).