[PATCH v3 0/5] virtio-crypto: Improve performance

[PATCH v3 0/5] virtio-crypto: Improve performance

[zhenwei pi]

v2 -> v3:
 - Jason suggested that spliting the first patch into two part:
     1, using private buffer
     2, remove the busy polling
   Rework as Jason's suggestion, this makes the smaller change in
   each one and clear.

v1 -> v2:
 - Use kfree instead of kfree_sensitive for insensitive buffer.
 - Several coding style fix.
 - Use memory from current node, instead of memory close to device
 - Add more message in commit, also explain why removing per-device
   request buffer.
 - Add necessary comment in code to explain why using kzalloc to
   allocate struct virtio_crypto_ctrl_request.

v1:
The main point of this series is to improve the performance for
virtio crypto:
- Use wait mechanism instead of busy polling for ctrl queue, this
  reduces CPU and lock racing, it's possiable to create/destroy session
  parallelly, QPS increases from ~40K/s to ~200K/s.
- Enable retry on crypto engine to improve performance for data queue,
  this allows the larger depth instead of 1.
- Fix dst data length in akcipher service.
- Other style fix.

lei he (2):
  virtio-crypto: adjust dst_len at ops callback
  virtio-crypto: enable retry for virtio-crypto-dev

zhenwei pi (3):
  virtio-crypto: use private buffer for control request
  virtio-crypto: wait ctrl queue instead of busy polling
  virtio-crypto: move helpers into virtio_crypto_common.c

 drivers/crypto/virtio/Makefile                |   1 +
 .../virtio/virtio_crypto_akcipher_algs.c      |  95 ++++++-------
 drivers/crypto/virtio/virtio_crypto_common.c  |  92 ++++++++++++
 drivers/crypto/virtio/virtio_crypto_common.h  |  29 +++-
 drivers/crypto/virtio/virtio_crypto_core.c    |  37 +----
 .../virtio/virtio_crypto_skcipher_algs.c      | 133 ++++++++----------
 6 files changed, 226 insertions(+), 161 deletions(-)
 create mode 100644 drivers/crypto/virtio/virtio_crypto_common.c

-- 
2.20.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 1/5] virtio-crypto: use private buffer for control request

[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@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/crypto/virtio/Makefile                |   1 +
 .../virtio/virtio_crypto_akcipher_algs.c      |  90 ++++++------
 drivers/crypto/virtio/virtio_crypto_common.c  |  39 +++++
 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);
+	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);
 	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;
 	}
 
-	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;
+	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);
 	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);
 		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@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;
 	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(
-- 
2.20.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 1/5] virtio-crypto: use private buffer for control request

[Jason Wang]

在 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@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: zhenwei pi <pizhenwei@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?


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


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


>   
> -	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);


> +	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@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


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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: Re: [PATCH v3 1/5] virtio-crypto: use private buffer for control request

[zhenwei pi]

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@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: zhenwei pi <pizhenwei@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@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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: Re: [PATCH v3 1/5] virtio-crypto: use private buffer for control request

[Jason Wang]

On Fri, Apr 22, 2022 at 5:12 PM zhenwei pi <pizhenwei@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@redhat.com>
> >> Cc: Jason Wang <jasowang@redhat.com>
> >> Cc: Gonglei <arei.gonglei@huawei.com>
> >> Signed-off-by: zhenwei pi <pizhenwei@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@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
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: Re: Re: [PATCH v3 1/5] virtio-crypto: use private buffer for control request

[zhenwei pi]

On 4/24/22 14:21, Jason Wang wrote:
> On Fri, Apr 22, 2022 at 5:12 PM zhenwei pi <pizhenwei@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@redhat.com>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> Cc: Gonglei <arei.gonglei@huawei.com>
>>>> Signed-off-by: zhenwei pi <pizhenwei@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.
> 

OK!
>>
>>>
>>>>    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@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
> 

Oh, sorry, I misunderstood this. I agree with this point, but I notice 
that 'vcrypto' is always a variable of 'type struct virtio_crypto *' in 
this driver, should we break this?

To simplify reviewing, I have another opinion in the v4 series, could 
you please review it?

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

-- 
zhenwei pi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 2/5] virtio-crypto: wait ctrl queue instead of busy polling

[zhenwei pi]

