[dpdk-dev] [PATCH v4 10/12] net/ixgbe: enable inline ipsec
Radu Nicolau
radu.nicolau at intel.com
Thu Oct 19 12:51:14 CEST 2017
Hi,
Comments inline
On 10/18/2017 10:29 PM, Ananyev, Konstantin wrote:
> Hi Radu,
> Few comments from me below.
> Konstantin
>
>> <snip>
>>
>> +#define IXGBE_WRITE_REG_THEN_POLL_MASK(hw, reg, val, mask, poll_ms) \
>> +{ \
>> + uint32_t cnt = poll_ms; \
>> + IXGBE_WRITE_REG(hw, (reg), (val)); \
>> + while (((IXGBE_READ_REG(hw, (reg))) & (mask)) && (cnt--)) \
>> + rte_delay_ms(1); \
>> +}
>> +
> As you have a macro that consists from multiple statements, you'll need a do { ..} while (0) wrapper
> around it.
> Though I still suggest to make it an inline function - would be much better.
I will add do-while wrapper, but making it an inline function there
brings in a circular dependency.
>
>> <snip>
>> +
>> +static int
>> +ixgbe_crypto_update_mb(void *device __rte_unused,
>> + struct rte_security_session *session,
>> + struct rte_mbuf *m, void *params __rte_unused)
>> +{
>> + struct ixgbe_crypto_session *ic_session =
>> + get_sec_session_private_data(session);
>> + if (ic_session->op == IXGBE_OP_AUTHENTICATED_ENCRYPTION) {
>> + struct ixgbe_crypto_tx_desc_md *mdata =
>> + (struct ixgbe_crypto_tx_desc_md *)&m->udata64;
>> + mdata->enc = 1;
>> + mdata->sa_idx = ic_session->sa_index;
>> + mdata->pad_len = *rte_pktmbuf_mtod_offset(m,
>> + uint8_t *, rte_pktmbuf_pkt_len(m) - 18) + 18;
> Could you explain what pad_len supposed to contain?
> Also what is a magical constant '18'?
> Could you create some macro if needed?
I added an explanation in the code, we read the payload padding size
that is stored at the len-18 bytes and add 18 bytes, 2 for ESP trailer
and 16 for ICV.
>> + }
>> + return 0;
>> +}
>> +
>> +struct rte_cryptodev_capabilities aes_gmac_crypto_capabilities[] = {
>> + { /* AES GMAC (128-bit) */
>> + .op = RTE_CRYPTO_OP_TYPE_SYMMETRIC,
>> + {.sym = {
>> + .xform_type = RTE_CRYPTO_SYM_XFORM_AUTH,
>> + {.auth = {
>> + .algo = RTE_CRYPTO_AUTH_AES_GMAC,
>> + .block_size = 16,
>> + .key_size = {
>> + .min = 16,
>> + .max = 16,
>> + .increment = 0
>> + },
>> + .digest_size = {
>> + .min = 12,
>> + .max = 12,
>> + .increment = 0
>> + },
>> + .iv_size = {
>> + .min = 12,
>> + .max = 12,
>> + .increment = 0
>> + }
>> + }, }
>> + }, }
>> + },
>> + {
>> + .op = RTE_CRYPTO_OP_TYPE_UNDEFINED,
>> + {.sym = {
>> + .xform_type = RTE_CRYPTO_SYM_XFORM_NOT_SPECIFIED
>> + }, }
>> + },
>> +};
>> +
>> +struct rte_cryptodev_capabilities aes_gcm_gmac_crypto_capabilities[] = {
>> + { /* AES GMAC (128-bit) */
>> + .op = RTE_CRYPTO_OP_TYPE_SYMMETRIC,
>> + {.sym = {
>> + .xform_type = RTE_CRYPTO_SYM_XFORM_AUTH,
>> + {.auth = {
>> + .algo = RTE_CRYPTO_AUTH_AES_GMAC,
>> + .block_size = 16,
>> + .key_size = {
>> + .min = 16,
>> + .max = 16,
>> + .increment = 0
>> + },
>> + .digest_size = {
>> + .min = 12,
>> + .max = 12,
>> + .increment = 0
>> + },
>> + .iv_size = {
>> + .min = 12,
>> + .max = 12,
>> + .increment = 0
>> + }
>> + }, }
>> + }, }
>> + },
>> + { /* AES GCM (128-bit) */
>> + .op = RTE_CRYPTO_OP_TYPE_SYMMETRIC,
>> + {.sym = {
>> + .xform_type = RTE_CRYPTO_SYM_XFORM_AEAD,
>> + {.aead = {
>> + .algo = RTE_CRYPTO_AEAD_AES_GCM,
>> + .block_size = 16,
>> + .key_size = {
>> + .min = 16,
>> + .max = 16,
>> + .increment = 0
>> + },
>> + .digest_size = {
>> + .min = 8,
>> + .max = 16,
>> + .increment = 4
>> + },
>> + .aad_size = {
>> + .min = 0,
>> + .max = 65535,
>> + .increment = 1
>> + },
>> + .iv_size = {
>> + .min = 12,
>> + .max = 12,
>> + .increment = 0
>> + }
>> + }, }
>> + }, }
>> + },
>> + {
>> + .op = RTE_CRYPTO_OP_TYPE_UNDEFINED,
>> + {.sym = {
>> + .xform_type = RTE_CRYPTO_SYM_XFORM_NOT_SPECIFIED
>> + }, }
>> + },
>> +};
>> +
>> +static const struct rte_security_capability ixgbe_security_capabilities[] = {
>> + { /* IPsec Inline Crypto ESP Transport Egress */
>> + .action = RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
>> + .protocol = RTE_SECURITY_PROTOCOL_IPSEC,
>> + .ipsec = {
>> + .proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP,
>> + .mode = RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
>> + .direction = RTE_SECURITY_IPSEC_SA_DIR_EGRESS,
>> + .options = { 0 }
>> + },
>> + .crypto_capabilities = aes_gcm_gmac_crypto_capabilities,
>> + .ol_flags = RTE_SECURITY_TX_OLOAD_NEED_MDATA
>> + },
>> + { /* IPsec Inline Crypto ESP Transport Ingress */
>> + .action = RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
>> + .protocol = RTE_SECURITY_PROTOCOL_IPSEC,
>> + .ipsec = {
>> + .proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP,
>> + .mode = RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
>> + .direction = RTE_SECURITY_IPSEC_SA_DIR_INGRESS,
>> + .options = { 0 }
>> + },
>> + .crypto_capabilities = aes_gcm_gmac_crypto_capabilities,
>> + .ol_flags = 0
>> + },
>> + { /* IPsec Inline Crypto ESP Tunnel Egress */
>> + .action = RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
>> + .protocol = RTE_SECURITY_PROTOCOL_IPSEC,
>> + .ipsec = {
>> + .proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP,
>> + .mode = RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
>> + .direction = RTE_SECURITY_IPSEC_SA_DIR_EGRESS,
>> + .options = { 0 }
>> + },
>> + .crypto_capabilities = aes_gcm_gmac_crypto_capabilities,
>> + .ol_flags = RTE_SECURITY_TX_OLOAD_NEED_MDATA
>> + },
>> + { /* IPsec Inline Crypto ESP Tunnel Ingress */
>> + .action = RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
>> + .protocol = RTE_SECURITY_PROTOCOL_IPSEC,
>> + .ipsec = {
>> + .proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP,
>> + .mode = RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
>> + .direction = RTE_SECURITY_IPSEC_SA_DIR_INGRESS,
>> + .options = { 0 }
>> + },
>> + .crypto_capabilities = aes_gcm_gmac_crypto_capabilities,
>> + .ol_flags = 0
>> + },
>> + {
>> + .action = RTE_SECURITY_ACTION_TYPE_NONE
>> + }
>> +};
>> +
>> +static const struct rte_security_capability *
>> +ixgbe_crypto_capabilities_get(void *device __rte_unused)
>> +{
> As a nit: if ixgbe_security_capabilities are not used in any other place -
> you can move its definition inside that function.
Done.
>
>> +};
>> +
>> +struct ixgbe_crypto_tx_desc_md {
>> + union {
>> + uint64_t data;
>> + struct {
>> + uint32_t sa_idx;
>> + uint8_t pad_len;
>> + uint8_t enc;
>> + };
>> + };
>> +};
>
> Why just not:
> union ixgbe_crypto_tx_desc_md {
> uint64_t data;
> struct {...};
> };
> ?
Done.
>
>> +
>> +struct ixgbe_ipsec {
>> + struct ixgbe_crypto_rx_ip_table rx_ip_tbl[IPSEC_MAX_RX_IP_COUNT];
>> + struct ixgbe_crypto_rx_sa_table rx_sa_tbl[IPSEC_MAX_SA_COUNT];
>> + struct ixgbe_crypto_tx_sa_table tx_sa_tbl[IPSEC_MAX_SA_COUNT];
>> +};
>> +
>> +extern struct rte_security_ops ixgbe_security_ops;
>> +
>> +
>> +int ixgbe_crypto_enable_ipsec(struct rte_eth_dev *dev);
>> +int ixgbe_crypto_add_ingress_sa_from_flow(const void *sess,
>> + const void *ip_spec,
>> + uint8_t is_ipv6);
>> +
>> +
>> +
>> +#endif /*IXGBE_IPSEC_H_*/
>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
>> index 0038dfb..279e3fa 100644
>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
>> @@ -93,6 +93,7 @@
>> PKT_TX_TCP_SEG | \
>> PKT_TX_MACSEC | \
>> PKT_TX_OUTER_IP_CKSUM | \
>> + PKT_TX_SEC_OFFLOAD | \
>> IXGBE_TX_IEEE1588_TMST)
>>
>> #define IXGBE_TX_OFFLOAD_NOTSUP_MASK \
>> @@ -395,7 +396,8 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
>> static inline void
>> ixgbe_set_xmit_ctx(struct ixgbe_tx_queue *txq,
>> volatile struct ixgbe_adv_tx_context_desc *ctx_txd,
>> - uint64_t ol_flags, union ixgbe_tx_offload tx_offload)
>> + uint64_t ol_flags, union ixgbe_tx_offload tx_offload,
>> + struct rte_mbuf *mb)
> I don't think you need to pass mb as a parameter to that function:
> you already have ol_flags as a parameter and all you need is just struct ixgbe_crypto_tx_desc_md md
> here as an extra parameter.
Done.
>
>> {
>> uint32_t type_tucmd_mlhl;
>> uint32_t mss_l4len_idx = 0;
>> @@ -479,6 +481,18 @@ ixgbe_set_xmit_ctx(struct ixgbe_tx_queue *txq,
>> seqnum_seed |= tx_offload.l2_len
>> << IXGBE_ADVTXD_TUNNEL_LEN;
>> }
>> + if (mb->ol_flags & PKT_TX_SEC_OFFLOAD) {
>> + struct ixgbe_crypto_tx_desc_md *mdata =
>> + (struct ixgbe_crypto_tx_desc_md *)
>> + &mb->udata64;
>> + seqnum_seed |=
>> + (IXGBE_ADVTXD_IPSEC_SA_INDEX_MASK & mdata->sa_idx);
>> + type_tucmd_mlhl |= mdata->enc ?
>> + (IXGBE_ADVTXD_TUCMD_IPSEC_TYPE_ESP |
>> + IXGBE_ADVTXD_TUCMD_IPSEC_ENCRYPT_EN) : 0;
>> + type_tucmd_mlhl |=
>> + (mdata->pad_len & IXGBE_ADVTXD_IPSEC_ESP_LEN_MASK);
> Shouldn't we also update tx_offload_mask here?
We do - updated.
>
More information about the dev
mailing list