Re: [Qemu-devel] [PATCH v3 04/10] cryptodev: introduce gcrypt lib as a new cryptodev backend

[Gonglei (Arei)]

> -Original Message-
> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Monday, September 19, 2016 4:56 PM
> Subject: Re: [PATCH v3 04/10] cryptodev: introduce gcrypt lib as a new
> cryptodev backend
> 
> On Mon, Sep 19, 2016 at 04:16:16PM +0800, Gonglei wrote:
> > Signed-off-by: Gonglei 
> > ---
> >  crypto/Makefile.objs  |   1 +
> >  crypto/cryptodev-gcrypt.c | 329
> ++
> >  qemu-options.hx   |  18 +++
> >  3 files changed, 348 insertions(+)
> >  create mode 100644 crypto/cryptodev-gcrypt.c
> >
> > diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
> > index f7f3c4f..bd8aea7 100644
> > --- a/crypto/Makefile.objs
> > +++ b/crypto/Makefile.objs
> > @@ -27,6 +27,7 @@ crypto-obj-y += block.o
> >  crypto-obj-y += block-qcow.o
> >  crypto-obj-y += block-luks.o
> >  crypto-obj-y += cryptodev.o
> > +crypto-obj-$(CONFIG_GCRYPT) += cryptodev-gcrypt.o
> 
> This can be just crypto-obj-y +=
> 
Yes.

> >  # Let the userspace emulators avoid linking gnutls/etc
> >  crypto-aes-obj-y = aes.o
> > diff --git a/crypto/cryptodev-gcrypt.c b/crypto/cryptodev-gcrypt.c
> > new file mode 100644
> > index 000..66a0e5e
> > --- /dev/null
> > +++ b/crypto/cryptodev-gcrypt.c
> > +/**
> > + * @TYPE_QCRYPTO_CRYPTODEV_BACKEND_GCRYPT:
> > + * name of backend that uses gcrypt library
> > + */
> > +#define TYPE_QCRYPTO_CRYPTODEV_BACKEND_GCRYPT
> "cryptodev-backend-gcrypt"
> 
> I'd suggest we just call this backend "builtin", so do a
> replace of "gcrypt" with "builtin" throughout.
> 
OK, good name ;)

