[dpdk-dev,v2,3/4] ether: add more protocol support in flow API

  Add new protocol header match support as below

RTE_FLOW_ITEM_TYPE_ARP
	- match IPv4 ARP header
RTE_FLOW_ITEM_TYPE_EXT_HDR_ANY
	- match any IPv6 extension header
RTE_FLOW_ITEM_TYPE_ICMPV6
	- match IPv6 ICMP header
RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
	- match IPv6 ICMP Target address
RTE_FLOW_ITEM_TYPE_ICMPV6_SSL
	- match IPv6 ICMP Source Link-layer address
RTE_FLOW_ITEM_TYPE_ICMPV6_TTL
	- match IPv6 ICMP Target Link-layer address

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_ether/rte_flow.h | 160 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 160 insertions(+)
  

On Sun, Apr 01, 2018 at 05:19:21PM -0400, Qi Zhang wrote:
> Add new protocol header match support as below
> 
> RTE_FLOW_ITEM_TYPE_ARP
> 	- match IPv4 ARP header
> RTE_FLOW_ITEM_TYPE_EXT_HDR_ANY
> 	- match any IPv6 extension header

While properly defined in the patch, "IPV6" is missing here.

> RTE_FLOW_ITEM_TYPE_ICMPV6
> 	- match IPv6 ICMP header
> RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
> 	- match IPv6 ICMP Target address
> RTE_FLOW_ITEM_TYPE_ICMPV6_SSL
> 	- match IPv6 ICMP Source Link-layer address
> RTE_FLOW_ITEM_TYPE_ICMPV6_TTL
> 	- match IPv6 ICMP Target Link-layer address
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>

First, since they are added at the end of enum rte_flow_item_type, no ABI
breakage notice is necessary.

However testpmd implementation [1][2] and documentation update [3][4] are
mandatory for all new pattern items and actions.

More comments below regarding these definitions.

[1] flow_item[] in app/test-pmd/config.c
[2] using ITEM_ICMP as an example in app/test-pmd/cmdline_flow.c
[3] "Pattern items" section in doc/guides/testpmd_app_ug/testpmd_funcs.rst
[4] using "Item: ``ICMP``" section as an example in
    doc/guides/prog_guide/rte_flow.rst

> ---
>  lib/librte_ether/rte_flow.h | 160 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 160 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 8f75db0..a8ec780 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -323,6 +323,49 @@ enum rte_flow_item_type {
>  	 * See struct rte_flow_item_geneve.
>  	 */
>  	RTE_FLOW_ITEM_TYPE_GENEVE,
> +
> +	/**
> +	 * Matches ARP IPv4 header.

=> Matches an IPv4 ARP header.

> +	 *
> +	 * See struct rte_flow_item_arp.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_ARP,

While you're right to make "IPv4" clear since ARP is also used for other
protocols DPDK doesn't support (and likely never will), the ARP header has
both a fixed and a variably-sized part.

Ideally an ARP pattern item should match the fixed part only and a separate
ARP_IPV4 match its payload, somewhat like you did for ICMPv6/NDP below.

Problem is that in DPDK, struct arp_hdr includes struct arp_ipv4, so one
suggestion would be to rename this pattern item ARP_IPV4 directly:

=> RTE_FLOW_ITEM_TYPE_ARP_IPV4

> +
> +	/**
> +	 * Matches any IPv6 Extension header.

=> Matches an IPv6 extension header.

> +	 *
> +	 * See struct rte_flow_item_ipv6_ext_any.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY,

I'm not sure this definition is necessary, more below about that.

Also I don't see a benefit in having "ANY" part of the name, if you want to
keep it, I suggest the simpler:

=> RTE_FLOW_ITEM_TYPE_IPV6_EXT

> +
> +	/**
> +	 * Matches ICMPv6 header.

=> Matches an ICMPv6 header.

> +	 *
> +	 * See struct rte_flow_item_icmpv6

Missing "."

> +	 */
> +	RTE_FLOW_ITEM_TYPE_ICMPV6,
> +

Before entering NDP territory below, I understand those should be stacked on
top of RTE_FLOW_ITEM_TYPE_ICMPV6. It's fine but for clarity they should be
named after the NDP types they represent, not inner data fields.

Also I think we should consider NDP as a protocol sitting on top of
ICMPv6. We could therefore drop "ICMP" from these definitions.

Since "ND" is a common shorthand for this protocol and "6" another when
doing something related to IPv6, I suggest to use "ND6" to name he related
pattern items.

These are the reasons behind my next suggestions:

> +	/**
> +	 * Match ICMPv6 target address.
> +	 *
> +	 * See struct rte_flow_item_icmpv6_tgt_addr.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR,

=> Matches an IPv6 network discovery router solicitation.
=> See struct rte_flow_item_nd6_rs.
=> RTE_FLOW_ITEM_TYPE_ND6_RS,

You should add another item for neighbor advertisement messages using the
same template:

=> Match an IPv6 network discovery neighbor advertisement.
=> See struct rte_flow_item_nd6_na.
=> RTE_FLOW_ITEM_TYPE_ND6_NA,

The following are possible options for these headers, if specified they must
be found afterward. Also since IPv6 may run on top of protocols other than
Ethernet, you need to clarify these link-layer addresses use the Ethernet
format:

> +
> +	/**
> +	 * Match ICMPv6 Source Link-Layer Address.
> +	 *
> +	 * See struct rte_flow_item_icmpv6_sll.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_ICMPV6_SLL,

=> Matches an IPv6 network discovery source Ethernet link-layer address option.
=> See struct rte_flow_item_nd6_opt_sla_eth.
=> RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH,

> +
> +	/**
> +	 * Match ICMPv6 Target Link-Layer Address.
> +	 *
> +	 * See struct rte_flow_item_icmpv6_tll.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_ICMPV6_TLL,

=> Matches an IPv6 network discovery target Ethernet link-layer address option.
=> See struct rte_flow_item_nd6_opt_tla_eth.
=> RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH,

> +

Unnecessary empty line.

>  };
>  
>  /**
> @@ -815,6 +858,123 @@ static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
>  #endif
>  
>  /**
> + * RTE_FLOW_ITEM_TYPE_ARP
> + *
> + * Matches IPv4 ARP packet header

As above:

=> Matches an IPv4 ARP header.
=> RTE_FLOW_ITEM_TYPE_ARP_IPV4

> + */
> +struct rte_flow_item_arp {
> +	struct arp_hdr hdr;
> +};

Needs #include <rte_arp.h> and a Doxygen comment next to hdr for
consistency, see ICMP and other definitions.

> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_ARP. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_arp rte_flow_item_arp_mask = {
> +	.hdr = {
> +		.arp_data = {
> +			.arp_sha = {
> +				.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> +			},
> +			.arp_sip = RTE_BE32(0xffffffff),
> +			.arp_tha = {
> +				.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> +			},
> +			.arp_tip = RTE_BE32(0xffffffff),
> +		},
> +	},
> +};
> +#endif
> +
> +/**
> + * RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY
> + *
> + * Matches any IPv6 extension header.
> + */
> +struct rte_flow_item_ipv6_ext_hdr_any {
> +	uint8_t next_hdr;
> +};

So what's the point? next_hdr is already part of either struct ipv6_hdr
("proto") and individual extension headers. Moreover it's implicit if an
extension header is provided in a pattern.

How about removing it?

> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY. */
> +#ifndef __cplusplus
> +static const
> +struct rte_flow_item_ipv6_ext_hdr_any rte_flow_item_ipv6_ext_any_mask = {
> +	.next_hdr = 0xff,
> +};
> +#endif

Ditto.

> +
> +/**
> + * RTE_FLOW_ITEM_TYPE_ICMPV6
> + *
> + * Matches ICMPv6 header.

=> Matches an ICMPv6 header.

> + */
> +struct rte_flow_item_icmpv6 {
> +	uint8_t type;
> +	uint8_t code;
> +	uint16_t checksum;

The last 32-bit "reserved" data field is missing.

> +};

Too bad there is no struct icmp6_hdr definition in rte_icmp.h. You could add
it. In any case Doxygen comments are missing, please add them (see other
structure definitions for examples).

> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6 */

Missing "."

> +#ifndef __cplusplus
> +static const struct rte_flow_item_icmpv6 rte_flow_item_icmpv6_mask = {
> +	.type = 0xff,
> +	.code = 0xff,
> +	.checksum = RTE_BE16(0xffff),
> +};
> +#endif

You must remove checksum matching from the default mask. That's the last
thing an application might want to match exactly :)

