[dpdk-dev] app/testpmd: fix PPPOES flow API

[dpdk-dev] app/testpmd: fix PPPOES flow API

[Xiao Zhang]

The command line to create RTE flow for specific proto_id of PPPOES is
not correct. This patch is to fix this issue.

Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
Cc: stable@dpdk.org

Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
---
 app/test-pmd/cmdline_flow.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index a78154502..c25a2598d 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -768,7 +768,6 @@ static const enum index next_item[] = {
 	ITEM_GTP_PSC,
 	ITEM_PPPOES,
 	ITEM_PPPOED,
-	ITEM_PPPOE_PROTO_ID,
 	ITEM_HIGIG2,
 	ITEM_TAG,
 	ITEM_L2TPV3OIP,
@@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] = {
 
 static const enum index item_pppoes[] = {
 	ITEM_PPPOE_SEID,
-	ITEM_NEXT,
-	ZERO,
-};
-
-static const enum index item_pppoe_proto_id[] = {
 	ITEM_PPPOE_PROTO_ID,
 	ITEM_NEXT,
 	ZERO,
@@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
 	[ITEM_PPPOE_PROTO_ID] = {
 		.name = "proto_id",
 		.help = "match PPPoE session protocol identifier",
-		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
-				sizeof(struct rte_flow_item_pppoe_proto_id)),
-		.next = NEXT(item_pppoe_proto_id),
-		.call = parse_vc,
+		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY_HTON
+			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
 	},
 	[ITEM_HIGIG2] = {
 		.name = "higig2",
-- 
2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Ori Kam]

Hi Xiao,

Is the proto_id part of the basic header or not?

From the spec it looks like a different header.

If it is part of the original header then all documentations and rte_structs should be
changed, to reflect this.

It will be very helpful if the patch message would explain the bug and why it was
changed.

Also please see inline other comment.

Best,
Ori

> -----Original Message-----
> From: Xiao Zhang <xiao.zhang@intel.com>
> Sent: Friday, March 27, 2020 11:19 AM
> To: dev@dpdk.org
> Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> <xiao.zhang@intel.com>; stable@dpdk.org
> Subject: app/testpmd: fix PPPOES flow API
> 
> The command line to create RTE flow for specific proto_id of PPPOES is
> not correct. This patch is to fix this issue.
> 
> Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> ---
>  app/test-pmd/cmdline_flow.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index a78154502..c25a2598d 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -768,7 +768,6 @@ static const enum index next_item[] = {
>  	ITEM_GTP_PSC,
>  	ITEM_PPPOES,
>  	ITEM_PPPOED,
> -	ITEM_PPPOE_PROTO_ID,
>  	ITEM_HIGIG2,
>  	ITEM_TAG,
>  	ITEM_L2TPV3OIP,
> @@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] = {
> 
>  static const enum index item_pppoes[] = {
>  	ITEM_PPPOE_SEID,
> -	ITEM_NEXT,
> -	ZERO,
> -};
> -
> -static const enum index item_pppoe_proto_id[] = {
>  	ITEM_PPPOE_PROTO_ID,
>  	ITEM_NEXT,
>  	ZERO,
> @@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
>  	[ITEM_PPPOE_PROTO_ID] = {
>  		.name = "proto_id",
>  		.help = "match PPPoE session protocol identifier",
> -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> -				sizeof(struct rte_flow_item_pppoe_proto_id)),
> -		.next = NEXT(item_pppoe_proto_id),
> -		.call = parse_vc,
> +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> item_param),
> +		.args = ARGS(ARGS_ENTRY_HTON
> +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),

Where is the memory for this proto_id is defined?

>  	},
>  	[ITEM_HIGIG2] = {
>  		.name = "higig2",
> --
> 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Zhang, Xiao]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Sunday, March 29, 2020 2:28 PM
> To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Xiao,
> 
> Is the proto_id part of the basic header or not?

Proto_id is part of PPPOE session header,

> 
> From the spec it looks like a different header.
> 
> If it is part of the original header then all documentations and rte_structs should
> be changed, to reflect this.
> 
> It will be very helpful if the patch message would explain the bug and why it was
> changed.

Okay, will add more message. The next value of the ITEM_PPPOE_PROTO_ID should be unsigned value but not item list.

> 
> Also please see inline other comment.
> 
> Best,
> Ori
> 
> > -----Original Message-----
> > From: Xiao Zhang <xiao.zhang@intel.com>
> > Sent: Friday, March 27, 2020 11:19 AM
> > To: dev@dpdk.org
> > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > <xiao.zhang@intel.com>; stable@dpdk.org
> > Subject: app/testpmd: fix PPPOES flow API
> >
> > The command line to create RTE flow for specific proto_id of PPPOES is
> > not correct. This patch is to fix this issue.
> >
> > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > ---
> >  app/test-pmd/cmdline_flow.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index a78154502..c25a2598d 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> >  	ITEM_GTP_PSC,
> >  	ITEM_PPPOES,
> >  	ITEM_PPPOED,
> > -	ITEM_PPPOE_PROTO_ID,
> >  	ITEM_HIGIG2,
> >  	ITEM_TAG,
> >  	ITEM_L2TPV3OIP,
> > @@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] = {
> >
> >  static const enum index item_pppoes[] = {
> >  	ITEM_PPPOE_SEID,
> > -	ITEM_NEXT,
> > -	ZERO,
> > -};
> > -
> > -static const enum index item_pppoe_proto_id[] = {
> >  	ITEM_PPPOE_PROTO_ID,
> >  	ITEM_NEXT,
> >  	ZERO,
> > @@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
> >  	[ITEM_PPPOE_PROTO_ID] = {
> >  		.name = "proto_id",
> >  		.help = "match PPPoE session protocol identifier",
> > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > -				sizeof(struct rte_flow_item_pppoe_proto_id)),
> > -		.next = NEXT(item_pppoe_proto_id),
> > -		.call = parse_vc,
> > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > item_param),
> > +		.args = ARGS(ARGS_ENTRY_HTON
> > +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
> 
> Where is the memory for this proto_id is defined?

Do you mean this?
lib/librte_ethdev/rte_flow.h
1360 struct rte_flow_item_pppoe_proto_id {
1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
1362 };

> 
> >  	},
> >  	[ITEM_HIGIG2] = {
> >  		.name = "higig2",
> > --
> > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Ori Kam]

Hi Xiao,

> -----Original Message-----
> From: Zhang, Xiao <xiao.zhang@intel.com>
> Sent: Sunday, March 29, 2020 12:06 PM
> To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Ori,
> 
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> > Sent: Sunday, March 29, 2020 2:28 PM
> > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> stable@dpdk.org
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> > Hi Xiao,
> >
> > Is the proto_id part of the basic header or not?
> 
> Proto_id is part of PPPOE session header,
> 

Where is the porto_id located? Inside the payload?

> >
> > From the spec it looks like a different header.
> >
> > If it is part of the original header then all documentations and rte_structs
> should
> > be changed, to reflect this.
> >
> > It will be very helpful if the patch message would explain the bug and why it
> was
> > changed.
> 
> Okay, will add more message. The next value of the ITEM_PPPOE_PROTO_ID
> should be unsigned value but not item list.
> 
> >
> > Also please see inline other comment.
> >
> > Best,
> > Ori
> >
> > > -----Original Message-----
> > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > Sent: Friday, March 27, 2020 11:19 AM
> > > To: dev@dpdk.org
> > > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > Subject: app/testpmd: fix PPPOES flow API
> > >
> > > The command line to create RTE flow for specific proto_id of PPPOES is
> > > not correct. This patch is to fix this issue.
> > >
> > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > ---
> > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > > index a78154502..c25a2598d 100644
> > > --- a/app/test-pmd/cmdline_flow.c
> > > +++ b/app/test-pmd/cmdline_flow.c
> > > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> > >  	ITEM_GTP_PSC,
> > >  	ITEM_PPPOES,
> > >  	ITEM_PPPOED,
> > > -	ITEM_PPPOE_PROTO_ID,
> > >  	ITEM_HIGIG2,
> > >  	ITEM_TAG,
> > >  	ITEM_L2TPV3OIP,
> > > @@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] = {
> > >
> > >  static const enum index item_pppoes[] = {
> > >  	ITEM_PPPOE_SEID,
> > > -	ITEM_NEXT,
> > > -	ZERO,
> > > -};
> > > -
> > > -static const enum index item_pppoe_proto_id[] = {
> > >  	ITEM_PPPOE_PROTO_ID,
> > >  	ITEM_NEXT,
> > >  	ZERO,
> > > @@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
> > >  	[ITEM_PPPOE_PROTO_ID] = {
> > >  		.name = "proto_id",
> > >  		.help = "match PPPoE session protocol identifier",
> > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > -				sizeof(struct rte_flow_item_pppoe_proto_id)),
> > > -		.next = NEXT(item_pppoe_proto_id),
> > > -		.call = parse_vc,
> > > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > > item_param),
> > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
> >
> > Where is the memory for this proto_id is defined?
> 
> Do you mean this?
> lib/librte_ethdev/rte_flow.h
> 1360 struct rte_flow_item_pppoe_proto_id {
> 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> 1362 };
> 

Yes. Why don't you use this one?

> >
> > >  	},
> > >  	[ITEM_HIGIG2] = {
> > >  		.name = "higig2",
> > > --
> > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Zhang, Xiao]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Sunday, March 29, 2020 6:19 PM
> To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Xiao,
> 
> > -----Original Message-----
> > From: Zhang, Xiao <xiao.zhang@intel.com>
> > Sent: Sunday, March 29, 2020 12:06 PM
> > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > stable@dpdk.org
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> > Hi Ori,
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@mellanox.com>
> > > Sent: Sunday, March 29, 2020 2:28 PM
> > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > stable@dpdk.org
> > > Subject: RE: app/testpmd: fix PPPOES flow API
> > >
> > > Hi Xiao,
> > >
> > > Is the proto_id part of the basic header or not?
> >
> > Proto_id is part of PPPOE session header,
> >
> 
> Where is the porto_id located? Inside the payload?

Yes, my previous explanation was not clear. The protocol ID is in the beginning of the payload in PPP Session Stage according to RFC2516.

                     	         1                                  2                                3
   0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3  4  5 6 7 8 9 0 1
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |  VER   | TYPE   |      CODE      |                  SESSION_ID                    |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                 LENGTH                    |                      payload                      ~
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

> 
> > >
> > > From the spec it looks like a different header.
> > >
> > > If it is part of the original header then all documentations and
> > > rte_structs
> > should
> > > be changed, to reflect this.
> > >
> > > It will be very helpful if the patch message would explain the bug
> > > and why it
> > was
> > > changed.
> >
> > Okay, will add more message. The next value of the ITEM_PPPOE_PROTO_ID
> > should be unsigned value but not item list.
> >
> > >
> > > Also please see inline other comment.
> > >
> > > Best,
> > > Ori
> > >
> > > > -----Original Message-----
> > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > To: dev@dpdk.org
> > > > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > > > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > Subject: app/testpmd: fix PPPOES flow API
> > > >
> > > > The command line to create RTE flow for specific proto_id of
> > > > PPPOES is not correct. This patch is to fix this issue.
> > > >
> > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > ---
> > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > b/app/test-pmd/cmdline_flow.c index a78154502..c25a2598d 100644
> > > > --- a/app/test-pmd/cmdline_flow.c
> > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> > > >  	ITEM_GTP_PSC,
> > > >  	ITEM_PPPOES,
> > > >  	ITEM_PPPOED,
> > > > -	ITEM_PPPOE_PROTO_ID,
> > > >  	ITEM_HIGIG2,
> > > >  	ITEM_TAG,
> > > >  	ITEM_L2TPV3OIP,
> > > > @@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] = {
> > > >
> > > >  static const enum index item_pppoes[] = {
> > > >  	ITEM_PPPOE_SEID,
> > > > -	ITEM_NEXT,
> > > > -	ZERO,
> > > > -};
> > > > -
> > > > -static const enum index item_pppoe_proto_id[] = {
> > > >  	ITEM_PPPOE_PROTO_ID,
> > > >  	ITEM_NEXT,
> > > >  	ZERO,
> > > > @@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
> > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > >  		.name = "proto_id",
> > > >  		.help = "match PPPoE session protocol identifier",
> > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > -				sizeof(struct rte_flow_item_pppoe_proto_id)),
> > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > -		.call = parse_vc,
> > > > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > > > item_param),
> > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
> > >
> > > Where is the memory for this proto_id is defined?
> >
> > Do you mean this?
> > lib/librte_ethdev/rte_flow.h
> > 1360 struct rte_flow_item_pppoe_proto_id {
> > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > 1362 };
> >
> 
> Yes. Why don't you use this one?