Originally, after submitting request into virtio crypto control
queue, the guest side polls the result from the virt queue. This
works like following:
    CPU0   CPU1               ...             CPUx  CPUy
     |      |                                  |     |
     \      \                                  /     /
      \--------spin_lock(&vcrypto->ctrl_lock)-------/
                           |
                 virtqueue add & kick
                           |
                  busy poll virtqueue
                           |
              spin_unlock(&vcrypto->ctrl_lock)
                          ...

There are two problems:
1, The queue depth is always 1, the performance of a virtio crypto
   device gets limited. Multi user processes share a single control
   queue, and hit spin lock race from control queue. Test on Intel
   Platinum 8260, a single worker gets ~35K/s create/close session
   operations, and 8 workers get ~40K/s operations with 800% CPU
   utilization.
2, The control request is supposed to get handled immediately, but
   in the current implementation of QEMU(v6.2), the vCPU thread kicks
   another thread to do this work, the latency also gets unstable.
   Tracking latency of virtio_crypto_alg_akcipher_close_session in 5s:
        usecs               : count     distribution
         0 -> 1          : 0        |                        |
         2 -> 3          : 7        |                        |
         4 -> 7          : 72       |                        |
         8 -> 15         : 186485   |************************|
        16 -> 31         : 687      |                        |
        32 -> 63         : 5        |                        |
        64 -> 127        : 3        |                        |
       128 -> 255        : 1        |                        |
       256 -> 511        : 0        |                        |
       512 -> 1023       : 0        |                        |
      1024 -> 2047       : 0        |                        |
      2048 -> 4095       : 0        |                        |
      4096 -> 8191       : 0        |                        |
      8192 -> 16383      : 2        |                        |
   This means that a CPU may hold vcrypto->ctrl_lock as long as 8192~16383us.

To improve the performance of control queue, a request on control queue waits
completion instead of busy polling to reduce lock racing, and gets completed by
control queue callback.
    CPU0   CPU1               ...             CPUx  CPUy
     |      |                                  |     |
     \      \                                  /     /
      \--------spin_lock(&vcrypto->ctrl_lock)-------/
                           |
                 virtqueue add & kick
                           |
      ---------spin_unlock(&vcrypto->ctrl_lock)------
     /      /                                  \     \
     |      |                                  |     |
    wait   wait                               wait  wait

Test this patch, the guest side get ~200K/s operations with 300% CPU
utilization.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/crypto/virtio/virtio_crypto_common.c | 42 +++++++++++++++-----
 drivers/crypto/virtio/virtio_crypto_common.h |  8 ++++
 drivers/crypto/virtio/virtio_crypto_core.c   |  2 +-
 3 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_common.c b/drivers/crypto/virtio/virtio_crypto_common.c
index e65125a74db2..93df73c40dd3 100644
--- a/drivers/crypto/virtio/virtio_crypto_common.c
+++ b/drivers/crypto/virtio/virtio_crypto_common.c
@@ -8,14 +8,21 @@
 
 #include "virtio_crypto_common.h"
 
+static void virtio_crypto_ctrlq_callback(struct virtio_crypto_ctrl_request *vc_ctrl_req)
+{
+	complete(&vc_ctrl_req->compl);
+}
+
 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;
 
+	init_completion(&vc_ctrl_req->compl);
+	vc_ctrl_req->ctrl_cb =  virtio_crypto_ctrlq_callback;
+
 	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) {
@@ -24,16 +31,31 @@ int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, struct scatterl
 	}
 
 	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);
 
+	wait_for_completion(&vc_ctrl_req->compl);
+
 	return 0;
 }
+
+void virtcrypto_ctrlq_callback(struct virtqueue *vq)
+{
+	struct virtio_crypto *vcrypto = vq->vdev->priv;
+	struct virtio_crypto_ctrl_request *vc_ctrl_req;
+	unsigned long flags;
+	unsigned int len;
+
+	spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
+	do {
+		virtqueue_disable_cb(vq);
+		while ((vc_ctrl_req = virtqueue_get_buf(vq, &len)) != NULL) {
+			spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
+			if (vc_ctrl_req->ctrl_cb)
+				vc_ctrl_req->ctrl_cb(vc_ctrl_req);
+			spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
+		}
+		if (unlikely(virtqueue_is_broken(vq)))
+			break;
+	} while (!virtqueue_enable_cb(vq));
+	spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
+}
diff --git a/drivers/crypto/virtio/virtio_crypto_common.h b/drivers/crypto/virtio/virtio_crypto_common.h
index d2a20fe6e13e..25b4f22e8605 100644
--- a/drivers/crypto/virtio/virtio_crypto_common.h
+++ b/drivers/crypto/virtio/virtio_crypto_common.h
@@ -81,6 +81,10 @@ struct virtio_crypto_sym_session_info {
 	__u64 session_id;
 };
 