> +
> +/**
> + * RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
> + *
> + * Matches ICMPv6's Target Address.
> + */
> +struct rte_flow_item_icmpv6_tgt_addr {
> +	uint8_t addr[16];
> +};

You need to expand this as two items, see prior comments regarding
RTE_FLOW_ITEM_TYPE_ND6_RS, RTE_FLOW_ITEM_TYPE_ND6_NA and their respective
structs rte_flow_item_nd6_rs and rte_flow_item_nd6_na.

Also Doxygen documentation is missing for the addr field and you need to
describe that these are only valid when used after
RTE_FLOW_ITEM_TYPE_ICMPV6.

> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR */

Missing "."

> +#ifndef __cplusplus
> +static const
> +struct rte_flow_item_icmpv6_tgt_addr rte_flow_item_icmpv6_tgt_addr_mask = {
> +	.addr =
> +		"\xff\xff\xff\xff\xff\xff\xff\xff"
> +		"\xff\xff\xff\xff\xff\xff\xff\xff",
> +};
> +#endif
> +
> +/**
> + * RTE_FLOW_ITEM_TYPE_ICPMV6_SLL.
> + *
> + * Matches ICMPv6 Source Link-Layer address.
> + */
> +struct rte_flow_item_icmpv6_sll {
> +	struct ether_addr addr;
> +};

See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH and struct
rte_flow_item_type_nd6_opt_sla_eth.

Also Doxygen documentation is missing for the addr field and you need to
describe that it is only valid when found after either
RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA.

Also missing empty line here.

> +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_SLL */

Missing "."

> +#ifndef __cplusplus
> +static const struct rte_flow_item_icmpv6_sll rte_flow_item_icmpv6_sll_mask = {
> +	.addr = {
> +		.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> +	}
> +};
> +#endif
> +
> +/**
> + * RTE_FLOW_ITEM_TYPE_ICMPV6_TLL.
> + *
> + * Matches ICMPv6 Target Link-Layer address.
> + */
> +struct rte_flow_item_icmpv6_tll {
> +	struct ether_addr addr;
> +};

See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH and struct
rte_flow_item_type_nd6_opt_tla_eth.

Also Doxygen documentation is missing for the addr field and you need to
describe that it is only valid when found after either
RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA.

Also missing empty line here.

> +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TLL */

Missing "."

> +#ifndef __cplusplus
> +static const struct rte_flow_item_icmpv6_tll rte_flow_item_icmpv6_tll_mask = {
> +	.addr = {
> +		.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> +	}
> +};
> +#endif
> +
> +/**
>   * Matching pattern item definition.
>   *
>   * A pattern is formed by stacking items starting from the lowest protocol
> -- 
> 2.7.4
>
  

Hi Adrien:

	Thank you so much for your careful review and helpful suggestions!
	I agree with most of your comments, except couple question about RTE_FLOW_ITEM_TYPE_TGT_ADDR and RTE_FLOW_ITEM_IPV6_EXT_HDR
	Please see my comment inline.

Thanks!
Qi

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, April 12, 2018 12:32 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [PATCH v2 3/4] ether: add more protocol support in flow API
> 
> On Sun, Apr 01, 2018 at 05:19:21PM -0400, Qi Zhang wrote:
> > Add new protocol header match support as below
> >
> > RTE_FLOW_ITEM_TYPE_ARP
> > 	- match IPv4 ARP header
> > RTE_FLOW_ITEM_TYPE_EXT_HDR_ANY
> > 	- match any IPv6 extension header
> 
> While properly defined in the patch, "IPV6" is missing here.
> 
> > RTE_FLOW_ITEM_TYPE_ICMPV6
> > 	- match IPv6 ICMP header
> > RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
> > 	- match IPv6 ICMP Target address
> > RTE_FLOW_ITEM_TYPE_ICMPV6_SSL
> > 	- match IPv6 ICMP Source Link-layer address
> > RTE_FLOW_ITEM_TYPE_ICMPV6_TTL
> > 	- match IPv6 ICMP Target Link-layer address
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> 
> First, since they are added at the end of enum rte_flow_item_type, no ABI
> breakage notice is necessary.
> 
> However testpmd implementation [1][2] and documentation update [3][4] are
> mandatory for all new pattern items and actions.

OK, will add this into v2.

> 
> More comments below regarding these definitions.
> 
> [1] flow_item[] in app/test-pmd/config.c [2] using ITEM_ICMP as an example
> in app/test-pmd/cmdline_flow.c [3] "Pattern items" section in
> doc/guides/testpmd_app_ug/testpmd_funcs.rst
> [4] using "Item: ``ICMP``" section as an example in
>     doc/guides/prog_guide/rte_flow.rst
> 
> > ---
> >  lib/librte_ether/rte_flow.h | 160
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 160 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> > index 8f75db0..a8ec780 100644
> > --- a/lib/librte_ether/rte_flow.h
> > +++ b/lib/librte_ether/rte_flow.h
> > @@ -323,6 +323,49 @@ enum rte_flow_item_type {
> >  	 * See struct rte_flow_item_geneve.
> >  	 */
> >  	RTE_FLOW_ITEM_TYPE_GENEVE,
> > +
> > +	/**
> > +	 * Matches ARP IPv4 header.
> 
> => Matches an IPv4 ARP header.
> 
> > +	 *
> > +	 * See struct rte_flow_item_arp.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_ARP,
> 
> While you're right to make "IPv4" clear since ARP is also used for other
> protocols DPDK doesn't support (and likely never will), the ARP header has
> both a fixed and a variably-sized part.
> 
> Ideally an ARP pattern item should match the fixed part only and a separate
> ARP_IPV4 match its payload, somewhat like you did for ICMPv6/NDP below.
> 
> Problem is that in DPDK, struct arp_hdr includes struct arp_ipv4, so one
> suggestion would be to rename this pattern item ARP_IPV4 directly:
> 
> => RTE_FLOW_ITEM_TYPE_ARP_IPV4
> 
> > +
> > +	/**
> > +	 * Matches any IPv6 Extension header.
> 
> => Matches an IPv6 extension header.
> 
> > +	 *
> > +	 * See struct rte_flow_item_ipv6_ext_any.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY,
> 
> I'm not sure this definition is necessary, more below about that.
> 
> Also I don't see a benefit in having "ANY" part of the name, if you want to keep
> it, I suggest the simpler:
> 
> => RTE_FLOW_ITEM_TYPE_IPV6_EXT
> 
> > +
> > +	/**
> > +	 * Matches ICMPv6 header.
> 
> => Matches an ICMPv6 header.
> 
> > +	 *
> > +	 * See struct rte_flow_item_icmpv6
> 
> Missing "."
> 
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_ICMPV6,
> > +
> 
> Before entering NDP territory below, I understand those should be stacked on
> top of RTE_FLOW_ITEM_TYPE_ICMPV6. It's fine but for clarity they should be
> named after the NDP types they represent, not inner data fields.
> 
> Also I think we should consider NDP as a protocol sitting on top of ICMPv6. We
> could therefore drop "ICMP" from these definitions.
> 
> Since "ND" is a common shorthand for this protocol and "6" another when
> doing something related to IPv6, I suggest to use "ND6" to name he related
> pattern items.

I agree.

> 
> These are the reasons behind my next suggestions:
> 
> > +	/**
> > +	 * Match ICMPv6 target address.
> > +	 *
> > +	 * See struct rte_flow_item_icmpv6_tgt_addr.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR,
> 
> => Matches an IPv6 network discovery router solicitation.
> => See struct rte_flow_item_nd6_rs.
> => RTE_FLOW_ITEM_TYPE_ND6_RS,
> 
> You should add another item for neighbor advertisement messages using the
> same template:
> 
> => Match an IPv6 network discovery neighbor advertisement.
> => See struct rte_flow_item_nd6_na.
> => RTE_FLOW_ITEM_TYPE_ND6_NA,

The purpose of RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR is to match a "target address"
according to IPv6 ND spec https://tools.ietf.org/html/rfc4861#page-22, when type = 135/136

so do you mean we should have RTE_FLOW_ITEM_TYPE_ND6_NS (Neighbor Solicitation)
 and RTE_FLOW_ITEM_TYPE_ND6_NA (Neighbor Advertisement) here,
and with the same template (an IPV6 addr) for rte_flow_item_icmpv6_tgt_addr?

> 
> The following are possible options for these headers, if specified they must be
> found afterward. Also since IPv6 may run on top of protocols other than
> Ethernet, you need to clarify these link-layer addresses use the Ethernet
> format:
> 
> > +
> > +	/**
> > +	 * Match ICMPv6 Source Link-Layer Address.
> > +	 *
> > +	 * See struct rte_flow_item_icmpv6_sll.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_ICMPV6_SLL,
> 
> => Matches an IPv6 network discovery source Ethernet link-layer address
> option.
> => See struct rte_flow_item_nd6_opt_sla_eth.
> => RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH,
> 
> > +
> > +	/**
> > +	 * Match ICMPv6 Target Link-Layer Address.
> > +	 *
> > +	 * See struct rte_flow_item_icmpv6_tll.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_ICMPV6_TLL,
> 
> => Matches an IPv6 network discovery target Ethernet link-layer address
> option.
> => See struct rte_flow_item_nd6_opt_tla_eth.
> => RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH,
> 

Agree to rename.

> > +
> 
> Unnecessary empty line.
> 
> >  };
> >
> >  /**
> > @@ -815,6 +858,123 @@ static const struct rte_flow_item_geneve
> > rte_flow_item_geneve_mask = {  #endif
> >
> >  /**
> > + * RTE_FLOW_ITEM_TYPE_ARP
> > + *
> > + * Matches IPv4 ARP packet header
> 
> As above:
> 
> => Matches an IPv4 ARP header.
> => RTE_FLOW_ITEM_TYPE_ARP_IPV4
> 
> > + */
> > +struct rte_flow_item_arp {
> > +	struct arp_hdr hdr;
> > +};
> 
> Needs #include <rte_arp.h> and a Doxygen comment next to hdr for
> consistency, see ICMP and other definitions.
> 
> > +
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_ARP. */ #ifndef __cplusplus
> > +static const struct rte_flow_item_arp rte_flow_item_arp_mask = {
> > +	.hdr = {
> > +		.arp_data = {
> > +			.arp_sha = {
> > +				.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > +			},
> > +			.arp_sip = RTE_BE32(0xffffffff),
> > +			.arp_tha = {
> > +				.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > +			},
> > +			.arp_tip = RTE_BE32(0xffffffff),
> > +		},
> > +	},
> > +};
> > +#endif
> > +
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY
> > + *
> > + * Matches any IPv6 extension header.
> > + */
> > +struct rte_flow_item_ipv6_ext_hdr_any {
> > +	uint8_t next_hdr;
> > +};
> 
> So what's the point? next_hdr is already part of either struct ipv6_hdr
> ("proto") and individual extension headers. Moreover it's implicit if an
> extension header is provided in a pattern.
> 
> How about removing it?

