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

[Jay Zhou <jianjay.zhou@huawei.com>]

Hi Fan,

On 2018/1/30 1:19, Zhang, Roy Fan wrote:
> 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?

Yes.

> Could you use " #include <linux/virtio_crypto.h>" instead of creating a copy of the file?

Okay.

>
>> 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.

Okay, will do. Thanks again for reviewing!

Regards,
Jay

>
> Best regards,
> Fan
>
>
>
>
> .
>