+struct virtio_crypto_ctrl_request;
+typedef void (*virtio_crypto_ctrl_callback)
+		(struct virtio_crypto_ctrl_request *vc_ctrl_req);
+
 /*
  * 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]
@@ -89,6 +93,8 @@ struct virtio_crypto_ctrl_request {
 	struct virtio_crypto_op_ctrl_req ctrl;
 	struct virtio_crypto_session_input input;
 	struct virtio_crypto_inhdr ctrl_status;
+	virtio_crypto_ctrl_callback ctrl_cb;
+	struct completion compl;
 };
 
 struct virtio_crypto_request;
@@ -141,7 +147,9 @@ 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);
 
+void virtcrypto_ctrlq_callback(struct virtqueue *vq);
 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_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
index c6f482db0bc0..e668d4b1bc6a 100644
--- a/drivers/crypto/virtio/virtio_crypto_core.c
+++ b/drivers/crypto/virtio/virtio_crypto_core.c
@@ -73,7 +73,7 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
 		goto err_names;
 
 	/* Parameters for control virtqueue */
-	callbacks[total_vqs - 1] = NULL;
+	callbacks[total_vqs - 1] = virtcrypto_ctrlq_callback;
 	names[total_vqs - 1] = "controlq";
 
 	/* Allocate/initialize parameters for data virtqueues */
-- 
2.20.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/5] virtio-crypto: wait ctrl queue instead of busy polling

[Jason Wang]

在 2022/4/21 18:40, zhenwei pi 写道:
> Originally, after submitting request into virtio crypto control
> queue, the guest side polls the result from the virt queue. This
> works like following:
>      CPU0   CPU1               ...             CPUx  CPUy
>       |      |                                  |     |
>       \      \                                  /     /
>        \--------spin_lock(&vcrypto->ctrl_lock)-------/
>                             |
>                   virtqueue add & kick
>                             |
>                    busy poll virtqueue
>                             |
>                spin_unlock(&vcrypto->ctrl_lock)
>                            ...
>
> There are two problems:
> 1, The queue depth is always 1, the performance of a virtio crypto
>     device gets limited. Multi user processes share a single control
>     queue, and hit spin lock race from control queue. Test on Intel
>     Platinum 8260, a single worker gets ~35K/s create/close session
>     operations, and 8 workers get ~40K/s operations with 800% CPU
>     utilization.
> 2, The control request is supposed to get handled immediately, but
>     in the current implementation of QEMU(v6.2), the vCPU thread kicks
>     another thread to do this work, the latency also gets unstable.
>     Tracking latency of virtio_crypto_alg_akcipher_close_session in 5s:
>          usecs               : count     distribution
>           0 -> 1          : 0        |                        |
>           2 -> 3          : 7        |                        |
>           4 -> 7          : 72       |                        |
>           8 -> 15         : 186485   |************************|
>          16 -> 31         : 687      |                        |
>          32 -> 63         : 5        |                        |
>          64 -> 127        : 3        |                        |
>         128 -> 255        : 1        |                        |
>         256 -> 511        : 0        |                        |
>         512 -> 1023       : 0        |                        |
>        1024 -> 2047       : 0        |                        |
>        2048 -> 4095       : 0        |                        |
>        4096 -> 8191       : 0        |                        |
>        8192 -> 16383      : 2        |                        |
>     This means that a CPU may hold vcrypto->ctrl_lock as long as 8192~16383us.
>
> To improve the performance of control queue, a request on control queue waits
> completion instead of busy polling to reduce lock racing, and gets completed by
> control queue callback.
>      CPU0   CPU1               ...             CPUx  CPUy
>       |      |                                  |     |
>       \      \                                  /     /
>        \--------spin_lock(&vcrypto->ctrl_lock)-------/
>                             |
>                   virtqueue add & kick
>                             |
>        ---------spin_unlock(&vcrypto->ctrl_lock)------
>       /      /                                  \     \
>       |      |                                  |     |
>      wait   wait                               wait  wait
>
> Test this patch, the guest side get ~200K/s operations with 300% CPU
> utilization.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   drivers/crypto/virtio/virtio_crypto_common.c | 42 +++++++++++++++-----
>   drivers/crypto/virtio/virtio_crypto_common.h |  8 ++++
>   drivers/crypto/virtio/virtio_crypto_core.c   |  2 +-
>   3 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_common.c b/drivers/crypto/virtio/virtio_crypto_common.c
> index e65125a74db2..93df73c40dd3 100644
> --- a/drivers/crypto/virtio/virtio_crypto_common.c
> +++ b/drivers/crypto/virtio/virtio_crypto_common.c
> @@ -8,14 +8,21 @@
>   
>   #include "virtio_crypto_common.h"
>   
> +static void virtio_crypto_ctrlq_callback(struct virtio_crypto_ctrl_request *vc_ctrl_req)
> +{
> +	complete(&vc_ctrl_req->compl);
> +}
> +
>   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;
>   
> +	init_completion(&vc_ctrl_req->compl);
> +	vc_ctrl_req->ctrl_cb =  virtio_crypto_ctrlq_callback;


