[Qemu-devel] [RFC v1 0/9] virtio-crypto: add stateless mode support

[Qemu-devel] [RFC v1 0/9] virtio-crypto: add stateless mode support

[Gonglei]

In session mode, the process of create/close a session
makes we have a least one full round-trip cost from guest to host to guest
to be able to send any data for symmetric algorithms. It gets ourself into
synchronization troubles in some scenarios like a web server handling lots
of small requests whose algorithms and keys are different.

We can support one-blob request (no sessions) as well for symmetric
algorithms, including HASH, MAC services. The benefit is obvious for
HASH service because it's usually a one-blob operation.

The code realization is based on the latest virtio crypto spec:

[PATCH v18 0/2] virtio-crypto: virtio crypto device specification
  https://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg03860.html

Patch 1~4 introduce stateless symmetric crypto operations stuff for
stateless mode in cryptodev backend framework.

Patch 5~8 introduce stateless operation for virtio crypto device and
add 5 feature bits to support it.

Patch 9 is a experimental patch for testing the new stateless functions, so
that people don't need to compile a new linux driver to test it (I havn't
realize the driver yet, It's not a big deal). Of cource the patch
can't be upsteamed until the qtest framework supports virtio 1.0 or latter.

Gonglei (9):
  cryptodev: introduce stateless sym operation stuff
  cryptodev: extract one util function
  cryptodev: add missing op_code for symmertric crypto
  cryptodev-builtin: realize stateless operation function
  virtio-crypto: update header file
  virtio-crypto: rework virtio_crypto_handle_request
  virtio-crypto: add stateless crypto request handler
  virtio-crypto: add host feature bits support
  qtest: emulate virtio crypto as a legacy device for experiment

 backends/cryptodev-builtin.c                   | 189 ++++++--
 backends/cryptodev.c                           |  21 +
 docs/specs/pci-ids.txt                         |   2 +
 hw/virtio/virtio-crypto-pci.c                  |   4 +-
 hw/virtio/virtio-crypto.c                      | 340 +++++++++++++-
 include/hw/pci/pci.h                           |   2 +
 include/hw/virtio/virtio-crypto.h              |   2 +
 include/standard-headers/linux/virtio_crypto.h | 210 ++++++++-
 include/sysemu/cryptodev.h                     |  22 +
 tests/Makefile.include                         |   3 +
 tests/virtio-crypto-test.c                     | 596 +++++++++++++++++++++++++
 11 files changed, 1332 insertions(+), 59 deletions(-)
 create mode 100644 tests/virtio-crypto-test.c

-- 
1.8.3.1

[Qemu-devel] [RFC v1 1/9] cryptodev: introduce stateless sym operation stuff

[Gonglei]

Because a stateless crypto request include all
information about one crypto operation, we should
introduce a wrapper for it, and add a new crypto
operation callback to do statless crypto operation.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 backends/cryptodev-builtin.c      | 10 ++++++++++
 backends/cryptodev.c              | 21 +++++++++++++++++++++
 include/hw/virtio/virtio-crypto.h |  1 +
 include/sysemu/cryptodev.h        | 19 +++++++++++++++++++
 4 files changed, 51 insertions(+)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index 657c0ba..1fa2cab 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -372,6 +372,15 @@ static void cryptodev_builtin_cleanup(
     cryptodev_backend_set_ready(backend, false);
 }
 
