ethdev: add L2TPv3 header to flow API

  - RTE_FLOW_ITEM_TYPE_L2TPV3: matches a L2TPv3 header

Signed-off-by: Rory Sexton <rory.sexton@intel.com>
Signed-off-by: Dariusz Jagus <dariuszx.jagus@intel.com>
---
 app/test-pmd/cmdline_flow.c                 | 28 +++++++++++++++++++++
 doc/guides/prog_guide/rte_flow.rst          |  8 ++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
 lib/librte_ethdev/rte_flow.c                |  1 +
 lib/librte_ethdev/rte_flow.h                | 27 ++++++++++++++++++++
 5 files changed, 68 insertions(+)
  

Hi Rory,

One general question why do we want to support only v3 and not also v2?

PSB for some other comments.

Best,
Ori Kam

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Rory Sexton
> Sent: Wednesday, December 4, 2019 4:11 PM
> To: dev@dpdk.org
> Cc: qi.z.zhang@intel.com; beilei.xing@intel.com; rory.sexton@intel.com;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>; Dariusz Jagus
> <dariuszx.jagus@intel.com>
> Subject: [dpdk-dev] [PATCH] ethdev: add L2TPv3 header to flow API
> 
> - RTE_FLOW_ITEM_TYPE_L2TPV3: matches a L2TPv3 header
> 
> Signed-off-by: Rory Sexton <rory.sexton@intel.com>
> Signed-off-by: Dariusz Jagus <dariuszx.jagus@intel.com>
> ---
>  app/test-pmd/cmdline_flow.c                 | 28 +++++++++++++++++++++
>  doc/guides/prog_guide/rte_flow.rst          |  8 ++++++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
>  lib/librte_ethdev/rte_flow.c                |  1 +
>  lib/librte_ethdev/rte_flow.h                | 27 ++++++++++++++++++++
>  5 files changed, 68 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 99dade7d8..a7fe7a7a8 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -213,6 +213,8 @@ enum index {
>  	ITEM_TAG,
>  	ITEM_TAG_DATA,
>  	ITEM_TAG_INDEX,
> +	ITEM_L2TPV3,
> +	ITEM_L2TPV3_SESSION_ID,
> 
>  	/* Validate/create actions. */
>  	ACTIONS,
> @@ -746,6 +748,7 @@ static const enum index next_item[] = {
>  	ITEM_PPPOE_PROTO_ID,
>  	ITEM_HIGIG2,
>  	ITEM_TAG,
> +	ITEM_L2TPV3,
>  	END_SET,
>  	ZERO,
>  };
> @@ -1030,6 +1033,12 @@ static const enum index item_tag[] = {
>  	ZERO,
>  };
> 
> +static const enum index item_l2tpv3[] = {
> +	ITEM_L2TPV3_SESSION_ID,
> +	ITEM_NEXT,
> +	ZERO,
> +};
> +
>  static const enum index next_action[] = {
>  	ACTION_END,
>  	ACTION_VOID,
> @@ -2593,6 +2602,21 @@ static const struct token token_list[] = {
>  			     NEXT_ENTRY(ITEM_PARAM_IS)),
>  		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_tag,
> index)),
>  	},
> +	[ITEM_L2TPV3] = {
> +		.name = "l2tpv3",
> +		.help = "match L2TPv3 header",
> +		.priv = PRIV_ITEM(L2TPV3, sizeof(struct
> rte_flow_item_l2tpv3)),
> +		.next = NEXT(item_l2tpv3),
> +		.call = parse_vc,
> +	},
> +	[ITEM_L2TPV3_SESSION_ID] = {
> +		.name = "session_id",
> +		.help = "session identifier",
> +		.next = NEXT(item_l2tpv3, NEXT_ENTRY(UNSIGNED),
> item_param),
> +		.args = ARGS(ARGS_ENTRY_HTON(struct
> rte_flow_item_l2tpv3,
> +					     session_id)),
> +	},
> +
>  	/* Validate/create actions. */
>  	[ACTIONS] = {
>  		.name = "actions",
> @@ -6238,6 +6262,10 @@ flow_item_default_mask(const struct
> rte_flow_item *item)
>  		break;
>  	case RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID:
>  		mask = &rte_flow_item_pppoe_proto_id_mask;
> +		break;
> +	case RTE_FLOW_ITEM_TYPE_L2TPV3:
> +		mask = &rte_flow_item_l2tpv3_mask;
> +		break;
>  	default:
>  		break;
>  	}
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index a254c81ef..c7ddb38c5 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1336,6 +1336,14 @@ Broadcom switches.
> 
>  - Default ``mask`` matches classification and vlan.
> 
> +Item: ``L2TPV3``
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Matches a L2TPv3 header.
> +
> +- ``session_id``: L2TPv3 session identifier.
> +- Default ``mask`` matches session_id only.
> +
> 
>  Actions
>  ~~~~~~~
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 73ef0b41d..a48e77619 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3954,6 +3954,10 @@ This section lists supported pattern items and their
> attributes, if any.
> 
>    - ``proto_id {unsigned}``: PPP protocol identifier.
> 
> +- ``l2tpv3``: match L2TPv3 header.
> +
> +  - ``session_id {unsigned}``: L2TPv3 session identifier.
> +
>  Actions list
>  ^^^^^^^^^^^^
> 
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 87a3e8c4c..fcda73320 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -93,6 +93,7 @@ static const struct rte_flow_desc_data
> rte_flow_desc_item[] = {
>  	MK_FLOW_ITEM(IGMP, sizeof(struct rte_flow_item_igmp)),
>  	MK_FLOW_ITEM(AH, sizeof(struct rte_flow_item_ah)),
>  	MK_FLOW_ITEM(HIGIG2, sizeof(struct rte_flow_item_higig2_hdr)),
> +	MK_FLOW_ITEM(L2TPV3, sizeof(struct rte_flow_item_l2tpv3)),
>  };
> 
>  /** Generate flow_action[] entry. */
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 452d359a1..5ee055c28 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -510,6 +510,16 @@ enum rte_flow_item_type {
>  	 * See struct rte_flow_item_tag.
>  	 */
>  	RTE_FLOW_ITEM_TYPE_TAG,
> +
> +	/*

Missing *. It should be /**

> +	 * Matches a L2TPv3 header.
> +	 *
> +	 * Configure flow for L2TPv3 packets.
> +	 *
> +	 * See struct rte_flow_item_l2tpv3.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_L2TPV3,
> +
>  };
> 
>  /**
> @@ -1373,6 +1383,23 @@ static const struct rte_flow_item_tag
> rte_flow_item_tag_mask = {
>  };
>  #endif
> 
> +/**
> + * RTE_FLOW_ITEM_TYPE_L2TPV3.
> + *
> + * Matches a L2TPv3 header.
> + */
> +struct rte_flow_item_l2tpv3 {
> +	rte_be32_t session_id; /**< Session ID. */
> +};

Does it make sense that in future we will want to match on the 
T / L / ver / Ns / Nr?

Please also consider that any change will be ABI / API breakage, which will be allowed
only next year.

> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_L2TPV3. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_l2tpv3 rte_flow_item_l2tpv3_mask = {
> +	.session_id = RTE_BE32(UINT32_MAX),
> +};
> +#endif
> +
> +
>  /**
>   * @warning
>   * @b EXPERIMENTAL: this structure may change without prior notice
> --
> 2.17.1
> 
> --------------------------------------------------------------
> Intel Research and Development Ireland Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
> 
> 
> This e-mail and any attachments may contain confidential material for the
> sole
> use of the intended recipient(s). Any review or distribution by others is
> strictly prohibited. If you are not the intended recipient, please contact the
> sender and delete all copies.
  

Hi Ori,

> One general question why do we want to support only v3 and not also v2?
l2tpv3 is more widely used as a tunneling protocol hence it's inclusion.
A specific example is the cable industry where DOCSIS cable traffic is encapsulated using depi and uepi protocols which both make use of l2tpv3 session ids.
Directing flows based on l2tpv3 can be very useful in these cases.

Some more comments inline below.

Rory

>> diff --git a/lib/librte_ethdev/rte_flow.h 
>> b/lib/librte_ethdev/rte_flow.h index 452d359a1..5ee055c28 100644
>> --- a/lib/librte_ethdev/rte_flow.h
>> +++ b/lib/librte_ethdev/rte_flow.h
>> @@ -510,6 +510,16 @@ enum rte_flow_item_type {
>>  	 * See struct rte_flow_item_tag.
>>  	 */
>>  	RTE_FLOW_ITEM_TYPE_TAG,
>> +
>> +	/*
>
>Missing *. It should be /**
>

Will correct this in another version of this patch.

>> +/**
>> + * RTE_FLOW_ITEM_TYPE_L2TPV3.
>> + *
>> + * Matches a L2TPv3 header.
>> + */
>> +struct rte_flow_item_l2tpv3 {
>> +	rte_be32_t session_id; /**< Session ID. */ };
>
>Does it make sense that in future we will want to match on the T / L / ver / Ns / Nr?
>
>Please also consider that any change will be ABI / API breakage, which will be allowed only next year. 
>

I don't foresee us wanting to match on any of the other fields, T / L / ver / Ns/ Nr.
The session id would typically be the only field of interest in the l2tpv3 header.
  

Hi Rory,

> -----Original Message-----
> From: Sexton, Rory <rory.sexton@intel.com>
> Sent: Tuesday, December 10, 2019 4:52 PM
> To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>; Jagus, DariuszX
> <dariuszx.jagus@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] ethdev: add L2TPv3 header to flow API
> 
> Hi Ori,
> 
> > One general question why do we want to support only v3 and not also v2?
> l2tpv3 is more widely used as a tunneling protocol hence it's inclusion.
> A specific example is the cable industry where DOCSIS cable traffic is
> encapsulated using depi and uepi protocols which both make use of l2tpv3
> session ids.
> Directing flows based on l2tpv3 can be very useful in these cases.
> 

