Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1532258 > unrolled thread

[PATCH v4 0/1] virtio-crypto: add Linux driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/1] virtio-crypto: add Linux driver Gonglei <arei.gonglei@huawei.com> - 2016-11-29 14:00 +0100
    Re: [PATCH v4 1/1] crypto: add virtio-crypto driver Stefan Hajnoczi <stefanha@redhat.com> - 2016-11-30 12:20 +0100
      RE: [PATCH v4 1/1] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-12-01 03:30 +0100
        Re: [PATCH v4 1/1] crypto: add virtio-crypto driver Stefan Hajnoczi <stefanha@redhat.com> - 2016-12-01 13:00 +0100
          RE: [PATCH v4 1/1] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-12-01 13:40 +0100
        Re: [PATCH v4 1/1] crypto: add virtio-crypto driver Stefan Hajnoczi <stefanha@redhat.com> - 2016-12-01 13:00 +0100
          RE: [PATCH v4 1/1] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-12-01 13:30 +0100

#1532258 — [PATCH v4 0/1] virtio-crypto: add Linux driver

v4:
 - rework unknow status bit handler by calling virtio_break_device(). [Cornelia]
 - convert space to tab in Kconfig. [Stefan]
 - rename virtio_crypto.c to virtio_crypto_core.c and then make the
   moudle named virtio_crypto.ko for consistency. [Stefan]
 - don't call virtcrypto_dev_stop() on failure path. [Stefan]
 - don't add two empty lines. [Michael]
 - fix possible race by add spin_lock in virtio_crypto_alg_ablkcipher_init_session() [Michael and Halil]
 - drop virtcrypto_devmgr_get_first() calling in virtio_crypto_ablkcipher_setkey. [Michael]
 - drop superfluous assigned value for virtio_crypto_algs[i].cra_flags
   in virtio_crypto_algs_register(). [Stefan]
 - decrease virtio_crypto_active_devs if calling crypto_register_algs() failed. [Stefan]
 - fix some typos here and there. [Stefan]
 - fix missing table_lock usage in virtio_crypto_mgr.c. [Stefan]
 - drop confused comments in virtio_crypto_alg_ablkcipher_init_session()
   for virtqueue_kick(). [Halil]

v3:
 - set cpu affinity when data queues are not equal to the number of online cpus. [Michael]
 - add TODO comments for cpu hotplug (changing the relationship of binding virtqueue and cpu)
 - use __u32/64 in the config space since the virtio->get() doesn't support byte-swap yet. [Michael]
 - drop the whole patch 1 of v2 because the above reason.
 - add VERSION_1 check at the beginning of virtcrypto_probe()
 - s/-1/EPERM/g in virtcrypto_update_status(), don't change err to EFAULT then. [Michael]
 - add reset operation before delete the virtqueus. [Micheal]
 - drop an unnecessiry spin_lock calling in virtcrypto_freeze(), avoid possible dead lock. [Micheal]
 - redefine parameter alg's type in order to use a cast for it. [Michael]
 - pad all structures to have the same size in one union, and add a member to
   show the union's size in virtio_crypto.h. [Michael]
 - update MAINTAINER file to add virtio-crypto stuff to Michael's entry so that
   the corresponding patches can be CC'ed to Michael as well because the virtio-crypto
   doesn't lay in driver/virtio directory. 

The virtio crypto device is a virtual cryptography device
as well as a kind of virtual hardware accelerator for
virtual machines. The encryption anddecryption requests
are placed in the data queue and are ultimately handled by
thebackend crypto accelerators. The second queue is the
control queue used to create or destroy sessions for
symmetric algorithms and will control some advanced features
in the future. The virtio crypto device provides the following
cryptoservices: CIPHER, MAC, HASH, and AEAD.

For more information about virtio-crypto device, please see:
  http://qemu-project.org/Features/VirtioCrypto

For better reviewing, pls see below explaination.

The patch mainly includes five files:

 1) virtio_crypto.h is the header file for virtio-crypto device,
which is based on the virtio-crypto specification. 
 2) virtio_crypto_core.c is the entry of the driver module,
which is similar with other virtio devices, such as virtio-net,
virtio-input etc. 
 3) virtio_crypto_mgr.c is used to manage the virtio
crypto devices in the system. We support up to 32 virtio-crypto
devices currently. I use a global list to store the virtio crypto
devices which refer to Intel QAT driver. Meanwhile, the file
includs the functions of add/del/search/start/stop for virtio
crypto devices.
 4) virtio_crypto_common.h is a private header file for virtio