I think I was using this, am I using it incorrectly?

+		.args = ARGS(ARGS_ENTRY_HTON
+			     (struct rte_flow_item_pppoe_proto_id, proto_id)),

> 
> > >
> > > >  	},
> > > >  	[ITEM_HIGIG2] = {
> > > >  		.name = "higig2",
> > > > --
> > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Ori Kam]

Hi Xiao,

> -----Original Message-----
> From: Zhang, Xiao <xiao.zhang@intel.com>
> Sent: Sunday, March 29, 2020 3:00 PM
> To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Ori,
> 
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> > Sent: Sunday, March 29, 2020 6:19 PM
> > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> stable@dpdk.org
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> > Hi Xiao,
> >
> > > -----Original Message-----
> > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > Sent: Sunday, March 29, 2020 12:06 PM
> > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > stable@dpdk.org
> > > Subject: RE: app/testpmd: fix PPPOES flow API
> > >
> > > Hi Ori,
> > >
> > > > -----Original Message-----
> > > > From: Ori Kam <orika@mellanox.com>
> > > > Sent: Sunday, March 29, 2020 2:28 PM
> > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > stable@dpdk.org
> > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > >
> > > > Hi Xiao,
> > > >
> > > > Is the proto_id part of the basic header or not?
> > >
> > > Proto_id is part of PPPOE session header,
> > >
> >
> > Where is the porto_id located? Inside the payload?
> 
> Yes, my previous explanation was not clear. The protocol ID is in the beginning
> of the payload in PPP Session Stage according to RFC2516.
> 
>                      	         1                                  2                                3
>    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3  4  5 6 7 8 9 0 1
>  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  |  VER   | TYPE   |      CODE      |                  SESSION_ID                    |
>  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>  |                 LENGTH                    |                      payload                      ~
>  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 

Yes this is what I thought, does proto_id must be the first part of the payload?

> >
> > > >
> > > > From the spec it looks like a different header.
> > > >
> > > > If it is part of the original header then all documentations and
> > > > rte_structs
> > > should
> > > > be changed, to reflect this.
> > > >
> > > > It will be very helpful if the patch message would explain the bug
> > > > and why it
> > > was
> > > > changed.
> > >
> > > Okay, will add more message. The next value of the
> ITEM_PPPOE_PROTO_ID
> > > should be unsigned value but not item list.
> > >
> > > >
> > > > Also please see inline other comment.
> > > >
> > > > Best,
> > > > Ori
> > > >
> > > > > -----Original Message-----
> > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > To: dev@dpdk.org
> > > > > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > > > > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > >
> > > > > The command line to create RTE flow for specific proto_id of
> > > > > PPPOES is not correct. This patch is to fix this issue.
> > > > >
> > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > ---
> > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > b/app/test-pmd/cmdline_flow.c index a78154502..c25a2598d 100644
> > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> > > > >  	ITEM_GTP_PSC,
> > > > >  	ITEM_PPPOES,
> > > > >  	ITEM_PPPOED,
> > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > >  	ITEM_HIGIG2,
> > > > >  	ITEM_TAG,
> > > > >  	ITEM_L2TPV3OIP,
> > > > > @@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] = {
> > > > >
> > > > >  static const enum index item_pppoes[] = {
> > > > >  	ITEM_PPPOE_SEID,
> > > > > -	ITEM_NEXT,
> > > > > -	ZERO,
> > > > > -};
> > > > > -
> > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > >  	ITEM_NEXT,
> > > > >  	ZERO,
> > > > > @@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
> > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > >  		.name = "proto_id",
> > > > >  		.help = "match PPPoE session protocol identifier",
> > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > -				sizeof(struct
> rte_flow_item_pppoe_proto_id)),
> > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > -		.call = parse_vc,
> > > > > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > > > > item_param),
> > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> proto_id)),
> > > >
> > > > Where is the memory for this proto_id is defined?
> > >
> > > Do you mean this?
> > > lib/librte_ethdev/rte_flow.h
> > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > > 1362 };
> > >
> >
> > Yes. Why don't you use this one?
> 
> I think I was using this, am I using it incorrectly?
> 
> +		.args = ARGS(ARGS_ENTRY_HTON
> +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
> 

Yes but there is no space to save this data since you deleted the priv.
I think you are trying to implement something like RTE_FLOW_ITEM_TYPE_IPV6_EXT.

And I don't understand what was the problem with the previous implementation.

> >
> > > >
> > > > >  	},
> > > > >  	[ITEM_HIGIG2] = {
> > > > >  		.name = "higig2",
> > > > > --
> > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Zhang, Xiao]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Sunday, March 29, 2020 8:46 PM
> To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Xiao,
> 
> > -----Original Message-----
> > From: Zhang, Xiao <xiao.zhang@intel.com>
> > Sent: Sunday, March 29, 2020 3:00 PM
> > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > stable@dpdk.org
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> > Hi Ori,
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@mellanox.com>
> > > Sent: Sunday, March 29, 2020 6:19 PM
> > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > stable@dpdk.org
> > > Subject: RE: app/testpmd: fix PPPOES flow API
> > >
> > > Hi Xiao,
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > stable@dpdk.org
> > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > >
> > > > Hi Ori,
> > > >
> > > > > -----Original Message-----
> > > > > From: Ori Kam <orika@mellanox.com>
> > > > > Sent: Sunday, March 29, 2020 2:28 PM
> > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > stable@dpdk.org
> > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > >
> > > > > Hi Xiao,
> > > > >
> > > > > Is the proto_id part of the basic header or not?
> > > >
> > > > Proto_id is part of PPPOE session header,
> > > >
> > >
> > > Where is the porto_id located? Inside the payload?
> >
> > Yes, my previous explanation was not clear. The protocol ID is in the
> > beginning of the payload in PPP Session Stage according to RFC2516.
> >
> >                      	         1                                  2                                3
> >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3  4  5 6 7 8 9
> > 0 1  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >  |  VER   | TYPE   |      CODE      |                  SESSION_ID                    |
> >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >  |                 LENGTH                    |                      payload                      ~
> >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >
> 
> Yes this is what I thought, does proto_id must be the first part of the payload?

It must be the first part of the payload for PPP Session Stage, not all PPPOE packets.

> 
> > >
> > > > >
> > > > > From the spec it looks like a different header.
> > > > >
> > > > > If it is part of the original header then all documentations and
> > > > > rte_structs
> > > > should
> > > > > be changed, to reflect this.
> > > > >
> > > > > It will be very helpful if the patch message would explain the
> > > > > bug and why it
> > > > was
> > > > > changed.
> > > >
> > > > Okay, will add more message. The next value of the
> > ITEM_PPPOE_PROTO_ID
> > > > should be unsigned value but not item list.
> > > >
> > > > >
> > > > > Also please see inline other comment.
> > > > >
> > > > > Best,
> > > > > Ori
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > To: dev@dpdk.org
> > > > > > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > > > > > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > >
> > > > > > The command line to create RTE flow for specific proto_id of
> > > > > > PPPOES is not correct. This patch is to fix this issue.
> > > > > >
> > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > > > > Cc: stable@dpdk.org
> > > > > >
> > > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > ---
> > > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > b/app/test-pmd/cmdline_flow.c index a78154502..c25a2598d
> > > > > > 100644
> > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> > > > > >  	ITEM_GTP_PSC,
> > > > > >  	ITEM_PPPOES,
> > > > > >  	ITEM_PPPOED,
> > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > >  	ITEM_HIGIG2,
> > > > > >  	ITEM_TAG,
> > > > > >  	ITEM_L2TPV3OIP,
> > > > > > @@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] =
> > > > > > {
> > > > > >
> > > > > >  static const enum index item_pppoes[] = {
> > > > > >  	ITEM_PPPOE_SEID,
> > > > > > -	ITEM_NEXT,
> > > > > > -	ZERO,
> > > > > > -};
> > > > > > -
> > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > >  	ITEM_NEXT,
> > > > > >  	ZERO,
> > > > > > @@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
> > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > >  		.name = "proto_id",
> > > > > >  		.help = "match PPPoE session protocol identifier",
> > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > -				sizeof(struct
> > rte_flow_item_pppoe_proto_id)),
> > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > -		.call = parse_vc,
> > > > > > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > > > > > item_param),
> > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> > proto_id)),
> > > > >
> > > > > Where is the memory for this proto_id is defined?
> > > >
> > > > Do you mean this?
> > > > lib/librte_ethdev/rte_flow.h
> > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > > > 1362 };
> > > >
> > >
> > > Yes. Why don't you use this one?
> >
> > I think I was using this, am I using it incorrectly?
> >
> > +		.args = ARGS(ARGS_ENTRY_HTON
> > +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
> >
> 
> Yes but there is no space to save this data since you deleted the priv.
> I think you are trying to implement something like
> RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> 
> And I don't understand what was the problem with the previous implementation.

I deleted the priv because it changed to a subcommand in pppoes, the command line will be like this:
testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / pppoes proto_id is 21

The previous implementation would be infinite loop for proto_id command and can not specific the value for proto_id.
testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / proto_id
 proto_id [TOKEN]: match PPPoE session protocol identifier
 / [TOKEN]: specify next pattern item
testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / proto_id proto_id
 proto_id [TOKEN]: match PPPoE session protocol identifier
 / [TOKEN]: specify next pattern item
testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / proto_id proto_id proto_id
 proto_id [TOKEN]: match PPPoE session protocol identifier
 / [TOKEN]: specify next pattern item
testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / proto_id proto_id proto_id

> 
> > >
> > > > >
> > > > > >  	},
> > > > > >  	[ITEM_HIGIG2] = {
> > > > > >  		.name = "higig2",
> > > > > > --
> > > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Ori Kam]

Hi Xiao

> -----Original Message-----
> From: Zhang, Xiao <xiao.zhang@intel.com>
> Sent: Monday, March 30, 2020 5:09 AM
> To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Ori,
> 
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> > Sent: Sunday, March 29, 2020 8:46 PM
> > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> stable@dpdk.org
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> > Hi Xiao,
> >
> > > -----Original Message-----
> > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > Sent: Sunday, March 29, 2020 3:00 PM
> > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > stable@dpdk.org
> > > Subject: RE: app/testpmd: fix PPPOES flow API
> > >
> > > Hi Ori,
> > >
> > > > -----Original Message-----
> > > > From: Ori Kam <orika@mellanox.com>
> > > > Sent: Sunday, March 29, 2020 6:19 PM
> > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > stable@dpdk.org
> > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > >
> > > > Hi Xiao,
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > stable@dpdk.org
> > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > >
> > > > > Hi Ori,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > Sent: Sunday, March 29, 2020 2:28 PM
> > > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > stable@dpdk.org
> > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > >
> > > > > > Hi Xiao,
> > > > > >
> > > > > > Is the proto_id part of the basic header or not?
> > > > >
> > > > > Proto_id is part of PPPOE session header,
> > > > >
> > > >
> > > > Where is the porto_id located? Inside the payload?
> > >
> > > Yes, my previous explanation was not clear. The protocol ID is in the
> > > beginning of the payload in PPP Session Stage according to RFC2516.
> > >
> > >                      	         1                                  2                                3
> > >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3  4  5 6 7 8 9
> > > 0 1  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > >  |  VER   | TYPE   |      CODE      |                  SESSION_ID                    |
> > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > >  |                 LENGTH                    |                      payload                      ~
> > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > >
> >
> > Yes this is what I thought, does proto_id must be the first part of the payload?
> 
> It must be the first part of the payload for PPP Session Stage, not all PPPOE
> packets.
> 
> >
> > > >
> > > > > >
> > > > > > From the spec it looks like a different header.
> > > > > >
> > > > > > If it is part of the original header then all documentations and
> > > > > > rte_structs
> > > > > should
> > > > > > be changed, to reflect this.
> > > > > >
> > > > > > It will be very helpful if the patch message would explain the
> > > > > > bug and why it
> > > > > was
> > > > > > changed.
> > > > >
> > > > > Okay, will add more message. The next value of the
> > > ITEM_PPPOE_PROTO_ID
> > > > > should be unsigned value but not item list.
> > > > >
> > > > > >
> > > > > > Also please see inline other comment.
> > > > > >
> > > > > > Best,
> > > > > > Ori
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > > To: dev@dpdk.org
> > > > > > > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > > > > > > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > > >
> > > > > > > The command line to create RTE flow for specific proto_id of
> > > > > > > PPPOES is not correct. This patch is to fix this issue.
> > > > > > >
> > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > > > > > Cc: stable@dpdk.org
> > > > > > >
> > > > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > ---
> > > > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > > >
> > > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > > b/app/test-pmd/cmdline_flow.c index a78154502..c25a2598d
> > > > > > > 100644
> > > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> > > > > > >  	ITEM_GTP_PSC,
> > > > > > >  	ITEM_PPPOES,
> > > > > > >  	ITEM_PPPOED,
> > > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > > >  	ITEM_HIGIG2,
> > > > > > >  	ITEM_TAG,
> > > > > > >  	ITEM_L2TPV3OIP,
> > > > > > > @@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] =
> > > > > > > {
> > > > > > >
> > > > > > >  static const enum index item_pppoes[] = {
> > > > > > >  	ITEM_PPPOE_SEID,
> > > > > > > -	ITEM_NEXT,
> > > > > > > -	ZERO,
> > > > > > > -};
> > > > > > > -
> > > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > > >  	ITEM_NEXT,
> > > > > > >  	ZERO,
> > > > > > > @@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
> > > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > > >  		.name = "proto_id",
> > > > > > >  		.help = "match PPPoE session protocol identifier",
> > > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > > -				sizeof(struct
> > > rte_flow_item_pppoe_proto_id)),
> > > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > > -		.call = parse_vc,
> > > > > > > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > > > > > > item_param),
> > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> > > proto_id)),
> > > > > >
> > > > > > Where is the memory for this proto_id is defined?
> > > > >
> > > > > Do you mean this?
> > > > > lib/librte_ethdev/rte_flow.h
> > > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > > > > 1362 };
> > > > >
> > > >
> > > > Yes. Why don't you use this one?
> > >
> > > I think I was using this, am I using it incorrectly?
> > >
> > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
> > >
> >
> > Yes but there is no space to save this data since you deleted the priv.
> > I think you are trying to implement something like
> > RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> >
> > And I don't understand what was the problem with the previous
> implementation.
> 
> I deleted the priv because it changed to a subcommand in pppoes, the
> command line will be like this:
> testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / pppoes
> proto_id is 21
> 

