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 at 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? -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 455 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20161130/e7b33198/attachment.sig>
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 at 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
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 at 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 455 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20161201/200f40de/attachment.sig>
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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 455 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20161201/31f0afb4/attachment-0001.sig>