crypto driver, includes structure definations, and function declarations.
 5) virtio_crypto_algs.c is the realization of algs based on Linux Crypto Framwork,
which can register different crypto algorithms. Currently it's only support AES-CBC.
The Crypto guys can mainly focus on this file. 


v2:
 - stop doing DMA from the stack, CONFIG_VMAP_STACK=y [Salvatore]
 - convert __virtio32/64 to __le32/64 in virtio_crypto.h
 - remove VIRTIO_CRYPTO_S_STARTED based on the lastest virtio crypto spec.
 - introduces the little edian functions for VIRTIO_1 devices in patch 1.


Gonglei (1):
  crypto: add virtio-crypto driver

 MAINTAINERS                                  |   9 +
 drivers/crypto/Kconfig                       |   2 +
 drivers/crypto/Makefile                      |   1 +
 drivers/crypto/virtio/Kconfig                |  10 +
 drivers/crypto/virtio/Makefile               |   5 +
 drivers/crypto/virtio/virtio_crypto_algs.c   | 518 +++++++++++++++++++++++++++
 drivers/crypto/virtio/virtio_crypto_common.h | 124 +++++++
 drivers/crypto/virtio/virtio_crypto_core.c   | 460 ++++++++++++++++++++++++
 drivers/crypto/virtio/virtio_crypto_mgr.c    | 262 ++++++++++++++
 include/uapi/linux/Kbuild                    |   1 +
 include/uapi/linux/virtio_crypto.h           | 450 +++++++++++++++++++++++
 include/uapi/linux/virtio_ids.h              |   1 +
 12 files changed, 1843 insertions(+)
 create mode 100644 drivers/crypto/virtio/Kconfig
 create mode 100644 drivers/crypto/virtio/Makefile
 create mode 100644 drivers/crypto/virtio/virtio_crypto_algs.c
 create mode 100644 drivers/crypto/virtio/virtio_crypto_common.h
 create mode 100644 drivers/crypto/virtio/virtio_crypto_core.c
 create mode 100644 drivers/crypto/virtio/virtio_crypto_mgr.c
 create mode 100644 include/uapi/linux/virtio_crypto.h

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1533229 — Re: [PATCH v4 1/1] crypto: add virtio-crypto driver

[Multipart message — attachments visible in raw view] — view raw

On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c b/drivers/crypto/virtio/virtio_crypto_algs.c
> new file mode 100644
> index 0000000..08b077f
> --- /dev/null
> +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> @@ -0,0 +1,518 @@
> + /* Algorithms supported by virtio crypto device
> +  *
> +  * Authors: Gonglei <arei.gonglei@huawei.com>
> +  *
> +  * Copyright 2016 HUAWEI TECHNOLOGIES CO., LTD.
> +  *
> +  * This program is free software; you can redistribute it and/or modify
> +  * it under the terms of the GNU General Public License as published by
> +  * the Free Software Foundation; either version 2 of the License, or
> +  * (at your option) any later version.
> +  *
> +  * This program is distributed in the hope that it will be useful,
> +  * but WITHOUT ANY WARRANTY; without even the implied warranty of
> +  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +  * GNU General Public License for more details.
> +  *
> +  * You should have received a copy of the GNU General Public License
> +  * along with this program; if not, see <http://www.gnu.org/licenses/>.
> +  */
> +
> +#include <linux/scatterlist.h>
> +#include <crypto/algapi.h>
> +#include <linux/err.h>
> +#include <crypto/scatterwalk.h>
> +#include <linux/atomic.h>
> +
> +#include <uapi/linux/virtio_crypto.h>
> +#include "virtio_crypto_common.h"
> +
> +static DEFINE_MUTEX(algs_lock);

Did you run checkpatch.pl?  I think it encourages you to document what
the lock protects.

