> -----Original Message-----
> From: Bing Zhao <bi...@nvidia.com>
> Sent: Saturday, November 28, 2020 17:07
> To: Wang, Haiyue <haiyue.w...@intel.com>; Yigit, Ferruh
> <ferruh.yi...@intel.com>; Olivier Matz
> <olivier.m...@6wind.com>; Slava Ovsiienko <viachesl...@nvidia.com>; Thomas
> Monjalon
> <tmonja...@nvidia.com>
> Cc: dev@dpdk.org; Stephen Hemminger <step...@networkplumber.org>
> Subject: RE: [RFC] net: make eCPRI header host network order
>
> Hi Haiyue & Ferruh,
>
> PSB
>
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.w...@intel.com>
> > Sent: Saturday, November 28, 2020 1:31 PM
> > To: Bing Zhao <bi...@nvidia.com>; Yigit, Ferruh
> > <ferruh.yi...@intel.com>; Olivier Matz <olivier.m...@6wind.com>
> > Cc: dev@dpdk.org; Stephen Hemminger <step...@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 <bi...@nvidia.com>
> > > Sent: Saturday, November 28, 2020 11:18
> > > To: Yigit, Ferruh <ferruh.yi...@intel.com>; Olivier Matz
> > > <olivier.m...@6wind.com>
> > > Cc: dev@dpdk.org; Wang, Haiyue <haiyue.w...@intel.com>; Stephen
> > > Hemminger <step...@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-bi...@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.yi...@intel.com>
> > > > Sent: Saturday, November 28, 2020 3:09 AM
> > > > To: Olivier Matz <olivier.m...@6wind.com>
> > > > Cc: Ferruh Yigit <ferruh.yi...@intel.com>; dev@dpdk.org; Haiyue
> > Wang
> > > > <haiyue.w...@intel.com>; Stephen Hemminger
> > > > <step...@networkplumber.org>; Bing Zhao <bi...@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.w...@intel.com>
> > > > Signed-off-by: Ferruh Yigit <ferruh.yi...@intel.com>
> > > > ---
> > > > Cc: Stephen Hemminger <step...@networkplumber.org>
> > > > Cc: Bing Zhao <bi...@nvidia.com>
> > > > Cc: Olivier Matz <olivier.m...@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