The issue is that the pppoe struct doesn't have definition to the proto_id.
If you wish a possible solution will be to add it to the pppoe struct,
I'm not sure if this is the correct approach since this field is not a must.

Like I said there are examples on how to work with extended headers, which
I think are more correct, buy may be the problem is that the pppoe struct is not aligned
and this result in an issue when adding the last bytes.

> The previous implementation would be infinite loop for proto_id command and
> can not specific the value for proto_id.
> testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / proto_id
>  proto_id [TOKEN]: match PPPoE session protocol identifier
>  / [TOKEN]: specify next pattern item
> testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / proto_id
> proto_id
>  proto_id [TOKEN]: match PPPoE session protocol identifier
>  / [TOKEN]: specify next pattern item
> testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / proto_id
> proto_id proto_id
>  proto_id [TOKEN]: match PPPoE session protocol identifier
>  / [TOKEN]: specify next pattern item
> testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / proto_id
> proto_id proto_id
> 
> >
> > > >
> > > > > >
> > > > > > >  	},
> > > > > > >  	[ITEM_HIGIG2] = {
> > > > > > >  		.name = "higig2",
> > > > > > > --
> > > > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Zhao1, Wei]

Hi, Ori

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Monday, March 30, 2020 3:43 PM
> To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Xiao
> 
> > -----Original Message-----
> > From: Zhang, Xiao <xiao.zhang@intel.com>
> > Sent: Monday, March 30, 2020 5:09 AM
> > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > stable@dpdk.org
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> > Hi Ori,
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@mellanox.com>
> > > Sent: Sunday, March 29, 2020 8:46 PM
> > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > stable@dpdk.org
> > > Subject: RE: app/testpmd: fix PPPOES flow API
> > >
> > > Hi Xiao,
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > Sent: Sunday, March 29, 2020 3:00 PM
> > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > stable@dpdk.org
> > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > >
> > > > Hi Ori,
> > > >
> > > > > -----Original Message-----
> > > > > From: Ori Kam <orika@mellanox.com>
> > > > > Sent: Sunday, March 29, 2020 6:19 PM
> > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > stable@dpdk.org
> > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > >
> > > > > Hi Xiao,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > > stable@dpdk.org
> > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > >
> > > > > > Hi Ori,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > Sent: Sunday, March 29, 2020 2:28 PM
> > > > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > > stable@dpdk.org
> > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > >
> > > > > > > Hi Xiao,
> > > > > > >
> > > > > > > Is the proto_id part of the basic header or not?
> > > > > >
> > > > > > Proto_id is part of PPPOE session header,
> > > > > >
> > > > >
> > > > > Where is the porto_id located? Inside the payload?
> > > >
> > > > Yes, my previous explanation was not clear. The protocol ID is in
> > > > the beginning of the payload in PPP Session Stage according to RFC2516.
> > > >
> > > >                      	         1
> 2                                3
> > > >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3  4  5 6 7
> > > > 8 9
> > > > 0 1  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > >  |  VER   | TYPE   |      CODE      |
> SESSION_ID                    |
> > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > >  |                 LENGTH                    |
> payload                      ~
> > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > >
> > >
> > > Yes this is what I thought, does proto_id must be the first part of the
> payload?
> >
> > It must be the first part of the payload for PPP Session Stage, not
> > all PPPOE packets.
> >
> > >
> > > > >
> > > > > > >
> > > > > > > From the spec it looks like a different header.
> > > > > > >
> > > > > > > If it is part of the original header then all documentations
> > > > > > > and rte_structs
> > > > > > should
> > > > > > > be changed, to reflect this.
> > > > > > >
> > > > > > > It will be very helpful if the patch message would explain
> > > > > > > the bug and why it
> > > > > > was
> > > > > > > changed.
> > > > > >
> > > > > > Okay, will add more message. The next value of the
> > > > ITEM_PPPOE_PROTO_ID
> > > > > > should be unsigned value but not item list.
> > > > > >
> > > > > > >
> > > > > > > Also please see inline other comment.
> > > > > > >
> > > > > > > Best,
> > > > > > > Ori
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > > > To: dev@dpdk.org
> > > > > > > > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > > > > > > > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > > > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > > > >
> > > > > > > > The command line to create RTE flow for specific proto_id
> > > > > > > > of PPPOES is not correct. This patch is to fix this issue.
> > > > > > > >
> > > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > > > > > > Cc: stable@dpdk.org
> > > > > > > >
> > > > > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > ---
> > > > > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > > > b/app/test-pmd/cmdline_flow.c index a78154502..c25a2598d
> > > > > > > > 100644
> > > > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > > > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> > > > > > > >  	ITEM_GTP_PSC,
> > > > > > > >  	ITEM_PPPOES,
> > > > > > > >  	ITEM_PPPOED,
> > > > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > > > >  	ITEM_HIGIG2,
> > > > > > > >  	ITEM_TAG,
> > > > > > > >  	ITEM_L2TPV3OIP,
> > > > > > > > @@ -1030,11 +1029,6 @@ static const enum index
> > > > > > > > item_pppoed[] = {
> > > > > > > >
> > > > > > > >  static const enum index item_pppoes[] = {
> > > > > > > >  	ITEM_PPPOE_SEID,
> > > > > > > > -	ITEM_NEXT,
> > > > > > > > -	ZERO,
> > > > > > > > -};
> > > > > > > > -
> > > > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > > > >  	ITEM_NEXT,
> > > > > > > >  	ZERO,
> > > > > > > > @@ -2643,10 +2637,9 @@ static const struct token token_list[] =
> {
> > > > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > > > >  		.name = "proto_id",
> > > > > > > >  		.help = "match PPPoE session protocol identifier",
> > > > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > > > -				sizeof(struct
> > > > rte_flow_item_pppoe_proto_id)),
> > > > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > > > -		.call = parse_vc,
> > > > > > > > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > > > > > > > item_param),
> > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> > > > proto_id)),
> > > > > > >
> > > > > > > Where is the memory for this proto_id is defined?
> > > > > >
> > > > > > Do you mean this?
> > > > > > lib/librte_ethdev/rte_flow.h
> > > > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > > > > > 1362 };
> > > > > >
> > > > >
> > > > > Yes. Why don't you use this one?
> > > >
> > > > I think I was using this, am I using it incorrectly?
> > > >
> > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
> > > >
> > >
> > > Yes but there is no space to save this data since you deleted the priv.
> > > I think you are trying to implement something like
> > > RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> > >
> > > And I don't understand what was the problem with the previous
> > implementation.
> >
> > I deleted the priv because it changed to a subcommand in pppoes, the
> > command line will be like this:
> > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > testpmd> pppoes
> > proto_id is 21
> >
> 
> The issue is that the pppoe struct doesn't have definition to the proto_id.
> If you wish a possible solution will be to add it to the pppoe struct, I'm not sure
> if this is the correct approach since this field is not a must.
> 
> Like I said there are examples on how to work with extended headers, which I
> think are more correct, buy may be the problem is that the pppoe struct is not
> aligned and this result in an issue when adding the last bytes.


There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do you mean make use of that?
That is the reason for use extended header for this?
But that seems as you say, has some bug.


> 
> > The previous implementation would be infinite loop for proto_id
> > command and can not specific the value for proto_id.
> > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > testpmd> proto_id
> >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > specify next pattern item
> > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > testpmd> proto_id
> > proto_id
> >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > specify next pattern item
> > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > testpmd> proto_id
> > proto_id proto_id
> >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > specify next pattern item
> > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > testpmd> proto_id
> > proto_id proto_id
> >
> > >
> > > > >
> > > > > > >
> > > > > > > >  	},
> > > > > > > >  	[ITEM_HIGIG2] = {
> > > > > > > >  		.name = "higig2",
> > > > > > > > --
> > > > > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Ori Kam]

Hi Xiao,

