Re: [dpdk-dev] [PATCH v2 2/4] net/iavf: add iAVF IPsec inline crypto support

["Wu, Jingjing" <jingjing.wu@intel.com>]

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?

[...]