+static int
+cryptodev_builtin_sym_stateless_operation(
+                 CryptoDevBackend *backend,
+                 CryptoDevBackendSymStatelessInfo *op_info,
+                 uint32_t queue_index, Error **errp)
+{
+    return -VIRTIO_CRYPTO_ERR;
+}
+
 static void
 cryptodev_builtin_class_init(ObjectClass *oc, void *data)
 {
@@ -382,6 +391,7 @@ cryptodev_builtin_class_init(ObjectClass *oc, void *data)
     bc->create_session = cryptodev_builtin_sym_create_session;
     bc->close_session = cryptodev_builtin_sym_close_session;
     bc->do_sym_op = cryptodev_builtin_sym_operation;
+    bc->do_sym_stateless_op = cryptodev_builtin_sym_stateless_operation;
 }
 
 static const TypeInfo cryptodev_builtin_info = {
diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index 832f056..c6d7c5f 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -120,6 +120,21 @@ static int cryptodev_backend_sym_operation(
     return -VIRTIO_CRYPTO_ERR;
 }
 
+static int cryptodev_backend_sym_stateless_operation(
+                 CryptoDevBackend *backend,
+                 CryptoDevBackendSymStatelessInfo *op_info,
+                 uint32_t queue_index, Error **errp)
+{
+    CryptoDevBackendClass *bc =
+                      CRYPTODEV_BACKEND_GET_CLASS(backend);
+
+    if (bc->do_sym_stateless_op) {
+        return bc->do_sym_stateless_op(backend, op_info, queue_index, errp);
+    }
+
+    return -VIRTIO_CRYPTO_ERR;
+}
+
 int cryptodev_backend_crypto_operation(
                  CryptoDevBackend *backend,
                  void *opaque,
@@ -133,6 +148,12 @@ int cryptodev_backend_crypto_operation(
 
         return cryptodev_backend_sym_operation(backend,
                          op_info, queue_index, errp);
+    } else if (req->flags == CRYPTODEV_BACKEND_ALG_SYM_STATELESS) {
+        CryptoDevBackendSymStatelessInfo *op_info;
+        op_info = req->u.sym_stateless_info;
+
+        return cryptodev_backend_sym_stateless_operation(backend,
+                         op_info, queue_index, errp);
     } else {
         error_setg(errp, "Unsupported cryptodev alg type: %" PRIu32 "",
                    req->flags);
diff --git a/include/hw/virtio/virtio-crypto.h b/include/hw/virtio/virtio-crypto.h
index a00a0bf..465ad20 100644
--- a/include/hw/virtio/virtio-crypto.h
+++ b/include/hw/virtio/virtio-crypto.h
@@ -73,6 +73,7 @@ typedef struct VirtIOCryptoReq {
     struct VirtIOCrypto *vcrypto;
     union {
         CryptoDevBackendSymOpInfo *sym_op_info;
+        CryptoDevBackendSymStatelessInfo *sym_stateless_info;
     } u;
 } VirtIOCryptoReq;
 
diff --git a/include/sysemu/cryptodev.h b/include/sysemu/cryptodev.h
index a9d0d1e..be507f7 100644
--- a/include/sysemu/cryptodev.h
+++ b/include/sysemu/cryptodev.h
@@ -58,6 +58,7 @@ typedef struct CryptoDevBackend CryptoDevBackend;
 
 enum CryptoDevBackendAlgType {
     CRYPTODEV_BACKEND_ALG_SYM,
+    CRYPTODEV_BACKEND_ALG_SYM_STATELESS,
     CRYPTODEV_BACKEND_ALG__MAX,
 };
 
@@ -146,6 +147,21 @@ typedef struct CryptoDevBackendSymOpInfo {
     uint8_t data[0];
 } CryptoDevBackendSymOpInfo;
 
+/**
+ * CryptoDevBackendSymStatelessInfo:
+ *
+ * @session_info: session information, see above
+ *   CryptoDevBackendSymSessionInfo
+ * @op_info: crypto operation information, see above
+ *   CryptoDevBackendSymOpInfo, @session_id is ignored
+ *
+ */
+typedef struct CryptoDevBackendSymStatelessInfo {
+    CryptoDevBackendSymSessionInfo session_info;
+    CryptoDevBackendSymOpInfo op_info;
+} CryptoDevBackendSymStatelessInfo;
+
+
 typedef struct CryptoDevBackendClass {
     ObjectClass parent_class;
 
@@ -161,6 +177,9 @@ typedef struct CryptoDevBackendClass {
     int (*do_sym_op)(CryptoDevBackend *backend,
                      CryptoDevBackendSymOpInfo *op_info,
                      uint32_t queue_index, Error **errp);
+    int (*do_sym_stateless_op)(CryptoDevBackend *backend,
+                     CryptoDevBackendSymStatelessInfo *op_info,
+                     uint32_t queue_index, Error **errp);
 } CryptoDevBackendClass;
 
 
-- 
1.8.3.1

[Qemu-devel] [RFC v1 2/9] cryptodev: extract one util function

[Gonglei]

So that the new function can be used by both seesion creation
and the following stateless crypto operation.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 backends/cryptodev-builtin.c | 100 +++++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 42 deletions(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index 1fa2cab..abc2655 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -151,73 +151,56 @@ err:
    return -1;
 }
 
-static int cryptodev_builtin_create_cipher_session(
-                    CryptoDevBackendBuiltin *builtin,
+static int
+cryptodev_builtin_get_cipher_alg_mode(
                     CryptoDevBackendSymSessionInfo *sess_info,
+                    int *algo, int *mode,
                     Error **errp)
 {
-    int algo;
-    int mode;
-    QCryptoCipher *cipher;
-    int index;
-    CryptoDevBackendBuiltinSession *sess;
-
-    if (sess_info->op_type != VIRTIO_CRYPTO_SYM_OP_CIPHER) {
-        error_setg(errp, "Unsupported optype :%u", sess_info->op_type);
-        return -1;
-    }
-
-    index = cryptodev_builtin_get_unused_session_index(builtin);
-    if (index < 0) {
-        error_setg(errp, "Total number of sessions created exceeds %u",
-                  MAX_NUM_SESSIONS);
-        return -1;
-    }
-
     switch (sess_info->cipher_alg) {
     case VIRTIO_CRYPTO_CIPHER_AES_ECB:
-        mode = QCRYPTO_CIPHER_MODE_ECB;
-        algo = cryptodev_builtin_get_aes_algo(sess_info->key_len,
-                                                    mode, errp);
-        if (algo < 0)  {
+        *mode = QCRYPTO_CIPHER_MODE_ECB;
+        *algo = cryptodev_builtin_get_aes_algo(sess_info->key_len,
+                                               *mode, errp);
+        if (*algo < 0)  {
             return -1;
         }
         break;
     case VIRTIO_CRYPTO_CIPHER_AES_CBC:
-        mode = QCRYPTO_CIPHER_MODE_CBC;
-        algo = cryptodev_builtin_get_aes_algo(sess_info->key_len,
-                                                    mode, errp);
-        if (algo < 0)  {
+        *mode = QCRYPTO_CIPHER_MODE_CBC;
+        *algo = cryptodev_builtin_get_aes_algo(sess_info->key_len,
+                                               *mode, errp);
+        if (*algo < 0)  {
             return -1;
         }
         break;
     case VIRTIO_CRYPTO_CIPHER_AES_CTR:
-        mode = QCRYPTO_CIPHER_MODE_CTR;
-        algo = cryptodev_builtin_get_aes_algo(sess_info->key_len,
-                                                    mode, errp);
-        if (algo < 0)  {
+        *mode = QCRYPTO_CIPHER_MODE_CTR;
+        *algo = cryptodev_builtin_get_aes_algo(sess_info->key_len,
+                                              *mode, errp);
+        if (*algo < 0)  {
             return -1;
         }
         break;
     case VIRTIO_CRYPTO_CIPHER_AES_XTS:
-        mode = QCRYPTO_CIPHER_MODE_XTS;
-        algo = cryptodev_builtin_get_aes_algo(sess_info->key_len,
-                                                    mode, errp);
-        if (algo < 0)  {
+        *mode = QCRYPTO_CIPHER_MODE_XTS;
+        *algo = cryptodev_builtin_get_aes_algo(sess_info->key_len,
+                                               *mode, errp);
+        if (*algo < 0)  {
             return -1;
         }
         break;
     case VIRTIO_CRYPTO_CIPHER_3DES_ECB:
-        mode = QCRYPTO_CIPHER_MODE_ECB;
-        algo = QCRYPTO_CIPHER_ALG_3DES;
+        *mode = QCRYPTO_CIPHER_MODE_ECB;
+        *algo = QCRYPTO_CIPHER_ALG_3DES;
         break;
     case VIRTIO_CRYPTO_CIPHER_3DES_CBC:
-        mode = QCRYPTO_CIPHER_MODE_CBC;
-        algo = QCRYPTO_CIPHER_ALG_3DES;
+        *mode = QCRYPTO_CIPHER_MODE_CBC;
+        *algo = QCRYPTO_CIPHER_ALG_3DES;
         break;
     case VIRTIO_CRYPTO_CIPHER_3DES_CTR:
-        mode = QCRYPTO_CIPHER_MODE_CTR;
-        algo = QCRYPTO_CIPHER_ALG_3DES;
+        *mode = QCRYPTO_CIPHER_MODE_CTR;
+        *algo = QCRYPTO_CIPHER_ALG_3DES;
         break;
     default:
         error_setg(errp, "Unsupported cipher alg :%u",
@@ -225,6 +208,39 @@ static int cryptodev_builtin_create_cipher_session(
         return -1;
     }
 
+    return 0;
+}
+
+static int cryptodev_builtin_create_cipher_session(
+                    CryptoDevBackendBuiltin *builtin,
+                    CryptoDevBackendSymSessionInfo *sess_info,
+                    Error **errp)
+{
+    int algo;
+    int mode;
+    QCryptoCipher *cipher;
+    int index;
+    CryptoDevBackendBuiltinSession *sess;
+    int ret;
+
+    if (sess_info->op_type != VIRTIO_CRYPTO_SYM_OP_CIPHER) {
+        error_setg(errp, "Unsupported optype :%u", sess_info->op_type);
+        return -1;
+    }
+
+    index = cryptodev_builtin_get_unused_session_index(builtin);
+    if (index < 0) {
+        error_setg(errp, "Total number of sessions created exceeds %u",
+                  MAX_NUM_SESSIONS);
+        return -1;
+    }
+
+    ret = cryptodev_builtin_get_cipher_alg_mode(sess_info,
+                                                &algo, &mode, errp);
+    if (ret < 0) {
+        return -1;
+    }
+
     cipher = qcrypto_cipher_new(algo, mode,
                                sess_info->cipher_key,
                                sess_info->key_len,
-- 
1.8.3.1

[Qemu-devel] [RFC v1 3/9] cryptodev: add missing op_code for symmertric crypto

[Gonglei]

We need the op_code to identify which of crypto
operations for stateless crypto operation.

Let's add it.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 backends/cryptodev-builtin.c | 7 +++++++
 include/sysemu/cryptodev.h   | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index abc2655..1e72985 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -334,6 +334,13 @@ static int cryptodev_builtin_sym_operation(
         return -VIRTIO_CRYPTO_NOTSUPP;
     }
 
+    if (op_info->op_code != VIRTIO_CRYPTO_CIPHER_ENCRYPT &&
+        op_info->op_code != VIRTIO_CRYPTO_CIPHER_DECRYPT) {
+        error_setg(errp,
+               "Unsupported op code: %u", op_info->op_code);
+        return -VIRTIO_CRYPTO_NOTSUPP;
+    }
+
     sess = builtin->sessions[op_info->session_id];
 
     if (op_info->iv_len > 0) {
diff --git a/include/sysemu/cryptodev.h b/include/sysemu/cryptodev.h
index be507f7..d75f2e7 100644
--- a/include/sysemu/cryptodev.h
+++ b/include/sysemu/cryptodev.h
@@ -103,6 +103,7 @@ typedef struct CryptoDevBackendSymSessionInfo {
  *
  * @session_id: session index which was previously
  *              created by cryptodev_backend_sym_create_session()
+ * @op_code: operation code (refer to virtio_crypto.h)
  * @aad_len: byte length of additional authenticated data
  * @iv_len: byte length of initialization vector or counter
  * @src_len: byte length of source data
@@ -129,6 +130,8 @@ typedef struct CryptoDevBackendSymSessionInfo {
  */
 typedef struct CryptoDevBackendSymOpInfo {
     uint64_t session_id;
+    /* corresponding with virtio crypto spec */
+    uint32_t op_code;
     uint32_t aad_len;
     uint32_t iv_len;
     uint32_t src_len;
-- 
1.8.3.1

[Qemu-devel] [RFC v1 4/9] cryptodev-builtin: realize stateless operation function

[Gonglei]

At this point, the stateless crypto operation for
cryptodev-builtin backend works.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 backends/cryptodev-builtin.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index 1e72985..7829999 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -401,7 +401,76 @@ cryptodev_builtin_sym_stateless_operation(
                  CryptoDevBackendSymStatelessInfo *op_info,
                  uint32_t queue_index, Error **errp)
 {
-    return -VIRTIO_CRYPTO_ERR;
+    CryptoDevBackendSymSessionInfo *sess_info;
+    CryptoDevBackendSymOpInfo *sym_op_info;
+    int algo, mode;
+    int ret;
+    QCryptoCipher *cipher = NULL;
+
+    sess_info = &op_info->session_info;
+    sym_op_info = &op_info->op_info;
+
+    if (sess_info->op_type != VIRTIO_CRYPTO_SYM_OP_CIPHER) {
+        error_setg(errp, "Unsupported op_type: %u", sess_info->op_type);
+        return -VIRTIO_CRYPTO_ERR;
+    }
+
+    switch (sym_op_info->op_code) {
+    case VIRTIO_CRYPTO_CIPHER_ENCRYPT:
+    case VIRTIO_CRYPTO_CIPHER_DECRYPT:
+        ret = cryptodev_builtin_get_cipher_alg_mode(sess_info,
+                                                &algo, &mode, errp);
+        if (ret < 0) {
+            return -VIRTIO_CRYPTO_ERR;
+        }
+
+        cipher = qcrypto_cipher_new(algo, mode,
+                                   sess_info->cipher_key,
+                                   sess_info->key_len,
+                                   errp);
+        if (!cipher) {
+            return -VIRTIO_CRYPTO_ERR;
+        }
+
+        if (sym_op_info->iv_len > 0) {
+            ret = qcrypto_cipher_setiv(cipher, sym_op_info->iv,
+                                       sym_op_info->iv_len, errp);
+            if (ret < 0) {
+                ret = -VIRTIO_CRYPTO_ERR;
+                goto out;
+            }
+        }
+
+        if (sess_info->direction == VIRTIO_CRYPTO_OP_ENCRYPT) {
+            ret = qcrypto_cipher_encrypt(cipher, sym_op_info->src,
+                                         sym_op_info->dst,
+                                         sym_op_info->src_len, errp);
+            if (ret < 0) {
+                ret = -VIRTIO_CRYPTO_ERR;
+                goto out;
+            }
+        } else {
+            ret = qcrypto_cipher_decrypt(cipher, sym_op_info->src,
+                                         sym_op_info->dst,
+                                         sym_op_info->src_len, errp);
+            if (ret < 0) {
+                ret = -VIRTIO_CRYPTO_ERR;
+                goto out;
+            }
+        }
+        break;
+
+    default:
+        error_setg(errp, "Unsupported op_code: %" PRIu32 "",
+                   sym_op_info->op_code);
+        return -VIRTIO_CRYPTO_ERR;
+    }
+
+    ret = VIRTIO_CRYPTO_OK;
+
+out:
+    qcrypto_cipher_free(cipher);
+    return ret;
 }
 
 static void
-- 
1.8.3.1

[Qemu-devel] [RFC v1 5/9] virtio-crypto: update header file

[Gonglei]

Update the header based on the newset virtio
crypto spec, so that the virtio crypto can
support both session and stateless based
crypto services and keep compatibility with
the pre-existing code by introducing five feature bits.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 include/standard-headers/linux/virtio_crypto.h | 210 ++++++++++++++++++++++++-
 1 file changed, 208 insertions(+), 2 deletions(-)

diff --git a/include/standard-headers/linux/virtio_crypto.h b/include/standard-headers/linux/virtio_crypto.h
index 5ff0b4e..c447829 100644
--- a/include/standard-headers/linux/virtio_crypto.h
+++ b/include/standard-headers/linux/virtio_crypto.h
@@ -33,11 +33,20 @@
 #include "standard-headers/linux/virtio_config.h"
 
 
+/* The crypto service for virtio crypto */
 #define VIRTIO_CRYPTO_SERVICE_CIPHER 0
 #define VIRTIO_CRYPTO_SERVICE_HASH   1
 #define VIRTIO_CRYPTO_SERVICE_MAC    2
 #define VIRTIO_CRYPTO_SERVICE_AEAD   3
 
+/* The feature bitmap for virtio crypto */
+#define VIRTIO_CRYPTO_F_MUX_MODE 0  /* Multiplexing mode is available */
+#define VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE 1
+#define VIRTIO_CRYPTO_F_HASH_STATELESS_MODE 2
+#define VIRTIO_CRYPTO_F_MAC_STATELESS_MODE	3
+#define VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE	4
+
+
 #define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
 
 struct virtio_crypto_ctrl_header {
@@ -166,8 +175,8 @@ struct virtio_crypto_aead_session_para {
 	uint32_t algo;
 	/* length of key */
 	uint32_t key_len;
-	/* hash result length */
-	uint32_t hash_result_len;
+	/* Authentication tag length */
+	uint32_t tag_len;
 	/* length of the additional authenticated data (AAD) in bytes */
 	uint32_t aad_len;
 	/* encrypt or decrypt, See above VIRTIO_CRYPTO_OP_* */
@@ -271,6 +280,8 @@ struct virtio_crypto_op_header {
 	uint32_t algo;
 	/* session_id should be service-specific algorithms */
 	uint64_t session_id;
+#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
+#define VIRTIO_CRYPTO_FLAG_STATELESS_MODE 2
 	/* control flag to control the request */
 	uint32_t flag;
 	uint32_t padding;
@@ -403,6 +414,201 @@ struct virtio_crypto_op_data_req {
 	} u;
 };
 
+struct virtio_crypto_cipher_para_stateless {
+	struct {
+		/* See VIRTIO_CRYPTO_CIPHER* above */
+		uint32_t algo;
+		/* length of key */
+		uint32_t keylen;
+
+		/* See VIRTIO_CRYPTO_OP_* above */
+		uint32_t op;
+	} sess_para;
+
+	/*
+	 * Byte Length of valid IV/Counter
+	 */
+	uint32_t iv_len;
+	/* length of source data */
+	uint32_t src_data_len;
+	/* length of dst data */
+	uint32_t dst_data_len;
+};
+
+struct virtio_crypto_alg_chain_data_para_stateless {
+	struct {
+		/* See VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_* above */
+		uint32_t alg_chain_order;
+		/* length of the additional authenticated data in bytes */
+		uint32_t aad_len;
+
+		struct {
+			/* See VIRTIO_CRYPTO_CIPHER* above */
+			uint32_t algo;
+			/* length of key */
+			uint32_t keylen;
+			/* See VIRTIO_CRYPTO_OP_* above */
+			uint32_t op;
+		} cipher;
+
+		struct {
+			/* See VIRTIO_CRYPTO_HASH_* or _MAC_* above */
+			uint32_t algo;
+			/* length of authenticated key */
+			uint32_t auth_key_len;
+			/* See VIRTIO_CRYPTO_SYM_HASH_MODE_* above */
+			uint32_t hash_mode;
+		} hash;
+	} sess_para;
+
+	uint32_t iv_len;
+	/* Length of source data */
+	uint32_t src_data_len;
+	/* Length of destination data */
+	uint32_t dst_data_len;
+	/* Starting point for cipher processing in source data */
+	uint32_t cipher_start_src_offset;
+	/* Length of the source data that the cipher will be computed on */
+	uint32_t len_to_cipher;
+	/* Starting point for hash processing in source data */
+	uint32_t hash_start_src_offset;
+	/* Length of the source data that the hash will be computed on */
+	uint32_t len_to_hash;
+	/* Length of the additional auth data */
+	uint32_t aad_len;
+	/* Length of the hash result */
+	uint32_t hash_result_len;
+	uint32_t reserved;
+};
+
+struct virtio_crypto_hash_para_stateless {
+	struct {
+		/* See VIRTIO_CRYPTO_HASH_* above */
+		uint32_t algo;
+	} sess_para;
+
+	/* length of source data */
+	uint32_t src_data_len;
+	/* hash result length */
+	uint32_t hash_result_len;
+	uint32_t reserved;
+};
+
+struct virtio_crypto_mac_para_stateless {
+	struct {
+		/* See VIRTIO_CRYPTO_MAC_* above */
+		uint32_t algo;
+		/* length of authenticated key */
+		uint32_t auth_key_len;
+	} sess_para;
+
+	/* length of source data */
+	uint32_t src_data_len;
+	/* hash result length */
+	uint32_t hash_result_len;
+};
+
+struct virtio_crypto_aead_para_stateless {
+	struct {
+		/* See VIRTIO_CRYPTO_AEAD_* above */
+		uint32_t algo;
+		/* length of key */
+		uint32_t key_len;
+		/* encrypt or decrypt, See above VIRTIO_CRYPTO_OP_* */
+		uint32_t op;
+	} sess_para;
+
+	/*
+	 * Byte Length of valid IV data pointed to by the below iv_addr
+	 * parameter.
+	 */
+	uint32_t iv_len;
+	/* Authentication tag length */
+	uint32_t tag_len;
+	/* length of the additional authenticated data (AAD) in bytes */
+	uint32_t aad_len;
+	/* length of source data */
+	uint32_t src_data_len;
+	/* length of dst data, it should be at least src_data_len + tag_len */
+	uint32_t dst_data_len;
+};
+
+struct virtio_crypto_cipher_data_req_stateless {
+	/* Device-readable part */
+	struct virtio_crypto_cipher_para_stateless para;
+	uint8_t padding[48];
+};
+
+struct virtio_crypto_hash_data_req_stateless {
+	/* Device-readable part */
+	struct virtio_crypto_hash_para_stateless para;
+	uint8_t padding[64];
+};
+
+struct virtio_crypto_mac_data_req_stateless {
+	/* Device-readable part */
+	struct virtio_crypto_mac_para_stateless para;
+	uint8_t padding[64];
+};
+
+struct virtio_crypto_alg_chain_data_req_stateless {
+	/* Device-readable part */
+	struct virtio_crypto_alg_chain_data_para_stateless para;
+};
+
+struct virtio_crypto_sym_data_req_stateless {
+	union {
+		struct virtio_crypto_cipher_data_req_stateless cipher;
+		struct virtio_crypto_alg_chain_data_req_stateless chain;
+		uint8_t padding[72];
+	} u;
+
+	/* See above VIRTIO_CRYPTO_SYM_OP_* */
+	uint32_t op_type;
+	uint32_t padding;
+};
+
+struct virtio_crypto_aead_data_req_stateless {
+	/* Device-readable part */
+	struct virtio_crypto_aead_para_stateless para;
+	uint8_t padding[48];
+};
+
+/* The request of the data virtqueue's packet */
+struct virtio_crypto_op_data_req_mux {
+    /* The size is 24 byte */
+	struct virtio_crypto_op_header header;
+
+	union {
+		struct {
+			struct virtio_crypto_sym_data_req  data;
+			uint8_t padding[56];
+		} sym_req;
+		struct {
+			struct virtio_crypto_hash_data_req data;
+			uint8_t padding[56];
+		} hash_req;
+		struct {
+			struct virtio_crypto_mac_data_req data;
+			uint8_t padding[56];
+		} mac_req;
+		struct {
+			struct virtio_crypto_aead_data_req data;
+			uint8_t padding[56];
+		} aead_req;
+
+		struct virtio_crypto_sym_data_req_stateless  sym_stateless_req;
+		struct virtio_crypto_hash_data_req_stateless hash_stateless_req;
+		struct virtio_crypto_mac_data_req_stateless mac_stateless_req;
+		struct virtio_crypto_aead_data_req_stateless aead_stateless_req;
+		/*
+		 * Making the request's total size is equal to 128 byte, and
+		 * reserving 24 byte for future extension.
+		 */
+		uint8_t padding[104];
+	} u;
+};
+
 #define VIRTIO_CRYPTO_OK        0
 #define VIRTIO_CRYPTO_ERR       1
 #define VIRTIO_CRYPTO_BADMSG    2
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v1 5/9] virtio-crypto: update header file

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

On Mon, May 08, 2017 at 07:38:20PM +0800, Gonglei wrote:
> @@ -166,8 +175,8 @@ struct virtio_crypto_aead_session_para {
>  	uint32_t algo;
>  	/* length of key */
>  	uint32_t key_len;
> -	/* hash result length */
> -	uint32_t hash_result_len;
> +	/* Authentication tag length */
> +	uint32_t tag_len;

Why is this field being renamed?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [RFC v1 5/9] virtio-crypto: update header file

[Gonglei (Arei)]

>
> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Tuesday, May 16, 2017 11:43 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; mst@redhat.com; Huangweidong (C);
> pasic@linux.vnet.ibm.com; Luonengjun; cornelia.huck@de.ibm.com;
> Linqiangmin; xin.zeng@intel.com; Wubin (H)
> Subject: Re: [RFC v1 5/9] virtio-crypto: update header file
> 
> On Mon, May 08, 2017 at 07:38:20PM +0800, Gonglei wrote:
> > @@ -166,8 +175,8 @@ struct virtio_crypto_aead_session_para {
> >  	uint32_t algo;
> >  	/* length of key */
> >  	uint32_t key_len;
> > -	/* hash result length */
> > -	uint32_t hash_result_len;
> > +	/* Authentication tag length */
> > +	uint32_t tag_len;
> 
> Why is this field being renamed?

Oh, I just think the latter name ' tag_len' is more proper than 'hash_result_len',
and the current code don't touch this field.

Thanks,
-Gonglei

[Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Gonglei]

According to the new spec, we should use different
requst structure to store the data request based
on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
negotiated or not.

In this patch, we havn't supported stateless mode
yet. The device reportes an error if both
VIRTIO_CRYPTO_F_MUX_MODE and VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
are negotiated, meanwhile the header.flag doesn't set
to VIRTIO_CRYPTO_FLAG_SESSION_MODE.

Let's handle this scenario in the following patches.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/virtio/virtio-crypto.c | 83 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 71 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 0353eb6..c4b8a2c 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     VirtQueueElement *elem = &request->elem;
     int queue_index = virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
     struct virtio_crypto_op_data_req req;
+    struct virtio_crypto_op_data_req_mux req_mux;
     int ret;
     struct iovec *in_iov;
     struct iovec *out_iov;
@@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     uint64_t session_id;
     CryptoDevBackendSymOpInfo *sym_op_info = NULL;
     Error *local_err = NULL;
+    bool mux_mode_is_negotiated;
+    struct virtio_crypto_op_header *header;
+    bool is_stateless_req = false;
 
     if (elem->out_num < 1 || elem->in_num < 1) {
         virtio_error(vdev, "virtio-crypto dataq missing headers");
@@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     out_iov = elem->out_sg;
     in_num = elem->in_num;
     in_iov = elem->in_sg;
-    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
-                != sizeof(req))) {
-        virtio_error(vdev, "virtio-crypto request outhdr too short");
-        return -1;
+
+    mux_mode_is_negotiated =
+        virtio_vdev_has_feature(vdev, VIRTIO_CRYPTO_F_MUX_MODE);
+    if (!mux_mode_is_negotiated) {
+        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
+                    != sizeof(req))) {
+            virtio_error(vdev, "virtio-crypto request outhdr too short");
+            return -1;
+        }
+        iov_discard_front(&out_iov, &out_num, sizeof(req));
+
+        header = &req.header;
+    } else {
+        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
+                sizeof(req_mux)) != sizeof(req_mux))) {
+            virtio_error(vdev, "virtio-crypto request outhdr too short");
+            return -1;
+        }
+        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
+
+        header = &req_mux.header;
     }
-    iov_discard_front(&out_iov, &out_num, sizeof(req));
 
     if (in_iov[in_num - 1].iov_len <
             sizeof(struct virtio_crypto_inhdr)) {
@@ -623,16 +643,51 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     request->in_num = in_num;
     request->in_iov = in_iov;
 
-    opcode = ldl_le_p(&req.header.opcode);
-    session_id = ldq_le_p(&req.header.session_id);
+    opcode = ldl_le_p(&header->opcode);
 
     switch (opcode) {
     case VIRTIO_CRYPTO_CIPHER_ENCRYPT:
     case VIRTIO_CRYPTO_CIPHER_DECRYPT:
-        ret = virtio_crypto_handle_sym_req(vcrypto,
+        if (!mux_mode_is_negotiated) {
+            ret = virtio_crypto_handle_sym_req(vcrypto,
                          &req.u.sym_req,
                          &sym_op_info,
                          out_iov, out_num);
+        } else {
+            if (!virtio_vdev_has_feature(vdev,
+                    VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE)) {
+                /*
+                 * If the VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE is not
+                 * negotiated, the driver MUST use the session mode
+                 */
+                ret = virtio_crypto_handle_sym_req(vcrypto,
+                         &req_mux.u.sym_req.data,
+                         &sym_op_info,
+                         out_iov, out_num);
+            } else {
+                /*
+                 * If the VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE is
+                 * negotiated, the device MUST parse header.flag
+                 * in order to decide which mode the driver uses.
+                 */
+                if (header->flag == VIRTIO_CRYPTO_FLAG_SESSION_MODE) {
+                    ret = virtio_crypto_handle_sym_req(vcrypto,
+                         &req_mux.u.sym_req.data,
+                         &sym_op_info,
+                         out_iov, out_num);
+                } else {
+                    is_stateless_req = true;
+                    /*
+                     * Handle stateless mode, that is
+                     * header->flag == VIRTIO_CRYPTO_FLAG_STATELESS_MODE
+                     */
+                    virtio_error(vdev,
+                        "virtio-crypto do not support stateless mode");
+                    return -1;
+                }
+            }
+        }
+
         /* Serious errors, need to reset virtio crypto device */
         if (ret == -EFAULT) {
             return -1;
@@ -640,11 +695,15 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
             virtio_crypto_req_complete(request, VIRTIO_CRYPTO_NOTSUPP);
             virtio_crypto_free_request(request);
         } else {
-            sym_op_info->session_id = session_id;
+            if (!is_stateless_req) {
+                sym_op_info->op_code = opcode;
+                session_id = ldq_le_p(&header->session_id);
+                sym_op_info->session_id = session_id;
+                /* Set request's parameter */
+                request->flags = CRYPTODEV_BACKEND_ALG_SYM;
+                request->u.sym_op_info = sym_op_info;
+            }
 
-            /* Set request's parameter */
-            request->flags = CRYPTODEV_BACKEND_ALG_SYM;
-            request->u.sym_op_info = sym_op_info;
             ret = cryptodev_backend_crypto_operation(vcrypto->cryptodev,
                                     request, queue_index, &local_err);
             if (ret < 0) {
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/08/2017 01:38 PM, Gonglei wrote:
> According to the new spec, we should use different
> requst structure to store the data request based
> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
> negotiated or not.
> 
> In this patch, we havn't supported stateless mode
> yet. The device reportes an error if both
> VIRTIO_CRYPTO_F_MUX_MODE and VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
> are negotiated, meanwhile the header.flag doesn't set
> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> 
> Let's handle this scenario in the following patches.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/virtio/virtio-crypto.c | 83 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index 0353eb6..c4b8a2c 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
>      VirtQueueElement *elem = &request->elem;
>      int queue_index = virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
>      struct virtio_crypto_op_data_req req;
> +    struct virtio_crypto_op_data_req_mux req_mux;
>      int ret;
>      struct iovec *in_iov;
>      struct iovec *out_iov;
> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
>      uint64_t session_id;
>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
>      Error *local_err = NULL;
> +    bool mux_mode_is_negotiated;
> +    struct virtio_crypto_op_header *header;
> +    bool is_stateless_req = false;
> 
>      if (elem->out_num < 1 || elem->in_num < 1) {
>          virtio_error(vdev, "virtio-crypto dataq missing headers");
> @@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
>      out_iov = elem->out_sg;
>      in_num = elem->in_num;
>      in_iov = elem->in_sg;
> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> -                != sizeof(req))) {
> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
> -        return -1;
> +
> +    mux_mode_is_negotiated =
> +        virtio_vdev_has_feature(vdev, VIRTIO_CRYPTO_F_MUX_MODE);
> +    if (!mux_mode_is_negotiated) {
> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> +                    != sizeof(req))) {
> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
> +            return -1;
> +        }
> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
> +
> +        header = &req.header;
> +    } else {
> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
> +                sizeof(req_mux)) != sizeof(req_mux))) {
> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
> +            return -1;
> +        }
> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
> +
> +        header = &req_mux.header;

I wonder if this request length checking logic is conform to the
most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
virtio crypto device specification").

AFAIU here you allow only requests of two sizes: one fixed size
for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
means that some requests need quite some padding between what
you call the 'request' and the actual data on which the crypto
operation dictated by the 'request' needs to be performed.
What are the benefits of this approach?


Regards,
Halil

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Gonglei (Arei)]

> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
> Sent: Friday, May 12, 2017 7:02 PM
> 
> 
> On 05/08/2017 01:38 PM, Gonglei wrote:
> > According to the new spec, we should use different
> > requst structure to store the data request based
> > on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
> > negotiated or not.
> >
> > In this patch, we havn't supported stateless mode
> > yet. The device reportes an error if both
> > VIRTIO_CRYPTO_F_MUX_MODE and
> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
> > are negotiated, meanwhile the header.flag doesn't set
> > to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> >
> > Let's handle this scenario in the following patches.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/virtio/virtio-crypto.c | 83
> ++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 71 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> > index 0353eb6..c4b8a2c 100644
> > --- a/hw/virtio/virtio-crypto.c
> > +++ b/hw/virtio/virtio-crypto.c
> > @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> *request)
> >      VirtQueueElement *elem = &request->elem;
> >      int queue_index =
> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> >      struct virtio_crypto_op_data_req req;
> > +    struct virtio_crypto_op_data_req_mux req_mux;
> >      int ret;
> >      struct iovec *in_iov;
> >      struct iovec *out_iov;
> > @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> *request)
> >      uint64_t session_id;
> >      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
> >      Error *local_err = NULL;
> > +    bool mux_mode_is_negotiated;
> > +    struct virtio_crypto_op_header *header;
> > +    bool is_stateless_req = false;
> >
> >      if (elem->out_num < 1 || elem->in_num < 1) {
> >          virtio_error(vdev, "virtio-crypto dataq missing headers");
> > @@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> *request)
> >      out_iov = elem->out_sg;
> >      in_num = elem->in_num;
> >      in_iov = elem->in_sg;
> > -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> > -                != sizeof(req))) {
> > -        virtio_error(vdev, "virtio-crypto request outhdr too short");
> > -        return -1;
> > +
> > +    mux_mode_is_negotiated =
> > +        virtio_vdev_has_feature(vdev, VIRTIO_CRYPTO_F_MUX_MODE);
> > +    if (!mux_mode_is_negotiated) {
> > +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> > +                    != sizeof(req))) {
> > +            virtio_error(vdev, "virtio-crypto request outhdr too short");
> > +            return -1;
> > +        }
> > +        iov_discard_front(&out_iov, &out_num, sizeof(req));
> > +
> > +        header = &req.header;
> > +    } else {
> > +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
> > +                sizeof(req_mux)) != sizeof(req_mux))) {
> > +            virtio_error(vdev, "virtio-crypto request outhdr too short");
> > +            return -1;
> > +        }
> > +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
> > +
> > +        header = &req_mux.header;
> 
> I wonder if this request length checking logic is conform to the
> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
> virtio crypto device specification").
> 
Sure. Please see below normative formulation:

'''
\drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
...
\item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto requests.
Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
...
'''

> AFAIU here you allow only requests of two sizes: one fixed size
> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
> means that some requests need quite some padding between what
> you call the 'request' and the actual data on which the crypto
> operation dictated by the 'request' needs to be performed.

Yes, that's true.

> What are the benefits of this approach?
> 
We could unify the request for all algorithms, both symmetric algos and asymmetric algos,
which is very convenient for handling tens of hundreds of different algorithm requests.


Thanks,
-Gonglei

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
> 
>> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
>> Sent: Friday, May 12, 2017 7:02 PM
>>
>>
>> On 05/08/2017 01:38 PM, Gonglei wrote:
>>> According to the new spec, we should use different
>>> requst structure to store the data request based
>>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
>>> negotiated or not.
>>>
>>> In this patch, we havn't supported stateless mode
>>> yet. The device reportes an error if both
>>> VIRTIO_CRYPTO_F_MUX_MODE and
>> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
>>> are negotiated, meanwhile the header.flag doesn't set
>>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
>>>
>>> Let's handle this scenario in the following patches.
>>>
>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>> ---
>>>  hw/virtio/virtio-crypto.c | 83
>> ++++++++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 71 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
>>> index 0353eb6..c4b8a2c 100644
>>> --- a/hw/virtio/virtio-crypto.c
>>> +++ b/hw/virtio/virtio-crypto.c
>>> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>> *request)
>>>      VirtQueueElement *elem = &request->elem;
>>>      int queue_index =
>> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
>>>      struct virtio_crypto_op_data_req req;
>>> +    struct virtio_crypto_op_data_req_mux req_mux;
>>>      int ret;
>>>      struct iovec *in_iov;
>>>      struct iovec *out_iov;
>>> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>> *request)
>>>      uint64_t session_id;
>>>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
>>>      Error *local_err = NULL;
>>> +    bool mux_mode_is_negotiated;
>>> +    struct virtio_crypto_op_header *header;
>>> +    bool is_stateless_req = false;
>>>
>>>      if (elem->out_num < 1 || elem->in_num < 1) {
>>>          virtio_error(vdev, "virtio-crypto dataq missing headers");
>>> @@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>> *request)
>>>      out_iov = elem->out_sg;
>>>      in_num = elem->in_num;
>>>      in_iov = elem->in_sg;
>>> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>> -                != sizeof(req))) {
>>> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
>>> -        return -1;
>>> +
>>> +    mux_mode_is_negotiated =
>>> +        virtio_vdev_has_feature(vdev, VIRTIO_CRYPTO_F_MUX_MODE);
>>> +    if (!mux_mode_is_negotiated) {
>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>> +                    != sizeof(req))) {
>>> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
>>> +            return -1;
>>> +        }
>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
>>> +
>>> +        header = &req.header;
>>> +    } else {
>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
>>> +                sizeof(req_mux)) != sizeof(req_mux))) {
>>> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
>>> +            return -1;
>>> +        }
>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
>>> +
>>> +        header = &req_mux.header;
>>
>> I wonder if this request length checking logic is conform to the
>> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
>> virtio crypto device specification").
>>
> Sure. Please see below normative formulation:
> 
> '''
> \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
> ...
> \item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto requests.
> Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
> ...
> '''
> 

As far as I can remember, we have already agreed that in terms of the
spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your
code you have a substantially different struct virtio_crypto_op_data_req
than in your spec! For instance in the spec virtio_crypto_op_data_req is
the full request and contains the data buffers (src_data and the
dest_data), while in your code it's effectively just a header and does
not contain any data buffers.


>> AFAIU here you allow only requests of two sizes: one fixed size
>> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
>> means that some requests need quite some padding between what
>> you call the 'request' and the actual data on which the crypto
>> operation dictated by the 'request' needs to be performed.
> 
> Yes, that's true.
> 

This implies that should we need more than 128 bytes for a request,
we will need to introduce jet another request format and negotiate it
via feature bits.

By the way, I'm having a hard time understing how is the requirement form 
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260004
(2.4.4 Message Framing) satisfied by this code. Could you explain this
to me please?


>> What are the benefits of this approach?
>>
> We could unify the request for all algorithms, both symmetric algos and asymmetric algos,
> which is very convenient for handling tens of hundreds of different algorithm requests.
> 

AFAIU the reason is ease of implementation. If everybody else is fine
with this, I won't object either.

> 
> Thanks,
> -Gonglei
> 

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Gonglei (Arei)]

