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