[Qemu-devel] [PATCH v16 0/2] virtio-crypto: virtio crypto device specification

[Qemu-devel] [PATCH v16 0/2] virtio-crypto: virtio crypto device specification

[Gonglei]

Changes since v15:
 - use feature bits for non-session mode in order to keep compatibility with
   pre-existing code. [Halil & Michael]
 - introduce VIRTIO_CRYPTO_F_ NON_SESSION_MODE feature bit to control all other
   non-session mode feature bits.
 - fix some typos. [Stefan]
 - introduce struct virtio_crypto_op_data_req_mux to support both session
   and non-session based crypto operations and keep compatibility with
   pre-existing code.

Changes since v14:
 - drop VIRTIO_CRYPTO_S_STARTED status [Halil & Cornelia]
 - correct a sentence about dataqueue and controlq in the first paragraph. [Halil]
 - change a MAY to MUST about max_dataqueues. [Halil]
 - add non-session mode support
   a) add four features for different crypto services to identify wheather support session mode.
   b) rewrite some

For pervious versions of virtio crypto spec, Pls see:

[v14]:
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02212.html

[v13]:
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07348.html

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

Please help to review, thanks.

Gonglei (2):
  virtio-crypto: Add virtio crypto device specification
  virtio-crypto: Add conformance clauses

 conformance.tex   |   30 ++
 content.tex       |    2 +
 virtio-crypto.tex | 1245 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1277 insertions(+)
 create mode 100644 virtio-crypto.tex

-- 
1.7.12.4

[Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei]

The virtio crypto device is a virtual crypto device (ie. hardware
crypto accelerator card). Currently, the virtio crypto device provides
the following crypto services: CIPHER, MAC, HASH, and AEAD.

In this patch, CIPHER, MAC, HASH, AEAD services are introduced.

VIRTIO-153

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Cornelia Huck <cornelia.huck@de.ibm.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Lingli Deng <denglingli@chinamobile.com>
CC: Jani Kokkonen <Jani.Kokkonen@huawei.com>
CC: Ola Liljedahl <Ola.Liljedahl@arm.com>
CC: Varun Sethi <Varun.Sethi@freescale.com>
CC: Zeng Xin <xin.zeng@intel.com>
CC: Keating Brian <brian.a.keating@intel.com>
CC: Ma Liang J <liang.j.ma@intel.com>
CC: Griffin John <john.griffin@intel.com>
CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
---
 content.tex       |    2 +
 virtio-crypto.tex | 1245 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 1247 insertions(+)
 create mode 100644 virtio-crypto.tex

diff --git a/content.tex b/content.tex
index 4b45678..ab75f78 100644
--- a/content.tex
+++ b/content.tex
@@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len}, \field{residual},
 \field{status_qualifier}, \field{status}, \field{response} and
 \field{sense} fields.
 
+\input{virtio-crypto.tex}
+
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
 Currently there are three device-independent feature bits defined:
diff --git a/virtio-crypto.tex b/virtio-crypto.tex
new file mode 100644
index 0000000..732cd30
--- /dev/null
+++ b/virtio-crypto.tex
@@ -0,0 +1,1245 @@
+\section{Crypto Device}\label{sec:Device Types / Crypto Device}
+
+The virtio crypto device is a virtual cryptography device as well as a kind of
+virtual hardware accelerator for virtual machines. The encryption and
+decryption requests are placed in any of the data active queues and are ultimately handled by the
+backend crypto accelerators. The second kind of queue is the control queue used to create 
+or destroy sessions for symmetric algorithms and will control some advanced
+features in the future. The virtio crypto device provides the following crypto
+services: CIPHER, MAC, HASH, and AEAD.
+
+
+\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
+
+20
+
+\subsection{Virtqueues}\label{sec:Device Types / Crypto Device / Virtqueues}
+
+\begin{description}
+\item[0] dataq1
+\item[\ldots]
+\item[N-1] dataqN
+\item[N] controlq
+\end{description}
+
+N is set by \field{max_dataqueues}.
+
+\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature bits}
+
+VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
+VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for CIPHER service.
+VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for HASH service.
+VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC service.
+VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for AEAD service.
+
+\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto Device / Feature bits}
+
+Some crypto feature bits require other crypto feature bits
+(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}):
+
+\begin{description}
+\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
+\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
+\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
+\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
+\end{description}
+
+\subsection{Device configuration layout}\label{sec:Device Types / Crypto Device / Device configuration layout}
+
+The following driver-read-only configuration fields are defined:
+
+\begin{lstlisting}
+struct virtio_crypto_config {
+    le32 status;
+    le32 max_dataqueues;
+    le32 crypto_services;
+    /* Detailed algorithms mask */
+    le32 cipher_algo_l;
+    le32 cipher_algo_h;
+    le32 hash_algo;
+    le32 mac_algo_l;
+    le32 mac_algo_h;
+    le32 aead_algo;
+    /* Maximum length of cipher key */
+    le32 max_cipher_key_len;
+    /* Maximum length of authenticated key */
+    le32 max_auth_key_len;
+    le32 reserved;
+    /* Maximum size of each crypto request's content */
+    le64 max_size;
+};
+\end{lstlisting}
+
+The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or ~VIRTIO_CRYPTO_S_HW_READY.
+
+\begin{lstlisting}
+#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
+\end{lstlisting}
+
+The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the hardware is ready to work or not.
+
+The following driver-read-only fields include \field{max_dataqueues}, which specifies the
+maximum number of data virtqueues (dataq1\ldots dataqN), and \field{crypto_services},
+which indicates the crypto services the virtio crypto supports.
+
+The following services are defined:
+
+\begin{lstlisting}
+/* CIPHER service */
+#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
+/* HASH service */
+#define VIRTIO_CRYPTO_SERVICE_HASH   1
+/* MAC (Message Authentication Codes) service */
+#define VIRTIO_CRYPTO_SERVICE_MAC    2
+/* AEAD (Authenticated Encryption with Associated Data) service */
+#define VIRTIO_CRYPTO_SERVICE_AEAD   3
+\end{lstlisting}
+
+The last driver-read-only fields specify detailed algorithms masks 
+the device offers for corresponding services. The following CIPHER algorithms
+are defined:
+
+\begin{lstlisting}
+#define VIRTIO_CRYPTO_NO_CIPHER                 0
+#define VIRTIO_CRYPTO_CIPHER_ARC4               1
+#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
+#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
+#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
+#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
+#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
+#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
+#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
+#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
+#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
+#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
+#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
+#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
+#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
+\end{lstlisting}
+
+The following HASH algorithms are defined:
+
+\begin{lstlisting}
+#define VIRTIO_CRYPTO_NO_HASH            0
+#define VIRTIO_CRYPTO_HASH_MD5           1
+#define VIRTIO_CRYPTO_HASH_SHA1          2
+#define VIRTIO_CRYPTO_HASH_SHA_224       3
+#define VIRTIO_CRYPTO_HASH_SHA_256       4
+#define VIRTIO_CRYPTO_HASH_SHA_384       5
+#define VIRTIO_CRYPTO_HASH_SHA_512       6
+#define VIRTIO_CRYPTO_HASH_SHA3_224      7
+#define VIRTIO_CRYPTO_HASH_SHA3_256      8
+#define VIRTIO_CRYPTO_HASH_SHA3_384      9
+#define VIRTIO_CRYPTO_HASH_SHA3_512      10
+#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128      11
+#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256      12
+\end{lstlisting}
+
+The following MAC algorithms are defined:
+
+\begin{lstlisting}
+#define VIRTIO_CRYPTO_NO_MAC                       0
+#define VIRTIO_CRYPTO_MAC_HMAC_MD5                 1
+#define VIRTIO_CRYPTO_MAC_HMAC_SHA1                2
+#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224             3
+#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256             4
+#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384             5
+#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512             6
+#define VIRTIO_CRYPTO_MAC_CMAC_3DES                25
+#define VIRTIO_CRYPTO_MAC_CMAC_AES                 26
+#define VIRTIO_CRYPTO_MAC_KASUMI_F9                27
+#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2              28
+#define VIRTIO_CRYPTO_MAC_GMAC_AES                 41
+#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             42
+#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               49
+#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         50
+#define VIRTIO_CRYPTO_MAC_XCBC_AES                 53
+#define VIRTIO_CRYPTO_MAC_ZUC_EIA3                 54
+\end{lstlisting}
+
+The following AEAD algorithms are defined:
+
+\begin{lstlisting}
+#define VIRTIO_CRYPTO_NO_AEAD     0
+#define VIRTIO_CRYPTO_AEAD_GCM    1
+#define VIRTIO_CRYPTO_AEAD_CCM    2
+#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
+\end{lstlisting}
+
+\begin{note}
+Any other values are reserved for future use.
+\end{note}
+
+\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Crypto Device / Device configuration layout}
+
+\begin{itemize*}
+\item The device MUST set \field{max_dataqueues} to between 1 and 65535 inclusive.
+\item The device MUST set \field{status} based on the status of the hardware-backed implementation. 
+\item The device MUST accept and handle requests after \field{status} is set to VIRTIO_CRYPTO_S_HW_READY.
+\item The device MUST set \field{crypto_services} based on the crypto services the device offers.
+\item The device MUST set detailed algorithms masks based on the \field{crypto_services} field.
+\item The device MUST set \field{max_size} to show the maximum size of crypto request the device supports.
+\item The device MUST set \field{max_cipher_key_len} to show the maximum length of cipher key if the device supports CIPHER service.
+\item The device MUST set \field{max_auth_key_len} to show the maximum length of authenticated key if the device supports MAC service.
+\end{itemize*}
+
+\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Crypto Device / Device configuration layout}
+
+\begin{itemize*}
+\item The driver MUST read the ready \field{status} from the bottom bit of status to check whether the hardware-backed
+      implementation is ready or not, and the driver MUST reread it after the device reset. 
+\item The driver MUST NOT transmit any packets to the device if the ready \field{status} is not set.
+\item The driver MUST read \field{max_dataqueues} field to discover the number of data queues the device supports.
+\item The driver MUST read \field{crypto_services} field to discover which services the device is able to offer.
+\item The driver MUST read the detailed algorithms fields based on \field{crypto_services} field.
+\item The driver SHOULD read \field{max_size} to discover the maximum size of crypto request the device supports.
+\item The driver SHOULD read \field{max_cipher_key_len} to discover the maximum length of cipher key the device supports.
+\item The driver SHOULD read \field{max_auth_key_len} to discover the maximum length of authenticated key the device supports.
+\end{itemize*}
+
+\subsection{Device Initialization}\label{sec:Device Types / Crypto Device / Device Initialization}
+
+\drivernormative{\subsubsection}{Device Initialization}{Device Types / Crypto Device / Device Initialization}
+
+\begin{itemize*}
+\item The driver MUST identify and initialize the control virtqueue.
+\item The driver MUST read the supported crypto services from bits of \field{crypto_services}. 
+\item The driver MUST read the supported algorithms based on \field{crypto_services} field.
+\end{itemize*}
+
+\devicenormative{\subsubsection}{Device Initialization}{Device Types / Crypto Device / Device Initialization}
+
+\begin{itemize*}
+\item The device MUST be configured with at least one accelerator which executes backend crypto operations.
+\item The device MUST write the \field{crypto_services} field based on the capacities of the backend accelerator.
+\end{itemize*}
+
+\subsection{Device Operation}\label{sec:Device Types / Crypto Device / Device Operation}
+
+Packets can be transmitted by placing them in both the controlq and dataq.
+Packets consist of a general header and a service-specific request.
+Where 'general header' is for all crypto requests, and 'service specific requests'
+are composed of operation parameter + output data + input data in general.
+Operation parameters are algorithm-specific parameters, output data is the
+data that should be utilized in operations, and input data is equal to
+"operation result + result data".
+
+The device can support both session mode (See \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation}) and non-session mode, for example,
+As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is negotiated, the driver can use non-session mode for CIPHER service, otherwise it can only use session mode.
+
+\begin{note}
+The basic unit of all data length the byte.
+\end{note}
+
+The general header for controlq is as follows:
+
+\begin{lstlisting}
+#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
+
+struct virtio_crypto_ctrl_header {
+#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
+       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x02)
+#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
+       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x03)
+#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
+       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
+#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
+       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
+#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
+       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
+#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
+       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
+#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
+       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
+#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
+       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
+    le32 opcode;
+    le32 algo;
+    le32 flag;
+    /* data virtqueue id */
+    le32 queue_id;
+};
+\end{lstlisting}
+
+The general header of dataq:
+
+\begin{lstlisting}
+struct virtio_crypto_op_header {
+#define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
+    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
+#define VIRTIO_CRYPTO_CIPHER_DECRYPT \
+    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
+#define VIRTIO_CRYPTO_HASH \
+    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
+#define VIRTIO_CRYPTO_MAC \
+    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
+#define VIRTIO_CRYPTO_AEAD_ENCRYPT \
+    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
+#define VIRTIO_CRYPTO_AEAD_DECRYPT \
+    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
+    le32 opcode;
+    /* algo should be service-specific algorithms */
+    le32 algo;
+    /* session_id should be service-specific algorithms */
+    le64 session_id;
+#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
+#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2
+    /* control flag to control the request */
+    le32 flag;
+    le32 padding;
+};
+\end{lstlisting}
+
+The device can set the operation status as follows: VIRTIO_CRYPTO_OK: success;
+VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP: not supported;
+VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto operations.
+
+\begin{lstlisting}
+enum VIRITO_CRYPTO_STATUS {
+    VIRTIO_CRYPTO_OK = 0,
+    VIRTIO_CRYPTO_ERR = 1,
+    VIRTIO_CRYPTO_BADMSG = 2,
+    VIRTIO_CRYPTO_NOTSUPP = 3,
+    VIRTIO_CRYPTO_INVSESS = 4,
+    VIRTIO_CRYPTO_MAX
+};
+\end{lstlisting}
+
+\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue}
+
+The driver uses the control virtqueue to send control commands to the
+device which handles the non-data plane operations, such as session
+operations (See \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation}).
+
+The packet of controlq:
+
+\begin{lstlisting}
+struct virtio_crypto_op_ctrl_req {
+    struct virtio_crypto_ctrl_header header;
+
+    union {
+        struct virtio_crypto_sym_create_session_req   sym_create_session;
+        struct virtio_crypto_hash_create_session_req  hash_create_session;
+        struct virtio_crypto_mac_create_session_req   mac_create_session;
+        struct virtio_crypto_aead_create_session_req  aead_create_session;
+        struct virtio_crypto_destroy_session_req      destroy_session;
+    } u;
+};
+\end{lstlisting}
+
+The header is the general header, and the union is of the algorithm-specific type,
+which is set by the driver. All the properties in the union are shown as follows.
+
+\paragraph{Session operation}\label{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation}
+
+The symmetric algorithms involve the concept of sessions. A session is a
+handle which describes the cryptographic parameters to be applied to
+a number of buffers. The data within a session handle includes the following:
+
+\begin{enumerate}
+\item The operation (CIPHER, HASH/MAC or both, and if both, the order in
+      which the algorithms should be applied).
+\item The CIPHER set data, including the CIPHER algorithm and mode,
+      the key and its length, and the direction (encryption or decryption).
+\item The HASH/MAC set data, including the HASH algorithm or MAC algorithm,
+      and hash result length (to allow for truncation).
+\begin{itemize*}
+\item Authenticated mode can refer to MAC, which requires that the key and
+      its length are also specified.
+\item For nested mode, the inner and outer prefix data and length are specified,
+      as well as the outer HASH algorithm.
+\end{itemize*}
+\end{enumerate}
+
+The following structure stores the result of session creation set by the device:
+
+\begin{lstlisting}
+struct virtio_crypto_session_input {
+    /* Device-writable part */
+    le64 session_id;
+    le32 status;
+    le32 padding;
+};
+\end{lstlisting}
+
+A request to destroy a session includes the following information:
+
+\begin{lstlisting}
+struct virtio_crypto_destroy_session_req {
+    /* Device-readable part */
+    le64  session_id;
+    /* Device-writable part */
+    le32  status;
+    le32  padding;
+};
+\end{lstlisting}
+
+\subparagraph{Session operation: HASH session}\label{sec:Device Types / Crypto Device / Device
+Operation / Control Virtqueue / Session operation / Session operation: HASH session}
+
+The packet of HASH session is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_hash_session_para {
+    /* See VIRTIO_CRYPTO_HASH_* above */
+    le32 algo;
+    /* hash result length */
+    le32 hash_result_len;
+};
+struct virtio_crypto_hash_create_session_req {
+    /* Device-readable part */
+    struct virtio_crypto_hash_session_para para;
+    /* Device-writable part */
+    struct virtio_crypto_session_input input;
+};
+\end{lstlisting}
+
+\subparagraph{Session operation: MAC session}\label{sec:Device Types / Crypto Device / Device
+Operation / Control Virtqueue / Session operation / Session operation: MAC session}
+
+The packet of MAC session is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_mac_session_para {
+    /* See VIRTIO_CRYPTO_MAC_* above */
+    le32 algo;
+    /* hash result length */
+    le32 hash_result_len;
+    /* length of authenticated key */
+    le32 auth_key_len;
+    le32 padding;
+};
+
+struct virtio_crypto_mac_create_session_req {
+    /* Device-readable part */
+    struct virtio_crypto_mac_session_para para;
+    /* The authenticated key */
+    u8 auth_key[auth_key_len];
+
+    /* Device-writable part */
+    struct virtio_crypto_session_input input;
+};
+\end{lstlisting}
+
+\subparagraph{Session operation: Symmetric algorithms session}\label{sec:Device Types / Crypto Device / Device
+Operation / Control Virtqueue / Session operation / Session operation: Symmetric algorithms session}
+
+The request of symmetric session includes two parts, CIPHER algorithms and chain
+algorithms (chaining CIPHER and HASH/MAC). The packet for CIPHER session is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_cipher_session_para {
+    /* See VIRTIO_CRYPTO_CIPHER* above */
+    le32 algo;
+    /* length of key */
+    le32 keylen;
+#define VIRTIO_CRYPTO_OP_ENCRYPT  1
+#define VIRTIO_CRYPTO_OP_DECRYPT  2
+    /* encryption or decryption */
+    le32 op;
+    le32 padding;
+};
+
+struct virtio_crypto_cipher_session_req {
+    /* Device-readable part */
+    struct virtio_crypto_cipher_session_para para;
+    /* The cipher key */
+    u8 cipher_key[keylen];
+
+    /* Device-writable part */
+    struct virtio_crypto_session_input input;
+};
+\end{lstlisting}
+
+The packet for algorithm chaining is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_alg_chain_session_para {
+#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER  1
+#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH  2
+    le32 alg_chain_order;
+/* Plain hash */
+#define VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN    1
+/* Authenticated hash (mac) */
+#define VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH     2
+/* Nested hash */
+#define VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED   3
+    le32 hash_mode;
+    struct virtio_crypto_cipher_session_para cipher_param;
+    union {
+        struct virtio_crypto_hash_session_para hash_param;
+        struct virtio_crypto_mac_session_para mac_param;
+    } u;
+    /* length of the additional authenticated data (AAD) in bytes */
+    le32 aad_len;
+    le32 padding;
+};
+
+struct virtio_crypto_alg_chain_session_req {
+    /* Device-readable part */
+    struct virtio_crypto_alg_chain_session_para para;
+    /* The cipher key */
+    u8 cipher_key[keylen];
+    /* The authenticated key */
+    u8 auth_key[auth_key_len];
+
+    /* Device-writable part */
+    struct virtio_crypto_session_input input;
+};
+\end{lstlisting}
+
+The packet for symmetric algorithm is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_sym_create_session_req {
+    union {
+        struct virtio_crypto_cipher_session_req cipher;
+        struct virtio_crypto_alg_chain_session_req chain;
+    } u;
+
+    /* Device-readable part */
+
+/* No operation */
+#define VIRTIO_CRYPTO_SYM_OP_NONE  0
+/* Cipher only operation on the data */
+#define VIRTIO_CRYPTO_SYM_OP_CIPHER  1
+/* Chain any cipher with any hash or mac operation. The order
+   depends on the value of alg_chain_order param */
+#define VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING  2
+    le32 op_type;
+    le32 padding;
+};
+\end{lstlisting}
+
+\subparagraph{Session operation: AEAD session}\label{sec:Device Types / Crypto Device / Device
+Operation / Control Virtqueue / Session operation / Session operation: AEAD session}
+
+The packet for AEAD session is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_aead_session_para {
+    /* See VIRTIO_CRYPTO_AEAD_* above */
+    le32 algo;
+    /* length of key */
+    le32 key_len;
+    /* Authentication tag length */
+    le32 tag_len;
+    /* The length of the additional authenticated data (AAD) in bytes */
+    le32 aad_len;
+    /* encryption or decryption, See above VIRTIO_CRYPTO_OP_* */
+    le32 op;
+    le32 padding;
+};
+
+struct virtio_crypto_aead_create_session_req {
+    /* Device-readable part */
+    struct virtio_crypto_aead_session_para para;
+    u8 key[key_len];
+
+    /* Device-writeable part */
+    struct virtio_crypto_session_input input;
+};
+\end{lstlisting}
+
+\drivernormative{\subparagraph}{Session operation: create session}{Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation / Session operation: create session}
+
+\begin{itemize*}
+\item The driver MUST set the control general header and corresponding properties of the union in structure virtio_crypto_op_ctrl_req. See \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue}.
+\item The driver MUST set \field{opcode} field based on service type: CIPHER, HASH, MAC, or AEAD.
+\item The driver MUST set \field{queue_id} field to show used dataq.
+\end{itemize*}
+
+\devicenormative{\subparagraph}{Session operation: create session}{Device Types / Crypto Device / Device
+Operation / Control Virtqueue / Session operation / Session operation: create session}
+
+\begin{itemize*}
+\item The device MUST set \field{session_id} field as a session identifier return to the driver when the device finishes processing session creation.
+\item The device MUST set \field{status} field to one of the values of enum VIRITO_CRYPTO_STATUS.
+\end{itemize*}
+
+\drivernormative{\subparagraph}{Session operation: destroy session}{Device Types / Crypto Device / Device
+Operation / Control Virtqueue / Session operation / Session operation: destroy session}
+
+\begin{itemize*}
+\item The driver MUST set \field{opcode} field based on service type: CIPHER, HASH, MAC, or AEAD.
+\item The driver MUST set the \field{session_id} to a valid value which assigned by the device when a session is created.
+\end{itemize*}
+
+\devicenormative{\subparagraph}{Session operation: destroy session}{Device Types / Crypto Device / Device
+Operation / Control Virtqueue / Session operation / Session operation: destroy session}
+
+\begin{itemize*}
+\item The device MUST set \field{status} field to one of the values of enum VIRITO_CRYPTO_STATUS.
+\end{itemize*}
+
+\subsubsection{Data Virtqueue}\label{sec:Device Types / Crypto Device / Device Operation / Data Virtqueue}
+
+The driver uses the data virtqueue to transmit the requests of crypto operation to the device,
+and completes the data plane operations (such as crypto operation).
+
+The session mode packet of dataq is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_op_data_req {
+    struct virtio_crypto_op_header header;
+
+    union {
+        struct virtio_crypto_sym_data_req   sym_req;
+        struct virtio_crypto_hash_data_req  hash_req;
+        struct virtio_crypto_mac_data_req   mac_req;
+        struct virtio_crypto_aead_data_req  aead_req;
+    } u;
+};
+\end{lstlisting}
+
+The packet of dataq, mixing both session and non-session mode is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_op_data_req_mux {
+    struct virtio_crypto_op_header header;
+
+    union {
+        struct virtio_crypto_sym_data_req   sym_req;
+        struct virtio_crypto_hash_data_req  hash_req;
+        struct virtio_crypto_mac_data_req   mac_req;
+        struct virtio_crypto_aead_data_req  aead_req;
+        struct virtio_crypto_sym_data_req_non_sess   sym_non_sess_req;
+        struct virtio_crypto_hash_data_req_non_sess  hash_non_sess_req;
+        struct virtio_crypto_mac_data_req_non_sess   mac_non_sess_req;
+        struct virtio_crypto_aead_data_req_non_sess  aead_non_sess_req;
+    } u;
+};
+\end{lstlisting}
+
+The header is the general header and the union is of the algorithm-specific type,
+which is set by the driver. All properties in the union are shown as follows.
+
+There is a unified input header structure for all crypto services.
+
+The structure is defined as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_inhdr {
+    u8 status;
+};
+\end{lstlisting}
+
+\subsubsection{HASH Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / HASH Service Operation}
+
+The session mode packet of HASH service:
+
+\begin{lstlisting}
+struct virtio_crypto_hash_para {
+    /* length of source data */
+    le32 src_data_len;
+    /* hash result length */
+    le32 hash_result_len;
+};
+
+struct virtio_crypto_hash_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_hash_para 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}
+
+Each data request uses virtio_crypto_hash_data_req structure to store information
+used to run the HASH operations. 
+
+The information includes the hash parameters stored by \field{para}, output data and input data.
+The output data here includes the source data and the input data includes the hash result data used to save the results of the HASH operations.
+\field{inhdr} stores status of executing the HASH operations.
+
+The non-session mode packet of HASH service is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_hash_para_non_session {
+    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_non_sess {
+    /* Device-readable part */
+    struct virtio_crypto_hash_para_non_session 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}
+
+\drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
+
+\begin{itemize*}
+\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
+      to a valid value which assigned by the device when a session is created.
+\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto packets. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.
+\item If the VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE feature bit is negotiated, 1) if the driver use the non-session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header
+      to VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE and MUST set fields in struct virtio_crypto_hash_para_non_session.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
+\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_HASH.
+\end{itemize*}
+
+\devicenormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
+
+\begin{itemize*}
+\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto packets. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
+\item If the VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE feature bit is negotiated, the device MUST parse \field{flag} field in struct virtio_crypto_op_header in order to decide which mode the driver uses.
+\item The device MUST copy the results of HASH operations to the hash_result[] if HASH operations success.
+\item The device MUST set \field{status} in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
+\end{itemize*}
+
+\subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / MAC Service Operation}
+
+The session mode packet of MAC service is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_mac_para {
+    struct virtio_crypto_hash_para hash;
+};
+
+struct virtio_crypto_mac_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_mac_para 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}
+
+Each data request uses virtio_crypto_mac_data_req structure to store information
+used to run the MAC operations. 
+
+The information includes the hash parameters stored by \field{para}, output data and input data.
+The output data here includes the source data and the input data includes the hash result data used to save the results of the MAC operations.
+\field{inhdr} stores status of executing the MAC operations.
+
+The non-session mode packet of MAC service:
+
+\begin{lstlisting}
+struct virtio_crypto_mac_para_non_sess {
+    struct {
+        /* See VIRTIO_CRYPTO_MAC_* above */
+        le32 algo;
+        /* length of authenticated key */
+        le32 auth_key_len;
+    } sess_para;
+
+    /* length of source data */
+    le32 src_data_len;
+    /* hash result length */
+    le32 hash_result_len;
+};
+
+struct virtio_crypto_mac_data_req_non_sess {
+    /* Device-readable part */
+    struct virtio_crypto_mac_para_non_sess para;
+    /* The authenticated key */
+    u8 auth_key[auth_key_len];
+    /* 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}
+
+\drivernormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
+
+\begin{itemize*}
+\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
+      to a valid value which assigned by the device when a session is created.
+\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto packets. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.
+\item If the VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE feature bit is negotiated, 1) if the driver use the non-session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header
+      to VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE and MUST set fields in struct virtio_crypto_mac_para_non_session.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
+\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_MAC.
+\end{itemize*}
+
+\devicenormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
+
+\begin{itemize*}
+\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto packets. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
+\item If the VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE feature bit is negotiated, the device MUST parse \field{flag} field in struct virtio_crypto_op_header in order to decide which mode the driver uses.
+\item The device MUST copy the results of MAC operations to the hash_result[] if HASH operations success.
+\item The device MUST set \field{status} in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
+\end{itemize*}
+
+\subsubsection{Symmetric algorithms Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
+
+The session mode packet of plain CIPHER service is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_cipher_para {
+    /*
+     * Byte Length of valid IV/Counter data pointed to by the below iv data.
+     *
+     * For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
+     *   SNOW3G in UEA2 mode, this is the length of the IV (which
+     *   must be the same as the block length of the cipher).
+     * For block ciphers in CTR mode, this is the length of the counter
+     *   (which must be the same as the block length of the cipher).
+     */
+    le32 iv_len;
+    /* length of source data */
+    le32 src_data_len;
+    /* length of destination data */
+    le32 dst_data_len;
+    le32 padding;
+};
+
+struct virtio_crypto_cipher_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_cipher_para para;
+    /*
+     * Initialization Vector or Counter data.
+     *
+     * For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
+     *   SNOW3G in UEA2 mode, this is the Initialization Vector (IV)
+     *   value.
+     * For block ciphers in CTR mode, this is the counter.
+     * For AES-XTS, this is the 128bit tweak, i, from IEEE Std 1619-2007.
+     *
+     * The IV/Counter will be updated after every partial cryptographic
+     * operation.
+     */
+    u8 iv[iv_len];
+    /* Source data */
+    u8 src_data[src_data_len];
+
+    /* Device-writable part */
+    /* Destination data */
+    u8 dst_data[dst_data_len];
+    struct virtio_crypto_inhdr inhdr;
+};
+\end{lstlisting}
+
+The session mode packet of algorithm chaining is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_alg_chain_data_para {
+    le32 iv_len;
+    /* Length of source data */
+    le32 src_data_len;
+    /* Length of destination data */
+    le32 dst_data_len;
+    /* Starting point for cipher processing in source data */
+    le32 cipher_start_src_offset;
+    /* Length of the source data that the cipher will be computed on */
+    le32 len_to_cipher;
+    /* Starting point for hash processing in source data */
+    le32 hash_start_src_offset;
+    /* Length of the source data that the hash will be computed on */
+    le32 len_to_hash;
+    /* Length of the additional auth data */
+    le32 aad_len;
+    /* Length of the hash result */
+    le32 hash_result_len;
+    le32 reserved;
+};
+
+struct virtio_crypto_alg_chain_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_alg_chain_data_para para;
+    /* Initialization Vector or Counter data */
+    u8 iv[iv_len];
+    /* Source data */
+    u8 src_data[src_data_len];
+    /* Additional authenticated data if exists  */
+    u8 aad[aad_len];
+
+    /* Device-writable part */
+    /* Destination data */
+    u8 dst_data[dst_data_len];
+    /* Hash result data */
+    u8 hash_result[hash_result_len];
+    struct virtio_crypto_inhdr inhdr;
+};
+\end{lstlisting}
+
+The session mode packet of symmetric algorithm is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_sym_data_req {
+    union {
+        struct virtio_crypto_cipher_data_req cipher;
+        struct virtio_crypto_alg_chain_data_req chain;
+    } u;
+
+    /* Device-readable part */
+
+    /* See above VIRTIO_CRYPTO_SYM_OP_* */
+    le32 op_type;
+    le32 padding;
+};
+\end{lstlisting}
+
+Each data request uses virtio_crypto_sym_data_req structure to store information
+used to run the CIPHER operations. 
+
+The information includes the cipher parameters stored by \field{para}, output data and input data.
+In the first virtio_crypto_cipher_para structure, \field{iv_len} specifies the length of the initialization vector or counter,
+\field{src_data_len} specifies the length of the source data, and \field{dst_data_len} specifies the
+length of the destination data. 
+For plain CIPHER operations, the output data here includes the IV/Counter data and source data, and the input data includes the destination data used to save the results of the CIPHER operations. 
+
+For algorithms chain, the output data here includes the IV/Counter data, source data and additional authenticated data if exists.
+The input data includes both destination data and hash result data used to store the results of the HASH/MAC operations.
+\field{inhdr} stores status of executing the crypto operations.
+
+The non-session mode packet of plain CIPHER service is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_cipher_para_non_sess {
+    struct {
+        /* See VIRTIO_CRYPTO_CIPHER* above */
+        le32 algo;
+        /* length of key */
+        le32 keylen;
+
+        /* See VIRTIO_CRYPTO_OP_* above */
+        le32 op;
+    } sess_para;
+
+    /*
+     * Byte Length of valid IV/Counter data pointed to by the below iv data.
+     */
+    le32 iv_len;
+    /* length of source data */
+    le32 src_data_len;
+    /* length of destination data */
+    le32 dst_data_len;
+};
+
+struct virtio_crypto_cipher_data_req_non_sess {
+    /* Device-readable part */
+    struct virtio_crypto_cipher_para_non_sess para;
+    /* The cipher key */
+    u8 cipher_key[keylen];
+
+    /* Initialization Vector or Counter data. */
+    u8 iv[iv_len];
+    /* Source data */
+    u8 src_data[src_data_len];
+
+    /* Device-writable part */
+    /* Destination data */
+    u8 dst_data[dst_data_len];
+    struct virtio_crypto_inhdr inhdr;
+};
+\end{lstlisting}
+
+The non-session mode packet of algorithm chaining is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_alg_chain_data_para_non_sess {
+    struct {
+        /* See VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_* above */
+        le32 alg_chain_order;
+        /* length of the additional authenticated data in bytes */
+        le32 aad_len;
+
+        struct {
+            /* See VIRTIO_CRYPTO_CIPHER* above */
+            le32 algo;
+            /* length of key */
+            le32 keylen;
+            /* See VIRTIO_CRYPTO_OP_* above */
+            le32 op;
+        } cipher;
+
+        struct {
+            /* See VIRTIO_CRYPTO_HASH_* or VIRTIO_CRYPTO_MAC_* above */
+            le32 algo;
+            /* length of authenticated key */
+            le32 auth_key_len;
+            /* See VIRTIO_CRYPTO_SYM_HASH_MODE_* above */
+            le32 hash_mode;
+        } hash;
+    } sess_para;
+
+    le32 iv_len;
+    /* Length of source data */
+    le32 src_data_len;
+    /* Length of destination data */
+    le32 dst_data_len;
+    /* Starting point for cipher processing in source data */
+    le32 cipher_start_src_offset;
+    /* Length of the source data that the cipher will be computed on */
+    le32 len_to_cipher;
+    /* Starting point for hash processing in source data */
+    le32 hash_start_src_offset;
+    /* Length of the source data that the hash will be computed on */
+    le32 len_to_hash;
+    /* Length of the additional auth data */
+    le32 aad_len;
+    /* Length of the hash result */
+    le32 hash_result_len;
+    le32 reserved;
+};
+
+struct virtio_crypto_alg_chain_data_req_non_sess {
+    /* Device-readable part */
+    struct virtio_crypto_alg_chain_data_para_non_sess para;
+    /* The cipher key */
+    u8 cipher_key[keylen];
+    /* The auth key */
+    u8 auth_key[auth_key_len];
+    /* Initialization Vector or Counter data */
+    u8 iv[iv_len];
+    /* Source data */
+    u8 src_data[src_data_len];
+    /* Additional authenticated data if exists  */
+    u8 aad[aad_len];
+
+    /* Device-writable part */
+    /* Destination data */
+    u8 dst_data[dst_data_len];
+    /* Hash result data */
+    u8 hash_result[hash_result_len];
+    struct virtio_crypto_inhdr inhdr;
+};
+\end{lstlisting}
+
+The non-session mode packet of symmetric algorithm is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_sym_data_req_non_sess {
+    union {
+        struct virtio_crypto_cipher_data_req_non_sess cipher;
+        struct virtio_crypto_alg_chain_data_req_non_sess chain;
+    } u;
+
+    /* Device-readable part */
+
+    /* See above VIRTIO_CRYPTO_SYM_OP_* */
+    le32 op_type;
+    le32 padding;
+};
+\end{lstlisting}
+
+\drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
+
+\begin{itemize*}
+\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
+      to a valid value which assigned by the device when a session is created.
+\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto packets. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.
+\item If the VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is negotiated, 1) if the driver use the non-session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header
+      to VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE and MUST set fields in struct virtio_crypto_cipher_para_non_session.sess_para or struct virtio_crypto_alg_chain_data_para_non_sess.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
+\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_CIPHER_ENCRYPT or VIRTIO_CRYPTO_CIPHER_DECRYPT.
+\item The driver MUST specify the fields of struct virtio_crypto_cipher_data_req in struct virtio_crypto_sym_data_req if the packet is based on VIRTIO_CRYPTO_SYM_OP_CIPHER.
+\item The driver MUST specify the fields of both struct virtio_crypto_cipher_data_req and struct virtio_crypto_mac_data_req in struct virtio_crypto_sym_data_req if the packet
+      is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type and in the VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH mode.
+\end{itemize*}
+
+\devicenormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
+
+\begin{itemize*}
+\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto packets. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
+\item If the VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is negotiated, the device MUST parse \field{flag} field in struct virtio_crypto_op_header in order to decide which mode the driver uses.
+\item The device MUST parse the virtio_crypto_sym_data_req based on the \field{opcode} in general header.
+\item The device SHOULD only parse fields of struct virtio_crypto_cipher_data_req in struct virtio_crypto_sym_data_req if the packet is VIRTIO_CRYPTO_SYM_OP_CIPHER type.
+\item The device MUST parse fields of both struct virtio_crypto_cipher_data_req and struct virtio_crypto_mac_data_req in struct virtio_crypto_sym_data_req if the packet
+      is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING operation type and in the VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH mode.
+\item The device MUST copy the result of cryptographic operation to the dst_data[] in both plain CIPHER mode and algorithms chain mode.
+\item The device MUST check the \field{para}.\field{add_len} is bigger than 0 before parse the additional authenticated data in plain algorithms chain mode.
+\item The device MUST copy the result of HASH/MAC operation to the hash_result[] is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type.
+\item The device MUST set the \field{status} field in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
+\end{itemize*}
+
+\paragraph{Steps of Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation / Steps of Operation}
+
+\subparagraph{Step1: Create session}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation / Steps of Operation / Step1: Create session on session mode}
+
+\begin{enumerate}
+\item The driver specifies information in struct virtio_crypto_op_ctrl_req, including the algorithm name, key, keylen etc;
+\item The driver adds the request of session creation into the controlq's Vring Descriptor Table;
+\item The driver kicks the device;
+\item The device receives the request from controlq;
+\item The device parses information about the request, and determines the information concerning the backend crypto accelerator;
+\item The device packs information based on the APIs of the backend crypto accelerator;
+\item The device invokes the session creation APIs of the backend crypto accelerator to create a session;
+\item The device returns the session id to the driver.
+\end{enumerate}
+
+\subparagraph{Step2: Execute cryptographic operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation / Steps of Operation / Step2: Execute cryptographic operation}
+
+\begin{enumerate}
+\item The driver specifies information in struct virtio_crypto_op_data_req, including struct virtio_crypto_op_header and struct virtio_crypto_sym_data_req, see \ref{sec:Device Types / Crypto Device / Device
+      Operation / Symmetric algorithms Operation};
+\item The driver adds the request for cryptographic operation into the dataq's Vring Descriptor Table;
+\item The driver kicks the device (Or the device actively polls the dataq's Vring Descriptor Table);
+\item The device receives the request from dataq;
+\item The device parses information about the request, and determines the identification information for the backend crypto accelerator. For example, converting guest physical addresses to host physical addresses;
+\item The device packs identification information based on the API of the backend crypto accelerator;
+\item The device invokes the cryptographic APIs of the backend crypto accelerator;
+\item The backend crypto accelerator executes the cryptographic operation implicitly;
+\item The device receives the cryptographic results from the backend crypto accelerator (synchronous or asynchronous);
+\item The device sets the \field{status} in struct virtio_crypto_inhdr;
+\item The device updates and flushes the Used Ring to return the cryptographic results to the driver;
+\item The device notifies the driver (Or the driver actively polls the dataq's Used Ring);
+\item The driver saves the cryptographic results.
+\end{enumerate}
+
+\begin{note}
+\begin{itemize*}
+\item For better performance, the device should by preference use vhost scheme (user space or kernel space)
+      as the backend crypto accelerator in the real production environment.
+\end{itemize*}
+\end{note}
+
+\subsubsection{AEAD Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / AEAD Service Operation}
+
+The session mode packet of symmetric algorithm is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_aead_para {
+    /*
+     * Byte Length of valid IV data.
+     *
+     * For GCM mode, this is either 12 (for 96-bit IVs) or 16, in which
+     *   case iv points to J0.
+     * For CCM mode, this is the length of the nonce, which can be in the
+     *   range 7 to 13 inclusive.
+     */
+    le32 iv_len;
+    /* length of additional auth data */
+    le32 aad_len;
+    /* length of source data */
+    le32 src_data_len;
+    /* length of dst data, this should be at least src_data_len + tag_len */
+    le32 dst_data_len;
+    /* Authentication tag length */
+    le32 tag_len;
+    le32 reserved;
+};
+
+struct virtio_crypto_aead_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_aead_para para;
+    /*
+     * Initialization Vector data.
+     *
+     * For GCM mode, this is either the IV (if the length is 96 bits) or J0
+     *   (for other sizes), where J0 is as defined by NIST SP800-38D.
+     *   Regardless of the IV length, a full 16 bytes needs to be allocated.
+     * For CCM mode, the first byte is reserved, and the nonce should be
+     *   written starting at &iv[1] (to allow space for the implementation
+     *   to write in the flags in the first byte).  Note that a full 16 bytes
+     *   should be allocated, even though the iv_len field will have
+     *   a value less than this.
+     *
+     * The IV will be updated after every partial cryptographic operation.
+     */
+    u8 iv[iv_len];
+    /* Source data */
+    u8 src_data[src_data_len];
+    /* Additional authenticated data if exists  */
+    u8 aad[aad_len];
+
+    /* Device-writable part */
+    /* Pointer to output data */
+    u8 dst_data[dst_data_len];
+
+    struct virtio_crypto_inhdr inhdr;
+};
+\end{lstlisting}
+
+Each data request uses virtio_crypto_aead_data_req structure to store information
+used to run the AEAD operations. 
+
+The information includes the hash parameters stored by \field{para}, output data and input data.
+In the first virtio_crypto_aead_para structure, \field{iv_len} specifies the length of the initialization vector. \field{tag_len} specifies the length of the authentication tag;
+\field{aad_len} specifies the length of additional authentication data, \field{src_data_len} specifies the
+length of the source data; \field{dst_data_len} specifies the length of the destination data, which is at least \field{src_data_len} + \field{tag_len}.
+
+The output data here includes the IV/Counter data, source data and additional authenticated data if exists.
+The input data includes both destination data used to save the results of the AEAD operations.
+\field{inhdr} stores status of executing the AEAD operations.
+
+The non-session mode packet of AEAD service is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_aead_para_non_sess {
+    struct {
+        /* See VIRTIO_CRYPTO_AEAD_* above */
+        le32 algo;
+        /* length of key */
+        le32 key_len;
+        /* encrypt or decrypt, See above VIRTIO_CRYPTO_OP_* */
+        le32 op;
+    } sess_para;
+
+    /* Byte Length of valid IV data. */
+    le32 iv_len;
+    /* Authentication tag length */
+    le32 tag_len;
+    /* length of additional auth data */
+    le32 aad_len;
+    /* length of source data */
+    le32 src_data_len;
+    /* length of dst data, this should be at least src_data_len + tag_len */
+    le32 dst_data_len;
+};
+
+struct virtio_crypto_aead_data_req_non_sess {
+    /* Device-readable part */
+    struct virtio_crypto_aead_para_non_sess para;
+    /* The cipher key */
+    u8 key[key_len];
+    /* Initialization Vector data. */
+    u8 iv[iv_len];
+    /* Source data */
+    u8 src_data[src_data_len];
+    /* Additional authenticated data if exists  */
+    u8 aad[aad_len];
+
+    /* Device-writable part */
+    /* Pointer to output data */
+    u8 dst_data[dst_data_len];
+
+    struct virtio_crypto_inhdr inhdr;
+};
+\end{lstlisting}
+
+\drivernormative{\paragraph}{AEAD Service Operation}{Device Types / Crypto Device / Device Operation / AEAD Service Operation}
+
+\begin{itemize*}
+\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
+      to a valid value which assigned by the device when a session is created.
+\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto packets. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.
+\item If the VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE feature bit is negotiated, 1) if the driver use the non-session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header
+      to VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE and MUST set fields in struct virtio_crypto_aead_para_non_session.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
+\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_AEAD_ENCRYPT or VIRTIO_CRYPTO_AEAD_DECRYPT.
+\end{itemize*}
+
+\devicenormative{\paragraph}{AEAD Service Operation}{Device Types / Crypto Device / Device Operation / AEAD Service Operation}
+
+\begin{itemize*}
+\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto packets. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
+\item If the VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the virtio_crypto_aead_data_req based on the \field{opcode} in general header.
+\item The device MUST copy the result of cryptographic operation to the dst_data[].
+\item The device MUST copy the authentication tag to the dst_data[] offset the cipher result.
+\item The device MUST set the \field{status} field in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
+\item When the \field{opcode} is VIRTIO_CRYPTO_AEAD_DECRYPT, the device MUST verify and return the verification result to the driver, and if the verification result is incorrect, VIRTIO_CRYPTO_BADMSG (bad message) MUST be returned to the driver.
+\end{itemize*}
\ No newline at end of file
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Stefan Hajnoczi]

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