Is there a chance that the cb would not be virtio_crypto_ctrlq_callback()?


> +
>   	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) {
> @@ -24,16 +31,31 @@ int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, struct scatterl
>   	}
>   
>   	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);
>   
> +	wait_for_completion(&vc_ctrl_req->compl);
> +
>   	return 0;
>   }
> +
> +void virtcrypto_ctrlq_callback(struct virtqueue *vq)
> +{
> +	struct virtio_crypto *vcrypto = vq->vdev->priv;
> +	struct virtio_crypto_ctrl_request *vc_ctrl_req;
> +	unsigned long flags;
> +	unsigned int len;
> +
> +	spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> +	do {
> +		virtqueue_disable_cb(vq);
> +		while ((vc_ctrl_req = virtqueue_get_buf(vq, &len)) != NULL) {
> +			spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> +			if (vc_ctrl_req->ctrl_cb)
> +				vc_ctrl_req->ctrl_cb(vc_ctrl_req);
> +			spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> +		}
> +		if (unlikely(virtqueue_is_broken(vq)))
> +			break;
> +	} while (!virtqueue_enable_cb(vq));
> +	spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> +}
> diff --git a/drivers/crypto/virtio/virtio_crypto_common.h b/drivers/crypto/virtio/virtio_crypto_common.h
> index d2a20fe6e13e..25b4f22e8605 100644
> --- a/drivers/crypto/virtio/virtio_crypto_common.h
> +++ b/drivers/crypto/virtio/virtio_crypto_common.h
> @@ -81,6 +81,10 @@ struct virtio_crypto_sym_session_info {
>   	__u64 session_id;
>   };
>   
> +struct virtio_crypto_ctrl_request;
> +typedef void (*virtio_crypto_ctrl_callback)
> +		(struct virtio_crypto_ctrl_request *vc_ctrl_req);
> +
>   /*
>    * 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]
> @@ -89,6 +93,8 @@ struct virtio_crypto_ctrl_request {
>   	struct virtio_crypto_op_ctrl_req ctrl;
>   	struct virtio_crypto_session_input input;
>   	struct virtio_crypto_inhdr ctrl_status;
> +	virtio_crypto_ctrl_callback ctrl_cb;
> +	struct completion compl;
>   };
>   
>   struct virtio_crypto_request;
> @@ -141,7 +147,9 @@ 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);
>   
> +void virtcrypto_ctrlq_callback(struct virtqueue *vq);
>   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);
> +


Unnecessary changes.

Thanks


>   #endif /* _VIRTIO_CRYPTO_COMMON_H */
> diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
> index c6f482db0bc0..e668d4b1bc6a 100644
> --- a/drivers/crypto/virtio/virtio_crypto_core.c
> +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> @@ -73,7 +73,7 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
>   		goto err_names;
>   
>   	/* Parameters for control virtqueue */
> -	callbacks[total_vqs - 1] = NULL;
> +	callbacks[total_vqs - 1] = virtcrypto_ctrlq_callback;
>   	names[total_vqs - 1] = "controlq";
>   
>   	/* Allocate/initialize parameters for data virtqueues */

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: Re: [PATCH v3 2/5] virtio-crypto: wait ctrl queue instead of busy polling

