From: "Michael S. Tsirkin" <mst@redhat.com>
To: Lei He <helei.sig11@bytedance.com>
Cc: berrange@redhat.com, arei.gonglei@huawei.com,
pizhenwei@bytedance.com, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 1/4] virtio-crypto: Support asynchronous mode
Date: Tue, 1 Nov 2022 15:51:36 -0400 [thread overview]
Message-ID: <20221101154944-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20221101063515-mutt-send-email-mst@kernel.org>
On Tue, Nov 01, 2022 at 06:37:26AM -0400, Michael S. Tsirkin wrote:
> On Sat, Oct 08, 2022 at 04:50:27PM +0800, Lei He wrote:
> > virtio-crypto: Modify the current interface of virtio-crypto
> > device to support asynchronous mode.
> >
> > Signed-off-by: lei he <helei.sig11@bytedance.com>
> > ---
> > backends/cryptodev-builtin.c | 69 ++++++---
> > backends/cryptodev-vhost-user.c | 51 +++++--
> > backends/cryptodev.c | 44 +++---
> > hw/virtio/virtio-crypto.c | 324 ++++++++++++++++++++++------------------
> > include/sysemu/cryptodev.h | 60 +++++---
> > 5 files changed, 336 insertions(+), 212 deletions(-)
> >
> > diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
> > index 125cbad1d3..cda6ca3b71 100644
> > --- a/backends/cryptodev-builtin.c
> > +++ b/backends/cryptodev-builtin.c
> > @@ -355,42 +355,62 @@ static int cryptodev_builtin_create_akcipher_session(
> > return index;
> > }
> >
> > -static int64_t cryptodev_builtin_create_session(
> > +static int cryptodev_builtin_create_session(
> > CryptoDevBackend *backend,
> > CryptoDevBackendSessionInfo *sess_info,
> > - uint32_t queue_index, Error **errp)
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque)
> > {
> > CryptoDevBackendBuiltin *builtin =
> > CRYPTODEV_BACKEND_BUILTIN(backend);
> > CryptoDevBackendSymSessionInfo *sym_sess_info;
> > CryptoDevBackendAsymSessionInfo *asym_sess_info;
> > + int ret, status;
> > + Error *local_error = NULL;
> >
> > switch (sess_info->op_code) {
> > case VIRTIO_CRYPTO_CIPHER_CREATE_SESSION:
> > sym_sess_info = &sess_info->u.sym_sess_info;
> > - return cryptodev_builtin_create_cipher_session(
> > - builtin, sym_sess_info, errp);
> > + ret = cryptodev_builtin_create_cipher_session(
> > + builtin, sym_sess_info, &local_error);
> > + break;
> >
> > case VIRTIO_CRYPTO_AKCIPHER_CREATE_SESSION:
> > asym_sess_info = &sess_info->u.asym_sess_info;
> > - return cryptodev_builtin_create_akcipher_session(
> > - builtin, asym_sess_info, errp);
> > + ret = cryptodev_builtin_create_akcipher_session(
> > + builtin, asym_sess_info, &local_error);
> > + break;
> >
> > case VIRTIO_CRYPTO_HASH_CREATE_SESSION:
> > case VIRTIO_CRYPTO_MAC_CREATE_SESSION:
> > default:
> > - error_setg(errp, "Unsupported opcode :%" PRIu32 "",
> > + error_setg(&local_error, "Unsupported opcode :%" PRIu32 "",
> > sess_info->op_code);
> > - return -1;
> > + return -VIRTIO_CRYPTO_NOTSUPP;
> > }
> >
> > - return -1;
> > + if (local_error) {
> > + error_report_err(local_error);
> > + }
> > + if (ret < 0) {
> > + status = -VIRTIO_CRYPTO_ERR;
> > + } else {
> > + sess_info->session_id = ret;
> > + status = VIRTIO_CRYPTO_OK;
> > + }
> > + if (cb) {
> > + cb(opaque, status);
> > + }
> > + return 0;
> > }
> >
> > static int cryptodev_builtin_close_session(
> > CryptoDevBackend *backend,
> > uint64_t session_id,
> > - uint32_t queue_index, Error **errp)
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque)
> > {
> > CryptoDevBackendBuiltin *builtin =
> > CRYPTODEV_BACKEND_BUILTIN(backend);
> > @@ -407,6 +427,9 @@ static int cryptodev_builtin_close_session(
> >
> > g_free(session);
> > builtin->sessions[session_id] = NULL;
> > + if (cb) {
> > + cb(opaque, VIRTIO_CRYPTO_OK);
> > + }
> > return 0;
> > }
> >
> > @@ -506,7 +529,9 @@ static int cryptodev_builtin_asym_operation(
> > static int cryptodev_builtin_operation(
> > CryptoDevBackend *backend,
> > CryptoDevBackendOpInfo *op_info,
> > - uint32_t queue_index, Error **errp)
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque)
> > {
> > CryptoDevBackendBuiltin *builtin =
> > CRYPTODEV_BACKEND_BUILTIN(backend);
> > @@ -514,11 +539,12 @@ static int cryptodev_builtin_operation(
> > CryptoDevBackendSymOpInfo *sym_op_info;
> > CryptoDevBackendAsymOpInfo *asym_op_info;
> > enum CryptoDevBackendAlgType algtype = op_info->algtype;
> > - int ret = -VIRTIO_CRYPTO_ERR;
> > + int status = -VIRTIO_CRYPTO_ERR;
> > + Error *local_error = NULL;
> >
> > if (op_info->session_id >= MAX_NUM_SESSIONS ||
> > builtin->sessions[op_info->session_id] == NULL) {
> > - error_setg(errp, "Cannot find a valid session id: %" PRIu64 "",
> > + error_setg(&local_error, "Cannot find a valid session id: %" PRIu64 "",
> > op_info->session_id);
> > return -VIRTIO_CRYPTO_INVSESS;
> > }
> > @@ -526,14 +552,21 @@ static int cryptodev_builtin_operation(
> > sess = builtin->sessions[op_info->session_id];
> > if (algtype == CRYPTODEV_BACKEND_ALG_SYM) {
> > sym_op_info = op_info->u.sym_op_info;
> > - ret = cryptodev_builtin_sym_operation(sess, sym_op_info, errp);
> > + status = cryptodev_builtin_sym_operation(sess, sym_op_info,
> > + &local_error);
> > } else if (algtype == CRYPTODEV_BACKEND_ALG_ASYM) {
> > asym_op_info = op_info->u.asym_op_info;
> > - ret = cryptodev_builtin_asym_operation(sess, op_info->op_code,
> > - asym_op_info, errp);
> > + status = cryptodev_builtin_asym_operation(sess, op_info->op_code,
> > + asym_op_info, &local_error);
> > }
> >
> > - return ret;
> > + if (local_error) {
> > + error_report_err(local_error);
> > + }
> > + if (cb) {
> > + cb(opaque, status);
> > + }
> > + return 0;
> > }
> >
> > static void cryptodev_builtin_cleanup(
> > @@ -548,7 +581,7 @@ static void cryptodev_builtin_cleanup(
> >
> > for (i = 0; i < MAX_NUM_SESSIONS; i++) {
> > if (builtin->sessions[i] != NULL) {
> > - cryptodev_builtin_close_session(backend, i, 0, &error_abort);
> > + cryptodev_builtin_close_session(backend, i, 0, NULL, NULL);
> > }
> > }
> >
> > diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
> > index 5443a59153..351b2729e0 100644
> > --- a/backends/cryptodev-vhost-user.c
> > +++ b/backends/cryptodev-vhost-user.c
> > @@ -259,13 +259,18 @@ static int64_t cryptodev_vhost_user_sym_create_session(
> > return -1;
> > }
> >
> > -static int64_t cryptodev_vhost_user_create_session(
> > +static int cryptodev_vhost_user_create_session(
> > CryptoDevBackend *backend,
> > CryptoDevBackendSessionInfo *sess_info,
> > - uint32_t queue_index, Error **errp)
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque)
> > {
> > uint32_t op_code = sess_info->op_code;
> > CryptoDevBackendSymSessionInfo *sym_sess_info;
> > + int64_t ret;
> > + Error *local_error = NULL;
> > + int status;
> >
> > switch (op_code) {
> > case VIRTIO_CRYPTO_CIPHER_CREATE_SESSION:
> > @@ -273,27 +278,42 @@ static int64_t cryptodev_vhost_user_create_session(
> > case VIRTIO_CRYPTO_MAC_CREATE_SESSION:
> > case VIRTIO_CRYPTO_AEAD_CREATE_SESSION:
> > sym_sess_info = &sess_info->u.sym_sess_info;
> > - return cryptodev_vhost_user_sym_create_session(backend, sym_sess_info,
> > - queue_index, errp);
> > + ret = cryptodev_vhost_user_sym_create_session(backend, sym_sess_info,
> > + queue_index, &local_error);
> > + break;
> > +
> > default:
> > - error_setg(errp, "Unsupported opcode :%" PRIu32 "",
> > + error_setg(&local_error, "Unsupported opcode :%" PRIu32 "",
> > sess_info->op_code);
> > - return -1;
> > -
> > + return -VIRTIO_CRYPTO_NOTSUPP;
> > }
> >
> > - return -1;
> > + if (local_error) {
> > + error_report_err(local_error);
> > + }
> > + if (ret < 0) {
> > + status = -VIRTIO_CRYPTO_ERR;
> > + } else {
> > + sess_info->session_id = ret;
> > + status = VIRTIO_CRYPTO_OK;
> > + }
> > + if (cb) {
> > + cb(opaque, status);
> > + }
> > + return 0;
> > }
> >
> > static int cryptodev_vhost_user_close_session(
> > CryptoDevBackend *backend,
> > uint64_t session_id,
> > - uint32_t queue_index, Error **errp)
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque)
> > {
> > CryptoDevBackendClient *cc =
> > backend->conf.peers.ccs[queue_index];
> > CryptoDevBackendVhost *vhost_crypto;
> > - int ret;
> > + int ret = -1, status;
> >
> > vhost_crypto = cryptodev_vhost_user_get_vhost(cc, backend, queue_index);
> > if (vhost_crypto) {
> > @@ -301,12 +321,17 @@ static int cryptodev_vhost_user_close_session(
> > ret = dev->vhost_ops->vhost_crypto_close_session(dev,
> > session_id);
> > if (ret < 0) {
> > - return -1;
> > + status = -VIRTIO_CRYPTO_ERR;
> > } else {
> > - return 0;
> > + status = VIRTIO_CRYPTO_OK;
> > }
> > + } else {
> > + status = -VIRTIO_CRYPTO_NOTSUPP;
> > }
> > - return -1;
> > + if (cb) {
> > + cb(opaque, status);
> > + }
> > + return 0;
> > }
> >
> > static void cryptodev_vhost_user_cleanup(
> > diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> > index 33eb4e1a70..54ee8c81f5 100644
> > --- a/backends/cryptodev.c
> > +++ b/backends/cryptodev.c
> > @@ -26,6 +26,7 @@
> > #include "qapi/error.h"
> > #include "qapi/visitor.h"
> > #include "qemu/config-file.h"
> > +#include "qemu/error-report.h"
> > #include "qom/object_interfaces.h"
> > #include "hw/virtio/virtio-crypto.h"
> >
> > @@ -72,69 +73,72 @@ void cryptodev_backend_cleanup(
> > }
> > }
> >
> > -int64_t cryptodev_backend_create_session(
> > +int cryptodev_backend_create_session(
> > CryptoDevBackend *backend,
> > CryptoDevBackendSessionInfo *sess_info,
> > - uint32_t queue_index, Error **errp)
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque)
> > {
> > CryptoDevBackendClass *bc =
> > CRYPTODEV_BACKEND_GET_CLASS(backend);
> >
> > if (bc->create_session) {
> > - return bc->create_session(backend, sess_info, queue_index, errp);
> > + return bc->create_session(backend, sess_info, queue_index, cb, opaque);
> > }
> > -
> > - return -1;
> > + return -VIRTIO_CRYPTO_NOTSUPP;
> > }
> >
> > int cryptodev_backend_close_session(
> > CryptoDevBackend *backend,
> > uint64_t session_id,
> > - uint32_t queue_index, Error **errp)
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque)
> > {
> > CryptoDevBackendClass *bc =
> > CRYPTODEV_BACKEND_GET_CLASS(backend);
> >
> > if (bc->close_session) {
> > - return bc->close_session(backend, session_id, queue_index, errp);
> > + return bc->close_session(backend, session_id, queue_index, cb, opaque);
> > }
> > -
> > - return -1;
> > + return -VIRTIO_CRYPTO_NOTSUPP;
> > }
> >
> > static int cryptodev_backend_operation(
> > CryptoDevBackend *backend,
> > CryptoDevBackendOpInfo *op_info,
> > - uint32_t queue_index, Error **errp)
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque)
> > {
> > CryptoDevBackendClass *bc =
> > CRYPTODEV_BACKEND_GET_CLASS(backend);
> >
> > if (bc->do_op) {
> > - return bc->do_op(backend, op_info, queue_index, errp);
> > + return bc->do_op(backend, op_info, queue_index, cb, opaque);
> > }
> > -
> > - return -VIRTIO_CRYPTO_ERR;
> > + return -VIRTIO_CRYPTO_NOTSUPP;
> > }
> >
> > int cryptodev_backend_crypto_operation(
> > CryptoDevBackend *backend,
> > - void *opaque,
> > - uint32_t queue_index, Error **errp)
> > + void *opaque1,
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb, void *opaque2)
> > {
> > - VirtIOCryptoReq *req = opaque;
> > + VirtIOCryptoReq *req = opaque1;
> > CryptoDevBackendOpInfo *op_info = &req->op_info;
> > enum CryptoDevBackendAlgType algtype = req->flags;
> >
> > if ((algtype != CRYPTODEV_BACKEND_ALG_SYM)
> > && (algtype != CRYPTODEV_BACKEND_ALG_ASYM)) {
> > - error_setg(errp, "Unsupported cryptodev alg type: %" PRIu32 "",
> > - algtype);
> > -
> > + error_report("Unsupported cryptodev alg type: %" PRIu32 "", algtype);
> > return -VIRTIO_CRYPTO_NOTSUPP;
> > }
> >
> > - return cryptodev_backend_operation(backend, op_info, queue_index, errp);
> > + return cryptodev_backend_operation(backend, op_info, queue_index,
> > + cb, opaque2);
> > }
> >
> > static void
> > diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> > index df4bde210b..39c8f5914e 100644
> > --- a/hw/virtio/virtio-crypto.c
> > +++ b/hw/virtio/virtio-crypto.c
> > @@ -27,6 +27,39 @@
> >
> > #define VIRTIO_CRYPTO_VM_VERSION 1
> >
> > +typedef struct VirtIOCryptoSessionReq {
> > + VirtIODevice *vdev;
> > + VirtQueue *vq;
> > + VirtQueueElement *elem;
> > + CryptoDevBackendSessionInfo info;
> > + CryptoDevCompletionFunc cb;
> > +} VirtIOCryptoSessionReq;
> > +
> > +static void virtio_crypto_free_create_session_req(VirtIOCryptoSessionReq *sreq)
> > +{
> > + switch (sreq->info.op_code) {
> > + case VIRTIO_CRYPTO_CIPHER_CREATE_SESSION:
> > + g_free(sreq->info.u.sym_sess_info.cipher_key);
> > + g_free(sreq->info.u.sym_sess_info.auth_key);
> > + break;
> > +
> > + case VIRTIO_CRYPTO_AKCIPHER_CREATE_SESSION:
> > + g_free(sreq->info.u.asym_sess_info.key);
> > + break;
> > +
> > + case VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION:
> > + case VIRTIO_CRYPTO_HASH_DESTROY_SESSION:
> > + case VIRTIO_CRYPTO_MAC_DESTROY_SESSION:
> > + case VIRTIO_CRYPTO_AEAD_DESTROY_SESSION:
> > + case VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION:
> > + break;
> > +
> > + default:
> > + error_report("Unknown opcode: %u", sreq->info.op_code);
> > + }
> > + g_free(sreq);
> > +}
> > +
> > /*
> > * Transfer virtqueue index to crypto queue index.
> > * The control virtqueue is after the data virtqueues
> > @@ -75,27 +108,24 @@ virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
> > return 0;
> > }
> >
> > -static int64_t
> > +static int
> > virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
> > struct virtio_crypto_sym_create_session_req *sess_req,
> > uint32_t queue_id,
> > uint32_t opcode,
> > - struct iovec *iov, unsigned int out_num)
> > + struct iovec *iov, unsigned int out_num,
> > + VirtIOCryptoSessionReq *sreq)
> > {
> > VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > - CryptoDevBackendSessionInfo info;
> > - CryptoDevBackendSymSessionInfo *sym_info;
> > - int64_t session_id;
> > + CryptoDevBackendSymSessionInfo *sym_info = &sreq->info.u.sym_sess_info;
> > int queue_index;
> > uint32_t op_type;
> > - Error *local_err = NULL;
> > int ret;
> >
> > - memset(&info, 0, sizeof(info));
> > op_type = ldl_le_p(&sess_req->op_type);
> > - info.op_code = opcode;
> > + sreq->info.op_code = opcode;
> >
> > - sym_info = &info.u.sym_sess_info;
> > + sym_info = &sreq->info.u.sym_sess_info;
> > sym_info->op_type = op_type;
> >
> > if (op_type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> > @@ -103,7 +133,7 @@ virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
> > &sess_req->u.cipher.para,
> > &iov, &out_num);
> > if (ret < 0) {
> > - goto err;
> > + return ret;
> > }
> > } else if (op_type == VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING) {
> > size_t s;
> > @@ -112,7 +142,7 @@ virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
> > &sess_req->u.chain.para.cipher_param,
> > &iov, &out_num);
> > if (ret < 0) {
> > - goto err;
> > + return ret;
> > }
> > /* hash part */
> > sym_info->alg_chain_order = ldl_le_p(
> > @@ -129,8 +159,7 @@ virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
> > if (sym_info->auth_key_len > vcrypto->conf.max_auth_key_len) {
> > error_report("virtio-crypto length of auth key is too big: %u",
> > sym_info->auth_key_len);
> > - ret = -VIRTIO_CRYPTO_ERR;
> > - goto err;
> > + return -VIRTIO_CRYPTO_ERR;
> > }
> > /* get auth key */
> > if (sym_info->auth_key_len > 0) {
> > @@ -140,8 +169,7 @@ virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
> > if (unlikely(s != sym_info->auth_key_len)) {
> > virtio_error(vdev,
> > "virtio-crypto authenticated key incorrect");
> > - ret = -EFAULT;
> > - goto err;
> > + return -EFAULT;
> > }
> > iov_discard_front(&iov, &out_num, sym_info->auth_key_len);
> > }
> > @@ -153,49 +181,30 @@ virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
> > } else {
> > /* VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED */
> > error_report("unsupported hash mode");
> > - ret = -VIRTIO_CRYPTO_NOTSUPP;
> > - goto err;
> > + return -VIRTIO_CRYPTO_NOTSUPP;
> > }
> > } else {
> > /* VIRTIO_CRYPTO_SYM_OP_NONE */
> > error_report("unsupported cipher op_type: VIRTIO_CRYPTO_SYM_OP_NONE");
> > - ret = -VIRTIO_CRYPTO_NOTSUPP;
> > - goto err;
> > + return -VIRTIO_CRYPTO_NOTSUPP;
> > }
> >
> > queue_index = virtio_crypto_vq2q(queue_id);
> > - session_id = cryptodev_backend_create_session(
> > - vcrypto->cryptodev,
> > - &info, queue_index, &local_err);
> > - if (session_id >= 0) {
> > - ret = session_id;
> > - } else {
> > - if (local_err) {
> > - error_report_err(local_err);
> > - }
> > - ret = -VIRTIO_CRYPTO_ERR;
> > - }
> > -
> > -err:
> > - g_free(sym_info->cipher_key);
> > - g_free(sym_info->auth_key);
> > - return ret;
> > + return cryptodev_backend_create_session(vcrypto->cryptodev, &sreq->info,
> > + queue_index, sreq->cb, sreq);
> > }
> >
> > -static int64_t
> > +static int
> > virtio_crypto_create_asym_session(VirtIOCrypto *vcrypto,
> > struct virtio_crypto_akcipher_create_session_req *sess_req,
> > uint32_t queue_id, uint32_t opcode,
> > - struct iovec *iov, unsigned int out_num)
> > + struct iovec *iov, unsigned int out_num,
> > + VirtIOCryptoSessionReq *sreq)
> > {
> > VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > - CryptoDevBackendSessionInfo info = {0};
> > - CryptoDevBackendAsymSessionInfo *asym_info;
> > - int64_t session_id;
> > + CryptoDevBackendAsymSessionInfo *asym_info = &sreq->info.u.asym_sess_info;
> > int queue_index;
> > uint32_t algo, keytype, keylen;
> > - g_autofree uint8_t *key = NULL;
> > - Error *local_err = NULL;
> >
> > algo = ldl_le_p(&sess_req->para.algo);
> > keytype = ldl_le_p(&sess_req->para.keytype);
> > @@ -208,20 +217,19 @@ virtio_crypto_create_asym_session(VirtIOCrypto *vcrypto,
> > }
> >
> > if (keylen) {
> > - key = g_malloc(keylen);
> > - if (iov_to_buf(iov, out_num, 0, key, keylen) != keylen) {
> > + asym_info->key = g_malloc(keylen);
> > + if (iov_to_buf(iov, out_num, 0, asym_info->key, keylen) != keylen) {
> > virtio_error(vdev, "virtio-crypto asym key incorrect");
> > return -EFAULT;
> > }
> > iov_discard_front(&iov, &out_num, keylen);
> > }
> >
> > - info.op_code = opcode;
> > - asym_info = &info.u.asym_sess_info;
> > + sreq->info.op_code = opcode;
> > + asym_info = &sreq->info.u.asym_sess_info;
> > asym_info->algo = algo;
> > asym_info->keytype = keytype;
> > asym_info->keylen = keylen;
> > - asym_info->key = key;
> > switch (asym_info->algo) {
> > case VIRTIO_CRYPTO_AKCIPHER_RSA:
> > asym_info->u.rsa.padding_algo =
> > @@ -237,45 +245,95 @@ virtio_crypto_create_asym_session(VirtIOCrypto *vcrypto,
> > }
> >
> > queue_index = virtio_crypto_vq2q(queue_id);
> > - session_id = cryptodev_backend_create_session(vcrypto->cryptodev, &info,
> > - queue_index, &local_err);
> > - if (session_id < 0) {
> > - if (local_err) {
> > - error_report_err(local_err);
> > - }
> > - return -VIRTIO_CRYPTO_ERR;
> > - }
> > -
> > - return session_id;
> > + return cryptodev_backend_create_session(vcrypto->cryptodev, &sreq->info,
> > + queue_index, sreq->cb, sreq);
> > }
> >
> > -static uint8_t
> > +static int
> > virtio_crypto_handle_close_session(VirtIOCrypto *vcrypto,
> > struct virtio_crypto_destroy_session_req *close_sess_req,
> > - uint32_t queue_id)
> > + uint32_t queue_id,
> > + VirtIOCryptoSessionReq *sreq)
> > {
> > - int ret;
> > uint64_t session_id;
> > - uint32_t status;
> > - Error *local_err = NULL;
> >
> > session_id = ldq_le_p(&close_sess_req->session_id);
> > DPRINTF("close session, id=%" PRIu64 "\n", session_id);
> >
> > - ret = cryptodev_backend_close_session(
> > - vcrypto->cryptodev, session_id, queue_id, &local_err);
> > - if (ret == 0) {
> > - status = VIRTIO_CRYPTO_OK;
> > + return cryptodev_backend_close_session(
> > + vcrypto->cryptodev, session_id, queue_id, sreq->cb, sreq);
> > +}
> > +
> > +static void virtio_crypto_create_session_completion(void *opaque, int ret)
> > +{
> > + VirtIOCryptoSessionReq *sreq = (VirtIOCryptoSessionReq *)opaque;
> > + VirtQueue *vq = sreq->vq;
> > + VirtQueueElement *elem = sreq->elem;
> > + VirtIODevice *vdev = sreq->vdev;
> > + struct virtio_crypto_session_input input;
> > + struct iovec *in_iov = elem->in_sg;
> > + unsigned in_num = elem->in_num;
> > + size_t s;
> > +
> > + memset(&input, 0, sizeof(input));
> > + /* Serious errors, need to reset virtio crypto device */
> > + if (ret == -EFAULT) {
> > + virtqueue_detach_element(vq, elem, 0);
> > + goto out;
> > + } else if (ret == -VIRTIO_CRYPTO_NOTSUPP) {
> > + stl_le_p(&input.status, VIRTIO_CRYPTO_NOTSUPP);
> > + } else if (ret == -VIRTIO_CRYPTO_KEY_REJECTED) {
> > + stl_le_p(&input.status, VIRTIO_CRYPTO_KEY_REJECTED);
> > + } else if (ret != VIRTIO_CRYPTO_OK) {
> > + stl_le_p(&input.status, VIRTIO_CRYPTO_ERR);
> > } else {
> > - if (local_err) {
> > - error_report_err(local_err);
> > - } else {
> > - error_report("destroy session failed");
> > - }
> > + /* Set the session id */
> > + stq_le_p(&input.session_id, sreq->info.session_id);
> > + stl_le_p(&input.status, VIRTIO_CRYPTO_OK);
> > + }
> > +
> > + s = iov_from_buf(in_iov, in_num, 0, &input, sizeof(input));
> > + if (unlikely(s != sizeof(input))) {
> > + virtio_error(vdev, "virtio-crypto input incorrect");
> > + virtqueue_detach_element(vq, elem, 0);
> > + goto out;
> > + }
> > + virtqueue_push(vq, elem, sizeof(input));
> > + virtio_notify(vdev, vq);
> > +
> > +out:
> > + g_free(elem);
> > + virtio_crypto_free_create_session_req(sreq);
> > +}
> > +
> > +static void virtio_crypto_destroy_session_completion(void *opaque, int ret)
> > +{
> > + VirtIOCryptoSessionReq *sreq = (VirtIOCryptoSessionReq *)opaque;
> > + VirtQueue *vq = sreq->vq;
> > + VirtQueueElement *elem = sreq->elem;
> > + VirtIODevice *vdev = sreq->vdev;
> > + struct iovec *in_iov = elem->in_sg;
> > + unsigned in_num = elem->in_num;
> > + uint8_t status;
> > + size_t s;
> > +
> > + if (ret < 0) {
> > status = VIRTIO_CRYPTO_ERR;
> > + } else {
> > + status = VIRTIO_CRYPTO_OK;
> > + }
> > + s = iov_from_buf(in_iov, in_num, 0, &status, sizeof(status));
> > + if (unlikely(s != sizeof(status))) {
> > + virtio_error(vdev, "virtio-crypto status incorrect");
> > + virtqueue_detach_element(vq, elem, 0);
> > + goto out;
> > }
> > + virtqueue_push(vq, elem, sizeof(status));
> > + virtio_notify(vdev, vq);
> >
> > - return status;
> > +out:
> > + g_free(elem);
> > + g_free(sreq);
> > }
> >
> > static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> > @@ -283,16 +341,16 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> > VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
> > struct virtio_crypto_op_ctrl_req ctrl;
> > VirtQueueElement *elem;
> > - struct iovec *in_iov;
> > - struct iovec *out_iov;
> > - unsigned in_num;
> > + VirtIOCryptoSessionReq *sreq;
> > unsigned out_num;
> > + unsigned in_num;
> > uint32_t queue_id;
> > uint32_t opcode;
> > struct virtio_crypto_session_input input;
> > - int64_t session_id;
> > - uint8_t status;
> > size_t s;
> > + int ret;
> > + struct iovec *out_iov;
> > + struct iovec *in_iov;
> >
> > for (;;) {
> > g_autofree struct iovec *out_iov_copy = NULL;
> > @@ -327,44 +385,34 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> > opcode = ldl_le_p(&ctrl.header.opcode);
> > queue_id = ldl_le_p(&ctrl.header.queue_id);
> >
> > - memset(&input, 0, sizeof(input));
> > + sreq = g_new0(VirtIOCryptoSessionReq, 1);
> > + sreq->vdev = vdev;
> > + sreq->vq = vq;
> > + sreq->elem = elem;
> > +
> > switch (opcode) {
> > case VIRTIO_CRYPTO_CIPHER_CREATE_SESSION:
> > - session_id = virtio_crypto_create_sym_session(vcrypto,
> > - &ctrl.u.sym_create_session,
> > - queue_id, opcode,
> > - out_iov, out_num);
> > - goto check_session;
> > + sreq->cb = virtio_crypto_create_session_completion;
> > + ret = virtio_crypto_create_sym_session(vcrypto,
> > + &ctrl.u.sym_create_session,
> > + queue_id, opcode,
> > + out_iov, out_num,
> > + sreq);
> > + if (ret < 0) {
> > + virtio_crypto_create_session_completion(sreq, ret);
> > + }
> > + break;
> >
> > case VIRTIO_CRYPTO_AKCIPHER_CREATE_SESSION:
> > - session_id = virtio_crypto_create_asym_session(vcrypto,
> > + sreq->cb = virtio_crypto_create_session_completion;
> > + ret = virtio_crypto_create_asym_session(vcrypto,
> > &ctrl.u.akcipher_create_session,
> > queue_id, opcode,
> > - out_iov, out_num);
> > -
> > -check_session:
> > - /* Serious errors, need to reset virtio crypto device */
> > - if (session_id == -EFAULT) {
> > - virtqueue_detach_element(vq, elem, 0);
> > - break;
> > - } else if (session_id == -VIRTIO_CRYPTO_NOTSUPP) {
> > - stl_le_p(&input.status, VIRTIO_CRYPTO_NOTSUPP);
> > - } else if (session_id == -VIRTIO_CRYPTO_ERR) {
> > - stl_le_p(&input.status, VIRTIO_CRYPTO_ERR);
> > - } else {
> > - /* Set the session id */
> > - stq_le_p(&input.session_id, session_id);
> > - stl_le_p(&input.status, VIRTIO_CRYPTO_OK);
> > - }
> > -
> > - s = iov_from_buf(in_iov, in_num, 0, &input, sizeof(input));
> > - if (unlikely(s != sizeof(input))) {
> > - virtio_error(vdev, "virtio-crypto input incorrect");
> > - virtqueue_detach_element(vq, elem, 0);
> > - break;
> > + out_iov, out_num,
> > + sreq);
> > + if (ret < 0) {
> > + virtio_crypto_create_session_completion(sreq, ret);
> > }
> > - virtqueue_push(vq, elem, sizeof(input));
> > - virtio_notify(vdev, vq);
> > break;
> >
> > case VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION:
> > @@ -372,37 +420,36 @@ check_session:
> > case VIRTIO_CRYPTO_MAC_DESTROY_SESSION:
> > case VIRTIO_CRYPTO_AEAD_DESTROY_SESSION:
> > case VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION:
> > - status = virtio_crypto_handle_close_session(vcrypto,
> > - &ctrl.u.destroy_session, queue_id);
> > - /* The status only occupy one byte, we can directly use it */
> > - s = iov_from_buf(in_iov, in_num, 0, &status, sizeof(status));
> > - if (unlikely(s != sizeof(status))) {
> > - virtio_error(vdev, "virtio-crypto status incorrect");
> > - virtqueue_detach_element(vq, elem, 0);
> > - break;
> > + sreq->cb = virtio_crypto_destroy_session_completion;
> > + ret = virtio_crypto_handle_close_session(vcrypto,
> > + &ctrl.u.destroy_session, queue_id,
> > + sreq);
> > + if (ret < 0) {
> > + virtio_crypto_destroy_session_completion(sreq, ret);
> > }
> > - virtqueue_push(vq, elem, sizeof(status));
> > - virtio_notify(vdev, vq);
> > break;
> > +
> > case VIRTIO_CRYPTO_HASH_CREATE_SESSION:
> > case VIRTIO_CRYPTO_MAC_CREATE_SESSION:
> > case VIRTIO_CRYPTO_AEAD_CREATE_SESSION:
> > default:
> > + memset(&input, 0, sizeof(input));
> > error_report("virtio-crypto unsupported ctrl opcode: %d", opcode);
> > stl_le_p(&input.status, VIRTIO_CRYPTO_NOTSUPP);
> > s = iov_from_buf(in_iov, in_num, 0, &input, sizeof(input));
> > if (unlikely(s != sizeof(input))) {
> > virtio_error(vdev, "virtio-crypto input incorrect");
> > virtqueue_detach_element(vq, elem, 0);
> > - break;
> > + } else {
> > + virtqueue_push(vq, elem, sizeof(input));
> > + virtio_notify(vdev, vq);
> > }
> > - virtqueue_push(vq, elem, sizeof(input));
> > - virtio_notify(vdev, vq);
> > + g_free(sreq);
> > + g_free(elem);
> >
> > break;
> > } /* end switch case */
> >
> > - g_free(elem);
> > } /* end for loop */
> > }
> >
> > @@ -513,11 +560,13 @@ virtio_crypto_akcipher_input_data_helper(VirtIODevice *vdev,
> > req->in_len = sizeof(struct virtio_crypto_inhdr) + asym_op_info->dst_len;
> > }
> >
> > -
> > -static void virtio_crypto_req_complete(VirtIOCryptoReq *req, uint8_t status)
> > +static void virtio_crypto_req_complete(void *opaque, int ret)
> > {
> > + VirtIOCryptoReq *req = (VirtIOCryptoReq *)opaque;
> > VirtIOCrypto *vcrypto = req->vcrypto;
> > VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > + uint8_t status = -ret;
> > + g_autofree struct iovec *in_iov_copy = req->in_iov;
> >
> > if (req->flags == CRYPTODEV_BACKEND_ALG_SYM) {
> > virtio_crypto_sym_input_data_helper(vdev, req, status,
>
> I am not sure why is this g_autofree here.
> The variable is unused and clang gets confused (a bug in
> clang but oh well).
> Is the point to free when this goes out of scope?
> I suspect we should just g_free inside virtio_crypto_free_request
> or something like this.
> Pls send a fixup as otherwise I'll have to defer this until after
> the coming release.
Also to me it looks like not all paths call virtio_crypto_req_complete
so there might be a memory leak here.
Generally using g_autofree for something not allocated in the same
function is a hack.
I think I'll drop this patchset unless we can fix this quickly.
>
>
> > @@ -529,6 +578,7 @@ static void virtio_crypto_req_complete(VirtIOCryptoReq *req, uint8_t status)
> > stb_p(&req->in->status, status);
> > virtqueue_push(req->vq, &req->elem, req->in_len);
> > virtio_notify(vdev, req->vq);
> > + virtio_crypto_free_request(req);
> > }
> >
> > static VirtIOCryptoReq *
> > @@ -773,9 +823,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
> > unsigned in_num;
> > unsigned out_num;
> > uint32_t opcode;
> > - uint8_t status = VIRTIO_CRYPTO_ERR;
> > CryptoDevBackendOpInfo *op_info = &request->op_info;
> > - Error *local_err = NULL;
> >
> > if (elem->out_num < 1 || elem->in_num < 1) {
> > virtio_error(vdev, "virtio-crypto dataq missing headers");
> > @@ -815,6 +863,8 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
> > */
> > request->in_num = in_num;
> > request->in_iov = in_iov;
> > + /* now, we free the in_iov_copy inside virtio_crypto_req_complete */
> > + in_iov_copy = NULL;
> >
> > opcode = ldl_le_p(&req.header.opcode);
> > op_info->session_id = ldq_le_p(&req.header.session_id);
> > @@ -844,22 +894,11 @@ check_result:
> > return -1;
> > } else if (ret == -VIRTIO_CRYPTO_NOTSUPP) {
> > virtio_crypto_req_complete(request, VIRTIO_CRYPTO_NOTSUPP);
> > - virtio_crypto_free_request(request);
> > } else {
> > -
> > - /* Set request's parameter */
> > - ret = cryptodev_backend_crypto_operation(vcrypto->cryptodev,
> > - request, queue_index, &local_err);
> > - if (ret < 0) {
> > - status = -ret;
> > - if (local_err) {
> > - error_report_err(local_err);
> > - }
> > - } else { /* ret == VIRTIO_CRYPTO_OK */
> > - status = ret;
> > - }
> > - virtio_crypto_req_complete(request, status);
> > - virtio_crypto_free_request(request);
> > + cryptodev_backend_crypto_operation(vcrypto->cryptodev,
> > + request, queue_index,
> > + virtio_crypto_req_complete,
> > + request);
> > }
> > break;
> >
> > @@ -871,7 +910,6 @@ check_result:
> > error_report("virtio-crypto unsupported dataq opcode: %u",
> > opcode);
> > virtio_crypto_req_complete(request, VIRTIO_CRYPTO_NOTSUPP);
> > - virtio_crypto_free_request(request);
> > }
> >
> > return 0;
> > @@ -1011,7 +1049,7 @@ static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
> > vcrypto->vqs[i].vcrypto = vcrypto;
> > }
> >
> > - vcrypto->ctrl_vq = virtio_add_queue(vdev, 64, virtio_crypto_handle_ctrl);
> > + vcrypto->ctrl_vq = virtio_add_queue(vdev, 1024, virtio_crypto_handle_ctrl);
> > if (!cryptodev_backend_is_ready(vcrypto->cryptodev)) {
> > vcrypto->status &= ~VIRTIO_CRYPTO_S_HW_READY;
> > } else {
> > diff --git a/include/sysemu/cryptodev.h b/include/sysemu/cryptodev.h
> > index 37c3a360fd..32e9f4cf8a 100644
> > --- a/include/sysemu/cryptodev.h
> > +++ b/include/sysemu/cryptodev.h
> > @@ -113,6 +113,7 @@ typedef struct CryptoDevBackendSessionInfo {
> > CryptoDevBackendSymSessionInfo sym_sess_info;
> > CryptoDevBackendAsymSessionInfo asym_sess_info;
> > } u;
> > + uint64_t session_id;
> > } CryptoDevBackendSessionInfo;
> >
> > /**
> > @@ -188,21 +189,30 @@ typedef struct CryptoDevBackendOpInfo {
> > } u;
> > } CryptoDevBackendOpInfo;
> >
> > +typedef void (*CryptoDevCompletionFunc) (void *opaque, int ret);
> > struct CryptoDevBackendClass {
> > ObjectClass parent_class;
> >
> > void (*init)(CryptoDevBackend *backend, Error **errp);
> > void (*cleanup)(CryptoDevBackend *backend, Error **errp);
> >
> > - int64_t (*create_session)(CryptoDevBackend *backend,
> > - CryptoDevBackendSessionInfo *sess_info,
> > - uint32_t queue_index, Error **errp);
> > + int (*create_session)(CryptoDevBackend *backend,
> > + CryptoDevBackendSessionInfo *sess_info,
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque);
> > +
> > int (*close_session)(CryptoDevBackend *backend,
> > - uint64_t session_id,
> > - uint32_t queue_index, Error **errp);
> > + uint64_t session_id,
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque);
> > +
> > int (*do_op)(CryptoDevBackend *backend,
> > - CryptoDevBackendOpInfo *op_info,
> > - uint32_t queue_index, Error **errp);
> > + CryptoDevBackendOpInfo *op_info,
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque);
> > };
> >
> > typedef enum CryptoDevBackendOptionsType {
> > @@ -303,15 +313,20 @@ void cryptodev_backend_cleanup(
> > * @sess_info: parameters needed by session creating
> > * @queue_index: queue index of cryptodev backend client
> > * @errp: pointer to a NULL-initialized error object
> > + * @cb: callback when session create is compeleted
> > + * @opaque: parameter passed to callback
> > *
> > - * Create a session for symmetric/symmetric algorithms
> > + * Create a session for symmetric/asymmetric algorithms
> > *
> > - * Returns: session id on success, or -1 on error
> > + * Returns: 0 for success and cb will be called when creation is completed,
> > + * negative value for error, and cb will not be called.
> > */
> > -int64_t cryptodev_backend_create_session(
> > +int cryptodev_backend_create_session(
> > CryptoDevBackend *backend,
> > CryptoDevBackendSessionInfo *sess_info,
> > - uint32_t queue_index, Error **errp);
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque);
> >
> > /**
> > * cryptodev_backend_close_session:
> > @@ -319,34 +334,43 @@ int64_t cryptodev_backend_create_session(
> > * @session_id: the session id
> > * @queue_index: queue index of cryptodev backend client
> > * @errp: pointer to a NULL-initialized error object
> > + * @cb: callback when session create is compeleted
> > + * @opaque: parameter passed to callback
> > *
> > * Close a session for which was previously
> > * created by cryptodev_backend_create_session()
> > *
> > - * Returns: 0 on success, or Negative on error
> > + * Returns: 0 for success and cb will be called when creation is completed,
> > + * negative value for error, and cb will not be called.
> > */
> > int cryptodev_backend_close_session(
> > CryptoDevBackend *backend,
> > uint64_t session_id,
> > - uint32_t queue_index, Error **errp);
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque);
> >
> > /**
> > * cryptodev_backend_crypto_operation:
> > * @backend: the cryptodev backend object
> > - * @opaque: pointer to a VirtIOCryptoReq object
> > + * @opaque1: pointer to a VirtIOCryptoReq object
> > * @queue_index: queue index of cryptodev backend client
> > * @errp: pointer to a NULL-initialized error object
> > + * @cb: callbacks when operation is completed
> > + * @opaque2: parameter passed to cb
> > *
> > * Do crypto operation, such as encryption and
> > * decryption
> > *
> > - * Returns: VIRTIO_CRYPTO_OK on success,
> > - * or -VIRTIO_CRYPTO_* on error
> > + * Returns: 0 for success and cb will be called when creation is completed,
> > + * negative value for error, and cb will not be called.
> > */
> > int cryptodev_backend_crypto_operation(
> > CryptoDevBackend *backend,
> > - void *opaque,
> > - uint32_t queue_index, Error **errp);
> > + void *opaque1,
> > + uint32_t queue_index,
> > + CryptoDevCompletionFunc cb,
> > + void *opaque2);
> >
> > /**
> > * cryptodev_backend_set_used:
> > --
> > 2.11.0
next prev parent reply other threads:[~2022-11-01 19:52 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-08 8:50 [PATCH v2 0/4] Add a new backend for cryptodev Lei He
2022-10-08 8:50 ` [PATCH v2 1/4] virtio-crypto: Support asynchronous mode Lei He
2022-11-01 10:37 ` Michael S. Tsirkin
2022-11-01 19:51 ` Michael S. Tsirkin [this message]
2022-11-02 2:24 ` Lei He
2022-11-02 8:18 ` Lei He
2022-10-08 8:50 ` [PATCH v2 2/4] crypto: Support DER encodings Lei He
2022-10-11 9:31 ` Daniel P. Berrangé
2022-10-08 8:50 ` [PATCH v2 3/4] crypto: Support export akcipher to pkcs8 Lei He
2022-10-11 9:33 ` Daniel P. Berrangé
2022-10-08 8:50 ` [PATCH v2 4/4] cryptodev: Add a lkcf-backend for cryptodev Lei He
2022-10-24 8:55 ` [PATCH v2 0/4] Add a new backend " Lei He
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221101154944-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=arei.gonglei@huawei.com \
--cc=berrange@redhat.com \
--cc=helei.sig11@bytedance.com \
--cc=pizhenwei@bytedance.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).