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

["Gonglei (Arei)" <arei.gonglei@huawei.com>]

> From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@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