[PATCH 0/4] virtio-crypto: Improve performance

[PATCH 0/4] virtio-crypto: Improve performance

[zhenwei pi]

Hi,

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 (2):
  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      |  92 ++++++------
 drivers/crypto/virtio/virtio_crypto_common.c  |  92 ++++++++++++
 drivers/crypto/virtio/virtio_crypto_common.h  |  25 +++-
 drivers/crypto/virtio/virtio_crypto_core.c    |  37 +----
 .../virtio/virtio_crypto_skcipher_algs.c      | 134 ++++++++----------
 6 files changed, 222 insertions(+), 159 deletions(-)
 create mode 100644 drivers/crypto/virtio/virtio_crypto_common.c

-- 
2.20.1

[PATCH 1/4] 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. 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        |                        |

To improve the performance of control queue, wait completion instead
of busy polling without lock race, and get completed by control queue
callback. Test this patch, the guest side get ~200K/s operations with
300% CPU utilization.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/crypto/virtio/Makefile                |   1 +
 .../virtio/virtio_crypto_akcipher_algs.c      |  87 ++++++------
 drivers/crypto/virtio/virtio_crypto_common.c  |  61 ++++++++
 drivers/crypto/virtio/virtio_crypto_common.h  |  23 ++-
 drivers/crypto/virtio/virtio_crypto_core.c    |   2 +-
 .../virtio/virtio_crypto_skcipher_algs.c      | 134 ++++++++----------
 6 files changed, 183 insertions(+), 125 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..bf7c1aa4be37 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -102,107 +102,100 @@ 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 = NULL;
 	uint8_t *pkey;
-	unsigned int inlen;
-	int err;
+	int err = -ENOMEM;
 	unsigned int num_out = 0, num_in = 0;
+	int node = dev_to_node(&vcrypto->vdev->dev);
 
 	pkey = kmemdup(key, keylen, GFP_ATOMIC);
 	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_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
+	if (!vc_ctrl_req)
+		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_sensitive(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 = NULL;
 	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;
+	int node = dev_to_node(&vcrypto->vdev->dev);
 
-	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;
+
+	vc_ctrl_req = kzalloc_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
+	if (!vc_ctrl_req)
+		return -ENOMEM;
 
-	destroy_session = &vcrypto->ctrl.u.destroy_session;
+	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;
-
+	err = 0;
 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_sensitive(vc_ctrl_req);
 
 	return 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..93df73c40dd3
--- /dev/null
+++ b/drivers/crypto/virtio/virtio_crypto_common.c
@@ -0,0 +1,61 @@
+// 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"
+
+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 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) {
+		spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
+		return err;
+	}
+
+	virtqueue_kick(vcrypto->ctrl_vq);
+	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 e693d4ee83a6..125ad2300b83 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,18 @@ 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);
+
+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;
 typedef void (*virtio_crypto_data_callback)
 		(struct virtio_crypto_request *vc_req, int len);
@@ -135,4 +143,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 */
diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
index a618c46a52b8..b8999dab3e66 100644
--- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
@@ -118,11 +118,14 @@ 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;
+	int err = -ENOMEM;
 	unsigned int num_out = 0, num_in = 0;
