Re: [PATCH v5 4/8] ethdev: add GTP items to support flow API

[Sean Harte <seanbh@gmail.com>]

On 28 September 2017 at 09:13, Beilei Xing <beilei.xing@intel.com> wrote:
> This patch adds GTP, GTPC and GTPU items for
> generic flow API, and also exposes item fields
> through the flow command.
>
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  app/test-pmd/cmdline_flow.c                 | 40 ++++++++++++++++++++++
>  app/test-pmd/config.c                       |  3 ++
>  doc/guides/prog_guide/rte_flow.rst          | 18 ++++++++++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
>  lib/librte_ether/rte_flow.h                 | 52 +++++++++++++++++++++++++++++
>  5 files changed, 117 insertions(+)
<snip>
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -955,6 +955,24 @@ Usage example, fuzzy match a TCPv4 packets:
>     | 4     | END      |
>     +-------+----------+
>
> +Item: ``GTP``, ``GTPC``, ``GTPU``
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Matches a GTP header.
> +
> +Note: GTP, GTPC and GTPU use the same structure. Since only UDP destination port
> +is used to distinguish GTP_C (port is 2123) and GTP_U packets (port is 2152),
> +GTPC and GTPU item are defined for a user-friendly API when creating GTP-C and
> +GTP-U flow.

In GTPv1-C, request messages are sent from any port to port 2123, and
in the response message the ports are reversed (in GTPv2-C, it's a
little more complicated). Is the intention to only match requests?
It's not clear.

Also, it should be mentioned that GTPv0 is not included (it uses port 3386)

> +
> +- ``v_pt_rsv_flags``: version (3b), protocol type (1b), reserved (1b),
> +  extension header flag (1b), sequence number flag (1b), N-PDU number
> +  flag (1b).
> +- ``msg_type``: message type.
> +- ``msg_len``: message length.
> +- ``teid``: tunnel endpoint identifier.
> +- Default ``mask`` matches teid only.
> +
>  Actions
>  ~~~~~~~
>
<snip>
>  /**
> + * RTE_FLOW_ITEM_TYPE_GTP.
> + *
> + * Matches a GTP header.
> + */
> +struct rte_flow_item_gtp {
> +       /**
> +        * Version (2b), protocol type (1b), reserved (1b),
> +        * Extension header flag (1b),
> +        * Sequence number flag (1b),
> +        * N-PDU number flag (1b).
> +        */
> +       uint8_t v_pt_rsv_flags;

The version field has 3 bits, not 2 (it was correct above). The
meaning of the 5 flags in this byte is different in GTPv2-C compared
to GTPv1-C. Is the intention to only support GTPv1? If so that should
be stated. If GTPv2 is supported, then the teid field below is not
present in a few cases and matching on it could cause some strange
behaviour.

> +       uint8_t msg_type; /**< Message type. */
> +       rte_be16_t msg_len; /**< Message length. */
> +       rte_be32_t teid; /**< Tunnel endpoint identifier. */
> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_GTP. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_gtp rte_flow_item_gtp_mask = {
> +       .teid = RTE_BE32(0xffffffff),
> +};
> +#endif
> +
> +/**
>   * Matching pattern item definition.
>   *
>   * A pattern is formed by stacking items starting from the lowest protocol
> --
> 2.5.5
>