Hi Ori, Thanks for your comments, please take a look my reply inline.
Regards, Li Zhang > -----Original Message----- > From: Ori Kam <or...@nvidia.com> > Sent: Wednesday, September 30, 2020 10:46 PM > To: Li Zhang <l...@nvidia.com>; Dekel Peled <dek...@nvidia.com>; Slava > Ovsiienko <viachesl...@nvidia.com>; Matan Azrad <ma...@nvidia.com> > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon > <tho...@monjalon.net>; Raslan Darawsheh <rasl...@nvidia.com> > Subject: RE: [PATCH v2 1/1] net/mlx5: support match ICMP identifier fields > > Hi Li, > > PSB, > > Thanks, > Ori > > > -----Original Message----- > > From: Li Zhang <l...@nvidia.com> > > Sent: Monday, September 28, 2020 6:38 AM > > Subject: [PATCH v2 1/1] net/mlx5: support match ICMP identifier fields > > > > PRM expose fields "Icmp_header_data" in 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. > > > Is it relevant only for ipv4 ICMP? Yes it is only for ipv4 ICMP. For Ipv6 ICMP, it is icmpv6_header_data. > > > Signed-off-by: Li Zhang <l...@nvidia.com> > > --- > > doc/guides/nics/mlx5.rst | 4 ++-- > > doc/guides/rel_notes/release_20_11.rst | 2 +- > > drivers/net/mlx5/mlx5_flow.c | 9 +++++++-- > > drivers/net/mlx5/mlx5_flow_dv.c | 16 +++++++++++++++- > > 4 files changed, 25 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..7bd5c5da94 100644 > > --- a/drivers/net/mlx5/mlx5_flow.c > > +++ b/drivers/net/mlx5/mlx5_flow.c > > @@ -1303,6 +1303,7 @@ mlx5_flow_validate_item_icmp(const struct > > rte_flow_item *item, > > struct rte_flow_error *error) > > This function is shared between the dv and the verbs. > I think we can support this only in dv, so we need to split this function. Thanks, I will split this function in v3 patch. > > > { > > const struct rte_flow_item_icmp *mask = item->mask; > > + struct rte_flow_item_icmp default_mask; > > I think the correct name is nic_mask. Thanks, I will change in v3 patch. > > > 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; > @@ -1324,11 +1325,15 @@ > > mlx5_flow_validate_item_icmp(const struct rte_flow_item *item, > > return rte_flow_error_set(error, EINVAL, > > RTE_FLOW_ERROR_TYPE_ITEM, > > item, > > "multiple L4 layers not supported"); > > + memcpy(&default_mask, &rte_flow_item_icmp_mask, > > + sizeof(struct rte_flow_item_icmp)); > > + default_mask.hdr.icmp_ident = RTE_BE16(0xFFFF); > > + default_mask.hdr.icmp_seq_nb = RTE_BE16(0xFFFF); > > You don't need to mem copy just init this structure when you declare it. Thanks, I will change in v3 patch. > > > if (!mask) > > - mask = &rte_flow_item_icmp_mask; > > + mask = &default_mask; > > ret = mlx5_flow_item_acceptable > > (item, (const uint8_t *)mask, > > - (const uint8_t *)&rte_flow_item_icmp_mask, > > + (const uint8_t *)&default_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); > > Why are you setting the data as default mask? > Because icmp_code and icmp_type still need the default mask. We do not need to change the old one. We just add the new one for our new match fields. > > + } else { > > + icmp_header_data_m = rte_cpu_to_be_16(icmp_m- > > >hdr.icmp_seq_nb); > > Isn't the icmp_seq_nb already in BE? No, it is not BE and need use rte_cpu_to_be_16 > > > + icmp_header_data_m |= > > + rte_cpu_to_be_16(icmp_m->hdr.icmp_ident) << 16; > > Again the icmp_ident is already in BE > It is not BE and need use rte_cpu_to_be_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; > > The BE issue again, It is not BE and need use 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