Hi Cornelia,
>
> 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.
>
Sounds good.
> (...)
>
> > > +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}
> > > +
> > > +
> >
> > 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.)
Actually these are both bit numbers and values.
On the one hand, the device can set algorithms by '1 << bit_num' notation to
tell the driver what
algorithms are supported. On the other hand, the driver can set the value to
tell
the device a virtio crypto request's algorithm in struct
virtio_crypto_op_header.algo.
Pls see cryptodev-builtin.c:: cryptodev_buitlin_init()
backend->conf.crypto_services =
1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
1u << VIRTIO_CRYPTO_SERVICE_HASH |
1u << VIRTIO_CRYPTO_SERVICE_MAC;
backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
Perhaps I should add a specific formulation about them.
Thanks,
-Gonglei
