Re: Re: Re: [PATCH v3 1/5] virtio-crypto: use private buffer for control request
On 4/24/22 14:21, Jason Wang wrote:
On Fri, Apr 22, 2022 at 5:12 PM zhenwei pi 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
Cc: Jason Wang
Cc: Gonglei
Signed-off-by: zhenwei pi
---
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:
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 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
> >> Cc: Jason Wang
> >> Cc: Gonglei
> >> Signed-off-by: zhenwei pi
> >> ---
> >> 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 s
Re: Re: [PATCH v3 1/5] virtio-crypto: use private buffer for control request
On 4/22/22 15:41, Jason Wang wrote:
在 2022/4/21 18:40, zhenwei pi 写道:
Originally, all of the control requests share a single buffer(
ctrl & input & ctrl_status fields in struct virtio_crypto), this
allows queue depth 1 only, the performance of control queue gets
limited by this design.
In this patch, each request allocates request buffer dynamically, and
free buffer after request, it's possible to optimize control queue
depth in the next step.
A necessary comment is already in code, still describe it again:
/*
* Note: there are padding fields in request, clear them to zero before
* sending to host,
* Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48]
*/
So use kzalloc to allocate buffer of struct virtio_crypto_ctrl_request.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Signed-off-by: zhenwei pi
---
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)