I'm not saying that v3 is not used more, I just thought from looking at the spec
that both can be supported and the only difference is the version, so matching on
the version we will be able to support both versions.
Your decision.

> Some more comments inline below.
> 
> Rory
> 
> >> diff --git a/lib/librte_ethdev/rte_flow.h
> >> b/lib/librte_ethdev/rte_flow.h index 452d359a1..5ee055c28 100644
> >> --- a/lib/librte_ethdev/rte_flow.h
> >> +++ b/lib/librte_ethdev/rte_flow.h
> >> @@ -510,6 +510,16 @@ enum rte_flow_item_type {
> >>  	 * See struct rte_flow_item_tag.
> >>  	 */
> >>  	RTE_FLOW_ITEM_TYPE_TAG,
> >> +
> >> +	/*
> >
> >Missing *. It should be /**
> >
> 
> Will correct this in another version of this patch.
> 
> >> +/**
> >> + * RTE_FLOW_ITEM_TYPE_L2TPV3.
> >> + *
> >> + * Matches a L2TPv3 header.
> >> + */
> >> +struct rte_flow_item_l2tpv3 {
> >> +	rte_be32_t session_id; /**< Session ID. */ };
> >
> >Does it make sense that in future we will want to match on the T / L / ver /
> Ns / Nr?
> >
> >Please also consider that any change will be ABI / API breakage, which will
> be allowed only next year.
> >
> 
> I don't foresee us wanting to match on any of the other fields, T / L / ver /
> Ns/ Nr.
> The session id would typically be the only field of interest in the l2tpv3
> header.

I think that adding all fields to the structure will solve the possible issue with adding matching later.
Also and even more important you will be able to use it for creating the  raw_encap / raw_decap buffers.

What do you think?

Best,
Ori
  

Hi Ori,

See my comments below.

Regards,
Rory

>> 
>>> One general question why do we want to support only v3 and not also v2?
>> l2tpv3 is more widely used as a tunneling protocol hence it's inclusion.
>> A specific example is the cable industry where DOCSIS cable traffic is 
>> encapsulated using depi and uepi protocols which both make use of 
>> l2tpv3 session ids.
>> Directing flows based on l2tpv3 can be very useful in these cases.
>> 
>
>I'm not saying that v3 is not used more, I just thought from looking at the spec that both can be supported and the only difference is the version, so matching on the version we will be able to support both versions.
>Your decision.
>

There is more difference between l2tpv2 and l2tpv3 than just the version number.
In L2TPv2 there exists a 16 bit Tunnel ID and 16 bit Session ID. This is changed to a 32 bit Session ID in L2TPv3
Please see difference in headers below for v2 and v3.
Due to the differences between v2 and v3 I don't think both can be supported with same flow item.

                                                          L2TPv2
    0...............................................15, 16......................................31
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |T|L|x|x|S|x|O|P|x|x|x|x|  Ver  |          Length (opt)                  |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |           Tunnel ID                               |           Session ID                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |             Ns (opt)                               |             Nr (opt)                        |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |      Offset Size (opt)                        |    Offset pad... (opt)               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

                                                          L2TPv3
    0...............................................15, 16......................................31
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                           Session ID                                                                  |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |               Cookie (optional, maximum 64 bits)...
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                                                                                                                    |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

>> >> +/**
>> >> + * RTE_FLOW_ITEM_TYPE_L2TPV3.
>> >> + *
>> >> + * Matches a L2TPv3 header.
>> >> + */
>> >> +struct rte_flow_item_l2tpv3 {
>> >> +	rte_be32_t session_id; /**< Session ID. */ };
>> >
>> >Does it make sense that in future we will want to match on the T / L 
>> >/ ver /
>> Ns / Nr?
>> >
>> >Please also consider that any change will be ABI / API breakage, 
>> >which will
>> be allowed only next year.
>> >
>> 
>> I don't foresee us wanting to match on any of the other fields, T / L 
>> / ver / Ns/ Nr.
>> The session id would typically be the only field of interest in the 
>> l2tpv3 header.
>
> I think that adding all fields to the structure will solve the possible issue with adding matching later.
> Also and even more important you will be able to use it for creating the  raw_encap / raw_decap buffers.
>
>What do you think?

Based on the differences between v2 and v3 the only field of interest in l2tpv3 over IP is the Session ID.
I agree it would make sense to add all fields if we were implementing l2tpv2 however v2 would require a different implementation to v3 due to the differences between both Protocols.
  

Hi Rory,

PSB

> -----Original Message-----
> From: Sexton, Rory <rory.sexton@intel.com>
> Sent: Wednesday, December 11, 2019 1:36 PM
> To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>; Jagus, DariuszX
> <dariuszx.jagus@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] ethdev: add L2TPv3 header to flow API
> 
> Hi Ori,
> 
> See my comments below.
> 
> Regards,
> Rory
> 
> >>
> >>> One general question why do we want to support only v3 and not also
> v2?
> >> l2tpv3 is more widely used as a tunneling protocol hence it's inclusion.
> >> A specific example is the cable industry where DOCSIS cable traffic is
> >> encapsulated using depi and uepi protocols which both make use of
> >> l2tpv3 session ids.
> >> Directing flows based on l2tpv3 can be very useful in these cases.
> >>
> >
> >I'm not saying that v3 is not used more, I just thought from looking at the
> spec that both can be supported and the only difference is the version, so
> matching on the version we will be able to support both versions.
> >Your decision.
> >
> 
> There is more difference between l2tpv2 and l2tpv3 than just the version
> number.
> In L2TPv2 there exists a 16 bit Tunnel ID and 16 bit Session ID. This is changed
> to a 32 bit Session ID in L2TPv3
> Please see difference in headers below for v2 and v3.
> Due to the differences between v2 and v3 I don't think both can be
> supported with same flow item.
> 
>                                                           L2TPv2
>     0...............................................15, 16......................................31
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |T|L|x|x|S|x|O|P|x|x|x|x|  Ver  |          Length (opt)                  |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |           Tunnel ID                               |           Session ID                     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |             Ns (opt)                               |             Nr (opt)                        |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |      Offset Size (opt)                        |    Offset pad... (opt)               |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
>                                                           L2TPv3
>     0...............................................15, 16......................................31
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                           Session ID                                                                  |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |               Cookie (optional, maximum 64 bits)...
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>                                                                                                                     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 