> 
> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
> >
> >> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
> >> Sent: Friday, May 12, 2017 7:02 PM
> >>
> >>
> >> On 05/08/2017 01:38 PM, Gonglei wrote:
> >>> According to the new spec, we should use different
> >>> requst structure to store the data request based
> >>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
> >>> negotiated or not.
> >>>
> >>> In this patch, we havn't supported stateless mode
> >>> yet. The device reportes an error if both
> >>> VIRTIO_CRYPTO_F_MUX_MODE and
> >> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
> >>> are negotiated, meanwhile the header.flag doesn't set
> >>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> >>>
> >>> Let's handle this scenario in the following patches.
> >>>
> >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >>> ---
> >>>  hw/virtio/virtio-crypto.c | 83
> >> ++++++++++++++++++++++++++++++++++++++++-------
> >>>  1 file changed, 71 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> >>> index 0353eb6..c4b8a2c 100644
> >>> --- a/hw/virtio/virtio-crypto.c
> >>> +++ b/hw/virtio/virtio-crypto.c
> >>> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> >> *request)
> >>>      VirtQueueElement *elem = &request->elem;
> >>>      int queue_index =
> >> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> >>>      struct virtio_crypto_op_data_req req;
> >>> +    struct virtio_crypto_op_data_req_mux req_mux;
> >>>      int ret;
> >>>      struct iovec *in_iov;
> >>>      struct iovec *out_iov;
> >>> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> >> *request)
> >>>      uint64_t session_id;
> >>>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
> >>>      Error *local_err = NULL;
> >>> +    bool mux_mode_is_negotiated;
> >>> +    struct virtio_crypto_op_header *header;
> >>> +    bool is_stateless_req = false;
> >>>
> >>>      if (elem->out_num < 1 || elem->in_num < 1) {
> >>>          virtio_error(vdev, "virtio-crypto dataq missing headers");
> >>> @@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> >> *request)
> >>>      out_iov = elem->out_sg;
> >>>      in_num = elem->in_num;
> >>>      in_iov = elem->in_sg;
> >>> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> >>> -                != sizeof(req))) {
> >>> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>> -        return -1;
> >>> +
> >>> +    mux_mode_is_negotiated =
> >>> +        virtio_vdev_has_feature(vdev,
> VIRTIO_CRYPTO_F_MUX_MODE);
> >>> +    if (!mux_mode_is_negotiated) {
> >>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> >>> +                    != sizeof(req))) {
> >>> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>> +            return -1;
> >>> +        }
> >>> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
> >>> +
> >>> +        header = &req.header;
> >>> +    } else {
> >>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
> >>> +                sizeof(req_mux)) != sizeof(req_mux))) {
> >>> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>> +            return -1;
> >>> +        }
> >>> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
> >>> +
> >>> +        header = &req_mux.header;
> >>
> >> I wonder if this request length checking logic is conform to the
> >> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
> >> virtio crypto device specification").
> >>
> > Sure. Please see below normative formulation:
> >
> > '''
> > \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types
> / Crypto Device / Device Operation / Symmetric algorithms Operation}
> > ...
> > \item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the
> driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto
> requests.
> > Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
> > ...
> > '''
> >
> 
> As far as I can remember, we have already agreed that in terms of the
> spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your

Sorry, I don't think so. :(

> code you have a substantially different struct virtio_crypto_op_data_req
> than in your spec! For instance in the spec virtio_crypto_op_data_req is
> the full request and contains the data buffers (src_data and the
> dest_data), while in your code it's effectively just a header and does
> not contain any data buffers.
> 
I said struct virtio_crypto_op_data_req in the spec is just a symbol.
I didn't find a better way to express the src_data and dst_data etc. So
I used u8[len] xxx_data to occupy a sit in the request.

> 
> >> AFAIU here you allow only requests of two sizes: one fixed size
> >> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
> >> means that some requests need quite some padding between what
> >> you call the 'request' and the actual data on which the crypto
> >> operation dictated by the 'request' needs to be performed.
> >
> > Yes, that's true.
> >
> 
> This implies that should we need more than 128 bytes for a request,
> we will need to introduce jet another request format and negotiate it
> via feature bits.
> 
Why do we need other feature bits?

> By the way, I'm having a hard time understing how is the requirement form
> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
> 004
> (2.4.4 Message Framing) satisfied by this code. Could you explain this
> to me please?
> 
Sorry for my bad English, 
I don't know which normative formulation the code violates?

Thanks
-Gonglei
> 
> >> What are the benefits of this approach?
> >>
> > We could unify the request for all algorithms, both symmetric algos and
> asymmetric algos,
> > which is very convenient for handling tens of hundreds of different algorithm
> requests.
> >
> 
> AFAIU the reason is ease of implementation. If everybody else is fine
> with this, I won't object either.
> 
> >
> > Thanks,
> > -Gonglei
> >

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/16/2017 04:52 AM, Gonglei (Arei) wrote:
>>
>> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
>>>
>>>> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
>>>> Sent: Friday, May 12, 2017 7:02 PM
>>>>
>>>>
>>>> On 05/08/2017 01:38 PM, Gonglei wrote:
>>>>> According to the new spec, we should use different
>>>>> requst structure to store the data request based
>>>>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
>>>>> negotiated or not.
>>>>>
>>>>> In this patch, we havn't supported stateless mode
>>>>> yet. The device reportes an error if both
>>>>> VIRTIO_CRYPTO_F_MUX_MODE and
>>>> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
>>>>> are negotiated, meanwhile the header.flag doesn't set
>>>>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
>>>>>
>>>>> Let's handle this scenario in the following patches.
>>>>>
>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>> ---
>>>>>  hw/virtio/virtio-crypto.c | 83
>>>> ++++++++++++++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 71 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
>>>>> index 0353eb6..c4b8a2c 100644
>>>>> --- a/hw/virtio/virtio-crypto.c
>>>>> +++ b/hw/virtio/virtio-crypto.c
>>>>> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>> *request)
>>>>>      VirtQueueElement *elem = &request->elem;
>>>>>      int queue_index =
>>>> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
>>>>>      struct virtio_crypto_op_data_req req;
>>>>> +    struct virtio_crypto_op_data_req_mux req_mux;
>>>>>      int ret;
>>>>>      struct iovec *in_iov;
>>>>>      struct iovec *out_iov;
>>>>> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>> *request)
>>>>>      uint64_t session_id;
>>>>>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
>>>>>      Error *local_err = NULL;
>>>>> +    bool mux_mode_is_negotiated;
>>>>> +    struct virtio_crypto_op_header *header;
>>>>> +    bool is_stateless_req = false;
>>>>>
>>>>>      if (elem->out_num < 1 || elem->in_num < 1) {
>>>>>          virtio_error(vdev, "virtio-crypto dataq missing headers");
>>>>> @@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>> *request)
>>>>>      out_iov = elem->out_sg;
>>>>>      in_num = elem->in_num;
>>>>>      in_iov = elem->in_sg;
>>>>> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>> -                != sizeof(req))) {
>>>>> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>> -        return -1;
>>>>> +
>>>>> +    mux_mode_is_negotiated =
>>>>> +        virtio_vdev_has_feature(vdev,
>> VIRTIO_CRYPTO_F_MUX_MODE);
>>>>> +    if (!mux_mode_is_negotiated) {
>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>> +                    != sizeof(req))) {
>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>> +            return -1;
>>>>> +        }
>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
>>>>> +
>>>>> +        header = &req.header;
>>>>> +    } else {
>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
>>>>> +                sizeof(req_mux)) != sizeof(req_mux))) {
>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>> +            return -1;
>>>>> +        }
>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
>>>>> +
>>>>> +        header = &req_mux.header;
>>>>
>>>> I wonder if this request length checking logic is conform to the
>>>> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
>>>> virtio crypto device specification").
>>>>
>>> Sure. Please see below normative formulation:
>>>
>>> '''
>>> \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types
>> / Crypto Device / Device Operation / Symmetric algorithms Operation}
>>> ...
>>> \item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the
>> driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto
>> requests.
>>> Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
>>> ...
>>> '''
>>>
>>
>> As far as I can remember, we have already agreed that in terms of the
>> spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your
> 
> Sorry, I don't think so. :(
> 
>> code you have a substantially different struct virtio_crypto_op_data_req
>> than in your spec! For instance in the spec virtio_crypto_op_data_req is
>> the full request and contains the data buffers (src_data and the
>> dest_data), while in your code it's effectively just a header and does
>> not contain any data buffers.
>>
> I said struct virtio_crypto_op_data_req in the spec is just a symbol.
> I didn't find a better way to express the src_data and dst_data etc. So
> I used u8[len] xxx_data to occupy a sit in the request.
> 

OK, tell me how is the reader/implementer of the spec supposed to figure
out that a 124 byte padded "header" needs to be precede any "data"?

Besides if you look at

+Stateless mode HASH service requests are as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_hash_para_statelesss {
+    struct {
+        /* See VIRTIO_CRYPTO_HASH_* above */
+        le32 algo;
+    } sess_para;
+
+    /* length of source data */
+    le32 src_data_len;
+    /* hash result length */
+    le32 hash_result_len;
+    le32 reserved;
+};
+struct virtio_crypto_hash_data_req_stateless {
+    /* Device-readable part */
+    struct virtio_crypto_hash_para_stateless para;
+    /* Source data */
+    u8 src_data[src_data_len];
+
+    /* Device-writable part */
+    /* Hash result data */
+    u8 hash_result[hash_result_len];
+    struct virtio_crypto_inhdr inhdr;
+};
+\end{lstlisting}
+

from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device
specification". You need the padding to 128 bytes after
virtio_crypto_hash_para_stateless para, but before src_data. But there is
no indication in the definition of virtio_crypto_hash_data_req_stateless
nor somewhere in the spec about that. On the contrary based on this
one would expect para.reserved and src_data being pretty adjecent.

Thus the normative statement you quoted is (IMHO) ill suited and
insufficient to express what you have been trying to express.

>>
>>>> AFAIU here you allow only requests of two sizes: one fixed size
>>>> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
>>>> means that some requests need quite some padding between what
>>>> you call the 'request' and the actual data on which the crypto
>>>> operation dictated by the 'request' needs to be performed.
>>>
>>> Yes, that's true.
>>>
>>
>> This implies that should we need more than 128 bytes for a request,
>> we will need to introduce jet another request format and negotiate it
>> via feature bits.
>>
> Why do we need other feature bits?

Because assume you have something that needs more that 128 bytes for
its request, how do you solve the problem without new format end new
feature bit?

> 
>> By the way, I'm having a hard time understing how is the requirement form
>> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
>> 004
>> (2.4.4 Message Framing) satisfied by this code. Could you explain this
>> to me please?
>>
> Sorry for my bad English, 
> I don't know which normative formulation the code violates?

I'm not sure it's actually violated, but I'm concerned about the following
normative statement: "The device MUST NOT make assumptions about the particular
arrangement of descriptors. The device MAY have a reasonable limit of
descriptors it will allow in a chain." 

Please also read the explanatory part I've linked, because the normative
statement is pretty abstract.

In my understanding, the spec says, that e.g. the virti-crypto device
should not fail if a single request is split up into let's say two chunks
and transmitted by the means of two top level descriptors.

Do you agree with my reading of the spec? 

What does the virtio-crypto device do if it encounters such a situation?

Regards,
Halil

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Gonglei (Arei)]

> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
> Sent: Wednesday, May 17, 2017 6:18 AM
> 
> 
> On 05/16/2017 04:52 AM, Gonglei (Arei) wrote:
> >>
> >> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
> >>>
> >>>> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
> >>>> Sent: Friday, May 12, 2017 7:02 PM
> >>>>
> >>>>
> >>>> On 05/08/2017 01:38 PM, Gonglei wrote:
> >>>>> According to the new spec, we should use different
> >>>>> requst structure to store the data request based
> >>>>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
> >>>>> negotiated or not.
> >>>>>
> >>>>> In this patch, we havn't supported stateless mode
> >>>>> yet. The device reportes an error if both
> >>>>> VIRTIO_CRYPTO_F_MUX_MODE and
> >>>> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
> >>>>> are negotiated, meanwhile the header.flag doesn't set
> >>>>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> >>>>>
> >>>>> Let's handle this scenario in the following patches.
> >>>>>
> >>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >>>>> ---
> >>>>>  hw/virtio/virtio-crypto.c | 83
> >>>> ++++++++++++++++++++++++++++++++++++++++-------
> >>>>>  1 file changed, 71 insertions(+), 12 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> >>>>> index 0353eb6..c4b8a2c 100644
> >>>>> --- a/hw/virtio/virtio-crypto.c
> >>>>> +++ b/hw/virtio/virtio-crypto.c
> >>>>> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> >>>> *request)
> >>>>>      VirtQueueElement *elem = &request->elem;
> >>>>>      int queue_index =
> >>>> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> >>>>>      struct virtio_crypto_op_data_req req;
> >>>>> +    struct virtio_crypto_op_data_req_mux req_mux;
> >>>>>      int ret;
> >>>>>      struct iovec *in_iov;
> >>>>>      struct iovec *out_iov;
> >>>>> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> >>>> *request)
> >>>>>      uint64_t session_id;
> >>>>>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
> >>>>>      Error *local_err = NULL;
> >>>>> +    bool mux_mode_is_negotiated;
> >>>>> +    struct virtio_crypto_op_header *header;
> >>>>> +    bool is_stateless_req = false;
> >>>>>
> >>>>>      if (elem->out_num < 1 || elem->in_num < 1) {
> >>>>>          virtio_error(vdev, "virtio-crypto dataq missing headers");
> >>>>> @@ -597,12 +601,28 @@
> virtio_crypto_handle_request(VirtIOCryptoReq
> >>>> *request)
> >>>>>      out_iov = elem->out_sg;
> >>>>>      in_num = elem->in_num;
> >>>>>      in_iov = elem->in_sg;
> >>>>> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> >>>>> -                != sizeof(req))) {
> >>>>> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>>>> -        return -1;
> >>>>> +
> >>>>> +    mux_mode_is_negotiated =
> >>>>> +        virtio_vdev_has_feature(vdev,
> >> VIRTIO_CRYPTO_F_MUX_MODE);
> >>>>> +    if (!mux_mode_is_negotiated) {
> >>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> >>>>> +                    != sizeof(req))) {
> >>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too
> short");
> >>>>> +            return -1;
> >>>>> +        }
> >>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
> >>>>> +
> >>>>> +        header = &req.header;
> >>>>> +    } else {
> >>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
> >>>>> +                sizeof(req_mux)) != sizeof(req_mux))) {
> >>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too
> short");
> >>>>> +            return -1;
> >>>>> +        }
> >>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
> >>>>> +
> >>>>> +        header = &req_mux.header;
> >>>>
> >>>> I wonder if this request length checking logic is conform to the
> >>>> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
> >>>> virtio crypto device specification").
> >>>>
> >>> Sure. Please see below normative formulation:
> >>>
> >>> '''
> >>> \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device
> Types
> >> / Crypto Device / Device Operation / Symmetric algorithms Operation}
> >>> ...
> >>> \item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the
> >> driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto
> >> requests.
> >>> Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
> >>> ...
> >>> '''
> >>>
> >>
> >> As far as I can remember, we have already agreed that in terms of the
> >> spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your
> >
> > Sorry, I don't think so. :(
> >
> >> code you have a substantially different struct virtio_crypto_op_data_req
> >> than in your spec! For instance in the spec virtio_crypto_op_data_req is
> >> the full request and contains the data buffers (src_data and the
> >> dest_data), while in your code it's effectively just a header and does
> >> not contain any data buffers.
> >>
> > I said struct virtio_crypto_op_data_req in the spec is just a symbol.
> > I didn't find a better way to express the src_data and dst_data etc. So
> > I used u8[len] xxx_data to occupy a sit in the request.
> >
> 
> OK, tell me how is the reader/implementer of the spec supposed to figure
> out that a 124 byte padded "header" needs to be precede any "data"?
> 
If those people use the asked request based on the spec, 
they don't need to worry about the pad IMHO.

