[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification |
Date: |
Thu, 9 Feb 2017 13:48:39 +0100 |
On Wed, 8 Feb 2017 03:46:53 +0000
"Gonglei (Arei)" <address@hidden> wrote:
> 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:
>
> > > > +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.
Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification, Gonglei (Arei), 2017/02/08