> -----Original Message-----
> From: Zhao1, Wei <wei.zhao1@intel.com>
> Sent: Monday, March 30, 2020 11:50 AM
> To: Ori Kam <orika@mellanox.com>; Zhang, Xiao <xiao.zhang@intel.com>;
> dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi, Ori
> 
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> > Sent: Monday, March 30, 2020 3:43 PM
> > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > stable@dpdk.org
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> > Hi Xiao
> >
> > > -----Original Message-----
> > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > Sent: Monday, March 30, 2020 5:09 AM
> > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > stable@dpdk.org
> > > Subject: RE: app/testpmd: fix PPPOES flow API
> > >
> > > Hi Ori,
> > >
> > > > -----Original Message-----
> > > > From: Ori Kam <orika@mellanox.com>
> > > > Sent: Sunday, March 29, 2020 8:46 PM
> > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > stable@dpdk.org
> > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > >
> > > > Hi Xiao,
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > Sent: Sunday, March 29, 2020 3:00 PM
> > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > stable@dpdk.org
> > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > >
> > > > > Hi Ori,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > Sent: Sunday, March 29, 2020 6:19 PM
> > > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > stable@dpdk.org
> > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > >
> > > > > > Hi Xiao,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > > > stable@dpdk.org
> > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > >
> > > > > > > Hi Ori,
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > Sent: Sunday, March 29, 2020 2:28 PM
> > > > > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > > > stable@dpdk.org
> > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > > >
> > > > > > > > Hi Xiao,
> > > > > > > >
> > > > > > > > Is the proto_id part of the basic header or not?
> > > > > > >
> > > > > > > Proto_id is part of PPPOE session header,
> > > > > > >
> > > > > >
> > > > > > Where is the porto_id located? Inside the payload?
> > > > >
> > > > > Yes, my previous explanation was not clear. The protocol ID is in
> > > > > the beginning of the payload in PPP Session Stage according to RFC2516.
> > > > >
> > > > >                      	         1
> > 2                                3
> > > > >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3  4  5 6 7
> > > > > 8 9
> > > > > 0 1  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > >  |  VER   | TYPE   |      CODE      |
> > SESSION_ID                    |
> > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > >  |                 LENGTH                    |
> > payload                      ~
> > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > >
> > > >
> > > > Yes this is what I thought, does proto_id must be the first part of the
> > payload?
> > >
> > > It must be the first part of the payload for PPP Session Stage, not
> > > all PPPOE packets.
> > >
> > > >
> > > > > >
> > > > > > > >
> > > > > > > > From the spec it looks like a different header.
> > > > > > > >
> > > > > > > > If it is part of the original header then all documentations
> > > > > > > > and rte_structs
> > > > > > > should
> > > > > > > > be changed, to reflect this.
> > > > > > > >
> > > > > > > > It will be very helpful if the patch message would explain
> > > > > > > > the bug and why it
> > > > > > > was
> > > > > > > > changed.
> > > > > > >
> > > > > > > Okay, will add more message. The next value of the
> > > > > ITEM_PPPOE_PROTO_ID
> > > > > > > should be unsigned value but not item list.
> > > > > > >
> > > > > > > >
> > > > > > > > Also please see inline other comment.
> > > > > > > >
> > > > > > > > Best,
> > > > > > > > Ori
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > > > > To: dev@dpdk.org
> > > > > > > > > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > > > > > > > > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > > > > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > > > > >
> > > > > > > > > The command line to create RTE flow for specific proto_id
> > > > > > > > > of PPPOES is not correct. This patch is to fix this issue.
> > > > > > > > >
> > > > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > > > > > > > Cc: stable@dpdk.org
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > ---
> > > > > > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > > > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > > > > b/app/test-pmd/cmdline_flow.c index a78154502..c25a2598d
> > > > > > > > > 100644
> > > > > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > > > > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> > > > > > > > >  	ITEM_GTP_PSC,
> > > > > > > > >  	ITEM_PPPOES,
> > > > > > > > >  	ITEM_PPPOED,
> > > > > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > > > > >  	ITEM_HIGIG2,
> > > > > > > > >  	ITEM_TAG,
> > > > > > > > >  	ITEM_L2TPV3OIP,
> > > > > > > > > @@ -1030,11 +1029,6 @@ static const enum index
> > > > > > > > > item_pppoed[] = {
> > > > > > > > >
> > > > > > > > >  static const enum index item_pppoes[] = {
> > > > > > > > >  	ITEM_PPPOE_SEID,
> > > > > > > > > -	ITEM_NEXT,
> > > > > > > > > -	ZERO,
> > > > > > > > > -};
> > > > > > > > > -
> > > > > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > > > > >  	ITEM_NEXT,
> > > > > > > > >  	ZERO,
> > > > > > > > > @@ -2643,10 +2637,9 @@ static const struct token token_list[]
> =
> > {
> > > > > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > > > > >  		.name = "proto_id",
> > > > > > > > >  		.help = "match PPPoE session protocol identifier",
> > > > > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > > > > -				sizeof(struct
> > > > > rte_flow_item_pppoe_proto_id)),
> > > > > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > > > > -		.call = parse_vc,
> > > > > > > > > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > > > > > > > > item_param),
> > > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> > > > > proto_id)),
> > > > > > > >
> > > > > > > > Where is the memory for this proto_id is defined?
> > > > > > >
> > > > > > > Do you mean this?
> > > > > > > lib/librte_ethdev/rte_flow.h
> > > > > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > > > > > > 1362 };
> > > > > > >
> > > > > >
> > > > > > Yes. Why don't you use this one?
> > > > >
> > > > > I think I was using this, am I using it incorrectly?
> > > > >
> > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> proto_id)),
> > > > >
> > > >
> > > > Yes but there is no space to save this data since you deleted the priv.
> > > > I think you are trying to implement something like
> > > > RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> > > >
> > > > And I don't understand what was the problem with the previous
> > > implementation.
> > >
> > > I deleted the priv because it changed to a subcommand in pppoes, the
> > > command line will be like this:
> > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > > testpmd> pppoes
> > > proto_id is 21
> > >
> >
> > The issue is that the pppoe struct doesn't have definition to the proto_id.
> > If you wish a possible solution will be to add it to the pppoe struct, I'm not
> sure
> > if this is the correct approach since this field is not a must.
> >
> > Like I said there are examples on how to work with extended headers, which I
> > think are more correct, buy may be the problem is that the pppoe struct is
> not
> > aligned and this result in an issue when adding the last bytes.
> 
> 
> There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do you
> mean make use of that?
> That is the reason for use extended header for this?
> But that seems as you say, has some bug.
> 

I understand there is a bug, the question how to solve it.
I suggested two approaches. Add the proto_id to the pppoe struct, but this
means that we will add a new member that is not part of the original definition.
Maybe the issue is in the PMD and it needs to understand that the
proto_id should be located in a different offset.
In any case it doesn't look like the current fix the right one.

> 
> >
> > > The previous implementation would be infinite loop for proto_id
> > > command and can not specific the value for proto_id.
> > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > > testpmd> proto_id
> > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > specify next pattern item
> > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > > testpmd> proto_id
> > > proto_id
> > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > specify next pattern item
> > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > > testpmd> proto_id
> > > proto_id proto_id
> > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > specify next pattern item
> > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 /
> > > testpmd> proto_id
> > > proto_id proto_id
> > >
> > > >
> > > > > >
> > > > > > > >
> > > > > > > > >  	},
> > > > > > > > >  	[ITEM_HIGIG2] = {
> > > > > > > > >  		.name = "higig2",
> > > > > > > > > --
> > > > > > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Zhang, Xiao]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Monday, March 30, 2020 4:57 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Zhang, Xiao <xiao.zhang@intel.com>;
> dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Xiao,
> 
> > -----Original Message-----
> > From: Zhao1, Wei <wei.zhao1@intel.com>
> > Sent: Monday, March 30, 2020 11:50 AM
> > To: Ori Kam <orika@mellanox.com>; Zhang, Xiao <xiao.zhang@intel.com>;
> > dev@dpdk.org
> > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; stable@dpdk.org
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> > Hi, Ori
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@mellanox.com>
> > > Sent: Monday, March 30, 2020 3:43 PM
> > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > stable@dpdk.org
> > > Subject: RE: app/testpmd: fix PPPOES flow API
> > >
> > > Hi Xiao
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > Sent: Monday, March 30, 2020 5:09 AM
> > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > stable@dpdk.org
> > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > >
> > > > Hi Ori,
> > > >
> > > > > -----Original Message-----
> > > > > From: Ori Kam <orika@mellanox.com>
> > > > > Sent: Sunday, March 29, 2020 8:46 PM
> > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > stable@dpdk.org
> > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > >
> > > > > Hi Xiao,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > Sent: Sunday, March 29, 2020 3:00 PM
> > > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > > stable@dpdk.org
> > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > >
> > > > > > Hi Ori,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > Sent: Sunday, March 29, 2020 6:19 PM
> > > > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > > stable@dpdk.org
> > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > >
> > > > > > > Hi Xiao,
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > > > > stable@dpdk.org
> > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > > >
> > > > > > > > Hi Ori,
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > > Sent: Sunday, March 29, 2020 2:28 PM
> > > > > > > > > To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> > > > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei
> > > > > > > > > <wei.zhao1@intel.com>;
> > > > > > > > stable@dpdk.org
> > > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > > > >
> > > > > > > > > Hi Xiao,
> > > > > > > > >
> > > > > > > > > Is the proto_id part of the basic header or not?
> > > > > > > >
> > > > > > > > Proto_id is part of PPPOE session header,
> > > > > > > >
> > > > > > >
> > > > > > > Where is the porto_id located? Inside the payload?
> > > > > >
> > > > > > Yes, my previous explanation was not clear. The protocol ID is
> > > > > > in the beginning of the payload in PPP Session Stage according to
> RFC2516.
> > > > > >
> > > > > >                      	         1
> > > 2                                3
> > > > > >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3  4  5
> > > > > > 6 7
> > > > > > 8 9
> > > > > > 0 1  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > > >  |  VER   | TYPE   |      CODE      |
> > > SESSION_ID                    |
> > > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > > >  |                 LENGTH                    |
> > > payload                      ~
> > > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > > >
> > > > >
> > > > > Yes this is what I thought, does proto_id must be the first part
> > > > > of the
> > > payload?
> > > >
> > > > It must be the first part of the payload for PPP Session Stage,
> > > > not all PPPOE packets.
> > > >
> > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > From the spec it looks like a different header.
> > > > > > > > >
> > > > > > > > > If it is part of the original header then all
> > > > > > > > > documentations and rte_structs
> > > > > > > > should
> > > > > > > > > be changed, to reflect this.
> > > > > > > > >
> > > > > > > > > It will be very helpful if the patch message would
> > > > > > > > > explain the bug and why it
> > > > > > > > was
> > > > > > > > > changed.
> > > > > > > >
> > > > > > > > Okay, will add more message. The next value of the
> > > > > > ITEM_PPPOE_PROTO_ID
> > > > > > > > should be unsigned value but not item list.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Also please see inline other comment.
> > > > > > > > >
> > > > > > > > > Best,
> > > > > > > > > Ori
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > > > > > To: dev@dpdk.org
> > > > > > > > > > Cc: Ori Kam <orika@mellanox.com>;
> > > > > > > > > > ying.a.wang@intel.com; qi.z.zhang@intel.com;
> > > > > > > > > > wei.zhao1@intel.com; Xiao Zhang
> > > > > > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > > > > > >
> > > > > > > > > > The command line to create RTE flow for specific
> > > > > > > > > > proto_id of PPPOES is not correct. This patch is to fix this issue.
> > > > > > > > > >
> > > > > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > > > > > > > > Cc: stable@dpdk.org
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > ---
> > > > > > > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > > > > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > b/app/test-pmd/cmdline_flow.c index
> > > > > > > > > > a78154502..c25a2598d
> > > > > > > > > > 100644
> > > > > > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > > > > > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> > > > > > > > > >  	ITEM_GTP_PSC,
> > > > > > > > > >  	ITEM_PPPOES,
> > > > > > > > > >  	ITEM_PPPOED,
> > > > > > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > >  	ITEM_HIGIG2,
> > > > > > > > > >  	ITEM_TAG,
> > > > > > > > > >  	ITEM_L2TPV3OIP,
> > > > > > > > > > @@ -1030,11 +1029,6 @@ static const enum index
> > > > > > > > > > item_pppoed[] = {
> > > > > > > > > >
> > > > > > > > > >  static const enum index item_pppoes[] = {
> > > > > > > > > >  	ITEM_PPPOE_SEID,
> > > > > > > > > > -	ITEM_NEXT,
> > > > > > > > > > -	ZERO,
> > > > > > > > > > -};
> > > > > > > > > > -
> > > > > > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > >  	ITEM_NEXT,
> > > > > > > > > >  	ZERO,
> > > > > > > > > > @@ -2643,10 +2637,9 @@ static const struct token
> > > > > > > > > > token_list[]
> > =
> > > {
> > > > > > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > > > > > >  		.name = "proto_id",
> > > > > > > > > >  		.help = "match PPPoE session protocol
> identifier",
> > > > > > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > > > > > -				sizeof(struct
> > > > > > rte_flow_item_pppoe_proto_id)),
> > > > > > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > > > > > -		.call = parse_vc,
> > > > > > > > > > +		.next = NEXT(item_pppoes,
> NEXT_ENTRY(UNSIGNED),
> > > > > > > > > > item_param),
> > > > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > > > +			     (struct
> rte_flow_item_pppoe_proto_id,
> > > > > > proto_id)),
> > > > > > > > >
> > > > > > > > > Where is the memory for this proto_id is defined?
> > > > > > > >
> > > > > > > > Do you mean this?
> > > > > > > > lib/librte_ethdev/rte_flow.h
> > > > > > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > > > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > > > > > > > 1362 };
> > > > > > > >
> > > > > > >
> > > > > > > Yes. Why don't you use this one?
> > > > > >
> > > > > > I think I was using this, am I using it incorrectly?
> > > > > >
> > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> > proto_id)),
> > > > > >
> > > > >
> > > > > Yes but there is no space to save this data since you deleted the priv.
> > > > > I think you are trying to implement something like
> > > > > RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> > > > >
> > > > > And I don't understand what was the problem with the previous
> > > > implementation.
> > > >
> > > > I deleted the priv because it changed to a subcommand in pppoes,
> > > > the command line will be like this:
> > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > testpmd> 00:11:22:33:44:55 / pppoes
> > > > proto_id is 21
> > > >
> > >
> > > The issue is that the pppoe struct doesn't have definition to the proto_id.
> > > If you wish a possible solution will be to add it to the pppoe
> > > struct, I'm not
> > sure
> > > if this is the correct approach since this field is not a must.
> > >
> > > Like I said there are examples on how to work with extended headers,
> > > which I think are more correct, buy may be the problem is that the
> > > pppoe struct is
> > not
> > > aligned and this result in an issue when adding the last bytes.
> >
> >
> > There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do you
> > mean make use of that?
> > That is the reason for use extended header for this?
> > But that seems as you say, has some bug.
> >
> 
> I understand there is a bug, the question how to solve it.
> I suggested two approaches. Add the proto_id to the pppoe struct, but this
> means that we will add a new member that is not part of the original definition.
> Maybe the issue is in the PMD and it needs to understand that the proto_id
> should be located in a different offset.
> In any case it doesn't look like the current fix the right one.