We need this to match a packet that have extend header
For example:
IPV6(proto = 43, <Routing EH >) / EXT_HDR (next_head = 60 <Destination EH>) / EXT_HDR (next_head = 44, <Fragment EH)/ EXT_HDR (next_head = 6 <tcp>) / TCP ...

I use "ANY" to match any extend header regardless their content.
There is no conflict if we can add multiple RTE_FLOW_ITEM_EXT_HDR_XXX in futures

> 
> > +
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY. */
> #ifndef
> > +__cplusplus static const struct rte_flow_item_ipv6_ext_hdr_any
> > +rte_flow_item_ipv6_ext_any_mask = {
> > +	.next_hdr = 0xff,
> > +};
> > +#endif
> 
> Ditto.
> 
> > +
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_ICMPV6
> > + *
> > + * Matches ICMPv6 header.
> 
> => Matches an ICMPv6 header.
> 
> > + */
> > +struct rte_flow_item_icmpv6 {
> > +	uint8_t type;
> > +	uint8_t code;
> > +	uint16_t checksum;
> 
> The last 32-bit "reserved" data field is missing.
> 
> > +};
> 
> Too bad there is no struct icmp6_hdr definition in rte_icmp.h. You could add it.
> In any case Doxygen comments are missing, please add them (see other
> structure definitions for examples).
> 
> > +
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6 */
> 
> Missing "."
> 
> > +#ifndef __cplusplus
> > +static const struct rte_flow_item_icmpv6 rte_flow_item_icmpv6_mask = {
> > +	.type = 0xff,
> > +	.code = 0xff,
> > +	.checksum = RTE_BE16(0xffff),
> > +};
> > +#endif
> 
> You must remove checksum matching from the default mask. That's the last
> thing an application might want to match exactly :)
> 
> > +
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
> > + *
> > + * Matches ICMPv6's Target Address.
> > + */
> > +struct rte_flow_item_icmpv6_tgt_addr {
> > +	uint8_t addr[16];
> > +};
> 
> You need to expand this as two items, see prior comments regarding
> RTE_FLOW_ITEM_TYPE_ND6_RS, RTE_FLOW_ITEM_TYPE_ND6_NA and their
> respective structs rte_flow_item_nd6_rs and rte_flow_item_nd6_na.
> 
> Also Doxygen documentation is missing for the addr field and you need to
> describe that these are only valid when used after
> RTE_FLOW_ITEM_TYPE_ICMPV6.
> 
> > +
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR */
> 
> Missing "."
> 
> > +#ifndef __cplusplus
> > +static const
> > +struct rte_flow_item_icmpv6_tgt_addr
> rte_flow_item_icmpv6_tgt_addr_mask = {
> > +	.addr =
> > +		"\xff\xff\xff\xff\xff\xff\xff\xff"
> > +		"\xff\xff\xff\xff\xff\xff\xff\xff",
> > +};
> > +#endif
> > +
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_ICPMV6_SLL.
> > + *
> > + * Matches ICMPv6 Source Link-Layer address.
> > + */
> > +struct rte_flow_item_icmpv6_sll {
> > +	struct ether_addr addr;
> > +};
> 
> See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH and
> struct rte_flow_item_type_nd6_opt_sla_eth.
> 
> Also Doxygen documentation is missing for the addr field and you need to
> describe that it is only valid when found after either
> RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA.
> 
> Also missing empty line here.
> 
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_SLL */
> 
> Missing "."
> 
> > +#ifndef __cplusplus
> > +static const struct rte_flow_item_icmpv6_sll
> rte_flow_item_icmpv6_sll_mask = {
> > +	.addr = {
> > +		.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > +	}
> > +};
> > +#endif
> > +
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TLL.
> > + *
> > + * Matches ICMPv6 Target Link-Layer address.
> > + */
> > +struct rte_flow_item_icmpv6_tll {
> > +	struct ether_addr addr;
> > +};
> 
> See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH
> and struct rte_flow_item_type_nd6_opt_tla_eth.
> 
> Also Doxygen documentation is missing for the addr field and you need to
> describe that it is only valid when found after either
> RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA.
> 
> Also missing empty line here.
> 
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TLL */
> 
> Missing "."
> 
> > +#ifndef __cplusplus
> > +static const struct rte_flow_item_icmpv6_tll
> rte_flow_item_icmpv6_tll_mask = {
> > +	.addr = {
> > +		.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > +	}
> > +};
> > +#endif
> > +
> > +/**
> >   * Matching pattern item definition.
> >   *
> >   * A pattern is formed by stacking items starting from the lowest
> > protocol
> > --
> > 2.7.4
> >
> 
> --
> Adrien Mazarguil
> 6WIND
  

On Thu, Apr 12, 2018 at 05:12:08AM +0000, Zhang, Qi Z wrote:
> Hi Adrien:
> 
> 	Thank you so much for your careful review and helpful suggestions!
> 	I agree with most of your comments, except couple question about RTE_FLOW_ITEM_TYPE_TGT_ADDR and RTE_FLOW_ITEM_IPV6_EXT_HDR
> 	Please see my comment inline.
> 
> Thanks!
> Qi

Thanks, replying inline also.

> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Thursday, April 12, 2018 12:32 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> > Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>
> > Subject: Re: [PATCH v2 3/4] ether: add more protocol support in flow API
> > 
> > On Sun, Apr 01, 2018 at 05:19:21PM -0400, Qi Zhang wrote:
> > > Add new protocol header match support as below
> > >
> > > RTE_FLOW_ITEM_TYPE_ARP
> > > 	- match IPv4 ARP header
> > > RTE_FLOW_ITEM_TYPE_EXT_HDR_ANY
> > > 	- match any IPv6 extension header
> > 
> > While properly defined in the patch, "IPV6" is missing here.
> > 
> > > RTE_FLOW_ITEM_TYPE_ICMPV6
> > > 	- match IPv6 ICMP header
> > > RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
> > > 	- match IPv6 ICMP Target address
> > > RTE_FLOW_ITEM_TYPE_ICMPV6_SSL
> > > 	- match IPv6 ICMP Source Link-layer address
> > > RTE_FLOW_ITEM_TYPE_ICMPV6_TTL
> > > 	- match IPv6 ICMP Target Link-layer address
> > >
> > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > 
> > First, since they are added at the end of enum rte_flow_item_type, no ABI
> > breakage notice is necessary.
> > 
> > However testpmd implementation [1][2] and documentation update [3][4] are
> > mandatory for all new pattern items and actions.
> 
> OK, will add this into v2.
> 
> > 
> > More comments below regarding these definitions.
> > 
> > [1] flow_item[] in app/test-pmd/config.c [2] using ITEM_ICMP as an example
> > in app/test-pmd/cmdline_flow.c [3] "Pattern items" section in
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > [4] using "Item: ``ICMP``" section as an example in
> >     doc/guides/prog_guide/rte_flow.rst
> > 
> > > ---
> > >  lib/librte_ether/rte_flow.h | 160
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 160 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> > > index 8f75db0..a8ec780 100644
> > > --- a/lib/librte_ether/rte_flow.h
> > > +++ b/lib/librte_ether/rte_flow.h
> > > @@ -323,6 +323,49 @@ enum rte_flow_item_type {
> > >  	 * See struct rte_flow_item_geneve.
> > >  	 */
> > >  	RTE_FLOW_ITEM_TYPE_GENEVE,
> > > +
> > > +	/**
> > > +	 * Matches ARP IPv4 header.
> > 
> > => Matches an IPv4 ARP header.
> > 
> > > +	 *
> > > +	 * See struct rte_flow_item_arp.
> > > +	 */
> > > +	RTE_FLOW_ITEM_TYPE_ARP,
> > 
> > While you're right to make "IPv4" clear since ARP is also used for other
> > protocols DPDK doesn't support (and likely never will), the ARP header has
> > both a fixed and a variably-sized part.
> > 
> > Ideally an ARP pattern item should match the fixed part only and a separate
> > ARP_IPV4 match its payload, somewhat like you did for ICMPv6/NDP below.
> > 
> > Problem is that in DPDK, struct arp_hdr includes struct arp_ipv4, so one
> > suggestion would be to rename this pattern item ARP_IPV4 directly:
> > 
> > => RTE_FLOW_ITEM_TYPE_ARP_IPV4
> > 
> > > +
> > > +	/**
> > > +	 * Matches any IPv6 Extension header.
> > 
> > => Matches an IPv6 extension header.
> > 
> > > +	 *
> > > +	 * See struct rte_flow_item_ipv6_ext_any.
> > > +	 */
> > > +	RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY,
> > 
> > I'm not sure this definition is necessary, more below about that.
> > 
> > Also I don't see a benefit in having "ANY" part of the name, if you want to keep
> > it, I suggest the simpler:
> > 
> > => RTE_FLOW_ITEM_TYPE_IPV6_EXT
> > 
> > > +
> > > +	/**
> > > +	 * Matches ICMPv6 header.
> > 
> > => Matches an ICMPv6 header.
> > 
> > > +	 *
> > > +	 * See struct rte_flow_item_icmpv6
> > 
> > Missing "."
> > 
> > > +	 */
> > > +	RTE_FLOW_ITEM_TYPE_ICMPV6,
> > > +
> > 
> > Before entering NDP territory below, I understand those should be stacked on
> > top of RTE_FLOW_ITEM_TYPE_ICMPV6. It's fine but for clarity they should be
> > named after the NDP types they represent, not inner data fields.
> > 
> > Also I think we should consider NDP as a protocol sitting on top of ICMPv6. We
> > could therefore drop "ICMP" from these definitions.
> > 
> > Since "ND" is a common shorthand for this protocol and "6" another when
> > doing something related to IPv6, I suggest to use "ND6" to name he related
> > pattern items.
> 
> I agree.
> 
> > 
> > These are the reasons behind my next suggestions:
> > 
> > > +	/**
> > > +	 * Match ICMPv6 target address.
> > > +	 *
> > > +	 * See struct rte_flow_item_icmpv6_tgt_addr.
> > > +	 */
> > > +	RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR,
> > 
> > => Matches an IPv6 network discovery router solicitation.
> > => See struct rte_flow_item_nd6_rs.
> > => RTE_FLOW_ITEM_TYPE_ND6_RS,