Since we have the version, they can by using union,
But like I said your call.

> >> >> +/**
> >> >> + * RTE_FLOW_ITEM_TYPE_L2TPV3.
> >> >> + *
> >> >> + * Matches a L2TPv3 header.
> >> >> + */
> >> >> +struct rte_flow_item_l2tpv3 {
> >> >> +	rte_be32_t session_id; /**< Session ID. */ };
> >> >
> >> >Does it make sense that in future we will want to match on the T / L
> >> >/ ver /
> >> Ns / Nr?
> >> >
> >> >Please also consider that any change will be ABI / API breakage,
> >> >which will
> >> be allowed only next year.
> >> >
> >>
> >> I don't foresee us wanting to match on any of the other fields, T / L
> >> / ver / Ns/ Nr.
> >> The session id would typically be the only field of interest in the
> >> l2tpv3 header.
> >
> > I think that adding all fields to the structure will solve the possible issue
> with adding matching later.
> > Also and even more important you will be able to use it for creating the
> raw_encap / raw_decap buffers.
> >
> >What do you think?
> 
> Based on the differences between v2 and v3 the only field of interest in
> l2tpv3 over IP is the Session ID.
> I agree it would make sense to add all fields if we were implementing l2tpv2
> however v2 would require a different implementation to v3 due to the
> differences between both Protocols.