> > +static void qcrypto_cryptodev_backend_gcrypt_init(
> > + QCryptoCryptoDevBackend *backend, Error **errp)
> > +{
> > +/* Only support one queue */
> > +int queues = MAX(backend->conf.peers.queues, 1);
> > +int i;
> 
> Nitpick, I prefer to see 'size_t' for list iterators
> that are always positive. Similar comment in other
> places in this series using int i
> 
OK

> > +QCryptoCryptoDevBackendClientState *cc;
> > +
> > +for (i = 0; i < queues; i++) {
> > +cc = qcrypto_cryptodev_backend_new_client(
> > +  "cryptodev-gcrypt", NULL);
> > +snprintf(cc->info_str, sizeof(cc->info_str),
> > + "cryptodev-gcrypt%d", i);
> > +cc->queue_index = i;
> > +
> > +backend->conf.peers.ccs[i] = cc;
> > +}
> > +
> > +backend->conf.crypto_services =
> > + 1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
> > + 1u << VIRTIO_CRYPTO_SERVICE_HASH |
> > + 1u << VIRTIO_CRYPTO_SERVICE_MAC;
> > +backend->conf.cipher_algo_l = 1u <<
> VIRTIO_CRYPTO_CIPHER_AES_CBC;
> > +backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> > +}
> > +
> > +static int
> > +qcrypto_cryptodev_backend_gcrypt_get_unused_session_index(
> > +  QCryptoCryptoDevBackendGcrypt *gcrypt)
> > +{
> > +int i;
> > +
> > +for (i = 0; i < MAX_NUM_SESSIONS; i++) {
> > +if (gcrypt->sessions[i] == NULL) {
> > +return i;
> > +}
> > +}
> > +
> > +return -1;
> > +}
> > +
> > +static int qcrypto_cryptodev_backend_gcrypt_create_cipher_session(
> > +QCryptoCryptoDevBackendGcrypt *gcrypt,
> > +QCryptoCryptoDevBackendSymSessionInfo
> *sess_info,
> > +Error **errp)
> > +{
> > +int algo;
> > +int mode;
> > +QCryptoCipher *cipher;
> > +int index;
> > +QCryptoCryptoDevBackendGcryptSession *sess;
> > +
> > +if (sess_info->op_type != VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> > +error_setg(errp, "unsupported optype :%u", sess_info->op_type);
> > +return -1;
> > +}
> > +
> > +index =
> qcrypto_cryptodev_backend_gcrypt_get_unused_session_index(gcrypt);
> > +if (index < 0) {
> > +error_setg(errp, "the total number of created session
> exceed %u",
> > +  MAX_NUM_SESSIONS);
> > +return -1;
> > +}
> > +
> > +switch (sess_info->cipher_alg) {
> > +case VIRTIO_CRYPTO_CIPHER_AES_ECB:
> > +if (sess_info->key_len == 128 / 8) {
> > +algo = QCRYPTO_CIPHER_ALG_AES_128;
> > +} else if (sess_info->key_len == 192 / 8) {
> > +algo = QCRYPTO_CIPHER_ALG_AES_192;
> > +} else if (sess_info->key_len == 256 / 8) {
> > +algo = QCRYPTO_CIPHER_ALG_AES_256;
> > +} else {
> > +error_setg(errp, "unsupported key length :%u",
> > +   sess_info->key_len);
> > +return -1;
> > +}
> > +mode = QCRYPTO_CIPHER_MODE_ECB;
> > +break;
> > +case VIRTIO_CRYPTO_CIPHER_AES_CBC:
> > +if (sess_info->key_len == 128 / 8) {
> > +algo = QCRYPTO_CIPHER_ALG_AES_128;
> > +} else if (sess_info->key_len == 192 / 8) {
> > +algo = QCRYPTO_CIPHER_ALG_AES_192;
> > +} else if (sess_info->key_len == 256 / 8) {
> > +

Re: [Qemu-devel] [PATCH v3 04/10] cryptodev: introduce gcrypt lib as a new cryptodev backend

[Daniel P. Berrange]

On Mon, Sep 19, 2016 at 04:16:16PM +0800, Gonglei wrote:
> Signed-off-by: Gonglei 
> ---
>  crypto/Makefile.objs  |   1 +
>  crypto/cryptodev-gcrypt.c | 329 
> ++
>  qemu-options.hx   |  18 +++
>  3 files changed, 348 insertions(+)
>  create mode 100644 crypto/cryptodev-gcrypt.c
> 
> diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
> index f7f3c4f..bd8aea7 100644
> --- a/crypto/Makefile.objs
> +++ b/crypto/Makefile.objs
> @@ -27,6 +27,7 @@ crypto-obj-y += block.o
>  crypto-obj-y += block-qcow.o
>  crypto-obj-y += block-luks.o
>  crypto-obj-y += cryptodev.o
> +crypto-obj-$(CONFIG_GCRYPT) += cryptodev-gcrypt.o

This can be just crypto-obj-y +=

>  # Let the userspace emulators avoid linking gnutls/etc
>  crypto-aes-obj-y = aes.o
> diff --git a/crypto/cryptodev-gcrypt.c b/crypto/cryptodev-gcrypt.c
> new file mode 100644
> index 000..66a0e5e
> --- /dev/null
> +++ b/crypto/cryptodev-gcrypt.c
> +/**
> + * @TYPE_QCRYPTO_CRYPTODEV_BACKEND_GCRYPT:
> + * name of backend that uses gcrypt library
> + */
> +#define TYPE_QCRYPTO_CRYPTODEV_BACKEND_GCRYPT "cryptodev-backend-gcrypt"

I'd suggest we just call this backend "builtin", so do a
replace of "gcrypt" with "builtin" throughout.

> +static void qcrypto_cryptodev_backend_gcrypt_init(
> + QCryptoCryptoDevBackend *backend, Error **errp)
> +{
> +/* Only support one queue */
> +int queues = MAX(backend->conf.peers.queues, 1);
> +int i;

Nitpick, I prefer to see 'size_t' for list iterators
that are always positive. Similar comment in other
places in this series using int i

> +QCryptoCryptoDevBackendClientState *cc;
> +
> +for (i = 0; i < queues; i++) {
> +cc = qcrypto_cryptodev_backend_new_client(
> +  "cryptodev-gcrypt", NULL);
> +snprintf(cc->info_str, sizeof(cc->info_str),
> + "cryptodev-gcrypt%d", i);
> +cc->queue_index = i;
> +
> +backend->conf.peers.ccs[i] = cc;
> +}
> +
> +backend->conf.crypto_services =
> + 1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
> + 1u << VIRTIO_CRYPTO_SERVICE_HASH |
> + 1u << VIRTIO_CRYPTO_SERVICE_MAC;
> +backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
> +backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> +}
> +
> +static int
> +qcrypto_cryptodev_backend_gcrypt_get_unused_session_index(
> +  QCryptoCryptoDevBackendGcrypt *gcrypt)
> +{
> +int i;
> +
> +for (i = 0; i < MAX_NUM_SESSIONS; i++) {
> +if (gcrypt->sessions[i] == NULL) {
> +return i;
> +}
> +}
> +
> +return -1;
> +}
> +
> +static int qcrypto_cryptodev_backend_gcrypt_create_cipher_session(
> +QCryptoCryptoDevBackendGcrypt *gcrypt,
> +QCryptoCryptoDevBackendSymSessionInfo *sess_info,
> +Error **errp)
> +{
> +int algo;
> +int mode;
> +QCryptoCipher *cipher;
> +int index;
> +QCryptoCryptoDevBackendGcryptSession *sess;
> +
> +if (sess_info->op_type != VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> +error_setg(errp, "unsupported optype :%u", sess_info->op_type);
> +return -1;
> +}
> +
> +index = 
> qcrypto_cryptodev_backend_gcrypt_get_unused_session_index(gcrypt);
> +if (index < 0) {
> +error_setg(errp, "the total number of created session exceed %u",
> +  MAX_NUM_SESSIONS);
> +return -1;
> +}
> +
> +switch (sess_info->cipher_alg) {
> +case VIRTIO_CRYPTO_CIPHER_AES_ECB:
> +if (sess_info->key_len == 128 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_128;
> +} else if (sess_info->key_len == 192 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_192;
> +} else if (sess_info->key_len == 256 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_256;
> +} else {
> +error_setg(errp, "unsupported key length :%u",
> +   sess_info->key_len);
> +return -1;
> +}
> +mode = QCRYPTO_CIPHER_MODE_ECB;
> +break;
> +case VIRTIO_CRYPTO_CIPHER_AES_CBC:
> +if (sess_info->key_len == 128 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_128;
> +} else if (sess_info->key_len == 192 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_192;
> +} else if (sess_info->key_len == 256 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_256;
> +} else {
> +error_setg(errp, "unsupported key length :%u",
> +   sess_info->key_len);
> +return -1;
> +}
> +mode = QCRYPTO_CIPHER_MODE_CBC;
> +break;
> +case VIRTIO_CRYPTO_CIPHER_AES_CTR:

Although the QEMU cipher.h API does not export CTR mode currently
it should be trivial to add it. So feel free to add a patch at
the start of the series implementing CTR mode in the cipher API.
Both gcrypt 

Re: [Qemu-devel] [PATCH v3 04/10] cryptodev: introduce gcrypt lib as a new cryptodev backend

[Gonglei (Arei)]

Hi Daniel,


> -Original Message-
> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Monday, September 19, 2016 4:30 PM
> Subject: Re: [PATCH v3 04/10] cryptodev: introduce gcrypt lib as a new
> cryptodev backend
> 
> AFAICT you are not using gcrypt here - you're using QEMU
> cipher APIs (which is good). These APIs can be backed by
> either nettle or gcrypt though, so the subject is misleading.
> 
Oops, thanks for your reminding :)

Will fix in the next version.


Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v3 04/10] cryptodev: introduce gcrypt lib as a new cryptodev backend

[Daniel P. Berrange]

AFAICT you are not using gcrypt here - you're using QEMU
cipher APIs (which is good). These APIs can be backed by
either nettle or gcrypt though, so the subject is misleading.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

[Qemu-devel] [PATCH v3 04/10] cryptodev: introduce gcrypt lib as a new cryptodev backend

[Gonglei]

Signed-off-by: Gonglei 
---
 crypto/Makefile.objs  |   1 +
 crypto/cryptodev-gcrypt.c | 329 ++
 qemu-options.hx   |  18 +++
 3 files changed, 348 insertions(+)
 create mode 100644 crypto/cryptodev-gcrypt.c

diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index f7f3c4f..bd8aea7 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -27,6 +27,7 @@ crypto-obj-y += block.o
 crypto-obj-y += block-qcow.o
 crypto-obj-y += block-luks.o
 crypto-obj-y += cryptodev.o
+crypto-obj-$(CONFIG_GCRYPT) += cryptodev-gcrypt.o
 
 # Let the userspace emulators avoid linking gnutls/etc
 crypto-aes-obj-y = aes.o
diff --git a/crypto/cryptodev-gcrypt.c b/crypto/cryptodev-gcrypt.c
new file mode 100644
index 000..66a0e5e
--- /dev/null
+++ b/crypto/cryptodev-gcrypt.c
@@ -0,0 +1,329 @@
+/*
+ * QEMU Cryptodev backend for gcrypt
+ *
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ * Authors:
+ *Gonglei 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "crypto/cryptodev.h"
+#include "hw/boards.h"
+#include "qapi/error.h"
+#include "standard-headers/linux/virtio_crypto.h"
+#include "crypto/cipher.h"
+
+
+/**
+ * @TYPE_QCRYPTO_CRYPTODEV_BACKEND_GCRYPT:
+ * name of backend that uses gcrypt library
+ */
+#define TYPE_QCRYPTO_CRYPTODEV_BACKEND_GCRYPT "cryptodev-backend-gcrypt"
+
+#define QCRYPTO_CRYPTODEV_BACKEND_GCRYPT(obj) \
+OBJECT_CHECK(QCryptoCryptoDevBackendGcrypt, \
+ (obj), TYPE_QCRYPTO_CRYPTODEV_BACKEND_GCRYPT)
+
+typedef struct QCryptoCryptoDevBackendGcrypt
+ QCryptoCryptoDevBackendGcrypt;
+
+typedef struct QCryptoCryptoDevBackendGcryptSession {
+QCryptoCipher *cipher;
+uint8_t direction; /* encryption or decryption */
+uint8_t type; /* cipher? hash? aead? */
+QTAILQ_ENTRY(QCryptoCryptoDevBackendGcryptSession) next;
+} QCryptoCryptoDevBackendGcryptSession;
+
+/* Max number of symetrical sessions */
+#define MAX_NUM_SESSIONS 256
+
+
+struct QCryptoCryptoDevBackendGcrypt {
+QCryptoCryptoDevBackend parent_obj;
+
+QCryptoCryptoDevBackendGcryptSession *sessions[MAX_NUM_SESSIONS];
+};
+
+static void qcrypto_cryptodev_backend_gcrypt_init(
+ QCryptoCryptoDevBackend *backend, Error **errp)
+{
+/* Only support one queue */
+int queues = MAX(backend->conf.peers.queues, 1);
+int i;
+QCryptoCryptoDevBackendClientState *cc;
+
+for (i = 0; i < queues; i++) {
+cc = qcrypto_cryptodev_backend_new_client(
+  "cryptodev-gcrypt", NULL);
+snprintf(cc->info_str, sizeof(cc->info_str),
+ "cryptodev-gcrypt%d", i);
+cc->queue_index = i;
+
+backend->conf.peers.ccs[i] = cc;
+}
+
+backend->conf.crypto_services =
+ 1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
+ 1u << VIRTIO_CRYPTO_SERVICE_HASH |
+ 1u << VIRTIO_CRYPTO_SERVICE_MAC;
+backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
+backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
+}
+
+static int
+qcrypto_cryptodev_backend_gcrypt_get_unused_session_index(
+  QCryptoCryptoDevBackendGcrypt *gcrypt)
+{
+int i;
+
+for (i = 0; i < MAX_NUM_SESSIONS; i++) {
+if (gcrypt->sessions[i] == NULL) {
+return i;
+}
+}
+
+return -1;
+}
+
+static int qcrypto_cryptodev_backend_gcrypt_create_cipher_session(
+QCryptoCryptoDevBackendGcrypt *gcrypt,
+QCryptoCryptoDevBackendSymSessionInfo *sess_info,
+Error **errp)
+{
+int algo;
+int mode;
+QCryptoCipher *cipher;
+int index;
+QCryptoCryptoDevBackendGcryptSession *sess;
+
+if (sess_info->op_type != VIRTIO_CRYPTO_SYM_OP_CIPHER) {
+error_setg(errp, "unsupported optype :%u", sess_info->op_type);
+return -1;
+}
+
+index = qcrypto_cryptodev_backend_gcrypt_get_unused_session_index(gcrypt);
+if (index < 0) {
+error_setg(errp, "the total number of created session exceed %u",
+  MAX_NUM_SESSIONS);
+return -1;
+}
+
+switch (sess_info->cipher_alg) {
+case VIRTIO_CRYPTO_CIPHER_AES_ECB:
+if (sess_info->key_len == 128 / 8) {
+