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

[Gonglei (Arei)]

Hi Halil,

> 
> > 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.
> 
OK, will do.

> 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:
> 
Make pretty sense! I tried to find a good way to express those kind of data,
but I didn't notice it. Thank you so much.

> 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.
> 
OK, will recheck. But I'm not a native English speaker, so if you
give me some specific comments about formulations
will be appreciated ;)

> 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.
> 
Thanks, Let's make it better and better.

Regards,
-Gonglei