On Tue, 7 Feb 2017 12:39:44 +0100
Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
> On 01/18/2017 09:22 AM, Gonglei wrote:
> > +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> > +
> > +The virtio crypto device is a virtual cryptography device as well as a
> > kind of
> > +virtual hardware accelerator for virtual machines. The encryption and
> > +decryption requests are placed in any of the data active queues and are
> > ultimately handled by the
>
> Am I the only one having a problem with 'data active queues'?
No. I think it looks weird.
(...)
> > +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
> > ~VIRTIO_CRYPTO_S_HW_READY.
>
> Not entirely happy with this. What you want to say is reserved
> for future use, or? Would it make sense to have a general note
> -- in a similar fashion like for 'sizes are in bytes' -- for
> reserved for future use?
>
> One possible formulation would be:
>
> "In this specification, unless explicitly stated otherwise,
> fields and bits reserved for future use shall be zeroed out.
> Both the a device or a driver device and the driver should
> detect violations of this rule, and deny the requested
> operation in an appropriate way if possible."
If we go with reserved-and-must-be-zero, we need to make rejecting
non-zero for reserved value a MUST, or we may run into problems later.
In this case, I'd opt for a specific formulation, though; like
"The \field{status} field can be either zero or have one or more flags
set. Valid flags are listed below."
And then state that non-valid flags MUST NOT be set resp. MUST be
rejected in a normative statement.
(...)
> > +The following services are defined:
> > +
> > +\begin{lstlisting}
> > +/* CIPHER service */
> > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> > +/* HASH service */
> > +#define VIRTIO_CRYPTO_SERVICE_HASH 1
> > +/* MAC (Message Authentication Codes) service */
> > +#define VIRTIO_CRYPTO_SERVICE_MAC 2
> > +/* AEAD (Authenticated Encryption with Associated Data) service */
> > +#define VIRTIO_CRYPTO_SERVICE_AEAD 3
> > +\end{lstlisting}
> > +
> > +The last driver-read-only fields specify detailed algorithms masks
> > +the device offers for corresponding services. The following CIPHER
> > algorithms
> > +are defined:
>
> You do not establish an explicit relationship between the fields and the
> macros for the flags. These are flags or? It seems quite common in the
> spec to use _F_ in flag names. Would it be appropriate here?
The values as specified do not look very flaggy to me...
>
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_CIPHER 0
> > +#define VIRTIO_CRYPTO_CIPHER_ARC4 1
> > +#define VIRTIO_CRYPTO_CIPHER_AES_ECB 2
> > +#define VIRTIO_CRYPTO_CIPHER_AES_CBC 3
> > +#define VIRTIO_CRYPTO_CIPHER_AES_CTR 4
> > +#define VIRTIO_CRYPTO_CIPHER_DES_ECB 5
> > +#define VIRTIO_CRYPTO_CIPHER_DES_CBC 6
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB 7
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC 8
> > +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR 9
> > +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8 10
> > +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2 11
> > +#define VIRTIO_CRYPTO_CIPHER_AES_F8 12
> > +#define VIRTIO_CRYPTO_CIPHER_AES_XTS 13
> > +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3 14
> > +\end{lstlisting}
> > +
> > +The following HASH algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_HASH 0
> > +#define VIRTIO_CRYPTO_HASH_MD5 1
> > +#define VIRTIO_CRYPTO_HASH_SHA1 2
> > +#define VIRTIO_CRYPTO_HASH_SHA_224 3
> > +#define VIRTIO_CRYPTO_HASH_SHA_256 4
> > +#define VIRTIO_CRYPTO_HASH_SHA_384 5
> > +#define VIRTIO_CRYPTO_HASH_SHA_512 6
> > +#define VIRTIO_CRYPTO_HASH_SHA3_224 7
> > +#define VIRTIO_CRYPTO_HASH_SHA3_256 8
> > +#define VIRTIO_CRYPTO_HASH_SHA3_384 9
> > +#define VIRTIO_CRYPTO_HASH_SHA3_512 10
> > +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128 11
> > +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256 12
> > +\end{lstlisting}
> > +
> > +The following MAC algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_MAC 0
> > +#define VIRTIO_CRYPTO_MAC_HMAC_MD5 1
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1 2
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224 3
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256 4
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384 5
> > +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512 6
> > +#define VIRTIO_CRYPTO_MAC_CMAC_3DES 25
> > +#define VIRTIO_CRYPTO_MAC_CMAC_AES 26
> > +#define VIRTIO_CRYPTO_MAC_KASUMI_F9 27
> > +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2 28
> > +#define VIRTIO_CRYPTO_MAC_GMAC_AES 41
> > +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH 42
> > +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES 49
> > +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9 50
> > +#define VIRTIO_CRYPTO_MAC_XCBC_AES 53
> > +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3 54
> > +\end{lstlisting}
> > +
> > +The following AEAD algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_AEAD 0
> > +#define VIRTIO_CRYPTO_AEAD_GCM 1
> > +#define VIRTIO_CRYPTO_AEAD_CCM 2
> > +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305 3
> > +\end{lstlisting}
> > +
>
> Would it make sense to interleave the flag definition
> with the struct virtio_crypto_config?
>
> > +\begin{note}
> > +Any other values are reserved for future use.
>
> Are these flags or values? I do not think values is appropriate here.
These look like values to me and not flags.
The more I stare at this, the more confused I become. Is the device
supposed to indicate exactly one algorithm only? Or are these supposed
to be bit numbers? (If yes, it would make sense to either call them
that or specify flags in the (1 << bit_num) notation.)