On Wed, Jan 18, 2017 at 04:22:36PM +0800, Gonglei wrote:
> The virtio crypto device is a virtual crypto device (ie. hardware
> crypto accelerator card). Currently, the virtio crypto device provides
> the following crypto services: CIPHER, MAC, HASH, and AEAD.
> 
> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
> 
> VIRTIO-153
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Cornelia Huck <cornelia.huck@de.ibm.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lingli Deng <denglingli@chinamobile.com>
> CC: Jani Kokkonen <Jani.Kokkonen@huawei.com>
> CC: Ola Liljedahl <Ola.Liljedahl@arm.com>
> CC: Varun Sethi <Varun.Sethi@freescale.com>
> CC: Zeng Xin <xin.zeng@intel.com>
> CC: Keating Brian <brian.a.keating@intel.com>
> CC: Ma Liang J <liang.j.ma@intel.com>
> CC: Griffin John <john.griffin@intel.com>
> CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> ---
>  content.tex       |    2 +
>  virtio-crypto.tex | 1245 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 1247 insertions(+)
>  create mode 100644 virtio-crypto.tex

I'm happy with the device overall.  See specific comments below.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> 
> diff --git a/content.tex b/content.tex
> index 4b45678..ab75f78 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len}, \field{residual},
>  \field{status_qualifier}, \field{status}, \field{response} and
>  \field{sense} fields.
>  
> +\input{virtio-crypto.tex}
> +
>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>  
>  Currently there are three device-independent feature bits defined:
> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> new file mode 100644
> index 0000000..732cd30
> --- /dev/null
> +++ b/virtio-crypto.tex
> @@ -0,0 +1,1245 @@
> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> +
> +The virtio crypto device is a virtual cryptography device as well as a kind of
> +virtual hardware accelerator for virtual machines. The encryption and
> +decryption requests are placed in any of the data active queues and are ultimately handled by the
> +backend crypto accelerators. The second kind of queue is the control queue used to create 
> +or destroy sessions for symmetric algorithms and will control some advanced
> +features in the future. The virtio crypto device provides the following crypto
> +services: CIPHER, MAC, HASH, and AEAD.
> +
> +
> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
> +
> +20
> +
> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] dataq1
> +\item[\ldots]
> +\item[N-1] dataqN
> +\item[N] controlq
> +\end{description}
> +
> +N is set by \field{max_dataqueues}.
> +
> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature bits}
> +
> +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
> +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for CIPHER service.
> +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for HASH service.
> +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC service.
> +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for AEAD service.

Bits 1-4 require bit 0.  Is bit 0 necessary at all?  Or may bits 1-4 can
be eliminated in favor of just bit 0.

> +
> +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto Device / Feature bits}
> +
> +Some crypto feature bits require other crypto feature bits
> +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}):
> +
> +\begin{description}
> +\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> +\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> +\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> +\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> +\end{description}
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / Crypto Device / Device configuration layout}
> +
> +The following driver-read-only configuration fields are defined:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_config {
> +    le32 status;
> +    le32 max_dataqueues;
> +    le32 crypto_services;
> +    /* Detailed algorithms mask */
> +    le32 cipher_algo_l;
> +    le32 cipher_algo_h;
> +    le32 hash_algo;
> +    le32 mac_algo_l;
> +    le32 mac_algo_h;
> +    le32 aead_algo;
> +    /* Maximum length of cipher key */
> +    le32 max_cipher_key_len;
> +    /* Maximum length of authenticated key */
> +    le32 max_auth_key_len;
> +    le32 reserved;
> +    /* Maximum size of each crypto request's content */
> +    le64 max_size;
> +};
> +\end{lstlisting}
> +
> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or ~VIRTIO_CRYPTO_S_HW_READY.
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
> +\end{lstlisting}
> +
> +The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the hardware is ready to work or not.
> +
> +The following driver-read-only fields include \field{max_dataqueues}, which specifies the
> +maximum number of data virtqueues (dataq1\ldots dataqN), and \field{crypto_services},
> +which indicates the crypto services the virtio crypto supports.
> +
> +The following services are defined:
> +
> +\begin{lstlisting}
> +/* CIPHER service */
> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> +/* HASH service */
> +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> +/* MAC (Message Authentication Codes) service */
> +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> +/* AEAD (Authenticated Encryption with Associated Data) service */
> +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> +\end{lstlisting}
> +
> +The last driver-read-only fields specify detailed algorithms masks 

s/The last driver-read-only fields/Further driver-read-only fields/

(cipher_algo_l, cipher_algo_h, etc are not the last fields.)

> +the device offers for corresponding services. The following CIPHER algorithms
> +are defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> +\end{lstlisting}
> +
> +The following HASH algorithms are defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_HASH            0
> +#define VIRTIO_CRYPTO_HASH_MD5           1
> +#define VIRTIO_CRYPTO_HASH_SHA1          2
> +#define VIRTIO_CRYPTO_HASH_SHA_224       3
> +#define VIRTIO_CRYPTO_HASH_SHA_256       4
> +#define VIRTIO_CRYPTO_HASH_SHA_384       5
> +#define VIRTIO_CRYPTO_HASH_SHA_512       6
> +#define VIRTIO_CRYPTO_HASH_SHA3_224      7
> +#define VIRTIO_CRYPTO_HASH_SHA3_256      8
> +#define VIRTIO_CRYPTO_HASH_SHA3_384      9
> +#define VIRTIO_CRYPTO_HASH_SHA3_512      10
> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128      11
> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256      12
> +\end{lstlisting}
> +
> +The following MAC algorithms are defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_MAC                       0
> +#define VIRTIO_CRYPTO_MAC_HMAC_MD5                 1
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1                2
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224             3
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256             4
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384             5
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512             6
> +#define VIRTIO_CRYPTO_MAC_CMAC_3DES                25
> +#define VIRTIO_CRYPTO_MAC_CMAC_AES                 26
> +#define VIRTIO_CRYPTO_MAC_KASUMI_F9                27
> +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2              28
> +#define VIRTIO_CRYPTO_MAC_GMAC_AES                 41
> +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             42
> +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               49
> +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         50
> +#define VIRTIO_CRYPTO_MAC_XCBC_AES                 53
> +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3                 54
> +\end{lstlisting}
> +
> +The following AEAD algorithms are defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_AEAD     0
> +#define VIRTIO_CRYPTO_AEAD_GCM    1
> +#define VIRTIO_CRYPTO_AEAD_CCM    2
> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
> +\end{lstlisting}
> +
> +\begin{note}
> +Any other values are reserved for future use.
> +\end{note}
> +
> +\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Crypto Device / Device configuration layout}
> +
> +\begin{itemize*}
> +\item The device MUST set \field{max_dataqueues} to between 1 and 65535 inclusive.
> +\item The device MUST set \field{status} based on the status of the hardware-backed implementation. 
> +\item The device MUST accept and handle requests after \field{status} is set to VIRTIO_CRYPTO_S_HW_READY.
> +\item The device MUST set \field{crypto_services} based on the crypto services the device offers.
> +\item The device MUST set detailed algorithms masks based on the \field{crypto_services} field.
> +\item The device MUST set \field{max_size} to show the maximum size of crypto request the device supports.

In bytes?

> +\item The device MUST set \field{max_cipher_key_len} to show the maximum length of cipher key if the device supports CIPHER service.

In bits or bytes?

> +\item The device MUST set \field{max_auth_key_len} to show the maximum length of authenticated key if the device supports MAC service.

In bits or bytes?

> +\end{itemize*}
> +
> +\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Crypto Device / Device configuration layout}
> +
> +\begin{itemize*}
> +\item The driver MUST read the ready \field{status} from the bottom bit of status to check whether the hardware-backed
> +      implementation is ready or not, and the driver MUST reread it after the device reset. 
> +\item The driver MUST NOT transmit any packets to the device if the ready \field{status} is not set.
> +\item The driver MUST read \field{max_dataqueues} field to discover the number of data queues the device supports.
> +\item The driver MUST read \field{crypto_services} field to discover which services the device is able to offer.
> +\item The driver MUST read the detailed algorithms fields based on \field{crypto_services} field.
> +\item The driver SHOULD read \field{max_size} to discover the maximum size of crypto request the device supports.
> +\item The driver SHOULD read \field{max_cipher_key_len} to discover the maximum length of cipher key the device supports.
> +\item The driver SHOULD read \field{max_auth_key_len} to discover the maximum length of authenticated key the device supports.
> +\end{itemize*}
> +
> +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device / Device Initialization}
> +
> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / Crypto Device / Device Initialization}
> +
> +\begin{itemize*}
> +\item The driver MUST identify and initialize the control virtqueue.
> +\item The driver MUST read the supported crypto services from bits of \field{crypto_services}. 
> +\item The driver MUST read the supported algorithms based on \field{crypto_services} field.
> +\end{itemize*}
> +
> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Crypto Device / Device Initialization}
> +
> +\begin{itemize*}
> +\item The device MUST be configured with at least one accelerator which executes backend crypto operations.
> +\item The device MUST write the \field{crypto_services} field based on the capacities of the backend accelerator.
> +\end{itemize*}
> +
> +\subsection{Device Operation}\label{sec:Device Types / Crypto Device / Device Operation}
> +
> +Packets can be transmitted by placing them in both the controlq and dataq.
> +Packets consist of a general header and a service-specific request.
> +Where 'general header' is for all crypto requests, and 'service specific requests'
> +are composed of operation parameter + output data + input data in general.
> +Operation parameters are algorithm-specific parameters, output data is the
> +data that should be utilized in operations, and input data is equal to
> +"operation result + result data".
> +
> +The device can support both session mode (See \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation}) and non-session mode, for example,
> +As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is negotiated, the driver can use non-session mode for CIPHER service, otherwise it can only use session mode.
> +
> +\begin{note}
> +The basic unit of all data length the byte.

s/data length the byte/data length is the byte/

> +\end{note}
> +
> +The general header for controlq is as follows:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
> +
> +struct virtio_crypto_ctrl_header {
> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x02)
> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x03)
> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
> +    le32 opcode;
> +    le32 algo;
> +    le32 flag;
> +    /* data virtqueue id */
> +    le32 queue_id;
> +};
> +\end{lstlisting}
> +
> +The general header of dataq:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_op_header {
> +#define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
> +#define VIRTIO_CRYPTO_CIPHER_DECRYPT \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
> +#define VIRTIO_CRYPTO_HASH \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
> +#define VIRTIO_CRYPTO_MAC \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
> +#define VIRTIO_CRYPTO_AEAD_ENCRYPT \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> +#define VIRTIO_CRYPTO_AEAD_DECRYPT \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> +    le32 opcode;
> +    /* algo should be service-specific algorithms */
> +    le32 algo;
> +    /* session_id should be service-specific algorithms */
> +    le64 session_id;
> +#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
> +#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2
> +    /* control flag to control the request */
> +    le32 flag;
> +    le32 padding;
> +};
> +\end{lstlisting}
> +
> +The device can set the operation status as follows: VIRTIO_CRYPTO_OK: success;
> +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP: not supported;
> +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto operations.
> +
> +\begin{lstlisting}
> +enum VIRITO_CRYPTO_STATUS {
> +    VIRTIO_CRYPTO_OK = 0,
> +    VIRTIO_CRYPTO_ERR = 1,
> +    VIRTIO_CRYPTO_BADMSG = 2,
> +    VIRTIO_CRYPTO_NOTSUPP = 3,
> +    VIRTIO_CRYPTO_INVSESS = 4,
> +    VIRTIO_CRYPTO_MAX
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue}
> +
> +The driver uses the control virtqueue to send control commands to the
> +device which handles the non-data plane operations, such as session
> +operations (See \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation}).
> +
> +The packet of controlq:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_op_ctrl_req {
> +    struct virtio_crypto_ctrl_header header;
> +
> +    union {
> +        struct virtio_crypto_sym_create_session_req   sym_create_session;
> +        struct virtio_crypto_hash_create_session_req  hash_create_session;
> +        struct virtio_crypto_mac_create_session_req   mac_create_session;
> +        struct virtio_crypto_aead_create_session_req  aead_create_session;
> +        struct virtio_crypto_destroy_session_req      destroy_session;
> +    } u;
> +};
> +\end{lstlisting}
> +
> +The header is the general header, and the union is of the algorithm-specific type,
> +which is set by the driver. All the properties in the union are shown as follows.