By the way, I wrote "router solicitation" (RS) here but it should have been
"neighbor solicitation" (NS) obviously.

> > 
> > You should add another item for neighbor advertisement messages using the
> > same template:
> > 
> > => Match an IPv6 network discovery neighbor advertisement.
> > => See struct rte_flow_item_nd6_na.
> > => RTE_FLOW_ITEM_TYPE_ND6_NA,
> 
> The purpose of RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR is to match a "target address"
> according to IPv6 ND spec https://tools.ietf.org/html/rfc4861#page-22, when type = 135/136
> 
> so do you mean we should have RTE_FLOW_ITEM_TYPE_ND6_NS (Neighbor Solicitation)
>  and RTE_FLOW_ITEM_TYPE_ND6_NA (Neighbor Advertisement) here,
> and with the same template (an IPV6 addr) for rte_flow_item_icmpv6_tgt_addr?

The rationale is that while they share a similar format, they are in fact
different messages that applications could want to match more conveniently
than providing ICMP type/code values. It would be done for consistency given
the same RFC also defines router solicitation/advertisement messages.

However a problem remains since these messages are part of the ICMP format
whose "reserved" field sometimes contains message flags, particularly with
RA. These structures would lack that data.

Honestly your approach makes sense, but it shouldn't be possible to mix
target addresses with RA/RS and it should be convenient to match these
messages without specifically matching their contents.