From my understanding, you mean there are two approaches. One is adding proto_id to pppoe struct. But you don't prefer this one since proto_id is not a must. I am not clear about the other one.

And also how do you suggest the command line be for proto_id? 
"proto_id is 0x0021" or "pppoes proto_id is 0x0021"? If the former just like what it was, I think it maybe a little confused. If the latter (as proto_id is part of pppoes), do we still need to put proto_id in rte_flow_item_pppoe?

Thanks,
Xiao

> 
> >
> > >
> > > > The previous implementation would be infinite loop for proto_id
> > > > command and can not specific the value for proto_id.
> > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > specify next pattern item
> > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > proto_id
> > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > specify next pattern item
> > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > proto_id proto_id
> > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > specify next pattern item
> > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > proto_id proto_id
> > > >
> > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > >  	},
> > > > > > > > > >  	[ITEM_HIGIG2] = {
> > > > > > > > > >  		.name = "higig2",
> > > > > > > > > > --
> > > > > > > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Ori Kam]

> -----Original Message-----
> From: Zhang, Xiao <xiao.zhang@intel.com>
> 
> Hi Ori,
> 
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> >
> > Hi Xiao,
> >
> > > -----Original Message-----
> > > From: Zhao1, Wei <wei.zhao1@intel.com>
> > >
> > > Hi, Ori
> > >
> > > > -----Original Message-----
> > > > From: Ori Kam <orika@mellanox.com>
> > > >
> > > > Hi Xiao
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > >
> > > > > Hi Ori,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > >
> > > > > > Hi Xiao,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > >
> > > > > > > Hi Ori,
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > >
> > > > > > > > Hi Xiao,
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > > > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> > > > > > > > > stable@dpdk.org
> > > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > > > >
> > > > > > > > > Hi Ori,
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > > >
> > > > > > > > > > Hi Xiao,
> > > > > > > > > >
> > > > > > > > > > Is the proto_id part of the basic header or not?
> > > > > > > > >
> > > > > > > > > Proto_id is part of PPPOE session header,
> > > > > > > > >
> > > > > > > >
> > > > > > > > Where is the porto_id located? Inside the payload?
> > > > > > >
> > > > > > > Yes, my previous explanation was not clear. The protocol ID is
> > > > > > > in the beginning of the payload in PPP Session Stage according to
> > RFC2516.
> > > > > > >
> > > > > > >                      	         1
> > > > 2                                3
> > > > > > >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3  4  5
> > > > > > > 6 7
> > > > > > > 8 9
> > > > > > > 0 1  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> +
> > > > > > >  |  VER   | TYPE   |      CODE      |
> > > > SESSION_ID                    |
> > > > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > > > >  |                 LENGTH                    |
> > > > payload                      ~
> > > > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > > > >
> > > > > >
> > > > > > Yes this is what I thought, does proto_id must be the first part
> > > > > > of the
> > > > payload?
> > > > >
> > > > > It must be the first part of the payload for PPP Session Stage,
> > > > > not all PPPOE packets.
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > From the spec it looks like a different header.
> > > > > > > > > >
> > > > > > > > > > If it is part of the original header then all
> > > > > > > > > > documentations and rte_structs
> > > > > > > > > should
> > > > > > > > > > be changed, to reflect this.
> > > > > > > > > >
> > > > > > > > > > It will be very helpful if the patch message would
> > > > > > > > > > explain the bug and why it
> > > > > > > > > was
> > > > > > > > > > changed.
> > > > > > > > >
> > > > > > > > > Okay, will add more message. The next value of the
> > > > > > > ITEM_PPPOE_PROTO_ID
> > > > > > > > > should be unsigned value but not item list.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Also please see inline other comment.
> > > > > > > > > >
> > > > > > > > > > Best,
> > > > > > > > > > Ori
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > > > > > > To: dev@dpdk.org
> > > > > > > > > > > Cc: Ori Kam <orika@mellanox.com>;
> > > > > > > > > > > ying.a.wang@intel.com; qi.z.zhang@intel.com;
> > > > > > > > > > > wei.zhao1@intel.com; Xiao Zhang
> > > > > > > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > > > > > > >
> > > > > > > > > > > The command line to create RTE flow for specific
> > > > > > > > > > > proto_id of PPPOES is not correct. This patch is to fix this
> issue.
> > > > > > > > > > >
> > > > > > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > > > > > > > > > > Cc: stable@dpdk.org
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > > > > > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > b/app/test-pmd/cmdline_flow.c index
> > > > > > > > > > > a78154502..c25a2598d
> > > > > > > > > > > 100644
> > > > > > > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > @@ -768,7 +768,6 @@ static const enum index next_item[]
> = {
> > > > > > > > > > >  	ITEM_GTP_PSC,
> > > > > > > > > > >  	ITEM_PPPOES,
> > > > > > > > > > >  	ITEM_PPPOED,
> > > > > > > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > > >  	ITEM_HIGIG2,
> > > > > > > > > > >  	ITEM_TAG,
> > > > > > > > > > >  	ITEM_L2TPV3OIP,
> > > > > > > > > > > @@ -1030,11 +1029,6 @@ static const enum index
> > > > > > > > > > > item_pppoed[] = {
> > > > > > > > > > >
> > > > > > > > > > >  static const enum index item_pppoes[] = {
> > > > > > > > > > >  	ITEM_PPPOE_SEID,
> > > > > > > > > > > -	ITEM_NEXT,
> > > > > > > > > > > -	ZERO,
> > > > > > > > > > > -};
> > > > > > > > > > > -
> > > > > > > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > > > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > > >  	ITEM_NEXT,
> > > > > > > > > > >  	ZERO,
> > > > > > > > > > > @@ -2643,10 +2637,9 @@ static const struct token
> > > > > > > > > > > token_list[]
> > > =
> > > > {
> > > > > > > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > > > > > > >  		.name = "proto_id",
> > > > > > > > > > >  		.help = "match PPPoE session protocol
> > identifier",
> > > > > > > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > > > > > > -				sizeof(struct
> > > > > > > rte_flow_item_pppoe_proto_id)),
> > > > > > > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > > > > > > -		.call = parse_vc,
> > > > > > > > > > > +		.next = NEXT(item_pppoes,
> > NEXT_ENTRY(UNSIGNED),
> > > > > > > > > > > item_param),
> > > > > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > > > > +			     (struct
> > rte_flow_item_pppoe_proto_id,
> > > > > > > proto_id)),
> > > > > > > > > >
> > > > > > > > > > Where is the memory for this proto_id is defined?
> > > > > > > > >
> > > > > > > > > Do you mean this?
> > > > > > > > > lib/librte_ethdev/rte_flow.h
> > > > > > > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > > > > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > > > > > > > > 1362 };
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yes. Why don't you use this one?
> > > > > > >
> > > > > > > I think I was using this, am I using it incorrectly?
> > > > > > >
> > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> > > proto_id)),
> > > > > > >
> > > > > >
> > > > > > Yes but there is no space to save this data since you deleted the priv.
> > > > > > I think you are trying to implement something like
> > > > > > RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> > > > > >
> > > > > > And I don't understand what was the problem with the previous
> > > > > implementation.
> > > > >
> > > > > I deleted the priv because it changed to a subcommand in pppoes,
> > > > > the command line will be like this:
> > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > testpmd> 00:11:22:33:44:55 / pppoes
> > > > > proto_id is 21
> > > > >
> > > >
> > > > The issue is that the pppoe struct doesn't have definition to the proto_id.
> > > > If you wish a possible solution will be to add it to the pppoe
> > > > struct, I'm not
> > > sure
> > > > if this is the correct approach since this field is not a must.
> > > >
> > > > Like I said there are examples on how to work with extended headers,
> > > > which I think are more correct, buy may be the problem is that the
> > > > pppoe struct is
> > > not
> > > > aligned and this result in an issue when adding the last bytes.
> > >
> > >
> > > There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do you
> > > mean make use of that?
> > > That is the reason for use extended header for this?
> > > But that seems as you say, has some bug.
> > >
> >
> > I understand there is a bug, the question how to solve it.
> > I suggested two approaches. Add the proto_id to the pppoe struct, but this
> > means that we will add a new member that is not part of the original
> definition.
> > Maybe the issue is in the PMD and it needs to understand that the proto_id
> > should be located in a different offset.
> > In any case it doesn't look like the current fix the right one.
> 
> From my understanding, you mean there are two approaches. One is adding
> proto_id to pppoe struct. But you don't prefer this one since proto_id is not a
> must. I am not clear about the other one.
> 

The solution should be just like the pdu_type which is part of the gtp_psc.
You can find also my comments on this, in the ML.
I think it is exactly the same case.
Example line for pdu type: flow create 0 ingress pattern gtp_psc pdu_t is xxx
The thread https://patches.dpdk.org/patch/67198/