> +static int virtio_crypto_alg_ablkcipher_init_session(
> +		struct virtio_crypto_ablkcipher_ctx *ctx,
> +		uint32_t alg, const uint8_t *key,
> +		unsigned int keylen,
> +		int encrypt)
> +{
> +	struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> +	unsigned int tmp;
> +	struct virtio_crypto *vcrypto = ctx->vcrypto;
> +	int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT : VIRTIO_CRYPTO_OP_DECRYPT;
> +	int err;
> +	unsigned int num_out = 0, num_in = 0;
> +
> +	/*
> +	 * Avoid to do DMA from the stack, switch to using
> +	 * dynamically-allocated for the key
> +	 */
> +	uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> +
> +	if (!cipher_key)
> +		return -ENOMEM;
> +
> +	memcpy(cipher_key, key, keylen);

Are there any rules on handling key material in the kernel?  This buffer
is just kfreed later.  Do you need to zero it out before freeing it?

> +
> +	spin_lock(&vcrypto->ctrl_lock);

The QAT accelerator driver doesn't spin while talking to the device in
virtio_crypto_alg_ablkcipher_init_session().  I didn't find any other
driver examples in the kernel tree, but this function seems like a
weakness in the virtio-crypto device.

While QEMU is servicing the create session command this vcpu is blocked.
The QEMU global mutex is held so no other vcpu can enter QEMU and the
QMP monitor is also blocked.

This is a scalability and performance problem.  Can you look at how QAT
avoids this synchronous session setup?

[toc] | [prev] | [next] | [standalone]

#1533755 — RE: [PATCH v4 1/1] crypto: add virtio-crypto driver

Hi Stefan,

> 
> On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> b/drivers/crypto/virtio/virtio_crypto_algs.c
> > new file mode 100644
> > index 0000000..08b077f
> > --- /dev/null
> > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > @@ -0,0 +1,518 @@
> > + /* Algorithms supported by virtio crypto device
> > +  *
> > +  * Authors: Gonglei <arei.gonglei@huawei.com>
> > +  *
> > +  * Copyright 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > +  *
> > +  * This program is free software; you can redistribute it and/or modify
> > +  * it under the terms of the GNU General Public License as published by
> > +  * the Free Software Foundation; either version 2 of the License, or
> > +  * (at your option) any later version.
> > +  *
> > +  * This program is distributed in the hope that it will be useful,
> > +  * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +  * GNU General Public License for more details.
> > +  *
> > +  * You should have received a copy of the GNU General Public License
> > +  * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > +  */
> > +
> > +#include <linux/scatterlist.h>
> > +#include <crypto/algapi.h>
> > +#include <linux/err.h>
> > +#include <crypto/scatterwalk.h>
> > +#include <linux/atomic.h>
> > +
> > +#include <uapi/linux/virtio_crypto.h>
> > +#include "virtio_crypto_common.h"
> > +
> > +static DEFINE_MUTEX(algs_lock);
> 
> Did you run checkpatch.pl?  I think it encourages you to document what
> the lock protects.
> 
Sure. Basically I run checkpatch.py each time. :)

# ./scripts/checkpatch.pl 0001-crypto-add-virtio-crypto-driver.patch 
total: 0 errors, 0 warnings, 1873 lines checked

0001-crypto-add-virtio-crypto-driver.patch has no obvious style problems and is ready for submission.

