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

Groups > linux.kernel > #1541871 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v7 0/1] virtio-crypto: add Linux driver Gonglei <arei.gonglei@huawei.com> - 2016-12-14 13:00 +0100
    Re: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver Halil Pasic <pasic@linux.vnet.ibm.com> - 2016-12-14 16:40 +0100
      RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-12-15 01:50 +0100
        RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver "Zeng, Xin" <xin.zeng@intel.com> - 2016-12-15 02:00 +0100
          RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-12-15 02:10 +0100
            Re: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-15 17:50 +0100
              RE: [virtio-dev] Re: [Qemu-devel] [PATCH v7 1/1] crypto: add  virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-12-16 01:50 +0100

#1541871 — [PATCH v7 0/1] virtio-crypto: add Linux driver

v7:
 - fix "BUG: smp_processor_id() in preemptible [00000000] code" reported by Halil,
   using get_cpu/put_cpu instead of calling smp_processor_id() directly.
 - fix a possible spinlock recursion in virtcrypto_dataq_callback(), we should
   release the spinlock before invoking the callback.
 - rebase on the latest kernel master tree.

v6:
 - add patch 1/2 to make sparc architecture happy. [Sam]
 - close created sessions previousely when rekeying.
 - convert the priority of virtio crypto algs from 4001 to 501
   which is enough.

v5:
 - add comments for algs_lock and table_lock. [Stefan]
 - use kzfree instead of kfree for key material security. [Stefan]
 - drop unnecessary spin_lock for struct virtio_crypto_ablkcipher_ctx.
 - dynamically allocated memory for iv in order to avoid to do DMA from
   the stack memory in __virtio_crypto_ablkcipher_do_req().
 - add logs for error path in virtio_crypto_alg_validate_key().
 - add lock before calling virtio_break_device() in virtcrypto_update_status()

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   | 541 +++++++++++++++++++++++++++
 drivers/crypto/virtio/virtio_crypto_common.h | 128 +++++++
 drivers/crypto/virtio/virtio_crypto_core.c   | 474 +++++++++++++++++++++++
 drivers/crypto/virtio/virtio_crypto_mgr.c    | 264 +++++++++++++
 include/uapi/linux/Kbuild                    |   1 +
 include/uapi/linux/virtio_crypto.h           | 450 ++++++++++++++++++++++
 include/uapi/linux/virtio_ids.h              |   1 +
 12 files changed, 1886 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]

#1541989 — Re: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

On 12/14/2016 12:50 PM, Gonglei wrote:
> diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
> new file mode 100644
> index 0000000..c0854a1
> --- /dev/null
> +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> @@ -0,0 +1,474 @@
[..]
> +
> +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;
> +	struct ablkcipher_request *ablk_req;
> +	int error;
> +
> +	spin_lock_irqsave(&vcrypto->lock, flags);

Would it make sense to use a per virtqueue lock
like in virtio_blk for example instead of locking on the whole
device? OK, it seems you use only one dataqueue, so it
may not be that relevant.

> +	do {
> +		virtqueue_disable_cb(vq);
> +		while ((vc_req = virtqueue_get_buf(vq, &len)) != NULL) {
> +			if (vc_req->type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> +				switch (vc_req->status) {
> +				case VIRTIO_CRYPTO_OK:
> +					error = 0;
> +					break;
> +				case VIRTIO_CRYPTO_INVSESS:
> +				case VIRTIO_CRYPTO_ERR:
> +					error = -EINVAL;
> +					break;
> +				case VIRTIO_CRYPTO_BADMSG:
> +					error = -EBADMSG;
> +					break;
> +				default:
> +					error = -EIO;
> +					break;
> +				}
> +				ablk_req = vc_req->ablkcipher_req;
> +				virtcrypto_clear_request(vc_req);
> +
> +				spin_unlock_irqrestore(&vcrypto->lock, flags);
> +				/* Finish the encrypt or decrypt process */
> +				ablk_req->base.complete(&ablk_req->base, error);
> +				spin_lock_irqsave(&vcrypto->lock, flags);
> +			}
> +		}
> +	} while (!virtqueue_enable_cb(vq));
> +	spin_unlock_irqrestore(&vcrypto->lock, flags);
> +}

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