The spec isn't clear on whether crtl reqs should be sizeof(struct
virtio_crypto_op_ctrl_req) or if it's okay to use the smaller
sizeof(struct virtio_crypto_ctrl_header) + sizeof(struct
virtio_crypto_sym_create_session_req), for example.

Please clarify.

> +
> +\paragraph{Session operation}\label{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation}
> +
> +The symmetric algorithms involve the concept of sessions. A session is a
> +handle which describes the cryptographic parameters to be applied to
> +a number of buffers. The data within a session handle includes the following:
> +
> +\begin{enumerate}
> +\item The operation (CIPHER, HASH/MAC or both, and if both, the order in
> +      which the algorithms should be applied).
> +\item The CIPHER set data, including the CIPHER algorithm and mode,
> +      the key and its length, and the direction (encryption or decryption).
> +\item The HASH/MAC set data, including the HASH algorithm or MAC algorithm,
> +      and hash result length (to allow for truncation).
> +\begin{itemize*}
> +\item Authenticated mode can refer to MAC, which requires that the key and
> +      its length are also specified.
> +\item For nested mode, the inner and outer prefix data and length are specified,
> +      as well as the outer HASH algorithm.
> +\end{itemize*}
> +\end{enumerate}
> +
> +The following structure stores the result of session creation set by the device:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_session_input {
> +    /* Device-writable part */
> +    le64 session_id;
> +    le32 status;
> +    le32 padding;
> +};
> +\end{lstlisting}
> +
> +A request to destroy a session includes the following information:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_destroy_session_req {
> +    /* Device-readable part */
> +    le64  session_id;
> +    /* Device-writable part */
> +    le32  status;
> +    le32  padding;
> +};
> +\end{lstlisting}
> +
> +\subparagraph{Session operation: HASH session}\label{sec:Device Types / Crypto Device / Device
> +Operation / Control Virtqueue / Session operation / Session operation: HASH session}
> +
> +The packet of HASH session is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_hash_session_para {
> +    /* See VIRTIO_CRYPTO_HASH_* above */
> +    le32 algo;
> +    /* hash result length */
> +    le32 hash_result_len;
> +};
> +struct virtio_crypto_hash_create_session_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_hash_session_para para;
> +    /* Device-writable part */
> +    struct virtio_crypto_session_input input;
> +};
> +\end{lstlisting}
> +
> +\subparagraph{Session operation: MAC session}\label{sec:Device Types / Crypto Device / Device
> +Operation / Control Virtqueue / Session operation / Session operation: MAC session}
> +
> +The packet of MAC session is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_mac_session_para {
> +    /* See VIRTIO_CRYPTO_MAC_* above */
> +    le32 algo;
> +    /* hash result length */
> +    le32 hash_result_len;
> +    /* length of authenticated key */
> +    le32 auth_key_len;
> +    le32 padding;
> +};
> +
> +struct virtio_crypto_mac_create_session_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_mac_session_para para;
> +    /* The authenticated key */
> +    u8 auth_key[auth_key_len];
> +
> +    /* Device-writable part */
> +    struct virtio_crypto_session_input input;
> +};
> +\end{lstlisting}
> +
> +\subparagraph{Session operation: Symmetric algorithms session}\label{sec:Device Types / Crypto Device / Device
> +Operation / Control Virtqueue / Session operation / Session operation: Symmetric algorithms session}
> +
> +The request of symmetric session includes two parts, CIPHER algorithms and chain
> +algorithms (chaining CIPHER and HASH/MAC). The packet for CIPHER session is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_cipher_session_para {
> +    /* See VIRTIO_CRYPTO_CIPHER* above */
> +    le32 algo;
> +    /* length of key */
> +    le32 keylen;
> +#define VIRTIO_CRYPTO_OP_ENCRYPT  1
> +#define VIRTIO_CRYPTO_OP_DECRYPT  2
> +    /* encryption or decryption */
> +    le32 op;
> +    le32 padding;
> +};
> +
> +struct virtio_crypto_cipher_session_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_cipher_session_para para;
> +    /* The cipher key */
> +    u8 cipher_key[keylen];
> +
> +    /* Device-writable part */
> +    struct virtio_crypto_session_input input;
> +};
> +\end{lstlisting}
> +
> +The packet for algorithm chaining is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_alg_chain_session_para {
> +#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER  1
> +#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH  2
> +    le32 alg_chain_order;
> +/* Plain hash */
> +#define VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN    1
> +/* Authenticated hash (mac) */
> +#define VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH     2
> +/* Nested hash */
> +#define VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED   3
> +    le32 hash_mode;
> +    struct virtio_crypto_cipher_session_para cipher_param;
> +    union {
> +        struct virtio_crypto_hash_session_para hash_param;
> +        struct virtio_crypto_mac_session_para mac_param;
> +    } u;
> +    /* length of the additional authenticated data (AAD) in bytes */
> +    le32 aad_len;
> +    le32 padding;
> +};
> +
> +struct virtio_crypto_alg_chain_session_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_alg_chain_session_para para;
> +    /* The cipher key */
> +    u8 cipher_key[keylen];
> +    /* The authenticated key */
> +    u8 auth_key[auth_key_len];
> +
> +    /* Device-writable part */
> +    struct virtio_crypto_session_input input;
> +};
> +\end{lstlisting}
> +
> +The packet for symmetric algorithm is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_sym_create_session_req {
> +    union {
> +        struct virtio_crypto_cipher_session_req cipher;
> +        struct virtio_crypto_alg_chain_session_req chain;
> +    } u;
> +
> +    /* Device-readable part */
> +
> +/* No operation */
> +#define VIRTIO_CRYPTO_SYM_OP_NONE  0
> +/* Cipher only operation on the data */
> +#define VIRTIO_CRYPTO_SYM_OP_CIPHER  1
> +/* Chain any cipher with any hash or mac operation. The order
> +   depends on the value of alg_chain_order param */
> +#define VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING  2
> +    le32 op_type;
> +    le32 padding;
> +};
> +\end{lstlisting}
> +
> +\subparagraph{Session operation: AEAD session}\label{sec:Device Types / Crypto Device / Device
> +Operation / Control Virtqueue / Session operation / Session operation: AEAD session}
> +
> +The packet for AEAD session is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_aead_session_para {
> +    /* See VIRTIO_CRYPTO_AEAD_* above */
> +    le32 algo;
> +    /* length of key */
> +    le32 key_len;
> +    /* Authentication tag length */
> +    le32 tag_len;
> +    /* The length of the additional authenticated data (AAD) in bytes */
> +    le32 aad_len;
> +    /* encryption or decryption, See above VIRTIO_CRYPTO_OP_* */
> +    le32 op;
> +    le32 padding;
> +};
> +
> +struct virtio_crypto_aead_create_session_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_aead_session_para para;
> +    u8 key[key_len];
> +
> +    /* Device-writeable part */
> +    struct virtio_crypto_session_input input;
> +};
> +\end{lstlisting}
> +
> +\drivernormative{\subparagraph}{Session operation: create session}{Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation / Session operation: create session}
> +
> +\begin{itemize*}
> +\item The driver MUST set the control general header and corresponding properties of the union in structure virtio_crypto_op_ctrl_req. See \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue}.
> +\item The driver MUST set \field{opcode} field based on service type: CIPHER, HASH, MAC, or AEAD.
> +\item The driver MUST set \field{queue_id} field to show used dataq.
> +\end{itemize*}
> +
> +\devicenormative{\subparagraph}{Session operation: create session}{Device Types / Crypto Device / Device
> +Operation / Control Virtqueue / Session operation / Session operation: create session}
> +
> +\begin{itemize*}
> +\item The device MUST set \field{session_id} field as a session identifier return to the driver when the device finishes processing session creation.
> +\item The device MUST set \field{status} field to one of the values of enum VIRITO_CRYPTO_STATUS.
> +\end{itemize*}
> +
> +\drivernormative{\subparagraph}{Session operation: destroy session}{Device Types / Crypto Device / Device
> +Operation / Control Virtqueue / Session operation / Session operation: destroy session}
> +
> +\begin{itemize*}
> +\item The driver MUST set \field{opcode} field based on service type: CIPHER, HASH, MAC, or AEAD.
> +\item The driver MUST set the \field{session_id} to a valid value which assigned by the device when a session is created.
> +\end{itemize*}
> +
> +\devicenormative{\subparagraph}{Session operation: destroy session}{Device Types / Crypto Device / Device
> +Operation / Control Virtqueue / Session operation / Session operation: destroy session}
> +
> +\begin{itemize*}
> +\item The device MUST set \field{status} field to one of the values of enum VIRITO_CRYPTO_STATUS.
> +\end{itemize*}
> +
> +\subsubsection{Data Virtqueue}\label{sec:Device Types / Crypto Device / Device Operation / Data Virtqueue}
> +
> +The driver uses the data virtqueue to transmit the requests of crypto operation to the device,
> +and completes the data plane operations (such as crypto operation).
> +
> +The session mode packet of dataq is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_op_data_req {
> +    struct virtio_crypto_op_header header;
> +
> +    union {
> +        struct virtio_crypto_sym_data_req   sym_req;
> +        struct virtio_crypto_hash_data_req  hash_req;
> +        struct virtio_crypto_mac_data_req   mac_req;
> +        struct virtio_crypto_aead_data_req  aead_req;
> +    } u;
> +};
> +\end{lstlisting}
> +
> +The packet of dataq, mixing both session and non-session mode is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_op_data_req_mux {
> +    struct virtio_crypto_op_header header;
> +
> +    union {
> +        struct virtio_crypto_sym_data_req   sym_req;
> +        struct virtio_crypto_hash_data_req  hash_req;
> +        struct virtio_crypto_mac_data_req   mac_req;
> +        struct virtio_crypto_aead_data_req  aead_req;
> +        struct virtio_crypto_sym_data_req_non_sess   sym_non_sess_req;
> +        struct virtio_crypto_hash_data_req_non_sess  hash_non_sess_req;
> +        struct virtio_crypto_mac_data_req_non_sess   mac_non_sess_req;
> +        struct virtio_crypto_aead_data_req_non_sess  aead_non_sess_req;
> +    } u;
> +};
> +\end{lstlisting}
> +
> +The header is the general header and the union is of the algorithm-specific type,
> +which is set by the driver. All properties in the union are shown as follows.
> +
> +There is a unified input header structure for all crypto services.
> +
> +The structure is defined as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_inhdr {
> +    u8 status;
> +};
> +\end{lstlisting}
> +
> +\subsubsection{HASH Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / HASH Service Operation}
> +
> +The session mode packet of HASH service:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_hash_para {
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* hash result length */
> +    le32 hash_result_len;
> +};
> +
> +struct virtio_crypto_hash_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_hash_para 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}
> +
> +Each data request uses virtio_crypto_hash_data_req structure to store information
> +used to run the HASH operations. 
> +
> +The information includes the hash parameters stored by \field{para}, output data and input data.
> +The output data here includes the source data and the input data includes the hash result data used to save the results of the HASH operations.
> +\field{inhdr} stores status of executing the HASH operations.
> +
> +The non-session mode packet of HASH service is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_hash_para_non_session {
> +    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_non_sess {
> +    /* Device-readable part */
> +    struct virtio_crypto_hash_para_non_session 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}
> +
> +\drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
> +
> +\begin{itemize*}
> +\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
> +      to a valid value which assigned by the device when a session is created.
> +\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto packets. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE feature bit is negotiated, 1) if the driver use the non-session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header
> +      to VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE and MUST set fields in struct virtio_crypto_hash_para_non_session.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> +\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_HASH.
> +\end{itemize*}
> +
> +\devicenormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
> +
> +\begin{itemize*}
> +\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto packets. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE feature bit is negotiated, the device MUST parse \field{flag} field in struct virtio_crypto_op_header in order to decide which mode the driver uses.
> +\item The device MUST copy the results of HASH operations to the hash_result[] if HASH operations success.
> +\item The device MUST set \field{status} in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
> +\end{itemize*}
> +
> +\subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / MAC Service Operation}
> +
> +The session mode packet of MAC service is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_mac_para {
> +    struct virtio_crypto_hash_para hash;
> +};
> +
> +struct virtio_crypto_mac_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_mac_para 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}
> +
> +Each data request uses virtio_crypto_mac_data_req structure to store information
> +used to run the MAC operations. 
> +
> +The information includes the hash parameters stored by \field{para}, output data and input data.
> +The output data here includes the source data and the input data includes the hash result data used to save the results of the MAC operations.
> +\field{inhdr} stores status of executing the MAC operations.
> +
> +The non-session mode packet of MAC service:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_mac_para_non_sess {
> +    struct {
> +        /* See VIRTIO_CRYPTO_MAC_* above */
> +        le32 algo;
> +        /* length of authenticated key */
> +        le32 auth_key_len;
> +    } sess_para;
> +
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* hash result length */
> +    le32 hash_result_len;
> +};
> +
> +struct virtio_crypto_mac_data_req_non_sess {
> +    /* Device-readable part */
> +    struct virtio_crypto_mac_para_non_sess para;
> +    /* The authenticated key */
> +    u8 auth_key[auth_key_len];
> +    /* 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}
> +
> +\drivernormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
> +
> +\begin{itemize*}
> +\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
> +      to a valid value which assigned by the device when a session is created.
> +\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto packets. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE feature bit is negotiated, 1) if the driver use the non-session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header
> +      to VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE and MUST set fields in struct virtio_crypto_mac_para_non_session.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> +\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_MAC.
> +\end{itemize*}
> +
> +\devicenormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
> +
> +\begin{itemize*}
> +\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto packets. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE feature bit is negotiated, the device MUST parse \field{flag} field in struct virtio_crypto_op_header in order to decide which mode the driver uses.
> +\item The device MUST copy the results of MAC operations to the hash_result[] if HASH operations success.
> +\item The device MUST set \field{status} in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
> +\end{itemize*}
> +
> +\subsubsection{Symmetric algorithms Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
> +
> +The session mode packet of plain CIPHER service is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_cipher_para {
> +    /*
> +     * Byte Length of valid IV/Counter data pointed to by the below iv data.
> +     *
> +     * For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
> +     *   SNOW3G in UEA2 mode, this is the length of the IV (which
> +     *   must be the same as the block length of the cipher).
> +     * For block ciphers in CTR mode, this is the length of the counter
> +     *   (which must be the same as the block length of the cipher).
> +     */
> +    le32 iv_len;
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* length of destination data */
> +    le32 dst_data_len;
> +    le32 padding;
> +};
> +
> +struct virtio_crypto_cipher_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_cipher_para para;
> +    /*
> +     * Initialization Vector or Counter data.
> +     *
> +     * For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
> +     *   SNOW3G in UEA2 mode, this is the Initialization Vector (IV)
> +     *   value.
> +     * For block ciphers in CTR mode, this is the counter.
> +     * For AES-XTS, this is the 128bit tweak, i, from IEEE Std 1619-2007.
> +     *
> +     * The IV/Counter will be updated after every partial cryptographic
> +     * operation.
> +     */
> +    u8 iv[iv_len];
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +
> +    /* Device-writable part */
> +    /* Destination data */
> +    u8 dst_data[dst_data_len];
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> +The session mode packet of algorithm chaining is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_alg_chain_data_para {
> +    le32 iv_len;
> +    /* Length of source data */
> +    le32 src_data_len;
> +    /* Length of destination data */
> +    le32 dst_data_len;
> +    /* Starting point for cipher processing in source data */
> +    le32 cipher_start_src_offset;
> +    /* Length of the source data that the cipher will be computed on */
> +    le32 len_to_cipher;
> +    /* Starting point for hash processing in source data */
> +    le32 hash_start_src_offset;
> +    /* Length of the source data that the hash will be computed on */
> +    le32 len_to_hash;
> +    /* Length of the additional auth data */
> +    le32 aad_len;
> +    /* Length of the hash result */
> +    le32 hash_result_len;
> +    le32 reserved;
> +};
> +
> +struct virtio_crypto_alg_chain_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_alg_chain_data_para para;
> +    /* Initialization Vector or Counter data */
> +    u8 iv[iv_len];
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +    /* Additional authenticated data if exists  */
> +    u8 aad[aad_len];
> +
> +    /* Device-writable part */
> +    /* Destination data */
> +    u8 dst_data[dst_data_len];
> +    /* Hash result data */
> +    u8 hash_result[hash_result_len];
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> +The session mode packet of symmetric algorithm is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_sym_data_req {
> +    union {
> +        struct virtio_crypto_cipher_data_req cipher;
> +        struct virtio_crypto_alg_chain_data_req chain;
> +    } u;
> +
> +    /* Device-readable part */
> +
> +    /* See above VIRTIO_CRYPTO_SYM_OP_* */
> +    le32 op_type;
> +    le32 padding;
> +};
> +\end{lstlisting}
> +
> +Each data request uses virtio_crypto_sym_data_req structure to store information
> +used to run the CIPHER operations. 
> +
> +The information includes the cipher parameters stored by \field{para}, output data and input data.
> +In the first virtio_crypto_cipher_para structure, \field{iv_len} specifies the length of the initialization vector or counter,
> +\field{src_data_len} specifies the length of the source data, and \field{dst_data_len} specifies the
> +length of the destination data. 
> +For plain CIPHER operations, the output data here includes the IV/Counter data and source data, and the input data includes the destination data used to save the results of the CIPHER operations. 
> +
> +For algorithms chain, the output data here includes the IV/Counter data, source data and additional authenticated data if exists.
> +The input data includes both destination data and hash result data used to store the results of the HASH/MAC operations.
> +\field{inhdr} stores status of executing the crypto operations.
> +
> +The non-session mode packet of plain CIPHER service is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_cipher_para_non_sess {
> +    struct {
> +        /* See VIRTIO_CRYPTO_CIPHER* above */
> +        le32 algo;
> +        /* length of key */
> +        le32 keylen;
> +
> +        /* See VIRTIO_CRYPTO_OP_* above */
> +        le32 op;
> +    } sess_para;
> +
> +    /*
> +     * Byte Length of valid IV/Counter data pointed to by the below iv data.
> +     */
> +    le32 iv_len;
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* length of destination data */
> +    le32 dst_data_len;
> +};
> +
> +struct virtio_crypto_cipher_data_req_non_sess {
> +    /* Device-readable part */
> +    struct virtio_crypto_cipher_para_non_sess para;
> +    /* The cipher key */
> +    u8 cipher_key[keylen];
> +
> +    /* Initialization Vector or Counter data. */
> +    u8 iv[iv_len];
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +
> +    /* Device-writable part */
> +    /* Destination data */
> +    u8 dst_data[dst_data_len];
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> +The non-session mode packet of algorithm chaining is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_alg_chain_data_para_non_sess {
> +    struct {
> +        /* See VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_* above */
> +        le32 alg_chain_order;
> +        /* length of the additional authenticated data in bytes */
> +        le32 aad_len;
> +
> +        struct {
> +            /* See VIRTIO_CRYPTO_CIPHER* above */
> +            le32 algo;
> +            /* length of key */
> +            le32 keylen;
> +            /* See VIRTIO_CRYPTO_OP_* above */
> +            le32 op;
> +        } cipher;
> +
> +        struct {
> +            /* See VIRTIO_CRYPTO_HASH_* or VIRTIO_CRYPTO_MAC_* above */
> +            le32 algo;
> +            /* length of authenticated key */
> +            le32 auth_key_len;
> +            /* See VIRTIO_CRYPTO_SYM_HASH_MODE_* above */
> +            le32 hash_mode;
> +        } hash;
> +    } sess_para;
> +
> +    le32 iv_len;
> +    /* Length of source data */
> +    le32 src_data_len;
> +    /* Length of destination data */
> +    le32 dst_data_len;
> +    /* Starting point for cipher processing in source data */
> +    le32 cipher_start_src_offset;
> +    /* Length of the source data that the cipher will be computed on */
> +    le32 len_to_cipher;
> +    /* Starting point for hash processing in source data */
> +    le32 hash_start_src_offset;
> +    /* Length of the source data that the hash will be computed on */
> +    le32 len_to_hash;
> +    /* Length of the additional auth data */
> +    le32 aad_len;
> +    /* Length of the hash result */
> +    le32 hash_result_len;
> +    le32 reserved;
> +};
> +
> +struct virtio_crypto_alg_chain_data_req_non_sess {
> +    /* Device-readable part */
> +    struct virtio_crypto_alg_chain_data_para_non_sess para;
> +    /* The cipher key */
> +    u8 cipher_key[keylen];
> +    /* The auth key */
> +    u8 auth_key[auth_key_len];
> +    /* Initialization Vector or Counter data */
> +    u8 iv[iv_len];
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +    /* Additional authenticated data if exists  */
> +    u8 aad[aad_len];
> +
> +    /* Device-writable part */
> +    /* Destination data */
> +    u8 dst_data[dst_data_len];
> +    /* Hash result data */
> +    u8 hash_result[hash_result_len];
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> +The non-session mode packet of symmetric algorithm is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_sym_data_req_non_sess {
> +    union {
> +        struct virtio_crypto_cipher_data_req_non_sess cipher;
> +        struct virtio_crypto_alg_chain_data_req_non_sess chain;
> +    } u;
> +
> +    /* Device-readable part */
> +
> +    /* See above VIRTIO_CRYPTO_SYM_OP_* */
> +    le32 op_type;
> +    le32 padding;
> +};
> +\end{lstlisting}
> +
> +\drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
> +
> +\begin{itemize*}
> +\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
> +      to a valid value which assigned by the device when a session is created.
> +\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto packets. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is negotiated, 1) if the driver use the non-session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header
> +      to VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE and MUST set fields in struct virtio_crypto_cipher_para_non_session.sess_para or struct virtio_crypto_alg_chain_data_para_non_sess.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> +\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_CIPHER_ENCRYPT or VIRTIO_CRYPTO_CIPHER_DECRYPT.
> +\item The driver MUST specify the fields of struct virtio_crypto_cipher_data_req in struct virtio_crypto_sym_data_req if the packet is based on VIRTIO_CRYPTO_SYM_OP_CIPHER.
> +\item The driver MUST specify the fields of both struct virtio_crypto_cipher_data_req and struct virtio_crypto_mac_data_req in struct virtio_crypto_sym_data_req if the packet
> +      is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type and in the VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH mode.
> +\end{itemize*}
> +
> +\devicenormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
> +
> +\begin{itemize*}
> +\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto packets. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is negotiated, the device MUST parse \field{flag} field in struct virtio_crypto_op_header in order to decide which mode the driver uses.
> +\item The device MUST parse the virtio_crypto_sym_data_req based on the \field{opcode} in general header.
> +\item The device SHOULD only parse fields of struct virtio_crypto_cipher_data_req in struct virtio_crypto_sym_data_req if the packet is VIRTIO_CRYPTO_SYM_OP_CIPHER type.
> +\item The device MUST parse fields of both struct virtio_crypto_cipher_data_req and struct virtio_crypto_mac_data_req in struct virtio_crypto_sym_data_req if the packet
> +      is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING operation type and in the VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH mode.
> +\item The device MUST copy the result of cryptographic operation to the dst_data[] in both plain CIPHER mode and algorithms chain mode.
> +\item The device MUST check the \field{para}.\field{add_len} is bigger than 0 before parse the additional authenticated data in plain algorithms chain mode.
> +\item The device MUST copy the result of HASH/MAC operation to the hash_result[] is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type.
> +\item The device MUST set the \field{status} field in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
> +\end{itemize*}
> +
> +\paragraph{Steps of Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation / Steps of Operation}
> +
> +\subparagraph{Step1: Create session}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation / Steps of Operation / Step1: Create session on session mode}
> +
> +\begin{enumerate}
> +\item The driver specifies information in struct virtio_crypto_op_ctrl_req, including the algorithm name, key, keylen etc;
> +\item The driver adds the request of session creation into the controlq's Vring Descriptor Table;
> +\item The driver kicks the device;
> +\item The device receives the request from controlq;
> +\item The device parses information about the request, and determines the information concerning the backend crypto accelerator;
> +\item The device packs information based on the APIs of the backend crypto accelerator;
> +\item The device invokes the session creation APIs of the backend crypto accelerator to create a session;
> +\item The device returns the session id to the driver.
> +\end{enumerate}
> +
> +\subparagraph{Step2: Execute cryptographic operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation / Steps of Operation / Step2: Execute cryptographic operation}
> +
> +\begin{enumerate}
> +\item The driver specifies information in struct virtio_crypto_op_data_req, including struct virtio_crypto_op_header and struct virtio_crypto_sym_data_req, see \ref{sec:Device Types / Crypto Device / Device
> +      Operation / Symmetric algorithms Operation};
> +\item The driver adds the request for cryptographic operation into the dataq's Vring Descriptor Table;
> +\item The driver kicks the device (Or the device actively polls the dataq's Vring Descriptor Table);
> +\item The device receives the request from dataq;
> +\item The device parses information about the request, and determines the identification information for the backend crypto accelerator. For example, converting guest physical addresses to host physical addresses;
> +\item The device packs identification information based on the API of the backend crypto accelerator;
> +\item The device invokes the cryptographic APIs of the backend crypto accelerator;
> +\item The backend crypto accelerator executes the cryptographic operation implicitly;
> +\item The device receives the cryptographic results from the backend crypto accelerator (synchronous or asynchronous);
> +\item The device sets the \field{status} in struct virtio_crypto_inhdr;
> +\item The device updates and flushes the Used Ring to return the cryptographic results to the driver;
> +\item The device notifies the driver (Or the driver actively polls the dataq's Used Ring);
> +\item The driver saves the cryptographic results.
> +\end{enumerate}
> +
> +\begin{note}
> +\begin{itemize*}
> +\item For better performance, the device should by preference use vhost scheme (user space or kernel space)
> +      as the backend crypto accelerator in the real production environment.
> +\end{itemize*}
> +\end{note}
> +
> +\subsubsection{AEAD Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / AEAD Service Operation}
> +
> +The session mode packet of symmetric algorithm is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_aead_para {
> +    /*
> +     * Byte Length of valid IV data.
> +     *
> +     * For GCM mode, this is either 12 (for 96-bit IVs) or 16, in which
> +     *   case iv points to J0.
> +     * For CCM mode, this is the length of the nonce, which can be in the
> +     *   range 7 to 13 inclusive.
> +     */
> +    le32 iv_len;
> +    /* length of additional auth data */
> +    le32 aad_len;
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* length of dst data, this should be at least src_data_len + tag_len */
> +    le32 dst_data_len;
> +    /* Authentication tag length */
> +    le32 tag_len;
> +    le32 reserved;
> +};
> +
> +struct virtio_crypto_aead_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_aead_para para;
> +    /*
> +     * Initialization Vector data.
> +     *
> +     * For GCM mode, this is either the IV (if the length is 96 bits) or J0
> +     *   (for other sizes), where J0 is as defined by NIST SP800-38D.
> +     *   Regardless of the IV length, a full 16 bytes needs to be allocated.
> +     * For CCM mode, the first byte is reserved, and the nonce should be
> +     *   written starting at &iv[1] (to allow space for the implementation
> +     *   to write in the flags in the first byte).  Note that a full 16 bytes
> +     *   should be allocated, even though the iv_len field will have
> +     *   a value less than this.
> +     *
> +     * The IV will be updated after every partial cryptographic operation.
> +     */
> +    u8 iv[iv_len];
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +    /* Additional authenticated data if exists  */
> +    u8 aad[aad_len];
> +
> +    /* Device-writable part */
> +    /* Pointer to output data */
> +    u8 dst_data[dst_data_len];
> +
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> +Each data request uses virtio_crypto_aead_data_req structure to store information
> +used to run the AEAD operations. 
> +
> +The information includes the hash parameters stored by \field{para}, output data and input data.
> +In the first virtio_crypto_aead_para structure, \field{iv_len} specifies the length of the initialization vector. \field{tag_len} specifies the length of the authentication tag;
> +\field{aad_len} specifies the length of additional authentication data, \field{src_data_len} specifies the
> +length of the source data; \field{dst_data_len} specifies the length of the destination data, which is at least \field{src_data_len} + \field{tag_len}.
> +
> +The output data here includes the IV/Counter data, source data and additional authenticated data if exists.
> +The input data includes both destination data used to save the results of the AEAD operations.
> +\field{inhdr} stores status of executing the AEAD operations.
> +
> +The non-session mode packet of AEAD service is as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_aead_para_non_sess {
> +    struct {
> +        /* See VIRTIO_CRYPTO_AEAD_* above */
> +        le32 algo;
> +        /* length of key */
> +        le32 key_len;
> +        /* encrypt or decrypt, See above VIRTIO_CRYPTO_OP_* */
> +        le32 op;
> +    } sess_para;
> +
> +    /* Byte Length of valid IV data. */
> +    le32 iv_len;
> +    /* Authentication tag length */
> +    le32 tag_len;
> +    /* length of additional auth data */
> +    le32 aad_len;
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* length of dst data, this should be at least src_data_len + tag_len */
> +    le32 dst_data_len;
> +};
> +
> +struct virtio_crypto_aead_data_req_non_sess {
> +    /* Device-readable part */
> +    struct virtio_crypto_aead_para_non_sess para;
> +    /* The cipher key */
> +    u8 key[key_len];
> +    /* Initialization Vector data. */
> +    u8 iv[iv_len];
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +    /* Additional authenticated data if exists  */
> +    u8 aad[aad_len];
> +
> +    /* Device-writable part */
> +    /* Pointer to output data */
> +    u8 dst_data[dst_data_len];
> +
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> +\drivernormative{\paragraph}{AEAD Service Operation}{Device Types / Crypto Device / Device Operation / AEAD Service Operation}
> +
> +\begin{itemize*}
> +\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
> +      to a valid value which assigned by the device when a session is created.
> +\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto packets. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE feature bit is negotiated, 1) if the driver use the non-session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header
> +      to VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE and MUST set fields in struct virtio_crypto_aead_para_non_session.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> +\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_AEAD_ENCRYPT or VIRTIO_CRYPTO_AEAD_DECRYPT.
> +\end{itemize*}
> +
> +\devicenormative{\paragraph}{AEAD Service Operation}{Device Types / Crypto Device / Device Operation / AEAD Service Operation}
> +
> +\begin{itemize*}
> +\item If the VIRTIO_CRYPTO_F_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto packets. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE feature bit is negotiated, the device MUST parse the virtio_crypto_aead_data_req based on the \field{opcode} in general header.
> +\item The device MUST copy the result of cryptographic operation to the dst_data[].
> +\item The device MUST copy the authentication tag to the dst_data[] offset the cipher result.
> +\item The device MUST set the \field{status} field in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
> +\item When the \field{opcode} is VIRTIO_CRYPTO_AEAD_DECRYPT, the device MUST verify and return the verification result to the driver, and if the verification result is incorrect, VIRTIO_CRYPTO_BADMSG (bad message) MUST be returned to the driver.
> +\end{itemize*}
> \ No newline at end of file
> -- 
> 1.7.12.4
> 
> 
> 

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

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Michael S. Tsirkin]

On Fri, Feb 03, 2017 at 10:33:16AM +0000, Stefan Hajnoczi wrote:
> > +\begin{description}
> > +\item[0] dataq1
> > +\item[\ldots]
> > +\item[N-1] dataqN
> > +\item[N] controlq
> > +\end{description}
> > +
> > +N is set by \field{max_dataqueues}.
> > +
> > +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature bits}
> > +
> > +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
> > +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for CIPHER service.
> > +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for HASH service.
> > +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC service.
> > +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for AEAD service.
> 
> Bits 1-4 require bit 0.  Is bit 0 necessary at all?  Or may bits 1-4 can
> be eliminated in favor of just bit 0.

