Re: [dpdk-dev] [PATCH v4 1/2] rte_flow: add eCPRI key fields to flow API

[Bing Zhao <bingz@mellanox.com>]

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, July 9, 2020 2:50 AM
> To: Bing Zhao <bingz@mellanox.com>; 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; olivier.matz@6wind.com
> Cc: dev@dpdk.org; wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com
> Subject: RE: [dpdk-dev] [PATCH v4 1/2] rte_flow: add eCPRI key fields
> to flow API
> 
> 
> > +#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_UPTO_20		1
> > +
> > +/**
> > + * eCPRI message types in specifications
> > + * IWF* types will only be supported from rev.2  */
> > +#define RTE_ECPRI_MSG_TYPE_IQ_DATA	0
> > +#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
> 
> Should we have a comment for reserved and vendor specific message
> types as well?

Yes, thanks. We can have one line or two lines of comments to describe here.

> 
> > +
> > +/**
> > + * 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;
> > +
> > +/**
> > + * 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 */
> 
> Should we also add timestamp and compensation fields as well here?

OK, we can add it. I am not sure if there is a strong requirement for these fields.
This file is planned to define the headers of an eCPRI message. From the specification,
only the common header is named with "header". And the first bytes in the message
body will be recognized with different formats based on the type, as we know.
Regarding some message types, the first several bytes could be considered as the
"sub header" of the message, then followed by user data bytes. The length of the user
specific data is variable as well as the content.
In this case, we can that the timestamp and compensation are the user specific
data, and in each packet, it will have a different value.

> 
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #6: Remote Reset  */ struct
> > +rte_ecpri_msg_remote_reset {
> > +	uint8_t msr_id;			/**< Measurement ID */
> > +	uint8_t act_type;		/**< Action Type */
> > +};
> 
> I think it is a copy paste error.
> It should have uint16_t reset_id and uint8_t reset_code_op

Nice catch, thanks a lot.

> 
> > +
> > +/**
> > + * 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 */
> > +};
> Should we also define enums for evt_type and other fields in this file.

For event indication type, we could. We can use #define instead of enum since this
should be fixed value and there is no change in the next releases of the spec.
Also, other fields are not exposed in the "header" now, so to my understanding, no
need to cover them at this stage.

> 
> > +
> > +/**
> > + * 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];
> 
> Why 3 dummy? IWF messages are 4.

This is a union of the message payload body but not the "place holders" for
IWF messages. IWF messages headers are a little bit complex and not defined
in revision 1.x IIRC. This filed as well as the "dw0" is only SW level concept to
simplify the work in the driver and make compiler happy without any cost.
Some critical flags of a compiler may complain when forcing casting the structure
starts from a bit-field to a u32. And when the DW 4 bytes needs to be dumped or
checked, and when doing endianness swap, this would be easier. Maybe this field
should have a better name 😊
3 DWs for the payload sub header part is enough now. If we add the timestamp and
comp part, then we should enlarge this.

> 
> > +	};
> > +};
> > +
> > +#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