Re: [Qemu-devel] [PATCH REPOST v19 1/2] virtio-crypto: Add virtio crypto device specification

[Longpeng (Mike)]

Hi Halil,

On 2017/9/15 23:59, Halil Pasic wrote:

> 
> 
> On 09/11/2017 03:12 AM, Longpeng(Mike) wrote:
>> From: Gonglei <address@hidden>
>>
>> The virtio crypto device is a virtual crypto device (ie. hardware
>> crypto accelerator card). Currently, the virtio crypto device provides
>> the following crypto services: CIPHER, MAC, HASH, and AEAD.
>>
>> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
>>
>> VIRTIO-153
>>
>> Signed-off-by: Gonglei <address@hidden>
>> Signed-off-by: Longpeng(Mike) <address@hidden>
>> ---
> [..]
> 
> 
>> +\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}).
>> +
>> +The header for controlq is of the following form:
>> +\begin{lstlisting}
>> +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
>> +
>> +struct virtio_crypto_ctrl_header {
>> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x02)
>> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x03)
>> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
>> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
>> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
>> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
>> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
>> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
>> +       VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
>> +    le32 opcode;
>> +    /* algo should be service-specific algorithms */
>> +    le32 algo;
>> +    le32 flag;
>> +    /* data virtqueue id */
>> +    le32 queue_id;
>> +};
>> +\end{lstlisting}
>> +
>> +The format of the controlq request depends on the VIRTIO_CRYPTO_F_MUX_MODE 
>> feature bit:
>> +
>> +\begin{itemize*}
>> +\item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is NOT negotiated the 
>> controlq request is
>> +    a fixed-size structure of form:
>> +\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}
>> +The header is the general header, and the union is of the 
>> algorithm-specific type or the
>> +virtio_crypto_destroy_session_req structure, which is set by the driver. 
>> All the properties
>> +in the union are shown as follows.
>> +
>> +\item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated the 
>> controlq request is composed
>> +    of two parts, the additional paramenters are preceded by the general 
>> header.
>> +
>> +\begin{lstlisting}
>> +struct virtio_crypto_op_ctrl_req_mux {
>> +    struct virtio_crypto_ctrl_header header;
>> +
>> +    /* additional paramenter */
>> +    u8 additional_para[addl_para_len];
>> +};
>> +\end{lstlisting}
>> +
>> +The additional paramenters are stored in a 
>> virtio_crypto_destroy_session_req structure or
>> +in a algorithm-specific structure:
>> +\begin{itemize*}
>> +\item struct virtio_crypto_sym_create_session_req
>> +\item struct virtio_crypto_hash_create_session_req
>> +\item struct virtio_crypto_mac_create_session_req
>> +\item struct virtio_crypto_aead_create_session_req
>> +\end{itemize*}
>> +All of the structures above are shown as follows.
>> +\end{itemize*}
>> +
>> +\paragraph{Session operation}\label{sec:Device Types / Crypto Device / 
>> Device Operation / Control Virtqueue / Session operation}
>> +
>> +The session is a
>> +handle which describes the cryptographic parameters to be applied to
>> +a number of buffers.
>> +
>> +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}
>> +
>> +The information required by HASH session creation is stored in the 
>> virtio_crypto_hash_create_session_req
>> +structure, including the hash parameters stored in \field{para}. 
>> \field{input} stores the result of this operation.
>> +
>> +\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}
>> +
>> +The information required by MAC session creation is stored in the 
>> virtio_crypto_mac_create_session_req
>> +structure, including the mac parameters stored in \field{para} and the 
>> authenticated key in \field{auth_key}.
>> +\field{input} stores the result of this operation.
>> +
>> +\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;that
>> +};
>> +\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 information required by symmetric algorithms session creation is stored 
>> in the
>> +virtio_crypto_sym_create_session_req structure, including the symmetric 
>> operation
>> +type in \field{op_type} and the cipher parameters stored in \field{cipher} 
>> or the
>> +algorithm chaining paramenters in \field{chain}.
>> +
>> +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}
>> +
>> +The information required by AEAD session creation is stored in the 
>> virtio_crypto_aead_create_session_req
>> +structure, including the aead parameters stored in \field{para} and the 
>> cipher key in \field{key}.
>> +\field{input} stores the result of this operation.
>> +
>> +\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 the corresponding 
>> algorithm-specific structure.
>> +    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.
> 
> I've failed to figure out the semantic behind queue_id. This could mean,
> that sessions and session_id's are queue scoped. But then we should be
> (IMHO) more explicit on this -- my guess is that session_id's aren't
> dataqueue scoped.
> 
>> +\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 use the corresponding algorithm-specific structure 
>> according to the
>> +    \field{opcode} in the control general header.
>> +\item The device MUST set the \field{status} field to one of the following 
>> values of enum
>> +    VIRTIO_CRYPTO_STATUS after finish a session creation:
>> +\begin{itemize*}
>> +\item VIRTIO_CRYPTO_OK if a session is created successfully.
>> +\item VIRTIO_CRYPTO_NOTSUPP if the requested algorithm or operation is 
>> unsupported.
>> +\item VIRTIO_CRYPTO_NOSPC if no free session ID (only when the 
>> VIRTIO_CRYPTO_F_MUX_MODE
>> +    feature bit is negotiated).
>> +\item VIRTIO_CRYPTO_ERR if failure not mentioned above occurs.
> 
> I guess an invalid queue_id would be among these.
> 
>> +\end{itemize*}
>> +\item The device MUST set the \field{session_id} field to a unique session 
>> identifieronly
> s/identifieronly/identifier only
>> +    if the status is set to VIRTIO_CRYPTO_OK.
>> +\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.
> 
> Destroy does not need to specify queue_id. That means session_id's aren't
> queue scoped from namespace perspective. The question remains what is
> queue_id good for, and whether a session type op request should be
> rejected if the the session id originates from a session creation
> request specifying a different dataqueue (not the dataqueue containing
> the given request)? 
> 

Gonglei and you have reached consensus, so we will make it reserved and must be
zero in the next version.

>> +\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 following 
>> values of enum VIRTIO_CRYPTO_STATUS.
>> +\begin{itemize*}
>> +\item VIRTIO_CRYPTO_OK if a session is created successfully.
>> +\item VIRTIO_CRYPTO_ERR if any failure occurs.
>> +\end{itemize*}
>> +\end{itemize*}
>> +
>> +\subsubsection{Data Virtqueue}\label{sec:Device Types / Crypto Device / 
>> Device Operation / Data Virtqueue}
>>
> 
> [..]
> 
> I've run through the rest. It some stuff seems very repetitive. I wonder if
> we can do better.
> 

> I also dislike this 5.7.7.5.3 Steps of Operation part. I also don't
> understand why is symmetric special in this respect (HASH, MAC and
> AEAD don't have a 'Steps of Operation' section.
> 

I have a offline discussion with Gonglei about it, we prefer to drop the
5.7.7.5.3 Steps of Operation part.

> I would like to try some things out with the reference implementation.
> Depending on how that goes I may or may not end up providing a detailed
> review for the rest before discussing what I've already addressed.
> 
> Regards,
> Halil
> 
> 
> .
> 


-- 
Regards,
Longpeng(Mike)