Too late to change it as QEMU released a device with that feature.

-- 
MST

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 02/03/2017 04:37 PM, Michael S. Tsirkin wrote:
> On Fri, Feb 03, 2017 at 10:33:16AM +0000, Stefan Hajnoczi wrote:
>>> +\begin{description}
>>> +\item[0] dataq1
>>> +\item[\ldots]
>>> +\item[N-1] dataqN
>>> +\item[N] controlq
>>> +\end{description}
>>> +
>>> +N is set by \field{max_dataqueues}.
>>> +
>>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature bits}
>>> +
>>> +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
>>> +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for CIPHER service.
>>> +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for HASH service.
>>> +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC service.
>>> +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for AEAD service.
>>
>> Bits 1-4 require bit 0.  Is bit 0 necessary at all?  Or may bits 1-4 can
>> be eliminated in favor of just bit 0.
> 
> Too late to change it as QEMU released a device with that feature.
> 

When was this introduced to QEMU? Can't find it in current master
and I do not remember seeing the patches.

Regards,
Halil

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Stefan Hajnoczi]

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

On Fri, Feb 03, 2017 at 07:07:02PM +0100, Halil Pasic wrote:
> 
> 
> On 02/03/2017 04:37 PM, Michael S. Tsirkin wrote:
> > On Fri, Feb 03, 2017 at 10:33:16AM +0000, Stefan Hajnoczi wrote:
> >>> +\begin{description}
> >>> +\item[0] dataq1
> >>> +\item[\ldots]
> >>> +\item[N-1] dataqN
> >>> +\item[N] controlq
> >>> +\end{description}
> >>> +
> >>> +N is set by \field{max_dataqueues}.
> >>> +
> >>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature bits}
> >>> +
> >>> +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
> >>> +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for CIPHER service.
> >>> +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for HASH service.
> >>> +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC service.
> >>> +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for AEAD service.
> >>
> >> Bits 1-4 require bit 0.  Is bit 0 necessary at all?  Or may bits 1-4 can
> >> be eliminated in favor of just bit 0.
> > 
> > Too late to change it as QEMU released a device with that feature.
> > 
> 
> When was this introduced to QEMU? Can't find it in current master
> and I do not remember seeing the patches.

s/QEMU/Linux/

It's in drivers/crypto/virtio/.

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

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

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



On 02/13/2017 02:41 PM, Stefan Hajnoczi wrote:
> On Fri, Feb 03, 2017 at 07:07:02PM +0100, Halil Pasic wrote:
>>
>>
>> On 02/03/2017 04:37 PM, Michael S. Tsirkin wrote:
>>> On Fri, Feb 03, 2017 at 10:33:16AM +0000, Stefan Hajnoczi wrote:
>>>>> +\begin{description}
>>>>> +\item[0] dataq1
>>>>> +\item[\ldots]
>>>>> +\item[N-1] dataqN
>>>>> +\item[N] controlq
>>>>> +\end{description}
>>>>> +
>>>>> +N is set by \field{max_dataqueues}.
>>>>> +
>>>>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature bits}
>>>>> +
>>>>> +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
>>>>> +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for CIPHER service.
>>>>> +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for HASH service.
>>>>> +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC service.
>>>>> +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for AEAD service.
>>>>
>>>> Bits 1-4 require bit 0.  Is bit 0 necessary at all?  Or may bits 1-4 can
>>>> be eliminated in favor of just bit 0.
>>>
>>> Too late to change it as QEMU released a device with that feature.
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>
>> When was this introduced to QEMU? Can't find it in current master
>> and I do not remember seeing the patches.
> 
> s/QEMU/Linux/
> 
> It's in drivers/crypto/virtio/.
> 


Thanks! Sorry but I could not find the NON_SESSION_MODE in Linus master
(Linux) either, and Michael has stated that a QEMU device was released
with these. I'm still confused. But if it's only Linux, it most likely is
not interface, so theoretically should not break anything if we change it
-- should we want to change it.

I think your original questions regarding these feature bits are valid
(and have not been addressed in a satisfactory way yet) by the way.

Halil






[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

Hi Stefan,

I'm just back from Chinese New year's holiday.


>
> Subject: Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto
> device specification
> 
> On Wed, Jan 18, 2017 at 04:22:36PM +0800, Gonglei wrote:
> > The virtio crypto device is a virtual crypto device (ie. hardware
> > crypto accelerator card). Currently, the virtio crypto device provides
> > the following crypto services: CIPHER, MAC, HASH, and AEAD.
> >
> > In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
> >
> > VIRTIO-153
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Cornelia Huck <cornelia.huck@de.ibm.com>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> > CC: Lingli Deng <denglingli@chinamobile.com>
> > CC: Jani Kokkonen <Jani.Kokkonen@huawei.com>
> > CC: Ola Liljedahl <Ola.Liljedahl@arm.com>
> > CC: Varun Sethi <Varun.Sethi@freescale.com>
> > CC: Zeng Xin <xin.zeng@intel.com>
> > CC: Keating Brian <brian.a.keating@intel.com>
> > CC: Ma Liang J <liang.j.ma@intel.com>
> > CC: Griffin John <john.griffin@intel.com>
> > CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> > ---
> >  content.tex       |    2 +
> >  virtio-crypto.tex | 1245
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 1247 insertions(+)
> >  create mode 100644 virtio-crypto.tex
> 
> I'm happy with the device overall.  See specific comments below.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
Thanks!

> >
> > diff --git a/content.tex b/content.tex
> > index 4b45678..ab75f78 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len},
> \field{residual},
> >  \field{status_qualifier}, \field{status}, \field{response} and
> >  \field{sense} fields.
> >
> > +\input{virtio-crypto.tex}
> > +
> >  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >
> >  Currently there are three device-independent feature bits defined:
> > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> > new file mode 100644
> > index 0000000..732cd30
> > --- /dev/null
> > +++ b/virtio-crypto.tex
> > @@ -0,0 +1,1245 @@
> > +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> > +
> > +The virtio crypto device is a virtual cryptography device as well as a kind of
> > +virtual hardware accelerator for virtual machines. The encryption and
> > +decryption requests are placed in any of the data active queues and are
> ultimately handled by the
> > +backend crypto accelerators. The second kind of queue is the control queue
> used to create
> > +or destroy sessions for symmetric algorithms and will control some
> advanced
> > +features in the future. The virtio crypto device provides the following crypto
> > +services: CIPHER, MAC, HASH, and AEAD.
> > +
> > +
> > +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
> > +
> > +20
> > +
> > +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device /
> Virtqueues}
> > +
> > +\begin{description}
> > +\item[0] dataq1
> > +\item[\ldots]
> > +\item[N-1] dataqN
> > +\item[N] controlq
> > +\end{description}
> > +
> > +N is set by \field{max_dataqueues}.
> > +
> > +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature
> bits}
> > +
> > +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
> > +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is
> available for CIPHER service.
> > +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is
> available for HASH service.
> > +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is
> available for MAC service.
> > +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is
> available for AEAD service.
> 
> Bits 1-4 require bit 0.  Is bit 0 necessary at all?  Or may bits 1-4 can
> be eliminated in favor of just bit 0.
> 
I tried to explain why those bits are necessary in below thread: 

https://www.mail-archive.com/qemu-devel@nongnu.org/msg423072.html


> > +
> > +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto
> Device / Feature bits}
> > +
> > +Some crypto feature bits require other crypto feature bits
> > +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}):
> > +
> > +\begin{description}
> > +\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires
> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> > +\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires
> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> > +\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires
> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> > +\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires
> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> > +\end{description}
> > +
> > +\subsection{Device configuration layout}\label{sec:Device Types / Crypto
> Device / Device configuration layout}
> > +
> > +The following driver-read-only configuration fields are defined:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_config {
> > +    le32 status;
> > +    le32 max_dataqueues;
> > +    le32 crypto_services;
> > +    /* Detailed algorithms mask */
> > +    le32 cipher_algo_l;
> > +    le32 cipher_algo_h;
> > +    le32 hash_algo;
> > +    le32 mac_algo_l;
> > +    le32 mac_algo_h;
> > +    le32 aead_algo;
> > +    /* Maximum length of cipher key */
> > +    le32 max_cipher_key_len;
> > +    /* Maximum length of authenticated key */
> > +    le32 max_auth_key_len;
> > +    le32 reserved;
> > +    /* Maximum size of each crypto request's content */
> > +    le64 max_size;
> > +};
> > +\end{lstlisting}
> > +
> > +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
> ~VIRTIO_CRYPTO_S_HW_READY.
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
> > +\end{lstlisting}
> > +
> > +The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the
> hardware is ready to work or not.
> > +
> > +The following driver-read-only fields include \field{max_dataqueues}, which
> specifies the
> > +maximum number of data virtqueues (dataq1\ldots dataqN), and
> \field{crypto_services},
> > +which indicates the crypto services the virtio crypto supports.
> > +
> > +The following services are defined:
> > +
> > +\begin{lstlisting}
> > +/* CIPHER service */
> > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> > +/* HASH service */
> > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> > +/* MAC (Message Authentication Codes) service */
> > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> > +/* AEAD (Authenticated Encryption with Associated Data) service */
> > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> > +\end{lstlisting}
> > +
> > +The last driver-read-only fields specify detailed algorithms masks
> 
> s/The last driver-read-only fields/Further driver-read-only fields/
> 
> (cipher_algo_l, cipher_algo_h, etc are not the last fields.)
> 
Yes, good insight.

> > +the device offers for corresponding services. The following CIPHER
> algorithms
> > +are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> > +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> > +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> > +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> > +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> > +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> > +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> > +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> > +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> > +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> > +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> > +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> > +\end{lstlisting}
> > +
> > +The following HASH algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_HASH            0
> > +#define VIRTIO_CRYPTO_HASH_MD5           1
> > +#define VIRTIO_CRYPTO_HASH_SHA1          2
> > +#define VIRTIO_CRYPTO_HASH_SHA_224       3
> > +#define VIRTIO_CRYPTO_HASH_SHA_256       4
> > +#define VIRTIO_CRYPTO_HASH_SHA_384       5
> > +#define VIRTIO_CRYPTO_HASH_SHA_512       6
> > +#define VIRTIO_CRYPTO_HASH_SHA3_224      7
> > +#define VIRTIO_CRYPTO_HASH_SHA3_256      8
> > +#define VIRTIO_CRYPTO_HASH_SHA3_384      9
> > +#define VIRTIO_CRYPTO_HASH_SHA3_512      10
> > +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128      11
> > +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256      12
> > +\end{lstlisting}
> > +
> > +The following MAC algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_MAC                       0
> > +#define VIRTIO_CRYPTO_MAC_HMAC_MD5                 1
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1                2
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224             3
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256             4
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384             5
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512             6
> > +#define VIRTIO_CRYPTO_MAC_CMAC_3DES                25
> > +#define VIRTIO_CRYPTO_MAC_CMAC_AES                 26
> > +#define VIRTIO_CRYPTO_MAC_KASUMI_F9                27
> > +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2              28
> > +#define VIRTIO_CRYPTO_MAC_GMAC_AES                 41
> > +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             42
> > +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               49
> > +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         50
> > +#define VIRTIO_CRYPTO_MAC_XCBC_AES                 53
> > +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3                 54
> > +\end{lstlisting}
> > +
> > +The following AEAD algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_AEAD     0
> > +#define VIRTIO_CRYPTO_AEAD_GCM    1
> > +#define VIRTIO_CRYPTO_AEAD_CCM    2
> > +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
> > +\end{lstlisting}
> > +
> > +\begin{note}
> > +Any other values are reserved for future use.
> > +\end{note}
> > +
> > +\devicenormative{\subsubsection}{Device configuration layout}{Device Types
> / Crypto Device / Device configuration layout}
> > +
> > +\begin{itemize*}
> > +\item The device MUST set \field{max_dataqueues} to between 1 and 65535
> inclusive.
> > +\item The device MUST set \field{status} based on the status of the
> hardware-backed implementation.
> > +\item The device MUST accept and handle requests after \field{status} is set
> to VIRTIO_CRYPTO_S_HW_READY.
> > +\item The device MUST set \field{crypto_services} based on the crypto
> services the device offers.
> > +\item The device MUST set detailed algorithms masks based on the
> \field{crypto_services} field.
> > +\item The device MUST set \field{max_size} to show the maximum size of
> crypto request the device supports.
> 
> In bytes?
> 
> > +\item The device MUST set \field{max_cipher_key_len} to show the
> maximum length of cipher key if the device supports CIPHER service.
> 
> In bits or bytes?
> 
> > +\item The device MUST set \field{max_auth_key_len} to show the maximum
> length of authenticated key if the device supports MAC service.
> 
> In bits or bytes?
> 

All lengths in virtio crypto spec are bytes.

> > +\end{itemize*}
> > +
> > +\drivernormative{\subsubsection}{Device configuration layout}{Device Types
> / Crypto Device / Device configuration layout}
> > +
> > +\begin{itemize*}
> > +\item The driver MUST read the ready \field{status} from the bottom bit of
> status to check whether the hardware-backed
> > +      implementation is ready or not, and the driver MUST reread it after
> the device reset.
> > +\item The driver MUST NOT transmit any packets to the device if the ready
> \field{status} is not set.
> > +\item The driver MUST read \field{max_dataqueues} field to discover the
> number of data queues the device supports.
> > +\item The driver MUST read \field{crypto_services} field to discover which
> services the device is able to offer.
> > +\item The driver MUST read the detailed algorithms fields based on
> \field{crypto_services} field.
> > +\item The driver SHOULD read \field{max_size} to discover the maximum
> size of crypto request the device supports.
> > +\item The driver SHOULD read \field{max_cipher_key_len} to discover the
> maximum length of cipher key the device supports.
> > +\item The driver SHOULD read \field{max_auth_key_len} to discover the
> maximum length of authenticated key the device supports.
> > +\end{itemize*}
> > +
> > +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device /
> Device Initialization}
> > +
> > +\drivernormative{\subsubsection}{Device Initialization}{Device Types /
> Crypto Device / Device Initialization}
> > +
> > +\begin{itemize*}
> > +\item The driver MUST identify and initialize the control virtqueue.
> > +\item The driver MUST read the supported crypto services from bits of
> \field{crypto_services}.
> > +\item The driver MUST read the supported algorithms based on
> \field{crypto_services} field.
> > +\end{itemize*}
> > +
> > +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
> Crypto Device / Device Initialization}
> > +
> > +\begin{itemize*}
> > +\item The device MUST be configured with at least one accelerator which
> executes backend crypto operations.
> > +\item The device MUST write the \field{crypto_services} field based on the
> capacities of the backend accelerator.
> > +\end{itemize*}
> > +
> > +\subsection{Device Operation}\label{sec:Device Types / Crypto Device /
> Device Operation}
> > +
> > +Packets can be transmitted by placing them in both the controlq and dataq.
> > +Packets consist of a general header and a service-specific request.
> > +Where 'general header' is for all crypto requests, and 'service specific
> requests'
> > +are composed of operation parameter + output data + input data in general.
> > +Operation parameters are algorithm-specific parameters, output data is the
> > +data that should be utilized in operations, and input data is equal to
> > +"operation result + result data".
> > +
> > +The device can support both session mode (See \ref{sec:Device Types /
> Crypto Device / Device Operation / Control Virtqueue / Session operation}) and
> non-session mode, for example,
> > +As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is
> negotiated, the driver can use non-session mode for CIPHER service, otherwise
> it can only use session mode.
> > +
> > +\begin{note}
> > +The basic unit of all data length the byte.
> 
> s/data length the byte/data length is the byte/
> 
Yes.

> > +\end{note}
> > +
> > +The general header for controlq is as follows:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
> > +
> > +struct virtio_crypto_ctrl_header {
> > +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> 0x02)
> > +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> 0x03)
> > +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
> > +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
> > +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
> > +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
> > +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
> > +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
> > +    le32 opcode;
> > +    le32 algo;
> > +    le32 flag;
> > +    /* data virtqueue id */
> > +    le32 queue_id;
> > +};
> > +\end{lstlisting}
> > +
> > +The general header of dataq:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_op_header {
> > +#define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
> > +#define VIRTIO_CRYPTO_CIPHER_DECRYPT \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
> > +#define VIRTIO_CRYPTO_HASH \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
> > +#define VIRTIO_CRYPTO_MAC \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
> > +#define VIRTIO_CRYPTO_AEAD_ENCRYPT \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> > +#define VIRTIO_CRYPTO_AEAD_DECRYPT \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> > +    le32 opcode;
> > +    /* algo should be service-specific algorithms */
> > +    le32 algo;
> > +    /* session_id should be service-specific algorithms */
> > +    le64 session_id;
> > +#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
> > +#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2
> > +    /* control flag to control the request */
> > +    le32 flag;
> > +    le32 padding;
> > +};
> > +\end{lstlisting}
> > +
> > +The device can set the operation status as follows: VIRTIO_CRYPTO_OK:
> success;
> > +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP:
> not supported;
> > +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto
> operations.
> > +
> > +\begin{lstlisting}
> > +enum VIRITO_CRYPTO_STATUS {
> > +    VIRTIO_CRYPTO_OK = 0,
> > +    VIRTIO_CRYPTO_ERR = 1,
> > +    VIRTIO_CRYPTO_BADMSG = 2,
> > +    VIRTIO_CRYPTO_NOTSUPP = 3,
> > +    VIRTIO_CRYPTO_INVSESS = 4,
> > +    VIRTIO_CRYPTO_MAX
> > +};
> > +\end{lstlisting}
> > +
> > +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device /
> Device Operation / Control Virtqueue}
> > +
> > +The driver uses the control virtqueue to send control commands to the
> > +device which handles the non-data plane operations, such as session
> > +operations (See \ref{sec:Device Types / Crypto Device / Device Operation /
> Control Virtqueue / Session operation}).
> > +
> > +The packet of controlq:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_op_ctrl_req {
> > +    struct virtio_crypto_ctrl_header header;
> > +
> > +    union {
> > +        struct virtio_crypto_sym_create_session_req
> sym_create_session;
> > +        struct virtio_crypto_hash_create_session_req
> hash_create_session;
> > +        struct virtio_crypto_mac_create_session_req
> mac_create_session;
> > +        struct virtio_crypto_aead_create_session_req
> aead_create_session;
> > +        struct virtio_crypto_destroy_session_req      destroy_session;
> > +    } u;
> > +};
> > +\end{lstlisting}
> > +
> > +The header is the general header, and the union is of the algorithm-specific
> type,
> > +which is set by the driver. All the properties in the union are shown as
> follows.
> 
> The spec isn't clear on whether crtl reqs should be sizeof(struct
> virtio_crypto_op_ctrl_req) or if it's okay to use the smaller
> sizeof(struct virtio_crypto_ctrl_header) + sizeof(struct
> virtio_crypto_sym_create_session_req), for example.
> 
> Please clarify.
> 
It MUST be sizeof(struct virtio_crypto_op_ctrl_req).
I'll clarify it.

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Stefan Hajnoczi]

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

On Mon, Feb 06, 2017 at 01:48:09AM +0000, Gonglei (Arei) wrote:
> > > +\item The device MUST set \field{max_size} to show the maximum size of
> > crypto request the device supports.
> > 
> > In bytes?
> > 
> > > +\item The device MUST set \field{max_cipher_key_len} to show the
> > maximum length of cipher key if the device supports CIPHER service.
> > 
> > In bits or bytes?
> > 
> > > +\item The device MUST set \field{max_auth_key_len} to show the maximum
> > length of authenticated key if the device supports MAC service.
> > 
> > In bits or bytes?
> > 
> 
> All lengths in virtio crypto spec are bytes.

Please move this before the first length field is defined:

  +\begin{note}
  +The basic unit of all data length the byte.
  +\end{note}

It should be at the beginning of the spec.

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

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Michael S. Tsirkin]

On Mon, Feb 06, 2017 at 03:46:25PM +0000, Stefan Hajnoczi wrote:
> On Mon, Feb 06, 2017 at 01:48:09AM +0000, Gonglei (Arei) wrote:
> > > > +\item The device MUST set \field{max_size} to show the maximum size of
> > > crypto request the device supports.
> > > 
> > > In bytes?
> > > 
> > > > +\item The device MUST set \field{max_cipher_key_len} to show the
> > > maximum length of cipher key if the device supports CIPHER service.
> > > 
> > > In bits or bytes?
> > > 
> > > > +\item The device MUST set \field{max_auth_key_len} to show the maximum
> > > length of authenticated key if the device supports MAC service.
> > > 
> > > In bits or bytes?
> > > 
> > 
> > All lengths in virtio crypto spec are bytes.
> 
> Please move this before the first length field is defined:
> 
>   +\begin{note}
>   +The basic unit of all data length the byte.
>   +\end{note}

And I'd rewrite as

   All data length fields specify the length in bytes.

> 
> It should be at the beginning of the spec.

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

Hi,

> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, February 07, 2017 2:20 AM
> Subject: Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto
> device specification
> 
> On Mon, Feb 06, 2017 at 03:46:25PM +0000, Stefan Hajnoczi wrote:
> > On Mon, Feb 06, 2017 at 01:48:09AM +0000, Gonglei (Arei) wrote:
> > > > > +\item The device MUST set \field{max_size} to show the maximum size
> of
> > > > crypto request the device supports.
> > > >
> > > > In bytes?
> > > >
> > > > > +\item The device MUST set \field{max_cipher_key_len} to show the
> > > > maximum length of cipher key if the device supports CIPHER service.
> > > >
> > > > In bits or bytes?
> > > >
> > > > > +\item The device MUST set \field{max_auth_key_len} to show the
> maximum
> > > > length of authenticated key if the device supports MAC service.
> > > >
> > > > In bits or bytes?
> > > >
> > >
> > > All lengths in virtio crypto spec are bytes.
> >
> > Please move this before the first length field is defined:
> >
> >   +\begin{note}
> >   +The basic unit of all data length the byte.
> >   +\end{note}
> 
> And I'd rewrite as
> 
>    All data length fields specify the length in bytes.
> 

Okay, will do it.

BTW if no other objections or comments, 
I'd like to send v17 as the final version to ballot, can I?

Thanks,
-Gonglei

> >
> > It should be at the beginning of the spec.
> 

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 02/07/2017 01:59 AM, Gonglei (Arei) wrote:
> Hi,
> 
>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
>> Sent: Tuesday, February 07, 2017 2:20 AM
>> Subject: Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto
>> device specification
>>
>> On Mon, Feb 06, 2017 at 03:46:25PM +0000, Stefan Hajnoczi wrote:
>>> On Mon, Feb 06, 2017 at 01:48:09AM +0000, Gonglei (Arei) wrote:
>>>>>> +\item The device MUST set \field{max_size} to show the maximum size
>> of
>>>>> crypto request the device supports.
>>>>>
>>>>> In bytes?
>>>>>
>>>>>> +\item The device MUST set \field{max_cipher_key_len} to show the
>>>>> maximum length of cipher key if the device supports CIPHER service.
>>>>>
>>>>> In bits or bytes?
>>>>>
>>>>>> +\item The device MUST set \field{max_auth_key_len} to show the
>> maximum
>>>>> length of authenticated key if the device supports MAC service.
>>>>>
>>>>> In bits or bytes?
>>>>>
>>>>
>>>> All lengths in virtio crypto spec are bytes.
>>>
>>> Please move this before the first length field is defined:
>>>
>>>   +\begin{note}
>>>   +The basic unit of all data length the byte.
>>>   +\end{note}
>>
>> And I'd rewrite as
>>
>>    All data length fields specify the length in bytes.
>>
> 
> Okay, will do it.
> 
> BTW if no other objections or comments, 
> I'd like to send v17 as the final version to ballot, can I?
> 
> Thanks,
> -Gonglei
> 

I think it's a bit early for a 'final version' and a 'ballot'.
We need at least one more iteration (IMHO).

Halil

>>>
>>> It should be at the beginning of the spec.
>>
> 
> 

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

