[EXT] Re: [PATCH 1/3] net: add MACsec header

[Akhil Goyal]

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;