Even if we just support v3, I think that it is a good idea to add all fields of v3.
This will allow the use of the raw_encap / raw_decap actions.
Please also note that you didn't add the new item to  cmd_set_raw_parsed function.
this function is used in order to create raw_encap/ raw_decap encapsulations.

Best,
Ori
  

Hi Ori,

See comments below.

Regards,
Rory

> > > > > > +/**
> > > > > > + * RTE_FLOW_ITEM_TYPE_L2TPV3.
> > > > > > + *
> > > > > > + * Matches a L2TPv3 header.
> > > > > > + */
> > > > > > +struct rte_flow_item_l2tpv3 {
> > > > > > +	rte_be32_t session_id; /**< Session ID. */ };
> > > > >
> > > > >Does it make sense that in future we will want to match on the T / 
> > > > >L / ver /
> > > > Ns / Nr?
> > > > >
> > > > >Please also consider that any change will be ABI / API breakage, 
> > > > >which will
> > > > >be allowed only next year.
> > > > >
> > > >
> > > > I don't foresee us wanting to match on any of the other fields, T / 
> > > > L / ver / Ns/ Nr.
> > > > The session id would typically be the only field of interest in the
> > > > l2tpv3 header.
> > >
> > > I think that adding all fields to the structure will solve the 
> > > possible issue
> > with adding matching later.
> > > Also and even more important you will be able to use it for creating 
> > > the
> > raw_encap / raw_decap buffers.
> > >
> > >What do you think?
> > 
> > Based on the differences between v2 and v3 the only field of interest 
> > in
> > l2tpv3 over IP is the Session ID.
> > I agree it would make sense to add all fields if we were implementing 
> > l2tpv2 however v2 would require a different implementation to v3 due 
> > to the differences between both Protocols.
>
> Even if we just support v3, I think that it is a good idea to add all fields of v3. 
> This will allow the use of the raw_encap / raw_decap actions.
> Please also note that you didn't add the new item to  cmd_set_raw_parsed function.
> this function is used in order to create raw_encap/ raw_decap encapsulations.

