> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> Sent: Sunday, October 16, 2016 9:23 PM
> Subject: Re: [Qemu-devel] [PATCH v7 09/12] virtio-crypto: add data queue
> processing handler
>
> On Thu, Oct 13, 2016 at 03:12:03PM +0800, Gonglei wrote:
> > Introduces VirtIOCryptoReq structure to store
> > crypto request so that we can support sync and async
> > crypto operation in the future.
>
> What do you mean by "sync and async" operations?
>
Synchronous and asynchronous.
> >
> > At present, we only support cipher and algorithm
> > chainning.
>
> s/chainning/chaining/
>
Will fix it.
> >
> > Signed-off-by: Gonglei <arei.gong...@huawei.com>
> > ---
> > hw/virtio/virtio-crypto.c | 338
> +++++++++++++++++++++++++++++++++++++-
> > include/hw/virtio/virtio-crypto.h | 10 +-
> > 2 files changed, 345 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> > index db86796..094bc48 100644
> > --- a/hw/virtio/virtio-crypto.c
> > +++ b/hw/virtio/virtio-crypto.c
> > @@ -306,6 +306,342 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
> > } /* end for loop */
> > }
> >
> > +static void virtio_crypto_init_request(VirtIOCrypto *vcrypto, VirtQueue
> > *vq,
> > + VirtIOCryptoReq *req)
> > +{
> > + req->vcrypto = vcrypto;
> > + req->vq = vq;
> > + req->in = NULL;
> > + req->in_iov = NULL;
> > + req->in_num = 0;
> > + req->in_len = 0;
> > + req->flags = CRYPTODEV_BACKEND_ALG__MAX;
> > + req->u.sym_op_info = NULL;
> > +}
> > +
> > +static void virtio_crypto_free_request(VirtIOCryptoReq *req)
> > +{
> > + if (req) {
> > + if (req->flags == CRYPTODEV_BACKEND_ALG_SYM) {
> > + g_free(req->u.sym_op_info);
> > + }
> > + g_free(req);
> > + }
> > +}
> > +
> > +static void
> > +virtio_crypto_sym_input_data_helper(VirtIODevice *vdev,
> > + VirtIOCryptoReq *req,
> > + uint32_t status,
> > + CryptoDevBackendSymOpInfo *sym_op_info)
> > +{
> > + size_t s, len;
> > +
> > + if (status != VIRTIO_CRYPTO_OK) {
> > + return;
> > + }
> > +
> > + len = sym_op_info->dst_len;
> > + /* Save the cipher result */
> > + s = iov_from_buf(req->in_iov, req->in_num, 0, sym_op_info->dst, len);
> > + if (s != len) {
> > + virtio_error(vdev, "virtio-crypto dest data incorrect");
> > + return;
> > + }
> > +
> > + iov_discard_front(&req->in_iov, &req->in_num, len);
> > +
> > + if (sym_op_info->op_type ==
> > +
> VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING) {
> > + /* Save the digest result */
> > + s = iov_from_buf(req->in_iov, req->in_num, 0,
> > + sym_op_info->digest_result,
> > + sym_op_info->digest_result_len);
> > + if (s != sym_op_info->digest_result_len) {
> > + virtio_error(vdev, "virtio-crypto digest result incorrect");
> > + }
> > + }
> > +}
> > +
> > +static void virtio_crypto_req_complete(VirtIOCryptoReq *req, uint32_t
> status)
> > +{
> > + VirtIOCrypto *vcrypto = req->vcrypto;
> > + VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > +
> > + if (req->flags == CRYPTODEV_BACKEND_ALG_SYM) {
> > + virtio_crypto_sym_input_data_helper(vdev, req, status,
> > + req->u.sym_op_info);
> > + }
> > + stl_he_p(&req->in->status, status);
>
> This should be virtio_stl_p().
>
> > + virtqueue_push(req->vq, &req->elem, req->in_len);
> > + virtio_notify(vdev, req->vq);
> > +}
> > +
> > +static VirtIOCryptoReq *
> > +virtio_crypto_get_request(VirtIOCrypto *s, VirtQueue *vq)
> > +{
> > + VirtIOCryptoReq *req = virtqueue_pop(vq, sizeof(VirtIOCryptoReq));
> > +
> > + if (req) {
> > + virtio_crypto_init_request(s, vq, req);
> > + }
> > + return req;
> > +}
> > +
> > +static CryptoDevBackendSymOpInfo *
> > +virtio_crypto_sym_op_helper(VirtIODevice *vdev,
> > + struct virtio_crypto_cipher_para *para,
> > + uint32_t aad_len,
> > + struct iovec *iov, unsigned int out_num,
> > + uint32_t hash_result_len,
> > + uint32_t hash_start_src_offset)
> > +{
> > + CryptoDevBackendSymOpInfo *op_info;
> > + uint32_t src_len, dst_len;
> > + uint32_t iv_len;
> > + size_t max_len, curr_size = 0;
> > + size_t s;
> > +
> > + iv_len = virtio_ldl_p(vdev, ¶->iv_len);
> > + src_len = virtio_ldl_p(vdev, ¶->src_data_len);
> > + dst_len = virtio_ldl_p(vdev, ¶->dst_data_len);
> > +
> > + max_len = iv_len + aad_len + src_len + dst_len + hash_result_len;
>
> Integer overflow checks are needed here to prevent memory corruption
> later on.
>
> Imagine a 32-bit host where sizeof(max_len) == 4 and iv_len + aad_len +
> ... == UINT32_MAX + 1. The malloc below will allocate just 1 byte
> instead of UINT32_MAX + 1 due to overflow.
>
OK. Will fix it.
> > + op_info = g_malloc0(sizeof(CryptoDevBackendSymOpInfo) + max_len);
> > + op_info->iv_len = iv_len;
> > + op_info->src_len = src_len;
> > + op_info->dst_len = dst_len;
> > + op_info->aad_len = aad_len;
> > + op_info->digest_result_len = hash_result_len;
> > + op_info->hash_start_src_offset = hash_start_src_offset;
> > + /* Handle the initilization vector */
> > + if (op_info->iv_len > 0) {
> > + DPRINTF("iv_len=%" PRIu32 "\n", op_info->iv_len);
> > + op_info->iv = op_info->data + curr_size;
> > +
> > + s = iov_to_buf(iov, out_num, 0, op_info->iv, op_info->iv_len);
> > + if (unlikely(s != op_info->iv_len)) {
> > + virtio_error(vdev, "virtio-crypto iv incorrect");
> > + goto err;
> > + }
> > + iov_discard_front(&iov, &out_num, op_info->iv_len);
> > + curr_size += op_info->iv_len;
> > + }
> > +
> > + /* Handle additional authentication data if exist */
> > + if (op_info->aad_len > 0) {
> > + DPRINTF("aad_len=%" PRIu32 "\n", op_info->aad_len);
> > + op_info->aad_data = op_info->data + curr_size;
> > +
> > + s = iov_to_buf(iov, out_num, 0, op_info->aad_data,
> op_info->aad_len);
> > + if (unlikely(s != op_info->aad_len)) {
> > + virtio_error(vdev, "virtio-crypto additional auth data
> incorrect");
> > + goto err;
> > + }
> > + iov_discard_front(&iov, &out_num, op_info->aad_len);
> > +
> > + curr_size += op_info->aad_len;
> > + }
> > +
> > + /* Handle the source data */
> > + if (op_info->src_len > 0) {
> > + DPRINTF("src_len=%" PRIu32 "\n", op_info->src_len);
> > + op_info->src = op_info->data + curr_size;
> > +
> > + s = iov_to_buf(iov, out_num, 0, op_info->src, op_info->src_len);
> > + if (unlikely(s != op_info->src_len)) {
> > + virtio_error(vdev, "virtio-crypto source data incorrect");
> > + goto err;
> > + }
> > + iov_discard_front(&iov, &out_num, op_info->src_len);
> > +
> > + curr_size += op_info->src_len;
> > + }
> > +
> > + /* Handle the destination data */
> > + op_info->dst = op_info->data + curr_size;
> > + curr_size += op_info->dst_len;
> > +
> > + DPRINTF("dst_len=%" PRIu32 "\n", op_info->dst_len);
> > +
> > + /* Handle the hash digest result */
> > + if (hash_result_len > 0) {
> > + DPRINTF("hash_result_len=%" PRIu32 "\n", hash_result_len);
> > + op_info->digest_result = op_info->data + curr_size;
> > + }
> > +
> > + return op_info;
> > +
> > +err:
> > + g_free(op_info);
> > + return NULL;
> > +}
> > +
> > +static int
> > +virtio_crypto_handle_sym_req(VirtIOCrypto *vcrypto,
> > + struct virtio_crypto_sym_data_req *req,
> > + CryptoDevBackendSymOpInfo **sym_op_info,
> > + struct iovec *iov, unsigned int out_num)
> > +{
> > + VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > + uint32_t op_type;
> > + CryptoDevBackendSymOpInfo *op_info;
> > +
> > + op_type = virtio_ldl_p(vdev, &req->op_type);
> > +
> > + if (op_type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> > + op_info = virtio_crypto_sym_op_helper(vdev, &req->u.cipher.para,
> > + 0, iov, out_num, 0,
> 0);
> > + if (!op_info) {
> > + return -EFAULT;
> > + }
> > + op_info->op_type = op_type;
> > + } else if (op_type ==
> VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING) {
> > + uint32_t aad_len, hash_result_len;
> > + uint32_t hash_start_src_offset;
> > +
> > + aad_len = virtio_ldl_p(vdev, &req->u.chain.para.aad_len);
> > + hash_result_len = virtio_ldl_p(vdev,
> > + &req->u.chain.para.hash_result_len);
> > + hash_start_src_offset = virtio_ldl_p(vdev,
> > + &req->u.chain.para.hash_start_src_offset);
> > + /* cipher part */
> > + op_info = virtio_crypto_sym_op_helper(vdev,
> &req->u.chain.para.cipher,
> > + aad_len, iov,
> out_num,
> > + hash_result_len,
> > +
> hash_start_src_offset);
> > + if (!op_info) {
> > + return -EFAULT;
> > + }
> > + op_info->op_type = op_type;
> > + } else {
> > + /* VIRTIO_CRYPTO_SYM_OP_NONE */
> > + error_report("virtio-crypto unsupported cipher type");
> > + return -VIRTIO_CRYPTO_NOTSUPP;
> > + }
> > +
> > + *sym_op_info = op_info;
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +virtio_crypto_handle_request(VirtIOCryptoReq *request)
> > +{
> > + VirtIOCrypto *vcrypto = request->vcrypto;
> > + VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > + VirtQueueElement *elem = &request->elem;
> > + int queue_index =
> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> > + struct virtio_crypto_op_data_req req;
> > + int ret;
> > + struct iovec *in_iov;
> > + struct iovec *out_iov;
> > + unsigned in_num;
> > + unsigned out_num;
> > + uint32_t opcode, status = VIRTIO_CRYPTO_ERR;
> > + uint64_t session_id;
> > + CryptoDevBackendSymOpInfo *sym_op_info = NULL;
> > + Error *local_err = NULL;
> > +
> > + if (elem->out_num < 1 || elem->in_num < 1) {
> > + virtio_error(vdev, "virtio-crypto dataq missing headers");
> > + return -1;
> > + }
> > +
> > + out_num = elem->out_num;
> > + out_iov = elem->out_sg;
> > + in_num = elem->in_num;
> > + in_iov = elem->in_sg;
> > + if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> > + != sizeof(req))) {
> > + virtio_error(vdev, "virtio-crypto request outhdr too short");
> > + return -1;
> > + }
> > + iov_discard_front(&out_iov, &out_num, sizeof(req));
> > +
> > + if (in_iov[in_num - 1].iov_len <
> > + sizeof(struct virtio_crypto_inhdr)) {
> > + virtio_error(vdev, "virtio-crypto request inhdr too short");
> > + return -1;
> > + }
> > +
> > + request->in_len = iov_size(in_iov, in_num);
> > + request->in = (void *)in_iov[in_num - 1].iov_base
> > + + in_iov[in_num - 1].iov_len
> > + - sizeof(struct virtio_crypto_inhdr);
> > + iov_discard_back(in_iov, &in_num, sizeof(struct virtio_crypto_inhdr));
> > +
> > + /*
> > + * The length of operation result, including dest_data
> > + * and digest_result if exist.
> > + */
> > + request->in_num = in_num;
> > + request->in_iov = in_iov;
> > +
> > + opcode = virtio_ldl_p(vdev, &req.header.opcode);
> > + session_id = virtio_ldq_p(vdev, &req.header.session_id);
> > +
> > + switch (opcode) {
> > + case VIRTIO_CRYPTO_CIPHER_ENCRYPT:
> > + case VIRTIO_CRYPTO_CIPHER_DECRYPT:
> > + ret = virtio_crypto_handle_sym_req(vcrypto,
> > + &req.u.sym_req,
> > + &sym_op_info,
> > + out_iov, out_num);
> > + /* Serious errors, need to reset virtio crypto device */
> > + if (ret == -EFAULT) {
> > + return -1;
> > + } else if (ret == -VIRTIO_CRYPTO_NOTSUPP) {
> > + virtio_crypto_req_complete(request,
> VIRTIO_CRYPTO_NOTSUPP);
> > + virtio_crypto_free_request(request);
> > + } else {
> > + sym_op_info->session_id = session_id;
> > +
> > + /* Set request's parameter */
> > + request->flags = CRYPTODEV_BACKEND_ALG_SYM;
> > + request->u.sym_op_info = sym_op_info;
> > + ret = cryptodev_backend_sym_operation(vcrypto->cryptodev,
> > + sym_op_info, queue_index,
> &local_err);
> > + if (ret < 0) {
> > + status = VIRTIO_CRYPTO_ERR;
> > + if (local_err) {
> > + error_report_err(local_err);
> > + }
> > + } else { /* ret >= 0 */
> > + status = VIRTIO_CRYPTO_OK;
> > + }
> > + virtio_crypto_req_complete(request, status);
> > + virtio_crypto_free_request(request);
>
> Shouldn't the operation be asynchronous from the start? There is no
> point in having a 1024 element virtqueue if you can only process one
> element at a time.
>
Good insight. Currently I only realize the synchronous operation for QEMU
builtin backend, but we can realized the asynchronous operation in future.
And for cryptodev-vhost-user, we can get better performance using 1024
as the data virtuqueue's size, so I'd like to keep it by default.
> > + }
> > + break;
> > + case VIRTIO_CRYPTO_HASH:
> > + case VIRTIO_CRYPTO_MAC:
> > + case VIRTIO_CRYPTO_AEAD_ENCRYPT:
> > + case VIRTIO_CRYPTO_AEAD_DECRYPT:
> > + default:
> > + error_report("virtio-crypto unsupported dataq opcode: %u",
> > + opcode);
> > + virtio_crypto_req_complete(request, VIRTIO_CRYPTO_NOTSUPP);
> > + virtio_crypto_free_request(request);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void virtio_crypto_handle_dataq(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > + VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
> > + VirtIOCryptoReq *req;
> > +
> > + while ((req = virtio_crypto_get_request(vcrypto, vq))) {
> > + if (virtio_crypto_handle_request(req) < 0) {
> > + virtqueue_detach_element(req->vq, &req->elem, 0);
> > + virtio_crypto_free_request(req);
> > + break;
> > + }
> > + }
> > +}
> > +
> > static uint64_t virtio_crypto_get_features(VirtIODevice *vdev,
> > uint64_t features,
> > Error **errp)
> > @@ -365,7 +701,7 @@ static void virtio_crypto_device_realize(DeviceState
> *dev, Error **errp)
> > vcrypto->curr_queues = 1;
> >
> > for (i = 0; i < vcrypto->max_queues; i++) {
> > - virtio_add_queue(vdev, 1024, NULL);
> > + virtio_add_queue(vdev, 1024, virtio_crypto_handle_dataq);
> > }
> >
> > vcrypto->ctrl_vq = virtio_add_queue(vdev, 64,
> virtio_crypto_handle_ctrl);
> > diff --git a/include/hw/virtio/virtio-crypto.h
> > b/include/hw/virtio/virtio-crypto.h
> > index a5e826c..1427d39 100644
> > --- a/include/hw/virtio/virtio-crypto.h
> > +++ b/include/hw/virtio/virtio-crypto.h
> > @@ -36,6 +36,10 @@ do { \
> > #define VIRTIO_CRYPTO_GET_PARENT_CLASS(obj) \
> > OBJECT_GET_PARENT_CLASS(obj, TYPE_VIRTIO_CRYPTO)
> >
> > +/* This is the last element of the write scatter-gather list */
>
> This comment is misleading. The last element of the scatter-gather list
> could contain any number of other structs too. Remember the
> specification says virtio devices cannot rely on specific iovec
> boundaries.
>
OK, will remove it in the next version.
> > +struct virtio_crypto_inhdr {
> > + uint32_t status;
> > +};
>
> Why is this defined here? I think it should be in virtio_crypto.h and
> the field type should be __virtio32.
>
Yes. I'd like to define the status to uint8_t because it's enough.
> >
> > typedef struct VirtIOCryptoConf {
> > CryptoDevBackend *cryptodev;
> > @@ -61,8 +65,10 @@ typedef struct VirtIOCryptoReq {
> > VirtQueueElement elem;
> > /* flags of operation, such as type of algorithm */
> > uint32_t flags;
> > - /* address of in data (Device to Driver) */
> > - void *idata_hva;
>
> This field was unused. Please remove its definition and wait until this
> patch to define it properly.
>
> This way the code is easier to review because there are no unused fields
> that the reviewer has to guess about when reading the code.
>
You are right, it's useless here. Let's drop it.
Regards,
-Gonglei
> >