#1542409 — RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

> 
> 
> On 12/14/2016 12:50 PM, Gonglei wrote:
> > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> b/drivers/crypto/virtio/virtio_crypto_core.c
> > new file mode 100644
> > index 0000000..c0854a1
> > --- /dev/null
> > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> > @@ -0,0 +1,474 @@
> [..]
> > +
> > +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;
> > +	struct ablkcipher_request *ablk_req;
> > +	int error;
> > +
> > +	spin_lock_irqsave(&vcrypto->lock, flags);
> 
> Would it make sense to use a per virtqueue lock
> like in virtio_blk for example instead of locking on the whole
> device? OK, it seems you use only one dataqueue, so it
> may not be that relevant.
> 
Currently yes, both the backend device (cryptodev-backend-builtin)
and the frontend driver use one dataqueue.

Regards,
-Gonglei

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

#1542417 — RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

On Thursday, December 15, 2016 8:45 AM, Gonglei (Arei) Wrote:
< > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
< > b/drivers/crypto/virtio/virtio_crypto_core.c
< > > new file mode 100644
< > > index 0000000..c0854a1
< > > --- /dev/null
< > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
< > > @@ -0,0 +1,474 @@
< > [..]
< > > +
< > > +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;
< > > +	struct ablkcipher_request *ablk_req;
< > > +	int error;
< > > +
< > > +	spin_lock_irqsave(&vcrypto->lock, flags);
< >
< > Would it make sense to use a per virtqueue lock
< > like in virtio_blk for example instead of locking on the whole
< > device? OK, it seems you use only one dataqueue, so it
< > may not be that relevant.
< >
< Currently yes, both the backend device (cryptodev-backend-builtin)
< and the frontend driver use one dataqueue.
< 

I think it makes sense to use per virtqueue lock here though it only uses one queue so far,
but in the spec we already have multi queues support.

< Regards,
< -Gonglei

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

#1542421 — RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

Regards,
-Gonglei


> -----Original Message-----
> From: Zeng, Xin [mailto:xin.zeng@intel.com]
> Sent: Thursday, December 15, 2016 8:59 AM
> To: Gonglei (Arei); Halil Pasic; linux-kernel@vger.kernel.org;
> qemu-devel@nongnu.org; virtio-dev@lists.oasis-open.org;
> virtualization@lists.linux-foundation.org; linux-crypto@vger.kernel.org
> Cc: Huangweidong (C); Claudio Fontana; mst@redhat.com; Luonengjun;
> Hanweidong (Randy); Xuquan (Quan Xu); Wanzongshun (Vincent);
> stefanha@redhat.com; Zhoujian (jay, Euler); cornelia.huck@de.ibm.com;
> longpeng; arei.gonglei@hotmail.com; davem@davemloft.net; Wubin (H);
> herbert@gondor.apana.org.au
> Subject: RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver
> 
> On Thursday, December 15, 2016 8:45 AM, Gonglei (Arei) Wrote:
> < > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> < > b/drivers/crypto/virtio/virtio_crypto_core.c
> < > > new file mode 100644
> < > > index 0000000..c0854a1
> < > > --- /dev/null
> < > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> < > > @@ -0,0 +1,474 @@
> < > [..]
> < > > +
> < > > +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;
> < > > +	struct ablkcipher_request *ablk_req;
> < > > +	int error;
> < > > +
> < > > +	spin_lock_irqsave(&vcrypto->lock, flags);
> < >
> < > Would it make sense to use a per virtqueue lock
> < > like in virtio_blk for example instead of locking on the whole
> < > device? OK, it seems you use only one dataqueue, so it
> < > may not be that relevant.
> < >
> < Currently yes, both the backend device (cryptodev-backend-builtin)
> < and the frontend driver use one dataqueue.
> <
> 
> I think it makes sense to use per virtqueue lock here though it only uses one
> queue so far,
> but in the spec we already have multi queues support.
> 
Yes, I agree. Will do that in V8 soon. 
Hope to catch up with Michael's pull request for 4.10.

Regards,
-Gonglei

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

