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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/20/21 12:32 PM, Jie Wang wrote:
> 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>

Newly added items must be added to
doc/guides/nics/features/default.ini in correct order.

When the item is supported, ini file of the corresponding
driver must be updated.

> ---
>  doc/api/doxy-api-index.md                   |   2 +
>  doc/guides/prog_guide/rte_flow.rst          |  25 +++
>  doc/guides/rel_notes/release_21_11.rst      |   5 +
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  28 +++
>  lib/ethdev/rte_flow.c                       |   2 +
>  lib/ethdev/rte_flow.h                       |  66 ++++++
>  lib/net/meson.build                         |   2 +
>  lib/net/rte_l2tpv2.h                        | 234 ++++++++++++++++++++
>  lib/net/rte_ppp.h                           |  35 +++
>  9 files changed, 399 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..43b64d06c1 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)

Is it a canonical spelling? Shouldn't it be L2TPv2?
If you change, it should be changed everywhere in comments.

> +  [PPP]                (@ref rte_ppp.h)
>  
>  - **QoS**:
>    [metering]           (@ref rte_meter.h),
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index fa05fe0845..6277c641ef 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1548,6 +1548,31 @@ This item is meant to use the same structure as `Item: PORT_REPRESENTOR`_.
>  
>  See also `Action: REPRESENTED_PORT`_.
>  
> +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.

ppp -> PPP everywhere above

> +- 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 c5f76081e5..9fea43ece7 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -98,6 +98,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.
> @@ -121,6 +125,7 @@ New Features
>  
>    * Added Intel iavf support on Windows.
>    * Added IPv4 and L4 (TCP/UDP/SCTP) checksum hash support in RSS flow.
> +  * Added PPPoL2TPv2oUDP RSS hash based on inner IP address and TCP/UDP port.

I think it belongs to the next patch which adds driver support.

>  
>  * **Updated Intel ice driver.**
>  
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 22ba8f0516..84cccb238d 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3810,6 +3810,20 @@ This section lists supported pattern items and their attributes, if any.
>  
>    - ``ethdev_port_id {unsigned}``: ethdev port ID
>  
> +- ``l2tpv2``: match L2TPV2 header.
> +
> +  - ``length {unsigned}``: L2TPV2 option length.
> +  - ``tunnel_id {unsigned}``: L2TPV2 tunnel identifier.
> +  - ``session_id {unsigned}``: L2TPV2 session identifier.
> +  - ``ns {unsigned}``: L2TPV2 option ns.
> +  - ``nr {unsigned}``: L2TPV2 option nr.
> +
> +- ``ppp``: match PPP header.
> +
> +  - ``addr {unsigned}``: PPP address.
> +  - ``ctrl {unsigned}``: PPP control.
> +  - ``proto_id {unsigned}``: PPP protocol identifier.
> +
>  Actions list
>  ^^^^^^^^^^^^
>  
> @@ -5036,6 +5050,20 @@ The meter policy action list: ``green -> green, yellow -> yellow, red -> red``.
>     testpmd> create port meter 0 1 13 1 yes 0xffff 0 0
>     testpmd> flow create 0 priority 0 ingress group 1 pattern eth / end actions meter mtr_id 1 / end
>  
> +Sample PPPoL2TPv2oUDP RSS rules
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +PPPoL2TPv2oUDP RSS rules can be created by the following commands::
> +
> + testpmd> flow create 0 ingress pattern eth / ipv4 / udp / l2tpv2 / ppp / ipv4
> +          / end actions rss types ipv4 end queues end / end
> + testpmd> flow create 0 ingress pattern eth / ipv4 / udp / l2tpv2 / ppp / ipv6
> +          / udp / end actions rss types ipv6-udp end queues end / end
> + testpmd> flow create 0 ingress pattern eth / ipv6 / udp / l2tpv2 / ppp / ipv4
> +          / tcp / end actions rss types ipv4-tcp end queues end / end
> + testpmd> flow create 0 ingress pattern eth / ipv6 / udp / l2tpv2 / ppp / ipv6
> +          / end actions rss types ipv6 end queues end / end
> +
>  BPF Functions
>  --------------
>  
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index 29f2b0e954..0bfbeb73e8 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -102,6 +102,8 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
>  	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
>  	MK_FLOW_ITEM(PORT_REPRESENTOR, sizeof(struct rte_flow_item_ethdev)),
>  	MK_FLOW_ITEM(REPRESENTED_PORT, sizeof(struct rte_flow_item_ethdev)),
> +	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 d5bfdaaaf2..2bc825b567 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" {
> @@ -635,6 +637,21 @@ enum rte_flow_item_type {
>  	 * @see struct rte_flow_item_ethdev
>  	 */
>  	RTE_FLOW_ITEM_TYPE_REPRESENTED_PORT,
> +
> +	/**
> +	 * 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,
> +
>  };
>  
>  /**
> @@ -1891,6 +1908,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 = 0xcb0f,

RTE_BE16 ?

> +		},
> +	},
> +};
> +#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 = 0xffff,

RTE_BE16 ?

> +	}
> +};
> +#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..1dd1a0a1e8
> --- /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 {
> +		rte_be16_t flags_version;
> +		/**< header flags and protocol version */
> +		struct {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> +			rte_be16_t t:1;		/**< message Type */
> +			rte_be16_t l:1;		/**< length option bit*/

missing space before */ here and in a number of cases below

> +			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) */

id -> ID in comments as word-cases.txt says

> +	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) */

