Hi Ori, > -----Original Message----- > From: Ori Kam <or...@mellanox.com> > Sent: Monday, March 30, 2020 4:57 PM > To: Zhao1, Wei <wei.zh...@intel.com>; Zhang, Xiao <xiao.zh...@intel.com>; > dev@dpdk.org > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > <qi.z.zh...@intel.com>; sta...@dpdk.org > Subject: RE: app/testpmd: fix PPPOES flow API > > Hi Xiao, > > > -----Original Message----- > > From: Zhao1, Wei <wei.zh...@intel.com> > > Sent: Monday, March 30, 2020 11:50 AM > > To: Ori Kam <or...@mellanox.com>; Zhang, Xiao <xiao.zh...@intel.com>; > > dev@dpdk.org > > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > > <qi.z.zh...@intel.com>; sta...@dpdk.org > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > Hi, Ori > > > > > -----Original Message----- > > > From: Ori Kam <or...@mellanox.com> > > > Sent: Monday, March 30, 2020 3:43 PM > > > To: Zhang, Xiao <xiao.zh...@intel.com>; dev@dpdk.org > > > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > > > <qi.z.zh...@intel.com>; Zhao1, Wei <wei.zh...@intel.com>; > > > sta...@dpdk.org > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > Hi Xiao > > > > > > > -----Original Message----- > > > > From: Zhang, Xiao <xiao.zh...@intel.com> > > > > Sent: Monday, March 30, 2020 5:09 AM > > > > To: Ori Kam <or...@mellanox.com>; dev@dpdk.org > > > > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > > > > <qi.z.zh...@intel.com>; Zhao1, Wei <wei.zh...@intel.com>; > > > > sta...@dpdk.org > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > Hi Ori, > > > > > > > > > -----Original Message----- > > > > > From: Ori Kam <or...@mellanox.com> > > > > > Sent: Sunday, March 29, 2020 8:46 PM > > > > > To: Zhang, Xiao <xiao.zh...@intel.com>; dev@dpdk.org > > > > > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > > > > > <qi.z.zh...@intel.com>; Zhao1, Wei <wei.zh...@intel.com>; > > > > sta...@dpdk.org > > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > > Hi Xiao, > > > > > > > > > > > -----Original Message----- > > > > > > From: Zhang, Xiao <xiao.zh...@intel.com> > > > > > > Sent: Sunday, March 29, 2020 3:00 PM > > > > > > To: Ori Kam <or...@mellanox.com>; dev@dpdk.org > > > > > > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > > > > > > <qi.z.zh...@intel.com>; Zhao1, Wei <wei.zh...@intel.com>; > > > > > > sta...@dpdk.org > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > > > > Hi Ori, > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Ori Kam <or...@mellanox.com> > > > > > > > Sent: Sunday, March 29, 2020 6:19 PM > > > > > > > To: Zhang, Xiao <xiao.zh...@intel.com>; dev@dpdk.org > > > > > > > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > > > > > > > <qi.z.zh...@intel.com>; Zhao1, Wei <wei.zh...@intel.com>; > > > > > > sta...@dpdk.org > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > > > > > > Hi Xiao, > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Zhang, Xiao <xiao.zh...@intel.com> > > > > > > > > Sent: Sunday, March 29, 2020 12:06 PM > > > > > > > > To: Ori Kam <or...@mellanox.com>; dev@dpdk.org > > > > > > > > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > > > > > > > > <qi.z.zh...@intel.com>; Zhao1, Wei <wei.zh...@intel.com>; > > > > > > > > sta...@dpdk.org > > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > > > > > > > > Hi Ori, > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > From: Ori Kam <or...@mellanox.com> > > > > > > > > > Sent: Sunday, March 29, 2020 2:28 PM > > > > > > > > > To: Zhang, Xiao <xiao.zh...@intel.com>; dev@dpdk.org > > > > > > > > > Cc: Wang, Ying A <ying.a.w...@intel.com>; Zhang, Qi Z > > > > > > > > > <qi.z.zh...@intel.com>; Zhao1, Wei > > > > > > > > > <wei.zh...@intel.com>; > > > > > > > > sta...@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.zh...@intel.com> > > > > > > > > > > Sent: Friday, March 27, 2020 11:19 AM > > > > > > > > > > To: dev@dpdk.org > > > > > > > > > > Cc: Ori Kam <or...@mellanox.com>; > > > > > > > > > > ying.a.w...@intel.com; qi.z.zh...@intel.com; > > > > > > > > > > wei.zh...@intel.com; Xiao Zhang > > > > > > > > > > <xiao.zh...@intel.com>; sta...@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: sta...@dpdk.org > > > > > > > > > > > > > > > > > > > > Signed-off-by: Xiao Zhang <xiao.zh...@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