[zhenwei pi]

On 4/22/22 15:46, Jason Wang wrote:
> 
> 在 2022/4/21 18:40, zhenwei pi 写道:
>> Originally, after submitting request into virtio crypto control
>> queue, the guest side polls the result from the virt queue. This
>> works like following:
>>      CPU0   CPU1               ...             CPUx  CPUy
>>       |      |                                  |     |
>>       \      \                                  /     /
>>        \--------spin_lock(&vcrypto->ctrl_lock)-------/
>>                             |
>>                   virtqueue add & kick
>>                             |
>>                    busy poll virtqueue
>>                             |
>>                spin_unlock(&vcrypto->ctrl_lock)
>>                            ...
>>
>> There are two problems:
>> 1, The queue depth is always 1, the performance of a virtio crypto
>>     device gets limited. Multi user processes share a single control
>>     queue, and hit spin lock race from control queue. Test on Intel
>>     Platinum 8260, a single worker gets ~35K/s create/close session
>>     operations, and 8 workers get ~40K/s operations with 800% CPU
>>     utilization.
>> 2, The control request is supposed to get handled immediately, but
>>     in the current implementation of QEMU(v6.2), the vCPU thread kicks
>>     another thread to do this work, the latency also gets unstable.
>>     Tracking latency of virtio_crypto_alg_akcipher_close_session in 5s:
>>          usecs               : count     distribution
>>           0 -> 1          : 0        |                        |
>>           2 -> 3          : 7        |                        |
>>           4 -> 7          : 72       |                        |
>>           8 -> 15         : 186485   |************************|
>>          16 -> 31         : 687      |                        |
>>          32 -> 63         : 5        |                        |
>>          64 -> 127        : 3        |                        |
>>         128 -> 255        : 1        |                        |
>>         256 -> 511        : 0        |                        |
>>         512 -> 1023       : 0        |                        |
>>        1024 -> 2047       : 0        |                        |
>>        2048 -> 4095       : 0        |                        |
>>        4096 -> 8191       : 0        |                        |
>>        8192 -> 16383      : 2        |                        |
>>     This means that a CPU may hold vcrypto->ctrl_lock as long as 
>> 8192~16383us.
>>
>> To improve the performance of control queue, a request on control 
>> queue waits
>> completion instead of busy polling to reduce lock racing, and gets 
>> completed by
>> control queue callback.
>>      CPU0   CPU1               ...             CPUx  CPUy
>>       |      |                                  |     |
>>       \      \                                  /     /
>>        \--------spin_lock(&vcrypto->ctrl_lock)-------/
>>                             |
>>                   virtqueue add & kick
>>                             |
>>        ---------spin_unlock(&vcrypto->ctrl_lock)------
>>       /      /                                  \     \
>>       |      |                                  |     |
>>      wait   wait                               wait  wait
>>
>> Test this patch, the guest side get ~200K/s operations with 300% CPU
>> utilization.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>> ---
>>   drivers/crypto/virtio/virtio_crypto_common.c | 42 +++++++++++++++-----
>>   drivers/crypto/virtio/virtio_crypto_common.h |  8 ++++
>>   drivers/crypto/virtio/virtio_crypto_core.c   |  2 +-
>>   3 files changed, 41 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/crypto/virtio/virtio_crypto_common.c 
>> b/drivers/crypto/virtio/virtio_crypto_common.c
>> index e65125a74db2..93df73c40dd3 100644
>> --- a/drivers/crypto/virtio/virtio_crypto_common.c
>> +++ b/drivers/crypto/virtio/virtio_crypto_common.c
>> @@ -8,14 +8,21 @@
>>   #include "virtio_crypto_common.h"
>> +static void virtio_crypto_ctrlq_callback(struct 
>> virtio_crypto_ctrl_request *vc_ctrl_req)
>> +{
>> +    complete(&vc_ctrl_req->compl);
>> +}
>> +
>>   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;
>> +    init_completion(&vc_ctrl_req->compl);
>> +    vc_ctrl_req->ctrl_cb =  virtio_crypto_ctrlq_callback;
> 
> 
> Is there a chance that the cb would not be virtio_crypto_ctrlq_callback()?
> 

