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

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Wed, 8 Feb 2017 03:46:53 +0000
"Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:

> Hi Cornelia,
> 
> > 
> > On Tue, 7 Feb 2017 12:39:44 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > 
> > > On 01/18/2017 09:22 AM, Gonglei wrote:
>
> > > > +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.

Can you add, for example, a note that the respective fields define the
supported services (...) with bits corresponding to a specific service
(...) on a 1:1 basis? Then define which way your bit numbering is done
and that the following defines specify the bit number. Then your
existing lists are fine, I think.