[dpdk-dev] [PATCH 0/2] ethdev: add GENEVE to flow API

[dpdk-dev] [PATCH 0/2] ethdev: add GENEVE to flow API

[Andrew Rybchenko]

enum rte_flow_item_type states that items matching protocol headers
must be stacked in the same order as the protocol layers to match.
As the result the patch changes ABI since Geneve is added just after
VXLAN (the closest protocol).

In fact as far as I can see many items do not follow the requirement
already. May be the comment/requirement should be removed and GENEVE
should be added at the end of the list. If so, should be keep it just
after VXLAN in all other places or move after ESP as well?

Roman Zhukov (2):
  ethdev: add GENEVE flow pattern item
  app/testpmd: support GENEVE pattern item in flow rules

 app/test-pmd/cmdline_flow.c                 | 31 +++++++++++++++++++++++++++++
 app/test-pmd/config.c                       |  1 +
 doc/guides/prog_guide/rte_flow.rst          | 12 +++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  5 +++++
 lib/librte_ether/rte_flow.c                 |  1 +
 lib/librte_ether/rte_flow.h                 | 31 +++++++++++++++++++++++++++++
 6 files changed, 81 insertions(+)

-- 
2.7.4

[dpdk-dev] [PATCH 1/2] ethdev: add GENEVE flow pattern item

[Andrew Rybchenko]

From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>

Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
This commit also adds default mask for these item.

Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
 lib/librte_ether/rte_flow.c        |  1 +
 lib/librte_ether/rte_flow.h        | 31 +++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index d158be5..2f96623 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -863,6 +863,18 @@ Matches a VXLAN header (RFC 7348).
 - ``rsvd1``: reserved, normally 0x00.
 - Default ``mask`` matches VNI only.
 
+Item: ``GENEVE``
+^^^^^^^^^^^^^^^
+
+Matches a GENEVE header.
+
+- ``ver_opt_len_o_c_rsvd0``: version (2b), length of the options fields (6b),
+  OAM packet (1b), critical options present (1b), reserved 0 (6b).
+- ``protocol``: protocol type.
+- ``vni``: virtual network identifier.
+- ``rsvd1``: reserved, normally 0x00.
+- Default ``mask`` matches protocol type and VNI.
+
 Item: ``E_TAG``
 ^^^^^^^^^^^^^^^
 
diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
index 6659063..bf1b253 100644
--- a/lib/librte_ether/rte_flow.c
+++ b/lib/librte_ether/rte_flow.c
@@ -77,6 +77,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
 	MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)),
 	MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)),
 	MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)),
+	MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
 	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
 	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
 	MK_FLOW_ITEM(E_TAG, sizeof(struct rte_flow_item_e_tag)),
diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index 47c88ea..29d81d4 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -272,6 +272,13 @@ enum rte_flow_item_type {
 	RTE_FLOW_ITEM_TYPE_VXLAN,
 
 	/**
+	 * Matches a GENEVE header.
+	 *
+	 * See struct rte_flow_item_geneve.
+	 */
+	RTE_FLOW_ITEM_TYPE_GENEVE,
+
+	/**
 	 * Matches a E_TAG header.
 	 *
 	 * See struct rte_flow_item_e_tag.
@@ -651,6 +658,30 @@ static const struct rte_flow_item_vxlan rte_flow_item_vxlan_mask = {
 #endif
 
 /**
+ * RTE_FLOW_ITEM_TYPE_GENEVE.
+ *
+ * Matches a GENEVE header.
+ */
+struct rte_flow_item_geneve {
+	/**
+	 * Version (2b), length of the options fields (6b), OAM packet (1b),
+	 * critical options present (1b), reserved 0 (6b).
+	 */
+	rte_be16_t ver_opt_len_o_c_rsvd0;
+	rte_be16_t protocol; /**< Protocol type. */
+	uint8_t vni[3]; /**< Virtual Network Identifier. */
+	uint8_t rsvd1; /**< Reserved, normally 0x00. */
+};
+
+/** Default mask for RTE_FLOW_ITEM_TYPE_GENEVE. */
+#ifndef __cplusplus
+static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
+	.protocol = RTE_BE16(0xffff),
+	.vni = "\xff\xff\xff",
+};
+#endif
+
+/**
  * RTE_FLOW_ITEM_TYPE_E_TAG.
  *
  * Matches a E-tag header.
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/2] ethdev: add GENEVE flow pattern item

[Adrien Mazarguil]

On Mon, Nov 20, 2017 at 08:22:00AM +0000, Andrew Rybchenko wrote:
> From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> 
> Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
> This commit also adds default mask for these item.
> 
> Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

OK, the main issue in this patch is you're inserting GENEVE in the middle of
everything (enums, documentation, etc). Just append it to the enum to avoid
ABI breakage as described in my previous message and use the same position
everywhere else for consistency.

It must appear after RTE_FLOW_ITEM_TYPE_ESP.

More comments below.

> ---
>  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
>  lib/librte_ether/rte_flow.c        |  1 +
>  lib/librte_ether/rte_flow.h        | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d158be5..2f96623 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -863,6 +863,18 @@ Matches a VXLAN header (RFC 7348).
>  - ``rsvd1``: reserved, normally 0x00.
>  - Default ``mask`` matches VNI only.
>  
> +Item: ``GENEVE``
> +^^^^^^^^^^^^^^^
> +
> +Matches a GENEVE header.
> +
> +- ``ver_opt_len_o_c_rsvd0``: version (2b), length of the options fields (6b),
> +  OAM packet (1b), critical options present (1b), reserved 0 (6b).
> +- ``protocol``: protocol type.
> +- ``vni``: virtual network identifier.
> +- ``rsvd1``: reserved, normally 0x00.
> +- Default ``mask`` matches protocol type and VNI.
> +

I'm not sure about the default mask. It should be the least common
denominator, not necessarily what the first implementation supports. How
about making it like VXLAN, i.e. VNI only? Does that make sense at all?

>  Item: ``E_TAG``
>  ^^^^^^^^^^^^^^^
>  
> diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
> index 6659063..bf1b253 100644
> --- a/lib/librte_ether/rte_flow.c
> +++ b/lib/librte_ether/rte_flow.c
> @@ -77,6 +77,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
>  	MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)),
>  	MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)),
>  	MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)),
> +	MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),

You should add it at the end but some items are already missing from that
list. Since I plan to send an overhaul for this function, you can leave this
change out for the time being.

>  	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
>  	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
>  	MK_FLOW_ITEM(E_TAG, sizeof(struct rte_flow_item_e_tag)),
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 47c88ea..29d81d4 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -272,6 +272,13 @@ enum rte_flow_item_type {
>  	RTE_FLOW_ITEM_TYPE_VXLAN,
>  
>  	/**
> +	 * Matches a GENEVE header.
> +	 *
> +	 * See struct rte_flow_item_geneve.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_GENEVE,
> +
> +	/**
>  	 * Matches a E_TAG header.
>  	 *
>  	 * See struct rte_flow_item_e_tag.
> @@ -651,6 +658,30 @@ static const struct rte_flow_item_vxlan rte_flow_item_vxlan_mask = {
>  #endif
>  
>  /**
> + * RTE_FLOW_ITEM_TYPE_GENEVE.
> + *
> + * Matches a GENEVE header.
> + */
> +struct rte_flow_item_geneve {
> +	/**
> +	 * Version (2b), length of the options fields (6b), OAM packet (1b),
> +	 * critical options present (1b), reserved 0 (6b).
> +	 */
> +	rte_be16_t ver_opt_len_o_c_rsvd0;
> +	rte_be16_t protocol; /**< Protocol type. */
> +	uint8_t vni[3]; /**< Virtual Network Identifier. */
> +	uint8_t rsvd1; /**< Reserved, normally 0x00. */
> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_GENEVE. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
> +	.protocol = RTE_BE16(0xffff),
> +	.vni = "\xff\xff\xff",
> +};
> +#endif

So how about removing .protocol from the default mask?