> 
> On 02/07/2017 01:59 AM, Gonglei (Arei) wrote:
> > Hi,
> >
> >> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> >> Sent: Tuesday, February 07, 2017 2:20 AM
> >> Subject: Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto
> >> device specification
> >>
> >> On Mon, Feb 06, 2017 at 03:46:25PM +0000, Stefan Hajnoczi wrote:
> >>> On Mon, Feb 06, 2017 at 01:48:09AM +0000, Gonglei (Arei) wrote:
> >>>>>> +\item The device MUST set \field{max_size} to show the maximum
> size
> >> of
> >>>>> crypto request the device supports.
> >>>>>
> >>>>> In bytes?
> >>>>>
> >>>>>> +\item The device MUST set \field{max_cipher_key_len} to show the
> >>>>> maximum length of cipher key if the device supports CIPHER service.
> >>>>>
> >>>>> In bits or bytes?
> >>>>>
> >>>>>> +\item The device MUST set \field{max_auth_key_len} to show the
> >> maximum
> >>>>> length of authenticated key if the device supports MAC service.
> >>>>>
> >>>>> In bits or bytes?
> >>>>>
> >>>>
> >>>> All lengths in virtio crypto spec are bytes.
> >>>
> >>> Please move this before the first length field is defined:
> >>>
> >>>   +\begin{note}
> >>>   +The basic unit of all data length the byte.
> >>>   +\end{note}
> >>
> >> And I'd rewrite as
> >>
> >>    All data length fields specify the length in bytes.
> >>
> >
> > Okay, will do it.
> >
> > BTW if no other objections or comments,
> > I'd like to send v17 as the final version to ballot, can I?
> >
> > Thanks,
> > -Gonglei
> >
> 
> I think it's a bit early for a 'final version' and a 'ballot'.
> We need at least one more iteration (IMHO).
> 
> Halil
> 
Sure. Many comments should be addressed. 

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 02/06/2017 07:19 PM, Michael S. Tsirkin wrote:
> On Mon, Feb 06, 2017 at 03:46:25PM +0000, Stefan Hajnoczi wrote:
>> On Mon, Feb 06, 2017 at 01:48:09AM +0000, Gonglei (Arei) wrote:
>>>>> +\item The device MUST set \field{max_size} to show the maximum size of
>>>> crypto request the device supports.
>>>>
>>>> In bytes?
>>>>
>>>>> +\item The device MUST set \field{max_cipher_key_len} to show the
>>>> maximum length of cipher key if the device supports CIPHER service.
>>>>
>>>> In bits or bytes?
>>>>
>>>>> +\item The device MUST set \field{max_auth_key_len} to show the maximum
>>>> length of authenticated key if the device supports MAC service.
>>>>
>>>> In bits or bytes?
>>>>
>>>
>>> All lengths in virtio crypto spec are bytes.
>>
>> Please move this before the first length field is defined:
>>
>>   +\begin{note}
>>   +The basic unit of all data length the byte.
>>   +\end{note}
> 
> And I'd rewrite as
> 
>    All data length fields specify the length in bytes.

I clearly prefer this to the 'basic unit' formulation.

We could also do something like

"Unless explicitly stated otherwise all lengths and sizes are in bytes."

but I'm quite happy with your formulation too.

Regards,
Halil

> 
>>
>> It should be at the beginning of the spec.
> 
> 
> 

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 01/18/2017 09:22 AM, Gonglei wrote:
> The virtio crypto device is a virtual crypto device (ie. hardware
> crypto accelerator card). Currently, the virtio crypto device provides
> the following crypto services: CIPHER, MAC, HASH, and AEAD.
> 
> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
> 
> VIRTIO-153
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Cornelia Huck <cornelia.huck@de.ibm.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lingli Deng <denglingli@chinamobile.com>
> CC: Jani Kokkonen <Jani.Kokkonen@huawei.com>
> CC: Ola Liljedahl <Ola.Liljedahl@arm.com>
> CC: Varun Sethi <Varun.Sethi@freescale.com>
> CC: Zeng Xin <xin.zeng@intel.com>
> CC: Keating Brian <brian.a.keating@intel.com>
> CC: Ma Liang J <liang.j.ma@intel.com>
> CC: Griffin John <john.griffin@intel.com>
> CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> ---
>  content.tex       |    2 +
>  virtio-crypto.tex | 1245 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 1247 insertions(+)
>  create mode 100644 virtio-crypto.tex
> 
> diff --git a/content.tex b/content.tex
> index 4b45678..ab75f78 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len}, \field{residual},
>  \field{status_qualifier}, \field{status}, \field{response} and
>  \field{sense} fields.
> 
> +\input{virtio-crypto.tex}
> +
>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> 
>  Currently there are three device-independent feature bits defined:
> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> new file mode 100644
> index 0000000..732cd30
> --- /dev/null
> +++ b/virtio-crypto.tex
> @@ -0,0 +1,1245 @@
> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> +
> +The virtio crypto device is a virtual cryptography device as well as a kind of
> +virtual hardware accelerator for virtual machines. The encryption and
> +decryption requests are placed in any of the data active queues and are ultimately handled by the

Am I the only one having a problem with 'data active queues'?
I have objected on this before.

> +backend crypto accelerators. The second kind of queue is the control queue used to create 
> +or destroy sessions for symmetric algorithms and will control some advanced
> +features in the future. The virtio crypto device provides the following crypto
> +services: CIPHER, MAC, HASH, and AEAD.
> +
> +
> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
> +
> +20
> +
> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] dataq1
> +\item[\ldots]
> +\item[N-1] dataqN
> +\item[N] controlq
> +\end{description}
> +
> +N is set by \field{max_dataqueues}.
> +
> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature bits}
> +
> +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
> +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for CIPHER service.
> +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for HASH service.
> +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC service.
> +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for AEAD service.
> +

I'm not sure that "non-session" entirely correct grammatically. I would
prefer sessionless as alternatively proposed by Stefan, or even stateless.
I think stateless is the phrase most frequently used to describe what
we want to introduce -- that is basically response = f(request) and
not response = f(request, server_state) where the server_state is
a is determined by a series of previous interactions between the server
and the client).

> +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto Device / Feature bits}
> +
> +Some crypto feature bits require other crypto feature bits
> +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}):
> +
> +\begin{description}
> +\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> +\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> +\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> +\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> +\end{description}
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / Crypto Device / Device configuration layout}
> +
> +The following driver-read-only configuration fields are defined:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_config {
> +    le32 status;
> +    le32 max_dataqueues;
> +    le32 crypto_services;
> +    /* Detailed algorithms mask */
> +    le32 cipher_algo_l;
> +    le32 cipher_algo_h;
> +    le32 hash_algo;
> +    le32 mac_algo_l;
> +    le32 mac_algo_h;
> +    le32 aead_algo;
> +    /* Maximum length of cipher key */
> +    le32 max_cipher_key_len;
> +    /* Maximum length of authenticated key */
> +    le32 max_auth_key_len;
> +    le32 reserved;
> +    /* Maximum size of each crypto request's content */
> +    le64 max_size;
> +};
> +\end{lstlisting}
> +
> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or ~VIRTIO_CRYPTO_S_HW_READY.

Not entirely happy with this. What you want to say is reserved
for future use, or? Would it make sense to have a general note
-- in a similar fashion like for 'sizes are in bytes' -- for
reserved for future use? 

One possible formulation would be:

"In this specification, unless explicitly stated otherwise,
fields and bits reserved for future use shall be zeroed out.
Both the a device or a driver device and the driver should
detect violations of this rule, and deny the requested
operation in an appropriate way if possible."
 

> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
> +\end{lstlisting}
> +
> +The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the hardware is ready to work or not.

I do not like hardware here.

> +
> +The following driver-read-only fields include \field{max_dataqueues}, which specifies the

Why following?

> +maximum number of data virtqueues (dataq1\ldots dataqN), and \field{crypto_services},
> +which indicates the crypto services the virtio crypto supports.
> +
> +The following services are defined:
> +
> +\begin{lstlisting}
> +/* CIPHER service */
> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> +/* HASH service */
> +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> +/* MAC (Message Authentication Codes) service */
> +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> +/* AEAD (Authenticated Encryption with Associated Data) service */
> +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> +\end{lstlisting}
> +
> +The last driver-read-only fields specify detailed algorithms masks 
> +the device offers for corresponding services. The following CIPHER algorithms
> +are defined:

You do not establish an explicit relationship between the fields and the
macros for the flags. These are flags or? It seems quite common in the
spec to use _F_ in flag names. Would it be appropriate here?

> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> +\end{lstlisting}
> +
> +The following HASH algorithms are defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_HASH            0
> +#define VIRTIO_CRYPTO_HASH_MD5           1
> +#define VIRTIO_CRYPTO_HASH_SHA1          2
> +#define VIRTIO_CRYPTO_HASH_SHA_224       3
> +#define VIRTIO_CRYPTO_HASH_SHA_256       4
> +#define VIRTIO_CRYPTO_HASH_SHA_384       5
> +#define VIRTIO_CRYPTO_HASH_SHA_512       6
> +#define VIRTIO_CRYPTO_HASH_SHA3_224      7
> +#define VIRTIO_CRYPTO_HASH_SHA3_256      8
> +#define VIRTIO_CRYPTO_HASH_SHA3_384      9
> +#define VIRTIO_CRYPTO_HASH_SHA3_512      10
> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128      11
> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256      12
> +\end{lstlisting}
> +
> +The following MAC algorithms are defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_MAC                       0
> +#define VIRTIO_CRYPTO_MAC_HMAC_MD5                 1
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1                2
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224             3
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256             4
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384             5
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512             6
> +#define VIRTIO_CRYPTO_MAC_CMAC_3DES                25
> +#define VIRTIO_CRYPTO_MAC_CMAC_AES                 26
> +#define VIRTIO_CRYPTO_MAC_KASUMI_F9                27
> +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2              28
> +#define VIRTIO_CRYPTO_MAC_GMAC_AES                 41
> +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             42
> +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               49
> +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         50
> +#define VIRTIO_CRYPTO_MAC_XCBC_AES                 53
> +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3                 54
> +\end{lstlisting}
> +
> +The following AEAD algorithms are defined:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_AEAD     0
> +#define VIRTIO_CRYPTO_AEAD_GCM    1
> +#define VIRTIO_CRYPTO_AEAD_CCM    2
> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
> +\end{lstlisting}
> +

Would it make sense to interleave the flag definition
with the struct virtio_crypto_config?

> +\begin{note}
> +Any other values are reserved for future use.

Are these flags or values? I do not think values is appropriate here.

> +\end{note}
> +

Some fields are missing here. This is inconsistent. Either
you should describe all or none (here).

> +\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Crypto Device / Device configuration layout}
> +
> +\begin{itemize*}
> +\item The device MUST set \field{max_dataqueues} to between 1 and 65535 inclusive.
> +\item The device MUST set \field{status} based on the status of the hardware-backed implementation.

What is a hardware-backend?

> +\item The device MUST accept and handle requests after \field{status} is set to VIRTIO_CRYPTO_S_HW_READY.

Shouldn't this be the other way around (if VIRTIO_CRYPTO_S_HW_READY
then reject). Is a not well formed request or a backend failure considered?
What does handle mean? What should happen if requests are submitted
before VIRTIO_CRYPTO_S_HW_READY is set?

> +\item The device MUST set \field{crypto_services} based on the crypto services the device offers.
> +\item The device MUST set detailed algorithms masks based on the \field{crypto_services} field.
> +\item The device MUST set \field{max_size} to show the maximum size of crypto request the device supports.
> +\item The device MUST set \field{max_cipher_key_len} to show the maximum length of cipher key if the device supports CIPHER service.
> +\item The device MUST set \field{max_auth_key_len} to show the maximum length of authenticated key if the device supports MAC service.
> +\end{itemize*}
> +
> +\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Crypto Device / Device configuration layout}
> +
> +\begin{itemize*}
> +\item The driver MUST read the ready \field{status} from the bottom bit of status to check whether the hardware-backed
> +      implementation is ready or not, and the driver MUST reread it after the device reset. 
> +\item The driver MUST NOT transmit any packets to the device if the ready \field{status} is not set.
> +\item The driver MUST read \field{max_dataqueues} field to discover the number of data queues the device supports.
> +\item The driver MUST read \field{crypto_services} field to discover which services the device is able to offer.
> +\item The driver MUST read the detailed algorithms fields based on \field{crypto_services} field.
> +\item The driver SHOULD read \field{max_size} to discover the maximum size of crypto request the device supports.
> +\item The driver SHOULD read \field{max_cipher_key_len} to discover the maximum length of cipher key the device supports.
> +\item The driver SHOULD read \field{max_auth_key_len} to discover the maximum length of authenticated key the device supports.
> +\end{itemize*}
> +
> +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device / Device Initialization}
> +
> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / Crypto Device / Device Initialization}
> +
> +\begin{itemize*}
> +\item The driver MUST identify and initialize the control virtqueue.

But does not have to identify and initialize any data virtqueues?

> +\item The driver MUST read the supported crypto services from bits of \field{crypto_services}. 
> +\item The driver MUST read the supported algorithms based on \field{crypto_services} field.
> +\end{itemize*}
> +
> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Crypto Device / Device Initialization}
> +
> +\begin{itemize*}
> +\item The device MUST be configured with at least one accelerator which executes backend crypto operations.

What does configured mean here? Is this initialization requirement.

> +\item The device MUST write the \field{crypto_services} field based on the capacities of the backend accelerator.
> +\end{itemize*}
> +

How do 'Initialization' and 'Configuration Layout' requirements relate to
each-other.

> +\subsection{Device Operation}\label{sec:Device Types / Crypto Device / Device Operation}
> +
> +Packets can be transmitted by placing them in both the controlq and dataq.
> +Packets consist of a general header and a service-specific request.

Are packets and requests synonyms?

> +Where 'general header' is for all crypto requests, and 'service specific requests'

>From below it seems you have two types of 'general header', but up until this
point it seems there is a single definition. Of course, this does not
really matter.

> +are composed of operation parameter + output data + input data in general.
> +Operation parameters are algorithm-specific parameters, output data is the
> +data that should be utilized in operations, and input data is equal to
> +"operation result + result data".
> +
> +The device can support both session mode (See \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation}) and non-session mode, for example,
> +As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is negotiated, the driver can use non-session mode for CIPHER service, otherwise it can only use session mode.

Grammar: Does not seem right to me. 'As' seems off and this could be two
sentences.


> +
> +\begin{note}
> +The basic unit of all data length the byte.
> +\end{note}
> +
> +The general header for controlq is as follows:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
> +
> +struct virtio_crypto_ctrl_header {
> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x02)
> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x03)
> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
> +    le32 opcode;
> +    le32 algo;
> +    le32 flag;
> +    /* data virtqueue id */
> +    le32 queue_id;
> +};
> +\end{lstlisting}
> +
> +The general header of dataq:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_op_header {
> +#define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
> +#define VIRTIO_CRYPTO_CIPHER_DECRYPT \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
> +#define VIRTIO_CRYPTO_HASH \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
> +#define VIRTIO_CRYPTO_MAC \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
> +#define VIRTIO_CRYPTO_AEAD_ENCRYPT \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> +#define VIRTIO_CRYPTO_AEAD_DECRYPT \
> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> +    le32 opcode;
> +    /* algo should be service-specific algorithms */
> +    le32 algo;
> +    /* session_id should be service-specific algorithms */
> +    le64 session_id;
> +#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
> +#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2

Use _F_ istead of _FLAG_? 

> +    /* control flag to control the request */
> +    le32 flag;
> +    le32 padding;
> +};
> +\end{lstlisting}
> +
> +The device can set the operation status as follows: VIRTIO_CRYPTO_OK: success;
> +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP: not supported;
> +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto operations.
> +
> +\begin{lstlisting}
> +enum VIRITO_CRYPTO_STATUS {
> +    VIRTIO_CRYPTO_OK = 0,
> +    VIRTIO_CRYPTO_ERR = 1,
> +    VIRTIO_CRYPTO_BADMSG = 2,
> +    VIRTIO_CRYPTO_NOTSUPP = 3,
> +    VIRTIO_CRYPTO_INVSESS = 4,
> +    VIRTIO_CRYPTO_MAX
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue}
> +
> +The driver uses the control virtqueue to send control commands to the
> +device which handles the non-data plane operations, such as session

What is a 'non-data plane'?

> +operations (See \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation}).

Reviewed up until here. Depending on how things evolve will try to
review the rest too in the following days.

A question and a remark as a closing word:
* Are there already some kernel and qemu patches for this 'non-session' stuff?
* I think some proofreading (and eventually also touch-up) by a native speaker
would really benefit us. Sadly my grammar skills are very questionable, so
I can't help much. Nevertheless since it is a spec, I think we sould strive
for high standards in language usage too.

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Cornelia Huck]

On Tue, 7 Feb 2017 12:39:44 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 01/18/2017 09:22 AM, Gonglei wrote:

> > +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> > +
> > +The virtio crypto device is a virtual cryptography device as well as a kind of
> > +virtual hardware accelerator for virtual machines. The encryption and
> > +decryption requests are placed in any of the data active queues and are ultimately handled by the
> 
> Am I the only one having a problem with 'data active queues'?

No. I think it looks weird.

(...)

> > +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or ~VIRTIO_CRYPTO_S_HW_READY.
> 
> Not entirely happy with this. What you want to say is reserved
> for future use, or? Would it make sense to have a general note
> -- in a similar fashion like for 'sizes are in bytes' -- for
> reserved for future use? 
> 
> One possible formulation would be:
> 
> "In this specification, unless explicitly stated otherwise,
> fields and bits reserved for future use shall be zeroed out.
> Both the a device or a driver device and the driver should
> detect violations of this rule, and deny the requested
> operation in an appropriate way if possible."

If we go with reserved-and-must-be-zero, we need to make rejecting
non-zero for reserved value a MUST, or we may run into problems later.

In this case, I'd opt for a specific formulation, though; like

"The \field{status} field can be either zero or have one or more flags
set. Valid flags are listed below."

And then state that non-valid flags MUST NOT be set resp. MUST be
rejected in a normative statement.

(...)

> > +The following services are defined:
> > +
> > +\begin{lstlisting}
> > +/* CIPHER service */
> > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> > +/* HASH service */
> > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> > +/* MAC (Message Authentication Codes) service */
> > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> > +/* AEAD (Authenticated Encryption with Associated Data) service */
> > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> > +\end{lstlisting}
> > +
> > +The last driver-read-only fields specify detailed algorithms masks 
> > +the device offers for corresponding services. The following CIPHER algorithms
> > +are defined:
> 
> You do not establish an explicit relationship between the fields and the
> macros for the flags. These are flags or? It seems quite common in the
> spec to use _F_ in flag names. Would it be appropriate here?

The values as specified do not look very flaggy to me...

> 
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> > +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> > +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> > +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> > +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> > +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> > +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> > +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> > +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> > +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> > +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> > +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> > +\end{lstlisting}
> > +
> > +The following HASH algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_HASH            0
> > +#define VIRTIO_CRYPTO_HASH_MD5           1
> > +#define VIRTIO_CRYPTO_HASH_SHA1          2
> > +#define VIRTIO_CRYPTO_HASH_SHA_224       3
> > +#define VIRTIO_CRYPTO_HASH_SHA_256       4
> > +#define VIRTIO_CRYPTO_HASH_SHA_384       5
> > +#define VIRTIO_CRYPTO_HASH_SHA_512       6
> > +#define VIRTIO_CRYPTO_HASH_SHA3_224      7
> > +#define VIRTIO_CRYPTO_HASH_SHA3_256      8
> > +#define VIRTIO_CRYPTO_HASH_SHA3_384      9
> > +#define VIRTIO_CRYPTO_HASH_SHA3_512      10
> > +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128      11
> > +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256      12
> > +\end{lstlisting}
> > +
> > +The following MAC algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_MAC                       0
> > +#define VIRTIO_CRYPTO_MAC_HMAC_MD5                 1
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1                2
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224             3
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256             4
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384             5
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512             6
> > +#define VIRTIO_CRYPTO_MAC_CMAC_3DES                25
> > +#define VIRTIO_CRYPTO_MAC_CMAC_AES                 26
> > +#define VIRTIO_CRYPTO_MAC_KASUMI_F9                27
> > +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2              28
> > +#define VIRTIO_CRYPTO_MAC_GMAC_AES                 41
> > +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             42
> > +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               49
> > +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         50
> > +#define VIRTIO_CRYPTO_MAC_XCBC_AES                 53
> > +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3                 54
> > +\end{lstlisting}
> > +
> > +The following AEAD algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_AEAD     0
> > +#define VIRTIO_CRYPTO_AEAD_GCM    1
> > +#define VIRTIO_CRYPTO_AEAD_CCM    2
> > +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
> > +\end{lstlisting}
> > +
> 
> Would it make sense to interleave the flag definition
> with the struct virtio_crypto_config?
> 
> > +\begin{note}
> > +Any other values are reserved for future use.
> 
> Are these flags or values? I do not think values is appropriate here.

These look like values to me and not flags.

The more I stare at this, the more confused I become. Is the device
supposed to indicate exactly one algorithm only? Or are these supposed
to be bit numbers? (If yes, it would make sense to either call them
that or specify flags in the (1 << bit_num) notation.)

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

Hi Cornelia,

> 
> On Tue, 7 Feb 2017 12:39:44 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> > On 01/18/2017 09:22 AM, Gonglei wrote:
> 
> > > +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> > > +
> > > +The virtio crypto device is a virtual cryptography device as well as a kind of
> > > +virtual hardware accelerator for virtual machines. The encryption and
> > > +decryption requests are placed in any of the data active queues and are
> ultimately handled by the
> >
> > Am I the only one having a problem with 'data active queues'?
> 
> No. I think it looks weird.
> 
> (...)
> 
> > > +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
> ~VIRTIO_CRYPTO_S_HW_READY.
> >
> > Not entirely happy with this. What you want to say is reserved
> > for future use, or? Would it make sense to have a general note
> > -- in a similar fashion like for 'sizes are in bytes' -- for
> > reserved for future use?
> >
> > One possible formulation would be:
> >
> > "In this specification, unless explicitly stated otherwise,
> > fields and bits reserved for future use shall be zeroed out.
> > Both the a device or a driver device and the driver should
> > detect violations of this rule, and deny the requested
> > operation in an appropriate way if possible."
> 
> If we go with reserved-and-must-be-zero, we need to make rejecting
> non-zero for reserved value a MUST, or we may run into problems later.
> 
> In this case, I'd opt for a specific formulation, though; like
> 
> "The \field{status} field can be either zero or have one or more flags
> set. Valid flags are listed below."
> 
> And then state that non-valid flags MUST NOT be set resp. MUST be
> rejected in a normative statement.
> 
Sounds good.

> (...)
> 
> > > +The following services are defined:
> > > +
> > > +\begin{lstlisting}
> > > +/* CIPHER service */
> > > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> > > +/* HASH service */
> > > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> > > +/* MAC (Message Authentication Codes) service */
> > > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> > > +/* AEAD (Authenticated Encryption with Associated Data) service */
> > > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> > > +\end{lstlisting}
> > > +
> > > +The last driver-read-only fields specify detailed algorithms masks
> > > +the device offers for corresponding services. The following CIPHER
> algorithms
> > > +are defined:
> >
> > You do not establish an explicit relationship between the fields and the
> > macros for the flags. These are flags or? It seems quite common in the
> > spec to use _F_ in flag names. Would it be appropriate here?
> 
> The values as specified do not look very flaggy to me...
> 
> >
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> > > +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> > > +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> > > +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> > > +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> > > +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> > > +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> > > +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> > > +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> > > +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> > > +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> > > +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> > > +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> > > +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> > > +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> > > +\end{lstlisting}
> > > +
> > > +
> >
> > Would it make sense to interleave the flag definition
> > with the struct virtio_crypto_config?
> >
> > > +\begin{note}
> > > +Any other values are reserved for future use.
> >
> > Are these flags or values? I do not think values is appropriate here.
> 
> These look like values to me and not flags.
> 
> The more I stare at this, the more confused I become. Is the device
> supposed to indicate exactly one algorithm only? Or are these supposed
> to be bit numbers? (If yes, it would make sense to either call them
> that or specify flags in the (1 << bit_num) notation.)

Actually these are both bit numbers and values.

On the one hand, the device can set algorithms by '1 << bit_num' notation to tell the driver what
algorithms are supported. On the other hand, the driver can set the value to tell
the device a virtio crypto request's algorithm in struct virtio_crypto_op_header.algo.

Pls see cryptodev-builtin.c:: cryptodev_buitlin_init()

backend->conf.crypto_services =
                         1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
                         1u << VIRTIO_CRYPTO_SERVICE_HASH |
                         1u << VIRTIO_CRYPTO_SERVICE_MAC;
backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;

Perhaps I should add a specific formulation about them.

Thanks,
-Gonglei

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 02/08/2017 04:46 AM, Gonglei (Arei) wrote:
> Hi Cornelia,
> 
>>
>> On Tue, 7 Feb 2017 12:39:44 +0100
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 01/18/2017 09:22 AM, Gonglei wrote:
>>
>>>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
>>>> +
>>>> +The virtio crypto device is a virtual cryptography device as well as a kind of
>>>> +virtual hardware accelerator for virtual machines. The encryption and
>>>> +decryption requests are placed in any of the data active queues and are
>> ultimately handled by the
>>>
>>> Am I the only one having a problem with 'data active queues'?
>>
>> No. I think it looks weird.
>>
>> (...)
>>
>>>> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
>> ~VIRTIO_CRYPTO_S_HW_READY.
>>>
>>> Not entirely happy with this. What you want to say is reserved
>>> for future use, or? Would it make sense to have a general note
>>> -- in a similar fashion like for 'sizes are in bytes' -- for
>>> reserved for future use?
>>>
>>> One possible formulation would be:
>>>
>>> "In this specification, unless explicitly stated otherwise,
>>> fields and bits reserved for future use shall be zeroed out.
>>> Both the a device or a driver device and the driver should
>>> detect violations of this rule, and deny the requested
>>> operation in an appropriate way if possible."
>>
>> If we go with reserved-and-must-be-zero, we need to make rejecting
>> non-zero for reserved value a MUST, or we may run into problems later.
>>
>> In this case, I'd opt for a specific formulation, though; like
>>
>> "The \field{status} field can be either zero or have one or more flags
>> set. Valid flags are listed below."
>>
>> And then state that non-valid flags MUST NOT be set resp. MUST be
>> rejected in a normative statement.
>>
> Sounds good.
> 

This is not only about \field{status} but about the algo mask fields
too. If we go down this path we should make sure to have a specific statement
in each case we reserve something for future use.

The part about the 'normative statement' is very important in my opinion too.

>> (...)
>>
>>>> +The following services are defined:
>>>> +
>>>> +\begin{lstlisting}
>>>> +/* CIPHER service */
>>>> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
>>>> +/* HASH service */
>>>> +#define VIRTIO_CRYPTO_SERVICE_HASH   1
>>>> +/* MAC (Message Authentication Codes) service */
>>>> +#define VIRTIO_CRYPTO_SERVICE_MAC    2
>>>> +/* AEAD (Authenticated Encryption with Associated Data) service */
>>>> +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
>>>> +\end{lstlisting}
>>>> +
>>>> +The last driver-read-only fields specify detailed algorithms masks
>>>> +the device offers for corresponding services. The following CIPHER
>> algorithms
>>>> +are defined:
>>>
>>> You do not establish an explicit relationship between the fields and the
>>> macros for the flags. These are flags or? It seems quite common in the
>>> spec to use _F_ in flag names. Would it be appropriate here?
>>
>> The values as specified do not look very flaggy to me...
>>
>>>
>>>> +
>>>> +\begin{lstlisting}
>>>> +#define VIRTIO_CRYPTO_NO_CIPHER                 0
>>>> +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
>>>> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
>>>> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
>>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
>>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
>>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
>>>> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
>>>> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
>>>> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
>>>> +\end{lstlisting}
>>>> +
>>>> +
>>>
>>> Would it make sense to interleave the flag definition
>>> with the struct virtio_crypto_config?
>>>
>>>> +\begin{note}
>>>> +Any other values are reserved for future use.
>>>
>>> Are these flags or values? I do not think values is appropriate here.
>>
>> These look like values to me and not flags.
>>
>> The more I stare at this, the more confused I become. Is the device
>> supposed to indicate exactly one algorithm only? Or are these supposed
>> to be bit numbers? (If yes, it would make sense to either call them
>> that or specify flags in the (1 << bit_num) notation.)
> 
> Actually these are both bit numbers and values.
> 
> On the one hand, the device can set algorithms by '1 << bit_num' notation to tell the driver what
> algorithms are supported. On the other hand, the driver can set the value to tell
> the device a virtio crypto request's algorithm in struct virtio_crypto_op_header.algo.
> 
> Pls see cryptodev-builtin.c:: cryptodev_buitlin_init()
> 
> backend->conf.crypto_services =
>                          1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
>                          1u << VIRTIO_CRYPTO_SERVICE_HASH |
>                          1u << VIRTIO_CRYPTO_SERVICE_MAC;
> backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
> backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> 
> Perhaps I should add a specific formulation about them.


Thanks for the clarification. I think a 'specific formulation' is a good idea.
I suggest to define the constants elsewhere, that is at the site where the
algorithms are defined, and only reference that (or those) definition(s).

Obviously we criteria for valid/invalid for the both usages.
so you will have to describe that separately for the distinct usages.

Concentrate on the fields you are describing and their semantic.

Cheers,
Halil

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