id -> ID in comments as word-cases.txt says

> +	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) */

id -> ID in comments as word-cases.txt says

> +	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) */

id -> ID in comments as word-cases.txt says

> +	rte_be16_t ns;			/**< Ns(16) */
> +	rte_be16_t nr;			/**< Nr(16) */
> +};
> +
> +/**
> + * 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) */

id -> ID in comments as word-cases.txt says

> +	rte_be16_t ns;			/**< Ns(16) */
> +	rte_be16_t nr;			/**< Nr(16) */
> +};
> +
> +/**
> + * L2TPv2 message Header contains option length.
> + */
> +struct rte_l2tpv2_msg_with_length {
> +	rte_be16_t length;		/**< length(16) */
> +	rte_be16_t tunnel_id;		/**< tunnel id(16) */
> +	rte_be16_t session_id;		/**< session id(16) */

id -> ID in comments as word-cases.txt says

> +};
> +
> +/**
> + * L2TPv2 message Header without all options.
> + */
> +struct rte_l2tpv2_msg_without_all_options {
> +	rte_be16_t tunnel_id;		/**< tunnel id(16) */
> +	rte_be16_t session_id;		/**< session id(16) */

id -> ID in comments as word-cases.txt says

> +};
> +
> +/**
> + * L2TPv2 Combined Message Header Format: Common Header + Options
> + */
> +RTE_STD_C11
> +struct rte_l2tpv2_combined_msg_hdr {
> +	struct rte_l2tpv2_common_hdr common; /**< common header */
> +	union {
> +		struct rte_l2tpv2_msg_with_all_options type0;
> +		/**< header with all options */

Please, no documentation after documented code.
Put it before as coding style suggests.
Same everywhere below

> +		struct rte_l2tpv2_msg_without_length type1;
> +		/**< header with all options except length */
> +		struct rte_l2tpv2_msg_without_ns_nr type2;
> +		/**< header with all options except ns/nr */
> +		struct rte_l2tpv2_msg_without_offset type3;
> +		/**< header with all options except offset */
> +		struct rte_l2tpv2_msg_with_offset type4;
> +		/**< header with offset options */
> +		struct rte_l2tpv2_msg_with_ns_nr type5;
> +		/**< header with ns/nr options */
> +		struct rte_l2tpv2_msg_with_length type6;
> +		/**< header with length option */
> +		struct rte_l2tpv2_msg_without_all_options type7;
> +		/**< header without all options */
> +	};
> +};
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_L2TPV2_H_ */
> diff --git a/lib/net/rte_ppp.h b/lib/net/rte_ppp.h
> new file mode 100644
> index 0000000000..9ea633baa7
> --- /dev/null
> +++ b/lib/net/rte_ppp.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + */
> +
> +#ifndef _RTE_PPP_H_
> +#define _RTE_PPP_H_
> +
> +/**
> + * @file
> + *
> + * PPP headers definition.
> + *

Is the empty line required here?

> + */
> +
> +#include <stdint.h>
> +#include <rte_byteorder.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * PPP Header
> + */
> +struct rte_ppp_hdr {
> +	uint8_t addr; /**< ppp address(8) */
> +	uint8_t ctrl; /**< ppp control(8) */
> +	rte_be16_t proto_id; /**< ppp protocol id(16) */

ppp -> PPP everywhere above
id -> ID

> +} __rte_packed;
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_PPP_H_ */
>