Re: [dpdk-dev] [RFC] net: make eCPRI header host network order

["Wang, Haiyue" <haiyue.wang@intel.com>]

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