I think the confusion here is based on the fact that there are 2 separate types of l2tpv3.
- l2tpv3 over UDP, which is very similar to l2tpv2 with only change being 16 bit Tunnel ID 
  and 16 bit Session ID changing to 32 bit Session ID. All other fields remain (T/L/Ver/Ns/Nr).
- l2tpv3 over IP is another type of l2tpv3 that is carried over IP rather than UDP and as such
  the message format is entirely different and consists of just a 32 bit Session ID. The other
  fields mentioned above do not exist at all in this l2tpv3 header.

This patch was targeted at creating a flow to handle l2tpv3 over IP exclusively. This is why
the Session ID is the only field in the flow item.

I can add the item to cmd_set_raw_parsed function.
  

Hi Ori,

Let me rework the patch to make it clearer that this is supporting new flow type for l2tpv3 over IP, rather than l2tpv2/v3 over UDP which is how you interpreted it.
Will take into account all your feedback. Please review v2 once I submit.

Regards,
Rory

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Sexton, Rory
Sent: Wednesday, December 11, 2019 4:31 PM
To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Adrien Mazarguil <adrien.mazarguil@6wind.com>; Jagus, DariuszX <dariuszx.jagus@intel.com>
Subject: Re: [dpdk-dev] [PATCH] ethdev: add L2TPv3 header to flow API