> +
> +/**
>   * RTE_FLOW_ITEM_TYPE_E_TAG.
>   *
>   * Matches a E-tag header.
> -- 
> 2.7.4
> 

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCH 2/2] app/testpmd: support GENEVE pattern item in flow rules

[Andrew Rybchenko]

From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>

Add the ability to match a VNI field of GENEVE protocol header.

Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 app/test-pmd/cmdline_flow.c                 | 31 +++++++++++++++++++++++++++++
 app/test-pmd/config.c                       |  1 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  5 +++++
 3 files changed, 37 insertions(+)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index df16d2a..cee99f3 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -161,6 +161,9 @@ enum index {
 	ITEM_SCTP_CKSUM,
 	ITEM_VXLAN,
 	ITEM_VXLAN_VNI,
+	ITEM_GENEVE,
+	ITEM_GENEVE_VNI,
+	ITEM_GENEVE_PROTO,
 	ITEM_E_TAG,
 	ITEM_E_TAG_GRP_ECID_B,
 	ITEM_NVGRE,
@@ -452,6 +455,7 @@ static const enum index next_item[] = {
 	ITEM_TCP,
 	ITEM_SCTP,
 	ITEM_VXLAN,
+	ITEM_GENEVE,
 	ITEM_E_TAG,
 	ITEM_NVGRE,
 	ITEM_MPLS,
@@ -573,6 +577,13 @@ static const enum index item_vxlan[] = {
 	ZERO,
 };
 
+static const enum index item_geneve[] = {
+	ITEM_GENEVE_VNI,
+	ITEM_GENEVE_PROTO,
+	ITEM_NEXT,
+	ZERO,
+};
+
 static const enum index item_e_tag[] = {
 	ITEM_E_TAG_GRP_ECID_B,
 	ITEM_NEXT,
@@ -1371,6 +1382,26 @@ static const struct token token_list[] = {
 		.next = NEXT(item_vxlan, NEXT_ENTRY(UNSIGNED), item_param),
 		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_vxlan, vni)),
 	},
+	[ITEM_GENEVE] = {
+		.name = "geneve",
+		.help = "match GENEVE header",
+		.priv = PRIV_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
+		.next = NEXT(item_geneve),
+		.call = parse_vc,
+	},
+	[ITEM_GENEVE_VNI] = {
+		.name = "vni",
+		.help = "Virtual Network Identifier",
+		.next = NEXT(item_geneve, NEXT_ENTRY(UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_geneve, vni)),
+	},
+	[ITEM_GENEVE_PROTO] = {
+		.name = "protocol",
+		.help = "GENEVE protocol type",
+		.next = NEXT(item_geneve, NEXT_ENTRY(UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_geneve,
+					     protocol)),
+	},
 	[ITEM_E_TAG] = {
 		.name = "e_tag",
 		.help = "match E-Tag header",
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index cd2ac11..4eda37f 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -965,6 +965,7 @@ static const struct {
 	MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)),
 	MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)),
 	MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)),
+	MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
 	MK_FLOW_ITEM(E_TAG, sizeof(struct rte_flow_item_e_tag)),
 	MK_FLOW_ITEM(NVGRE, sizeof(struct rte_flow_item_nvgre)),
 	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 9789139..8c2fd12 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3083,6 +3083,11 @@ This section lists supported pattern items and their attributes, if any.
 
   - ``vni {unsigned}``: VXLAN identifier.
 
+- ``geneve``: match GENEVE header.
+
+  - ``vni {unsigned}``: virtual network identifier.
+  - ``protocol {unsigned}``: protocol type.
+
 - ``e_tag``: match IEEE 802.1BR E-Tag header.
 
   - ``grp_ecid_b {unsigned}``: GRP and E-CID base.
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/2] app/testpmd: support GENEVE pattern item in flow rules

[Adrien Mazarguil]

On Mon, Nov 20, 2017 at 08:22:01AM +0000, Andrew Rybchenko wrote:
> From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> 
> Add the ability to match a VNI field of GENEVE protocol header.
> 
> Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

Same comments as previously basically, keep the same order as rte_flow.h
after fixing the ABI breakage.

One minor comment below.

> ---
>  app/test-pmd/cmdline_flow.c                 | 31 +++++++++++++++++++++++++++++
>  app/test-pmd/config.c                       |  1 +
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  5 +++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index df16d2a..cee99f3 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -161,6 +161,9 @@ enum index {
>  	ITEM_SCTP_CKSUM,
>  	ITEM_VXLAN,
>  	ITEM_VXLAN_VNI,
> +	ITEM_GENEVE,
> +	ITEM_GENEVE_VNI,
> +	ITEM_GENEVE_PROTO,
>  	ITEM_E_TAG,
>  	ITEM_E_TAG_GRP_ECID_B,
>  	ITEM_NVGRE,
> @@ -452,6 +455,7 @@ static const enum index next_item[] = {
>  	ITEM_TCP,
>  	ITEM_SCTP,
>  	ITEM_VXLAN,
> +	ITEM_GENEVE,
>  	ITEM_E_TAG,
>  	ITEM_NVGRE,
>  	ITEM_MPLS,
> @@ -573,6 +577,13 @@ static const enum index item_vxlan[] = {
>  	ZERO,
>  };
>  
> +static const enum index item_geneve[] = {
> +	ITEM_GENEVE_VNI,
> +	ITEM_GENEVE_PROTO,
> +	ITEM_NEXT,
> +	ZERO,
> +};
> +
>  static const enum index item_e_tag[] = {
>  	ITEM_E_TAG_GRP_ECID_B,
>  	ITEM_NEXT,
> @@ -1371,6 +1382,26 @@ static const struct token token_list[] = {
>  		.next = NEXT(item_vxlan, NEXT_ENTRY(UNSIGNED), item_param),
>  		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_vxlan, vni)),
>  	},
> +	[ITEM_GENEVE] = {
> +		.name = "geneve",
> +		.help = "match GENEVE header",
> +		.priv = PRIV_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
> +		.next = NEXT(item_geneve),
> +		.call = parse_vc,
> +	},
> +	[ITEM_GENEVE_VNI] = {
> +		.name = "vni",
> +		.help = "Virtual Network Identifier",

How about "virtual network identifier" (all lower caps)?

> +		.next = NEXT(item_geneve, NEXT_ENTRY(UNSIGNED), item_param),
> +		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_geneve, vni)),
> +	},
> +	[ITEM_GENEVE_PROTO] = {
> +		.name = "protocol",
> +		.help = "GENEVE protocol type",
> +		.next = NEXT(item_geneve, NEXT_ENTRY(UNSIGNED), item_param),
> +		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_geneve,
> +					     protocol)),
> +	},
>  	[ITEM_E_TAG] = {
>  		.name = "e_tag",
>  		.help = "match E-Tag header",
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index cd2ac11..4eda37f 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -965,6 +965,7 @@ static const struct {
>  	MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)),
>  	MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)),
>  	MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)),
> +	MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
>  	MK_FLOW_ITEM(E_TAG, sizeof(struct rte_flow_item_e_tag)),
>  	MK_FLOW_ITEM(NVGRE, sizeof(struct rte_flow_item_nvgre)),
>  	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 9789139..8c2fd12 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3083,6 +3083,11 @@ This section lists supported pattern items and their attributes, if any.
>  
>    - ``vni {unsigned}``: VXLAN identifier.
>  
> +- ``geneve``: match GENEVE header.
> +
> +  - ``vni {unsigned}``: virtual network identifier.
> +  - ``protocol {unsigned}``: protocol type.
> +
>  - ``e_tag``: match IEEE 802.1BR E-Tag header.
>  
>    - ``grp_ecid_b {unsigned}``: GRP and E-CID base.
> -- 
> 2.7.4
> 

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH 0/2] ethdev: add GENEVE to flow API

[Adrien Mazarguil]

Hi Andrew,

On Mon, Nov 20, 2017 at 08:21:59AM +0000, Andrew Rybchenko wrote:
> enum rte_flow_item_type states that items matching protocol headers
> must be stacked in the same order as the protocol layers to match.
> As the result the patch changes ABI since Geneve is added just after
> VXLAN (the closest protocol).
> 
> In fact as far as I can see many items do not follow the requirement
> already. May be the comment/requirement should be removed and GENEVE
> should be added at the end of the list. If so, should be keep it just
> after VXLAN in all other places or move after ESP as well?

Perhaps documentation is unclear, this requirement only applies to
applications when constructing patterns out of those items (e.g. to make
sense, TCP is supposed to come after IPv4, not before).

New item/action definitions must obviously be added at the end of both lists
to avoid ABI breakage, there is no specific order to follow other than that.

What may have confused you is most of them are apparently ordered by
protocol layer, that's because those are here from day one; it's not the
case anymore starting with E_TAG, which was added much later.

Besides addressing the ABI breakage, I don't see any issue with adding
GENEVE to rte_flow, I only have a few more comments on subsequent patches in
the series. Otherwise good job, looks like you didn't miss anything.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH 0/2] ethdev: add GENEVE to flow API

[Andrew Rybchenko]

Hi Adrien,

On 11/23/2017 12:39 PM, Adrien Mazarguil wrote:
> Hi Andrew,
>
> On Mon, Nov 20, 2017 at 08:21:59AM +0000, Andrew Rybchenko wrote:
>> enum rte_flow_item_type states that items matching protocol headers
>> must be stacked in the same order as the protocol layers to match.
>> As the result the patch changes ABI since Geneve is added just after
>> VXLAN (the closest protocol).
>>
>> In fact as far as I can see many items do not follow the requirement
>> already. May be the comment/requirement should be removed and GENEVE
>> should be added at the end of the list. If so, should be keep it just
>> after VXLAN in all other places or move after ESP as well?
> Perhaps documentation is unclear, this requirement only applies to
> applications when constructing patterns out of those items (e.g. to make
> sense, TCP is supposed to come after IPv4, not before).
>
> New item/action definitions must obviously be added at the end of both lists
> to avoid ABI breakage, there is no specific order to follow other than that.
>
> What may have confused you is most of them are apparently ordered by
> protocol layer, that's because those are here from day one; it's not the
> case anymore starting with E_TAG, which was added much later.
>
> Besides addressing the ABI breakage, I don't see any issue with adding
> GENEVE to rte_flow, I only have a few more comments on subsequent patches in
> the series. Otherwise good job, looks like you didn't miss anything.

Many thinks for review and clarification. We'll process review notes and 
send v2.

--
Andrew

[dpdk-dev] [PATCH v2 0/2] ethdev: add GENEVE to flow API

[Andrew Rybchenko]

v2:
 - add after ESP to avoid ABI breakage
 - make default mask to include VNI only
 - minor style fixes

Roman Zhukov (2):
  ethdev: add GENEVE flow pattern item
  app/testpmd: support GENEVE pattern item in flow rules

 app/test-pmd/cmdline_flow.c                 | 31 +++++++++++++++++++++++++++++
 app/test-pmd/config.c                       |  1 +
 doc/guides/prog_guide/rte_flow.rst          | 12 +++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  5 +++++
 lib/librte_ether/rte_flow.c                 |  1 +
 lib/librte_ether/rte_flow.h                 | 30 ++++++++++++++++++++++++++++
 6 files changed, 80 insertions(+)

-- 
2.7.4

[dpdk-dev] [PATCH v2 1/2] ethdev: add GENEVE flow pattern item

[Andrew Rybchenko]

From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>

Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
Add default mask for the item.

Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
 lib/librte_ether/rte_flow.c        |  1 +
 lib/librte_ether/rte_flow.h        | 30 ++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index d158be5..5b8f9c5 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -980,6 +980,18 @@ Matches an ESP header.
 - ``hdr``: ESP header definition (``rte_esp.h``).
 - Default ``mask`` matches SPI only.
 
+Item: ``GENEVE``
+^^^^^^^^^^^^^^^
+
+Matches a GENEVE header.
+
+- ``ver_opt_len_o_c_rsvd0``: version (2b), length of the options fields (6b),
+  OAM packet (1b), critical options present (1b), reserved 0 (6b).
+- ``protocol``: protocol type.
+- ``vni``: virtual network identifier.
+- ``rsvd1``: reserved, normally 0x00.
+- Default ``mask`` matches VNI only.
+
 Actions
 ~~~~~~~
 
diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
index 6659063..913d1a5 100644
--- a/lib/librte_ether/rte_flow.c
+++ b/lib/librte_ether/rte_flow.c
@@ -81,6 +81,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
 	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
 	MK_FLOW_ITEM(E_TAG, sizeof(struct rte_flow_item_e_tag)),
 	MK_FLOW_ITEM(NVGRE, sizeof(struct rte_flow_item_nvgre)),
+	MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
 };
 
 /** Generate flow_action[] entry. */
diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index 47c88ea..e0402cf 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -344,6 +344,13 @@ enum rte_flow_item_type {
 	 * See struct rte_flow_item_esp.
 	 */
 	RTE_FLOW_ITEM_TYPE_ESP,
+
+	/**
+	 * Matches a GENEVE header.
+	 *
+	 * See struct rte_flow_item_geneve.
+	 */
+	RTE_FLOW_ITEM_TYPE_GENEVE,
 };
 
 /**
@@ -813,6 +820,29 @@ static const struct rte_flow_item_esp rte_flow_item_esp_mask = {
 #endif
 
 /**
+ * RTE_FLOW_ITEM_TYPE_GENEVE.
+ *
+ * Matches a GENEVE header.
+ */
+struct rte_flow_item_geneve {
+	/**
+	 * Version (2b), length of the options fields (6b), OAM packet (1b),
+	 * critical options present (1b), reserved 0 (6b).
+	 */
+	rte_be16_t ver_opt_len_o_c_rsvd0;
+	rte_be16_t protocol; /**< Protocol type. */
+	uint8_t vni[3]; /**< Virtual Network Identifier. */
+	uint8_t rsvd1; /**< Reserved, normally 0x00. */
+};
+
+/** Default mask for RTE_FLOW_ITEM_TYPE_GENEVE. */
+#ifndef __cplusplus
+static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
+	.vni = "\xff\xff\xff",
+};
+#endif
+
+/**
  * Matching pattern item definition.
  *
  * A pattern is formed by stacking items starting from the lowest protocol
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add GENEVE flow pattern item

[Adrien Mazarguil]

On Fri, Dec 01, 2017 at 10:43:15AM +0000, Andrew Rybchenko wrote:
> From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> 
> Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
> Add default mask for the item.
> 
> Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

Just one remaining nit, please see below. While you could address it through
a subsequent patch, you might as well send v3 directly to make things easier
for Ferruh.

Otherwise it's all good, thanks.

> ---
>  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
>  lib/librte_ether/rte_flow.c        |  1 +
>  lib/librte_ether/rte_flow.h        | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d158be5..5b8f9c5 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -980,6 +980,18 @@ Matches an ESP header.
>  - ``hdr``: ESP header definition (``rte_esp.h``).
>  - Default ``mask`` matches SPI only.
>  
> +Item: ``GENEVE``
> +^^^^^^^^^^^^^^^

Missing "^" under title, this may trigger a warning during documentation
generation.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add GENEVE flow pattern item

[Ferruh Yigit]

On 12/4/2017 6:03 AM, Adrien Mazarguil wrote:
> On Fri, Dec 01, 2017 at 10:43:15AM +0000, Andrew Rybchenko wrote:
>> From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
>>
>> Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
>> Add default mask for the item.
>>
>> Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> Just one remaining nit, please see below. While you could address it through
> a subsequent patch, you might as well send v3 directly to make things easier
> for Ferruh.
> 
> Otherwise it's all good, thanks.

May I take this as (for series):
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

> 
>> ---
>>  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
>>  lib/librte_ether/rte_flow.c        |  1 +
>>  lib/librte_ether/rte_flow.h        | 30 ++++++++++++++++++++++++++++++
>>  3 files changed, 43 insertions(+)
>>
>> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
>> index d158be5..5b8f9c5 100644
>> --- a/doc/guides/prog_guide/rte_flow.rst
>> +++ b/doc/guides/prog_guide/rte_flow.rst
>> @@ -980,6 +980,18 @@ Matches an ESP header.
>>  - ``hdr``: ESP header definition (``rte_esp.h``).
>>  - Default ``mask`` matches SPI only.
>>  
>> +Item: ``GENEVE``
>> +^^^^^^^^^^^^^^^
> 
> Missing "^" under title, this may trigger a warning during documentation
> generation.

I can add that one missing '^' while applying, no patch required.

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add GENEVE flow pattern item

[Adrien Mazarguil]

On Thu, Dec 07, 2017 at 03:52:44PM -0800, Ferruh Yigit wrote:
> On 12/4/2017 6:03 AM, Adrien Mazarguil wrote:
> > On Fri, Dec 01, 2017 at 10:43:15AM +0000, Andrew Rybchenko wrote:
> >> From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> >>
> >> Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
> >> Add default mask for the item.
> >>
> >> Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> >> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > 
> > Just one remaining nit, please see below. While you could address it through
> > a subsequent patch, you might as well send v3 directly to make things easier
> > for Ferruh.
> > 
> > Otherwise it's all good, thanks.
> 
> May I take this as (for series):
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Sure, thanks.

> >> ---
> >>  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
> >>  lib/librte_ether/rte_flow.c        |  1 +
> >>  lib/librte_ether/rte_flow.h        | 30 ++++++++++++++++++++++++++++++
> >>  3 files changed, 43 insertions(+)
> >>
> >> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> >> index d158be5..5b8f9c5 100644
> >> --- a/doc/guides/prog_guide/rte_flow.rst
> >> +++ b/doc/guides/prog_guide/rte_flow.rst
> >> @@ -980,6 +980,18 @@ Matches an ESP header.
> >>  - ``hdr``: ESP header definition (``rte_esp.h``).
> >>  - Default ``mask`` matches SPI only.
> >>  
> >> +Item: ``GENEVE``
> >> +^^^^^^^^^^^^^^^
> > 
> > Missing "^" under title, this may trigger a warning during documentation
> > generation.
> 
> I can add that one missing '^' while applying, no patch required.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add GENEVE flow pattern item

[Ferruh Yigit]

On 12/7/2017 3:52 PM, Ferruh Yigit wrote:
> On 12/4/2017 6:03 AM, Adrien Mazarguil wrote:
>> On Fri, Dec 01, 2017 at 10:43:15AM +0000, Andrew Rybchenko wrote:
>>> From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
>>>
>>> Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
>>> Add default mask for the item.
>>>
>>> Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>> Just one remaining nit, please see below. While you could address it through
>> a subsequent patch, you might as well send v3 directly to make things easier
>> for Ferruh.
>>
>> Otherwise it's all good, thanks.
> 
> May I take this as (for series):
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Series applied to dpdk-next-net/master, thanks.

[dpdk-dev] [PATCH v2 2/2] app/testpmd: support GENEVE pattern item in flow rules

[Andrew Rybchenko]

From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>

Add the ability to match VNI and protocol fields of GENEVE protocol header.

Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 app/test-pmd/cmdline_flow.c                 | 31 +++++++++++++++++++++++++++++
 app/test-pmd/config.c                       |  1 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  5 +++++
 3 files changed, 37 insertions(+)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index df16d2a..561e057 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -175,6 +175,9 @@ enum index {
 	ITEM_GTP_TEID,
 	ITEM_GTPC,
 	ITEM_GTPU,
+	ITEM_GENEVE,
+	ITEM_GENEVE_VNI,
+	ITEM_GENEVE_PROTO,
 
 	/* Validate/create actions. */
 	ACTIONS,