> 
> On 02/08/2017 04:46 AM, Gonglei (Arei) wrote:
> > Hi Cornelia,
> >
> >>
> >> On Tue, 7 Feb 2017 12:39:44 +0100
> >> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>
> >>> On 01/18/2017 09:22 AM, Gonglei wrote:
> >>
> >>>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> >>>> +
> >>>> +The virtio crypto device is a virtual cryptography device as well as a kind
> of
> >>>> +virtual hardware accelerator for virtual machines. The encryption and
> >>>> +decryption requests are placed in any of the data active queues and are
> >> ultimately handled by the
> >>>
> >>> Am I the only one having a problem with 'data active queues'?
> >>
> >> No. I think it looks weird.
> >>
> >> (...)
> >>
> >>>> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
> >> ~VIRTIO_CRYPTO_S_HW_READY.
> >>>
> >>> Not entirely happy with this. What you want to say is reserved
> >>> for future use, or? Would it make sense to have a general note
> >>> -- in a similar fashion like for 'sizes are in bytes' -- for
> >>> reserved for future use?
> >>>
> >>> One possible formulation would be:
> >>>
> >>> "In this specification, unless explicitly stated otherwise,
> >>> fields and bits reserved for future use shall be zeroed out.
> >>> Both the a device or a driver device and the driver should
> >>> detect violations of this rule, and deny the requested
> >>> operation in an appropriate way if possible."
> >>
> >> If we go with reserved-and-must-be-zero, we need to make rejecting
> >> non-zero for reserved value a MUST, or we may run into problems later.
> >>
> >> In this case, I'd opt for a specific formulation, though; like
> >>
> >> "The \field{status} field can be either zero or have one or more flags
> >> set. Valid flags are listed below."
> >>
> >> And then state that non-valid flags MUST NOT be set resp. MUST be
> >> rejected in a normative statement.
> >>
> > Sounds good.
> >
> 
> This is not only about \field{status} but about the algo mask fields
> too. If we go down this path we should make sure to have a specific statement
> in each case we reserve something for future use.
> 
> The part about the 'normative statement' is very important in my opinion too.
> 
I agree.

> >> (...)
> >>
> >>>> +The following services are defined:
> >>>> +
> >>>> +\begin{lstlisting}
> >>>> +/* CIPHER service */
> >>>> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> >>>> +/* HASH service */
> >>>> +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> >>>> +/* MAC (Message Authentication Codes) service */
> >>>> +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> >>>> +/* AEAD (Authenticated Encryption with Associated Data) service */
> >>>> +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> >>>> +\end{lstlisting}
> >>>> +
> >>>> +The last driver-read-only fields specify detailed algorithms masks
> >>>> +the device offers for corresponding services. The following CIPHER
> >> algorithms
> >>>> +are defined:
> >>>
> >>> You do not establish an explicit relationship between the fields and the
> >>> macros for the flags. These are flags or? It seems quite common in the
> >>> spec to use _F_ in flag names. Would it be appropriate here?
> >>
> >> The values as specified do not look very flaggy to me...
> >>
> >>>
> >>>> +
> >>>> +\begin{lstlisting}
> >>>> +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> >>>> +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> >>>> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> >>>> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> >>>> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> >>>> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> >>>> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> >>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> >>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> >>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> >>>> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> >>>> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> >>>> +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> >>>> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> >>>> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> >>>> +\end{lstlisting}
> >>>> +
> >>>> +
> >>>
> >>> Would it make sense to interleave the flag definition
> >>> with the struct virtio_crypto_config?
> >>>
> >>>> +\begin{note}
> >>>> +Any other values are reserved for future use.
> >>>
> >>> Are these flags or values? I do not think values is appropriate here.
> >>
> >> These look like values to me and not flags.
> >>
> >> The more I stare at this, the more confused I become. Is the device
> >> supposed to indicate exactly one algorithm only? Or are these supposed
> >> to be bit numbers? (If yes, it would make sense to either call them
> >> that or specify flags in the (1 << bit_num) notation.)
> >
> > Actually these are both bit numbers and values.
> >
> > On the one hand, the device can set algorithms by '1 << bit_num' notation to
> tell the driver what
> > algorithms are supported. On the other hand, the driver can set the value to
> tell
> > the device a virtio crypto request's algorithm in struct
> virtio_crypto_op_header.algo.
> >
> > Pls see cryptodev-builtin.c:: cryptodev_buitlin_init()
> >
> > backend->conf.crypto_services =
> >                          1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
> >                          1u << VIRTIO_CRYPTO_SERVICE_HASH |
> >                          1u << VIRTIO_CRYPTO_SERVICE_MAC;
> > backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
> > backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> >
> > Perhaps I should add a specific formulation about them.
> 
> 
> Thanks for the clarification. I think a 'specific formulation' is a good idea.
> I suggest to define the constants elsewhere, that is at the site where the
> algorithms are defined, and only reference that (or those) definition(s).
> 
Good idea.

> Obviously we criteria for valid/invalid for the both usages.
> so you will have to describe that separately for the distinct usages.
> 
Oh, so much work need to be done.

Halil, Would you mind work together with me to perfect the spec?
And feel free to add your signed-off-by tag. :)

TBH as a non-native English speaker, it's more difficult writing a
spec than coding. :(

Look forward to your reply.

Thanks,
-Gonglei

> Concentrate on the fields you are describing and their semantic.
> 
> Cheers,
> Halil

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 02/09/2017 03:29 AM, Gonglei (Arei) wrote:
[..]
> Oh, so much work need to be done.
> 
> Halil, Would you mind work together with me to perfect the spec?
> And feel free to add your signed-off-by tag. :)
> 
> TBH as a non-native English speaker, it's more difficult writing a
> spec than coding. :(
> 
> Look forward to your reply.
> 


First, sorry for the long delay -- was busy and then ill. Thank you
very much for your offer. I would prefer continuing as a reviewer,
but I would very much appreciate if you could add me to the
'Acknowledgments' appendix as a part of your patch ;). Unfortunately
I do not have the time now to allocate significantly more time for
this. I'm also having difficulties to think of another way of working
efficiently together on this, than what we already do. I can try to
provide more suggestions in terms of formulation, but it's still 
just review. Thank you very much!

Halil

Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

Hi Halil,

> 
> On 02/09/2017 03:29 AM, Gonglei (Arei) wrote:
> [..]
> > Oh, so much work need to be done.
> >
> > Halil, Would you mind work together with me to perfect the spec?
> > And feel free to add your signed-off-by tag. :)
> >
> > TBH as a non-native English speaker, it's more difficult writing a
> > spec than coding. :(
> >
> > Look forward to your reply.
> >
> 
> 
> First, sorry for the long delay -- was busy and then ill. Thank you

I hope you feel better now.

> very much for your offer. I would prefer continuing as a reviewer,
> but I would very much appreciate if you could add me to the
> 'Acknowledgments' appendix as a part of your patch ;). 

No problem, I can do that.

> Unfortunately
> I do not have the time now to allocate significantly more time for
> this. I'm also having difficulties to think of another way of working
> efficiently together on this, than what we already do. I can try to
> provide more suggestions in terms of formulation, but it's still
> just review. Thank you very much!
> 

OK, thank you, I can understand. Just like me, I am also busy solving bugs
of inner projects and some works with high priority. So, recently
I have little time on the spec. :( But I'll do it once I have time again.

Thanks,
-Gonglei

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Cornelia Huck]

On Wed, 8 Feb 2017 03:46:53 +0000
"Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:

> Hi Cornelia,
> 
> > 
> > On Tue, 7 Feb 2017 12:39:44 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > 
> > > On 01/18/2017 09:22 AM, Gonglei wrote:
>
> > > > +The following services are defined:
> > > > +
> > > > +\begin{lstlisting}
> > > > +/* CIPHER service */
> > > > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> > > > +/* HASH service */
> > > > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> > > > +/* MAC (Message Authentication Codes) service */
> > > > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> > > > +/* AEAD (Authenticated Encryption with Associated Data) service */
> > > > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> > > > +\end{lstlisting}
> > > > +
> > > > +The last driver-read-only fields specify detailed algorithms masks
> > > > +the device offers for corresponding services. The following CIPHER
> > algorithms
> > > > +are defined:
> > >
> > > You do not establish an explicit relationship between the fields and the
> > > macros for the flags. These are flags or? It seems quite common in the
> > > spec to use _F_ in flag names. Would it be appropriate here?
> > 
> > The values as specified do not look very flaggy to me...
> > 
> > >
> > > > +
> > > > +\begin{lstlisting}
> > > > +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> > > > +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> > > > +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> > > > +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> > > > +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> > > > +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> > > > +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> > > > +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> > > > +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> > > > +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> > > > +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> > > > +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> > > > +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> > > > +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> > > > +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> > > > +\end{lstlisting}
> > > > +
> > > > +
> > >
> > > Would it make sense to interleave the flag definition
> > > with the struct virtio_crypto_config?
> > >
> > > > +\begin{note}
> > > > +Any other values are reserved for future use.
> > >
> > > Are these flags or values? I do not think values is appropriate here.
> > 
> > These look like values to me and not flags.
> > 
> > The more I stare at this, the more confused I become. Is the device
> > supposed to indicate exactly one algorithm only? Or are these supposed
> > to be bit numbers? (If yes, it would make sense to either call them
> > that or specify flags in the (1 << bit_num) notation.)
> 
> Actually these are both bit numbers and values.
> 
> On the one hand, the device can set algorithms by '1 << bit_num' notation to tell the driver what
> algorithms are supported. On the other hand, the driver can set the value to tell
> the device a virtio crypto request's algorithm in struct virtio_crypto_op_header.algo.
> 
> Pls see cryptodev-builtin.c:: cryptodev_buitlin_init()
> 
> backend->conf.crypto_services =
>                          1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
>                          1u << VIRTIO_CRYPTO_SERVICE_HASH |
>                          1u << VIRTIO_CRYPTO_SERVICE_MAC;
> backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
> backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> 
> Perhaps I should add a specific formulation about them.

Can you add, for example, a note that the respective fields define the
supported services (...) with bits corresponding to a specific service
(...) on a 1:1 basis? Then define which way your bit numbering is done
and that the following defines specify the bit number. Then your
existing lists are fine, I think.

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

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

Hi Halil,

Thanks for your comments firstly.

> 
> On 01/18/2017 09:22 AM, Gonglei wrote:
> > The virtio crypto device is a virtual crypto device (ie. hardware
> > crypto accelerator card). Currently, the virtio crypto device provides
> > the following crypto services: CIPHER, MAC, HASH, and AEAD.
> >
> > In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
> >
> > VIRTIO-153
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Cornelia Huck <cornelia.huck@de.ibm.com>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> > CC: Lingli Deng <denglingli@chinamobile.com>
> > CC: Jani Kokkonen <Jani.Kokkonen@huawei.com>
> > CC: Ola Liljedahl <Ola.Liljedahl@arm.com>
> > CC: Varun Sethi <Varun.Sethi@freescale.com>
> > CC: Zeng Xin <xin.zeng@intel.com>
> > CC: Keating Brian <brian.a.keating@intel.com>
> > CC: Ma Liang J <liang.j.ma@intel.com>
> > CC: Griffin John <john.griffin@intel.com>
> > CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> > ---
> >  content.tex       |    2 +
> >  virtio-crypto.tex | 1245
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 1247 insertions(+)
> >  create mode 100644 virtio-crypto.tex
> >
> > diff --git a/content.tex b/content.tex
> > index 4b45678..ab75f78 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len},
> \field{residual},
> >  \field{status_qualifier}, \field{status}, \field{response} and
> >  \field{sense} fields.
> >
> > +\input{virtio-crypto.tex}
> > +
> >  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >
> >  Currently there are three device-independent feature bits defined:
> > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> > new file mode 100644
> > index 0000000..732cd30
> > --- /dev/null
> > +++ b/virtio-crypto.tex
> > @@ -0,0 +1,1245 @@
> > +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> > +
> > +The virtio crypto device is a virtual cryptography device as well as a kind of
> > +virtual hardware accelerator for virtual machines. The encryption and
> > +decryption requests are placed in any of the data active queues and are
> ultimately handled by the
> 
> Am I the only one having a problem with 'data active queues'?

Maybe 'data queues' here is enough?

> I have objected on this before.
> 
> > +backend crypto accelerators. The second kind of queue is the control queue
> used to create
> > +or destroy sessions for symmetric algorithms and will control some
> advanced
> > +features in the future. The virtio crypto device provides the following crypto
> > +services: CIPHER, MAC, HASH, and AEAD.
> > +
> > +
> > +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
> > +
> > +20
> > +
> > +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device /
> Virtqueues}
> > +
> > +\begin{description}
> > +\item[0] dataq1
> > +\item[\ldots]
> > +\item[N-1] dataqN
> > +\item[N] controlq
> > +\end{description}
> > +
> > +N is set by \field{max_dataqueues}.
> > +
> > +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature
> bits}
> > +
> > +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
> > +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is
> available for CIPHER service.
> > +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is
> available for HASH service.
> > +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is
> available for MAC service.
> > +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is
> available for AEAD service.
> > +
> 
> I'm not sure that "non-session" entirely correct grammatically. I would
> prefer sessionless as alternatively proposed by Stefan, or even stateless.
> I think stateless is the phrase most frequently used to describe what
> we want to introduce -- that is basically response = f(request) and
> not response = f(request, server_state) where the server_state is
> a is determined by a series of previous interactions between the server
> and the client).
> 
Makes sense. I discussed with Xin about this on call meeting two weeks ago.
I decide to use stateless mode instead of non-session mode here.

> > +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto
> Device / Feature bits}
> > +
> > +Some crypto feature bits require other crypto feature bits
> > +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}):
> > +
> > +\begin{description}
> > +\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires
> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> > +\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires
> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> > +\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires
> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> > +\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires
> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> > +\end{description}
> > +
> > +\subsection{Device configuration layout}\label{sec:Device Types / Crypto
> Device / Device configuration layout}
> > +
> > +The following driver-read-only configuration fields are defined:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_config {
> > +    le32 status;
> > +    le32 max_dataqueues;
> > +    le32 crypto_services;
> > +    /* Detailed algorithms mask */
> > +    le32 cipher_algo_l;
> > +    le32 cipher_algo_h;
> > +    le32 hash_algo;
> > +    le32 mac_algo_l;
> > +    le32 mac_algo_h;
> > +    le32 aead_algo;
> > +    /* Maximum length of cipher key */
> > +    le32 max_cipher_key_len;
> > +    /* Maximum length of authenticated key */
> > +    le32 max_auth_key_len;
> > +    le32 reserved;
> > +    /* Maximum size of each crypto request's content */
> > +    le64 max_size;
> > +};
> > +\end{lstlisting}
> > +
> > +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
> ~VIRTIO_CRYPTO_S_HW_READY.
> 
> Not entirely happy with this. What you want to say is reserved
> for future use, or? Would it make sense to have a general note
> -- in a similar fashion like for 'sizes are in bytes' -- for
> reserved for future use?
> 
> One possible formulation would be:
> 
> "In this specification, unless explicitly stated otherwise,
> fields and bits reserved for future use shall be zeroed out.
> Both the a device or a driver device and the driver should
> detect violations of this rule, and deny the requested
> operation in an appropriate way if possible."
> 
> 
Cornelia also provided a good suggestion about this. :)

> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
> > +\end{lstlisting}
> > +
> > +The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the
> hardware is ready to work or not.
> 
> I do not like hardware here.
> 
Sorry about that, but it has existed in both Qemu and Linux driver. :(

> > +
> > +The following driver-read-only fields include \field{max_dataqueues}, which
> specifies the
> 
> Why following?
> 
Er, I used 'following' at here and there... It's just an auxiliary word.

> > +maximum number of data virtqueues (dataq1\ldots dataqN), and
> \field{crypto_services},
> > +which indicates the crypto services the virtio crypto supports.
> > +
> > +The following services are defined:
> > +
> > +\begin{lstlisting}
> > +/* CIPHER service */
> > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> > +/* HASH service */
> > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> > +/* MAC (Message Authentication Codes) service */
> > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> > +/* AEAD (Authenticated Encryption with Associated Data) service */
> > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> > +\end{lstlisting}
> > +
> > +The last driver-read-only fields specify detailed algorithms masks
> > +the device offers for corresponding services. The following CIPHER
> algorithms
> > +are defined:
> 
> You do not establish an explicit relationship between the fields and the
> macros for the flags. These are flags or? It seems quite common in the
> spec to use _F_ in flag names. Would it be appropriate here?
> 
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> > +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> > +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> > +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> > +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> > +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> > +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> > +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> > +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> > +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> > +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> > +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> > +\end{lstlisting}
> > +
> > +The following HASH algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_HASH            0
> > +#define VIRTIO_CRYPTO_HASH_MD5           1
> > +#define VIRTIO_CRYPTO_HASH_SHA1          2
> > +#define VIRTIO_CRYPTO_HASH_SHA_224       3
> > +#define VIRTIO_CRYPTO_HASH_SHA_256       4
> > +#define VIRTIO_CRYPTO_HASH_SHA_384       5
> > +#define VIRTIO_CRYPTO_HASH_SHA_512       6
> > +#define VIRTIO_CRYPTO_HASH_SHA3_224      7
> > +#define VIRTIO_CRYPTO_HASH_SHA3_256      8
> > +#define VIRTIO_CRYPTO_HASH_SHA3_384      9
> > +#define VIRTIO_CRYPTO_HASH_SHA3_512      10
> > +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128      11
> > +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256      12
> > +\end{lstlisting}
> > +
> > +The following MAC algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_MAC                       0
> > +#define VIRTIO_CRYPTO_MAC_HMAC_MD5                 1
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1                2
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224             3
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256             4
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384             5
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512             6
> > +#define VIRTIO_CRYPTO_MAC_CMAC_3DES                25
> > +#define VIRTIO_CRYPTO_MAC_CMAC_AES                 26
> > +#define VIRTIO_CRYPTO_MAC_KASUMI_F9                27
> > +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2              28
> > +#define VIRTIO_CRYPTO_MAC_GMAC_AES                 41
> > +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             42
> > +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               49
> > +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         50
> > +#define VIRTIO_CRYPTO_MAC_XCBC_AES                 53
> > +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3                 54
> > +\end{lstlisting}
> > +
> > +The following AEAD algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_AEAD     0
> > +#define VIRTIO_CRYPTO_AEAD_GCM    1
> > +#define VIRTIO_CRYPTO_AEAD_CCM    2
> > +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
> > +\end{lstlisting}
> > +
> 
> Would it make sense to interleave the flag definition
> with the struct virtio_crypto_config?
> 
> > +\begin{note}
> > +Any other values are reserved for future use.
> 
> Are these flags or values? I do not think values is appropriate here.
> 
Please refer to my reply in another thread.

> > +\end{note}
> > +
> 
> Some fields are missing here. This is inconsistent. Either
> you should describe all or none (here).
> 
Ok, will add other fields.

> > +\devicenormative{\subsubsection}{Device configuration layout}{Device Types
> / Crypto Device / Device configuration layout}
> > +
> > +\begin{itemize*}
> > +\item The device MUST set \field{max_dataqueues} to between 1 and 65535
> inclusive.
> > +\item The device MUST set \field{status} based on the status of the
> hardware-backed implementation.
> 
> What is a hardware-backend?
> 
I meant the cryptodev backend, cryptodev-builtin, cryptodev-vhost-user for example.

> > +\item The device MUST accept and handle requests after \field{status} is set
> to VIRTIO_CRYPTO_S_HW_READY.
> 
> Shouldn't this be the other way around (if VIRTIO_CRYPTO_S_HW_READY
> then reject). Is a not well formed request or a backend failure considered?
> What does handle mean? What should happen if requests are submitted
> before VIRTIO_CRYPTO_S_HW_READY is set?
> 
The requests MUST not be transmitted before VIRTIO_CRYPTO_S_HW_READY is set.
Which is stated by the driver requirements. Otherwise the requests will be dropped.

The handle means execute crypto operations.

> > +\item The device MUST set \field{crypto_services} based on the crypto
> services the device offers.
> > +\item The device MUST set detailed algorithms masks based on the
> \field{crypto_services} field.
> > +\item The device MUST set \field{max_size} to show the maximum size of
> crypto request the device supports.
> > +\item The device MUST set \field{max_cipher_key_len} to show the
> maximum length of cipher key if the device supports CIPHER service.
> > +\item The device MUST set \field{max_auth_key_len} to show the maximum
> length of authenticated key if the device supports MAC service.
> > +\end{itemize*}
> > +
> > +\drivernormative{\subsubsection}{Device configuration layout}{Device Types
> / Crypto Device / Device configuration layout}
> > +
> > +\begin{itemize*}
> > +\item The driver MUST read the ready \field{status} from the bottom bit of
> status to check whether the hardware-backed
> > +      implementation is ready or not, and the driver MUST reread it after
> the device reset.
> > +\item The driver MUST NOT transmit any packets to the device if the ready
> \field{status} is not set.
> > +\item The driver MUST read \field{max_dataqueues} field to discover the
> number of data queues the device supports.
> > +\item The driver MUST read \field{crypto_services} field to discover which
> services the device is able to offer.
> > +\item The driver MUST read the detailed algorithms fields based on
> \field{crypto_services} field.
> > +\item The driver SHOULD read \field{max_size} to discover the maximum
> size of crypto request the device supports.
> > +\item The driver SHOULD read \field{max_cipher_key_len} to discover the
> maximum length of cipher key the device supports.
> > +\item The driver SHOULD read \field{max_auth_key_len} to discover the
> maximum length of authenticated key the device supports.
> > +\end{itemize*}
> > +
> > +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device /
> Device Initialization}
> > +
> > +\drivernormative{\subsubsection}{Device Initialization}{Device Types /
> Crypto Device / Device Initialization}
> > +
> > +\begin{itemize*}
> > +\item The driver MUST identify and initialize the control virtqueue.
> 
> But does not have to identify and initialize any data virtqueues?
> 
Good catch. Both controlq and dataq.

> > +\item The driver MUST read the supported crypto services from bits of
> \field{crypto_services}.
> > +\item The driver MUST read the supported algorithms based on
> \field{crypto_services} field.
> > +\end{itemize*}
> > +
> > +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
> Crypto Device / Device Initialization}
> > +
> > +\begin{itemize*}
> > +\item The device MUST be configured with at least one accelerator which
> executes backend crypto operations.
> 
> What does configured mean here? Is this initialization requirement.
> 
It means the virtio crypto device MUST attach cryptodev backend. 
Yes, it's a requirement.

> > +\item The device MUST write the \field{crypto_services} field based on the
> capacities of the backend accelerator.
> > +\end{itemize*}
> > +
> 
> How do 'Initialization' and 'Configuration Layout' requirements relate to
> each-other.
> 
Basically they do the same things. But 'configuration layout' can't state the
specific time.
 
> > +\subsection{Device Operation}\label{sec:Device Types / Crypto Device /
> Device Operation}
> > +
> > +Packets can be transmitted by placing them in both the controlq and dataq.
> > +Packets consist of a general header and a service-specific request.
> 
> Are packets and requests synonyms?
> 
Yes, they are in virtio crypto spec.

> > +Where 'general header' is for all crypto requests, and 'service specific
> requests'
> 
> From below it seems you have two types of 'general header', but up until this
> point it seems there is a single definition. Of course, this does not
> really matter.
> 
I distinguish it based on controlq and dataq in following statement.

> > +are composed of operation parameter + output data + input data in general.
> > +Operation parameters are algorithm-specific parameters, output data is the
> > +data that should be utilized in operations, and input data is equal to
> > +"operation result + result data".
> > +
> > +The device can support both session mode (See \ref{sec:Device Types /
> Crypto Device / Device Operation / Control Virtqueue / Session operation}) and
> non-session mode, for example,
> > +As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is
> negotiated, the driver can use non-session mode for CIPHER service, otherwise
> it can only use session mode.
> 
> Grammar: Does not seem right to me. 'As' seems off and this could be two
> sentences.
> 
OK. Let me s/As/If the/ here.

> 
> > +
> > +\begin{note}
> > +The basic unit of all data length the byte.
> > +\end{note}
> > +
> > +The general header for controlq is as follows:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
> > +
> > +struct virtio_crypto_ctrl_header {
> > +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> 0x02)
> > +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> 0x03)
> > +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
> > +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
> > +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
> > +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
> > +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
> > +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> > +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
> > +    le32 opcode;
> > +    le32 algo;
> > +    le32 flag;
> > +    /* data virtqueue id */
> > +    le32 queue_id;
> > +};
> > +\end{lstlisting}
> > +
> > +The general header of dataq:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_op_header {
> > +#define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
> > +#define VIRTIO_CRYPTO_CIPHER_DECRYPT \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
> > +#define VIRTIO_CRYPTO_HASH \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
> > +#define VIRTIO_CRYPTO_MAC \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
> > +#define VIRTIO_CRYPTO_AEAD_ENCRYPT \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> > +#define VIRTIO_CRYPTO_AEAD_DECRYPT \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> > +    le32 opcode;
> > +    /* algo should be service-specific algorithms */
> > +    le32 algo;
> > +    /* session_id should be service-specific algorithms */
> > +    le64 session_id;
> > +#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
> > +#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2
> 
> Use _F_ istead of _FLAG_?
> 
I'm afraid they are confused with the feature bits.

> > +    /* control flag to control the request */
> > +    le32 flag;
> > +    le32 padding;
> > +};
> > +\end{lstlisting}
> > +
> > +The device can set the operation status as follows: VIRTIO_CRYPTO_OK:
> success;
> > +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP:
> not supported;
> > +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto
> operations.
> > +
> > +\begin{lstlisting}
> > +enum VIRITO_CRYPTO_STATUS {
> > +    VIRTIO_CRYPTO_OK = 0,
> > +    VIRTIO_CRYPTO_ERR = 1,
> > +    VIRTIO_CRYPTO_BADMSG = 2,
> > +    VIRTIO_CRYPTO_NOTSUPP = 3,
> > +    VIRTIO_CRYPTO_INVSESS = 4,
> > +    VIRTIO_CRYPTO_MAX
> > +};
> > +\end{lstlisting}
> > +
> > +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device /
> Device Operation / Control Virtqueue}
> > +
> > +The driver uses the control virtqueue to send control commands to the
> > +device which handles the non-data plane operations, such as session
> 
> What is a 'non-data plane'?
> 
I named controlling plane as the non-data plane. Maybe it's superfluous.

> > +operations (See \ref{sec:Device Types / Crypto Device / Device Operation /
> Control Virtqueue / Session operation}).
> 
> Reviewed up until here. Depending on how things evolve will try to
> review the rest too in the following days.
> 
I knew it's a hard and time-consuming work to review such long spec.

Great thanks for your reviewing. :)

> A question and a remark as a closing word:
> * Are there already some kernel and qemu patches for this 'non-session' stuff?

THB currently I have a patch for virtio_crypto.h. 

Attaching it for better review.

> * I think some proofreading (and eventually also touch-up) by a native speaker
> would really benefit us. Sadly my grammar skills are very questionable, so
> I can't help much. Nevertheless since it is a spec, I think we sould strive
> for high standards in language usage too.
> 
I agree and I actually asked a native speaker in my company to review it 
several months ago. When all comments are handled, I'll do it again.

Thanks,
-Gonglei


