* [dpdk-dev] [RFC] net: make eCPRI header host network order @ 2020-11-27 19:09 Ferruh Yigit 2020-11-28 3:18 ` Bing Zhao 2023-06-14 21:57 ` Stephen Hemminger 0 siblings, 2 replies; 7+ messages in thread From: Ferruh Yigit @ 2020-11-27 19:09 UTC (permalink / raw) To: Olivier Matz; +Cc: Ferruh Yigit, dev, Haiyue Wang, Stephen Hemminger, Bing Zhao Other protocol structs are in the host byte order, having eCPRI in network byte order is insistent and error prone. Making eCPRI protocol header host byte order. Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> --- Cc: Stephen Hemminger <stephen@networkplumber.org> Cc: Bing Zhao <bingz@nvidia.com> Cc: Olivier Matz <olivier.matz@6wind.com> --- lib/librte_net/rte_ecpri.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/librte_net/rte_ecpri.h b/lib/librte_net/rte_ecpri.h index 1cbd6d813363..67bf9186ff6f 100644 --- a/lib/librte_net/rte_ecpri.h +++ b/lib/librte_net/rte_ecpri.h @@ -60,21 +60,20 @@ extern "C" { RTE_STD_C11 struct rte_ecpri_common_hdr { union { - rte_be32_t u32; /**< 4B common header in BE */ + uint32_t u32; /**< 4B common header in host byte order */ struct { #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN - uint32_t size:16; /**< Payload Size */ - uint32_t type:8; /**< Message Type */ uint32_t c:1; /**< Concatenation Indicator */ uint32_t res:3; /**< Reserved */ uint32_t revision:4; /**< Protocol Revision */ + uint32_t type:8; /**< Message Type */ #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN uint32_t revision:4; /**< Protocol Revision */ uint32_t res:3; /**< Reserved */ uint32_t c:1; /**< Concatenation Indicator */ uint32_t type:8; /**< Message Type */ - uint32_t size:16; /**< Payload Size */ #endif + uint32_t size:16; /**< Payload Size */ }; }; }; -- 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [RFC] net: make eCPRI header host network order 2020-11-27 19:09 [dpdk-dev] [RFC] net: make eCPRI header host network order Ferruh Yigit @ 2020-11-28 3:18 ` Bing Zhao 2020-11-28 3:55 ` Wang, Haiyue 2020-11-28 5:31 ` Wang, Haiyue 2023-06-14 21:57 ` Stephen Hemminger 1 sibling, 2 replies; 7+ messages in thread From: Bing Zhao @ 2020-11-28 3:18 UTC (permalink / raw) To: Ferruh Yigit, Olivier Matz; +Cc: dev, Haiyue Wang, Stephen Hemminger Hi Ferruh & Haiyue, Have you checked other headers? Like: rte_ipv4_hdr rte_ipv6_hdr rte_tcp_hdr ... Also [ITEM_UDP_SRC] = { .name = "src", .help = "UDP source port", .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), item_param), .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_udp, hdr.src_port)), }, [ITEM_UDP_DST] = { .name = "dst", .help = "UDP destination port", .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), item_param), .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_udp, hdr.dst_port)), }, Or did I get sth. wrong? BR. Bing > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Saturday, November 28, 2020 3:09 AM > To: Olivier Matz <olivier.matz@6wind.com> > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Haiyue Wang > <haiyue.wang@intel.com>; Stephen Hemminger > <stephen@networkplumber.org>; Bing Zhao <bingz@nvidia.com> > Subject: [RFC] net: make eCPRI header host network order > > External email: Use caution opening links or attachments > > > Other protocol structs are in the host byte order, having eCPRI in > network byte order is insistent and error prone. > > Making eCPRI protocol header host byte order. > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> > --- > Cc: Stephen Hemminger <stephen@networkplumber.org> > Cc: Bing Zhao <bingz@nvidia.com> > Cc: Olivier Matz <olivier.matz@6wind.com> > --- > lib/librte_net/rte_ecpri.h | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_net/rte_ecpri.h b/lib/librte_net/rte_ecpri.h > index 1cbd6d813363..67bf9186ff6f 100644 > --- a/lib/librte_net/rte_ecpri.h > +++ b/lib/librte_net/rte_ecpri.h > @@ -60,21 +60,20 @@ extern "C" { > RTE_STD_C11 > struct rte_ecpri_common_hdr { > union { > - rte_be32_t u32; /**< 4B common > header in BE */ > + uint32_t u32; /**< 4B common > header in host byte order */ > struct { > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > - uint32_t size:16; /**< Payload Size */ > - uint32_t type:8; /**< Message Type */ > uint32_t c:1; /**< Concatenation > Indicator */ > uint32_t res:3; /**< Reserved */ > uint32_t revision:4; /**< Protocol > Revision */ > + uint32_t type:8; /**< Message Type */ > #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > uint32_t revision:4; /**< Protocol > Revision */ > uint32_t res:3; /**< Reserved */ > uint32_t c:1; /**< Concatenation > Indicator */ > uint32_t type:8; /**< Message Type */ > - uint32_t size:16; /**< Payload Size */ > #endif > + uint32_t size:16; /**< Payload Size */ > }; > }; > }; > -- > 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [RFC] net: make eCPRI header host network order 2020-11-28 3:18 ` Bing Zhao @ 2020-11-28 3:55 ` Wang, Haiyue 2020-11-28 5:31 ` Wang, Haiyue 1 sibling, 0 replies; 7+ messages in thread From: Wang, Haiyue @ 2020-11-28 3:55 UTC (permalink / raw) To: Bing Zhao, Yigit, Ferruh, Olivier Matz; +Cc: dev, Stephen Hemminger Hi Bing, These definition just use the basic data type, not use the bit field method. BR, Haiyue > -----Original Message----- > From: Bing Zhao <bingz@nvidia.com> > Sent: Saturday, November 28, 2020 11:18 > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Olivier Matz <olivier.matz@6wind.com> > Cc: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Stephen Hemminger <stephen@networkplumber.org> > Subject: RE: [RFC] net: make eCPRI header host network order > > Hi Ferruh & Haiyue, > Have you checked other headers? Like: > rte_ipv4_hdr > rte_ipv6_hdr > rte_tcp_hdr > ... > > Also > [ITEM_UDP_SRC] = { > .name = "src", > .help = "UDP source port", > .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), item_param), > .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_udp, > hdr.src_port)), > }, > [ITEM_UDP_DST] = { > .name = "dst", > .help = "UDP destination port", > .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), item_param), > .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_udp, > hdr.dst_port)), > }, > > Or did I get sth. wrong? > > BR. Bing > > > -----Original Message----- > > From: Ferruh Yigit <ferruh.yigit@intel.com> > > Sent: Saturday, November 28, 2020 3:09 AM > > To: Olivier Matz <olivier.matz@6wind.com> > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Haiyue Wang > > <haiyue.wang@intel.com>; Stephen Hemminger > > <stephen@networkplumber.org>; Bing Zhao <bingz@nvidia.com> > > Subject: [RFC] net: make eCPRI header host network order > > > > External email: Use caution opening links or attachments > > > > > > Other protocol structs are in the host byte order, having eCPRI in > > network byte order is insistent and error prone. > > > > Making eCPRI protocol header host byte order. > > > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> > > --- > > Cc: Stephen Hemminger <stephen@networkplumber.org> > > Cc: Bing Zhao <bingz@nvidia.com> > > Cc: Olivier Matz <olivier.matz@6wind.com> > > --- > > lib/librte_net/rte_ecpri.h | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/lib/librte_net/rte_ecpri.h b/lib/librte_net/rte_ecpri.h > > index 1cbd6d813363..67bf9186ff6f 100644 > > --- a/lib/librte_net/rte_ecpri.h > > +++ b/lib/librte_net/rte_ecpri.h > > @@ -60,21 +60,20 @@ extern "C" { > > RTE_STD_C11 > > struct rte_ecpri_common_hdr { > > union { > > - rte_be32_t u32; /**< 4B common > > header in BE */ > > + uint32_t u32; /**< 4B common > > header in host byte order */ > > struct { > > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > - uint32_t size:16; /**< Payload Size */ > > - uint32_t type:8; /**< Message Type */ > > uint32_t c:1; /**< Concatenation > > Indicator */ > > uint32_t res:3; /**< Reserved */ > > uint32_t revision:4; /**< Protocol > > Revision */ > > + uint32_t type:8; /**< Message Type */ > > #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > > uint32_t revision:4; /**< Protocol > > Revision */ > > uint32_t res:3; /**< Reserved */ > > uint32_t c:1; /**< Concatenation > > Indicator */ > > uint32_t type:8; /**< Message Type */ > > - uint32_t size:16; /**< Payload Size */ > > #endif > > + uint32_t size:16; /**< Payload Size */ > > }; > > }; > > }; > > -- > > 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [RFC] net: make eCPRI header host network order 2020-11-28 3:18 ` Bing Zhao 2020-11-28 3:55 ` Wang, Haiyue @ 2020-11-28 5:31 ` Wang, Haiyue 2020-11-28 9:07 ` Bing Zhao 1 sibling, 1 reply; 7+ messages in thread From: Wang, Haiyue @ 2020-11-28 5:31 UTC (permalink / raw) To: Bing Zhao, Yigit, Ferruh, Olivier Matz; +Cc: dev, Stephen Hemminger Hi Bing, > -----Original Message----- > From: Bing Zhao <bingz@nvidia.com> > Sent: Saturday, November 28, 2020 11:18 > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Olivier Matz <olivier.matz@6wind.com> > Cc: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Stephen Hemminger <stephen@networkplumber.org> > Subject: RE: [RFC] net: make eCPRI header host network order > > Hi Ferruh & Haiyue, > Have you checked other headers? Like: > rte_ipv4_hdr > rte_ipv6_hdr > rte_tcp_hdr > ... > > Also > [ITEM_UDP_SRC] = { > .name = "src", > .help = "UDP source port", > .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), item_param), > .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_udp, > hdr.src_port)), > }, > [ITEM_UDP_DST] = { > .name = "dst", > .help = "UDP destination port", > .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), item_param), > .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_udp, > hdr.dst_port)), > }, > > Or did I get sth. wrong? > The original design is not wrong. ;-) Since it is defined in librte_net, people will think it is just union for network order quick access like 'struct rte_gre_hdr', but in fact the bit field here is something like auxiliary data structure, we have to translate the whole 4 byte from network order to host order for accessing one bit field member, otherwise it will be wrong. Like: struct rte_ecpri_common_hdr *eh; uint8_t pkt[4]; pkt[0] = 0x10; pkt[1] = 0x03; pkt[2] = 0x00; pkt[3] = 0x18; eh = (struct rte_ecpri_common_hdr *)pkt; printf("eCPRI: 0x%08x, revision = %u, type = %u size = %u\n", eh->u32, eh->revision, eh->type, ntohs(eh->size)); eCPRI: 0x18000310, revision = 1, type = 0 size = 4099 But in fact it should be: eCPRI: 0x18000310, revision = 1, type = 3 size = 24 After the enhancement (new revision from RFCv1): struct rte_ecpri_common_hdr { union { rte_be32_t u32; /**< 4B common header in BE */ struct { #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN uint16_t c:1; /**< Concatenation Indicator */ uint16_t res:3; /**< Reserved */ uint16_t revision:4; /**< Protocol Revision */ uint16_t type:8; /**< Message Type */ #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN uint16_t revision:4; /**< Protocol Revision */ uint16_t res:3; /**< Reserved */ uint16_t c:1; /**< Concatenation Indicator */ uint16_t type:8; /**< Message Type */ #endif rte_be16_t size; /**< Payload Size */ }; }; }; The assignment in flow_dv_validate can also be simple: .common = { .u32 = RTE_BE32(((const struct rte_ecpri_common_hdr) { .type = 0xFF, }).u32), }, New: struct rte_ecpri_common_hdr common = { .type = 0xff }; BR, Haiyue > BR. Bing > > > -----Original Message----- > > From: Ferruh Yigit <ferruh.yigit@intel.com> > > Sent: Saturday, November 28, 2020 3:09 AM > > To: Olivier Matz <olivier.matz@6wind.com> > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Haiyue Wang > > <haiyue.wang@intel.com>; Stephen Hemminger > > <stephen@networkplumber.org>; Bing Zhao <bingz@nvidia.com> > > Subject: [RFC] net: make eCPRI header host network order > > > > External email: Use caution opening links or attachments > > > > > > Other protocol structs are in the host byte order, having eCPRI in > > network byte order is insistent and error prone. > > > > Making eCPRI protocol header host byte order. > > > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> > > --- > > Cc: Stephen Hemminger <stephen@networkplumber.org> > > Cc: Bing Zhao <bingz@nvidia.com> > > Cc: Olivier Matz <olivier.matz@6wind.com> > > --- > > lib/librte_net/rte_ecpri.h | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/lib/librte_net/rte_ecpri.h b/lib/librte_net/rte_ecpri.h > > index 1cbd6d813363..67bf9186ff6f 100644 > > --- a/lib/librte_net/rte_ecpri.h > > +++ b/lib/librte_net/rte_ecpri.h > > @@ -60,21 +60,20 @@ extern "C" { > > RTE_STD_C11 > > struct rte_ecpri_common_hdr { > > union { > > - rte_be32_t u32; /**< 4B common > > header in BE */ > > + uint32_t u32; /**< 4B common > > header in host byte order */ > > struct { > > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > - uint32_t size:16; /**< Payload Size */ > > - uint32_t type:8; /**< Message Type */ > > uint32_t c:1; /**< Concatenation > > Indicator */ > > uint32_t res:3; /**< Reserved */ > > uint32_t revision:4; /**< Protocol > > Revision */ > > + uint32_t type:8; /**< Message Type */ > > #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > > uint32_t revision:4; /**< Protocol > > Revision */ > > uint32_t res:3; /**< Reserved */ > > uint32_t c:1; /**< Concatenation > > Indicator */ > > uint32_t type:8; /**< Message Type */ > > - uint32_t size:16; /**< Payload Size */ > > #endif > > + uint32_t size:16; /**< Payload Size */ > > }; > > }; > > }; > > -- > > 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [RFC] net: make eCPRI header host network order 2020-11-28 5:31 ` Wang, Haiyue @ 2020-11-28 9:07 ` Bing Zhao 2020-11-30 1:15 ` Wang, Haiyue 0 siblings, 1 reply; 7+ messages in thread From: Bing Zhao @ 2020-11-28 9:07 UTC (permalink / raw) To: Wang, Haiyue, Yigit, Ferruh, Olivier Matz, Slava Ovsiienko, Thomas Monjalon Cc: dev, Stephen Hemminger Hi Haiyue & Ferruh, PSB > -----Original Message----- > From: Wang, Haiyue <haiyue.wang@intel.com> > Sent: Saturday, November 28, 2020 1:31 PM > To: Bing Zhao <bingz@nvidia.com>; Yigit, Ferruh > <ferruh.yigit@intel.com>; Olivier Matz <olivier.matz@6wind.com> > Cc: dev@dpdk.org; Stephen Hemminger <stephen@networkplumber.org> > Subject: RE: [RFC] net: make eCPRI header host network order > > External email: Use caution opening links or attachments > > > Hi Bing, > > > -----Original Message----- > > From: Bing Zhao <bingz@nvidia.com> > > Sent: Saturday, November 28, 2020 11:18 > > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Olivier Matz > > <olivier.matz@6wind.com> > > Cc: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Stephen > > Hemminger <stephen@networkplumber.org> > > Subject: RE: [RFC] net: make eCPRI header host network order > > > > Hi Ferruh & Haiyue, > > Have you checked other headers? Like: > > rte_ipv4_hdr > > rte_ipv6_hdr > > rte_tcp_hdr > > ... > > > > Also > > [ITEM_UDP_SRC] = { > > .name = "src", > > .help = "UDP source port", > > .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), > item_param), > > .args = ARGS(ARGS_ENTRY_HTON(struct > rte_flow_item_udp, > > hdr.src_port)), > > }, > > [ITEM_UDP_DST] = { > > .name = "dst", > > .help = "UDP destination port", > > .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), > item_param), > > .args = ARGS(ARGS_ENTRY_HTON(struct > rte_flow_item_udp, > > hdr.dst_port)), > > }, > > > > Or did I get sth. wrong? > > > > The original design is not wrong. ;-) > > Since it is defined in librte_net, people will think it is just > union for network order quick access like 'struct rte_gre_hdr', but > in fact the bit field here is something like auxiliary data > structure, we have to translate the whole 4 byte from network order > to host order for accessing one bit field member, otherwise it will > be wrong. I also checked the definitions in the file " rte_higig.h", and I got your point. Yes, I agree. Indeed, in my original RFC (in the link) http://inbox.dpdk.org/dev/1591717358-194133-1-git-send-email-bingz@mellanox.com/ I used the same definition method as you suggested. Then during the implementation, since some old compilers gave some warning to the uint8_t or uint16_t bit fields, I decided to use uint32_t bit fields to make them happy 😊. Then I noticed the common header took the entire 4 bytes that could be treated as an u32, so I also moved the sequence of the type and size members. And yes, the header usage in the host SW is missed then. And for an ingress packet, after swapping the u32 in little endian host, the correct value of each field could be got, but the offset of each field is wrong then. I personally prefer to Ferruh's method which remaining u32 bit fields. Or else some instruction should be added to get rid of the warnings. I also checked the Linux code, "/usr/include/linux/ip.h" Like the IP header, one definition is using u8 with bit fields. In BSD socket file "/usr/include/netinet/ip.h" It uses "unsigned int" bit fields. Since the following is an u8 and it will be aligned naturally. Maybe we could also use this favor in "/usr/include/netinet/ip.h". > > Like: > > struct rte_ecpri_common_hdr *eh; > uint8_t pkt[4]; > > pkt[0] = 0x10; > pkt[1] = 0x03; > pkt[2] = 0x00; > pkt[3] = 0x18; > > eh = (struct rte_ecpri_common_hdr *)pkt; > > printf("eCPRI: 0x%08x, revision = %u, type = %u size = %u\n", > eh->u32, eh->revision, eh->type, ntohs(eh->size)); > > eCPRI: 0x18000310, revision = 1, type = 0 size = 4099 > > But in fact it should be: > > eCPRI: 0x18000310, revision = 1, type = 3 size = 24 > > > After the enhancement (new revision from RFCv1): > > struct rte_ecpri_common_hdr { > union { > rte_be32_t u32; /**< 4B common > header in BE */ > struct { > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > uint16_t c:1; /**< Concatenation > Indicator */ > uint16_t res:3; /**< Reserved */ > uint16_t revision:4; /**< Protocol > Revision */ > uint16_t type:8; /**< Message Type */ > #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > uint16_t revision:4; /**< Protocol > Revision */ > uint16_t res:3; /**< Reserved */ > uint16_t c:1; /**< Concatenation > Indicator */ > uint16_t type:8; /**< Message Type */ > #endif > rte_be16_t size; /**< Payload Size */ > }; > }; > }; > So Ferruh, would you also please move the + uint32_t type:8; out of the "#if" macro? And since the size field should be in big-endian. How about to use rte_be32_t to indicate this? Regarding the commit message: " Other protocol structs are in the host byte order, having eCPRI in network byte order is insistent and error prone. Making eCPRI protocol header host byte order. " To my understanding, this might not be accurate. All the protocol structures are in the network order, and the fields of them are also in the network order. In the structure, the addresses (offset) of the members will be in an ascending order inside the struct. This is just like what the network order did, hard to say whether it is network order or host order. If a member is with u16, u32 or even u64 type, then that part of memory will be treated with the endianness of the host. And also, Ferruh, would you mind to send a v2 with the fix of type #4 "rte_ecpri_msg_rm_access". Then I will fix the rte_flow, check the testpmd and also do the adaption for the PMD part. > > The assignment in flow_dv_validate can also be simple: > > .common = { > .u32 = > RTE_BE32(((const struct > rte_ecpri_common_hdr) { > .type = 0xFF, > }).u32), > }, > > New: > > struct rte_ecpri_common_hdr common = { .type = 0xff }; > > > BR, > Haiyue > Many thanks > > BR. Bing > > > > > -----Original Message----- > > > From: Ferruh Yigit <ferruh.yigit@intel.com> > > > Sent: Saturday, November 28, 2020 3:09 AM > > > To: Olivier Matz <olivier.matz@6wind.com> > > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Haiyue > Wang > > > <haiyue.wang@intel.com>; Stephen Hemminger > > > <stephen@networkplumber.org>; Bing Zhao <bingz@nvidia.com> > > > Subject: [RFC] net: make eCPRI header host network order > > > > > > External email: Use caution opening links or attachments > > > > > > > > > Other protocol structs are in the host byte order, having eCPRI > in > > > network byte order is insistent and error prone. > > > > > > Making eCPRI protocol header host byte order. > > > > > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> > > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> > > > --- > > > Cc: Stephen Hemminger <stephen@networkplumber.org> > > > Cc: Bing Zhao <bingz@nvidia.com> > > > Cc: Olivier Matz <olivier.matz@6wind.com> > > > --- > > > lib/librte_net/rte_ecpri.h | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/lib/librte_net/rte_ecpri.h > b/lib/librte_net/rte_ecpri.h > > > index 1cbd6d813363..67bf9186ff6f 100644 > > > --- a/lib/librte_net/rte_ecpri.h > > > +++ b/lib/librte_net/rte_ecpri.h > > > @@ -60,21 +60,20 @@ extern "C" { > > > RTE_STD_C11 > > > struct rte_ecpri_common_hdr { > > > union { > > > - rte_be32_t u32; /**< 4B common > > > header in BE */ > > > + uint32_t u32; /**< 4B common > > > header in host byte order */ > > > struct { > > > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > > - uint32_t size:16; /**< Payload > Size */ > > > - uint32_t type:8; /**< Message > Type */ > > > uint32_t c:1; /**< > Concatenation > > > Indicator */ > > > uint32_t res:3; /**< Reserved */ > > > uint32_t revision:4; /**< Protocol > > > Revision */ > > > + uint32_t type:8; /**< Message > Type */ > > > #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > > > uint32_t revision:4; /**< Protocol > > > Revision */ > > > uint32_t res:3; /**< Reserved */ > > > uint32_t c:1; /**< > Concatenation > > > Indicator */ > > > uint32_t type:8; /**< Message > Type */ > > > - uint32_t size:16; /**< Payload > Size */ > > > #endif > > > + uint32_t size:16; /**< Payload > Size */ > > > }; > > > }; > > > }; > > > -- > > > 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [RFC] net: make eCPRI header host network order 2020-11-28 9:07 ` Bing Zhao @ 2020-11-30 1:15 ` Wang, Haiyue 0 siblings, 0 replies; 7+ messages in thread From: Wang, Haiyue @ 2020-11-30 1:15 UTC (permalink / raw) To: Bing Zhao, Yigit, Ferruh, Olivier Matz, Slava Ovsiienko, Thomas Monjalon Cc: dev, Stephen Hemminger > -----Original Message----- > From: Bing Zhao <bingz@nvidia.com> > Sent: Saturday, November 28, 2020 17:07 > To: Wang, Haiyue <haiyue.wang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Olivier Matz > <olivier.matz@6wind.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Thomas Monjalon > <tmonjalon@nvidia.com> > Cc: dev@dpdk.org; Stephen Hemminger <stephen@networkplumber.org> > Subject: RE: [RFC] net: make eCPRI header host network order > > Hi Haiyue & Ferruh, > > PSB > > > -----Original Message----- > > From: Wang, Haiyue <haiyue.wang@intel.com> > > Sent: Saturday, November 28, 2020 1:31 PM > > To: Bing Zhao <bingz@nvidia.com>; Yigit, Ferruh > > <ferruh.yigit@intel.com>; Olivier Matz <olivier.matz@6wind.com> > > Cc: dev@dpdk.org; Stephen Hemminger <stephen@networkplumber.org> > > Subject: RE: [RFC] net: make eCPRI header host network order > > > > External email: Use caution opening links or attachments > > > > > > Hi Bing, > > > > > -----Original Message----- > > > From: Bing Zhao <bingz@nvidia.com> > > > Sent: Saturday, November 28, 2020 11:18 > > > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Olivier Matz > > > <olivier.matz@6wind.com> > > > Cc: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Stephen > > > Hemminger <stephen@networkplumber.org> > > > Subject: RE: [RFC] net: make eCPRI header host network order > > > > > > Hi Ferruh & Haiyue, > > > Have you checked other headers? Like: > > > rte_ipv4_hdr > > > rte_ipv6_hdr > > > rte_tcp_hdr > > > ... > > > > > > Also > > > [ITEM_UDP_SRC] = { > > > .name = "src", > > > .help = "UDP source port", > > > .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), > > item_param), > > > .args = ARGS(ARGS_ENTRY_HTON(struct > > rte_flow_item_udp, > > > hdr.src_port)), > > > }, > > > [ITEM_UDP_DST] = { > > > .name = "dst", > > > .help = "UDP destination port", > > > .next = NEXT(item_udp, NEXT_ENTRY(UNSIGNED), > > item_param), > > > .args = ARGS(ARGS_ENTRY_HTON(struct > > rte_flow_item_udp, > > > hdr.dst_port)), > > > }, > > > > > > Or did I get sth. wrong? > > > > > > > The original design is not wrong. ;-) > > > > Since it is defined in librte_net, people will think it is just > > union for network order quick access like 'struct rte_gre_hdr', but > > in fact the bit field here is something like auxiliary data > > structure, we have to translate the whole 4 byte from network order > > to host order for accessing one bit field member, otherwise it will > > be wrong. > > I also checked the definitions in the file " rte_higig.h", and I got your point. > Yes, I agree. > Indeed, in my original RFC (in the link) > http://inbox.dpdk.org/dev/1591717358-194133-1-git-send-email-bingz@mellanox.com/ > I used the same definition method as you suggested. > > Then during the implementation, since some old compilers gave some warning to the uint8_t or uint16_t > bit fields, I decided to use uint32_t bit fields to make them happy 😊. Then I noticed the common Interesting, if double uint16_t like 'struct rte_gre_hdr', will still have warning ? Since if we decide to change, then 'size' will be network order now, then use 'rte_be16_t' can capture order issue by tool like: http://git.dpdk.org/dpdk/commit/?id=fbb25a3878cc7c6de4c68c8cee01983d127e2205 > header took the entire 4 bytes that could be treated as an u32, so I also moved the sequence of the > type and size members. And yes, the header usage in the host SW is missed then. > And for an ingress packet, after swapping the u32 in little endian host, the correct value of each > field could be got, but the offset of each field is wrong then. > I personally prefer to Ferruh's method which remaining u32 bit fields. Or else some instruction should > be added to get rid of the warnings. > > I also checked the Linux code, "/usr/include/linux/ip.h" > Like the IP header, one definition is using u8 with bit fields. > > In BSD socket file "/usr/include/netinet/ip.h" > It uses "unsigned int" bit fields. Since the following is an u8 and it will be aligned naturally. > Maybe we could also use this favor in "/usr/include/netinet/ip.h". > > > > > Like: > > > > struct rte_ecpri_common_hdr *eh; > > uint8_t pkt[4]; > > > > pkt[0] = 0x10; > > pkt[1] = 0x03; > > pkt[2] = 0x00; > > pkt[3] = 0x18; > > > > eh = (struct rte_ecpri_common_hdr *)pkt; > > > > printf("eCPRI: 0x%08x, revision = %u, type = %u size = %u\n", > > eh->u32, eh->revision, eh->type, ntohs(eh->size)); > > > > eCPRI: 0x18000310, revision = 1, type = 0 size = 4099 > > > > But in fact it should be: > > > > eCPRI: 0x18000310, revision = 1, type = 3 size = 24 > > > > > > After the enhancement (new revision from RFCv1): > > > > struct rte_ecpri_common_hdr { > > union { > > rte_be32_t u32; /**< 4B common > > header in BE */ > > struct { > > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > uint16_t c:1; /**< Concatenation > > Indicator */ > > uint16_t res:3; /**< Reserved */ > > uint16_t revision:4; /**< Protocol > > Revision */ > > uint16_t type:8; /**< Message Type */ > > #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > > uint16_t revision:4; /**< Protocol > > Revision */ > > uint16_t res:3; /**< Reserved */ > > uint16_t c:1; /**< Concatenation > > Indicator */ > > uint16_t type:8; /**< Message Type */ > > #endif > > rte_be16_t size; /**< Payload Size */ > > }; > > }; > > }; > > > > So Ferruh, would you also please move the > + uint32_t type:8; > out of the "#if" macro? > And since the size field should be in big-endian. > How about to use rte_be32_t to indicate this? > > Regarding the commit message: > " > Other protocol structs are in the host byte order, having eCPRI in > network byte order is insistent and error prone. > Making eCPRI protocol header host byte order. > " > > To my understanding, this might not be accurate. All the protocol structures are in the network order, > and the fields of them are also in the network order. > In the structure, the addresses (offset) of the members will be in an ascending order inside the > struct. This is just like what the network order did, hard to say whether it is network order or host > order. > If a member is with u16, u32 or even u64 type, then that part of memory will be treated with the > endianness of the host. > > And also, Ferruh, would you mind to send a v2 with the fix of type #4 "rte_ecpri_msg_rm_access". > > Then I will fix the rte_flow, check the testpmd and also do the adaption for the PMD part. > > > > > The assignment in flow_dv_validate can also be simple: > > > > .common = { > > .u32 = > > RTE_BE32(((const struct > > rte_ecpri_common_hdr) { > > .type = 0xFF, > > }).u32), > > }, > > > > New: > > > > struct rte_ecpri_common_hdr common = { .type = 0xff }; > > > > > > BR, > > Haiyue > > > > Many thanks > > > > BR. Bing > > > > > > > -----Original Message----- > > > > From: Ferruh Yigit <ferruh.yigit@intel.com> > > > > Sent: Saturday, November 28, 2020 3:09 AM > > > > To: Olivier Matz <olivier.matz@6wind.com> > > > > Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Haiyue > > Wang > > > > <haiyue.wang@intel.com>; Stephen Hemminger > > > > <stephen@networkplumber.org>; Bing Zhao <bingz@nvidia.com> > > > > Subject: [RFC] net: make eCPRI header host network order > > > > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > Other protocol structs are in the host byte order, having eCPRI > > in > > > > network byte order is insistent and error prone. > > > > > > > > Making eCPRI protocol header host byte order. > > > > > > > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> > > > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> > > > > --- > > > > Cc: Stephen Hemminger <stephen@networkplumber.org> > > > > Cc: Bing Zhao <bingz@nvidia.com> > > > > Cc: Olivier Matz <olivier.matz@6wind.com> > > > > --- > > > > lib/librte_net/rte_ecpri.h | 7 +++---- > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/lib/librte_net/rte_ecpri.h > > b/lib/librte_net/rte_ecpri.h > > > > index 1cbd6d813363..67bf9186ff6f 100644 > > > > --- a/lib/librte_net/rte_ecpri.h > > > > +++ b/lib/librte_net/rte_ecpri.h > > > > @@ -60,21 +60,20 @@ extern "C" { > > > > RTE_STD_C11 > > > > struct rte_ecpri_common_hdr { > > > > union { > > > > - rte_be32_t u32; /**< 4B common > > > > header in BE */ > > > > + uint32_t u32; /**< 4B common > > > > header in host byte order */ > > > > struct { > > > > #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > > > - uint32_t size:16; /**< Payload > > Size */ > > > > - uint32_t type:8; /**< Message > > Type */ > > > > uint32_t c:1; /**< > > Concatenation > > > > Indicator */ > > > > uint32_t res:3; /**< Reserved */ > > > > uint32_t revision:4; /**< Protocol > > > > Revision */ > > > > + uint32_t type:8; /**< Message > > Type */ > > > > #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > > > > uint32_t revision:4; /**< Protocol > > > > Revision */ > > > > uint32_t res:3; /**< Reserved */ > > > > uint32_t c:1; /**< > > Concatenation > > > > Indicator */ > > > > uint32_t type:8; /**< Message > > Type */ > > > > - uint32_t size:16; /**< Payload > > Size */ > > > > #endif > > > > + uint32_t size:16; /**< Payload > > Size */ > > > > }; > > > > }; > > > > }; > > > > -- > > > > 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [RFC] net: make eCPRI header host network order 2020-11-27 19:09 [dpdk-dev] [RFC] net: make eCPRI header host network order Ferruh Yigit 2020-11-28 3:18 ` Bing Zhao @ 2023-06-14 21:57 ` Stephen Hemminger 1 sibling, 0 replies; 7+ messages in thread From: Stephen Hemminger @ 2023-06-14 21:57 UTC (permalink / raw) To: Ferruh Yigit; +Cc: Olivier Matz, dev, Haiyue Wang, Bing Zhao On Fri, 27 Nov 2020 19:09:20 +0000 Ferruh Yigit <ferruh.yigit@intel.com> wrote: > Other protocol structs are in the host byte order, having eCPRI in > network byte order is insistent and error prone. > > Making eCPRI protocol header host byte order. > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> The discussion around this never concluded and there was no followon patch. Marking it as "Changes Requested". The most common thing for the existing headers is to declare them to be in network byte order. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-14 21:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-27 19:09 [dpdk-dev] [RFC] net: make eCPRI header host network order Ferruh Yigit 2020-11-28 3:18 ` Bing Zhao 2020-11-28 3:55 ` Wang, Haiyue 2020-11-28 5:31 ` Wang, Haiyue 2020-11-28 9:07 ` Bing Zhao 2020-11-30 1:15 ` Wang, Haiyue 2023-06-14 21:57 ` Stephen Hemminger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).