> > +static int virtio_crypto_alg_ablkcipher_init_session(
> > +		struct virtio_crypto_ablkcipher_ctx *ctx,
> > +		uint32_t alg, const uint8_t *key,
> > +		unsigned int keylen,
> > +		int encrypt)
> > +{
> > +	struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> > +	unsigned int tmp;
> > +	struct virtio_crypto *vcrypto = ctx->vcrypto;
> > +	int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> VIRTIO_CRYPTO_OP_DECRYPT;
> > +	int err;
> > +	unsigned int num_out = 0, num_in = 0;
> > +
> > +	/*
> > +	 * Avoid to do DMA from the stack, switch to using
> > +	 * dynamically-allocated for the key
> > +	 */
> > +	uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> > +
> > +	if (!cipher_key)
> > +		return -ENOMEM;
> > +
> > +	memcpy(cipher_key, key, keylen);
> 
> Are there any rules on handling key material in the kernel?  This buffer
> is just kfreed later.  Do you need to zero it out before freeing it?
> 
Good questions. For kernel crypto core, each cipher request should be freed
by skcipher_request_free(): zeroize and free request data structure.

I need to use kzfree() for key as well. I'll also check other stuffs. Thanks. 

> > +
> > +	spin_lock(&vcrypto->ctrl_lock);
> 
> The QAT accelerator driver doesn't spin while talking to the device in
> virtio_crypto_alg_ablkcipher_init_session().  I didn't find any other
> driver examples in the kernel tree, but this function seems like a
> weakness in the virtio-crypto device.
> 
The control queues of virtio-net and virtio-console are also be locked
Please see:
 __send_control_msg() in virtio_console.c and virtio-net's control queue
protected by rtnl lock.

I didn't want to protect session creations but the virtqueue's operations
like what other virtio devices do.

> While QEMU is servicing the create session command this vcpu is blocked.
> The QEMU global mutex is held so no other vcpu can enter QEMU and the
> QMP monitor is also blocked.
> 
> This is a scalability and performance problem.  Can you look at how QAT
> avoids this synchronous session setup?

For QAT driver, the session creation is synchronous as well because it's a
plain software operation which can be completed ASAP.

Regards,
-Gonglei

[toc] | [prev] | [next] | [standalone]

#1533999 — Re: [PATCH v4 1/1] crypto: add virtio-crypto driver

[Multipart message — attachments visible in raw view] — view raw

On Thu, Dec 01, 2016 at 02:27:19AM +0000, Gonglei (Arei) wrote:
> > On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> > b/drivers/crypto/virtio/virtio_crypto_algs.c
> > > new file mode 100644
> > > index 0000000..08b077f
> > > --- /dev/null
> > > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > > @@ -0,0 +1,518 @@
> > > + /* Algorithms supported by virtio crypto device
> > > +  *
> > > +  * Authors: Gonglei <arei.gonglei@huawei.com>
> > > +  *
> > > +  * Copyright 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > > +  *
> > > +  * This program is free software; you can redistribute it and/or modify
> > > +  * it under the terms of the GNU General Public License as published by
> > > +  * the Free Software Foundation; either version 2 of the License, or
> > > +  * (at your option) any later version.
> > > +  *
> > > +  * This program is distributed in the hope that it will be useful,
> > > +  * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +  * GNU General Public License for more details.
> > > +  *
> > > +  * You should have received a copy of the GNU General Public License
> > > +  * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > > +  */
> > > +
> > > +#include <linux/scatterlist.h>
> > > +#include <crypto/algapi.h>
> > > +#include <linux/err.h>
> > > +#include <crypto/scatterwalk.h>
> > > +#include <linux/atomic.h>
> > > +
> > > +#include <uapi/linux/virtio_crypto.h>
> > > +#include "virtio_crypto_common.h"
> > > +
> > > +static DEFINE_MUTEX(algs_lock);
> > 
> > Did you run checkpatch.pl?  I think it encourages you to document what
> > the lock protects.
> > 
> Sure. Basically I run checkpatch.py each time. :)
> 
> # ./scripts/checkpatch.pl 0001-crypto-add-virtio-crypto-driver.patch 
> total: 0 errors, 0 warnings, 1873 lines checked
> 
> 0001-crypto-add-virtio-crypto-driver.patch has no obvious style problems and is ready for submission.

Looks like a bug in checkpatch.pl:

