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

["Zhang, Xiao" <xiao.zhang@intel.com>]

> -----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