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