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

[Halil Pasic]

On 11/10/2016 02:15 PM, Michael S. Tsirkin wrote:
> On Thu, Nov 10, 2016 at 09:37:40AM +0000, Gonglei (Arei) wrote:
>> > Hi,
>> > 
>> > I attach a diff for next version in order to review more convenient, with
>> > 
>> >  - Drop the all gap stuff;
>> >  - Drop all structures undefined in virtio_crypto.h
>> >  - re-describe per request for per crypto service avoid confusion
>> > 
>> > Pls review, thanks!
>> > 
>> > 
>> > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
>> > index 448296e..ab17e7b 100644
>> > --- a/virtio-crypto.tex
>> > +++ b/virtio-crypto.tex
>> > @@ -69,13 +69,13 @@ The following services are defined:
>> >  
>> >  \begin{lstlisting}
>> >  /* CIPHER service */
>> > -#define VIRTIO_CRYPTO_SERVICE_CIPHER (0)
>> > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
>> >  /* HASH service */
>> > -#define VIRTIO_CRYPTO_SERVICE_HASH   (1)
>> > +#define VIRTIO_CRYPTO_SERVICE_HASH   1
>> >  /* MAC (Message Authentication Codes) service */
>> > -#define VIRTIO_CRYPTO_SERVICE_MAC    (2)
>> > +#define VIRTIO_CRYPTO_SERVICE_MAC    2
>> >  /* AEAD (Authenticated Encryption with Associated Data) service */
>> > -#define VIRTIO_CRYPTO_SERVICE_AEAD   (3)
>> > +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
>> >  \end{lstlisting}
>> >  
>> >  The last driver-read-only fields specify detailed algorithms masks 
>> > @@ -210,7 +210,7 @@ data that should be utilized in operations, and input 
>> > data is equal to
>> >  The general header for controlq is as follows:
>> >  
>> >  \begin{lstlisting}
>> > -#define VIRTIO_CRYPTO_OPCODE(service, op)   ((service << 8) | (op))
>> > +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
>> >  
>> >  struct virtio_crypto_ctrl_header {
>> >  #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
>> > @@ -380,20 +380,18 @@ struct virtio_crypto_mac_session_para {
>> >      le32 auth_key_len;
>> >      le32 padding;
>> >  };
>> > -struct virtio_crypto_mac_session_output {
>> > -    le64 auth_key_addr; /* guest key physical address */
>> > -};
>> >  
>> >  struct virtio_crypto_mac_create_session_req {
>> >      /* Device-readable part */
>> >      struct virtio_crypto_mac_session_para para;
>> > -    struct virtio_crypto_mac_session_output out;
>> > +    /* The authenticated key buffer */
>> > +    /* output data here */
>> > +
>> >      /* Device-writable part */
>> >      struct virtio_crypto_session_input input;
>> >  };
>> >  \end{lstlisting}
>> >  
>> > -The output data are
>> >  \subparagraph{Session operation: Symmetric algorithms 
>> > session}\label{sec:Device Types / Crypto Device / Device
>> >  Operation / Control Virtqueue / Session operation / Session operation: 
>> > Symmetric algorithms session}
>> >  
>> > @@ -413,13 +411,13 @@ struct virtio_crypto_cipher_session_para {
>> >      le32 padding;
>> >  };
>> >  
>> > -struct virtio_crypto_cipher_session_output {
>> > -    le64 key_addr; /* guest key physical address */
>> > -};
>> > -
>> >  struct virtio_crypto_cipher_session_req {
>> > +    /* Device-readable part */
>> >      struct virtio_crypto_cipher_session_para para;
>> > -    struct virtio_crypto_cipher_session_output out;
>> > +    /* The cipher key buffer */
>> > +    /* Output data here */
>> > +
>> > +    /* Device-writable part */
>> >      struct virtio_crypto_session_input input;
>> >  };
>> >  \end{lstlisting}
>> > @@ -448,18 +446,20 @@ struct virtio_crypto_alg_chain_session_para {
>> >      le32 padding;
>> >  };
>> >  
>> > -struct virtio_crypto_alg_chain_session_output {
>> > -    struct virtio_crypto_cipher_session_output cipher;
>> > -    struct virtio_crypto_mac_session_output mac;
>> > -};
>> > -
>> >  struct virtio_crypto_alg_chain_session_req {
>> > +    /* Device-readable part */
>> >      struct virtio_crypto_alg_chain_session_para para;
>> > -    struct virtio_crypto_alg_chain_session_output out;
>> > +    /* The cipher key buffer */
>> > +    /* The authenticated key buffer */
>> > +    /* Output data here */
>> > +
>> > +    /* Device-writable part */
>> >      struct virtio_crypto_session_input input;
>> >  };
>> >  \end{lstlisting}
>> >  
>> > +The output data includs both cipher key buffer and authenticated key 
>> > buffer.
> .. includes both the cipher key buffer and the uthenticated key buffer.
> 
>> > +
>> >  The packet for symmetric algorithm is as follows:
>> >  
>> >  \begin{lstlisting}
>> > @@ -503,13 +503,13 @@ struct virtio_crypto_aead_session_para {
>> >      le32 padding;
>> >  };
>> >  
>> > -struct virtio_crypto_aead_session_output {
>> > -    le64 key_addr; /* guest key physical address */
>> > -};
>> > -
>> >  struct virtio_crypto_aead_create_session_req {
>> > +    /* Device-readable part */
>> >      struct virtio_crypto_aead_session_para para;
>> > -    struct virtio_crypto_aead_session_output out;
>> > +    /* The cipher key buffer */
>> > +    /* Output data here */
>> > +
>> > +    /* Device-writeable part */
>> >      struct virtio_crypto_session_input input;
>> >  };
>> >  \end{lstlisting}
>> > @@ -568,19 +568,13 @@ struct virtio_crypto_op_data_req {
>> >  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 idata structure for all symmetric algorithms, 
>> > including CIPHER, HASH, MAC, and AEAD.
>> > +There is a unified idata structure for all service, including CIPHER, 
>> > HASH, MAC, and AEAD.
> for all services
> 
>> >  
>> >  The structure is defined as follows:
>> >  
>> >  \begin{lstlisting}
>> > -struct virtio_crypto_sym_input {
>> > -    /* Destination data guest address, it's useless for plain HASH and 
>> > MAC */
>> > -    le64 dst_data_addr;
>> > -    /* Digest result guest address, it's useless for plain cipher algos */
> guest address -> physical address?
> it's useless -> unused?
> 
>> > -    le64 digest_result_addr;
>> > -
>> > -    le32 status;
>> > -    le32 padding;
>> > +struct virtio_crypto_inhdr {
>> > +    u8 status;
>> >  };
>> >  
>> >  \end{lstlisting}
>> > @@ -595,39 +589,29 @@ struct virtio_crypto_hash_para {
>> >      le32 hash_result_len;
>> >  };
>> >  
>> > -struct virtio_crypto_hash_input {
>> > -    struct virtio_crypto_sym_input input;
>> > -};
>> > -
>> > -struct virtio_crypto_hash_output {
>> > -    /* source data guest address */
> guest -> physical?
> 
>> > -    le64 src_data_addr;
>> > -};
>> > -
>> >  struct virtio_crypto_hash_data_req {
>> >      /* Device-readable part */
>> >      struct virtio_crypto_hash_para para;
>> > -    struct virtio_crypto_hash_output odata;
>> > +    /* Source buffer */
>> > +    /* Output data here */
>> > +
>> >      /* Device-writable part */
>> > -    struct virtio_crypto_hash_input idata;
>> > +    /* Digest result buffer */
>> > +    /* Input data here */
>> > +    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 request only occupies one entry
>> > -in the Vring Descriptor Table in the virtio crypto device's dataq, which 
>> > improves
>> > -the throughput of data transmitted for the HASH service, so that the 
>> > virtio crypto
>> > -device can be better accelerated.
>> > +used to run the HASH operations. 
>> >  
>> > -The information includes the source data guest physical address stored by 
>> > \field{odata}.\field{src_data_addr},
>> > -length of source data stored by \field{para}.\field{src_data_len}, and 
>> > the digest result guest physical address
>> > -stored by \field{digest_result_addr} used to save the results of the HASH 
>> > operations.
>> > -The address and length can determine exclusive content in the guest 
>> > memory.
>> > +The information includes the hash paramenters stored by \field{para}, 
>> > output data and input data.
>> > +The output data here includs the source buffer and the input data 
>> > includes the digest result buffer used to save the results of the HASH 
>> > operations.
> includs -> includes
> 
>> > +\field{inhdr} store the executing status of HASH operations.
>> >  
>> >  \begin{note}
>> > -The guest memory is always guaranteed to be allocated and 
>> > physically-contiguous
>> > -pointed by \field{digest_result_addr} in struct virtio_crypto_hash_input 
>> > and
>> > -\field{src_data_addr} in struct virtio_crypto_hash_output.
>> > +The request should by preference occupies one entry in the Vring 
>> > Descriptor Table in the virtio crypto device's dataq, which improves
> Don't use should outside confirmance statements.
> 
> occupies -> occupy
> 
>> > +the throughput of data transmitted for the HASH service, so that the 
>> > virtio crypto device can be better accelerated.
> I'd just drop this note - I don't see why is crypto special here.
> 
>> >  \end{note}
>> >  
>> >  \drivernormative{\paragraph}{HASH Service Operation}{Device Types / 
>> > Crypto Device / Device Operation / HASH Service Operation}
>> > @@ -641,8 +625,8 @@ pointed by \field{digest_result_addr} in struct 
>> > virtio_crypto_hash_input and
>> >  \devicenormative{\paragraph}{HASH Service Operation}{Device Types / 
>> > Crypto Device / Device Operation / HASH Service Operation}
>> >  
>> >  \begin{itemize*}
>> > -\item The device MUST copy the results of HASH operations to the guest 
>> > memory recorded by \field{digest_result_addr} field in struct 
>> > virtio_crypto_hash_input.
>> > -\item The device MUST set \field{status} in strut 
>> > virtio_crypto_hash_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: 
>> > creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
>> > +\item The device MUST copy the results of HASH operations to the guest 
>> > memory recorded by digest result buffer if HASH operations success.
>> > +\item The device MUST set \field{status} in strut virtio_crypto_inhdr: 
>> > VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device 
>> > error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS: invalid 
>> > session ID when the crypto operation is implemented.
> strut -> struct?
> 
> add "to one of the values"? Also, just list the enum name here in case
> we add more values?
> 
> not support - not supported?
> 
>> >  \end{itemize*}
>> >  
>> >  \subsubsection{MAC Service Operation}\label{sec:Device Types / Crypto 
>> > Device / Device Operation / MAC Service Operation}
>> > @@ -652,39 +636,29 @@ struct virtio_crypto_mac_para {
>> >      struct virtio_crypto_hash_para hash;
>> >  };
>> >  
>> > -struct virtio_crypto_mac_input {
>> > -    struct virtio_crypto_sym_input input;
>> > -};
>> > -
>> > -struct virtio_crypto_mac_output {
>> > -    struct virtio_crypto_hash_output hash_output;
>> > -};
>> > -
>> >  struct virtio_crypto_mac_data_req {
>> >      /* Device-readable part */
>> >      struct virtio_crypto_mac_para para;
>> > -    struct virtio_crypto_mac_output odata;
>> > +    /* Source buffer */
>> > +    /* Output data here */
>> > +
>> >      /* Device-writable part */
>> > -    struct virtio_crypto_mac_input idata;
>> > +    /* Digest result buffer */
>> > +    /* Input data here */
>> > +    struct virtio_crypto_inhdr inhdr;
>> >  };
>> >  \end{lstlisting}
>> >  
>> >  Each data request uses virtio_crypto_mac_data_req structure to store 
>> > information
>> > -used to run the MAC operations. The request only occupies one entry
>> > -in the Vring Descriptor Table in the virtio crypto device's dataq, which 
>> > improves
>> > -the throughput of data transmitted for the MAC service, so that the 
>> > virtio crypto
>> > -device can get the better result of acceleration.
>> > -
>> > -The information includes the source data guest physical address stored by 
>> > \field{hash_output}.\field{src_data}.\field{addr},
>> > -the length of source data stored by 
>> > \field{hash_output}.\field{src_data}.\field{len}, and the digest result 
>> > guest physical address
>> > -stored by \field{digest_result_addr} used to save the results of the MAC 
>> > operations.
>> > +used to run the MAC operations. 
>> >  
>> > -The address and length can determine exclusive content in the guest 
>> > memory.
>> > +The information includes the hash paramenters stored by \field{para}, 
>> > output data and input data.
>> > +The output data here includs the source buffer and the input data 
>> > includes the digest result buffer used to save the results of the MAC 
>> > operations.
> 
> 
> includes
> 
>> > +\field{inhdr} store the executing status of MAC operations.
> stores
> 
> the executing status -> status of executing the MAC operations?
> 
>> >  
>> >  \begin{note}
>> > -The guest memory is always guaranteed to be allocated and 
>> > physically-contiguous
>> > -pointed by \field{digest_result_addr} in struct virtio_crypto_sym_input 
>> > and
>> > -\field{hash_output}.\field{src_data_addr} in struct 
>> > virtio_crypto_mac_output.
>> > +The request should by preference occupies one entry in the Vring 
>> > Descriptor Table in the virtio crypto device's dataq, which improves
>> > +the throughput of data transmitted for the MAC service, so that the 
>> > virtio crypto device can be better accelerated.
> Again, let's just drop.
> 
>> >  \end{note}
>> >  
>> >  \drivernormative{\paragraph}{MAC Service Operation}{Device Types / Crypto 
>> > Device / Device Operation / MAC Service Operation}
>> > @@ -698,8 +672,8 @@ pointed by \field{digest_result_addr} in struct 
>> > virtio_crypto_sym_input and
>> >  \devicenormative{\paragraph}{MAC Service Operation}{Device Types / Crypto 
>> > Device / Device Operation / MAC Service Operation}
>> >  
>> >  \begin{itemize*}
>> > -\item The device MUST copy the results of MAC operations to the guest 
>> > memory recorded by \field{digest_result_addr} field in struct 
>> > virtio_crypto_mac_input.
>> > -\item The device MUST set \field{status} in strut 
>> > virtio_crypto_mac_input: VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: 
>> > creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not support.
>> > +\item The device MUST copy the results of MAC operations to the guest 
>> > memory recorded by digest result buffer if HASH operations success.
>> > +\item The device MUST set \field{status} in strut virtio_crypto_inhdr: 
>> > VIRTIO_CRYPTO_OK: success; VIRTIO_CRYPTO_ERR: creation failed or device 
>> > error; VIRTIO_CRYPTO_NOTSUPP: not support; VIRTIO_CRYPTO_INVSESS: invalid 
>> > session ID when the crypto operation is implemented.
> 
> same as above. I guess same issues repeat below, did not review.
> 

With this fixup patch commenting on the series becomes quite a hassle.
I would recommend you to fix the issues Michael has pointed out, maybe
do another consistency check regarding naming and regarding layout/format
notation across the whole specification, and re-spin.

An example for the lack chapter internal consistency is: "There is a unified
idata structure for all service, including CIPHER, HASH, MAC, and AEAD." since
this is the only occurrence of idata in the specification. Another one is 
struct virtio_crypto_hash_para {
/* length of source data */
le32 src_data_len;         <-- here you call it source data
/* hash result length */   <-- here you call it hash_result
le32 hash_result_len;
};
struct virtio_crypto_hash_data_req {
/* Device-readable part */
struct virtio_crypto_hash_para para;
/* Source buffer */         <-- here you call it buffer
/* Output data here */
/* Device-writable part */
/* Digest result buffer */  <-- here you call it digest result
/* Input data here */
struct virtio_crypto_inhdr inhdr;
};

For the cross specification consistency I would encourage you to
not use comments ambiguously for comments and for representing a
byte array variable size holding some kind of data (like src buf
dest/result buf, key buf).

The rest of the specification seems to use (variable length)
array of u8 notation for that when specifying the layout of
requests (or just describes stuff with words). For example:

struct virtio_blk_req {
le32 type;
le32 reserved;
le64 sector;
u8 data[][512];                    <-- here
u8 status;
};

or

struct virtio_scsi_req_cmd {
// Device-readable part
u8 lun[8];
le64 id;
u8 task_attr;
u8 prio;
u8 crn;
u8 cdb[cdb_size];
// The next two fields are only present if VIRTIO_SCSI_F_T10_PI
// is negotiated.
le32 pi_bytesout;
le32 pi_bytesin;
u8 pi_out[pi_bytesout];              <-- here
u8 dataout[];                        <-- here
// Device-writable part
le32 sense_len;
le32 residual;
le16 status_qualifier;
u8 status;
u8 response;
u8 sense[sense_size];
// The next two fields are only present if VIRTIO_SCSI_F_T10_PI
// is negotiated
u8 pi_in[pi_bytesin];                <-- here
u8 datain[];                         <-- here
};


Furthermore I would refrain from formulations like "guest
memory recorded by digest result buffer". Instead try to
use formulations consistent with the rest of the specification.

Finally I want to point out that the things got much easier
to understand with this last fixup (IMHO) despite of all
the typos, and that there are still more issues we did not
point out explicitly.

Regards
Halil