> And also how do you suggest the command line be for proto_id?
> "proto_id is 0x0021" or "pppoes proto_id is 0x0021"? If the former just like
> what it was, I think it maybe a little confused. If the latter (as proto_id is part of
> pppoes), do we still need to put proto_id in rte_flow_item_pppoe?
> 
> Thanks,
> Xiao
> 
> >
> > >
> > > >
> > > > > The previous implementation would be infinite loop for proto_id
> > > > > command and can not specific the value for proto_id.
> > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > > specify next pattern item
> > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > proto_id
> > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > > specify next pattern item
> > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > proto_id proto_id
> > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > > specify next pattern item
> > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > proto_id proto_id
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >  	},
> > > > > > > > > > >  	[ITEM_HIGIG2] = {
> > > > > > > > > > >  		.name = "higig2",
> > > > > > > > > > > --
> > > > > > > > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Zhang, Xiao]

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Tuesday, March 31, 2020 2:55 PM
> To: Zhang, Xiao <xiao.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Xiao <xiao.zhang@intel.com>
> >
> > Hi Ori,
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@mellanox.com>
> > >
> > > Hi Xiao,
> > >
> > > > -----Original Message-----
> > > > From: Zhao1, Wei <wei.zhao1@intel.com>
> > > >
> > > > Hi, Ori
> > > >
> > > > > -----Original Message-----
> > > > > From: Ori Kam <orika@mellanox.com>
> > > > >
> > > > > Hi Xiao
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > >
> > > > > > Hi Ori,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > >
> > > > > > > Hi Xiao,
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > >
> > > > > > > > Hi Ori,
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > >
> > > > > > > > > Hi Xiao,
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > > > > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei
> > > > > > > > > > <wei.zhao1@intel.com>; stable@dpdk.org
> > > > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > > > > >
> > > > > > > > > > Hi Ori,
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > > > >
> > > > > > > > > > > Hi Xiao,
> > > > > > > > > > >
> > > > > > > > > > > Is the proto_id part of the basic header or not?
> > > > > > > > > >
> > > > > > > > > > Proto_id is part of PPPOE session header,
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Where is the porto_id located? Inside the payload?
> > > > > > > >
> > > > > > > > Yes, my previous explanation was not clear. The protocol
> > > > > > > > ID is in the beginning of the payload in PPP Session Stage
> > > > > > > > according to
> > > RFC2516.
> > > > > > > >
> > > > > > > >                      	         1
> > > > > 2                                3
> > > > > > > >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3
> > > > > > > > 4  5
> > > > > > > > 6 7
> > > > > > > > 8 9
> > > > > > > > 0 1
> > > > > > > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> > > > > > > > +-+-+-
> > +
> > > > > > > >  |  VER   | TYPE   |      CODE      |
> > > > > SESSION_ID                    |
> > > > > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > > > > >  |                 LENGTH                    |
> > > > > payload                      ~
> > > > > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > > > > >
> > > > > > >
> > > > > > > Yes this is what I thought, does proto_id must be the first
> > > > > > > part of the
> > > > > payload?
> > > > > >
> > > > > > It must be the first part of the payload for PPP Session
> > > > > > Stage, not all PPPOE packets.
> > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > From the spec it looks like a different header.
> > > > > > > > > > >
> > > > > > > > > > > If it is part of the original header then all
> > > > > > > > > > > documentations and rte_structs
> > > > > > > > > > should
> > > > > > > > > > > be changed, to reflect this.
> > > > > > > > > > >
> > > > > > > > > > > It will be very helpful if the patch message would
> > > > > > > > > > > explain the bug and why it
> > > > > > > > > > was
> > > > > > > > > > > changed.
> > > > > > > > > >
> > > > > > > > > > Okay, will add more message. The next value of the
> > > > > > > > ITEM_PPPOE_PROTO_ID
> > > > > > > > > > should be unsigned value but not item list.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Also please see inline other comment.
> > > > > > > > > > >
> > > > > > > > > > > Best,
> > > > > > > > > > > Ori
> > > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > > > > > > > To: dev@dpdk.org
> > > > > > > > > > > > Cc: Ori Kam <orika@mellanox.com>;
> > > > > > > > > > > > ying.a.wang@intel.com; qi.z.zhang@intel.com;
> > > > > > > > > > > > wei.zhao1@intel.com; Xiao Zhang
> > > > > > > > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > > > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > > > > > > > >
> > > > > > > > > > > > The command line to create RTE flow for specific
> > > > > > > > > > > > proto_id of PPPOES is not correct. This patch is
> > > > > > > > > > > > to fix this
> > issue.
> > > > > > > > > > > >
> > > > > > > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow
> > > > > > > > > > > > API")
> > > > > > > > > > > > Cc: stable@dpdk.org
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > > > > > > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > b/app/test-pmd/cmdline_flow.c index
> > > > > > > > > > > > a78154502..c25a2598d
> > > > > > > > > > > > 100644
> > > > > > > > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > @@ -768,7 +768,6 @@ static const enum index
> > > > > > > > > > > > next_item[]
> > = {
> > > > > > > > > > > >  	ITEM_GTP_PSC,
> > > > > > > > > > > >  	ITEM_PPPOES,
> > > > > > > > > > > >  	ITEM_PPPOED,
> > > > > > > > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > > > >  	ITEM_HIGIG2,
> > > > > > > > > > > >  	ITEM_TAG,
> > > > > > > > > > > >  	ITEM_L2TPV3OIP,
> > > > > > > > > > > > @@ -1030,11 +1029,6 @@ static const enum index
> > > > > > > > > > > > item_pppoed[] = {
> > > > > > > > > > > >
> > > > > > > > > > > >  static const enum index item_pppoes[] = {
> > > > > > > > > > > >  	ITEM_PPPOE_SEID,
> > > > > > > > > > > > -	ITEM_NEXT,
> > > > > > > > > > > > -	ZERO,
> > > > > > > > > > > > -};
> > > > > > > > > > > > -
> > > > > > > > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > > > > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > > > >  	ITEM_NEXT,
> > > > > > > > > > > >  	ZERO,
> > > > > > > > > > > > @@ -2643,10 +2637,9 @@ static const struct token
> > > > > > > > > > > > token_list[]
> > > > =
> > > > > {
> > > > > > > > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > > > > > > > >  		.name = "proto_id",
> > > > > > > > > > > >  		.help = "match PPPoE session protocol
> > > identifier",
> > > > > > > > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > > > > > > > -				sizeof(struct
> > > > > > > > rte_flow_item_pppoe_proto_id)),
> > > > > > > > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > > > > > > > -		.call = parse_vc,
> > > > > > > > > > > > +		.next = NEXT(item_pppoes,
> > > NEXT_ENTRY(UNSIGNED),
> > > > > > > > > > > > item_param),
> > > > > > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > > > > > +			     (struct
> > > rte_flow_item_pppoe_proto_id,
> > > > > > > > proto_id)),
> > > > > > > > > > >
> > > > > > > > > > > Where is the memory for this proto_id is defined?
> > > > > > > > > >
> > > > > > > > > > Do you mean this?
> > > > > > > > > > lib/librte_ethdev/rte_flow.h
> > > > > > > > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > > > > > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
> > > > > > > > > > 1362 };
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yes. Why don't you use this one?
> > > > > > > >
> > > > > > > > I think I was using this, am I using it incorrectly?
> > > > > > > >
> > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> > > > proto_id)),
> > > > > > > >
> > > > > > >
> > > > > > > Yes but there is no space to save this data since you deleted the priv.
> > > > > > > I think you are trying to implement something like
> > > > > > > RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> > > > > > >
> > > > > > > And I don't understand what was the problem with the
> > > > > > > previous
> > > > > > implementation.
> > > > > >
> > > > > > I deleted the priv because it changed to a subcommand in
> > > > > > pppoes, the command line will be like this:
> > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > testpmd> 00:11:22:33:44:55 / pppoes
> > > > > > proto_id is 21
> > > > > >
> > > > >
> > > > > The issue is that the pppoe struct doesn't have definition to the proto_id.
> > > > > If you wish a possible solution will be to add it to the pppoe
> > > > > struct, I'm not
> > > > sure
> > > > > if this is the correct approach since this field is not a must.
> > > > >
> > > > > Like I said there are examples on how to work with extended
> > > > > headers, which I think are more correct, buy may be the problem
> > > > > is that the pppoe struct is
> > > > not
> > > > > aligned and this result in an issue when adding the last bytes.
> > > >
> > > >
> > > > There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do you
> > > > mean make use of that?
> > > > That is the reason for use extended header for this?
> > > > But that seems as you say, has some bug.
> > > >
> > >
> > > I understand there is a bug, the question how to solve it.
> > > I suggested two approaches. Add the proto_id to the pppoe struct,
> > > but this means that we will add a new member that is not part of the
> > > original
> > definition.
> > > Maybe the issue is in the PMD and it needs to understand that the
> > > proto_id should be located in a different offset.
> > > In any case it doesn't look like the current fix the right one.
> >
> > From my understanding, you mean there are two approaches. One is
> > adding proto_id to pppoe struct. But you don't prefer this one since
> > proto_id is not a must. I am not clear about the other one.
> >
> 
> The solution should be just like the pdu_type which is part of the gtp_psc.
> You can find also my comments on this, in the ML.
> I think it is exactly the same case.
> Example line for pdu type: flow create 0 ingress pattern gtp_psc pdu_t is xxx The
> thread https://patches.dpdk.org/patch/67198/

Yes, so the line for should be:
flow create 0 ingress pattern pppoes proto_id is xxx .

But since we already have pppoes line for command pppoes seid is xxx, we need use another word instead of pppoes for proto_id, right? If yes, do you have any suggestion?

> 
> 
> > And also how do you suggest the command line be for proto_id?
> > "proto_id is 0x0021" or "pppoes proto_id is 0x0021"? If the former
> > just like what it was, I think it maybe a little confused. If the
> > latter (as proto_id is part of pppoes), do we still need to put proto_id in
> rte_flow_item_pppoe?
> >
> > Thanks,
> > Xiao
> >
> > >
> > > >
> > > > >
> > > > > > The previous implementation would be infinite loop for
> > > > > > proto_id command and can not specific the value for proto_id.
> > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > > > specify next pattern item
> > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > proto_id
> > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > > > specify next pattern item
> > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > proto_id proto_id
> > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  / [TOKEN]:
> > > > > > specify next pattern item
> > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > proto_id proto_id
> > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >  	},
> > > > > > > > > > > >  	[ITEM_HIGIG2] = {
> > > > > > > > > > > >  		.name = "higig2",
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Ori Kam]

> -----Original Message-----
> From: Zhang, Xiao <xiao.zhang@intel.com>
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> 
> 
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > >
> > > Hi Ori,
> > >
> > > > -----Original Message-----
> > > > From: Ori Kam <orika@mellanox.com>
> > > >
> > > > Hi Xiao,
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhao1, Wei <wei.zhao1@intel.com>
> > > > >
> > > > > Hi, Ori
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > >
> > > > > > Hi Xiao
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > >
> > > > > > > Hi Ori,
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > >
> > > > > > > > Hi Xiao,
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > > >
> > > > > > > > > Hi Ori,
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > > >
> > > > > > > > > > Hi Xiao,
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > > > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > > > > > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> > > > > > > > > > > <qi.z.zhang@intel.com>; Zhao1, Wei
> > > > > > > > > > > <wei.zhao1@intel.com>; stable@dpdk.org
> > > > > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > > > > > >
> > > > > > > > > > > Hi Ori,
> > > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Xiao,
> > > > > > > > > > > >
> > > > > > > > > > > > Is the proto_id part of the basic header or not?
> > > > > > > > > > >
> > > > > > > > > > > Proto_id is part of PPPOE session header,
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Where is the porto_id located? Inside the payload?
> > > > > > > > >
> > > > > > > > > Yes, my previous explanation was not clear. The protocol
> > > > > > > > > ID is in the beginning of the payload in PPP Session Stage
> > > > > > > > > according to
> > > > RFC2516.
> > > > > > > > >
> > > > > > > > >                      	         1
> > > > > > 2                                3
> > > > > > > > >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2 3
> > > > > > > > > 4  5
> > > > > > > > > 6 7
> > > > > > > > > 8 9
> > > > > > > > > 0 1
> > > > > > > > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> > > > > > > > > +-+-+-
> > > +
> > > > > > > > >  |  VER   | TYPE   |      CODE      |
> > > > > > SESSION_ID                    |
> > > > > > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> +
> > > > > > > > >  |                 LENGTH                    |
> > > > > > payload                      ~
> > > > > > > > >  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yes this is what I thought, does proto_id must be the first
> > > > > > > > part of the
> > > > > > payload?
> > > > > > >
> > > > > > > It must be the first part of the payload for PPP Session
> > > > > > > Stage, not all PPPOE packets.
> > > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > From the spec it looks like a different header.
> > > > > > > > > > > >
> > > > > > > > > > > > If it is part of the original header then all
> > > > > > > > > > > > documentations and rte_structs
> > > > > > > > > > > should
> > > > > > > > > > > > be changed, to reflect this.
> > > > > > > > > > > >
> > > > > > > > > > > > It will be very helpful if the patch message would
> > > > > > > > > > > > explain the bug and why it
> > > > > > > > > > > was
> > > > > > > > > > > > changed.
> > > > > > > > > > >
> > > > > > > > > > > Okay, will add more message. The next value of the
> > > > > > > > > ITEM_PPPOE_PROTO_ID
> > > > > > > > > > > should be unsigned value but not item list.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Also please see inline other comment.
> > > > > > > > > > > >
> > > > > > > > > > > > Best,
> > > > > > > > > > > > Ori
> > > > > > > > > > > >
> > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > > > > > > > > To: dev@dpdk.org
> > > > > > > > > > > > > Cc: Ori Kam <orika@mellanox.com>;
> > > > > > > > > > > > > ying.a.wang@intel.com; qi.z.zhang@intel.com;
> > > > > > > > > > > > > wei.zhao1@intel.com; Xiao Zhang
> > > > > > > > > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > > > > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > > > > > > > > >
> > > > > > > > > > > > > The command line to create RTE flow for specific
> > > > > > > > > > > > > proto_id of PPPOES is not correct. This patch is
> > > > > > > > > > > > > to fix this
> > > issue.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow
> > > > > > > > > > > > > API")
> > > > > > > > > > > > > Cc: stable@dpdk.org
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  app/test-pmd/cmdline_flow.c | 13 +++----------
> > > > > > > > > > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > > b/app/test-pmd/cmdline_flow.c index
> > > > > > > > > > > > > a78154502..c25a2598d
> > > > > > > > > > > > > 100644
> > > > > > > > > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > > @@ -768,7 +768,6 @@ static const enum index
> > > > > > > > > > > > > next_item[]
> > > = {
> > > > > > > > > > > > >  	ITEM_GTP_PSC,
> > > > > > > > > > > > >  	ITEM_PPPOES,
> > > > > > > > > > > > >  	ITEM_PPPOED,
> > > > > > > > > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > > > > >  	ITEM_HIGIG2,
> > > > > > > > > > > > >  	ITEM_TAG,
> > > > > > > > > > > > >  	ITEM_L2TPV3OIP,
> > > > > > > > > > > > > @@ -1030,11 +1029,6 @@ static const enum index
> > > > > > > > > > > > > item_pppoed[] = {
> > > > > > > > > > > > >
> > > > > > > > > > > > >  static const enum index item_pppoes[] = {
> > > > > > > > > > > > >  	ITEM_PPPOE_SEID,
> > > > > > > > > > > > > -	ITEM_NEXT,
> > > > > > > > > > > > > -	ZERO,
> > > > > > > > > > > > > -};
> > > > > > > > > > > > > -
> > > > > > > > > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > > > > > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > > > > >  	ITEM_NEXT,
> > > > > > > > > > > > >  	ZERO,
> > > > > > > > > > > > > @@ -2643,10 +2637,9 @@ static const struct token
> > > > > > > > > > > > > token_list[]
> > > > > =
> > > > > > {
> > > > > > > > > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > > > > > > > > >  		.name = "proto_id",
> > > > > > > > > > > > >  		.help = "match PPPoE session protocol
> > > > identifier",
> > > > > > > > > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > > > > > > > > -				sizeof(struct
> > > > > > > > > rte_flow_item_pppoe_proto_id)),
> > > > > > > > > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > > > > > > > > -		.call = parse_vc,
> > > > > > > > > > > > > +		.next = NEXT(item_pppoes,
> > > > NEXT_ENTRY(UNSIGNED),
> > > > > > > > > > > > > item_param),
> > > > > > > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > > > > > > +			     (struct
> > > > rte_flow_item_pppoe_proto_id,
> > > > > > > > > proto_id)),
> > > > > > > > > > > >
> > > > > > > > > > > > Where is the memory for this proto_id is defined?
> > > > > > > > > > >
> > > > > > > > > > > Do you mean this?
> > > > > > > > > > > lib/librte_ethdev/rte_flow.h
> > > > > > > > > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > > > > > > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier.
> */
> > > > > > > > > > > 1362 };
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Yes. Why don't you use this one?
> > > > > > > > >
> > > > > > > > > I think I was using this, am I using it incorrectly?
> > > > > > > > >
> > > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > > +			     (struct rte_flow_item_pppoe_proto_id,
> > > > > proto_id)),
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yes but there is no space to save this data since you deleted the
> priv.
> > > > > > > > I think you are trying to implement something like
> > > > > > > > RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> > > > > > > >
> > > > > > > > And I don't understand what was the problem with the
> > > > > > > > previous
> > > > > > > implementation.
> > > > > > >
> > > > > > > I deleted the priv because it changed to a subcommand in
> > > > > > > pppoes, the command line will be like this:
> > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > testpmd> 00:11:22:33:44:55 / pppoes
> > > > > > > proto_id is 21
> > > > > > >
> > > > > >
> > > > > > The issue is that the pppoe struct doesn't have definition to the
> proto_id.
> > > > > > If you wish a possible solution will be to add it to the pppoe
> > > > > > struct, I'm not
> > > > > sure
> > > > > > if this is the correct approach since this field is not a must.
> > > > > >
> > > > > > Like I said there are examples on how to work with extended
> > > > > > headers, which I think are more correct, buy may be the problem
> > > > > > is that the pppoe struct is
> > > > > not
> > > > > > aligned and this result in an issue when adding the last bytes.
> > > > >
> > > > >
> > > > > There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do
> you
> > > > > mean make use of that?
> > > > > That is the reason for use extended header for this?
> > > > > But that seems as you say, has some bug.
> > > > >
> > > >
> > > > I understand there is a bug, the question how to solve it.
> > > > I suggested two approaches. Add the proto_id to the pppoe struct,
> > > > but this means that we will add a new member that is not part of the
> > > > original
> > > definition.
> > > > Maybe the issue is in the PMD and it needs to understand that the
> > > > proto_id should be located in a different offset.
> > > > In any case it doesn't look like the current fix the right one.
> > >
> > > From my understanding, you mean there are two approaches. One is
> > > adding proto_id to pppoe struct. But you don't prefer this one since
> > > proto_id is not a must. I am not clear about the other one.
> > >
> >
> > The solution should be just like the pdu_type which is part of the gtp_psc.
> > You can find also my comments on this, in the ML.
> > I think it is exactly the same case.
> > Example line for pdu type: flow create 0 ingress pattern gtp_psc pdu_t is xxx
> The
> > thread
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatches.d
> pdk.org%2Fpatch%2F67198%2F&amp;data=02%7C01%7Corika%40mellanox.co
> m%7Cb393253da4f84b2e909908d7d54b817b%7Ca652971c7d2e4d9ba6a4d149
> 256f461b%7C0%7C0%7C637212392573440621&amp;sdata=QBGaw8RypOoikbb
> veKhbgv4PxZbOJ7p7pNESV6D%2FBT0%3D&amp;reserved=0
> 
> Yes, so the line for should be:
> flow create 0 ingress pattern pppoes proto_id is xxx .
> 
> But since we already have pppoes line for command pppoes seid is xxx, we
> need use another word instead of pppoes for proto_id, right? If yes, do you
> have any suggestion?
> 