Yes, it's the only callback function used for control queue, removing 
this and calling virtio_crypto_ctrlq_callback directly in 
virtcrypto_ctrlq_callback seems better. I'll fix this in the next version.
> 
>> +
>>       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) {
>> @@ -24,16 +31,31 @@ int virtio_crypto_ctrl_vq_request(struct 
>> virtio_crypto *vcrypto, struct scatterl
>>       }
>>       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);
>> +    wait_for_completion(&vc_ctrl_req->compl);
>> +
>>       return 0;
>>   }
>> +
>> +void virtcrypto_ctrlq_callback(struct virtqueue *vq)
>> +{
>> +    struct virtio_crypto *vcrypto = vq->vdev->priv;
>> +    struct virtio_crypto_ctrl_request *vc_ctrl_req;
>> +    unsigned long flags;
>> +    unsigned int len;
>> +
>> +    spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
>> +    do {
>> +        virtqueue_disable_cb(vq);
>> +        while ((vc_ctrl_req = virtqueue_get_buf(vq, &len)) != NULL) {
>> +            spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
>> +            if (vc_ctrl_req->ctrl_cb)
>> +                vc_ctrl_req->ctrl_cb(vc_ctrl_req);
>> +            spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
>> +        }
>> +        if (unlikely(virtqueue_is_broken(vq)))
>> +            break;
>> +    } while (!virtqueue_enable_cb(vq));
>> +    spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
>> +}
>> diff --git a/drivers/crypto/virtio/virtio_crypto_common.h 
>> b/drivers/crypto/virtio/virtio_crypto_common.h
>> index d2a20fe6e13e..25b4f22e8605 100644
>> --- a/drivers/crypto/virtio/virtio_crypto_common.h
>> +++ b/drivers/crypto/virtio/virtio_crypto_common.h
>> @@ -81,6 +81,10 @@ struct virtio_crypto_sym_session_info {
>>       __u64 session_id;
>>   };
>> +struct virtio_crypto_ctrl_request;
>> +typedef void (*virtio_crypto_ctrl_callback)
>> +        (struct virtio_crypto_ctrl_request *vc_ctrl_req);
>> +
>>   /*
>>    * 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]
>> @@ -89,6 +93,8 @@ struct virtio_crypto_ctrl_request {
>>       struct virtio_crypto_op_ctrl_req ctrl;
>>       struct virtio_crypto_session_input input;
>>       struct virtio_crypto_inhdr ctrl_status;
>> +    virtio_crypto_ctrl_callback ctrl_cb;
>> +    struct completion compl;
>>   };
>>   struct virtio_crypto_request;
>> @@ -141,7 +147,9 @@ 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);
>> +void virtcrypto_ctrlq_callback(struct virtqueue *vq);
>>   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);
>> +
> 
> 
> Unnecessary changes.
> 
> Thanks
> 
> 
>>   #endif /* _VIRTIO_CRYPTO_COMMON_H */
>> diff --git a/drivers/crypto/virtio/virtio_crypto_core.c 
>> b/drivers/crypto/virtio/virtio_crypto_core.c
>> index c6f482db0bc0..e668d4b1bc6a 100644
>> --- a/drivers/crypto/virtio/virtio_crypto_core.c
>> +++ b/drivers/crypto/virtio/virtio_crypto_core.c
>> @@ -73,7 +73,7 @@ static int virtcrypto_find_vqs(struct virtio_crypto 
>> *vi)
>>           goto err_names;
>>       /* Parameters for control virtqueue */
>> -    callbacks[total_vqs - 1] = NULL;
>> +    callbacks[total_vqs - 1] = virtcrypto_ctrlq_callback;
>>       names[total_vqs - 1] = "controlq";
>>       /* Allocate/initialize parameters for data virtqueues */
> 

-- 
zhenwei pi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 3/5] virtio-crypto: move helpers into virtio_crypto_common.c

