Re: [dpdk-dev] [PATCH v7 1/3] ethdev: support L2TPv2 and PPP procotol

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/21/2021 8:50 AM, Ori Kam wrote:
> Hi Jie,
> 
>> -----Original Message-----
>> From: Jie Wang <jie1x.wang@intel.com>
>> Sent: Thursday, October 21, 2021 9:26 AM
>> Subject: [PATCH v7 1/3] ethdev: support L2TPv2 and PPP procotol
>>
>> Added flow pattern items and header formats of L2TPv2 and PPP.
>>
>> Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
>> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
>> ---
>>   doc/api/doxy-api-index.md              |   2 +
>>   doc/guides/nics/features/default.ini   |   2 +
>>   doc/guides/nics/features/iavf.ini      |   2 +
>>   doc/guides/prog_guide/rte_flow.rst     |  25 +++
>>   doc/guides/rel_notes/release_21_11.rst |   4 +
>>   lib/ethdev/rte_flow.c                  |   2 +
>>   lib/ethdev/rte_flow.h                  |  65 +++++++
>>   lib/net/meson.build                    |   2 +
>>   lib/net/rte_l2tpv2.h                   | 234 +++++++++++++++++++++++++
>>   lib/net/rte_ppp.h                      |  34 ++++
>>   10 files changed, 372 insertions(+)
>>   create mode 100644 lib/net/rte_l2tpv2.h
>>   create mode 100644 lib/net/rte_ppp.h
>>
>> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
>> index 1992107a03..42db196afe 100644
>> --- a/doc/api/doxy-api-index.md
>> +++ b/doc/api/doxy-api-index.md
>> @@ -121,6 +121,8 @@ The public API headers are grouped by topics:
>>     [VXLAN]              (@ref rte_vxlan.h),
>>     [Geneve]             (@ref rte_geneve.h),
>>     [eCPRI]              (@ref rte_ecpri.h)
>> +  [L2TPv2]             (@ref rte_l2tpv2.h)
>> +  [PPP]                (@ref rte_ppp.h)
>>
>>   - **QoS**:
>>     [metering]           (@ref rte_meter.h),
>> diff --git a/doc/guides/nics/features/default.ini b/doc/guides/nics/features/default.ini
>> index 09914b1ad3..8e6a28c419 100644
>> --- a/doc/guides/nics/features/default.ini
>> +++ b/doc/guides/nics/features/default.ini
>> @@ -110,6 +110,7 @@ ipv4                 =
>>   ipv6                 =
>>   ipv6_ext             =
>>   ipv6_frag_ext        =
>> +l2tpv2               =
>>   l2tpv3oip            =
>>   mark                 =
>>   meta                 =
>> @@ -121,6 +122,7 @@ pfcp                 =
>>   phy_port             =
>>   port_id              =
>>   port_representor     =
>> +ppp                  =
>>   pppoed               =
>>   pppoes               =
>>   pppoe_proto_id       =
>> diff --git a/doc/guides/nics/features/iavf.ini b/doc/guides/nics/features/iavf.ini
>> index d00ca934c3..a916275b88 100644
>> --- a/doc/guides/nics/features/iavf.ini
>> +++ b/doc/guides/nics/features/iavf.ini
>> @@ -50,8 +50,10 @@ icmp6                = Y
>>   ipv4                 = Y
>>   ipv6                 = Y
>>   ipv6_frag_ext        = Y
>> +l2tpv2               = Y
>>   l2tpv3oip            = Y
>>   pfcp                 = Y
>> +ppp                  = Y
>>   sctp                 = Y
>>   tcp                  = Y
>>   udp                  = Y
>> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
>> index aeba374182..a2169517c3 100644
>> --- a/doc/guides/prog_guide/rte_flow.rst
>> +++ b/doc/guides/prog_guide/rte_flow.rst
>> @@ -1573,6 +1573,31 @@ rte_flow_flex_item_create() routine.
>>     as padded with trailing zeroes up to full configured length, both for
>>     value and mask.
>>
>> +Item: ``L2TPV2``
>> +^^^^^^^^^^^^^^^^^^^
>> +
>> +Matches a L2TPv2 header.
>> +
>> +- ``flags_version``: flags(12b), version(4b).
>> +- ``length``: total length of the message.
>> +- ``tunnel_id``: identifier for the control connection.
>> +- ``session_id``: identifier for a session within a tunnel.
>> +- ``ns``: sequence number for this date or control message.
>> +- ``nr``: sequence number expected in the next control message to be received.
>> +- ``offset_size``: offset of payload data.
>> +- ``offset_padding``: offset padding, variable length.
>> +- Default ``mask`` matches flags_version only.
>> +
>> +Item: ``PPP``
>> +^^^^^^^^^^^^^^^^^^^
>> +
>> +Matches a PPP header.
>> +
>> +- ``addr``: PPP address.
>> +- ``ctrl``: PPP control.
>> +- ``proto_id``: PPP protocol identifier.
>> +- Default ``mask`` matches addr, ctrl, proto_id.
>> +
>>   Actions
>>   ~~~~~~~
>>
>> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
>> index 041383ee2a..283770131c 100644
>> --- a/doc/guides/rel_notes/release_21_11.rst
>> +++ b/doc/guides/rel_notes/release_21_11.rst
>> @@ -105,6 +105,10 @@ New Features
>>
>>     Added an ethdev API which can help users get device configuration.
>>
>> +* **Added L2TPv2 and PPP protocol support in rte_flow.**
>> +
>> +  Added flow pattern items and header formats of L2TPv2 and PPP protocol.
>> +
>>   * **Updated AF_XDP PMD.**
>>
>>     * Disabled secondary process support.
>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>> index bcf0513b3c..d268784532 100644
>> --- a/lib/ethdev/rte_flow.c
>> +++ b/lib/ethdev/rte_flow.c
>> @@ -156,6 +156,8 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
>>   	MK_FLOW_ITEM(REPRESENTED_PORT, sizeof(struct rte_flow_item_ethdev)),
>>   	MK_FLOW_ITEM_FN(FLEX, sizeof(struct rte_flow_item_flex),
>>   			rte_flow_item_flex_conv),
>> +	MK_FLOW_ITEM(L2TPV2, sizeof(struct rte_flow_item_l2tpv2)),
>> +	MK_FLOW_ITEM(PPP, sizeof(struct rte_flow_item_ppp)),
>>   };
>>
>>   /** Generate flow_action[] entry. */
>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>> index 64ed7f2618..300e99e16b 100644
>> --- a/lib/ethdev/rte_flow.h
>> +++ b/lib/ethdev/rte_flow.h
>> @@ -35,6 +35,8 @@
>>   #include <rte_mbuf_dyn.h>
>>   #include <rte_meter.h>
>>   #include <rte_gtp.h>
>> +#include <rte_l2tpv2.h>
>> +#include <rte_ppp.h>
>>
>>   #ifdef __cplusplus
>>   extern "C" {
>> @@ -644,6 +646,20 @@ enum rte_flow_item_type {
>>   	 * @see struct rte_flow_item_flex.
>>   	 */
>>   	RTE_FLOW_ITEM_TYPE_FLEX,
>> +
>> +	/**
>> +	 * Matches L2TPv2 Header.
>> +	 *
>> +	 * See struct rte_flow_item_l2tpv2.
>> +	 */
>> +	RTE_FLOW_ITEM_TYPE_L2TPV2,
>> +
>> +	/**
>> +	 * Matches PPP Header.
>> +	 *
>> +	 * See struct rte_flow_item_ppp.
>> +	 */
>> +	RTE_FLOW_ITEM_TYPE_PPP,
>>   };
>>
>>   /**
>> @@ -1900,6 +1916,55 @@ static const struct rte_flow_item_ethdev rte_flow_item_ethdev_mask = {
>>   };
>>   #endif
>>
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this structure may change without prior notice
>> + * RTE_FLOW_ITEM_TYPE_L2TPV2
>> + *
>> + * Matches L2TPv2 Header
>> + */
>> +struct rte_flow_item_l2tpv2 {
>> +	struct rte_l2tpv2_combined_msg_hdr hdr;
>> +};
>> +
>> +/** Default mask for RTE_FLOW_ITEM_TYPE_L2TPV2. */
>> +#ifndef __cplusplus
>> +static const struct rte_flow_item_l2tpv2 rte_flow_item_l2tpv2_mask = {
>> +	/*
>> +	 * flags and version bit mask
>> +	 * 7 6 5 4 3 2 1 0 7 6 5 4 3 2 1 0
>> +	 * T L x x S x O P x x x x V V V V
>> +	 */
>> +	.hdr = {
>> +		.common = {
>> +			.flags_version = RTE_BE16(0xcb0f),
>> +		},
>> +	},
>> +};
>> +#endif
>> +
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this structure may change without prior notice
>> + * RTE_FLOW_ITEM_TYPE_PPP
>> + *
>> + * Matches PPP Header
>> + */
>> +struct rte_flow_item_ppp {
>> +	struct rte_ppp_hdr hdr;
>> +};
>> +
>> +/** Default mask for RTE_FLOW_ITEM_TYPE_PPP. */
>> +#ifndef __cplusplus
>> +static const struct rte_flow_item_ppp rte_flow_item_ppp_mask = {
>> +	.hdr = {
>> +		.addr = 0xff,
>> +		.ctrl = 0xff,
>> +		.proto_id = RTE_BE16(0xffff),
>> +	}
>> +};
>> +#endif
>> +
>>   /**
>>    * Matching pattern item definition.
>>    *
>> diff --git a/lib/net/meson.build b/lib/net/meson.build
>> index a4e395e9c5..e899846578 100644
>> --- a/lib/net/meson.build
>> +++ b/lib/net/meson.build
>> @@ -19,6 +19,8 @@ headers = files(
>>           'rte_higig.h',
>>           'rte_ecpri.h',
>>           'rte_geneve.h',
>> +        'rte_l2tpv2.h',
>> +        'rte_ppp.h',
>>   )
>>
>>   sources = files(
>> diff --git a/lib/net/rte_l2tpv2.h b/lib/net/rte_l2tpv2.h
>> new file mode 100644
>> index 0000000000..18ea759ff4
>> --- /dev/null
>> +++ b/lib/net/rte_l2tpv2.h
>> @@ -0,0 +1,234 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright 2021 Mellanox Technologies, Ltd
>> + */
>> +
>> +#ifndef _RTE_L2TPV2_H_
>> +#define _RTE_L2TPV2_H_
>> +
>> +/**
>> + * @file
>> + *
>> + * L2TP header:
>> + *
>> + * `-0--------------------1----------------2-------------------3`
>> + *
>> + * `-0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1`
>> + *
>> + * `+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+`
>> + *
>> + * `|T|L|x|x|S|x|O|P|x|x|x|x|--Ver--|-----------Length (opt)--------|`
>> + *
>> + * `+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+`
>> + *
>> + * `|-----------Tunnel ID-----------|-----------Session ID----------|`
>> + *
>> + * `+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+`
>> + *
>> + * `|-----------Ns (opt)------------|-----------Nr (opt)------------|`
>> + *
>> + * `+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+`
>> + *
>> + * `|---------Offset Size (opt)-----|---------Offset pad... (opt)`
>> + *
>> + * `+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+`
>> + *
>> + * The Type (T) bit indicates the type of message. It is set to 0 for a data
>> + * message and 1 for a control message.
>> + *
>> + * If the Length (L) bit is 1, the Length field is present. This bit MUST be
>> + * set to 1 for control messages.
>> + *
>> + * The x bits are reserved for future extensions. All reserved bits MUST
>> + * be set to 0 on outgoing messages and ignored on incoming messages.
>> + *
>> + * If the Sequence (S) bit is set to 1 the Ns and Nr fields are present.
>> + * The S bit MUST be set to 1 for control messages.
>> + *
>> + * If the Offset (O) bit is 1, the Offset Size field is present. The O
>> + * bit MUST be set to 0 for control messages.
>> + *
>> + * If the Priority (P) bit is 1, this data message should receive
>> + * preferential treatment in its local queuing and transmission.
>> + * The P bit MUST be set to 0 for control messages.
>> + *
>> + * Ver MUST be 2, indicating the version of the L2TP data message header.
>> + *
>> + * The Length field indicates the total length of the message in octets.
>> + *
>> + * Tunnel ID indicates the identifier for the control connection.
>> + *
>> + * Session ID indicates the identifier for a session within a tunnel.
>> + *
>> + * Ns indicates the sequence number for this data or control message.
>> + *
>> + * Nr indicates the sequence number expected in the next control message
>> + * to be received.
>> + *
>> + * The Offset Size field, if present, specifies the number of octets
>> + * past the L2TP header at which the payload data is expected to start.
>> + * Actual data within the offset padding is undefined. If the offset
>> + * field is present, the L2TP header ends after the last octet of the
>> + * offset padding.
>> + */
>> +
>> +#include <stdint.h>
>> +#include <rte_byteorder.h>
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +/**
>> + * L2TPv2 Common Header
>> + */
>> +RTE_STD_C11
>> +struct rte_l2tpv2_common_hdr {
>> +	union {
>> +		/** header flags and protocol version */
>> +		rte_be16_t flags_version;
>> +		struct {
>> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>> +			rte_be16_t t:1;		/**< message Type */
>> +			rte_be16_t l:1;		/**< length option bit */
>> +			rte_be16_t res1:2;	/**< reserved */
>> +			rte_be16_t s:1;		/**< ns/nr option bit */
>> +			rte_be16_t res2:1;	/**< reserved */
>> +			rte_be16_t o:1;		/**< offset option bit */
>> +			rte_be16_t p:1;		/**< priority option bit */
>> +			rte_be16_t res3:4;	/**< reserved */
>> +			rte_be16_t ver:4;	/**< protocol version */
>> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>> +			rte_be16_t ver:4;	/**< protocol version */
>> +			rte_be16_t res3:4;	/**< reserved */
>> +			rte_be16_t p:1;		/**< priority option bit */
>> +			rte_be16_t o:1;		/**< offset option bit */
>> +			rte_be16_t res2:1;	/**< reserved */
>> +			rte_be16_t s:1;		/**< ns/nr option bit */
>> +			rte_be16_t res1:2;	/**< reserved */
>> +			rte_be16_t l:1;		/**< length option bit */
>> +			rte_be16_t t:1;		/**< message Type */
>> +#endif
>> +		};
>> +	};
>> +};
>> +
>> +/**
>> + * L2TPv2 message Header contains all options(length, ns, nr,
>> + * offset size, offset padding).
>> + */
>> +struct rte_l2tpv2_msg_with_all_options {
>> +	rte_be16_t length;		/**< length(16) */
>> +	rte_be16_t tunnel_id;		/**< tunnel ID(16) */
>> +	rte_be16_t session_id;		/**< session ID(16) */
>> +	rte_be16_t ns;			/**< Ns(16) */
>> +	rte_be16_t nr;			/**< Nr(16) */
>> +	rte_be16_t offset_size;		/**< offset size(16) */
>> +	uint8_t   *offset_padding;	/**< offset padding(variable length) */
>> +};
>> +
>> +/**
>> + * L2TPv2 message Header contains all options except length(ns, nr,
>> + * offset size, offset padding).
>> + */
>> +struct rte_l2tpv2_msg_without_length {
>> +	rte_be16_t tunnel_id;		/**< tunnel ID(16) */
>> +	rte_be16_t session_id;		/**< session ID(16) */
>> +	rte_be16_t ns;			/**< Ns(16) */
>> +	rte_be16_t nr;			/**< Nr(16) */
>> +	rte_be16_t offset_size;		/**< offset size(16) */
>> +	uint8_t   *offset_padding;	/**< offset padding(variable length) */
>> +};
>> +
>> +/**
>> + * L2TPv2 message Header contains all options except ns_nr(length,
>> + * offset size, offset padding).
>> + * Ns and Nr MUST be toghter.
>> + */
>> +struct rte_l2tpv2_msg_without_ns_nr {
>> +	rte_be16_t length;		/**< length(16) */
>> +	rte_be16_t tunnel_id;		/**< tunnel ID(16) */
>> +	rte_be16_t session_id;		/**< session ID(16) */
>> +	rte_be16_t offset_size;		/**< offset size(16) */
>> +	uint8_t   *offset_padding;	/**< offset padding(variable length) */
>> +};
>> +
>> +/**
>> + * L2TPv2 message Header contains all options except ns_nr(length, ns, nr).
>> + * offset size and offset padding MUST be toghter.
>> + */
>> +struct rte_l2tpv2_msg_without_offset {
>> +	rte_be16_t length;		/**< length(16) */
>> +	rte_be16_t tunnel_id;		/**< tunnel ID(16) */
>> +	rte_be16_t session_id;		/**< session ID(16) */
>> +	rte_be16_t ns;			/**< Ns(16) */
>> +	rte_be16_t nr;			/**< Nr(16) */
>> +};
> 
> Why not packed?
> 
>> +
>> +/**
>> + * L2TPv2 message Header contains options offset size and offset padding.
>> + */
>> +struct rte_l2tpv2_msg_with_offset {
>> +	rte_be16_t tunnel_id;		/**< tunnel ID(16) */
>> +	rte_be16_t session_id;		/**< session ID(16) */
>> +	rte_be16_t offset_size;		/**< offset size(16) */
>> +	uint8_t   *offset_padding;	/**< offset padding(variable length) */
>> +};
>> +
>> +/**
>> + * L2TPv2 message Header contains options ns and nr.
>> + */
>> +struct rte_l2tpv2_msg_with_ns_nr {
>> +	rte_be16_t tunnel_id;		/**< tunnel ID(16) */
>> +	rte_be16_t session_id;		/**< session ID(16) */
>> +	rte_be16_t ns;			/**< Ns(16) */
>> +	rte_be16_t nr;			/**< Nr(16) */
>> +};
> 
> Why not packed? Same for all structs.
> 

There is not gap in this struct, should we still pack it?
What it the expectation on packing the protocol structs? Have '__rte_packed' always?