So another suggestion would be to define new types at the ICMPv6 level to
use directly on top of ETH for each possible message and define separate
structures for options.

First let's drop one character here and in all other definitions in this
patch:

 ICMPV6 => ICMP6 

Then the new items would respectively be:

 RTE_FLOW_ITEM_TYPE_ICMP6
 RTE_FLOW_ITEM_TYPE_ICMP6_ND_NA
 RTE_FLOW_ITEM_TYPE_ICMP6_ND_NS
 RTE_FLOW_ITEM_TYPE_ICMP6_ND_OPT_SLA
 RTE_FLOW_ITEM_TYPE_ICMP6_ND_OPT_TLA

All the related structure definitions would include the ICMPv6 header part
defined according to the RFC and except for RTE_FLOW_ITEM_TYPE_ICMP6, a
default mask that excludes type/code since they are implicit:

 struct rte_flow_item_icmp6_nd_na {
      uint8_t type; /**< ICMPv6 type, normally 136. */
      uint8_t code; /**< ICMPv6 code, normally 0. */
      rte_be16_t checksum; /**< ICMPv6 checksum. */
      /**
       * Router flag (1b), solicited flag (1b), override flag (1b),
       * reserved (29b).
       */
      rte_be32_t rso_reserved;
      uint8_t target[16]; /**< Target address. */
 };

 static const struct rte_flow_item_icmp6_nd_na rte_flow_item_icmp6_nd_na_mask = {
     .target =
          "\xff\xff\xff\xff\xff\xff\xff\xff"
          "\xff\xff\xff\xff\xff\xff\xff\xff",
 };

Also notice how uint(16|32)_t were modified as rte_be(16|32)_t while there.

What's your opinion?

> 
> > 
> > The following are possible options for these headers, if specified they must be
> > found afterward. Also since IPv6 may run on top of protocols other than
> > Ethernet, you need to clarify these link-layer addresses use the Ethernet
> > format:
> > 
> > > +
> > > +	/**
> > > +	 * Match ICMPv6 Source Link-Layer Address.
> > > +	 *
> > > +	 * See struct rte_flow_item_icmpv6_sll.
> > > +	 */
> > > +	RTE_FLOW_ITEM_TYPE_ICMPV6_SLL,
> > 
> > => Matches an IPv6 network discovery source Ethernet link-layer address
> > option.
> > => See struct rte_flow_item_nd6_opt_sla_eth.
> > => RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH,
> > 
> > > +
> > > +	/**
> > > +	 * Match ICMPv6 Target Link-Layer Address.
> > > +	 *
> > > +	 * See struct rte_flow_item_icmpv6_tll.
> > > +	 */
> > > +	RTE_FLOW_ITEM_TYPE_ICMPV6_TLL,
> > 
> > => Matches an IPv6 network discovery target Ethernet link-layer address
> > option.
> > => See struct rte_flow_item_nd6_opt_tla_eth.
> > => RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH,
> > 
> 
> Agree to rename.
> 
> > > +
> > 
> > Unnecessary empty line.
> > 
> > >  };
> > >
> > >  /**
> > > @@ -815,6 +858,123 @@ static const struct rte_flow_item_geneve
> > > rte_flow_item_geneve_mask = {  #endif
> > >
> > >  /**
> > > + * RTE_FLOW_ITEM_TYPE_ARP
> > > + *
> > > + * Matches IPv4 ARP packet header
> > 
> > As above:
> > 
> > => Matches an IPv4 ARP header.
> > => RTE_FLOW_ITEM_TYPE_ARP_IPV4
> > 
> > > + */
> > > +struct rte_flow_item_arp {
> > > +	struct arp_hdr hdr;
> > > +};
> > 
> > Needs #include <rte_arp.h> and a Doxygen comment next to hdr for
> > consistency, see ICMP and other definitions.
> > 
> > > +
> > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ARP. */ #ifndef __cplusplus
> > > +static const struct rte_flow_item_arp rte_flow_item_arp_mask = {
> > > +	.hdr = {
> > > +		.arp_data = {
> > > +			.arp_sha = {
> > > +				.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > > +			},
> > > +			.arp_sip = RTE_BE32(0xffffffff),
> > > +			.arp_tha = {
> > > +				.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > > +			},
> > > +			.arp_tip = RTE_BE32(0xffffffff),
> > > +		},
> > > +	},
> > > +};
> > > +#endif
> > > +
> > > +/**
> > > + * RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY
> > > + *
> > > + * Matches any IPv6 extension header.
> > > + */
> > > +struct rte_flow_item_ipv6_ext_hdr_any {
> > > +	uint8_t next_hdr;
> > > +};
> > 
> > So what's the point? next_hdr is already part of either struct ipv6_hdr
> > ("proto") and individual extension headers. Moreover it's implicit if an
> > extension header is provided in a pattern.
> > 
> > How about removing it?
> 
> We need this to match a packet that have extend header
> For example:
> IPV6(proto = 43, <Routing EH >) / EXT_HDR (next_head = 60 <Destination EH>) / EXT_HDR (next_head = 44, <Fragment EH)/ EXT_HDR (next_head = 6 <tcp>) / TCP ...
> 
> I use "ANY" to match any extend header regardless their content.
> There is no conflict if we can add multiple RTE_FLOW_ITEM_EXT_HDR_XXX in futures

I see, makes sense. How about doing like ICMPv6 above? Generic item uses
the base name and can only match the generic part specifically (next_hdr),
while specific items don't match the generic part but whatever additions
their dedicated structures define, i.e.:

 RTE_FLOW_ITEM_TYPE_IPV6_EXT
 RTE_FLOW_ITEM_TYPE_IPV6_EXT_HBH
 RTE_FLOW_ITEM_TYPE_IPV6_EXT_DEST
 RTE_FLOW_ITEM_TYPE_IPV6_EXT_RTHDR
 RTE_FLOW_ITEM_TYPE_IPV6_EXT_FRAG
 ...

No need to define them all if you only need EXT, this is just to describe
the idea (it's also OK if you want to define them while you're at it).

> 
> > 
> > > +
> > > +/** Default mask for RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY. */
> > #ifndef
> > > +__cplusplus static const struct rte_flow_item_ipv6_ext_hdr_any
> > > +rte_flow_item_ipv6_ext_any_mask = {
> > > +	.next_hdr = 0xff,
> > > +};
> > > +#endif
> > 
> > Ditto.
> > 
> > > +
> > > +/**
> > > + * RTE_FLOW_ITEM_TYPE_ICMPV6
> > > + *
> > > + * Matches ICMPv6 header.
> > 
> > => Matches an ICMPv6 header.
> > 
> > > + */
> > > +struct rte_flow_item_icmpv6 {
> > > +	uint8_t type;
> > > +	uint8_t code;
> > > +	uint16_t checksum;
> > 
> > The last 32-bit "reserved" data field is missing.
> > 
> > > +};
> > 
> > Too bad there is no struct icmp6_hdr definition in rte_icmp.h. You could add it.
> > In any case Doxygen comments are missing, please add them (see other
> > structure definitions for examples).

No need to rely on an external definition due to the above suggestions by
the way.

