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

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

[Gonglei]

This is the specification about a new virtio crypto device.

You can get the source code from the below website:

[PATCH v3 00/10] virtio-crypto: introduce framework and device emulation
  https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04132.html

[PATCH v4 00/13] virtio-crypto: introduce framework and device emulation
 https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg07327.html

[PATCH v5 00/14] virtio-crypto: introduce framework and device emulation
 https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00963.html

 ...

[PATCH v9 00/12] virtio-crypto: introduce framework and device emulation
 https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04755.html

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

Please help to review, thanks.

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: Hanweidong <hanweidong@huawei.com>
CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>

Changes since v12:
 - add max_size field in the virtio-crypto device config in order
   to tell the driver what's maximum size of crypto request the
   device supports.     [Michael]
 - add max_cipher_key_len and max_auth_key_len in the device config
   too for the symmetric algorithms to limit resource utilization by
   guest. [Thoughts come from Michael]

Changes since v11:
 - drop scatter-gather I/O definition for virtio crypto device because
   The vring already provides scatter-gather I/O.  It is usually not
   necessary to define scatter-gather I/O at the device level.      [Stefan]
 - perfect algorithm chain parameters' definition.
 - add HASH/MAC parameter structure.

Changes since v10:
 - fix typos s/filed/field/. [Xin]
 - replace 'real cypto accelerator' with 'backend crypto accelerator'. [mst]
 - drop KDF, ASYM, PRIMITIVE services description temporarily. [mst]
 - write a device requirement are testable about VIRTIO_CRYPTO_S_HW_READY. [mst]
 - add a space before * in one code comment. [mst]
 - reset the layout of all crypto operations for better asymmetric algos support. [Xin]
 - add more detailed description for initialization vector under different modes.
 - sed -i 's/VIRTIO_CRYPTO_OP_/VIRTIO_CRYPTO_/g' for general usage in asym algos. [Xin]

Changes since v9:
 - request a native speaker go over the text and fix corresponding grammar issues. [mst]
 - make some description more appropriated over here and there. [mst]
 - rewrite some requirement for both device and driver. [mst]
 - use RFC 2119 keywords. [mst]
 - fix some complaints by Xelatex and typoes. [Xin Zeng]
 - add scatter/getter chain support for possible large block data.

Thanks for your review, Michael and Xin.

Changes from v8:
 - add additional auth gpa and length to struct virtio_crypto_sym_data_req;
 - add definition of op in struct virtio_crypto_cipher_session_para,
  VIRTIO_CRYPTO_OP_ENCRYPT and VIRTIO_CRYPTO_OP_DECRYPT;
 - make all structures 64bit aligned in order to support different
  architectures more conveniently [Alex & Stefan]
 - change to devicenormative{\subsection} and \drivernormative{\subsection} in some sections [Stefan]
 - driver does not have to initialize all data virtqueues if it wants to use fewer [Stefan]
 - drop VIRTIO_CRYPTO_NO_SERVICE definition [Stefan]
 - many grammatical problems and typos. [Stefan]
 - rename VIRTIO_CRYPTO_MAC_CMAC_KASUMI_F9 to VIRTIO_CRYPTO_MAC_CMAC_KASUMI_F9,
  and VIRTIO_CRYPTO_MAC_CMAC_SNOW3G_UIA2 to VIRTIO_CRYPTO_MAC_SNOW3G_UIA2. [Liang Ma]
 - drop queue_id property of struct virtio_crypto_op_data_req.
 - reconstruct some structures about session operation request.
 - introduce struct virtio_crypto_alg_chain_session_req and struct virtio_crypto_alg_chain_data_req,
  introduce chain para, output, input structures as well.
 - change some sections' layout for better compatibility, for asymmetric algos. [Xin Zeng]

Changes from v7:
 - fix some grammar or typo problems.
 - add more detailed description at steps of encryption section.

Changes from v6:
 - drop verion filed in struct virtio_crypto_config. [Michael & Cornelia]
 - change the incorrect description in initialization routine. [Zeng Xin]
 - redefine flag u16 to make structure alignment. [Zeng Xin]
 - move the content of virtio_crypto_hash_session_para into
   virtio_crypto_hash_session_input directly, Same to MAC/SYM/AEAD session creation. [Zeng Xin]
 - adjuest the sequence of idata and odata refer to the virtio scsi parts,
   meanwhile add the comments of device-readable/writable for them.
 - add restrictive documents for the guest memory in some structure, which
   MUST be gauranted to be allocated and physically-contiguous.

Changes from v5:
 - add conformance clauses for virtio crypto device. [Michael]
 - drop VIRTIO_CRYPTO_S_STARTED. [Michael]
 - fix some characters problems. [Stefan]
 - add a MAC algorithm, named VIRTIO_CRYPTO_MAC_ZUC_EIA3. [Zeng Xin]
 - add the fourth return code, named VIRTIO_CRYPTO_OP_INVSESS used
   for invalid session id when executing crypto operations.
 - drop some gpu stuff forgot to delete. [Michael]
 - convert tab to space all over the content.

Changes from v4:
 - introduce crypto services into virtio crypto device. The services
   currently defined are CIPHER, MAC, HASH, AEAD, KDF, ASYM, PRIMITIVE.
 - define a unified crypto request format that is consisted of
   general header + service specific request,  Where 'general header' is for all
   crypto request,  'service specific request' is composed of
   operation parameter + input data + output data in generally.
   operation parameter is algorithm-specific parameters,
   input data is the data should be operated ,
   output data is the "operation result + result buffer".
 - redefine the algorithms and structure based on above crypto services.
 - rearrange the title and subtitle
 - Only support CIPHER, MAC, HASH and AEAD crypto services, and Xin will
   focus KDF, ASYM and PRIMITIVE services.
 - Some other corresponding fixes.
 - Make a formal patch using tex type.

This version is a big reconstruction based on Zeng, Xin' comments, thanks a lot.

Changes from v3:
 - Don't use enum is the spec but macros in specific structures. [Michael & Stefan]
 - Add two complete structures for session creation and closing, so that
  the spec is clear on how to lay out the request.  [Stefan]
 - Definite the crypto operation request with assigned structure, in this way,
  each data request only occupies *one entry* of the Vring descriptor table,
  which *improves* the *throughput* of data transferring.

Changes from v2:
 - Reserve virtio device ID 20 for crypto device. [Cornelia]
 - Drop all feature bits, those capabilities are offered by the device all the time.  [Stefan & Cornelia]
 - Add a new section 1.4.2 for driver requirements. [Stefan]
 - Use definite type definition instead of enum type in some structure. [Stefan]
 - Add virtio_crypto_cipher_alg definition. [Stefan]
 - Add a "Device requirements" section as using MUST. [Stefan]
 - Some grammar nits fixes and typo fixes. [Stefan & Cornelia]
 - Add one VIRTIO_CRYPTO_S_STARTED status for the driver as the flag of virtio-crypto device started and can work now.

Great thanks for Stefan and Cornelia!

Changes from v1:
 - Drop the feature bit definition for each algorithm, and using config space instead  [Cornelia]
 - Add multiqueue support and add corresponding feature bit
 - Update Encryption process and header definition
 - Add session operation process and add corresponding header description
 - Other better description in order to fit for virtio spec  [Michael]
 - Some other trivial fixes.

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

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

-- 
1.7.12.4

[Qemu-devel] [PATCH v13 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: Hanweidong <hanweidong@huawei.com>
CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
---
 content.tex       |    2 +
 virtio-crypto.tex | 1009 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 1011 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..448296e
--- /dev/null
+++ b/virtio-crypto.tex
@@ -0,0 +1,1009 @@
+\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 the data queue and are ultimately handled by the
+backend crypto accelerators. The second queue is the control queue used to create 
+or destroy sessions for symmetric algorithms and will control some advanced
+features in the future. The virtio crypto device provides the following 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}
+
+Undefined currently.
+
+\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 reserve;
+    /* 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_STARTED.
+
+\begin{lstlisting}
+#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
+#define VIRTIO_CRYPTO_S_STARTED  (1 << 1)
+\end{lstlisting}
+
+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 value is 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 MAY 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 supportes.
+\item The driver SHOULD read \field{max_cipher_key_len} to discover the maximum length of cipher key the device supportes.
+\item The driver SHOULD read \field{max_auth_key_len} to discover the maximum length of authenticated key the device supportes.
+\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 buffer".
+
+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;
+    /* 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}
+#define VIRTIO_CRYPTO_OK        0
+#define VIRTIO_CRYPTO_ERR       1
+#define VIRTIO_CRYPTO_BADMSG    2
+#define VIRTIO_CRYPTO_NOTSUPP   3
+#define VIRTIO_CRYPTO_INVSESS   4
+\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 (encrypt or decrypt).
+\item The HASH/MAC set data, including the HASH algorithm or MAC algorithm,
+      and digest 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_session_output {
+    le64 auth_key_addr; /* guest key physical address */
+};
+
+struct virtio_crypto_mac_create_session_req {
+    /* Device-readable part */
+    struct virtio_crypto_mac_session_para para;
+    struct virtio_crypto_mac_session_output out;
+    /* Device-writable part */
+    struct virtio_crypto_session_input input;
+};
+\end{lstlisting}
+
+The output data are
+\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
+    /* encrypt or decrypt */
+    le32 op;
+    le32 padding;
+};
+
+struct virtio_crypto_cipher_session_output {
+    le64 key_addr; /* guest key physical address */
+};
+
+struct virtio_crypto_cipher_session_req {
+    struct virtio_crypto_cipher_session_para para;
+    struct virtio_crypto_cipher_session_output out;
+    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_output {
+    struct virtio_crypto_cipher_session_output cipher;
+    struct virtio_crypto_mac_session_output mac;
+};
+
+struct virtio_crypto_alg_chain_session_req {
+    struct virtio_crypto_alg_chain_session_para para;
+    struct virtio_crypto_alg_chain_session_output out;
+    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;
+    /* digest result length */
+    le32 digest_result_len;
+    /* The length of the additional authenticated data (AAD) in bytes */
+    le32 aad_len;
+    /* encrypt or decrypt, See above VIRTIO_CRYPTO_* */
+    le32 op;
+    le32 padding;
+};
+
+struct virtio_crypto_aead_session_output {
+    le64 key_addr; /* guest key physical address */
+};
+
+struct virtio_crypto_aead_create_session_req {
+    struct virtio_crypto_aead_session_para para;
+    struct virtio_crypto_aead_session_output out;
+    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: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
+\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: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: failure or device error.
+\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 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 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 idata structure for all symmetric algorithms, including CIPHER, HASH, MAC, and AEAD.
+
+The structure is defined as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_sym_input {
+    /* Destination data guest address, it's useless for plain HASH and MAC */
+    le64 dst_data_addr;
+    /* Digest result guest address, it's useless for plain cipher algos */
+    le64 digest_result_addr;
+
+    le32 status;
+    le32 padding;
+};
+
+\end{lstlisting}
+
+\subsubsection{HASH Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / HASH Service Operation}
+
+\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_input {
+    struct virtio_crypto_sym_input input;
+};
+
+struct virtio_crypto_hash_output {
+    /* source data guest address */
+    le64 src_data_addr;
+};
+
+struct virtio_crypto_hash_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_hash_para para;
+    struct virtio_crypto_hash_output odata;
+    /* Device-writable part */
+    struct virtio_crypto_hash_input idata;
+};
+\end{lstlisting}
+
+Each data request uses virtio_crypto_hash_data_req structure to store information
+used to run the HASH operations. The request only occupies one entry
+in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
+the throughput of data transmitted for the HASH service, so that the virtio crypto
+device can be better accelerated.
+
+The information includes the source data guest physical address stored by \field{odata}.\field{src_data_addr},
+length of source data stored by \field{para}.\field{src_data_len}, and the digest result guest physical address
+stored by \field{digest_result_addr} used to save the results of the HASH operations.
+The address and length can determine exclusive content in the guest memory.
+
+\begin{note}
+The guest memory is always guaranteed to be allocated and physically-contiguous
+pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input and
+\field{src_data_addr} in struct virtio_crypto_hash_output.
+\end{note}
+
+\drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
+
+\begin{itemize*}
+\item 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 The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_HASH.
+\item The driver MUST set the \field{queue_id} field to show used dataq in struct virtio_crypto_op_header.
+\end{itemize*}
+
+\devicenormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
+
+\begin{itemize*}
+\item The device MUST copy the results of HASH operations to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_hash_input.
+\item The device MUST set \field{status} in strut virtio_crypto_hash_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
+\end{itemize*}
+
+\subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / MAC Service Operation}
+
+\begin{lstlisting}
+struct virtio_crypto_mac_para {
+    struct virtio_crypto_hash_para hash;
+};
+
+struct virtio_crypto_mac_input {
+    struct virtio_crypto_sym_input input;
+};
+
+struct virtio_crypto_mac_output {
+    struct virtio_crypto_hash_output hash_output;
+};
+
+struct virtio_crypto_mac_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_mac_para para;
+    struct virtio_crypto_mac_output odata;
+    /* Device-writable part */
+    struct virtio_crypto_mac_input idata;
+};
+\end{lstlisting}
+
+Each data request uses virtio_crypto_mac_data_req structure to store information
+used to run the MAC operations. The request only occupies one entry
+in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
+the throughput of data transmitted for the MAC service, so that the virtio crypto
+device can get the better result of acceleration.
+
+The information includes the source data guest physical address stored by \field{hash_output}.\field{src_data}.\field{addr},
+the length of source data stored by \field{hash_output}.\field{src_data}.\field{len}, and the digest result guest physical address
+stored by \field{digest_result_addr} used to save the results of the MAC operations.
+
+The address and length can determine exclusive content in the guest memory.
+
+\begin{note}
+The guest memory is always guaranteed to be allocated and physically-contiguous
+pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input and
+\field{hash_output}.\field{src_data_addr} in struct virtio_crypto_mac_output.
+\end{note}
+
+\drivernormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
+
+\begin{itemize*}
+\item 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 The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_MAC.
+\item The driver MUST set the \field{queue_id} field to show used dataq in struct virtio_crypto_op_header.
+\end{itemize*}
+
+\devicenormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
+
+\begin{itemize*}
+\item The device MUST copy the results of MAC operations to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_mac_input.
+\item The device MUST set \field{status} in strut virtio_crypto_mac_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
+\end{itemize*}
+
+\subsubsection{Symmetric algorithms Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
+
+The packet of plain CIPHER service is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_cipher_para {
+    /*
+     * Byte Length of valid IV data pointed to by the below iv_addr.
+     *
+     * - 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 dst data */
+    le32 dst_data_len;
+    le32 padding;
+};
+
+struct virtio_crypto_cipher_input {
+    struct virtio_crypto_sym_input input;
+};
+
+struct virtio_crypto_cipher_output {
+   /*
+     * Initialization Vector or Counter Guest Address.
+     *
+     * - 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.
+     */
+    le64 iv_addr;
+    /* source data guest address */
+    le64 src_data_addr;
+};
+
+struct virtio_crypto_cipher_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_cipher_para para;
+    struct virtio_crypto_cipher_output odata;
+    /* Device-writable part */
+    struct virtio_crypto_cipher_input idata;
+};
+\end{lstlisting}
+
+The packet of algorithm chaining is as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_alg_chain_data_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_alg_chain_data_output {
+    /* Device-readable part */
+    struct virtio_crypto_cipher_output cipher;
+    /* Additional auth data guest address */
+    le64 aad_data_addr;
+};
+
+struct virtio_crypto_alg_chain_data_input {
+    struct virtio_crypto_sym_input input;
+};
+
+struct virtio_crypto_alg_chain_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_alg_chain_data_para para;
+    struct virtio_crypto_alg_chain_data_output odata;
+    /* Device-writable part */
+    struct virtio_crypto_alg_chain_data_input idata;
+};
+\end{lstlisting}
+
+The 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 the virtio_crypto_cipher_data_req structure to store information
+used to run the CIPHER operations. The request only occupies one entry
+in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
+the throughput of data transmitted for the CIPHER service, so that the virtio crypto
+device can get the better result of acceleration.
+
+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.
+
+In the following virtio_crypto_cipher_input structure, \field{dst_data_addr} specifies the destination
+data guest physical address used to store the results of the CIPHER operations, and \field{status} specifies
+the CIPHER operation status.
+
+In the virtio_crypto_cipher_output structure, \field{iv_addr} specifies the guest physical address of initialization vector,
+\field{src_data_addr} specifies the source data guest physical address.
+
+The addresses and length can determine exclusive content in the guest memory.
+
+\begin{note}
+The guest memory is always guaranteed to be allocated and physically-contiguous
+pointed by \field{dst_data_addr} in struct virtio_crypto_cipher_input, 
+\field{iv_addr} and \field{src_data_addr} in struct virtio_crypto_cipher_output.
+\end{note}
+
+\drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
+
+\begin{itemize*}
+\item 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 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 set the \field{queue_id} field to show used dataq in struct virtio_crypto_op_header.
+\item The driver MUST specify the fields of struct virtio_crypto_cipher_data_req in struct virtio_crypto_sym_data_req if the created session 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 created session
+\item 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 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 created session 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 created
+session 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 guest memory recorded by \field{dst_data_addr} field in struct virtio_crypto_cipher_input in plain CIPHER mode.
+\item The device MUST copy the result of HASH/MAC operation to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_alg_chain_data_input is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type.
+\item The device MUST set the \field{status} field in strut virtio_crypto_cipher_input or virtio_crypto_alg_chain_data_input structure:
+VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported.
+\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}
+
+\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_cipher_input;
+\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 result.
+\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}
+
+\begin{lstlisting}
+struct virtio_crypto_aead_para {
+    /*
+     * Byte Length of valid IV data pointed to by the below iv_addr parameter.
+     *
+     * - For GCM mode, this is either 12 (for 96-bit IVs) or 16, in which
+     *   case iv_addr 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 */
+    le32 dst_data_len;
+};
+
+struct virtio_crypto_aead_input {
+    struct virtio_crypto_sym_input input;
+};
+
+struct virtio_crypto_aead_output {
+    /*
+     * Initialization Vector Guest Address.
+     *
+     * - 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_addr[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.
+     */
+    le64 iv_addr;
+    /* source data guest address */
+    le64 src_data_addr;
+    /* additional auth data guest address */
+    le64 add_data_addr;
+};
+
+struct virtio_crypto_aead_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_aead_para para;
+    struct virtio_crypto_aead_output odata;
+    /* Device-writable part */
+    struct virtio_crypto_aead_input idata;
+};
+\end{lstlisting}
+
+Each data request uses virtio_crypto_aead_data_req structure to store information
+used to implement the CIPHER operations. The request only occupies one entry
+in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
+the throughput of data transmitted for the AEAD service, so that the virtio crypto
+device can be better accelerated.
+
+In the first virtio_crypto_aead_para structure, \field{iv_len} specifies the length of the initialization vector;
+\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.
+
+In the following virtio_crypto_aead_input structure, \field{dst_data_addr} specifies destination
+data guest physical address used to store the results of the CIPHER operations; \field{digest_result_addr} specifies
+the digest result guest physical address used to store the results of the HASH/MAC operations; \field{status} specifies
+the status of AEAD operation.
+
+In the virtio_crypto_aead_output structure, \field{iv_addr} specifies the guest physical address of initialization vector,
+\field{src_data_addr} specifies the source data guest physical address, \field{add_data_addr} specifies the guest physical address
+of additional authentication data.
+
+The addresses and length can determine exclusive content in the guest memory.
+
+\begin{note}
+The guest memory MUST be guaranteed to be allocated and physically-contiguous
+pointed by \field{dst_data_addr} and \field{digest_result_addr} in struct virtio_crypto_aead_input, 
+\field{iv_addr}, \field{src_data_addr}  and \field{add_data_addr} in struct virtio_crypto_aead_output.
+\end{note}
+
+\drivernormative{\paragraph}{AEAD Service Operation}{Device Types / Crypto Device / Device Operation / AEAD Service Operation}
+
+\begin{itemize*}
+\item 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 The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_AEAD_ENCRYPT or VIRTIO_CRYPTO_AEAD_DECRYPT.
+\item The driver MUST set the \field{queue_id} field to show used dataq in struct virtio_crypto_op_header.
+\end{itemize*}
+
+\devicenormative{\paragraph}{AEAD Service Operation}{Device Types / Crypto Device / Device Operation / AEAD Service Operation}
+
+\begin{itemize*}
+\item 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 guest memory recorded by \field{dst_data_addr} field in struct virtio_crypto_aead_input.
+\item The device MUST copy the digest result to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_aead_input.
+\item The device MUST set the \field{status} field in strut virtio_crypto_aead_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported.
+\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