+	struct virtio_crypto_ctrl_request *vc_ctrl_req = NULL;
+	struct virtio_crypto_ctrl_header *header;
+	struct virtio_crypto_sym_create_session_req *sym_create_session;
+	int node = dev_to_node(&vcrypto->vdev->dev);
 
 	/*
 	 * Avoid to do DMA from the stack, switch to using
@@ -133,26 +136,25 @@ static int virtio_crypto_alg_skcipher_init_session(
 	if (!cipher_key)
 		return -ENOMEM;
 
-	spin_lock(&vcrypto->ctrl_lock);
+	vc_ctrl_req = kzalloc_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
+	if (!vc_ctrl_req)
+		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 +162,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);
+	err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, num_in, vc_ctrl_req);
+	if (err < 0)
+		goto out;
 
-	/*
-	 * 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();
-
-	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_sensitive(vc_ctrl_req);
 	kfree_sensitive(cipher_key);
-	return 0;
+
+	return err;
 }
 
 static int virtio_crypto_alg_skcipher_close_session(
@@ -206,21 +197,25 @@ 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 = NULL;
+	struct virtio_crypto_ctrl_header *header;
+	int node = dev_to_node(&vcrypto->vdev->dev);
+
+	vc_ctrl_req = kzalloc_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
+	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,32 @@ 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;
 	}
-	spin_unlock(&vcrypto->ctrl_lock);
 
-	return 0;
+	err = 0;
+out:
+	kfree_sensitive(vc_ctrl_req);
+
+	return err;
 }
 
 static int virtio_crypto_alg_skcipher_init_sessions(
-- 
2.20.1

[PATCH 2/4] 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

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 125ad2300b83..a003935f91a9 100644
--- a/drivers/crypto/virtio/virtio_crypto_common.h
+++ b/drivers/crypto/virtio/virtio_crypto_common.h
@@ -148,4 +148,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

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

[zhenwei pi]

From: lei he <helei@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.

Signed-off-by: lei he <helei@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 bf7c1aa4be37..7cd932c2d9ee 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

[PATCH 4/4] 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.

Signed-off-by: lei he <helei.sig11@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

Re: [PATCH 1/4] virtio-crypto: wait ctrl queue instead of busy polling

[Michael S. Tsirkin]

On Fri, Apr 15, 2022 at 02:41:33PM +0800, zhenwei pi wrote:
> Originally, after submitting request into virtio crypto control
> queue, the guest side polls the result from the virt queue. 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        |                        |
> 
> To improve the performance of control queue, wait completion instead
> of busy polling without lock race, and get completed by control queue
> callback. Test this patch, the guest side get ~200K/s operations with
> 300% CPU utilization.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  drivers/crypto/virtio/Makefile                |   1 +
>  .../virtio/virtio_crypto_akcipher_algs.c      |  87 ++++++------
>  drivers/crypto/virtio/virtio_crypto_common.c  |  61 ++++++++
>  drivers/crypto/virtio/virtio_crypto_common.h  |  23 ++-
>  drivers/crypto/virtio/virtio_crypto_core.c    |   2 +-
>  .../virtio/virtio_crypto_skcipher_algs.c      | 134 ++++++++----------
>  6 files changed, 183 insertions(+), 125 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..bf7c1aa4be37 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -102,107 +102,100 @@ 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 = NULL;

this is initialized down the road, I think you can skip = NULL here.

>  	uint8_t *pkey;
> -	unsigned int inlen;
> -	int err;
> +	int err = -ENOMEM;

I would assign this in the single case where this value is used.

>  	unsigned int num_out = 0, num_in = 0;
> +	int node = dev_to_node(&vcrypto->vdev->dev);
>  
are you sure it is
better to allocate close to device and not to current node
which is the default?


>  	pkey = kmemdup(key, keylen, GFP_ATOMIC);
>  	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_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
> +	if (!vc_ctrl_req)
> +		goto out;

do you need to allocate it with kzalloc?
is anything wrong with just keeping it part of device?
even if yes this change is better split in a separate patch, would make the patch smaller.



>  
> -	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_sensitive(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)
>  {


same comments apply to this function.

>  	struct scatterlist outhdr_sg, inhdr_sg, *sgs[2];
> +	struct virtio_crypto_ctrl_request *vc_ctrl_req = NULL;
>  	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;
> +	int node = dev_to_node(&vcrypto->vdev->dev);
>  
> -	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;
> +
> +	vc_ctrl_req = kzalloc_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
> +	if (!vc_ctrl_req)
> +		return -ENOMEM;
>  
> -	destroy_session = &vcrypto->ctrl.u.destroy_session;
> +	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;
> -
> +	err = 0;
>  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_sensitive(vc_ctrl_req);
>  
>  	return 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..93df73c40dd3
> --- /dev/null
> +++ b/drivers/crypto/virtio/virtio_crypto_common.c
> @@ -0,0 +1,61 @@
> +// 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"
> +
> +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 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) {
> +		spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> +		return err;
> +	}
> +
> +	virtqueue_kick(vcrypto->ctrl_vq);
> +	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) {


you really need to break out of this loop if vq is broken,
virtqueue_get_buf will keep returning NULL in this case.


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

speaking of which existing code does not handle vq broken case
all that well but it looks like this patch makes it a bit worse.
want to try fixing? basically report an error ...


> +}
> diff --git a/drivers/crypto/virtio/virtio_crypto_common.h b/drivers/crypto/virtio/virtio_crypto_common.h
> index e693d4ee83a6..125ad2300b83 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;

which is ok as far as it goes but seems unrelated to what
the patch is trying to do ...

> @@ -85,6 +81,18 @@ 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);
> +
> +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;
>  typedef void (*virtio_crypto_data_callback)
>  		(struct virtio_crypto_request *vc_req, int len);
> @@ -135,4 +143,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 */
> diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> index a618c46a52b8..b8999dab3e66 100644
> --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> @@ -118,11 +118,14 @@ 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;
> +	int err = -ENOMEM;
>  	unsigned int num_out = 0, num_in = 0;
> +	struct virtio_crypto_ctrl_request *vc_ctrl_req = NULL;
> +	struct virtio_crypto_ctrl_header *header;
> +	struct virtio_crypto_sym_create_session_req *sym_create_session;
> +	int node = dev_to_node(&vcrypto->vdev->dev);
>  
>  	/*
>  	 * Avoid to do DMA from the stack, switch to using
> @@ -133,26 +136,25 @@ static int virtio_crypto_alg_skcipher_init_session(
>  	if (!cipher_key)
>  		return -ENOMEM;
>  
> -	spin_lock(&vcrypto->ctrl_lock);
> +	vc_ctrl_req = kzalloc_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
> +	if (!vc_ctrl_req)
> +		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 +162,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);
> +	err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, num_in, vc_ctrl_req);
> +	if (err < 0)
> +		goto out;
>  
> -	/*
> -	 * 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();
> -
> -	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_sensitive(vc_ctrl_req);

it is interesting that you use kfree_sensitive here. why is that?
is there in fact anything sensitive here? if yes this is a security
improvement and might need its own patch, or at least documentation.

>  	kfree_sensitive(cipher_key);
> -	return 0;
> +
> +	return err;
>  }
>  
>  static int virtio_crypto_alg_skcipher_close_session(
> @@ -206,21 +197,25 @@ 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 = NULL;
> +	struct virtio_crypto_ctrl_header *header;
> +	int node = dev_to_node(&vcrypto->vdev->dev);
> +
> +	vc_ctrl_req = kzalloc_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
> +	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,32 @@ 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;
>  	}
> -	spin_unlock(&vcrypto->ctrl_lock);
>  
> -	return 0;
> +	err = 0;
> +out:
> +	kfree_sensitive(vc_ctrl_req);
> +
> +	return err;


same comments apply to this function.

>  }
>  
>  static int virtio_crypto_alg_skcipher_init_sessions(
> -- 
> 2.20.1

Re: Re: [PATCH 1/4] virtio-crypto: wait ctrl queue instead of busy polling

[zhenwei pi]

On 4/15/22 16:41, Michael S. Tsirkin wrote:
>> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
>> index f3ec9420215e..bf7c1aa4be37 100644
>> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
>> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
>> @@ -102,107 +102,100 @@ 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 = NULL;
> 
> this is initialized down the road, I think you can skip = NULL here.
> 
OK.
>>   	uint8_t *pkey;
>> -	unsigned int inlen;
>> -	int err;
>> +	int err = -ENOMEM;
> 
> I would assign this in the single case where this value is used.
> 
OK
>>   	unsigned int num_out = 0, num_in = 0;
>> +	int node = dev_to_node(&vcrypto->vdev->dev);
>>   
> are you sure it is
> better to allocate close to device and not to current node
> which is the default?
>
Also with this part:
  /* 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;

Orignally virtio crypto driver allocates ctrl&input&ctrl_status 
per-device, and protects this with ctrl_lock. This is the reason why the 
control queue reaches the bottleneck of performance. I'll append this in 
the next version in commit message.

Instead of the single request buffer, declare struct 
virtio_crypto_ctrl_request {
         struct virtio_crypto_op_ctrl_req ctrl;
         struct virtio_crypto_session_input input;
         struct virtio_crypto_inhdr ctrl_status;
	... }

The motivation of this change is to allocate buffer from the same node 
with device during control queue operations.

> 
>>   	pkey = kmemdup(key, keylen, GFP_ATOMIC);
>>   	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_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
>> +	if (!vc_ctrl_req)
>> +		goto out;
> 
> do you need to allocate it with kzalloc?
> is anything wrong with just keeping it part of device?
> even if yes this change is better split in a separate patch, would make the patch smaller.
Because there are padding field in 
virtio_crypto_op_ctrl_req&virtio_crypto_session_input, I suppose the 
original version also needs to clear padding field.
So I use kzalloc to make sure that the padding field gets cleared.
If this is reasonable, to separate this patch is OK to me, or I append 
this reason into commit message and comments in code.

>> +
>> +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) {
> 
> 
> you really need to break out of this loop if vq is broken,
> virtqueue_get_buf will keep returning NULL in this case.
> 
I'm a little confused here, if virtqueue_get_buf return NULL, this loop 
will break. Could you please give me more hints?
> 
>> +			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);
> 
> speaking of which existing code does not handle vq broken case
> all that well but it looks like this patch makes it a bit worse.
> want to try fixing? basically report an error ...
> 
>if virtqueue is broken, I can print log.

>> 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 */
>> diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
>> index a618c46a52b8..b8999dab3e66 100644
>> --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
>> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
>> +	err = 0;
>> +out:
>> +	kfree_sensitive(vc_ctrl_req);
> 
> it is interesting that you use kfree_sensitive here. why is that?
> is there in fact anything sensitive here? if yes this is a security
> improvement and might need its own patch, or at least documentation.
>