> > 
> > > +
> > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6 */
> > 
> > Missing "."
> > 
> > > +#ifndef __cplusplus
> > > +static const struct rte_flow_item_icmpv6 rte_flow_item_icmpv6_mask = {
> > > +	.type = 0xff,
> > > +	.code = 0xff,
> > > +	.checksum = RTE_BE16(0xffff),
> > > +};
> > > +#endif
> > 
> > You must remove checksum matching from the default mask. That's the last
> > thing an application might want to match exactly :)
> > 
> > > +
> > > +/**
> > > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
> > > + *
> > > + * Matches ICMPv6's Target Address.
> > > + */
> > > +struct rte_flow_item_icmpv6_tgt_addr {
> > > +	uint8_t addr[16];
> > > +};
> > 
> > You need to expand this as two items, see prior comments regarding
> > RTE_FLOW_ITEM_TYPE_ND6_RS, RTE_FLOW_ITEM_TYPE_ND6_NA and their
> > respective structs rte_flow_item_nd6_rs and rte_flow_item_nd6_na.
> > 
> > Also Doxygen documentation is missing for the addr field and you need to
> > describe that these are only valid when used after
> > RTE_FLOW_ITEM_TYPE_ICMPV6.
> > 
> > > +
> > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR */
> > 
> > Missing "."
> > 
> > > +#ifndef __cplusplus
> > > +static const
> > > +struct rte_flow_item_icmpv6_tgt_addr
> > rte_flow_item_icmpv6_tgt_addr_mask = {
> > > +	.addr =
> > > +		"\xff\xff\xff\xff\xff\xff\xff\xff"
> > > +		"\xff\xff\xff\xff\xff\xff\xff\xff",
> > > +};
> > > +#endif
> > > +
> > > +/**
> > > + * RTE_FLOW_ITEM_TYPE_ICPMV6_SLL.
> > > + *
> > > + * Matches ICMPv6 Source Link-Layer address.
> > > + */
> > > +struct rte_flow_item_icmpv6_sll {
> > > +	struct ether_addr addr;
> > > +};
> > 
> > See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH and
> > struct rte_flow_item_type_nd6_opt_sla_eth.
> > 
> > Also Doxygen documentation is missing for the addr field and you need to
> > describe that it is only valid when found after either
> > RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA.
> > 
> > Also missing empty line here.
> > 
> > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_SLL */
> > 
> > Missing "."
> > 
> > > +#ifndef __cplusplus
> > > +static const struct rte_flow_item_icmpv6_sll
> > rte_flow_item_icmpv6_sll_mask = {
> > > +	.addr = {
> > > +		.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > > +	}
> > > +};
> > > +#endif
> > > +
> > > +/**
> > > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TLL.
> > > + *
> > > + * Matches ICMPv6 Target Link-Layer address.
> > > + */
> > > +struct rte_flow_item_icmpv6_tll {
> > > +	struct ether_addr addr;
> > > +};
> > 
> > See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH
> > and struct rte_flow_item_type_nd6_opt_tla_eth.
> > 
> > Also Doxygen documentation is missing for the addr field and you need to
> > describe that it is only valid when found after either
> > RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA.
> > 
> > Also missing empty line here.
> > 
> > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TLL */
> > 
> > Missing "."
> > 
> > > +#ifndef __cplusplus
> > > +static const struct rte_flow_item_icmpv6_tll
> > rte_flow_item_icmpv6_tll_mask = {
> > > +	.addr = {
> > > +		.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > > +	}
> > > +};
> > > +#endif
> > > +
> > > +/**
> > >   * Matching pattern item definition.
> > >   *
> > >   * A pattern is formed by stacking items starting from the lowest
> > > protocol
> > > --
> > > 2.7.4
> > >
  

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, April 12, 2018 5:20 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [PATCH v2 3/4] ether: add more protocol support in flow API
> 
> On Thu, Apr 12, 2018 at 05:12:08AM +0000, Zhang, Qi Z wrote:
> > Hi Adrien:
> >
> > 	Thank you so much for your careful review and helpful suggestions!
> > 	I agree with most of your comments, except couple question about
> RTE_FLOW_ITEM_TYPE_TGT_ADDR and RTE_FLOW_ITEM_IPV6_EXT_HDR
> > 	Please see my comment inline.
> >
> > Thanks!
> > Qi
> 
> Thanks, replying inline also.
> 
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > Sent: Thursday, April 12, 2018 12:32 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > Bruce <bruce.richardson@intel.com>
> > > Subject: Re: [PATCH v2 3/4] ether: add more protocol support in flow
> > > API
> > >
> > > On Sun, Apr 01, 2018 at 05:19:21PM -0400, Qi Zhang wrote:
> > > > Add new protocol header match support as below
> > > >
> > > > RTE_FLOW_ITEM_TYPE_ARP
> > > > 	- match IPv4 ARP header
> > > > RTE_FLOW_ITEM_TYPE_EXT_HDR_ANY
> > > > 	- match any IPv6 extension header
> > >
> > > While properly defined in the patch, "IPV6" is missing here.
> > >
> > > > RTE_FLOW_ITEM_TYPE_ICMPV6
> > > > 	- match IPv6 ICMP header
> > > > RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
> > > > 	- match IPv6 ICMP Target address
> > > > RTE_FLOW_ITEM_TYPE_ICMPV6_SSL
> > > > 	- match IPv6 ICMP Source Link-layer address
> > > > RTE_FLOW_ITEM_TYPE_ICMPV6_TTL
> > > > 	- match IPv6 ICMP Target Link-layer address
> > > >
> > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > >
> > > First, since they are added at the end of enum rte_flow_item_type,
> > > no ABI breakage notice is necessary.
> > >
> > > However testpmd implementation [1][2] and documentation update
> > > [3][4] are mandatory for all new pattern items and actions.
> >
> > OK, will add this into v2.
> >
> > >
> > > More comments below regarding these definitions.
> > >
> > > [1] flow_item[] in app/test-pmd/config.c [2] using ITEM_ICMP as an
> > > example in app/test-pmd/cmdline_flow.c [3] "Pattern items" section
> > > in doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > [4] using "Item: ``ICMP``" section as an example in
> > >     doc/guides/prog_guide/rte_flow.rst
> > >
> > > > ---
> > > >  lib/librte_ether/rte_flow.h | 160
> > > > ++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 160 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ether/rte_flow.h
> > > > b/lib/librte_ether/rte_flow.h index 8f75db0..a8ec780 100644
> > > > --- a/lib/librte_ether/rte_flow.h
> > > > +++ b/lib/librte_ether/rte_flow.h
> > > > @@ -323,6 +323,49 @@ enum rte_flow_item_type {
> > > >  	 * See struct rte_flow_item_geneve.
> > > >  	 */
> > > >  	RTE_FLOW_ITEM_TYPE_GENEVE,
> > > > +
> > > > +	/**
> > > > +	 * Matches ARP IPv4 header.
> > >
> > > => Matches an IPv4 ARP header.
> > >
> > > > +	 *
> > > > +	 * See struct rte_flow_item_arp.
> > > > +	 */
> > > > +	RTE_FLOW_ITEM_TYPE_ARP,
> > >
> > > While you're right to make "IPv4" clear since ARP is also used for
> > > other protocols DPDK doesn't support (and likely never will), the
> > > ARP header has both a fixed and a variably-sized part.
> > >
> > > Ideally an ARP pattern item should match the fixed part only and a
> > > separate
> > > ARP_IPV4 match its payload, somewhat like you did for ICMPv6/NDP
> below.
> > >
> > > Problem is that in DPDK, struct arp_hdr includes struct arp_ipv4, so
> > > one suggestion would be to rename this pattern item ARP_IPV4 directly:
> > >
> > > => RTE_FLOW_ITEM_TYPE_ARP_IPV4
> > >
> > > > +
> > > > +	/**
> > > > +	 * Matches any IPv6 Extension header.
> > >
> > > => Matches an IPv6 extension header.
> > >
> > > > +	 *
> > > > +	 * See struct rte_flow_item_ipv6_ext_any.
> > > > +	 */
> > > > +	RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY,
> > >
> > > I'm not sure this definition is necessary, more below about that.
> > >
> > > Also I don't see a benefit in having "ANY" part of the name, if you
> > > want to keep it, I suggest the simpler:
> > >
> > > => RTE_FLOW_ITEM_TYPE_IPV6_EXT
> > >
> > > > +
> > > > +	/**
> > > > +	 * Matches ICMPv6 header.
> > >
> > > => Matches an ICMPv6 header.
> > >
> > > > +	 *
> > > > +	 * See struct rte_flow_item_icmpv6
> > >
> > > Missing "."
> > >
> > > > +	 */
> > > > +	RTE_FLOW_ITEM_TYPE_ICMPV6,
> > > > +
> > >
> > > Before entering NDP territory below, I understand those should be
> > > stacked on top of RTE_FLOW_ITEM_TYPE_ICMPV6. It's fine but for
> > > clarity they should be named after the NDP types they represent, not inner
> data fields.
> > >
> > > Also I think we should consider NDP as a protocol sitting on top of
> > > ICMPv6. We could therefore drop "ICMP" from these definitions.
> > >
> > > Since "ND" is a common shorthand for this protocol and "6" another
> > > when doing something related to IPv6, I suggest to use "ND6" to name
> > > he related pattern items.
> >
> > I agree.
> >
> > >
> > > These are the reasons behind my next suggestions:
> > >
> > > > +	/**
> > > > +	 * Match ICMPv6 target address.
> > > > +	 *
> > > > +	 * See struct rte_flow_item_icmpv6_tgt_addr.
> > > > +	 */
> > > > +	RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR,
> > >
> > > => Matches an IPv6 network discovery router solicitation.
> > > => See struct rte_flow_item_nd6_rs.
> > > => RTE_FLOW_ITEM_TYPE_ND6_RS,
> 
> By the way, I wrote "router solicitation" (RS) here but it should have been
> "neighbor solicitation" (NS) obviously.
> 
> > >
> > > You should add another item for neighbor advertisement messages
> > > using the same template:
> > >
> > > => Match an IPv6 network discovery neighbor advertisement.
> > > => See struct rte_flow_item_nd6_na.
> > > => RTE_FLOW_ITEM_TYPE_ND6_NA,
> >
> > The purpose of RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR is to match a
> "target address"
> > according to IPv6 ND spec https://tools.ietf.org/html/rfc4861#page-22,
> > when type = 135/136
> >
> > so do you mean we should have RTE_FLOW_ITEM_TYPE_ND6_NS (Neighbor
> > Solicitation)  and RTE_FLOW_ITEM_TYPE_ND6_NA (Neighbor
> Advertisement)
> > here, and with the same template (an IPV6 addr) for
> rte_flow_item_icmpv6_tgt_addr?
> 
> The rationale is that while they share a similar format, they are in fact different
> messages that applications could want to match more conveniently than
> providing ICMP type/code values. It would be done for consistency given the
> same RFC also defines router solicitation/advertisement messages.
> 
> However a problem remains since these messages are part of the ICMP format
> whose "reserved" field sometimes contains message flags, particularly with RA.
> These structures would lack that data.
> 
> Honestly your approach makes sense, but it shouldn't be possible to mix target
> addresses with RA/RS and it should be convenient to match these messages
> without specifically matching their contents.
> 
> So another suggestion would be to define new types at the ICMPv6 level to use
> directly on top of ETH for each possible message and define separate
> structures for options.
> 
> First let's drop one character here and in all other definitions in this
> patch:
> 
>  ICMPV6 => ICMP6
> 
> Then the new items would respectively be:
> 
>  RTE_FLOW_ITEM_TYPE_ICMP6
>  RTE_FLOW_ITEM_TYPE_ICMP6_ND_NA
>  RTE_FLOW_ITEM_TYPE_ICMP6_ND_NS
>  RTE_FLOW_ITEM_TYPE_ICMP6_ND_OPT_SLA
>  RTE_FLOW_ITEM_TYPE_ICMP6_ND_OPT_TLA
> 
> All the related structure definitions would include the ICMPv6 header part
> defined according to the RFC and except for RTE_FLOW_ITEM_TYPE_ICMP6, a
> default mask that excludes type/code since they are implicit:
> 
>  struct rte_flow_item_icmp6_nd_na {
>       uint8_t type; /**< ICMPv6 type, normally 136. */
>       uint8_t code; /**< ICMPv6 code, normally 0. */
>       rte_be16_t checksum; /**< ICMPv6 checksum. */
>       /**
>        * Router flag (1b), solicited flag (1b), override flag (1b),
>        * reserved (29b).
>        */
>       rte_be32_t rso_reserved;
>       uint8_t target[16]; /**< Target address. */  };
> 
>  static const struct rte_flow_item_icmp6_nd_na
> rte_flow_item_icmp6_nd_na_mask = {
>      .target =
>           "\xff\xff\xff\xff\xff\xff\xff\xff"
>           "\xff\xff\xff\xff\xff\xff\xff\xff",
>  };
> 
> Also notice how uint(16|32)_t were modified as rte_be(16|32)_t while there.
> 
> What's your opinion?

