Re: [virtio-dev] [PATCH v17 1/2] virtio-crypto: Add virtio crypto device specification

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Apr 13, 2017 at 05:11:13PM +0800, Gonglei wrote:
> +The request of dataq, mixing both session and stateless mode is as follows:

It sounds more natural when "request" is plural:

"Dataq requests for both session and stateless modes are as follows:"

> +
> +\begin{lstlisting}
> +struct virtio_crypto_op_data_req_mux {
> +    struct virtio_crypto_op_header header;
> +
> +    union {
> +        struct virtio_crypto_sym_data_req   sym_req;
> +        struct virtio_crypto_hash_data_req  hash_req;
> +        struct virtio_crypto_mac_data_req   mac_req;
> +        struct virtio_crypto_aead_data_req  aead_req;
> +        struct virtio_crypto_sym_data_req_stateless   sym_stateless_req;
> +        struct virtio_crypto_hash_data_req_stateless  hash_stateless_req;
> +        struct virtio_crypto_mac_data_req_stateless   mac_stateless_req;
> +        struct virtio_crypto_aead_data_req_stateless  aead_stateless_req;
> +    } u;
> +};
> +\end{lstlisting}
> +
> +The header is the general header and the union is of the algorithm-specific type,
> +which is set by the driver. All properties in the union are shown as follows.
> +
> +There is a unified input header structure for all crypto services.
> +
> +The structure is defined as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_inhdr {
> +    u8 status;
> +};
> +\end{lstlisting}
> +
> +\subsubsection{HASH Service Operation}\label{sec:Device Types / Crypto Device / Device Operation / HASH Service Operation}
> +
> +The session mode request of HASH service:

Plural "request":

"Session mode HASH service requests are defined as follows:"

> +
> +\begin{lstlisting}
> +struct virtio_crypto_hash_para {
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* hash result length */
> +    le32 hash_result_len;
> +};
> +
> +struct virtio_crypto_hash_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_hash_para para;
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +
> +    /* Device-writable part */
> +    /* Hash result data */
> +    u8 hash_result[hash_result_len];
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> +Each data request uses virtio_crypto_hash_data_req structure to store information
> +used to run the HASH operations. 
> +
> +The information includes the hash parameters stored by \field{para}, output data and input data.

"stored in $container" vs "stored by $actor_or_service_or_action".  For
example:

"stored in register EAX" <-- container
"stored in /etc/passwd" <-- container
"stored by the worker thread" <-- actor
"stored by calling api_write()" <-- action
"stored by ext4" <-- service

Since \field{para} is a container it needs to be "stored in
\field{para}".

> +The output data here includes the source data and the input data includes the hash result data used to save the results of the HASH operations.
> +\field{inhdr} stores status of executing the HASH operations.

Missing article: "stores the status"

> +
> +The stateless mode request of HASH service is as follows:

Plural "requests":

"Stateless mode HASH service requests are as follows:"

(The same applies to more instances below.)

> +
> +\begin{lstlisting}
> +struct virtio_crypto_hash_para_statelesss {
> +    struct {
> +        /* See VIRTIO_CRYPTO_HASH_* above */
> +        le32 algo;
> +    } sess_para;
> +
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* hash result length */
> +    le32 hash_result_len;
> +    le32 reserved;
> +};
> +struct virtio_crypto_hash_data_req_stateless {
> +    /* Device-readable part */
> +    struct virtio_crypto_hash_para_stateless para;
> +    /* Source data */
> +    u8 src_data[src_data_len];
> +
> +    /* Device-writable part */
> +    /* Hash result data */
> +    u8 hash_result[hash_result_len];
> +    struct virtio_crypto_inhdr inhdr;
> +};
> +\end{lstlisting}
> +
> +\drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
> +
> +\begin{itemize*}
> +\item If the driver uses the session mode, then the driver MUST set the \field{session_id} in struct virtio_crypto_op_header
> +      to a valid value which assigned by the device when a session is created.

Please see my previous email for a change to "a valid value which
assigned by the device when a session is created".

> +\item If the VIRTIO_CRYPTO_F_STATELESS_MODE feature bit is negotiated, the driver MUST use the struct virtio_crypto_op_data_req_mux to wrap crypto requests. Otherwise, the driver MUST use the struct virtio_crypto_op_data_req.

The article is omitted when referring to a specific named object:
"use struct virtio_crypto_op_data_req_mux" and "use struct virtio_crypto_op_data_req"

> +\item If the VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is negotiated, 1) if the driver use the stateless mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header

"he/she/it uses" so it should be "if the driver uses the stateless
mode".

"set X field" is missing a definite article.  Either add "the" ("set the
X field") or drop "field" so that X isn't a member of a group ("set X"):
"set \field{flag} in struct virtio_crypto_op_header"

https://owl.english.purdue.edu/owl/resource/540/01/

> +      to VIRTIO_CRYPTO_FLAG_STATELESS_MODE and MUST set fields in struct virtio_crypto_hash_para_statelession.sess_para, 2) if the driver still uses the session mode, then the driver MUST set \field{flag} field in struct virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_STATE_MODE.

I don't understand the meaning of "still" here.  Maybe it was meant to
be "instead"?  It's simplest to drop it:
s/driver still uses/driver uses/

s/MUST set \field{flag} field/MUST set \field{flag}/

> +\item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to VIRTIO_CRYPTO_HASH.
> +\end{itemize*}
> +
> +\devicenormative{\paragraph}{HASH Service Operation}{Device Types / Crypto Device / Device Operation / HASH Service Operation}
> +
> +\begin{itemize*}
> +\item If the VIRTIO_CRYPTO_F_STATELESS_MODE feature bit is negotiated, the device MUST parse the struct virtio_crypto_op_data_req_mux for crypto requests. Otherwise, the device MUST parse the struct virtio_crypto_op_data_req.
> +\item If the VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is negotiated, the device MUST parse \field{flag} field in struct virtio_crypto_op_header in order to decide which mode the driver uses.
> +\item The device MUST copy the results of HASH operations to the hash_result[] if HASH operations success.
> +\item The device MUST set \field{status} in struct virtio_crypto_inhdr to one of the values of enum VIRITO_CRYPTO_STATUS.
> +\end{itemize*}

More instances of the same grammar issues mentioned above.  This is a
good point to stop review for now and wait for the next revision that
fixes them throughout the document.

Attachment: signature.asc
Description: PGP signature