[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [virtio-dev] [PATCH v17 1/2] virtio-crypto: Add virtio crypto device specification
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@redhat.com] > 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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]