> Besides if you look at
> 
> +Stateless mode HASH service requests are as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_hash_para_statelesss {
> +    struct {
> +        /* See VIRTIO_CRYPTO_HASH_* above */
> +        le32 algo;
> +    } sess_para;
> +
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* hash result length */
> +    le32 hash_result_len;
> +    le32 reserved;
> +};
> +struct virtio_crypto_hash_data_req_stateless {
> +    /* Device-readable part */
> +    struct virtio_crypto_hash_para_stateless para;
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +
> +    /* Device-writable part */
> +    /* Hash result data */
> +    u8 hash_result[hash_result_len];
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> 
> from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device
> specification". You need the padding to 128 bytes after
> virtio_crypto_hash_para_stateless para, but before src_data. But there is
> no indication in the definition of virtio_crypto_hash_data_req_stateless
> nor somewhere in the spec about that. On the contrary based on this
> one would expect para.reserved and src_data being pretty adjecent.
> 
> Thus the normative statement you quoted is (IMHO) ill suited and
> insufficient to express what you have been trying to express.
> 
That's indeed a problem. I can't find a good way to express my thoughts
in the spec. Make me sad.~

> >>
> >>>> AFAIU here you allow only requests of two sizes: one fixed size
> >>>> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
> >>>> means that some requests need quite some padding between what
> >>>> you call the 'request' and the actual data on which the crypto
> >>>> operation dictated by the 'request' needs to be performed.
> >>>
> >>> Yes, that's true.
> >>>
> >>
> >> This implies that should we need more than 128 bytes for a request,
> >> we will need to introduce jet another request format and negotiate it
> >> via feature bits.
> >>
> > Why do we need other feature bits?
> 
> Because assume you have something that needs more that 128 bytes for
> its request, how do you solve the problem without new format end new
> feature bit?
> 
Oh, maybe I get your point now. You mean the future use for some algorithm requests
use more than 128 bytes? If so, we have to introduce new feature bits.
AFAICT the 128 bytes is enough for sym and asym algorithms currently. Xin Zeng? Any thoughts?

> >
> >> By the way, I'm having a hard time understing how is the requirement form
> >>
> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
> >> 004
> >> (2.4.4 Message Framing) satisfied by this code. Could you explain this
> >> to me please?
> >>
> > Sorry for my bad English,
> > I don't know which normative formulation the code violates?
> 
> I'm not sure it's actually violated, but I'm concerned about the following
> normative statement: "The device MUST NOT make assumptions about the
> particular
> arrangement of descriptors. The device MAY have a reasonable limit of
> descriptors it will allow in a chain."
> 
> Please also read the explanatory part I've linked, because the normative
> statement is pretty abstract.
> 
> In my understanding, the spec says, that e.g. the virti-crypto device
> should not fail if a single request is split up into let's say two chunks
> and transmitted by the means of two top level descriptors.
> 
> Do you agree with my reading of the spec?
> 
Yes, I agree. But what's the relationship about the request here?
We don't assume the request have to use one desc entity, it can
use scatter-gather list for one request header. 
The device can cover the situation in the QEMU. 

> What does the virtio-crypto device do if it encounters such a situation?
> 
This isn't a problem. Pls see blow code segment:

virtio_crypto_handle_request()
{...
if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
                != sizeof(req))) {
    virtio_error(vdev, "virtio-crypto request outhdr too short");
    return -1;
}
iov_discard_front(&out_iov, &out_num, sizeof(req));
...


Thanks,
-Gonglei

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
>>>> By the way, I'm having a hard time understing how is the requirement form
>>>>
>> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
>>>> 004
>>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this
>>>> to me please?
>>>>
>>> Sorry for my bad English,
>>> I don't know which normative formulation the code violates?
>> I'm not sure it's actually violated, but I'm concerned about the following
>> normative statement: "The device MUST NOT make assumptions about the
>> particular
>> arrangement of descriptors. The device MAY have a reasonable limit of
>> descriptors it will allow in a chain."
>>
>> Please also read the explanatory part I've linked, because the normative
>> statement is pretty abstract.
>>
>> In my understanding, the spec says, that e.g. the virti-crypto device
>> should not fail if a single request is split up into let's say two chunks
>> and transmitted by the means of two top level descriptors.
>>
>> Do you agree with my reading of the spec?
>>
> Yes, I agree. But what's the relationship about the request here?
> We don't assume the request have to use one desc entity, it can
> use scatter-gather list for one request header. 
> The device can cover the situation in the QEMU. 
> 
>> What does the virtio-crypto device do if it encounters such a situation?
>>
> This isn't a problem. Pls see blow code segment:
> 
> virtio_crypto_handle_request()
> {...
> if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>                 != sizeof(req))) {
>     virtio_error(vdev, "virtio-crypto request outhdr too short");
>     return -1;
> }
> iov_discard_front(&out_iov, &out_num, sizeof(req));
> ...
> 

Thats exactly what worries me. I see a call to virtio_error there...


void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
{
    va_list ap;

    va_start(ap, fmt);
    error_vreport(fmt, ap);
    va_end(ap);

    vdev->broken = true;

    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
        virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
        virtio_notify_config(vdev);
    }
}

This however seems to make the device 'broken'. Or am I missing something?

Halil

> 
> Thanks,
> -Gonglei
> 

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Gonglei (Arei)]

> 
> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
> >>>> By the way, I'm having a hard time understing how is the requirement
> form
> >>>>
> >>
> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
> >>>> 004
> >>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this
> >>>> to me please?
> >>>>
> >>> Sorry for my bad English,
> >>> I don't know which normative formulation the code violates?
> >> I'm not sure it's actually violated, but I'm concerned about the following
> >> normative statement: "The device MUST NOT make assumptions about the
> >> particular
> >> arrangement of descriptors. The device MAY have a reasonable limit of
> >> descriptors it will allow in a chain."
> >>
> >> Please also read the explanatory part I've linked, because the normative
> >> statement is pretty abstract.
> >>
> >> In my understanding, the spec says, that e.g. the virti-crypto device
> >> should not fail if a single request is split up into let's say two chunks
> >> and transmitted by the means of two top level descriptors.
> >>
> >> Do you agree with my reading of the spec?
> >>
> > Yes, I agree. But what's the relationship about the request here?
> > We don't assume the request have to use one desc entity, it can
> > use scatter-gather list for one request header.
> > The device can cover the situation in the QEMU.
> >
> >> What does the virtio-crypto device do if it encounters such a situation?
> >>
> > This isn't a problem. Pls see blow code segment:
> >
> > virtio_crypto_handle_request()
> > {...
> > if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> >                 != sizeof(req))) {
> >     virtio_error(vdev, "virtio-crypto request outhdr too short");
> >     return -1;
> > }
> > iov_discard_front(&out_iov, &out_num, sizeof(req));
> > ...
> >
> 
> Thats exactly what worries me. I see a call to virtio_error there...
> 
> 
> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> {
>     va_list ap;
> 
>     va_start(ap, fmt);
>     error_vreport(fmt, ap);
>     va_end(ap);
> 
>     vdev->broken = true;
> 
>     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>         virtio_set_status(vdev, vdev->status |
> VIRTIO_CONFIG_S_NEEDS_RESET);
>         virtio_notify_config(vdev);
>     }
> }
> 
> This however seems to make the device 'broken'. Or am I missing something?
> 
Yes, if the parse process failed, the device will broke. 
But This is a exception scenario IMHO, which is the same situation
with other virtio devices. 

Stefan introduced the virtio_error().

Thanks,
-Gonglei

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/17/2017 12:13 PM, Gonglei (Arei) wrote:
>>
>> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
>>>>>> By the way, I'm having a hard time understing how is the requirement
>> form
>>>>>>
>>>>
>> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
>>>>>> 004
>>>>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this
>>>>>> to me please?
>>>>>>
>>>>> Sorry for my bad English,
>>>>> I don't know which normative formulation the code violates?
>>>> I'm not sure it's actually violated, but I'm concerned about the following
>>>> normative statement: "The device MUST NOT make assumptions about the
>>>> particular
>>>> arrangement of descriptors. The device MAY have a reasonable limit of
>>>> descriptors it will allow in a chain."
>>>>
>>>> Please also read the explanatory part I've linked, because the normative
>>>> statement is pretty abstract.
>>>>
>>>> In my understanding, the spec says, that e.g. the virti-crypto device
>>>> should not fail if a single request is split up into let's say two chunks
>>>> and transmitted by the means of two top level descriptors.
>>>>
>>>> Do you agree with my reading of the spec?
>>>>
>>> Yes, I agree. But what's the relationship about the request here?
>>> We don't assume the request have to use one desc entity, it can
>>> use scatter-gather list for one request header.
>>> The device can cover the situation in the QEMU.
>>>
>>>> What does the virtio-crypto device do if it encounters such a situation?
>>>>
>>> This isn't a problem. Pls see blow code segment:
>>>
>>> virtio_crypto_handle_request()
>>> {...
>>> if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>                 != sizeof(req))) {
>>>     virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>     return -1;
>>> }
>>> iov_discard_front(&out_iov, &out_num, sizeof(req));
>>> ...
>>>
>>
>> Thats exactly what worries me. I see a call to virtio_error there...
>>
>>
>> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
>> {
>>     va_list ap;
>>
>>     va_start(ap, fmt);
>>     error_vreport(fmt, ap);
>>     va_end(ap);
>>
>>     vdev->broken = true;
>>
>>     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>>         virtio_set_status(vdev, vdev->status |
>> VIRTIO_CONFIG_S_NEEDS_RESET);
>>         virtio_notify_config(vdev);
>>     }
>> }
>>
>> This however seems to make the device 'broken'. Or am I missing something?
>>
> Yes, if the parse process failed, the device will broke. 
> But This is a exception scenario IMHO, which is the same situation
> with other virtio devices. 

I know that virtio-blk does the same. I'm not sure my reading of the
spec is correct. Maybe Stefan, Michael or Connie can clarify this
for us!

By the way for virtio-blk the current handling was introduced by
commit 20ea686a0 (by Greg Kurz), but before we were failing even harder.

Regards,
Halil

> 
> Stefan introduced the virtio_error().
> 
> Thanks,
> -Gonglei
> 

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Cornelia Huck]

On Wed, 17 May 2017 12:33:20 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 05/17/2017 12:13 PM, Gonglei (Arei) wrote:
> >>
> >> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
> >>>>>> By the way, I'm having a hard time understing how is the requirement
> >> form
> >>>>>>
> >>>>
> >> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
> >>>>>> 004
> >>>>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this
> >>>>>> to me please?
> >>>>>>
> >>>>> Sorry for my bad English,
> >>>>> I don't know which normative formulation the code violates?
> >>>> I'm not sure it's actually violated, but I'm concerned about the following
> >>>> normative statement: "The device MUST NOT make assumptions about the
> >>>> particular
> >>>> arrangement of descriptors. The device MAY have a reasonable limit of
> >>>> descriptors it will allow in a chain."
> >>>>
> >>>> Please also read the explanatory part I've linked, because the normative
> >>>> statement is pretty abstract.
> >>>>
> >>>> In my understanding, the spec says, that e.g. the virti-crypto device
> >>>> should not fail if a single request is split up into let's say two chunks
> >>>> and transmitted by the means of two top level descriptors.
> >>>>
> >>>> Do you agree with my reading of the spec?
> >>>>
> >>> Yes, I agree. But what's the relationship about the request here?
> >>> We don't assume the request have to use one desc entity, it can
> >>> use scatter-gather list for one request header.
> >>> The device can cover the situation in the QEMU.
> >>>
> >>>> What does the virtio-crypto device do if it encounters such a situation?
> >>>>
> >>> This isn't a problem. Pls see blow code segment:
> >>>
> >>> virtio_crypto_handle_request()
> >>> {...
> >>> if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> >>>                 != sizeof(req))) {
> >>>     virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>>     return -1;
> >>> }
> >>> iov_discard_front(&out_iov, &out_num, sizeof(req));
> >>> ...
> >>>
> >>
> >> Thats exactly what worries me. I see a call to virtio_error there...
> >>
> >>
> >> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> >> {
> >>     va_list ap;
> >>
> >>     va_start(ap, fmt);
> >>     error_vreport(fmt, ap);
> >>     va_end(ap);
> >>
> >>     vdev->broken = true;
> >>
> >>     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> >>         virtio_set_status(vdev, vdev->status |
> >> VIRTIO_CONFIG_S_NEEDS_RESET);
> >>         virtio_notify_config(vdev);
> >>     }
> >> }
> >>
> >> This however seems to make the device 'broken'. Or am I missing something?
> >>
> > Yes, if the parse process failed, the device will broke. 
> > But This is a exception scenario IMHO, which is the same situation
> > with other virtio devices. 
> 
> I know that virtio-blk does the same. I'm not sure my reading of the
> spec is correct. Maybe Stefan, Michael or Connie can clarify this
> for us!

The spec says for NEEDS_RESET:

"Indicates that the device has experienced an error from which it can't
recover."

For me, this means:
- If this is something that might happen in the normal course of events
and there's a good recovery path, just recover.
- Else, use virtio_error() and require the driver to recover via reset.

If the driver is sending requests in an unexpected format, I'd use
virtio_error(). The format needs to be properly described, though.

> 
> By the way for virtio-blk the current handling was introduced by
> commit 20ea686a0 (by Greg Kurz), but before we were failing even
> harder.
> 
> Regards,
> Halil
> 
> > 
> > Stefan introduced the virtio_error().
> > 
> > Thanks,
> > -Gonglei
> > 

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/17/2017 01:10 PM, Cornelia Huck wrote:
> On Wed, 17 May 2017 12:33:20 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 05/17/2017 12:13 PM, Gonglei (Arei) wrote:
>>>>
>>>> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
>>>>>>>> By the way, I'm having a hard time understing how is the requirement
>>>> form
>>>>>>>>
>>>>>>
>>>> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
>>>>>>>> 004
>>>>>>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this
>>>>>>>> to me please?
>>>>>>>>
>>>>>>> Sorry for my bad English,
>>>>>>> I don't know which normative formulation the code violates?
>>>>>> I'm not sure it's actually violated, but I'm concerned about the following
>>>>>> normative statement: "The device MUST NOT make assumptions about the
>>>>>> particular
>>>>>> arrangement of descriptors. The device MAY have a reasonable limit of
>>>>>> descriptors it will allow in a chain."
>>>>>>
>>>>>> Please also read the explanatory part I've linked, because the normative
>>>>>> statement is pretty abstract.
>>>>>>
>>>>>> In my understanding, the spec says, that e.g. the virti-crypto device
>>>>>> should not fail if a single request is split up into let's say two chunks
>>>>>> and transmitted by the means of two top level descriptors.
>>>>>>
>>>>>> Do you agree with my reading of the spec?
>>>>>>
>>>>> Yes, I agree. But what's the relationship about the request here?
>>>>> We don't assume the request have to use one desc entity, it can
>>>>> use scatter-gather list for one request header.
>>>>> The device can cover the situation in the QEMU.
>>>>>
>>>>>> What does the virtio-crypto device do if it encounters such a situation?
>>>>>>
>>>>> This isn't a problem. Pls see blow code segment:
>>>>>
>>>>> virtio_crypto_handle_request()
>>>>> {...
>>>>> if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>>                 != sizeof(req))) {
>>>>>     virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>>     return -1;
>>>>> }
>>>>> iov_discard_front(&out_iov, &out_num, sizeof(req));
>>>>> ...
>>>>>
>>>>
>>>> Thats exactly what worries me. I see a call to virtio_error there...
>>>>
>>>>
>>>> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
>>>> {
>>>>     va_list ap;
>>>>
>>>>     va_start(ap, fmt);
>>>>     error_vreport(fmt, ap);
>>>>     va_end(ap);
>>>>
>>>>     vdev->broken = true;
>>>>
>>>>     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>>>>         virtio_set_status(vdev, vdev->status |
>>>> VIRTIO_CONFIG_S_NEEDS_RESET);
>>>>         virtio_notify_config(vdev);
>>>>     }
>>>> }
>>>>
>>>> This however seems to make the device 'broken'. Or am I missing something?
>>>>
>>> Yes, if the parse process failed, the device will broke. 
>>> But This is a exception scenario IMHO, which is the same situation
>>> with other virtio devices. 
>>
>> I know that virtio-blk does the same. I'm not sure my reading of the
>> spec is correct. Maybe Stefan, Michael or Connie can clarify this
>> for us!
> 
> The spec says for NEEDS_RESET:
> 
> "Indicates that the device has experienced an error from which it can't
> recover."
> 
> For me, this means:
> - If this is something that might happen in the normal course of events
> and there's a good recovery path, just recover.
> - Else, use virtio_error() and require the driver to recover via reset.
> 
> If the driver is sending requests in an unexpected format, I'd use
> virtio_error(). The format needs to be properly described, though.
> 


All-clear, my bad. After re-reading the spec and virtio_pop I have
realized that virtio_pop pops full descriptor chains. These however
correspond to single requests. Thus at the given point the full request
has to be placed into the queue. Sorry!