[zhenwei pi]

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

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/crypto/virtio/virtio_crypto_common.c | 31 +++++++++++++++++++
 drivers/crypto/virtio/virtio_crypto_common.h |  2 ++
 drivers/crypto/virtio/virtio_crypto_core.c   | 32 --------------------
 3 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_common.c b/drivers/crypto/virtio/virtio_crypto_common.c
index 93df73c40dd3..4a23524896fe 100644
--- a/drivers/crypto/virtio/virtio_crypto_common.c
+++ b/drivers/crypto/virtio/virtio_crypto_common.c
@@ -8,6 +8,14 @@
 
 #include "virtio_crypto_common.h"
 
+void virtcrypto_clear_request(struct virtio_crypto_request *vc_req)
+{
+	if (vc_req) {
+		kfree_sensitive(vc_req->req_data);
+		kfree(vc_req->sgs);
+	}
+}
+
 static void virtio_crypto_ctrlq_callback(struct virtio_crypto_ctrl_request *vc_ctrl_req)
 {
 	complete(&vc_ctrl_req->compl);
@@ -59,3 +67,26 @@ void virtcrypto_ctrlq_callback(struct virtqueue *vq)
 	} while (!virtqueue_enable_cb(vq));
 	spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
 }
+
+void virtcrypto_dataq_callback(struct virtqueue *vq)
+{
+	struct virtio_crypto *vcrypto = vq->vdev->priv;
+	struct virtio_crypto_request *vc_req;
+	unsigned long flags;
+	unsigned int len;
+	unsigned int qid = vq->index;
+
+	spin_lock_irqsave(&vcrypto->data_vq[qid].lock, flags);
+	do {
+		virtqueue_disable_cb(vq);
+		while ((vc_req = virtqueue_get_buf(vq, &len)) != NULL) {
+			spin_unlock_irqrestore(
+				&vcrypto->data_vq[qid].lock, flags);
+			if (vc_req->alg_cb)
+				vc_req->alg_cb(vc_req, len);
+			spin_lock_irqsave(
+				&vcrypto->data_vq[qid].lock, flags);
+		}
+	} while (!virtqueue_enable_cb(vq));
+	spin_unlock_irqrestore(&vcrypto->data_vq[qid].lock, flags);
+}
diff --git a/drivers/crypto/virtio/virtio_crypto_common.h b/drivers/crypto/virtio/virtio_crypto_common.h
index 25b4f22e8605..4d33ed5593d4 100644
--- a/drivers/crypto/virtio/virtio_crypto_common.h
+++ b/drivers/crypto/virtio/virtio_crypto_common.h
@@ -152,4 +152,6 @@ int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, struct scatterl
 				  unsigned int out_sgs, unsigned int in_sgs,
 				  struct virtio_crypto_ctrl_request *vc_ctrl_req);
 
+void virtcrypto_dataq_callback(struct virtqueue *vq);
+
 #endif /* _VIRTIO_CRYPTO_COMMON_H */
diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
index e668d4b1bc6a..d8edefcb966c 100644
--- a/drivers/crypto/virtio/virtio_crypto_core.c
+++ b/drivers/crypto/virtio/virtio_crypto_core.c
@@ -13,38 +13,6 @@
 #include "virtio_crypto_common.h"
 
 
-void
-virtcrypto_clear_request(struct virtio_crypto_request *vc_req)
-{
-	if (vc_req) {
-		kfree_sensitive(vc_req->req_data);
-		kfree(vc_req->sgs);
-	}
-}
-
-static void virtcrypto_dataq_callback(struct virtqueue *vq)
-{
-	struct virtio_crypto *vcrypto = vq->vdev->priv;
-	struct virtio_crypto_request *vc_req;
-	unsigned long flags;
-	unsigned int len;
-	unsigned int qid = vq->index;
-
-	spin_lock_irqsave(&vcrypto->data_vq[qid].lock, flags);
-	do {
-		virtqueue_disable_cb(vq);
-		while ((vc_req = virtqueue_get_buf(vq, &len)) != NULL) {
-			spin_unlock_irqrestore(
-				&vcrypto->data_vq[qid].lock, flags);
-			if (vc_req->alg_cb)
-				vc_req->alg_cb(vc_req, len);
-			spin_lock_irqsave(
-				&vcrypto->data_vq[qid].lock, flags);
-		}
-	} while (!virtqueue_enable_cb(vq));
-	spin_unlock_irqrestore(&vcrypto->data_vq[qid].lock, flags);
-}
-
 static int virtcrypto_find_vqs(struct virtio_crypto *vi)
 {
 	vq_callback_t **callbacks;
-- 
2.20.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 4/5] virtio-crypto: adjust dst_len at ops callback