[Qemu-devel] [PATCH v13 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 v13 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

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



On 10/28/2016 07:23 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: Hanweidong <hanweidong@huawei.com>
> CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> ---
[..]
> +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 idata structure for all symmetric algorithms, including CIPHER, HASH, MAC, and AEAD.
> +
> +The structure is defined as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_sym_input {
> +    /* Destination data guest address, it's useless for plain HASH and MAC */
> +    le64 dst_data_addr;
> +    /* Digest result guest address, it's useless for plain cipher algos */
> +    le64 digest_result_addr;
> +
> +    le32 status;
> +    le32 padding;
> +};
> +

This seems to be out of sync regarding the code (e.g. can't find it in virtio-crypto.h of the
series linked in the cover-letter). It seems to me this reflects v4 since the stuff is gone
since v5 (qemu code).

> +\end{lstlisting}
> +
> +\subsubsection{HASH Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / HASH Service Operation}
> +
> +\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_input {
> +    struct virtio_crypto_sym_input input;
> +};
> +
> +struct virtio_crypto_hash_output {
> +    /* source data guest address */
> +    le64 src_data_addr;
> +};
> +
> +struct virtio_crypto_hash_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_hash_para para;
> +    struct virtio_crypto_hash_output odata;
> +    /* Device-writable part */
> +    struct virtio_crypto_hash_input idata;
> +};
> +\end{lstlisting}
> +
> +Each data request uses virtio_crypto_hash_data_req structure to store information
> +used to run the HASH operations. The request only occupies one entry
> +in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> +the throughput of data transmitted for the HASH service, so that the virtio crypto
> +device can be better accelerated.
> +
> +The information includes the source data guest physical address stored by \field{odata}.\field{src_data_addr},
> +length of source data stored by \field{para}.\field{src_data_len}, and the digest result guest physical address
> +stored by \field{digest_result_addr} used to save the results of the HASH operations.
> +The address and length can determine exclusive content in the guest memory.
> +

Thus this does not make any sense to me. Furthermore the problem seems to persist
across the specification. Thus in my opinion there is no point in reviewing
this version. Or am I missing something here? In case I'm not missing anything
and the spec describes something quite outdated when should we expect a new
version of the spec?

Regards,
Halil


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

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

[Gonglei (Arei)]

Hi,

> 
> [..]
> > +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 idata structure for all symmetric algorithms, including
> CIPHER, HASH, MAC, and AEAD.
> > +
> > +The structure is defined as follows:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_sym_input {
> > +    /* Destination data guest address, it's useless for plain HASH and MAC
> */
> > +    le64 dst_data_addr;
> > +    /* Digest result guest address, it's useless for plain cipher algos */
> > +    le64 digest_result_addr;
> > +
> > +    le32 status;
> > +    le32 padding;
> > +};
> > +
> 
> This seems to be out of sync regarding the code (e.g. can't find it in
> virtio-crypto.h of the
> series linked in the cover-letter). It seems to me this reflects v4 since the stuff
> is gone
> since v5 (qemu code).
> 
> > +\end{lstlisting}
> > +
> > +\subsubsection{HASH Service Operation}\label{sec:Device Types / Crypto
> Device / Device Operation / HASH Service Operation}
> > +
> > +\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_input {
> > +    struct virtio_crypto_sym_input input;
> > +};
> > +
> > +struct virtio_crypto_hash_output {
> > +    /* source data guest address */
> > +    le64 src_data_addr;
> > +};
> > +
> > +struct virtio_crypto_hash_data_req {
> > +    /* Device-readable part */
> > +    struct virtio_crypto_hash_para para;
> > +    struct virtio_crypto_hash_output odata;
> > +    /* Device-writable part */
> > +    struct virtio_crypto_hash_input idata;
> > +};
> > +\end{lstlisting}
> > +
> > +Each data request uses virtio_crypto_hash_data_req structure to store
> information
> > +used to run the HASH operations. The request only occupies one entry
> > +in the Vring Descriptor Table in the virtio crypto device's dataq, which
> improves
> > +the throughput of data transmitted for the HASH service, so that the virtio
> crypto
> > +device can be better accelerated.
> > +
> > +The information includes the source data guest physical address stored by
> \field{odata}.\field{src_data_addr},
> > +length of source data stored by \field{para}.\field{src_data_len}, and the
> digest result guest physical address
> > +stored by \field{digest_result_addr} used to save the results of the HASH
> operations.
> > +The address and length can determine exclusive content in the guest
> memory.
> > +
> 
> Thus this does not make any sense to me. Furthermore the problem seems to
> persist
> across the specification. Thus in my opinion there is no point in reviewing
> this version. Or am I missing something here? In case I'm not missing anything
> and the spec describes something quite outdated when should we expect a
> new
> version of the spec?
> 
Nope, Actually I kept those description here is because I wanted to represent each packet
Intuitionally, otherwise I don't know how to explain them only occupy one entry in desc table
by indirect table method. So I changed the code completely as Stefan's suggestion and
revised the spec a little.

This just is a representative of the realization so that the people can easily understand what
the virtio crypto request's components. It isn't completely same with the code.
For virtio-scsi device, the struct virtio_scsi_req_cmd also used this way IIUR.

Thanks,
-Gonglei

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

[Cornelia Huck]

On Wed, 9 Nov 2016 01:11:20 +0000
"Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:

> Nope, Actually I kept those description here is because I wanted to represent each packet
> Intuitionally, otherwise I don't know how to explain them only occupy one entry in desc table
> by indirect table method. So I changed the code completely as Stefan's suggestion and
> revised the spec a little.

I think you need to revise the spec a bit more :) IIUC, you should
simply state that you use the indirect table method and not mention any
guest physical addresses.

> This just is a representative of the realization so that the people can easily understand what
> the virtio crypto request's components. It isn't completely same with the code.
> For virtio-scsi device, the struct virtio_scsi_req_cmd also used this way IIUR.

It seemes to have caused at least some confusion - perhaps you
simplyfied too much?

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

[Michael S. Tsirkin]

On Wed, Nov 09, 2016 at 01:11:20AM +0000, Gonglei (Arei) wrote:
> Nope, Actually I kept those description here is because I wanted to represent each packet
> Intuitionally, otherwise I don't know how to explain them only occupy one entry in desc table
> by indirect table method.

whether to use the indirect method should not be up to device.
I don't see where does the spec require indirect method,
ANY_LAYOUT is implied so spec should not dictate that IMO.

> So I changed the code completely as Stefan's suggestion and
> revised the spec a little.
> 
> This just is a representative of the realization so that the people can easily understand what
> the virtio crypto request's components. It isn't completely same with the code.
> For virtio-scsi device, the struct virtio_scsi_req_cmd also used this way IIUR.
> 
> Thanks,
> -Gonglei

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

[Gonglei (Arei)]

>
> Subject: [virtio-dev] Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio
> crypto device specification
> 
> On Wed, Nov 09, 2016 at 01:11:20AM +0000, Gonglei (Arei) wrote:
> > Nope, Actually I kept those description here is because I wanted to represent
> each packet
> > Intuitionally, otherwise I don't know how to explain them only occupy one
> entry in desc table
> > by indirect table method.
> 
> whether to use the indirect method should not be up to device.
> I don't see where does the spec require indirect method,
> ANY_LAYOUT is implied so spec should not dictate that IMO.
> 
Yes, I reread the virtio spec. And

2.4.4 says: 
 " The framing of messages with descriptors is independent of the contents of the buffers."

2.4.4.2 says:
 "The driver SHOULD NOT use an excessive number of descriptors to describe a buffer."

For virtio-crypto, I think I can just add a NOTE to remind people using indirect method 
if they want to get better performance in production environment, can I? 


Thanks,
-Gonglei

> > So I changed the code completely as Stefan's suggestion and
> > revised the spec a little.
> >
> > This just is a representative of the realization so that the people can easily
> understand what
> > the virtio crypto request's components. It isn't completely same with the
> code.
> > For virtio-scsi device, the struct virtio_scsi_req_cmd also used this way IIUR.
> >
> > 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

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

[Gonglei (Arei)]

> From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> Sent: Wednesday, November 09, 2016 11:25 PM
> Subject: Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio crypto
> device specification
> 
> On Wed, 9 Nov 2016 01:11:20 +0000
> "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> 
> > Nope, Actually I kept those description here is because I wanted to represent
> each packet
> > Intuitionally, otherwise I don't know how to explain them only occupy one
> entry in desc table
> > by indirect table method. So I changed the code completely as Stefan's
> suggestion and
> > revised the spec a little.
> 
> I think you need to revise the spec a bit more :) IIUC, you should
> simply state that you use the indirect table method and not mention any
> guest physical addresses.
> 
Yes, will remove the gpa description stuff. Using indirect table is a method for high performance
and throughput, I'll mention it as a note. Pls see my another reply to Michael.

> > This just is a representative of the realization so that the people can easily
> understand what
> > the virtio crypto request's components. It isn't completely same with the
> code.
> > For virtio-scsi device, the struct virtio_scsi_req_cmd also used this way IIUR.
> 
> It seemes to have caused at least some confusion - perhaps you
> simplyfied too much?

Yes, I will. 

Thanks,
-Gonglei

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

[Gonglei (Arei)]

Hi,

I attach a diff for next version in order to review more convenient, with

 - Drop the all gap stuff;
 - Drop all structures undefined in virtio_crypto.h
 - re-describe per request for per crypto service avoid confusion

Pls review, thanks!


diff --git a/virtio-crypto.tex b/virtio-crypto.tex
index 448296e..ab17e7b 100644
--- a/virtio-crypto.tex
+++ b/virtio-crypto.tex
@@ -69,13 +69,13 @@ The following services are defined:
 
 \begin{lstlisting}
 /* CIPHER service */
-#define VIRTIO_CRYPTO_SERVICE_CIPHER (0)
+#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
 /* HASH service */
-#define VIRTIO_CRYPTO_SERVICE_HASH   (1)
+#define VIRTIO_CRYPTO_SERVICE_HASH   1
 /* MAC (Message Authentication Codes) service */
-#define VIRTIO_CRYPTO_SERVICE_MAC    (2)
+#define VIRTIO_CRYPTO_SERVICE_MAC    2
 /* AEAD (Authenticated Encryption with Associated Data) service */
-#define VIRTIO_CRYPTO_SERVICE_AEAD   (3)
+#define VIRTIO_CRYPTO_SERVICE_AEAD   3
 \end{lstlisting}
 
 The last driver-read-only fields specify detailed algorithms masks 
@@ -210,7 +210,7 @@ data that should be utilized in operations, and input data is equal to
 The general header for controlq is as follows:
 
 \begin{lstlisting}
