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

[Gonglei (Arei)]

Hi Cornelia,

> 
> On Tue, 7 Feb 2017 12:39:44 +0100
> Halil Pasic <address@hidden> 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