Hi Olivier, > > +#define RTE_MACSEC_TCI_E 0x08 /**< User data is encrypted */ > > +#define RTE_MACSEC_TCI_C 0x04 /**< User data was changed (because of > encryption) */ > E bit means the user data is encrypted if set. Above defines are mask to each of the fields in tci_an. I would add a comment that these are masks to each of the fields.
C bit means the user data is changed (because of encryption) > > In [1], I can read the following, which is not similar: > > E and C bits used to determine if packet is encrypted > • E, C = 1, 1 – Encrypted > • E, C = 0, 0 – Authenticated-Only > > Is there any reference paper for macsec header? The reference is IEEE802.1AE standard. https://ieeexplore.ieee.org/document/8585421 > > [1] https://www.marvell.com/content/dam/marvell/en/public- > collateral/automotive-solutions/marvell-macsec-security-in-ethernet-based- > vehicle-white-paper.pdf > > > > > > +#define RTE_MACSEC_NUM_AN 4 /**< 2 bits for the association > number */ > > I don't get how this defined is used. Can the comment be clarified? In case of MACsec we can have 4 different SAs for each of the secure channel Based on the AN field. RTE_MACSEC_NUM_AN is basically added to make an upper limit to the array of SAs. I will re-write the comment as #define RTE_MACSEC_NUM_AN 4 /**< Max number of association numbers. */ Or shall I move it to rte_security? > > > +#define RTE_MACSEC_SALT_LEN 12 /**< Salt length for MACsec SA */ For MACsec SA configuration, Salt is used which has a fixed size of 12 bytes. Do you want me to move it to rte_security? > > Same here. > > > + > > +/** > > + * MACsec Header > > + */ > > +struct rte_macsec_hdr { > > + /* SecTAG */ > > Is the SecTAG comment required? Or maybe it should be moved > above the struct? > > > + uint8_t tci_an; /**< Tag control information and Association > > number > of SC */ > > nit: duplicated spaces after uint8_t > > Can we use a bitfield here for tci and an, like you did for short_length? Would it be really necessary to split it to bitfields. Can we not use it like vtc_flow in rte_ipv6_hdr? > > Like this: > > uint8_t tci:6; > uint8_t an:2; > > Or: > > uint8_t tci_version:1; > uint8_t tci_es:1; > uint8_t tci_sc:1; > uint8_t tci_scb:1; > uint8_t tci_e:1; > uint8_t tci_c:1; > uint8_t an:2; > > I think the 2nd one is easier to use. > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > + uint8_t short_length : 6; /**< Short Length */ > > + uint8_t unused : 2; > > nit: no spaces around ':' > > > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN > > + uint8_t unused : 2; > > + uint8_t short_length : 6; > > +#endif > > + rte_be32_t packet_number; /**< Packet number to support replay > protection */ > > + uint8_t secure_channel_id[8]; /* optional */ > > 8 -> RTE_MACSEC_SCI_LEN ? > > I think it would be more convenient to have another struct > for the secure_channel_id. Ok Can we use it like this struct rte_macsec_sci_hdr { uint8_t sci[RTE_MACSEC_SCI_LEN]; /**< Optional secure channel id. */ } __rte_packed; struct rte_macsec_hdr { uint8_t tci_an; /**< Tag control information and Association number of SC. */ #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN uint8_t short_length:6; /**< Short Length. */ uint8_t unused:2; #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN uint8_t unused:2; uint8_t short_length:6; /**< Short Length. */ #endif rte_be32_t packet_number; /**< Packet number to support replay protection. */ union { uint8_t payload[0]; struct rte_macsec_sci_hdr sci[0]; }; } __rte_packed;
