Message-ID: <CACGkMEutBJpeZbO52KM6=2HPTBuGx00hVyjBuj7nu5EUHuofrQ@mail.gmail.com>
Date:   Sun, 24 Apr 2022 14:21:24 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     zhenwei pi <pizhenwei@...edance.com>
Cc:     "Gonglei (Arei)" <arei.gonglei@...wei.com>, mst <mst@...hat.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        linux-crypto@...r.kernel.org, helei.sig11@...edance.com,
        davem <davem@...emloft.net>
Subject: Re: Re: [PATCH v3 1/5] virtio-crypto: use private buffer for control request

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

Powered by blists - more mailing lists