Hi Halil, Sorry for delay because I'm busy on inner production project recently.
> > > START HERE > > > +The device can set the operation status as follows: VIRTIO_CRYPTO_OK: > success; > > +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP: > not supported; > > +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto > operations. > > You describe everything but BADMSG. Could you be a bit more specific > about the differences between _ERR _BADMSG and _NOTSUPP? Is for instance > trying > to do something with a not-advertised service or algo a _NOTSUPP or a > _BADMSG or just a generic _ERR? Same qestion goes for different sorts of > out of resources. > Sure. Will do. > > + > > +\begin{lstlisting} > > +enum VIRTIO_CRYPTO_STATUS { > > + VIRTIO_CRYPTO_OK = 0, > > + VIRTIO_CRYPTO_ERR = 1, > > + VIRTIO_CRYPTO_BADMSG = 2, > > + VIRTIO_CRYPTO_NOTSUPP = 3, > > + VIRTIO_CRYPTO_INVSESS = 4, > > + VIRTIO_CRYPTO_MAX > > +}; > > +\end{lstlisting} > > + > > There are no more mentions of the values in the spec, so the description > above should be real good. > Agree. > > +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device / > Device Operation / Control Virtqueue} > > + > > +The driver uses the control virtqueue to send control commands to the > > +device, such as session operations (See \ref{sec:Device Types / Crypto > Device / Device Operation / Control Virtqueue / Session operation}). > > + > > +Controlq requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_op_ctrl_req { > > + struct virtio_crypto_ctrl_header header; > > + > > + union { > > + struct virtio_crypto_sym_create_session_req > sym_create_session; > > + struct virtio_crypto_hash_create_session_req > hash_create_session; > > + struct virtio_crypto_mac_create_session_req > mac_create_session; > > + struct virtio_crypto_aead_create_session_req > aead_create_session; > > + struct virtio_crypto_destroy_session_req destroy_session; > > + } u; > > +}; > > +\end{lstlisting} > > + > > +struct virtio_crypto_op_ctrl_req is the only allowed control request. > > +The header is the general header, and the union is of the > > algorithm-specific > type, > > +which is set by the driver. All the properties in the union are shown as > follows. > > + > > +\paragraph{Session operation}\label{sec:Device Types / Crypto Device / > Device Operation / Control Virtqueue / Session operation} > > + > > +The symmetric algorithms involve the concept of sessions. A session is a > > +handle which describes the cryptographic parameters to be applied to > > +a number of buffers. > > 8< > > The data within a session handle includes: > > + > > +\begin{enumerate} > > +\item The operation (CIPHER, HASH/MAC or both, and if both, the order in > > + which the algorithms should be applied). > > +\item The CIPHER set data, including the CIPHER algorithm and mode, > > + the key and its length, and the direction (encryption or decryption). > > +\item The HASH/MAC set data, including the HASH algorithm or MAC > algorithm, > > + and hash result length (to allow for truncation). > > +\begin{itemize*} > > +\item Authenticated mode can refer to MAC, which requires that the key > and > > + its length are also specified. > > +\item For nested mode, the inner and outer prefix data and length are > specified, > > + as well as the outer HASH algorithm. > > +\end{itemize*} > > +\end{enumerate} > > + > >8 > > This part is slightly confusing for me. I guess you are trying to describe > what data can live in the session context (considering all the different > applications). I think we do not have to describe that here, because we have > to describe the individual session operations, and there we must describe > the precise impact of these operations (and their parameters). > Make sense to me. > > +The following structure stores the result of session creation set by the > device: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_session_input { > > + /* Device-writable part */ > > + le64 session_id; > > + le32 status; > > + le32 padding; > > +}; > > +\end{lstlisting} > > + > > +A request to destroy a session includes the following information: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_destroy_session_req { > > + /* Device-readable part */ > > + le64 session_id; > > + /* Device-writable part */ > > + le32 status; > > + le32 padding; > > +}; > > +\end{lstlisting} > > + > > +\subparagraph{Session operation: HASH session}\label{sec:Device Types / > Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: HASH > session} > > + > > +HASH session requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_hash_session_para { > > + /* See VIRTIO_CRYPTO_HASH_* above */ > > + le32 algo; > > + /* hash result length */ > > + le32 hash_result_len; > > +}; > > +struct virtio_crypto_hash_create_session_req { > > + /* Device-readable part */ > > + struct virtio_crypto_hash_session_para para; > > + /* Device-writable part */ > > + struct virtio_crypto_session_input input; > > +}; > > +\end{lstlisting} > > + > > +\subparagraph{Session operation: MAC session}\label{sec:Device Types / > Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: MAC > session} > > + > > +MAC session requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_mac_session_para { > > + /* See VIRTIO_CRYPTO_MAC_* above */ > > + le32 algo; > > + /* hash result length */ > > + le32 hash_result_len; > > + /* length of authenticated key */ > > + le32 auth_key_len; > > + le32 padding; > > +}; > > + > > +struct virtio_crypto_mac_create_session_req { > > + /* Device-readable part */ > > + struct virtio_crypto_mac_session_para para; > > + /* The authenticated key */ > > + u8 auth_key[auth_key_len]; > > + > > + /* Device-writable part */ > > + struct virtio_crypto_session_input input; > > +}; > > +\end{lstlisting} > > + > > +\subparagraph{Session operation: Symmetric algorithms > session}\label{sec:Device Types / Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: > Symmetric algorithms session} > > + > > +The request of symmetric session includes two parts, CIPHER algorithms > and chain > > +algorithms (chaining CIPHER and HASH/MAC). > > + > > +CIPHER session requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_cipher_session_para { > > + /* See VIRTIO_CRYPTO_CIPHER* above */ > > + le32 algo; > > + /* length of key */ > > + le32 keylen; > > +#define VIRTIO_CRYPTO_OP_ENCRYPT 1 > > +#define VIRTIO_CRYPTO_OP_DECRYPT 2 > > + /* encryption or decryption */ > > + le32 op; > > + le32 padding; > > +}; > > + > > +struct virtio_crypto_cipher_session_req { > > + /* Device-readable part */ > > + struct virtio_crypto_cipher_session_para para; > > + /* The cipher key */ > > + u8 cipher_key[keylen]; > > + > > + /* Device-writable part */ > > + struct virtio_crypto_session_input input; > > +}; > > +\end{lstlisting} > > + > > +Algorithm chaining requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_alg_chain_session_para { > > +#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER > 1 > > +#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH > 2 > > + le32 alg_chain_order; > > +/* Plain hash */ > > +#define VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN 1 > > +/* Authenticated hash (mac) */ > > +#define VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH 2 > > +/* Nested hash */ > > +#define VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED 3 > > + le32 hash_mode; > > + struct virtio_crypto_cipher_session_para cipher_param; > > + union { > > + struct virtio_crypto_hash_session_para hash_param; > > + struct virtio_crypto_mac_session_para mac_param; > > + } u; > > + /* length of the additional authenticated data (AAD) in bytes */ > > + le32 aad_len; > > + le32 padding; > > +}; > > + > > +struct virtio_crypto_alg_chain_session_req { > > + /* Device-readable part */ > > + struct virtio_crypto_alg_chain_session_para para; > > + /* The cipher key */ > > + u8 cipher_key[keylen]; > > + /* The authenticated key */ > > + u8 auth_key[auth_key_len]; > > + > > + /* Device-writable part */ > > + struct virtio_crypto_session_input input; > > +}; > > +\end{lstlisting} > > + > > +Symmetric algorithm requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_sym_create_session_req { > > + union { > > + struct virtio_crypto_cipher_session_req cipher; > > + struct virtio_crypto_alg_chain_session_req chain; > > + } u; > > + > > + /* Device-readable part */ > > + > > +/* No operation */ > > +#define VIRTIO_CRYPTO_SYM_OP_NONE 0 > > +/* Cipher only operation on the data */ > > +#define VIRTIO_CRYPTO_SYM_OP_CIPHER 1 > > +/* Chain any cipher with any hash or mac operation. The order > > + depends on the value of alg_chain_order param */ > > +#define VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING 2 > > + le32 op_type; > > + le32 padding; > > +}; > > +\end{lstlisting} > > + > > +The driver can set the \field{op_type} field in struct > virtio_crypto_sym_create_session_req as follows: > VIRTIO_CRYPTO_SYM_OP_NONE: no operation; > > +VIRTIO_CRYPTO_SYM_OP_CIPHER: Cipher only operation on the data; > VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING: Chain any cipher with any > hash or mac operation. > > + > > +\subparagraph{Session operation: AEAD session}\label{sec:Device Types / > Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: AEAD > session} > > + > > +AEAD session requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_aead_session_para { > > + /* See VIRTIO_CRYPTO_AEAD_* above */ > > + le32 algo; > > + /* length of key */ > > + le32 key_len; > > + /* Authentication tag length */ > > + le32 tag_len; > > + /* The length of the additional authenticated data (AAD) in bytes */ > > + le32 aad_len; > > + /* encryption or decryption, See above VIRTIO_CRYPTO_OP_* */ > > + le32 op; > > + le32 padding; > > +}; > > + > > +struct virtio_crypto_aead_create_session_req { > > + /* Device-readable part */ > > + struct virtio_crypto_aead_session_para para; > > + u8 key[key_len]; > > + > > + /* Device-writeable part */ > > + struct virtio_crypto_session_input input; > > +}; > > +\end{lstlisting} > > + > > +\drivernormative{\subparagraph}{Session operation: create session}{Device > Types / Crypto Device / Device Operation / Control Virtqueue / Session > operation / Session operation: create session} > > + > > +\begin{itemize*} > > +\item The driver MUST set the control general header and corresponding > properties of the union in structure virtio_crypto_op_ctrl_req. See > \ref{sec:Device Types / Crypto Device / Device Operation / Control Virtqueue}. > > +\item The driver MUST set the \field{opcode} field based on service type: > CIPHER, HASH, MAC, or AEAD. > > > +\item The driver MUST set the \field{queue_id} field to show used dataq. > > Used for what? Is the driver obligued to use that dataq for the op reqs > associated > with the given session. If yes where is the normative statement? > Yes, I missed. > > +\end{itemize*} > > + > > +\devicenormative{\subparagraph}{Session operation: create session}{Device > Types / Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: > create session} > > + > > +\begin{itemize*} > > +\item The device MUST set the \field{session_id} field to a unique session > identifier when the device finishes processing session creation. > > I guess only if successfull (that is status == VIRTIO_CRYPTO_OK). > Sure. Let me add the condition. > > +\item The device MUST set the \field{status} field to one of the values of > enum VIRTIO_CRYPTO_STATUS. > > Maybe put this one first. The formulation could be more fortunate in > a sense that it's required to set the right status (_OK if successs, > _NOTSUPP if ...). > > What shall happen if the operation fails, e.g. becasue of out of resources? > That means the driver can't create session, and the following crypto operation are forbidden. > Is there some sort of limit for the amount of live sessions (related to > the previous question)? Obviously the device needs storage for the > session data. Can the guest DOS attack the host by creating an absurd number > of sessions? > Good question. It's true that the guest can trigger DoS attack. What's your opinion about the limit in the spec? Add a new feature? > > +\end{itemize*} > > + > > +\drivernormative{\subparagraph}{Session operation: destroy > session}{Device Types / Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: > destroy session} > > + > > +\begin{itemize*} > > +\item The driver MUST set the \field{opcode} field based on service type: > CIPHER, HASH, MAC, or AEAD. > > +\item The driver MUST set the \field{session_id} to a valid value assigned > > by > the device when the session was created. > > +\end{itemize*} > > + > > +\devicenormative{\subparagraph}{Session operation: destroy > session}{Device Types / Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: > destroy session} > > + > > +\begin{itemize*} > > +\item The device MUST set the \field{status} field to one of the values of > enum VIRTIO_CRYPTO_STATUS. > > Same as above. > OK. > > +\end{itemize*} > > + > > +\subsubsection{Data Virtqueue}\label{sec:Device Types / Crypto Device / > Device Operation / Data Virtqueue} > > + > > +The driver uses the data virtqueue to transmit crypto operation requests to > the device, > > +and completes the crypto operations. > > + > > +Session mode dataq requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_op_data_req { > > + 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; > > + } u; > > +}; > > +\end{lstlisting} > > + > > +Dataq requests for both session and stateless modes are as follows: > > This ain't very nice, I mean the 'both session and stateless modes' together So what's your idea? > with the above 'session mode dataq requests are'. In the meanwhile I know > what > you mean: if the SESSION_MODE feature bit was negotiated. > Yes, that's what I want. If the MUX_MODE feature bit is negotiated, the driver MUST use struct virtio_crypto_op_data_req_mux for requests. > > + > > +\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} > > + > > +Session mode HASH service requests are 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 in \field{para}, output > data and input data. > > +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 the status of executing the HASH operations. > > + > > +Stateless mode HASH service requests are as follows: > > + > > +\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 > \field{session_id} in struct virtio_crypto_op_header > > + to a valid value assigned by the device when the session was > created. > > +\item If the VIRTIO_CRYPTO_F_STATELESS_MODE feature bit is negotiated, > the driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto > requests. Otherwise, the driver MUST use struct virtio_crypto_op_data_req. > > +\item If the VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is > negotiated, 1) if the driver uses the stateless mode, then the driver MUST set > the \field{flag} field in struct virtio_crypto_op_header > > + to VIRTIO_CRYPTO_FLAG_STATELESS_MODE and MUST set the fields > in struct virtio_crypto_hash_para_statelession.sess_para, 2) if the driver > uses > the session mode, then the driver MUST set the \field{flag} field in struct > virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_STATE_MODE. > > +\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 in 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 VIRTIO_CRYPTO_STATUS. > > +\end{itemize*} > > + > > OK, I have read this to the end and I don't think there is anything which > requires > a comment at this stage. I would like to concentrate on your QEMU patches > first, > and I think we have enough questions/issues already. > Great thanks for your comments. Pls go ahead. :) Thanks, -Gonglei