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; address@hidden; address@hidden;
> > Huangweidong (C); address@hidden; address@hidden; Shiqing Fan;
> > Zhoujian (jay, Euler); address@hidden; address@hidden;
> > address@hidden; Hanweidong (Randy); address@hidden;
> > address@hidden; address@hidden; address@hidden;
> > Luonengjun; address@hidden; Huangpeng (Peter); address@hidden;
> > address@hidden; Jani Kokkonen; address@hidden; Claudio
> > Fontana; address@hidden; Wubin (H)
> > Subject: RE: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add virtio crypto
> > device specification
> > 
> > > From: Cornelia Huck [mailto:address@hidden
> > > 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)" <address@hidden> 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