[-- Attachment #2: 0001-virtio-crypto-update-header-file.patch --]
[-- Type: application/octet-stream, Size: 6550 bytes --]

From f12f7836b240c868031c5e6a0b9cff377dc3c966 Mon Sep 17 00:00:00 2001
From: Gonglei <arei.gonglei@huawei.com>
Date: Fri, 20 Jan 2017 16:22:54 +0800
Subject: [PATCH] virtio-crypto: update header file

Update the header based on the newset virtio
crypto spec, so that the virtio crypto can
support both session and non-session based
crypto services and keep compatibility with
the pre-existing code.

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

diff --git a/include/uapi/linux/virtio_crypto.h b/include/uapi/linux/virtio_crypto.h
index 50cdc8a..f30389c 100644
--- a/include/uapi/linux/virtio_crypto.h
+++ b/include/uapi/linux/virtio_crypto.h
@@ -166,8 +166,8 @@ struct virtio_crypto_aead_session_para {
 	__le32 algo;
 	/* length of key */
 	__le32 key_len;
-	/* hash result length */
-	__le32 hash_result_len;
+	/* Authentication tag length */
+	__virtio32 tag_len;
 	/* length of the additional authenticated data (AAD) in bytes */
 	__le32 aad_len;
 	/* encrypt or decrypt, See above VIRTIO_CRYPTO_OP_* */
@@ -271,6 +271,8 @@ struct virtio_crypto_op_header {
 	__le32 algo;
 	/* session_id should be service-specific algorithms */
 	__le64 session_id;
+#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
+#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2
 	/* control flag to control the request */
 	__le32 flag;
 	__le32 padding;
@@ -403,6 +405,196 @@ struct virtio_crypto_op_data_req {
 	} u;
 };
 
+struct virtio_crypto_cipher_para_non_sess {
+	struct {
+		/* See VIRTIO_CRYPTO_CIPHER* above */
+		__virtio32 algo;
+		/* length of key */
+		__virtio32 keylen;
+
+		/* See VIRTIO_CRYPTO_OP_* above */
+		__virtio32 op;
+	} sess_para;
+
+	/*
+	 * Byte Length of valid IV/Counter
+	 */
+	__virtio32 iv_len;
+	/* length of source data */
+	__virtio32 src_data_len;
+	/* length of dst data */
+	__virtio32 dst_data_len;
+};
+
+struct virtio_crypto_alg_chain_data_para_non_sess {
+	struct {
+		/* See VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_* above */
+		__virtio32 alg_chain_order;
+		/* length of the additional authenticated data in bytes */
+		__virtio32 aad_len;
+
+		struct {
+			/* See VIRTIO_CRYPTO_CIPHER* above */
+			__virtio32 algo;
+			/* length of key */
+			__virtio32 keylen;
+			/* See VIRTIO_CRYPTO_OP_* above */
+			__virtio32 op;
+		} cipher;
+
+		struct {
+			/* See VIRTIO_CRYPTO_HASH_* or _MAC_* above */
+			__virtio32 algo;
+			/* length of authenticated key */
+			__virtio32 auth_key_len;
+			/* See VIRTIO_CRYPTO_SYM_HASH_MODE_* above */
+			__virtio32 hash_mode;
+		} hash;
+	} sess_para;
+
+	__virtio32 iv_len;
+	/* Length of source data */
+	__virtio32 src_data_len;
+	/* Length of destination data */
+	__virtio32 dst_data_len;
+	/* Starting point for cipher processing in source data */
+	__virtio32 cipher_start_src_offset;
+	/* Length of the source data that the cipher will be computed on */
+	__virtio32 len_to_cipher;
+	/* Starting point for hash processing in source data */
+	__virtio32 hash_start_src_offset;
+	/* Length of the source data that the hash will be computed on */
+	__virtio32 len_to_hash;
+	/* Length of the additional auth data */
+	__virtio32 aad_len;
+	/* Length of the hash result */
+	__virtio32 hash_result_len;
+	__virtio32 reserved;
+};
+
+struct virtio_crypto_hash_para_non_sess {
+	struct {
+		/* See VIRTIO_CRYPTO_HASH_* above */
+		__virtio32 algo;
+	} sess_para;
+
+	/* length of source data */
+	__virtio32 src_data_len;
+	/* hash result length */
+	__virtio32 hash_result_len;
+	__virtio32 reserved;
+};
+
+struct virtio_crypto_mac_para_non_sess {
+	struct {
+		/* See VIRTIO_CRYPTO_MAC_* above */
+		__virtio32 algo;
+		/* length of authenticated key */
+		__virtio32 auth_key_len;
+	} sess_para;
+
+	/* length of source data */
+	__virtio32 src_data_len;
+	/* hash result length */
+	__virtio32 hash_result_len;
+};
+
+struct virtio_crypto_aead_para_non_sess {
+	struct {
+		/* See VIRTIO_CRYPTO_AEAD_* above */
+		__virtio32 algo;
+		/* length of key */
+		__virtio32 key_len;
+		/* encrypt or decrypt, See above VIRTIO_CRYPTO_OP_* */
+		__virtio32 op;
+	} sess_para;
+
+	/*
+	 * Byte Length of valid IV data pointed to by the below iv_addr
+	 * parameter.
+	 */
+	__virtio32 iv_len;
+	/* Authentication tag length */
+	__virtio32 tag_len;
+	/* length of the additional authenticated data (AAD) in bytes */
+	__virtio32 aad_len;
+	/* length of source data */
+	__virtio32 src_data_len;
+	/* length of dst data, it should be at least src_data_len + tag_len */
+	__virtio32 dst_data_len;
+};
+
+struct virtio_crypto_cipher_data_req_non_sess {
+	/* Device-readable part */
+	struct virtio_crypto_cipher_para_non_sess para;
+	__u8 padding[48];
+};
+
+struct virtio_crypto_hash_data_req_non_sess {
+	/* Device-readable part */
+	struct virtio_crypto_hash_para_non_sess para;
+	__u8 padding[64];
+};
+
+struct virtio_crypto_mac_data_req_non_sess {
+	/* Device-readable part */
+	struct virtio_crypto_mac_para_non_sess para;
+	__u8 padding[64];
+};
+
+struct virtio_crypto_alg_chain_data_req_non_sess {
+	/* Device-readable part */
+	struct virtio_crypto_alg_chain_data_para_non_sess para;
+};
+
+struct virtio_crypto_sym_data_req_non_sess {
+	union {
+		struct virtio_crypto_cipher_data_req_non_sess cipher;
+		struct virtio_crypto_alg_chain_data_req_non_sess chain;
+		__u8 padding[72];
+	} u;
+
+	/* See above VIRTIO_CRYPTO_SYM_OP_* */
+	__le32 op_type;
+	__le32 padding;
+};
+
+struct virtio_crypto_aead_data_req_non_sess {
+	/* Device-readable part */
+	struct virtio_crypto_aead_para_non_sess para;
+	__u8 padding[48];
+};
+
+/* The request of the data virtqueue's packet */
+struct virtio_crypto_op_data_req_mux {
+	struct virtio_crypto_op_header header;
+
+	union {
+		struct {
+			struct virtio_crypto_sym_data_req  data;
+			__u8 padding[32];
+		} sym_req;
+		struct {
+			struct virtio_crypto_hash_data_req data;
+			__u8 padding[32];
+		} hash_req;
+		struct {
+			struct virtio_crypto_mac_data_req data;
+			__u8 padding[32];
+		} mac_req;
+		struct {
+			struct virtio_crypto_aead_data_req data;
+			__u8 padding[32];
+		} aead_req;
+
+		struct virtio_crypto_sym_data_req_non_sess  sym_non_sess_req;
+		struct virtio_crypto_hash_data_req_non_sess hash_non_sess_req;
+		struct virtio_crypto_mac_data_req_non_sess mac_non_sess_req;
+		struct virtio_crypto_aead_data_req_non_sess aead_non_sess_req;
+		__u8 padding[80];
+	} u;
+};
+
 #define VIRTIO_CRYPTO_OK        0
 #define VIRTIO_CRYPTO_ERR       1
 #define VIRTIO_CRYPTO_BADMSG    2
-- 
1.8.3.1

Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 02/08/2017 07:24 AM, Gonglei (Arei) wrote:
> Hi Halil,
> 
> Thanks for your comments firstly.
> 

You are welcome :). Sorry it took so long -- I'm currently
quite busy.

>>
>> On 01/18/2017 09:22 AM, Gonglei wrote:
>>> The virtio crypto device is a virtual crypto device (ie. hardware
>>> crypto accelerator card). Currently, the virtio crypto device provides
>>> the following crypto services: CIPHER, MAC, HASH, and AEAD.
>>>
>>> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
>>>
>>> VIRTIO-153
>>>
>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>> CC: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Lingli Deng <denglingli@chinamobile.com>
>>> CC: Jani Kokkonen <Jani.Kokkonen@huawei.com>
>>> CC: Ola Liljedahl <Ola.Liljedahl@arm.com>
>>> CC: Varun Sethi <Varun.Sethi@freescale.com>
>>> CC: Zeng Xin <xin.zeng@intel.com>
>>> CC: Keating Brian <brian.a.keating@intel.com>
>>> CC: Ma Liang J <liang.j.ma@intel.com>
>>> CC: Griffin John <john.griffin@intel.com>
>>> CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
>>> ---
>>>  content.tex       |    2 +
>>>  virtio-crypto.tex | 1245
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 1247 insertions(+)
>>>  create mode 100644 virtio-crypto.tex
>>>
>>> diff --git a/content.tex b/content.tex
>>> index 4b45678..ab75f78 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len},
>> \field{residual},
>>>  \field{status_qualifier}, \field{status}, \field{response} and
>>>  \field{sense} fields.
>>>
>>> +\input{virtio-crypto.tex}
>>> +
>>>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>>
>>>  Currently there are three device-independent feature bits defined:
>>> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
>>> new file mode 100644
>>> index 0000000..732cd30
>>> --- /dev/null
>>> +++ b/virtio-crypto.tex
>>> @@ -0,0 +1,1245 @@
>>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
>>> +
>>> +The virtio crypto device is a virtual cryptography device as well as a kind of
>>> +virtual hardware accelerator for virtual machines. The encryption and
>>> +decryption requests are placed in any of the data active queues and are
>> ultimately handled by the
>>
>> Am I the only one having a problem with 'data active queues'?
> 
> Maybe 'data queues' here is enough?
> 

I guess 'active' came from virtio-net. For virtio-crypto you do not define
active and passive queues, and my guess is there is no intention/reason
to do this in the future either. Thus yes 'data queues' is fine. And
'active data queues' would be grammatically correct, but substantially
confusing and wrong.

>> I have objected on this before.
>>
>>> +backend crypto accelerators. The second kind of queue is the control queue
>> used to create
>>> +or destroy sessions for symmetric algorithms and will control some
>> advanced
>>> +features in the future. The virtio crypto device provides the following crypto
>>> +services: CIPHER, MAC, HASH, and AEAD.
>>> +
>>> +
>>> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
>>> +
>>> +20
>>> +
>>> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device /
>> Virtqueues}
>>> +
>>> +\begin{description}
>>> +\item[0] dataq1
>>> +\item[\ldots]
>>> +\item[N-1] dataqN
>>> +\item[N] controlq
>>> +\end{description}
>>> +
>>> +N is set by \field{max_dataqueues}.
>>> +
>>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature
>> bits}
>>> +
>>> +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
>>> +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is
>> available for CIPHER service.
>>> +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is
>> available for HASH service.
>>> +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is
>> available for MAC service.
>>> +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is
>> available for AEAD service.
>>> +
>>
>> I'm not sure that "non-session" entirely correct grammatically. I would
>> prefer sessionless as alternatively proposed by Stefan, or even stateless.
>> I think stateless is the phrase most frequently used to describe what
>> we want to introduce -- that is basically response = f(request) and
>> not response = f(request, server_state) where the server_state is
>> a is determined by a series of previous interactions between the server
>> and the client).
>>
> Makes sense. I discussed with Xin about this on call meeting two weeks ago.
> I decide to use stateless mode instead of non-session mode here.
> 
>>> +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto
>> Device / Feature bits}
>>> +
>>> +Some crypto feature bits require other crypto feature bits
>>> +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}):
>>> +
>>> +\begin{description}
>>> +\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires
>> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
>>> +\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires
>> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
>>> +\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires
>> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
>>> +\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires
>> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
>>> +\end{description}
>>> +
>>> +\subsection{Device configuration layout}\label{sec:Device Types / Crypto
>> Device / Device configuration layout}
>>> +
>>> +The following driver-read-only configuration fields are defined:
>>> +
>>> +\begin{lstlisting}
>>> +struct virtio_crypto_config {
>>> +    le32 status;
>>> +    le32 max_dataqueues;
>>> +    le32 crypto_services;
>>> +    /* Detailed algorithms mask */
>>> +    le32 cipher_algo_l;
>>> +    le32 cipher_algo_h;
>>> +    le32 hash_algo;
>>> +    le32 mac_algo_l;
>>> +    le32 mac_algo_h;
>>> +    le32 aead_algo;
>>> +    /* Maximum length of cipher key */
>>> +    le32 max_cipher_key_len;
>>> +    /* Maximum length of authenticated key */
>>> +    le32 max_auth_key_len;
>>> +    le32 reserved;
>>> +    /* Maximum size of each crypto request's content */
>>> +    le64 max_size;
>>> +};
>>> +\end{lstlisting}
>>> +
>>> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
>> ~VIRTIO_CRYPTO_S_HW_READY.
>>
>> Not entirely happy with this. What you want to say is reserved
>> for future use, or? Would it make sense to have a general note
>> -- in a similar fashion like for 'sizes are in bytes' -- for
>> reserved for future use?
>>
>> One possible formulation would be:
>>
>> "In this specification, unless explicitly stated otherwise,
>> fields and bits reserved for future use shall be zeroed out.
>> Both the a device or a driver device and the driver should
>> detect violations of this rule, and deny the requested
>> operation in an appropriate way if possible."
>>
>>
> Cornelia also provided a good suggestion about this. :)
> 

Yeah.

>>> +
>>> +\begin{lstlisting}
>>> +#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
>>> +\end{lstlisting}
>>> +
>>> +The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the
>> hardware is ready to work or not.
>>
>> I do not like hardware here.
>>
> Sorry about that, but it has existed in both Qemu and Linux driver. :(
> 

This is a spec, that's an implementation. You could theoretically use
different names, but I would not go that far. I would keep the constant
name but reword the sentence stating the semantic.

You seem to use "hardware", "accelerator", "backend", "hardware-backend",
"backend accelerator", "backend crypto accelerator(s)" interchangeably -- as
if these had the same meaning. Moreover I do not remember seeing a (precise)
definition for any of these phrases. Or am I wrong?

AFAIK, while avoiding word repetition (by using synonyms for example) is
considered good style in general, technical writing and especially
specifications are a prominent exception. In specifications consistent wording
should be preferred over avoiding repetition, and IMHO this is
especially important that the concepts of the domain under specification
are referred to in a consistent way (e.g. called always the same).

Do you agree? If you do, could we consolidate the virtio crypto spec
in this sense?

>>> +
>>> +The following driver-read-only fields include \field{max_dataqueues}, which
>> specifies the
>>
>> Why following?
>>
> Er, I used 'following' at here and there... It's just an auxiliary word.
> 

Well, I wanted to say 'following' and 'include' together make this
sentence look very strange to me.

AFAIU your answer 'following' is meant to be kind of ornamental here.
I also think we should drop any elements (words, sentences) having
purely ornamental function.

>>> +maximum number of data virtqueues (dataq1\ldots dataqN), and
>> \field{crypto_services},
>>> +which indicates the crypto services the virtio crypto supports.
>>> +
>>> +The following services are defined:
>>> +
>>> +\begin{lstlisting}
>>> +/* CIPHER service */
>>> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
>>> +/* HASH service */
>>> +#define VIRTIO_CRYPTO_SERVICE_HASH   1
>>> +/* MAC (Message Authentication Codes) service */
>>> +#define VIRTIO_CRYPTO_SERVICE_MAC    2
>>> +/* AEAD (Authenticated Encryption with Associated Data) service */
>>> +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
>>> +\end{lstlisting}
>>> +
>>> +The last driver-read-only fields specify detailed algorithms masks
>>> +the device offers for corresponding services. The following CIPHER
>> algorithms
>>> +are defined:
>>
>> You do not establish an explicit relationship between the fields and the
>> macros for the flags. These are flags or? It seems quite common in the
>> spec to use _F_ in flag names. Would it be appropriate here?
>>
>>> +
>>> +\begin{lstlisting}
>>> +#define VIRTIO_CRYPTO_NO_CIPHER                 0
>>> +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
>>> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
>>> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
>>> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
>>> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
>>> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
>>> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
>>> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
>>> +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
>>> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
>>> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
>>> +\end{lstlisting}
>>> +
>>> +The following HASH algorithms are defined:
>>> +
>>> +\begin{lstlisting}
>>> +#define VIRTIO_CRYPTO_NO_HASH            0
>>> +#define VIRTIO_CRYPTO_HASH_MD5           1
>>> +#define VIRTIO_CRYPTO_HASH_SHA1          2
>>> +#define VIRTIO_CRYPTO_HASH_SHA_224       3
>>> +#define VIRTIO_CRYPTO_HASH_SHA_256       4
>>> +#define VIRTIO_CRYPTO_HASH_SHA_384       5
>>> +#define VIRTIO_CRYPTO_HASH_SHA_512       6
>>> +#define VIRTIO_CRYPTO_HASH_SHA3_224      7
>>> +#define VIRTIO_CRYPTO_HASH_SHA3_256      8
>>> +#define VIRTIO_CRYPTO_HASH_SHA3_384      9
>>> +#define VIRTIO_CRYPTO_HASH_SHA3_512      10
>>> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128      11
>>> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256      12
>>> +\end{lstlisting}
>>> +
>>> +The following MAC algorithms are defined:
>>> +
>>> +\begin{lstlisting}
>>> +#define VIRTIO_CRYPTO_NO_MAC                       0
>>> +#define VIRTIO_CRYPTO_MAC_HMAC_MD5                 1
>>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1                2
>>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224             3
>>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256             4
>>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384             5
>>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512             6
>>> +#define VIRTIO_CRYPTO_MAC_CMAC_3DES                25
>>> +#define VIRTIO_CRYPTO_MAC_CMAC_AES                 26
>>> +#define VIRTIO_CRYPTO_MAC_KASUMI_F9                27
>>> +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2              28
>>> +#define VIRTIO_CRYPTO_MAC_GMAC_AES                 41
>>> +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             42
>>> +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               49
>>> +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         50
>>> +#define VIRTIO_CRYPTO_MAC_XCBC_AES                 53
>>> +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3                 54
>>> +\end{lstlisting}
>>> +
>>> +The following AEAD algorithms are defined:
>>> +
>>> +\begin{lstlisting}
>>> +#define VIRTIO_CRYPTO_NO_AEAD     0
>>> +#define VIRTIO_CRYPTO_AEAD_GCM    1
>>> +#define VIRTIO_CRYPTO_AEAD_CCM    2
>>> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
>>> +\end{lstlisting}
>>> +
>>
>> Would it make sense to interleave the flag definition
>> with the struct virtio_crypto_config?
>>
>>> +\begin{note}
>>> +Any other values are reserved for future use.
>>
>> Are these flags or values? I do not think values is appropriate here.
>>
> Please refer to my reply in another thread.
> 

Will answer there.

>>> +\end{note}
>>> +
>>
>> Some fields are missing here. This is inconsistent. Either
>> you should describe all or none (here).
>>
> Ok, will add other fields.
> 

Thx.

>>> +\devicenormative{\subsubsection}{Device configuration layout}{Device Types
>> / Crypto Device / Device configuration layout}
>>> +
>>> +\begin{itemize*}
>>> +\item The device MUST set \field{max_dataqueues} to between 1 and 65535
>> inclusive.
>>> +\item The device MUST set \field{status} based on the status of the
>> hardware-backed implementation.
>>
>> What is a hardware-backend?
>>
> I meant the cryptodev backend, cryptodev-builtin, cryptodev-vhost-user for example.
> 

See above. This was a rhetorical question. Many of my question
are rhetorical, and are meant to be a gentle indicator of 
concern -- in opposition to saying something is straight-out
incomprehensible or wrong. 

I understood what hardware-backend means because I'm familiar with your
Linux/QEMU implementation. Please keep in mind, this specification
should make clean-room implementations possible (I mean, with
no looking at the linux kernel, or qemu code or whatherver).

>>> +\item The device MUST accept and handle requests after \field{status} is set
>> to VIRTIO_CRYPTO_S_HW_READY.
>>
>> Shouldn't this be the other way around (if VIRTIO_CRYPTO_S_HW_READY

Sorry, of course I have meant if ~VIRTIO_CRYPTO_S_HW_READY then reject!

>> then reject). Is a not well formed request or a backend failure considered?
>> What does handle mean? What should happen if requests are submitted
>> before VIRTIO_CRYPTO_S_HW_READY is set?
>>
> The requests MUST not be transmitted before VIRTIO_CRYPTO_S_HW_READY is set.
> Which is stated by the driver requirements. Otherwise the requests will be dropped.

This ('Otherwise the requests will be dropped.') ain't specified or?
Should not we specify this? How exactly are they 'dropped'?

> 
> The handle means execute crypto operations.
> 

But things can fail, you have a status (VIRTIO_CRYPT_(ERR|NOTSUPP|BADMSG))
for that. If that happens, does the device violate this point?

Do (all) other virtio devices have a similar conformance statement on when
they 'MUST accept and handle' requests or packets? If not what do you think,
should they?

>>> +\item The device MUST set \field{crypto_services} based on the crypto
>> services the device offers.
>>> +\item The device MUST set detailed algorithms masks based on the
>> \field{crypto_services} field.
>>> +\item The device MUST set \field{max_size} to show the maximum size of
>> crypto request the device supports.
>>> +\item The device MUST set \field{max_cipher_key_len} to show the
>> maximum length of cipher key if the device supports CIPHER service.
>>> +\item The device MUST set \field{max_auth_key_len} to show the maximum
>> length of authenticated key if the device supports MAC service.
>>> +\end{itemize*}
>>> +
>>> +\drivernormative{\subsubsection}{Device configuration layout}{Device Types
>> / Crypto Device / Device configuration layout}
>>> +
>>> +\begin{itemize*}
>>> +\item The driver MUST read the ready \field{status} from the bottom bit of
>> status to check whether the hardware-backed
>>> +      implementation is ready or not, and the driver MUST reread it after
>> the device reset.
>>> +\item The driver MUST NOT transmit any packets to the device if the ready
>> \field{status} is not set.
>>> +\item The driver MUST read \field{max_dataqueues} field to discover the
>> number of data queues the device supports.
>>> +\item The driver MUST read \field{crypto_services} field to discover which
>> services the device is able to offer.
>>> +\item The driver MUST read the detailed algorithms fields based on
>> \field{crypto_services} field.
>>> +\item The driver SHOULD read \field{max_size} to discover the maximum
>> size of crypto request the device supports.
>>> +\item The driver SHOULD read \field{max_cipher_key_len} to discover the
>> maximum length of cipher key the device supports.
>>> +\item The driver SHOULD read \field{max_auth_key_len} to discover the
>> maximum length of authenticated key the device supports.
>>> +\end{itemize*}
>>> +
>>> +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device /
>> Device Initialization}
>>> +
>>> +\drivernormative{\subsubsection}{Device Initialization}{Device Types /
>> Crypto Device / Device Initialization}
>>> +
>>> +\begin{itemize*}
>>> +\item The driver MUST identify and initialize the control virtqueue.
>>
>> But does not have to identify and initialize any data virtqueues?
>>
> Good catch. Both controlq and dataq.
> 
>>> +\item The driver MUST read the supported crypto services from bits of
>> \field{crypto_services}.
>>> +\item The driver MUST read the supported algorithms based on
>> \field{crypto_services} field.
>>> +\end{itemize*}
>>> +
>>> +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
>> Crypto Device / Device Initialization}
>>> +
>>> +\begin{itemize*}
>>> +\item The device MUST be configured with at least one accelerator which
>> executes backend crypto operations.
>>
>> What does configured mean here? Is this initialization requirement.
>>
> It means the virtio crypto device MUST attach cryptodev backend. 
> Yes, it's a requirement.

What happens if an implementation fails to fulfill this requirement?
What is the benefit of having this requirement?

> 
>>> +\item The device MUST write the \field{crypto_services} field based on the
>> capacities of the backend accelerator.
>>> +\end{itemize*}
>>> +
>>
>> How do 'Initialization' and 'Configuration Layout' requirements relate to
>> each-other.
>>
> Basically they do the same things. But 'configuration layout' can't state the
> specific time.
>
>>> +\subsection{Device Operation}\label{sec:Device Types / Crypto Device /
>> Device Operation}
>>> +
>>> +Packets can be transmitted by placing them in both the controlq and dataq.
>>> +Packets consist of a general header and a service-specific request.
>>
>> Are packets and requests synonyms?
>>
> Yes, they are in virtio crypto spec.
> 

See above regarding consistent naming.

>>> +Where 'general header' is for all crypto requests, and 'service specific
>> requests'
>>
>> From below it seems you have two types of 'general header', but up until this
>> point it seems there is a single definition. Of course, this does not
>> really matter.
>>
> I distinguish it based on controlq and dataq in following statement.
> 

How about 'request header' or 'queue header'? Or something like 
'Packets consist of a queue-type specific header specifying among
others the operation, and an operation specific payload.'

I do not like 'general' here.

>>> +are composed of operation parameter + output data + input data in general.
>>> +Operation parameters are algorithm-specific parameters, output data is the
>>> +data that should be utilized in operations, and input data is equal to
>>> +"operation result + result data".
>>> +
>>> +The device can support both session mode (See \ref{sec:Device Types /
>> Crypto Device / Device Operation / Control Virtqueue / Session operation}) and
>> non-session mode, for example,
>>> +As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is
>> negotiated, the driver can use non-session mode for CIPHER service, otherwise
>> it can only use session mode.
>>
>> Grammar: Does not seem right to me. 'As' seems off and this could be two
>> sentences.
>>
> OK. Let me s/As/If the/ here.
> 

Capital won't do due to the 'for example, ' in the previous line.
Otherwise OK.

>>
>>> +
>>> +\begin{note}
>>> +The basic unit of all data length the byte.
>>> +\end{note}
>>> +
>>> +The general header for controlq is as follows:
>>> +
>>> +\begin{lstlisting}
>>> +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
>>> +
>>> +struct virtio_crypto_ctrl_header {
>>> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
>>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
>> 0x02)
>>> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
>>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
>> 0x03)
>>> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
>>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
>>> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
>>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
>>> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
>>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
>>> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
>>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
>>> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
>>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
>>> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
>>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
>>> +    le32 opcode;
>>> +    le32 algo;
>>> +    le32 flag;
>>> +    /* data virtqueue id */
>>> +    le32 queue_id;
>>> +};
>>> +\end{lstlisting}
>>> +
>>> +The general header of dataq:
>>> +
>>> +\begin{lstlisting}
>>> +struct virtio_crypto_op_header {
>>> +#define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
>>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
>>> +#define VIRTIO_CRYPTO_CIPHER_DECRYPT \
>>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
>>> +#define VIRTIO_CRYPTO_HASH \
>>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
>>> +#define VIRTIO_CRYPTO_MAC \
>>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
>>> +#define VIRTIO_CRYPTO_AEAD_ENCRYPT \
>>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
>>> +#define VIRTIO_CRYPTO_AEAD_DECRYPT \
>>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
>>> +    le32 opcode;
>>> +    /* algo should be service-specific algorithms */
>>> +    le32 algo;
>>> +    /* session_id should be service-specific algorithms */
>>> +    le64 session_id;
>>> +#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
>>> +#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2
>>
>> Use _F_ istead of _FLAG_?
>>
> I'm afraid they are confused with the feature bits.
> 

Have a look at the desc flags (for example VIRTQ_DESC_F_INDIRECT).

It is your call in the end.

>>> +    /* control flag to control the request */
>>> +    le32 flag;
>>> +    le32 padding;
>>> +};
>>> +\end{lstlisting}
>>> +
>>> +The device can set the operation status as follows: VIRTIO_CRYPTO_OK:
>> success;
>>> +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP:
>> not supported;
>>> +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto
>> operations.
>>> +
>>> +\begin{lstlisting}
>>> +enum VIRITO_CRYPTO_STATUS {
>>> +    VIRTIO_CRYPTO_OK = 0,
>>> +    VIRTIO_CRYPTO_ERR = 1,
>>> +    VIRTIO_CRYPTO_BADMSG = 2,
>>> +    VIRTIO_CRYPTO_NOTSUPP = 3,
>>> +    VIRTIO_CRYPTO_INVSESS = 4,
>>> +    VIRTIO_CRYPTO_MAX
>>> +};
>>> +\end{lstlisting}
>>> +
>>> +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device /
>> Device Operation / Control Virtqueue}
>>> +
>>> +The driver uses the control virtqueue to send control commands to the
>>> +device which handles the non-data plane operations, such as session
>>
>> What is a 'non-data plane'?
>>
> I named controlling plane as the non-data plane. Maybe it's superfluous.
> 

Again consisten naming. I would prefer avoiding introducing this extra
name.

>>> +operations (See \ref{sec:Device Types / Crypto Device / Device Operation /
>> Control Virtqueue / Session operation}).
>>
>> Reviewed up until here. Depending on how things evolve will try to
>> review the rest too in the following days.
>>
> I knew it's a hard and time-consuming work to review such long spec.
> 
> Great thanks for your reviewing. :)
> 

I know, its annoying to deal with my comments ;). IMHO as spec
without a certain quantity of rigor is not really a spec. I think
it's very hard to write a good spec, so be patient.

>> A question and a remark as a closing word:
>> * Are there already some kernel and qemu patches for this 'non-session' stuff?
> 
> THB currently I have a patch for virtio_crypto.h. 
> 
> Attaching it for better review.
> 

