Re: [dpdk-dev] [PATCH] virtio: add new driver for crypto devices

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Jay, 

A few more comments inline.

> -----Original Message-----
> From: Jay Zhou [mailto:jianjay.zhou@huawei.com]
> Sent: Friday, November 17, 2017 5:10 PM
> To: dev@dpdk.org
> Cc: yliu@fridaylinux.org; maxime.coquelin@redhat.com;
> arei.gonglei@huawei.com; Zhang, Roy Fan <roy.fan.zhang@intel.com>; Zeng,
> Xin <xin.zeng@intel.com>; weidong.huang@huawei.com;
> wangxinxin.wang@huawei.com; longpeng2@huawei.com;
> jianjay.zhou@huawei.com
> Subject: [PATCH] virtio: add new driver for crypto devices
> 
> +++ b/drivers/crypto/virtio/virtio_crypto.h
> @@ -0,0 +1,452 @@

The file "virtio_crypto.h" is identical to the one in the linux kernel header, right? 
Could you use " #include <linux/virtio_crypto.h>" instead of creating a copy of the file?

> diff --git a/drivers/crypto/virtio/virtio_cryptodev.c
> b/drivers/crypto/virtio/virtio_cryptodev.c
> new file mode 100644
> index 0000000..9e6cd20
> --- /dev/null
> +++ b/drivers/crypto/virtio/virtio_cryptodev.c
> @@ -0,0 +1,1542 @@

...

> +	switch (cmd_id) {
> +	case VIRTIO_CRYPTO_CMD_CIPHER_HASH:
> +	case VIRTIO_CRYPTO_CMD_HASH_CIPHER:
> +		ctrl->u.sym_create_session.op_type
> +			= VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING;

The above line is clearly a bug.
 
> +		ret = virtio_crypto_sym_pad_op_ctrl_req(ctrl,
> +			xform, true, &cipher_key_data, &auth_key_data,
> session);
> +		if (ret < 0) {
> +			PMD_SESSION_LOG(ERR,
> +				"padding sym op ctrl req failed");
> +			goto error_out;
> +		}
> +		ret = virtio_crypto_send_command(vq, ctrl,
> +			cipher_key_data, auth_key_data, session);
> +		if (ret < 0) {
> +			PMD_SESSION_LOG(ERR,
> +				"create session failed: %d", ret);
> +			goto error_out;
> +		}
> +		break;
> +	case VIRTIO_CRYPTO_CMD_CIPHER:

Again, please try to replace the mallocs into rte_mempool_get() or rte_mempool_get_bulk() for performance reason.

Best regards,
Fan