[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [virtio-dev] [PATCH v17 1/2] virtio-crypto: Add virtio
From: |
Gonglei (Arei) |
Subject: |
Re: [Qemu-devel] [virtio-dev] [PATCH v17 1/2] virtio-crypto: Add virtio crypto device specification |
Date: |
Fri, 21 Apr 2017 13:39:32 +0000 |
> -----Original Message-----
> From: Stefan Hajnoczi [mailto:address@hidden
> Sent: Friday, April 21, 2017 9:07 PM
>
> Subject: Re: [virtio-dev] [PATCH v17 1/2] virtio-crypto: Add virtio crypto
> device
> specification
>
> 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:"
>
OK.
> > +
> > +\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:"
>
Fixed.
> > +
> > +\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}".
>
Great thanks :)
All fixed.
> > +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"
>
All fixed.
> > +
> > +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.)
>
All fixed.
> > +
> > +\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".
>
Sure, all fixed.
> > +\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"
>
OK, all fixed.
> > +\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".
>
Yes, all fixed.
> "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/
>
OK, I get it. All fixed.
> > + 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/
>
OK. All fixed.
> 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.
Thanks for your patience Stefan. I have finished the fixes as your suggestions
Throughout the document.
Thanks!
-Gonglei