@@ -460,6 +463,7 @@ static const enum index next_item[] = {
 	ITEM_GTP,
 	ITEM_GTPC,
 	ITEM_GTPU,
+	ITEM_GENEVE,
 	ZERO,
 };
 
@@ -603,6 +607,13 @@ static const enum index item_gtp[] = {
 	ZERO,
 };
 
+static const enum index item_geneve[] = {
+	ITEM_GENEVE_VNI,
+	ITEM_GENEVE_PROTO,
+	ITEM_NEXT,
+	ZERO,
+};
+
 static const enum index next_action[] = {
 	ACTION_END,
 	ACTION_VOID,
@@ -1470,6 +1481,26 @@ static const struct token token_list[] = {
 		.next = NEXT(item_gtp),
 		.call = parse_vc,
 	},
+	[ITEM_GENEVE] = {
+		.name = "geneve",
+		.help = "match GENEVE header",
+		.priv = PRIV_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
+		.next = NEXT(item_geneve),
+		.call = parse_vc,
+	},
+	[ITEM_GENEVE_VNI] = {
+		.name = "vni",
+		.help = "virtual network identifier",
+		.next = NEXT(item_geneve, NEXT_ENTRY(UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_geneve, vni)),
+	},
+	[ITEM_GENEVE_PROTO] = {
+		.name = "protocol",
+		.help = "GENEVE protocol type",
+		.next = NEXT(item_geneve, NEXT_ENTRY(UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_geneve,
+					     protocol)),
+	},
 
 	/* Validate/create actions. */
 	[ACTIONS] = {
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index cd2ac11..86ca3aa 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -973,6 +973,7 @@ static const struct {
 	MK_FLOW_ITEM(GTP, sizeof(struct rte_flow_item_gtp)),
 	MK_FLOW_ITEM(GTPC, sizeof(struct rte_flow_item_gtp)),
 	MK_FLOW_ITEM(GTPU, sizeof(struct rte_flow_item_gtp)),
+	MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
 };
 
 /** Compute storage space needed by item specification. */
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 9789139..2b00be8 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3107,6 +3107,11 @@ This section lists supported pattern items and their attributes, if any.
 
   - ``teid {unsigned}``: tunnel endpoint identifier.
 
+- ``geneve``: match GENEVE header.
+
+  - ``vni {unsigned}``: virtual network identifier.
+  - ``protocol {unsigned}``: protocol type.
+
 Actions list
 ^^^^^^^^^^^^
 
-- 
2.7.4