[zhenwei pi]

From: lei he <helei.sig11@bytedance.com>

For some akcipher operations(eg, decryption of pkcs1pad(rsa)),
the length of returned result maybe less than akcipher_req->dst_len,
we need to recalculate the actual dst_len through the virt-queue
protocol.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: lei he <helei.sig11@bytedance.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 9561bc2df62b..82db86e088c2 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -90,9 +90,12 @@ static void virtio_crypto_dataq_akcipher_callback(struct virtio_crypto_request *
 	}
 
 	akcipher_req = vc_akcipher_req->akcipher_req;
-	if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY)
+	if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY) {
+		/* actuall length maybe less than dst buffer */
+		akcipher_req->dst_len = len - sizeof(vc_req->status);
 		sg_copy_from_buffer(akcipher_req->dst, sg_nents(akcipher_req->dst),
 				    vc_akcipher_req->dst_buf, akcipher_req->dst_len);
+	}
 	virtio_crypto_akcipher_finalize_req(vc_akcipher_req, akcipher_req, error);
 }
 
-- 
2.20.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v3 4/5] virtio-crypto: adjust dst_len at ops callback

[Gonglei (Arei) via Virtualization]

> -----Original Message-----
> From: zhenwei pi [mailto:pizhenwei@bytedance.com]
> Sent: Thursday, April 21, 2022 6:40 PM
> To: Gonglei (Arei) <arei.gonglei@huawei.com>; mst@redhat.com
> Cc: jasowang@redhat.com; herbert@gondor.apana.org.au;
> linux-kernel@vger.kernel.org; virtualization@lists.linux-foundation.org;
> linux-crypto@vger.kernel.org; helei.sig11@bytedance.com;
> davem@davemloft.net; zhenwei pi <pizhenwei@bytedance.com>
> Subject: [PATCH v3 4/5] virtio-crypto: adjust dst_len at ops callback
> 
> From: lei he <helei.sig11@bytedance.com>
> 
> For some akcipher operations(eg, decryption of pkcs1pad(rsa)), the length of
> returned result maybe less than akcipher_req->dst_len, we need to recalculate
> the actual dst_len through the virt-queue protocol.
> 
OK ...

> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: lei he <helei.sig11@bytedance.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 9561bc2df62b..82db86e088c2 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -90,9 +90,12 @@ static void
> virtio_crypto_dataq_akcipher_callback(struct virtio_crypto_request *
>  	}
> 
>  	akcipher_req = vc_akcipher_req->akcipher_req;
> -	if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY)
> +	if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY) {
> +		/* actuall length maybe less than dst buffer */
> +		akcipher_req->dst_len = len - sizeof(vc_req->status);

...but why minus sizeof(vc_req->status)?


Regards,
-Gonglei


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 5/5] virtio-crypto: enable retry for virtio-crypto-dev

[zhenwei pi]

From: lei he <helei.sig11@bytedance.com>

Enable retry for virtio-crypto-dev, so that crypto-engine
can process cipher-requests parallelly.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: lei he <helei.sig11@bytedance.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/crypto/virtio/virtio_crypto_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
index d8edefcb966c..5c0d68c9e894 100644
--- a/drivers/crypto/virtio/virtio_crypto_core.c
+++ b/drivers/crypto/virtio/virtio_crypto_core.c
@@ -62,7 +62,8 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
 		spin_lock_init(&vi->data_vq[i].lock);
 		vi->data_vq[i].vq = vqs[i];
 		/* Initialize crypto engine */
-		vi->data_vq[i].engine = crypto_engine_alloc_init(dev, 1);
+		vi->data_vq[i].engine = crypto_engine_alloc_init_and_set(dev, true, NULL, 1,
+						virtqueue_get_vring_size(vqs[i]));
 		if (!vi->data_vq[i].engine) {
 			ret = -ENOMEM;
 			goto err_engine;
-- 
2.20.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization