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

["Nicolau, Radu" <radu.nicolau@intel.com>]

Hi Jingjing, thanks for reviewing!


On 9/18/2021 6:28 AM, Wu, Jingjing wrote:
> In general, the patch is too big to review. Patch split would help a lot!
I will do my best to split in in the next revision.
>
> [...]
>> +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.
The condition in the while statement plus the last element in the array 
set as RTE_CRYPTO_OP_TYPE_UNDEFINED prevents the loop from going out of 
bounds.
> [...]
>
>> +
>> +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.
Will do.
> [...]
>
>> +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.
I reworked the messages to be consistent
> [...]
>
>> +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.
Removed
>
>> +	}
>> +
>> +	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.
The length here is checked to fit into the array, it can still be valid; 
I moved this check in the valid_length function that can actually return 
an error.
> [...]
>
>
>> +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.
I added this back, most likely it was a rebase mistake.
> [...]
>
>
>>   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.
I removed this altogether as it has no functional purpose.
> [....]
>
>
>> +
>> +#include <netinet/in.h>
>> +
> Put this inline at the beginning of file?
I removed this section
> [...]
>
>> @@ -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?
Yes, from my understanding the 1st message is the confirmation of the 
request and the second is the reply.
>
> [...]