zhenwei pi
2022-Apr-22 09:08 UTC
[PATCH v3 1/5] virtio-crypto: use private buffer for control request
On 4/22/22 15:41, Jason Wang wrote:> > ? 2022/4/21 18:40, zhenwei pi ??: >> Originally, all of the control requests share a single buffer( >> ctrl & input & ctrl_status fields in struct virtio_crypto), this >> allows queue depth 1 only, the performance of control queue gets >> limited by this design. >> >> In this patch, each request allocates request buffer dynamically, and >> free buffer after request, it's possible to optimize control queue >> depth in the next step. >> >> A necessary comment is already in code, still describe it again: >> /* >> ? * Note: there are padding fields in request, clear them to zero before >> ? * sending to host, >> ? * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48] >> ? */ >> So use kzalloc to allocate buffer of struct virtio_crypto_ctrl_request. >> >> Cc: Michael S. Tsirkin <mst at redhat.com> >> Cc: Jason Wang <jasowang at redhat.com> >> Cc: Gonglei <arei.gonglei at huawei.com> >> Signed-off-by: zhenwei pi <pizhenwei at bytedance.com> >> --- >> ? drivers/crypto/virtio/Makefile??????????????? |?? 1 + >> ? .../virtio/virtio_crypto_akcipher_algs.c????? |? 90 ++++++------ >> ? drivers/crypto/virtio/virtio_crypto_common.c? |? 39 +++++ > > > Any reason we can't use virtio_crypto_core.c? >Another patch in this series: [PATCH v3 3/5] virtio-crypto: move helpers into virtio_crypto_common.c Move virtcrypto_clear_request and virtcrypto_dataq_callback into virtio_crypto_common.c to make code clear. Then the xx_core.c supports: - probe/remove/irq affinity seting for a virtio device - basic virtio related operations xx_common.c supports: - common helpers/functions for algos So I put this into a new file.> >> ? drivers/crypto/virtio/virtio_crypto_common.h? |? 19 ++- >> ? .../virtio/virtio_crypto_skcipher_algs.c????? | 133 ++++++++---------- >> ? 5 files changed, 156 insertions(+), 126 deletions(-) >> ? create mode 100644 drivers/crypto/virtio/virtio_crypto_common.c >> >> diff --git a/drivers/crypto/virtio/Makefile >> b/drivers/crypto/virtio/Makefile >> index bfa6cbae342e..49c1fa80e465 100644 >> --- a/drivers/crypto/virtio/Makefile >> +++ b/drivers/crypto/virtio/Makefile >> @@ -3,5 +3,6 @@ obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio_crypto.o >> ? virtio_crypto-objs := \ >> ????? virtio_crypto_skcipher_algs.o \ >> ????? virtio_crypto_akcipher_algs.o \ >> +??? virtio_crypto_common.o \ >> ????? virtio_crypto_mgr.o \ >> ????? virtio_crypto_core.o >> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c >> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c >> index f3ec9420215e..9561bc2df62b 100644 >> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c >> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c >> @@ -102,8 +102,8 @@ static int >> virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher >> ? { >> ????? struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3]; >> ????? struct virtio_crypto *vcrypto = ctx->vcrypto; >> +??? struct virtio_crypto_ctrl_request *vc_ctrl_req; >> ????? uint8_t *pkey; >> -??? unsigned int inlen; >> ????? int err; >> ????? unsigned int num_out = 0, num_in = 0; >> @@ -111,98 +111,91 @@ static int >> virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher >> ????? if (!pkey) >> ????????? return -ENOMEM; >> -??? spin_lock(&vcrypto->ctrl_lock); >> -??? memcpy(&vcrypto->ctrl.header, header, sizeof(vcrypto->ctrl.header)); >> -??? memcpy(&vcrypto->ctrl.u, para, sizeof(vcrypto->ctrl.u)); >> -??? vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR); >> +??? vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL); >> +??? if (!vc_ctrl_req) { >> +??????? err = -ENOMEM; >> +??????? goto out; >> +??? } >> -??? sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl)); >> +??? memcpy(&vc_ctrl_req->ctrl.header, header, >> sizeof(vc_ctrl_req->ctrl.header)); >> +??? memcpy(&vc_ctrl_req->ctrl.u, para, sizeof(vc_ctrl_req->ctrl.u)); >> +??? sg_init_one(&outhdr_sg, &vc_ctrl_req->ctrl, >> sizeof(vc_ctrl_req->ctrl)); >> ????? sgs[num_out++] = &outhdr_sg; >> ????? sg_init_one(&key_sg, pkey, keylen); >> ????? sgs[num_out++] = &key_sg; >> -??? sg_init_one(&inhdr_sg, &vcrypto->input, sizeof(vcrypto->input)); >> +??? vc_ctrl_req->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR); > > > Nit: if there's no special reason, let's move this after the above > memcpys as what's done previously. > > >> +??? sg_init_one(&inhdr_sg, &vc_ctrl_req->input, >> sizeof(vc_ctrl_req->input)); >> ????? sgs[num_out + num_in++] = &inhdr_sg; >> -??? err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, >> vcrypto, GFP_ATOMIC); >> +??? err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, >> num_in, vc_ctrl_req); > > > I'd split this into a separate patch. >OK!> >> ????? if (err < 0) >> ????????? goto out; >> -??? virtqueue_kick(vcrypto->ctrl_vq); >> -??? while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) && >> -?????????? !virtqueue_is_broken(vcrypto->ctrl_vq)) >> -??????? cpu_relax(); >> - >> -??? if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) { >> +??? if (le32_to_cpu(vc_ctrl_req->input.status) != VIRTIO_CRYPTO_OK) { >> +??????? pr_err("virtio_crypto: Create session failed status: %u\n", >> +??????????? le32_to_cpu(vc_ctrl_req->input.status)); >> ????????? err = -EINVAL; >> ????????? goto out; >> ????? } > > > Do we need a warning for -ENOMEM? >Memory(especially small size) allocation is unlikely case, I also check the virtio_net and virtio_blk, both handles -ENOMEM only without error reporting.> >> -??? ctx->session_id = le64_to_cpu(vcrypto->input.session_id); >> +??? ctx->session_id = le64_to_cpu(vc_ctrl_req->input.session_id); >> ????? ctx->session_valid = true; >> ????? err = 0; >> ? out: >> -??? spin_unlock(&vcrypto->ctrl_lock); >> +??? kfree(vc_ctrl_req); >> ????? kfree_sensitive(pkey); >> -??? if (err < 0) >> -??????? pr_err("virtio_crypto: Create session failed status: %u\n", >> -??????????? le32_to_cpu(vcrypto->input.status)); >> - >> ????? return err; >> ? } >> ? static int virtio_crypto_alg_akcipher_close_session(struct >> virtio_crypto_akcipher_ctx *ctx) >> ? { >> ????? struct scatterlist outhdr_sg, inhdr_sg, *sgs[2]; >> +??? struct virtio_crypto_ctrl_request *vc_ctrl_req; >> ????? struct virtio_crypto_destroy_session_req *destroy_session; >> ????? struct virtio_crypto *vcrypto = ctx->vcrypto; >> -??? unsigned int num_out = 0, num_in = 0, inlen; >> +??? unsigned int num_out = 0, num_in = 0; >> ????? int err; >> -??? spin_lock(&vcrypto->ctrl_lock); >> -??? if (!ctx->session_valid) { >> -??????? err = 0; >> -??????? goto out; >> -??? } >> -??? vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR; >> -??? vcrypto->ctrl.header.opcode = >> cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION); >> -??? vcrypto->ctrl.header.queue_id = 0; >> +??? if (!ctx->session_valid) >> +??????? return 0; >> -??? destroy_session = &vcrypto->ctrl.u.destroy_session; >> +??? vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL); >> +??? if (!vc_ctrl_req) >> +??????? return -ENOMEM; >> + >> +??? vc_ctrl_req->ctrl.header.opcode = >> cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION); >> +??? vc_ctrl_req->ctrl.header.queue_id = 0; >> + >> +??? destroy_session = &vc_ctrl_req->ctrl.u.destroy_session; >> ????? destroy_session->session_id = cpu_to_le64(ctx->session_id); >> -??? sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl)); >> +??? sg_init_one(&outhdr_sg, &vc_ctrl_req->ctrl, >> sizeof(vc_ctrl_req->ctrl)); >> ????? sgs[num_out++] = &outhdr_sg; >> -??? sg_init_one(&inhdr_sg, &vcrypto->ctrl_status.status, >> sizeof(vcrypto->ctrl_status.status)); >> +??? vc_ctrl_req->ctrl_status.status = VIRTIO_CRYPTO_ERR; > > > If no special reason, let's move this above: > > vc_ctrl_req->ctrl.header.opcode = > cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION); >OK!> >> +??? sg_init_one(&inhdr_sg, &vc_ctrl_req->ctrl_status.status, >> +??????? sizeof(vc_ctrl_req->ctrl_status.status)); >> ????? sgs[num_out + num_in++] = &inhdr_sg; >> -??? err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, >> vcrypto, GFP_ATOMIC); >> +??? err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, >> num_in, vc_ctrl_req); >> ????? if (err < 0) >> ????????? goto out; >> -??? virtqueue_kick(vcrypto->ctrl_vq); >> -??? while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) && >> -?????????? !virtqueue_is_broken(vcrypto->ctrl_vq)) >> -??????? cpu_relax(); >> - >> -??? if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) { >> +??? if (vc_ctrl_req->ctrl_status.status != VIRTIO_CRYPTO_OK) { >> ????????? err = -EINVAL; >> +??????? pr_err("virtio_crypto: Close session failed status: %u, >> session_id: 0x%llx\n", >> +??????????? vc_ctrl_req->ctrl_status.status, >> destroy_session->session_id); >> ????????? goto out; >> ????? } >> ????? err = 0; >> ????? ctx->session_valid = false; >> - >> ? out: >> -??? spin_unlock(&vcrypto->ctrl_lock); >> -??? if (err < 0) { >> -??????? pr_err("virtio_crypto: Close session failed status: %u, >> session_id: 0x%llx\n", >> -??????????? vcrypto->ctrl_status.status, destroy_session->session_id); >> -??? } >> +??? kfree(vc_ctrl_req); >> ????? return err; >> ? } >> @@ -210,14 +203,11 @@ static int >> virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe >> ? static int __virtio_crypto_akcipher_do_req(struct >> virtio_crypto_akcipher_request *vc_akcipher_req, >> ????????? struct akcipher_request *req, struct data_queue *data_vq) >> ? { >> -??? struct virtio_crypto_akcipher_ctx *ctx = >> vc_akcipher_req->akcipher_ctx; >> ????? struct virtio_crypto_request *vc_req = &vc_akcipher_req->base; >> -??? struct virtio_crypto *vcrypto = ctx->vcrypto; >> ????? struct virtio_crypto_op_data_req *req_data = vc_req->req_data; >> ????? struct scatterlist *sgs[4], outhdr_sg, inhdr_sg, srcdata_sg, >> dstdata_sg; >> ????? void *src_buf = NULL, *dst_buf = NULL; >> ????? unsigned int num_out = 0, num_in = 0; >> -??? int node = dev_to_node(&vcrypto->vdev->dev); >> ????? unsigned long flags; >> ????? int ret = -ENOMEM; >> ????? bool verify = vc_akcipher_req->opcode == >> VIRTIO_CRYPTO_AKCIPHER_VERIFY; >> @@ -228,7 +218,7 @@ static int __virtio_crypto_akcipher_do_req(struct >> virtio_crypto_akcipher_request >> ????? sgs[num_out++] = &outhdr_sg; >> ????? /* src data */ >> -??? src_buf = kcalloc_node(src_len, 1, GFP_KERNEL, node); >> +??? src_buf = kcalloc(src_len, 1, GFP_KERNEL); > > > This seems not a relevant change. If it is a must we need use a separate > for this and describe the rationale. > > >> ????? if (!src_buf) >> ????????? goto err; >> @@ -243,7 +233,7 @@ static int __virtio_crypto_akcipher_do_req(struct >> virtio_crypto_akcipher_request >> ????????? sgs[num_out++] = &srcdata_sg; >> ????????? /* dst data */ >> -??????? dst_buf = kcalloc_node(req->dst_len, 1, GFP_KERNEL, node); >> +??????? dst_buf = kcalloc(req->dst_len, 1, GFP_KERNEL); > > > And this. > > >> ????????? if (!dst_buf) >> ????????????? goto err; >> diff --git a/drivers/crypto/virtio/virtio_crypto_common.c >> b/drivers/crypto/virtio/virtio_crypto_common.c >> new file mode 100644 >> index 000000000000..e65125a74db2 >> --- /dev/null >> +++ b/drivers/crypto/virtio/virtio_crypto_common.c >> @@ -0,0 +1,39 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* Common functions and helpers >> + * >> + * Authors: zhenwei pi <pizhenwei at bytedance.com> >> + * >> + * Copyright 2022 Bytedance CO., LTD. >> + */ >> + >> +#include "virtio_crypto_common.h" >> + >> +int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, >> struct scatterlist *sgs[], >> +????????????????? unsigned int out_sgs, unsigned int in_sgs, >> +????????????????? struct virtio_crypto_ctrl_request *vc_ctrl_req) >> +{ >> +??? int err; >> +??? unsigned int inlen; >> +??? unsigned long flags; >> + >> +??? spin_lock_irqsave(&vcrypto->ctrl_lock, flags); >> +??? err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, out_sgs, in_sgs, >> vc_ctrl_req, GFP_ATOMIC); >> +??? if (err < 0) { >> +??????? spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags); >> +??????? return err; >> +??? } >> + >> +??? virtqueue_kick(vcrypto->ctrl_vq); >> + >> +??? /* >> +???? * Trapping into the hypervisor, so the request should be >> +???? * handled immediately. >> +???? */ >> +??? while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) && >> +??????? !virtqueue_is_broken(vcrypto->ctrl_vq)) >> +??????? cpu_relax(); >> + >> +??? spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags); >> + >> +??? return 0; >> +} >> diff --git a/drivers/crypto/virtio/virtio_crypto_common.h >> b/drivers/crypto/virtio/virtio_crypto_common.h >> index e693d4ee83a6..d2a20fe6e13e 100644 >> --- a/drivers/crypto/virtio/virtio_crypto_common.h >> +++ b/drivers/crypto/virtio/virtio_crypto_common.h >> @@ -13,6 +13,7 @@ >> ? #include <crypto/aead.h> >> ? #include <crypto/aes.h> >> ? #include <crypto/engine.h> >> +#include <uapi/linux/virtio_crypto.h> >> ? /* Internal representation of a data virtqueue */ >> @@ -65,11 +66,6 @@ struct virtio_crypto { >> ????? /* Maximum size of per request */ >> ????? u64 max_size; >> -??? /* Control VQ buffers: protected by the ctrl_lock */ >> -??? struct virtio_crypto_op_ctrl_req ctrl; >> -??? struct virtio_crypto_session_input input; >> -??? struct virtio_crypto_inhdr ctrl_status; >> - >> ????? unsigned long status; >> ????? atomic_t ref_count; >> ????? struct list_head list; >> @@ -85,6 +81,16 @@ struct virtio_crypto_sym_session_info { >> ????? __u64 session_id; >> ? }; >> +/* >> + * Note: there are padding fields in request, clear them to zero >> before sending to host, >> + * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48] >> + */ >> +struct virtio_crypto_ctrl_request { >> +??? struct virtio_crypto_op_ctrl_req ctrl; >> +??? struct virtio_crypto_session_input input; >> +??? struct virtio_crypto_inhdr ctrl_status; >> +}; >> + >> ? struct virtio_crypto_request; >> ? typedef void (*virtio_crypto_data_callback) >> ????????? (struct virtio_crypto_request *vc_req, int len); >> @@ -135,4 +141,7 @@ void virtio_crypto_skcipher_algs_unregister(struct >> virtio_crypto *vcrypto); >> ? int virtio_crypto_akcipher_algs_register(struct virtio_crypto >> *vcrypto); >> ? void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto >> *vcrypto); >> +int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, >> struct scatterlist *sgs[], >> +????????????????? unsigned int out_sgs, unsigned int in_sgs, >> +????????????????? struct virtio_crypto_ctrl_request *vc_ctrl_req); >> ? #endif /* _VIRTIO_CRYPTO_COMMON_H */ >> diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c >> b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c >> index a618c46a52b8..fef355ff461c 100644 >> --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c >> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c >> @@ -118,11 +118,13 @@ static int virtio_crypto_alg_skcipher_init_session( >> ????????? int encrypt) >> ? { >> ????? struct scatterlist outhdr, key_sg, inhdr, *sgs[3]; >> -??? unsigned int tmp; >> ????? struct virtio_crypto *vcrypto = ctx->vcrypto; > > > Can we simply rename this to virtcrypto and then we can use the name > "vcrypto" for virtio_crypto_ctrl_request then we save tons lot changes? > > It simplify the life of reviewers and backporting. > > Thanks >This series focuses on performance improvment, and keeps the style with the orignal style. What about fixing this in another series? (If so, I'll fix this later)> >> ????? int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT : >> VIRTIO_CRYPTO_OP_DECRYPT; >> ????? int err; >> ????? unsigned int num_out = 0, num_in = 0; >> +??? struct virtio_crypto_ctrl_request *vc_ctrl_req; >> +??? struct virtio_crypto_ctrl_header *header; >> +??? struct virtio_crypto_sym_create_session_req *sym_create_session; >> ????? /* >> ?????? * Avoid to do DMA from the stack, switch to using >> @@ -133,26 +135,27 @@ static int virtio_crypto_alg_skcipher_init_session( >> ????? if (!cipher_key) >> ????????? return -ENOMEM; >> -??? spin_lock(&vcrypto->ctrl_lock); >> +??? vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL); >> +??? if (!vc_ctrl_req) { >> +??????? err = -ENOMEM; >> +??????? goto out; >> +??? } >> + >> ????? /* Pad ctrl header */ >> -??? vcrypto->ctrl.header.opcode >> -??????? cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION); >> -??? vcrypto->ctrl.header.algo = cpu_to_le32(alg); >> +??? header = &vc_ctrl_req->ctrl.header; >> +??? header->opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION); >> +??? header->algo = cpu_to_le32(alg); >> ????? /* Set the default dataqueue id to 0 */ >> -??? vcrypto->ctrl.header.queue_id = 0; >> +??? header->queue_id = 0; >> -??? vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR); >> ????? /* Pad cipher's parameters */ >> -??? vcrypto->ctrl.u.sym_create_session.op_type >> -??????? cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER); >> -??? vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo >> -??????? vcrypto->ctrl.header.algo; >> -??? vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen >> -??????? cpu_to_le32(keylen); >> -??? vcrypto->ctrl.u.sym_create_session.u.cipher.para.op >> -??????? cpu_to_le32(op); >> - >> -??? sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl)); >> +??? sym_create_session = &vc_ctrl_req->ctrl.u.sym_create_session; >> +??? sym_create_session->op_type = >> cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER); >> +??? sym_create_session->u.cipher.para.algo = header->algo; >> +??? sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen); >> +??? sym_create_session->u.cipher.para.op = cpu_to_le32(op); >> + >> +??? sg_init_one(&outhdr, &vc_ctrl_req->ctrl, sizeof(vc_ctrl_req->ctrl)); >> ????? sgs[num_out++] = &outhdr; >> ????? /* Set key */ >> @@ -160,45 +163,34 @@ static int virtio_crypto_alg_skcipher_init_session( >> ????? sgs[num_out++] = &key_sg; >> ????? /* Return status and session id back */ >> -??? sg_init_one(&inhdr, &vcrypto->input, sizeof(vcrypto->input)); >> +??? vc_ctrl_req->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR); >> +??? sg_init_one(&inhdr, &vc_ctrl_req->input, >> sizeof(vc_ctrl_req->input)); >> ????? sgs[num_out + num_in++] = &inhdr; >> -??? err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, >> -??????????????? num_in, vcrypto, GFP_ATOMIC); >> -??? if (err < 0) { >> -??????? spin_unlock(&vcrypto->ctrl_lock); >> -??????? kfree_sensitive(cipher_key); >> -??????? return err; >> -??? } >> -??? virtqueue_kick(vcrypto->ctrl_vq); >> - >> -??? /* >> -???? * Trapping into the hypervisor, so the request should be >> -???? * handled immediately. >> -???? */ >> -??? while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) && >> -?????????? !virtqueue_is_broken(vcrypto->ctrl_vq)) >> -??????? cpu_relax(); >> +??? err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, >> num_in, vc_ctrl_req); >> +??? if (err < 0) >> +??????? goto out; >> -??? if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) { >> -??????? spin_unlock(&vcrypto->ctrl_lock); >> +??? if (le32_to_cpu(vc_ctrl_req->input.status) != VIRTIO_CRYPTO_OK) { >> ????????? pr_err("virtio_crypto: Create session failed status: %u\n", >> -??????????? le32_to_cpu(vcrypto->input.status)); >> -??????? kfree_sensitive(cipher_key); >> -??????? return -EINVAL; >> +??????????? le32_to_cpu(vc_ctrl_req->input.status)); >> +??????? err = -EINVAL; >> +??????? goto out; >> ????? } >> ????? if (encrypt) >> ????????? ctx->enc_sess_info.session_id >> -??????????? le64_to_cpu(vcrypto->input.session_id); >> +??????????? le64_to_cpu(vc_ctrl_req->input.session_id); >> ????? else >> ????????? ctx->dec_sess_info.session_id >> -??????????? le64_to_cpu(vcrypto->input.session_id); >> - >> -??? spin_unlock(&vcrypto->ctrl_lock); >> +??????????? le64_to_cpu(vc_ctrl_req->input.session_id); >> +??? err = 0; >> +out: >> +??? kfree(vc_ctrl_req); >> ????? kfree_sensitive(cipher_key); >> -??? return 0; >> + >> +??? return err; >> ? } >> ? static int virtio_crypto_alg_skcipher_close_session( >> @@ -206,21 +198,24 @@ static int >> virtio_crypto_alg_skcipher_close_session( >> ????????? int encrypt) >> ? { >> ????? struct scatterlist outhdr, status_sg, *sgs[2]; >> -??? unsigned int tmp; >> ????? struct virtio_crypto_destroy_session_req *destroy_session; >> ????? struct virtio_crypto *vcrypto = ctx->vcrypto; >> ????? int err; >> ????? unsigned int num_out = 0, num_in = 0; >> +??? struct virtio_crypto_ctrl_request *vc_ctrl_req; >> +??? struct virtio_crypto_ctrl_header *header; >> + >> +??? vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL); >> +??? if (!vc_ctrl_req) >> +??????? return -ENOMEM; >> -??? spin_lock(&vcrypto->ctrl_lock); >> -??? vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR; >> ????? /* Pad ctrl header */ >> -??? vcrypto->ctrl.header.opcode >> -??????? cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION); >> +??? header = &vc_ctrl_req->ctrl.header; >> +??? header->opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION); >> ????? /* Set the default virtqueue id to 0 */ >> -??? vcrypto->ctrl.header.queue_id = 0; >> +??? header->queue_id = 0; >> -??? destroy_session = &vcrypto->ctrl.u.destroy_session; >> +??? destroy_session = &vc_ctrl_req->ctrl.u.destroy_session; >> ????? if (encrypt) >> ????????? destroy_session->session_id >> @@ -229,37 +224,33 @@ static int >> virtio_crypto_alg_skcipher_close_session( >> ????????? destroy_session->session_id >> ????????????? cpu_to_le64(ctx->dec_sess_info.session_id); >> -??? sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl)); >> +??? sg_init_one(&outhdr, &vc_ctrl_req->ctrl, sizeof(vc_ctrl_req->ctrl)); >> ????? sgs[num_out++] = &outhdr; >> ????? /* Return status and session id back */ >> -??? sg_init_one(&status_sg, &vcrypto->ctrl_status.status, >> -??????? sizeof(vcrypto->ctrl_status.status)); >> +??? vc_ctrl_req->ctrl_status.status = VIRTIO_CRYPTO_ERR; >> +??? sg_init_one(&status_sg, &vc_ctrl_req->ctrl_status.status, >> +??????? sizeof(vc_ctrl_req->ctrl_status.status)); >> ????? sgs[num_out + num_in++] = &status_sg; >> -??? err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, >> -??????????? num_in, vcrypto, GFP_ATOMIC); >> -??? if (err < 0) { >> -??????? spin_unlock(&vcrypto->ctrl_lock); >> -??????? return err; >> -??? } >> -??? virtqueue_kick(vcrypto->ctrl_vq); >> - >> -??? while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) && >> -?????????? !virtqueue_is_broken(vcrypto->ctrl_vq)) >> -??????? cpu_relax(); >> +??? err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, >> num_in, vc_ctrl_req); >> +??? if (err < 0) >> +??????? goto out; >> -??? if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) { >> -??????? spin_unlock(&vcrypto->ctrl_lock); >> +??? if (vc_ctrl_req->ctrl_status.status != VIRTIO_CRYPTO_OK) { >> ????????? pr_err("virtio_crypto: Close session failed status: %u, >> session_id: 0x%llx\n", >> -??????????? vcrypto->ctrl_status.status, >> +??????????? vc_ctrl_req->ctrl_status.status, >> ????????????? destroy_session->session_id); >> -??????? return -EINVAL; >> +??????? err = -EINVAL; >> +??????? goto out; >> ????? } >> -??? spin_unlock(&vcrypto->ctrl_lock); >> -??? return 0; >> +??? err = 0; >> +out: >> +??? kfree(vc_ctrl_req); >> + >> +??? return err; >> ? } >> ? static int virtio_crypto_alg_skcipher_init_sessions( >-- zhenwei pi
Jason Wang
2022-Apr-24 06:21 UTC
[PATCH v3 1/5] virtio-crypto: use private buffer for control request
On Fri, Apr 22, 2022 at 5:12 PM zhenwei pi <pizhenwei at bytedance.com> wrote:> > On 4/22/22 15:41, Jason Wang wrote: > > > > ? 2022/4/21 18:40, zhenwei pi ??: > >> Originally, all of the control requests share a single buffer( > >> ctrl & input & ctrl_status fields in struct virtio_crypto), this > >> allows queue depth 1 only, the performance of control queue gets > >> limited by this design. > >> > >> In this patch, each request allocates request buffer dynamically, and > >> free buffer after request, it's possible to optimize control queue > >> depth in the next step. > >> > >> A necessary comment is already in code, still describe it again: > >> /* > >> * Note: there are padding fields in request, clear them to zero before > >> * sending to host, > >> * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48] > >> */ > >> So use kzalloc to allocate buffer of struct virtio_crypto_ctrl_request. > >> > >> Cc: Michael S. Tsirkin <mst at redhat.com> > >> Cc: Jason Wang <jasowang at redhat.com> > >> Cc: Gonglei <arei.gonglei at huawei.com> > >> Signed-off-by: zhenwei pi <pizhenwei at bytedance.com> > >> --- > >> drivers/crypto/virtio/Makefile | 1 + > >> .../virtio/virtio_crypto_akcipher_algs.c | 90 ++++++------ > >> drivers/crypto/virtio/virtio_crypto_common.c | 39 +++++ > > > > > > Any reason we can't use virtio_crypto_core.c? > > > Another patch in this series: [PATCH v3 3/5] virtio-crypto: move helpers > into virtio_crypto_common.c > > Move virtcrypto_clear_request and virtcrypto_dataq_callback into > virtio_crypto_common.c to make code clear. Then the xx_core.c > supports: > - probe/remove/irq affinity seting for a virtio device > - basic virtio related operations > > xx_common.c supports: > - common helpers/functions for algos > > So I put this into a new file.I don't see obvious differences but we can leave it to the virtio-crypto maintainers to decide.> > > > >> drivers/crypto/virtio/virtio_crypto_common.h | 19 ++- > >> .../virtio/virtio_crypto_skcipher_algs.c | 133 ++++++++---------- > >> 5 files changed, 156 insertions(+), 126 deletions(-) > >> create mode 100644 drivers/crypto/virtio/virtio_crypto_common.c > >> > >> diff --git a/drivers/crypto/virtio/Makefile > >> b/drivers/crypto/virtio/Makefile > >> index bfa6cbae342e..49c1fa80e465 100644 > >> --- a/drivers/crypto/virtio/Makefile > >> +++ b/drivers/crypto/virtio/Makefile > >> @@ -3,5 +3,6 @@ obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio_crypto.o > >> virtio_crypto-objs := \ > >> virtio_crypto_skcipher_algs.o \ > >> virtio_crypto_akcipher_algs.o \ > >> + virtio_crypto_common.o \ > >> virtio_crypto_mgr.o \ > >> virtio_crypto_core.o > >> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c > >> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c > >> index f3ec9420215e..9561bc2df62b 100644 > >> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c > >> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c > >> @@ -102,8 +102,8 @@ static int > >> virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher > >> { > >> struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3]; > >> struct virtio_crypto *vcrypto = ctx->vcrypto; > >> + struct virtio_crypto_ctrl_request *vc_ctrl_req; > >> uint8_t *pkey; > >> - unsigned int inlen; > >> int err; > >> unsigned int num_out = 0, num_in = 0; > >> @@ -111,98 +111,91 @@ static int > >> virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher > >> if (!pkey) > >> return -ENOMEM; > >> - spin_lock(&vcrypto->ctrl_lock); > >> - memcpy(&vcrypto->ctrl.header, header, sizeof(vcrypto->ctrl.header)); > >> - memcpy(&vcrypto->ctrl.u, para, sizeof(vcrypto->ctrl.u)); > >> - vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR); > >> + vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL); > >> + if (!vc_ctrl_req) { > >> + err = -ENOMEM; > >> + goto out; > >> + } > >> - sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl)); > >> + memcpy(&vc_ctrl_req->ctrl.header, header, > >> sizeof(vc_ctrl_req->ctrl.header)); > >> + memcpy(&vc_ctrl_req->ctrl.u, para, sizeof(vc_ctrl_req->ctrl.u)); > >> + sg_init_one(&outhdr_sg, &vc_ctrl_req->ctrl, > >> sizeof(vc_ctrl_req->ctrl)); > >> sgs[num_out++] = &outhdr_sg; > >> sg_init_one(&key_sg, pkey, keylen); > >> sgs[num_out++] = &key_sg; > >> - sg_init_one(&inhdr_sg, &vcrypto->input, sizeof(vcrypto->input)); > >> + vc_ctrl_req->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR); > > > > > > Nit: if there's no special reason, let's move this after the above > > memcpys as what's done previously. > > > > > >> + sg_init_one(&inhdr_sg, &vc_ctrl_req->input, > >> sizeof(vc_ctrl_req->input)); > >> sgs[num_out + num_in++] = &inhdr_sg; > >> - err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, > >> vcrypto, GFP_ATOMIC); > >> + err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, > >> num_in, vc_ctrl_req); > > > > > > I'd split this into a separate patch. > > > > OK! > > > >> if (err < 0) > >> goto out; > >> - virtqueue_kick(vcrypto->ctrl_vq); > >> - while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) && > >> - !virtqueue_is_broken(vcrypto->ctrl_vq)) > >> - cpu_relax(); > >> - > >> - if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) { > >> + if (le32_to_cpu(vc_ctrl_req->input.status) != VIRTIO_CRYPTO_OK) { > >> + pr_err("virtio_crypto: Create session failed status: %u\n", > >> + le32_to_cpu(vc_ctrl_req->input.status)); > >> err = -EINVAL; > >> goto out; > >> } > > > > > > Do we need a warning for -ENOMEM? > > > > Memory(especially small size) allocation is unlikely case, I also check > the virtio_net and virtio_blk, both handles -ENOMEM only without error > reporting.Ok.> > > > >> - ctx->session_id = le64_to_cpu(vcrypto->input.session_id); > >> + ctx->session_id = le64_to_cpu(vc_ctrl_req->input.session_id); > >> ctx->session_valid = true; > >> err = 0; > >> out: > >> - spin_unlock(&vcrypto->ctrl_lock); > >> + kfree(vc_ctrl_req); > >> kfree_sensitive(pkey); > >> - if (err < 0) > >> - pr_err("virtio_crypto: Create session failed status: %u\n", > >> - le32_to_cpu(vcrypto->input.status)); > >> - > >> return err; > >> } > >> static int virtio_crypto_alg_akcipher_close_session(struct > >> virtio_crypto_akcipher_ctx *ctx) > >> { > >> struct scatterlist outhdr_sg, inhdr_sg, *sgs[2]; > >> + struct virtio_crypto_ctrl_request *vc_ctrl_req; > >> struct virtio_crypto_destroy_session_req *destroy_session; > >> struct virtio_crypto *vcrypto = ctx->vcrypto; > >> - unsigned int num_out = 0, num_in = 0, inlen; > >> + unsigned int num_out = 0, num_in = 0; > >> int err; > >> - spin_lock(&vcrypto->ctrl_lock); > >> - if (!ctx->session_valid) { > >> - err = 0; > >> - goto out; > >> - } > >> - vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR; > >> - vcrypto->ctrl.header.opcode > >> cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION); > >> - vcrypto->ctrl.header.queue_id = 0; > >> + if (!ctx->session_valid) > >> + return 0; > >> - destroy_session = &vcrypto->ctrl.u.destroy_session; > >> + vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL); > >> + if (!vc_ctrl_req) > >> + return -ENOMEM; > >> + > >> + vc_ctrl_req->ctrl.header.opcode > >> cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION); > >> + vc_ctrl_req->ctrl.header.queue_id = 0; > >> + > >> + destroy_session = &vc_ctrl_req->ctrl.u.destroy_session; > >> destroy_session->session_id = cpu_to_le64(ctx->session_id); > >> - sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl)); > >> + sg_init_one(&outhdr_sg, &vc_ctrl_req->ctrl, > >> sizeof(vc_ctrl_req->ctrl)); > >> sgs[num_out++] = &outhdr_sg; > >> - sg_init_one(&inhdr_sg, &vcrypto->ctrl_status.status, > >> sizeof(vcrypto->ctrl_status.status)); > >> + vc_ctrl_req->ctrl_status.status = VIRTIO_CRYPTO_ERR; > > > > > > If no special reason, let's move this above: > > > > vc_ctrl_req->ctrl.header.opcode > > cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION); > > > > OK! > > > >> + sg_init_one(&inhdr_sg, &vc_ctrl_req->ctrl_status.status, > >> + sizeof(vc_ctrl_req->ctrl_status.status)); > >> sgs[num_out + num_in++] = &inhdr_sg; > >> - err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, > >> vcrypto, GFP_ATOMIC); > >> + err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, > >> num_in, vc_ctrl_req); > >> if (err < 0) > >> goto out; > >> - virtqueue_kick(vcrypto->ctrl_vq); > >> - while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) && > >> - !virtqueue_is_broken(vcrypto->ctrl_vq)) > >> - cpu_relax(); > >> - > >> - if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) { > >> + if (vc_ctrl_req->ctrl_status.status != VIRTIO_CRYPTO_OK) { > >> err = -EINVAL; > >> + pr_err("virtio_crypto: Close session failed status: %u, > >> session_id: 0x%llx\n", > >> + vc_ctrl_req->ctrl_status.status, > >> destroy_session->session_id); > >> goto out; > >> } > >> err = 0; > >> ctx->session_valid = false; > >> - > >> out: > >> - spin_unlock(&vcrypto->ctrl_lock); > >> - if (err < 0) { > >> - pr_err("virtio_crypto: Close session failed status: %u, > >> session_id: 0x%llx\n", > >> - vcrypto->ctrl_status.status, destroy_session->session_id); > >> - } > >> + kfree(vc_ctrl_req); > >> return err; > >> } > >> @@ -210,14 +203,11 @@ static int > >> virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe > >> static int __virtio_crypto_akcipher_do_req(struct > >> virtio_crypto_akcipher_request *vc_akcipher_req, > >> struct akcipher_request *req, struct data_queue *data_vq) > >> { > >> - struct virtio_crypto_akcipher_ctx *ctx > >> vc_akcipher_req->akcipher_ctx; > >> struct virtio_crypto_request *vc_req = &vc_akcipher_req->base; > >> - struct virtio_crypto *vcrypto = ctx->vcrypto; > >> struct virtio_crypto_op_data_req *req_data = vc_req->req_data; > >> struct scatterlist *sgs[4], outhdr_sg, inhdr_sg, srcdata_sg, > >> dstdata_sg; > >> void *src_buf = NULL, *dst_buf = NULL; > >> unsigned int num_out = 0, num_in = 0; > >> - int node = dev_to_node(&vcrypto->vdev->dev); > >> unsigned long flags; > >> int ret = -ENOMEM; > >> bool verify = vc_akcipher_req->opcode => >> VIRTIO_CRYPTO_AKCIPHER_VERIFY; > >> @@ -228,7 +218,7 @@ static int __virtio_crypto_akcipher_do_req(struct > >> virtio_crypto_akcipher_request > >> sgs[num_out++] = &outhdr_sg; > >> /* src data */ > >> - src_buf = kcalloc_node(src_len, 1, GFP_KERNEL, node); > >> + src_buf = kcalloc(src_len, 1, GFP_KERNEL); > > > > > > This seems not a relevant change. If it is a must we need use a separate > > for this and describe the rationale. > > > > > >> if (!src_buf) > >> goto err; > >> @@ -243,7 +233,7 @@ static int __virtio_crypto_akcipher_do_req(struct > >> virtio_crypto_akcipher_request > >> sgs[num_out++] = &srcdata_sg; > >> /* dst data */ > >> - dst_buf = kcalloc_node(req->dst_len, 1, GFP_KERNEL, node); > >> + dst_buf = kcalloc(req->dst_len, 1, GFP_KERNEL); > > > > > > And this. > > > > > >> if (!dst_buf) > >> goto err; > >> diff --git a/drivers/crypto/virtio/virtio_crypto_common.c > >> b/drivers/crypto/virtio/virtio_crypto_common.c > >> new file mode 100644 > >> index 000000000000..e65125a74db2 > >> --- /dev/null > >> +++ b/drivers/crypto/virtio/virtio_crypto_common.c > >> @@ -0,0 +1,39 @@ > >> +// SPDX-License-Identifier: GPL-2.0-or-later > >> +/* Common functions and helpers > >> + * > >> + * Authors: zhenwei pi <pizhenwei at bytedance.com> > >> + * > >> + * Copyright 2022 Bytedance CO., LTD. > >> + */ > >> + > >> +#include "virtio_crypto_common.h" > >> + > >> +int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, > >> struct scatterlist *sgs[], > >> + unsigned int out_sgs, unsigned int in_sgs, > >> + struct virtio_crypto_ctrl_request *vc_ctrl_req) > >> +{ > >> + int err; > >> + unsigned int inlen; > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&vcrypto->ctrl_lock, flags); > >> + err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, out_sgs, in_sgs, > >> vc_ctrl_req, GFP_ATOMIC); > >> + if (err < 0) { > >> + spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags); > >> + return err; > >> + } > >> + > >> + virtqueue_kick(vcrypto->ctrl_vq); > >> + > >> + /* > >> + * Trapping into the hypervisor, so the request should be > >> + * handled immediately. > >> + */ > >> + while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) && > >> + !virtqueue_is_broken(vcrypto->ctrl_vq)) > >> + cpu_relax(); > >> + > >> + spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags); > >> + > >> + return 0; > >> +} > >> diff --git a/drivers/crypto/virtio/virtio_crypto_common.h > >> b/drivers/crypto/virtio/virtio_crypto_common.h > >> index e693d4ee83a6..d2a20fe6e13e 100644 > >> --- a/drivers/crypto/virtio/virtio_crypto_common.h > >> +++ b/drivers/crypto/virtio/virtio_crypto_common.h > >> @@ -13,6 +13,7 @@ > >> #include <crypto/aead.h> > >> #include <crypto/aes.h> > >> #include <crypto/engine.h> > >> +#include <uapi/linux/virtio_crypto.h> > >> /* Internal representation of a data virtqueue */ > >> @@ -65,11 +66,6 @@ struct virtio_crypto { > >> /* Maximum size of per request */ > >> u64 max_size; > >> - /* Control VQ buffers: protected by the ctrl_lock */ > >> - struct virtio_crypto_op_ctrl_req ctrl; > >> - struct virtio_crypto_session_input input; > >> - struct virtio_crypto_inhdr ctrl_status; > >> - > >> unsigned long status; > >> atomic_t ref_count; > >> struct list_head list; > >> @@ -85,6 +81,16 @@ struct virtio_crypto_sym_session_info { > >> __u64 session_id; > >> }; > >> +/* > >> + * Note: there are padding fields in request, clear them to zero > >> before sending to host, > >> + * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48] > >> + */ > >> +struct virtio_crypto_ctrl_request { > >> + struct virtio_crypto_op_ctrl_req ctrl; > >> + struct virtio_crypto_session_input input; > >> + struct virtio_crypto_inhdr ctrl_status; > >> +}; > >> + > >> struct virtio_crypto_request; > >> typedef void (*virtio_crypto_data_callback) > >> (struct virtio_crypto_request *vc_req, int len); > >> @@ -135,4 +141,7 @@ void virtio_crypto_skcipher_algs_unregister(struct > >> virtio_crypto *vcrypto); > >> int virtio_crypto_akcipher_algs_register(struct virtio_crypto > >> *vcrypto); > >> void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto > >> *vcrypto); > >> +int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, > >> struct scatterlist *sgs[], > >> + unsigned int out_sgs, unsigned int in_sgs, > >> + struct virtio_crypto_ctrl_request *vc_ctrl_req); > >> #endif /* _VIRTIO_CRYPTO_COMMON_H */ > >> diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c > >> b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c > >> index a618c46a52b8..fef355ff461c 100644 > >> --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c > >> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c > >> @@ -118,11 +118,13 @@ static int virtio_crypto_alg_skcipher_init_session( > >> int encrypt) > >> { > >> struct scatterlist outhdr, key_sg, inhdr, *sgs[3]; > >> - unsigned int tmp; > >> struct virtio_crypto *vcrypto = ctx->vcrypto; > > > > > > Can we simply rename this to virtcrypto and then we can use the name > > "vcrypto" for virtio_crypto_ctrl_request then we save tons lot changes? > > > > It simplify the life of reviewers and backporting. > > > > Thanks > > > > This series focuses on performance improvment, and keeps the style with > the orignal style. What about fixing this in another series? (If so, > I'll fix this later)Just in case we are at the same page, what I suggest can save hundreds lines of codes: E.g:> >> - vcrypto->ctrl.header.queue_id = 0; > >> + header->queue_id = 0;If we stick to the name of "vcrypto" we don't need this change at all. It can simplify: 1) future context difference when backporting patches to -stable 2) reviewing Thanks> > > >> int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT : > >> VIRTIO_CRYPTO_OP_DECRYPT; > >> int err; > >> unsigned int num_out = 0, num_in = 0; > >> + struct virtio_crypto_ctrl_request *vc_ctrl_req; > >> + struct virtio_crypto_ctrl_header *header; > >> + struct virtio_crypto_sym_create_session_req *sym_create_session; > >> /* > >> * Avoid to do DMA from the stack, switch to using > >> @@ -133,26 +135,27 @@ static int virtio_crypto_alg_skcipher_init_session( > >> if (!cipher_key) > >> return -ENOMEM; > >> - spin_lock(&vcrypto->ctrl_lock); > >> + vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL); > >> + if (!vc_ctrl_req) { > >> + err = -ENOMEM; > >> + goto out; > >> + } > >> + > >> /* Pad ctrl header */ > >> - vcrypto->ctrl.header.opcode > >> - cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION); > >> - vcrypto->ctrl.header.algo = cpu_to_le32(alg); > >> + header = &vc_ctrl_req->ctrl.header; > >> + header->opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION); > >> + header->algo = cpu_to_le32(alg); > >> /* Set the default dataqueue id to 0 */ > >> - vcrypto->ctrl.header.queue_id = 0; > >> + header->queue_id = 0; > >> - vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR); > >> /* Pad cipher's parameters */ > >> - vcrypto->ctrl.u.sym_create_session.op_type > >> - cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER); > >> - vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo > >> - vcrypto->ctrl.header.algo; > >> - vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen > >> - cpu_to_le32(keylen); > >> - vcrypto->ctrl.u.sym_create_session.u.cipher.para.op > >> - cpu_to_le32(op); > >> - > >> - sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl)); > >> + sym_create_session = &vc_ctrl_req->ctrl.u.sym_create_session; > >> + sym_create_session->op_type > >> cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER); > >> + sym_create_session->u.cipher.para.algo = header->algo; > >> + sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen); > >> + sym_create_session->u.cipher.para.op = cpu_to_le32(op); > >> + > >> + sg_init_one(&outhdr, &vc_ctrl_req->ctrl, sizeof(vc_ctrl_req->ctrl)); > >> sgs[num_out++] = &outhdr; > >> /* Set key */ > >> @@ -160,45 +163,34 @@ static int virtio_crypto_alg_skcipher_init_session( > >> sgs[num_out++] = &key_sg; > >> /* Return status and session id back */ > >> - sg_init_one(&inhdr, &vcrypto->input, sizeof(vcrypto->input)); > >> + vc_ctrl_req->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR); > >> + sg_init_one(&inhdr, &vc_ctrl_req->input, > >> sizeof(vc_ctrl_req->input)); > >> sgs[num_out + num_in++] = &inhdr; > >> - err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, > >> - num_in, vcrypto, GFP_ATOMIC); > >> - if (err < 0) { > >> - spin_unlock(&vcrypto->ctrl_lock); > >> - kfree_sensitive(cipher_key); > >> - return err; > >> - } > >> - virtqueue_kick(vcrypto->ctrl_vq); > >> - > >> - /* > >> - * Trapping into the hypervisor, so the request should be > >> - * handled immediately. > >> - */ > >> - while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) && > >> - !virtqueue_is_broken(vcrypto->ctrl_vq)) > >> - cpu_relax(); > >> + err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, > >> num_in, vc_ctrl_req); > >> + if (err < 0) > >> + goto out; > >> - if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) { > >> - spin_unlock(&vcrypto->ctrl_lock); > >> + if (le32_to_cpu(vc_ctrl_req->input.status) != VIRTIO_CRYPTO_OK) { > >> pr_err("virtio_crypto: Create session failed status: %u\n", > >> - le32_to_cpu(vcrypto->input.status)); > >> - kfree_sensitive(cipher_key); > >> - return -EINVAL; > >> + le32_to_cpu(vc_ctrl_req->input.status)); > >> + err = -EINVAL; > >> + goto out; > >> } > >> if (encrypt) > >> ctx->enc_sess_info.session_id > >> - le64_to_cpu(vcrypto->input.session_id); > >> + le64_to_cpu(vc_ctrl_req->input.session_id); > >> else > >> ctx->dec_sess_info.session_id > >> - le64_to_cpu(vcrypto->input.session_id); > >> - > >> - spin_unlock(&vcrypto->ctrl_lock); > >> + le64_to_cpu(vc_ctrl_req->input.session_id); > >> + err = 0; > >> +out: > >> + kfree(vc_ctrl_req); > >> kfree_sensitive(cipher_key); > >> - return 0; > >> + > >> + return err; > >> } > >> static int virtio_crypto_alg_skcipher_close_session( > >> @@ -206,21 +198,24 @@ static int > >> virtio_crypto_alg_skcipher_close_session( > >> int encrypt) > >> { > >> struct scatterlist outhdr, status_sg, *sgs[2]; > >> - unsigned int tmp; > >> struct virtio_crypto_destroy_session_req *destroy_session; > >> struct virtio_crypto *vcrypto = ctx->vcrypto; > >> int err; > >> unsigned int num_out = 0, num_in = 0; > >> + struct virtio_crypto_ctrl_request *vc_ctrl_req; > >> + struct virtio_crypto_ctrl_header *header; > >> + > >> + vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL); > >> + if (!vc_ctrl_req) > >> + return -ENOMEM; > >> - spin_lock(&vcrypto->ctrl_lock); > >> - vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR; > >> /* Pad ctrl header */ > >> - vcrypto->ctrl.header.opcode > >> - cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION); > >> + header = &vc_ctrl_req->ctrl.header; > >> + header->opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION); > >> /* Set the default virtqueue id to 0 */ > >> - vcrypto->ctrl.header.queue_id = 0; > >> + header->queue_id = 0; > >> - destroy_session = &vcrypto->ctrl.u.destroy_session; > >> + destroy_session = &vc_ctrl_req->ctrl.u.destroy_session; > >> if (encrypt) > >> destroy_session->session_id > >> @@ -229,37 +224,33 @@ static int > >> virtio_crypto_alg_skcipher_close_session( > >> destroy_session->session_id > >> cpu_to_le64(ctx->dec_sess_info.session_id); > >> - sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl)); > >> + sg_init_one(&outhdr, &vc_ctrl_req->ctrl, sizeof(vc_ctrl_req->ctrl)); > >> sgs[num_out++] = &outhdr; > >> /* Return status and session id back */ > >> - sg_init_one(&status_sg, &vcrypto->ctrl_status.status, > >> - sizeof(vcrypto->ctrl_status.status)); > >> + vc_ctrl_req->ctrl_status.status = VIRTIO_CRYPTO_ERR; > >> + sg_init_one(&status_sg, &vc_ctrl_req->ctrl_status.status, > >> + sizeof(vc_ctrl_req->ctrl_status.status)); > >> sgs[num_out + num_in++] = &status_sg; > >> - err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, > >> - num_in, vcrypto, GFP_ATOMIC); > >> - if (err < 0) { > >> - spin_unlock(&vcrypto->ctrl_lock); > >> - return err; > >> - } > >> - virtqueue_kick(vcrypto->ctrl_vq); > >> - > >> - while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) && > >> - !virtqueue_is_broken(vcrypto->ctrl_vq)) > >> - cpu_relax(); > >> + err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, > >> num_in, vc_ctrl_req); > >> + if (err < 0) > >> + goto out; > >> - if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) { > >> - spin_unlock(&vcrypto->ctrl_lock); > >> + if (vc_ctrl_req->ctrl_status.status != VIRTIO_CRYPTO_OK) { > >> pr_err("virtio_crypto: Close session failed status: %u, > >> session_id: 0x%llx\n", > >> - vcrypto->ctrl_status.status, > >> + vc_ctrl_req->ctrl_status.status, > >> destroy_session->session_id); > >> - return -EINVAL; > >> + err = -EINVAL; > >> + goto out; > >> } > >> - spin_unlock(&vcrypto->ctrl_lock); > >> - return 0; > >> + err = 0; > >> +out: > >> + kfree(vc_ctrl_req); > >> + > >> + return err; > >> } > >> static int virtio_crypto_alg_skcipher_init_sessions( > > > > -- > zhenwei pi >