[v3,1/1] net/mlx5: support match ICMP identifier fields
Checks
Commit Message
PRM expose fields "Icmp_header_data" in IPv4 ICMP.
Update ICMP mask parameter with ICMP identifier and sequence number fields.
ICMP sequence number spec with mask, Icmp_header_data low 16 bits are set.
ICMP identifier spec with mask, Icmp_header_data high 16 bits are set.
Signed-off-by: Li Zhang <lizh@nvidia.com>
---
doc/guides/nics/mlx5.rst | 4 ++--
doc/guides/rel_notes/release_20_11.rst | 2 +-
drivers/net/mlx5/mlx5_flow.c | 10 ++++++++--
drivers/net/mlx5/mlx5_flow_dv.c | 16 +++++++++++++++-
4 files changed, 26 insertions(+), 6 deletions(-)
Comments
Hi
Sorry I didn't see that you sent V3 and responded on V2
So just rewriting my comments.
Best,
Ori
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Li Zhang
> Subject: [dpdk-dev] [PATCH v3 1/1] net/mlx5: support match ICMP identifier
> fields
>
> PRM expose fields "Icmp_header_data" in IPv4 ICMP.
> Update ICMP mask parameter with ICMP identifier and sequence number
> fields.
> ICMP sequence number spec with mask, Icmp_header_data low 16 bits are set.
> ICMP identifier spec with mask, Icmp_header_data high 16 bits are set.
>
> Signed-off-by: Li Zhang <lizh@nvidia.com>
> ---
> doc/guides/nics/mlx5.rst | 4 ++--
> doc/guides/rel_notes/release_20_11.rst | 2 +-
> drivers/net/mlx5/mlx5_flow.c | 10 ++++++++--
> drivers/net/mlx5/mlx5_flow_dv.c | 16 +++++++++++++++-
> 4 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
> index 211c0c5a6c..576dbe5efd 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -288,7 +288,7 @@ Limitations
> - The input buffer, providing the removal size, is not validated.
> - The buffer size must match the length of the headers to be removed.
>
> -- ICMP/ICMP6 code/type matching, IP-in-IP and MPLS flow matching are all
> +- ICMP(code/type/identifier/sequence number) / ICMP6(code/type) matching,
> IP-in-IP and MPLS flow matching are all
> mutually exclusive features which cannot be supported together
> (see :ref:`mlx5_firmware_config`).
>
> @@ -1009,7 +1009,7 @@ Below are some firmware configurations listed.
>
> FLEX_PARSER_PROFILE_ENABLE=1
>
> -- enable ICMP/ICMP6 code/type fields matching::
> +- enable ICMP(code/type/identifier/sequence number) / ICMP6(code/type)
> fields matching::
>
> FLEX_PARSER_PROFILE_ENABLE=2
>
> diff --git a/doc/guides/rel_notes/release_20_11.rst
> b/doc/guides/rel_notes/release_20_11.rst
> index c6642f5f94..791f133d8f 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -73,7 +73,7 @@ New Features
> * Added flag action.
> * Added raw encap/decap actions.
> * Added VXLAN encap/decap actions.
> - * Added ICMP and ICMP6 matching items.
> + * Added ICMP(code/type/identifier/sequence number) and
> ICMP6(code/type) matching items.
> * Added option to set port mask for insertion/deletion:
> ``--portmask=N``
> where N represents the hexadecimal bitmask of ports used.
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 416505f1c8..3cabfd4627 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -1303,6 +1303,12 @@ mlx5_flow_validate_item_icmp(const struct
> rte_flow_item *item,
> struct rte_flow_error *error)
> {
> const struct rte_flow_item_icmp *mask = item->mask;
> + const struct rte_flow_item_icmp nic_mask = {
> + .hdr.icmp_type = 0xff,
> + .hdr.icmp_code = 0xff,
> + .hdr.icmp_ident = RTE_BE16(0xffff),
> + .hdr.icmp_seq_nb = RTE_BE16(0xffff),
> + };
> const int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
> const uint64_t l3m = tunnel ? MLX5_FLOW_LAYER_INNER_L3_IPV4 :
> MLX5_FLOW_LAYER_OUTER_L3_IPV4;
> @@ -1325,10 +1331,10 @@ mlx5_flow_validate_item_icmp(const struct
> rte_flow_item *item,
> RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> "multiple L4 layers not supported");
> if (!mask)
> - mask = &rte_flow_item_icmp_mask;
> + mask = &nic_mask;
> ret = mlx5_flow_item_acceptable
> (item, (const uint8_t *)mask,
> - (const uint8_t *)&rte_flow_item_icmp_mask,
> + (const uint8_t *)&nic_mask,
> sizeof(struct rte_flow_item_icmp), error);
> if (ret < 0)
> return ret;
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index 3819cdb266..b5d6455067 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -7378,6 +7378,8 @@ flow_dv_translate_item_icmp(void *matcher, void
> *key,
> {
> const struct rte_flow_item_icmp *icmp_m = item->mask;
> const struct rte_flow_item_icmp *icmp_v = item->spec;
> + uint32_t icmp_header_data_m = 0;
> + uint32_t icmp_header_data_v = 0;
> void *headers_m;
> void *headers_v;
> void *misc3_m = MLX5_ADDR_OF(fte_match_param, matcher,
> @@ -7396,8 +7398,14 @@ flow_dv_translate_item_icmp(void *matcher, void
> *key,
> MLX5_SET(fte_match_set_lyr_2_4, headers_v, ip_protocol,
> IPPROTO_ICMP);
> if (!icmp_v)
> return;
> - if (!icmp_m)
> + if (!icmp_m) {
> icmp_m = &rte_flow_item_icmp_mask;
> + icmp_header_data_m = RTE_BE32(UINT32_MAX);
> + } else {
> + icmp_header_data_m = rte_cpu_to_be_16(icmp_m-
> >hdr.icmp_seq_nb);
> + icmp_header_data_m |=
> + rte_cpu_to_be_16(icmp_m->hdr.icmp_ident) << 16;
> + }
Yes but there is no need to add new fields to the mask, it will break existing applications.
So please remove it.
> /*
> * Force flow only to match the non-fragmented IPv4 ICMP packets.
> * If only the protocol is specified, no need to match the frag.
> @@ -7412,6 +7420,12 @@ flow_dv_translate_item_icmp(void *matcher, void
> *key,
> icmp_m->hdr.icmp_code);
> MLX5_SET(fte_match_set_misc3, misc3_v, icmp_code,
> icmp_v->hdr.icmp_code & icmp_m->hdr.icmp_code);
> + icmp_header_data_v = rte_cpu_to_be_16(icmp_v->hdr.icmp_seq_nb);
> + icmp_header_data_v |= rte_cpu_to_be_16(icmp_v->hdr.icmp_ident)
> << 16;
Why is it not BE? From the structure definition:
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;
Also you are setting cpu value with BE.
Maybe you want to rte_be_to_cpu?
> + MLX5_SET(fte_match_set_misc3, misc3_m, icmp_header_data,
> + icmp_header_data_m);
> + MLX5_SET(fte_match_set_misc3, misc3_v, icmp_header_data,
> + icmp_header_data_v & icmp_header_data_m);
> }
>
> /**
> --
> 2.21.0
Hi Ori,
Thanks for your comments.
My answer inline.
I will update them in V4 patch.
Regards,
Li Zhang
> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Thursday, October 1, 2020 4:14 PM
> To: Li Zhang <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Raslan Darawsheh <rasland@nvidia.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/1] net/mlx5: support match ICMP
> identifier fields
>
> Hi
> Sorry I didn't see that you sent V3 and responded on V2 So just rewriting my
> comments.
>
> Best,
> Ori
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Li Zhang
> > Subject: [dpdk-dev] [PATCH v3 1/1] net/mlx5: support match ICMP
> > identifier fields
> >
> > PRM expose fields "Icmp_header_data" in IPv4 ICMP.
> > Update ICMP mask parameter with ICMP identifier and sequence number
> > fields.
> > ICMP sequence number spec with mask, Icmp_header_data low 16 bits are
> set.
> > ICMP identifier spec with mask, Icmp_header_data high 16 bits are set.
> >
> > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > ---
> > doc/guides/nics/mlx5.rst | 4 ++--
> > doc/guides/rel_notes/release_20_11.rst | 2 +-
> > drivers/net/mlx5/mlx5_flow.c | 10 ++++++++--
> > drivers/net/mlx5/mlx5_flow_dv.c | 16 +++++++++++++++-
> > 4 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> > 211c0c5a6c..576dbe5efd 100644
> > --- a/doc/guides/nics/mlx5.rst
> > +++ b/doc/guides/nics/mlx5.rst
> > @@ -288,7 +288,7 @@ Limitations
> > - The input buffer, providing the removal size, is not validated.
> > - The buffer size must match the length of the headers to be removed.
> >
> > -- ICMP/ICMP6 code/type matching, IP-in-IP and MPLS flow matching are
> > all
> > +- ICMP(code/type/identifier/sequence number) / ICMP6(code/type)
> > +matching,
> > IP-in-IP and MPLS flow matching are all
> > mutually exclusive features which cannot be supported together
> > (see :ref:`mlx5_firmware_config`).
> >
> > @@ -1009,7 +1009,7 @@ Below are some firmware configurations listed.
> >
> > FLEX_PARSER_PROFILE_ENABLE=1
> >
> > -- enable ICMP/ICMP6 code/type fields matching::
> > +- enable ICMP(code/type/identifier/sequence number) /
> > +ICMP6(code/type)
> > fields matching::
> >
> > FLEX_PARSER_PROFILE_ENABLE=2
> >
> > diff --git a/doc/guides/rel_notes/release_20_11.rst
> > b/doc/guides/rel_notes/release_20_11.rst
> > index c6642f5f94..791f133d8f 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -73,7 +73,7 @@ New Features
> > * Added flag action.
> > * Added raw encap/decap actions.
> > * Added VXLAN encap/decap actions.
> > - * Added ICMP and ICMP6 matching items.
> > + * Added ICMP(code/type/identifier/sequence number) and
> > ICMP6(code/type) matching items.
> > * Added option to set port mask for insertion/deletion:
> > ``--portmask=N``
> > where N represents the hexadecimal bitmask of ports used.
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index 416505f1c8..3cabfd4627 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -1303,6 +1303,12 @@ mlx5_flow_validate_item_icmp(const struct
> > rte_flow_item *item,
> > struct rte_flow_error *error) {
> > const struct rte_flow_item_icmp *mask = item->mask;
> > + const struct rte_flow_item_icmp nic_mask = {
> > + .hdr.icmp_type = 0xff,
> > + .hdr.icmp_code = 0xff,
> > + .hdr.icmp_ident = RTE_BE16(0xffff),
> > + .hdr.icmp_seq_nb = RTE_BE16(0xffff),
> > + };
> > const int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
> > const uint64_t l3m = tunnel ? MLX5_FLOW_LAYER_INNER_L3_IPV4 :
> > MLX5_FLOW_LAYER_OUTER_L3_IPV4;
> @@ -1325,10 +1331,10 @@
> > mlx5_flow_validate_item_icmp(const struct rte_flow_item *item,
> > RTE_FLOW_ERROR_TYPE_ITEM,
> > item,
> > "multiple L4 layers not supported");
> > if (!mask)
> > - mask = &rte_flow_item_icmp_mask;
> > + mask = &nic_mask;
> > ret = mlx5_flow_item_acceptable
> > (item, (const uint8_t *)mask,
> > - (const uint8_t *)&rte_flow_item_icmp_mask,
> > + (const uint8_t *)&nic_mask,
> > sizeof(struct rte_flow_item_icmp), error);
> > if (ret < 0)
> > return ret;
> > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> > b/drivers/net/mlx5/mlx5_flow_dv.c index 3819cdb266..b5d6455067
> 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -7378,6 +7378,8 @@ flow_dv_translate_item_icmp(void *matcher,
> void
> > *key, {
> > const struct rte_flow_item_icmp *icmp_m = item->mask;
> > const struct rte_flow_item_icmp *icmp_v = item->spec;
> > + uint32_t icmp_header_data_m = 0;
> > + uint32_t icmp_header_data_v = 0;
> > void *headers_m;
> > void *headers_v;
> > void *misc3_m = MLX5_ADDR_OF(fte_match_param, matcher, @@ -
> 7396,8
> > +7398,14 @@ flow_dv_translate_item_icmp(void *matcher, void *key,
> > MLX5_SET(fte_match_set_lyr_2_4, headers_v, ip_protocol,
> > IPPROTO_ICMP);
> > if (!icmp_v)
> > return;
> > - if (!icmp_m)
> > + if (!icmp_m) {
> > icmp_m = &rte_flow_item_icmp_mask;
> > + icmp_header_data_m = RTE_BE32(UINT32_MAX);
> > + } else {
> > + icmp_header_data_m = rte_cpu_to_be_16(icmp_m-
> > >hdr.icmp_seq_nb);
> > + icmp_header_data_m |=
> > + rte_cpu_to_be_16(icmp_m->hdr.icmp_ident) << 16;
> > + }
>
> Yes but there is no need to add new fields to the mask, it will break existing
> applications.
> So please remove it.
>
Thanks, I will remove &rte_flow_item_icmp_mask.
For icmp_header_data_m, it is used to merge 16bit icmp_seq_nb and 16bit icmp_ident into 32bit data.
So that it can use icmp_header_data_m set 32bit misc3_m directly.
> > /*
> > * Force flow only to match the non-fragmented IPv4 ICMP packets.
> > * If only the protocol is specified, no need to match the frag.
> > @@ -7412,6 +7420,12 @@ flow_dv_translate_item_icmp(void *matcher,
> void
> > *key,
> > icmp_m->hdr.icmp_code);
> > MLX5_SET(fte_match_set_misc3, misc3_v, icmp_code,
> > icmp_v->hdr.icmp_code & icmp_m->hdr.icmp_code);
> > + icmp_header_data_v = rte_cpu_to_be_16(icmp_v-
> >hdr.icmp_seq_nb);
> > + icmp_header_data_v |= rte_cpu_to_be_16(icmp_v->hdr.icmp_ident)
> > << 16;
>
> Why is it not BE? From the structure definition:
> 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; Also you are setting cpu value with BE.
> Maybe you want to rte_be_to_cpu?
>
You are right I should use the rte_be_to_cpu_16 instead of rte_cpu_to_be_16.
> > + MLX5_SET(fte_match_set_misc3, misc3_m, icmp_header_data,
> > + icmp_header_data_m);
> > + MLX5_SET(fte_match_set_misc3, misc3_v, icmp_header_data,
> > + icmp_header_data_v & icmp_header_data_m);
> > }
> >
> > /**
> > --
> > 2.21.0
@@ -288,7 +288,7 @@ Limitations
- The input buffer, providing the removal size, is not validated.
- The buffer size must match the length of the headers to be removed.
-- ICMP/ICMP6 code/type matching, IP-in-IP and MPLS flow matching are all
+- ICMP(code/type/identifier/sequence number) / ICMP6(code/type) matching, IP-in-IP and MPLS flow matching are all
mutually exclusive features which cannot be supported together
(see :ref:`mlx5_firmware_config`).
@@ -1009,7 +1009,7 @@ Below are some firmware configurations listed.
FLEX_PARSER_PROFILE_ENABLE=1
-- enable ICMP/ICMP6 code/type fields matching::
+- enable ICMP(code/type/identifier/sequence number) / ICMP6(code/type) fields matching::
FLEX_PARSER_PROFILE_ENABLE=2
@@ -73,7 +73,7 @@ New Features
* Added flag action.
* Added raw encap/decap actions.
* Added VXLAN encap/decap actions.
- * Added ICMP and ICMP6 matching items.
+ * Added ICMP(code/type/identifier/sequence number) and ICMP6(code/type) matching items.
* Added option to set port mask for insertion/deletion:
``--portmask=N``
where N represents the hexadecimal bitmask of ports used.
@@ -1303,6 +1303,12 @@ mlx5_flow_validate_item_icmp(const struct rte_flow_item *item,
struct rte_flow_error *error)
{
const struct rte_flow_item_icmp *mask = item->mask;
+ const struct rte_flow_item_icmp nic_mask = {
+ .hdr.icmp_type = 0xff,
+ .hdr.icmp_code = 0xff,
+ .hdr.icmp_ident = RTE_BE16(0xffff),
+ .hdr.icmp_seq_nb = RTE_BE16(0xffff),
+ };
const int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
const uint64_t l3m = tunnel ? MLX5_FLOW_LAYER_INNER_L3_IPV4 :
MLX5_FLOW_LAYER_OUTER_L3_IPV4;
@@ -1325,10 +1331,10 @@ mlx5_flow_validate_item_icmp(const struct rte_flow_item *item,
RTE_FLOW_ERROR_TYPE_ITEM, item,
"multiple L4 layers not supported");
if (!mask)
- mask = &rte_flow_item_icmp_mask;
+ mask = &nic_mask;
ret = mlx5_flow_item_acceptable
(item, (const uint8_t *)mask,
- (const uint8_t *)&rte_flow_item_icmp_mask,
+ (const uint8_t *)&nic_mask,
sizeof(struct rte_flow_item_icmp), error);
if (ret < 0)
return ret;
@@ -7378,6 +7378,8 @@ flow_dv_translate_item_icmp(void *matcher, void *key,
{
const struct rte_flow_item_icmp *icmp_m = item->mask;
const struct rte_flow_item_icmp *icmp_v = item->spec;
+ uint32_t icmp_header_data_m = 0;
+ uint32_t icmp_header_data_v = 0;
void *headers_m;
void *headers_v;
void *misc3_m = MLX5_ADDR_OF(fte_match_param, matcher,
@@ -7396,8 +7398,14 @@ flow_dv_translate_item_icmp(void *matcher, void *key,
MLX5_SET(fte_match_set_lyr_2_4, headers_v, ip_protocol, IPPROTO_ICMP);
if (!icmp_v)
return;
- if (!icmp_m)
+ if (!icmp_m) {
icmp_m = &rte_flow_item_icmp_mask;
+ icmp_header_data_m = RTE_BE32(UINT32_MAX);
+ } else {
+ icmp_header_data_m = rte_cpu_to_be_16(icmp_m->hdr.icmp_seq_nb);
+ icmp_header_data_m |=
+ rte_cpu_to_be_16(icmp_m->hdr.icmp_ident) << 16;
+ }
/*
* Force flow only to match the non-fragmented IPv4 ICMP packets.
* If only the protocol is specified, no need to match the frag.
@@ -7412,6 +7420,12 @@ flow_dv_translate_item_icmp(void *matcher, void *key,
icmp_m->hdr.icmp_code);
MLX5_SET(fte_match_set_misc3, misc3_v, icmp_code,
icmp_v->hdr.icmp_code & icmp_m->hdr.icmp_code);
+ icmp_header_data_v = rte_cpu_to_be_16(icmp_v->hdr.icmp_seq_nb);
+ icmp_header_data_v |= rte_cpu_to_be_16(icmp_v->hdr.icmp_ident) << 16;
+ MLX5_SET(fte_match_set_misc3, misc3_m, icmp_header_data,
+ icmp_header_data_m);
+ MLX5_SET(fte_match_set_misc3, misc3_v, icmp_header_data,
+ icmp_header_data_v & icmp_header_data_m);
}
/**