>>
>> By the way for virtio-blk the current handling was introduced by
>> commit 20ea686a0 (by Greg Kurz), but before we were failing even
>> harder.
>>
>> Regards,
>> Halil
>>
>>>
>>> Stefan introduced the virtio_error().
>>>
>>> Thanks,
>>> -Gonglei
>>>
> 
> 

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
>> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
>> Sent: Wednesday, May 17, 2017 6:18 AM
>>
>>
>> On 05/16/2017 04:52 AM, Gonglei (Arei) wrote:
>>>> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
>>>>>> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
>>>>>> Sent: Friday, May 12, 2017 7:02 PM
>>>>>>
>>>>>>
>>>>>> On 05/08/2017 01:38 PM, Gonglei wrote:
>>>>>>> According to the new spec, we should use different
>>>>>>> requst structure to store the data request based
>>>>>>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
>>>>>>> negotiated or not.
>>>>>>>
>>>>>>> In this patch, we havn't supported stateless mode
>>>>>>> yet. The device reportes an error if both
>>>>>>> VIRTIO_CRYPTO_F_MUX_MODE and
>>>>>> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
>>>>>>> are negotiated, meanwhile the header.flag doesn't set
>>>>>>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
>>>>>>>
>>>>>>> Let's handle this scenario in the following patches.
>>>>>>>
>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>>>> ---
>>>>>>>  hw/virtio/virtio-crypto.c | 83
>>>>>> ++++++++++++++++++++++++++++++++++++++++-------
>>>>>>>  1 file changed, 71 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
>>>>>>> index 0353eb6..c4b8a2c 100644
>>>>>>> --- a/hw/virtio/virtio-crypto.c
>>>>>>> +++ b/hw/virtio/virtio-crypto.c
>>>>>>> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>>>> *request)
>>>>>>>      VirtQueueElement *elem = &request->elem;
>>>>>>>      int queue_index =
>>>>>> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
>>>>>>>      struct virtio_crypto_op_data_req req;
>>>>>>> +    struct virtio_crypto_op_data_req_mux req_mux;
>>>>>>>      int ret;
>>>>>>>      struct iovec *in_iov;
>>>>>>>      struct iovec *out_iov;
>>>>>>> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>>>> *request)
>>>>>>>      uint64_t session_id;
>>>>>>>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
>>>>>>>      Error *local_err = NULL;
>>>>>>> +    bool mux_mode_is_negotiated;
>>>>>>> +    struct virtio_crypto_op_header *header;
>>>>>>> +    bool is_stateless_req = false;
>>>>>>>
>>>>>>>      if (elem->out_num < 1 || elem->in_num < 1) {
>>>>>>>          virtio_error(vdev, "virtio-crypto dataq missing headers");
>>>>>>> @@ -597,12 +601,28 @@
>> virtio_crypto_handle_request(VirtIOCryptoReq
>>>>>> *request)
>>>>>>>      out_iov = elem->out_sg;
>>>>>>>      in_num = elem->in_num;
>>>>>>>      in_iov = elem->in_sg;
>>>>>>> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>>>> -                != sizeof(req))) {
>>>>>>> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>>>> -        return -1;
>>>>>>> +
>>>>>>> +    mux_mode_is_negotiated =
>>>>>>> +        virtio_vdev_has_feature(vdev,
>>>> VIRTIO_CRYPTO_F_MUX_MODE);
>>>>>>> +    if (!mux_mode_is_negotiated) {
>>>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>>>> +                    != sizeof(req))) {
>>>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too
>> short");
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
>>>>>>> +
>>>>>>> +        header = &req.header;
>>>>>>> +    } else {
>>>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
>>>>>>> +                sizeof(req_mux)) != sizeof(req_mux))) {
>>>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too
>> short");
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
>>>>>>> +
>>>>>>> +        header = &req_mux.header;
>>>>>> I wonder if this request length checking logic is conform to the
>>>>>> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
>>>>>> virtio crypto device specification").
>>>>>>
>>>>> Sure. Please see below normative formulation:
>>>>>
>>>>> '''
>>>>> \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device
>> Types
>>>> / Crypto Device / Device Operation / Symmetric algorithms Operation}
>>>>> ...
>>>>> \item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the
>>>> driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto
>>>> requests.
>>>>> Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
>>>>> ...
>>>>> '''
>>>>>
>>>> As far as I can remember, we have already agreed that in terms of the
>>>> spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your
>>> Sorry, I don't think so. :(
>>>
>>>> code you have a substantially different struct virtio_crypto_op_data_req
>>>> than in your spec! For instance in the spec virtio_crypto_op_data_req is
>>>> the full request and contains the data buffers (src_data and the
>>>> dest_data), while in your code it's effectively just a header and does
>>>> not contain any data buffers.
>>>>
>>> I said struct virtio_crypto_op_data_req in the spec is just a symbol.
>>> I didn't find a better way to express the src_data and dst_data etc. So
>>> I used u8[len] xxx_data to occupy a sit in the request.
>>>
>> OK, tell me how is the reader/implementer of the spec supposed to figure
>> out that a 124 byte padded "header" needs to be precede any "data"?
>>
> If those people use the asked request based on the spec, 
> they don't need to worry about the pad IMHO.
> 

Is this comment of yours outdated? I have described below why I think
there are problems, and below you seem to agree...

>> Besides if you look at
>>
>> +Stateless mode HASH service requests are as follows:
>> +
>> +\begin{lstlisting}
>> +struct virtio_crypto_hash_para_statelesss {
>> +    struct {
>> +        /* See VIRTIO_CRYPTO_HASH_* above */
>> +        le32 algo;
>> +    } sess_para;
>> +
>> +    /* length of source data */
>> +    le32 src_data_len;
>> +    /* hash result length */
>> +    le32 hash_result_len;
>> +    le32 reserved;
>> +};
>> +struct virtio_crypto_hash_data_req_stateless {
>> +    /* Device-readable part */
>> +    struct virtio_crypto_hash_para_stateless para;
>> +    /* Source data */
>> +    u8 src_data[src_data_len];
>> +
>> +    /* Device-writable part */
>> +    /* Hash result data */
>> +    u8 hash_result[hash_result_len];
>> +    struct virtio_crypto_inhdr inhdr;
>> +};
>> +\end{lstlisting}
>> +
>>
>> from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device
>> specification". You need the padding to 128 bytes after
>> virtio_crypto_hash_para_stateless para, but before src_data. But there is
>> no indication in the definition of virtio_crypto_hash_data_req_stateless
>> nor somewhere in the spec about that. On the contrary based on this
>> one would expect para.reserved and src_data being pretty adjecent.
>>
>> Thus the normative statement you quoted is (IMHO) ill suited and
>> insufficient to express what you have been trying to express.
>>
> That's indeed a problem. I can't find a good way to express my thoughts
> in the spec. Make me sad.~
> 

Can't really tell if we are in an agreement based on your reply above.
If we are not, please tell.

If we are we have two paths:
1) Give up on the concept of same 'header' length. You could easily
branch on the common header and do everything else algorithm specific.
2) Find a way to clean up the mess:
   * Bring to expression that the  struct virtio_crypto_op_data_req
     from the code ain't the full request (e.g. by postfix-ing _header).
     Same for mux.
   * Update the description in the spec so that it's compatible with
     what your implementations are doing.

>>>>>> AFAIU here you allow only requests of two sizes: one fixed size
>>>>>> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
>>>>>> means that some requests need quite some padding between what
>>>>>> you call the 'request' and the actual data on which the crypto
>>>>>> operation dictated by the 'request' needs to be performed.
>>>>> Yes, that's true.
>>>>>
>>>> This implies that should we need more than 128 bytes for a request,
>>>> we will need to introduce jet another request format and negotiate it
>>>> via feature bits.
>>>>
>>> Why do we need other feature bits?
>> Because assume you have something that needs more that 128 bytes for
>> its request, how do you solve the problem without new format end new
>> feature bit?
>>
> Oh, maybe I get your point now. You mean the future use for some algorithm requests
> use more than 128 bytes? If so, we have to introduce new feature bits.
> AFAICT the 128 bytes is enough for sym and asym algorithms currently. Xin Zeng? Any thoughts?
> 

That's what I was trying to say.

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Gonglei (Arei)]

> 
> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
> >> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
> >> Sent: Wednesday, May 17, 2017 6:18 AM
> >>
> >>
> >> On 05/16/2017 04:52 AM, Gonglei (Arei) wrote:
> >>>> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
> >>>>>> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
> >>>>>> Sent: Friday, May 12, 2017 7:02 PM
> >>>>>>
> >>>>>>
> >>>>>> On 05/08/2017 01:38 PM, Gonglei wrote:
> >>>>>>> According to the new spec, we should use different
> >>>>>>> requst structure to store the data request based
> >>>>>>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
> >>>>>>> negotiated or not.
> >>>>>>>
> >>>>>>> In this patch, we havn't supported stateless mode
> >>>>>>> yet. The device reportes an error if both
> >>>>>>> VIRTIO_CRYPTO_F_MUX_MODE and
> >>>>>> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
> >>>>>>> are negotiated, meanwhile the header.flag doesn't set
> >>>>>>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> >>>>>>>
> >>>>>>> Let's handle this scenario in the following patches.
> >>>>>>>
> >>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >>>>>>> ---
> >>>>>>>  hw/virtio/virtio-crypto.c | 83
> >>>>>> ++++++++++++++++++++++++++++++++++++++++-------
> >>>>>>>  1 file changed, 71 insertions(+), 12 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> >>>>>>> index 0353eb6..c4b8a2c 100644
> >>>>>>> --- a/hw/virtio/virtio-crypto.c
> >>>>>>> +++ b/hw/virtio/virtio-crypto.c
> >>>>>>> @@ -577,6 +577,7 @@
> virtio_crypto_handle_request(VirtIOCryptoReq
> >>>>>> *request)
> >>>>>>>      VirtQueueElement *elem = &request->elem;
> >>>>>>>      int queue_index =
> >>>>>> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> >>>>>>>      struct virtio_crypto_op_data_req req;
> >>>>>>> +    struct virtio_crypto_op_data_req_mux req_mux;
> >>>>>>>      int ret;
> >>>>>>>      struct iovec *in_iov;
> >>>>>>>      struct iovec *out_iov;
> >>>>>>> @@ -587,6 +588,9 @@
> virtio_crypto_handle_request(VirtIOCryptoReq
> >>>>>> *request)
> >>>>>>>      uint64_t session_id;
> >>>>>>>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
> >>>>>>>      Error *local_err = NULL;
> >>>>>>> +    bool mux_mode_is_negotiated;
> >>>>>>> +    struct virtio_crypto_op_header *header;
> >>>>>>> +    bool is_stateless_req = false;
> >>>>>>>
> >>>>>>>      if (elem->out_num < 1 || elem->in_num < 1) {
> >>>>>>>          virtio_error(vdev, "virtio-crypto dataq missing headers");
> >>>>>>> @@ -597,12 +601,28 @@
> >> virtio_crypto_handle_request(VirtIOCryptoReq
> >>>>>> *request)
> >>>>>>>      out_iov = elem->out_sg;
> >>>>>>>      in_num = elem->in_num;
> >>>>>>>      in_iov = elem->in_sg;
> >>>>>>> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> >>>>>>> -                != sizeof(req))) {
> >>>>>>> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>>>>>> -        return -1;
> >>>>>>> +
> >>>>>>> +    mux_mode_is_negotiated =
> >>>>>>> +        virtio_vdev_has_feature(vdev,
> >>>> VIRTIO_CRYPTO_F_MUX_MODE);
> >>>>>>> +    if (!mux_mode_is_negotiated) {
> >>>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req,
> sizeof(req))
> >>>>>>> +                    != sizeof(req))) {
> >>>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too
> >> short");
> >>>>>>> +            return -1;
> >>>>>>> +        }
> >>>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
> >>>>>>> +
> >>>>>>> +        header = &req.header;
> >>>>>>> +    } else {
> >>>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
> >>>>>>> +                sizeof(req_mux)) != sizeof(req_mux))) {
> >>>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too
> >> short");
> >>>>>>> +            return -1;
> >>>>>>> +        }
> >>>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
> >>>>>>> +
> >>>>>>> +        header = &req_mux.header;
> >>>>>> I wonder if this request length checking logic is conform to the
> >>>>>> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
> >>>>>> virtio crypto device specification").
> >>>>>>
> >>>>> Sure. Please see below normative formulation:
> >>>>>
> >>>>> '''
> >>>>> \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device
> >> Types
> >>>> / Crypto Device / Device Operation / Symmetric algorithms Operation}
> >>>>> ...
> >>>>> \item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated,
> the
> >>>> driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto
> >>>> requests.
> >>>>> Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
> >>>>> ...
> >>>>> '''
> >>>>>
> >>>> As far as I can remember, we have already agreed that in terms of the
> >>>> spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your
> >>> Sorry, I don't think so. :(
> >>>
> >>>> code you have a substantially different struct virtio_crypto_op_data_req
> >>>> than in your spec! For instance in the spec virtio_crypto_op_data_req is
> >>>> the full request and contains the data buffers (src_data and the
> >>>> dest_data), while in your code it's effectively just a header and does
> >>>> not contain any data buffers.
> >>>>
> >>> I said struct virtio_crypto_op_data_req in the spec is just a symbol.
> >>> I didn't find a better way to express the src_data and dst_data etc. So
> >>> I used u8[len] xxx_data to occupy a sit in the request.
> >>>
> >> OK, tell me how is the reader/implementer of the spec supposed to figure
> >> out that a 124 byte padded "header" needs to be precede any "data"?
> >>
> > If those people use the asked request based on the spec,
> > they don't need to worry about the pad IMHO.
> >
> 
> Is this comment of yours outdated? I have described below why I think
> there are problems, and below you seem to agree...
> 
Yes.

> >> Besides if you look at
> >>
> >> +Stateless mode HASH service requests are as follows:
> >> +
> >> +\begin{lstlisting}
> >> +struct virtio_crypto_hash_para_statelesss {
> >> +    struct {
> >> +        /* See VIRTIO_CRYPTO_HASH_* above */
> >> +        le32 algo;
> >> +    } sess_para;
> >> +
> >> +    /* length of source data */
> >> +    le32 src_data_len;
> >> +    /* hash result length */
> >> +    le32 hash_result_len;
> >> +    le32 reserved;
> >> +};
> >> +struct virtio_crypto_hash_data_req_stateless {
> >> +    /* Device-readable part */
> >> +    struct virtio_crypto_hash_para_stateless para;
> >> +    /* Source data */
> >> +    u8 src_data[src_data_len];
> >> +
> >> +    /* Device-writable part */
> >> +    /* Hash result data */
> >> +    u8 hash_result[hash_result_len];
> >> +    struct virtio_crypto_inhdr inhdr;
> >> +};
> >> +\end{lstlisting}
> >> +
> >>
> >> from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device
> >> specification". You need the padding to 128 bytes after
> >> virtio_crypto_hash_para_stateless para, but before src_data. But there is
> >> no indication in the definition of virtio_crypto_hash_data_req_stateless
> >> nor somewhere in the spec about that. On the contrary based on this
> >> one would expect para.reserved and src_data being pretty adjecent.
> >>
> >> Thus the normative statement you quoted is (IMHO) ill suited and
> >> insufficient to express what you have been trying to express.
> >>
> > That's indeed a problem. I can't find a good way to express my thoughts
> > in the spec. Make me sad.~
> >
> 
> Can't really tell if we are in an agreement based on your reply above.
> If we are not, please tell.
> 
> If we are we have two paths:
> 1) Give up on the concept of same 'header' length. You could easily
> branch on the common header and do everything else algorithm specific.
> 2) Find a way to clean up the mess:
>    * Bring to expression that the  struct virtio_crypto_op_data_req
>      from the code ain't the full request (e.g. by postfix-ing _header).
>      Same for mux.
>    * Update the description in the spec so that it's compatible with
>      what your implementations are doing.
> 
Could you pls explain more about those two ways? Oh give me an example
for each other. Which way do you like better?

Thanks,
-Gonglei

> >>>>>> AFAIU here you allow only requests of two sizes: one fixed size
> >>>>>> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
> >>>>>> means that some requests need quite some padding between what
> >>>>>> you call the 'request' and the actual data on which the crypto
> >>>>>> operation dictated by the 'request' needs to be performed.
> >>>>> Yes, that's true.
> >>>>>
> >>>> This implies that should we need more than 128 bytes for a request,
> >>>> we will need to introduce jet another request format and negotiate it
> >>>> via feature bits.
> >>>>
> >>> Why do we need other feature bits?
> >> Because assume you have something that needs more that 128 bytes for
> >> its request, how do you solve the problem without new format end new
> >> feature bit?
> >>
> > Oh, maybe I get your point now. You mean the future use for some algorithm
> requests
> > use more than 128 bytes? If so, we have to introduce new feature bits.
> > AFAICT the 128 bytes is enough for sym and asym algorithms currently. Xin
> Zeng? Any thoughts?
> >
> 
> That's what I was trying to say.

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/18/2017 03:21 PM, Gonglei (Arei) wrote:
>>>> Besides if you look at
>>>>
>>>> +Stateless mode HASH service requests are as follows:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_crypto_hash_para_statelesss {
>>>> +    struct {
>>>> +        /* See VIRTIO_CRYPTO_HASH_* above */
>>>> +        le32 algo;
>>>> +    } sess_para;
>>>> +
>>>> +    /* length of source data */
>>>> +    le32 src_data_len;
>>>> +    /* hash result length */
>>>> +    le32 hash_result_len;
>>>> +    le32 reserved;
>>>> +};
>>>> +struct virtio_crypto_hash_data_req_stateless {
>>>> +    /* Device-readable part */
>>>> +    struct virtio_crypto_hash_para_stateless para;
>>>> +    /* Source data */
>>>> +    u8 src_data[src_data_len];
>>>> +
>>>> +    /* Device-writable part */
>>>> +    /* Hash result data */
>>>> +    u8 hash_result[hash_result_len];
>>>> +    struct virtio_crypto_inhdr inhdr;
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>>
>>>> from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device
>>>> specification". You need the padding to 128 bytes after
>>>> virtio_crypto_hash_para_stateless para, but before src_data. But there is
>>>> no indication in the definition of virtio_crypto_hash_data_req_stateless
>>>> nor somewhere in the spec about that. On the contrary based on this
>>>> one would expect para.reserved and src_data being pretty adjecent.
>>>>
>>>> Thus the normative statement you quoted is (IMHO) ill suited and
>>>> insufficient to express what you have been trying to express.
>>>>
>>> That's indeed a problem. I can't find a good way to express my thoughts
>>> in the spec. Make me sad.~
>>>
>> Can't really tell if we are in an agreement based on your reply above.
>> If we are not, please tell.
>>
>> If we are we have two paths:
>> 1) Give up on the concept of same 'header' length. You could easily
>> branch on the common header and do everything else algorithm specific.
>> 2) Find a way to clean up the mess:
>>    * Bring to expression that the  struct virtio_crypto_op_data_req
>>      from the code ain't the full request (e.g. by postfix-ing _header).
>>      Same for mux.
>>    * Update the description in the spec so that it's compatible with
>>      what your implementations are doing.
>>
> Could you pls explain more about those two ways? Oh give me an example
> for each other. Which way do you like better?