OK, I will take this method, it looks good, thanks 

> 
> >
> > >
> > > The following are possible options for these headers, if specified
> > > they must be found afterward. Also since IPv6 may run on top of
> > > protocols other than Ethernet, you need to clarify these link-layer
> > > addresses use the Ethernet
> > > format:
> > >
> > > > +
> > > > +	/**
> > > > +	 * Match ICMPv6 Source Link-Layer Address.
> > > > +	 *
> > > > +	 * See struct rte_flow_item_icmpv6_sll.
> > > > +	 */
> > > > +	RTE_FLOW_ITEM_TYPE_ICMPV6_SLL,
> > >
> > > => Matches an IPv6 network discovery source Ethernet link-layer
> > > address option.
> > > => See struct rte_flow_item_nd6_opt_sla_eth.
> > > => RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH,
> > >
> > > > +
> > > > +	/**
> > > > +	 * Match ICMPv6 Target Link-Layer Address.
> > > > +	 *
> > > > +	 * See struct rte_flow_item_icmpv6_tll.
> > > > +	 */
> > > > +	RTE_FLOW_ITEM_TYPE_ICMPV6_TLL,
> > >
> > > => Matches an IPv6 network discovery target Ethernet link-layer
> > > address option.
> > > => See struct rte_flow_item_nd6_opt_tla_eth.
> > > => RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH,
> > >
> >
> > Agree to rename.
> >
> > > > +
> > >
> > > Unnecessary empty line.
> > >
> > > >  };
> > > >
> > > >  /**
> > > > @@ -815,6 +858,123 @@ static const struct rte_flow_item_geneve
> > > > rte_flow_item_geneve_mask = {  #endif
> > > >
> > > >  /**
> > > > + * RTE_FLOW_ITEM_TYPE_ARP
> > > > + *
> > > > + * Matches IPv4 ARP packet header
> > >
> > > As above:
> > >
> > > => Matches an IPv4 ARP header.
> > > => RTE_FLOW_ITEM_TYPE_ARP_IPV4
> > >
> > > > + */
> > > > +struct rte_flow_item_arp {
> > > > +	struct arp_hdr hdr;
> > > > +};
> > >
> > > Needs #include <rte_arp.h> and a Doxygen comment next to hdr for
> > > consistency, see ICMP and other definitions.
> > >
> > > > +
> > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ARP. */ #ifndef
> > > > +__cplusplus static const struct rte_flow_item_arp
> rte_flow_item_arp_mask = {
> > > > +	.hdr = {
> > > > +		.arp_data = {
> > > > +			.arp_sha = {
> > > > +				.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > > > +			},
> > > > +			.arp_sip = RTE_BE32(0xffffffff),
> > > > +			.arp_tha = {
> > > > +				.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > > > +			},
> > > > +			.arp_tip = RTE_BE32(0xffffffff),
> > > > +		},
> > > > +	},
> > > > +};
> > > > +#endif
> > > > +
> > > > +/**
> > > > + * RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY
> > > > + *
> > > > + * Matches any IPv6 extension header.
> > > > + */
> > > > +struct rte_flow_item_ipv6_ext_hdr_any {
> > > > +	uint8_t next_hdr;
> > > > +};
> > >
> > > So what's the point? next_hdr is already part of either struct
> > > ipv6_hdr
> > > ("proto") and individual extension headers. Moreover it's implicit
> > > if an extension header is provided in a pattern.
> > >
> > > How about removing it?
> >
> > We need this to match a packet that have extend header For example:
> > IPV6(proto = 43, <Routing EH >) / EXT_HDR (next_head = 60 <Destination EH>)
> / EXT_HDR (next_head = 44, <Fragment EH)/ EXT_HDR (next_head = 6 <tcp>) /
> TCP ...
> >
> > I use "ANY" to match any extend header regardless their content.
> > There is no conflict if we can add multiple RTE_FLOW_ITEM_EXT_HDR_XXX
> > in futures
> 
> I see, makes sense. How about doing like ICMPv6 above? Generic item uses the
> base name and can only match the generic part specifically (next_hdr), while
> specific items don't match the generic part but whatever additions their
> dedicated structures define, i.e.:
> 
>  RTE_FLOW_ITEM_TYPE_IPV6_EXT
>  RTE_FLOW_ITEM_TYPE_IPV6_EXT_HBH
>  RTE_FLOW_ITEM_TYPE_IPV6_EXT_DEST
>  RTE_FLOW_ITEM_TYPE_IPV6_EXT_RTHDR
>  RTE_FLOW_ITEM_TYPE_IPV6_EXT_FRAG
>  ...

