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?
> 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.
> {
> const struct rte_flow_item_icmp *mask = item->mask;
> + struct rte_flow_item_icmp default_mask;
I think the correct name is nic_mask.
> 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.
> 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?
> + } 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?
> + icmp_header_data_m |=
> + rte_cpu_to_be_16(icmp_m->hdr.icmp_ident) << 16;
Again the icmp_ident is already in BE
> + }
> /*
> * 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,
> + 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
