[v5,1/3] app/testpmd: add GENEVE parsing
Checks
Commit Message
From: Ophir Munk <ophirmu@mellanox.com>
GENEVE is a widely used tunneling protocol in modern Virtualized
Networks. testpmd already supports parsing of several tunneling
protocols including VXLAN, VXLAN-GPE, GRE. This commit adds GENEVE
parsing of inner protocols (IPv4-0x0800, IPv6-0x86dd, Ethernet-0x6558)
based on IETF draft-ietf-nvo3-geneve-09. GENEVE is considered more
flexible than the other protocols. In terms of protocol format GENEVE
header has a variable length options as opposed to other tunneling
protocols which have a fixed header size.
Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
app/test-pmd/csumonly.c | 70 ++++++++++++++++++++++++++++++++++++++++++++-
app/test-pmd/testpmd.h | 1 +
lib/librte_net/meson.build | 3 +-
lib/librte_net/rte_geneve.h | 66 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 138 insertions(+), 2 deletions(-)
create mode 100644 lib/librte_net/rte_geneve.h
Comments
On 9/18/2020 3:17 PM, Ophir Munk wrote:
> From: Ophir Munk <ophirmu@mellanox.com>
>
> GENEVE is a widely used tunneling protocol in modern Virtualized
> Networks. testpmd already supports parsing of several tunneling
> protocols including VXLAN, VXLAN-GPE, GRE. This commit adds GENEVE
> parsing of inner protocols (IPv4-0x0800, IPv6-0x86dd, Ethernet-0x6558)
> based on IETF draft-ietf-nvo3-geneve-09. GENEVE is considered more
> flexible than the other protocols. In terms of protocol format GENEVE
> header has a variable length options as opposed to other tunneling
> protocols which have a fixed header size.
>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> app/test-pmd/csumonly.c | 70 ++++++++++++++++++++++++++++++++++++++++++++-
> app/test-pmd/testpmd.h | 1 +
> lib/librte_net/meson.build | 3 +-
> lib/librte_net/rte_geneve.h | 66 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 138 insertions(+), 2 deletions(-)
> create mode 100644 lib/librte_net/rte_geneve.h
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 7ece398..a9f33c6 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -43,6 +43,7 @@
> #include <rte_flow.h>
> #include <rte_gro.h>
> #include <rte_gso.h>
> +#include <rte_geneve.h>
>
> #include "testpmd.h"
>
> @@ -63,6 +64,7 @@
> #endif
>
> uint16_t vxlan_gpe_udp_port = 4790;
> +uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
>
There is a testpmd command to configure the NIC for GENEVE port,
"port config (port_id) udp_tunnel_port add|rm vxlan|geneve|vxlan-gpe (udp_port)"
You are adding support to testpmd to parse the GENEVE packets, but I think these
two commads should be consistent.
What do you think "port config N add geneve X" update the 'geneve_udp_port=X"?
<...>
> @@ -865,9 +925,17 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> }
> parse_vxlan(udp_hdr, &info,
> m->packet_type);
> - if (info.is_tunnel)
> + if (info.is_tunnel) {
> tx_ol_flags |=
> PKT_TX_TUNNEL_VXLAN;
> + goto tunnel_update;
> + }
> + parse_geneve(udp_hdr, &info);
> + if (info.is_tunnel) {
> + tx_ol_flags |=
> + PKT_TX_TUNNEL_GENEVE;
> + goto tunnel_update;
> + }
Can you please update the "csum parse-tunnel" documentation to mention "geneve"
protocol?
Hi Ferruh,
Please find comments inline.
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, October 6, 2020 5:31 PM
> To: Ophir Munk <ophirmu@nvidia.com>; dev@dpdk.org; Wenzhuo Lu
<...>
> >
> > uint16_t vxlan_gpe_udp_port = 4790;
> > +uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
> >
>
> There is a testpmd command to configure the NIC for GENEVE port, "port
> config (port_id) udp_tunnel_port add|rm vxlan|geneve|vxlan-gpe
> (udp_port)"
>
> You are adding support to testpmd to parse the GENEVE packets, but I think
> these two commads should be consistent.
>
> What do you think "port config N add geneve X" update the
> 'geneve_udp_port=X"?
>
> <...>
>
It is not as simple as suggested.
The command "port config N add geneve X" can be repeated several times - adding another geneve port to the NIC Hardware/Firmware to recognize. As a result the NIC is configured with multiple geneve ports. On the other hand geneve_udp_port is a single variable - so whenever a new "port config" command is executed - we will override the last geneve port and forget all the precedent ones.
The port config command also has a "rm" option in addition to "add". So if we removed the last port - what will we assigned to geneve_udp_port? We need to have a small data base for managing geneve ports.
testpmd also has an option " --geneve-port=N". It should be synched with the "port config" command.
Another issue to consider is if there is a need for the suggested feature. I think the "port config" is mainly targeted for offloading. So the NIC will probably encap/decap a packet with geneve - leaving testpmd paring unneeded.
My point is that your suggestion requires an RFC before implementation.
>
> > @@ -865,9 +925,17 @@ pkt_burst_checksum_forward(struct fwd_stream
> *fs)
> > }
> > parse_vxlan(udp_hdr, &info,
> > m->packet_type);
> > - if (info.is_tunnel)
> > + if (info.is_tunnel) {
> > tx_ol_flags |=
> > PKT_TX_TUNNEL_VXLAN;
> > + goto tunnel_update;
> > + }
> > + parse_geneve(udp_hdr, &info);
> > + if (info.is_tunnel) {
> > + tx_ol_flags |=
> > + PKT_TX_TUNNEL_GENEVE;
> > + goto tunnel_update;
> > + }
>
> Can you please update the "csum parse-tunnel" documentation to mention
> "geneve"
> protocol?
Done in v6. I added other missing tunnel protocols (in alphabetical order) such as "gtp". Since it is more than geneve I added it to the 3rd (cleanup) commit.
On 10/7/2020 3:52 PM, Ophir Munk wrote:
> Hi Ferruh,
> Please find comments inline.
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, October 6, 2020 5:31 PM
>> To: Ophir Munk <ophirmu@nvidia.com>; dev@dpdk.org; Wenzhuo Lu
> <...>
>>>
>>> uint16_t vxlan_gpe_udp_port = 4790;
>>> +uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
>>>
>>
>> There is a testpmd command to configure the NIC for GENEVE port, "port
>> config (port_id) udp_tunnel_port add|rm vxlan|geneve|vxlan-gpe
>> (udp_port)"
>>
>> You are adding support to testpmd to parse the GENEVE packets, but I think
>> these two commads should be consistent.
>>
>> What do you think "port config N add geneve X" update the
>> 'geneve_udp_port=X"?
>>
>> <...>
>>
>
> It is not as simple as suggested.
> The command "port config N add geneve X" can be repeated several times - adding another geneve port to the NIC Hardware/Firmware to recognize. As a result the NIC is configured with multiple geneve ports. On the other hand geneve_udp_port is a single variable - so whenever a new "port config" command is executed - we will override the last geneve port and forget all the precedent ones.
> The port config command also has a "rm" option in addition to "add". So if we removed the last port - what will we assigned to geneve_udp_port? We need to have a small data base for managing geneve ports.
> testpmd also has an option " --geneve-port=N". It should be synched with the "port config" command.
>
> Another issue to consider is if there is a need for the suggested feature. I think the "port config" is mainly targeted for offloading. So the NIC will probably encap/decap a packet with geneve - leaving testpmd paring unneeded.
>
> My point is that your suggestion requires an RFC before implementation.
>
I am not sure about adding database for geneve ports, I think no need to make it
more complex.
Only when user set via "--geneve-port=N", it is the port for testpmd to parse
(same for 'geneve_udp_port' global variable), but when user give command "port
config N add geneve X" it is to configure the NIC to offload the parsing.
This is too confusing, user can't know (or remember) this without checking the
source code.
Can we rename the command and variable to highlight that it is for testpmd to
parse, I think that will be enough, instead of trying to merge them, which is
hard as you described above.
What do you think for "--testpmd-geneve-port=N" parameter and
'testpmd_geneve_udp_port' variable name?
We can also update the documentation to say this is only the port testpmd uses
for parsing, HW may be configured to use another port.
>>
>>> @@ -865,9 +925,17 @@ pkt_burst_checksum_forward(struct fwd_stream
>> *fs)
>>> }
>>> parse_vxlan(udp_hdr, &info,
>>> m->packet_type);
>>> - if (info.is_tunnel)
>>> + if (info.is_tunnel) {
>>> tx_ol_flags |=
>>> PKT_TX_TUNNEL_VXLAN;
>>> + goto tunnel_update;
>>> + }
>>> + parse_geneve(udp_hdr, &info);
>>> + if (info.is_tunnel) {
>>> + tx_ol_flags |=
>>> + PKT_TX_TUNNEL_GENEVE;
>>> + goto tunnel_update;
>>> + }
>>
>> Can you please update the "csum parse-tunnel" documentation to mention
>> "geneve"
>> protocol?
>
> Done in v6. I added other missing tunnel protocols (in alphabetical order) such as "gtp". Since it is more than geneve I added it to the 3rd (cleanup) commit.
>
Would you mind adding the 'geneve' with the patch adding 'geneve' support (1/3),
and add the other missing ones in the patch 3/3 ?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 7, 2020 7:25 PM
> To: Ophir Munk <ophirmu@nvidia.com>; dev@dpdk.org; Wenzhuo Lu
<...>
> Only when user set via "--geneve-port=N", it is the port for testpmd to parse
> (same for 'geneve_udp_port' global variable), but when user give command
> "port config N add geneve X" it is to configure the NIC to offload the parsing.
> This is too confusing, user can't know (or remember) this without checking
> the source code.
>
> Can we rename the command and variable to highlight that it is for testpmd
> to parse, I think that will be enough, instead of trying to merge them, which
> is hard as you described above.
>
> What do you think for "--testpmd-geneve-port=N" parameter and
> 'testpmd_geneve_udp_port' variable name?
I am suggesting two options:
1. "--geneve-port-identifier=N" and "geneve_udp_port_identifier" as variable name.
2. "--geneve-parsed-port=N" and "geneve_udp_port" as variable name.
Can you select one of them?
> We can also update the documentation to say this is only the port testpmd
> uses for parsing, HW may be configured to use another port.
>
Will update documentation.
<...>
> > Done in v6. I added other missing tunnel protocols (in alphabetical order)
> such as "gtp". Since it is more than geneve I added it to the 3rd (cleanup)
> commit.
> >
>
> Would you mind adding the 'geneve' with the patch adding 'geneve' support
> (1/3), and add the other missing ones in the patch 3/3 ?
Will update accordingly
On 10/8/2020 9:44 AM, Ophir Munk wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, October 7, 2020 7:25 PM
>> To: Ophir Munk <ophirmu@nvidia.com>; dev@dpdk.org; Wenzhuo Lu
> <...>
>> Only when user set via "--geneve-port=N", it is the port for testpmd to parse
>> (same for 'geneve_udp_port' global variable), but when user give command
>> "port config N add geneve X" it is to configure the NIC to offload the parsing.
>> This is too confusing, user can't know (or remember) this without checking
>> the source code.
>>
>> Can we rename the command and variable to highlight that it is for testpmd
>> to parse, I think that will be enough, instead of trying to merge them, which
>> is hard as you described above.
>>
>> What do you think for "--testpmd-geneve-port=N" parameter and
>> 'testpmd_geneve_udp_port' variable name?
>
> I am suggesting two options:
> 1. "--geneve-port-identifier=N" and "geneve_udp_port_identifier" as variable name.
> 2. "--geneve-parsed-port=N" and "geneve_udp_port" as variable name.
> Can you select one of them?
>
I think 'identifier' doesn't add any more clarification, lets go with (2). Thanks.
>> We can also update the documentation to say this is only the port testpmd
>> uses for parsing, HW may be configured to use another port.
>>
>
> Will update documentation.
>
> <...>
>>> Done in v6. I added other missing tunnel protocols (in alphabetical order)
>> such as "gtp". Since it is more than geneve I added it to the 3rd (cleanup)
>> commit.
>>>
>>
>> Would you mind adding the 'geneve' with the patch adding 'geneve' support
>> (1/3), and add the other missing ones in the patch 3/3 ?
>
> Will update accordingly
>
@@ -43,6 +43,7 @@
#include <rte_flow.h>
#include <rte_gro.h>
#include <rte_gso.h>
+#include <rte_geneve.h>
#include "testpmd.h"
@@ -63,6 +64,7 @@
#endif
uint16_t vxlan_gpe_udp_port = 4790;
+uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
/* structure that caches offload info for the current packet */
struct testpmd_offload_info {
@@ -333,6 +335,64 @@ parse_vxlan_gpe(struct rte_udp_hdr *udp_hdr,
info->l2_len += RTE_ETHER_VXLAN_GPE_HLEN;
}
+/* Fill in outer layers length */
+static void
+update_tunnel_outer(struct testpmd_offload_info *info)
+{
+ info->is_tunnel = 1;
+ info->outer_ethertype = info->ethertype;
+ info->outer_l2_len = info->l2_len;
+ info->outer_l3_len = info->l3_len;
+ info->outer_l4_proto = info->l4_proto;
+}
+
+/* Parse a geneve header */
+static void
+parse_geneve(struct rte_udp_hdr *udp_hdr,
+ struct testpmd_offload_info *info)
+{
+ struct rte_ether_hdr *eth_hdr;
+ struct rte_ipv4_hdr *ipv4_hdr;
+ struct rte_ipv6_hdr *ipv6_hdr;
+ struct rte_geneve_hdr *geneve_hdr;
+ uint16_t geneve_len;
+
+ /* Check udp destination port. */
+ if (udp_hdr->dst_port != _htons(geneve_udp_port))
+ return;
+
+ geneve_hdr = (struct rte_geneve_hdr *)((char *)udp_hdr +
+ sizeof(struct rte_udp_hdr));
+ geneve_len = sizeof(struct rte_geneve_hdr) + geneve_hdr->opt_len * 4;
+ if (!geneve_hdr->proto || geneve_hdr->proto ==
+ _htons(RTE_ETHER_TYPE_IPV4)) {
+ update_tunnel_outer(info);
+ ipv4_hdr = (struct rte_ipv4_hdr *)((char *)geneve_hdr +
+ geneve_len);
+ parse_ipv4(ipv4_hdr, info);
+ info->ethertype = _htons(RTE_ETHER_TYPE_IPV4);
+ info->l2_len = 0;
+ } else if (geneve_hdr->proto == _htons(RTE_ETHER_TYPE_IPV6)) {
+ update_tunnel_outer(info);
+ ipv6_hdr = (struct rte_ipv6_hdr *)((char *)geneve_hdr +
+ geneve_len);
+ info->ethertype = _htons(RTE_ETHER_TYPE_IPV6);
+ parse_ipv6(ipv6_hdr, info);
+ info->l2_len = 0;
+
+ } else if (geneve_hdr->proto == _htons(RTE_GENEVE_TYPE_ETH)) {
+ update_tunnel_outer(info);
+ eth_hdr = (struct rte_ether_hdr *)((char *)geneve_hdr +
+ geneve_len);
+ parse_ethernet(eth_hdr, info);
+ } else
+ return;
+
+ info->l2_len +=
+ (sizeof(struct rte_udp_hdr) + sizeof(struct rte_geneve_hdr) +
+ ((struct rte_geneve_hdr *)geneve_hdr)->opt_len * 4);
+}
+
/* Parse a gre header */
static void
parse_gre(struct simple_gre_hdr *gre_hdr, struct testpmd_offload_info *info)
@@ -865,9 +925,17 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
}
parse_vxlan(udp_hdr, &info,
m->packet_type);
- if (info.is_tunnel)
+ if (info.is_tunnel) {
tx_ol_flags |=
PKT_TX_TUNNEL_VXLAN;
+ goto tunnel_update;
+ }
+ parse_geneve(udp_hdr, &info);
+ if (info.is_tunnel) {
+ tx_ol_flags |=
+ PKT_TX_TUNNEL_GENEVE;
+ goto tunnel_update;
+ }
} else if (info.l4_proto == IPPROTO_GRE) {
struct simple_gre_hdr *gre_hdr;
@@ -452,6 +452,7 @@ extern struct fwd_lcore **fwd_lcores;
extern struct fwd_stream **fwd_streams;
extern uint16_t vxlan_gpe_udp_port; /**< UDP port of tunnel VXLAN-GPE. */
+extern uint16_t geneve_udp_port; /**< UDP port of tunnel GENEVE. */
extern portid_t nb_peer_eth_addrs; /**< Number of peer ethernet addresses. */
extern struct rte_ether_addr peer_eth_addrs[RTE_MAX_ETHPORTS];
@@ -16,7 +16,8 @@ headers = files('rte_ip.h',
'rte_net_crc.h',
'rte_mpls.h',
'rte_higig.h',
- 'rte_ecpri.h')
+ 'rte_ecpri.h',
+ 'rte_geneve.h')
sources = files('rte_arp.c', 'rte_ether.c', 'rte_net.c', 'rte_net_crc.c')
deps += ['mbuf']
new file mode 100644
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2020 Mellanox Technologies, Ltd
+ */
+
+#ifndef _RTE_GENEVE_H_
+#define _RTE_GENEVE_H_
+
+/**
+ * @file
+ *
+ * GENEVE-related definitions
+ */
+#include <stdint.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/** GENEVE default port. */
+#define RTE_GENEVE_DEFAULT_PORT 6081
+
+/**
+ * GENEVE protocol header. (draft-ietf-nvo3-geneve-09)
+ * Contains:
+ * 2-bits version (must be 0).
+ * 6-bits option length in four byte multiples, not including the eight
+ * bytes of the fixed tunnel header.
+ * 1-bit control packet.
+ * 1-bit critical options in packet.
+ * 6-bits reserved
+ * 16-bits Protocol Type. The protocol data unit after the Geneve header
+ * following the EtherType convention. Ethernet itself is represented by
+ * the value 0x6558.
+ * 24-bits Virtual Network Identifier (VNI). Virtual network unique identified.
+ * 8-bits reserved bits (must be 0 on transmission and ignored on receipt).
+ * More-bits (optional) variable length options.
+ */
+__extension__
+struct rte_geneve_hdr {
+#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+ uint8_t ver:2; /**< Version. */
+ uint8_t opt_len:6; /**< Options length. */
+ uint8_t oam:1; /**< Control packet. */
+ uint8_t critical:1; /**< Critical packet. */
+ uint8_t reserved1:6; /**< Reserved. */
+#else
+ uint8_t opt_len:6; /**< Options length. */
+ uint8_t ver:2; /**< Version. */
+ uint8_t reserved1:6; /**< Reserved. */
+ uint8_t critical:1; /**< Critical packet. */
+ uint8_t oam:1; /**< Control packet. */
+#endif
+ rte_be16_t proto; /**< Protocol type. */
+ uint8_t vni[3]; /**< Virtual network identifier. */
+ uint8_t reserved2; /**< Reserved. */
+ uint32_t opts[]; /**< Variable length options. */
+} __rte_packed;
+
+/* GENEVE ETH next protocol types */
+#define RTE_GENEVE_TYPE_ETH 0x6558 /**< Ethernet Protocol. */
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_GENEVE_H_ */