#1542882 — Re: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

On Thu, Dec 15, 2016 at 01:08:51AM +0000, Gonglei (Arei) wrote:
> 
> 
> 
> 
> Regards,
> -Gonglei
> 
> 
> > -----Original Message-----
> > From: Zeng, Xin [mailto:xin.zeng@intel.com]
> > Sent: Thursday, December 15, 2016 8:59 AM
> > To: Gonglei (Arei); Halil Pasic; linux-kernel@vger.kernel.org;
> > qemu-devel@nongnu.org; virtio-dev@lists.oasis-open.org;
> > virtualization@lists.linux-foundation.org; linux-crypto@vger.kernel.org
> > Cc: Huangweidong (C); Claudio Fontana; mst@redhat.com; Luonengjun;
> > Hanweidong (Randy); Xuquan (Quan Xu); Wanzongshun (Vincent);
> > stefanha@redhat.com; Zhoujian (jay, Euler); cornelia.huck@de.ibm.com;
> > longpeng; arei.gonglei@hotmail.com; davem@davemloft.net; Wubin (H);
> > herbert@gondor.apana.org.au
> > Subject: RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver
> > 
> > On Thursday, December 15, 2016 8:45 AM, Gonglei (Arei) Wrote:
> > < > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> > < > b/drivers/crypto/virtio/virtio_crypto_core.c
> > < > > new file mode 100644
> > < > > index 0000000..c0854a1
> > < > > --- /dev/null
> > < > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> > < > > @@ -0,0 +1,474 @@
> > < > [..]
> > < > > +
> > < > > +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;
> > < > > +	struct ablkcipher_request *ablk_req;
> > < > > +	int error;
> > < > > +
> > < > > +	spin_lock_irqsave(&vcrypto->lock, flags);
> > < >
> > < > Would it make sense to use a per virtqueue lock
> > < > like in virtio_blk for example instead of locking on the whole
> > < > device? OK, it seems you use only one dataqueue, so it
> > < > may not be that relevant.
> > < >
> > < Currently yes, both the backend device (cryptodev-backend-builtin)
> > < and the frontend driver use one dataqueue.
> > <
> > 
> > I think it makes sense to use per virtqueue lock here though it only uses one
> > queue so far,
> > but in the spec we already have multi queues support.
> > 
> Yes, I agree. Will do that in V8 soon. 
> Hope to catch up with Michael's pull request for 4.10.
> 
> Regards,
> -Gonglei

I merged v7, this change will have to wait. Sorry.

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

#1543150 — RE: [virtio-dev] Re: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

Hi Michael,

>
> >
> >
> > > Subject: RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver
> > >
> > > On Thursday, December 15, 2016 8:45 AM, Gonglei (Arei) Wrote:
> > > < > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> > > < > b/drivers/crypto/virtio/virtio_crypto_core.c
> > > < > > new file mode 100644
> > > < > > index 0000000..c0854a1
> > > < > > --- /dev/null
> > > < > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> > > < > > @@ -0,0 +1,474 @@
> > > < > [..]
> > > < > > +
> > > < > > +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;
> > > < > > +	struct ablkcipher_request *ablk_req;
> > > < > > +	int error;
> > > < > > +
> > > < > > +	spin_lock_irqsave(&vcrypto->lock, flags);
> > > < >
> > > < > Would it make sense to use a per virtqueue lock
> > > < > like in virtio_blk for example instead of locking on the whole
> > > < > device? OK, it seems you use only one dataqueue, so it
> > > < > may not be that relevant.
> > > < >
> > > < Currently yes, both the backend device (cryptodev-backend-builtin)
> > > < and the frontend driver use one dataqueue.
> > > <
> > >
> > > I think it makes sense to use per virtqueue lock here though it only uses one
> > > queue so far,
> > > but in the spec we already have multi queues support.
> > >
> > Yes, I agree. Will do that in V8 soon.
> > Hope to catch up with Michael's pull request for 4.10.
> >
> > Regards,
> > -Gonglei
> 
> I merged v7, this change will have to wait. Sorry.
> 
That's OK. Thanks!

I can post a separate patch after this pull request. 

Regards,
-Gonglei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web