Hi Ori,

See comments below.

Regards,
Rory

> > > > > > +/**
> > > > > > + * RTE_FLOW_ITEM_TYPE_L2TPV3.
> > > > > > + *
> > > > > > + * Matches a L2TPv3 header.
> > > > > > + */
> > > > > > +struct rte_flow_item_l2tpv3 {
> > > > > > +	rte_be32_t session_id; /**< Session ID. */ };
> > > > >
> > > > >Does it make sense that in future we will want to match on the 
> > > > >T / L / ver /
> > > > Ns / Nr?
> > > > >
> > > > >Please also consider that any change will be ABI / API 
> > > > >breakage, which will be allowed only next year.
> > > > >
> > > >
> > > > I don't foresee us wanting to match on any of the other fields, 
> > > > T / L / ver / Ns/ Nr.
> > > > The session id would typically be the only field of interest in 
> > > > the
> > > > l2tpv3 header.
> > >
> > > I think that adding all fields to the structure will solve the 
> > > possible issue
> > with adding matching later.
> > > Also and even more important you will be able to use it for 
> > > creating the
> > raw_encap / raw_decap buffers.
> > >
> > >What do you think?
> > 
> > Based on the differences between v2 and v3 the only field of 
> > interest in
> > l2tpv3 over IP is the Session ID.
> > I agree it would make sense to add all fields if we were 
> > implementing
> > l2tpv2 however v2 would require a different implementation to v3 due 
> > to the differences between both Protocols.
>
> Even if we just support v3, I think that it is a good idea to add all fields of v3. 
> This will allow the use of the raw_encap / raw_decap actions.
> Please also note that you didn't add the new item to  cmd_set_raw_parsed function.
> this function is used in order to create raw_encap/ raw_decap encapsulations.

I think the confusion here is based on the fact that there are 2 separate types of l2tpv3.
- l2tpv3 over UDP, which is very similar to l2tpv2 with only change being 16 bit Tunnel ID
  and 16 bit Session ID changing to 32 bit Session ID. All other fields remain (T/L/Ver/Ns/Nr).
- l2tpv3 over IP is another type of l2tpv3 that is carried over IP rather than UDP and as such
  the message format is entirely different and consists of just a 32 bit Session ID. The other
  fields mentioned above do not exist at all in this l2tpv3 header.

This patch was targeted at creating a flow to handle l2tpv3 over IP exclusively. This is why the Session ID is the only field in the flow item.

I can add the item to cmd_set_raw_parsed function.