-#define VIRTIO_CRYPTO_OPCODE(service, op)   ((service << 8) | (op))
+#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
 
 struct virtio_crypto_ctrl_header {
 #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
@@ -380,20 +380,18 @@ struct virtio_crypto_mac_session_para {
     le32 auth_key_len;
     le32 padding;
 };
-struct virtio_crypto_mac_session_output {
-    le64 auth_key_addr; /* guest key physical address */
-};
 
 struct virtio_crypto_mac_create_session_req {
     /* Device-readable part */
     struct virtio_crypto_mac_session_para para;
-    struct virtio_crypto_mac_session_output out;
+    /* The authenticated key buffer */
+    /* output data here */
+
     /* Device-writable part */
     struct virtio_crypto_session_input input;
 };
 \end{lstlisting}
 
-The output data are
 \subparagraph{Session operation: Symmetric algorithms session}\label{sec:Device Types / Crypto Device / Device
 Operation / Control Virtqueue / Session operation / Session operation: Symmetric algorithms session}
 
@@ -413,13 +411,13 @@ struct virtio_crypto_cipher_session_para {
     le32 padding;
 };
 
-struct virtio_crypto_cipher_session_output {
-    le64 key_addr; /* guest key physical address */
-};
-
 struct virtio_crypto_cipher_session_req {
+    /* Device-readable part */
     struct virtio_crypto_cipher_session_para para;
-    struct virtio_crypto_cipher_session_output out;
+    /* The cipher key buffer */
+    /* Output data here */
+
+    /* Device-writable part */
     struct virtio_crypto_session_input input;
 };
 \end{lstlisting}
@@ -448,18 +446,20 @@ struct virtio_crypto_alg_chain_session_para {
     le32 padding;
 };
 
-struct virtio_crypto_alg_chain_session_output {
-    struct virtio_crypto_cipher_session_output cipher;
-    struct virtio_crypto_mac_session_output mac;
-};
-
 struct virtio_crypto_alg_chain_session_req {
+    /* Device-readable part */
     struct virtio_crypto_alg_chain_session_para para;
-    struct virtio_crypto_alg_chain_session_output out;
+    /* The cipher key buffer */
+    /* The authenticated key buffer */
+    /* Output data here */
+
+    /* Device-writable part */
     struct virtio_crypto_session_input input;
 };
 \end{lstlisting}
 
+The output data includs both cipher key buffer and authenticated key buffer.
+
 The packet for symmetric algorithm is as follows:
 
 \begin{lstlisting}
@@ -503,13 +503,13 @@ struct virtio_crypto_aead_session_para {
     le32 padding;
 };
 
-struct virtio_crypto_aead_session_output {
-    le64 key_addr; /* guest key physical address */
-};
-
 struct virtio_crypto_aead_create_session_req {
+    /* Device-readable part */
     struct virtio_crypto_aead_session_para para;
-    struct virtio_crypto_aead_session_output out;
+    /* The cipher key buffer */
+    /* Output data here */
+
+    /* Device-writeable part */
     struct virtio_crypto_session_input input;
 };
 \end{lstlisting}
@@ -568,19 +568,13 @@ struct virtio_crypto_op_data_req {
 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 idata structure for all symmetric algorithms, including CIPHER, HASH, MAC, and AEAD.
+There is a unified idata structure for all service, including CIPHER, HASH, MAC, and AEAD.
 
 The structure is defined as follows:
 
 \begin{lstlisting}
-struct virtio_crypto_sym_input {
-    /* Destination data guest address, it's useless for plain HASH and MAC */
-    le64 dst_data_addr;
-    /* Digest result guest address, it's useless for plain cipher algos */
-    le64 digest_result_addr;
-
-    le32 status;
-    le32 padding;
+struct virtio_crypto_inhdr {
+    u8 status;
 };
 
 \end{lstlisting}
@@ -595,39 +589,29 @@ struct virtio_crypto_hash_para {
     le32 hash_result_len;
 };
 
-struct virtio_crypto_hash_input {
-    struct virtio_crypto_sym_input input;
-};
-
-struct virtio_crypto_hash_output {
-    /* source data guest address */
-    le64 src_data_addr;
-};
-
 struct virtio_crypto_hash_data_req {
     /* Device-readable part */
     struct virtio_crypto_hash_para para;
-    struct virtio_crypto_hash_output odata;
+    /* Source buffer */
+    /* Output data here */
+
     /* Device-writable part */
-    struct virtio_crypto_hash_input idata;
+    /* Digest result buffer */
+    /* Input data here */
+    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 request only occupies one entry
-in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
-the throughput of data transmitted for the HASH service, so that the virtio crypto
-device can be better accelerated.
+used to run the HASH operations. 
 
-The information includes the source data guest physical address stored by \field{odata}.\field{src_data_addr},
-length of source data stored by \field{para}.\field{src_data_len}, and the digest result guest physical address
-stored by \field{digest_result_addr} used to save the results of the HASH operations.
-The address and length can determine exclusive content in the guest memory.
+The information includes the hash paramenters stored by \field{para}, output data and input data.
+The output data here includs the source buffer and the input data includes the digest result buffer used to save the results of the HASH operations.
+\field{inhdr} store the executing status of HASH operations.
 
 \begin{note}
-The guest memory is always guaranteed to be allocated and physically-contiguous
-pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input and
-\field{src_data_addr} in struct virtio_crypto_hash_output.
+The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
+the throughput of data transmitted for the HASH service, so that the virtio crypto device can be better accelerated.
 \end{note}
 
 \drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
@@ -641,8 +625,8 @@ pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input and
 \devicenormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
 
 \begin{itemize*}
-\item The device MUST copy the results of HASH operations to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_hash_input.
-\item The device MUST set \field{status} in strut virtio_crypto_hash_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
+\item The device MUST copy the results of HASH operations to the guest memory recorded by digest result buffer if HASH operations success.
+\item The device MUST set \field{status} in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
 \end{itemize*}
 
 \subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / MAC Service Operation}
@@ -652,39 +636,29 @@ struct virtio_crypto_mac_para {
     struct virtio_crypto_hash_para hash;
 };
 
-struct virtio_crypto_mac_input {
-    struct virtio_crypto_sym_input input;
-};
-
-struct virtio_crypto_mac_output {
-    struct virtio_crypto_hash_output hash_output;
-};
-
 struct virtio_crypto_mac_data_req {
     /* Device-readable part */
     struct virtio_crypto_mac_para para;
-    struct virtio_crypto_mac_output odata;
+    /* Source buffer */
+    /* Output data here */
+
     /* Device-writable part */
-    struct virtio_crypto_mac_input idata;
+    /* Digest result buffer */
+    /* Input data here */
+    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 request only occupies one entry
-in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
-the throughput of data transmitted for the MAC service, so that the virtio crypto
-device can get the better result of acceleration.
-
-The information includes the source data guest physical address stored by \field{hash_output}.\field{src_data}.\field{addr},
-the length of source data stored by \field{hash_output}.\field{src_data}.\field{len}, and the digest result guest physical address
-stored by \field{digest_result_addr} used to save the results of the MAC operations.
+used to run the MAC operations. 
 
-The address and length can determine exclusive content in the guest memory.
+The information includes the hash paramenters stored by \field{para}, output data and input data.
+The output data here includs the source buffer and the input data includes the digest result buffer used to save the results of the MAC operations.
+\field{inhdr} store the executing status of MAC operations.
 
 \begin{note}
-The guest memory is always guaranteed to be allocated and physically-contiguous
-pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input and
-\field{hash_output}.\field{src_data_addr} in struct virtio_crypto_mac_output.
+The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
+the throughput of data transmitted for the MAC service, so that the virtio crypto device can be better accelerated.
 \end{note}
 
 \drivernormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
@@ -698,8 +672,8 @@ pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input and
 \devicenormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
 
 \begin{itemize*}
-\item The device MUST copy the results of MAC operations to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_mac_input.
-\item The device MUST set \field{status} in strut virtio_crypto_mac_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
+\item The device MUST copy the results of MAC operations to the guest memory recorded by digest result buffer if HASH operations success.
+\item The device MUST set \field{status} in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
 \end{itemize*}
 
 \subsubsection{Symmetric algorithms Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
@@ -709,12 +683,12 @@ The packet of plain CIPHER service is as follows:
 \begin{lstlisting}
 struct virtio_crypto_cipher_para {
     /*
-     * Byte Length of valid IV data pointed to by the below iv_addr.
+     * Byte Length of valid IV/Counter data pointed to by the below iv buffer.
      *
-     * - For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
+     * 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
+     * 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;
@@ -725,34 +699,28 @@ struct virtio_crypto_cipher_para {
     le32 padding;
 };
 
-struct virtio_crypto_cipher_input {
-    struct virtio_crypto_sym_input input;
-};
-
-struct virtio_crypto_cipher_output {
-   /*
-     * Initialization Vector or Counter Guest Address.
+struct virtio_crypto_cipher_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_cipher_para para;
+    /*
+     * Initialization Vector or Counter buffer.
      *
-     * - For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
+     * 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.
+     * 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.
      */
-    le64 iv_addr;
-    /* source data guest address */
-    le64 src_data_addr;
-};
+    /* Source buffer */
+    /* Output data here */
 
-struct virtio_crypto_cipher_data_req {
-    /* Device-readable part */
-    struct virtio_crypto_cipher_para para;
-    struct virtio_crypto_cipher_output odata;
     /* Device-writable part */
-    struct virtio_crypto_cipher_input idata;
+    /* Destination data buffer */
+    /* Input data here */
+    struct virtio_crypto_inhdr inhdr;
 };
 \end{lstlisting}
 
@@ -780,23 +748,19 @@ struct virtio_crypto_alg_chain_data_para {
     __virtio32 reserved;
 };
 
-struct virtio_crypto_alg_chain_data_output {
-    /* Device-readable part */
-    struct virtio_crypto_cipher_output cipher;
-    /* Additional auth data guest address */
-    le64 aad_data_addr;
-};
-
-struct virtio_crypto_alg_chain_data_input {
-    struct virtio_crypto_sym_input input;
-};
-
 struct virtio_crypto_alg_chain_data_req {
     /* Device-readable part */
     struct virtio_crypto_alg_chain_data_para para;
-    struct virtio_crypto_alg_chain_data_output odata;
+    /* Initialization Vector or Counter buffer */
+    /* Source buffer */
+    /* Additional authenticated buffer if exists  */
+    /* Output data here */
+
     /* Device-writable part */
-    struct virtio_crypto_alg_chain_data_input idata;
+    /* Destination data buffer */
+    /* Digest result buffer */
+    /* Input data here */
+    struct virtio_crypto_inhdr inhdr;
 };
 \end{lstlisting}
 
@@ -817,29 +781,22 @@ struct virtio_crypto_sym_data_req {
 };
 \end{lstlisting}
 
-Each data request uses the virtio_crypto_cipher_data_req structure to store information
-used to run the CIPHER operations. The request only occupies one entry
-in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
-the throughput of data transmitted for the CIPHER service, so that the virtio crypto
-device can get the better result of acceleration.
+Each data request uses virtio_crypto_sym_data_req structure to store information
+used to run the CIPHER operations. 
 
+The information includes the hash paramenters 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.
+length of the destination data. 
+For plain CIPHER operations, the output data here includs the IV/Counter buffer and source buffer, and the input data includes the destination buffer used to save the results of the CIPHER operations.
 
-In the following virtio_crypto_cipher_input structure, \field{dst_data_addr} specifies the destination
-data guest physical address used to store the results of the CIPHER operations, and \field{status} specifies
-the CIPHER operation status.
-
-In the virtio_crypto_cipher_output structure, \field{iv_addr} specifies the guest physical address of initialization vector,
-\field{src_data_addr} specifies the source data guest physical address.
-
-The addresses and length can determine exclusive content in the guest memory.
+For algorithms chain, the output data here includs the IV/Counter buffer, source buffer and additional authenticated data buffer if exists.
+The input data includes both destionation buffer and digest result buffer used to store the results of the HASH/MAC operations.
+\field{inhdr} store the executing status of crypto operations.
 
 \begin{note}
-The guest memory is always guaranteed to be allocated and physically-contiguous
-pointed by \field{dst_data_addr} in struct virtio_crypto_cipher_input, 
-\field{iv_addr} and \field{src_data_addr} in struct virtio_crypto_cipher_output.
+The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
+the throughput of data transmitted for the crypto service, so that the virtio crypto device can be better accelerated.
 \end{note}
 
 \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
@@ -860,10 +817,10 @@ pointed by \field{dst_data_addr} in struct virtio_crypto_cipher_input,
 \item The device SHOULD only parse fields of struct virtio_crypto_cipher_data_req in struct virtio_crypto_sym_data_req if the created session 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 created
 session 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 guest memory recorded by \field{dst_data_addr} field in struct virtio_crypto_cipher_input in plain CIPHER mode.
-\item The device MUST copy the result of HASH/MAC operation to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_alg_chain_data_input is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type.
-\item The device MUST set the \field{status} field in strut virtio_crypto_cipher_input or virtio_crypto_alg_chain_data_input structure:
-VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported.
+\item The device MUST copy the result of cryptographic operation to the guest memory recorded by destination buffer in plain CIPHER mode.
+\item The device MUST check the \field{para}.\field{add_len} is bigger than 0 before parse the additional authenticated buffer in plain algorithms chain mode.
+\item The device MUST copy the result of HASH/MAC operation to the guest memory recorded by digest result buffer is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type.
+\item The device MUST set the \field{status} field in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
 \end{itemize*}
 
 \paragraph{Steps of Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation / Steps of Operation}
@@ -894,17 +851,15 @@ VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; V
 \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_cipher_input;
+\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 result.
 \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*}
+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{note}
 
 \subsubsection{AEAD Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / AEAD Service Operation}
@@ -912,11 +867,11 @@ VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; V
 \begin{lstlisting}
 struct virtio_crypto_aead_para {
     /*
-     * Byte Length of valid IV data pointed to by the below iv_addr parameter.
+     * Byte Length of valid IV data.
      *
-     * - For GCM mode, this is either 12 (for 96-bit IVs) or 16, in which
-     *   case iv_addr points to J0.
-     * - For CCM mode, this is the length of the nonce, which can be in the
+     * 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;
@@ -928,66 +883,51 @@ struct virtio_crypto_aead_para {
     le32 dst_data_len;
 };
 
-struct virtio_crypto_aead_input {
-    struct virtio_crypto_sym_input input;
-};
-
-struct virtio_crypto_aead_output {
+struct virtio_crypto_aead_data_req {
+    /* Device-readable part */
+    struct virtio_crypto_aead_para para;
     /*
-     * Initialization Vector Guest Address.
+     * Initialization Vector buffer.
      *
-     * - For GCM mode, this is either the IV (if the length is 96 bits) or J0
+     * 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_addr[1] (to allow space for the implementation
+     * For CCM mode, the first byte is reserved, and the nonce should be
+     *   written starting at &iv_buffer[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.
      */
-    le64 iv_addr;
-    /* source data guest address */
-    le64 src_data_addr;
-    /* additional auth data guest address */
-    le64 add_data_addr;
-};
+    /* Source buffer */
+    /* Additional authenticated buffer if exists  */
+    /* Output data here */
 
-struct virtio_crypto_aead_data_req {
-    /* Device-readable part */
-    struct virtio_crypto_aead_para para;
-    struct virtio_crypto_aead_output odata;
     /* Device-writable part */
-    struct virtio_crypto_aead_input idata;
+    /* Destination data buffer */
+    /* Digest result buffer */
+    /* Input data here */
+    struct virtio_crypto_inhdr inhdr;
 };
 \end{lstlisting}
 
 Each data request uses virtio_crypto_aead_data_req structure to store information
-used to implement the CIPHER operations. The request only occupies one entry
-in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
-the throughput of data transmitted for the AEAD service, so that the virtio crypto
-device can be better accelerated.
+used to run the AEAD operations. 
 
-In the first virtio_crypto_aead_para structure, \field{iv_len} specifies the length of the initialization vector;
+The information includes the hash paramenters 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{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.
+The output data here includs the IV buffer and source buffer, and the input data includes the destination buffer used to save the results of the CIPHER operations.
 
-In the following virtio_crypto_aead_input structure, \field{dst_data_addr} specifies destination
-data guest physical address used to store the results of the CIPHER operations; \field{digest_result_addr} specifies
-the digest result guest physical address used to store the results of the HASH/MAC operations; \field{status} specifies
-the status of AEAD operation.
-
-In the virtio_crypto_aead_output structure, \field{iv_addr} specifies the guest physical address of initialization vector,
-\field{src_data_addr} specifies the source data guest physical address, \field{add_data_addr} specifies the guest physical address
-of additional authentication data.
-
-The addresses and length can determine exclusive content in the guest memory.
+The output data here includs the IV/Counter buffer, source buffer and additional authenticated data buffer if exists.
+The input data includes both destionation buffer used to save the results of the AEAD operations.
+\field{inhdr} store the executing status of AEAD operations.
 
 \begin{note}
-The guest memory MUST be guaranteed to be allocated and physically-contiguous
-pointed by \field{dst_data_addr} and \field{digest_result_addr} in struct virtio_crypto_aead_input, 
-\field{iv_addr}, \field{src_data_addr}  and \field{add_data_addr} in struct virtio_crypto_aead_output.
+The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
+the throughput of data transmitted for the AEAD service, so that the virtio crypto device can be better accelerated.
 \end{note}
 
 \drivernormative{\paragraph}{AEAD Service Operation}{Device Types / Crypto Device / Device Operation / AEAD Service Operation}
@@ -1002,8 +942,8 @@ pointed by \field{dst_data_addr} and \field{digest_result_addr} in struct virtio
 
 \begin{itemize*}
 \item 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 guest memory recorded by \field{dst_data_addr} field in struct virtio_crypto_aead_input.
-\item The device MUST copy the digest result to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_aead_input.
-\item The device MUST set the \field{status} field in strut virtio_crypto_aead_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported.
+\item The device MUST copy the result of cryptographic operation to the guest memory recorded by destination buffer.
+\item The device MUST copy the digest result to the guest memory recorded by digest result buffer.
+\item The device MUST set the \field{status} field in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
 \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


Regards,
-Gonglei



> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Thursday, November 10, 2016 10:33 AM
> To: 'Cornelia Huck'
> Cc: Halil Pasic; qemu-devel@nongnu.org; virtio-dev@lists.oasis-open.org;
> Huangweidong (C); mst@redhat.com; john.griffin@intel.com; Shiqing Fan;
> Zhoujian (jay, Euler); Varun.Sethi@freescale.com; denglingli@chinamobile.com;
> arei.gonglei@hotmail.com; Hanweidong (Randy); agraf@suse.de;
> nmorey@kalray.eu; vincent.jardin@6wind.com; Ola.Liljedahl@arm.com;
> Luonengjun; xin.zeng@intel.com; Huangpeng (Peter); liang.j.ma@intel.com;
> stefanha@redhat.com; Jani Kokkonen; brian.a.keating@intel.com; Claudio
> Fontana; mike.caraman@nxp.com; Wubin (H)
> Subject: RE: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio crypto
> device specification
> 
> > From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> > Sent: Wednesday, November 09, 2016 11:25 PM
> > Subject: Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio crypto
> > device specification
> >
> > On Wed, 9 Nov 2016 01:11:20 +0000
> > "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> >
> > > Nope, Actually I kept those description here is because I wanted to
> represent
> > each packet
> > > Intuitionally, otherwise I don't know how to explain them only occupy one
> > entry in desc table
> > > by indirect table method. So I changed the code completely as Stefan's
> > suggestion and
> > > revised the spec a little.
> >
> > I think you need to revise the spec a bit more :) IIUC, you should
> > simply state that you use the indirect table method and not mention any
> > guest physical addresses.
> >
> Yes, will remove the gpa description stuff. Using indirect table is a method for
> high performance
> and throughput, I'll mention it as a note. Pls see my another reply to Michael.
> 
> > > This just is a representative of the realization so that the people can easily
> > understand what
> > > the virtio crypto request's components. It isn't completely same with the
> > code.
> > > For virtio-scsi device, the struct virtio_scsi_req_cmd also used this way
> IIUR.
> >
> > It seemes to have caused at least some confusion - perhaps you
> > simplyfied too much?
> 
> Yes, I will.
> 
> Thanks,
> -Gonglei

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

[Michael S. Tsirkin]

On Thu, Nov 10, 2016 at 02:25:44AM +0000, Gonglei (Arei) wrote:
> 
> >
> > Subject: [virtio-dev] Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio
> > crypto device specification
> > 
> > On Wed, Nov 09, 2016 at 01:11:20AM +0000, Gonglei (Arei) wrote:
> > > Nope, Actually I kept those description here is because I wanted to represent
> > each packet
> > > Intuitionally, otherwise I don't know how to explain them only occupy one
> > entry in desc table
> > > by indirect table method.
> > 
> > whether to use the indirect method should not be up to device.
> > I don't see where does the spec require indirect method,
> > ANY_LAYOUT is implied so spec should not dictate that IMO.
> > 
> Yes, I reread the virtio spec. And
> 
> 2.4.4 says: 
>  " The framing of messages with descriptors is independent of the contents of the buffers."
> 
> 2.4.4.2 says:
>  "The driver SHOULD NOT use an excessive number of descriptors to describe a buffer."
> 
> For virtio-crypto, I think I can just add a NOTE to remind people using indirect method 
> if they want to get better performance in production environment, can I? 
> 
> 
> Thanks,
> -Gonglei

I'm not sure why frankly. Is there a reason crypto is special here?

> > > So I changed the code completely as Stefan's suggestion and
> > > revised the spec a little.
> > >
> > > This just is a representative of the realization so that the people can easily
> > understand what
> > > the virtio crypto request's components. It isn't completely same with the
> > code.
> > > For virtio-scsi device, the struct virtio_scsi_req_cmd also used this way IIUR.
> > >
> > > 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

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

[Michael S. Tsirkin]

On Thu, Nov 10, 2016 at 09:37:40AM +0000, Gonglei (Arei) wrote:
> Hi,
> 
> I attach a diff for next version in order to review more convenient, with
> 
>  - Drop the all gap stuff;
>  - Drop all structures undefined in virtio_crypto.h
>  - re-describe per request for per crypto service avoid confusion
> 
> Pls review, thanks!
> 
> 
> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> index 448296e..ab17e7b 100644
> --- a/virtio-crypto.tex
> +++ b/virtio-crypto.tex
> @@ -69,13 +69,13 @@ The following services are defined:
>  
>  \begin{lstlisting}
>  /* CIPHER service */
> -#define VIRTIO_CRYPTO_SERVICE_CIPHER (0)
> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
>  /* HASH service */
> -#define VIRTIO_CRYPTO_SERVICE_HASH   (1)
> +#define VIRTIO_CRYPTO_SERVICE_HASH   1
>  /* MAC (Message Authentication Codes) service */
> -#define VIRTIO_CRYPTO_SERVICE_MAC    (2)
> +#define VIRTIO_CRYPTO_SERVICE_MAC    2
>  /* AEAD (Authenticated Encryption with Associated Data) service */
> -#define VIRTIO_CRYPTO_SERVICE_AEAD   (3)
> +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
>  \end{lstlisting}
>  
>  The last driver-read-only fields specify detailed algorithms masks 
> @@ -210,7 +210,7 @@ data that should be utilized in operations, and input data is equal to
>  The general header for controlq is as follows:
>  
>  \begin{lstlisting}
> -#define VIRTIO_CRYPTO_OPCODE(service, op)   ((service << 8) | (op))
> +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
>  
>  struct virtio_crypto_ctrl_header {
>  #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> @@ -380,20 +380,18 @@ struct virtio_crypto_mac_session_para {
>      le32 auth_key_len;
>      le32 padding;
>  };
> -struct virtio_crypto_mac_session_output {
> -    le64 auth_key_addr; /* guest key physical address */
> -};
>  
>  struct virtio_crypto_mac_create_session_req {
>      /* Device-readable part */
>      struct virtio_crypto_mac_session_para para;
> -    struct virtio_crypto_mac_session_output out;
> +    /* The authenticated key buffer */
> +    /* output data here */
> +
>      /* Device-writable part */
>      struct virtio_crypto_session_input input;
>  };
>  \end{lstlisting}
>  
> -The output data are
>  \subparagraph{Session operation: Symmetric algorithms session}\label{sec:Device Types / Crypto Device / Device
>  Operation / Control Virtqueue / Session operation / Session operation: Symmetric algorithms session}
>  
> @@ -413,13 +411,13 @@ struct virtio_crypto_cipher_session_para {
>      le32 padding;
>  };
>  
> -struct virtio_crypto_cipher_session_output {
> -    le64 key_addr; /* guest key physical address */
> -};
> -
>  struct virtio_crypto_cipher_session_req {
> +    /* Device-readable part */
>      struct virtio_crypto_cipher_session_para para;
> -    struct virtio_crypto_cipher_session_output out;
> +    /* The cipher key buffer */
> +    /* Output data here */
> +
> +    /* Device-writable part */
>      struct virtio_crypto_session_input input;
>  };
>  \end{lstlisting}
> @@ -448,18 +446,20 @@ struct virtio_crypto_alg_chain_session_para {
>      le32 padding;
>  };
>  
> -struct virtio_crypto_alg_chain_session_output {
> -    struct virtio_crypto_cipher_session_output cipher;
> -    struct virtio_crypto_mac_session_output mac;
> -};
> -
>  struct virtio_crypto_alg_chain_session_req {
> +    /* Device-readable part */
>      struct virtio_crypto_alg_chain_session_para para;
> -    struct virtio_crypto_alg_chain_session_output out;
> +    /* The cipher key buffer */
> +    /* The authenticated key buffer */
> +    /* Output data here */
> +
> +    /* Device-writable part */
>      struct virtio_crypto_session_input input;
>  };
>  \end{lstlisting}
>  
> +The output data includs both cipher key buffer and authenticated key buffer.

.. includes both the cipher key buffer and the uthenticated key buffer.

> +
>  The packet for symmetric algorithm is as follows:
>  
>  \begin{lstlisting}
> @@ -503,13 +503,13 @@ struct virtio_crypto_aead_session_para {
>      le32 padding;
>  };
>  
> -struct virtio_crypto_aead_session_output {
> -    le64 key_addr; /* guest key physical address */
> -};
> -
>  struct virtio_crypto_aead_create_session_req {
> +    /* Device-readable part */
>      struct virtio_crypto_aead_session_para para;
> -    struct virtio_crypto_aead_session_output out;
> +    /* The cipher key buffer */
> +    /* Output data here */
> +
> +    /* Device-writeable part */
>      struct virtio_crypto_session_input input;
>  };
>  \end{lstlisting}
> @@ -568,19 +568,13 @@ struct virtio_crypto_op_data_req {
>  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 idata structure for all symmetric algorithms, including CIPHER, HASH, MAC, and AEAD.
> +There is a unified idata structure for all service, including CIPHER, HASH, MAC, and AEAD.

for all services

>  
>  The structure is defined as follows:
>  
>  \begin{lstlisting}
> -struct virtio_crypto_sym_input {
> -    /* Destination data guest address, it's useless for plain HASH and MAC */
> -    le64 dst_data_addr;
> -    /* Digest result guest address, it's useless for plain cipher algos */

guest address -> physical address?
it's useless -> unused?

> -    le64 digest_result_addr;
> -
> -    le32 status;
> -    le32 padding;
> +struct virtio_crypto_inhdr {
> +    u8 status;
>  };
>  
>  \end{lstlisting}
> @@ -595,39 +589,29 @@ struct virtio_crypto_hash_para {
>      le32 hash_result_len;
>  };
>  
> -struct virtio_crypto_hash_input {
> -    struct virtio_crypto_sym_input input;
> -};
> -
> -struct virtio_crypto_hash_output {
> -    /* source data guest address */

guest -> physical?

> -    le64 src_data_addr;
> -};
> -
>  struct virtio_crypto_hash_data_req {
>      /* Device-readable part */
>      struct virtio_crypto_hash_para para;
> -    struct virtio_crypto_hash_output odata;
> +    /* Source buffer */
> +    /* Output data here */
> +
>      /* Device-writable part */
> -    struct virtio_crypto_hash_input idata;
> +    /* Digest result buffer */
> +    /* Input data here */
> +    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 request only occupies one entry
> -in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> -the throughput of data transmitted for the HASH service, so that the virtio crypto
> -device can be better accelerated.
> +used to run the HASH operations. 
>  
> -The information includes the source data guest physical address stored by \field{odata}.\field{src_data_addr},
> -length of source data stored by \field{para}.\field{src_data_len}, and the digest result guest physical address
> -stored by \field{digest_result_addr} used to save the results of the HASH operations.
> -The address and length can determine exclusive content in the guest memory.
> +The information includes the hash paramenters stored by \field{para}, output data and input data.
> +The output data here includs the source buffer and the input data includes the digest result buffer used to save the results of the HASH operations.

includs -> includes

> +\field{inhdr} store the executing status of HASH operations.
>  
>  \begin{note}
> -The guest memory is always guaranteed to be allocated and physically-contiguous
> -pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input and
> -\field{src_data_addr} in struct virtio_crypto_hash_output.
> +The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves

Don't use should outside confirmance statements.

occupies -> occupy

> +the throughput of data transmitted for the HASH service, so that the virtio crypto device can be better accelerated.

I'd just drop this note - I don't see why is crypto special here.

>  \end{note}
>  
>  \drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
> @@ -641,8 +625,8 @@ pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input and
>  \devicenormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
>  
>  \begin{itemize*}
> -\item The device MUST copy the results of HASH operations to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_hash_input.
> -\item The device MUST set \field{status} in strut virtio_crypto_hash_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
> +\item The device MUST copy the results of HASH operations to the guest memory recorded by digest result buffer if HASH operations success.
> +\item The device MUST set \field{status} in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.

strut -> struct?

add "to one of the values"? Also, just list the enum name here in case
we add more values?

not support - not supported?

>  \end{itemize*}
>  
>  \subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / MAC Service Operation}
> @@ -652,39 +636,29 @@ struct virtio_crypto_mac_para {
>      struct virtio_crypto_hash_para hash;
>  };
>  
> -struct virtio_crypto_mac_input {
> -    struct virtio_crypto_sym_input input;
> -};
> -
> -struct virtio_crypto_mac_output {
> -    struct virtio_crypto_hash_output hash_output;
> -};
> -
>  struct virtio_crypto_mac_data_req {
>      /* Device-readable part */
>      struct virtio_crypto_mac_para para;
> -    struct virtio_crypto_mac_output odata;
> +    /* Source buffer */
> +    /* Output data here */
> +
>      /* Device-writable part */
> -    struct virtio_crypto_mac_input idata;
> +    /* Digest result buffer */
> +    /* Input data here */
> +    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 request only occupies one entry
> -in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> -the throughput of data transmitted for the MAC service, so that the virtio crypto
> -device can get the better result of acceleration.
> -
> -The information includes the source data guest physical address stored by \field{hash_output}.\field{src_data}.\field{addr},
> -the length of source data stored by \field{hash_output}.\field{src_data}.\field{len}, and the digest result guest physical address
> -stored by \field{digest_result_addr} used to save the results of the MAC operations.
> +used to run the MAC operations. 
>  
> -The address and length can determine exclusive content in the guest memory.
> +The information includes the hash paramenters stored by \field{para}, output data and input data.
> +The output data here includs the source buffer and the input data includes the digest result buffer used to save the results of the MAC operations.



includes

> +\field{inhdr} store the executing status of MAC operations.

stores

the executing status -> status of executing the MAC operations?

>  
>  \begin{note}
> -The guest memory is always guaranteed to be allocated and physically-contiguous
> -pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input and
> -\field{hash_output}.\field{src_data_addr} in struct virtio_crypto_mac_output.
> +The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> +the throughput of data transmitted for the MAC service, so that the virtio crypto device can be better accelerated.

Again, let's just drop.

>  \end{note}
>  
>  \drivernormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
> @@ -698,8 +672,8 @@ pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input and
>  \devicenormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
>  
>  \begin{itemize*}
> -\item The device MUST copy the results of MAC operations to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_mac_input.
> -\item The device MUST set \field{status} in strut virtio_crypto_mac_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
> +\item The device MUST copy the results of MAC operations to the guest memory recorded by digest result buffer if HASH operations success.
> +\item The device MUST set \field{status} in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.


same as above. I guess same issues repeat below, did not review.

>  \end{itemize*}
>  
>  \subsubsection{Symmetric algorithms Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
> @@ -709,12 +683,12 @@ The packet of plain CIPHER service is as follows:
>  \begin{lstlisting}
>  struct virtio_crypto_cipher_para {
>      /*
> -     * Byte Length of valid IV data pointed to by the below iv_addr.
> +     * Byte Length of valid IV/Counter data pointed to by the below iv buffer.
>       *
> -     * - For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
> +     * 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
> +     * 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;
> @@ -725,34 +699,28 @@ struct virtio_crypto_cipher_para {
>      le32 padding;
>  };
>  
> -struct virtio_crypto_cipher_input {
> -    struct virtio_crypto_sym_input input;
> -};
> -
> -struct virtio_crypto_cipher_output {
> -   /*
> -     * Initialization Vector or Counter Guest Address.
> +struct virtio_crypto_cipher_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_cipher_para para;
> +    /*
> +     * Initialization Vector or Counter buffer.
>       *
> -     * - For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
> +     * 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.
> +     * 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.
>       */
> -    le64 iv_addr;
> -    /* source data guest address */
> -    le64 src_data_addr;
> -};
> +    /* Source buffer */
> +    /* Output data here */
>  
> -struct virtio_crypto_cipher_data_req {
> -    /* Device-readable part */
> -    struct virtio_crypto_cipher_para para;
> -    struct virtio_crypto_cipher_output odata;
>      /* Device-writable part */
> -    struct virtio_crypto_cipher_input idata;
> +    /* Destination data buffer */
> +    /* Input data here */
> +    struct virtio_crypto_inhdr inhdr;
>  };
>  \end{lstlisting}
>  
> @@ -780,23 +748,19 @@ struct virtio_crypto_alg_chain_data_para {
>      __virtio32 reserved;
>  };
>  
> -struct virtio_crypto_alg_chain_data_output {
> -    /* Device-readable part */
> -    struct virtio_crypto_cipher_output cipher;
> -    /* Additional auth data guest address */
> -    le64 aad_data_addr;
> -};
> -
> -struct virtio_crypto_alg_chain_data_input {
> -    struct virtio_crypto_sym_input input;
> -};
> -
>  struct virtio_crypto_alg_chain_data_req {
>      /* Device-readable part */
>      struct virtio_crypto_alg_chain_data_para para;
> -    struct virtio_crypto_alg_chain_data_output odata;
> +    /* Initialization Vector or Counter buffer */
> +    /* Source buffer */
> +    /* Additional authenticated buffer if exists  */
> +    /* Output data here */
> +
>      /* Device-writable part */
> -    struct virtio_crypto_alg_chain_data_input idata;
> +    /* Destination data buffer */
> +    /* Digest result buffer */
> +    /* Input data here */
> +    struct virtio_crypto_inhdr inhdr;
>  };
>  \end{lstlisting}
>  
> @@ -817,29 +781,22 @@ struct virtio_crypto_sym_data_req {
>  };
>  \end{lstlisting}
>  
> -Each data request uses the virtio_crypto_cipher_data_req structure to store information
> -used to run the CIPHER operations. The request only occupies one entry
> -in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> -the throughput of data transmitted for the CIPHER service, so that the virtio crypto
> -device can get the better result of acceleration.
> +Each data request uses virtio_crypto_sym_data_req structure to store information
> +used to run the CIPHER operations. 
>  
> +The information includes the hash paramenters 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.
> +length of the destination data. 
> +For plain CIPHER operations, the output data here includs the IV/Counter buffer and source buffer, and the input data includes the destination buffer used to save the results of the CIPHER operations.
>  
> -In the following virtio_crypto_cipher_input structure, \field{dst_data_addr} specifies the destination
> -data guest physical address used to store the results of the CIPHER operations, and \field{status} specifies
> -the CIPHER operation status.
> -
> -In the virtio_crypto_cipher_output structure, \field{iv_addr} specifies the guest physical address of initialization vector,
> -\field{src_data_addr} specifies the source data guest physical address.
> -
> -The addresses and length can determine exclusive content in the guest memory.
> +For algorithms chain, the output data here includs the IV/Counter buffer, source buffer and additional authenticated data buffer if exists.
> +The input data includes both destionation buffer and digest result buffer used to store the results of the HASH/MAC operations.
> +\field{inhdr} store the executing status of crypto operations.
>  
>  \begin{note}
> -The guest memory is always guaranteed to be allocated and physically-contiguous
> -pointed by \field{dst_data_addr} in struct virtio_crypto_cipher_input, 
> -\field{iv_addr} and \field{src_data_addr} in struct virtio_crypto_cipher_output.
> +The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> +the throughput of data transmitted for the crypto service, so that the virtio crypto device can be better accelerated.
>  \end{note}
>  
>  \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation}
> @@ -860,10 +817,10 @@ pointed by \field{dst_data_addr} in struct virtio_crypto_cipher_input,
>  \item The device SHOULD only parse fields of struct virtio_crypto_cipher_data_req in struct virtio_crypto_sym_data_req if the created session 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 created
>  session 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 guest memory recorded by \field{dst_data_addr} field in struct virtio_crypto_cipher_input in plain CIPHER mode.
> -\item The device MUST copy the result of HASH/MAC operation to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_alg_chain_data_input is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type.
> -\item The device MUST set the \field{status} field in strut virtio_crypto_cipher_input or virtio_crypto_alg_chain_data_input structure:
> -VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported.
> +\item The device MUST copy the result of cryptographic operation to the guest memory recorded by destination buffer in plain CIPHER mode.
> +\item The device MUST check the \field{para}.\field{add_len} is bigger than 0 before parse the additional authenticated buffer in plain algorithms chain mode.
> +\item The device MUST copy the result of HASH/MAC operation to the guest memory recorded by digest result buffer is of the VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING type.
> +\item The device MUST set the \field{status} field in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
>  \end{itemize*}
>  
>  \paragraph{Steps of Operation}\label{sec:Device Types / Crypto Device / Device Operation / Symmetric algorithms Operation / Steps of Operation}
> @@ -894,17 +851,15 @@ VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; V
>  \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_cipher_input;
> +\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 result.
>  \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*}
> +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{note}
>  
>  \subsubsection{AEAD Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / AEAD Service Operation}
> @@ -912,11 +867,11 @@ VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; V
>  \begin{lstlisting}
>  struct virtio_crypto_aead_para {
>      /*
> -     * Byte Length of valid IV data pointed to by the below iv_addr parameter.
> +     * Byte Length of valid IV data.
>       *
> -     * - For GCM mode, this is either 12 (for 96-bit IVs) or 16, in which
> -     *   case iv_addr points to J0.
> -     * - For CCM mode, this is the length of the nonce, which can be in the
> +     * 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;
> @@ -928,66 +883,51 @@ struct virtio_crypto_aead_para {
>      le32 dst_data_len;
>  };
>  
> -struct virtio_crypto_aead_input {
> -    struct virtio_crypto_sym_input input;
> -};
> -
> -struct virtio_crypto_aead_output {
> +struct virtio_crypto_aead_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_aead_para para;
>      /*
> -     * Initialization Vector Guest Address.
> +     * Initialization Vector buffer.
>       *
> -     * - For GCM mode, this is either the IV (if the length is 96 bits) or J0
> +     * 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_addr[1] (to allow space for the implementation
> +     * For CCM mode, the first byte is reserved, and the nonce should be
> +     *   written starting at &iv_buffer[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.
>       */
> -    le64 iv_addr;
> -    /* source data guest address */
> -    le64 src_data_addr;
> -    /* additional auth data guest address */
> -    le64 add_data_addr;
> -};
> +    /* Source buffer */
> +    /* Additional authenticated buffer if exists  */
> +    /* Output data here */
>  
> -struct virtio_crypto_aead_data_req {
> -    /* Device-readable part */
> -    struct virtio_crypto_aead_para para;
> -    struct virtio_crypto_aead_output odata;
>      /* Device-writable part */
> -    struct virtio_crypto_aead_input idata;
> +    /* Destination data buffer */
> +    /* Digest result buffer */
> +    /* Input data here */
> +    struct virtio_crypto_inhdr inhdr;
>  };
>  \end{lstlisting}
>  
>  Each data request uses virtio_crypto_aead_data_req structure to store information
> -used to implement the CIPHER operations. The request only occupies one entry
> -in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> -the throughput of data transmitted for the AEAD service, so that the virtio crypto
> -device can be better accelerated.
> +used to run the AEAD operations. 
>  
> -In the first virtio_crypto_aead_para structure, \field{iv_len} specifies the length of the initialization vector;
> +The information includes the hash paramenters 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{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.
> +The output data here includs the IV buffer and source buffer, and the input data includes the destination buffer used to save the results of the CIPHER operations.
>  
> -In the following virtio_crypto_aead_input structure, \field{dst_data_addr} specifies destination
> -data guest physical address used to store the results of the CIPHER operations; \field{digest_result_addr} specifies
> -the digest result guest physical address used to store the results of the HASH/MAC operations; \field{status} specifies
> -the status of AEAD operation.
> -
> -In the virtio_crypto_aead_output structure, \field{iv_addr} specifies the guest physical address of initialization vector,
> -\field{src_data_addr} specifies the source data guest physical address, \field{add_data_addr} specifies the guest physical address
> -of additional authentication data.
> -
> -The addresses and length can determine exclusive content in the guest memory.
> +The output data here includs the IV/Counter buffer, source buffer and additional authenticated data buffer if exists.
> +The input data includes both destionation buffer used to save the results of the AEAD operations.
> +\field{inhdr} store the executing status of AEAD operations.
>  
>  \begin{note}
> -The guest memory MUST be guaranteed to be allocated and physically-contiguous
> -pointed by \field{dst_data_addr} and \field{digest_result_addr} in struct virtio_crypto_aead_input, 
> -\field{iv_addr}, \field{src_data_addr}  and \field{add_data_addr} in struct virtio_crypto_aead_output.
> +The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> +the throughput of data transmitted for the AEAD service, so that the virtio crypto device can be better accelerated.
>  \end{note}
>  
>  \drivernormative{\paragraph}{AEAD Service Operation}{Device Types / Crypto Device / Device Operation / AEAD Service Operation}
> @@ -1002,8 +942,8 @@ pointed by \field{dst_data_addr} and \field{digest_result_addr} in struct virtio
>  
>  \begin{itemize*}
>  \item 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 guest memory recorded by \field{dst_data_addr} field in struct virtio_crypto_aead_input.
> -\item The device MUST copy the digest result to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_aead_input.
> -\item The device MUST set the \field{status} field in strut virtio_crypto_aead_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported.
> +\item The device MUST copy the result of cryptographic operation to the guest memory recorded by destination buffer.
> +\item The device MUST copy the digest result to the guest memory recorded by digest result buffer.
> +\item The device MUST set the \field{status} field in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
>  \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
> 
> 
> Regards,
> -Gonglei
> 
> 
> 
> > -----Original Message-----
> > From: Gonglei (Arei)
> > Sent: Thursday, November 10, 2016 10:33 AM
> > To: 'Cornelia Huck'
> > Cc: Halil Pasic; qemu-devel@nongnu.org; virtio-dev@lists.oasis-open.org;
> > Huangweidong (C); mst@redhat.com; john.griffin@intel.com; Shiqing Fan;
> > Zhoujian (jay, Euler); Varun.Sethi@freescale.com; denglingli@chinamobile.com;
> > arei.gonglei@hotmail.com; Hanweidong (Randy); agraf@suse.de;
> > nmorey@kalray.eu; vincent.jardin@6wind.com; Ola.Liljedahl@arm.com;
> > Luonengjun; xin.zeng@intel.com; Huangpeng (Peter); liang.j.ma@intel.com;
> > stefanha@redhat.com; Jani Kokkonen; brian.a.keating@intel.com; Claudio
> > Fontana; mike.caraman@nxp.com; Wubin (H)
> > Subject: RE: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio crypto
> > device specification
> > 
> > > From: Cornelia Huck [mailto:cornelia.huck@de.ibm.com]
> > > Sent: Wednesday, November 09, 2016 11:25 PM
> > > Subject: Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio crypto
> > > device specification
> > >
> > > On Wed, 9 Nov 2016 01:11:20 +0000
> > > "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> > >
> > > > Nope, Actually I kept those description here is because I wanted to
> > represent
> > > each packet
> > > > Intuitionally, otherwise I don't know how to explain them only occupy one
> > > entry in desc table
> > > > by indirect table method. So I changed the code completely as Stefan's
> > > suggestion and
> > > > revised the spec a little.
> > >
> > > I think you need to revise the spec a bit more :) IIUC, you should
> > > simply state that you use the indirect table method and not mention any
> > > guest physical addresses.
> > >
> > Yes, will remove the gpa description stuff. Using indirect table is a method for
> > high performance
> > and throughput, I'll mention it as a note. Pls see my another reply to Michael.
> > 
> > > > This just is a representative of the realization so that the people can easily
> > > understand what
> > > > the virtio crypto request's components. It isn't completely same with the
> > > code.
> > > > For virtio-scsi device, the struct virtio_scsi_req_cmd also used this way
> > IIUR.
> > >
> > > It seemes to have caused at least some confusion - perhaps you
> > > simplyfied too much?
> > 
> > Yes, I will.
> > 
> > Thanks,
> > -Gonglei

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

[Halil Pasic]

On 11/10/2016 02:15 PM, Michael S. Tsirkin wrote:
> On Thu, Nov 10, 2016 at 09:37:40AM +0000, Gonglei (Arei) wrote:
>> > Hi,
>> > 
>> > I attach a diff for next version in order to review more convenient, with
>> > 
>> >  - Drop the all gap stuff;
>> >  - Drop all structures undefined in virtio_crypto.h
>> >  - re-describe per request for per crypto service avoid confusion
>> > 
>> > Pls review, thanks!
>> > 
>> > 
>> > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
>> > index 448296e..ab17e7b 100644
>> > --- a/virtio-crypto.tex
>> > +++ b/virtio-crypto.tex
>> > @@ -69,13 +69,13 @@ The following services are defined:
>> >  
>> >  \begin{lstlisting}
>> >  /* CIPHER service */
>> > -#define VIRTIO_CRYPTO_SERVICE_CIPHER (0)
>> > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
>> >  /* HASH service */
>> > -#define VIRTIO_CRYPTO_SERVICE_HASH   (1)
>> > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
>> >  /* MAC (Message Authentication Codes) service */
>> > -#define VIRTIO_CRYPTO_SERVICE_MAC    (2)
>> > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
>> >  /* AEAD (Authenticated Encryption with Associated Data) service */
>> > -#define VIRTIO_CRYPTO_SERVICE_AEAD   (3)
>> > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
>> >  \end{lstlisting}
>> >  
>> >  The last driver-read-only fields specify detailed algorithms masks 
>> > @@ -210,7 +210,7 @@ data that should be utilized in operations, and input data is equal to
>> >  The general header for controlq is as follows:
>> >  
>> >  \begin{lstlisting}
>> > -#define VIRTIO_CRYPTO_OPCODE(service, op)   ((service << 8) | (op))
>> > +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
>> >  
>> >  struct virtio_crypto_ctrl_header {
>> >  #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
>> > @@ -380,20 +380,18 @@ struct virtio_crypto_mac_session_para {
>> >      le32 auth_key_len;
>> >      le32 padding;
>> >  };
>> > -struct virtio_crypto_mac_session_output {
>> > -    le64 auth_key_addr; /* guest key physical address */
>> > -};
>> >  
>> >  struct virtio_crypto_mac_create_session_req {
>> >      /* Device-readable part */
>> >      struct virtio_crypto_mac_session_para para;
>> > -    struct virtio_crypto_mac_session_output out;
>> > +    /* The authenticated key buffer */
>> > +    /* output data here */
>> > +
>> >      /* Device-writable part */
>> >      struct virtio_crypto_session_input input;
>> >  };
>> >  \end{lstlisting}
>> >  
>> > -The output data are
>> >  \subparagraph{Session operation: Symmetric algorithms session}\label{sec:Device Types / Crypto Device / Device
>> >  Operation / Control Virtqueue / Session operation / Session operation: Symmetric algorithms session}
>> >  
>> > @@ -413,13 +411,13 @@ struct virtio_crypto_cipher_session_para {
>> >      le32 padding;
>> >  };
>> >  
>> > -struct virtio_crypto_cipher_session_output {
>> > -    le64 key_addr; /* guest key physical address */
>> > -};
>> > -
>> >  struct virtio_crypto_cipher_session_req {
>> > +    /* Device-readable part */
>> >      struct virtio_crypto_cipher_session_para para;
>> > -    struct virtio_crypto_cipher_session_output out;
>> > +    /* The cipher key buffer */
>> > +    /* Output data here */
>> > +
>> > +    /* Device-writable part */
>> >      struct virtio_crypto_session_input input;
>> >  };
>> >  \end{lstlisting}
>> > @@ -448,18 +446,20 @@ struct virtio_crypto_alg_chain_session_para {
>> >      le32 padding;
>> >  };
>> >  
>> > -struct virtio_crypto_alg_chain_session_output {
>> > -    struct virtio_crypto_cipher_session_output cipher;
>> > -    struct virtio_crypto_mac_session_output mac;
>> > -};
>> > -
>> >  struct virtio_crypto_alg_chain_session_req {
>> > +    /* Device-readable part */
>> >      struct virtio_crypto_alg_chain_session_para para;
>> > -    struct virtio_crypto_alg_chain_session_output out;
>> > +    /* The cipher key buffer */
>> > +    /* The authenticated key buffer */
>> > +    /* Output data here */
>> > +
>> > +    /* Device-writable part */
>> >      struct virtio_crypto_session_input input;
>> >  };
>> >  \end{lstlisting}
>> >  
>> > +The output data includs both cipher key buffer and authenticated key buffer.
> .. includes both the cipher key buffer and the uthenticated key buffer.
> 
>> > +
>> >  The packet for symmetric algorithm is as follows:
>> >  
>> >  \begin{lstlisting}
>> > @@ -503,13 +503,13 @@ struct virtio_crypto_aead_session_para {
>> >      le32 padding;
>> >  };
>> >  
>> > -struct virtio_crypto_aead_session_output {
>> > -    le64 key_addr; /* guest key physical address */
>> > -};
>> > -
>> >  struct virtio_crypto_aead_create_session_req {
>> > +    /* Device-readable part */
>> >      struct virtio_crypto_aead_session_para para;
>> > -    struct virtio_crypto_aead_session_output out;
>> > +    /* The cipher key buffer */
>> > +    /* Output data here */
>> > +
>> > +    /* Device-writeable part */
>> >      struct virtio_crypto_session_input input;
>> >  };
>> >  \end{lstlisting}
>> > @@ -568,19 +568,13 @@ struct virtio_crypto_op_data_req {
>> >  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 idata structure for all symmetric algorithms, including CIPHER, HASH, MAC, and AEAD.
>> > +There is a unified idata structure for all service, including CIPHER, HASH, MAC, and AEAD.
> for all services
> 
>> >  
>> >  The structure is defined as follows:
>> >  
>> >  \begin{lstlisting}
>> > -struct virtio_crypto_sym_input {
>> > -    /* Destination data guest address, it's useless for plain HASH and MAC */
>> > -    le64 dst_data_addr;
>> > -    /* Digest result guest address, it's useless for plain cipher algos */
> guest address -> physical address?
> it's useless -> unused?
> 
>> > -    le64 digest_result_addr;
>> > -
>> > -    le32 status;
>> > -    le32 padding;
>> > +struct virtio_crypto_inhdr {
>> > +    u8 status;
>> >  };
>> >  
>> >  \end{lstlisting}
>> > @@ -595,39 +589,29 @@ struct virtio_crypto_hash_para {
>> >      le32 hash_result_len;
>> >  };
>> >  
>> > -struct virtio_crypto_hash_input {
>> > -    struct virtio_crypto_sym_input input;
>> > -};
>> > -
>> > -struct virtio_crypto_hash_output {
>> > -    /* source data guest address */
> guest -> physical?
> 
>> > -    le64 src_data_addr;
>> > -};
>> > -
>> >  struct virtio_crypto_hash_data_req {
>> >      /* Device-readable part */
>> >      struct virtio_crypto_hash_para para;
>> > -    struct virtio_crypto_hash_output odata;
>> > +    /* Source buffer */
>> > +    /* Output data here */
>> > +
>> >      /* Device-writable part */
>> > -    struct virtio_crypto_hash_input idata;
>> > +    /* Digest result buffer */
>> > +    /* Input data here */
>> > +    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 request only occupies one entry
>> > -in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
>> > -the throughput of data transmitted for the HASH service, so that the virtio crypto
>> > -device can be better accelerated.
>> > +used to run the HASH operations. 
>> >  
>> > -The information includes the source data guest physical address stored by \field{odata}.\field{src_data_addr},
>> > -length of source data stored by \field{para}.\field{src_data_len}, and the digest result guest physical address
>> > -stored by \field{digest_result_addr} used to save the results of the HASH operations.
>> > -The address and length can determine exclusive content in the guest memory.
>> > +The information includes the hash paramenters stored by \field{para}, output data and input data.
>> > +The output data here includs the source buffer and the input data includes the digest result buffer used to save the results of the HASH operations.
> includs -> includes
> 
>> > +\field{inhdr} store the executing status of HASH operations.
>> >  
>> >  \begin{note}
>> > -The guest memory is always guaranteed to be allocated and physically-contiguous
>> > -pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input and
>> > -\field{src_data_addr} in struct virtio_crypto_hash_output.
>> > +The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
> Don't use should outside confirmance statements.
> 
> occupies -> occupy
> 
>> > +the throughput of data transmitted for the HASH service, so that the virtio crypto device can be better accelerated.
> I'd just drop this note - I don't see why is crypto special here.
> 
>> >  \end{note}
>> >  
>> >  \drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
>> > @@ -641,8 +625,8 @@ pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input and
>> >  \devicenormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
>> >  
>> >  \begin{itemize*}
>> > -\item The device MUST copy the results of HASH operations to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_hash_input.
>> > -\item The device MUST set \field{status} in strut virtio_crypto_hash_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
>> > +\item The device MUST copy the results of HASH operations to the guest memory recorded by digest result buffer if HASH operations success.
>> > +\item The device MUST set \field{status} in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
> strut -> struct?
> 
> add "to one of the values"? Also, just list the enum name here in case
> we add more values?
> 
> not support - not supported?
> 
>> >  \end{itemize*}
>> >  
>> >  \subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / MAC Service Operation}
>> > @@ -652,39 +636,29 @@ struct virtio_crypto_mac_para {
>> >      struct virtio_crypto_hash_para hash;
>> >  };
>> >  
>> > -struct virtio_crypto_mac_input {
>> > -    struct virtio_crypto_sym_input input;
>> > -};
>> > -
>> > -struct virtio_crypto_mac_output {
>> > -    struct virtio_crypto_hash_output hash_output;
>> > -};
>> > -
>> >  struct virtio_crypto_mac_data_req {
>> >      /* Device-readable part */
>> >      struct virtio_crypto_mac_para para;
>> > -    struct virtio_crypto_mac_output odata;
>> > +    /* Source buffer */
>> > +    /* Output data here */
>> > +
>> >      /* Device-writable part */
>> > -    struct virtio_crypto_mac_input idata;
>> > +    /* Digest result buffer */
>> > +    /* Input data here */
>> > +    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 request only occupies one entry
>> > -in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
>> > -the throughput of data transmitted for the MAC service, so that the virtio crypto
>> > -device can get the better result of acceleration.
>> > -
>> > -The information includes the source data guest physical address stored by \field{hash_output}.\field{src_data}.\field{addr},
>> > -the length of source data stored by \field{hash_output}.\field{src_data}.\field{len}, and the digest result guest physical address
>> > -stored by \field{digest_result_addr} used to save the results of the MAC operations.
>> > +used to run the MAC operations. 
>> >  
>> > -The address and length can determine exclusive content in the guest memory.
>> > +The information includes the hash paramenters stored by \field{para}, output data and input data.
>> > +The output data here includs the source buffer and the input data includes the digest result buffer used to save the results of the MAC operations.
> 
> 
> includes
> 
>> > +\field{inhdr} store the executing status of MAC operations.
> stores
> 
> the executing status -> status of executing the MAC operations?
> 
>> >  
>> >  \begin{note}
>> > -The guest memory is always guaranteed to be allocated and physically-contiguous
>> > -pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input and
>> > -\field{hash_output}.\field{src_data_addr} in struct virtio_crypto_mac_output.
>> > +The request should by preference occupies one entry in the Vring Descriptor Table in the virtio crypto device's dataq, which improves
>> > +the throughput of data transmitted for the MAC service, so that the virtio crypto device can be better accelerated.
> Again, let's just drop.
> 
>> >  \end{note}
>> >  
>> >  \drivernormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
>> > @@ -698,8 +672,8 @@ pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input and
>> >  \devicenormative{\paragraph}{MAC Service Operation}{Device Types / Crypto Device / Device Operation / MAC Service Operation}
>> >  
>> >  \begin{itemize*}
>> > -\item The device MUST copy the results of MAC operations to the guest memory recorded by \field{digest_result_addr} field in struct virtio_crypto_mac_input.
>> > -\item The device MUST set \field{status} in strut virtio_crypto_mac_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
>> > +\item The device MUST copy the results of MAC operations to the guest memory recorded by digest result buffer if HASH operations success.
>> > +\item The device MUST set \field{status} in strut virtio_crypto_inhdr: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS: invalid session ID when the crypto operation is implemented.
> 
> same as above. I guess same issues repeat below, did not review.
> 

With this fixup patch commenting on the series becomes quite a hassle.
I would recommend you to fix the issues Michael has pointed out, maybe
do another consistency check regarding naming and regarding layout/format
notation across the whole specification, and re-spin.

An example for the lack chapter internal consistency is: "There is a unified
idata structure for all service, including CIPHER, HASH, MAC, and AEAD." since
this is the only occurrence of idata in the specification. Another one is 
struct virtio_crypto_hash_para {
/* length of source data */
le32 src_data_len;         <-- here you call it source data
/* hash result length */   <-- here you call it hash_result
le32 hash_result_len;
};
struct virtio_crypto_hash_data_req {
/* Device-readable part */
struct virtio_crypto_hash_para para;
/* Source buffer */         <-- here you call it buffer
/* Output data here */
/* Device-writable part */
/* Digest result buffer */  <-- here you call it digest result
/* Input data here */
struct virtio_crypto_inhdr inhdr;
};

For the cross specification consistency I would encourage you to
not use comments ambiguously for comments and for representing a
byte array variable size holding some kind of data (like src buf
dest/result buf, key buf).

The rest of the specification seems to use (variable length)
array of u8 notation for that when specifying the layout of
requests (or just describes stuff with words). For example:

struct virtio_blk_req {
le32 type;
le32 reserved;
le64 sector;
u8 data[][512];                    <-- here
u8 status;
};

or

struct virtio_scsi_req_cmd {
// Device-readable part
u8 lun[8];
le64 id;
u8 task_attr;
u8 prio;
u8 crn;
u8 cdb[cdb_size];
// The next two fields are only present if VIRTIO_SCSI_F_T10_PI
// is negotiated.
le32 pi_bytesout;
le32 pi_bytesin;
u8 pi_out[pi_bytesout];              <-- here
u8 dataout[];                        <-- here
// Device-writable part
le32 sense_len;
le32 residual;
le16 status_qualifier;
u8 status;
u8 response;
u8 sense[sense_size];
// The next two fields are only present if VIRTIO_SCSI_F_T10_PI
// is negotiated
u8 pi_in[pi_bytesin];                <-- here
u8 datain[];                         <-- here
};


Furthermore I would refrain from formulations like "guest
memory recorded by digest result buffer". Instead try to
use formulations consistent with the rest of the specification.

Finally I want to point out that the things got much easier
to understand with this last fixup (IMHO) despite of all
the typos, and that there are still more issues we did not
point out explicitly.

Regards
Halil

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

[Michael S. Tsirkin]

On Thu, Nov 10, 2016 at 05:47:54PM +0100, Halil Pasic wrote:
> the typos

That's something that's avoidable btw, please use
a spell and grammar checker. I hear http://www.languagetool.org/
is useful to some people.

-- 
MST

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

[Gonglei (Arei)]

> From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@lists.oasis-open.org]
> On Behalf Of Michael S. Tsirkin
> Sent: Thursday, November 10, 2016 9:03 PM
> brian.a.keating@intel.com; Claudio Fontana; mike.caraman@nxp.com; Wubin
> (H)
> Subject: Re: [virtio-dev] Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add
> virtio crypto device specification
> 
> On Thu, Nov 10, 2016 at 02:25:44AM +0000, Gonglei (Arei) wrote:
> >
> > >
> > > Subject: [virtio-dev] Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add
> virtio
> > > crypto device specification
> > >
> > > On Wed, Nov 09, 2016 at 01:11:20AM +0000, Gonglei (Arei) wrote:
> > > > Nope, Actually I kept those description here is because I wanted to
> represent
> > > each packet
> > > > Intuitionally, otherwise I don't know how to explain them only occupy one
> > > entry in desc table
> > > > by indirect table method.
> > >
> > > whether to use the indirect method should not be up to device.
> > > I don't see where does the spec require indirect method,
> > > ANY_LAYOUT is implied so spec should not dictate that IMO.
> > >
> > Yes, I reread the virtio spec. And
> >
> > 2.4.4 says:
> >  " The framing of messages with descriptors is independent of the contents
> of the buffers."
> >
> > 2.4.4.2 says:
> >  "The driver SHOULD NOT use an excessive number of descriptors to
> describe a buffer."
> >
> > For virtio-crypto, I think I can just add a NOTE to remind people using indirect
> method
> > if they want to get better performance in production environment, can I?
> >
> >
> > Thanks,
> > -Gonglei
> 
> I'm not sure why frankly. Is there a reason crypto is special here?
> 
I just think each virtio crypto device maybe use many entries for one request. 
But you are right, whether to use the indirect method should not be up to device.

The device shouldn't touch this. 

Thanks,
-Gonglei

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

[Gonglei (Arei)]

> From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@lists.oasis-open.org]
> On Behalf Of Michael S. Tsirkin
> Subject: [virtio-dev] Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio
> crypto device specification
> 
> On Thu, Nov 10, 2016 at 09:37:40AM +0000, Gonglei (Arei) wrote:
> > Hi,
> >
> > I attach a diff for next version in order to review more convenient, with
> >
> >  - Drop the all gap stuff;
> >  - Drop all structures undefined in virtio_crypto.h
> >  - re-describe per request for per crypto service avoid confusion
> >
> > Pls review, thanks!
> >
> >
> > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> > index 448296e..ab17e7b 100644
> > --- a/virtio-crypto.tex
> > +++ b/virtio-crypto.tex
> > @@ -69,13 +69,13 @@ The following services are defined:
> >
> >  \begin{lstlisting}
> >  /* CIPHER service */
> > -#define VIRTIO_CRYPTO_SERVICE_CIPHER (0)
> > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> >  /* HASH service */
> > -#define VIRTIO_CRYPTO_SERVICE_HASH   (1)
> > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> >  /* MAC (Message Authentication Codes) service */
> > -#define VIRTIO_CRYPTO_SERVICE_MAC    (2)
> > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> >  /* AEAD (Authenticated Encryption with Associated Data) service */
> > -#define VIRTIO_CRYPTO_SERVICE_AEAD   (3)
> > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> >  \end{lstlisting}
> >
> >  The last driver-read-only fields specify detailed algorithms masks
> > @@ -210,7 +210,7 @@ data that should be utilized in operations, and input
> data is equal to
> >  The general header for controlq is as follows:
> >
> >  \begin{lstlisting}
> > -#define VIRTIO_CRYPTO_OPCODE(service, op)   ((service << 8) | (op))
> > +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
> >
> >  struct virtio_crypto_ctrl_header {
> >  #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> > @@ -380,20 +380,18 @@ struct virtio_crypto_mac_session_para {
> >      le32 auth_key_len;
> >      le32 padding;
> >  };
> > -struct virtio_crypto_mac_session_output {
> > -    le64 auth_key_addr; /* guest key physical address */
> > -};
> >
> >  struct virtio_crypto_mac_create_session_req {
> >      /* Device-readable part */
> >      struct virtio_crypto_mac_session_para para;
> > -    struct virtio_crypto_mac_session_output out;
> > +    /* The authenticated key buffer */
> > +    /* output data here */
> > +
> >      /* Device-writable part */
> >      struct virtio_crypto_session_input input;
> >  };
> >  \end{lstlisting}
> >
> > -The output data are
> >  \subparagraph{Session operation: Symmetric algorithms
> session}\label{sec:Device Types / Crypto Device / Device
> >  Operation / Control Virtqueue / Session operation / Session operation:
> Symmetric algorithms session}
> >
> > @@ -413,13 +411,13 @@ struct virtio_crypto_cipher_session_para {
> >      le32 padding;
> >  };
> >
> > -struct virtio_crypto_cipher_session_output {
> > -    le64 key_addr; /* guest key physical address */
> > -};
> > -
> >  struct virtio_crypto_cipher_session_req {
> > +    /* Device-readable part */
> >      struct virtio_crypto_cipher_session_para para;
> > -    struct virtio_crypto_cipher_session_output out;
> > +    /* The cipher key buffer */
> > +    /* Output data here */
> > +
> > +    /* Device-writable part */
> >      struct virtio_crypto_session_input input;
> >  };
> >  \end{lstlisting}
> > @@ -448,18 +446,20 @@ struct virtio_crypto_alg_chain_session_para {
> >      le32 padding;
> >  };
> >
> > -struct virtio_crypto_alg_chain_session_output {
> > -    struct virtio_crypto_cipher_session_output cipher;
> > -    struct virtio_crypto_mac_session_output mac;
> > -};
> > -
> >  struct virtio_crypto_alg_chain_session_req {
> > +    /* Device-readable part */
> >      struct virtio_crypto_alg_chain_session_para para;
> > -    struct virtio_crypto_alg_chain_session_output out;
> > +    /* The cipher key buffer */
> > +    /* The authenticated key buffer */
> > +    /* Output data here */
> > +
> > +    /* Device-writable part */
> >      struct virtio_crypto_session_input input;
> >  };
> >  \end{lstlisting}
> >
> > +The output data includs both cipher key buffer and authenticated key buffer.
> 
> .. includes both the cipher key buffer and the uthenticated key buffer.
> 
OK.

> > +
> >  The packet for symmetric algorithm is as follows:
> >
> >  \begin{lstlisting}
> > @@ -503,13 +503,13 @@ struct virtio_crypto_aead_session_para {
> >      le32 padding;
> >  };
> >
> > -struct virtio_crypto_aead_session_output {
> > -    le64 key_addr; /* guest key physical address */
> > -};
> > -
> >  struct virtio_crypto_aead_create_session_req {
> > +    /* Device-readable part */
> >      struct virtio_crypto_aead_session_para para;
> > -    struct virtio_crypto_aead_session_output out;
> > +    /* The cipher key buffer */
> > +    /* Output data here */
> > +
> > +    /* Device-writeable part */
> >      struct virtio_crypto_session_input input;
> >  };
> >  \end{lstlisting}
> > @@ -568,19 +568,13 @@ struct virtio_crypto_op_data_req {
> >  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 idata structure for all symmetric algorithms, including
> CIPHER, HASH, MAC, and AEAD.
> > +There is a unified idata structure for all service, including CIPHER, HASH,
> MAC, and AEAD.
> 
> for all services
> 
Yes.

> >
> >  The structure is defined as follows:
> >
> >  \begin{lstlisting}
> > -struct virtio_crypto_sym_input {
> > -    /* Destination data guest address, it's useless for plain HASH and MAC
> */
> > -    le64 dst_data_addr;
> > -    /* Digest result guest address, it's useless for plain cipher algos */
> 
> guest address -> physical address?
> it's useless -> unused?
> 
Dropped.

> > -    le64 digest_result_addr;
> > -
> > -    le32 status;
> > -    le32 padding;
> > +struct virtio_crypto_inhdr {
> > +    u8 status;
> >  };
> >
> >  \end{lstlisting}
> > @@ -595,39 +589,29 @@ struct virtio_crypto_hash_para {
> >      le32 hash_result_len;
> >  };
> >
> > -struct virtio_crypto_hash_input {
> > -    struct virtio_crypto_sym_input input;
> > -};
> > -
> > -struct virtio_crypto_hash_output {
> > -    /* source data guest address */
> 
> guest -> physical?
> 
Dropped.

> > -    le64 src_data_addr;
> > -};
> > -
> >  struct virtio_crypto_hash_data_req {
> >      /* Device-readable part */
> >      struct virtio_crypto_hash_para para;
> > -    struct virtio_crypto_hash_output odata;
> > +    /* Source buffer */
> > +    /* Output data here */
> > +
> >      /* Device-writable part */
> > -    struct virtio_crypto_hash_input idata;
> > +    /* Digest result buffer */
> > +    /* Input data here */
> > +    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 request only occupies one entry
> > -in the Vring Descriptor Table in the virtio crypto device's dataq, which
> improves
> > -the throughput of data transmitted for the HASH service, so that the virtio
> crypto
> > -device can be better accelerated.
> > +used to run the HASH operations.
> >
> > -The information includes the source data guest physical address stored by
> \field{odata}.\field{src_data_addr},
> > -length of source data stored by \field{para}.\field{src_data_len}, and the
> digest result guest physical address
> > -stored by \field{digest_result_addr} used to save the results of the HASH
> operations.
> > -The address and length can determine exclusive content in the guest
> memory.
> > +The information includes the hash paramenters stored by \field{para},
> output data and input data.
> > +The output data here includs the source buffer and the input data includes
> the digest result buffer used to save the results of the HASH operations.
> 
> includs -> includes
> 
OK.

> > +\field{inhdr} store the executing status of HASH operations.
> >
> >  \begin{note}
> > -The guest memory is always guaranteed to be allocated and
> physically-contiguous
> > -pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input and
> > -\field{src_data_addr} in struct virtio_crypto_hash_output.
> > +The request should by preference occupies one entry in the Vring Descriptor
> Table in the virtio crypto device's dataq, which improves
> 
> Don't use should outside confirmance statements.
> 
> occupies -> occupy
> 
> > +the throughput of data transmitted for the HASH service, so that the virtio
> crypto device can be better accelerated.
> 
> I'd just drop this note - I don't see why is crypto special here.
> 
OK, will drop it.

> >  \end{note}
> >
> >  \drivernormative{\paragraph}{HASH Service Operation}{Device Types /
> Crypto Device / Device Operation / HASH Service Operation}
> > @@ -641,8 +625,8 @@ pointed by \field{digest_result_addr} in struct
> virtio_crypto_hash_input and
> >  \devicenormative{\paragraph}{HASH Service Operation}{Device Types /
> Crypto Device / Device Operation / HASH Service Operation}
> >
> >  \begin{itemize*}
> > -\item The device MUST copy the results of HASH operations to the guest
> memory recorded by \field{digest_result_addr} field in struct
> virtio_crypto_hash_input.
> > -\item The device MUST set \field{status} in strut virtio_crypto_hash_input:
> VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device
> error; VIRTIO_CRYPTO_NOTSUPP: not support.
> > +\item The device MUST copy the results of HASH operations to the guest
> memory recorded by digest result buffer if HASH operations success.
> > +\item The device MUST set \field{status} in strut virtio_crypto_inhdr:
> VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device
> error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS:
> invalid session ID when the crypto operation is implemented.
> 
> strut -> struct?
> 
Yes.

> add "to one of the values"? Also, just list the enum name here in case
> we add more values?
> 
OK, make sense.

> not support - not supported?
> 
OK.

> >  \end{itemize*}
> >
> >  \subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto
> Device / Device Operation / MAC Service Operation}
> > @@ -652,39 +636,29 @@ struct virtio_crypto_mac_para {
> >      struct virtio_crypto_hash_para hash;
> >  };
> >
> > -struct virtio_crypto_mac_input {
> > -    struct virtio_crypto_sym_input input;
> > -};
> > -
> > -struct virtio_crypto_mac_output {
> > -    struct virtio_crypto_hash_output hash_output;
> > -};
> > -
> >  struct virtio_crypto_mac_data_req {
> >      /* Device-readable part */
> >      struct virtio_crypto_mac_para para;
> > -    struct virtio_crypto_mac_output odata;
> > +    /* Source buffer */
> > +    /* Output data here */
> > +
> >      /* Device-writable part */
> > -    struct virtio_crypto_mac_input idata;
> > +    /* Digest result buffer */
> > +    /* Input data here */
> > +    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 request only occupies one entry
> > -in the Vring Descriptor Table in the virtio crypto device's dataq, which
> improves
> > -the throughput of data transmitted for the MAC service, so that the virtio
> crypto
> > -device can get the better result of acceleration.
> > -
> > -The information includes the source data guest physical address stored by
> \field{hash_output}.\field{src_data}.\field{addr},
> > -the length of source data stored by
> \field{hash_output}.\field{src_data}.\field{len}, and the digest result guest
> physical address
> > -stored by \field{digest_result_addr} used to save the results of the MAC
> operations.
> > +used to run the MAC operations.
> >
> > -The address and length can determine exclusive content in the guest
> memory.
> > +The information includes the hash paramenters stored by \field{para},
> output data and input data.
> > +The output data here includs the source buffer and the input data includes
> the digest result buffer used to save the results of the MAC operations.
> 
> 
> 
> includes
> 
> > +\field{inhdr} store the executing status of MAC operations.
> 
> stores
> 
OK.

> the executing status -> status of executing the MAC operations?
> 
> >
> >  \begin{note}
> > -The guest memory is always guaranteed to be allocated and
> physically-contiguous
> > -pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input and
> > -\field{hash_output}.\field{src_data_addr} in struct
> virtio_crypto_mac_output.
> > +The request should by preference occupies one entry in the Vring Descriptor
> Table in the virtio crypto device's dataq, which improves
> > +the throughput of data transmitted for the MAC service, so that the virtio
> crypto device can be better accelerated.
> 
> Again, let's just drop.
> 
OK.

> >  \end{note}
> >
> >  \drivernormative{\paragraph}{MAC Service Operation}{Device Types /
> Crypto Device / Device Operation / MAC Service Operation}
> > @@ -698,8 +672,8 @@ pointed by \field{digest_result_addr} in struct
> virtio_crypto_sym_input and
> >  \devicenormative{\paragraph}{MAC Service Operation}{Device Types /
> Crypto Device / Device Operation / MAC Service Operation}
> >
> >  \begin{itemize*}
> > -\item The device MUST copy the results of MAC operations to the guest
> memory recorded by \field{digest_result_addr} field in struct
> virtio_crypto_mac_input.
> > -\item The device MUST set \field{status} in strut virtio_crypto_mac_input:
> VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device
> error; VIRTIO_CRYPTO_NOTSUPP: not support.
> > +\item The device MUST copy the results of MAC operations to the guest
> memory recorded by digest result buffer if HASH operations success.
> > +\item The device MUST set \field{status} in strut virtio_crypto_inhdr:
> VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device
> error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS:
> invalid session ID when the crypto operation is implemented.
> 
> 
> same as above. I guess same issues repeat below, did not review.
> 
Yes, I'll check all of them, thanks!

Regards,
-Gonglei

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

[Gonglei (Arei)]

Hi guys,

> From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@lists.oasis-open.org]
> On Behalf Of Michael S. Tsirkin
> Sent: Friday, November 11, 2016 1:04 AM
> Subject: [virtio-dev] Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio
> crypto device specification
> 
> On Thu, Nov 10, 2016 at 05:47:54PM +0100, Halil Pasic wrote:
> > the typos
> 
> That's something that's avoidable btw, please use
> a spell and grammar checker. I hear http://www.languagetool.org/
> is useful to some people.
> 
Thanks for your suggestion :)

Regards,
-Gonglei

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

[Gonglei (Arei)]

Hi Halil,

> 
> > On Thu, Nov 10, 2016 at 09:37:40AM +0000, Gonglei (Arei) wrote:
> >> > Hi,
> >> >
> >> > I attach a diff for next version in order to review more convenient, with
> >> >
> >> >  - Drop the all gap stuff;
> >> >  - Drop all structures undefined in virtio_crypto.h
> >> >  - re-describe per request for per crypto service avoid confusion
> >> >
> >> > Pls review, thanks!
> >> >
> >> >
> >> > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> >> > index 448296e..ab17e7b 100644
> >> > --- a/virtio-crypto.tex
> >> > +++ b/virtio-crypto.tex
> >> > @@ -69,13 +69,13 @@ The following services are defined:
> >> >
> >> >  \begin{lstlisting}
> >> >  /* CIPHER service */
> >> > -#define VIRTIO_CRYPTO_SERVICE_CIPHER (0)
> >> > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> >> >  /* HASH service */
> >> > -#define VIRTIO_CRYPTO_SERVICE_HASH   (1)
> >> > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
> >> >  /* MAC (Message Authentication Codes) service */
> >> > -#define VIRTIO_CRYPTO_SERVICE_MAC    (2)
> >> > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
> >> >  /* AEAD (Authenticated Encryption with Associated Data) service */
> >> > -#define VIRTIO_CRYPTO_SERVICE_AEAD   (3)
> >> > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
> >> >  \end{lstlisting}
> >> >
> >> >  The last driver-read-only fields specify detailed algorithms masks
> >> > @@ -210,7 +210,7 @@ data that should be utilized in operations, and
> input data is equal to
> >> >  The general header for controlq is as follows:
> >> >
> >> >  \begin{lstlisting}
> >> > -#define VIRTIO_CRYPTO_OPCODE(service, op)   ((service << 8) | (op))
> >> > +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
> >> >
> >> >  struct virtio_crypto_ctrl_header {
> >> >  #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> >> > @@ -380,20 +380,18 @@ struct virtio_crypto_mac_session_para {
> >> >      le32 auth_key_len;
> >> >      le32 padding;
> >> >  };
> >> > -struct virtio_crypto_mac_session_output {
> >> > -    le64 auth_key_addr; /* guest key physical address */
> >> > -};
> >> >
> >> >  struct virtio_crypto_mac_create_session_req {
> >> >      /* Device-readable part */
> >> >      struct virtio_crypto_mac_session_para para;
> >> > -    struct virtio_crypto_mac_session_output out;
> >> > +    /* The authenticated key buffer */
> >> > +    /* output data here */
> >> > +
> >> >      /* Device-writable part */
> >> >      struct virtio_crypto_session_input input;
> >> >  };
> >> >  \end{lstlisting}
> >> >
> >> > -The output data are
> >> >  \subparagraph{Session operation: Symmetric algorithms
> session}\label{sec:Device Types / Crypto Device / Device
> >> >  Operation / Control Virtqueue / Session operation / Session operation:
> Symmetric algorithms session}
> >> >
> >> > @@ -413,13 +411,13 @@ struct virtio_crypto_cipher_session_para {
> >> >      le32 padding;
> >> >  };
> >> >
> >> > -struct virtio_crypto_cipher_session_output {
> >> > -    le64 key_addr; /* guest key physical address */
> >> > -};
> >> > -
> >> >  struct virtio_crypto_cipher_session_req {
> >> > +    /* Device-readable part */
> >> >      struct virtio_crypto_cipher_session_para para;
> >> > -    struct virtio_crypto_cipher_session_output out;
> >> > +    /* The cipher key buffer */
> >> > +    /* Output data here */
> >> > +
> >> > +    /* Device-writable part */
> >> >      struct virtio_crypto_session_input input;
> >> >  };
> >> >  \end{lstlisting}
> >> > @@ -448,18 +446,20 @@ struct virtio_crypto_alg_chain_session_para {
> >> >      le32 padding;
> >> >  };
> >> >
> >> > -struct virtio_crypto_alg_chain_session_output {
> >> > -    struct virtio_crypto_cipher_session_output cipher;
> >> > -    struct virtio_crypto_mac_session_output mac;
> >> > -};
> >> > -
> >> >  struct virtio_crypto_alg_chain_session_req {
> >> > +    /* Device-readable part */
> >> >      struct virtio_crypto_alg_chain_session_para para;
> >> > -    struct virtio_crypto_alg_chain_session_output out;
> >> > +    /* The cipher key buffer */
> >> > +    /* The authenticated key buffer */
> >> > +    /* Output data here */
> >> > +
> >> > +    /* Device-writable part */
> >> >      struct virtio_crypto_session_input input;
> >> >  };
> >> >  \end{lstlisting}
> >> >
> >> > +The output data includs both cipher key buffer and authenticated key
> buffer.
> > .. includes both the cipher key buffer and the uthenticated key buffer.
> >
> >> > +
> >> >  The packet for symmetric algorithm is as follows:
> >> >
> >> >  \begin{lstlisting}
> >> > @@ -503,13 +503,13 @@ struct virtio_crypto_aead_session_para {
> >> >      le32 padding;
> >> >  };
> >> >
> >> > -struct virtio_crypto_aead_session_output {
> >> > -    le64 key_addr; /* guest key physical address */
> >> > -};
> >> > -
> >> >  struct virtio_crypto_aead_create_session_req {
> >> > +    /* Device-readable part */
> >> >      struct virtio_crypto_aead_session_para para;
> >> > -    struct virtio_crypto_aead_session_output out;
> >> > +    /* The cipher key buffer */
> >> > +    /* Output data here */
> >> > +
> >> > +    /* Device-writeable part */
> >> >      struct virtio_crypto_session_input input;
> >> >  };
> >> >  \end{lstlisting}
> >> > @@ -568,19 +568,13 @@ struct virtio_crypto_op_data_req {
> >> >  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 idata structure for all symmetric algorithms, including
> CIPHER, HASH, MAC, and AEAD.
> >> > +There is a unified idata structure for all service, including CIPHER, HASH,
> MAC, and AEAD.
> > for all services
> >
> >> >
> >> >  The structure is defined as follows:
> >> >
> >> >  \begin{lstlisting}
> >> > -struct virtio_crypto_sym_input {
> >> > -    /* Destination data guest address, it's useless for plain HASH and
> MAC */
> >> > -    le64 dst_data_addr;
> >> > -    /* Digest result guest address, it's useless for plain cipher algos */
> > guest address -> physical address?
> > it's useless -> unused?
> >
> >> > -    le64 digest_result_addr;
> >> > -
> >> > -    le32 status;
> >> > -    le32 padding;
> >> > +struct virtio_crypto_inhdr {
> >> > +    u8 status;
> >> >  };
> >> >
> >> >  \end{lstlisting}
> >> > @@ -595,39 +589,29 @@ struct virtio_crypto_hash_para {
> >> >      le32 hash_result_len;
> >> >  };
> >> >
> >> > -struct virtio_crypto_hash_input {
> >> > -    struct virtio_crypto_sym_input input;
> >> > -};
> >> > -
> >> > -struct virtio_crypto_hash_output {
> >> > -    /* source data guest address */
> > guest -> physical?
> >
> >> > -    le64 src_data_addr;
> >> > -};
> >> > -
> >> >  struct virtio_crypto_hash_data_req {
> >> >      /* Device-readable part */
> >> >      struct virtio_crypto_hash_para para;
> >> > -    struct virtio_crypto_hash_output odata;
> >> > +    /* Source buffer */
> >> > +    /* Output data here */
> >> > +
> >> >      /* Device-writable part */
> >> > -    struct virtio_crypto_hash_input idata;
> >> > +    /* Digest result buffer */
> >> > +    /* Input data here */
> >> > +    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 request only occupies one entry
> >> > -in the Vring Descriptor Table in the virtio crypto device's dataq, which
> improves
> >> > -the throughput of data transmitted for the HASH service, so that the
> virtio crypto
> >> > -device can be better accelerated.
> >> > +used to run the HASH operations.
> >> >
> >> > -The information includes the source data guest physical address stored by
> \field{odata}.\field{src_data_addr},
> >> > -length of source data stored by \field{para}.\field{src_data_len}, and the
> digest result guest physical address
> >> > -stored by \field{digest_result_addr} used to save the results of the HASH
> operations.
> >> > -The address and length can determine exclusive content in the guest
> memory.
> >> > +The information includes the hash paramenters stored by \field{para},
> output data and input data.
> >> > +The output data here includs the source buffer and the input data
> includes the digest result buffer used to save the results of the HASH
> operations.
> > includs -> includes
> >
> >> > +\field{inhdr} store the executing status of HASH operations.
> >> >
> >> >  \begin{note}
> >> > -The guest memory is always guaranteed to be allocated and
> physically-contiguous
> >> > -pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input
> and
> >> > -\field{src_data_addr} in struct virtio_crypto_hash_output.
> >> > +The request should by preference occupies one entry in the Vring
> Descriptor Table in the virtio crypto device's dataq, which improves
> > Don't use should outside confirmance statements.
> >
> > occupies -> occupy
> >
> >> > +the throughput of data transmitted for the HASH service, so that the
> virtio crypto device can be better accelerated.
> > I'd just drop this note - I don't see why is crypto special here.
> >
> >> >  \end{note}
> >> >
> >> >  \drivernormative{\paragraph}{HASH Service Operation}{Device Types /
> Crypto Device / Device Operation / HASH Service Operation}
> >> > @@ -641,8 +625,8 @@ pointed by \field{digest_result_addr} in struct
> virtio_crypto_hash_input and
> >> >  \devicenormative{\paragraph}{HASH Service Operation}{Device Types /
> Crypto Device / Device Operation / HASH Service Operation}
> >> >
> >> >  \begin{itemize*}
> >> > -\item The device MUST copy the results of HASH operations to the guest
> memory recorded by \field{digest_result_addr} field in struct
> virtio_crypto_hash_input.
> >> > -\item The device MUST set \field{status} in strut virtio_crypto_hash_input:
> VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device
> error; VIRTIO_CRYPTO_NOTSUPP: not support.
> >> > +\item The device MUST copy the results of HASH operations to the guest
> memory recorded by digest result buffer if HASH operations success.
> >> > +\item The device MUST set \field{status} in strut virtio_crypto_inhdr:
> VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device
> error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS:
> invalid session ID when the crypto operation is implemented.
> > strut -> struct?
> >
> > add "to one of the values"? Also, just list the enum name here in case
> > we add more values?
> >
> > not support - not supported?
> >
> >> >  \end{itemize*}
> >> >
> >> >  \subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto
> Device / Device Operation / MAC Service Operation}
> >> > @@ -652,39 +636,29 @@ struct virtio_crypto_mac_para {
> >> >      struct virtio_crypto_hash_para hash;
> >> >  };
> >> >
> >> > -struct virtio_crypto_mac_input {
> >> > -    struct virtio_crypto_sym_input input;
> >> > -};
> >> > -
> >> > -struct virtio_crypto_mac_output {
> >> > -    struct virtio_crypto_hash_output hash_output;
> >> > -};
> >> > -
> >> >  struct virtio_crypto_mac_data_req {
> >> >      /* Device-readable part */
> >> >      struct virtio_crypto_mac_para para;
> >> > -    struct virtio_crypto_mac_output odata;
> >> > +    /* Source buffer */
> >> > +    /* Output data here */
> >> > +
> >> >      /* Device-writable part */
> >> > -    struct virtio_crypto_mac_input idata;
> >> > +    /* Digest result buffer */
> >> > +    /* Input data here */
> >> > +    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 request only occupies one entry
> >> > -in the Vring Descriptor Table in the virtio crypto device's dataq, which
> improves
> >> > -the throughput of data transmitted for the MAC service, so that the virtio
> crypto
> >> > -device can get the better result of acceleration.
> >> > -
> >> > -The information includes the source data guest physical address stored by
> \field{hash_output}.\field{src_data}.\field{addr},
> >> > -the length of source data stored by
> \field{hash_output}.\field{src_data}.\field{len}, and the digest result guest
> physical address
> >> > -stored by \field{digest_result_addr} used to save the results of the MAC
> operations.
> >> > +used to run the MAC operations.
> >> >
> >> > -The address and length can determine exclusive content in the guest
> memory.
> >> > +The information includes the hash paramenters stored by \field{para},
> output data and input data.
> >> > +The output data here includs the source buffer and the input data
> includes the digest result buffer used to save the results of the MAC
> operations.
> >
> >
> > includes
> >
> >> > +\field{inhdr} store the executing status of MAC operations.
> > stores
> >
> > the executing status -> status of executing the MAC operations?
> >
> >> >
> >> >  \begin{note}
> >> > -The guest memory is always guaranteed to be allocated and
> physically-contiguous
> >> > -pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input
> and
> >> > -\field{hash_output}.\field{src_data_addr} in struct
> virtio_crypto_mac_output.
> >> > +The request should by preference occupies one entry in the Vring
> Descriptor Table in the virtio crypto device's dataq, which improves
> >> > +the throughput of data transmitted for the MAC service, so that the
> virtio crypto device can be better accelerated.
> > Again, let's just drop.
> >
> >> >  \end{note}
> >> >
> >> >  \drivernormative{\paragraph}{MAC Service Operation}{Device Types /
> Crypto Device / Device Operation / MAC Service Operation}
> >> > @@ -698,8 +672,8 @@ pointed by \field{digest_result_addr} in struct
> virtio_crypto_sym_input and
> >> >  \devicenormative{\paragraph}{MAC Service Operation}{Device Types /
> Crypto Device / Device Operation / MAC Service Operation}
> >> >
> >> >  \begin{itemize*}
> >> > -\item The device MUST copy the results of MAC operations to the guest
> memory recorded by \field{digest_result_addr} field in struct
> virtio_crypto_mac_input.
> >> > -\item The device MUST set \field{status} in strut virtio_crypto_mac_input:
> VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device
> error; VIRTIO_CRYPTO_NOTSUPP: not support.
> >> > +\item The device MUST copy the results of MAC operations to the guest
> memory recorded by digest result buffer if HASH operations success.
> >> > +\item The device MUST set \field{status} in strut virtio_crypto_inhdr:
> VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device
> error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS:
> invalid session ID when the crypto operation is implemented.
> >
> > same as above. I guess same issues repeat below, did not review.
> >
> 
> With this fixup patch commenting on the series becomes quite a hassle.
> I would recommend you to fix the issues Michael has pointed out, maybe
> do another consistency check regarding naming and regarding layout/format
> notation across the whole specification, and re-spin.
> 
OK, will do.

> An example for the lack chapter internal consistency is: "There is a unified
> idata structure for all service, including CIPHER, HASH, MAC, and AEAD." since
> this is the only occurrence of idata in the specification. Another one is
> struct virtio_crypto_hash_para {
> /* length of source data */
> le32 src_data_len;         <-- here you call it source data
> /* hash result length */   <-- here you call it hash_result
> le32 hash_result_len;
> };
> struct virtio_crypto_hash_data_req {
> /* Device-readable part */
> struct virtio_crypto_hash_para para;
> /* Source buffer */         <-- here you call it buffer
> /* Output data here */
> /* Device-writable part */
> /* Digest result buffer */  <-- here you call it digest result
> /* Input data here */
> struct virtio_crypto_inhdr inhdr;
> };
> 
> For the cross specification consistency I would encourage you to
> not use comments ambiguously for comments and for representing a
> byte array variable size holding some kind of data (like src buf
> dest/result buf, key buf).
> 
> The rest of the specification seems to use (variable length)
> array of u8 notation for that when specifying the layout of
> requests (or just describes stuff with words). For example:
> 
Make pretty sense! I tried to find a good way to express those kind of data,
but I didn't notice it. Thank you so much.

> struct virtio_blk_req {
> le32 type;
> le32 reserved;
> le64 sector;
> u8 data[][512];                    <-- here
> u8 status;
> };
> 
> or
> 
> struct virtio_scsi_req_cmd {
> // Device-readable part
> u8 lun[8];
> le64 id;
> u8 task_attr;
> u8 prio;
> u8 crn;
> u8 cdb[cdb_size];
> // The next two fields are only present if VIRTIO_SCSI_F_T10_PI
> // is negotiated.
> le32 pi_bytesout;
> le32 pi_bytesin;
> u8 pi_out[pi_bytesout];              <-- here
> u8 dataout[];                        <-- here
> // Device-writable part
> le32 sense_len;
> le32 residual;
> le16 status_qualifier;
> u8 status;
> u8 response;
> u8 sense[sense_size];
> // The next two fields are only present if VIRTIO_SCSI_F_T10_PI
> // is negotiated
> u8 pi_in[pi_bytesin];                <-- here
> u8 datain[];                         <-- here
> };
> 
> 
> Furthermore I would refrain from formulations like "guest
> memory recorded by digest result buffer". Instead try to
> use formulations consistent with the rest of the specification.
> 
OK, will recheck. But I'm not a native English speaker, so if you
give me some specific comments about formulations
will be appreciated ;)

> Finally I want to point out that the things got much easier
> to understand with this last fixup (IMHO) despite of all
> the typos, and that there are still more issues we did not
> point out explicitly.
> 
Thanks, Let's make it better and better.

Regards,
-Gonglei