Yes, agree.

> 
> No need to define them all if you only need EXT, this is just to describe the idea
> (it's also OK if you want to define them while you're at it).
> 
> >
> > >
> > > > +
> > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY. */
> > > #ifndef
> > > > +__cplusplus static const struct rte_flow_item_ipv6_ext_hdr_any
> > > > +rte_flow_item_ipv6_ext_any_mask = {
> > > > +	.next_hdr = 0xff,
> > > > +};
> > > > +#endif
> > >
> > > Ditto.
> > >
> > > > +
> > > > +/**
> > > > + * RTE_FLOW_ITEM_TYPE_ICMPV6
> > > > + *
> > > > + * Matches ICMPv6 header.
> > >
> > > => Matches an ICMPv6 header.
> > >
> > > > + */
> > > > +struct rte_flow_item_icmpv6 {
> > > > +	uint8_t type;
> > > > +	uint8_t code;
> > > > +	uint16_t checksum;
> > >
> > > The last 32-bit "reserved" data field is missing.
> > >
> > > > +};
> > >
> > > Too bad there is no struct icmp6_hdr definition in rte_icmp.h. You could
> add it.
> > > In any case Doxygen comments are missing, please add them (see other
> > > structure definitions for examples).
> 
> No need to rely on an external definition due to the above suggestions by the
> way.
> 
> > >
> > > > +
> > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6 */
> > >
> > > Missing "."
> > >
> > > > +#ifndef __cplusplus
> > > > +static const struct rte_flow_item_icmpv6 rte_flow_item_icmpv6_mask =
> {
> > > > +	.type = 0xff,
> > > > +	.code = 0xff,
> > > > +	.checksum = RTE_BE16(0xffff),
> > > > +};
> > > > +#endif
> > >
> > > You must remove checksum matching from the default mask. That's the
> > > last thing an application might want to match exactly :)
> > >
> > > > +
> > > > +/**
> > > > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR
> > > > + *
> > > > + * Matches ICMPv6's Target Address.
> > > > + */
> > > > +struct rte_flow_item_icmpv6_tgt_addr {
> > > > +	uint8_t addr[16];
> > > > +};
> > >
> > > You need to expand this as two items, see prior comments regarding
> > > RTE_FLOW_ITEM_TYPE_ND6_RS, RTE_FLOW_ITEM_TYPE_ND6_NA and
> their
> > > respective structs rte_flow_item_nd6_rs and rte_flow_item_nd6_na.
> > >
> > > Also Doxygen documentation is missing for the addr field and you
> > > need to describe that these are only valid when used after
> > > RTE_FLOW_ITEM_TYPE_ICMPV6.
> > >
> > > > +
> > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR */
> > >
> > > Missing "."
> > >
> > > > +#ifndef __cplusplus
> > > > +static const
> > > > +struct rte_flow_item_icmpv6_tgt_addr
> > > rte_flow_item_icmpv6_tgt_addr_mask = {
> > > > +	.addr =
> > > > +		"\xff\xff\xff\xff\xff\xff\xff\xff"
> > > > +		"\xff\xff\xff\xff\xff\xff\xff\xff",
> > > > +};
> > > > +#endif
> > > > +
> > > > +/**
> > > > + * RTE_FLOW_ITEM_TYPE_ICPMV6_SLL.
> > > > + *
> > > > + * Matches ICMPv6 Source Link-Layer address.
> > > > + */
> > > > +struct rte_flow_item_icmpv6_sll {
> > > > +	struct ether_addr addr;
> > > > +};
> > >
> > > See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH
> and
> > > struct rte_flow_item_type_nd6_opt_sla_eth.
> > >
> > > Also Doxygen documentation is missing for the addr field and you
> > > need to describe that it is only valid when found after either
> > > RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA.
> > >
> > > Also missing empty line here.
> > >
> > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_SLL */
> > >
> > > Missing "."
> > >
> > > > +#ifndef __cplusplus
> > > > +static const struct rte_flow_item_icmpv6_sll
> > > rte_flow_item_icmpv6_sll_mask = {
> > > > +	.addr = {
> > > > +		.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > > > +	}
> > > > +};
> > > > +#endif
> > > > +
> > > > +/**
> > > > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TLL.
> > > > + *
> > > > + * Matches ICMPv6 Target Link-Layer address.
> > > > + */
> > > > +struct rte_flow_item_icmpv6_tll {
> > > > +	struct ether_addr addr;
> > > > +};
> > >
> > > See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH
> > > and struct rte_flow_item_type_nd6_opt_tla_eth.
> > >
> > > Also Doxygen documentation is missing for the addr field and you
> > > need to describe that it is only valid when found after either
> > > RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA.
> > >
> > > Also missing empty line here.
> > >
> > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TLL */
> > >
> > > Missing "."
> > >
> > > > +#ifndef __cplusplus
> > > > +static const struct rte_flow_item_icmpv6_tll
> > > rte_flow_item_icmpv6_tll_mask = {
> > > > +	.addr = {
> > > > +		.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> > > > +	}
> > > > +};
> > > > +#endif
> > > > +
> > > > +/**
> > > >   * Matching pattern item definition.
> > > >   *
> > > >   * A pattern is formed by stacking items starting from the lowest
> > > > protocol
> > > > --
> > > > 2.7.4
> > > >
> 
> --
> Adrien Mazarguil
> 6WIND