I would prefer having an implementation of the new features on the list,
before voting on the spec itself.

>> * I think some proofreading (and eventually also touch-up) by a native speaker
>> would really benefit us. Sadly my grammar skills are very questionable, so
>> I can't help much. Nevertheless since it is a spec, I think we sould strive
>> for high standards in language usage too.
>>
> I agree and I actually asked a native speaker in my company to review it 
> several months ago. When all comments are handled, I'll do it again.

Thats cool.

Regards,
Halil

> 
> Thanks,
> -Gonglei
> 

Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

Hi,

> 
> 
> On 02/08/2017 07:24 AM, Gonglei (Arei) wrote:
> > Hi Halil,
> >
> > Thanks for your comments firstly.
> >
> 
> You are welcome :). Sorry it took so long -- I'm currently
> quite busy.
> 
It's fine. 

> >>
> >> On 01/18/2017 09:22 AM, Gonglei wrote:
> >>> The virtio crypto device is a virtual crypto device (ie. hardware
> >>> crypto accelerator card). Currently, the virtio crypto device provides
> >>> the following crypto services: CIPHER, MAC, HASH, and AEAD.
> >>>
> >>> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
> >>>
> >>> VIRTIO-153
> >>>
> >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >>> CC: Michael S. Tsirkin <mst@redhat.com>
> >>> CC: Cornelia Huck <cornelia.huck@de.ibm.com>
> >>> CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>> CC: Lingli Deng <denglingli@chinamobile.com>
> >>> CC: Jani Kokkonen <Jani.Kokkonen@huawei.com>
> >>> CC: Ola Liljedahl <Ola.Liljedahl@arm.com>
> >>> CC: Varun Sethi <Varun.Sethi@freescale.com>
> >>> CC: Zeng Xin <xin.zeng@intel.com>
> >>> CC: Keating Brian <brian.a.keating@intel.com>
> >>> CC: Ma Liang J <liang.j.ma@intel.com>
> >>> CC: Griffin John <john.griffin@intel.com>
> >>> CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> >>> ---
> >>>  content.tex       |    2 +
> >>>  virtio-crypto.tex | 1245
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 1247 insertions(+)
> >>>  create mode 100644 virtio-crypto.tex
> >>>
> >>> diff --git a/content.tex b/content.tex
> >>> index 4b45678..ab75f78 100644
> >>> --- a/content.tex
> >>> +++ b/content.tex
> >>> @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len},
> >> \field{residual},
> >>>  \field{status_qualifier}, \field{status}, \field{response} and
> >>>  \field{sense} fields.
> >>>
> >>> +\input{virtio-crypto.tex}
> >>> +
> >>>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >>>
> >>>  Currently there are three device-independent feature bits defined:
> >>> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> >>> new file mode 100644
> >>> index 0000000..732cd30
> >>> --- /dev/null
> >>> +++ b/virtio-crypto.tex
> >>> @@ -0,0 +1,1245 @@
> >>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> >>> +
> >>> +The virtio crypto device is a virtual cryptography device as well as a kind of
> >>> +virtual hardware accelerator for virtual machines. The encryption and
> >>> +decryption requests are placed in any of the data active queues and are
> >> ultimately handled by the
> >>
> >> Am I the only one having a problem with 'data active queues'?
> >
> > Maybe 'data queues' here is enough?
> >
> 
> I guess 'active' came from virtio-net. For virtio-crypto you do not define
> active and passive queues, and my guess is there is no intention/reason
> to do this in the future either. Thus yes 'data queues' is fine. And
> 'active data queues' would be grammatically correct, but substantially
> confusing and wrong.
> 
OK.

> >> I have objected on this before.
> >>
> >>> +backend crypto accelerators. The second kind of queue is the control
> queue
> >> used to create
> >>> +or destroy sessions for symmetric algorithms and will control some
> >> advanced
> >>> +features in the future. The virtio crypto device provides the following
> crypto
> >>> +services: CIPHER, MAC, HASH, and AEAD.
> >>> +
> >>> +
> >>> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device
> ID}
> >>> +
> >>> +20
> >>> +
> >>> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device /
> >> Virtqueues}
> >>> +
> >>> +\begin{description}
> >>> +\item[0] dataq1
> >>> +\item[\ldots]
> >>> +\item[N-1] dataqN
> >>> +\item[N] controlq
> >>> +\end{description}
> >>> +
> >>> +N is set by \field{max_dataqueues}.
> >>> +
> >>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device /
> Feature
> >> bits}
> >>> +
> >>> +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is
> available.
> >>> +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode
> is
> >> available for CIPHER service.
> >>> +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is
> >> available for HASH service.
> >>> +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is
> >> available for MAC service.
> >>> +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is
> >> available for AEAD service.
> >>> +
> >>
> >> I'm not sure that "non-session" entirely correct grammatically. I would
> >> prefer sessionless as alternatively proposed by Stefan, or even stateless.
> >> I think stateless is the phrase most frequently used to describe what
> >> we want to introduce -- that is basically response = f(request) and
> >> not response = f(request, server_state) where the server_state is
> >> a is determined by a series of previous interactions between the server
> >> and the client).
> >>
> > Makes sense. I discussed with Xin about this on call meeting two weeks ago.
> > I decide to use stateless mode instead of non-session mode here.
> >
> >>> +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto
> >> Device / Feature bits}
> >>> +
> >>> +Some crypto feature bits require other crypto feature bits
> >>> +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature
> Bits}):
> >>> +
> >>> +\begin{description}
> >>> +\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires
> >> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> >>> +\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires
> >> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> >>> +\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires
> >> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> >>> +\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires
> >> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> >>> +\end{description}
> >>> +
> >>> +\subsection{Device configuration layout}\label{sec:Device Types / Crypto
> >> Device / Device configuration layout}
> >>> +
> >>> +The following driver-read-only configuration fields are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +struct virtio_crypto_config {
> >>> +    le32 status;
> >>> +    le32 max_dataqueues;
> >>> +    le32 crypto_services;
> >>> +    /* Detailed algorithms mask */
> >>> +    le32 cipher_algo_l;
> >>> +    le32 cipher_algo_h;
> >>> +    le32 hash_algo;
> >>> +    le32 mac_algo_l;
> >>> +    le32 mac_algo_h;
> >>> +    le32 aead_algo;
> >>> +    /* Maximum length of cipher key */
> >>> +    le32 max_cipher_key_len;
> >>> +    /* Maximum length of authenticated key */
> >>> +    le32 max_auth_key_len;
> >>> +    le32 reserved;
> >>> +    /* Maximum size of each crypto request's content */
> >>> +    le64 max_size;
> >>> +};
> >>> +\end{lstlisting}
> >>> +
> >>> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
> >> ~VIRTIO_CRYPTO_S_HW_READY.
> >>
> >> Not entirely happy with this. What you want to say is reserved
> >> for future use, or? Would it make sense to have a general note
> >> -- in a similar fashion like for 'sizes are in bytes' -- for
> >> reserved for future use?
> >>
> >> One possible formulation would be:
> >>
> >> "In this specification, unless explicitly stated otherwise,
> >> fields and bits reserved for future use shall be zeroed out.
> >> Both the a device or a driver device and the driver should
> >> detect violations of this rule, and deny the requested
> >> operation in an appropriate way if possible."
> >>
> >>
> > Cornelia also provided a good suggestion about this. :)
> >
> 
> Yeah.
> 
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
> >>> +\end{lstlisting}
> >>> +
> >>> +The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the
> >> hardware is ready to work or not.
> >>
> >> I do not like hardware here.
> >>
> > Sorry about that, but it has existed in both Qemu and Linux driver. :(
> >
> 
> This is a spec, that's an implementation. You could theoretically use
> different names, but I would not go that far. I would keep the constant
> name but reword the sentence stating the semantic.
> 
> You seem to use "hardware", "accelerator", "backend", "hardware-backend",
> "backend accelerator", "backend crypto accelerator(s)" interchangeably -- as
> if these had the same meaning. Moreover I do not remember seeing a (precise)
> definition for any of these phrases. Or am I wrong?
> 
> AFAIK, while avoiding word repetition (by using synonyms for example) is
> considered good style in general, technical writing and especially
> specifications are a prominent exception. In specifications consistent wording
> should be preferred over avoiding repetition, and IMHO this is
> especially important that the concepts of the domain under specification
> are referred to in a consistent way (e.g. called always the same).
> 
> Do you agree? If you do, could we consolidate the virtio crypto spec
> in this sense?
> 
Yes, I do. I'll try my best to perfect the spec.

> >>> +
> >>> +The following driver-read-only fields include \field{max_dataqueues},
> which
> >> specifies the
> >>
> >> Why following?
> >>
> > Er, I used 'following' at here and there... It's just an auxiliary word.
> >
> 
> Well, I wanted to say 'following' and 'include' together make this
> sentence look very strange to me.
> 
OK. Will fix them.

> AFAIU your answer 'following' is meant to be kind of ornamental here.
> I also think we should drop any elements (words, sentences) having
> purely ornamental function.
> 
> >>> +maximum number of data virtqueues (dataq1\ldots dataqN), and
> >> \field{crypto_services},
> >>> +which indicates the crypto services the virtio crypto supports.
> >>> +
> >>> +The following services are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +/* CIPHER service */
> >>> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> >>> +/* HASH service */
> >>> +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> >>> +/* MAC (Message Authentication Codes) service */
> >>> +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> >>> +/* AEAD (Authenticated Encryption with Associated Data) service */
> >>> +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> >>> +\end{lstlisting}
> >>> +
> >>> +The last driver-read-only fields specify detailed algorithms masks
> >>> +the device offers for corresponding services. The following CIPHER
> >> algorithms
> >>> +are defined:
> >>
> >> You do not establish an explicit relationship between the fields and the
> >> macros for the flags. These are flags or? It seems quite common in the
> >> spec to use _F_ in flag names. Would it be appropriate here?
> >>
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_NO_CIPHER                 0
> >>> +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
> >>> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
> >>> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
> >>> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
> >>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
> >>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
> >>> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
> >>> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
> >>> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
> >>> +\end{lstlisting}
> >>> +
> >>> +The following HASH algorithms are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_NO_HASH            0
> >>> +#define VIRTIO_CRYPTO_HASH_MD5           1
> >>> +#define VIRTIO_CRYPTO_HASH_SHA1          2
> >>> +#define VIRTIO_CRYPTO_HASH_SHA_224       3
> >>> +#define VIRTIO_CRYPTO_HASH_SHA_256       4
> >>> +#define VIRTIO_CRYPTO_HASH_SHA_384       5
> >>> +#define VIRTIO_CRYPTO_HASH_SHA_512       6
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_224      7
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_256      8
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_384      9
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_512      10
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128      11
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256      12
> >>> +\end{lstlisting}
> >>> +
> >>> +The following MAC algorithms are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_NO_MAC                       0
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_MD5                 1
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1                2
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224             3
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256             4
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384             5
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512             6
> >>> +#define VIRTIO_CRYPTO_MAC_CMAC_3DES                25
> >>> +#define VIRTIO_CRYPTO_MAC_CMAC_AES                 26
> >>> +#define VIRTIO_CRYPTO_MAC_KASUMI_F9                27
> >>> +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2              28
> >>> +#define VIRTIO_CRYPTO_MAC_GMAC_AES                 41
> >>> +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             42
> >>> +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               49
> >>> +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         50
> >>> +#define VIRTIO_CRYPTO_MAC_XCBC_AES                 53
> >>> +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3                 54
> >>> +\end{lstlisting}
> >>> +
> >>> +The following AEAD algorithms are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_NO_AEAD     0
> >>> +#define VIRTIO_CRYPTO_AEAD_GCM    1
> >>> +#define VIRTIO_CRYPTO_AEAD_CCM    2
> >>> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
> >>> +\end{lstlisting}
> >>> +
> >>
> >> Would it make sense to interleave the flag definition
> >> with the struct virtio_crypto_config?
> >>
> >>> +\begin{note}
> >>> +Any other values are reserved for future use.
> >>
> >> Are these flags or values? I do not think values is appropriate here.
> >>
> > Please refer to my reply in another thread.
> >
> 
> Will answer there.
> 
> >>> +\end{note}
> >>> +
> >>
> >> Some fields are missing here. This is inconsistent. Either
> >> you should describe all or none (here).
> >>
> > Ok, will add other fields.
> >
> 
> Thx.
> 
> >>> +\devicenormative{\subsubsection}{Device configuration layout}{Device
> Types
> >> / Crypto Device / Device configuration layout}
> >>> +
> >>> +\begin{itemize*}
> >>> +\item The device MUST set \field{max_dataqueues} to between 1 and
> 65535
> >> inclusive.
> >>> +\item The device MUST set \field{status} based on the status of the
> >> hardware-backed implementation.
> >>
> >> What is a hardware-backend?
> >>
> > I meant the cryptodev backend, cryptodev-builtin, cryptodev-vhost-user for
> example.
> >
> 
> See above. This was a rhetorical question. Many of my question
> are rhetorical, and are meant to be a gentle indicator of
> concern -- in opposition to saying something is straight-out
> incomprehensible or wrong.
> 
Sorry for misunderstanding. :(

> I understood what hardware-backend means because I'm familiar with your
> Linux/QEMU implementation. Please keep in mind, this specification
> should make clean-room implementations possible (I mean, with
> no looking at the linux kernel, or qemu code or whatherver).
> 
You are right.

> >>> +\item The device MUST accept and handle requests after \field{status} is
> set
> >> to VIRTIO_CRYPTO_S_HW_READY.
> >>
> >> Shouldn't this be the other way around (if VIRTIO_CRYPTO_S_HW_READY
> 
> Sorry, of course I have meant if ~VIRTIO_CRYPTO_S_HW_READY then reject!
> 
> >> then reject). Is a not well formed request or a backend failure considered?
> >> What does handle mean? What should happen if requests are submitted
> >> before VIRTIO_CRYPTO_S_HW_READY is set?
> >>
> > The requests MUST not be transmitted before
> VIRTIO_CRYPTO_S_HW_READY is set.
> > Which is stated by the driver requirements. Otherwise the requests will be
> dropped.
> 
> This ('Otherwise the requests will be dropped.') ain't specified or?
> Should not we specify this? How exactly are they 'dropped'?
> 
Hmm, we should add those explanation.

> >
> > The handle means execute crypto operations.
> >
> 
> But things can fail, you have a status
> (VIRTIO_CRYPT_(ERR|NOTSUPP|BADMSG))
> for that. If that happens, does the device violate this point?
> 
> Do (all) other virtio devices have a similar conformance statement on when
> they 'MUST accept and handle' requests or packets? If not what do you think,
> should they?
> 
Take virtio-net as an example, there is only one requirement about VIRTIO_NET_S_LINK_UP:

Section 5.1.5
6. If the VIRTIO_NET_F_STATUS feature bit is negotiated, the link status comes from the bottom bit of
status. Otherwise, the driver assumes it's active.

It didn't tell us what will be happen if the link isn't active. And what will be happen if the driver still
transmit packets to the device if the link isn't active.

So I think the potential mean is the driver MUST not transmit packets if the link isn't active.


> >>> +\item The device MUST set \field{crypto_services} based on the crypto
> >> services the device offers.
> >>> +\item The device MUST set detailed algorithms masks based on the
> >> \field{crypto_services} field.
> >>> +\item The device MUST set \field{max_size} to show the maximum size of
> >> crypto request the device supports.
> >>> +\item The device MUST set \field{max_cipher_key_len} to show the
> >> maximum length of cipher key if the device supports CIPHER service.
> >>> +\item The device MUST set \field{max_auth_key_len} to show the
> maximum
> >> length of authenticated key if the device supports MAC service.
> >>> +\end{itemize*}
> >>> +
> >>> +\drivernormative{\subsubsection}{Device configuration layout}{Device
> Types
> >> / Crypto Device / Device configuration layout}
> >>> +
> >>> +\begin{itemize*}
> >>> +\item The driver MUST read the ready \field{status} from the bottom bit
> of
> >> status to check whether the hardware-backed
> >>> +      implementation is ready or not, and the driver MUST reread it
> after
> >> the device reset.
> >>> +\item The driver MUST NOT transmit any packets to the device if the
> ready
> >> \field{status} is not set.
> >>> +\item The driver MUST read \field{max_dataqueues} field to discover the
> >> number of data queues the device supports.
> >>> +\item The driver MUST read \field{crypto_services} field to discover which
> >> services the device is able to offer.
> >>> +\item The driver MUST read the detailed algorithms fields based on
> >> \field{crypto_services} field.
> >>> +\item The driver SHOULD read \field{max_size} to discover the maximum
> >> size of crypto request the device supports.
> >>> +\item The driver SHOULD read \field{max_cipher_key_len} to discover the
> >> maximum length of cipher key the device supports.
> >>> +\item The driver SHOULD read \field{max_auth_key_len} to discover the
> >> maximum length of authenticated key the device supports.
> >>> +\end{itemize*}
> >>> +
> >>> +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device /
> >> Device Initialization}
> >>> +
> >>> +\drivernormative{\subsubsection}{Device Initialization}{Device Types /
> >> Crypto Device / Device Initialization}
> >>> +
> >>> +\begin{itemize*}
> >>> +\item The driver MUST identify and initialize the control virtqueue.
> >>
> >> But does not have to identify and initialize any data virtqueues?
> >>
> > Good catch. Both controlq and dataq.
> >
> >>> +\item The driver MUST read the supported crypto services from bits of
> >> \field{crypto_services}.
> >>> +\item The driver MUST read the supported algorithms based on
> >> \field{crypto_services} field.
> >>> +\end{itemize*}
> >>> +
> >>> +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
> >> Crypto Device / Device Initialization}
> >>> +
> >>> +\begin{itemize*}
> >>> +\item The device MUST be configured with at least one accelerator which
> >> executes backend crypto operations.
> >>
> >> What does configured mean here? Is this initialization requirement.
> >>
> > It means the virtio crypto device MUST attach cryptodev backend.
> > Yes, it's a requirement.
> 
> What happens if an implementation fails to fulfill this requirement?
> What is the benefit of having this requirement?
> 
Let me drop it. We only care about the 'status' field. if an implementation
fails to fulfill this requirement the VIRTIO_CRYPTO_S_HW_READY bit doesn't be set.

> >
> >>> +\item The device MUST write the \field{crypto_services} field based on the
> >> capacities of the backend accelerator.
> >>> +\end{itemize*}
> >>> +
> >>
> >> How do 'Initialization' and 'Configuration Layout' requirements relate to
> >> each-other.
> >>
> > Basically they do the same things. But 'configuration layout' can't state the
> > specific time.
> >
> >>> +\subsection{Device Operation}\label{sec:Device Types / Crypto Device /
> >> Device Operation}
> >>> +
> >>> +Packets can be transmitted by placing them in both the controlq and
> dataq.
> >>> +Packets consist of a general header and a service-specific request.
> >>
> >> Are packets and requests synonyms?
> >>
> > Yes, they are in virtio crypto spec.
> >
> 
> See above regarding consistent naming.
> 
> >>> +Where 'general header' is for all crypto requests, and 'service specific
> >> requests'
> >>
> >> From below it seems you have two types of 'general header', but up until
> this
> >> point it seems there is a single definition. Of course, this does not
> >> really matter.
> >>
> > I distinguish it based on controlq and dataq in following statement.
> >
> 
> How about 'request header' or 'queue header'? Or something like
> 'Packets consist of a queue-type specific header specifying among
> others the operation, and an operation specific payload.'
> 
Sounds good.

> I do not like 'general' here.
> 
> >>> +are composed of operation parameter + output data + input data in
> general.
> >>> +Operation parameters are algorithm-specific parameters, output data is
> the
> >>> +data that should be utilized in operations, and input data is equal to
> >>> +"operation result + result data".
> >>> +
> >>> +The device can support both session mode (See \ref{sec:Device Types /
> >> Crypto Device / Device Operation / Control Virtqueue / Session operation})
> and
> >> non-session mode, for example,
> >>> +As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is
> >> negotiated, the driver can use non-session mode for CIPHER service,
> otherwise
> >> it can only use session mode.
> >>
> >> Grammar: Does not seem right to me. 'As' seems off and this could be two
> >> sentences.
> >>
> > OK. Let me s/As/If the/ here.
> >
> 
> Capital won't do due to the 'for example, ' in the previous line.
> Otherwise OK.
> 
Yes.

> >>
> >>> +
> >>> +\begin{note}
> >>> +The basic unit of all data length the byte.
> >>> +\end{note}
> >>> +
> >>> +The general header for controlq is as follows:
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
> >>> +
> >>> +struct virtio_crypto_ctrl_header {
> >>> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> >>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> >> 0x02)
> >>> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> >>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> >> 0x03)
> >>> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
> >>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH,
> 0x02)
> >>> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
> >>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH,
> 0x03)
> >>> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
> >>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC,
> 0x02)
> >>> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
> >>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC,
> 0x03)
> >>> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
> >>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD,
> 0x02)
> >>> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> >>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD,
> 0x03)
> >>> +    le32 opcode;
> >>> +    le32 algo;
> >>> +    le32 flag;
> >>> +    /* data virtqueue id */
> >>> +    le32 queue_id;
> >>> +};
> >>> +\end{lstlisting}
> >>> +
> >>> +The general header of dataq:
> >>> +
> >>> +\begin{lstlisting}
> >>> +struct virtio_crypto_op_header {
> >>> +#define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
> >>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
> >>> +#define VIRTIO_CRYPTO_CIPHER_DECRYPT \
> >>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
> >>> +#define VIRTIO_CRYPTO_HASH \
> >>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
> >>> +#define VIRTIO_CRYPTO_MAC \
> >>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
> >>> +#define VIRTIO_CRYPTO_AEAD_ENCRYPT \
> >>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> >>> +#define VIRTIO_CRYPTO_AEAD_DECRYPT \
> >>> +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> >>> +    le32 opcode;
> >>> +    /* algo should be service-specific algorithms */
> >>> +    le32 algo;
> >>> +    /* session_id should be service-specific algorithms */
> >>> +    le64 session_id;
> >>> +#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
> >>> +#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2
> >>
> >> Use _F_ istead of _FLAG_?
> >>
> > I'm afraid they are confused with the feature bits.
> >
> 
> Have a look at the desc flags (for example VIRTQ_DESC_F_INDIRECT).
> 
But in virtio crypto spec, we have the following feature bits:

VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for CIPHER service.
VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for HASH service.
VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC service.
VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for AEAD service.

They are too similar.

> It is your call in the end.
> 
> >>> +    /* control flag to control the request */
> >>> +    le32 flag;
> >>> +    le32 padding;
> >>> +};
> >>> +\end{lstlisting}
> >>> +
> >>> +The device can set the operation status as follows: VIRTIO_CRYPTO_OK:
> >> success;
> >>> +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP:
> >> not supported;
> >>> +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto
> >> operations.
> >>> +
> >>> +\begin{lstlisting}
> >>> +enum VIRITO_CRYPTO_STATUS {
> >>> +    VIRTIO_CRYPTO_OK = 0,
> >>> +    VIRTIO_CRYPTO_ERR = 1,
> >>> +    VIRTIO_CRYPTO_BADMSG = 2,
> >>> +    VIRTIO_CRYPTO_NOTSUPP = 3,
> >>> +    VIRTIO_CRYPTO_INVSESS = 4,
> >>> +    VIRTIO_CRYPTO_MAX
> >>> +};
> >>> +\end{lstlisting}
> >>> +
> >>> +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device
> /
> >> Device Operation / Control Virtqueue}
> >>> +
> >>> +The driver uses the control virtqueue to send control commands to the
> >>> +device which handles the non-data plane operations, such as session
> >>
> >> What is a 'non-data plane'?
> >>
> > I named controlling plane as the non-data plane. Maybe it's superfluous.
> >
> 
> Again consisten naming. I would prefer avoiding introducing this extra
> name.
> 
Yes. Let me drop it.

> >>> +operations (See \ref{sec:Device Types / Crypto Device / Device Operation
> /
> >> Control Virtqueue / Session operation}).
> >>
> >> Reviewed up until here. Depending on how things evolve will try to
> >> review the rest too in the following days.
> >>
> > I knew it's a hard and time-consuming work to review such long spec.
> >
> > Great thanks for your reviewing. :)
> >
> 
> I know, its annoying to deal with my comments ;). IMHO as spec
> without a certain quantity of rigor is not really a spec. I think
> it's very hard to write a good spec, so be patient.
> 
Yes, I agree. Please join me when you have a leisure if possible, haha...
I invited you too in other thread. ;)

> >> A question and a remark as a closing word:
> >> * Are there already some kernel and qemu patches for this 'non-session'
> stuff?
> >
> > THB currently I have a patch for virtio_crypto.h.
> >
> > Attaching it for better review.
> >
> 
> I would prefer having an implementation of the new features on the list,
> before voting on the spec itself.
> 
As we reach a general consensus, I'll start coding.

Thanks,
-Gonglei

> >> * I think some proofreading (and eventually also touch-up) by a native
> speaker
> >> would really benefit us. Sadly my grammar skills are very questionable, so
> >> I can't help much. Nevertheless since it is a spec, I think we sould strive
> >> for high standards in language usage too.
> >>
> > I agree and I actually asked a native speaker in my company to review it
> > several months ago. When all comments are handled, I'll do it again.
> 
> Thats cool.
> 
> Regards,
> Halil
> 
> >
> > Thanks,
> > -Gonglei
> >
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[Qemu-devel] [PATCH v16 2/2] virtio-crypto: Add conformance clauses

[Gonglei]

Add the conformance targets and clauses for
virtio-crypto device.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 conformance.tex | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index f59e360..3bde4b6 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -146,6 +146,21 @@ An SCSI host driver MUST conform to the following normative statements:
 \item \ref{drivernormative:Device Types / SCSI Host Device / Device Operation / Device Operation: eventq}
 \end{itemize}
 
+\subsection{Crypto Driver Conformance}\label{sec:Conformance / Driver Conformance / Crypto Driver Conformance}
+
+An Crypto driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / Crypto Device / Device configuration layout}
+\item \ref{drivernormative:Device Types / Crypto Device / Device Initialization}
+\item \ref{drivernormative:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation / Session operation: create session}
+\item \ref{drivernormative:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation / Session operation: destroy session}
+\item \ref{drivernormative:Device Types / Crypto Device / Device Operation / HASH Service operation}
+\item \ref{drivernormative:Device Types / Crypto Device / Device Operation / MAC Service operation}
+\item \ref{drivernormative:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
+\item \ref{drivernormative:Device Types / Crypto Device / Device Operation / AEAD Service operation}
+\end{itemize}
+
 \section{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -267,6 +282,21 @@ An SCSI host device MUST conform to the following normative statements:
 \item \ref{devicenormative:Device Types / SCSI Host Device / Device Operation / Device Operation: eventq}
 \end{itemize}
 
+\subsection{Crypto Device Conformance}\label{sec:Conformance / Device Conformance / Crypto Device Conformance}
+
+An Crypto device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / Crypto Device / Device configuration layout}
+\item \ref{devicenormative:Device Types / Crypto Device / Device Initialization}
+\item \ref{devicenormative:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation / Session operation: create session}
+\item \ref{devicenormative:Device Types / Crypto Device / Device Operation / Control Virtqueue / Session operation / Session operation: destroy session}
+\item \ref{devicenormative:Device Types / Crypto Device / Device Operation / HASH Service operation}
+\item \ref{devicenormative:Device Types / Crypto Device / Device Operation / MAC Service operation}
+\item \ref{devicenormative:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
+\item \ref{devicenormative:Device Types / Crypto Device / Device Operation / AEAD Service operation}
+\end{itemize}
+
 \section{Legacy Interface: Transitional Device and
 Transitional Driver Conformance}\label{sec:Conformance / Legacy
 Interface: Transitional Device and 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v16 2/2] virtio-crypto: Add conformance clauses

[Stefan Hajnoczi]

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

On Wed, Jan 18, 2017 at 04:22:37PM +0800, Gonglei wrote:
> Add the conformance targets and clauses for
> virtio-crypto device.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  conformance.tex | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v16 2/2] virtio-crypto: Add conformance clauses

[Gonglei (Arei)]

>
> Subject: Re: [Qemu-devel] [PATCH v16 2/2] virtio-crypto: Add conformance
> clauses
> 
> On Wed, Jan 18, 2017 at 04:22:37PM +0800, Gonglei wrote:
> > Add the conformance targets and clauses for
> > virtio-crypto device.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  conformance.tex | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks!

-Gonglei