Re: [dpdk-dev] [PATCH v2 1/1] net/mlx5: support match ICMP identifier fields

[Li Zhang <lizh@nvidia.com>]

Hi Ori,

Thanks for your comments, please take a look my reply inline.

Regards,
Li Zhang
> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Wednesday, September 30, 2020 10:46 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: [PATCH v2 1/1] net/mlx5: support match ICMP identifier fields
> 
> Hi Li,
> 
> PSB,
> 
> Thanks,
> Ori
> 
> > -----Original Message-----
> > From: Li Zhang <lizh@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 <lizh@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