OK, kfree is good enough here, I'll fix this.


Thanks a lot!


-- 
zhenwei pi

Re: Re: [PATCH 1/4] virtio-crypto: wait ctrl queue instead of busy polling

[Michael S. Tsirkin]

On Fri, Apr 15, 2022 at 06:50:19PM +0800, zhenwei pi wrote:
> On 4/15/22 16:41, Michael S. Tsirkin wrote:
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > index f3ec9420215e..bf7c1aa4be37 100644
> > > --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > @@ -102,107 +102,100 @@ 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 = NULL;
> > 
> > this is initialized down the road, I think you can skip = NULL here.
> > 
> OK.
> > >   	uint8_t *pkey;
> > > -	unsigned int inlen;
> > > -	int err;
> > > +	int err = -ENOMEM;
> > 
> > I would assign this in the single case where this value is used.
> > 
> OK
> > >   	unsigned int num_out = 0, num_in = 0;
> > > +	int node = dev_to_node(&vcrypto->vdev->dev);
> > are you sure it is
> > better to allocate close to device and not to current node
> > which is the default?
> > 
> Also with this part:
>  /* 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;
> 
> Orignally virtio crypto driver allocates ctrl&input&ctrl_status per-device,
> and protects this with ctrl_lock. This is the reason why the control queue
> reaches the bottleneck of performance. I'll append this in the next version
> in commit message.
> 
> Instead of the single request buffer, declare struct
> virtio_crypto_ctrl_request {
>         struct virtio_crypto_op_ctrl_req ctrl;
>         struct virtio_crypto_session_input input;
>         struct virtio_crypto_inhdr ctrl_status;
> 	... }
> 
> The motivation of this change is to allocate buffer from the same node with
> device during control queue operations.

But are you sure it's a win?  quite possibly it's a win to
have it close to driver not close to device.
This kind of change is really best done separately with some
testing showing it's a win. If that is too much to ask,
make it a separate patch and add some analysis explaining
why device accesses the structure more than the driver.


> > 
> > >   	pkey = kmemdup(key, keylen, GFP_ATOMIC);
> > >   	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_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
> > > +	if (!vc_ctrl_req)
> > > +		goto out;
> > 
> > do you need to allocate it with kzalloc?
> > is anything wrong with just keeping it part of device?
> > even if yes this change is better split in a separate patch, would make the patch smaller.
> Because there are padding field in
> virtio_crypto_op_ctrl_req&virtio_crypto_session_input, I suppose the
> original version also needs to clear padding field.
> So I use kzalloc to make sure that the padding field gets cleared.
> If this is reasonable, to separate this patch is OK to me, or I append this
> reason into commit message and comments in code.

Not sure I understand. Maybe add a code comment explaining
what is cleared and why.

> > > +
> > > +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) {
> > 
> > 
> > you really need to break out of this loop if vq is broken,
> > virtqueue_get_buf will keep returning NULL in this case.
> > 
> I'm a little confused here, if virtqueue_get_buf return NULL, this loop will
> break. Could you please give me more hints?

Oh right. Sorry was confused.

> > 
> > > +			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);
> > 
> > speaking of which existing code does not handle vq broken case
> > all that well but it looks like this patch makes it a bit worse.
> > want to try fixing? basically report an error ...
> > 
> > if virtqueue is broken, I can print log.
> 
> > > 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 */
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > index a618c46a52b8..b8999dab3e66 100644
> > > --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > +	err = 0;
> > > +out:
> > > +	kfree_sensitive(vc_ctrl_req);
> > 
> > it is interesting that you use kfree_sensitive here. why is that?
> > is there in fact anything sensitive here? if yes this is a security
> > improvement and might need its own patch, or at least documentation.
> > 
> 
> OK, kfree is good enough here, I'll fix this.
> 
> 
> Thanks a lot!
> 
> 
> -- 
> zhenwei pi