qfi field is 8 bits which represent single bit for PPP (paging Policy Presence) single bit for RQI (Reflective QoS Indicator) and 6 bits for qfi (QoS Flow Identifier) This update the doxygen format and the mask for qfi to properly identify the full 8 bits of the field. note: changing the default mask would cause different patterns generated by testpmd. Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") Cc: ying.a.wang@intel.com Cc: stable@dpdk.org Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> --- doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- lib/librte_ethdev/rte_flow.h | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst index f59eb8a27d..dd39c4c3c2 100644 --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any. - ``gtp_psc``: match GTP PDU extension header with type 0x85. - ``pdu_type {unsigned}``: PDU type. - - ``qfi {unsigned}``: QoS flow identifier. + + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. - ``pppoes``, ``pppoed``: match PPPoE header. diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h index 669e677e91..79106e0246 100644 --- a/lib/librte_ethdev/rte_flow.h +++ b/lib/librte_ethdev/rte_flow.h @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = { */ struct rte_flow_item_gtp_psc { uint8_t pdu_type; /**< PDU type. */ - uint8_t qfi; /**< QoS flow identifier. */ + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ }; /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ #ifndef __cplusplus static const struct rte_flow_item_gtp_psc rte_flow_item_gtp_psc_mask = { - .qfi = 0x3f, + .qfi = 0xff, }; #endif -- 2.17.1
On 3/23/2021 12:11 PM, Raslan Darawsheh wrote: > qfi field is 8 bits which represent single bit for > PPP (paging Policy Presence) single bit for RQI > (Reflective QoS Indicator) and 6 bits for qfi > (QoS Flow Identifier) Can you please put a reference for above information? > > This update the doxygen format and the mask for qfi > to properly identify the full 8 bits of the field. > > note: changing the default mask would cause different > patterns generated by testpmd. > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > Cc: ying.a.wang@intel.com > Cc: stable@dpdk.org > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > --- > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > lib/librte_ethdev/rte_flow.h | 4 ++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index f59eb8a27d..dd39c4c3c2 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any. > - ``gtp_psc``: match GTP PDU extension header with type 0x85. > > - ``pdu_type {unsigned}``: PDU type. > - - ``qfi {unsigned}``: QoS flow identifier. > + > + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > > - ``pppoes``, ``pppoed``: match PPPoE header. > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > index 669e677e91..79106e0246 100644 > --- a/lib/librte_ethdev/rte_flow.h > +++ b/lib/librte_ethdev/rte_flow.h > @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = { > */ > struct rte_flow_item_gtp_psc { > uint8_t pdu_type; /**< PDU type. */ > - uint8_t qfi; /**< QoS flow identifier. */ > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > }; By design requirement, rte_flow_item_* should start with the relevant protocol header. There is already a deprecation notice for using protocol header directly in the rte_flow_item* [1] and Adrew/Ivan already fixed a few of them [2]. Your patch is not directly related on this, but since you are touching to the flow_item, would you mind create a protocol struct for it first, and use it in the "struct rte_flow_item_gtp_psc"? So the protocol related update can be done in the protocol header, instead of rte_flow struct. [1] https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v21.02#n99 [2] https://git.dpdk.org/next/dpdk-next-net/commit/?id=4a061a7bd70bfa023de23e8e654e
Hi > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit > Subject: Re: [dpdk-dev] [PATCH] ethdev: update qfi definition > > On 3/23/2021 12:11 PM, Raslan Darawsheh wrote: > > qfi field is 8 bits which represent single bit for > > PPP (paging Policy Presence) single bit for RQI > > (Reflective QoS Indicator) and 6 bits for qfi > > (QoS Flow Identifier) > > Can you please put a reference for above information? > > > > This update the doxygen format and the mask for qfi > > to properly identify the full 8 bits of the field. > > > > note: changing the default mask would cause different > > patterns generated by testpmd. > > > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > > Cc: ying.a.wang@intel.com > > Cc: stable@dpdk.org > > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > --- > > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > > lib/librte_ethdev/rte_flow.h | 4 ++-- > > 2 files changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > index f59eb8a27d..dd39c4c3c2 100644 > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their > attributes, if any. > > - ``gtp_psc``: match GTP PDU extension header with type 0x85. > > > > - ``pdu_type {unsigned}``: PDU type. > > - - ``qfi {unsigned}``: QoS flow identifier. > > + > > + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > > > > - ``pppoes``, ``pppoed``: match PPPoE header. > > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > > index 669e677e91..79106e0246 100644 > > --- a/lib/librte_ethdev/rte_flow.h > > +++ b/lib/librte_ethdev/rte_flow.h > > @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta > rte_flow_item_meta_mask = { > > */ > > struct rte_flow_item_gtp_psc { > > uint8_t pdu_type; /**< PDU type. */ > > - uint8_t qfi; /**< QoS flow identifier. */ > > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > > }; > > By design requirement, rte_flow_item_* should start with the relevant protocol > header. > > There is already a deprecation notice for using protocol header directly in the > rte_flow_item* [1] and Adrew/Ivan already fixed a few of them [2]. > > Your patch is not directly related on this, but since you are touching to the > flow_item, would you mind create a protocol struct for it first, and use it in > the "struct rte_flow_item_gtp_psc"? > So the protocol related update can be done in the protocol header, instead of > rte_flow struct. > +1 > > [1] > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.dpdk. > org%2Fdpdk%2Ftree%2Fdoc%2Fguides%2Frel_notes%2Fdeprecation.rst%3Fh%3 > Dv21.02%23n99&data=04%7C01%7Corika%40nvidia.com%7C6391a4c0640 > f4592b70b08d8f058e322%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0 > %7C637523611870497932%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdat > a=vU%2F5oO47zb9erTnZL1pl9j0nHCKzea3NJgOeo1FTW0Q%3D&reserved= > 0 > > [2] > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.dpdk. > org%2Fnext%2Fdpdk-next- > net%2Fcommit%2F%3Fid%3D4a061a7bd70bfa023de23e8e654e&data=04% > 7C01%7Corika%40nvidia.com%7C6391a4c0640f4592b70b08d8f058e322%7C43 > 083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637523611870497932%7CU > nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik > 1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WurlQ5VSLqFGVthfRj363xZsC > No3xJuvxNQCFVcxdkk%3D&reserved=0
Hi, > -----Original Message----- > From: Ori Kam <orika@nvidia.com> > Sent: Monday, March 29, 2021 11:53 AM > To: Ferruh Yigit <ferruh.yigit@intel.com>; Raslan Darawsheh > <rasland@nvidia.com>; dev@dpdk.org; Andrew Rybchenko > <andrew.rybchenko@oktetlabs.ru>; Ivan Malov <ivan.malov@oktetlabs.ru> > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin > <shirik@nvidia.com>; ying.a.wang@intel.com; stable@dpdk.org; NBU- > Contact-Thomas Monjalon <thomas@monjalon.net>; Olivier Matz > <olivier.matz@6wind.com> > Subject: RE: [dpdk-dev] [PATCH] ethdev: update qfi definition > > Hi > > > -----Original Message----- > > From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit > > Subject: Re: [dpdk-dev] [PATCH] ethdev: update qfi definition > > > > On 3/23/2021 12:11 PM, Raslan Darawsheh wrote: > > > qfi field is 8 bits which represent single bit for > > > PPP (paging Policy Presence) single bit for RQI > > > (Reflective QoS Indicator) and 6 bits for qfi > > > (QoS Flow Identifier) > > > > Can you please put a reference for above information? > > > Sure, will send in V2, > > > This update the doxygen format and the mask for qfi > > > to properly identify the full 8 bits of the field. > > > > > > note: changing the default mask would cause different > > > patterns generated by testpmd. > > > > > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > > > Cc: ying.a.wang@intel.com > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > > --- > > > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > > > lib/librte_ethdev/rte_flow.h | 4 ++-- > > > 2 files changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > > index f59eb8a27d..dd39c4c3c2 100644 > > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and > their > > attributes, if any. > > > - ``gtp_psc``: match GTP PDU extension header with type 0x85. > > > > > > - ``pdu_type {unsigned}``: PDU type. > > > - - ``qfi {unsigned}``: QoS flow identifier. > > > + > > > + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > > > > > > - ``pppoes``, ``pppoed``: match PPPoE header. > > > > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > > > index 669e677e91..79106e0246 100644 > > > --- a/lib/librte_ethdev/rte_flow.h > > > +++ b/lib/librte_ethdev/rte_flow.h > > > @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta > > rte_flow_item_meta_mask = { > > > */ > > > struct rte_flow_item_gtp_psc { > > > uint8_t pdu_type; /**< PDU type. */ > > > - uint8_t qfi; /**< QoS flow identifier. */ > > > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > > > }; > > > > By design requirement, rte_flow_item_* should start with the relevant > protocol > > header. > > > > There is already a deprecation notice for using protocol header directly in > the > > rte_flow_item* [1] and Adrew/Ivan already fixed a few of them [2]. > > > > Your patch is not directly related on this, but since you are touching to the > > flow_item, would you mind create a protocol struct for it first, and use it in > > the "struct rte_flow_item_gtp_psc"? > > So the protocol related update can be done in the protocol header, instead > of > > rte_flow struct. > > > +1 Sure, I can create the new protocol and use it , and will send in V2. But, wouldn't it cause any ABI breakage ? > > > > > [1] > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.d > pdk. > > > org%2Fdpdk%2Ftree%2Fdoc%2Fguides%2Frel_notes%2Fdeprecation.rst%3F > h%3 > > > Dv21.02%23n99&data=04%7C01%7Corika%40nvidia.com%7C6391a4c064 > 0 > > > f4592b70b08d8f058e322%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0 > > > %7C637523611870497932%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > w > > > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sda > t > > > a=vU%2F5oO47zb9erTnZL1pl9j0nHCKzea3NJgOeo1FTW0Q%3D&reserve > d= > > 0 > > > > [2] > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.d > pdk. > > org%2Fnext%2Fdpdk-next- > > > net%2Fcommit%2F%3Fid%3D4a061a7bd70bfa023de23e8e654e&data=0 > 4% > > > 7C01%7Corika%40nvidia.com%7C6391a4c0640f4592b70b08d8f058e322%7C43 > > > 083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637523611870497932%7CU > > > nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI > 6Ik > > > 1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WurlQ5VSLqFGVthfRj363xZs > C > > No3xJuvxNQCFVcxdkk%3D&reserved=0
On 3/29/2021 10:06 AM, Raslan Darawsheh wrote: > Hi, > >> -----Original Message----- >> From: Ori Kam <orika@nvidia.com> >> Sent: Monday, March 29, 2021 11:53 AM >> To: Ferruh Yigit <ferruh.yigit@intel.com>; Raslan Darawsheh >> <rasland@nvidia.com>; dev@dpdk.org; Andrew Rybchenko >> <andrew.rybchenko@oktetlabs.ru>; Ivan Malov <ivan.malov@oktetlabs.ru> >> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin >> <shirik@nvidia.com>; ying.a.wang@intel.com; stable@dpdk.org; NBU- >> Contact-Thomas Monjalon <thomas@monjalon.net>; Olivier Matz >> <olivier.matz@6wind.com> >> Subject: RE: [dpdk-dev] [PATCH] ethdev: update qfi definition >> >> Hi >> >>> -----Original Message----- >>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit >>> Subject: Re: [dpdk-dev] [PATCH] ethdev: update qfi definition >>> >>> On 3/23/2021 12:11 PM, Raslan Darawsheh wrote: >>>> qfi field is 8 bits which represent single bit for >>>> PPP (paging Policy Presence) single bit for RQI >>>> (Reflective QoS Indicator) and 6 bits for qfi >>>> (QoS Flow Identifier) >>> >>> Can you please put a reference for above information? >>>> > Sure, will send in V2, > >>>> This update the doxygen format and the mask for qfi >>>> to properly identify the full 8 bits of the field. >>>> >>>> note: changing the default mask would cause different >>>> patterns generated by testpmd. >>>> >>>> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") >>>> Cc: ying.a.wang@intel.com >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> >>>> --- >>>> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- >>>> lib/librte_ethdev/rte_flow.h | 4 ++-- >>>> 2 files changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>> index f59eb8a27d..dd39c4c3c2 100644 >>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and >> their >>> attributes, if any. >>>> - ``gtp_psc``: match GTP PDU extension header with type 0x85. >>>> >>>> - ``pdu_type {unsigned}``: PDU type. >>>> - - ``qfi {unsigned}``: QoS flow identifier. >>>> + >>>> + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. >>>> >>>> - ``pppoes``, ``pppoed``: match PPPoE header. >>>> >>>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h >>>> index 669e677e91..79106e0246 100644 >>>> --- a/lib/librte_ethdev/rte_flow.h >>>> +++ b/lib/librte_ethdev/rte_flow.h >>>> @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta >>> rte_flow_item_meta_mask = { >>>> */ >>>> struct rte_flow_item_gtp_psc { >>>> uint8_t pdu_type; /**< PDU type. */ >>>> - uint8_t qfi; /**< QoS flow identifier. */ >>>> + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ >>>> }; >>> >>> By design requirement, rte_flow_item_* should start with the relevant >> protocol >>> header. >>> >>> There is already a deprecation notice for using protocol header directly in >> the >>> rte_flow_item* [1] and Adrew/Ivan already fixed a few of them [2]. >>> >>> Your patch is not directly related on this, but since you are touching to the >>> flow_item, would you mind create a protocol struct for it first, and use it in >>> the "struct rte_flow_item_gtp_psc"? >>> So the protocol related update can be done in the protocol header, instead >> of >>> rte_flow struct. >>> >> +1 > Sure, I can create the new protocol and use it , and will send in V2. But, wouldn't it cause any ABI breakage ? For the protocol header, it won't be problem since it is a new struct. For the flow item struct, it may be, that is why need to use a union for now, please check what Adrew/Ivan did. >> >>> >>> [1] >>> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.d >> pdk. >>> >> org%2Fdpdk%2Ftree%2Fdoc%2Fguides%2Frel_notes%2Fdeprecation.rst%3F >> h%3 >>> >> Dv21.02%23n99&data=04%7C01%7Corika%40nvidia.com%7C6391a4c064 >> 0 >>> >> f4592b70b08d8f058e322%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0 >>> >> %7C637523611870497932%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA >> w >>> >> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sda >> t >>> >> a=vU%2F5oO47zb9erTnZL1pl9j0nHCKzea3NJgOeo1FTW0Q%3D&reserve >> d= >>> 0 >>> >>> [2] >>> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.d >> pdk. >>> org%2Fnext%2Fdpdk-next- >>> >> net%2Fcommit%2F%3Fid%3D4a061a7bd70bfa023de23e8e654e&data=0 >> 4% >>> >> 7C01%7Corika%40nvidia.com%7C6391a4c0640f4592b70b08d8f058e322%7C43 >>> >> 083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637523611870497932%7CU >>> >> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI >> 6Ik >>> >> 1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WurlQ5VSLqFGVthfRj363xZs >> C >>> No3xJuvxNQCFVcxdkk%3D&reserved=0 >
This is fixing the gtp psc support to match the RFC 38415-g30 v2: introduce new header definition for gtp psc update commit msg for rte flow item change. Raslan Darawsheh (2): ethdev: add new ext hdr for gtp psc ethdev: update qfi definition doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 +- lib/librte_ethdev/rte_flow.h | 18 +++++++++-- lib/librte_net/rte_gtp.h | 34 +++++++++++++++++++++ 3 files changed, 51 insertions(+), 4 deletions(-) -- 2.29.0
Define new rte header for gtp PDU session container based on RFC 38415-g30 Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> --- lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h index 6a6f9b238d..8af48ea1ec 100644 --- a/lib/librte_net/rte_gtp.h +++ b/lib/librte_net/rte_gtp.h @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { uint8_t next_ext; /**< Next Extension Header Type. */ } __rte_packed; +/** + * Optional extention for GTP with next_ext set to 0x85 + * defined based on RFC 38415-g30. + */ +__extension__ +struct rte_gtp_psc { + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ + uint8_t type:4; /**< PDU type */ + uint8_t qmp:1; /**< Qos Monitoring Packet */ + union { + struct { + uint8_t snp:1; /**< Sequence number presence */ + uint8_t spare_dl1:2; /**< spare down link bits */ + }; + struct { + uint8_t dl_delay_ind:1; /**< dl delay result presence */ + uint8_t ul_delay_ind:1; /**< ul delay result presence */ + uint8_t snp_ul1:1; /**< Sequence number presence ul */ + }; + }; + union { + struct { + uint8_t ppp:1; /**< Paging policy presence */ + uint8_t rqi:1; /**< Reflective Qos Indicator */ + }; + struct { + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ + uint8_t spare_ul2:1; /**< spare up link bits */ + }; + }; + uint8_t qfi:6; /**< Qos Flow Identifier */ + uint8_t data[0]; /**< data feilds */ +} __rte_packed; + /** GTP header length */ #define RTE_ETHER_GTP_HLEN \ (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr)) -- 2.29.0
qfi field is 8 bits which represent single bit for PPP (paging Policy Presence) single bit for RQI (Reflective QoS Indicator) and 6 bits for qfi (QoS Flow Identifier) based on RFC 38415-g30 This update the doxygen format and the mask for qfi to properly identify the full 8 bits of the field. note: changing the default mask would cause different patterns generated by testpmd. Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") Cc: ying.a.wang@intel.com Cc: stable@dpdk.org Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> --- doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- lib/librte_ethdev/rte_flow.h | 18 +++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst index f59eb8a27d..dd39c4c3c2 100644 --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any. - ``gtp_psc``: match GTP PDU extension header with type 0x85. - ``pdu_type {unsigned}``: PDU type. - - ``qfi {unsigned}``: QoS flow identifier. + + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. - ``pppoes``, ``pppoed``: match PPPoE header. diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h index 6cc57136ac..1eb9711707 100644 --- a/lib/librte_ethdev/rte_flow.h +++ b/lib/librte_ethdev/rte_flow.h @@ -20,6 +20,7 @@ #include <rte_arp.h> #include <rte_common.h> #include <rte_ether.h> +#include <rte_gtp.h> #include <rte_icmp.h> #include <rte_ip.h> #include <rte_sctp.h> @@ -1421,16 +1422,27 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = { * * Matches a GTP PDU extension header with type 0x85. */ +RTE_STD_C11 struct rte_flow_item_gtp_psc { - uint8_t pdu_type; /**< PDU type. */ - uint8_t qfi; /**< QoS flow identifier. */ + union { + struct { + /* + * These fields are retained for compatibility. + * Please switch to the new header field below. + */ + uint8_t pdu_type; /**< PDU type. */ + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ + + }; + struct rte_gtp_psc gtp_psc; + }; }; /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ #ifndef __cplusplus static const struct rte_flow_item_gtp_psc rte_flow_item_gtp_psc_mask = { - .qfi = 0x3f, + .qfi = 0xff, }; #endif -- 2.29.0
This is fixing the gtp psc support to match the RFC 38415-g30 v2: introduce new header definition for gtp psc update commit msg for rte flow item change. v3: fixed typo in comment Cc relevant people. Raslan Darawsheh (2): ethdev: add new ext hdr for gtp psc ethdev: update qfi definition doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 +- lib/librte_ethdev/rte_flow.h | 18 +++++++++-- lib/librte_net/rte_gtp.h | 34 +++++++++++++++++++++ 3 files changed, 51 insertions(+), 4 deletions(-) -- 2.29.0
Define new rte header for gtp PDU session container based on RFC 38415-g30 Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> --- lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h index 6a6f9b238d..bfaae26535 100644 --- a/lib/librte_net/rte_gtp.h +++ b/lib/librte_net/rte_gtp.h @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { uint8_t next_ext; /**< Next Extension Header Type. */ } __rte_packed; +/** + * Optional extension for GTP with next_ext set to 0x85 + * defined based on RFC 38415-g30. + */ +__extension__ +struct rte_gtp_psc { + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ + uint8_t type:4; /**< PDU type */ + uint8_t qmp:1; /**< Qos Monitoring Packet */ + union { + struct { + uint8_t snp:1; /**< Sequence number presence */ + uint8_t spare_dl1:2; /**< spare down link bits */ + }; + struct { + uint8_t dl_delay_ind:1; /**< dl delay result presence */ + uint8_t ul_delay_ind:1; /**< ul delay result presence */ + uint8_t snp_ul1:1; /**< Sequence number presence ul */ + }; + }; + union { + struct { + uint8_t ppp:1; /**< Paging policy presence */ + uint8_t rqi:1; /**< Reflective Qos Indicator */ + }; + struct { + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ + uint8_t spare_ul2:1; /**< spare up link bits */ + }; + }; + uint8_t qfi:6; /**< Qos Flow Identifier */ + uint8_t data[0]; /**< data feilds */ +} __rte_packed; + /** GTP header length */ #define RTE_ETHER_GTP_HLEN \ (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr)) -- 2.29.0
qfi field is 8 bits which represent single bit for PPP (paging Policy Presence) single bit for RQI (Reflective QoS Indicator) and 6 bits for qfi (QoS Flow Identifier) based on RFC 38415-g30 This update the doxygen format and the mask for qfi to properly identify the full 8 bits of the field. note: changing the default mask would cause different patterns generated by testpmd. Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") Cc: ying.a.wang@intel.com Cc: stable@dpdk.org Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> --- doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- lib/librte_ethdev/rte_flow.h | 18 +++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst index f59eb8a27d..dd39c4c3c2 100644 --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any. - ``gtp_psc``: match GTP PDU extension header with type 0x85. - ``pdu_type {unsigned}``: PDU type. - - ``qfi {unsigned}``: QoS flow identifier. + + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. - ``pppoes``, ``pppoed``: match PPPoE header. diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h index 6cc57136ac..1eb9711707 100644 --- a/lib/librte_ethdev/rte_flow.h +++ b/lib/librte_ethdev/rte_flow.h @@ -20,6 +20,7 @@ #include <rte_arp.h> #include <rte_common.h> #include <rte_ether.h> +#include <rte_gtp.h> #include <rte_icmp.h> #include <rte_ip.h> #include <rte_sctp.h> @@ -1421,16 +1422,27 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = { * * Matches a GTP PDU extension header with type 0x85. */ +RTE_STD_C11 struct rte_flow_item_gtp_psc { - uint8_t pdu_type; /**< PDU type. */ - uint8_t qfi; /**< QoS flow identifier. */ + union { + struct { + /* + * These fields are retained for compatibility. + * Please switch to the new header field below. + */ + uint8_t pdu_type; /**< PDU type. */ + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ + + }; + struct rte_gtp_psc gtp_psc; + }; }; /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ #ifndef __cplusplus static const struct rte_flow_item_gtp_psc rte_flow_item_gtp_psc_mask = { - .qfi = 0x3f, + .qfi = 0xff, }; #endif -- 2.29.0
On 3/30/2021 9:00 AM, Raslan Darawsheh wrote: > Define new rte header for gtp PDU session container > based on RFC 38415-g30 > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > --- > lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h > index 6a6f9b238d..bfaae26535 100644 > --- a/lib/librte_net/rte_gtp.h > +++ b/lib/librte_net/rte_gtp.h > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { > uint8_t next_ext; /**< Next Extension Header Type. */ > } __rte_packed; > > +/** > + * Optional extension for GTP with next_ext set to 0x85 > + * defined based on RFC 38415-g30. > + */ > +__extension__ > +struct rte_gtp_psc { general consensus in other protocols seems having '_hdr' suffix in the struct name, I can see this is extension but do you think does it make to add suffix? > + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ > + uint8_t type:4; /**< PDU type */ > + uint8_t qmp:1; /**< Qos Monitoring Packet */ > + union { > + struct { > + uint8_t snp:1; /**< Sequence number presence */ > + uint8_t spare_dl1:2; /**< spare down link bits */ > + }; > + struct { > + uint8_t dl_delay_ind:1; /**< dl delay result presence */ > + uint8_t ul_delay_ind:1; /**< ul delay result presence */ > + uint8_t snp_ul1:1; /**< Sequence number presence ul */ > + }; > + }; > + union { > + struct { > + uint8_t ppp:1; /**< Paging policy presence */ > + uint8_t rqi:1; /**< Reflective Qos Indicator */ > + }; > + struct { > + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ > + uint8_t spare_ul2:1; /**< spare up link bits */ > + }; > + }; > + uint8_t qfi:6; /**< Qos Flow Identifier */ > + uint8_t data[0]; /**< data feilds */ > +} __rte_packed; > + > /** GTP header length */ > #define RTE_ETHER_GTP_HLEN \ > (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr)) >
On 3/30/2021 9:00 AM, Raslan Darawsheh wrote: > qfi field is 8 bits which represent single bit for > PPP (paging Policy Presence) single bit for RQI > (Reflective QoS Indicator) and 6 bits for qfi > (QoS Flow Identifier) based on RFC 38415-g30 > > This update the doxygen format and the mask for qfi > to properly identify the full 8 bits of the field. > > note: changing the default mask would cause different > patterns generated by testpmd. > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > Cc: ying.a.wang@intel.com > Cc: stable@dpdk.org > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > --- > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > lib/librte_ethdev/rte_flow.h | 18 +++++++++++++++--- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index f59eb8a27d..dd39c4c3c2 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any. > - ``gtp_psc``: match GTP PDU extension header with type 0x85. > > - ``pdu_type {unsigned}``: PDU type. > - - ``qfi {unsigned}``: QoS flow identifier. > + > + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > > - ``pppoes``, ``pppoed``: match PPPoE header. > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > index 6cc57136ac..1eb9711707 100644 > --- a/lib/librte_ethdev/rte_flow.h > +++ b/lib/librte_ethdev/rte_flow.h > @@ -20,6 +20,7 @@ > #include <rte_arp.h> > #include <rte_common.h> > #include <rte_ether.h> > +#include <rte_gtp.h> > #include <rte_icmp.h> > #include <rte_ip.h> > #include <rte_sctp.h> > @@ -1421,16 +1422,27 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = { > * > * Matches a GTP PDU extension header with type 0x85. > */ > +RTE_STD_C11 > struct rte_flow_item_gtp_psc { > - uint8_t pdu_type; /**< PDU type. */ > - uint8_t qfi; /**< QoS flow identifier. */ > + union { > + struct { > + /* > + * These fields are retained for compatibility. > + * Please switch to the new header field below. > + */ > + uint8_t pdu_type; /**< PDU type. */ > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > + > + }; > + struct rte_gtp_psc gtp_psc; Again for consistency, what do you think to rename the variable as 'hdr'? > + }; > }; > > /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ > #ifndef __cplusplus > static const struct rte_flow_item_gtp_psc > rte_flow_item_gtp_psc_mask = { > - .qfi = 0x3f, > + .qfi = 0xff, Since the protocol header is the preferred way, (individual fields may be deprecated in the future), can you please switch to new field, like: .gtp_psc.qfi = 0xff, > }; > #endif > >
> -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Thursday, April 1, 2021 7:51 PM > To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org > Cc: Ori Kam <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru; > ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; olivier.matz@6wind.com; > Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin <shirik@nvidia.com> > Subject: Re: [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc > > On 3/30/2021 9:00 AM, Raslan Darawsheh wrote: > > Define new rte header for gtp PDU session container > > based on RFC 38415-g30 > > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > --- > > lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h > > index 6a6f9b238d..bfaae26535 100644 > > --- a/lib/librte_net/rte_gtp.h > > +++ b/lib/librte_net/rte_gtp.h > > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { > > uint8_t next_ext; /**< Next Extension Header Type. */ > > } __rte_packed; > > > > +/** > > + * Optional extension for GTP with next_ext set to 0x85 > > + * defined based on RFC 38415-g30. > > + */ > > +__extension__ > > +struct rte_gtp_psc { > > general consensus in other protocols seems having '_hdr' suffix in the struct > name, I can see this is extension but do you think does it make to add suffix? Yes sure, will send in a new version. > > > + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ > > + uint8_t type:4; /**< PDU type */ > > + uint8_t qmp:1; /**< Qos Monitoring Packet */ > > + union { > > + struct { > > + uint8_t snp:1; /**< Sequence number presence */ > > + uint8_t spare_dl1:2; /**< spare down link bits */ > > + }; > > + struct { > > + uint8_t dl_delay_ind:1; /**< dl delay result presence > */ > > + uint8_t ul_delay_ind:1; /**< ul delay result presence > */ > > + uint8_t snp_ul1:1; /**< Sequence number presence > ul */ > > + }; > > + }; > > + union { > > + struct { > > + uint8_t ppp:1; /**< Paging policy presence */ > > + uint8_t rqi:1; /**< Reflective Qos Indicator */ > > + }; > > + struct { > > + uint8_t n_delay_ind:1; /**< N3/N9 delay result > presence */ > > + uint8_t spare_ul2:1; /**< spare up link bits */ > > + }; > > + }; > > + uint8_t qfi:6; /**< Qos Flow Identifier */ > > + uint8_t data[0]; /**< data feilds */ > > +} __rte_packed; > > + > > /** GTP header length */ > > #define RTE_ETHER_GTP_HLEN \ > > (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr)) > > Kindest regards Raslan Darawsheh
Hi Ferruh, > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Thursday, April 1, 2021 7:54 PM > To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org > Cc: Ori Kam <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru; > ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; olivier.matz@6wind.com; > Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin <shirik@nvidia.com>; > stable@dpdk.org > Subject: Re: [PATCH v3 2/2] ethdev: update qfi definition > > On 3/30/2021 9:00 AM, Raslan Darawsheh wrote: > > qfi field is 8 bits which represent single bit for > > PPP (paging Policy Presence) single bit for RQI > > (Reflective QoS Indicator) and 6 bits for qfi > > (QoS Flow Identifier) based on RFC 38415-g30 > > > > This update the doxygen format and the mask for qfi > > to properly identify the full 8 bits of the field. > > > > note: changing the default mask would cause different > > patterns generated by testpmd. > > > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > > Cc: ying.a.wang@intel.com > > Cc: stable@dpdk.org > > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > --- > > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > > lib/librte_ethdev/rte_flow.h | 18 +++++++++++++++--- > > 2 files changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > index f59eb8a27d..dd39c4c3c2 100644 > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and > their attributes, if any. > > - ``gtp_psc``: match GTP PDU extension header with type 0x85. > > > > - ``pdu_type {unsigned}``: PDU type. > > - - ``qfi {unsigned}``: QoS flow identifier. > > + > > + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > > > > - ``pppoes``, ``pppoed``: match PPPoE header. > > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > > index 6cc57136ac..1eb9711707 100644 > > --- a/lib/librte_ethdev/rte_flow.h > > +++ b/lib/librte_ethdev/rte_flow.h > > @@ -20,6 +20,7 @@ > > #include <rte_arp.h> > > #include <rte_common.h> > > #include <rte_ether.h> > > +#include <rte_gtp.h> > > #include <rte_icmp.h> > > #include <rte_ip.h> > > #include <rte_sctp.h> > > @@ -1421,16 +1422,27 @@ static const struct rte_flow_item_meta > rte_flow_item_meta_mask = { > > * > > * Matches a GTP PDU extension header with type 0x85. > > */ > > +RTE_STD_C11 > > struct rte_flow_item_gtp_psc { > > - uint8_t pdu_type; /**< PDU type. */ > > - uint8_t qfi; /**< QoS flow identifier. */ > > + union { > > + struct { > > + /* > > + * These fields are retained for compatibility. > > + * Please switch to the new header field below. > > + */ > > + uint8_t pdu_type; /**< PDU type. */ > > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > > + > > + }; > > + struct rte_gtp_psc gtp_psc; > > Again for consistency, what do you think to rename the variable as 'hdr'? Will do, > > > + }; > > }; > > > > /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ > > #ifndef __cplusplus > > static const struct rte_flow_item_gtp_psc > > rte_flow_item_gtp_psc_mask = { > > - .qfi = 0x3f, > > + .qfi = 0xff, > > Since the protocol header is the preferred way, (individual fields may be > deprecated in the future), can you please switch to new field, like: Yes, will do it accordingly > .gtp_psc.qfi = 0xff, > > > }; > > #endif > > > >
This is fixing the gtp psc support to match the RFC 38415-g30 v2: introduce new header definition for gtp psc update commit msg for rte flow item change. v3: fixed typo in comment Cc relevant people. v4: update hdr definition to have hdr suffix. update variable name to be hdr in the gtp_psc item. update default max to use the new added hdr. Raslan Darawsheh (2): ethdev: add new ext hdr for gtp psc ethdev: update qfi definition doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 +- lib/librte_ethdev/rte_flow.h | 20 ++++++++++-- lib/librte_net/rte_gtp.h | 34 +++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) -- 2.29.0
Define new rte header for gtp PDU session container based on RFC 38415-g30 Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> --- lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h index 6a6f9b238d..088b0b5a53 100644 --- a/lib/librte_net/rte_gtp.h +++ b/lib/librte_net/rte_gtp.h @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { uint8_t next_ext; /**< Next Extension Header Type. */ } __rte_packed; +/** + * Optional extension for GTP with next_ext set to 0x85 + * defined based on RFC 38415-g30. + */ +__extension__ +struct rte_gtp_psc_hdr { + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ + uint8_t type:4; /**< PDU type */ + uint8_t qmp:1; /**< Qos Monitoring Packet */ + union { + struct { + uint8_t snp:1; /**< Sequence number presence */ + uint8_t spare_dl1:2; /**< spare down link bits */ + }; + struct { + uint8_t dl_delay_ind:1; /**< dl delay result presence */ + uint8_t ul_delay_ind:1; /**< ul delay result presence */ + uint8_t snp_ul1:1; /**< Sequence number presence ul */ + }; + }; + union { + struct { + uint8_t ppp:1; /**< Paging policy presence */ + uint8_t rqi:1; /**< Reflective Qos Indicator */ + }; + struct { + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ + uint8_t spare_ul2:1; /**< spare up link bits */ + }; + }; + uint8_t qfi:6; /**< Qos Flow Identifier */ + uint8_t data[0]; /**< data feilds */ +} __rte_packed; + /** GTP header length */ #define RTE_ETHER_GTP_HLEN \ (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr)) -- 2.29.0
qfi field is 8 bits which represent single bit for PPP (paging Policy Presence) single bit for RQI (Reflective QoS Indicator) and 6 bits for qfi (QoS Flow Identifier) based on RFC 38415-g30 This update the doxygen format and the mask for qfi to properly identify the full 8 bits of the field. note: changing the default mask would cause different patterns generated by testpmd. Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") Cc: ying.a.wang@intel.com Cc: stable@dpdk.org Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> --- doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- lib/librte_ethdev/rte_flow.h | 20 +++++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst index f59eb8a27d..dd39c4c3c2 100644 --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any. - ``gtp_psc``: match GTP PDU extension header with type 0x85. - ``pdu_type {unsigned}``: PDU type. - - ``qfi {unsigned}``: QoS flow identifier. + + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. - ``pppoes``, ``pppoed``: match PPPoE header. diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h index 6cc57136ac..20b4389429 100644 --- a/lib/librte_ethdev/rte_flow.h +++ b/lib/librte_ethdev/rte_flow.h @@ -20,6 +20,7 @@ #include <rte_arp.h> #include <rte_common.h> #include <rte_ether.h> +#include <rte_gtp.h> #include <rte_icmp.h> #include <rte_ip.h> #include <rte_sctp.h> @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = { * * Matches a GTP PDU extension header with type 0x85. */ +RTE_STD_C11 struct rte_flow_item_gtp_psc { - uint8_t pdu_type; /**< PDU type. */ - uint8_t qfi; /**< QoS flow identifier. */ + union { + struct { + /* + * These fields are retained for compatibility. + * Please switch to the new header field below. + */ + uint8_t pdu_type; /**< PDU type. */ + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ + + }; + struct rte_gtp_psc_hdr hdr; + }; }; /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ #ifndef __cplusplus static const struct rte_flow_item_gtp_psc rte_flow_item_gtp_psc_mask = { - .qfi = 0x3f, + .hdr.ppp = 1, + .hdr.rqi = 1, + .hdr.qfi = 0x3f, }; #endif -- 2.29.0
On 4/4/2021 8:45 AM, Raslan Darawsheh wrote: > qfi field is 8 bits which represent single bit for > PPP (paging Policy Presence) single bit for RQI > (Reflective QoS Indicator) and 6 bits for qfi > (QoS Flow Identifier) based on RFC 38415-g30 > > This update the doxygen format and the mask for qfi > to properly identify the full 8 bits of the field. > > note: changing the default mask would cause different > patterns generated by testpmd. > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > Cc: ying.a.wang@intel.com > Cc: stable@dpdk.org > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > --- > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > lib/librte_ethdev/rte_flow.h | 20 +++++++++++++++++--- > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index f59eb8a27d..dd39c4c3c2 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any. > - ``gtp_psc``: match GTP PDU extension header with type 0x85. > > - ``pdu_type {unsigned}``: PDU type. > - - ``qfi {unsigned}``: QoS flow identifier. > + > + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > > - ``pppoes``, ``pppoed``: match PPPoE header. > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > index 6cc57136ac..20b4389429 100644 > --- a/lib/librte_ethdev/rte_flow.h > +++ b/lib/librte_ethdev/rte_flow.h > @@ -20,6 +20,7 @@ > #include <rte_arp.h> > #include <rte_common.h> > #include <rte_ether.h> > +#include <rte_gtp.h> > #include <rte_icmp.h> > #include <rte_ip.h> > #include <rte_sctp.h> > @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = { > * > * Matches a GTP PDU extension header with type 0x85. > */ > +RTE_STD_C11 > struct rte_flow_item_gtp_psc { > - uint8_t pdu_type; /**< PDU type. */ > - uint8_t qfi; /**< QoS flow identifier. */ > + union { > + struct { > + /* > + * These fields are retained for compatibility. > + * Please switch to the new header field below. > + */ > + uint8_t pdu_type; /**< PDU type. */ > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > + > + }; > + struct rte_gtp_psc_hdr hdr; > + }; > }; This will change the struct size even with union, since old version is missing all fields from protocol header. Struct size will go from 2 --> 5 bytes [1]. Since this is public struct, we can't change its size. @Ori, Andrew, Do you have a suggestion for next step? - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation notice for "struct rte_flow_item_gtp_psc" to say it will use "struct rte_gtp_psc_hdr" on 21.11. - And for this release use the Raslan's first version: - uint8_t qfi; /**< QoS flow identifier. */ + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ Does it make sense? Any comments? [1] struct rte_gtp_psc_hdr { uint8_t ext_hdr_len; /* 0 1 */ uint8_t type:4; /* 1: 0 1 */ uint8_t qmp:1; /* 1: 4 1 */ /* XXX 3 bits hole, try to pack */ union { struct { uint8_t snp:1; /* 2: 0 1 */ uint8_t spare_dl1:2; /* 2: 1 1 */ }; /* 2 1 */ struct { uint8_t dl_delay_ind:1; /* 2: 0 1 */ uint8_t ul_delay_ind:1; /* 2: 1 1 */ uint8_t snp_ul1:1; /* 2: 2 1 */ }; /* 2 1 */ }; /* 2 1 */ union { struct { uint8_t ppp:1; /* 3: 0 1 */ uint8_t rqi:1; /* 3: 1 1 */ }; /* 3 1 */ struct { uint8_t n_delay_ind:1; /* 3: 0 1 */ uint8_t spare_ul2:1; /* 3: 1 1 */ }; /* 3 1 */ }; /* 3 1 */ uint8_t qfi:6; /* 4: 0 1 */ /* XXX 2 bits hole, try to pack */ uint8_t data[]; /* 5 0 */ /* size: 5, cachelines: 1, members: 7 */ /* sum members: 3 */ /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits */ /* last cacheline: 5 bytes */ }; > > /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ > #ifndef __cplusplus > static const struct rte_flow_item_gtp_psc > rte_flow_item_gtp_psc_mask = { > - .qfi = 0x3f, > + .hdr.ppp = 1, > + .hdr.rqi = 1, > + .hdr.qfi = 0x3f, > }; > #endif > >
Hi Raslan, On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote: > Define new rte header for gtp PDU session container > based on RFC 38415-g30 Do you have a link to this RFC? > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > --- > lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h > index 6a6f9b238d..088b0b5a53 100644 > --- a/lib/librte_net/rte_gtp.h > +++ b/lib/librte_net/rte_gtp.h > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { > uint8_t next_ext; /**< Next Extension Header Type. */ > } __rte_packed; > > +/** > + * Optional extension for GTP with next_ext set to 0x85 > + * defined based on RFC 38415-g30. > + */ > +__extension__ > +struct rte_gtp_psc_hdr { > + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ > + uint8_t type:4; /**< PDU type */ > + uint8_t qmp:1; /**< Qos Monitoring Packet */ > + union { > + struct { > + uint8_t snp:1; /**< Sequence number presence */ > + uint8_t spare_dl1:2; /**< spare down link bits */ > + }; > + struct { > + uint8_t dl_delay_ind:1; /**< dl delay result presence */ > + uint8_t ul_delay_ind:1; /**< ul delay result presence */ > + uint8_t snp_ul1:1; /**< Sequence number presence ul */ > + }; > + }; > + union { > + struct { > + uint8_t ppp:1; /**< Paging policy presence */ > + uint8_t rqi:1; /**< Reflective Qos Indicator */ > + }; > + struct { > + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ > + uint8_t spare_ul2:1; /**< spare up link bits */ > + }; > + }; > + uint8_t qfi:6; /**< Qos Flow Identifier */ > + uint8_t data[0]; /**< data feilds */ > +} __rte_packed; With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected? It would help to see the specification to have a better idea of how to split, but a possible solution is to do something like this: struct rte_gtp_psc_generic_hdr { uint8_t ext_hdr_len; uint8_t type:4 uint8_t qmp:1; uint8_t pad:3; }; struct rte_gtp_psc_<name1>_hdr { uint8_t ext_hdr_len; uint8_t type:4 uint8_t qmp:1; uint8_t uint8_t snp:1; uint8_t spare_dl1:2; ... }; ... struct rte_gtp_psc_hdr { union { struct rte_gtp_psc_generic_hdr generic; struct rte_gtp_psc_<name1>_hdr <name1>; struct rte_gtp_psc_<name2>_hdr <name2>; }; }; Also, you need to take care about endianness. Regards, Olivier
Hi Olivier, > -----Original Message----- > From: Olivier Matz <olivier.matz@6wind.com> > Sent: Thursday, April 8, 2021 3:30 PM > To: Raslan Darawsheh <rasland@nvidia.com> > Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>; > andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru; > ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri > Kuzin <shirik@nvidia.com> > Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc > > Hi Raslan, > > On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote: > > Define new rte header for gtp PDU session container > > based on RFC 38415-g30 > > Do you have a link to this RFC? Yes sure, https://www.3gpp.org/ftp/Specs/archive/38_series/38.415/38415-g30.zip > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > --- > > lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h > > index 6a6f9b238d..088b0b5a53 100644 > > --- a/lib/librte_net/rte_gtp.h > > +++ b/lib/librte_net/rte_gtp.h > > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { > > uint8_t next_ext; /**< Next Extension Header Type. */ > > } __rte_packed; > > > > +/** > > + * Optional extension for GTP with next_ext set to 0x85 > > + * defined based on RFC 38415-g30. > > + */ > > +__extension__ > > +struct rte_gtp_psc_hdr { > > + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ > > + uint8_t type:4; /**< PDU type */ > > + uint8_t qmp:1; /**< Qos Monitoring Packet */ > > + union { > > + struct { > > + uint8_t snp:1; /**< Sequence number presence */ > > + uint8_t spare_dl1:2; /**< spare down link bits */ > > + }; > > + struct { > > + uint8_t dl_delay_ind:1; /**< dl delay result presence > */ > > + uint8_t ul_delay_ind:1; /**< ul delay result presence > */ > > + uint8_t snp_ul1:1; /**< Sequence number presence > ul */ > > + }; > > + }; > > + union { > > + struct { > > + uint8_t ppp:1; /**< Paging policy presence */ > > + uint8_t rqi:1; /**< Reflective Qos Indicator */ > > + }; > > + struct { > > + uint8_t n_delay_ind:1; /**< N3/N9 delay result > presence */ > > + uint8_t spare_ul2:1; /**< spare up link bits */ > > + }; > > + }; > > + uint8_t qfi:6; /**< Qos Flow Identifier */ > > + uint8_t data[0]; /**< data feilds */ > > +} __rte_packed; > > With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected? The data[0] is variable length data, I guess I should send another version to mention that in the comment maybe. The header size according to the spec should be 4 octets aligned in general. > > It would help to see the specification to have a better idea of how to Sure, I've just posted the link above, please let me know of any suggestion that you have, and I'll be glad to do accordingly. > split, but a possible solution is to do something like this: > > struct rte_gtp_psc_generic_hdr { > uint8_t ext_hdr_len; > uint8_t type:4 > uint8_t qmp:1; > uint8_t pad:3; > }; > > struct rte_gtp_psc_<name1>_hdr { > uint8_t ext_hdr_len; > uint8_t type:4 > uint8_t qmp:1; > uint8_t uint8_t snp:1; > uint8_t spare_dl1:2; > ... > }; > > ... > > struct rte_gtp_psc_hdr { > union { > struct rte_gtp_psc_generic_hdr generic; > struct rte_gtp_psc_<name1>_hdr <name1>; > struct rte_gtp_psc_<name2>_hdr <name2>; > }; > }; > > Also, you need to take care about endianness. > > > Regards, > Olivier Kindest regards Raslan Darawsheh
On 4/8/2021 1:37 PM, Raslan Darawsheh wrote: > Hi Olivier, > >> -----Original Message----- >> From: Olivier Matz <olivier.matz@6wind.com> >> Sent: Thursday, April 8, 2021 3:30 PM >> To: Raslan Darawsheh <rasland@nvidia.com> >> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>; >> andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru; >> ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri >> Kuzin <shirik@nvidia.com> >> Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc >> >> Hi Raslan, >> >> On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote: >>> Define new rte header for gtp PDU session container >>> based on RFC 38415-g30 >> >> Do you have a link to this RFC? > Yes sure, > https://www.3gpp.org/ftp/Specs/archive/38_series/38.415/38415-g30.zip > >> >>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> >>> --- >>> lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 34 insertions(+) >>> >>> diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h >>> index 6a6f9b238d..088b0b5a53 100644 >>> --- a/lib/librte_net/rte_gtp.h >>> +++ b/lib/librte_net/rte_gtp.h >>> @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { >>> uint8_t next_ext; /**< Next Extension Header Type. */ >>> } __rte_packed; >>> >>> +/** >>> + * Optional extension for GTP with next_ext set to 0x85 >>> + * defined based on RFC 38415-g30. >>> + */ >>> +__extension__ >>> +struct rte_gtp_psc_hdr { >>> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ >>> + uint8_t type:4; /**< PDU type */ >>> + uint8_t qmp:1; /**< Qos Monitoring Packet */ >>> + union { >>> + struct { >>> + uint8_t snp:1; /**< Sequence number presence */ >>> + uint8_t spare_dl1:2; /**< spare down link bits */ >>> + }; >>> + struct { >>> + uint8_t dl_delay_ind:1; /**< dl delay result presence >> */ >>> + uint8_t ul_delay_ind:1; /**< ul delay result presence >> */ >>> + uint8_t snp_ul1:1; /**< Sequence number presence >> ul */ >>> + }; >>> + }; >>> + union { >>> + struct { >>> + uint8_t ppp:1; /**< Paging policy presence */ >>> + uint8_t rqi:1; /**< Reflective Qos Indicator */ >>> + }; >>> + struct { >>> + uint8_t n_delay_ind:1; /**< N3/N9 delay result >> presence */ >>> + uint8_t spare_ul2:1; /**< spare up link bits */ >>> + }; >>> + }; >>> + uint8_t qfi:6; /**< Qos Flow Identifier */ >>> + uint8_t data[0]; /**< data feilds */ >>> +} __rte_packed; >> >> With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected? > The data[0] is variable length data, I guess I should send another version to mention that in the comment maybe. > The header size according to the spec should be 4 octets aligned in general. The struct is 5 btyes, this is not related to data[0], please check the pahole output: http://inbox.dpdk.org/dev/536631b9-c634-ddac-c154-91978ffc29a5@intel.com/ >> >> It would help to see the specification to have a better idea of how to > Sure, I've just posted the link above, please let me know of any suggestion that you have, and I'll be glad to do accordingly. > >> split, but a possible solution is to do something like this: >> >> struct rte_gtp_psc_generic_hdr { >> uint8_t ext_hdr_len; >> uint8_t type:4 >> uint8_t qmp:1; >> uint8_t pad:3; >> }; >> >> struct rte_gtp_psc_<name1>_hdr { >> uint8_t ext_hdr_len; >> uint8_t type:4 >> uint8_t qmp:1; >> uint8_t uint8_t snp:1; >> uint8_t spare_dl1:2; >> ... >> }; >> >> ... >> >> struct rte_gtp_psc_hdr { >> union { >> struct rte_gtp_psc_generic_hdr generic; >> struct rte_gtp_psc_<name1>_hdr <name1>; >> struct rte_gtp_psc_<name2>_hdr <name2>; >> }; >> }; >> >> Also, you need to take care about endianness. >> >> >> Regards, >> Olivier > Kindest regards > Raslan Darawsheh >
On Thu, Apr 08, 2021 at 12:37:27PM +0000, Raslan Darawsheh wrote: > Hi Olivier, > > > -----Original Message----- > > From: Olivier Matz <olivier.matz@6wind.com> > > Sent: Thursday, April 8, 2021 3:30 PM > > To: Raslan Darawsheh <rasland@nvidia.com> > > Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>; > > andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru; > > ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri > > Kuzin <shirik@nvidia.com> > > Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc > > > > Hi Raslan, > > > > On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote: > > > Define new rte header for gtp PDU session container > > > based on RFC 38415-g30 > > > > Do you have a link to this RFC? > Yes sure, > https://www.3gpp.org/ftp/Specs/archive/38_series/38.415/38415-g30.zip > > > > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > > --- > > > lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > > > > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h > > > index 6a6f9b238d..088b0b5a53 100644 > > > --- a/lib/librte_net/rte_gtp.h > > > +++ b/lib/librte_net/rte_gtp.h > > > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { > > > uint8_t next_ext; /**< Next Extension Header Type. */ > > > } __rte_packed; > > > > > > +/** > > > + * Optional extension for GTP with next_ext set to 0x85 > > > + * defined based on RFC 38415-g30. > > > + */ > > > +__extension__ > > > +struct rte_gtp_psc_hdr { > > > + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ > > > + uint8_t type:4; /**< PDU type */ > > > + uint8_t qmp:1; /**< Qos Monitoring Packet */ > > > + union { > > > + struct { > > > + uint8_t snp:1; /**< Sequence number presence */ > > > + uint8_t spare_dl1:2; /**< spare down link bits */ > > > + }; > > > + struct { > > > + uint8_t dl_delay_ind:1; /**< dl delay result presence > > */ > > > + uint8_t ul_delay_ind:1; /**< ul delay result presence > > */ > > > + uint8_t snp_ul1:1; /**< Sequence number presence > > ul */ > > > + }; > > > + }; > > > + union { > > > + struct { > > > + uint8_t ppp:1; /**< Paging policy presence */ > > > + uint8_t rqi:1; /**< Reflective Qos Indicator */ > > > + }; > > > + struct { > > > + uint8_t n_delay_ind:1; /**< N3/N9 delay result > > presence */ > > > + uint8_t spare_ul2:1; /**< spare up link bits */ > > > + }; > > > + }; > > > + uint8_t qfi:6; /**< Qos Flow Identifier */ > > > + uint8_t data[0]; /**< data feilds */ > > > +} __rte_packed; > > > > With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected? > The data[0] is variable length data, I guess I should send another version to mention that in the comment maybe. > The header size according to the spec should be 4 octets aligned in general. What I wanted to highlight is that using union of structs containing bitfields does not work as you expect: each union is at least 1 byte. This results in a structure that does not match the expected header. > > > > It would help to see the specification to have a better idea of how to > Sure, I've just posted the link above, please let me know of any suggestion that you have, and I'll be glad to do accordingly. > > > split, but a possible solution is to do something like this: > > > > struct rte_gtp_psc_generic_hdr { > > uint8_t ext_hdr_len; > > uint8_t type:4 > > uint8_t qmp:1; > > uint8_t pad:3; > > }; > > > > struct rte_gtp_psc_<name1>_hdr { > > uint8_t ext_hdr_len; > > uint8_t type:4 > > uint8_t qmp:1; > > uint8_t uint8_t snp:1; > > uint8_t spare_dl1:2; > > ... > > }; > > > > ... > > > > struct rte_gtp_psc_hdr { > > union { > > struct rte_gtp_psc_generic_hdr generic; > > struct rte_gtp_psc_<name1>_hdr <name1>; > > struct rte_gtp_psc_<name2>_hdr <name2>; > > }; > > }; From what I see in the documation, I think this approach should work. From afar, I suggest: struct rte_gtp_psc_generic_hdr { #if big endian uint8_t type:4 uint8_t qmp:1; uint8_t pad:3; #else uint8_t pad:3; uint8_t qmp:1; uint8_t type:4 #endif }; struct rte_gtp_psc_type0_hdr { #if big endian uint8_t type:4 uint8_t qmp:1; uint8_t snp:1; uint8_t spare:2; uint8_t ppp:1; ... #else uint8_t pad:3; uint8_t qmp:1; uint8_t type:4 uint8_t spare:2; uint8_t snp:1; ... #endif uint8_t data[0]; /* for variable fields */ }; struct rte_gtp_psc_type1_hdr { ... same for fixed fields of type1 uint8_t data[0]; /* for variable fields */ }; I don't see in the spec where is the reference to ext_hdr_len. Regards, Olivier
Hi and sorry for late response, > -----Original Message----- > From: Olivier Matz <olivier.matz@6wind.com> > Sent: Thursday, April 8, 2021 5:11 PM > To: Raslan Darawsheh <rasland@nvidia.com> > Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>; > andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru; > ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri > Kuzin <shirik@nvidia.com> > Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc > > On Thu, Apr 08, 2021 at 12:37:27PM +0000, Raslan Darawsheh wrote: > > Hi Olivier, > > > > > -----Original Message----- > > > From: Olivier Matz <olivier.matz@6wind.com> > > > Sent: Thursday, April 8, 2021 3:30 PM > > > To: Raslan Darawsheh <rasland@nvidia.com> > > > Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>; > > > andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru; > > > ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri > > > Kuzin <shirik@nvidia.com> > > > Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc > > > > > > Hi Raslan, > > > > > > On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote: > > > > Define new rte header for gtp PDU session container > > > > based on RFC 38415-g30 > > > > > > Do you have a link to this RFC? > > Yes sure, > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > w.3gpp.org%2Fftp%2FSpecs%2Farchive%2F38_series%2F38.415%2F38415- > g30.zip&data=04%7C01%7Crasland%40nvidia.com%7C5279dde172024bc > d1f6b08d8fa981668%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C6 > 37534878435191995%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD > AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata= > LuD7%2FvJgqJbZ78ncaM7UrpiLNPUt7zbPPfSMemyb2Y8%3D&reserved > =0 > > > > > > > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > > > --- > > > > lib/librte_net/rte_gtp.h | 34 > ++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 34 insertions(+) > > > > > > > > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h > > > > index 6a6f9b238d..088b0b5a53 100644 > > > > --- a/lib/librte_net/rte_gtp.h > > > > +++ b/lib/librte_net/rte_gtp.h > > > > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word { > > > > uint8_t next_ext; /**< Next Extension Header Type. */ > > > > } __rte_packed; > > > > > > > > +/** > > > > + * Optional extension for GTP with next_ext set to 0x85 > > > > + * defined based on RFC 38415-g30. > > > > + */ > > > > +__extension__ > > > > +struct rte_gtp_psc_hdr { > > > > + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ > > > > + uint8_t type:4; /**< PDU type */ > > > > + uint8_t qmp:1; /**< Qos Monitoring Packet */ > > > > + union { > > > > + struct { > > > > + uint8_t snp:1; /**< Sequence number presence */ > > > > + uint8_t spare_dl1:2; /**< spare down link bits */ > > > > + }; > > > > + struct { > > > > + uint8_t dl_delay_ind:1; /**< dl delay result presence > > > */ > > > > + uint8_t ul_delay_ind:1; /**< ul delay result presence > > > */ > > > > + uint8_t snp_ul1:1; /**< Sequence number presence > > > ul */ > > > > + }; > > > > + }; > > > > + union { > > > > + struct { > > > > + uint8_t ppp:1; /**< Paging policy presence */ > > > > + uint8_t rqi:1; /**< Reflective Qos Indicator */ > > > > + }; > > > > + struct { > > > > + uint8_t n_delay_ind:1; /**< N3/N9 delay result > > > presence */ > > > > + uint8_t spare_ul2:1; /**< spare up link bits */ > > > > + }; > > > > + }; > > > > + uint8_t qfi:6; /**< Qos Flow Identifier */ > > > > + uint8_t data[0]; /**< data feilds */ > > > > +} __rte_packed; > > > > > > With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected? > > The data[0] is variable length data, I guess I should send another version to > mention that in the comment maybe. > > The header size according to the spec should be 4 octets aligned in general. > > What I wanted to highlight is that using union of structs containing > bitfields does not work as you expect: each union is at least 1 byte. > This results in a structure that does not match the expected header. I see thanks for explaining. > > > > > > > It would help to see the specification to have a better idea of how to > > Sure, I've just posted the link above, please let me know of any suggestion > that you have, and I'll be glad to do accordingly. > > > > > split, but a possible solution is to do something like this: > > > > > > struct rte_gtp_psc_generic_hdr { > > > uint8_t ext_hdr_len; > > > uint8_t type:4 > > > uint8_t qmp:1; > > > uint8_t pad:3; > > > }; > > > > > > struct rte_gtp_psc_<name1>_hdr { > > > uint8_t ext_hdr_len; > > > uint8_t type:4 > > > uint8_t qmp:1; > > > uint8_t uint8_t snp:1; > > > uint8_t spare_dl1:2; > > > ... > > > }; > > > > > > ... > > > > > > struct rte_gtp_psc_hdr { > > > union { > > > struct rte_gtp_psc_generic_hdr generic; > > > struct rte_gtp_psc_<name1>_hdr <name1>; > > > struct rte_gtp_psc_<name2>_hdr <name2>; > > > }; > > > }; > > From what I see in the documation, I think this approach should > work. From afar, I suggest: > > struct rte_gtp_psc_generic_hdr { > #if big endian > uint8_t type:4 > uint8_t qmp:1; > uint8_t pad:3; > #else > uint8_t pad:3; > uint8_t qmp:1; > uint8_t type:4 > #endif > }; > > struct rte_gtp_psc_type0_hdr { > #if big endian > uint8_t type:4 > uint8_t qmp:1; > uint8_t snp:1; > uint8_t spare:2; > > uint8_t ppp:1; > ... > #else > uint8_t pad:3; > uint8_t qmp:1; > uint8_t type:4 > uint8_t spare:2; > uint8_t snp:1; > > ... > #endif > uint8_t data[0]; /* for variable fields */ > }; > > struct rte_gtp_psc_type1_hdr { > ... same for fixed fields of type1 > > > uint8_t data[0]; /* for variable fields */ > }; > Sure, will consider and do it this way in the next version. > I don't see in the spec where is the reference to ext_hdr_len. > The ext_hdr_len is part of the definition for all ext_hdrs see this link for more details: https://en.wikipedia.org/wiki/GPRS_Tunnelling_Protocol So, it's not mentioned in the doc. > Regards, > Olivier
Hi, > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Tuesday, April 6, 2021 7:10 PM > To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ori Kam > <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru > Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; > olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri > Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand > <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon > <thomas@monjalon.net> > Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition > > On 4/4/2021 8:45 AM, Raslan Darawsheh wrote: > > qfi field is 8 bits which represent single bit for > > PPP (paging Policy Presence) single bit for RQI > > (Reflective QoS Indicator) and 6 bits for qfi > > (QoS Flow Identifier) based on RFC 38415-g30 > > > > This update the doxygen format and the mask for qfi > > to properly identify the full 8 bits of the field. > > > > note: changing the default mask would cause different > > patterns generated by testpmd. > > > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > > Cc: ying.a.wang@intel.com > > Cc: stable@dpdk.org > > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > --- > > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > > lib/librte_ethdev/rte_flow.h | 20 +++++++++++++++++--- > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > index f59eb8a27d..dd39c4c3c2 100644 > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and > their attributes, if any. > > - ``gtp_psc``: match GTP PDU extension header with type 0x85. > > > > - ``pdu_type {unsigned}``: PDU type. > > - - ``qfi {unsigned}``: QoS flow identifier. > > + > > + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > > > > - ``pppoes``, ``pppoed``: match PPPoE header. > > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > > index 6cc57136ac..20b4389429 100644 > > --- a/lib/librte_ethdev/rte_flow.h > > +++ b/lib/librte_ethdev/rte_flow.h > > @@ -20,6 +20,7 @@ > > #include <rte_arp.h> > > #include <rte_common.h> > > #include <rte_ether.h> > > +#include <rte_gtp.h> > > #include <rte_icmp.h> > > #include <rte_ip.h> > > #include <rte_sctp.h> > > @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta > rte_flow_item_meta_mask = { > > * > > * Matches a GTP PDU extension header with type 0x85. > > */ > > +RTE_STD_C11 > > struct rte_flow_item_gtp_psc { > > - uint8_t pdu_type; /**< PDU type. */ > > - uint8_t qfi; /**< QoS flow identifier. */ > > + union { > > + struct { > > + /* > > + * These fields are retained for compatibility. > > + * Please switch to the new header field below. > > + */ > > + uint8_t pdu_type; /**< PDU type. */ > > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > > + > > + }; > > + struct rte_gtp_psc_hdr hdr; > > + }; > > }; > > This will change the struct size even with union, since old version is missing > all fields from protocol header. Struct size will go from 2 --> 5 bytes [1]. > > Since this is public struct, we can't change its size. > > @Ori, Andrew, > > Do you have a suggestion for next step? > > - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation notice > for "struct rte_flow_item_gtp_psc" to say it will use "struct > rte_gtp_psc_hdr" > on 21.11. > > - And for this release use the Raslan's first version: > - uint8_t qfi; /**< QoS flow identifier. */ > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > > > Does it make sense? Any comments? > @Ori Kam, @andrew.rybchenko@oktetlabs.ru, This is a kind remainder of this patch, > > > [1] > struct rte_gtp_psc_hdr { > uint8_t ext_hdr_len; /* 0 1 */ > uint8_t type:4; /* 1: 0 1 */ > uint8_t qmp:1; /* 1: 4 1 */ > > /* XXX 3 bits hole, try to pack */ > > union { > struct { > uint8_t snp:1; /* 2: 0 1 */ > uint8_t spare_dl1:2; /* 2: 1 1 */ > }; /* 2 1 */ > struct { > uint8_t dl_delay_ind:1; /* 2: 0 1 */ > uint8_t ul_delay_ind:1; /* 2: 1 1 */ > uint8_t snp_ul1:1; /* 2: 2 1 */ > }; /* 2 1 */ > }; /* 2 1 */ > union { > struct { > uint8_t ppp:1; /* 3: 0 1 */ > uint8_t rqi:1; /* 3: 1 1 */ > }; /* 3 1 */ > struct { > uint8_t n_delay_ind:1; /* 3: 0 1 */ > uint8_t spare_ul2:1; /* 3: 1 1 */ > }; /* 3 1 */ > }; /* 3 1 */ > uint8_t qfi:6; /* 4: 0 1 */ > > /* XXX 2 bits hole, try to pack */ > > uint8_t data[]; /* 5 0 */ > > /* size: 5, cachelines: 1, members: 7 */ > /* sum members: 3 */ > /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits */ > /* last cacheline: 5 bytes */ > }; > > > > > /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ > > #ifndef __cplusplus > > static const struct rte_flow_item_gtp_psc > > rte_flow_item_gtp_psc_mask = { > > - .qfi = 0x3f, > > + .hdr.ppp = 1, > > + .hdr.rqi = 1, > > + .hdr.qfi = 0x3f, > > }; > > #endif > > > >
Hi Raslan, > -----Original Message----- > From: Raslan Darawsheh <rasland@nvidia.com> > Subject: RE: [PATCH v4 2/2] ethdev: update qfi definition > > Hi, > > > -----Original Message----- > > From: Ferruh Yigit <ferruh.yigit@intel.com> > > Sent: Tuesday, April 6, 2021 7:10 PM > > To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ori Kam > > <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru > > Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; > > olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri > > Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand > > <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon > > <thomas@monjalon.net> > > Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition > > > > On 4/4/2021 8:45 AM, Raslan Darawsheh wrote: > > > qfi field is 8 bits which represent single bit for > > > PPP (paging Policy Presence) single bit for RQI > > > (Reflective QoS Indicator) and 6 bits for qfi > > > (QoS Flow Identifier) based on RFC 38415-g30 > > > > > > This update the doxygen format and the mask for qfi > > > to properly identify the full 8 bits of the field. > > > > > > note: changing the default mask would cause different > > > patterns generated by testpmd. > > > > > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > > > Cc: ying.a.wang@intel.com > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > > > --- > > > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > > > lib/librte_ethdev/rte_flow.h | 20 +++++++++++++++++--- > > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > > index f59eb8a27d..dd39c4c3c2 100644 > > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and > > their attributes, if any. > > > - ``gtp_psc``: match GTP PDU extension header with type 0x85. > > > > > > - ``pdu_type {unsigned}``: PDU type. > > > - - ``qfi {unsigned}``: QoS flow identifier. > > > + > > > + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > > > > > > - ``pppoes``, ``pppoed``: match PPPoE header. > > > > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > > > index 6cc57136ac..20b4389429 100644 > > > --- a/lib/librte_ethdev/rte_flow.h > > > +++ b/lib/librte_ethdev/rte_flow.h > > > @@ -20,6 +20,7 @@ > > > #include <rte_arp.h> > > > #include <rte_common.h> > > > #include <rte_ether.h> > > > +#include <rte_gtp.h> > > > #include <rte_icmp.h> > > > #include <rte_ip.h> > > > #include <rte_sctp.h> > > > @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta > > rte_flow_item_meta_mask = { > > > * > > > * Matches a GTP PDU extension header with type 0x85. > > > */ > > > +RTE_STD_C11 > > > struct rte_flow_item_gtp_psc { > > > - uint8_t pdu_type; /**< PDU type. */ > > > - uint8_t qfi; /**< QoS flow identifier. */ > > > + union { > > > + struct { > > > + /* > > > + * These fields are retained for compatibility. > > > + * Please switch to the new header field below. > > > + */ > > > + uint8_t pdu_type; /**< PDU type. */ > > > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > > > + > > > + }; > > > + struct rte_gtp_psc_hdr hdr; > > > + }; > > > }; > > > > This will change the struct size even with union, since old version is missing > > all fields from protocol header. Struct size will go from 2 --> 5 bytes [1]. > > > > Since this is public struct, we can't change its size. > > > > @Ori, Andrew, > > > > Do you have a suggestion for next step? > > I think Ferruh is right, and I think that we should at this point just update the documentation. Sorry for the detour Just small comment about the original patch. I don’t think you should change the default mask since it may break existing apps. > > - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation notice > > for "struct rte_flow_item_gtp_psc" to say it will use "struct > > rte_gtp_psc_hdr" > > on 21.11. > > > > - And for this release use the Raslan's first version: > > - uint8_t qfi; /**< QoS flow identifier. */ > > + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > > > > > > Does it make sense? Any comments? > > > @Ori Kam, @andrew.rybchenko@oktetlabs.ru, > This is a kind remainder of this patch, > > > > > > > [1] > > struct rte_gtp_psc_hdr { > > uint8_t ext_hdr_len; /* 0 1 */ > > uint8_t type:4; /* 1: 0 1 */ > > uint8_t qmp:1; /* 1: 4 1 */ > > > > /* XXX 3 bits hole, try to pack */ > > > > union { > > struct { > > uint8_t snp:1; /* 2: 0 1 */ > > uint8_t spare_dl1:2; /* 2: 1 1 */ > > }; /* 2 1 */ > > struct { > > uint8_t dl_delay_ind:1; /* 2: 0 1 */ > > uint8_t ul_delay_ind:1; /* 2: 1 1 */ > > uint8_t snp_ul1:1; /* 2: 2 1 */ > > }; /* 2 1 */ > > }; /* 2 1 */ > > union { > > struct { > > uint8_t ppp:1; /* 3: 0 1 */ > > uint8_t rqi:1; /* 3: 1 1 */ > > }; /* 3 1 */ > > struct { > > uint8_t n_delay_ind:1; /* 3: 0 1 */ > > uint8_t spare_ul2:1; /* 3: 1 1 */ > > }; /* 3 1 */ > > }; /* 3 1 */ > > uint8_t qfi:6; /* 4: 0 1 */ > > > > /* XXX 2 bits hole, try to pack */ > > > > uint8_t data[]; /* 5 0 */ > > > > /* size: 5, cachelines: 1, members: 7 */ > > /* sum members: 3 */ > > /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits */ > > /* last cacheline: 5 bytes */ > > }; > > > > > > > > /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ > > > #ifndef __cplusplus > > > static const struct rte_flow_item_gtp_psc > > > rte_flow_item_gtp_psc_mask = { > > > - .qfi = 0x3f, > > > + .hdr.ppp = 1, > > > + .hdr.rqi = 1, > > > + .hdr.qfi = 0x3f, > > > }; > > > #endif > > > > > >
On 4/13/2021 10:24 AM, Ori Kam wrote: > Hi Raslan, > >> -----Original Message----- >> From: Raslan Darawsheh <rasland@nvidia.com> >> Subject: RE: [PATCH v4 2/2] ethdev: update qfi definition >> >> Hi, >> >>> -----Original Message----- >>> From: Ferruh Yigit <ferruh.yigit@intel.com> >>> Sent: Tuesday, April 6, 2021 7:10 PM >>> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ori Kam >>> <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru >>> Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; >>> olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri >>> Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand >>> <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon >>> <thomas@monjalon.net> >>> Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition >>> >>> On 4/4/2021 8:45 AM, Raslan Darawsheh wrote: >>>> qfi field is 8 bits which represent single bit for >>>> PPP (paging Policy Presence) single bit for RQI >>>> (Reflective QoS Indicator) and 6 bits for qfi >>>> (QoS Flow Identifier) based on RFC 38415-g30 >>>> >>>> This update the doxygen format and the mask for qfi >>>> to properly identify the full 8 bits of the field. >>>> >>>> note: changing the default mask would cause different >>>> patterns generated by testpmd. >>>> >>>> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") >>>> Cc: ying.a.wang@intel.com >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> >>>> --- >>>> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- >>>> lib/librte_ethdev/rte_flow.h | 20 +++++++++++++++++--- >>>> 2 files changed, 19 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>> index f59eb8a27d..dd39c4c3c2 100644 >>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and >>> their attributes, if any. >>>> - ``gtp_psc``: match GTP PDU extension header with type 0x85. >>>> >>>> - ``pdu_type {unsigned}``: PDU type. >>>> - - ``qfi {unsigned}``: QoS flow identifier. >>>> + >>>> + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. >>>> >>>> - ``pppoes``, ``pppoed``: match PPPoE header. >>>> >>>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h >>>> index 6cc57136ac..20b4389429 100644 >>>> --- a/lib/librte_ethdev/rte_flow.h >>>> +++ b/lib/librte_ethdev/rte_flow.h >>>> @@ -20,6 +20,7 @@ >>>> #include <rte_arp.h> >>>> #include <rte_common.h> >>>> #include <rte_ether.h> >>>> +#include <rte_gtp.h> >>>> #include <rte_icmp.h> >>>> #include <rte_ip.h> >>>> #include <rte_sctp.h> >>>> @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta >>> rte_flow_item_meta_mask = { >>>> * >>>> * Matches a GTP PDU extension header with type 0x85. >>>> */ >>>> +RTE_STD_C11 >>>> struct rte_flow_item_gtp_psc { >>>> - uint8_t pdu_type; /**< PDU type. */ >>>> - uint8_t qfi; /**< QoS flow identifier. */ >>>> + union { >>>> + struct { >>>> + /* >>>> + * These fields are retained for compatibility. >>>> + * Please switch to the new header field below. >>>> + */ >>>> + uint8_t pdu_type; /**< PDU type. */ >>>> + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ >>>> + >>>> + }; >>>> + struct rte_gtp_psc_hdr hdr; >>>> + }; >>>> }; >>> >>> This will change the struct size even with union, since old version is missing >>> all fields from protocol header. Struct size will go from 2 --> 5 bytes [1]. >>> >>> Since this is public struct, we can't change its size. >>> >>> @Ori, Andrew, >>> >>> Do you have a suggestion for next step? >>> > > I think Ferruh is right, and I think that we should at this point just update the documentation. > Sorry for the detour > Just small comment about the original patch. > I don’t think you should change the default mask since it may break existing apps. > I will continue with first patch [1], and will update the protocol reference in the commit log. Meanwhile adding 'struct rte_gtp_psc_hdr' (patch v4 1/2) work can continue separately. And when it is added we can send a deprecation notice to update 'struct rte_flow_item_gtp_psc' and finally update it on v21.11 to have 'struct rte_gtp_psc_hdr' in it. Makes sense? [1] https://patches.dpdk.org/project/dpdk/patch/20210323121134.19113-1-rasland@nvidia.com/ >>> - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation notice >>> for "struct rte_flow_item_gtp_psc" to say it will use "struct >>> rte_gtp_psc_hdr" >>> on 21.11. >>> >>> - And for this release use the Raslan's first version: >>> - uint8_t qfi; /**< QoS flow identifier. */ >>> + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ >>> >>> >>> Does it make sense? Any comments? >>> >> @Ori Kam, @andrew.rybchenko@oktetlabs.ru, >> This is a kind remainder of this patch, >> >>> >>> >>> [1] >>> struct rte_gtp_psc_hdr { >>> uint8_t ext_hdr_len; /* 0 1 */ >>> uint8_t type:4; /* 1: 0 1 */ >>> uint8_t qmp:1; /* 1: 4 1 */ >>> >>> /* XXX 3 bits hole, try to pack */ >>> >>> union { >>> struct { >>> uint8_t snp:1; /* 2: 0 1 */ >>> uint8_t spare_dl1:2; /* 2: 1 1 */ >>> }; /* 2 1 */ >>> struct { >>> uint8_t dl_delay_ind:1; /* 2: 0 1 */ >>> uint8_t ul_delay_ind:1; /* 2: 1 1 */ >>> uint8_t snp_ul1:1; /* 2: 2 1 */ >>> }; /* 2 1 */ >>> }; /* 2 1 */ >>> union { >>> struct { >>> uint8_t ppp:1; /* 3: 0 1 */ >>> uint8_t rqi:1; /* 3: 1 1 */ >>> }; /* 3 1 */ >>> struct { >>> uint8_t n_delay_ind:1; /* 3: 0 1 */ >>> uint8_t spare_ul2:1; /* 3: 1 1 */ >>> }; /* 3 1 */ >>> }; /* 3 1 */ >>> uint8_t qfi:6; /* 4: 0 1 */ >>> >>> /* XXX 2 bits hole, try to pack */ >>> >>> uint8_t data[]; /* 5 0 */ >>> >>> /* size: 5, cachelines: 1, members: 7 */ >>> /* sum members: 3 */ >>> /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits */ >>> /* last cacheline: 5 bytes */ >>> }; >>> >>>> >>>> /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ >>>> #ifndef __cplusplus >>>> static const struct rte_flow_item_gtp_psc >>>> rte_flow_item_gtp_psc_mask = { >>>> - .qfi = 0x3f, >>>> + .hdr.ppp = 1, >>>> + .hdr.rqi = 1, >>>> + .hdr.qfi = 0x3f, >>>> }; >>>> #endif >>>> >>>> >
On 3/23/2021 12:11 PM, Raslan Darawsheh wrote:
> qfi field is 8 bits which represent single bit for
> PPP (paging Policy Presence) single bit for RQI
> (Reflective QoS Indicator) and 6 bits for qfi
> (QoS Flow Identifier)
>
> This update the doxygen format and the mask for qfi
> to properly identify the full 8 bits of the field.
>
> note: changing the default mask would cause different
> patterns generated by testpmd.
>
> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> Cc: ying.a.wang@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Applied to dpdk-next-net/main, thanks.
Sorry for going through multiple versions, hopefully proper solution can be
merged after deprecation notice on v21.11.
Kindest regards, Raslan Darawsheh > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Wednesday, April 14, 2021 3:17 PM > To: Ori Kam <orika@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; > dev@dpdk.org; andrew.rybchenko@oktetlabs.ru > Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; > olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri > Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand > <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon > <thomas@monjalon.net> > Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition > > On 4/13/2021 10:24 AM, Ori Kam wrote: > > Hi Raslan, > > > >> -----Original Message----- > >> From: Raslan Darawsheh <rasland@nvidia.com> > >> Subject: RE: [PATCH v4 2/2] ethdev: update qfi definition > >> > >> Hi, > >> > >>> -----Original Message----- > >>> From: Ferruh Yigit <ferruh.yigit@intel.com> > >>> Sent: Tuesday, April 6, 2021 7:10 PM > >>> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ori Kam > >>> <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru > >>> Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; > >>> olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; > Shiri > >>> Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand > >>> <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon > >>> <thomas@monjalon.net> > >>> Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition > >>> > >>> On 4/4/2021 8:45 AM, Raslan Darawsheh wrote: > >>>> qfi field is 8 bits which represent single bit for > >>>> PPP (paging Policy Presence) single bit for RQI > >>>> (Reflective QoS Indicator) and 6 bits for qfi > >>>> (QoS Flow Identifier) based on RFC 38415-g30 > >>>> > >>>> This update the doxygen format and the mask for qfi > >>>> to properly identify the full 8 bits of the field. > >>>> > >>>> note: changing the default mask would cause different > >>>> patterns generated by testpmd. > >>>> > >>>> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API") > >>>> Cc: ying.a.wang@intel.com > >>>> Cc: stable@dpdk.org > >>>> > >>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> > >>>> --- > >>>> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > >>>> lib/librte_ethdev/rte_flow.h | 20 +++++++++++++++++--- > >>>> 2 files changed, 19 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > >>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > >>>> index f59eb8a27d..dd39c4c3c2 100644 > >>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > >>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > >>>> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and > >>> their attributes, if any. > >>>> - ``gtp_psc``: match GTP PDU extension header with type 0x85. > >>>> > >>>> - ``pdu_type {unsigned}``: PDU type. > >>>> - - ``qfi {unsigned}``: QoS flow identifier. > >>>> + > >>>> + - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier. > >>>> > >>>> - ``pppoes``, ``pppoed``: match PPPoE header. > >>>> > >>>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > >>>> index 6cc57136ac..20b4389429 100644 > >>>> --- a/lib/librte_ethdev/rte_flow.h > >>>> +++ b/lib/librte_ethdev/rte_flow.h > >>>> @@ -20,6 +20,7 @@ > >>>> #include <rte_arp.h> > >>>> #include <rte_common.h> > >>>> #include <rte_ether.h> > >>>> +#include <rte_gtp.h> > >>>> #include <rte_icmp.h> > >>>> #include <rte_ip.h> > >>>> #include <rte_sctp.h> > >>>> @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta > >>> rte_flow_item_meta_mask = { > >>>> * > >>>> * Matches a GTP PDU extension header with type 0x85. > >>>> */ > >>>> +RTE_STD_C11 > >>>> struct rte_flow_item_gtp_psc { > >>>> - uint8_t pdu_type; /**< PDU type. */ > >>>> - uint8_t qfi; /**< QoS flow identifier. */ > >>>> + union { > >>>> + struct { > >>>> + /* > >>>> + * These fields are retained for compatibility. > >>>> + * Please switch to the new header field below. > >>>> + */ > >>>> + uint8_t pdu_type; /**< PDU type. */ > >>>> + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > >>>> + > >>>> + }; > >>>> + struct rte_gtp_psc_hdr hdr; > >>>> + }; > >>>> }; > >>> > >>> This will change the struct size even with union, since old version is > missing > >>> all fields from protocol header. Struct size will go from 2 --> 5 bytes [1]. > >>> > >>> Since this is public struct, we can't change its size. > >>> > >>> @Ori, Andrew, > >>> > >>> Do you have a suggestion for next step? > >>> > > > > I think Ferruh is right, and I think that we should at this point just update > the documentation. > > Sorry for the detour > > Just small comment about the original patch. > > I don't think you should change the default mask since it may break existing > apps. > > > > I will continue with first patch [1], and will update the protocol reference in > the commit log. > > Meanwhile adding 'struct rte_gtp_psc_hdr' (patch v4 1/2) work can continue > separately. > And when it is added we can send a deprecation notice to update 'struct > rte_flow_item_gtp_psc' and finally update it on v21.11 to have 'struct > rte_gtp_psc_hdr' in it. Makes sense? > Yes, makes sense to me, I'll work in the meanwhile on providing the new hdrs definition . > [1] > https://patches.dpdk.org/project/dpdk/patch/20210323121134.19113-1- > rasland@nvidia.com/ > > >>> - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation > notice > >>> for "struct rte_flow_item_gtp_psc" to say it will use "struct > >>> rte_gtp_psc_hdr" > >>> on 21.11. > >>> > >>> - And for this release use the Raslan's first version: > >>> - uint8_t qfi; /**< QoS flow identifier. */ > >>> + uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */ > >>> > >>> > >>> Does it make sense? Any comments? > >>> > >> @Ori Kam, @andrew.rybchenko@oktetlabs.ru, > >> This is a kind remainder of this patch, > >> > >>> > >>> > >>> [1] > >>> struct rte_gtp_psc_hdr { > >>> uint8_t ext_hdr_len; /* 0 1 */ > >>> uint8_t type:4; /* 1: 0 1 */ > >>> uint8_t qmp:1; /* 1: 4 1 */ > >>> > >>> /* XXX 3 bits hole, try to pack */ > >>> > >>> union { > >>> struct { > >>> uint8_t snp:1; /* 2: 0 1 */ > >>> uint8_t spare_dl1:2; /* 2: 1 1 */ > >>> }; /* 2 1 */ > >>> struct { > >>> uint8_t dl_delay_ind:1; /* 2: 0 1 */ > >>> uint8_t ul_delay_ind:1; /* 2: 1 1 */ > >>> uint8_t snp_ul1:1; /* 2: 2 1 */ > >>> }; /* 2 1 */ > >>> }; /* 2 1 */ > >>> union { > >>> struct { > >>> uint8_t ppp:1; /* 3: 0 1 */ > >>> uint8_t rqi:1; /* 3: 1 1 */ > >>> }; /* 3 1 */ > >>> struct { > >>> uint8_t n_delay_ind:1; /* 3: 0 1 */ > >>> uint8_t spare_ul2:1; /* 3: 1 1 */ > >>> }; /* 3 1 */ > >>> }; /* 3 1 */ > >>> uint8_t qfi:6; /* 4: 0 1 */ > >>> > >>> /* XXX 2 bits hole, try to pack */ > >>> > >>> uint8_t data[]; /* 5 0 */ > >>> > >>> /* size: 5, cachelines: 1, members: 7 */ > >>> /* sum members: 3 */ > >>> /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits > */ > >>> /* last cacheline: 5 bytes */ > >>> }; > >>> > >>>> > >>>> /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */ > >>>> #ifndef __cplusplus > >>>> static const struct rte_flow_item_gtp_psc > >>>> rte_flow_item_gtp_psc_mask = { > >>>> - .qfi = 0x3f, > >>>> + .hdr.ppp = 1, > >>>> + .hdr.rqi = 1, > >>>> + .hdr.qfi = 0x3f, > >>>> }; > >>>> #endif > >>>> > >>>> > >
This is introducin a new hdr definition of gtp psc support to match the RFC 38415-g30 v2: introduce new header definition for gtp psc update commit msg for rte flow item change. v3: fixed typo in comment Cc relevant people. v4: update hdr definition to have hdr suffix. update variable name to be hdr in the gtp_psc item. update default max to use the new added hdr. v5: updated the hdr definition after code review. dropped the change for rte_flow item psc to avoid ABI breakage added hdr definition for type0 and type1 psc's Raslan Darawsheh (1): ethdev: add new ext hdr for gtp psc lib/net/rte_gtp.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) -- 2.25.1
Define new rte header for gtp PDU session container based on RFC 38415-g30 Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> --- lib/net/rte_gtp.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/lib/net/rte_gtp.h b/lib/net/rte_gtp.h index 6a6f9b238d..5a850a26e4 100644 --- a/lib/net/rte_gtp.h +++ b/lib/net/rte_gtp.h @@ -61,6 +61,84 @@ struct rte_gtp_hdr_ext_word { uint8_t next_ext; /**< Next Extension Header Type. */ } __rte_packed; +/** + * Optional extension for GTP with next_ext set to 0x85 + * defined based on RFC 38415-g30. + */ +__extension__ +struct rte_gtp_psc_generic_hdr { + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN + uint8_t type:4; /**< PDU type */ + uint8_t qmp:1; /**< Qos Monitoring Packet */ + uint8_t pad:3; /**< type specfic pad bits */ + uint8_t spare:2; /**< type specific spare bits */ + uint8_t qfi:6; /**< Qos Flow Identifier */ +#else + uint8_t qfi:6; /**< Qos Flow Identifier */ + uint8_t spare:2; /**< type specific spare bits */ + uint8_t pad:3; /**< type specfic pad bits */ + uint8_t qmp:1; /**< Qos Monitoring Packet */ + uint8_t type:4; /**< PDU type */ +#endif + uint8_t data[0]; /**< variable length data feilds */ +} __rte_packed; + +/** + * Optional extension for GTP with next_ext set to 0x85 + * type0 defined based on RFC 38415-g30 + */ +__extension__ +struct rte_gtp_psc_type0_hdr { + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN + uint8_t type:4; /**< PDU type */ + uint8_t qmp:1; /**< Qos Monitoring Packet */ + uint8_t snp:1; /**< Sequence number presence */ + uint8_t spare_dl1:2; /**< spare down link bits */ + uint8_t ppp:1; /**< Paging policy presence */ + uint8_t rqi:1; /**< Reflective Qos Indicator */ + uint8_t qfi:6; /**< Qos Flow Identifier */ +#else + uint8_t qfi:6; /**< Qos Flow Identifier */ + uint8_t rqi:1; /**< Reflective Qos Indicator */ + uint8_t ppp:1; /**< Paging policy presence */ + uint8_t spare_dl1:2; /**< spare down link bits */ + uint8_t snp:1; /**< Sequence number presence */ + uint8_t type:4; /**< PDU type */ +#endif + uint8_t data[0]; /**< variable length data feilds */ +} __rte_packed; + +/** + * Optional extension for GTP with next_ext set to 0x85 + * type1 defined based on RFC 38415-g30 + */ +__extension__ +struct rte_gtp_psc_type1_hdr { + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN + uint8_t type:4; /**< PDU type */ + uint8_t qmp:1; /**< Qos Monitoring Packet */ + uint8_t dl_delay_ind:1; /**< dl delay result presence */ + uint8_t ul_delay_ind:1; /**< ul delay result presence */ + uint8_t snp:1; /**< Sequence number presence ul */ + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ + uint8_t spare_ul2:1; /**< spare up link bits */ + uint8_t qfi:6; /**< Qos Flow Identifier */ +#else + uint8_t qfi:6; /**< Qos Flow Identifier */ + uint8_t spare_ul2:1; /**< spare up link bits */ + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ + uint8_t snp:1; /**< Sequence number presence ul */ + uint8_t ul_delay_ind:1; /**< ul delay result presence */ + uint8_t dl_delay_ind:1; /**< dl delay result presence */ + uint8_t qmp:1; /**< Qos Monitoring Packet */ + uint8_t type:4; /**< PDU type */ +#endif + uint8_t data[0]; /**< variable length data feilds */ +} __rte_packed; + /** GTP header length */ #define RTE_ETHER_GTP_HLEN \ (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr)) -- 2.25.1
On Thu, Apr 08, 2021 at 02:29:56PM +0200, Olivier Matz wrote:
> Hi Raslan,
>
> > +/**
> > + * Optional extension for GTP with next_ext set to 0x85
> > + * defined based on RFC 38415-g30.
> > + */
> > +__extension__
> > +struct rte_gtp_psc_hdr {
> > + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> > + uint8_t type:4; /**< PDU type */
> > + uint8_t qmp:1; /**< Qos Monitoring Packet */
would it be a lot ot ask to have the structure defined using standard C
instead of using compiler specific extensions?
On 4/29/2021 9:10 AM, Raslan Darawsheh wrote:
> Define new rte header for gtp PDU session container
> based on RFC 38415-g30
>
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
This patch has not urgent for this release, postponing it to next release.
On 4/29/21 7:29 PM, Tyler Retzlaff wrote:
> On Thu, Apr 08, 2021 at 02:29:56PM +0200, Olivier Matz wrote:
>> Hi Raslan,
>>
>>> +/**
>>> + * Optional extension for GTP with next_ext set to 0x85
>>> + * defined based on RFC 38415-g30.
>>> + */
>>> +__extension__
>>> +struct rte_gtp_psc_hdr {
>>> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
>>> + uint8_t type:4; /**< PDU type */
>>> + uint8_t qmp:1; /**< Qos Monitoring Packet */
>
> would it be a lot ot ask to have the structure defined using standard C
> instead of using compiler specific extensions?
>
It is a valid request, but I'm afraid bit fields are used
in many-many core DPDK libraries. So, change of the approach
or at least direction for a new code should be discussed and
approved by techboard.
On 4/29/21 11:10 AM, Raslan Darawsheh wrote:
> Define new rte header for gtp PDU session container
> based on RFC 38415-g30
>
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
> lib/net/rte_gtp.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/lib/net/rte_gtp.h b/lib/net/rte_gtp.h
> index 6a6f9b238d..5a850a26e4 100644
> --- a/lib/net/rte_gtp.h
> +++ b/lib/net/rte_gtp.h
> @@ -61,6 +61,84 @@ struct rte_gtp_hdr_ext_word {
> uint8_t next_ext; /**< Next Extension Header Type. */
> } __rte_packed;
>
> +/**
> + * Optional extension for GTP with next_ext set to 0x85
> + * defined based on RFC 38415-g30.
> + */
> +__extension__
> +struct rte_gtp_psc_generic_hdr {
> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> + uint8_t type:4; /**< PDU type */
> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> + uint8_t pad:3; /**< type specfic pad bits */
> + uint8_t spare:2; /**< type specific spare bits */
> + uint8_t qfi:6; /**< Qos Flow Identifier */
> +#else
> + uint8_t qfi:6; /**< Qos Flow Identifier */
> + uint8_t spare:2; /**< type specific spare bits */
> + uint8_t pad:3; /**< type specfic pad bits */
> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> + uint8_t type:4; /**< PDU type */
> +#endif
> + uint8_t data[0]; /**< variable length data feilds */
> +} __rte_packed;
> +
> +/**
> + * Optional extension for GTP with next_ext set to 0x85
> + * type0 defined based on RFC 38415-g30
> + */
> +__extension__
> +struct rte_gtp_psc_type0_hdr {
> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> + uint8_t type:4; /**< PDU type */
> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> + uint8_t snp:1; /**< Sequence number presence */
> + uint8_t spare_dl1:2; /**< spare down link bits */
> + uint8_t ppp:1; /**< Paging policy presence */
> + uint8_t rqi:1; /**< Reflective Qos Indicator */
> + uint8_t qfi:6; /**< Qos Flow Identifier */
> +#else
> + uint8_t qfi:6; /**< Qos Flow Identifier */
> + uint8_t rqi:1; /**< Reflective Qos Indicator */
> + uint8_t ppp:1; /**< Paging policy presence */
> + uint8_t spare_dl1:2; /**< spare down link bits */
> + uint8_t snp:1; /**< Sequence number presence */
> + uint8_t type:4; /**< PDU type */
> +#endif
> + uint8_t data[0]; /**< variable length data feilds */
> +} __rte_packed;
> +
> +/**
> + * Optional extension for GTP with next_ext set to 0x85
> + * type1 defined based on RFC 38415-g30
> + */
> +__extension__
> +struct rte_gtp_psc_type1_hdr {
> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> + uint8_t type:4; /**< PDU type */
> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> + uint8_t dl_delay_ind:1; /**< dl delay result presence */
> + uint8_t ul_delay_ind:1; /**< ul delay result presence */
> + uint8_t snp:1; /**< Sequence number presence ul */
> + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> + uint8_t spare_ul2:1; /**< spare up link bits */
> + uint8_t qfi:6; /**< Qos Flow Identifier */
> +#else
> + uint8_t qfi:6; /**< Qos Flow Identifier */
> + uint8_t spare_ul2:1; /**< spare up link bits */
> + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> + uint8_t snp:1; /**< Sequence number presence ul */
> + uint8_t ul_delay_ind:1; /**< ul delay result presence */
> + uint8_t dl_delay_ind:1; /**< dl delay result presence */
> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> + uint8_t type:4; /**< PDU type */
> +#endif
> + uint8_t data[0]; /**< variable length data feilds */
> +} __rte_packed;
> +
> /** GTP header length */
> #define RTE_ETHER_GTP_HLEN \
> (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
>
This way the structure is very hard to read.
May I ask to indent field comments as it is done in
other rte_net structures (e.g. rte_ipv4_hdr, rte_udp_hdr,
rte_geneve_hdr). Personally I don't care if it will be
indent by TABs or spaces, however, it looks like TABs are
used in more places.
On 6/8/21 3:17 PM, Andrew Rybchenko wrote: > On 4/29/21 11:10 AM, Raslan Darawsheh wrote: >> Define new rte header for gtp PDU session container >> based on RFC 38415-g30 >> >> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com> >> --- >> lib/net/rte_gtp.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 78 insertions(+) >> >> diff --git a/lib/net/rte_gtp.h b/lib/net/rte_gtp.h >> index 6a6f9b238d..5a850a26e4 100644 >> --- a/lib/net/rte_gtp.h >> +++ b/lib/net/rte_gtp.h >> @@ -61,6 +61,84 @@ struct rte_gtp_hdr_ext_word { >> uint8_t next_ext; /**< Next Extension Header Type. */ >> } __rte_packed; >> >> +/** >> + * Optional extension for GTP with next_ext set to 0x85 >> + * defined based on RFC 38415-g30. >> + */ >> +__extension__ >> +struct rte_gtp_psc_generic_hdr { >> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ >> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN >> + uint8_t type:4; /**< PDU type */ >> + uint8_t qmp:1; /**< Qos Monitoring Packet */ >> + uint8_t pad:3; /**< type specfic pad bits */ >> + uint8_t spare:2; /**< type specific spare bits */ >> + uint8_t qfi:6; /**< Qos Flow Identifier */ >> +#else >> + uint8_t qfi:6; /**< Qos Flow Identifier */ >> + uint8_t spare:2; /**< type specific spare bits */ >> + uint8_t pad:3; /**< type specfic pad bits */ >> + uint8_t qmp:1; /**< Qos Monitoring Packet */ >> + uint8_t type:4; /**< PDU type */ >> +#endif >> + uint8_t data[0]; /**< variable length data feilds */ >> +} __rte_packed; >> + >> +/** >> + * Optional extension for GTP with next_ext set to 0x85 >> + * type0 defined based on RFC 38415-g30 >> + */ >> +__extension__ >> +struct rte_gtp_psc_type0_hdr { >> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ >> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN >> + uint8_t type:4; /**< PDU type */ >> + uint8_t qmp:1; /**< Qos Monitoring Packet */ >> + uint8_t snp:1; /**< Sequence number presence */ >> + uint8_t spare_dl1:2; /**< spare down link bits */ >> + uint8_t ppp:1; /**< Paging policy presence */ >> + uint8_t rqi:1; /**< Reflective Qos Indicator */ >> + uint8_t qfi:6; /**< Qos Flow Identifier */ >> +#else >> + uint8_t qfi:6; /**< Qos Flow Identifier */ >> + uint8_t rqi:1; /**< Reflective Qos Indicator */ >> + uint8_t ppp:1; /**< Paging policy presence */ >> + uint8_t spare_dl1:2; /**< spare down link bits */ >> + uint8_t snp:1; /**< Sequence number presence */ >> + uint8_t type:4; /**< PDU type */ >> +#endif >> + uint8_t data[0]; /**< variable length data feilds */ >> +} __rte_packed; >> + >> +/** >> + * Optional extension for GTP with next_ext set to 0x85 >> + * type1 defined based on RFC 38415-g30 >> + */ >> +__extension__ >> +struct rte_gtp_psc_type1_hdr { >> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */ >> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN >> + uint8_t type:4; /**< PDU type */ >> + uint8_t qmp:1; /**< Qos Monitoring Packet */ >> + uint8_t dl_delay_ind:1; /**< dl delay result presence */ >> + uint8_t ul_delay_ind:1; /**< ul delay result presence */ >> + uint8_t snp:1; /**< Sequence number presence ul */ >> + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ >> + uint8_t spare_ul2:1; /**< spare up link bits */ >> + uint8_t qfi:6; /**< Qos Flow Identifier */ >> +#else >> + uint8_t qfi:6; /**< Qos Flow Identifier */ >> + uint8_t spare_ul2:1; /**< spare up link bits */ >> + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */ >> + uint8_t snp:1; /**< Sequence number presence ul */ >> + uint8_t ul_delay_ind:1; /**< ul delay result presence */ >> + uint8_t dl_delay_ind:1; /**< dl delay result presence */ >> + uint8_t qmp:1; /**< Qos Monitoring Packet */ >> + uint8_t type:4; /**< PDU type */ >> +#endif >> + uint8_t data[0]; /**< variable length data feilds */ >> +} __rte_packed; >> + >> /** GTP header length */ >> #define RTE_ETHER_GTP_HLEN \ >> (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr)) >> > > This way the structure is very hard to read. > May I ask to indent field comments as it is done in > other rte_net structures (e.g. rte_ipv4_hdr, rte_udp_hdr, > rte_geneve_hdr). Personally I don't care if it will be > indent by TABs or spaces, however, it looks like TABs are > used in more places. > Also there are spelling bugs [1]. [1] http://mails.dpdk.org/archives/test-report/2021-April/191649.html
Hi Andrew,
Thanks for your comments and review, I'll attempt to handle and fix in the next version.
Kindest regards,
Raslan Darawsheh
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, June 8, 2021 3:19 PM
> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org
> Cc: ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>;
> ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; olivier.matz@6wind.com;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin <shirik@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc
>
> On 6/8/21 3:17 PM, Andrew Rybchenko wrote:
> > On 4/29/21 11:10 AM, Raslan Darawsheh wrote:
> >> Define new rte header for gtp PDU session container based on RFC
> >> 38415-g30
> >>
> >> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> >> ---
> >> lib/net/rte_gtp.h | 78
> >> +++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 78 insertions(+)
> >>
> >> diff --git a/lib/net/rte_gtp.h b/lib/net/rte_gtp.h index
> >> 6a6f9b238d..5a850a26e4 100644
> >> --- a/lib/net/rte_gtp.h
> >> +++ b/lib/net/rte_gtp.h
> >> @@ -61,6 +61,84 @@ struct rte_gtp_hdr_ext_word {
> >> uint8_t next_ext; /**< Next Extension Header Type. */
> >> } __rte_packed;
> >>
> >> +/**
> >> + * Optional extension for GTP with next_ext set to 0x85
> >> + * defined based on RFC 38415-g30.
> >> + */
> >> +__extension__
> >> +struct rte_gtp_psc_generic_hdr {
> >> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes
> >> +*/ #if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >> + uint8_t type:4; /**< PDU type */
> >> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> + uint8_t pad:3; /**< type specfic pad bits */
> >> + uint8_t spare:2; /**< type specific spare bits */
> >> + uint8_t qfi:6; /**< Qos Flow Identifier */ #else
> >> + uint8_t qfi:6; /**< Qos Flow Identifier */
> >> + uint8_t spare:2; /**< type specific spare bits */
> >> + uint8_t pad:3; /**< type specfic pad bits */
> >> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> + uint8_t type:4; /**< PDU type */
> >> +#endif
> >> + uint8_t data[0]; /**< variable length data feilds */ }
> >> +__rte_packed;
> >> +
> >> +/**
> >> + * Optional extension for GTP with next_ext set to 0x85
> >> + * type0 defined based on RFC 38415-g30 */ __extension__ struct
> >> +rte_gtp_psc_type0_hdr {
> >> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes
> >> +*/ #if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >> + uint8_t type:4; /**< PDU type */
> >> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> + uint8_t snp:1; /**< Sequence number presence */
> >> + uint8_t spare_dl1:2; /**< spare down link bits */
> >> + uint8_t ppp:1; /**< Paging policy presence */
> >> + uint8_t rqi:1; /**< Reflective Qos Indicator */
> >> + uint8_t qfi:6; /**< Qos Flow Identifier */ #else
> >> + uint8_t qfi:6; /**< Qos Flow Identifier */
> >> + uint8_t rqi:1; /**< Reflective Qos Indicator */
> >> + uint8_t ppp:1; /**< Paging policy presence */
> >> + uint8_t spare_dl1:2; /**< spare down link bits */
> >> + uint8_t snp:1; /**< Sequence number presence */
> >> + uint8_t type:4; /**< PDU type */
> >> +#endif
> >> + uint8_t data[0]; /**< variable length data feilds */ }
> >> +__rte_packed;
> >> +
> >> +/**
> >> + * Optional extension for GTP with next_ext set to 0x85
> >> + * type1 defined based on RFC 38415-g30 */ __extension__ struct
> >> +rte_gtp_psc_type1_hdr {
> >> + uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes
> >> +*/ #if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >> + uint8_t type:4; /**< PDU type */
> >> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> + uint8_t dl_delay_ind:1; /**< dl delay result presence */
> >> + uint8_t ul_delay_ind:1; /**< ul delay result presence */
> >> + uint8_t snp:1; /**< Sequence number presence ul */
> >> + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> >> + uint8_t spare_ul2:1; /**< spare up link bits */
> >> + uint8_t qfi:6; /**< Qos Flow Identifier */ #else
> >> + uint8_t qfi:6; /**< Qos Flow Identifier */
> >> + uint8_t spare_ul2:1; /**< spare up link bits */
> >> + uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> >> + uint8_t snp:1; /**< Sequence number presence ul */
> >> + uint8_t ul_delay_ind:1; /**< ul delay result presence */
> >> + uint8_t dl_delay_ind:1; /**< dl delay result presence */
> >> + uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> + uint8_t type:4; /**< PDU type */
> >> +#endif
> >> + uint8_t data[0]; /**< variable length data feilds */ }
> >> +__rte_packed;
> >> +
> >> /** GTP header length */
> >> #define RTE_ETHER_GTP_HLEN \
> >> (sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
> >>
> >
> > This way the structure is very hard to read.
> > May I ask to indent field comments as it is done in other rte_net
> > structures (e.g. rte_ipv4_hdr, rte_udp_hdr, rte_geneve_hdr).
> > Personally I don't care if it will be indent by TABs or spaces,
> > however, it looks like TABs are used in more places.
> >
>
> Also there are spelling bugs [1].
>
> [1] http://mails.dpdk.org/archives/test-report/2021-April/191649.html