# check for spinlock_t definitions without a comment.
if ($line =~ /^.\s*(struct\s+mutex|spinlock_t)\s+\S+;/ ||
    $line =~ /^.\s*(DEFINE_MUTEX)\s*\(/) {
        my $which = $1;
        if (!ctx_has_comment($first_line, $linenr)) {
                CHK("UNCOMMENTED_DEFINITION",
                    "$1 definition without comment\n" . $herecurr);
        }
}

Since your mutex definition has the 'static' keyword in front of it
checkpatch.pl misses it!

Andy: Is this checkpatch.pl behavior intentional?

Stefan

[toc] | [prev] | [next] | [standalone]

#1534040 — RE: [PATCH v4 1/1] crypto: add virtio-crypto driver

> 
> On Thu, Dec 01, 2016 at 02:27:19AM +0000, Gonglei (Arei) wrote:
> > > On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > > > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> > > b/drivers/crypto/virtio/virtio_crypto_algs.c
> > > > new file mode 100644
> > > > index 0000000..08b077f
> > > > --- /dev/null
> > > > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > > > @@ -0,0 +1,518 @@
> > > > + /* Algorithms supported by virtio crypto device
> > > > +  *
> > > > +  * Authors: Gonglei <arei.gonglei@huawei.com>
> > > > +  *
> > > > +  * Copyright 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > > > +  *
> > > > +  * This program is free software; you can redistribute it and/or modify
> > > > +  * it under the terms of the GNU General Public License as published by
> > > > +  * the Free Software Foundation; either version 2 of the License, or
> > > > +  * (at your option) any later version.
> > > > +  *
> > > > +  * This program is distributed in the hope that it will be useful,
> > > > +  * but WITHOUT ANY WARRANTY; without even the implied warranty
> of
> > > > +  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> the
> > > > +  * GNU General Public License for more details.
> > > > +  *
> > > > +  * You should have received a copy of the GNU General Public License
> > > > +  * along with this program; if not, see
> <http://www.gnu.org/licenses/>.
> > > > +  */
> > > > +
> > > > +#include <linux/scatterlist.h>
> > > > +#include <crypto/algapi.h>
> > > > +#include <linux/err.h>
> > > > +#include <crypto/scatterwalk.h>
> > > > +#include <linux/atomic.h>
> > > > +
> > > > +#include <uapi/linux/virtio_crypto.h>
> > > > +#include "virtio_crypto_common.h"
> > > > +
> > > > +static DEFINE_MUTEX(algs_lock);
> > >
> > > Did you run checkpatch.pl?  I think it encourages you to document what
> > > the lock protects.
> > >
> > Sure. Basically I run checkpatch.py each time. :)
> >
> > # ./scripts/checkpatch.pl 0001-crypto-add-virtio-crypto-driver.patch
> > total: 0 errors, 0 warnings, 1873 lines checked
> >
> > 0001-crypto-add-virtio-crypto-driver.patch has no obvious style problems and
> is ready for submission.
> 
> Looks like a bug in checkpatch.pl:
> 
> # check for spinlock_t definitions without a comment.
> if ($line =~ /^.\s*(struct\s+mutex|spinlock_t)\s+\S+;/ ||
>     $line =~ /^.\s*(DEFINE_MUTEX)\s*\(/) {
>         my $which = $1;
>         if (!ctx_has_comment($first_line, $linenr)) {
>                 CHK("UNCOMMENTED_DEFINITION",
>                     "$1 definition without comment\n" . $herecurr);
>         }
> }
> 
> Since your mutex definition has the 'static' keyword in front of it
> checkpatch.pl misses it!
> 
Anyway I added the comments. Thanks :)


Regards,
-Gonglei

> Andy: Is this checkpatch.pl behavior intentional?
> 
> Stefan

[toc] | [prev] | [next] | [standalone]

#1534004 — Re: [PATCH v4 1/1] crypto: add virtio-crypto driver

[Multipart message — attachments visible in raw view] — view raw

