> -----Original Message-----
> From: Zeng, Xin [mailto:xin.z...@intel.com]
> Sent: Friday, September 23, 2016 1:39 PM
> To: Gonglei (Arei); virtio-...@lists.oasis-open.org; qemu-devel@nongnu.org
> Cc: m...@redhat.com; Keating, Brian A; Griffin, John; Ma, Liang J; Hanweidong
> (Randy); Wubin (H)
> Subject: RE: [V0 1/1] virtio crypto device specification: asymmetric crypto
> service
> 
> On Wednesday, September 21, 2016 3:03 PM, Gonglei (Arei) Wrote:
> > > -----Original Message-----
> > > From: Xin Zeng [mailto:xin.z...@intel.com]
> > > Sent: Wednesday, September 21, 2016 1:15 PM
> > > To: virtio-...@lists.oasis-open.org; qemu-devel@nongnu.org; Gonglei
> > (Arei)
> > > Cc: m...@redhat.com; brian.a.keat...@intel.com; john.grif...@intel.com;
> > > liang.j...@intel.com; Huangweidong (C); Xin Zeng
> > > Subject: [V0 1/1] virtio crypto device specification: asymmetric crypto
> > service
> > >
> > > This patch introduces asymmetric crypto service into virtio crypto
> > > device. The asymmetric crypto service can be referred as signature,
> > > verification, encryption, decryption, key generation and key exchange.
> > > This patch depends on another virtio crypto device spec patch:
> > > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04563.html.
> > >
> > > Signed-off-by: Xin Zeng <xin.z...@intel.com>
> > > ---
> > >  virtio-crypto.tex | 932
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 931 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> > > index c3554e3..699d8dc 100644
> > > --- a/virtio-crypto.tex
> > > +++ b/virtio-crypto.tex
> > > @@ -46,6 +46,7 @@ struct virtio_crypto_config {
> > >      le32 kdf_algo;
> > >      le32 aead_algo;
> > >      le32 primitive_algo;
> > > +    le32 rsa_padding;
> >
> > The structure doesn't 64-bit aligned now. Please add a padding.
> >
> 
> Yes. We also need remove some fields for now as Michael suggested in another
> mail.
> 
> > >  };
> > >  \end{lstlisting}
> > >
> > > @@ -67,6 +68,7 @@ The following services are defined:
> > >  #define VIRTIO_CRYPTO_SERVICE_HASH   (1) /* HASH service */
> > >  #define VIRTIO_CRYPTO_SERVICE_MAC    (2) /* MAC (Message
> > > Authentication Codes) service */
> > >  #define VIRTIO_CRYPTO_SERVICE_AEAD   (3) /* AEAD (Authenticated
> > > Encryption with Associated Data) service */
> > > +#define VIRTIO_CRYPTO_SERVICE_ASYM  (4) /* Asymmetric crypto
> > service*/
> > >  \end{lstlisting}
> > >
> > >  The last driver-read-only fields specify detailed algorithms masks
> > > @@ -140,6 +142,28 @@ The following AEAD algorithms are defined:
> > >  #define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
> > >  \end{lstlisting}
> > >
> > > +The following asymmetric algorithms are defined:
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_CRYPTO_ASYM_NONE        0
> > > +#define VIRTIO_CRYPTO_ASYM_RSA         1
> > > +#define VIRTIO_CRYPTO_ASYM_DSA         2
> > > +#define VIRTIO_CRYPTO_ASYM_DH          3
> > > +#define VIRTIO_CRYPTO_ASYM_ECDSA       4
> > > +#define VIRTIO_CRYPTO_ASYM_ECDH         5
> > > +\end{lstlisting}
> > > +
> > > +The following rsa padding capabilities are defined:
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_CRYPTO_RSA_NO_PADDING         0
> > > +#define VIRTIO_CRYPTO_RSA_PKCS1_PADDING      1
> > > +#define VIRTIO_CRYPTO_RSA_SSLV23_PADDING     2
> > > +#define VIRTIO_CRYPTO_RSA_PKCS1_OAEP_PADDING 3
> > > +#define VIRTIO_CRYPTO_RSA_X931_PADDING       4
> > > +#define VIRTIO_CRYPTO_RSA_PKCS1_PSS_PADDING  5
> > > +\end{lstlisting}
> > > +
> > >  \begin{note}
> > >  More algorithms will be defined in the future.
> > >  \end{note}
> > > @@ -238,6 +262,18 @@ struct virtio_crypto_op_header {
> > >      VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> > >  #define VIRTIO_CRYPTO_AEAD_DECRYPT \
> > >      VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> > > +#define VIRTIO_CRYPTO_ASYM_SIGN    \
> > > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x00)
> > > +#define VIRTIO_CRYPTO_ASYM_VERIFY \
> > > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x01)
> > > +#define VIRTIO_CRYPTO_ASYM_ENCRYPT  \
> > > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x02)
> > > +#define VIRTIO_CRYPTO_ASYM_DECRYPT  \
> > > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x03)
> > > +#define VIRTIO_CRYPTO_ASYM_KEY_GEN  \
> > > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x04)
> > > +#define VIRTIO_CRYPTO_ASYM_KEY_EXCHG \
> > > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x05)
> > >      le32 opcode;
> > >      /* algo should be service-specific algorithms */
> > >      le32 algo;
> > > @@ -540,6 +576,26 @@ struct virtio_crypto_op_data_req {
> > >          struct virtio_crypto_hash_data_req  hash_req;
> > >          struct virtio_crypto_mac_data_req   mac_req;
> > >          struct virtio_crypto_aead_data_req  aead_req;
> > > +        struct virtio_crypto_ecdsa_sign_req ecdsa_sign_req;
> > > +        struct virtio_crypto_dsa_sign_req dsa_sign_req;
> > > +        struct virtio_crypto_rsa_sign_req rsa_sign_req;
> > > +
> > > +        struct virtio_crypto_ecdsa_verify_req ecdsa_verify_req;
> > > +        struct virtio_crypto_dsa_verify_req dsa_verify_req;
> > > +        struct virtio_crypto_rsa_verify_req rsa_verify_req;
> > > +
> > > +        struct virtio_crypto_rsa_enc_req rsa_enc_req
> > > +        struct virtio_crypto_rsa_dec_req rsa_dec_req;
> > > +
> > > +        struct virtio_crypto_rsa_keygen_req rsa_keygen_req;
> > > +        struct virtio_crypto_dsa_keygen_req dsa_keygen_req;
> > > +        struct virtio_crypto_ec_keygen_req ec_keygen_req;
> > > +
> > > +        struct virtio_crypto_dh_keyexchg_param_gen_req
> > > dh_keyexchg_param_gen_req;
> > > +        struct virtio_crypto_dh_keyexchg_key_gen_req
> > > dh_keyexchg_key_gen_req;
> > > +        struct virtio_crypto_dh_keyexchg_key_compute_req
> > > dh_keyexchg_key_compute_req;
> > > +        struct virtio_crypto_ecdh_keyexchg_key_gen_req
> > > ecdh_keyexchg_key_gen_req;
> > > +        struct virtio_crypto_ecdh_keyexchg_key_compute_req
> > > ecdh_keyexchg_key_compute_req;
> > >      } u;
> > >  };
> > >  \end{lstlisting}
> > > @@ -939,4 +995,878 @@ The device MUST parse the
> > > virtio_crypto_aead_data_req based on the \field{op_cod
> > >  The device MUST copy the result of cryptographic operation to the guest
> > > memory recorded by \field{dst_data}.\field{addr} field in struct
> > > virtio_crypto_aead_input.
> > >  The device MUST copy the digest result to the guest memory recorded by
> > > \field{digest_result_addr} field in struct virtio_crypto_aead_input.
> > >  The device MUST set the \field{status} field in strut
> > virtio_crypto_aead_input:
> > > VIRTIO_CRYPTO_OP_OK: success; VIRTIO_CRYPTO_OP_ERR: creation
> failed
> > or
> > > device error; VIRTIO_CRYPTO_OP_NOTSUPP: not supported.
> > > -When the \field{op_code} is VIRTIO_CRYPTO_AEAD_DECRYPT, the device
> > > MUST verify and return the verification result to the driver, and if the
> > > verification result is incorrect, VIRTIO_CRYPTO_OP_BADMSG (bad
> message)
> > > MUST be returned to the driver.
> > > \ No newline at end of file
> > > +When the \field{op_code} is VIRTIO_CRYPTO_AEAD_DECRYPT, the device
> > > MUST verify and return the verification result to the driver, and if the
> > > verification result is incorrect, VIRTIO_CRYPTO_OP_BADMSG (bad
> message)
> > > MUST be returned to the driver.
> > > +\subsubsection{Asymmetric Crypto Service}\label{sec:Device Types /
> > Crypto
> > > Device / Device Operation /
> > > +Crypto operation / Asymmetric Crypto Service}
> > > +
> > > +In general, asymmetric crypto service can be referred as signature,
> > > verification,
> > > +encryption, decryption, key generation and key exchange. Not each
> > algorithm
> > > supports
> > > +all these services.
> > > +
> > > +Unlike symmetric crypto service, asymmetric crypto service normally
> > does't
> > > +need session operations, the request is encapsulated within one packet
> > which
> > > is represented
> > > +by \field{virtio_crypto_op_data_req}.
> > > +(see \ref{sec:Device Types / Crypto Device / Device Operation / Data
> > > Virtqueue}).
> > > +
> > > +Once service request is handled by the device, the device writes back the
> > > operation results to the corresponding
> > > +fields in the request packet.
> > > +
> > > +\paragraph{General Device Requirements: Asymmetric Crypto
> > > Service}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> >
> > If they are requirements, please use unified
> > \devicenormative{\paragraph}{ Asymmetric Crypto Service}...
> >
> 
> Thanks, will fix it in next version.
> 
> > > +The device SHOULD parse the field \field{opcode} within
> > > \field{virtio_crypto_op_header} then handle the corresponding service
> > request.
> > > +
> > > +The device SHOULD set the operation status in field \field{status} within
> > > \field{idata}.
> > > +
> > > +The device SHOULD write back the operation result to the iovec buffer,
> > which
> > > is represented by \field{virtio_crypto_iovec} within \field{idata} if the
> > operation
> > > is successful.
> > > +
> > > +\paragraph{General Driver Requirements: Asymmetric Crypto
> > > Service}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> >
> > Please use \drivernormative{\paragraph}...
> > You'd better go over the whole patch about this point :)
> >
> > > +The driver SHOULD set the corresponding \field{opcode} in
> > > \field{virtio_crypto_op_header} according to different services.
> > > +
> > > +The driver SHOULD offers operatable iovec buffer represented by
> > > \field{virtio_crypto_iovec} within \field{idata} if the service request
> > structure
> > > defines.
> > > +
> > > +\paragraph{Signature: ECDSA signature operation}\label{sec:Device Types
> > /
> > > Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> >
> > The above label is incorrect. It should be:
> >
> > {sec:Device Types / Crypto Device / Device Operation / Crypto operation /
> > Asymmetric Crypto Service / Signature: ECDSA signature operation}
> >
> > So that we can identify the section by label. The same with below labels,
> > need to be fiexed.
> >
> 
> Thanks, will fix it.
> 
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_ec_group {
> > > +    struct virtio_crypto_iovec xg;
> > > +    struct virtio_crypto_iovec yg;
> > > +    struct virtio_crypto_iovec n;
> > > +    struct virtio_crypto_iovec q;
> > > +    struct virtio_crypto_iovec a;
> > > +    struct virtio_crypto_iovec b;
> > > +    struct virtio_crypto_iovec h;
> > > +};
> > > +
> > > +typedef enum {
> > > +    VIRTIO_CRYPTO_EC_FIELD_TYPE_PRIME,
> > > +    VIRTIO_CRYPTO_EC_FIELD_TYPE_BINARY
> > > +} VIRTIO_CRYPTO_EC_FIELD_TYPE;
> > > +
> > > +struct virtio_crypto_ecdsa_sign_para{
> > > +    /* Hash alogrithms applied to msg */
> > > +    le32 hash_algo;
> > > +    le32 reserved;
> > > +    VIRTIO_CRYPTO_EC_FIELD_TYPE field_type;
> > > +
> > > +    /* EC Group parameters */
> > > +    struct virtio_crypto_ec_group ec;
> > > +    /* Random value */
> > > +    struct virtio_crypto_iovec k;
> > > +    /* Private Key */
> > > +    struct virtio_crypto_iovec d;
> > > +};
> > > +
> > > +struct virtio_crypto_ecdsa_sign_output {
> > > +    /* Message need to be signed */
> > > +    struct virtio_crypto_iovec msg;
> > > +};
> > > +
> > > +struct virtio_crypto_ecdsa_sign_input {
> > > +    /* operation result */
> > > +    le32 status;
> > > +    le32 reserved;
> > > +
> > > +    /* signature result */
> > > +    struct virtio_crypto_iovec r;
> > > +    struct virtio_crypto_iovec s;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +ECDSA signature operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_ecdsa_sign_req {
> > > +    struct virtio_crypto_ecdsa_sign_para parameter;
> > > +    struct virtio_crypto_ecdsa_sign_output odata;
> > > +
> > > +    struct virtio_crypto_ecdsa_sign_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: ECDSA signature
> > > operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata} according to
> > the
> > > general device requirments for asymmetric crypto service.
> > > +
> > > +\subparagraph{Driver Requirements: ECDSA signature
> > > operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_SIGN,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_ECDSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} and \field{odata}
> > except
> > > field \field{reserved}.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it operates other fields in \field{idata}.
> > > +Only if the operation is successful, the \field{r,s} in \field{idata} are
> > operatable.
> >
> > \field{r,s} ? Is't legal?
> >
> 
> It's legal. No complaints for this from xelatex.
> 
> > > +
> > > +\paragraph{Signature: DSA signature operation}\label{sec:Device Types /
> > > Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_dsa_group {
> > > +    struct virtio_crypto_iovec p;
> > > +    struct virtio_crypto_iovec q;
> > > +    struct virtio_crypto_iovec g;
> > > +};
> > > +
> > > +struct virtio_crypto_dsa_sign_para {
> > > +    /* Hash alogrithms applied to msg */
> > > +    le32 hash_algo;
> > > +    le32 reserved;
> > > +
> > > +    /* DSA group parameter */
> > > +    struct virtio_crypto_dsa_group dsa;
> > > +    /* Random value */
> > > +    struct virtio_crypto_iovec k;
> > > +    /* Private Key */
> > > +    struct virtio_crypto_iovec x;
> > > +};
> > > +
> > > +struct virtio_crypto_dsa_sign_output {
> > > +    struct virtio_crypto_iovec msg;
> > > +};
> > > +
> > > +struct virtio_crypto_dsa_sign_input  {
> > > +    /* operation result */
> > > +    le32 status;
> > > +    le32 reserved;
> > > +
> > > +    /* signature result */
> > > +    struct virtio_crypto_iovec r;
> > > +    struct virtio_crypto_iovec s;
> > > +};
> > > +\end{lstlisting}
> > > +DSA signature operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_dsa_sign_req {
> > > +    struct virtio_crypto_dsa_sign_para parameter;
> > > +    struct virtio_crypto_dsa_sign_output odata;
> > > +
> > > +    struct virtio_crypto_dsa_sign_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: DSA signature
> > > operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata} according to
> > the
> > > general device requirments for asymmetric crypto service.
> > > +
> > > +\subparagraph{Driver Requirements: DSA signature
> > > operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_SIGN,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_DSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} and \field{odata}
> > except
> > > field \field{reserved}.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it operates other fields in \field{idata}.
> > > +Only if the operation is successful, the \field{r,s} in \field{idata} are
> > operatable.
> > > +
> >
> > I think s/SHOULD/MUST/ is more appropriate. Isn't it?
> 
> Yes, in this case it's better, thanks.
> 
> >
> > > +\paragraph{Signature: RSA signature operation}\label{sec:Device Types /
> > > Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_pub_key{
> > > +    /* The RSA modules */
> > > +    struct virtio_crypto_iovec n;
> > > +    /* The RSA public exponent */
> > > +    struct virtio_crypto_iovec e;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_priv_key_rep1{
> > > +    /* The RSA modules */
> > > +    struct virtio_crypto_iovec n;
> > > +    /* The RSA private exponent */
> > > +    struct virtio_crypto_iovec d;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_priv_key_rep2{
> > > +    /* The first factor */
> > > +    struct virtio_crypto_iovec p;
> > > +    /* The second factor */
> > > +    struct virtio_crypto_iovec q;
> > > +    /* The first factor's CRT exponent */
> > > +    struct virtio_crypto_iovec dp;
> > > +    /* The second factor's CRT exponent */
> > > +    struct virtio_crypto_iovec dq;
> > > +    /* The first CRT coeffieient */
> > > +    struct virtio_crypto_iovec qinv;
> > > +    /* The factors array when prime_number > 2 */
> > > +    struct virtio_crypto_iovec r[];
> > > +    /* The CRT exponent array when prime_number > 2 */
> > > +    struct virtio_crypto_iovec d[];
> > > +    /* The CRT coeffieient array when prime_number > 2 */
> > > +    struct virtio_crypto_iovec t[];
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_priv_key {
> > > +    le32 prime_number;
> > > +#define VIRTIO_CRYPTO_RSA_PRIV_KEY_REPTYPE1 0
> > > +#define VIRTIO_CRYPTO_RSA_PRIV_KEY_REPTYPE2 1
> > > +    le32 priv_key_rep_type;
> > > +    union {
> > > +        struct virtio_crypto_rsa_priv_key_rep1 privkey_rep1;
> > > +        struct virtio_crypto_rsa_priv_key_rep2 privkey_rep2;
> > > +    }key;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_sign_para {
> > > +    /* Hash alogrithms applied to msg */
> > > +    le32 hash_algo;
> > > +    le32 padding_mode;
> > > +    struct virtio_crypto_rsa_priv_key priv_key;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_sign_output {
> > > +    struct virtio_crypto_iovec msg;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_sign_input {
> > > +    le32 status;
> > > +    le32 reserved;
> > > +
> > > +    struct virtio_crypto_iovec signature;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +RSA signature operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_sign_req {
> > > +    struct virtio_crypto rsa_sign_para parameter;
> > > +    struct virtio_crypto rsa_sign_output odata;
> > > +
> > > +    struct virtio_crypto rsa_sign_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: RSA signature
> > > operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata} according to
> > the
> > > general device requirments for asymmetric crypto service.
> > > +
> > > +\subparagraph{Driver Requirements: RSA signature
> > > operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_SIGN,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_RSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} and \field{odata}
> > except
> > > field \field{reserved}.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it operates other fields in \field{idata}.
> > > +Only if the operation is successful, the \field{signature} in 
> > > \field{idata} is
> > > operatable.
> > > +
> > > +\paragraph{Verification: Common Structure}\label{sec:Device Types /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +The result of verification operation can be represented by structure
> > > \field{virtio_crypto_asym_verify_gen_input}.
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_asym_verify_gen_input {
> > > +    /* Operation result */
> > > +    le32 status;
> > > +    /* Verification result */
> > > +#define VIRTIO_CRYPTO_ASYM_VERIFY_SUCCESS 0
> > > +#define VIRTIO_CRYPTO_ASYM_VERIFY_FAIL -1
> >
> > Why not use the following generic results?
> >
> > #define VIRTIO_CRYPTO_OP_OK        0
> > #define VIRTIO_CRYPTO_OP_ERR       1
> > #define VIRTIO_CRYPTO_OP_BADMSG    2
> >
> > And the VIRTIO_CRYPTO_OP_BADMSG is used to show the verification result
> > is incorrect.
> 
> I think this is two different semantic meanings, one is operation result,
> the other is verification result especially it only exists in verification, 
> so I think it
> makes sense to
> use two different sets of error codes with different names.

Verification is a kind of operation I think, but...

> Probably we can remove OP from current error code, e.g,  s/
> VIRTIO_CRYPTO_OP_ERR /VIRTIO_CRYPTO_ERR,
> then it makes more sense to use these error codes through the entire
> virtio-crypto spec.
> 
...I agree. Let's do it in the following version.

Regards,
-Gonglei

> >
> > > +    le32 verify_result;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\paragraph{Verification: ECDSA Verification Operation}\label{sec:Device
> > Types
> > > /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_ecdsa_verify_para {
> > > +    /* Hash alogrithms applied to msg */
> > > +    le32 hash_algo;
> > > +    le32 reserved;
> > > +
> > > +    VIRTIO_CRYPTO_EC_FIELD_TYPE field_type;
> > > +    /* EC Group parameters */
> > > +    struct virtio_crypto_ec_group ec;
> > > +
> > > +    /* Public key */
> > > +    struct virtio_crypto_iovec x;
> > > +    struct virtio_crypto_iovec y;
> > > +};
> > > +
> > > +struct virtio_crypto_ecdsa_verify_output {
> > > +    /* Signature value */
> > > +    struct virtio_crypto_iovec r;
> > > +    struct virtio_crypto_iovec s;
> > > +
> > > +    struct virtio_crypto_iovec msg;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +ECDSA Verification Operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_ecdsa_verify_req {
> > > +    struct virtio_crypto_ecdsa_verify_para parameter;
> > > +    struct virtio_crypto_ecdsa_verify_output odata;
> > > +
> > > +    struct virtio_crypto_asym_verify_gen_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it checks \field{verify_result}.
> > > +
> > > +\paragraph{Verification: RSA Verification Operation}\label{sec:Device
> > Types /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_verify_para {
> > > +    le32 hash_algo;
> > > +    le32 padding_mode;
> > > +    struct virtio_crypto_rsa_pub_key pubkey;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_verify_output {
> > > +    struct virtio_crypto_iovec msg;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +RSA verification operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_verify_req {
> > > +    struct virtio_crypto_rsa_verify_para parameter;
> > > +    struct virtio_crypto_rsa_verify_output odata;
> > > +
> > > +    struct virtio_crypto_asym_verify_gen_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: RSA Verification
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata}.
> > > +
> > > +\subparagraph{Driver Requirements: RSA Verification
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_VERIFY,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_RSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} and \field{odata}
> > except
> > > field \field{reserved}.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it checks \field{verify_result}.
> > > +
> > > +\paragraph{Encryption: RSA Encryption Operation}\label{sec:Device Types
> > /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_enc_para {
> > > +    le32 padding_mode;
> > > +    le32 reserved;
> > > +    struct rsa_pub_key pub_key;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_enc_output {
> > > +    struct virtio_crypto_iovec msg;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_enc_input {
> > > +    le32 status;
> > > +    le32 reserved;
> > > +
> > > +    struct virtio_crypto_iovec cmsg;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +RSA encryption operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_enc_req {
> > > +    struct virtio_crypto_rsa_enc_para parameter;
> > > +    struct virtio_crypto_rsa_enc_output odata;
> > > +
> > > +    struct virtio_crypto_rsa_enc_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: RSA Encryption
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata} according to
> > the
> > > general device requirments for asymmetric crypto service.
> > > +
> > > +\subparagraph{Driver Requirements: RSA Encryption
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_ENCRYPT,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_RSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} and \field{odata}
> > except
> > > field \field{reserved}.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it operates other fields in \field{idata}.
> > > +Only if the operation is successful, the \field{cmsg} in \field{idata} is
> > > operatable.
> > > +
> > > +\paragraph{Decryption: RSA Decryption Operation}\label{sec:Device
> > Types /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_dec_para {
> > > +    le32 padding_mode;
> > > +    le32 reserved;
> > > +    struct virtio_crypto_rsa_priv_key priv_key;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_dec_output {
> > > +    struct virtio_crypto_iovec cmsg;
> > > +}
> > > +
> > > +struct virtio_crypto_rsa_dec_input {
> > > +    le32 status;
> > > +    le32 reserved;
> > > +
> > > +    struct virtio_crypto_iovec msg;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +RSA decryption operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_dec_req {
> > > +    struct virtio_crypto_rsa_dec_para parameter;
> > > +    struct virtio_crypto_rsa_dec_output odata;
> > > +
> > > +    struct virtio_crypto_rsa_dec_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: RSA Decryption
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata} according to
> > the
> > > general device requirments for asymmetric crypto service.
> > > +
> > > +\subparagraph{Driver Requirements: RSA Decryption
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_DECRYPT,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_RSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} and \field{odata}
> > except
> > > field \field{reserved}.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it operates other fields in \field{idata}.
> > > +Only if the operation is successful, the \field{msg} in \field{idata} is
> > operatable.
> > > +
> > > +\paragraph{Key Generation: RSA Key Generation
> > Operation}\label{sec:Device
> > > Types /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_keygen_para {
> > > +    /* The modules length in bytes */
> > > +    le32 modulus_len;
> > > +    le32 rsa_prime_number;
> > > +    le32 priv_key_rep_type;
> > > +    le32 reserved;
> > > +
> > > +    /* the public exponent */
> > > +    struct virtio_crypto_iovec e;
> > > +};
> > > +
> > > +struct virtio_crypto_rsa_keygen_input {
> > > +    le32 status;
> > > +    le32 reserved;
> > > +
> > > +    struct virtio_crypto_rsa_priv_key priv_key;
> > > +    struct virtio_crypto_rsa_pub_key pub_key;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +RSA key-generation operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_rsa_keygen_req {
> > > +    struct virtio_crypto_rsa_keygen_para parameter;
> > > +
> > > +    struct virtio_crypto_rsa_keygen_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: RSA Key Generation
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata} according to
> > the
> > > general device requirments for asymmetric crypto service.
> > > +
> > > +\subparagraph{Driver Requirements: RSA Key Generation
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_KEY_GEN,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_RSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} except
> > \field{reserved}
> > > field.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it operates other fields in \field{idata}.
> > > +Only if the operation is successful, the \field{priv_key} and 
> > > \field{pub_key}
> > in
> > > \field{idata} are operatable.
> > > +
> > > +\paragraph{Key Generation: DSA Key Generation
> > operation}\label{sec:Device
> > > Types /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_dsa_keygen_para {
> > > +    le32 modulus_len;
> > > +    le32 reserved;
> > > +};
> > > +
> > > +struct virtio_crypto_dsa_keygen_input {
> > > +    le32 status;
> > > +    le32 reserved;
> > > +
> > > +    /* DSA Group parameters */
> > > +    struct virtio_crypto_dsa_group dsa;
> > > +    /* Private key */
> > > +    struct virtio_crypto_iovec x;
> > > +    /* Public key */
> > > +    struct virtio_crypto_iovec y;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +DSA key-generation operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_dsa_keygen_req {
> > > +    struct virtio_crypto_dsa_keygen_para parameter;
> > > +
> > > +    struct virtio_crypto_dsa_keygen_input idata;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: DSA Key Generation
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata} according to
> > the
> > > general device requirments for asymmetric crypto service.
> > > +
> > > +\subparagraph{Driver Requirements: DSA Key Generation
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_KEY_GEN,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_DSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} except
> > \field{reserved}
> > > field.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it operates other fields in \field{idata}.
> > > +Only if the operation is successful, the \field{priv_key} and 
> > > \field{pub_key}
> > in
> > > \field{idata} are operatable.
> > > +
> > > +\paragraph{Key Generation: EC Key Generation
> > operation }\label{sec:Device
> > > Types /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_ec_keygen_para {
> > > +    VIRTIO_CRYPTO_EC_FIELD_TYPE field_type;
> > > +
> > > +    /* EC group parameters */
> > > +    struct virtio_crypto_ec_group ec;
> > > +};
> > > +
> > > +struct virtio_crypto_ec_keygen_input {
> > > +    le32 status;
> > > +    le32 reserved;
> > > +
> > > +    /* Private key */
> > > +    struct virtio_crypto_iovec d;
> > > +
> > > +    /* Public key */
> > > +    struct virtio_crypto_iovec x;
> > > +    struct virtio_crypto_iovec y;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +EC key-generation operation request is defined as below:
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_ec_keygen_req {
> > > +    struct virtio_crypto_ec_keygen_para parameter;
> > > +
> > > +    struct virtio_crypto_ec_keygen_input key;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subparagraph{Device Requirements: EC Key Generation
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The device SHOULD set the operation results in \field{idata} according to
> > the
> > > general device requirments for asymmetric crypto service.
> > > +
> > > +\subparagraph{Driver Requirements: EC Key Generation
> > > Operation}\label{sec:Device Types / Crypto Device /
> > > +Device Operation / Crypto operation / Asymmetric Crypto Service}
> > > +The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to
> > > VIRTIO_CRYPTO_ASYM_KEY_GEN,
> > > +set \field{algo} to VIRTIO_CRYPTO_ASYM_ECDH or
> > > VIRTIO_CRYPTO_ASYM_ECDSA.
> > > +
> > > +The driver SHOULD set all fields in \field{parameter} except
> > \field{reserved}
> > > field.
> > > +
> > > +The driver SHOULD check the operation result \field{status} in 
> > > \field{idata}
> > > before it operates other fields in \field{idata}.
> > > +Only if the operation is successful, the other fields in \field{idata} 
> > > are
> > > operatable.
> > > +
> > > +\paragraph{Key Exchange: Common Structure}\label{sec:Device Types /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +Key Exchange request has two kinds of operations in general, key
> > generation
> > > and key computation.
> > > +For DH, it has additional operation: parameter generation operation.
> > > +
> > > +\begin{lstlisting}
> > > +typdef enum VIRTIO_CRYPTO_KEY_EXCHANGE_OP {
> > > +    VIRTIO_CRYPTO_KEY_EXCHANGE_GEN_KEY,
> > > +    VIRTIO_CRYPTO_KEY_EXCHANGE_COMPUTE_KEY,
> > > +    /* It's only valid in case algorithm is DH */
> > > +    VIRTIO_CRYPTO_KEY_EXCHANGE_GEN_DH_PARAM
> > > +} VIRTIO_CRYPTO_KEY_EXCHANGE_OP;
> > > +\end{lstlisting}
> > > +
> > > +\paragraph{Key Exchange: DH Parameter Generation
> > > Operation}\label{sec:Device Types /
> > > +Crypto Device / Device Operation / Crypto operation / Asymmetric Crypto
> > > Service}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_crypto_dh_parameter {
> > > +    le32 modules_len;
> > > +    le32 reserved;
> > > +
> > > +    /* random odd prime number */
> > > +    struct virtio_crypto_iovec p;
> > > +    /* base (g) */
> > > +    struct virtio_crypto_iovec g;
> > > +};
> > > +
> > > +struct virtio_crypto_dh_keyexchg_para {
> > > +    VIRTIO_CRYPTO_KEY_EXCHANGE_OP keyexchg_op;
> >
> > sizeof(enum foo) is implementation-specific according to the C standard.
> > You must explicitly choose a type (u8, le16, le32) in this specification
> > so that the struct layout is compatible between compilers.
> >
> 
> You are right. Will fix this.
> 
> > You can move keyexchg_op to struct virtio_crypto_dh_parameter,
> > instead of the reserved property.
> 
> They represent two different semantic meaning. I preferred to keep current
> structure definition.
> 
> >
> > > --
> > > 1.9.3
> >
> >
> > Regards,
> > -Gonglei

Reply via email to