Sorry, I'm quite busy at the time and it does not look like I will be
able to find the time for providing a more detailed explanation. Regarding
my preferences I think 1) is the more straight-forward approach at least
conceptually. OTOH I would need to implement 1) to verify that...

Regards,
Halil

[Qemu-devel] [RFC v1 7/9] virtio-crypto: add stateless crypto request handler

[Gonglei]

We can support stateless crypto request now.
The stateless cipher request componet is:
 header + key + iv + src_data + dst_data
and The algorithm chainning stateless request
component is:
 header + key + auth_key + iv + aad + src_data +
 dst_data + hash_result

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 backends/cryptodev-builtin.c |   3 +-
 hw/virtio/virtio-crypto.c    | 248 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 246 insertions(+), 5 deletions(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index 7829999..40dc568 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -91,7 +91,8 @@ static void cryptodev_builtin_init(
      * Why this value? Just avoid to overflow when
      * memory allocation for each crypto request.
      */
-    backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendSymOpInfo);
+    backend->conf.max_size = LONG_MAX -
+                    sizeof(CryptoDevBackendSymStatelessInfo);
     backend->conf.max_cipher_key_len = CRYPTODEV_BUITLIN_MAX_CIPHER_KEY_LEN;
     backend->conf.max_auth_key_len = CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN;
 
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index c4b8a2c..5422f25 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -335,9 +335,10 @@ static void virtio_crypto_init_request(VirtIOCrypto *vcrypto, VirtQueue *vq,
 
 static void virtio_crypto_free_request(VirtIOCryptoReq *req)
 {
+    size_t max_len;
+
     if (req) {
         if (req->flags == CRYPTODEV_BACKEND_ALG_SYM) {
-            size_t max_len;
             CryptoDevBackendSymOpInfo *op_info = req->u.sym_op_info;
 
             max_len = op_info->iv_len +
@@ -349,6 +350,21 @@ static void virtio_crypto_free_request(VirtIOCryptoReq *req)
             /* Zeroize and free request data structure */
             memset(op_info, 0, sizeof(*op_info) + max_len);
             g_free(op_info);
+        } else if (req->flags == CRYPTODEV_BACKEND_ALG_SYM_STATELESS) {
+            CryptoDevBackendSymStatelessInfo *sym_stateless_info;
+
+            sym_stateless_info = req->u.sym_stateless_info;
+            max_len = sym_stateless_info->session_info.key_len +
+                sym_stateless_info->session_info.auth_key_len +
+                sym_stateless_info->op_info.iv_len +
+                sym_stateless_info->op_info.src_len +
+                sym_stateless_info->op_info.dst_len +
+                sym_stateless_info->op_info.aad_len +
+                sym_stateless_info->op_info.digest_result_len;
+            /* Zeroize and free request data structure */
+            memset(sym_stateless_info, 0,
+                sizeof(*sym_stateless_info) + max_len);
+            g_free(sym_stateless_info);
         }
         g_free(req);
     }
@@ -396,6 +412,9 @@ static void virtio_crypto_req_complete(VirtIOCryptoReq *req, uint8_t status)
     if (req->flags == CRYPTODEV_BACKEND_ALG_SYM) {
         virtio_crypto_sym_input_data_helper(vdev, req, status,
                                             req->u.sym_op_info);
+    } else if (req->flags == CRYPTODEV_BACKEND_ALG_SYM_STATELESS) {
+        virtio_crypto_sym_input_data_helper(vdev, req, status,
+                &req->u.sym_stateless_info->op_info);
     }
     stb_p(&req->in->status, status);
     virtqueue_push(req->vq, &req->elem, req->in_len);
@@ -570,6 +589,221 @@ virtio_crypto_handle_sym_req(VirtIOCrypto *vcrypto,
 }
 
 static int
+virtio_crypto_handle_sym_stateless_req(VirtIOCrypto *vcrypto,
+               struct virtio_crypto_sym_data_req_stateless *req,
+               CryptoDevBackendSymStatelessInfo **stateless_info,
+               struct iovec *iov, unsigned int out_num)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
+    CryptoDevBackendSymStatelessInfo *sym_stateless_info;
+
+    uint32_t op_type;
+    uint32_t src_len = 0, dst_len = 0;
+    uint32_t iv_len = 0;
+    uint32_t aad_len = 0, hash_result_len = 0;
+    uint32_t hash_start_src_offset = 0, len_to_hash = 0;
+    uint32_t cipher_start_src_offset = 0, len_to_cipher = 0;
+    uint32_t key_len = 0, auth_key_len = 0;
+
+    uint64_t max_len, curr_size = 0;
+    size_t s;
+
+    op_type = ldl_le_p(&req->op_type);
+
+    if (op_type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
+        key_len = ldl_le_p(&req->u.cipher.para.sess_para.keylen);
+        iv_len = ldl_le_p(&req->u.cipher.para.iv_len);
+        src_len = ldl_le_p(&req->u.cipher.para.src_data_len);
+        dst_len = ldl_le_p(&req->u.cipher.para.dst_data_len);
+    } else if (op_type == VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING) {
+        key_len = ldl_le_p(&req->u.chain.para.sess_para.cipher.keylen);
+        auth_key_len =
+            ldl_le_p(&req->u.chain.para.sess_para.hash.auth_key_len);
+        iv_len = ldl_le_p(&req->u.chain.para.iv_len);
+        src_len = ldl_le_p(&req->u.chain.para.src_data_len);
+        dst_len = ldl_le_p(&req->u.chain.para.dst_data_len);
+
+        aad_len = ldl_le_p(&req->u.chain.para.aad_len);
+        hash_result_len = ldl_le_p(&req->u.chain.para.hash_result_len);
+        hash_start_src_offset = ldl_le_p(
+                         &req->u.chain.para.hash_start_src_offset);
+        cipher_start_src_offset = ldl_le_p(
+                         &req->u.chain.para.cipher_start_src_offset);
+        len_to_cipher = ldl_le_p(&req->u.chain.para.len_to_cipher);
+        len_to_hash = ldl_le_p(&req->u.chain.para.len_to_hash);
+    } else {
+        /* VIRTIO_CRYPTO_SYM_OP_NONE */
+        error_report("virtio-crypto unsupported cipher type");
+        return -VIRTIO_CRYPTO_NOTSUPP;
+    }
+
+    if (key_len > vcrypto->conf.max_cipher_key_len) {
+        virtio_error(vdev,
+            "virtio-crypto length of cipher key is too big: %u", key_len);
+        return -EFAULT;
+    }
+
+    if (auth_key_len > vcrypto->conf.max_auth_key_len) {
+        virtio_error(vdev,
+         "virtio-crypto length of auth key is too big: %u", auth_key_len);
+        return -EFAULT;
+    }
+
+    max_len = (uint64_t)key_len + auth_key_len + iv_len + aad_len +
+                src_len + dst_len + hash_result_len;
+    if (unlikely(max_len > vcrypto->conf.max_size)) {
+        virtio_error(vdev, "virtio-crypto too big length");
+        return -EFAULT;
+    }
+
+    sym_stateless_info =
+            g_malloc0(sizeof(CryptoDevBackendSymStatelessInfo) + max_len);
+    sym_stateless_info->session_info.key_len = key_len;
+    sym_stateless_info->session_info.auth_key_len = auth_key_len;
+    sym_stateless_info->op_info.iv_len = iv_len;
+    sym_stateless_info->op_info.src_len = src_len;
+    sym_stateless_info->op_info.dst_len = dst_len;
+    sym_stateless_info->op_info.aad_len = aad_len;
+    sym_stateless_info->op_info.digest_result_len = hash_result_len;
+    sym_stateless_info->op_info.hash_start_src_offset =
+                                            hash_start_src_offset;
+    sym_stateless_info->op_info.len_to_hash = len_to_hash;
+    sym_stateless_info->op_info.cipher_start_src_offset =
+                                            cipher_start_src_offset;
+    sym_stateless_info->op_info.len_to_cipher = len_to_cipher;
+
+    sym_stateless_info->session_info.op_type =
+                    sym_stateless_info->op_info.op_type = op_type;
+    if (op_type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
+        sym_stateless_info->session_info.cipher_alg =
+                    ldl_le_p(&req->u.cipher.para.sess_para.algo);
+        sym_stateless_info->session_info.direction =
+                    ldl_le_p(&req->u.cipher.para.sess_para.op);
+    } else { /* It must be algorithm chain here */
+        sym_stateless_info->session_info.cipher_alg =
+                    ldl_le_p(&req->u.chain.para.sess_para.cipher.algo);
+        sym_stateless_info->session_info.direction =
+                    ldl_le_p(&req->u.chain.para.sess_para.cipher.op);
+        sym_stateless_info->session_info.hash_alg =
+                    ldl_le_p(&req->u.chain.para.sess_para.hash.algo);
+        sym_stateless_info->session_info.hash_mode =
+                    ldl_le_p(&req->u.chain.para.sess_para.hash.hash_mode);
+        sym_stateless_info->session_info.alg_chain_order =
+                    ldl_le_p(&req->u.chain.para.sess_para.alg_chain_order);
+    }
+
+    DPRINTF("cipher_alg=%" PRIu32 ", info->direction=%" PRIu32 "\n",
+            sym_stateless_info->session_info.cipher_alg,
+            sym_stateless_info->session_info.direction);
+    /* Begin to parse the buffer */
+
+    /*
+     * Cipher request components:
+     *   header + key + iv + src_data + dst_data
+     *
+     * Alg_chainning request components:
+     *   header + key + auth_key + iv + aad + src_data + dst_data + hash_result
+     */
+     if (key_len > 0) {
+        DPRINTF("key_len=%" PRIu32 "\n", key_len);
+        sym_stateless_info->session_info.cipher_key =
+                sym_stateless_info->op_info.data + curr_size;
+
+        s = iov_to_buf(iov, out_num, 0,
+            sym_stateless_info->session_info.cipher_key, key_len);
+        if (unlikely(s != key_len)) {
+            virtio_error(vdev, "virtio-crypto cipher key incorrect");
+            goto err;
+        }
+        iov_discard_front(&iov, &out_num, key_len);
+        curr_size += key_len;
+    }
+    if (auth_key_len > 0) {
+        DPRINTF("auth_key_len=%" PRIu32 "\n", auth_key_len);
+        sym_stateless_info->session_info.auth_key =
+                sym_stateless_info->op_info.data + curr_size;
+
+        s = iov_to_buf(iov, out_num, 0,
+            sym_stateless_info->session_info.auth_key, auth_key_len);
+        if (unlikely(s != auth_key_len)) {
+            virtio_error(vdev, "virtio-crypto auth key incorrect");
+            goto err;
+        }
+        iov_discard_front(&iov, &out_num, auth_key_len);
+        curr_size += auth_key_len;
+    }
+    if (iv_len > 0) {
+        DPRINTF("iv_len=%" PRIu32 "\n", iv_len);
+        sym_stateless_info->op_info.iv =
+            sym_stateless_info->op_info.data + curr_size;
+
+        s = iov_to_buf(iov, out_num, 0,
+            sym_stateless_info->op_info.iv, iv_len);
+        if (unlikely(s != iv_len)) {
+            virtio_error(vdev, "virtio-crypto iv incorrect");
+            goto err;
+        }
+        iov_discard_front(&iov, &out_num, iv_len);
+        curr_size += iv_len;
+    }
+
+    /* Handle additional authentication data if exists */
+    if (aad_len > 0) {
+        DPRINTF("aad_len=%" PRIu32 "\n", aad_len);
+        sym_stateless_info->op_info.aad_data =
+            sym_stateless_info->op_info.data + curr_size;
+
+        s = iov_to_buf(iov, out_num, 0,
+            sym_stateless_info->op_info.aad_data, aad_len);
+        if (unlikely(s != aad_len)) {
+            virtio_error(vdev, "virtio-crypto additional auth data incorrect");
+            goto err;
+        }
+        iov_discard_front(&iov, &out_num, aad_len);
+
+        curr_size += aad_len;
+    }
+    /* Handle the source data */
+    if (src_len > 0) {
+        DPRINTF("src_len=%" PRIu32 "\n", src_len);
+        sym_stateless_info->op_info.src =
+            sym_stateless_info->op_info.data + curr_size;
+
+        s = iov_to_buf(iov, out_num, 0,
+            sym_stateless_info->op_info.src, src_len);
+        if (unlikely(s != src_len)) {
+            virtio_error(vdev, "virtio-crypto source data incorrect");
+            goto err;
+        }
+        iov_discard_front(&iov, &out_num, src_len);
+
+        curr_size += src_len;
+    }
+
+    /* Handle the destination data */
+    sym_stateless_info->op_info.dst =
+        sym_stateless_info->op_info.data + curr_size;
+    curr_size += dst_len;
+
+    DPRINTF("dst_len=%" PRIu32 "\n", dst_len);
+
+    /* Handle the hash digest result */
+    if (hash_result_len > 0) {
+        DPRINTF("hash_result_len=%" PRIu32 "\n", hash_result_len);
+        sym_stateless_info->op_info.digest_result =
+            sym_stateless_info->op_info.data + curr_size;
+    }
+
+    *stateless_info = sym_stateless_info;
+
+    return 0;
+
+err:
+    g_free(sym_stateless_info);
+    return -EFAULT;
+}
+
+static int
 virtio_crypto_handle_request(VirtIOCryptoReq *request)
 {
     VirtIOCrypto *vcrypto = request->vcrypto;
@@ -591,6 +825,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     bool mux_mode_is_negotiated;
     struct virtio_crypto_op_header *header;
     bool is_stateless_req = false;
+    CryptoDevBackendSymStatelessInfo *stateless_info = NULL;
 
     if (elem->out_num < 1 || elem->in_num < 1) {
         virtio_error(vdev, "virtio-crypto dataq missing headers");
@@ -681,9 +916,10 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
                      * Handle stateless mode, that is
                      * header->flag == VIRTIO_CRYPTO_FLAG_STATELESS_MODE
                      */
-                    virtio_error(vdev,
-                        "virtio-crypto do not support stateless mode");
-                    return -1;
+                    ret = virtio_crypto_handle_sym_stateless_req(vcrypto,
+                            &req_mux.u.sym_stateless_req,
+                            &stateless_info,
+                            out_iov, out_num);
                 }
             }
         }
@@ -702,6 +938,10 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
                 /* Set request's parameter */
                 request->flags = CRYPTODEV_BACKEND_ALG_SYM;
                 request->u.sym_op_info = sym_op_info;
+            } else {
+                stateless_info->op_info.op_code = opcode;
+                request->flags = CRYPTODEV_BACKEND_ALG_SYM_STATELESS;
+                request->u.sym_stateless_info = stateless_info;
             }
 
             ret = cryptodev_backend_crypto_operation(vcrypto->cryptodev,
-- 
1.8.3.1

[Qemu-devel] [RFC v1 8/9] virtio-crypto: add host feature bits support

[Gonglei]

We enable all feature bits acquiescently.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/virtio/virtio-crypto.c         | 15 +++++++++++++++
 include/hw/virtio/virtio-crypto.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 5422f25..3dc0ff2 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -1034,6 +1034,11 @@ static uint64_t virtio_crypto_get_features(VirtIODevice *vdev,
                                            uint64_t features,
                                            Error **errp)
 {
+    VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
+
+    /* Firstly sync all virtio-crypto possible supported features */
+    features |= vcrypto->host_features;
+
     return features;
 }
 
@@ -1144,6 +1149,16 @@ static const VMStateDescription vmstate_virtio_crypto = {
 };
 
 static Property virtio_crypto_properties[] = {
+    DEFINE_PROP_BIT("mux_mode", VirtIOCrypto, host_features,
+                    VIRTIO_CRYPTO_F_MUX_MODE, true),
+    DEFINE_PROP_BIT("cipher_stateless_mode", VirtIOCrypto, host_features,
+                    VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE, true),
+    DEFINE_PROP_BIT("hash_stateless_mode", VirtIOCrypto, host_features,
+                    VIRTIO_CRYPTO_F_HASH_STATELESS_MODE, true),
+    DEFINE_PROP_BIT("mac_stateless_mode", VirtIOCrypto, host_features,
+                    VIRTIO_CRYPTO_F_MAC_STATELESS_MODE, true),
+    DEFINE_PROP_BIT("aead_stateless_mode", VirtIOCrypto, host_features,
+                    VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-crypto.h b/include/hw/virtio/virtio-crypto.h
index 465ad20..30ea51d 100644
--- a/include/hw/virtio/virtio-crypto.h
+++ b/include/hw/virtio/virtio-crypto.h
@@ -97,6 +97,7 @@ typedef struct VirtIOCrypto {
     int multiqueue;
     uint32_t curr_queues;
     size_t config_size;
+    uint32_t host_features;
 } VirtIOCrypto;
 
 #endif /* _QEMU_VIRTIO_CRYPTO_H */
-- 
1.8.3.1

Re: [Qemu-devel] [RFC v1 8/9] virtio-crypto: add host feature bits support

[Cornelia Huck]

On Mon, 8 May 2017 19:38:23 +0800
Gonglei <arei.gonglei@huawei.com> wrote:

> We enable all feature bits acquiescently.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/virtio/virtio-crypto.c         | 15 +++++++++++++++
>  include/hw/virtio/virtio-crypto.h |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index 5422f25..3dc0ff2 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -1034,6 +1034,11 @@ static uint64_t virtio_crypto_get_features(VirtIODevice *vdev,
>                                             uint64_t features,
>                                             Error **errp)
>  {
> +    VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
> +
> +    /* Firstly sync all virtio-crypto possible supported features */
> +    features |= vcrypto->host_features;
> +
>      return features;
>  }
> 
> @@ -1144,6 +1149,16 @@ static const VMStateDescription vmstate_virtio_crypto = {
>  };
> 
>  static Property virtio_crypto_properties[] = {
> +    DEFINE_PROP_BIT("mux_mode", VirtIOCrypto, host_features,
> +                    VIRTIO_CRYPTO_F_MUX_MODE, true),
> +    DEFINE_PROP_BIT("cipher_stateless_mode", VirtIOCrypto, host_features,
> +                    VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE, true),
> +    DEFINE_PROP_BIT("hash_stateless_mode", VirtIOCrypto, host_features,
> +                    VIRTIO_CRYPTO_F_HASH_STATELESS_MODE, true),
> +    DEFINE_PROP_BIT("mac_stateless_mode", VirtIOCrypto, host_features,
> +                    VIRTIO_CRYPTO_F_MAC_STATELESS_MODE, true),
> +    DEFINE_PROP_BIT("aead_stateless_mode", VirtIOCrypto, host_features,
> +                    VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> 
> diff --git a/include/hw/virtio/virtio-crypto.h b/include/hw/virtio/virtio-crypto.h
> index 465ad20..30ea51d 100644
> --- a/include/hw/virtio/virtio-crypto.h
> +++ b/include/hw/virtio/virtio-crypto.h
> @@ -97,6 +97,7 @@ typedef struct VirtIOCrypto {
>      int multiqueue;
>      uint32_t curr_queues;
>      size_t config_size;
> +    uint32_t host_features;

I'd just make that 64 bits from the start.

>  } VirtIOCrypto;
> 
>  #endif /* _QEMU_VIRTIO_CRYPTO_H */

Don't you need some kind of compat handling?

Re: [Qemu-devel] [RFC v1 8/9] virtio-crypto: add host feature bits support

[Gonglei (Arei)]

>
> From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> Sent: Thursday, May 11, 2017 11:05 PM
> Subject: Re: [RFC v1 8/9] virtio-crypto: add host feature bits support
> 
> On Mon, 8 May 2017 19:38:23 +0800
> Gonglei <arei.gonglei@huawei.com> wrote:
> 
> > We enable all feature bits acquiescently.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/virtio/virtio-crypto.c         | 15 +++++++++++++++
> >  include/hw/virtio/virtio-crypto.h |  1 +
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> > index 5422f25..3dc0ff2 100644
> > --- a/hw/virtio/virtio-crypto.c
> > +++ b/hw/virtio/virtio-crypto.c
> > @@ -1034,6 +1034,11 @@ static uint64_t
> virtio_crypto_get_features(VirtIODevice *vdev,
> >                                             uint64_t features,
> >                                             Error **errp)
> >  {
> > +    VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
> > +
> > +    /* Firstly sync all virtio-crypto possible supported features */
> > +    features |= vcrypto->host_features;
> > +
> >      return features;
> >  }
> >
> > @@ -1144,6 +1149,16 @@ static const VMStateDescription
> vmstate_virtio_crypto = {
> >  };
> >
> >  static Property virtio_crypto_properties[] = {
> > +    DEFINE_PROP_BIT("mux_mode", VirtIOCrypto, host_features,
> > +                    VIRTIO_CRYPTO_F_MUX_MODE, true),
> > +    DEFINE_PROP_BIT("cipher_stateless_mode", VirtIOCrypto,
> host_features,
> > +                    VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE,
> true),
> > +    DEFINE_PROP_BIT("hash_stateless_mode", VirtIOCrypto,
> host_features,
> > +                    VIRTIO_CRYPTO_F_HASH_STATELESS_MODE,
> true),
> > +    DEFINE_PROP_BIT("mac_stateless_mode", VirtIOCrypto,
> host_features,
> > +                    VIRTIO_CRYPTO_F_MAC_STATELESS_MODE, true),
> > +    DEFINE_PROP_BIT("aead_stateless_mode", VirtIOCrypto,
> host_features,
> > +                    VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE,
> true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/include/hw/virtio/virtio-crypto.h b/include/hw/virtio/virtio-crypto.h
> > index 465ad20..30ea51d 100644
> > --- a/include/hw/virtio/virtio-crypto.h
> > +++ b/include/hw/virtio/virtio-crypto.h
> > @@ -97,6 +97,7 @@ typedef struct VirtIOCrypto {
> >      int multiqueue;
> >      uint32_t curr_queues;
> >      size_t config_size;
> > +    uint32_t host_features;
> 
> I'd just make that 64 bits from the start.
> 
Yes, that's better.

> >  } VirtIOCrypto;
> >
> >  #endif /* _QEMU_VIRTIO_CRYPTO_H */
> 
> Don't you need some kind of compat handling?

I did that in patch 6 according to the results of those feature bits negotiated.
Patch 9 tests both session mode and stateless mode, they are work. :)

Thanks,
-Gonglei

Re: [Qemu-devel] [RFC v1 8/9] virtio-crypto: add host feature bits support

[Cornelia Huck]

On Fri, 12 May 2017 00:55:23 +0000
"Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:

> >
> > From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> > Sent: Thursday, May 11, 2017 11:05 PM
> > Subject: Re: [RFC v1 8/9] virtio-crypto: add host feature bits support
> > 
> > On Mon, 8 May 2017 19:38:23 +0800
> > Gonglei <arei.gonglei@huawei.com> wrote:
> > 
> > > We enable all feature bits acquiescently.
> > >
> > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > ---
> > >  hw/virtio/virtio-crypto.c         | 15 +++++++++++++++
> > >  include/hw/virtio/virtio-crypto.h |  1 +
> > >  2 files changed, 16 insertions(+)
> > >
> > > diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> > > index 5422f25..3dc0ff2 100644
> > > --- a/hw/virtio/virtio-crypto.c
> > > +++ b/hw/virtio/virtio-crypto.c
> > > @@ -1034,6 +1034,11 @@ static uint64_t
> > virtio_crypto_get_features(VirtIODevice *vdev,
> > >                                             uint64_t features,
> > >                                             Error **errp)
> > >  {
> > > +    VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
> > > +
> > > +    /* Firstly sync all virtio-crypto possible supported features */
> > > +    features |= vcrypto->host_features;
> > > +
> > >      return features;
> > >  }
> > >
> > > @@ -1144,6 +1149,16 @@ static const VMStateDescription
> > vmstate_virtio_crypto = {
> > >  };
> > >
> > >  static Property virtio_crypto_properties[] = {
> > > +    DEFINE_PROP_BIT("mux_mode", VirtIOCrypto, host_features,
> > > +                    VIRTIO_CRYPTO_F_MUX_MODE, true),
> > > +    DEFINE_PROP_BIT("cipher_stateless_mode", VirtIOCrypto,
> > host_features,
> > > +                    VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE,
> > true),
> > > +    DEFINE_PROP_BIT("hash_stateless_mode", VirtIOCrypto,
> > host_features,
> > > +                    VIRTIO_CRYPTO_F_HASH_STATELESS_MODE,
> > true),
> > > +    DEFINE_PROP_BIT("mac_stateless_mode", VirtIOCrypto,
> > host_features,
> > > +                    VIRTIO_CRYPTO_F_MAC_STATELESS_MODE, true),
> > > +    DEFINE_PROP_BIT("aead_stateless_mode", VirtIOCrypto,
> > host_features,
> > > +                    VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE,
> > true),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >
> > > diff --git a/include/hw/virtio/virtio-crypto.h b/include/hw/virtio/virtio-crypto.h
> > > index 465ad20..30ea51d 100644
> > > --- a/include/hw/virtio/virtio-crypto.h
> > > +++ b/include/hw/virtio/virtio-crypto.h
> > > @@ -97,6 +97,7 @@ typedef struct VirtIOCrypto {
> > >      int multiqueue;
> > >      uint32_t curr_queues;
> > >      size_t config_size;
> > > +    uint32_t host_features;
> > 
> > I'd just make that 64 bits from the start.
> > 
> Yes, that's better.
> 
> > >  } VirtIOCrypto;
> > >
> > >  #endif /* _QEMU_VIRTIO_CRYPTO_H */
> > 
> > Don't you need some kind of compat handling?
> 
> I did that in patch 6 according to the results of those feature bits negotiated.
> Patch 9 tests both session mode and stateless mode, they are work. :)

Ah, I meant machine compat handling. You probably don't want to offer
these feature bits in older compat machines.

Re: [Qemu-devel] [RFC v1 8/9] virtio-crypto: add host feature bits support

[Gonglei (Arei)]

> From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> Sent: Friday, May 12, 2017 7:22 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; mst@redhat.com; Huangweidong (C);
> pasic@linux.vnet.ibm.com; stefanha@redhat.com; Luonengjun; Linqiangmin;
> xin.zeng@intel.com; Wubin (H)
> Subject: Re: [RFC v1 8/9] virtio-crypto: add host feature bits support
> 
> On Fri, 12 May 2017 00:55:23 +0000
> "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> 
> > >
> > > From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> > > Sent: Thursday, May 11, 2017 11:05 PM
> > > Subject: Re: [RFC v1 8/9] virtio-crypto: add host feature bits support
> > >
> > > On Mon, 8 May 2017 19:38:23 +0800
> > > Gonglei <arei.gonglei@huawei.com> wrote:
> > >
> > > > We enable all feature bits acquiescently.
> > > >
> > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > > ---
> > > >  hw/virtio/virtio-crypto.c         | 15 +++++++++++++++
> > > >  include/hw/virtio/virtio-crypto.h |  1 +
> > > >  2 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> > > > index 5422f25..3dc0ff2 100644
> > > > --- a/hw/virtio/virtio-crypto.c
> > > > +++ b/hw/virtio/virtio-crypto.c
> > > > @@ -1034,6 +1034,11 @@ static uint64_t
> > > virtio_crypto_get_features(VirtIODevice *vdev,
> > > >                                             uint64_t features,
> > > >                                             Error **errp)
> > > >  {
> > > > +    VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
> > > > +
> > > > +    /* Firstly sync all virtio-crypto possible supported features */
> > > > +    features |= vcrypto->host_features;
> > > > +
> > > >      return features;
> > > >  }
> > > >
> > > > @@ -1144,6 +1149,16 @@ static const VMStateDescription
> > > vmstate_virtio_crypto = {
> > > >  };
> > > >
> > > >  static Property virtio_crypto_properties[] = {
> > > > +    DEFINE_PROP_BIT("mux_mode", VirtIOCrypto, host_features,
> > > > +                    VIRTIO_CRYPTO_F_MUX_MODE, true),
> > > > +    DEFINE_PROP_BIT("cipher_stateless_mode", VirtIOCrypto,
> > > host_features,
> > > > +                    VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE,
> > > true),
> > > > +    DEFINE_PROP_BIT("hash_stateless_mode", VirtIOCrypto,
> > > host_features,
> > > > +                    VIRTIO_CRYPTO_F_HASH_STATELESS_MODE,
> > > true),
> > > > +    DEFINE_PROP_BIT("mac_stateless_mode", VirtIOCrypto,
> > > host_features,
> > > > +                    VIRTIO_CRYPTO_F_MAC_STATELESS_MODE,
> true),
> > > > +    DEFINE_PROP_BIT("aead_stateless_mode", VirtIOCrypto,
> > > host_features,
> > > > +                    VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE,
> > > true),
> > > >      DEFINE_PROP_END_OF_LIST(),
> > > >  };
> > > >
> > > > diff --git a/include/hw/virtio/virtio-crypto.h
> b/include/hw/virtio/virtio-crypto.h
> > > > index 465ad20..30ea51d 100644
> > > > --- a/include/hw/virtio/virtio-crypto.h
> > > > +++ b/include/hw/virtio/virtio-crypto.h
> > > > @@ -97,6 +97,7 @@ typedef struct VirtIOCrypto {
> > > >      int multiqueue;
> > > >      uint32_t curr_queues;
> > > >      size_t config_size;
> > > > +    uint32_t host_features;
> > >
> > > I'd just make that 64 bits from the start.
> > >
> > Yes, that's better.
> >
> > > >  } VirtIOCrypto;
> > > >
> > > >  #endif /* _QEMU_VIRTIO_CRYPTO_H */
> > >
> > > Don't you need some kind of compat handling?
> >
> > I did that in patch 6 according to the results of those feature bits negotiated.
> > Patch 9 tests both session mode and stateless mode, they are work. :)
> 
> Ah, I meant machine compat handling. You probably don't want to offer
> these feature bits in older compat machines.

Oh, yes, the older compat machines only can support session mode.
I don't think it's necessary to offer these feature bits which have to
be false default.

Thanks,
-Gonglei

[Qemu-devel] [RFC v1 9/9] qtest: emulate virtio crypto as a legacy device for experiment

[Gonglei]

Because the current qtest framework do not support virtio-1
or latter devices. For experimental purpose,
let's emulate the virtio crypto device as a legacy virtio
device by default. Using 0x1014 as virtio crypto pci device ID
because virtio crypto ID is 20 (0x14).

Signed-off-by: Gonglei <arei.gonglei@huawei.com>

virtio-crypto-test: add qtest case for virtio-crypto

We can simply test the functions of virtio crypto
device, including session creation, session closing,
cipher encryption and decryption.

Quick usage:
 # make tests/virtio-crypto-test && ./tests/virtio-crypto-test
  CC    tests/virtio-crypto-test.o
  LINK  tests/virtio-crypto-test
/virtio/crypto/cbc(aes-128-session-mode): OK
/virtio/crypto/cbc(aes-128-stateless-mode): OK

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 docs/specs/pci-ids.txt        |   2 +
 hw/virtio/virtio-crypto-pci.c |   4 +-
 include/hw/pci/pci.h          |   2 +
 tests/Makefile.include        |   3 +
 tests/virtio-crypto-test.c    | 596 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 606 insertions(+), 1 deletion(-)
 create mode 100644 tests/virtio-crypto-test.c

diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
index 95adee0..fe1581d 100644
--- a/docs/specs/pci-ids.txt
+++ b/docs/specs/pci-ids.txt
@@ -22,6 +22,7 @@ maintained as part of the virtio specification.
 1af4:1004  SCSI host bus adapter device (legacy)
 1af4:1005  entropy generator device (legacy)
 1af4:1009  9p filesystem device (legacy)
+1af4:1014  crypto device (legacy)
 
 1af4:1041  network device (modern)
 1af4:1042  block device (modern)
@@ -32,6 +33,7 @@ maintained as part of the virtio specification.
 1af4:1049  9p filesystem device (modern)
 1af4:1050  virtio gpu device (modern)
 1af4:1052  virtio input device (modern)
+1af4:1054  crypto device (modern)
 
 1af4:10f0  Available for experimental usage without registration.  Must get
    to      official ID when the code leaves the test lab (i.e. when seeking
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
index 422aca3..06b5917 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -37,7 +37,6 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     }
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
-    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(vdev), true, "realized", errp);
     object_property_set_link(OBJECT(vcrypto),
                  OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev",
@@ -53,6 +52,9 @@ static void virtio_crypto_pci_class_init(ObjectClass *klass, void *data)
     k->realize = virtio_crypto_pci_realize;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->props = virtio_crypto_pci_properties;
+    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_CRYPTO;
+    pcidev_k->revision = 0;
     pcidev_k->class_id = PCI_CLASS_OTHERS;
 }
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index a37a2d5..9f06db6 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -83,6 +83,8 @@
 #define PCI_DEVICE_ID_VIRTIO_RNG         0x1005
 #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
 #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
+#define PCI_DEVICE_ID_VIRTIO_CRYPTO      0x1014
+
 
 #define PCI_VENDOR_ID_REDHAT             0x1b36
 #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 31931c0..e3af4b9 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -168,6 +168,8 @@ check-qtest-virtio-y += tests/virtio-serial-test$(EXESUF)
 gcov-files-virtio-y += i386-softmmu/hw/char/virtio-serial-bus.c
 check-qtest-virtio-y += $(check-qtest-virtioserial-y)
 gcov-files-virtio-y += $(gcov-files-virtioserial-y)
+check-qtest-virtio-y += tests/virtio-crypto-test$(EXESUF)
+gcov-files-virtio-y += i386-softmmu/hw/virtio/virtio-crypto.c
 
 check-qtest-pci-y += tests/e1000-test$(EXESUF)
 gcov-files-pci-y += hw/net/e1000.c
@@ -717,6 +719,7 @@ tests/virtio-scsi-test$(EXESUF): tests/virtio-scsi-test.o $(libqos-virtio-obj-y)
 tests/virtio-9p-test$(EXESUF): tests/virtio-9p-test.o $(libqos-virtio-obj-y)
 tests/virtio-serial-test$(EXESUF): tests/virtio-serial-test.o
 tests/virtio-console-test$(EXESUF): tests/virtio-console-test.o
+tests/virtio-crypto-test$(EXESUF): tests/virtio-crypto-test.o $(libqos-virtio-obj-y)
 tests/tpci200-test$(EXESUF): tests/tpci200-test.o
 tests/display-vga-test$(EXESUF): tests/display-vga-test.o
 tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
diff --git a/tests/virtio-crypto-test.c b/tests/virtio-crypto-test.c
new file mode 100644
index 0000000..630eb9d
--- /dev/null
+++ b/tests/virtio-crypto-test.c
@@ -0,0 +1,596 @@
+/*
+ * QTest testcase for VirtIO Crypto Device
+ *
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ * Authors:
+ *    Gonglei <arei.gonglei@huawei.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <stdlib.h>
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "libqos/virtio.h"
+#include "libqos/virtio-pci.h"
+#include "libqos/virtio-mmio.h"
+#include "libqos/pci-pc.h"
+#include "libqos/malloc.h"
+#include "libqos/malloc-pc.h"
+#include "libqos/malloc-generic.h"
+#include "qemu/bswap.h"
+#include "standard-headers/linux/virtio_ids.h"
+#include "standard-headers/linux/virtio_config.h"
+#include "standard-headers/linux/virtio_ring.h"
+#include "standard-headers/linux/virtio_crypto.h"
+#include "standard-headers/linux/virtio_pci.h"
+
+#define QVIRTIO_CRYPTO_TIMEOUT_US  (30 * 1000 * 1000)
+
+#define PCI_SLOT_HP             0x06
+#define PCI_SLOT                0x04
+#define PCI_FN                  0x00
+
+/*
+ * VirtIOCryptoCipherTestData:  structure to describe a cipher test
+ * @key:    A pointer to a key used by the test
+ * @key_len:    The length of @key
+ * @iv:     A pointer to the IV/Counter used by the test
+ * @iv_len: The length of @iv
+ * @input:  A pointer to data used as input
+ * @ilen    The length of data in @input
+ * @output: A pointer to what the test need to produce
+ * @olen:   The length of data in @output
+ * @algo:   The type of algorithm, refer to VIRTIO_CRYPTO_CIPHER_AES_*
+ */
+typedef struct VirtIOCryptoCipherTestData {
+    const char *path;
+    unsigned short algo;
+    const char *key;
+    const char *iv;
+    const char *input;
+    const char *output;
+    unsigned char key_len;
+    unsigned char iv_len;
+    unsigned short ilen;
+    unsigned short olen;
+    bool is_statelss_mode;
+} VirtIOCryptoCipherTestData;
+
+
+static VirtIOCryptoCipherTestData cipher_test_data[] = {
+    { /* From RFC 3602 */
+        .path = "/virtio/crypto/cbc(aes-128-session-mode)",
+        .algo = VIRTIO_CRYPTO_CIPHER_AES_CBC,
+        .key  = "\x06\xa9\x21\x40\x36\xb8\xa1\x5b"
+                "\x51\x2e\x03\xd5\x34\x12\x00\x06",
+        .key_len   = 16,
+        .iv = "\x3d\xaf\xba\x42\x9d\x9e\xb4\x30"
+              "\xb4\x22\xda\x80\x2c\x9f\xac\x41",
+        .iv_len = 16,
+        .input  = "Single block msg",
+        .ilen   = 16,
+        .output = "\xe3\x53\x77\x9c\x10\x79\xae\xb8"
+                  "\x27\x08\x94\x2d\xbe\x77\x18\x1a",
+        .olen   = 16,
+        .is_statelss_mode = false,
+    },
+    { /* From RFC 3602 */
+        .path = "/virtio/crypto/cbc(aes-128-stateless-mode)",
+        .algo = VIRTIO_CRYPTO_CIPHER_AES_CBC,
+        .key  = "\x06\xa9\x21\x40\x36\xb8\xa1\x5b"
+                "\x51\x2e\x03\xd5\x34\x12\x00\x06",
+        .key_len   = 16,
+        .iv = "\x3d\xaf\xba\x42\x9d\x9e\xb4\x30"
+              "\xb4\x22\xda\x80\x2c\x9f\xac\x41",
+        .iv_len = 16,
+        .input  = "Single block msg",
+        .ilen   = 16,
+        .output = "\xe3\x53\x77\x9c\x10\x79\xae\xb8"
+                  "\x27\x08\x94\x2d\xbe\x77\x18\x1a",
+        .olen   = 16,
+        .is_statelss_mode = true,
+    },
+};
+
+static QPCIBus *virtio_crypto_test_start(void)
+{
+    char *cmdline;
+
+    cmdline = g_strdup_printf(
+               "-object cryptodev-backend-builtin,id=cryptodev0 "
+               "-device virtio-crypto-pci,id=crypto0,"
+               "cryptodev=cryptodev0");
+
+    qtest_start(cmdline);
+    g_free(cmdline);
+
+    return qpci_init_pc(NULL);
+}
+
+static void test_end(void)
+{
+    qtest_end();
+}
+
+static QVirtioPCIDevice *virtio_crypto_pci_init(QPCIBus *bus, int slot)
+{
+    QVirtioPCIDevice *dev;
+
+    dev = qvirtio_pci_device_find(bus, VIRTIO_ID_CRYPTO);
+    g_assert(dev != NULL);
+    g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_CRYPTO);
+
+    qvirtio_pci_device_enable(dev);
+    qvirtio_reset(&dev->vdev);
+    qvirtio_set_acknowledge(&dev->vdev);
+    qvirtio_set_driver(&dev->vdev);
+
+    return dev;
+}
+
+static uint64_t
+virtio_crypto_ctrl_request(QGuestAllocator *alloc,
+                           struct virtio_crypto_op_ctrl_req *req)
+{
+    uint64_t addr;
+
+    addr = guest_alloc(alloc, sizeof(*req));
+
+    memwrite(addr, req, sizeof(*req));
+
+    return addr;
+}
+
+static uint64_t
+virtio_crypto_data_request(QGuestAllocator *alloc,
+                           struct virtio_crypto_op_data_req *req)
+{
+    uint64_t addr;
+
+    addr = guest_alloc(alloc, sizeof(*req));
+
+    memwrite(addr, req, sizeof(*req));
+
+    return addr;
+}
+
+static void
+virtio_crypto_driver_init(QVirtioDevice *dev)
+{
+    /* Read configure space to get  supported crypto services */
+
+    qvirtio_set_driver_ok(dev);
+}
+
+static uint64_t
+virtio_crypto_create_session(QVirtioDevice *dev,
+            QGuestAllocator *alloc, QVirtQueue *vq,
+            VirtIOCryptoCipherTestData *data,
+            int encrypt)
+{
+    uint32_t free_head;
+    struct virtio_crypto_op_ctrl_req ctrl;
+    struct virtio_crypto_session_input input;
+    uint32_t key_len = data->key_len;
+    uint64_t req_addr;
+    uint64_t key_addr, input_addr; /* cipher key guest physical address */
+    uint64_t session_id;
+    QVRingIndirectDesc *indirect;
+
+    /* Create an encryption session */
+    ctrl.header.opcode = VIRTIO_CRYPTO_CIPHER_CREATE_SESSION;
+    ctrl.header.algo = data->algo;
+    /* Set the default dataqueue id to 0 */
+    ctrl.header.queue_id = 0;
+
+    /* Pad cipher's parameters */
+    ctrl.u.sym_create_session.op_type = VIRTIO_CRYPTO_SYM_OP_CIPHER;
+    ctrl.u.sym_create_session.u.cipher.para.algo = ctrl.header.algo;
+    ctrl.u.sym_create_session.u.cipher.para.keylen = key_len;
+    if (encrypt) {
+        ctrl.u.sym_create_session.u.cipher.para.op = VIRTIO_CRYPTO_OP_ENCRYPT;
+    } else {
+        ctrl.u.sym_create_session.u.cipher.para.op = VIRTIO_CRYPTO_OP_DECRYPT;
+    }
+
+    req_addr = virtio_crypto_ctrl_request(alloc, &ctrl);
+
+    /* Pad cipher's output data */
+    key_addr = guest_alloc(alloc, key_len);
+    memwrite(key_addr, data->key, key_len);
+
+    input.status = VIRTIO_CRYPTO_ERR;
+    input_addr = guest_alloc(alloc, sizeof(input));
+    memwrite(input_addr, &input, sizeof(input));
+
+    indirect = qvring_indirect_desc_setup(dev, alloc, 3);
+    qvring_indirect_desc_add(indirect, req_addr, sizeof(ctrl), false);
+    qvring_indirect_desc_add(indirect, key_addr, key_len, false);
+    qvring_indirect_desc_add(indirect, input_addr, sizeof(input), true);
+    free_head = qvirtqueue_add_indirect(vq, indirect);
+
+    qvirtqueue_kick(dev, vq, free_head);
+
+    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_CRYPTO_TIMEOUT_US);
+
+    /* calculate the offset of input data */
+
+    memread(input_addr, &input, sizeof(input));
+
+    /* Verify the result */
+    g_assert_cmpint(input.status, ==, VIRTIO_CRYPTO_OK);
+
+    session_id = input.session_id;
+
+    g_free(indirect);
+    guest_free(alloc, input_addr);
+    guest_free(alloc, key_addr);
+    guest_free(alloc, req_addr);
+
+    return session_id;
+}
+
+static void
+virtio_crypto_close_session(QVirtioDevice *dev,
+            QGuestAllocator *alloc, QVirtQueue *vq,
+            uint64_t session_id)
+{
+    uint32_t free_head;
+    struct virtio_crypto_op_ctrl_req ctrl;
+    uint64_t req_addr, status_addr;
+    uint8_t status;
+    QVRingIndirectDesc *indirect;
+
+    /* Create an encryption session */
+    ctrl.header.opcode = VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION;
+    /* Set the default dataqueue id to 0 */
+    ctrl.header.queue_id = 0;
+
+    ctrl.u.destroy_session.session_id = session_id;
+
+    req_addr = virtio_crypto_ctrl_request(alloc, &ctrl);
+
+    status_addr = guest_alloc(alloc, sizeof(status));
+    writel(status_addr, VIRTIO_CRYPTO_ERR);
+
+    indirect = qvring_indirect_desc_setup(dev, alloc, 2);
+    qvring_indirect_desc_add(indirect, req_addr, sizeof(ctrl), false);
+    qvring_indirect_desc_add(indirect, status_addr, sizeof(status), true);
+    free_head = qvirtqueue_add_indirect(vq, indirect);
+
+    qvirtqueue_kick(dev, vq, free_head);
+
+    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_CRYPTO_TIMEOUT_US);
+
+    /* Verify the result */
+    status = readl(status_addr);
+    g_assert_cmpint(status, ==, VIRTIO_CRYPTO_OK);
+
+    g_free(indirect);
+    guest_free(alloc, req_addr);
+    guest_free(alloc, status_addr);
+}
+
+
+static void
+virtio_crypto_test_cipher_session_mode(QVirtioDevice *dev,
+            QGuestAllocator *alloc, QVirtQueue *ctrlq,
+            QVirtQueue *vq, VirtIOCryptoCipherTestData *data,
+            int encrypt)
+{
+    uint32_t free_head;
+    struct virtio_crypto_op_data_req req;
+    uint64_t req_addr, status_addr;
+    uint64_t iv_addr = 0, src_addr, dst_addr;
+    uint64_t session_id;
+    char *output;
+    uint32_t src_len, dst_len;
+    uint8_t status;
+    QVRingIndirectDesc *indirect;
+    uint8_t entry_num;
+
+    /* Create a session */
+    session_id = virtio_crypto_create_session(dev, alloc,
+                                             ctrlq, data, encrypt);
+
+    /* Head of operation */
+    req.header.session_id = session_id;
+    if (encrypt) {
+        req.header.opcode = VIRTIO_CRYPTO_CIPHER_ENCRYPT;
+    } else {
+        req.header.opcode = VIRTIO_CRYPTO_CIPHER_DECRYPT;
+    }
+
+    req.u.sym_req.op_type = VIRTIO_CRYPTO_SYM_OP_CIPHER;
+    req.u.sym_req.u.cipher.para.iv_len = data->iv_len;
+    req.u.sym_req.u.cipher.para.src_data_len = data->ilen;
+    req.u.sym_req.u.cipher.para.dst_data_len = data->olen;
+
+    req_addr = virtio_crypto_data_request(alloc, &req);
+
+    /* IV */
+    if (data->iv_len > 0) {
+        iv_addr = guest_alloc(alloc, data->iv_len);
+        memwrite(iv_addr, data->iv, data->iv_len);
+
+        /* header + iv + src + dst + status */
+        entry_num = 5;
+    } else {
+        /* header + src + dst + status */
+        entry_num = 4;
+    }
+
+    if (encrypt) {
+        src_len = data->ilen;
+        dst_len = data->olen;
+        /* Source data is the input data which is a single buffer */
+        src_addr = guest_alloc(alloc, src_len);
+        memwrite(src_addr, data->input, src_len);
+    } else {
+        src_len = data->olen;
+        dst_len = data->ilen;
+        /* Source data is the output data which is a single buffer */
+        src_addr = guest_alloc(alloc, src_len);
+        memwrite(src_addr, data->output, src_len);
+    }
+
+    dst_addr = guest_alloc(alloc, dst_len);
+
+    status_addr = guest_alloc(alloc, sizeof(status));
+    writel(status_addr, VIRTIO_CRYPTO_ERR);
+
+    /* Allocate descripto table entries */
+    indirect = qvring_indirect_desc_setup(dev, alloc, entry_num);
+    qvring_indirect_desc_add(indirect, req_addr, sizeof(req), false);
+    if (data->iv_len > 0) {
+        qvring_indirect_desc_add(indirect, iv_addr, data->iv_len, false);
+    }
+    qvring_indirect_desc_add(indirect, src_addr, src_len, false);
+    qvring_indirect_desc_add(indirect, dst_addr, dst_len, true);
+    qvring_indirect_desc_add(indirect, status_addr, sizeof(status), true);
+    free_head = qvirtqueue_add_indirect(vq, indirect);
+
+    qvirtqueue_kick(dev, vq, free_head);
+
+    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_CRYPTO_TIMEOUT_US);
+
+    /* Verify the result */
+    status = readl(status_addr);
+    g_assert_cmpint(status, ==, VIRTIO_CRYPTO_OK);
+
+    output = g_malloc0(dst_len);
+    memread(dst_addr, output, dst_len);
+    if (encrypt) {
+        g_assert_cmpstr(output, ==, data->output);
+    } else {
+        g_assert_cmpstr(output, ==, data->input);
+    }
+    g_free(output);
+
+    g_free(indirect);
+
+    if (data->iv_len > 0) {
+        guest_free(alloc, iv_addr);
+    }
+    guest_free(alloc, src_addr);
+    guest_free(alloc, dst_addr);
+    guest_free(alloc, req_addr);
+    guest_free(alloc, status_addr);
+
+    /* Close the session */
+    virtio_crypto_close_session(dev, alloc, ctrlq, session_id);
+}
+
+static void
+virtio_crypto_test_cipher_stateless_mode(QVirtioDevice *dev,
+            QGuestAllocator *alloc,
+            QVirtQueue *vq, VirtIOCryptoCipherTestData *data,
+            int encrypt)
+{
+    uint32_t free_head;
+    struct virtio_crypto_op_data_req_mux req;
+    uint64_t req_addr, status_addr;
+    uint64_t iv_addr = 0, src_addr, dst_addr, key_addr;
+    char *output;
+    uint32_t src_len, dst_len;
+    uint8_t status;
+    QVRingIndirectDesc *indirect;
+    uint8_t entry_num;
+
+    /* Head of operation */
+    req.header.flag = VIRTIO_CRYPTO_FLAG_STATELESS_MODE;
+    if (encrypt) {
+        req.header.opcode = VIRTIO_CRYPTO_CIPHER_ENCRYPT;
+        req.u.sym_stateless_req.u.cipher.para.sess_para.op =
+            VIRTIO_CRYPTO_OP_ENCRYPT;
+    } else {
+        req.header.opcode = VIRTIO_CRYPTO_CIPHER_DECRYPT;
+        req.u.sym_stateless_req.u.cipher.para.sess_para.op =
+            VIRTIO_CRYPTO_OP_DECRYPT;
+    }
+
+    req.u.sym_stateless_req.op_type = VIRTIO_CRYPTO_SYM_OP_CIPHER;
+    req.u.sym_stateless_req.u.cipher.para.sess_para.algo = data->algo;
+    req.u.sym_stateless_req.u.cipher.para.sess_para.keylen = data->key_len;
+    req.u.sym_stateless_req.u.cipher.para.iv_len = data->iv_len;
+    req.u.sym_stateless_req.u.cipher.para.src_data_len = data->ilen;
+    req.u.sym_stateless_req.u.cipher.para.dst_data_len = data->olen;
+
+    req_addr = guest_alloc(alloc, sizeof(req));
+    memwrite(req_addr, &req, sizeof(req));
+
+    g_assert(data->key_len > 0);
+    key_addr = guest_alloc(alloc, data->key_len);
+    memwrite(key_addr, data->key, data->key_len);
+
+    /* IV */
+    if (data->iv_len > 0) {
+        iv_addr = guest_alloc(alloc, data->iv_len);
+        memwrite(iv_addr, data->iv, data->iv_len);
+
+        /* header + key + iv + src + dst + status */
+        entry_num = 6;
+    } else {
+        /* header + key + src + dst + status */
+        entry_num = 5;
+    }
+
+    if (encrypt) {
+        src_len = data->ilen;
+        dst_len = data->olen;
+        /* Source data is the input data which is a single buffer */
+        src_addr = guest_alloc(alloc, src_len);
+        memwrite(src_addr, data->input, src_len);
+    } else {
+        src_len = data->olen;
+        dst_len = data->ilen;
+        /* Source data is the output data which is a single buffer */
+        src_addr = guest_alloc(alloc, src_len);
+        memwrite(src_addr, data->output, src_len);
+    }
+
+    dst_addr = guest_alloc(alloc, dst_len);
+
+    status_addr = guest_alloc(alloc, sizeof(status));
+    writel(status_addr, VIRTIO_CRYPTO_ERR);
+
+    /* Allocate desc table entries */
+    indirect = qvring_indirect_desc_setup(dev, alloc, entry_num);
+    qvring_indirect_desc_add(indirect, req_addr, sizeof(req), false);
+    qvring_indirect_desc_add(indirect, key_addr, data->key_len, false);
+    if (data->iv_len > 0) {
+        qvring_indirect_desc_add(indirect, iv_addr, data->iv_len, false);
+    }
+    qvring_indirect_desc_add(indirect, src_addr, src_len, false);
+    qvring_indirect_desc_add(indirect, dst_addr, dst_len, true);
+    qvring_indirect_desc_add(indirect, status_addr, sizeof(status), true);
+    free_head = qvirtqueue_add_indirect(vq, indirect);
+
+    qvirtqueue_kick(dev, vq, free_head);
+
+    qvirtio_wait_queue_isr(dev, vq, QVIRTIO_CRYPTO_TIMEOUT_US);
+
+    /* Verify the result */
+    status = readl(status_addr);
+    g_assert_cmpint(status, ==, VIRTIO_CRYPTO_OK);
+
+    output = g_malloc0(dst_len);
+    memread(dst_addr, output, dst_len);
+    if (encrypt) {
+        g_assert_cmpstr(output, ==, data->output);
+    } else {
+        g_assert_cmpstr(output, ==, data->input);
+    }
+    g_free(output);
+
+    g_free(indirect);
+    guest_free(alloc, key_addr);
+    if (data->iv_len > 0) {
+        guest_free(alloc, iv_addr);
+    }
+    guest_free(alloc, src_addr);
+    guest_free(alloc, dst_addr);
+    guest_free(alloc, req_addr);
+    guest_free(alloc, status_addr);
+}
+
+static void
+virtio_crypto_test_cipher(QVirtioDevice *dev,
+            QGuestAllocator *alloc, QVirtQueue *ctrlq,
+            QVirtQueue *dataq, VirtIOCryptoCipherTestData *data,
+            int encrypt)
+{
+    if (!data->is_statelss_mode) {
+        virtio_crypto_test_cipher_session_mode(dev, alloc,
+            ctrlq, dataq, data, encrypt);
+    } else {
+        virtio_crypto_test_cipher_stateless_mode(dev, alloc,
+            dataq, data, encrypt);
+    }
+}
+
+static void virtio_crypto_pci_basic(void *opaque)
+{
+    VirtIOCryptoCipherTestData *test_data = opaque;
+    QVirtioPCIDevice *dev;
+    QPCIBus *bus;
+    QGuestAllocator *alloc;
+    QVirtQueuePCI *dataq, *controlq;
+    uint32_t features;
+
+    bus = virtio_crypto_test_start();
+    dev = virtio_crypto_pci_init(bus, PCI_SLOT);
+
+    alloc = pc_alloc_init();
+
+    features = qvirtio_get_features(&dev->vdev);
+    g_assert_cmphex(features & (1u << VIRTIO_RING_F_INDIRECT_DESC), !=, 0);
+
+    if (!test_data->is_statelss_mode) {
+        features = features & ~(QVIRTIO_F_BAD_FEATURE |
+                                (1u << VIRTIO_RING_F_EVENT_IDX |
+                                1u << VIRTIO_CRYPTO_F_MUX_MODE |
+                                1u << VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE));
+    } else {
+        features = features & ~(QVIRTIO_F_BAD_FEATURE |
+                                (1u << VIRTIO_RING_F_EVENT_IDX));
+    }
+    qvirtio_set_features(&dev->vdev, features);
+
+    dataq = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev,
+                                           alloc, 0);
+    controlq = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev,
+                                           alloc, 1);
+
+    virtio_crypto_driver_init(&dev->vdev);
+
+    /* Step 1: Encryption */
+    virtio_crypto_test_cipher(&dev->vdev, alloc,
+                              &controlq->vq, &dataq->vq,
+                              test_data, 1);
+    /* Step 2: Decryption */
+    virtio_crypto_test_cipher(&dev->vdev, alloc,
+                              &controlq->vq, &dataq->vq,
+                              test_data, 0);
+
+    /* End test */
+    guest_free(alloc, dataq->vq.desc);
+    guest_free(alloc, controlq->vq.desc);
+    pc_alloc_uninit(alloc);
+    qvirtio_pci_device_disable(dev);
+    g_free(dev);
+    qpci_free_pc(bus);
+    test_end();
+}
+
+int main(int argc, char **argv)
+{
+    const char *qemu;
+    const char *arch;
+    int i, ret;
+
+    qemu = getenv("QTEST_QEMU_BINARY");
+    if (qemu == NULL) {
+        ret = setenv("QTEST_QEMU_BINARY",
+                     "x86_64-softmmu/qemu-system-x86_64", 0);
+        g_assert(ret == 0);
+    }
+
+    arch = qtest_get_arch();
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+        for (i = 0; i < G_N_ELEMENTS(cipher_test_data); i++) {
+            g_test_add_data_func(cipher_test_data[i].path,
+                                 (void *)&cipher_test_data[i],
+                                 (GTestDataFunc)virtio_crypto_pci_basic);
+        }
+    }
+
+    return g_test_run();
+}
-- 
1.8.3.1