In general, the patch is too big to review. Patch split would help a lot!
[...]
> +static const struct rte_cryptodev_symmetric_capability *
> +get_capability(struct iavf_security_ctx *iavf_sctx,
> + uint32_t algo, uint32_t type)
> +{
> + const struct rte_cryptodev_capabilities *capability;
> + int i = 0;
> +
> + capability = &iavf_sctx->crypto_capabilities[i];
> +
> + while (capability->op != RTE_CRYPTO_OP_TYPE_UNDEFINED) {
> + if (capability->op == RTE_CRYPTO_OP_TYPE_SYMMETRIC &&
> + capability->sym.xform_type == type &&
> + capability->sym.cipher.algo == algo)
> + return &capability->sym;
> + /** try next capability */
> + capability = &iavf_crypto_capabilities[i++];
Better to check i to avoid out of boundary.
[...]
> +
> +static int
> +valid_length(uint32_t len, uint32_t min, uint32_t max, uint32_t increment)
> +{
> + if (len < min || len > max)
> + return 0;
> +
> + if (increment == 0)
> + return 1;
> +
> + if ((len - min) % increment)
> + return 0;
> +
> + return 1;
> +}
Would it be better to use true/false instead of 1/0? And the same to following
valid functions.
[...]
> +static int
> +iavf_ipsec_crypto_session_validate_conf(struct iavf_security_ctx *iavf_sctx,
> + struct rte_security_session_conf *conf)
> +{
> + /** validate security action/protocol selection */
> + if (conf->action_type != RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO ||
> + conf->protocol != RTE_SECURITY_PROTOCOL_IPSEC) {
> + PMD_DRV_LOG(ERR, "Unsupported action / protocol specified");
> + return -EINVAL;
> + }
> +
> + /** validate IPsec protocol selection */
> + if (conf->ipsec.proto != RTE_SECURITY_IPSEC_SA_PROTO_ESP) {
> + PMD_DRV_LOG(ERR, "Unsupported IPsec protocol specified");
> + return -EINVAL;
> + }
> +
> + /** validate selected options */
> + if (conf->ipsec.options.copy_dscp ||
> + conf->ipsec.options.copy_flabel ||
> + conf->ipsec.options.copy_df ||
> + conf->ipsec.options.dec_ttl ||
> + conf->ipsec.options.ecn ||
> + conf->ipsec.options.stats) {
> + PMD_DRV_LOG(ERR, "Unsupported IPsec option specified");
> + return -EINVAL;
> + }
> +
> + /**
> + * Validate crypto xforms parameters.
> + *
> + * AEAD transforms can be used for either inbound/outbound IPsec SAs,
> + * for non-AEAD crypto transforms we explicitly only support CIPHER/AUTH
> + * for outbound and AUTH/CIPHER chained transforms for inbound IPsec.
> + */
> + if (conf->crypto_xform->type == RTE_CRYPTO_SYM_XFORM_AEAD) {
> + if (!valid_aead_xform(iavf_sctx, &conf->crypto_xform->aead)) {
> + PMD_DRV_LOG(ERR, "Unsupported IPsec option specified");
> + return -EINVAL;
> + }
Invalid parameter, but not unsupported option, right? Same to below.
[...]
> +static void
> +sa_add_set_aead_params(struct virtchnl_ipsec_crypto_cfg_item *cfg,
> + struct rte_crypto_aead_xform *aead, uint32_t salt)
> +{
> + cfg->crypto_type = VIRTCHNL_AEAD;
> +
> + switch (aead->algo) {
> + case RTE_CRYPTO_AEAD_AES_CCM:
> + cfg->algo_type = VIRTCHNL_AES_CCM; break;
> + case RTE_CRYPTO_AEAD_AES_GCM:
> + cfg->algo_type = VIRTCHNL_AES_GCM; break;
> + case RTE_CRYPTO_AEAD_CHACHA20_POLY1305:
> + cfg->algo_type = VIRTCHNL_CHACHA20_POLY1305; break;
> + default:
> + RTE_ASSERT("we should be here");
Assert just because invalid config? Similar comments to other valid functions.
> + }
> +
> + cfg->key_len = aead->key.length;
> + cfg->iv_len = aead->iv.length;
> + cfg->digest_len = aead->digest_length;
> + cfg->salt = salt;
> +
> + RTE_ASSERT(sizeof(cfg->key_data) < cfg->key_len);
> +
Not only data, but length, better to valid before setting? The same to other
kind params setting.
[...]
> +static inline void
> +iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
> + struct rte_mbuf *m)
> +{
> + uint64_t command = 0;
> + uint64_t offset = 0;
> + uint64_t l2tag1 = 0;
> +
> + *qw1 = IAVF_TX_DESC_DTYPE_DATA;
> +
> + command = (uint64_t)IAVF_TX_DESC_CMD_ICRC;
> +
> + /* Descriptor based VLAN insertion */
> + if (m->ol_flags & PKT_TX_VLAN_PKT) {
> + command |= (uint64_t)IAVF_TX_DESC_CMD_IL2TAG1;
> + l2tag1 |= m->vlan_tci;
> + }
> +
> /* Set MACLEN */
> - *td_offset |= (tx_offload.l2_len >> 1) <<
> - IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> -
> - /* Enable L3 checksum offloads */
> - if (ol_flags & PKT_TX_IP_CKSUM) {
> - *td_cmd |= IAVF_TX_DESC_CMD_IIPT_IPV4_CSUM;
> - *td_offset |= (tx_offload.l3_len >> 2) <<
> - IAVF_TX_DESC_LENGTH_IPLEN_SHIFT;
> - } else if (ol_flags & PKT_TX_IPV4) {
> - *td_cmd |= IAVF_TX_DESC_CMD_IIPT_IPV4;
> - *td_offset |= (tx_offload.l3_len >> 2) <<
> - IAVF_TX_DESC_LENGTH_IPLEN_SHIFT;
> - } else if (ol_flags & PKT_TX_IPV6) {
> - *td_cmd |= IAVF_TX_DESC_CMD_IIPT_IPV6;
> - *td_offset |= (tx_offload.l3_len >> 2) <<
> - IAVF_TX_DESC_LENGTH_IPLEN_SHIFT;
> - }
> -
> - if (ol_flags & PKT_TX_TCP_SEG) {
> - *td_cmd |= IAVF_TX_DESC_CMD_L4T_EOFT_TCP;
> - *td_offset |= (tx_offload.l4_len >> 2) <<
> - IAVF_TX_DESC_LENGTH_L4_FC_LEN_SHIFT;
> - return;
PKT_TX_TCP_SEG flag implies PKT_TX_TCP_CKSUM,
so the offset cannot removed by just check cusm offload fields.
[...]
> struct iavf_32b_rx_flex_desc_comms {
> + union {
> + struct {
> /* Qword 0 */
> u8 rxdid;
> u8 mir_id_umb_cast;
> @@ -305,6 +375,101 @@ struct iavf_32b_rx_flex_desc_comms {
> } flex;
> __le32 ts_high;
> } flex_ts;
> + };
> + struct {
> + /* Quad Word 0 */
> +
> + u8 rxdid; /**< Descriptor builder profile ID */
> +
> + u8 mirror_id:6;
> + u8 umbcast:2;
> +
> + __le16 ptype:10;
> + __le16 flexi_flags_0:6;
> +
> + __le16 packet_length:14;
> + __le16 rsv_0:2;
> +
> + __le16 hlen:11;
> + __le16 sph:1;
> + __le16 flexi_flags_1:4;
> +
> + /* Quad Word 1 */
> + union {
> + __le16 status_error0;
> + struct {
> + __le16 status_error0_dd:1;
> + /* descriptor done */
> + __le16 status_error0_eop:1;
> + /* end of packet */
> + __le16 status_error0_hbo:1;
> + /* header buffer overflow */
> + __le16 status_error0_l3l4p:1;
> + /* l3/l4 integrity check */
> + __le16 status_error0_xsum:4;
> + /* checksum report */
> + __le16 status_error0_lpbk:1;
> + /* loopback */
> + __le16 status_error0_ipv6exadd:1;
> + /* ipv6 w/ dst options or routing hdr */
> + __le16 status_error0_rxe:1;
> + /* rcv mac errors */
> + __le16 status_error0_crcp:1;
> + /* ethernet crc present */
> + __le16 status_error0_rsshash:1;
> + /* rss hash valid */
> + __le16 status_error0_l2tag1p:1;
> + /* l2 tag 1 present */
> + __le16 status_error0_flexi_md0:1;
> + /* flexi md field 0 valid */
> + __le16 status_error0_flexi_md1:1;
> + /* flexi md field 1 valid */
> + };
> + };
> + __le16 l2tag1;
> + __le16 flex_meta0; /**< flexi metadata field 0 */
> + __le16 flex_meta1; /**< flexi metadata field 1 */
> +
> + /* Quad Word 2 */
> + union {
> + __le16 status_error1;
> + struct {
> + __le16 status_error1_cpm:4;
> + /* Inline IPsec Crypto Status */
> + __le16 status_error1_udp_tunnel:1;
> + /* UDP tunnelled packet NAT-T/UDP-NAT */
> + __le16 status_error1_crypto:1;
> + /* Inline IPsec Crypto Offload */
> + __le16 status_error1_rsv:5;
> + /* Reserved */
> + __le16 status_error1_l2tag2p:1;
> + /* l2 tag 2 present */
> + __le16 status_error1_flexi_md2:1;
> + /* flexi md field 2 valid */
> + __le16 status_error1_flexi_md3:1;
> + /* flexi md field 3 valid */
> + __le16 status_error1_flexi_md4:1;
> + /* flexi md field 4 valid */
> + __le16 status_error1_flexi_md5:1;
> + /* flexi md field 5 valid */
> + };
> + };
> +
> + u8 flex_flags2;
> + u8 time_stamp_low;
> +
> + __le16 l2tag2_1st; /**< L2TAG */
> + __le16 l2tag2_2nd; /**< L2TAG */
> +
> + /* Quad Word 3 */
> +
> + __le16 flex_meta2; /**< flexi metadata field 2 */
> + __le16 flex_meta3; /**< flexi metadata field 3 */
> + __le16 flex_meta4; /**< flexi metadata field 4 */
> + __le16 flex_meta5; /**< flexi metadata field 5 */
> +
> + } debug;
> + };
> };
If you check the description of this struct, you will find it is for RxDID
Profile ID 16-21.
I think you need define a new struct for ipsec. And for debug, also prefer a
new struct
or some func instead of adding union and fields in defined formatted
descriptor.
[....]
> +
> +#include <netinet/in.h>
> +
Put this inline at the beginning of file?
[...]
> @@ -330,18 +339,40 @@ iavf_handle_virtchnl_msg(struct rte_eth_dev *dev)
> case iavf_aqc_opc_send_msg_to_vf:
> if (msg_opc == VIRTCHNL_OP_EVENT) {
> iavf_handle_pf_event_msg(dev, info.msg_buf,
> - info.msg_len);
> + info.msg_len);
> } else {
> + /* check for inline IPsec events */
> + struct inline_ipsec_msg *imsg =
> + (struct inline_ipsec_msg *)info.msg_buf;
> + struct rte_eth_event_ipsec_desc desc;
> + if (msg_opc == VIRTCHNL_OP_INLINE_IPSEC_CRYPTO
> + && imsg->ipsec_opcode ==
> + INLINE_IPSEC_OP_EVENT) {
> + struct virtchnl_ipsec_event *ev =
> + imsg->ipsec_data.event;
> + desc.subtype =
> + RTE_ETH_EVENT_IPSEC_UNKNOWN;
> + desc.metadata = ev->ipsec_event_data;
> + rte_eth_dev_callback_process(dev,
> + RTE_ETH_EVENT_IPSEC,
> + &desc);
> + return;
> + }
> +
> /* read message and it's expected one */
> - if (msg_opc == vf->pend_cmd)
> - _notify_cmd(vf, msg_ret);
> - else
> - PMD_DRV_LOG(ERR, "command mismatch,"
> - "expect %u, get %u",
> - vf->pend_cmd, msg_opc);
> + if (msg_opc == vf->pend_cmd) {
> + rte_atomic32_dec(&vf->pend_cmd_count);
> + if (rte_atomic32_read(
> + &vf->pend_cmd_count) == 0)
> + _notify_cmd(vf, msg_ret);
Only dec the count, does the async mean only the second message carries
response info?
[...]