I think it should be something like this:
Flow create 0 ingress pattern  pppoe_proto_id  proto_id is xxx

Since the pppoe_proto_id has only one field maybe we can go with the following approach:
Flow create 0 ingress pattern pppoe_proto_id is xxx


> >
> >
> > > And also how do you suggest the command line be for proto_id?
> > > "proto_id is 0x0021" or "pppoes proto_id is 0x0021"? If the former
> > > just like what it was, I think it maybe a little confused. If the
> > > latter (as proto_id is part of pppoes), do we still need to put proto_id in
> > rte_flow_item_pppoe?
> > >
> > > Thanks,
> > > Xiao
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > The previous implementation would be infinite loop for
> > > > > > > proto_id command and can not specific the value for proto_id.
> > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  /
> [TOKEN]:
> > > > > > > specify next pattern item
> > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > > proto_id
> > > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  /
> [TOKEN]:
> > > > > > > specify next pattern item
> > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > > proto_id proto_id
> > > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier  /
> [TOKEN]:
> > > > > > > specify next pattern item
> > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > > proto_id proto_id
> > > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > >  	},
> > > > > > > > > > > > >  	[ITEM_HIGIG2] = {
> > > > > > > > > > > > >  		.name = "higig2",
> > > > > > > > > > > > > --
> > > > > > > > > > > > > 2.17.1

Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

[Zhang, Xiao]

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Tuesday, March 31, 2020 5:06 PM
> To: Zhang, Xiao <xiao.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Xiao <xiao.zhang@intel.com>
> > Subject: RE: app/testpmd: fix PPPOES flow API
> >
> >
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@mellanox.com>
> > > Subject: RE: app/testpmd: fix PPPOES flow API
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > >
> > > > Hi Ori,
> > > >
> > > > > -----Original Message-----
> > > > > From: Ori Kam <orika@mellanox.com>
> > > > >
> > > > > Hi Xiao,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zhao1, Wei <wei.zhao1@intel.com>
> > > > > >
> > > > > > Hi, Ori
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > >
> > > > > > > Hi Xiao
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > >
> > > > > > > > Hi Ori,
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > >
> > > > > > > > > Hi Xiao,
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > > > >
> > > > > > > > > > Hi Ori,
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > > > >
> > > > > > > > > > > Hi Xiao,
> > > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Zhang, Xiao <xiao.zhang@intel.com>
> > > > > > > > > > > > Sent: Sunday, March 29, 2020 12:06 PM
> > > > > > > > > > > > To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> > > > > > > > > > > > Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang,
> > > > > > > > > > > > Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> > > > > > > > > > > > <wei.zhao1@intel.com>; stable@dpdk.org
> > > > > > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Ori,
> > > > > > > > > > > >
> > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > From: Ori Kam <orika@mellanox.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Xiao,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Is the proto_id part of the basic header or not?
> > > > > > > > > > > >
> > > > > > > > > > > > Proto_id is part of PPPOE session header,
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Where is the porto_id located? Inside the payload?
> > > > > > > > > >
> > > > > > > > > > Yes, my previous explanation was not clear. The
> > > > > > > > > > protocol ID is in the beginning of the payload in PPP
> > > > > > > > > > Session Stage according to
> > > > > RFC2516.
> > > > > > > > > >
> > > > > > > > > >                      	         1
> > > > > > > 2                                3
> > > > > > > > > >    0 1 2  3  4  5 6 7 8 9 0 1  2 3 4  5 6 7 8 9  0 1 2
> > > > > > > > > > 3
> > > > > > > > > > 4  5
> > > > > > > > > > 6 7
> > > > > > > > > > 8 9
> > > > > > > > > > 0 1
> > > > > > > > > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> > > > > > > > > > +-+-+-
> > > > +
> > > > > > > > > >  |  VER   | TYPE   |      CODE      |
> > > > > > > SESSION_ID                    |
> > > > > > > > > >
> > > > > > > > > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> > > > > > > > > > +-+-+-+-+-
> > +
> > > > > > > > > >  |                 LENGTH                    |
> > > > > > > payload                      ~
> > > > > > > > > >
> > > > > > > > > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> > > > > > > > > > +-+
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yes this is what I thought, does proto_id must be the
> > > > > > > > > first part of the
> > > > > > > payload?
> > > > > > > >
> > > > > > > > It must be the first part of the payload for PPP Session
> > > > > > > > Stage, not all PPPOE packets.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > From the spec it looks like a different header.
> > > > > > > > > > > > >
> > > > > > > > > > > > > If it is part of the original header then all
> > > > > > > > > > > > > documentations and rte_structs
> > > > > > > > > > > > should
> > > > > > > > > > > > > be changed, to reflect this.
> > > > > > > > > > > > >
> > > > > > > > > > > > > It will be very helpful if the patch message
> > > > > > > > > > > > > would explain the bug and why it
> > > > > > > > > > > > was
> > > > > > > > > > > > > changed.
> > > > > > > > > > > >
> > > > > > > > > > > > Okay, will add more message. The next value of the
> > > > > > > > > > ITEM_PPPOE_PROTO_ID
> > > > > > > > > > > > should be unsigned value but not item list.
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > Also please see inline other comment.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Best,
> > > > > > > > > > > > > Ori
> > > > > > > > > > > > >
> > > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > > From: Xiao Zhang <xiao.zhang@intel.com>
> > > > > > > > > > > > > > Sent: Friday, March 27, 2020 11:19 AM
> > > > > > > > > > > > > > To: dev@dpdk.org
> > > > > > > > > > > > > > Cc: Ori Kam <orika@mellanox.com>;
> > > > > > > > > > > > > > ying.a.wang@intel.com; qi.z.zhang@intel.com;
> > > > > > > > > > > > > > wei.zhao1@intel.com; Xiao Zhang
> > > > > > > > > > > > > > <xiao.zhang@intel.com>; stable@dpdk.org
> > > > > > > > > > > > > > Subject: app/testpmd: fix PPPOES flow API
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > The command line to create RTE flow for
> > > > > > > > > > > > > > specific proto_id of PPPOES is not correct.
> > > > > > > > > > > > > > This patch is to fix this
> > > > issue.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to
> > > > > > > > > > > > > > flow
> > > > > > > > > > > > > > API")
> > > > > > > > > > > > > > Cc: stable@dpdk.org
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Xiao Zhang
> > > > > > > > > > > > > > <xiao.zhang@intel.com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >  app/test-pmd/cmdline_flow.c | 13
> > > > > > > > > > > > > > +++----------
> > > > > > > > > > > > > >  1 file changed, 3 insertions(+), 10
> > > > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > > > b/app/test-pmd/cmdline_flow.c index
> > > > > > > > > > > > > > a78154502..c25a2598d
> > > > > > > > > > > > > > 100644
> > > > > > > > > > > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > > > > > > > > > > @@ -768,7 +768,6 @@ static const enum index
> > > > > > > > > > > > > > next_item[]
> > > > = {
> > > > > > > > > > > > > >  	ITEM_GTP_PSC,
> > > > > > > > > > > > > >  	ITEM_PPPOES,
> > > > > > > > > > > > > >  	ITEM_PPPOED,
> > > > > > > > > > > > > > -	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > > > > > >  	ITEM_HIGIG2,
> > > > > > > > > > > > > >  	ITEM_TAG,
> > > > > > > > > > > > > >  	ITEM_L2TPV3OIP, @@ -1030,11 +1029,6 @@
> > > > > > > > > > > > > > static const enum index item_pppoed[] = {
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >  static const enum index item_pppoes[] = {
> > > > > > > > > > > > > >  	ITEM_PPPOE_SEID,
> > > > > > > > > > > > > > -	ITEM_NEXT,
> > > > > > > > > > > > > > -	ZERO,
> > > > > > > > > > > > > > -};
> > > > > > > > > > > > > > -
> > > > > > > > > > > > > > -static const enum index item_pppoe_proto_id[] = {
> > > > > > > > > > > > > >  	ITEM_PPPOE_PROTO_ID,
> > > > > > > > > > > > > >  	ITEM_NEXT,
> > > > > > > > > > > > > >  	ZERO,
> > > > > > > > > > > > > > @@ -2643,10 +2637,9 @@ static const struct
> > > > > > > > > > > > > > token token_list[]
> > > > > > =
> > > > > > > {
> > > > > > > > > > > > > >  	[ITEM_PPPOE_PROTO_ID] = {
> > > > > > > > > > > > > >  		.name = "proto_id",
> > > > > > > > > > > > > >  		.help = "match PPPoE session protocol
> > > > > identifier",
> > > > > > > > > > > > > > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > > > > > > > > > > > > > -				sizeof(struct
> > > > > > > > > > rte_flow_item_pppoe_proto_id)),
> > > > > > > > > > > > > > -		.next = NEXT(item_pppoe_proto_id),
> > > > > > > > > > > > > > -		.call = parse_vc,
> > > > > > > > > > > > > > +		.next = NEXT(item_pppoes,
> > > > > NEXT_ENTRY(UNSIGNED),
> > > > > > > > > > > > > > item_param),
> > > > > > > > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > > > > > > > +			     (struct
> > > > > rte_flow_item_pppoe_proto_id,
> > > > > > > > > > proto_id)),
> > > > > > > > > > > > >
> > > > > > > > > > > > > Where is the memory for this proto_id is defined?
> > > > > > > > > > > >
> > > > > > > > > > > > Do you mean this?
> > > > > > > > > > > > lib/librte_ethdev/rte_flow.h
> > > > > > > > > > > > 1360 struct rte_flow_item_pppoe_proto_id {
> > > > > > > > > > > > 1361         rte_be16_t proto_id; /**< PPP protocol identifier.
> > */
> > > > > > > > > > > > 1362 };
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Yes. Why don't you use this one?
> > > > > > > > > >
> > > > > > > > > > I think I was using this, am I using it incorrectly?
> > > > > > > > > >
> > > > > > > > > > +		.args = ARGS(ARGS_ENTRY_HTON
> > > > > > > > > > +			     (struct
> rte_flow_item_pppoe_proto_id,
> > > > > > proto_id)),
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yes but there is no space to save this data since you
> > > > > > > > > deleted the
> > priv.
> > > > > > > > > I think you are trying to implement something like
> > > > > > > > > RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> > > > > > > > >
> > > > > > > > > And I don't understand what was the problem with the
> > > > > > > > > previous
> > > > > > > > implementation.
> > > > > > > >
> > > > > > > > I deleted the priv because it changed to a subcommand in
> > > > > > > > pppoes, the command line will be like this:
> > > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > > testpmd> 00:11:22:33:44:55 / pppoes
> > > > > > > > proto_id is 21
> > > > > > > >
> > > > > > >
> > > > > > > The issue is that the pppoe struct doesn't have definition
> > > > > > > to the
> > proto_id.
> > > > > > > If you wish a possible solution will be to add it to the
> > > > > > > pppoe struct, I'm not
> > > > > > sure
> > > > > > > if this is the correct approach since this field is not a must.
> > > > > > >
> > > > > > > Like I said there are examples on how to work with extended
> > > > > > > headers, which I think are more correct, buy may be the
> > > > > > > problem is that the pppoe struct is
> > > > > > not
> > > > > > > aligned and this result in an issue when adding the last bytes.
> > > > > >
> > > > > >
> > > > > > There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do
> > you
> > > > > > mean make use of that?
> > > > > > That is the reason for use extended header for this?
> > > > > > But that seems as you say, has some bug.
> > > > > >
> > > > >
> > > > > I understand there is a bug, the question how to solve it.
> > > > > I suggested two approaches. Add the proto_id to the pppoe
> > > > > struct, but this means that we will add a new member that is not
> > > > > part of the original
> > > > definition.
> > > > > Maybe the issue is in the PMD and it needs to understand that
> > > > > the proto_id should be located in a different offset.
> > > > > In any case it doesn't look like the current fix the right one.
> > > >
> > > > From my understanding, you mean there are two approaches. One is
> > > > adding proto_id to pppoe struct. But you don't prefer this one
> > > > since proto_id is not a must. I am not clear about the other one.
> > > >
> > >
> > > The solution should be just like the pdu_type which is part of the gtp_psc.
> > > You can find also my comments on this, in the ML.
> > > I think it is exactly the same case.
> > > Example line for pdu type: flow create 0 ingress pattern gtp_psc
> > > pdu_t is xxx
> > The
> > > thread
> > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hes.d
> >
> pdk.org%2Fpatch%2F67198%2F&amp;data=02%7C01%7Corika%40mellanox.co
> >
> m%7Cb393253da4f84b2e909908d7d54b817b%7Ca652971c7d2e4d9ba6a4d149
> >
> 256f461b%7C0%7C0%7C637212392573440621&amp;sdata=QBGaw8RypOoikbb
> > veKhbgv4PxZbOJ7p7pNESV6D%2FBT0%3D&amp;reserved=0
> >
> > Yes, so the line for should be:
> > flow create 0 ingress pattern pppoes proto_id is xxx .
> >
> > But since we already have pppoes line for command pppoes seid is xxx,
> > we need use another word instead of pppoes for proto_id, right? If
> > yes, do you have any suggestion?
> >
> 
> I think it should be something like this:
> Flow create 0 ingress pattern  pppoe_proto_id  proto_id is xxx
> 
> Since the pppoe_proto_id has only one field maybe we can go with the
> following approach:
> Flow create 0 ingress pattern pppoe_proto_id is xxx

Yes, "flow create 0 ingress pattern pppoe_proto_id is xxx" will be better, will change to it.

> 
> 
> > >
> > >
> > > > And also how do you suggest the command line be for proto_id?
> > > > "proto_id is 0x0021" or "pppoes proto_id is 0x0021"? If the former
> > > > just like what it was, I think it maybe a little confused. If the
> > > > latter (as proto_id is part of pppoes), do we still need to put
> > > > proto_id in
> > > rte_flow_item_pppoe?
> > > >
> > > > Thanks,
> > > > Xiao
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > The previous implementation would be infinite loop for
> > > > > > > > proto_id command and can not specific the value for proto_id.
> > > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier
> > > > > > > > /
> > [TOKEN]:
> > > > > > > > specify next pattern item
> > > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > > > proto_id
> > > > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier
> > > > > > > > /
> > [TOKEN]:
> > > > > > > > specify next pattern item
> > > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > > > proto_id proto_id
> > > > > > > >  proto_id [TOKEN]: match PPPoE session protocol identifier
> > > > > > > > /
> > [TOKEN]:
> > > > > > > > specify next pattern item
> > > > > > > > testpmd> flow create 0 ingress pattern eth dst is
> > > > > > > > testpmd> 00:11:22:33:44:55 / proto_id
> > > > > > > > proto_id proto_id
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > >  	},
> > > > > > > > > > > > > >  	[ITEM_HIGIG2] = {
> > > > > > > > > > > > > >  		.name = "higig2",
> > > > > > > > > > > > > > --
> > > > > > > > > > > > > > 2.17.1

[dpdk-dev] [v2] app/testpmd: fix PPPOES flow API

[Xiao Zhang]

The command line to create RTE flow for specific proto_id of PPPOES can
not work.

It was:
testpmd> flow create 0 ingress pattern proto_id
 proto_id [TOKEN]: match PPPoE session protocol identifier
testpmd> flow create 0 ingress pattern proto_id proto_id
 proto_id [TOKEN]: match PPPoE session protocol identifier
testpmd> flow create 0 ingress pattern proto_id proto_id proto_id
 proto_id [TOKEN]: match PPPoE session protocol identifier

The proto_id can not be set with previous implementation.

This patch is to fix this issue, and change the command line to:
testpmd> flow create 0 pattern pppoe_proto_id is xxxx

Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
Cc: stable@dpdk.org

Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
---
v2 change the command line according to review comments.
---
 app/test-pmd/cmdline_flow.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index a78154502..7ac01562e 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -1035,7 +1035,6 @@ static const enum index item_pppoes[] = {
 };
 
 static const enum index item_pppoe_proto_id[] = {
-	ITEM_PPPOE_PROTO_ID,
 	ITEM_NEXT,
 	ZERO,
 };
@@ -2641,11 +2640,14 @@ static const struct token token_list[] = {
 					session_id)),
 	},
 	[ITEM_PPPOE_PROTO_ID] = {
-		.name = "proto_id",
+		.name = "pppoe_proto_id",
 		.help = "match PPPoE session protocol identifier",
 		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
 				sizeof(struct rte_flow_item_pppoe_proto_id)),
-		.next = NEXT(item_pppoe_proto_id),
+		.next = NEXT(item_pppoe_proto_id, NEXT_ENTRY(UNSIGNED),
+			     item_param),
+		.args = ARGS(ARGS_ENTRY_HTON
+			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
 		.call = parse_vc,
 	},
 	[ITEM_HIGIG2] = {
-- 
2.17.1

Re: [dpdk-dev] [v2] app/testpmd: fix PPPOES flow API

[Ori Kam]

Hi Xiao,



> -----Original Message-----
> From: Xiao Zhang <xiao.zhang@intel.com>
> Sent: Tuesday, March 31, 2020 4:30 PM
> To: dev@dpdk.org
> Cc: Ori Kam <orika@mellanox.com>; qi.z.zhang@intel.com;
> wei.zhao1@intel.com; Xiao Zhang <xiao.zhang@intel.com>; stable@dpdk.org
> Subject: [v2] app/testpmd: fix PPPOES flow API
> 
> The command line to create RTE flow for specific proto_id of PPPOES can
> not work.
> 
> It was:
> testpmd> flow create 0 ingress pattern proto_id
>  proto_id [TOKEN]: match PPPoE session protocol identifier
> testpmd> flow create 0 ingress pattern proto_id proto_id
>  proto_id [TOKEN]: match PPPoE session protocol identifier
> testpmd> flow create 0 ingress pattern proto_id proto_id proto_id
>  proto_id [TOKEN]: match PPPoE session protocol identifier
> 
> The proto_id can not be set with previous implementation.
> 
> This patch is to fix this issue, and change the command line to:
> testpmd> flow create 0 pattern pppoe_proto_id is xxxx
> 
> Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> ---
> v2 change the command line according to review comments.
> ---

Acked-by: Ori Kam <orika@mellanox.com>
Thanks,
Ori

>  app/test-pmd/cmdline_flow.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index a78154502..7ac01562e 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -1035,7 +1035,6 @@ static const enum index item_pppoes[] = {
>  };
> 
>  static const enum index item_pppoe_proto_id[] = {
> -	ITEM_PPPOE_PROTO_ID,
>  	ITEM_NEXT,
>  	ZERO,
>  };
> @@ -2641,11 +2640,14 @@ static const struct token token_list[] = {
>  					session_id)),
>  	},
>  	[ITEM_PPPOE_PROTO_ID] = {
> -		.name = "proto_id",
> +		.name = "pppoe_proto_id",
>  		.help = "match PPPoE session protocol identifier",
>  		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
>  				sizeof(struct rte_flow_item_pppoe_proto_id)),
> -		.next = NEXT(item_pppoe_proto_id),
> +		.next = NEXT(item_pppoe_proto_id, NEXT_ENTRY(UNSIGNED),
> +			     item_param),
> +		.args = ARGS(ARGS_ENTRY_HTON
> +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
>  		.call = parse_vc,
>  	},
>  	[ITEM_HIGIG2] = {
> --
> 2.17.1

Re: [dpdk-dev] [dpdk-stable] [v2] app/testpmd: fix PPPOES flow API

[Ferruh Yigit]

On 4/5/2020 4:12 PM, Ori Kam wrote:
> Hi Xiao,
> 
> 
> 
>> -----Original Message-----
>> From: Xiao Zhang <xiao.zhang@intel.com>
>> Sent: Tuesday, March 31, 2020 4:30 PM
>> To: dev@dpdk.org
>> Cc: Ori Kam <orika@mellanox.com>; qi.z.zhang@intel.com;
>> wei.zhao1@intel.com; Xiao Zhang <xiao.zhang@intel.com>; stable@dpdk.org
>> Subject: [v2] app/testpmd: fix PPPOES flow API
>>
>> The command line to create RTE flow for specific proto_id of PPPOES can
>> not work.
>>
>> It was:
>> testpmd> flow create 0 ingress pattern proto_id
>>  proto_id [TOKEN]: match PPPoE session protocol identifier
>> testpmd> flow create 0 ingress pattern proto_id proto_id
>>  proto_id [TOKEN]: match PPPoE session protocol identifier
>> testpmd> flow create 0 ingress pattern proto_id proto_id proto_id
>>  proto_id [TOKEN]: match PPPoE session protocol identifier
>>
>> The proto_id can not be set with previous implementation.
>>
>> This patch is to fix this issue, and change the command line to:
>> testpmd> flow create 0 pattern pppoe_proto_id is xxxx
>>
>> Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
>> ---
>> v2 change the command line according to review comments.
>> ---
> 
> Acked-by: Ori Kam <orika@mellanox.com>

Applied to dpdk-next-net/master, thanks.