[v5,1/2] rte_flow: add eCPRI key fields to flow API

  Add a new item "rte_flow_item_ecpri" in order to match eCRPI header.

eCPRI is a packet based protocol used in the fronthaul interface of
5G networks. Header format definition could be found in the
specification via the link below:
https://www.gigalight.com/downloads/standards/ecpri-specification.pdf

eCPRI message can be over Ethernet layer (.1Q supported also) or over
UDP layer. Message header formats are the same in these two variants.

Signed-off-by: Bing Zhao <bingz@mellanox.com>
Acked-by: Ori Kam <orika@mellanox.com>
---
 doc/guides/prog_guide/rte_flow.rst     |   8 ++
 doc/guides/rel_notes/release_20_08.rst |   5 +
 lib/librte_ethdev/rte_flow.c           |   1 +
 lib/librte_ethdev/rte_flow.h           |  31 ++++++
 lib/librte_net/Makefile                |   1 +
 lib/librte_net/meson.build             |   3 +-
 lib/librte_net/rte_ecpri.h             | 182 +++++++++++++++++++++++++++++++++
 lib/librte_net/rte_ether.h             |   1 +
 8 files changed, 231 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_net/rte_ecpri.h
  

Hi Bing,

On Fri, Jul 10, 2020 at 04:45:22PM +0800, Bing Zhao wrote:
> Add a new item "rte_flow_item_ecpri" in order to match eCRPI header.
> 
> eCPRI is a packet based protocol used in the fronthaul interface of
> 5G networks. Header format definition could be found in the
> specification via the link below:
> https://www.gigalight.com/downloads/standards/ecpri-specification.pdf
> 
> eCPRI message can be over Ethernet layer (.1Q supported also) or over
> UDP layer. Message header formats are the same in these two variants.
> 
> Signed-off-by: Bing Zhao <bingz@mellanox.com>
> Acked-by: Ori Kam <orika@mellanox.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst     |   8 ++
>  doc/guides/rel_notes/release_20_08.rst |   5 +
>  lib/librte_ethdev/rte_flow.c           |   1 +
>  lib/librte_ethdev/rte_flow.h           |  31 ++++++
>  lib/librte_net/Makefile                |   1 +
>  lib/librte_net/meson.build             |   3 +-
>  lib/librte_net/rte_ecpri.h             | 182 +++++++++++++++++++++++++++++++++
>  lib/librte_net/rte_ether.h             |   1 +
>  8 files changed, 231 insertions(+), 1 deletion(-)
>  create mode 100644 lib/librte_net/rte_ecpri.h
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d5dd18c..669d519 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1362,6 +1362,14 @@ Matches a PFCP Header.
>  - ``seid``: session endpoint identifier.
>  - Default ``mask`` matches s_field and seid.
>  
> +Item: ``ECPRI``
> +^^^^^^^^^^^^^
> +
> +Matches a eCPRI header.
> +
> +- ``hdr``: eCPRI header definition (``rte_ecpri.h``).
> +- Default ``mask`` matches message type of common header only.
> +
>  Actions
>  ~~~~~~~
>  
> diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
> index 988474c..19feb68 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -184,6 +184,11 @@ New Features
>    which are used to access packet data in a safe manner. Currently JIT support
>    for these instructions is implemented for x86 only.
>  
> +* **Added eCPRI protocol support in rte_flow.**
> +
> +  The ``ECPRI`` item have been added to support eCPRI packet offloading for
> +  5G network.
> +
>  * **Added flow performance test application.**
>  
>    Added new application to test ``rte_flow`` performance, including:
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 1685be5..f8fdd68 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -95,6 +95,7 @@ struct rte_flow_desc_data {
>  	MK_FLOW_ITEM(HIGIG2, sizeof(struct rte_flow_item_higig2_hdr)),
>  	MK_FLOW_ITEM(L2TPV3OIP, sizeof(struct rte_flow_item_l2tpv3oip)),
>  	MK_FLOW_ITEM(PFCP, sizeof(struct rte_flow_item_pfcp)),
> +	MK_FLOW_ITEM(ECPRI, sizeof(struct rte_flow_item_ecpri)),
>  };
>  
>  /** Generate flow_action[] entry. */
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index b0e4199..8a90226 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -28,6 +28,7 @@
>  #include <rte_byteorder.h>
>  #include <rte_esp.h>
>  #include <rte_higig.h>
> +#include <rte_ecpri.h>
>  #include <rte_mbuf.h>
>  #include <rte_mbuf_dyn.h>
>  
> @@ -527,6 +528,15 @@ enum rte_flow_item_type {
>  	 */
>  	RTE_FLOW_ITEM_TYPE_PFCP,
>  
> +	/**
> +	 * Matches eCPRI Header.
> +	 *
> +	 * Configure flow for eCPRI over ETH or UDP packets.
> +	 *
> +	 * See struct rte_flow_item_ecpri.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_ECPRI,
> +
>  };
>  
>  /**
> @@ -1547,6 +1557,27 @@ struct rte_flow_item_pfcp {
>  #endif
>  
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ITEM_TYPE_ECPRI
> + *
> + * Match eCPRI Header
> + */
> +struct rte_flow_item_ecpri {
> +	struct rte_ecpri_msg_hdr hdr;
> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_ECPRI. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_ecpri rte_flow_item_ecpri_mask = {
> +	.hdr = {
> +		.dw0 = 0x0,
> +	},
> +};
> +#endif
> +
> +/**
>   * Matching pattern item definition.
>   *
>   * A pattern is formed by stacking items starting from the lowest protocol
> diff --git a/lib/librte_net/Makefile b/lib/librte_net/Makefile
> index aa1d6fe..9830e77 100644
> --- a/lib/librte_net/Makefile
> +++ b/lib/librte_net/Makefile
> @@ -20,5 +20,6 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_sctp.h rte_icmp.h rte_arp.h
>  SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_ether.h rte_gre.h rte_net.h
>  SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_net_crc.h rte_mpls.h rte_higig.h
>  SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_gtp.h rte_vxlan.h
> +SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_ecpri.h
>  
>  include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/lib/librte_net/meson.build b/lib/librte_net/meson.build
> index f799349..24ed825 100644
> --- a/lib/librte_net/meson.build
> +++ b/lib/librte_net/meson.build
> @@ -15,7 +15,8 @@ headers = files('rte_ip.h',
>  	'rte_net.h',
>  	'rte_net_crc.h',
>  	'rte_mpls.h',
> -	'rte_higig.h')
> +	'rte_higig.h',
> +	'rte_ecpri.h')
>  
>  sources = files('rte_arp.c', 'rte_ether.c', 'rte_net.c', 'rte_net_crc.c')
>  deps += ['mbuf']
> diff --git a/lib/librte_net/rte_ecpri.h b/lib/librte_net/rte_ecpri.h
> new file mode 100644
> index 0000000..60fb4f7
> --- /dev/null
> +++ b/lib/librte_net/rte_ecpri.h
> @@ -0,0 +1,182 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020 Mellanox Technologies, Ltd
> + */
> +
> +#ifndef _RTE_ECPRI_H_
> +#define _RTE_ECPRI_H_
> +
> +#include <stdint.h>
> +#include <rte_byteorder.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * eCPRI Protocol Revision 1.0, 1.1, 1.2, 2.0: 0001b
> + * Other values are reserved for future
> + */
> +#define RTE_ECPRI_REV_UP_TO_20		1
> +
> +/**
> + * eCPRI message types in specifications
> + * IWF* types will only be supported from rev.2
> + * 12-63: Reserved for future revision
> + * 64-255: Vendor Specific
> + */
> +#define RTE_ECPRI_MSG_TYPE_IQ_DATA	0

Here the doxygen comment applies to RTE_ECPRI_MSG_TYPE_IQ_DATA.

I think it should either be a standard comment (no doxygen), or
something more complex should be done, like grouping:
https://stackoverflow.com/questions/30803156/group-level-documentation-for-preprocessor-defines-in-doxygen
(I didn't try)


> +#define RTE_ECPRI_MSG_TYPE_BIT_SEQ	1
> +#define RTE_ECPRI_MSG_TYPE_RTC_CTRL	2
> +#define RTE_ECPRI_MSG_TYPE_GEN_DATA	3
> +#define RTE_ECPRI_MSG_TYPE_RM_ACC	4
> +#define RTE_ECPRI_MSG_TYPE_DLY_MSR	5
> +#define RTE_ECPRI_MSG_TYPE_RMT_RST	6
> +#define RTE_ECPRI_MSG_TYPE_EVT_IND	7
> +#define RTE_ECPRI_MSG_TYPE_IWF_UP	8
> +#define RTE_ECPRI_MSG_TYPE_IWF_OPT	9
> +#define RTE_ECPRI_MSG_TYPE_IWF_MAP	10
> +#define RTE_ECPRI_MSG_TYPE_IWF_DCTRL	11
> +
> +/**
> + * Event Type of Message Type #7: Event Indication
> + * 0x00: Fault(s) Indication
> + * 0x01: Fault(s) Indication Acknowledge
> + * 0x02: Notification(s) Indication
> + * 0x03: Synchronization Request
> + * 0x04: Synchronization Acknowledge
> + * 0x05: Synchronization End Indication
> + * 0x06...0xFF: Reserved
> + */
> +#define RTE_ECPRI_EVT_IND_FAULT_IND	0
> +#define RTE_ECPRI_EVT_IND_FAULT_ACK	1
> +#define RTE_ECPRI_EVT_IND_NTFY_IND	2
> +#define RTE_ECPRI_EVT_IND_SYNC_REQ	3
> +#define RTE_ECPRI_EVT_IND_SYNC_ACK	4
> +#define RTE_ECPRI_EVT_IND_SYNC_END	5
> +
> +/**
> + * eCPRI Common Header
> + */
> +RTE_STD_C11
> +struct rte_ecpri_common_hdr {
> +#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 */
> +#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
> +} __rte_packed;

Does it really need to be packed? Why next types do not need it?
It looks only those which have bitfields are.


I wonder if the 'dw0' could be in this definition instead of in
struct rte_ecpri_msg_hdr?

Something like this:

struct rte_ecpri_common_hdr {
	union {
		uint32_t u32;
		struct {
			...
		};
	};
};

I see 2 advantages:

- someone that only uses the common_hdr struct can use the .u32
  in its software
- when using it in messages, it looks clearer to me:
    msg.common_hdr.u32 = value;
  instead of:
    msg.dw0 = value;

What do you think?

> +
> +/**
> + * eCPRI Message Header of Type #0: IQ Data
> + */
> +struct rte_ecpri_msg_iq_data {
> +	rte_be16_t pc_id;		/**< Physical channel ID */
> +	rte_be16_t seq_id;		/**< Sequence ID */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #1: Bit Sequence
> + */
> +struct rte_ecpri_msg_bit_seq {
> +	rte_be16_t pc_id;		/**< Physical channel ID */
> +	rte_be16_t seq_id;		/**< Sequence ID */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #2: Real-Time Control Data
> + */
> +struct rte_ecpri_msg_rtc_ctrl {
> +	rte_be16_t rtc_id;		/**< Real-Time Control Data ID */
> +	rte_be16_t seq_id;		/**< Sequence ID */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #3: Generic Data Transfer
> + */
> +struct rte_ecpri_msg_gen_data {
> +	rte_be32_t pc_id;		/**< Physical channel ID */
> +	rte_be32_t seq_id;		/**< Sequence ID */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #4: Remote Memory Access
> + */
> +RTE_STD_C11
> +struct rte_ecpri_msg_rm_access {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> +	uint32_t ele_id:16;		/**< Element ID */
> +	uint32_t rr:4;			/**< Req/Resp */
> +	uint32_t rw:4;			/**< Read/Write */
> +	uint32_t rma_id:8;		/**< Remote Memory Access ID */
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +	uint32_t rma_id:8;		/**< Remote Memory Access ID */
> +	uint32_t rw:4;			/**< Read/Write */
> +	uint32_t rr:4;			/**< Req/Resp */
> +	uint32_t ele_id:16;		/**< Element ID */
> +#endif
> +	rte_be16_t addr_m;		/**< 48-bits address (16 MSB) */
> +	rte_be32_t addr_l;		/**< 48-bits address (32 LSB) */
> +	rte_be16_t length;		/**< number of bytes */
> +} __rte_packed;
> +
> +/**
> + * eCPRI Message Header of Type #5: One-Way Delay Measurement
> + */
> +struct rte_ecpri_msg_delay_measure {
> +	uint8_t msr_id;			/**< Measurement ID */
> +	uint8_t act_type;		/**< Action Type */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #6: Remote Reset
> + */
> +struct rte_ecpri_msg_remote_reset {
> +	rte_be16_t rst_id;		/**< Reset ID */
> +	uint8_t rst_op;			/**< Reset Code Op */
> +};
> +
> +/**
> + * eCPRI Message Header of Type #7: Event Indication
> + */
> +struct rte_ecpri_msg_event_ind {
> +	uint8_t evt_id;			/**< Event ID */
> +	uint8_t evt_type;		/**< Event Type */
> +	uint8_t seq;			/**< Sequence Number */
> +	uint8_t number;			/**< Number of Faults/Notif */
> +};
> +
> +/**
> + * eCPRI Message Header Format: Common Header + Message Types
> + */
> +RTE_STD_C11
> +struct rte_ecpri_msg_hdr {
> +	union {
> +		struct rte_ecpri_common_hdr common;
> +		uint32_t dw0;
> +	};
> +	union {
> +		struct rte_ecpri_msg_iq_data type0;
> +		struct rte_ecpri_msg_bit_seq type1;
> +		struct rte_ecpri_msg_rtc_ctrl type2;
> +		struct rte_ecpri_msg_bit_seq type3;
> +		struct rte_ecpri_msg_rm_access type4;
> +		struct rte_ecpri_msg_delay_measure type5;
> +		struct rte_ecpri_msg_remote_reset type6;
> +		struct rte_ecpri_msg_event_ind type7;
> +		uint32_t dummy[3];
> +	};
> +};

What is the point in having this struct?

From a software point of view, I think it is a bit risky, because
its size is the size of the largest message. This is probably what
you want in your case, but when a software will rx or tx such
packet, I think they shouldn't use this one. My understanding is
that you only need this structure for the mask in rte_flow.

Also, I'm not sure to understand the purpose of dummy[3], even after reading
your answer to Akhil's question.


> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_ECPRI_H_ */
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 0ae4e75..184a3f9 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -304,6 +304,7 @@ struct rte_vlan_hdr {
>  #define RTE_ETHER_TYPE_LLDP 0x88CC /**< LLDP Protocol. */
>  #define RTE_ETHER_TYPE_MPLS 0x8847 /**< MPLS ethertype. */
>  #define RTE_ETHER_TYPE_MPLSM 0x8848 /**< MPLS multicast ethertype. */
> +#define RTE_ETHER_TYPE_ECPRI 0xAEFE /**< eCPRI ethertype (.1Q supported). */
>  
>  /**
>   * Extract VLAN tag information into mbuf
> -- 
> 1.8.3.1
>
  

Hi Olivier,
Many thanks for your comments.

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Friday, July 10, 2020 10:31 PM
> To: Bing Zhao <bingz@mellanox.com>
> Cc: Ori Kam <orika@mellanox.com>; john.mcnamara@intel.com;
> marko.kovacevic@intel.com; Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> arybchenko@solarflare.com; akhil.goyal@nxp.com; dev@dpdk.org;
> wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com
> Subject: Re: [PATCH v5 1/2] rte_flow: add eCPRI key fields to flow API
> 
> Hi Bing,
> 
> On Fri, Jul 10, 2020 at 04:45:22PM +0800, Bing Zhao wrote:
> > Add a new item "rte_flow_item_ecpri" in order to match eCRPI
> header.
> >
> > eCPRI is a packet based protocol used in the fronthaul interface of 5G
> > networks. Header format definition could be found in the
> specification
> > via the link below:
> >
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> www.
> > gigalight.com%2Fdownloads%2Fstandards%2Fecpri-
> specification.pdf&amp;da
> >
> ta=02%7C01%7Cbingz%40mellanox.com%7C99fe8c970b6648d82f7908
> d824dded33%7
> >
> Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C6372998828794
> 68155&amp;sda
> >
> ta=hv1rAa9I2%2BPpZkoLKzHdEpjrLyvY%2F4IiaEvW3XHnhW4%3D&amp
> ;reserved=0
> >
> > eCPRI message can be over Ethernet layer (.1Q supported also) or
> over
> > UDP layer. Message header formats are the same in these two
> variants.
> >
> > Signed-off-by: Bing Zhao <bingz@mellanox.com>
> > Acked-by: Ori Kam <orika@mellanox.com>
> > ---
> >  doc/guides/prog_guide/rte_flow.rst     |   8 ++
> >  doc/guides/rel_notes/release_20_08.rst |   5 +
> >  lib/librte_ethdev/rte_flow.c           |   1 +
> >  lib/librte_ethdev/rte_flow.h           |  31 ++++++
> >  lib/librte_net/Makefile                |   1 +
> >  lib/librte_net/meson.build             |   3 +-
> >  lib/librte_net/rte_ecpri.h             | 182
> +++++++++++++++++++++++++++++++++
> >  lib/librte_net/rte_ether.h             |   1 +
> >  8 files changed, 231 insertions(+), 1 deletion(-)  create mode 100644
> > lib/librte_net/rte_ecpri.h
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index d5dd18c..669d519 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -1362,6 +1362,14 @@ Matches a PFCP Header.
> >  - ``seid``: session endpoint identifier.
> >  - Default ``mask`` matches s_field and seid.
> >
> > +Item: ``ECPRI``
> > +^^^^^^^^^^^^^
> > +
> > +Matches a eCPRI header.
> > +
> > +- ``hdr``: eCPRI header definition (``rte_ecpri.h``).
> > +- Default ``mask`` matches message type of common header only.
> > +
> >  Actions
> >  ~~~~~~~
> >
> > diff --git a/doc/guides/rel_notes/release_20_08.rst
> > b/doc/guides/rel_notes/release_20_08.rst
> > index 988474c..19feb68 100644
> > --- a/doc/guides/rel_notes/release_20_08.rst
> > +++ b/doc/guides/rel_notes/release_20_08.rst
> > @@ -184,6 +184,11 @@ New Features
> >    which are used to access packet data in a safe manner. Currently
> JIT support
> >    for these instructions is implemented for x86 only.
> >
> > +* **Added eCPRI protocol support in rte_flow.**
> > +
> > +  The ``ECPRI`` item have been added to support eCPRI packet
> > + offloading for  5G network.
> > +
> >  * **Added flow performance test application.**
> >
> >    Added new application to test ``rte_flow`` performance, including:
> > diff --git a/lib/librte_ethdev/rte_flow.c
> > b/lib/librte_ethdev/rte_flow.c index 1685be5..f8fdd68 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -95,6 +95,7 @@ struct rte_flow_desc_data {
> >  	MK_FLOW_ITEM(HIGIG2, sizeof(struct
> rte_flow_item_higig2_hdr)),
> >  	MK_FLOW_ITEM(L2TPV3OIP, sizeof(struct
> rte_flow_item_l2tpv3oip)),
> >  	MK_FLOW_ITEM(PFCP, sizeof(struct rte_flow_item_pfcp)),
> > +	MK_FLOW_ITEM(ECPRI, sizeof(struct rte_flow_item_ecpri)),
> >  };
> >
> >  /** Generate flow_action[] entry. */
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index b0e4199..8a90226 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -28,6 +28,7 @@
> >  #include <rte_byteorder.h>
> >  #include <rte_esp.h>
> >  #include <rte_higig.h>
> > +#include <rte_ecpri.h>
> >  #include <rte_mbuf.h>
> >  #include <rte_mbuf_dyn.h>
> >
> > @@ -527,6 +528,15 @@ enum rte_flow_item_type {
> >  	 */
> >  	RTE_FLOW_ITEM_TYPE_PFCP,
> >
> > +	/**
> > +	 * Matches eCPRI Header.
> > +	 *
> > +	 * Configure flow for eCPRI over ETH or UDP packets.
> > +	 *
> > +	 * See struct rte_flow_item_ecpri.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_ECPRI,
> > +
> >  };
> >
> >  /**
> > @@ -1547,6 +1557,27 @@ struct rte_flow_item_pfcp {  #endif
> >
> >  /**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior
> notice
> > + *
> > + * RTE_FLOW_ITEM_TYPE_ECPRI
> > + *
> > + * Match eCPRI Header
> > + */
> > +struct rte_flow_item_ecpri {
> > +	struct rte_ecpri_msg_hdr hdr;
> > +};
> > +
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_ECPRI. */ #ifndef
> __cplusplus
> > +static const struct rte_flow_item_ecpri rte_flow_item_ecpri_mask =
> {
> > +	.hdr = {
> > +		.dw0 = 0x0,
> > +	},
> > +};
> > +#endif
> > +
> > +/**
> >   * Matching pattern item definition.
> >   *
> >   * A pattern is formed by stacking items starting from the lowest
> > protocol diff --git a/lib/librte_net/Makefile
> > b/lib/librte_net/Makefile index aa1d6fe..9830e77 100644
> > --- a/lib/librte_net/Makefile
> > +++ b/lib/librte_net/Makefile
> > @@ -20,5 +20,6 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include
> +=
> > rte_sctp.h rte_icmp.h rte_arp.h
> > SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_ether.h
> rte_gre.h
> > rte_net.h  SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include +=
> rte_net_crc.h
> > rte_mpls.h rte_higig.h  SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include
> +=
> > rte_gtp.h rte_vxlan.h
> > +SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_ecpri.h
> >
> >  include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/lib/librte_net/meson.build b/lib/librte_net/meson.build
> > index f799349..24ed825 100644
> > --- a/lib/librte_net/meson.build
> > +++ b/lib/librte_net/meson.build
> > @@ -15,7 +15,8 @@ headers = files('rte_ip.h',
> >  	'rte_net.h',
> >  	'rte_net_crc.h',
> >  	'rte_mpls.h',
> > -	'rte_higig.h')
> > +	'rte_higig.h',
> > +	'rte_ecpri.h')
> >
> >  sources = files('rte_arp.c', 'rte_ether.c', 'rte_net.c',
> > 'rte_net_crc.c')  deps += ['mbuf'] diff --git
> > a/lib/librte_net/rte_ecpri.h b/lib/librte_net/rte_ecpri.h new file
> > mode 100644 index 0000000..60fb4f7
> > --- /dev/null
> > +++ b/lib/librte_net/rte_ecpri.h
> > @@ -0,0 +1,182 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2020 Mellanox Technologies, Ltd  */
> > +
> > +#ifndef _RTE_ECPRI_H_
> > +#define _RTE_ECPRI_H_
> > +
> > +#include <stdint.h>
> > +#include <rte_byteorder.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * eCPRI Protocol Revision 1.0, 1.1, 1.2, 2.0: 0001b
> > + * Other values are reserved for future  */
> > +#define RTE_ECPRI_REV_UP_TO_20		1
> > +
> > +/**
> > + * eCPRI message types in specifications
> > + * IWF* types will only be supported from rev.2
> > + * 12-63: Reserved for future revision
> > + * 64-255: Vendor Specific
> > + */
> > +#define RTE_ECPRI_MSG_TYPE_IQ_DATA	0
> 
> Here the doxygen comment applies to
> RTE_ECPRI_MSG_TYPE_IQ_DATA.
> 
> I think it should either be a standard comment (no doxygen), or
> something more complex should be done, like grouping:
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> stackoverflow.com%2Fquestions%2F30803156%2Fgroup-level-
> documentation-for-preprocessor-defines-in-
> doxygen&amp;data=02%7C01%7Cbingz%40mellanox.com%7C99fe8c9
> 70b6648d82f7908d824dded33%7Ca652971c7d2e4d9ba6a4d149256f46
> 1b%7C0%7C1%7C637299882879478151&amp;sdata=0BGV9rUpTYPpKA
> Thq9BgixZCtnlMVmILVty0f37xJ7I%3D&amp;reserved=0
> (I didn't try)
> 

Thanks, I will have a try to fix this comments.

> 
> > +#define RTE_ECPRI_MSG_TYPE_BIT_SEQ	1
> > +#define RTE_ECPRI_MSG_TYPE_RTC_CTRL	2
> > +#define RTE_ECPRI_MSG_TYPE_GEN_DATA	3
> > +#define RTE_ECPRI_MSG_TYPE_RM_ACC	4
> > +#define RTE_ECPRI_MSG_TYPE_DLY_MSR	5
> > +#define RTE_ECPRI_MSG_TYPE_RMT_RST	6
> > +#define RTE_ECPRI_MSG_TYPE_EVT_IND	7
> > +#define RTE_ECPRI_MSG_TYPE_IWF_UP	8
> > +#define RTE_ECPRI_MSG_TYPE_IWF_OPT	9
> > +#define RTE_ECPRI_MSG_TYPE_IWF_MAP	10
> > +#define RTE_ECPRI_MSG_TYPE_IWF_DCTRL	11
> > +
> > +/**
> > + * Event Type of Message Type #7: Event Indication
> > + * 0x00: Fault(s) Indication
> > + * 0x01: Fault(s) Indication Acknowledge
> > + * 0x02: Notification(s) Indication
> > + * 0x03: Synchronization Request
> > + * 0x04: Synchronization Acknowledge
> > + * 0x05: Synchronization End Indication
> > + * 0x06...0xFF: Reserved
> > + */
> > +#define RTE_ECPRI_EVT_IND_FAULT_IND	0
> > +#define RTE_ECPRI_EVT_IND_FAULT_ACK	1
> > +#define RTE_ECPRI_EVT_IND_NTFY_IND	2
> > +#define RTE_ECPRI_EVT_IND_SYNC_REQ	3
> > +#define RTE_ECPRI_EVT_IND_SYNC_ACK	4
> > +#define RTE_ECPRI_EVT_IND_SYNC_END	5
> > +
> > +/**
> > + * eCPRI Common Header
> > + */
> > +RTE_STD_C11
> > +struct rte_ecpri_common_hdr {
> > +#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 */
> > +#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
> > +} __rte_packed;
> 
> Does it really need to be packed? Why next types do not need it?
> It looks only those which have bitfields are.
> 

Nice catch, thanks. For the common header, there is no need to use
the packed attribute, because it is a u32 and the bitfields will be
aligned.
I checked all the definitions again. Only " Type #4: Remote Memory Access"
needs to use the packed attribute.
For other sub-types, "sub-header" part of the message payload will get
aligned by nature. For example, u16 after u16, u8 after u16, these should
be OK.
But in type #4, the address is 48bits wide, with 16bits MSB and 32bits LSB (no
detailed description in the specification, correct me if anything wrong.) Usually,
the 48bits address will be devided as this in a system. And there is no 48-bits
type at all. So we need to define two parts for it: 32b LSB follows 16b MSB.
u32 after u16 should be with packed attribute. Thanks

> 
> I wonder if the 'dw0' could be in this definition instead of in struct
> rte_ecpri_msg_hdr?
> 
> Something like this:
> 
> struct rte_ecpri_common_hdr {
> 	union {
> 		uint32_t u32;
> 		struct {
> 			...
> 		};
> 	};
> };
> 
> I see 2 advantages:
> 
> - someone that only uses the common_hdr struct can use the .u32
>   in its software
> - when using it in messages, it looks clearer to me:
>     msg.common_hdr.u32 = value;
>   instead of:
>     msg.dw0 = value;
> 
> What do you think?

Thanks for the suggestion, this is much better, I will change it.
Indeed, in my original version, no DW(u32) is defined for the header.
After that, I noticed that it would not be easy for the static casting to a u32
from bitfield(the compiler will complain), and it would not be clear to
swap the endian if the user wants to use this header. I added this DW(u32)
to simplify the usage of this header. But yes, if I do not add it here, it would
be not easy or clear for users who just use this header structure.
I will change it. Is it OK if I use the name "dw0"?

> 
> > +
> > +/**
> > + * eCPRI Message Header of Type #0: IQ Data  */ struct
> > +rte_ecpri_msg_iq_data {
> > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > +	rte_be16_t seq_id;		/**< Sequence ID */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #1: Bit Sequence  */ struct
> > +rte_ecpri_msg_bit_seq {
> > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > +	rte_be16_t seq_id;		/**< Sequence ID */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #2: Real-Time Control Data  */
> struct
> > +rte_ecpri_msg_rtc_ctrl {
> > +	rte_be16_t rtc_id;		/**< Real-Time Control Data ID
> */
> > +	rte_be16_t seq_id;		/**< Sequence ID */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #3: Generic Data Transfer  */
> struct
> > +rte_ecpri_msg_gen_data {
> > +	rte_be32_t pc_id;		/**< Physical channel ID */
> > +	rte_be32_t seq_id;		/**< Sequence ID */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #4: Remote Memory Access  */
> > +RTE_STD_C11
> > +struct rte_ecpri_msg_rm_access {
> > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > +	uint32_t ele_id:16;		/**< Element ID */
> > +	uint32_t rr:4;			/**< Req/Resp */
> > +	uint32_t rw:4;			/**< Read/Write */
> > +	uint32_t rma_id:8;		/**< Remote Memory Access
> ID */
> > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > +	uint32_t rma_id:8;		/**< Remote Memory Access
> ID */
> > +	uint32_t rw:4;			/**< Read/Write */
> > +	uint32_t rr:4;			/**< Req/Resp */
> > +	uint32_t ele_id:16;		/**< Element ID */
> > +#endif
> > +	rte_be16_t addr_m;		/**< 48-bits address (16 MSB)
> */
> > +	rte_be32_t addr_l;		/**< 48-bits address (32 LSB) */
> > +	rte_be16_t length;		/**< number of bytes */
> > +} __rte_packed;
> > +
> > +/**
> > + * eCPRI Message Header of Type #5: One-Way Delay Measurement
> */
> > +struct rte_ecpri_msg_delay_measure {
> > +	uint8_t msr_id;			/**< Measurement ID */
> > +	uint8_t act_type;		/**< Action Type */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #6: Remote Reset  */ struct
> > +rte_ecpri_msg_remote_reset {
> > +	rte_be16_t rst_id;		/**< Reset ID */
> > +	uint8_t rst_op;			/**< Reset Code Op */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #7: Event Indication  */ struct
> > +rte_ecpri_msg_event_ind {
> > +	uint8_t evt_id;			/**< Event ID */
> > +	uint8_t evt_type;		/**< Event Type */
> > +	uint8_t seq;			/**< Sequence Number */
> > +	uint8_t number;			/**< Number of
> Faults/Notif */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header Format: Common Header + Message
> Types  */
> > +RTE_STD_C11
> > +struct rte_ecpri_msg_hdr {
> > +	union {
> > +		struct rte_ecpri_common_hdr common;
> > +		uint32_t dw0;
> > +	};
> > +	union {
> > +		struct rte_ecpri_msg_iq_data type0;
> > +		struct rte_ecpri_msg_bit_seq type1;
> > +		struct rte_ecpri_msg_rtc_ctrl type2;
> > +		struct rte_ecpri_msg_bit_seq type3;
> > +		struct rte_ecpri_msg_rm_access type4;
> > +		struct rte_ecpri_msg_delay_measure type5;
> > +		struct rte_ecpri_msg_remote_reset type6;
> > +		struct rte_ecpri_msg_event_ind type7;
> > +		uint32_t dummy[3];
> > +	};
> > +};
> 
> What is the point in having this struct?
> 
> From a software point of view, I think it is a bit risky, because its size is
> the size of the largest message. This is probably what you want in your
> case, but when a software will rx or tx such packet, I think they
> shouldn't use this one. My understanding is that you only need this
> structure for the mask in rte_flow.
> 
> Also, I'm not sure to understand the purpose of dummy[3], even after
> reading your answer to Akhil's question.
> 

Basically YES and no. To my understanding, the eCPRI message format is something
like the ICMP packet format. The message (packet) itself will be parsed into
different formats based on the type of the common header. In the message
payload part, there is no distinct definition of the "sub-header". We can divide
them into the sub-header and data parts based on the specification.
E.g. physical channel ID / real-time control ID / Event ID + type are the parts
that datapath forwarding will only care about. The following timestamp or user data
parts are the parts that the higher layer in the application will use.
1. If an application wants to create some offload flow, or even handle it in the SW, the
common header + first several bytes in the payload should be enough. BUT YES, it is
not good or safe to use it in the higher layer of the application.
2. A higher layer of the application should have its own definition of the whole payload
of a specific sub-type, including the parsing of the user data part after the "sub-header".
It is better for them just skip the first 4 bytes of the eCPRI message or a known offset.
We do not need to cover the upper layers. 
I think some comments could be added here, is it OK?
3. Regarding this structure, I add it because I do not want to introduce a lot of new items
in the rte_flow: new items with structures, new enum types. I prefer one single structure
will cover most of the cases (subtypes). What do you think?
4. About the *dummy* u32, I calculated all the "subheaders" and choose the maximal value
of the length. Two purposes (same as the u32 in the common header):
  a. easy to swap the endianness, but not quite useful. Because some parts are u16 and u8,
    and should not be swapped in a u32. (some physical channel ID and address LSB have 32bits width)
    But if some HW parsed the header u32 by u32, then it would be helpful.
  b. easy for checking in flow API, if the user wants to insert a flow. Some checking should
      be done to confirm if it is wildcard flow (all eCPRI messages or eCPRI message in some specific type),
      or some precise flow (to match the pc id or rtc id, for example). With these fields, 3 DW
      of the masks only need to be check before continuing. Or else, the code needs to check the type
      and a lot of switch-case conditions and go through all different members of each header.

> 
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_ECPRI_H_ */
> > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> > index 0ae4e75..184a3f9 100644
> > --- a/lib/librte_net/rte_ether.h
> > +++ b/lib/librte_net/rte_ether.h
> > @@ -304,6 +304,7 @@ struct rte_vlan_hdr {  #define
> RTE_ETHER_TYPE_LLDP
> > 0x88CC /**< LLDP Protocol. */  #define RTE_ETHER_TYPE_MPLS
> 0x8847 /**<
> > MPLS ethertype. */  #define RTE_ETHER_TYPE_MPLSM 0x8848 /**<
> MPLS
> > multicast ethertype. */
> > +#define RTE_ETHER_TYPE_ECPRI 0xAEFE /**< eCPRI ethertype (.1Q
> > +supported). */
> >
> >  /**
> >   * Extract VLAN tag information into mbuf
> > --
> > 1.8.3.1
> >
  

Hi Bing,

On Sat, Jul 11, 2020 at 04:25:49AM +0000, Bing Zhao wrote:
> Hi Olivier,
> Many thanks for your comments.

[...]

> > > +/**
> > > + * eCPRI Common Header
> > > + */
> > > +RTE_STD_C11
> > > +struct rte_ecpri_common_hdr {
> > > +#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 */
> > > +#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
> > > +} __rte_packed;
> > 
> > Does it really need to be packed? Why next types do not need it?
> > It looks only those which have bitfields are.
> > 
> 
> Nice catch, thanks. For the common header, there is no need to use
> the packed attribute, because it is a u32 and the bitfields will be
> aligned.
> I checked all the definitions again. Only " Type #4: Remote Memory Access"
> needs to use the packed attribute.
> For other sub-types, "sub-header" part of the message payload will get
> aligned by nature. For example, u16 after u16, u8 after u16, these should
> be OK.
> But in type #4, the address is 48bits wide, with 16bits MSB and 32bits LSB (no
> detailed description in the specification, correct me if anything wrong.) Usually,
> the 48bits address will be devided as this in a system. And there is no 48-bits
> type at all. So we need to define two parts for it: 32b LSB follows 16b MSB.
> u32 after u16 should be with packed attribute. Thanks

What about using a bitfield into a uint64_t ? I mean:

	struct rte_ecpri_msg_rm_access {
if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
		...
		uint64_t length:16;		/**< number of bytes */
		uint64_t addr:48;		/**< address */
#else
		...
		uint64_t addr:48;		/**< address */
		uint64_t length:16;		/**< number of bytes */
#endif
	};


> 
> > 
> > I wonder if the 'dw0' could be in this definition instead of in struct
> > rte_ecpri_msg_hdr?
> > 
> > Something like this:
> > 
> > struct rte_ecpri_common_hdr {
> > 	union {
> > 		uint32_t u32;
> > 		struct {
> > 			...
> > 		};
> > 	};
> > };
> > 
> > I see 2 advantages:
> > 
> > - someone that only uses the common_hdr struct can use the .u32
> >   in its software
> > - when using it in messages, it looks clearer to me:
> >     msg.common_hdr.u32 = value;
> >   instead of:
> >     msg.dw0 = value;
> > 
> > What do you think?
> 
> Thanks for the suggestion, this is much better, I will change it.
> Indeed, in my original version, no DW(u32) is defined for the header.
> After that, I noticed that it would not be easy for the static casting to a u32
> from bitfield(the compiler will complain), and it would not be clear to
> swap the endian if the user wants to use this header. I added this DW(u32)
> to simplify the usage of this header. But yes, if I do not add it here, it would
> be not easy or clear for users who just use this header structure.
> I will change it. Is it OK if I use the name "dw0"?

In my opinion, u32 is more usual than dw0.

> 
> > 
> > > +
> > > +/**
> > > + * eCPRI Message Header of Type #0: IQ Data  */ struct
> > > +rte_ecpri_msg_iq_data {
> > > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > +};
> > > +
> > > +/**
> > > + * eCPRI Message Header of Type #1: Bit Sequence  */ struct
> > > +rte_ecpri_msg_bit_seq {
> > > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > +};
> > > +
> > > +/**
> > > + * eCPRI Message Header of Type #2: Real-Time Control Data  */
> > struct
> > > +rte_ecpri_msg_rtc_ctrl {
> > > +	rte_be16_t rtc_id;		/**< Real-Time Control Data ID
> > */
> > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > +};
> > > +
> > > +/**
> > > + * eCPRI Message Header of Type #3: Generic Data Transfer  */
> > struct
> > > +rte_ecpri_msg_gen_data {
> > > +	rte_be32_t pc_id;		/**< Physical channel ID */
> > > +	rte_be32_t seq_id;		/**< Sequence ID */
> > > +};
> > > +
> > > +/**
> > > + * eCPRI Message Header of Type #4: Remote Memory Access  */
> > > +RTE_STD_C11
> > > +struct rte_ecpri_msg_rm_access {
> > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > +	uint32_t ele_id:16;		/**< Element ID */
> > > +	uint32_t rr:4;			/**< Req/Resp */
> > > +	uint32_t rw:4;			/**< Read/Write */
> > > +	uint32_t rma_id:8;		/**< Remote Memory Access
> > ID */
> > > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > > +	uint32_t rma_id:8;		/**< Remote Memory Access
> > ID */
> > > +	uint32_t rw:4;			/**< Read/Write */
> > > +	uint32_t rr:4;			/**< Req/Resp */
> > > +	uint32_t ele_id:16;		/**< Element ID */
> > > +#endif
> > > +	rte_be16_t addr_m;		/**< 48-bits address (16 MSB)
> > */
> > > +	rte_be32_t addr_l;		/**< 48-bits address (32 LSB) */
> > > +	rte_be16_t length;		/**< number of bytes */
> > > +} __rte_packed;
> > > +
> > > +/**
> > > + * eCPRI Message Header of Type #5: One-Way Delay Measurement
> > */
> > > +struct rte_ecpri_msg_delay_measure {
> > > +	uint8_t msr_id;			/**< Measurement ID */
> > > +	uint8_t act_type;		/**< Action Type */
> > > +};
> > > +
> > > +/**
> > > + * eCPRI Message Header of Type #6: Remote Reset  */ struct
> > > +rte_ecpri_msg_remote_reset {
> > > +	rte_be16_t rst_id;		/**< Reset ID */
> > > +	uint8_t rst_op;			/**< Reset Code Op */
> > > +};
> > > +
> > > +/**
> > > + * eCPRI Message Header of Type #7: Event Indication  */ struct
> > > +rte_ecpri_msg_event_ind {
> > > +	uint8_t evt_id;			/**< Event ID */
> > > +	uint8_t evt_type;		/**< Event Type */
> > > +	uint8_t seq;			/**< Sequence Number */
> > > +	uint8_t number;			/**< Number of
> > Faults/Notif */
> > > +};
> > > +
> > > +/**
> > > + * eCPRI Message Header Format: Common Header + Message
> > Types  */
> > > +RTE_STD_C11
> > > +struct rte_ecpri_msg_hdr {
> > > +	union {
> > > +		struct rte_ecpri_common_hdr common;
> > > +		uint32_t dw0;
> > > +	};
> > > +	union {
> > > +		struct rte_ecpri_msg_iq_data type0;
> > > +		struct rte_ecpri_msg_bit_seq type1;
> > > +		struct rte_ecpri_msg_rtc_ctrl type2;
> > > +		struct rte_ecpri_msg_bit_seq type3;
> > > +		struct rte_ecpri_msg_rm_access type4;
> > > +		struct rte_ecpri_msg_delay_measure type5;
> > > +		struct rte_ecpri_msg_remote_reset type6;
> > > +		struct rte_ecpri_msg_event_ind type7;
> > > +		uint32_t dummy[3];
> > > +	};
> > > +};
> > 
> > What is the point in having this struct?
> > 
> > From a software point of view, I think it is a bit risky, because its size is
> > the size of the largest message. This is probably what you want in your
> > case, but when a software will rx or tx such packet, I think they
> > shouldn't use this one. My understanding is that you only need this
> > structure for the mask in rte_flow.
> > 
> > Also, I'm not sure to understand the purpose of dummy[3], even after
> > reading your answer to Akhil's question.
> > 
> 
> Basically YES and no. To my understanding, the eCPRI message format is something
> like the ICMP packet format. The message (packet) itself will be parsed into
> different formats based on the type of the common header. In the message
> payload part, there is no distinct definition of the "sub-header". We can divide
> them into the sub-header and data parts based on the specification.
> E.g. physical channel ID / real-time control ID / Event ID + type are the parts
> that datapath forwarding will only care about. The following timestamp or user data
> parts are the parts that the higher layer in the application will use.
> 1. If an application wants to create some offload flow, or even handle it in the SW, the
> common header + first several bytes in the payload should be enough. BUT YES, it is
> not good or safe to use it in the higher layer of the application.
> 2. A higher layer of the application should have its own definition of the whole payload
> of a specific sub-type, including the parsing of the user data part after the "sub-header".
> It is better for them just skip the first 4 bytes of the eCPRI message or a known offset.
> We do not need to cover the upper layers. 

Let me explain what is my vision of how an application would use the
structures (these are completly dummy examples, as I don't know ecpri
protocol at all).

Rx:

	int ecpri_input(struct rte_mbuf *m)
	{
		struct rte_ecpri_common_hdr hdr_copy, *hdr;
		struct rte_ecpri_msg_event_ind event_copy, *event;
		struct app_specific app_copy, *app;

		hdr = rte_pktmbuf_read(m, 0, sizeof(*hdr), &hdr_copy);
		if (unlikely(hdr == NULL))
			return -1;
		switch (hdr->type) {
		...
		case RTE_ECPRI_EVT_IND_NTFY_IND:
			event = rte_pktmbuf_read(m, sizeof(*hdr), sizeof(*event),
				&event_copy);
			if (unlikely(event == NULL))
				return -1;
			...
			app = rte_pktmbuf_read(m, sizeof(*app),
				sizeof(*hdr) + sizeof(*event),
				&app_copy);
			...

Tx:

	int ecpri_output(void)
	{
		struct rte_ecpri_common_hdr *hdr;
		struct rte_ecpri_msg_event_ind *event;
		struct app_specific *app;

		m = rte_pktmbuf_alloc(mp);
		if (unlikely(m == NULL))
			return -1;

		app = rte_pktmbuf_append(m, sizeof(*data));
		if (app == NULL)
			...
		app->... = ...;
		...
		event = rte_pktmbuf_prepend(m, sizeof(*event));
		if (event == NULL)
			...
		event->... = ...;
		...
		hdr = rte_pktmbuf_prepend(m, sizeof(*hdr));
		if (hdr == NULL)
			...
		hdr->... = ...;

		return packet_send(m);
	}

In these 2 examples, we never need the unioned structure (struct
rte_ecpri_msg_hdr).

Using it does not look possible to me, because its size is corresponds
to the largest message, not to the one we are parsing/building.

> I think some comments could be added here, is it OK?
> 3. Regarding this structure, I add it because I do not want to introduce a lot of new items
> in the rte_flow: new items with structures, new enum types. I prefer one single structure
> will cover most of the cases (subtypes). What do you think?
> 4. About the *dummy* u32, I calculated all the "subheaders" and choose the maximal value
> of the length. Two purposes (same as the u32 in the common header):
>   a. easy to swap the endianness, but not quite useful. Because some parts are u16 and u8,
>     and should not be swapped in a u32. (some physical channel ID and address LSB have 32bits width)
>     But if some HW parsed the header u32 by u32, then it would be helpful.
>   b. easy for checking in flow API, if the user wants to insert a flow. Some checking should
>       be done to confirm if it is wildcard flow (all eCPRI messages or eCPRI message in some specific type),
>       or some precise flow (to match the pc id or rtc id, for example). With these fields, 3 DW
>       of the masks only need to be check before continuing. Or else, the code needs to check the type
>       and a lot of switch-case conditions and go through all different members of each header.

Thanks for the clarification.

I'll tend to say that if the rte_ecpri_msg_hdr structure is only
useful for rte_flow, it should be defined inside rte_flow.

However, I have some fears about the dummy[3]. You said it could be
enlarged later: I think it is dangerous to change the size of a
structure that may be used to parse data (and this would be an ABI
change). Also, it seems dummy[3] cannot be used easily to swap
endianness, so is it really useful?


Thanks,
Olivier


> > > +
> > > +#ifdef __cplusplus
> > > +}
> > > +#endif
> > > +
> > > +#endif /* _RTE_ECPRI_H_ */
> > > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> > > index 0ae4e75..184a3f9 100644
> > > --- a/lib/librte_net/rte_ether.h
> > > +++ b/lib/librte_net/rte_ether.h
> > > @@ -304,6 +304,7 @@ struct rte_vlan_hdr {  #define
> > RTE_ETHER_TYPE_LLDP
> > > 0x88CC /**< LLDP Protocol. */  #define RTE_ETHER_TYPE_MPLS
> > 0x8847 /**<
> > > MPLS ethertype. */  #define RTE_ETHER_TYPE_MPLSM 0x8848 /**<
> > MPLS
> > > multicast ethertype. */
> > > +#define RTE_ETHER_TYPE_ECPRI 0xAEFE /**< eCPRI ethertype (.1Q
> > > +supported). */
> > >
> > >  /**
> > >   * Extract VLAN tag information into mbuf
> > > --
> > > 1.8.3.1
> > >
>
  

Hi Olivier,
Thanks

BR. Bing

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Sunday, July 12, 2020 9:18 PM
> To: Bing Zhao <bingz@mellanox.com>
> Cc: Ori Kam <orika@mellanox.com>; john.mcnamara@intel.com;
> marko.kovacevic@intel.com; Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> arybchenko@solarflare.com; akhil.goyal@nxp.com; dev@dpdk.org;
> wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com
> Subject: Re: [PATCH v5 1/2] rte_flow: add eCPRI key fields to flow API
> 
> Hi Bing,
> 
> On Sat, Jul 11, 2020 at 04:25:49AM +0000, Bing Zhao wrote:
> > Hi Olivier,
> > Many thanks for your comments.
> 
> [...]
> 
> > > > +/**
> > > > + * eCPRI Common Header
> > > > + */
> > > > +RTE_STD_C11
> > > > +struct rte_ecpri_common_hdr {
> > > > +#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 */
> > > > +#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
> > > > +} __rte_packed;
> > >
> > > Does it really need to be packed? Why next types do not need it?
> > > It looks only those which have bitfields are.
> > >
> >
> > Nice catch, thanks. For the common header, there is no need to use
> the
> > packed attribute, because it is a u32 and the bitfields will be
> > aligned.
> > I checked all the definitions again. Only " Type #4: Remote Memory
> Access"
> > needs to use the packed attribute.
> > For other sub-types, "sub-header" part of the message payload will
> get
> > aligned by nature. For example, u16 after u16, u8 after u16, these
> > should be OK.
> > But in type #4, the address is 48bits wide, with 16bits MSB and 32bits
> > LSB (no detailed description in the specification, correct me if
> > anything wrong.) Usually, the 48bits address will be devided as this
> > in a system. And there is no 48-bits type at all. So we need to define
> two parts for it: 32b LSB follows 16b MSB.
> > u32 after u16 should be with packed attribute. Thanks
> 
> What about using a bitfield into a uint64_t ? I mean:
> 
> 	struct rte_ecpri_msg_rm_access {
> if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> 		...
> 		uint64_t length:16;		/**< number of bytes
> */
> 		uint64_t addr:48;		/**< address */
> #else
> 		...
> 		uint64_t addr:48;		/**< address */
> 		uint64_t length:16;		/**< number of bytes
> */
> #endif
> 	};
> 

Thanks for your suggestion.
https://stackoverflow.com/questions/10906238/warning-when-using-bitfield-with-unsigned-char
AFAIK (from this page), bitfields support only support bool and int. uint64_t is some type of "long"
and most of the compilers should support it. But I am not sure if it is a standard implementation.

> 
> >
> > >
> > > I wonder if the 'dw0' could be in this definition instead of in
> > > struct rte_ecpri_msg_hdr?
> > >
> > > Something like this:
> > >
> > > struct rte_ecpri_common_hdr {
> > > 	union {
> > > 		uint32_t u32;
> > > 		struct {
> > > 			...
> > > 		};
> > > 	};
> > > };
> > >
> > > I see 2 advantages:
> > >
> > > - someone that only uses the common_hdr struct can use the .u32
> > >   in its software
> > > - when using it in messages, it looks clearer to me:
> > >     msg.common_hdr.u32 = value;
> > >   instead of:
> > >     msg.dw0 = value;
> > >
> > > What do you think?
> >
> > Thanks for the suggestion, this is much better, I will change it.
> > Indeed, in my original version, no DW(u32) is defined for the header.
> > After that, I noticed that it would not be easy for the static casting
> > to a u32 from bitfield(the compiler will complain), and it would not
> > be clear to swap the endian if the user wants to use this header. I
> > added this DW(u32) to simplify the usage of this header. But yes, if I
> > do not add it here, it would be not easy or clear for users who just
> use this header structure.
> > I will change it. Is it OK if I use the name "dw0"?
> 
> In my opinion, u32 is more usual than dw0.
> 

I sent patch set v6 with this change, thanks.

> >
> > >
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header of Type #0: IQ Data  */ struct
> > > > +rte_ecpri_msg_iq_data {
> > > > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > > +};
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header of Type #1: Bit Sequence  */ struct
> > > > +rte_ecpri_msg_bit_seq {
> > > > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > > +};
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header of Type #2: Real-Time Control Data  */
> > > struct
> > > > +rte_ecpri_msg_rtc_ctrl {
> > > > +	rte_be16_t rtc_id;		/**< Real-Time Control Data ID
> > > */
> > > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > > +};
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header of Type #3: Generic Data Transfer  */
> > > struct
> > > > +rte_ecpri_msg_gen_data {
> > > > +	rte_be32_t pc_id;		/**< Physical channel ID */
> > > > +	rte_be32_t seq_id;		/**< Sequence ID */
> > > > +};
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header of Type #4: Remote Memory Access
> */
> > > > +RTE_STD_C11
> > > > +struct rte_ecpri_msg_rm_access {
> > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > > +	uint32_t ele_id:16;		/**< Element ID */
> > > > +	uint32_t rr:4;			/**< Req/Resp */
> > > > +	uint32_t rw:4;			/**< Read/Write */
> > > > +	uint32_t rma_id:8;		/**< Remote Memory Access
> > > ID */
> > > > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > > > +	uint32_t rma_id:8;		/**< Remote Memory Access
> > > ID */
> > > > +	uint32_t rw:4;			/**< Read/Write */
> > > > +	uint32_t rr:4;			/**< Req/Resp */
> > > > +	uint32_t ele_id:16;		/**< Element ID */
> > > > +#endif
> > > > +	rte_be16_t addr_m;		/**< 48-bits address (16 MSB)
> > > */
> > > > +	rte_be32_t addr_l;		/**< 48-bits address (32 LSB) */
> > > > +	rte_be16_t length;		/**< number of bytes */
> > > > +} __rte_packed;
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header of Type #5: One-Way Delay
> Measurement
> > > */
> > > > +struct rte_ecpri_msg_delay_measure {
> > > > +	uint8_t msr_id;			/**< Measurement ID */
> > > > +	uint8_t act_type;		/**< Action Type */
> > > > +};
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header of Type #6: Remote Reset  */ struct
> > > > +rte_ecpri_msg_remote_reset {
> > > > +	rte_be16_t rst_id;		/**< Reset ID */
> > > > +	uint8_t rst_op;			/**< Reset Code Op */
> > > > +};
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header of Type #7: Event Indication  */ struct
> > > > +rte_ecpri_msg_event_ind {
> > > > +	uint8_t evt_id;			/**< Event ID */
> > > > +	uint8_t evt_type;		/**< Event Type */
> > > > +	uint8_t seq;			/**< Sequence Number */
> > > > +	uint8_t number;			/**< Number of
> > > Faults/Notif */
> > > > +};
> > > > +
> > > > +/**
> > > > + * eCPRI Message Header Format: Common Header + Message
> > > Types  */
> > > > +RTE_STD_C11
> > > > +struct rte_ecpri_msg_hdr {
> > > > +	union {
> > > > +		struct rte_ecpri_common_hdr common;
> > > > +		uint32_t dw0;
> > > > +	};
> > > > +	union {
> > > > +		struct rte_ecpri_msg_iq_data type0;
> > > > +		struct rte_ecpri_msg_bit_seq type1;
> > > > +		struct rte_ecpri_msg_rtc_ctrl type2;
> > > > +		struct rte_ecpri_msg_bit_seq type3;
> > > > +		struct rte_ecpri_msg_rm_access type4;
> > > > +		struct rte_ecpri_msg_delay_measure type5;
> > > > +		struct rte_ecpri_msg_remote_reset type6;
> > > > +		struct rte_ecpri_msg_event_ind type7;
> > > > +		uint32_t dummy[3];
> > > > +	};
> > > > +};
> > >
> > > What is the point in having this struct?
> > >
> > > From a software point of view, I think it is a bit risky, because
> > > its size is the size of the largest message. This is probably what
> > > you want in your case, but when a software will rx or tx such
> > > packet, I think they shouldn't use this one. My understanding is
> > > that you only need this structure for the mask in rte_flow.
> > >
> > > Also, I'm not sure to understand the purpose of dummy[3], even
> after
> > > reading your answer to Akhil's question.
> > >
> >
> > Basically YES and no. To my understanding, the eCPRI message
> format is
> > something like the ICMP packet format. The message (packet) itself
> > will be parsed into different formats based on the type of the
> common
> > header. In the message payload part, there is no distinct definition
> > of the "sub-header". We can divide them into the sub-header and
> data parts based on the specification.
> > E.g. physical channel ID / real-time control ID / Event ID + type are
> > the parts that datapath forwarding will only care about. The
> following
> > timestamp or user data parts are the parts that the higher layer in
> the application will use.
> > 1. If an application wants to create some offload flow, or even
> handle
> > it in the SW, the common header + first several bytes in the payload
> > should be enough. BUT YES, it is not good or safe to use it in the
> higher layer of the application.
> > 2. A higher layer of the application should have its own definition of
> > the whole payload of a specific sub-type, including the parsing of the
> user data part after the "sub-header".
> > It is better for them just skip the first 4 bytes of the eCPRI message or
> a known offset.
> > We do not need to cover the upper layers.
> 
> Let me explain what is my vision of how an application would use the
> structures (these are completly dummy examples, as I don't know
> ecpri protocol at all).
> 
> Rx:
> 
> 	int ecpri_input(struct rte_mbuf *m)
> 	{
> 		struct rte_ecpri_common_hdr hdr_copy, *hdr;
> 		struct rte_ecpri_msg_event_ind event_copy, *event;
> 		struct app_specific app_copy, *app;
> 
> 		hdr = rte_pktmbuf_read(m, 0, sizeof(*hdr),
> &hdr_copy);
> 		if (unlikely(hdr == NULL))
> 			return -1;
> 		switch (hdr->type) {
> 		...
> 		case RTE_ECPRI_EVT_IND_NTFY_IND:
> 			event = rte_pktmbuf_read(m, sizeof(*hdr),
> sizeof(*event),
> 				&event_copy);
> 			if (unlikely(event == NULL))
> 				return -1;
> 			...
> 			app = rte_pktmbuf_read(m, sizeof(*app),
> 				sizeof(*hdr) + sizeof(*event),
> 				&app_copy);
> 			...
> 
> Tx:
> 
> 	int ecpri_output(void)
> 	{
> 		struct rte_ecpri_common_hdr *hdr;
> 		struct rte_ecpri_msg_event_ind *event;
> 		struct app_specific *app;
> 
> 		m = rte_pktmbuf_alloc(mp);
> 		if (unlikely(m == NULL))
> 			return -1;
> 
> 		app = rte_pktmbuf_append(m, sizeof(*data));
> 		if (app == NULL)
> 			...
> 		app->... = ...;
> 		...
> 		event = rte_pktmbuf_prepend(m, sizeof(*event));
> 		if (event == NULL)
> 			...
> 		event->... = ...;
> 		...
> 		hdr = rte_pktmbuf_prepend(m, sizeof(*hdr));
> 		if (hdr == NULL)
> 			...
> 		hdr->... = ...;
> 
> 		return packet_send(m);
> 	}
> 
> In these 2 examples, we never need the unioned structure (struct
> rte_ecpri_msg_hdr).
> 
> Using it does not look possible to me, because its size is corresponds to
> the largest message, not to the one we are parsing/building.
> 

Yes, in the cases, we do not need the unioned structures at all.
Since the common header is always 4 bytes, an application could use the
sub-types structures started from an offset of 4 of eCPRI layer, as in your example.
This is in the datapath. My original purpose is for some "control path", typically
the flow (not sure if any other new lib implementation) API, then the union
could be used there w/o treating the common header and message payload
header in a separate way and then combine them together. In this case, only
the first several bytes will be accessed and checked, there will be no change
of message itself, and then just passing it to datapath for further handling as
in your example.

> > I think some comments could be added here, is it OK?
> > 3. Regarding this structure, I add it because I do not want to
> > introduce a lot of new items in the rte_flow: new items with
> > structures, new enum types. I prefer one single structure will cover
> most of the cases (subtypes). What do you think?
> > 4. About the *dummy* u32, I calculated all the "subheaders" and
> choose
> > the maximal value of the length. Two purposes (same as the u32 in
> the common header):
> >   a. easy to swap the endianness, but not quite useful. Because some
> parts are u16 and u8,
> >     and should not be swapped in a u32. (some physical channel ID
> and address LSB have 32bits width)
> >     But if some HW parsed the header u32 by u32, then it would be
> helpful.
> >   b. easy for checking in flow API, if the user wants to insert a flow.
> Some checking should
> >       be done to confirm if it is wildcard flow (all eCPRI messages or
> eCPRI message in some specific type),
> >       or some precise flow (to match the pc id or rtc id, for example).
> With these fields, 3 DW
> >       of the masks only need to be check before continuing. Or else, the
> code needs to check the type
> >       and a lot of switch-case conditions and go through all different
> members of each header.
> 
> Thanks for the clarification.
> 
> I'll tend to say that if the rte_ecpri_msg_hdr structure is only useful for
> rte_flow, it should be defined inside rte_flow.
> 

Right now, yes it will be used in rte_flow. But I checked the current implementations
of each item, almost all the headers are defined in their own protocol files. So in v6,
I change the name of it, in order not to confuse the users of this API, would it be OK?
Thanks

> However, I have some fears about the dummy[3]. You said it could be
> enlarged later: I think it is dangerous to change the size of a structure
> that may be used to parse data (and this would be an ABI change).
> Also, it seems dummy[3] cannot be used easily to swap endianness, so
> is it really useful?
> 

It might be enlarger but not for now, until a new revision of this specification. For
all the subtypes it has right now, the specification will remain them as same as today.
Only new types will be added then. So after several years, we may consider to change it
in the LTS. Is it OK?
In most cases, the endianness swap could be easy, we will swap it in each DW / u32. Tome
the exception is that some field crosses the u32 boundary, like the address in type#4, we may
treat it separately. And the most useful case is for the mask checking, it could simplify the
checking to at most 3 (u32==0?) without going through each member of different types.

And v6 already sent, I change the code based on your suggestion. Would you please
help to give some comments also?

Appreciate for your help and suggestion.

> 
> Thanks,
> Olivier
> 
> 
> > > > +
> > > > +#ifdef __cplusplus
> > > > +}
> > > > +#endif
> > > > +
> > > > +#endif /* _RTE_ECPRI_H_ */
> > > > diff --git a/lib/librte_net/rte_ether.h
> > > > b/lib/librte_net/rte_ether.h index 0ae4e75..184a3f9 100644
> > > > --- a/lib/librte_net/rte_ether.h
> > > > +++ b/lib/librte_net/rte_ether.h
> > > > @@ -304,6 +304,7 @@ struct rte_vlan_hdr {  #define
> > > RTE_ETHER_TYPE_LLDP
> > > > 0x88CC /**< LLDP Protocol. */  #define RTE_ETHER_TYPE_MPLS
> > > 0x8847 /**<
> > > > MPLS ethertype. */  #define RTE_ETHER_TYPE_MPLSM 0x8848
> /**<
> > > MPLS
> > > > multicast ethertype. */
> > > > +#define RTE_ETHER_TYPE_ECPRI 0xAEFE /**< eCPRI ethertype
> (.1Q
> > > > +supported). */
> > > >
> > > >  /**
> > > >   * Extract VLAN tag information into mbuf
> > > > --
> > > > 1.8.3.1
> > > >
> >
  

On Sun, Jul 12, 2020 at 02:28:03PM +0000, Bing Zhao wrote:
> Hi Olivier,
> Thanks
> 
> BR. Bing
> 
> > -----Original Message-----
> > From: Olivier Matz <olivier.matz@6wind.com>
> > Sent: Sunday, July 12, 2020 9:18 PM
> > To: Bing Zhao <bingz@mellanox.com>
> > Cc: Ori Kam <orika@mellanox.com>; john.mcnamara@intel.com;
> > marko.kovacevic@intel.com; Thomas Monjalon
> > <thomas@monjalon.net>; ferruh.yigit@intel.com;
> > arybchenko@solarflare.com; akhil.goyal@nxp.com; dev@dpdk.org;
> > wenzhuo.lu@intel.com; beilei.xing@intel.com;
> > bernard.iremonger@intel.com
> > Subject: Re: [PATCH v5 1/2] rte_flow: add eCPRI key fields to flow API
> > 
> > Hi Bing,
> > 
> > On Sat, Jul 11, 2020 at 04:25:49AM +0000, Bing Zhao wrote:
> > > Hi Olivier,
> > > Many thanks for your comments.
> > 
> > [...]
> > 
> > > > > +/**
> > > > > + * eCPRI Common Header
> > > > > + */
> > > > > +RTE_STD_C11
> > > > > +struct rte_ecpri_common_hdr {
> > > > > +#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 */
> > > > > +#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
> > > > > +} __rte_packed;
> > > >
> > > > Does it really need to be packed? Why next types do not need it?
> > > > It looks only those which have bitfields are.
> > > >
> > >
> > > Nice catch, thanks. For the common header, there is no need to use
> > the
> > > packed attribute, because it is a u32 and the bitfields will be
> > > aligned.
> > > I checked all the definitions again. Only " Type #4: Remote Memory
> > Access"
> > > needs to use the packed attribute.
> > > For other sub-types, "sub-header" part of the message payload will
> > get
> > > aligned by nature. For example, u16 after u16, u8 after u16, these
> > > should be OK.
> > > But in type #4, the address is 48bits wide, with 16bits MSB and 32bits
> > > LSB (no detailed description in the specification, correct me if
> > > anything wrong.) Usually, the 48bits address will be devided as this
> > > in a system. And there is no 48-bits type at all. So we need to define
> > two parts for it: 32b LSB follows 16b MSB.
> > > u32 after u16 should be with packed attribute. Thanks
> > 
> > What about using a bitfield into a uint64_t ? I mean:
> > 
> > 	struct rte_ecpri_msg_rm_access {
> > if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > 		...
> > 		uint64_t length:16;		/**< number of bytes
> > */
> > 		uint64_t addr:48;		/**< address */
> > #else
> > 		...
> > 		uint64_t addr:48;		/**< address */
> > 		uint64_t length:16;		/**< number of bytes
> > */
> > #endif
> > 	};
> > 
> 
> Thanks for your suggestion.
> https://stackoverflow.com/questions/10906238/warning-when-using-bitfield-with-unsigned-char
> AFAIK (from this page), bitfields support only support bool and int. uint64_t is some type of "long"
> and most of the compilers should support it. But I am not sure if it is a standard implementation.

The uint8_t[6], as in your v6, is a good idea.


> > >
> > > >
> > > > I wonder if the 'dw0' could be in this definition instead of in
> > > > struct rte_ecpri_msg_hdr?
> > > >
> > > > Something like this:
> > > >
> > > > struct rte_ecpri_common_hdr {
> > > > 	union {
> > > > 		uint32_t u32;
> > > > 		struct {
> > > > 			...
> > > > 		};
> > > > 	};
> > > > };
> > > >
> > > > I see 2 advantages:
> > > >
> > > > - someone that only uses the common_hdr struct can use the .u32
> > > >   in its software
> > > > - when using it in messages, it looks clearer to me:
> > > >     msg.common_hdr.u32 = value;
> > > >   instead of:
> > > >     msg.dw0 = value;
> > > >
> > > > What do you think?
> > >
> > > Thanks for the suggestion, this is much better, I will change it.
> > > Indeed, in my original version, no DW(u32) is defined for the header.
> > > After that, I noticed that it would not be easy for the static casting
> > > to a u32 from bitfield(the compiler will complain), and it would not
> > > be clear to swap the endian if the user wants to use this header. I
> > > added this DW(u32) to simplify the usage of this header. But yes, if I
> > > do not add it here, it would be not easy or clear for users who just
> > use this header structure.
> > > I will change it. Is it OK if I use the name "dw0"?
> > 
> > In my opinion, u32 is more usual than dw0.
> > 
> 
> I sent patch set v6 with this change, thanks.
> 
> > >
> > > >
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header of Type #0: IQ Data  */ struct
> > > > > +rte_ecpri_msg_iq_data {
> > > > > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > > > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header of Type #1: Bit Sequence  */ struct
> > > > > +rte_ecpri_msg_bit_seq {
> > > > > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > > > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header of Type #2: Real-Time Control Data  */
> > > > struct
> > > > > +rte_ecpri_msg_rtc_ctrl {
> > > > > +	rte_be16_t rtc_id;		/**< Real-Time Control Data ID
> > > > */
> > > > > +	rte_be16_t seq_id;		/**< Sequence ID */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header of Type #3: Generic Data Transfer  */
> > > > struct
> > > > > +rte_ecpri_msg_gen_data {
> > > > > +	rte_be32_t pc_id;		/**< Physical channel ID */
> > > > > +	rte_be32_t seq_id;		/**< Sequence ID */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header of Type #4: Remote Memory Access
> > */
> > > > > +RTE_STD_C11
> > > > > +struct rte_ecpri_msg_rm_access {
> > > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > > > +	uint32_t ele_id:16;		/**< Element ID */
> > > > > +	uint32_t rr:4;			/**< Req/Resp */
> > > > > +	uint32_t rw:4;			/**< Read/Write */
> > > > > +	uint32_t rma_id:8;		/**< Remote Memory Access
> > > > ID */
> > > > > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > > > > +	uint32_t rma_id:8;		/**< Remote Memory Access
> > > > ID */
> > > > > +	uint32_t rw:4;			/**< Read/Write */
> > > > > +	uint32_t rr:4;			/**< Req/Resp */
> > > > > +	uint32_t ele_id:16;		/**< Element ID */
> > > > > +#endif
> > > > > +	rte_be16_t addr_m;		/**< 48-bits address (16 MSB)
> > > > */
> > > > > +	rte_be32_t addr_l;		/**< 48-bits address (32 LSB) */
> > > > > +	rte_be16_t length;		/**< number of bytes */
> > > > > +} __rte_packed;
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header of Type #5: One-Way Delay
> > Measurement
> > > > */
> > > > > +struct rte_ecpri_msg_delay_measure {
> > > > > +	uint8_t msr_id;			/**< Measurement ID */
> > > > > +	uint8_t act_type;		/**< Action Type */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header of Type #6: Remote Reset  */ struct
> > > > > +rte_ecpri_msg_remote_reset {
> > > > > +	rte_be16_t rst_id;		/**< Reset ID */
> > > > > +	uint8_t rst_op;			/**< Reset Code Op */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header of Type #7: Event Indication  */ struct
> > > > > +rte_ecpri_msg_event_ind {
> > > > > +	uint8_t evt_id;			/**< Event ID */
> > > > > +	uint8_t evt_type;		/**< Event Type */
> > > > > +	uint8_t seq;			/**< Sequence Number */
> > > > > +	uint8_t number;			/**< Number of
> > > > Faults/Notif */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * eCPRI Message Header Format: Common Header + Message
> > > > Types  */
> > > > > +RTE_STD_C11
> > > > > +struct rte_ecpri_msg_hdr {
> > > > > +	union {
> > > > > +		struct rte_ecpri_common_hdr common;
> > > > > +		uint32_t dw0;
> > > > > +	};
> > > > > +	union {
> > > > > +		struct rte_ecpri_msg_iq_data type0;
> > > > > +		struct rte_ecpri_msg_bit_seq type1;
> > > > > +		struct rte_ecpri_msg_rtc_ctrl type2;
> > > > > +		struct rte_ecpri_msg_bit_seq type3;
> > > > > +		struct rte_ecpri_msg_rm_access type4;
> > > > > +		struct rte_ecpri_msg_delay_measure type5;
> > > > > +		struct rte_ecpri_msg_remote_reset type6;
> > > > > +		struct rte_ecpri_msg_event_ind type7;
> > > > > +		uint32_t dummy[3];
> > > > > +	};
> > > > > +};
> > > >
> > > > What is the point in having this struct?
> > > >
> > > > From a software point of view, I think it is a bit risky, because
> > > > its size is the size of the largest message. This is probably what
> > > > you want in your case, but when a software will rx or tx such
> > > > packet, I think they shouldn't use this one. My understanding is
> > > > that you only need this structure for the mask in rte_flow.
> > > >
> > > > Also, I'm not sure to understand the purpose of dummy[3], even
> > after
> > > > reading your answer to Akhil's question.
> > > >
> > >
> > > Basically YES and no. To my understanding, the eCPRI message
> > format is
> > > something like the ICMP packet format. The message (packet) itself
> > > will be parsed into different formats based on the type of the
> > common
> > > header. In the message payload part, there is no distinct definition
> > > of the "sub-header". We can divide them into the sub-header and
> > data parts based on the specification.
> > > E.g. physical channel ID / real-time control ID / Event ID + type are
> > > the parts that datapath forwarding will only care about. The
> > following
> > > timestamp or user data parts are the parts that the higher layer in
> > the application will use.
> > > 1. If an application wants to create some offload flow, or even
> > handle
> > > it in the SW, the common header + first several bytes in the payload
> > > should be enough. BUT YES, it is not good or safe to use it in the
> > higher layer of the application.
> > > 2. A higher layer of the application should have its own definition of
> > > the whole payload of a specific sub-type, including the parsing of the
> > user data part after the "sub-header".
> > > It is better for them just skip the first 4 bytes of the eCPRI message or
> > a known offset.
> > > We do not need to cover the upper layers.
> > 
> > Let me explain what is my vision of how an application would use the
> > structures (these are completly dummy examples, as I don't know
> > ecpri protocol at all).
> > 
> > Rx:
> > 
> > 	int ecpri_input(struct rte_mbuf *m)
> > 	{
> > 		struct rte_ecpri_common_hdr hdr_copy, *hdr;
> > 		struct rte_ecpri_msg_event_ind event_copy, *event;
> > 		struct app_specific app_copy, *app;
> > 
> > 		hdr = rte_pktmbuf_read(m, 0, sizeof(*hdr),
> > &hdr_copy);
> > 		if (unlikely(hdr == NULL))
> > 			return -1;
> > 		switch (hdr->type) {
> > 		...
> > 		case RTE_ECPRI_EVT_IND_NTFY_IND:
> > 			event = rte_pktmbuf_read(m, sizeof(*hdr),
> > sizeof(*event),
> > 				&event_copy);
> > 			if (unlikely(event == NULL))
> > 				return -1;
> > 			...
> > 			app = rte_pktmbuf_read(m, sizeof(*app),
> > 				sizeof(*hdr) + sizeof(*event),
> > 				&app_copy);
> > 			...
> > 
> > Tx:
> > 
> > 	int ecpri_output(void)
> > 	{
> > 		struct rte_ecpri_common_hdr *hdr;
> > 		struct rte_ecpri_msg_event_ind *event;
> > 		struct app_specific *app;
> > 
> > 		m = rte_pktmbuf_alloc(mp);
> > 		if (unlikely(m == NULL))
> > 			return -1;
> > 
> > 		app = rte_pktmbuf_append(m, sizeof(*data));
> > 		if (app == NULL)
> > 			...
> > 		app->... = ...;
> > 		...
> > 		event = rte_pktmbuf_prepend(m, sizeof(*event));
> > 		if (event == NULL)
> > 			...
> > 		event->... = ...;
> > 		...
> > 		hdr = rte_pktmbuf_prepend(m, sizeof(*hdr));
> > 		if (hdr == NULL)
> > 			...
> > 		hdr->... = ...;
> > 
> > 		return packet_send(m);
> > 	}
> > 
> > In these 2 examples, we never need the unioned structure (struct
> > rte_ecpri_msg_hdr).
> > 
> > Using it does not look possible to me, because its size is corresponds to
> > the largest message, not to the one we are parsing/building.
> > 
> 
> Yes, in the cases, we do not need the unioned structures at all.
> Since the common header is always 4 bytes, an application could use the
> sub-types structures started from an offset of 4 of eCPRI layer, as in your example.
> This is in the datapath. My original purpose is for some "control path", typically
> the flow (not sure if any other new lib implementation) API, then the union
> could be used there w/o treating the common header and message payload
> header in a separate way and then combine them together. In this case, only
> the first several bytes will be accessed and checked, there will be no change
> of message itself, and then just passing it to datapath for further handling as
> in your example.
> 
> > > I think some comments could be added here, is it OK?
> > > 3. Regarding this structure, I add it because I do not want to
> > > introduce a lot of new items in the rte_flow: new items with
> > > structures, new enum types. I prefer one single structure will cover
> > most of the cases (subtypes). What do you think?
> > > 4. About the *dummy* u32, I calculated all the "subheaders" and
> > choose
> > > the maximal value of the length. Two purposes (same as the u32 in
> > the common header):
> > >   a. easy to swap the endianness, but not quite useful. Because some
> > parts are u16 and u8,
> > >     and should not be swapped in a u32. (some physical channel ID
> > and address LSB have 32bits width)
> > >     But if some HW parsed the header u32 by u32, then it would be
> > helpful.
> > >   b. easy for checking in flow API, if the user wants to insert a flow.
> > Some checking should
> > >       be done to confirm if it is wildcard flow (all eCPRI messages or
> > eCPRI message in some specific type),
> > >       or some precise flow (to match the pc id or rtc id, for example).
> > With these fields, 3 DW
> > >       of the masks only need to be check before continuing. Or else, the
> > code needs to check the type
> > >       and a lot of switch-case conditions and go through all different
> > members of each header.
> > 
> > Thanks for the clarification.
> > 
> > I'll tend to say that if the rte_ecpri_msg_hdr structure is only useful for
> > rte_flow, it should be defined inside rte_flow.
> > 
> 
> Right now, yes it will be used in rte_flow. But I checked the current implementations
> of each item, almost all the headers are defined in their own protocol files. So in v6,
> I change the name of it, in order not to confuse the users of this API, would it be OK?
> Thanks

OK

> 
> > However, I have some fears about the dummy[3]. You said it could be
> > enlarged later: I think it is dangerous to change the size of a structure
> > that may be used to parse data (and this would be an ABI change).
> > Also, it seems dummy[3] cannot be used easily to swap endianness, so
> > is it really useful?
> > 
> 
> It might be enlarger but not for now, until a new revision of this specification. For
> all the subtypes it has right now, the specification will remain them as same as today.
> Only new types will be added then. So after several years, we may consider to change it
> in the LTS. Is it OK?

OK, I think in this case it may even be another structure

> In most cases, the endianness swap could be easy, we will swap it in each DW / u32. Tome
> the exception is that some field crosses the u32 boundary, like the address in type#4, we may
> treat it separately. And the most useful case is for the mask checking, it could simplify the
> checking to at most 3 (u32==0?) without going through each member of different types.
> 
> And v6 already sent, I change the code based on your suggestion. Would you please
> help to give some comments also?
> 
> Appreciate for your help and suggestion.
> 
> > 
> > Thanks,
> > Olivier
> > 
> > 
> > > > > +
> > > > > +#ifdef __cplusplus
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > +#endif /* _RTE_ECPRI_H_ */
> > > > > diff --git a/lib/librte_net/rte_ether.h
> > > > > b/lib/librte_net/rte_ether.h index 0ae4e75..184a3f9 100644
> > > > > --- a/lib/librte_net/rte_ether.h
> > > > > +++ b/lib/librte_net/rte_ether.h
> > > > > @@ -304,6 +304,7 @@ struct rte_vlan_hdr {  #define
> > > > RTE_ETHER_TYPE_LLDP
> > > > > 0x88CC /**< LLDP Protocol. */  #define RTE_ETHER_TYPE_MPLS
> > > > 0x8847 /**<
> > > > > MPLS ethertype. */  #define RTE_ETHER_TYPE_MPLSM 0x8848
> > /**<
> > > > MPLS
> > > > > multicast ethertype. */
> > > > > +#define RTE_ETHER_TYPE_ECPRI 0xAEFE /**< eCPRI ethertype
> > (.1Q
> > > > > +supported). */
> > > > >
> > > > >  /**
> > > > >   * Extract VLAN tag information into mbuf
> > > > > --
> > > > > 1.8.3.1
> > > > >
> > >