diff mbox series

[RFC] net: make eCPRI header host network order

Message ID 20201127190920.3312280-1-ferruh.yigit@intel.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers show
Series [RFC] net: make eCPRI header host network order | expand

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Ferruh Yigit Nov. 27, 2020, 7:09 p.m. UTC
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(-)

Comments

Bing Zhao Nov. 28, 2020, 3:18 a.m. UTC | #1
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
Wang, Haiyue Nov. 28, 2020, 3:55 a.m. UTC | #2
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
Wang, Haiyue Nov. 28, 2020, 5:31 a.m. UTC | #3
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
Bing Zhao Nov. 28, 2020, 9:07 a.m. UTC | #4
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
Wang, Haiyue Nov. 30, 2020, 1:15 a.m. UTC | #5
> -----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
diff mbox series

Patch

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 */
 		};
 	};
 };