On Thu, Dec 01, 2016 at 02:27:19AM +0000, Gonglei (Arei) wrote:
> > On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > > +static int virtio_crypto_alg_ablkcipher_init_session(
> > > +		struct virtio_crypto_ablkcipher_ctx *ctx,
> > > +		uint32_t alg, const uint8_t *key,
> > > +		unsigned int keylen,
> > > +		int encrypt)
> > > +{
> > > +	struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> > > +	unsigned int tmp;
> > > +	struct virtio_crypto *vcrypto = ctx->vcrypto;
> > > +	int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> > VIRTIO_CRYPTO_OP_DECRYPT;
> > > +	int err;
> > > +	unsigned int num_out = 0, num_in = 0;
> > > +
> > > +	/*
> > > +	 * Avoid to do DMA from the stack, switch to using
> > > +	 * dynamically-allocated for the key
> > > +	 */
> > > +	uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> > > +
> > > +	if (!cipher_key)
> > > +		return -ENOMEM;
> > > +
> > > +	memcpy(cipher_key, key, keylen);
> > 
> > Are there any rules on handling key material in the kernel?  This buffer
> > is just kfreed later.  Do you need to zero it out before freeing it?
> > 
> Good questions. For kernel crypto core, each cipher request should be freed
> by skcipher_request_free(): zeroize and free request data structure.
> 
> I need to use kzfree() for key as well. I'll also check other stuffs. Thanks. 
> 
> > > +
> > > +	spin_lock(&vcrypto->ctrl_lock);
> > 
> > The QAT accelerator driver doesn't spin while talking to the device in
> > virtio_crypto_alg_ablkcipher_init_session().  I didn't find any other
> > driver examples in the kernel tree, but this function seems like a
> > weakness in the virtio-crypto device.
> > 
> The control queues of virtio-net and virtio-console are also be locked
> Please see:
>  __send_control_msg() in virtio_console.c and virtio-net's control queue
> protected by rtnl lock.
> 
> I didn't want to protect session creations but the virtqueue's operations
> like what other virtio devices do.
> 
> > While QEMU is servicing the create session command this vcpu is blocked.
> > The QEMU global mutex is held so no other vcpu can enter QEMU and the
> > QMP monitor is also blocked.
> > 
> > This is a scalability and performance problem.  Can you look at how QAT
> > avoids this synchronous session setup?
> 
> For QAT driver, the session creation is synchronous as well because it's a
> plain software operation which can be completed ASAP.

I'm mentioning the vmexit and wait for request completion in a spinlock
because the same type of issue has been a performance bottleneck with
virtio guest driver in the past.

If there is a way to avoid spinning then that would be preferred.  It's
basically a known anti-pattern for virtio guest drivers.

Could you initialize the session on the host side when the first
asynchronous data is submitted for encryption/decryption instead of
during init_session()?

Stefan

[toc] | [prev] | [next] | [standalone]

#1534017 — RE: [PATCH v4 1/1] crypto: add virtio-crypto driver

>
> Subject: Re: [PATCH v4 1/1] crypto: add virtio-crypto driver
> 
> On Thu, Dec 01, 2016 at 02:27:19AM +0000, Gonglei (Arei) wrote:
> > > On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > > > +static int virtio_crypto_alg_ablkcipher_init_session(
> > > > +		struct virtio_crypto_ablkcipher_ctx *ctx,
> > > > +		uint32_t alg, const uint8_t *key,
> > > > +		unsigned int keylen,
> > > > +		int encrypt)
> > > > +{
> > > > +	struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> > > > +	unsigned int tmp;
> > > > +	struct virtio_crypto *vcrypto = ctx->vcrypto;
> > > > +	int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> > > VIRTIO_CRYPTO_OP_DECRYPT;
> > > > +	int err;
> > > > +	unsigned int num_out = 0, num_in = 0;
> > > > +
> > > > +	/*
> > > > +	 * Avoid to do DMA from the stack, switch to using
> > > > +	 * dynamically-allocated for the key
> > > > +	 */
> > > > +	uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> > > > +
> > > > +	if (!cipher_key)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	memcpy(cipher_key, key, keylen);
> > >
> > > Are there any rules on handling key material in the kernel?  This buffer
> > > is just kfreed later.  Do you need to zero it out before freeing it?
> > >
> > Good questions. For kernel crypto core, each cipher request should be freed
> > by skcipher_request_free(): zeroize and free request data structure.
> >
> > I need to use kzfree() for key as well. I'll also check other stuffs. Thanks.
> >
> > > > +
> > > > +	spin_lock(&vcrypto->ctrl_lock);
> > >
> > > The QAT accelerator driver doesn't spin while talking to the device in
> > > virtio_crypto_alg_ablkcipher_init_session().  I didn't find any other
> > > driver examples in the kernel tree, but this function seems like a
> > > weakness in the virtio-crypto device.
> > >
> > The control queues of virtio-net and virtio-console are also be locked
> > Please see:
> >  __send_control_msg() in virtio_console.c and virtio-net's control queue
> > protected by rtnl lock.
> >
> > I didn't want to protect session creations but the virtqueue's operations
> > like what other virtio devices do.
> >
> > > While QEMU is servicing the create session command this vcpu is blocked.
> > > The QEMU global mutex is held so no other vcpu can enter QEMU and the
> > > QMP monitor is also blocked.
> > >
> > > This is a scalability and performance problem.  Can you look at how QAT
> > > avoids this synchronous session setup?
> >
> > For QAT driver, the session creation is synchronous as well because it's a
> > plain software operation which can be completed ASAP.
> 
> I'm mentioning the vmexit and wait for request completion in a spinlock
> because the same type of issue has been a performance bottleneck with
> virtio guest driver in the past.
> 
> If there is a way to avoid spinning then that would be preferred.  It's
> basically a known anti-pattern for virtio guest drivers.
> 
Oh, sorry for my misunderstanding. ;)

> Could you initialize the session on the host side when the first
> asynchronous data is submitted for encryption/decryption instead of
> during init_session()?
> 
I remember I discuss this problem with Alex two/three moths
ago. In some scenarios, its indeed a performance problem, such as each request has
different algorithms or keys in HTTP connections. It's performance will be better
if we just use one data virtqueue to pass session and data to the backend at one time.

But for the batch cipher operations with the same algorithm, the performance is poor,
Because we can't do batch operations for those requests. That's the great function
of session operations on the control virtqueue. Refer to the our clients requirements
and the existing QAT driver, the scenario is even more in NFV.

As your mention here, It's hard to do this IMO, because the backend can't know
the previous session belongs to which requests if we don't pass the session_id
to the backend.

Regards,
-Gonglei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web