[v13,1/2] virtio-crypto: Add virtio crypto device specification

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!




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

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

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

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.

> 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

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

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