> From: virtio-...@lists.oasis-open.org [mailto:virtio-...@lists.oasis-open.org]
> On Behalf Of Michael S. Tsirkin
> Subject: [virtio-dev] Re: [Qemu-devel] [PATCH v13 1/2] virtio-crypto: Add
> virtio
> crypto device specification
>
> 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.
>
OK.
> > +
> > 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
>
Yes.
> >
> > 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?
>
Dropped.
> > - 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?
>
Dropped.
> > - 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
>
OK.
> > +\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.
>
OK, will drop it.
> > \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?
>
Yes.
> add "to one of the values"? Also, just list the enum name here in case
> we add more values?
>
OK, make sense.
> not support - not supported?
>
OK.
> > \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
>
OK.
> 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.
>
OK.
> > \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.
>
Yes, I'll check all of them, thanks!
Regards,
-Gonglei
