[v2,1/3] ethdev: add ICMPv6 ID and sequence
Checks
Commit Message
This patch adds API support for ICMPv6 ID and sequence.
1: Add two new pattern item types for ICMPv6 echo request and reply:
RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
2: Add new header file rte_icmp6.h for ICMPv6 packet definitions.
3: Enhance testpmd flow pattern to support ICMPv6 identifier and sequence.
Example of ICMPv6 echo pattern in testpmd command:
pattern eth / ipv6 / icmp6_echo_request / end
pattern eth / ipv6 / icmp6_echo_reply / end
pattern eth / ipv6 / icmp6_echo_request ident is 20 seq is 30 / end
Signed-off-by: Leo Xu <yongquanx@nvidia.com>
Signed-off-by: Bing Zhao <bingz@nvidia.com>
---
app/test-pmd/cmdline_flow.c | 70 +++++++++++++++++++++
doc/guides/prog_guide/rte_flow.rst | 14 +++++
doc/guides/rel_notes/release_23_03.rst | 4 ++
doc/guides/testpmd_app_ug/testpmd_funcs.rst | 10 +++
lib/ethdev/rte_flow.c | 4 ++
lib/ethdev/rte_flow.h | 25 ++++++++
lib/net/meson.build | 1 +
lib/net/rte_icmp6.h | 48 ++++++++++++++
8 files changed, 176 insertions(+)
create mode 100644 lib/net/rte_icmp6.h
Comments
Hi Leo,
> -----Original Message-----
> From: Leo Xu (Networking SW) <yongquanx@nvidia.com>
> Sent: Tuesday, 20 December 2022 9:44
>
> This patch adds API support for ICMPv6 ID and sequence.
> 1: Add two new pattern item types for ICMPv6 echo request and reply:
> RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
> RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
>
> 2: Add new header file rte_icmp6.h for ICMPv6 packet definitions.
> 3: Enhance testpmd flow pattern to support ICMPv6 identifier and sequence.
>
> Example of ICMPv6 echo pattern in testpmd command:
>
> pattern eth / ipv6 / icmp6_echo_request / end
> pattern eth / ipv6 / icmp6_echo_reply / end
> pattern eth / ipv6 / icmp6_echo_request ident is 20 seq is 30 / end
>
> Signed-off-by: Leo Xu <yongquanx@nvidia.com>
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> ---
Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori
20/12/2022 08:44, Leo Xu:
> +* **Added rte_flow support for matching ICMPv6 ID and sequence fields.**
> +
> + Added ``icmp6_echo`` item in rte_flow to support ID and sequence
> + matching in ICMPv6 echo request/reply packets.
Easier to read:
"
Added flow items to match ICMPv6 echo request and reply packets.
Matching patterns can include ICMP identifier and sequence numbers.
"
> + /**
> + * Matches an ICMPv6 echo request.
> + *
> + * See struct rte_flow_item_icmp6_echo.
> + */
> + RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST,
> +
> + /**
> + * Matches an ICMPv6 echo reply.
> + *
> + * See struct rte_flow_item_icmp6_echo.
> + */
> + RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY,
It is better to use @see doxygen syntax.
> +/**
> + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
> + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
> + *
> + * Matches an ICMPv6 echo request or reply.
> + */
> +struct rte_flow_item_icmp6_echo {
> + struct rte_icmp6_echo echo;
> +};
Other items are defined with "hdr" as first field
(instead of the name "echo" here).
> --- a/lib/net/meson.build
> +++ b/lib/net/meson.build
> @@ -22,6 +22,7 @@ headers = files(
> 'rte_geneve.h',
> 'rte_l2tpv2.h',
> 'rte_ppp.h',
> + 'rte_icmp6.h',
> )
Please insert it after rte_icmp.h.
> +#ifndef _RTE_ICMP6_H_
> +#define _RTE_ICMP6_H_
No need of underscores at the beginning and end, it is not reserved.
[...]
> +/**
> + * ICMP6 header
> + */
> +struct rte_icmp6_hdr {
> + uint8_t type;
> + uint8_t code;
> + rte_be16_t checksum;
> +} __rte_packed;
> +
> +/**
> + * ICMP6 echo
> + */
> +struct rte_icmp6_echo {
> + struct rte_icmp6_hdr hdr;
> + rte_be16_t identifier;
> + rte_be16_t sequence;
> +} __rte_packed;
It is exactly the same as struct rte_icmp_hdr.
Why not reuse it?
Maybe introduce struct rte_icmp_base_hdr
and define rte_icmp_echo_hdr as rte_icmp_hdr?
> +/* ICMP6 packet types */
> +#define RTE_ICMP6_ECHO_REQUEST 128
> +#define RTE_ICMP6_ECHO_REPLY 129
Can we avoid adding this file and add only these defines to rte_icmp.h?
On 12/20/2022 7:44 AM, Leo Xu wrote:
> This patch adds API support for ICMPv6 ID and sequence.
> 1: Add two new pattern item types for ICMPv6 echo request and reply:
> RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
> RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
>
> 2: Add new header file rte_icmp6.h for ICMPv6 packet definitions.
> 3: Enhance testpmd flow pattern to support ICMPv6 identifier and sequence.
>
> Example of ICMPv6 echo pattern in testpmd command:
>
> pattern eth / ipv6 / icmp6_echo_request / end
> pattern eth / ipv6 / icmp6_echo_reply / end
> pattern eth / ipv6 / icmp6_echo_request ident is 20 seq is 30 / end
>
> Signed-off-by: Leo Xu <yongquanx@nvidia.com>
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> ---
> app/test-pmd/cmdline_flow.c | 70 +++++++++++++++++++++
> doc/guides/prog_guide/rte_flow.rst | 14 +++++
> doc/guides/rel_notes/release_23_03.rst | 4 ++
> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 10 +++
> lib/ethdev/rte_flow.c | 4 ++
> lib/ethdev/rte_flow.h | 25 ++++++++
> lib/net/meson.build | 1 +
> lib/net/rte_icmp6.h | 48 ++++++++++++++
> 8 files changed, 176 insertions(+)
> create mode 100644 lib/net/rte_icmp6.h
Please update .mailmap with in this patch.
>
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 88108498e0..7dc1528899 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -360,6 +360,12 @@ enum index {
> ITEM_ICMP6,
> ITEM_ICMP6_TYPE,
> ITEM_ICMP6_CODE,
> + ITEM_ICMP6_ECHO_REQUEST,
> + ITEM_ICMP6_ECHO_REQUEST_ID,
> + ITEM_ICMP6_ECHO_REQUEST_SEQ,
> + ITEM_ICMP6_ECHO_REPLY,
> + ITEM_ICMP6_ECHO_REPLY_ID,
> + ITEM_ICMP6_ECHO_REPLY_SEQ,
> ITEM_ICMP6_ND_NS,
> ITEM_ICMP6_ND_NS_TARGET_ADDR,
> ITEM_ICMP6_ND_NA,
> @@ -1327,6 +1333,8 @@ static const enum index next_item[] = {
> ITEM_IPV6_EXT,
> ITEM_IPV6_FRAG_EXT,
> ITEM_ICMP6,
> + ITEM_ICMP6_ECHO_REQUEST,
> + ITEM_ICMP6_ECHO_REPLY,
> ITEM_ICMP6_ND_NS,
> ITEM_ICMP6_ND_NA,
> ITEM_ICMP6_ND_OPT,
> @@ -1575,6 +1583,20 @@ static const enum index item_icmp6[] = {
> ZERO,
> };
>
> +static const enum index item_icmp6_echo_request[] = {
> + ITEM_ICMP6_ECHO_REQUEST_ID,
> + ITEM_ICMP6_ECHO_REQUEST_SEQ,
> + ITEM_NEXT,
> + ZERO,
> +};
> +
> +static const enum index item_icmp6_echo_reply[] = {
> + ITEM_ICMP6_ECHO_REPLY_ID,
> + ITEM_ICMP6_ECHO_REPLY_SEQ,
> + ITEM_NEXT,
> + ZERO,
> +};
> +
> static const enum index item_icmp6_nd_ns[] = {
> ITEM_ICMP6_ND_NS_TARGET_ADDR,
> ITEM_NEXT,
> @@ -4323,6 +4345,54 @@ static const struct token token_list[] = {
> .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6,
> code)),
> },
> + [ITEM_ICMP6_ECHO_REQUEST] = {
> + .name = "icmp6_echo_request",
> + .help = "match ICMPv6 echo request",
> + .priv = PRIV_ITEM(ICMP6_ECHO_REQUEST,
> + sizeof(struct rte_flow_item_icmp6_echo)),
> + .next = NEXT(item_icmp6_echo_request),
> + .call = parse_vc,
> + },
> + [ITEM_ICMP6_ECHO_REQUEST_ID] = {
> + .name = "ident",
> + .help = "ICMPv6 echo request identifier",
> + .next = NEXT(item_icmp6_echo_request, NEXT_ENTRY(COMMON_UNSIGNED),
> + item_param),
> + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
> + echo.identifier)),
> + },
> + [ITEM_ICMP6_ECHO_REQUEST_SEQ] = {
> + .name = "seq",
> + .help = "ICMPv6 echo request sequence",
> + .next = NEXT(item_icmp6_echo_request, NEXT_ENTRY(COMMON_UNSIGNED),
> + item_param),
> + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
> + echo.sequence)),
> + },
> + [ITEM_ICMP6_ECHO_REPLY] = {
> + .name = "icmp6_echo_reply",
> + .help = "match ICMPv6 echo reply",
> + .priv = PRIV_ITEM(ICMP6_ECHO_REPLY,
> + sizeof(struct rte_flow_item_icmp6_echo)),
> + .next = NEXT(item_icmp6_echo_reply),
> + .call = parse_vc,
> + },
> + [ITEM_ICMP6_ECHO_REPLY_ID] = {
> + .name = "ident",
> + .help = "ICMPv6 echo reply identifier",
> + .next = NEXT(item_icmp6_echo_reply, NEXT_ENTRY(COMMON_UNSIGNED),
> + item_param),
> + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
> + echo.identifier)),
> + },
> + [ITEM_ICMP6_ECHO_REPLY_SEQ] = {
> + .name = "seq",
> + .help = "ICMPv6 echo reply sequence",
> + .next = NEXT(item_icmp6_echo_reply, NEXT_ENTRY(COMMON_UNSIGNED),
> + item_param),
> + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
> + echo.sequence)),
> + },
> [ITEM_ICMP6_ND_NS] = {
> .name = "icmp6_nd_ns",
> .help = "match ICMPv6 neighbor discovery solicitation",
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 3e6242803d..59932e82a6 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1165,6 +1165,20 @@ Matches any ICMPv6 header.
> - ``checksum``: ICMPv6 checksum.
> - Default ``mask`` matches ``type`` and ``code``.
>
> +Item: ``ICMP6_ECHO_REQUEST``
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Matches an ICMPv6 echo request.
> +
> +- ``echo``: ICMP6 echo definition (``rte_icmp6.h``).
> +
> +Item: ``ICMP6_ECHO_REPLY``
> +^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Matches an ICMPv6 echo reply.
> +
> +- ``echo``: ICMP6 echo definition (``rte_icmp6.h``).
> +
> Item: ``ICMP6_ND_NS``
> ^^^^^^^^^^^^^^^^^^^^^
>
> diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
> index b8c5b68d6c..5af9c43dd9 100644
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -55,6 +55,10 @@ New Features
> Also, make sure to start the actual text at the margin.
> =======================================================
>
> +* **Added rte_flow support for matching ICMPv6 ID and sequence fields.**
> +
> + Added ``icmp6_echo`` item in rte_flow to support ID and sequence
> + matching in ICMPv6 echo request/reply packets.
>
Should keep two empty lines before next section.
> Removed Items
> -------------
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 0037506a79..f497bba26d 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3622,6 +3622,16 @@ This section lists supported pattern items and their attributes, if any.
> - ``type {unsigned}``: ICMPv6 type.
> - ``code {unsigned}``: ICMPv6 code.
>
> +- ``icmp6_echo_request``: match ICMPv6 echo request.
> +
> + - ``ident {unsigned}``: ICMPv6 echo request identifier.
> + - ``seq {unsigned}``: ICMPv6 echo request sequence number.
> +
> +- ``icmp6_echo_reply``: match ICMPv6 echo reply.
> +
> + - ``ident {unsigned}``: ICMPv6 echo reply identifier.
> + - ``seq {unsigned}``: ICMPv6 echo reply sequence number.
> +
> - ``icmp6_nd_ns``: match ICMPv6 neighbor discovery solicitation.
>
> - ``target_addr {ipv6 address}``: target address.
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index 7d0c24366c..39cd4f9817 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -123,6 +123,10 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
> MK_FLOW_ITEM(IPV6_EXT, sizeof(struct rte_flow_item_ipv6_ext)),
> MK_FLOW_ITEM(IPV6_FRAG_EXT, sizeof(struct rte_flow_item_ipv6_frag_ext)),
> MK_FLOW_ITEM(ICMP6, sizeof(struct rte_flow_item_icmp6)),
> + MK_FLOW_ITEM(ICMP6_ECHO_REQUEST,
> + sizeof(struct rte_flow_item_icmp6_echo)),
> + MK_FLOW_ITEM(ICMP6_ECHO_REPLY,
> + sizeof(struct rte_flow_item_icmp6_echo)),
Syntax, please merge lines, to make it more readable.
> MK_FLOW_ITEM(ICMP6_ND_NS, sizeof(struct rte_flow_item_icmp6_nd_ns)),
> MK_FLOW_ITEM(ICMP6_ND_NA, sizeof(struct rte_flow_item_icmp6_nd_na)),
> MK_FLOW_ITEM(ICMP6_ND_OPT, sizeof(struct rte_flow_item_icmp6_nd_opt)),
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index b60987db4b..72695aca8a 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -21,6 +21,7 @@
> #include <rte_common.h>
> #include <rte_ether.h>
> #include <rte_icmp.h>
> +#include <rte_icmp6.h>
> #include <rte_ip.h>
> #include <rte_sctp.h>
> #include <rte_tcp.h>
> @@ -624,6 +625,20 @@ enum rte_flow_item_type {
> * See struct rte_flow_item_meter_color.
> */
> RTE_FLOW_ITEM_TYPE_METER_COLOR,
> +
> + /**
> + * Matches an ICMPv6 echo request.
> + *
> + * See struct rte_flow_item_icmp6_echo.
> + */
> + RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST,
> +
> + /**
> + * Matches an ICMPv6 echo reply.
> + *
> + * See struct rte_flow_item_icmp6_echo.
> + */
> + RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY,
> };
>
> /**
> @@ -1303,6 +1318,16 @@ static const struct rte_flow_item_icmp6 rte_flow_item_icmp6_mask = {
> };
> #endif
>
> +/**
> + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
> + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
> + *
> + * Matches an ICMPv6 echo request or reply.
> + */
> +struct rte_flow_item_icmp6_echo {
> + struct rte_icmp6_echo echo;
> +};
> +
> /**
> * RTE_FLOW_ITEM_TYPE_ICMP6_ND_NS
> *
> diff --git a/lib/net/meson.build b/lib/net/meson.build
> index 379d161ee0..ce3ca67bdc 100644
> --- a/lib/net/meson.build
> +++ b/lib/net/meson.build
> @@ -22,6 +22,7 @@ headers = files(
> 'rte_geneve.h',
> 'rte_l2tpv2.h',
> 'rte_ppp.h',
> + 'rte_icmp6.h',
Can you please move just below rte_icmp.h to group them.
> )
>
> sources = files(
> diff --git a/lib/net/rte_icmp6.h b/lib/net/rte_icmp6.h
> new file mode 100644
> index 0000000000..bf6956d7c9
> --- /dev/null
> +++ b/lib/net/rte_icmp6.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2022 NVIDIA Corporation & Affiliates
2023?
> + */
> +
> +#ifndef _RTE_ICMP6_H_
> +#define _RTE_ICMP6_H_
> +
> +/**
> + * @file
> + *
> + * ICMP6-related defines
> + */
> +
> +#include <stdint.h>
> +
> +#include <rte_byteorder.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * ICMP6 header
> + */
> +struct rte_icmp6_hdr {
> + uint8_t type;
> + uint8_t code;
> + rte_be16_t checksum;
> +} __rte_packed;
> +
> +/**
> + * ICMP6 echo
> + */
> +struct rte_icmp6_echo {
> + struct rte_icmp6_hdr hdr;
> + rte_be16_t identifier;
> + rte_be16_t sequence;
> +} __rte_packed;
> +
> +/* ICMP6 packet types */
> +#define RTE_ICMP6_ECHO_REQUEST 128
> +#define RTE_ICMP6_ECHO_REPLY 129
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* RTE_ICMP6_H_ */
Hi,
PSB
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, January 26, 2023 6:45 PM
> To: Leo Xu (Networking SW) <yongquanx@nvidia.com>; dev@dpdk.org
> Cc: Bing Zhao <bingz@nvidia.com>; Ori Kam <orika@nvidia.com>; Aman Singh
> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Olivier Matz
> <olivier.matz@6wind.com>
> Subject: Re: [PATCH v2 1/3] ethdev: add ICMPv6 ID and sequence
>
> External email: Use caution opening links or attachments
>
>
> On 12/20/2022 7:44 AM, Leo Xu wrote:
> > This patch adds API support for ICMPv6 ID and sequence.
> > 1: Add two new pattern item types for ICMPv6 echo request and reply:
> > RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
> > RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
> >
> > 2: Add new header file rte_icmp6.h for ICMPv6 packet definitions.
> > 3: Enhance testpmd flow pattern to support ICMPv6 identifier and sequence.
> >
> > Example of ICMPv6 echo pattern in testpmd command:
> >
> > pattern eth / ipv6 / icmp6_echo_request / end
> > pattern eth / ipv6 / icmp6_echo_reply / end
> > pattern eth / ipv6 / icmp6_echo_request ident is 20 seq is 30 / end
> >
> > Signed-off-by: Leo Xu <yongquanx@nvidia.com>
> > Signed-off-by: Bing Zhao <bingz@nvidia.com>
> > ---
> > app/test-pmd/cmdline_flow.c | 70 +++++++++++++++++++++
> > doc/guides/prog_guide/rte_flow.rst | 14 +++++
> > doc/guides/rel_notes/release_23_03.rst | 4 ++
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 10 +++
> > lib/ethdev/rte_flow.c | 4 ++
> > lib/ethdev/rte_flow.h | 25 ++++++++
> > lib/net/meson.build | 1 +
> > lib/net/rte_icmp6.h | 48 ++++++++++++++
> > 8 files changed, 176 insertions(+)
> > create mode 100644 lib/net/rte_icmp6.h
>
> Please update .mailmap with in this patch.
Thanks, will update it in next patch.
>
> >
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index 88108498e0..7dc1528899 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -360,6 +360,12 @@ enum index {
> > ITEM_ICMP6,
> > ITEM_ICMP6_TYPE,
> > ITEM_ICMP6_CODE,
> > + ITEM_ICMP6_ECHO_REQUEST,
> > + ITEM_ICMP6_ECHO_REQUEST_ID,
> > + ITEM_ICMP6_ECHO_REQUEST_SEQ,
> > + ITEM_ICMP6_ECHO_REPLY,
> > + ITEM_ICMP6_ECHO_REPLY_ID,
> > + ITEM_ICMP6_ECHO_REPLY_SEQ,
> > ITEM_ICMP6_ND_NS,
> > ITEM_ICMP6_ND_NS_TARGET_ADDR,
> > ITEM_ICMP6_ND_NA,
> > @@ -1327,6 +1333,8 @@ static const enum index next_item[] = {
> > ITEM_IPV6_EXT,
> > ITEM_IPV6_FRAG_EXT,
> > ITEM_ICMP6,
> > + ITEM_ICMP6_ECHO_REQUEST,
> > + ITEM_ICMP6_ECHO_REPLY,
> > ITEM_ICMP6_ND_NS,
> > ITEM_ICMP6_ND_NA,
> > ITEM_ICMP6_ND_OPT,
> > @@ -1575,6 +1583,20 @@ static const enum index item_icmp6[] = {
> > ZERO,
> > };
> >
> > +static const enum index item_icmp6_echo_request[] = {
> > + ITEM_ICMP6_ECHO_REQUEST_ID,
> > + ITEM_ICMP6_ECHO_REQUEST_SEQ,
> > + ITEM_NEXT,
> > + ZERO,
> > +};
> > +
> > +static const enum index item_icmp6_echo_reply[] = {
> > + ITEM_ICMP6_ECHO_REPLY_ID,
> > + ITEM_ICMP6_ECHO_REPLY_SEQ,
> > + ITEM_NEXT,
> > + ZERO,
> > +};
> > +
> > static const enum index item_icmp6_nd_ns[] = {
> > ITEM_ICMP6_ND_NS_TARGET_ADDR,
> > ITEM_NEXT,
> > @@ -4323,6 +4345,54 @@ static const struct token token_list[] = {
> > .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6,
> > code)),
> > },
> > + [ITEM_ICMP6_ECHO_REQUEST] = {
> > + .name = "icmp6_echo_request",
> > + .help = "match ICMPv6 echo request",
> > + .priv = PRIV_ITEM(ICMP6_ECHO_REQUEST,
> > + sizeof(struct rte_flow_item_icmp6_echo)),
> > + .next = NEXT(item_icmp6_echo_request),
> > + .call = parse_vc,
> > + },
> > + [ITEM_ICMP6_ECHO_REQUEST_ID] = {
> > + .name = "ident",
> > + .help = "ICMPv6 echo request identifier",
> > + .next = NEXT(item_icmp6_echo_request,
> NEXT_ENTRY(COMMON_UNSIGNED),
> > + item_param),
> > + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
> > + echo.identifier)),
> > + },
> > + [ITEM_ICMP6_ECHO_REQUEST_SEQ] = {
> > + .name = "seq",
> > + .help = "ICMPv6 echo request sequence",
> > + .next = NEXT(item_icmp6_echo_request,
> NEXT_ENTRY(COMMON_UNSIGNED),
> > + item_param),
> > + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
> > + echo.sequence)),
> > + },
> > + [ITEM_ICMP6_ECHO_REPLY] = {
> > + .name = "icmp6_echo_reply",
> > + .help = "match ICMPv6 echo reply",
> > + .priv = PRIV_ITEM(ICMP6_ECHO_REPLY,
> > + sizeof(struct rte_flow_item_icmp6_echo)),
> > + .next = NEXT(item_icmp6_echo_reply),
> > + .call = parse_vc,
> > + },
> > + [ITEM_ICMP6_ECHO_REPLY_ID] = {
> > + .name = "ident",
> > + .help = "ICMPv6 echo reply identifier",
> > + .next = NEXT(item_icmp6_echo_reply,
> NEXT_ENTRY(COMMON_UNSIGNED),
> > + item_param),
> > + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
> > + echo.identifier)),
> > + },
> > + [ITEM_ICMP6_ECHO_REPLY_SEQ] = {
> > + .name = "seq",
> > + .help = "ICMPv6 echo reply sequence",
> > + .next = NEXT(item_icmp6_echo_reply,
> NEXT_ENTRY(COMMON_UNSIGNED),
> > + item_param),
> > + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
> > + echo.sequence)),
> > + },
> > [ITEM_ICMP6_ND_NS] = {
> > .name = "icmp6_nd_ns",
> > .help = "match ICMPv6 neighbor discovery solicitation",
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index 3e6242803d..59932e82a6 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -1165,6 +1165,20 @@ Matches any ICMPv6 header.
> > - ``checksum``: ICMPv6 checksum.
> > - Default ``mask`` matches ``type`` and ``code``.
> >
> > +Item: ``ICMP6_ECHO_REQUEST``
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Matches an ICMPv6 echo request.
> > +
> > +- ``echo``: ICMP6 echo definition (``rte_icmp6.h``).
> > +
> > +Item: ``ICMP6_ECHO_REPLY``
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Matches an ICMPv6 echo reply.
> > +
> > +- ``echo``: ICMP6 echo definition (``rte_icmp6.h``).
> > +
> > Item: ``ICMP6_ND_NS``
> > ^^^^^^^^^^^^^^^^^^^^^
> >
> > diff --git a/doc/guides/rel_notes/release_23_03.rst
> > b/doc/guides/rel_notes/release_23_03.rst
> > index b8c5b68d6c..5af9c43dd9 100644
> > --- a/doc/guides/rel_notes/release_23_03.rst
> > +++ b/doc/guides/rel_notes/release_23_03.rst
> > @@ -55,6 +55,10 @@ New Features
> > Also, make sure to start the actual text at the margin.
> > =======================================================
> >
> > +* **Added rte_flow support for matching ICMPv6 ID and sequence
> > +fields.**
> > +
> > + Added ``icmp6_echo`` item in rte_flow to support ID and sequence
> > + matching in ICMPv6 echo request/reply packets.
> >
>
> Should keep two empty lines before next section.
Thanks for that catch, will fix it.
>
> > Removed Items
> > -------------
> > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index 0037506a79..f497bba26d 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -3622,6 +3622,16 @@ This section lists supported pattern items and
> their attributes, if any.
> > - ``type {unsigned}``: ICMPv6 type.
> > - ``code {unsigned}``: ICMPv6 code.
> >
> > +- ``icmp6_echo_request``: match ICMPv6 echo request.
> > +
> > + - ``ident {unsigned}``: ICMPv6 echo request identifier.
> > + - ``seq {unsigned}``: ICMPv6 echo request sequence number.
> > +
> > +- ``icmp6_echo_reply``: match ICMPv6 echo reply.
> > +
> > + - ``ident {unsigned}``: ICMPv6 echo reply identifier.
> > + - ``seq {unsigned}``: ICMPv6 echo reply sequence number.
> > +
> > - ``icmp6_nd_ns``: match ICMPv6 neighbor discovery solicitation.
> >
> > - ``target_addr {ipv6 address}``: target address.
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> > 7d0c24366c..39cd4f9817 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -123,6 +123,10 @@ static const struct rte_flow_desc_data
> rte_flow_desc_item[] = {
> > MK_FLOW_ITEM(IPV6_EXT, sizeof(struct rte_flow_item_ipv6_ext)),
> > MK_FLOW_ITEM(IPV6_FRAG_EXT, sizeof(struct
> rte_flow_item_ipv6_frag_ext)),
> > MK_FLOW_ITEM(ICMP6, sizeof(struct rte_flow_item_icmp6)),
> > + MK_FLOW_ITEM(ICMP6_ECHO_REQUEST,
> > + sizeof(struct rte_flow_item_icmp6_echo)),
> > + MK_FLOW_ITEM(ICMP6_ECHO_REPLY,
> > + sizeof(struct rte_flow_item_icmp6_echo)),
>
> Syntax, please merge lines, to make it more readable.
Thanks for that catch, will fix it.
>
> > MK_FLOW_ITEM(ICMP6_ND_NS, sizeof(struct
> rte_flow_item_icmp6_nd_ns)),
> > MK_FLOW_ITEM(ICMP6_ND_NA, sizeof(struct
> rte_flow_item_icmp6_nd_na)),
> > MK_FLOW_ITEM(ICMP6_ND_OPT, sizeof(struct
> > rte_flow_item_icmp6_nd_opt)), diff --git a/lib/ethdev/rte_flow.h
> > b/lib/ethdev/rte_flow.h index b60987db4b..72695aca8a 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -21,6 +21,7 @@
> > #include <rte_common.h>
> > #include <rte_ether.h>
> > #include <rte_icmp.h>
> > +#include <rte_icmp6.h>
> > #include <rte_ip.h>
> > #include <rte_sctp.h>
> > #include <rte_tcp.h>
> > @@ -624,6 +625,20 @@ enum rte_flow_item_type {
> > * See struct rte_flow_item_meter_color.
> > */
> > RTE_FLOW_ITEM_TYPE_METER_COLOR,
> > +
> > + /**
> > + * Matches an ICMPv6 echo request.
> > + *
> > + * See struct rte_flow_item_icmp6_echo.
> > + */
> > + RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST,
> > +
> > + /**
> > + * Matches an ICMPv6 echo reply.
> > + *
> > + * See struct rte_flow_item_icmp6_echo.
> > + */
> > + RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY,
> > };
> >
> > /**
> > @@ -1303,6 +1318,16 @@ static const struct rte_flow_item_icmp6
> > rte_flow_item_icmp6_mask = { }; #endif
> >
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
> > + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
> > + *
> > + * Matches an ICMPv6 echo request or reply.
> > + */
> > +struct rte_flow_item_icmp6_echo {
> > + struct rte_icmp6_echo echo;
> > +};
> > +
> > /**
> > * RTE_FLOW_ITEM_TYPE_ICMP6_ND_NS
> > *
> > diff --git a/lib/net/meson.build b/lib/net/meson.build index
> > 379d161ee0..ce3ca67bdc 100644
> > --- a/lib/net/meson.build
> > +++ b/lib/net/meson.build
> > @@ -22,6 +22,7 @@ headers = files(
> > 'rte_geneve.h',
> > 'rte_l2tpv2.h',
> > 'rte_ppp.h',
> > + 'rte_icmp6.h',
>
> Can you please move just below rte_icmp.h to group them.
Thanks, will merge the rte_icmp6.h into rte_icmp.h. Then, there will be no rte_icmp6.h file.
>
> > )
> >
> > sources = files(
> > diff --git a/lib/net/rte_icmp6.h b/lib/net/rte_icmp6.h new file mode
> > 100644 index 0000000000..bf6956d7c9
> > --- /dev/null
> > +++ b/lib/net/rte_icmp6.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright (c) 2022 NVIDIA Corporation & Affiliates
>
> 2023?
Thanks, will update it.
>
> > + */
> > +
> > +#ifndef _RTE_ICMP6_H_
> > +#define _RTE_ICMP6_H_
> > +
> > +/**
> > + * @file
> > + *
> > + * ICMP6-related defines
> > + */
> > +
> > +#include <stdint.h>
> > +
> > +#include <rte_byteorder.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * ICMP6 header
> > + */
> > +struct rte_icmp6_hdr {
> > + uint8_t type;
> > + uint8_t code;
> > + rte_be16_t checksum;
> > +} __rte_packed;
> > +
> > +/**
> > + * ICMP6 echo
> > + */
> > +struct rte_icmp6_echo {
> > + struct rte_icmp6_hdr hdr;
> > + rte_be16_t identifier;
> > + rte_be16_t sequence;
> > +} __rte_packed;
> > +
> > +/* ICMP6 packet types */
> > +#define RTE_ICMP6_ECHO_REQUEST 128
> > +#define RTE_ICMP6_ECHO_REPLY 129
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* RTE_ICMP6_H_ */
Hi Thomas,
Thanks for those comments.
PSB
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, January 18, 2023 5:31 PM
> To: Leo Xu (Networking SW) <yongquanx@nvidia.com>
> Cc: dev@dpdk.org; Bing Zhao <bingz@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Aman Singh <aman.deep.singh@intel.com>; Yuying
> Zhang <yuying.zhang@intel.com>; Ferruh Yigit <ferruh.yigit@amd.com>;
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Olivier Matz
> <olivier.matz@6wind.com>; david.marchand@redhat.com
> Subject: Re: [PATCH v2 1/3] ethdev: add ICMPv6 ID and sequence
>
> External email: Use caution opening links or attachments
>
>
> 20/12/2022 08:44, Leo Xu:
> > +* **Added rte_flow support for matching ICMPv6 ID and sequence
> > +fields.**
> > +
> > + Added ``icmp6_echo`` item in rte_flow to support ID and sequence
> > + matching in ICMPv6 echo request/reply packets.
>
> Easier to read:
> "
> Added flow items to match ICMPv6 echo request and reply packets.
> Matching patterns can include ICMP identifier and sequence numbers.
> "
Thanks for that good sentence, will update accordingly.
>
> > + /**
> > + * Matches an ICMPv6 echo request.
> > + *
> > + * See struct rte_flow_item_icmp6_echo.
> > + */
> > + RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST,
> > +
> > + /**
> > + * Matches an ICMPv6 echo reply.
> > + *
> > + * See struct rte_flow_item_icmp6_echo.
> > + */
> > + RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY,
>
> It is better to use @see doxygen syntax.
Ok, thanks.
>
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
> > + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
> > + *
> > + * Matches an ICMPv6 echo request or reply.
> > + */
> > +struct rte_flow_item_icmp6_echo {
> > + struct rte_icmp6_echo echo;
> > +};
>
> Other items are defined with "hdr" as first field (instead of the name "echo"
> here).
>
> > --- a/lib/net/meson.build
> > +++ b/lib/net/meson.build
> > @@ -22,6 +22,7 @@ headers = files(
> > 'rte_geneve.h',
> > 'rte_l2tpv2.h',
> > 'rte_ppp.h',
> > + 'rte_icmp6.h',
> > )
>
> Please insert it after rte_icmp.h.
Will merge the rte_icmp6.h into rte_icmp.h, then no rte_icmp6.h any more.
>
> > +#ifndef _RTE_ICMP6_H_
> > +#define _RTE_ICMP6_H_
>
> No need of underscores at the beginning and end, it is not reserved.
>
Will merge the rte_icmp6.h into rte_icmp.h, then no rte_icmp6.h any more.
> [...]
> > +/**
> > + * ICMP6 header
> > + */
> > +struct rte_icmp6_hdr {
> > + uint8_t type;
> > + uint8_t code;
> > + rte_be16_t checksum;
> > +} __rte_packed;
> > +
> > +/**
> > + * ICMP6 echo
> > + */
> > +struct rte_icmp6_echo {
> > + struct rte_icmp6_hdr hdr;
> > + rte_be16_t identifier;
> > + rte_be16_t sequence;
> > +} __rte_packed;
>
> It is exactly the same as struct rte_icmp_hdr.
> Why not reuse it?
> Maybe introduce struct rte_icmp_base_hdr and define rte_icmp_echo_hdr as
> rte_icmp_hdr?
Hi Thomas,
Looks like, using rte_icmp_hdr as base header for both icmp and icmpv6 is not that good.
since, rte_icmp_hdr default their headers always having id and sequence fields, which is not applicable for most other icmp6/icmp types packets.
I may suggest to keep icmp and icmp6 structures independent against each other, because, looks like these two protocols definitions do not share common base.
>
> > +/* ICMP6 packet types */
> > +#define RTE_ICMP6_ECHO_REQUEST 128
> > +#define RTE_ICMP6_ECHO_REPLY 129
>
> Can we avoid adding this file and add only these defines to rte_icmp.h?
>
Yes, good idea. rte_ip.h does not have independent v6 header file either.
We should not create v6 header file for icmp.
31/01/2023 07:53, Leo Xu (Networking SW):
> From: Thomas Monjalon <thomas@monjalon.net>
> > 20/12/2022 08:44, Leo Xu:
> > > +/**
> > > + * ICMP6 header
> > > + */
> > > +struct rte_icmp6_hdr {
> > > + uint8_t type;
> > > + uint8_t code;
> > > + rte_be16_t checksum;
> > > +} __rte_packed;
> > > +
> > > +/**
> > > + * ICMP6 echo
> > > + */
> > > +struct rte_icmp6_echo {
> > > + struct rte_icmp6_hdr hdr;
> > > + rte_be16_t identifier;
> > > + rte_be16_t sequence;
> > > +} __rte_packed;
> >
> > It is exactly the same as struct rte_icmp_hdr.
> > Why not reuse it?
> > Maybe introduce struct rte_icmp_base_hdr and define rte_icmp_echo_hdr as
> > rte_icmp_hdr?
>
> Hi Thomas,
> Looks like, using rte_icmp_hdr as base header for both icmp and icmpv6 is not that good.
> since, rte_icmp_hdr default their headers always having id and sequence fields, which is not applicable for most other icmp6/icmp types packets.
>
> I may suggest to keep icmp and icmp6 structures independent against each other, because, looks like these two protocols definitions do not share common base.
What about introducing rte_icmp_base_hdr?
We should try to avoid duplicating things.
Hi Thomas
PSB
> 31/01/2023 07:53, Leo Xu (Networking SW):
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 20/12/2022 08:44, Leo Xu:
> > > > +/**
> > > > + * ICMP6 header
> > > > + */
> > > > +struct rte_icmp6_hdr {
> > > > + uint8_t type;
> > > > + uint8_t code;
> > > > + rte_be16_t checksum;
> > > > +} __rte_packed;
> > > > +
> > > > +/**
> > > > + * ICMP6 echo
> > > > + */
> > > > +struct rte_icmp6_echo {
> > > > + struct rte_icmp6_hdr hdr;
> > > > + rte_be16_t identifier;
> > > > + rte_be16_t sequence;
> > > > +} __rte_packed;
> > >
> > > It is exactly the same as struct rte_icmp_hdr.
> > > Why not reuse it?
> > > Maybe introduce struct rte_icmp_base_hdr and define
> > > rte_icmp_echo_hdr as rte_icmp_hdr?
> >
> > Hi Thomas,
> > Looks like, using rte_icmp_hdr as base header for both icmp and icmpv6 is
> not that good.
> > since, rte_icmp_hdr default their headers always having id and sequence
> fields, which is not applicable for most other icmp6/icmp types packets.
> >
> > I may suggest to keep icmp and icmp6 structures independent against each
> other, because, looks like these two protocols definitions do not share common
> base.
>
> What about introducing rte_icmp_base_hdr?
> We should try to avoid duplicating things.
>
You mean introduce rte_icmp_base_hdr like following?
struct rte_icmp_base_hdr {
uint8_t icmp_type;
uint8_t icmp_code;
rte_be16_t icmp_cksum;
} __rte_packed;
And change the existing rte_icmp_hdr to be:
struct rte_icmp_hdr {
rte_icmp_base_hdr bash_hdr;
rte_be16_t icmp_ident;
rte_be16_t icmp_seq_nb;
} __rte_packed;
#define rte_icmp6_echo struct rte_icmp_hdr;
If it is, there will be some compatibilities issues, since we changed existing structure.
Or, maybe I'm missing something.
Would you help to give more details about above comment?
02/02/2023 19:33, Leo Xu (Networking SW):
> > 31/01/2023 07:53, Leo Xu (Networking SW):
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 20/12/2022 08:44, Leo Xu:
> > > > > +/**
> > > > > + * ICMP6 header
> > > > > + */
> > > > > +struct rte_icmp6_hdr {
> > > > > + uint8_t type;
> > > > > + uint8_t code;
> > > > > + rte_be16_t checksum;
> > > > > +} __rte_packed;
> > > > > +
> > > > > +/**
> > > > > + * ICMP6 echo
> > > > > + */
> > > > > +struct rte_icmp6_echo {
> > > > > + struct rte_icmp6_hdr hdr;
> > > > > + rte_be16_t identifier;
> > > > > + rte_be16_t sequence;
> > > > > +} __rte_packed;
> > > >
> > > > It is exactly the same as struct rte_icmp_hdr.
> > > > Why not reuse it?
> > > > Maybe introduce struct rte_icmp_base_hdr and define
> > > > rte_icmp_echo_hdr as rte_icmp_hdr?
> > >
> > > Hi Thomas,
> > > Looks like, using rte_icmp_hdr as base header for both icmp and icmpv6 is
> > not that good.
> > > since, rte_icmp_hdr default their headers always having id and sequence
> > fields, which is not applicable for most other icmp6/icmp types packets.
> > >
> > > I may suggest to keep icmp and icmp6 structures independent against each
> > other, because, looks like these two protocols definitions do not share common
> > base.
> >
> > What about introducing rte_icmp_base_hdr?
> > We should try to avoid duplicating things.
> >
>
> You mean introduce rte_icmp_base_hdr like following?
> struct rte_icmp_base_hdr {
> uint8_t icmp_type;
> uint8_t icmp_code;
> rte_be16_t icmp_cksum;
> } __rte_packed;
>
> And change the existing rte_icmp_hdr to be:
> struct rte_icmp_hdr {
> rte_icmp_base_hdr bash_hdr;
> rte_be16_t icmp_ident;
> rte_be16_t icmp_seq_nb;
> } __rte_packed;
> #define rte_icmp6_echo struct rte_icmp_hdr;
>
> If it is, there will be some compatibilities issues, since we changed existing structure.
>
> Or, maybe I'm missing something.
> Would you help to give more details about above comment?
Currently we have this:
struct rte_icmp_hdr {
uint8_t icmp_type; /* ICMP packet type. */
uint8_t icmp_code; /* ICMP packet code. */
rte_be16_t icmp_cksum; /* ICMP packet checksum. */
rte_be16_t icmp_ident; /* ICMP packet identifier. */
rte_be16_t icmp_seq_nb; /* ICMP packet sequence number. */
} __rte_packed;
I agree we can move some fields in a base struct,
it would change the API.
We could manage with a union, but we would lose the benefit.
It looks like we need to keep rte_icmp_hdr as is.
So we need to duplicate and define new structs.
What about removing the "6" from the new structs,
so it would apply both to IPv4 and IPv6?
struct rte_icmp_base_hdr {
uint8_t type;
uint8_t code;
rte_be16_t checksum;
} __rte_packed;
struct rte_icmp_echo_hdr {
struct rte_icmp_base_hdr base;
rte_be16_t identifier;
rte_be16_t sequence;
} __rte_packed;
> 02/02/2023 19:33, Leo Xu (Networking SW):
> > > 31/01/2023 07:53, Leo Xu (Networking SW):
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 20/12/2022 08:44, Leo Xu:
> > > > > > +/**
> > > > > > + * ICMP6 header
> > > > > > + */
> > > > > > +struct rte_icmp6_hdr {
> > > > > > + uint8_t type;
> > > > > > + uint8_t code;
> > > > > > + rte_be16_t checksum;
> > > > > > +} __rte_packed;
> > > > > > +
> > > > > > +/**
> > > > > > + * ICMP6 echo
> > > > > > + */
> > > > > > +struct rte_icmp6_echo {
> > > > > > + struct rte_icmp6_hdr hdr;
> > > > > > + rte_be16_t identifier;
> > > > > > + rte_be16_t sequence;
> > > > > > +} __rte_packed;
> > > > >
> > > > > It is exactly the same as struct rte_icmp_hdr.
> > > > > Why not reuse it?
> > > > > Maybe introduce struct rte_icmp_base_hdr and define
> > > > > rte_icmp_echo_hdr as rte_icmp_hdr?
> > > >
> > > > Hi Thomas,
> > > > Looks like, using rte_icmp_hdr as base header for both icmp and
> > > > icmpv6 is
> > > not that good.
> > > > since, rte_icmp_hdr default their headers always having id and
> > > > sequence
> > > fields, which is not applicable for most other icmp6/icmp types packets.
> > > >
> > > > I may suggest to keep icmp and icmp6 structures independent
> > > > against each
> > > other, because, looks like these two protocols definitions do not
> > > share common base.
> > >
> > > What about introducing rte_icmp_base_hdr?
> > > We should try to avoid duplicating things.
> > >
> >
> > You mean introduce rte_icmp_base_hdr like following?
> > struct rte_icmp_base_hdr {
> > uint8_t icmp_type;
> > uint8_t icmp_code;
> > rte_be16_t icmp_cksum;
> > } __rte_packed;
> >
> > And change the existing rte_icmp_hdr to be:
> > struct rte_icmp_hdr {
> > rte_icmp_base_hdr bash_hdr;
> > rte_be16_t icmp_ident;
> > rte_be16_t icmp_seq_nb;
> > } __rte_packed;
> > #define rte_icmp6_echo struct rte_icmp_hdr;
> >
> > If it is, there will be some compatibilities issues, since we changed existing
> structure.
> >
> > Or, maybe I'm missing something.
> > Would you help to give more details about above comment?
>
> Currently we have this:
> struct rte_icmp_hdr {
> uint8_t icmp_type; /* ICMP packet type. */
> uint8_t icmp_code; /* ICMP packet code. */
> rte_be16_t icmp_cksum; /* ICMP packet checksum. */
> rte_be16_t icmp_ident; /* ICMP packet identifier. */
> rte_be16_t icmp_seq_nb; /* ICMP packet sequence number. */ }
> __rte_packed;
>
> I agree we can move some fields in a base struct, it would change the API.
> We could manage with a union, but we would lose the benefit.
> It looks like we need to keep rte_icmp_hdr as is.
> So we need to duplicate and define new structs.
>
> What about removing the "6" from the new structs, so it would apply both to
> IPv4 and IPv6?
>
> struct rte_icmp_base_hdr {
> uint8_t type;
> uint8_t code;
> rte_be16_t checksum;
> } __rte_packed;
>
> struct rte_icmp_echo_hdr {
> struct rte_icmp_base_hdr base;
> rte_be16_t identifier;
> rte_be16_t sequence;
> } __rte_packed;
>
>
I agree with that proposal.
Then, we can deem existing struct rte_icmp_hdr as old one, which should not be used in new app.
And looks like, these new defined structures can cover all ICMP4/6 formats.
Good idea!
I will update accordingly, in next patch.
@@ -360,6 +360,12 @@ enum index {
ITEM_ICMP6,
ITEM_ICMP6_TYPE,
ITEM_ICMP6_CODE,
+ ITEM_ICMP6_ECHO_REQUEST,
+ ITEM_ICMP6_ECHO_REQUEST_ID,
+ ITEM_ICMP6_ECHO_REQUEST_SEQ,
+ ITEM_ICMP6_ECHO_REPLY,
+ ITEM_ICMP6_ECHO_REPLY_ID,
+ ITEM_ICMP6_ECHO_REPLY_SEQ,
ITEM_ICMP6_ND_NS,
ITEM_ICMP6_ND_NS_TARGET_ADDR,
ITEM_ICMP6_ND_NA,
@@ -1327,6 +1333,8 @@ static const enum index next_item[] = {
ITEM_IPV6_EXT,
ITEM_IPV6_FRAG_EXT,
ITEM_ICMP6,
+ ITEM_ICMP6_ECHO_REQUEST,
+ ITEM_ICMP6_ECHO_REPLY,
ITEM_ICMP6_ND_NS,
ITEM_ICMP6_ND_NA,
ITEM_ICMP6_ND_OPT,
@@ -1575,6 +1583,20 @@ static const enum index item_icmp6[] = {
ZERO,
};
+static const enum index item_icmp6_echo_request[] = {
+ ITEM_ICMP6_ECHO_REQUEST_ID,
+ ITEM_ICMP6_ECHO_REQUEST_SEQ,
+ ITEM_NEXT,
+ ZERO,
+};
+
+static const enum index item_icmp6_echo_reply[] = {
+ ITEM_ICMP6_ECHO_REPLY_ID,
+ ITEM_ICMP6_ECHO_REPLY_SEQ,
+ ITEM_NEXT,
+ ZERO,
+};
+
static const enum index item_icmp6_nd_ns[] = {
ITEM_ICMP6_ND_NS_TARGET_ADDR,
ITEM_NEXT,
@@ -4323,6 +4345,54 @@ static const struct token token_list[] = {
.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6,
code)),
},
+ [ITEM_ICMP6_ECHO_REQUEST] = {
+ .name = "icmp6_echo_request",
+ .help = "match ICMPv6 echo request",
+ .priv = PRIV_ITEM(ICMP6_ECHO_REQUEST,
+ sizeof(struct rte_flow_item_icmp6_echo)),
+ .next = NEXT(item_icmp6_echo_request),
+ .call = parse_vc,
+ },
+ [ITEM_ICMP6_ECHO_REQUEST_ID] = {
+ .name = "ident",
+ .help = "ICMPv6 echo request identifier",
+ .next = NEXT(item_icmp6_echo_request, NEXT_ENTRY(COMMON_UNSIGNED),
+ item_param),
+ .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
+ echo.identifier)),
+ },
+ [ITEM_ICMP6_ECHO_REQUEST_SEQ] = {
+ .name = "seq",
+ .help = "ICMPv6 echo request sequence",
+ .next = NEXT(item_icmp6_echo_request, NEXT_ENTRY(COMMON_UNSIGNED),
+ item_param),
+ .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
+ echo.sequence)),
+ },
+ [ITEM_ICMP6_ECHO_REPLY] = {
+ .name = "icmp6_echo_reply",
+ .help = "match ICMPv6 echo reply",
+ .priv = PRIV_ITEM(ICMP6_ECHO_REPLY,
+ sizeof(struct rte_flow_item_icmp6_echo)),
+ .next = NEXT(item_icmp6_echo_reply),
+ .call = parse_vc,
+ },
+ [ITEM_ICMP6_ECHO_REPLY_ID] = {
+ .name = "ident",
+ .help = "ICMPv6 echo reply identifier",
+ .next = NEXT(item_icmp6_echo_reply, NEXT_ENTRY(COMMON_UNSIGNED),
+ item_param),
+ .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
+ echo.identifier)),
+ },
+ [ITEM_ICMP6_ECHO_REPLY_SEQ] = {
+ .name = "seq",
+ .help = "ICMPv6 echo reply sequence",
+ .next = NEXT(item_icmp6_echo_reply, NEXT_ENTRY(COMMON_UNSIGNED),
+ item_param),
+ .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp6_echo,
+ echo.sequence)),
+ },
[ITEM_ICMP6_ND_NS] = {
.name = "icmp6_nd_ns",
.help = "match ICMPv6 neighbor discovery solicitation",
@@ -1165,6 +1165,20 @@ Matches any ICMPv6 header.
- ``checksum``: ICMPv6 checksum.
- Default ``mask`` matches ``type`` and ``code``.
+Item: ``ICMP6_ECHO_REQUEST``
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Matches an ICMPv6 echo request.
+
+- ``echo``: ICMP6 echo definition (``rte_icmp6.h``).
+
+Item: ``ICMP6_ECHO_REPLY``
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Matches an ICMPv6 echo reply.
+
+- ``echo``: ICMP6 echo definition (``rte_icmp6.h``).
+
Item: ``ICMP6_ND_NS``
^^^^^^^^^^^^^^^^^^^^^
@@ -55,6 +55,10 @@ New Features
Also, make sure to start the actual text at the margin.
=======================================================
+* **Added rte_flow support for matching ICMPv6 ID and sequence fields.**
+
+ Added ``icmp6_echo`` item in rte_flow to support ID and sequence
+ matching in ICMPv6 echo request/reply packets.
Removed Items
-------------
@@ -3622,6 +3622,16 @@ This section lists supported pattern items and their attributes, if any.
- ``type {unsigned}``: ICMPv6 type.
- ``code {unsigned}``: ICMPv6 code.
+- ``icmp6_echo_request``: match ICMPv6 echo request.
+
+ - ``ident {unsigned}``: ICMPv6 echo request identifier.
+ - ``seq {unsigned}``: ICMPv6 echo request sequence number.
+
+- ``icmp6_echo_reply``: match ICMPv6 echo reply.
+
+ - ``ident {unsigned}``: ICMPv6 echo reply identifier.
+ - ``seq {unsigned}``: ICMPv6 echo reply sequence number.
+
- ``icmp6_nd_ns``: match ICMPv6 neighbor discovery solicitation.
- ``target_addr {ipv6 address}``: target address.
@@ -123,6 +123,10 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
MK_FLOW_ITEM(IPV6_EXT, sizeof(struct rte_flow_item_ipv6_ext)),
MK_FLOW_ITEM(IPV6_FRAG_EXT, sizeof(struct rte_flow_item_ipv6_frag_ext)),
MK_FLOW_ITEM(ICMP6, sizeof(struct rte_flow_item_icmp6)),
+ MK_FLOW_ITEM(ICMP6_ECHO_REQUEST,
+ sizeof(struct rte_flow_item_icmp6_echo)),
+ MK_FLOW_ITEM(ICMP6_ECHO_REPLY,
+ sizeof(struct rte_flow_item_icmp6_echo)),
MK_FLOW_ITEM(ICMP6_ND_NS, sizeof(struct rte_flow_item_icmp6_nd_ns)),
MK_FLOW_ITEM(ICMP6_ND_NA, sizeof(struct rte_flow_item_icmp6_nd_na)),
MK_FLOW_ITEM(ICMP6_ND_OPT, sizeof(struct rte_flow_item_icmp6_nd_opt)),
@@ -21,6 +21,7 @@
#include <rte_common.h>
#include <rte_ether.h>
#include <rte_icmp.h>
+#include <rte_icmp6.h>
#include <rte_ip.h>
#include <rte_sctp.h>
#include <rte_tcp.h>
@@ -624,6 +625,20 @@ enum rte_flow_item_type {
* See struct rte_flow_item_meter_color.
*/
RTE_FLOW_ITEM_TYPE_METER_COLOR,
+
+ /**
+ * Matches an ICMPv6 echo request.
+ *
+ * See struct rte_flow_item_icmp6_echo.
+ */
+ RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST,
+
+ /**
+ * Matches an ICMPv6 echo reply.
+ *
+ * See struct rte_flow_item_icmp6_echo.
+ */
+ RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY,
};
/**
@@ -1303,6 +1318,16 @@ static const struct rte_flow_item_icmp6 rte_flow_item_icmp6_mask = {
};
#endif
+/**
+ * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
+ * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
+ *
+ * Matches an ICMPv6 echo request or reply.
+ */
+struct rte_flow_item_icmp6_echo {
+ struct rte_icmp6_echo echo;
+};
+
/**
* RTE_FLOW_ITEM_TYPE_ICMP6_ND_NS
*
@@ -22,6 +22,7 @@ headers = files(
'rte_geneve.h',
'rte_l2tpv2.h',
'rte_ppp.h',
+ 'rte_icmp6.h',
)
sources = files(
new file mode 100644
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2022 NVIDIA Corporation & Affiliates
+ */
+
+#ifndef _RTE_ICMP6_H_
+#define _RTE_ICMP6_H_
+
+/**
+ * @file
+ *
+ * ICMP6-related defines
+ */
+
+#include <stdint.h>
+
+#include <rte_byteorder.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * ICMP6 header
+ */
+struct rte_icmp6_hdr {
+ uint8_t type;
+ uint8_t code;
+ rte_be16_t checksum;
+} __rte_packed;
+
+/**
+ * ICMP6 echo
+ */
+struct rte_icmp6_echo {
+ struct rte_icmp6_hdr hdr;
+ rte_be16_t identifier;
+ rte_be16_t sequence;
+} __rte_packed;
+
+/* ICMP6 packet types */
+#define RTE_ICMP6_ECHO_REQUEST 128
+#define RTE_ICMP6_ECHO_REPLY 129
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_ICMP6_H_ */